Junio C Hamano <gitster@xxxxxxxxx> writes:
> Linus Arver <linusa@xxxxxxxxxx> 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.
