git cherry-pick --continue?

Heya,

At the moment git cherry-pick stands out from the sequencer tools in that it doesn't support --continue but requires you to manually supply the '-c ...' argument to 'git commit' when there's a conflict instead. Is it desirable that we add such an option? And if so, how would one go about implementing it?

On Wed, Feb 10, 2010 at 09:37:10PM +0100, Sverre Rabbelier wrote:

I think it makes sense. It is perhaps a little iffy to use "continue" when you are not really continuing on to further cherry-picks (and in a rebase, continue is not just about resolving conflicts, but about continuing to the next item in the rebase). Cherry-picks are more like "am --resolved"[1].

But semantic nitpicks aside, I think "continue" is a good enough name. The differences between what it means for each command are fairly obvious based on the command, and there is real value in a user just having to remember one verb.

-Peff

[1] On the other hand, I usually mistype that as "git am --continue", which _does_ make sense, since you are applying a sequence of patches. Maybe "am" should support both.

On Wed, Feb 10, 2010 at 04:04:19PM -0500, Jeff King wrote:

Hmm. I was thinking "am" was the odd man out, but really there are only two sequencer commands that I noted: rebase and am. So you could perhaps argue that rebase should also learn "--resolved". Or am I forgetting one?

I find the patch below convenient. I dunno if anybody else actually cares.

-- >8 --
Subject: [PATCH] am: allow --continue as a synonym for --resolved

Rebase calls this same function "--continue", which means users may be trained to type it. There is no reason to deprecate --resolved (or -r), but adding this synonym is friendly to users.

Signed-off-by: Jeff King <peff@peff.net>
---
 git-am.sh |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

Heya,

On Wed, Feb 10, 2010 at 22:24, Jeff King <peff@peff.net> wrote:

I do, it's useful when applying a bunch of patches and conflicts arise :). Typing 'git am --continue' is what I'm used to from 'git rebase'.

Jeff King <peff@peff.net> writes:

I don't think you are forgetting anything, except that "am" came first with "resolved".

The focus of the verb is "I declare I am finished marking the resolution". Taking that declaration and continuing is ultimately "am"'s decision. IOW the user is not telling "am" to continue---the difference is subtle, but it was a conscious design decision.

"rebase --continue" came later, and I think its focus is placed more heavily on the instruction side ("Please continue"), and not on the declaration side ("I now have marked the resolution for all paths").

This causes people sometimes to want to see it "continue", even when then haven't marked the resolved paths as resolved. I personally think the focus is misplaced.

But that is just a philosophical difference ;-).

Junio C Hamano <gitster@pobox.com> writes:

Having said all I did in the previous message, I think "am --continue" would be a good addition.

And "rebase --resolved" would not make any sense if the reason the control is given back to you was because you ran "rebase -i" and marked a commit to be "edit"ed. Of course, we could add "--resolved" and "--edited" (or perhaps "--amended") to "rebase", and have it make sure that the correct one is given. For example, when it stopped for "edit", it would reject "rebase --resolved". But I do not think it is worth the hassle.

Heya,

On Wed, Feb 10, 2010 at 23:21, Junio C Hamano <gitster@pobox.com> wrote:

How about 'cherry-pick --resolved' though ;).

I don't see any benefit to that, in fact, I'd recommend against it.

Sverre Rabbelier <srabbelier@gmail.com> writes:

Changing the insn to suggest using "-C topic" when the original command line was "git cherry-pick topic" would be a good addition, too. Currently we suggest "-c" and abbreviated object name, neither of which is sensible.

While I think "--resolved" makes sense, I do not see much benefit, and it largely depends on what you do. If you are suggesting to commit with what is kept in $GIT_DIR/MERGE_MSG, I would actually recommend against it. The message will have "Conflicts:" information but that is meaningless unless you are recording from what context the commit was cherry-picked from at the same time.

Heya,

On Wed, Feb 10, 2010 at 23:34, Junio C Hamano <gitster@pobox.com> wrote:

I think it's sensible to use '-c' instead of '-C', just like 'git rebase -i' lets you change the commit message after you resolve a conflict.

I'm not sure how to implement it, but I was thinking to just automate the 'git commit -c ...' part. I don't like having to copy/paste some instruction, so maybe we can record the original commit somewhere and have 'git cherry-pick --resolve' be equivalent to 'git commit -c `cat .git/CHERRY_PICK_CMT`', or somesuch?

On Wed, Feb 10, 2010 at 02:34:21PM -0800, Junio C Hamano wrote:

I think using the actual name instead of the abbreviated sha1 is sensible. But I think "-c" makes sense, as it gives the user the chance to look over the commit message to see if they need to tweak it to match the conflict fixups. Savvy users who don't want to do that will know to use "-C".

Series to follow:

  [1/4]: cherry-pick: rewrap advice message
  [2/4]: cherry-pick: refactor commit parsing code
  [3/4]: cherry-pick: format help message as strbuf
  [4/4]: cherry-pick: show commit name instead of sha1

-Peff

The current message overflows on an 80-character terminal. While we're at it, fix the spelling of 'committing'.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin-revert.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

These lines are really just lookup_commit_reference re-implemented.

Signed-off-by: Jeff King <peff@peff.net>
---
Obviously this changes the exact text of the error messages, but I doubt
anybody cares too much.

 builtin-revert.c |   10 ++--------
 1 files changed, 2 insertions(+), 8 deletions(-)

This gets rid of the fixed-size buffer and an unchecked sprintf. That sprintf is actually OK as the only variable-sized thing put in it is an abbreviated sha1, which is bounded at 40 characters. However, the next patch will change that to something unbounded.

Note that this function now returns an allocated buffer instead of a static one; however, it doesn't matter as the only caller exits immediately.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin-revert.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

When we have a conflict, we advise the user to do:

  git commit -c $sha1

This works fine, but is unnecessarily confusing and annoying for the user to type, when:

  git commit -c $the_thing_you_called_cherry_pick_with

works just as well.

Signed-off-by: Jeff King <peff@peff.net>
---
I am pretty sure of the "works just as well". I couldn't think of a
reason why the commit name they gave would have changed between the
cherry-pick and the time they commit the resolved state.

 builtin-revert.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

On Thu, Feb 11, 2010 at 04:04:45PM -0500, Jeff King wrote:

Actually, I think the message is still a bit ugly after this, so perhaps this 5/4 would help:

-- >8 --
Subject: [PATCH] cherry-pick: prettify the advice message

It's hard to see the "how to commit" part of this message, which users may want to cut and paste. On top of that, having it in paragraph form means that a really long commit name may cause ugly wrapping. Let's make it prettier, like:

  Automatic cherry-pick failed.  After resolving the conflicts,
  mark the corrected paths with 'git add <paths>' or 'git rm <paths>'
  and commit the result with:

          git commit -c HEAD~23

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin-revert.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

On Thu, Feb 11, 2010 at 4:19 PM, Jeff King <peff@peff.net> wrote:

Blech, how is this an improvement? Why can't I just say "git cherry-pick --continue"?

If I've still got the message in my terminal, it's no harder to use the SHA1. And if I've lost the message in my terminal, HEAD~23 is lost and I've got to dig the SHA1 out of my shell history anyway.

(This isn't hypothetical; typically I'll cherry-pick, do some crap in that terminal such that the message is lost, then I've got to go through my history to see which commit I had been cherry picking. So changing the message doesn't help with that situation at all.)

j.

Jay Soffian <jaysoffian@gmail.com> writes:

Maybe it doesn't look like an improvement to you, but I usually do my editing in other terminal and come back to the shell, so this is a huge improvement. It depends on the user.

I don't think Peff meant this change as the sole replacement for --cont anyway, so I don't understand why you are so upset about it.

On Thu, Feb 11, 2010 at 06:05:18PM -0500, Jay Soffian wrote:

Because nobody has implemented it yet. ;P

The two improvements here are:

  1. We show what you typed as "git cherry-pick $X", so if you want to
     re-type it, you know it is just "git commit -c $X". This is not
     about speed, but about being more sensible for users to read.

  2. The output is now cut-and-pasteable, so you don't have to type it
     if it is still available in your terminal.

These were meant to make the situation better than it was; I agree that "git cherry-pick --continue" would be better still.

-Peff

On Wed, Feb 10, 2010 at 02:21:14PM -0800, Junio C Hamano wrote:

OK. I agree with your philosophical ramblings in the previous message, but I also think there is some value in making it simple for the user to remember.

Do you just want to pick up my patch from earlier in the thread, or do you have further comments? The only thing I could think to change would be that we may not want to even bother advertising --continue in the usage message (conversely, we could go a step further and actually advertise it in the manpage).

Agreed.

-Peff

Jeff King <peff@peff.net> writes:

Certainly I am in agreement and that is why I think "am --continue" is a good thing.

I would say our eventual goal should be to make "--continue" the primary word the end users would see. It would bring us closer to that goal to start advertising --continue early.

On Thu, Feb 11, 2010 at 12:36:52PM -0800, Junio C Hamano wrote:

OK. Then I think my patch is fine. But we could also do this if we wanted to push it further now:

-- >8 --
Subject: [PATCH] am: switch --resolved to --continue

Rebase calls this same function "--continue", which means users may be trained to type it. There is no reason to deprecate --resolved (or -r), so we will keep it as a synonym.

Signed-off-by: Jeff King <peff@peff.net>
---
Between this and the previous patch, I don't have a strong preference.
You can decide.

 Documentation/git-am.txt |    3 ++-
 git-am.sh                |    5 +++--
 2 files changed, 5 insertions(+), 3 deletions(-)

Hi,

On Thu, Feb 11, 2010 at 05:27:14PM -0500, Jeff King wrote:

Then maybe we should have this, too.

Best, Gábor

 -- >8 --
Subject: [PATCH] bash: support 'git am's new '--continue' option

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---
 contrib/completion/git-completion.bash |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)