From: Phillip Wood <phillip.wood123@gmail.com>
To: Linus Arver <linus@ucla.edu>,
phillip.wood@dunelm.org.uk,
Linus Arver via GitGitGadget <gitgitgadget@gmail.com>,
git@vger.kernel.org
Cc: Christian Couder <chriscool@tuxfamily.org>,
Junio C Hamano <gitster@pobox.com>,
Emily Shaffer <nasamuffin@google.com>,
Josh Steadmon <steadmon@google.com>,
"Randall S. Becker" <rsbecker@nexbridge.com>,
Christian Couder <christian.couder@gmail.com>,
Kristoffer Haugsbakk <code@khaugsbakk.name>
Subject: Re: [PATCH v4 03/10] trailer: teach iterator about non-trailer lines
Date: Mon, 13 May 2024 16:11:55 +0100 [thread overview]
Message-ID: <f182c3f0-4d9e-491e-9b0d-59560cd93b8d@gmail.com> (raw)
In-Reply-To: <CAMo6p=FS3ShvBdutprWBiAVef6A1XjsXB1UJSQBk0s5euN=tog@mail.gmail.com>
Hi Linus
On 09/05/2024 08:11, Linus Arver wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
>> On 05/05/2024 02:37, Linus Arver wrote:
>>> Phillip Wood <phillip.wood123@gmail.com> writes:
>>>> On 02/05/2024 05:54, Linus Arver via GitGitGadget wrote:
>>>>> From: Linus Arver <linus@ucla.edu>
>>>>>
>>>>> The new "raw" member is important because anyone currently not using the
>>>>> iterator is using trailer_info's raw string array directly to access
>>>>> lines to check what the combined key + value looks like. If we didn't
>>>>> provide a "raw" member here, iterator users would have to re-construct
>>>>> the unparsed line by concatenating the key and value back together again
>>>>> --- which places an undue burden for iterator users.
>>>>
>>>> Comparing the raw line is error prone as it ignores custom separators
>>>> and variations in the amount of space between the key and the value.
>>>> Therefore I'd argue that the sequencer should in fact be comparing the
>>>> trailer key and value separately rather than comparing the whole line.
>>>
>>> I agree, but that is likely beyond the scope of this series as the
>>> behavior of comparing the whole line was preserved (not introduced) by
>>> this series.
>>
>> Right but this series is changing the trailer iterator api to
>> accommodate the sub-optimal sequencer code. My thought was that if the
>> sequencer did the right thing we wouldn't need to expose the raw line in
>> the iterator in the first place.
>
> Well, having familiarized myself with the trailer machinery I was more
> comfortable updating this area than reworking the details of the
> sequencer code.
>
> To me the sequencer code is a bit hard to read so I feel more
> comfortable updating the trailer code as I did here in this series. I
> also have another 40, 50 patches in the trailer area I want to continue
> pushing up for review, so I would rather focus on that first before
> coming back to the sequencer to try to clean it up. My other trailer
> patches make the trailer API more precise and aware of the separator and
> spaces around it,
I'm a bit confused by this as it looks like the
trailer_iterator_advance() already respects trailer.separators and
trims the key and value.
> so using those richer interfaces later would make it
> easier to clean up the sequencer area, I think.
>
> So in summary I would rather not get into refactoring the sequencer at
> this time.
Far enough, I hoped I might have time over the weekend to look at the
sequencer code in more detail but that didn't work out. So long as we're
not building bad abstractions into the trailer iterator to accommodate
the sequencer we can come back and clean up append_signoff() later.
Best Wishes
Phillip
next prev parent reply other threads:[~2024-05-13 15:12 UTC|newest]
Thread overview: 66+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-16 6:27 [PATCH 0/6] Make trailer_info struct private (plus sequencer cleanup) Linus Arver via GitGitGadget
2024-03-16 6:27 ` [PATCH 1/6] trailer: teach iterator about non-trailer lines Linus Arver via GitGitGadget
2024-03-16 6:27 ` [PATCH 2/6] sequencer: use the trailer iterator Linus Arver via GitGitGadget
2024-03-16 6:27 ` [PATCH 3/6] interpret-trailers: access trailer_info with new helpers Linus Arver via GitGitGadget
2024-03-16 6:27 ` [PATCH 4/6] trailer: make parse_trailers() return trailer_info pointer Linus Arver via GitGitGadget
2024-03-16 6:27 ` [PATCH 5/6] trailer: make trailer_info struct private Linus Arver via GitGitGadget
2024-03-16 6:27 ` [PATCH 6/6] trailer: retire trailer_info_get() from API Linus Arver via GitGitGadget
2024-03-16 17:06 ` [PATCH 0/6] Make trailer_info struct private (plus sequencer cleanup) Junio C Hamano
2024-03-26 22:00 ` Junio C Hamano
2024-04-19 5:36 ` Linus Arver
2024-04-19 5:22 ` [PATCH v2 0/8] " Linus Arver via GitGitGadget
2024-04-19 5:22 ` [PATCH v2 1/8] Makefile: sort UNIT_TEST_PROGRAMS Linus Arver via GitGitGadget
2024-04-19 5:22 ` [PATCH v2 2/8] trailer: add unit tests for trailer iterator Linus Arver via GitGitGadget
2024-04-19 5:33 ` Linus Arver
2024-04-19 18:46 ` Linus Arver
2024-04-19 21:52 ` Junio C Hamano
2024-04-20 0:14 ` Linus Arver
2024-04-19 5:22 ` [PATCH v2 3/8] trailer: teach iterator about non-trailer lines Linus Arver via GitGitGadget
2024-04-19 5:22 ` [PATCH v2 4/8] sequencer: use the trailer iterator Linus Arver via GitGitGadget
2024-04-23 21:19 ` Junio C Hamano
2024-04-19 5:22 ` [PATCH v2 5/8] interpret-trailers: access trailer_info with new helpers Linus Arver via GitGitGadget
2024-04-19 5:22 ` [PATCH v2 6/8] trailer: make parse_trailers() return trailer_info pointer Linus Arver via GitGitGadget
2024-04-23 23:17 ` Junio C Hamano
2024-04-19 5:22 ` [PATCH v2 7/8] trailer: make trailer_info struct private Linus Arver via GitGitGadget
2024-04-23 23:27 ` Junio C Hamano
2024-04-25 3:17 ` Linus Arver
2024-04-19 5:22 ` [PATCH v2 8/8] trailer: retire trailer_info_get() from API Linus Arver via GitGitGadget
2024-04-23 23:27 ` Junio C Hamano
2024-04-24 0:27 ` [PATCH v2 0/8] Make trailer_info struct private (plus sequencer cleanup) Junio C Hamano
2024-04-26 0:26 ` [PATCH v3 00/10] " Linus Arver via GitGitGadget
2024-04-26 0:26 ` [PATCH v3 01/10] Makefile: sort UNIT_TEST_PROGRAMS Linus Arver via GitGitGadget
2024-04-26 0:26 ` [PATCH v3 02/10] trailer: add unit tests for trailer iterator Linus Arver via GitGitGadget
2024-04-26 14:51 ` Christian Couder
2024-04-26 16:20 ` Junio C Hamano
2024-04-26 16:25 ` Linus Arver
2024-04-26 0:26 ` [PATCH v3 03/10] trailer: teach iterator about non-trailer lines Linus Arver via GitGitGadget
2024-04-27 12:50 ` Christian Couder
2024-04-30 4:42 ` Linus Arver
2024-04-30 4:55 ` Linus Arver
2024-04-26 0:26 ` [PATCH v3 04/10] sequencer: use the trailer iterator Linus Arver via GitGitGadget
2024-04-26 0:26 ` [PATCH v3 05/10] interpret-trailers: access trailer_info with new helpers Linus Arver via GitGitGadget
2024-04-26 0:26 ` [PATCH v3 06/10] trailer: make parse_trailers() return trailer_info pointer Linus Arver via GitGitGadget
2024-04-26 0:26 ` [PATCH v3 07/10] trailer: make trailer_info struct private Linus Arver via GitGitGadget
2024-04-26 0:26 ` [PATCH v3 08/10] trailer: retire trailer_info_get() from API Linus Arver via GitGitGadget
2024-04-26 0:26 ` [PATCH v3 09/10] trailer: document parse_trailers() usage Linus Arver via GitGitGadget
2024-04-26 0:26 ` [PATCH v3 10/10] trailer unit tests: inspect iterator contents Linus Arver via GitGitGadget
2024-04-27 12:51 ` [PATCH v3 00/10] Make trailer_info struct private (plus sequencer cleanup) Christian Couder
2024-05-02 4:54 ` [PATCH v4 " Linus Arver via GitGitGadget
2024-05-02 4:54 ` [PATCH v4 01/10] Makefile: sort UNIT_TEST_PROGRAMS Linus Arver via GitGitGadget
2024-05-02 4:54 ` [PATCH v4 02/10] trailer: add unit tests for trailer iterator Linus Arver via GitGitGadget
2024-05-02 16:54 ` Junio C Hamano
2024-05-02 4:54 ` [PATCH v4 03/10] trailer: teach iterator about non-trailer lines Linus Arver via GitGitGadget
2024-05-04 15:33 ` Phillip Wood
2024-05-05 1:37 ` Linus Arver
2024-05-05 14:09 ` Phillip Wood
2024-05-09 7:11 ` Linus Arver
2024-05-13 15:11 ` Phillip Wood [this message]
2024-05-13 15:13 ` Phillip Wood
2024-05-02 4:54 ` [PATCH v4 04/10] sequencer: use the trailer iterator Linus Arver via GitGitGadget
2024-05-02 4:54 ` [PATCH v4 05/10] interpret-trailers: access trailer_info with new helpers Linus Arver via GitGitGadget
2024-05-02 4:54 ` [PATCH v4 06/10] trailer: make parse_trailers() return trailer_info pointer Linus Arver via GitGitGadget
2024-05-02 4:54 ` [PATCH v4 07/10] trailer: make trailer_info struct private Linus Arver via GitGitGadget
2024-05-02 4:54 ` [PATCH v4 08/10] trailer: retire trailer_info_get() from API Linus Arver via GitGitGadget
2024-05-02 4:54 ` [PATCH v4 09/10] trailer: document parse_trailers() usage Linus Arver via GitGitGadget
2024-05-02 4:54 ` [PATCH v4 10/10] trailer unit tests: inspect iterator contents Linus Arver via GitGitGadget
2024-05-02 17:15 ` [PATCH v4 00/10] Make trailer_info struct private (plus sequencer cleanup) Junio C Hamano
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=f182c3f0-4d9e-491e-9b0d-59560cd93b8d@gmail.com \
--to=phillip.wood123@gmail.com \
--cc=chriscool@tuxfamily.org \
--cc=christian.couder@gmail.com \
--cc=code@khaugsbakk.name \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=gitster@pobox.com \
--cc=linus@ucla.edu \
--cc=nasamuffin@google.com \
--cc=phillip.wood@dunelm.org.uk \
--cc=rsbecker@nexbridge.com \
--cc=steadmon@google.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).