Re: [PATCH v1] builtin/merge.c - cleanup of code in for-cycle that tests strategies

From

Edmundo Carmona Antoranz <eantoranz@gmail.com>

Date

Jul 8, 2019, 20:52 UTC

Message-ID

<CAOc6etbwHAAFSM-GwuD8BaC76EQNv+pQ5TgNCWN65DhNfV3KEw@mail.gmail.com>

In-Reply-To

<xmqqh87wxoza.fsf@gitster-ct.c.googlers.com>

On Mon, Jul 8, 2019 at 2:15 PM Junio C Hamano <gitster@pobox.com> wrote:

> In the body of the proposed commit log message, please finish each
> sentence with a full-stop.

Ok

>
> These bullets are all subjective, and do not add any value to what
> you already said in the second sentence.

Ok

>
> These are facts that readers can see for themselves and agree with.
>
> Something like...
>
>         The cmd_merge() function has a loop that tries different
>         merge strategies in turn, and stops when a strategy gets a
>         clean merge, while keeping the "best" conflicted merge so
>         far.
>
>         Make the loop easier to follow by moving the code around,
>         ensuring that there is only one "break" in the loop where
>         an automerge succeeds.  Also group the actions that are
>         performed after an automerge succeeds together to a single
>         location, outside and after the loop.
>
> perhaps?

I like it.

> > +                     int cnt = evaluate_result();
> This introduces -Wdeclaration-after-statement, doesn't it?

> Perhaps just declare the variable at the top of the for loop, next
> to where the local 'ret' is declared?

Yep, make sense

>
> Other than this single glitch, I think the code with this patch does
> become easier to follow.

Great!

Let me come back with v2 during the day.

Thanks!