further adventures in git(/hub)ery

2 views
Skip to first unread message

Myk Melez

unread,
Nov 27, 2010, 11:44:57 PM11/27/10
to mozilla-la...@googlegroups.com
This evening I decided to check if there were any outstanding pull requests for the SDK repository (to which I haven't been paying attention).

There were! The oldest was pull request 29 from Thomas Bassetto, which contains two small fixes (first, second) to the docs.

So I fetched the branch of his fork in which the changes reside:

$ git fetch https://github.com/tbassetto/addon-sdk.git master

But that branch (and the fork in general) is a few weeks out-of-date, so "git diff HEAD FETCH_HEAD" showed a bunch of changes, and it was unclear how painful the merge would be.

Thus I decided to try cherry-picking the changes, my first time using "git cherry-pick".

The first one went great:

$ git cherry-pick 8268334070d03a896d5c006d1b4db94d4cb44b17
Finished one cherry-pick.
[master ceadb1f] Fixed an internal link in the widget doc
 1 files changed, 1 insertions(+), 1 deletions(-)

Except that I realized afterward I hadn't added "r,a=myk" to the commit message. So I tried "git commit --amend" for the first time, which worked just fine:

$ git commit --amend
[master 2d674a6] Fixed an internal link in the widget doc; r,a=myk
 1 files changed, 1 insertions(+), 1 deletions(-)

Next time I'll remember to use the "--edit" flag to "git cherry-pick", which lets one "edit the commit message prior to committing."

The second cherry-pick was more complicated, because I only wanted one of the two changes in the commit (in my review, I had identified the second change as unnecessary); and, as it turned out, also because there was a merge conflict with other commits.

I started by cherry-picking the commit with the "--no-commit" option (so I could remove the second change):

$ git cherry-pick --no-commit 666ad7a99e05e338348dfc579d5b1f75e8d3bb1b
Automatic cherry-pick failed.  After resolving the conflicts,
mark the corrected paths with 'git add <paths>' or 'git rm <paths>' and commit the result.
When commiting, use the option '-c 666ad7a' to retain authorship and message.

The conflict was trivial, and I knew where it was, so I resolved it manually (instead of trying "git mergetool" for the first time), removed the second change, added the merged file, and committed the result, using the "-c" option to preserve the original author and commit message while allowing me to edit the message to add "r,a=myk":

$ git add packages/addon-kit/docs/request.md
$ git commit -c 666ad7a
[master 774d1cb] Completed the example in the Request module documentation; r,a=myk
 1 files changed, 1 insertions(+), 0 deletions(-)

Then I used "gitg" and "git log master ^upstream/master" to verify that the commits looked good to go, after which I pushed them:

$ git push upstream master
[git's standard obscure and disconcerting gobbledygook]

Finally, I closed the pull request with this comment that summarized what I did and provided links to the cherry-picked commits.

It would have been nice if the cherry-picked commit that didn't have merge conflicts (and which I didn't change in the process of merging) had kept its original commit ID, but I sense that that is somehow a fundamental violation of the model.

It would also have been nice if the cherry-picked commit messages had been automatically annotated with references to the original commits.

But overall the process seemed pretty reasonable, it was fairly easy to do what I wanted and recover from mistakes, and the author, committer, reviewer, and approver are clearly indicated in the cherry-picked commits (first, second).

-myk

Brian Warner

unread,
Nov 29, 2010, 8:32:23 PM11/29/10
to mozilla-la...@googlegroups.com
On 11/27/10 8:44 PM, Myk Melez wrote:

> But that branch (and the fork in general) is a few weeks out-of-date,
> so "git diff HEAD FETCH_HEAD" showed a bunch of changes, and it was
> unclear how painful the merge would be.

Incidentally, the best way to get the diff you want is:

git diff HEAD...FETCH_HEAD

The "A...B" syntax means "find a common ancestor of both A and B, then
show me the diff between that ancestor and B". (note that this means
A...B is not just the reverse of B...A).

That will tell you what they changed, without also telling you (the
inverse of) what you changed.

If you happen to know that they only made a few changes, you could also
use:

git diff FETCH_HEAD^ FETCH_HEAD

to see their last change ("A^" means "parent of A"). Also "git diff
FETCH_HEAD^^ FETCH_HEAD^" to see the next-to-last change. Or "git diff
FETCH_HEAD~2 FETCH_HEAD~1" to do the same.

"git rev-parse --help" will give you the full messy details, including
such awesomeness as "git diff master@{1} master" or "git diff
master@{yesterday} master".

> It would have been nice if the cherry-picked commit that didn't have
> merge conflicts (and which I didn't change in the process of merging)
> had kept its original commit ID, but I sense that that is somehow a
> fundamental violation of the model.

Right. The commit you recorded contained exactly the same *code* as the
original, but had different comments, specifically your "r,a=myk"
message. (internally, that results in two separate "revision" objects
which both point to the same "tree" object). The SHA1 revision
identifier covers all code, comments, and history, so there's no way to
get the same commit ID without having exactly the same comments too.

> It would also have been nice if the cherry-picked commit messages had
> been automatically annotated with references to the original commits.

Yeah, it looks like you can add an "-x" option to "git cherry-pick" to
get this: it appends "(cherry picked from commit 12345)" to the message.
I also think some of the github tools might include this.

Now, what would be really snazzy is if the original commits were
annotated with the IDs of the new commits that now contain their
changes, so you could look at them and tell that they could be retired.
The original commits are immutable, of course, but there's a "git notes"
feature that could conceivably be put to use here.


cheers,
-Brian

Reply all
Reply to author
Forward
0 new messages