From: Junio C Hamano <gitster@pobox.com>
Date: Wed, 12 Jun 2019 18:11:45 GMT
Subject: Re: [GSoC][PATCH v2 2/3] cherry-pick/revert: add --skip option
Message-ID: <xmqqsgseisgu.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <6d3c1c1e-6140-dd8c-c37f-8c625b04ddc9@gmail.com>

Phillip Wood <phillip.wood123@gmail.com> writes:

> Thanks for the new version, this is looking pretty good now, just a
> few comments below

I agree that this step is looking pretty good now.

I didn't check closely, but when 1/3 undergoes necessary polishing,
it may have repercussions on this step, though (I did see that the
change in 3/3 would have overlaps with what was touched by 1/3 that
needs to be done differently).

Thanks for guiding Rohit's series forward.  

> This is only slightly different from reset_for_rollback() if you
> decide to keep a separate code path for skip vs abort then I'd be
> tempted to combine the two like this.
>
> diff --git a/sequencer.c b/sequencer.c
> index ecf4be7e15..b187b4222e 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2740,11 +2740,13 @@ static int reset_merge(void) {
>  static int reset_for_rollback(const struct object_id *oid)
>  {
>         const char *argv[4];    /* reset --merge <arg> + NULL */
> +       size_t i = 0;

That size_t is, eh, "unusual".  For an index into a small local
array of known size, just sticking to bog-standard-and-boring 'int'
would make it less distracting for future readers of the code.

Or even better, perhaps use argv-array, so that you do not have to
worry about sizing the local array sufficiently large in the first
place.

> +       argv[i++] = "reset";
> +       argv[i++] = "--merge";
> +       if (oid)
> +               argv[i++] = oid_to_hex(oid);
> +       argv[i] = NULL;
>         return run_command_v_opt(argv, RUN_GIT_CMD);
>  }