Re: [PATCH 4/5] trailer: teach find_patch_start about --no-divider

From: Linus Arver <linusa@google.com>
Date: Aug 11, 2023, 01:25 UTC
Message-ID: <owlyjzu2fjz9.fsf@fine.c.googlers.com>
In-Reply-To: <kl6ly1im8ma5.fsf@chooglen-macbookpro.roam.corp.google.com>

Glen Choo <chooglen@google.com> writes:

Show 20 quoted lines

> "Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
>> @@ -812,14 +812,14 @@ static ssize_t last_line(const char *buf, size_t len)
>>   * Return the position of the start of the patch or the length of str if there
>>   * is no patch in the message.
>>   */
>> -static size_t find_patch_start(const char *str)
>> +static size_t find_patch_start(const char *str, int no_divider)
>>  {
>>  	const char *s;
>>  
>>  	for (s = str; *s; s = next_line(s)) {
>>  		const char *v;
>>  
>> -		if (skip_prefix(s, "---", &v) && isspace(*v))
>> +		if (!no_divider && skip_prefix(s, "---", &v) && isspace(*v))
>>  			return s - str;
>>  	}
>
> Assuming we wanted to make this unit-testable anyway, could we just move
> the strlen() call into this function?

I don't see why we should preserve the if-statement and associated strlen() call if we can just get rid of it.

Show 6 quoted lines

> [...] I
> don't find this easier to understand. Now the reader needs to read the
> code to see "if no_divider is given, noop until the end of the string,
> at which point str will point to the end, and s - str will give us the
> length of str", as opposed to "there are no dividers, so just return
> strlen(str)".

The main idea behind this patch is to make find_patch_start() return the correct answer. Currently it does not in all cases (whether --no-divider is provided), and so the caller has to calculate the start of the patch with strlen manually. By moving the --no-divider flag into this function, we force all callers to consider this important option.

For additional context we recently had to fix a bug where we weren't passing in this flag to the interpret-trailers builtin. See be3d654343 (commit: pass --no-divider to interpret-trailers, 2023-06-17). There we acknowledged that some callers forgot to pass in --no-divider to interpret-trailers (such as the caller that was fixed up in that commit).

I mention the above example because although it's not the exact same thing as here, I think the scenario of "sometimes callers can forget about --no-divider" is an important one to prevent wherever possible. That's why I like this patch (in addition to the reasons cited in the commit message).