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

[Edmundo Carmona Antoranz <eantoranz@gmail.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?

It does! Let me take care of it. (Didn't know about DEVELOPER
environment variable up until... 5 minutes ago?)

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