Subject: Re: [PATCH v2 4/6] trailer: teach find_patch_start about --no-divider

From: Linus Arver <[email-masked]>
Date: Wed, 13 Sep 2023 19:19:01 -0700

Junio C Hamano <[email-masked]> writes:

> "Linus Arver via GitGitGadget" <[email-masked]> writes:
>
>> From: Linus Arver <[email-masked]>
>>
>> Currently, find_patch_start only finds the start of the patch part of
>> the input (by looking at the "---" divider) for cases where the
>> "--no-divider" flag has not been provided. If the user provides this
>> flag, we do not rely on find_patch_start at all and just call strlen()
>> directly on the input.
>>
>> Instead, make find_patch_start aware of "--no-divider" and make it
>> handle that case as well. This means we no longer need to call strlen at
>> all and can just rely on the existing code in find_patch_start. By
>> forcing callers to consider this important option, we avoid the kind of
>> mistake described in be3d654343 (commit: pass --no-divider to
>> interpret-trailers, 2023-06-17).
>
> OK.  The code pays attention to "---" so making it stop doing so
> when the "--no-*" option is given will make the function responsible
> for finding the beginning of the patch.
>
> I wonder if we should rename this function while we are at it,
> though.  When "--no-divider" is given, the expected use case is
> *not* to have a patch at all, and it is dubious that a function
> whose name is find_patch_start() can possibly do anything useful.

IOW, saying as the caller of this function, "find the patch start in
this input I'm giving you, but also FYI the input has no patch in it"
sounds wrong. Agreed.

> The real purpose of this function is to find the end of the log
> message, isn't it?

Indeed.

> And the caller uses the end of the log message
> it found and gives it to find_trailer_start() and find_trailer_end()
> as the upper boundary of the search for the trailer block.

Right! So a better name might be something like
"find_trailer_search_boundary" with a comment like

    Find the point at which we should stop searching for trailers (to
    parse them). This is either the end of the input string (obviously),
    or the point when we see "---" indicating the start of the "patch
    part".

Will update.