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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 23, 2024, 21:19 UTC
Message-ID: <xmqqh6frojz9.fsf@gitster.g>
In-Reply-To: <84897cf5c83eb67c023603016b49fb7b56870aa3.1713504153.git.gitgitgadget@gmail.com>

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

Show 17 quoted lines

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

Show 11 quoted lines

> +	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.

Show 6 quoted lines

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