Glen Choo <chooglen@xxxxxxxxxx> writes:
> "Linus Arver via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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.
> [...] 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).
