patchsequencer: clarify intention to break out of loop

> Signed-off-by: Martin Ågren <martin.agren@gmail.com>
> ---
> diff --git a/sequencer.c b/sequencer.c
> @@ -2849,10 +2849,14 @@ static int do_reset(const char *name, int len, struct replay_opts *opts)
>                 /* Determine the length of the label */
> +               for (i = 0; i < len; i++) {
> +                       if (isspace(name[i])) {
>                                 len = i;
> +                               break;
> +                       }
> +               }
>                 strbuf_addf(&ref_name, "refs/rewritten/%.*s", len, name);

>
> On Sun, Oct 28, 2018 at 11:32 AM Martin Ågren <martin.agren@gmail.com> wrote:
> > [...]
> > Let's be explicit about breaking out of the loop. This helps the
> > compiler grok our intention. As a bonus, it might make it (even) more
> > obvious to human readers that the loop stops at the first space.
>
> This did come up in review[1,2]. I had a hard time understanding the
> loop because it looked idiomatic but wasn't (and we have preconceived
> notions about behavior of things which look idiomatic).
>
> [1]: https://public-inbox.org/git/CAPig+cQbG2s-LrAo9+7C7=dXifbWFJ3SzuNa-QePHDk7egK=jg@mail.gmail.com/
> [2]: https://public-inbox.org/git/CAPig+cRjU6niXpT2FrDWZ0x1HmGf1ojVZj3uk2qXEGe-S7i_HQ@mail.gmail.com/

> >                 /* Determine the length of the label */
> > +               for (i = 0; i < len; i++) {
> > +                       if (isspace(name[i])) {
> >                                 len = i;
> > +                               break;
> > +                       }
> > +               }
> >                 strbuf_addf(&ref_name, "refs/rewritten/%.*s", len, name);
>
> At least for me, this would be more idiomatic if it was coded like this:
>
>     for (i = 0; i < len; i++)
>         if (isspace(name[i]))
>             break;
>     strbuf_addf(..., i, name);
>
> or, perhaps (less concise):
>
>     for (i = 0; i < len; i++)
>         if (isspace(name[i]))
>             break;
>     len = i;
>     strbuf_addf(..., len, name);

>> diff --git a/sequencer.c b/sequencer.c
>> @@ -2849,10 +2849,14 @@ static int do_reset(const char *name, int len, struct replay_opts *opts)
>>                 /* Determine the length of the label */
>> +               for (i = 0; i < len; i++) {
>> +                       if (isspace(name[i])) {
>>                                 len = i;
>> +                               break;
>> +                       }
>> +               }
>>                 strbuf_addf(&ref_name, "refs/rewritten/%.*s", len, name);
>
> At least for me, this would be more idiomatic if it was coded like this:
>
>     for (i = 0; i < len; i++)
>         if (isspace(name[i]))
>             break;
>     strbuf_addf(..., i, name);
>
> or, perhaps (less concise):
>
>     for (i = 0; i < len; i++)
>         if (isspace(name[i]))
>             break;
>     len = i;
>     strbuf_addf(..., len, name);

> It came up in review [1, 2] that this non-idiomatic loop is a bit tricky.
> When we find a space, we set `len = i`, which gives us the answer we are
> looking for, but which also breaks out of the loop.
> 
> It turns out that this loop can confuse compilers as well. My copy of
> gcc 7.3.0 realizes that we are essentially evaluating `(len + 1) < len`
> and warns that the behavior is undefined if `len` is `INT_MAX`. (Because
> the assignment `len = i` is guaranteed to decrease `len`, such undefined
> behavior is not actually possible.)
> 
> Rewrite the loop to a more idiomatic variant which doesn't muck with
> `len` in the loop body. That should help compilers and human readers
> figure out what is going on here. But do note that we need to update
> `len` since it is not only used just after this loop (where we could
> have used `i` directly), but also later in this function.
> 
> While at it, reduce the scope of `i`.
> 
> [1] https://public-inbox.org/git/CAPig+cQbG2s-LrAo9+7C7=dXifbWFJ3SzuNa-QePHDk7egK=jg@mail.gmail.com/
> 
> [2] https://public-inbox.org/git/CAPig+cRjU6niXpT2FrDWZ0x1HmGf1ojVZj3uk2qXEGe-S7i_HQ@mail.gmail.com/
> 
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Martin Ågren <martin.agren@gmail.com>
> ---

>  Thanks for the comments on v1. Based on them, I decided to go for
>  Eric's option 2, and to make the log message less technical in favor of
>  "compilers and humans alike can get this wrong".
> 
>  sequencer.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index 0c164d5f98..e7aa4d5020 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2829,7 +2829,7 @@ static int do_reset(const char *name, int len, struct replay_opts *opts)
>  	struct tree_desc desc;
>  	struct tree *tree;
>  	struct unpack_trees_options unpack_tree_opts;
> -	int ret = 0, i;
> +	int ret = 0;
>  
>  	if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0)
>  		return -1;
> @@ -2849,10 +2849,13 @@ static int do_reset(const char *name, int len, struct replay_opts *opts)
>  		}
>  		oidcpy(&oid, &opts->squash_onto);
>  	} else {
> +		int i;
> +
>  		/* Determine the length of the label */
>  		for (i = 0; i < len; i++)
>  			if (isspace(name[i]))
> -				len = i;
> +				break;
> +		len = i;
>  
>  		strbuf_addf(&ref_name, "refs/rewritten/%.*s", len, name);
>  		if (get_oid(ref_name.buf, &oid) &&
> -- 
> 2.19.1.593.gc670b1f876.dirty
> 
> 

> On Tue, 30 Oct 2018, Martin Ågren wrote:
> > Rewrite the loop to a more idiomatic variant which doesn't muck with
> > `len` in the loop body. That should help compilers and human readers
> > figure out what is going on here. But do note that we need to update
> > `len` since it is not only used just after this loop (where we could
> > have used `i` directly), but also later in this function.
> >
> > Signed-off-by: Martin Ågren <martin.agren@gmail.com>
> > ---
>
> ACK. Thanks for cleaning up after me,