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

From: Linus Arver <[email-masked]>
Date: Wed, 13 Sep 2023 22:31:40 -0700

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

> Linus Arver <[email-masked]> writes:
>
>>> The real purpose of this function is to find the end of the log
>>> message, isn't it?
>>
>> Indeed.
>> ...
>> Right! So a better name might be something like
>> "find_trailer_search_boundary" with a comment like
>
> Or "find_end_of_log_message()", which we agreed to be the real
> purpose of this function ;-)

I did this locally, but in doing so realized that we have in trailer.c

    /* Return the position of the end of the trailers. */
    static size_t find_trailer_end(const char *buf, size_t len)
    {
        return len - ignore_non_trailer(buf, len);
    }

and the ignore_non_trailer() comes from commit.c, which says

    /*
    * Inspect the given string and determine the true "end" of the log message, in
    * order to find where to put a new Signed-off-by trailer.  Ignored are
    * trailing comment lines and blank lines.  To support "git commit -s
    * --amend" on an existing commit, we also ignore "Conflicts:".  To
    * support "git commit -v", we truncate at cut lines.
    *
    * Returns the number of bytes from the tail to ignore, to be fed as
    * the second parameter to append_signoff().
    */
    size_t ignore_non_trailer(const char *buf, size_t len)
    {

...and I am not so sure the new "find_end_of_log_message" name for
find_patch_start() is desirable because of the overlap in meaning with
the comment for ignore_non_trailer(). To recap, we have in
trailer_info_get() in trailer.c which (without this patch) has

    if (opts->no_divider)
        patch_start = strlen(str);
    else
        patch_start = find_patch_start(str);

    trailer_end = find_trailer_end(str, patch_start);
    trailer_start = find_trailer_start(str, trailer_end);
                
to find the trailer_end and trailer_start positions in the input. These
positions are the boundaries for parsing for actual trailers (if any).
More precisely, the "patch_start" variable helps us _skip_ the "patch
part" of the input (if any, denoted by "---"). The call to
find_trailer_end() helps us (again) _skip_ any parts that are not part
of the actual log message (per the comment in ignore_non_trailer()). So
as far as trailer_info_get() is concerned, we are just trying to skip
over areas where we shouldn't search/parse for trailers.

The above analysis leads me to some new ideas:

(1) For "find_end_of_log_message()", I think this name should really
    belong to ignore_non_trailer() in commit.c instead of
    find_patch_start() in trailer.c. The existing comment for
    ignore_non_trailer() already says that its job is to determine the
    true end of the log message (and does a lot of the necessary work to
    do this job).

(2) find_patch_start() should be named something like
    "shrink_trailer_search_space" (although this meaning belongs equally
    well to find_trailer_end()), because the point is to reduce the
    search space for parsing trailers.

(3) "find_trailer_end" is not a great function name because on first
    glance it implies that it found the end of trailers, but this is
    only true for the "happy path" of actually finding trailers.

I need to consider all of the above ideas and reroll this patch. Because
the ideas here closely relate to the "trailer_end" and "trailer_start"
variables, I will probably reorder the series so that this patch and the
last patch are closer together.