Proper way to abort incorrect cherry-picking?

Proper way to abort incorrect cherry-picking?

[Eugene Sajine]

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

Re: Proper way to abort incorrect cherry-picking?

[David Borowitz]

On Wed, Apr 28, 2010 at 12:38, Eugene Sajine <euguess@gmail.com> wrote:
>
> 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?

git reset --hard HEAD@{1}?

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

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}.

> Thanks,
> Eugene
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Proper way to abort incorrect cherry-picking?

[Jonathan Nieder]

Eugene Sajine wrote:

> 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?

git reset --merge

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

Thanks,
Jonathan

Re: Proper way to abort incorrect cherry-picking?

[Eugene Sajine]

On Wed, Apr 28, 2010 at 3:49 PM, David Borowitz <dborowitz@google.com> wrote:
> On Wed, Apr 28, 2010 at 12:38, Eugene Sajine <euguess@gmail.com> wrote:
>>
>> 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?
>
> git reset --hard HEAD@{1}?

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

Re: Proper way to abort incorrect cherry-picking?

[Eugene Sajine]

On Wed, Apr 28, 2010 at 3:50 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Eugene Sajine wrote:
>
>> 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?
>
> git reset --merge
>
> Ideas for where this should be put in the git-cherry-pick.txt manual?
>

That's interesting - thanks a lot!

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

Thanks,
Eugene

Re: Proper way to abort incorrect cherry-picking?

[Jon Seymour]

On Thu, Apr 29, 2010 at 5:59 AM, Eugene Sajine <euguess@gmail.com> wrote:
> On Wed, Apr 28, 2010 at 3:49 PM, David Borowitz <dborowitz@google.com> wrote:
>> On Wed, Apr 28, 2010 at 12:38, Eugene Sajine <euguess@gmail.com> wrote:
>>>
>>> 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?
>>
>> git reset --hard HEAD@{1}?
>
> 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.
>

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.

> Thanks,
> Eugene
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: Proper way to abort incorrect cherry-picking?

[Jonathan Nieder]

Hi Jon,

Jon Seymour wrote:

> If you use git reset --mixed HEAD@{1} you can reset the index to
> HEAD@{1} to reflect the pre-merge state.

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

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

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

Re: Proper way to abort incorrect cherry-picking?

[Jon Seymour]

On Thu, Apr 29, 2010 at 9:37 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Hi Jon,
>
> Jon Seymour wrote:
>
>> If you use git reset --mixed HEAD@{1} you can reset the index to
>> HEAD@{1} to reflect the pre-merge state.
>
> The HEAD doesn’t advance in a failed merge, right?

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.

git cherry(pick) dumps core

[Andreas Krey]

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

Re: git cherry(pick) dumps core

[Jonathan Nieder]

Hi Andreas,

Andreas Krey wrote:

> 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.

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.

diff --git a/builtin/revert.c b/builtin/revert.c
index bbaa937..9f8ceab 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -114,6 +114,8 @@ static int get_message(const char *raw_message, struct commit_message *out)
 	if ((out->reencoded_message = reencode_string(raw_message,
 					git_commit_encoding, encoding)))
 		out->message = out->reencoded_message;
+	else
+		out->message = raw_message;
 
 	abbrev = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);
 	abbrev_len = strlen(abbrev);

Re: git cherry(pick) dumps core

[Andreas Krey]

On Thu, 29 Apr 2010 14:49:36 +0000, Jonathan Nieder wrote:
> Hi Andreas,
...
> Thanks for the report.  What encoding were you using?  (You can check
> with ???git cat-file commit <revision you were trying to cherry-pick>???.)

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.

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

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

Andreas

Re: git cherry(pick) dumps core

[Jonathan Nieder]

Andreas Krey wrote:
> On Thu, 29 Apr 2010 14:49:36 +0000, Jonathan Nieder 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>’.)
>
> Actually, it was both UTF-8, both by the defaults in get_message():
[...]
> 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.

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

[PATCH] cherry-pick: do not dump core when iconv fails

[Jonathan Nieder]

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(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 5a5b721..5adaf27 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -97,8 +97,13 @@ static int get_message(const char *raw_message, struct commit_message *out)
 		encoding = "UTF-8";
 	if (!git_commit_encoding)
 		git_commit_encoding = "UTF-8";
-	if ((out->reencoded_message = reencode_string(raw_message,
-					git_commit_encoding, encoding)))
+
+	out->reencoded_message = NULL;
+	out->message = raw_message;
+	if (strcmp(encoding, git_commit_encoding))
+		out->reencoded_message = reencode_string(raw_message,
+					git_commit_encoding, encoding);
+	if (out->reencoded_message)
 		out->message = out->reencoded_message;
 
 	abbrev = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);
-- 
1.7.1

Re: [PATCH] cherry-pick: do not dump core when iconv fails

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> 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.

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.