Re: [PATCH 4/7] revert: allow single-pick in the middle of cherry-pick sequence

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Dec 14, 2011, 16:21 UTC
Message-ID: <20111214162148.GA481@elie.hsd1.il.comcast.net>
In-Reply-To: <CALkWK0mt03SSNT-svUO1wHdq5OpM=0xQO3FHkSGGEDuW-jUEXA@mail.gmail.com>

(out of order for convenience) Ramkumar Ramachandra wrote:

> Jonathan Nieder wrote:

>> Suggested-by: Johannes Sixt <j6t@kdbg.org>
>
> Could you link to the corresponding thread with Johannes?

No, I prefer not to. If I did a good job, the commit message would explain enough already, and in exceptional cases, the interested reader can look up the mailing list message the commit comes from and walk upthread, no?

> Cute feature, although I don't ever recall needing it personally.  Why
> does this relatively esoteric "feature" belong along with the other
> "maintenance patches" in  jn/maint-sequencer-fixes?

Read ahead in the series, or read the cover letter. :)

>                              What I'm really interested in seeing is
> how you persist opts for "cherry-pick --continue" when a single-commit
> pick fails: in other words, how you manage to get " --continue of
> single-pick respects -x" to pass.

That's a good question. I did the lazy thing and let the existing "git cherry-pick" logic take care of it (it writes MERGE_MSG).

Show 10 quoted lines

>> +               struct commit *cmit;
>> +               if (prepare_revision_walk(opts->revs))
>> +                       die(_("revision walk setup failed"));
>> +               cmit = get_revision(opts->revs);
>> +               if (!cmit || get_revision(opts->revs))
>> +                       die("BUG: expected exactly one commit from walk");
>> +               return single_pick(cmit, opts);
>> +       }
>
> I'd have expected you to reuse prepare_revs().

Why?  The purposes do not overlap much.

Show 9 quoted lines

>> +       if (opts->revs->cmdline.nr == 1 &&
>> +           opts->revs->cmdline.rev->whence == REV_CMD_REV &&
>> +           opts->revs->no_walk &&
>> +           !opts->revs->cmdline.rev->flags) {
>
> Yuck, seriously.
> 1. I'd have expected you to check opts->revs->commits, not
> opts->revs->cmdline.nr.  Okay, you're using the cmdline because the
> revision walk hasn't happened yet.

It would have been easy to do a revision walk and count and I'm using the cmdline instead deliberately --- the goal really is "anything more complicated than a simple rev on the command line should trip the multi-pick logic".

I admit though that I'm not too familiar with the new cmdline_info API. I'd welcome a simpler expression with the same effect.

Also, I probably should have included a test that does some

		git cherry-pick picked^..picked

thing and verifies that this is treated as a multi-pick. And documented this. :)

Thanks for pointing out the questionable bits. I am tempted to reroll to put this after patch 6/7, which would make it possible to use "git reset --merge" in the commit message for a more natural explanation.

That would also provide an opportunity to reuse some text from [1], which in hindsight seems to have explained some aspects of each patch a little more clearly.

Thanks, and hoping that clarifies a little, Jonathan

[1] http://thread.gmane.org/gmane.comp.version-control.git/185716/focus=186811