Proper way to abort incorrect cherry-picking?

hi,

we have tried to cherry-pick 2 commits from one branch to another branch, but unfortunately the incorrect commit was chosen to be applied first.

Thus, the automatic cherry-pick failed and caused conflicts, so in order to to cancel the whole operation i had to do the following:

1. mark the conflicting files as resolved (without even resolving
them) by doing git add.
2. unstage all files staged for commit as a result of incomplete cherry picking
3. manually checkout touched files to their correct state (git checkout file)

and then i was able to repeat cherry-picking with correct commits.

Is there a better way? Shouldn't there be a "git cherry-pick --abort" for such cases as it exists for rebase?

Thanks, Eugene

On Wed, Apr 28, 2010 at 12:38, Eugene Sajine <euguess@gmail.com> wrote:

git reset --hard HEAD@{1}?

ISTM the --abort flag to rebase is useful because HEAD may have changed an arbitrary number of times between the start of the rebase and now, so you wouldn't know which reflog entry to choose. (The same holds for "git bisect reset".) But with a cherry-pick it's always @{1}.

On Wed, Apr 28, 2010 at 3:49 PM, David Borowitz <dborowitz@google.com> wrote:

not always working. In our particular case there were some local modifications to other files, which would be blown away with this for no reason. That's why I went the long way of resetting specific files.

Thanks, Eugene

On Thu, Apr 29, 2010 at 5:59 AM, Eugene Sajine <euguess@gmail.com> wrote:

If you use git reset --mixed HEAD@{1} you can reset the index to HEAD@{1} to reflect the pre-merge state. The unstaged changes will then be a combination of the failed merge and the local modifications to the files. You can then revert the changes from the merge.

Then you can use git stash to move the local modifications out of the way, then repeat the cherry pick in the correct order, then use git stash pop to reapply the local modifications to the working tree and index.

This is more complicated than it needs to be - if you had stashed (or committed) before cherry picking, things would be simpler.

jon.

Hi Jon,

Jon Seymour wrote:

The HEAD doesn’t advance in a failed merge, right?

[...]

If this were really necessary, I would consider it a bug.

I do think recovery is more complicated than it needs to be, since one has to check whether the merge/cherry-pick failed before cancelling it. There are three cases.

 - If an early check prevented the operation (message with “fatal:”,
   status = 128), then the index and work tree were not touched.

   No recovery required.

 - If there were conflicts (message with “Conflicts:”, status = 1),
   the index will record the competing versions of conflicted files,
   and the work tree will represent the situation with conflict
   markers.

   Use ‘git reset --merge’ to recover.

 - If the merge proceeded cleanly (status = 0), but it was a bad
   idea after all, the index and work tree record the new version now.

   Use ‘git reset --keep HEAD@{1}’ to undo the operation.

Have fun, Jonathan

On Thu, Apr 29, 2010 at 9:37 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:

True, but I understood this issue to be that there were two cherry picks, one of which worked, one of which failed because it was out of order. Hence git reset HEAD@{1} would reset prior to the first successful (but out of order) cherry-pick.

Any way, thanks for the explaining the virtue of git reset --merge.

jon.

Hi,

I just have a case of git cherry-pick dieing with a core dump. The directly offending lines are get_message() in buildin/revert.c:

	if ((out->reencoded_message = reencode_string(raw_message,
					git_commit_encoding, encoding)))
		out->message = out->reencoded_message;

	abbrev = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);
	abbrev_len = strlen(abbrev);

	/* Find beginning and end of commit subject. */
	p = out->message;
	while (*p && (*p != '\n' || p[1] != '\n'))

and out->message is null at that point.

It looks like reencode_string is returning NULL, and get_message can't quite cope with that.

This is in v1.7.1 (plus a few mods in git-daemon.c).

I suppose it is a failing of iconv_open, as this is on sparc SunOS 5.9, but the reaction is a bit harsh.

Unfortunately the debugger is giving me strange results, like:

434 if (!in_encoding) (gdb) next 436 conv = iconv_open(out_encoding, in_encoding); (gdb) print conv $3 = 0x1003f8 (gdb) next 437 if (conv == (iconv_t) -1) (gdb) print conv $4 = 0x1003f8 (gdb) step 474 }

and the disasembly is just as counter-intuitive (with the branch targets).

Andreas

Hi Andreas,

Andreas Krey wrote:

Thanks for the report. What encoding were you using? (You can check with ‘git cat-file commit <revision you were trying to cherry-pick>’.)

The code you mention was just wrong, sorry. If the local iconv doesn’t understand some recording, the correct behavior is to pass the message through, not to segfault.

With this change, cherry-pick would treat the old message as UTF-8 (or whatever the current [i18n] commitencoding setting specifies, if present). If the message happens to contain an illegal byte sequence, the cherry-pick will not segfault but it will still fail.

In other words, this patch should fix the segfault, but it does not fix the underlying problem which was there before.

On Thu, 29 Apr 2010 14:49:36 +0000, Jonathan Nieder wrote:

...

Actually, it was both UTF-8, both by the defaults in get_message():

reencode_string (
    in=0x14d500 "tree 97cea0f57e06e719c9e7e38c3fc17022048b4f38\nparent 1ef1bb56e95284e683076f0d0cebe26e8ba02eca\nauthor Andreas Krey <a.krey@gmx.de> 1272557698 +0200\ncommitter Andreas Krey <a.krey@gmx.de> 1272557698 +02"..., out_encoding=0x1003f8 "UTF-8", 
        in_encoding=0x1003f8 "UTF-8") at utf8.c:434

I'd guess that in that case, reencode_string wouldn't need to fire up iconv at all, and just copy the original string.

There being two different incarnations of iconv on this machine isn't making anything easier. :-( Will report when I find out what's wrong here. utf8 should be pretty universally known by now.

I was wondering whether to be or not to be, er, to just use the raw message or to die in this case.

Andreas

Andreas Krey wrote:

[...]

Yes, the problem is that the to and from encodings match. I hadn’t realized so before, but iconv doesn’t like that on some platforms (e.g., Solaris).

Since this is the usual case, we should probably check for it like git mailinfo does instead of waiting for iconv to fail.

Jonathan

When cherry-picking, usually the new and old commit encodings are both UTF-8. Most old iconv implementations do not support this trivial conversion, so on old platforms, out->message remains NULL, and later attempts to read it segfault.

Fix this by noticing the input and output encodings match and skipping the iconv step, like the other reencode_string() call sites already do. Also stop segfaulting on other iconv failures: if iconv fails for some other reason, the best we can do is to pass the old message through.

This fixes a regression introduced in v1.7.1-rc0~15^2~2 (revert: clarify label on conflict hunks, 2010-03-20).

Reported-by: Andreas Krey <a.krey@gmx.de>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Andreas reported this cherry-pick regression about a week ago, and a
patch similar to this one seemed to fix it.  The only question that
made me stall was what to do if iconv just doesn’t understand an
encoding:

 - from a certain point of view it might make sense to pass through
   the message and note the old encoding

 - but on the other hand, that would not respect the user’s preference
   as expressed in i18n.commitencoding, and it would deny the caller
   the chance to fix the encoding problem in a text editor before
   commiting.

That second point was driven home when I tried to implement this: it required teaching ‘git commit’ to respect a new GIT_COMMIT_ENCODING variable, but this was a pain (there is a sizeable amount of code paying attention to i18n.commitencoding) and the result would have been cherry-pick ignoring the $GIT_COMMIT_ENCODING preference, creating encoding mismatches between ident and commit message to boot.

Everyone should be using utf8 anyway. :)

Anyway, this minimal fix should be safe (it just restores the old behavior, except we do not use iconv for trivial conversions any more). Patch is against v1.7.1-rc0~15^2~2.

Thanks to Andreas for the help.

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

Jonathan Nieder <jrnieder@gmail.com> writes:

This agrees with what builtin/commit.c does around ll.896; even if the encoding pair says "utf8" and "UTF-8", we would do the right thing.

Thanks.

Eugene Sajine wrote:

git reset --merge

Ideas for where this should be put in the git-cherry-pick.txt manual?

Thanks, Jonathan

On Wed, Apr 28, 2010 at 3:50 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

That's interesting - thanks a lot!

Yes, it wold be nice to have it mentioned somewhere in help for cherry-picking.

Thanks, Eugene