some Git workflow experiences

[Myk Melez]

Hi all,

Warning: Git wonkery ahead, with excruciating details. I would not want to read this message. I recommend you skip it. ;-)


Part 1: Wherein I Handle My First Pull Request

To fix some test failures, Atul submitted GitHub pull request 33, I reviewed the changes (comprising two commits) on GitHub, and then I pushed them to the canonical repository via the following set of commands:

1. git checkout -b toolness-4.0b7-bustage-fixes master
2. git pull https://github.com/toolness/jetpack-sdk.git 4.0b7-bustage-fixes
3. git checkout master
4. git merge toolness-4.0b7-bustage-fixes
5. git push upstream master

That landed the two commits in the canonical repository, but it isn't obvious that they were related (i.e. part of the same pull request), that I was the one who reviewed them, or that I was the one who pushed them.


Part 2: Wherein I Handle My Second Pull Request

Thus, for the fix for bug 611042, for which Atul submitted GitHub pull request 34, I again reviewed the changes (also comprising two commits) on GitHub, but then I pushed them to the canonical repository via this different set of commands (after discussion with Atul and Patrick Walton of the Rust team):

1. git checkout -b toolness-bug-611042 master
2. git pull https://github.com/toolness/jetpack-sdk.git bug-611042
3. (There might have been something else here, since the pull request resulted in a merge; I don't quite remember.)
   
4. git checkout master
5. git merge --no-ff --no-commit toolness-bug-611042
6. git commit --signoff -m "bug 611042: remove request.response.xml for e10s compatibility; r=myk" --author "atul"
7. git push upstream master

Because Atul's pull request was no longer against the tip (since I had just merged those previous changes), when I pulled the remote bug-611042 branch into my local toolness-bug-611042 branch (step 2), I had to merge his changes, which resulted in a merge commit.

Merging the changes to my local master with "--no-ff" and "--no-commit" (step 5) then allowed me to commit the merge to my master branch manually (step 6), resulting in another merge commit.

For the second merge commit, I specified "--signoff", which added "Signed-off-by: Myk Melez <m...@mozilla.org>" to the commit message; crafted a custom commit message that included "r=myk"; and specified '--author "atul"', which made Atul the author of the merge.

I dislike having the former merge commit in history, since it's extraneous, unuseful details about how I did the merging locally before I pushed to the canonical repository. I'm not sure how to avoid it, though.

On the other hand, I like having the latter merge commit in history, since it provides context for Atul's two commits: the bug number, the fact that the changes were reviewed, and a commit message that describes the changes as a whole.

I'm ambivalent about --signoff vs. adding "r=myk" to the commit message, as they seem equivalentish, with --signoff being more explicit (so in theory it might form part of an enlightened workflow in the future), while "r=myk" is simpler.

And I dislike having made Atul the author of the merge, since it's incorrect: he wasn't the author of the merge, he was only the author of the changes (for which he is correctly credited). And if the merge itself caused problems (f.e. I accidentally backed out other recent changes in the process), I would be the one responsible for fixing those problems, not Atul.


Part 3: Pushing Patches

In addition to pull requests, one can also contribute via patches. I've pushed a few of these via something like the following set of commands:

1. git apply patch.diff
2. git commit -a -m "bug <number>: <description of changes>; r=myk" --author "<author name>"
   
3. git push upstream master

That results in a commit like this one, which shows me as the committer and the patch author as the author. And that seems like a fine record of what happened.


Part 4: To Bug or Not To Bug?

One of the questions GitHub raises is whether or not every change deserves a bug report. And if not, how do we differentiate those that do from the rest?

I don't have the definitive answers to these questions, but my sense, from my experience so far, is that we shouldn't require all changes to be accompanied by bug reports, but larger, riskier, time-consuming, and/or controversial changes should have reports to capture history, provide a forum for discussion, and permit project planning; while bug reports should be optional for smaller, safer, quickly-resolved, and/or non-controversial changes.


Thoughts?

-myk

[Erik Vold]

Hey Myk,

On Fri, Nov 12, 2010 at 18:26, Myk Melez <m...@mozilla.org> wrote:

Part 1: Wherein I Handle My First Pull Request

To fix some test failures, Atul submitted GitHub pull request 33, I reviewed the changes (comprising two commits) on GitHub, and then I pushed them to the canonical repository via the following set of commands:

1. git checkout -b toolness-4.0b7-bustage-fixes master
2. git pull https://github.com/toolness/jetpack-sdk.git 4.0b7-bustage-fixes
3. git checkout master
4. git merge toolness-4.0b7-bustage-fixes
5. git push upstream master

That landed the two commits in the canonical repository, but it isn't obvious that they were related (i.e. part of the same pull request), that I was the one who reviewed them, or that I was the one who pushed them.

Some people prefer a strictly linear history, and you can do that with git. Others like to see branches and merges in the history, and you can do that with git. Others like to have the best of both worlds, and git lets you do that too. I like to see a merged branch of related commits made to accomplish a set of goals (or a single goal) in the history of a project. If however a branch has only a single commit (or I choose to reduce it to a single commit) then I'll cherry pick it. If a branch is made of commits that accomplish different goals then I would rebase it on to master, though I don't see that kind of branch often.

I'm guessing you want to see branches being merged in the history, based on your part 2 attempt. I like to do pulls this way because it captures more of the history, and makes it easier to understand.

Part 2: Wherein I Handle My Second Pull Request

Thus, for the fix for bug 611042, for which Atul submitted GitHub pull request 34, I again reviewed the changes (also comprising two commits) on GitHub, but then I pushed them to the canonical repository via this different set of commands (after discussion with Atul and Patrick Walton of the Rust team):

1. git checkout -b toolness-bug-611042 master
2. git pull https://github.com/toolness/jetpack-sdk.git bug-611042

Danger Myk Melez! Danger!

Doing a git pull is the same as doing a git fetch and then a git merge. The git merge is not what you want here.

1. (There might have been something else here, since the pull request resulted in a merge; I don't quite remember.)
   
2. git checkout master
3. git merge --no-ff --no-commit toolness-bug-611042
4. git commit --signoff -m "bug 611042: remove request.response.xml for e10s compatibility; r=myk" --author "atul"
5. git push upstream master

What you want to do is checkout the commit that atul's branch points to, (if there will be conflicts then I suggest merging your master branch to atul's branch here, handle conflicts, commit, then procede in order to keep the blame correct and the merge to master conflict free) merge that to your master branch and then push your master branch upstream.

There are a number of ways to "checkout atul's branch" here are the two I've used, there's probably a easier way (there always seems to be):

1) - git fetch atul

    - git checkout -b newBranchName sha1OfTheTipOfAtulsBranch

2) - git fetch persons_name branch_name:refs/remotes/persons_name/branch_name

    - git checkout -t persons_name/branch_name

Because Atul's pull request was no longer against the tip (since I had just merged those previous changes), when I pulled the remote bug-611042 branch into my local toolness-bug-611042 branch (step 2), I had to merge his changes, which resulted in a merge commit.

Merging the changes to my local master with "--no-ff" and "--no-commit" (step 5) then allowed me to commit the merge to my master branch manually (step 6), resulting in another merge commit.

For the second merge commit, I specified "--signoff", which added "Signed-off-by: Myk Melez <m...@mozilla.org>" to the commit message; crafted a custom commit message that included "r=myk"; and specified '--author "atul"', which made Atul the author of the merge.

I dislike having the former merge commit in history, since it's extraneous, unuseful details about how I did the merging locally before I pushed to the canonical repository. I'm not sure how to avoid it, though.

Yah they suck; a nice feature of git is that you can fix the history, and without much trouble usually, so if you really want to correct it then you can, but it may not be worth it, because it will mean everyone that has forked it will need to do some fancy git work to be in sync with upstream again. It's not that hard to do though, especially since the code wouldn't change in this case, and it's something that all gits should learn to deal with imo.

On the other hand, I like having the latter merge commit in history, since it provides context for Atul's two commits: the bug number, the fact that the changes were reviewed, and a commit message that describes the changes as a whole.

I'm ambivalent about --signoff vs. adding "r=myk" to the commit message, as they seem equivalentish, with --signoff being more explicit (so in theory it might form part of an enlightened workflow in the future), while "r=myk" is simpler.  

 

And I dislike having made Atul the author of the merge, since it's incorrect: he wasn't the author of the merge, he was only the author of the changes (for which he is correctly credited). And if the merge itself caused problems (f.e. I accidentally backed out other recent changes in the process), I would be the one responsible for fixing those problems, not Atul.

The person doing the merge should be the author of a merge commit, I agree; this way the proper person is given credit+blame for handling conflicts if there were any.

Part 4: To Bug or Not To Bug?

One of the questions GitHub raises is whether or not every change deserves a bug report. And if not, how do we differentiate those that do from the rest?

I don't have the definitive answers to these questions, but my sense, from my experience so far, is that we shouldn't require all changes to be accompanied by bug reports, but larger, riskier, time-consuming, and/or controversial changes should have reports to capture history, provide a forum for discussion, and permit project planning; while bug reports should be optional for smaller, safer, quickly-resolved, and/or non-controversial changes.

Sounds reasonable to me.

--
Erik Vergobbi Vold

Email: erik...@gmail.com
Website: http://erikvold.com/
Twitter: http://twitter.com/erikvold
Identi.ca: http://identi.ca/erikvold
LinkedIn: http://www.linkedin.com/in/erikvold

[Erik Vold]

On Fri, Nov 12, 2010 at 21:03, Erik Vold <erik...@gmail.com> wrote:

There are a number of ways to "checkout atul's branch" here are the two I've used, there's probably a easier way (there always seems to be):

1) - git fetch atul

    - git checkout -b newBranchName sha1OfTheTipOfAtulsBranch

This works too: git checkout -b new_branch_name atul/branch_name

Erik

[Erik Vold]

On Fri, Nov 12, 2010 at 21:03, Erik Vold <erik...@gmail.com> wrote:

What you want to do is checkout the commit that atul's branch points to, (if there will be conflicts then I suggest merging your master branch to atul's branch here, handle conflicts, commit, then procede in order to keep the blame correct and the merge to master conflict free) merge that to your master branch and then push your master branch upstream.

when I said merge here, I meant merge --no-ff

Erik

[Dietrich Ayala]

Was there anything that changed from the steps for these things in
your post, due to this thread?

> --
> You received this message because you are subscribed to the Google Groups
> "mozilla-labs-jetpack" group.
> To post to this group, send email to mozilla-la...@googlegroups.com.
> To unsubscribe from this group, send email to
> mozilla-labs-jet...@googlegroups.com.
> For more options, visit this group at
> http://groups.google.com/group/mozilla-labs-jetpack?hl=en.
>

[Irakli Gozalishvili]

Hi Myk,

On Sat, Nov 13, 2010 at 03:26, Myk Melez <m...@mozilla.org> wrote:

Hi all,

Warning: Git wonkery ahead, with excruciating details. I would not want to read this message. I recommend you skip it. ;-)


Part 1: Wherein I Handle My First Pull Request

To fix some test failures, Atul submitted GitHub pull request 33, I reviewed the changes (comprising two commits) on GitHub, and then I pushed them to the canonical repository via the following set of commands:

1. git checkout -b toolness-4.0b7-bustage-fixes master
2. git pull https://github.com/toolness/jetpack-sdk.git 4.0b7-bustage-fixes
3. git checkout master
4. git merge toolness-4.0b7-bustage-fixes
5. git push upstream master

That landed the two commits in the canonical repository, but it isn't obvious that they were related (i.e. part of the same pull request), that I was the one who reviewed them, or that I was the one who pushed them.


Part 2: Wherein I Handle My Second Pull Request

Thus, for the fix for bug 611042, for which Atul submitted GitHub pull request 34, I again reviewed the changes (also comprising two commits) on GitHub, but then I pushed them to the canonical repository via this different set of commands (after discussion with Atul and Patrick Walton of the Rust team):

1. git checkout -b toolness-bug-611042 master
2. git pull https://github.com/toolness/jetpack-sdk.git bug-611042
3. (There might have been something else here, since the pull request resulted in a merge; I don't quite remember.)
   
4. git checkout master
5. git merge --no-ff --no-commit toolness-bug-611042
6. git commit --signoff -m "bug 611042: remove request.response.xml for e10s compatibility; r=myk" --author "atul"
7. git push upstream master

Erik mentioned all most of it but let me jest post reduced snippet that does the same, except making atul an an author of a merge.

1. git remote add atul  https://github.com/toolness/jetpack-sdk.git #this is one time only

2. git fetch atul

3. git merge atul/toolness-bug-611042 --no-ff -m 'bug 611042: remove request.response.xml for e10s compatibility; r=myk'  

4. git push upstream master 

 

Assuming that when you run this comments your are on master and in sync with upstream.

Because Atul's pull request was no longer against the tip (since I had just merged those previous changes), when I pulled the remote bug-611042 branch into my local toolness-bug-611042 branch (step 2), I had to merge his changes, which resulted in a merge commit.

Merging the changes to my local master with "--no-ff" and "--no-commit" (step 5) then allowed me to commit the merge to my master branch manually (step 6), resulting in another merge commit.

For the second merge commit, I specified "--signoff", which added "Signed-off-by: Myk Melez <m...@mozilla.org>" to the commit message; crafted a custom commit message that included "r=myk"; and specified '--author "atul"', which made Atul the author of the merge.
 

I dislike having the former merge commit in history, since it's extraneous, unuseful details about how I did the merging locally before I pushed to the canonical repository. I'm not sure how to avoid it, though.

You can add --amend to the commit and it will just add your changes to the last commit. I would still recommend to stick to the flaw I've posted above since it won't require manual commits and will keep history clean.

 

 

On the other hand, I like having the latter merge commit in history, since it provides context for Atul's two commits: the bug number, the fact that the changes were reviewed, and a commit message that describes the changes as a whole.

I'm ambivalent about --signoff vs. adding "r=myk" to the commit message, as they seem equivalentish, with --signoff being more explicit (so in theory it might form part of an enlightened workflow in the future), while "r=myk" is simpler.

And I dislike having made Atul the author of the merge, since it's incorrect: he wasn't the author of the merge, he was only the author of the changes (for which he is correctly credited). And if the merge itself caused problems (f.e. I accidentally backed out other recent changes in the process), I would be the one responsible for fixing those problems, not Atul.

I totally agree and in fact I don't think that you should be adding --author "atul". In history it will be pretty clear anyway that changes are done are done by Atul and merged & pushed by you:

git log --oneline --graph

Or some visual tool like gitk will do a great job here.

Part 3: Pushing Patches

In addition to pull requests, one can also contribute via patches. I've pushed a few of these via something like the following set of commands:

1. git apply patch.diff
2. git commit -a -m "bug <number>: <description of changes>; r=myk" --author "<author name>"
   
3. git push upstream master

That results in a commit like this one, which shows me as the committer and the patch author as the author. And that seems like a fine record of what happened.


Part 4: To Bug or Not To Bug?

One of the questions GitHub raises is whether or not every change deserves a bug report. And if not, how do we differentiate those that do from the rest?

I don't have the definitive answers to these questions, but my sense, from my experience so far, is that we shouldn't require all changes to be accompanied by bug reports, but larger, riskier, time-consuming, and/or controversial changes should have reports to capture history, provide a forum for discussion, and permit project planning; while bug reports should be optional for smaller, safer, quickly-resolved, and/or non-controversial changes.


Thoughts?

-myk

[Myk Melez]

On 11/18/2010 03:00 AM, Irakli Gozalishvili wrote:

Erik mentioned all most of it but let me jest post reduced snippet that does the same, except making atul an an author of a merge.

1. git remote add atul  https://github.com/toolness/jetpack-sdk.git #this is one time only

2. git fetch atul

3. git merge atul/toolness-bug-611042 --no-ff -m 'bug 611042: remove request.response.xml for e10s compatibility; r=myk'  

4. git push upstream master

Thanks Erik and Irakli, this is really helpful!

With a tip from Brian and a bit more experimentation (looking for ways to simplify as much as possible), here is where I'm currently at:

1. From the "how to merge this pull request" section of the pull request page (f.e. pull request 34), copy the command from step two, but change the word "pull" to "fetch", f.e.:
   
   git fetch https://github.com/toolness/jetpack-sdk.git bug-610507


2. Merge the remote branch into the local master branch with a custom commit message:
   
   git merge FETCH_HEAD --no-ff -m"bug 610507: get rid of the nsjetpack package; r=myk"
   
   
3. Push the changes upstream:
   
   git push upstream master
   

I like this set of commands because it doesn't require me to add a remote, I can copy/paste the fetch command from GitHub (although I have to be careful not to copy a line break character along with it and issue a pull before I have had a chance to change it to a fetch), and I always type the same alias to the branch name in step three.

However, I wish the merge commit page referenced the specific commits that were merged. It does mention that it's a branch merge, but there's no way to get from that page to the pages for the commits I merged from the branch.

"git log --oneline --graph", gitg, and gitk do give me that information, though, so I'm ok on the command line, anyway.

-myk

[Myk Melez]

On 11/23/2010 06:06 PM, Myk Melez wrote:

1. From the "how to merge this pull request" section of the pull request page (f.e. pull request 34), copy the command from step two, but change the word "pull" to "fetch", f.e.:
   
   git fetch https://github.com/toolness/jetpack-sdk.git bug-610507


2. Merge the remote branch into the local master branch with a custom commit message:
   
   git merge FETCH_HEAD --no-ff -m"bug 610507: get rid of the nsjetpack package; r=myk"
   
   
3. Push the changes upstream:
   
   git push upstream master
   

One more tip from Brian! Between steps one and two, verify that the set of changes is as you expect with "git diff FETCH_HEAD HEAD".

And in step two, you could merge and commit separately to use --signoff, such as I did for pull request 44:

$ git merge FETCH_HEAD --no-ff --no-commit
Automatic merge went well; stopped before committing as requested
$ git commit -m"Bug 613341 - addons should be marked as compatible with 4.0b8pre" --signoff
[master b1d60f5] Bug 613341 - addons should be marked as compatible with 4.0b8pre

But that results in this merge commit that doesn't look like a merge commit along with the original change. So I don't like that as much.

-myk

[Erik Vold]

On Tue, Nov 23, 2010 at 18:06, Myk Melez <m...@mozilla.org> wrote:

On 11/18/2010 03:00 AM, Irakli Gozalishvili wrote:

Erik mentioned all most of it but let me jest post reduced snippet that does the same, except making atul an an author of a merge.

1. git remote add atul  https://github.com/toolness/jetpack-sdk.git #this is one time only

2. git fetch atul

3. git merge atul/toolness-bug-611042 --no-ff -m 'bug 611042: remove request.response.xml for e10s compatibility; r=myk'  

4. git push upstream master

Thanks Erik and Irakli, this is really helpful!

With a tip from Brian and a bit more experimentation (looking for ways to simplify as much as possible), here is where I'm currently at:

1. From the "how to merge this pull request" section of the pull request page (f.e. pull request 34), copy the command from step two, but change the word "pull" to "fetch", f.e.:
   
   git fetch https://github.com/toolness/jetpack-sdk.git bug-610507


2. Merge the remote branch into the local master branch with a custom commit message:
   
   git merge FETCH_HEAD --no-ff -m"bug 610507: get rid of the nsjetpack package; r=myk"
   

nice!

I like this set of commands because it doesn't require me to add a remote, I can copy/paste the fetch command from GitHub (although I have to be careful not to copy a line break character along with it and issue a pull before I have had a chance to change it to a fetch), and I always type the same alias to the branch name in step three.

I like it, but if there are conflicts to resolve then this would result in a merge commit on master with conflict resolutions, which I dislike.
 

However, I wish the merge commit page referenced the specific commits that were merged.

A merge commit has two parent commits (which make them obvious), and github links to both of them. 68b6e306dfeccef103b071e0812dc3a375830ac0 is not a parent of 0e23d1c1555d5de228ed7ad62c8715e2775d2390 though.
 

It does mention that it's a branch merge, but there's no way to get from that page to the pages for the commits I merged from the branch.

"git log --oneline --graph", gitg, and gitk do give me that information, though, so I'm ok on the command line, anyway.

-myk

[Erik Vold]

I like to make a normal commit, and then make my modifications to it afterward with git commit --amend ...

[Jean-Marc Desperrier]

On 24 nov, 04:09, Erik Vold <erikvv...@gmail.com> wrote:
> On Tue, Nov 23, 2010 at 18:06, Myk Melez <m...@mozilla.org> wrote:

> > However, I wish the merge commit page<https://github.com/mozilla/addon-sdk/commit/0e23d1c1555d5de228ed7ad62...>referenced the
> > specific<https://github.com/mozilla/addon-sdk/commit/68b6e306dfeccef103b071e08...>
> > commits<https://github.com/mozilla/addon-sdk/commit/715cb47c720bcdd11846cae6c...>that were merged.

>
> A merge commit has two parent commits (which make them obvious), and github
> links to both of them. 68b6e306dfeccef103b071e0812dc3a375830ac0 is not a
> parent of 0e23d1c1555d5de228ed7ad62c8715e2775d2390 though.

I don't see what Myk expected to be another way, I think he got a
little confused.
The branch/tail he was merging https://github.com/mozilla/addon-sdk/pull/43
had two patches, where 715cb47c720bcdd11846cae6c6cab325bb1a982b was
applied above the older one 68b6e306dfeccef103b071e0812dc3a375830ac0
The 0e23d1c1555d5de228ed7ad62... commit does show
715cb47c720bcdd11846cae6c6cab325bb1a982b as one of the 2 tails that
were merged.
To see the other patches from that merged branch you must go up from
there. If you go to 715cb47c720bcdd11846cae6c6cab325bb1a982b, you do
see 68b6e306dfeccef103b071e0812dc3a375830ac0 as it's parent