From: Linus Arver <linusa@google.com>
To: Jonathan Tan <jonathantanmy@google.com>,
Linus Arver via GitGitGadget <gitgitgadget@gmail.com>
Cc: Jonathan Tan <jonathantanmy@google.com>,
git@vger.kernel.org, Glen Choo <glencbz@gmail.com>,
Christian Couder <chriscool@tuxfamily.org>,
Phillip Wood <phillip.wood123@gmail.com>
Subject: Re: [PATCH v4 2/4] trailer: find the end of the log message
Date: Thu, 19 Oct 2023 17:24:55 -0700 [thread overview]
Message-ID: <owlymsweqgx4.fsf@fine.c.googlers.com> (raw)
In-Reply-To: <20230928231644.3529127-1-jonathantanmy@google.com>
Hi Jonathan, it's been a while because I was on vacation. I've forgotten
about most of the intricacies of this patch (I think this was a good
thing, read on below).
Jonathan Tan <jonathantanmy@google.com> writes:
> "Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
>> From: Linus Arver <linusa@google.com>
>>
>> Previously, trailer_info_get() computed the trailer block end position
>> by
>>
>> (1) checking for the opts->no_divider flag and optionally calling
>> find_patch_start() to find the "patch start" location (patch_start), and
>> (2) calling find_trailer_end() to find the end of the trailer block
>> using patch_start as a guide, saving the return value into
>> "trailer_end".
>>
>> The logic in (1) was awkward because the variable "patch_start" is
>> misleading if there is no patch in the input. The logic in (2) was
>> misleading because it could be the case that no trailers are in the
>> input (yet we are setting a "trailer_end" variable before even searching
>> for trailers, which happens later in find_trailer_start()). The name
>> "find_trailer_end" was misleading because that function did not look for
>> any trailer block itself --- instead it just computed the end position
>> of the log message in the input where the end of the trailer block (if
>> it exists) would be (because trailer blocks must always come after the
>> end of the log message).
>
> [...]
>
> As it is, the reader still needs to know that trailer_start is where
> the trailer would start if it was present, and then I think it's quite
> natural to have trailer_end be where the trailer would end if it was
> present.
>
> I believe the code is simpler this way, because trailer absence now no
> longer needs to be special-cased when we use these variables (or maybe
> they sometimes do, but not as often, since code that writes to the end
> of the trailers, for example, can now just write at trailer_end instead
> of having to check whether trailers exist). Same comment for patch 4
> regarding using the special value 0 if no trailers are found (I think
> the existing code makes more sense).
I think the root cause of my confusion with this codebase is due to the
variables being named as if the things they refer to exist, but without
any guarantees that they indeed exist. This applies to "patch_start"
(the patch part might not be present in the input),
"trailer_{start,end}" (trailers block might not exist (yet)). IOW these
variables are named as if the intent is to always add new trailers into
the input, which may not be the case (we have "--parse", after all).
Looking again at patch 4, I'm now leaning toward dropping it. Other
than the reasons you cited, we also add a new struct field which by
itself does not add new information (the information can already be
deduced from the other fields). In the near future I want to simplify
the data structures as much as possible, and adding a new field seems to
go against this desire of mine.
>> Combine the logic in (1) and (2) together into find_patch_start() by
>> renaming it to find_end_of_log_message(). The end of the log message is
>> the starting point which find_trailer_start() needs to start searching
>> backward to parse individual trailers (if any).
>
> Having said that, if patch_start is too confusing for whatever reason,
> this refactoring makes sense. (Avoid the confusing name by avoiding
> needing to name it in the first place.)
I think the existing code is confusing, and would prefer to keep this
patch.
>> -static size_t find_patch_start(const char *str)
>> +static size_t find_end_of_log_message(const char *input, int no_divider)
>> {
>> + size_t end;
>> +
>> const char *s;
>>
>> - for (s = str; *s; s = next_line(s)) {
>> + /* Assume the naive end of the input is already what we want. */
>> + end = strlen(input);
>> +
>> + /* Optionally skip over any patch part ("---" line and below). */
>> + for (s = input; *s; s = next_line(s)) {
>> const char *v;
>>
>> - if (skip_prefix(s, "---", &v) && isspace(*v))
>> - return s - str;
>> + if (!no_divider && skip_prefix(s, "---", &v) && isspace(*v)) {
>> + end = s - input;
>> + break;
>> + }
>> }
>>
>> - return s - str;
>> + /* Skip over other ignorable bits. */
>> + return end - ignored_log_message_bytes(input, end);
>> }
>
> This sometimes redundantly calls strlen and sometimes redundantly loops.
> I think it's better to do as the code currently does - so, have a big
> if/else at the beginning of this function that checks no_divider.
Will update, thanks.
next prev parent reply other threads:[~2023-10-20 0:24 UTC|newest]
Thread overview: 72+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-08-05 5:04 [PATCH 0/5] Trailer readability cleanups Linus Arver via GitGitGadget
2023-08-05 5:04 ` [PATCH 1/5] trailer: separate public from internal portion of trailer_iterator Linus Arver via GitGitGadget
2023-08-07 21:16 ` Glen Choo
2023-08-08 12:19 ` Phillip Wood
2023-08-10 23:15 ` Linus Arver
2023-08-10 22:50 ` Linus Arver
2023-08-05 5:04 ` [PATCH 2/5] trailer: split process_input_file into separate pieces Linus Arver via GitGitGadget
2023-08-07 22:39 ` Glen Choo
2023-08-11 0:41 ` Linus Arver
2023-08-05 5:04 ` [PATCH 3/5] trailer: split process_command_line_args into separate functions Linus Arver via GitGitGadget
2023-08-07 22:51 ` Glen Choo
2023-08-11 0:59 ` Linus Arver
2023-08-11 1:02 ` Linus Arver
2023-08-11 21:11 ` Glen Choo
2023-08-05 5:04 ` [PATCH 4/5] trailer: teach find_patch_start about --no-divider Linus Arver via GitGitGadget
2023-08-07 23:28 ` Glen Choo
2023-08-11 1:25 ` Linus Arver
2023-08-11 20:51 ` Glen Choo
2023-08-05 5:04 ` [PATCH 5/5] trailer: rename *_DEFAULT enums to *_UNSPECIFIED Linus Arver via GitGitGadget
2023-08-07 23:45 ` Glen Choo
2023-08-11 18:00 ` Linus Arver
2023-09-09 6:16 ` [PATCH v2 0/6] Trailer readability cleanups Linus Arver via GitGitGadget
2023-09-09 6:16 ` [PATCH v2 1/6] trailer: separate public from internal portion of trailer_iterator Linus Arver via GitGitGadget
2023-09-11 17:10 ` Junio C Hamano
2023-09-09 6:16 ` [PATCH v2 2/6] trailer: split process_input_file into separate pieces Linus Arver via GitGitGadget
2023-09-11 17:10 ` Junio C Hamano
2023-09-09 6:16 ` [PATCH v2 3/6] trailer: split process_command_line_args into separate functions Linus Arver via GitGitGadget
2023-09-09 6:16 ` [PATCH v2 4/6] trailer: teach find_patch_start about --no-divider Linus Arver via GitGitGadget
2023-09-11 17:25 ` Junio C Hamano
2023-09-14 2:19 ` Linus Arver
2023-09-14 3:12 ` Junio C Hamano
2023-09-14 5:31 ` Linus Arver
2023-09-09 6:16 ` [PATCH v2 5/6] trailer: rename *_DEFAULT enums to *_UNSPECIFIED Linus Arver via GitGitGadget
2023-09-11 18:54 ` Junio C Hamano
2023-09-14 2:41 ` Linus Arver
2023-09-14 3:16 ` Junio C Hamano
2023-09-22 18:23 ` Linus Arver
2023-09-22 19:48 ` Junio C Hamano
2023-09-26 5:30 ` Linus Arver
2023-09-09 6:16 ` [PATCH v2 6/6] trailer: use offsets for trailer_start/trailer_end Linus Arver via GitGitGadget
2023-09-11 19:01 ` Junio C Hamano
2023-09-14 1:21 ` Linus Arver
2023-09-14 3:18 ` Linus Arver
2023-09-22 19:50 ` [PATCH v3 0/9] Trailer readability cleanups Linus Arver via GitGitGadget
2023-09-22 19:50 ` [PATCH v3 1/9] trailer: separate public from internal portion of trailer_iterator Linus Arver via GitGitGadget
2023-09-22 19:50 ` [PATCH v3 2/9] trailer: split process_input_file into separate pieces Linus Arver via GitGitGadget
2023-09-22 19:50 ` [PATCH v3 3/9] trailer: split process_command_line_args into separate functions Linus Arver via GitGitGadget
2023-09-22 19:50 ` [PATCH v3 4/9] trailer: rename *_DEFAULT enums to *_UNSPECIFIED Linus Arver via GitGitGadget
2023-09-22 19:50 ` [PATCH v3 5/9] commit: ignore_non_trailer computes number of bytes to ignore Linus Arver via GitGitGadget
2023-09-22 19:50 ` [PATCH v3 6/9] trailer: find the end of the log message Linus Arver via GitGitGadget
2023-09-22 19:50 ` [PATCH v3 7/9] trailer: use offsets for trailer_start/trailer_end Linus Arver via GitGitGadget
2023-09-22 19:50 ` [PATCH v3 8/9] trailer: only use trailer_block_* variables if trailers were found Linus Arver via GitGitGadget
2023-09-22 19:50 ` [PATCH v3 9/9] trailer: make stack variable names match field names Linus Arver via GitGitGadget
2023-09-22 22:47 ` [PATCH v3 0/9] Trailer readability cleanups Junio C Hamano
2023-09-22 23:13 ` Linus Arver
2023-09-23 0:48 ` Junio C Hamano
2023-09-26 5:40 ` Linus Arver
2023-09-26 6:22 ` [PATCH v4 0/4] " Linus Arver via GitGitGadget
2023-09-26 6:22 ` [PATCH v4 1/4] commit: ignore_non_trailer computes number of bytes to ignore Linus Arver via GitGitGadget
2023-09-26 6:22 ` [PATCH v4 2/4] trailer: find the end of the log message Linus Arver via GitGitGadget
2023-09-28 23:16 ` Jonathan Tan
2023-10-20 0:24 ` Linus Arver [this message]
2023-10-20 0:36 ` Junio C Hamano
2023-09-26 6:22 ` [PATCH v4 3/4] trailer: use offsets for trailer_start/trailer_end Linus Arver via GitGitGadget
2023-09-26 6:22 ` [PATCH v4 4/4] trailer: only use trailer_block_* variables if trailers were found Linus Arver via GitGitGadget
2023-10-20 19:01 ` [PATCH v5 0/3] Trailer readability cleanups Linus Arver via GitGitGadget
2023-10-20 19:01 ` [PATCH v5 1/3] commit: ignore_non_trailer computes number of bytes to ignore Linus Arver via GitGitGadget
2023-10-20 19:01 ` [PATCH v5 2/3] trailer: find the end of the log message Linus Arver via GitGitGadget
2023-10-20 21:29 ` Junio C Hamano
2023-12-29 6:42 ` Linus Arver
2023-12-29 21:03 ` Linus Arver
2023-10-20 19:01 ` [PATCH v5 3/3] trailer: use offsets for trailer_start/trailer_end Linus Arver via GitGitGadget
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=owlymsweqgx4.fsf@fine.c.googlers.com \
--to=linusa@google.com \
--cc=chriscool@tuxfamily.org \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=glencbz@gmail.com \
--cc=jonathantanmy@google.com \
--cc=phillip.wood123@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).