taskGNU Astronomy Utilities - Tasks: task #14289, Adding bug/task fixed/completed...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

task #14289: Adding bug/task fixed/completed when merging with master

Submitter:  Mohammad Akhlaghi <makhlaghi>
Submitted:  Mon 26 Dec 2016 03:09:27 PM UTC
   
 
Should Start On:  Sun 25 Dec 2016 03:00:00 PM UTC Should be Finished on:  Sun 25 Dec 2016 03:00:00 PM UTC
Category:  Development Priority:  5 - Normal
Item Group:  Enhancement Status:  Need Info
Privacy:  Public Assigned to:  makhlaghi
Percent Complete:  0% Open/Closed:  Open
Effort:  0.00

Tue 27 Dec 2016 12:01:03 AM UTC, comment #2: 

Thanks, I completely agree with your argument. I just wanted to add that I am not proposing to force bug/task reports to have a solution. When we have just seen the bug and don't know the solution or don't have time to dig into it, we will indeed report any symptom we see and won't provide a solution.

What happens in practice for people who are familiar with the code is that they cannot be sure it is a bug unless they look into the code first and confirm it (it might be due to their inputs). During this code check, it sometimes happens that they find the solution during this confirmation. So they have to go through this double reporting phase (one report to define the bug before their commit, and another for the link after they push), which is inconvenient for the reporter and also, everyone on the mailing list (who recieves two mails).

Infact I came up with this suggestion after your commits that didn't include these statements. I noticed that I had also forgot to add these statements for you as an ammendment before pushing. It then occurred to me that a `git commit --amend' by the person who does the final merge to master is much more easier to manage (one less thing for the maintainer to worry about!). It also had the practical benefit to the submitter, that if they have a solution, they can also share their branch link with the same initial report.

Having the person in charge or merging with master add the `This finishes task #XXXX.' or `This fixes bug #XXX' can also act as a kind of stamp for double check on the final commit that gets merged. Infact I hope that in the future, even when the main maintainer has a branch to merge with master, they don't do it themselves, and other developers would have to check the work, sign it off with this statement, and then merge with master.

The cases of your last two commits are interesting examples: please compare the `author' and `committer' in the typo commit and the genauthors commit. It would be great if all commits that get merged into master have different names in these fields (even those by the main maintainer).

Mohammad Akhlaghi <makhlaghi>
Group administrator
Mon 26 Dec 2016 04:26:08 PM UTC, comment #1: 

I think that for v0.3, the current commit guideline should be
retained, but possibly reworded and augmented with an exception made
for minor bugs whose fixes really seem trivial, and in that case the
master maintainer on savannah can add the bug number using _git commit
--amend_ prior to pushing up to savannah. I admit that I violated
(unintentionally :) this guideline, but I think that separating the
bug report from a proposed fix (and doing the bug report first) is
justified by what is described in a well-known essay by Simon Tatham:

http://www.chiark.greenend.org.uk/~sgtatham/bugs.html

> Some of the worst bug reports I've ever seen come from programmers,
> and even from good programmers. ...


> Providing your own diagnosis might be helpful sometimes, but always
> state the symptoms. The diagnosis is an optional extra, and not an
> alternative to giving the symptoms. Equally, sending a modification
> to the code to fix the problem is a useful addition to a bug report
> but not an adequate substitute for one.


Given that commits are to be made on side branches, (so far) not on
savannah, it seems to me that the idea is that a bug or task number is
needed every time a commit is considered ready, i.e. a _pull
request_ is carried out by either a task or bug report. So the text
corresponding to

https://www.gnu.org/software/gnuastro/manual/html_node/Commit-guidelines.html

should probably be restructured to say this right from the beginning.
I think a link to Tatham's essay above could be used to point out
the usefulness of separating bug reports from their proposed fixes,
even when we we're sure we've solved the bug. The reality is that
someone else will see things somewhat differently from the person
proposing the bug+fix.





Boud Roukema <boud>
Group Member
Mon 26 Dec 2016 03:09:27 PM UTC, original submission:  

In the current Commit guidelines, we have asked for commiters to include a statement at the the end of the commit message saying  ‘This fixes bug #ID.’, or ‘This finishes task #ID.’.

This makes things complicated when submitting a new bug/task issue with a link to the branch with the fix: since you don't have the bug/task number. So the solution was to first define the bug/task, use its number in the commit message, push the branch and then include the link. This is inconvenient.

I wanted to bring up a solution to this here for discussion: I can add these statements to the commit (through `git commit --amend') after merging it with the master branch. All the commit information (author, dates, and etc) will remain unchanged, only the commit body will be changed. This way, the initial bug/task report can also contain a link to the fix and if the commit needs further work, others can easily work based on it, without worrying about an immature statement (in complicated cases): only after it is merged with master will this statement be added.

If you have any ideas/suggestions, please share them here, we can start this convention with the 0.3 release.

Mohammad Akhlaghi <makhlaghi>
Group administrator

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by boud (Posted a comment)
  • -email is unavailable- added by makhlaghi (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    No changes have been made to this item

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code