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 <https://github.com/mozilla/addon-sdk/commit/6a3c9e2a614f29b61e580a7a7...>.
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.
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:
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.
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.
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):
> For the second merge commit, I specified "--signoff", which added > "Signed-off-by: Myk Melez <m...@mozilla.org> <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
> 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.
On Fri, Nov 12, 2010 at 21:03, Erik Vold <erikvv...@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):
On Fri, Nov 12, 2010 at 21:03, Erik Vold <erikvv...@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.
On Sat, Nov 13, 2010 at 9:26 AM, 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:
> 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):
> git checkout -b toolness-bug-611042 master > git pull https://github.com/toolness/jetpack-sdk.git bug-611042 > (There might have been something else here, since the pull request resulted > in a merge; I don't quite remember.) > git checkout master > git merge --no-ff --no-commit toolness-bug-611042 > git commit --signoff -m "bug 611042: remove request.response.xml for e10s > compatibility; r=myk" --author "atul" > 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:
> 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
> -- > 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-labs-jetpack@googlegroups.com. > To unsubscribe from this group, send email to > mozilla-labs-jetpack+unsubscribe@googlegroups.com. > For more options, visit this group at > http://groups.google.com/group/mozilla-labs-jetpack?hl=en.
> For the second merge commit, I specified "--signoff", which added > "Signed-off-by: Myk Melez <m...@mozilla.org> <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.
> 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.
> 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
> -- > 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-labs-jetpack@googlegroups.com > . > To unsubscribe from this group, send email to > mozilla-labs-jetpack+unsubscribe@googlegroups.com<mozilla-labs-jetpack%2Bun subscribe@googlegroups.com> > . > For more options, visit this group at > http://groups.google.com/group/mozilla-labs-jetpack?hl=en.
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 <https://github.com/mozilla/addon-sdk/pull/43>), copy the command from step two, but change the word "pull" to "fetch", f.e.:
| 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.
> 1. From the "how to merge this pull request" section of the pull > request page (f.e. pull request 34 > <https://github.com/mozilla/addon-sdk/pull/43>), copy the > command from step two, but change the word "pull" to "fetch", f.e.:
$ 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
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<https://github.com/mozilla/addon-sdk/pull/43>), > copy the command from step two, but change the word "pull" to "fetch", f.e.:
> 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.
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.
On Tue, Nov 23, 2010 at 18:26, Myk Melez <m...@mozilla.org> wrote: > 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<https://github.com/mozilla/addon-sdk/pull/43>), > copy the command from step two, but change the word "pull" to "fetch", f.e.:
> $ 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
> 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
