Re: [PATCH v2 4/8] sequencer: use the trailer iterator

[Junio C Hamano <gitster@pobox.com>]

"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Linus Arver <linusa@google.com>
>
> Instead of calling "trailer_info_get()", which is a low-level function
> in the trailers implementation (trailer.c), call
> trailer_iterator_advance(), which was specifically designed for public
> consumption in f0939a0eb1 (trailer: add interface for iterating over
> commit trailers, 2020-09-27).
>
> Avoiding "trailer_info_get()" means we don't have to worry about options
> like "no_divider" (relevant for parsing trailers). We also don't have to
> check for things like "info.trailer_start == info.trailer_end" to see
> whether there were any trailers (instead we can just check to see
> whether the iterator advanced at all).
>
> Note how we have to use "iter.raw" in order to get the same behavior as
> before when we iterated over the unparsed string array (char **trailers)
> in trailer_info.

> Signed-off-by: Linus Arver <linusa@google.com>
> ---
>  sequencer.c | 27 ++++++++++++---------------
>  1 file changed, 12 insertions(+), 15 deletions(-)

OK.  The code reduction primarily comes from the fact that the
original was manually iterating over the trailer lines that you can
get from the iterator API.

> +	while (trailer_iterator_advance(&iter)) {
> +		i++;
> +		if (sob && !strncmp(iter.raw, sob->buf, sob->len))
> +			found_sob = i;
> +	}
> +	trailer_iterator_release(&iter);
>  
> +	if (!i)
> +		return 0;
>  
> +	found_sob_last = (int)i == found_sob;

This is slightly harder to reason about, as we cannot directly say
"the collection being iterated over has .nr members, and what we
found was at the end" like the original could do in its loop.

> -	for (i = 0; i < info.trailer_nr; i++)
> -		if (sob && !strncmp(info.trailers[i], sob->buf, sob->len)) {
> -			found_sob = 1;
> -			if (i == info.trailer_nr - 1)
> -				found_sob_last = 1;
> -		}

As 'i' is incremented before we set found_sob to it in the new loop,
when it is assigned in the loop, the value of found_sob will never
be zero.  It used to be that found_sob takes only 0 or 1, but
because we only care about found_sob and found_sob_last being
zero/non-zero in the remainder of the code, this does not affect the
correctness of the code.

Looking good.