support rebase and cherry-pick #25

thewoolleyman · 2015-11-20T23:45:06Z

Fixes issue #24

FYI, I was unable to get the bats tests all running locally against master, I'm glad to provide tests if you can help me get the test environment going on my box.

jszwedko · 2015-11-21T21:30:24Z

src/git-duet/git-duet-cherry-pick/main.go

+
+func main() {
+	err := cmdrunner.Execute(
+		cmd.New("cherry-pick"))


👍 I didn't realize cherry-pick supports --signoff off the bat.

I may be missing something, but shouldn't this be cmd.NewWithSignoff("cherry-pick")?

Yes you're right, cherry-pick does support signoff - I'll fix.

After a little more thought, I think the --signoff on cherry-pick was probably meant to sign off on the commits you are cherry picking (that previously only had an author). By setting this here, it would override any committer present on the original commit -- it feels like this might be desirable, but definitely deserves a second thought.

I think you are right. Assuming that they were probably using git-duet for the original commit, it should have both an author and a committer.

And, I don't think those should be touched during cherry-picking. You are only moving the commit from one branch/ref to another, the "ownership" of the original commit isn't changing.

And in practice (at least in most workflows I know), this is usually only used to get individual commits onto a production release or hotfix branch (otherwise you'd be doing a merge or rebase of a whole branch to git all commits). And, this often isn't even done by the original committer on large teams - it could be someone responsible for deploying the release or hotfix, not the original coder.

So, I think this is fine the way it is :)

jszwedko · 2015-11-21T21:34:58Z

Thanks for getting the ball rolling on this @thewoolleyman! I responded on the issue regarding getting your environment setup.

rafecolton · 2015-11-22T19:09:53Z

README.md

 Suggested aliases:

 ```
 dci = duet-commit
 drv = duet-revert
 dmg = duet-merge
+drb = duet-merge


Looks like you have a couple copy/pasta errors here

Thanks, I'll fix.

I still see drb = duet-merge - should be drb = duet-rebase?

Yeah, I think the github UI is confused, I amended the commit and it's correct in my repo now:

thewoolleyman@master#diff-04c6e90faac2675aa89e2176d2eec7d8R145

I just cloned it down from your repo on master, commit c21558e

It shows:

dmg = duet-merge drb = duet-merge dcp = duet-cherry-pick

Oh sorry, there were two typos! sorry, fixed the other now...

Np, looks good now

glestaris · 2015-12-22T10:01:15Z

Sorry for spamming but +1. We use rebase quite often and it's always a pain having to fix the authors of each commit.

iamralch · 2015-12-22T10:01:54Z

When should I expect this pull request to be accepted?

jszwedko · 2015-12-22T16:57:20Z

@glestaris no worries -- can you describe your use case a little more? git rebase keeps the author/committer of the existing commit unless I'm missing something. The issue this PR was opened to address was the fact that git complains about the author not being set during a git rebase which I think is better solved by #4.

glestaris · 2015-12-24T08:06:25Z

@jszwedko The problem we are seeing is that we lose information about the commiter in all the rebased commits. Just to give an example:

I made a repo with master and wip-docs branches. wip-docs needs rebasing since master is updated after wip-docs was created:

$> g lola --pretty=format:"%h %s
    '%an' (author) and '%cn' (commiter)"
* 0e7578f Add getting started chapter in docs (HEAD, wip-docs)
|     'Alice Brown' (author) and 'John Spith' (commiter)
* a1ce86d Add docs and introduction chapter
|     'Alice Brown' (author) and 'John Spith' (commiter)
| * 32d3458 Add end-to-end tests (master)
|/      'Alice Brown' (author) and 'John Spith' (commiter)
* 636ee15 Update project title
|     'Alice Brown' (author) and 'John Spith' (commiter)
* 024e4e4 Add readme
      'Alice Brown' (author) and 'John Spith' (commiter)

After running git rebase master in wip-docs, I get to this state:

* f5563e5 Add getting started chapter in docs (HEAD, wip-docs)
|     'Alice Brown' (author) and 'George Lestaris' (commiter)
* 2b89e95 Add docs and introduction chapter
|     'Alice Brown' (author) and 'George Lestaris' (commiter)
* 32d3458 Add end-to-end tests (master)
|     'Alice Brown' (author) and 'John Spith' (commiter)
* 636ee15 Update project title
|     'Alice Brown' (author) and 'John Spith' (commiter)
* 024e4e4 Add readme
      'Alice Brown' (author) and 'John Spith' (commiter)

So you are probably right that it has to do with user.email and user.name since this is what it applies to the commiter. However, rebase does not keep the commiter unchanged and now we have to go back to the rebased commits, git duet with the correct author/commiter and then run git duet-commit --amend --no-edit --reset-author.

Is there any better way of doing it?

Thanks and sorry if this is now in the wrong PR. I am happy to create an issue if you prefer so :)

jszwedko · 2016-01-24T20:37:38Z

Hi @glestaris, apologies for the delay, you are completely right, I missed that it uses the current user as the committer during rebases for rebased commits (which was noted in the conversation above).

Giving that some additional thought, I do feel that if I am driving, I'd expect any rebased commits without a committer (read: the committer is same as the author with identical timestamps) to have me as the committer and keep the original author. However, if the commit I'm rebasing already has a committer different than the author (as it often will if users are pairing using this tool) I can see the case for wanting to keep the original committer on that commit instead of changing it to me.

This PR in particular simply introduces a git duet-rebase that has the effect of using the author from git solo rather than git config user.* when rebasing, but would suffer from the same issue: rebased commits would have their committer updated to the current author.

Unfortunately, given my quick review of the git porcelain there doesn't seem to be an easy way to achieve what I've described above. I think it might be possible by passing a --exec command to be run after each commit during git rebase, but that is only valid in interactive mode.

That leaves us with the following options:

Trying to determine a way to rebase without altering author/committer information at all -- which doesn't seem possible with the normal git porcelain, but feels like it might be with the lower level tools
Updating all rebased commits to have the current pair from git duet and erase the original author/committer (not sure if this is easily possible). This is what your workaround currently does, but only for the latest commit in the branch that was rebased.
Documenting the deficiency and possible workaround using interactive mode and --exec
?

I'll investigate the first option, but would love any other suggestions or insights.

Thanks!

jszwedko · 2016-01-24T20:39:24Z

There is also this language from http://git-scm.com/book/ch2-3.html:

You may be wondering what the difference is between author and committer. The author is the person who originally wrote the work, whereas the committer is the person who last applied the work.

which we might use to inform our approach.

thewoolleyman changed the title ~~PR#24 - support rebase and cherry-pick~~ support rebase and cherry-pick Nov 20, 2015

thewoolleyman force-pushed the master branch from e1c5d34 to 28e1d5c Compare November 20, 2015 23:49

jszwedko reviewed Nov 21, 2015
View reviewed changes

rafecolton reviewed Nov 22, 2015
View reviewed changes

thewoolleyman force-pushed the master branch 2 times, most recently from 3068804 to c21558e Compare November 23, 2015 16:56

support rebase and cherry-pick

8c0fd0e

thewoolleyman force-pushed the master branch from c21558e to 8c0fd0e Compare November 24, 2015 00:05

This was referenced Nov 24, 2015

Add support for duet-rebase and duet-cherry-pick and git stash #24

Open

Support setting user.name and user.email #4

Closed

cjcjameson mentioned this pull request Jul 6, 2017

Proposal: new flag --respect-author would solve some pain around rebases #52

Closed

jszwedko force-pushed the master branch from d8fb9d9 to fedf460 Compare April 11, 2018 03:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support rebase and cherry-pick #25

support rebase and cherry-pick #25

thewoolleyman commented Nov 20, 2015

jszwedko Nov 21, 2015

rafecolton Nov 22, 2015

thewoolleyman Nov 23, 2015

jszwedko Nov 25, 2015

thewoolleyman Nov 25, 2015

jszwedko commented Nov 21, 2015

rafecolton Nov 22, 2015

thewoolleyman Nov 23, 2015

thewoolleyman Nov 23, 2015

rafecolton Nov 23, 2015

thewoolleyman Nov 23, 2015

rafecolton Nov 24, 2015

thewoolleyman Nov 24, 2015

rafecolton Nov 24, 2015

glestaris commented Dec 22, 2015

iamralch commented Dec 22, 2015

jszwedko commented Dec 22, 2015

glestaris commented Dec 24, 2015

jszwedko commented Jan 24, 2016

jszwedko commented Jan 24, 2016

support rebase and cherry-pick #25

Are you sure you want to change the base?

support rebase and cherry-pick #25

Conversation

thewoolleyman commented Nov 20, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jszwedko commented Nov 21, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glestaris commented Dec 22, 2015

iamralch commented Dec 22, 2015

jszwedko commented Dec 22, 2015

glestaris commented Dec 24, 2015

jszwedko commented Jan 24, 2016

jszwedko commented Jan 24, 2016