Re: [PATCH v2 0/8] fast-import: tighten parsing of paths

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 4362 bytes --]

On Sun, Apr 07, 2024 at 07:46:52PM -0400, Eric Sunshine wrote:
> On Sun, Apr 7, 2024 at 5:19 PM Thalia Archibald <thalia@archibald.dev> wrote:
> > On Apr 1, 2024, at 02:02, Thalia Archibald wrote:
> > >> fast-import has subtle differences in how it parses file paths between each
> > >> occurrence of <path> in the grammar. Many errors are suppressed or not checked,
> > >> which could lead to silent data corruption. A particularly bad case is when a
> > >> front-end sent escapes that Git doesn't recognize (e.g., hex escapes are not
> > >> supported), it would be treated as literal bytes instead of a quoted string.
> > >>
> > >> Bring path parsing into line with the documented behavior and improve
> > >> documentation to fill in missing details.
> > >
> > > Thanks for the review, Patrick. I've made several changes, which I think address
> > > your feedback. What's the protocol for adding `Reviewed-by`? Since I don't know
> > > whether I, you, or Junio add it, I've refrained from attaching your name to
> > > these patches.
> >
> > Hello! Friendly ping here. It’s been a week since the last emails for this patch
> > set, so I’d like to check in on the status.
> 
> Pinging is certainly the correct thing to do.
> 
> Regarding `Reviewed-by:`: that trailer doesn't mean that someone
> merely read and commented on a patch. Instead, it's explicitly _given_
> by a reviewer to indicate that the reviewer has thoroughly reviewed
> the patch set and is confident that it is ready to be merged into the
> project, at which point Junio typically adds the `Reviewed-by:`.
> 
> > As a new contributor to the project, I don’t yet have a full view of the
> > contribution flow, aside from what I’ve read. I suspect the latency is because I
> > may not have cc’d all the area experts. (When I sent v1, I used separate Cc
> > lines in send-email --compose, but it dropped all but the last. After Patrick
> > reviewed it, I figured I could leave the cc list as-is for v2, assuming I’d get
> > another review.) I’ve now cc’d everyone listed by contrib/contacts, as well as
> > the maintainer. For anyone not a part of the earlier discussion, the latest
> > version is at https://lore.kernel.org/git/cover.1711960552.git.thalia@archibald.dev/.
> > If that’s not the problem, I’d be keen to know what I could do better.
> 
> Lack of response may be due to the series being overlooked, or it
> could mean that nobody has any particular interest in the changes
> (which is not to say that there is anything wrong with the changes),
> or that people are simply busy elsewhere. It could also mean that this
> reroll is good enough and reviewers have nothing else to add. So,
> cc'ing potentially interested people makes sense.

Yeah, for this patch series I think it's mostly a lack of interest.
Which is too bad, because it does address some real issues with
git-fast-import(1). Part of the problem is also that this area does not
really have an area expert at all -- if you git-shortlog(1) for example
"builtin/fast-import.c" for the last year you will see that it didn't
get much attention at all.

Anyway, I'm currently trying to make it a habit to pick up and review
random patch series that didn't get any attention at all every once in a
while, which is also why I reviewed your first version. I'm taking these
a bit slower though, also in the hope that some initial discussion may
motivate others to chime in, as well. Which may explain why I didn't yet
review your v2.

In any case I do have it in my backlog and will get to it somewhen this
week.

> > I have several more patch sets in the works, that I’ve held back on sending
> > until this one is finished, in case I’ve been doing something wrong. I want to
> > move forward. Thank you for your time.
> 
> If the additional patch sets are unrelated to this patch set, then I
> don't see a reason to hold them back. Feel free to send them. Even if
> they are related to this patch set, you may still want to send them.
> After all, doing so may get the ball rolling again on this patch set.

Agreed. Especially given that this is your first contribution, the
quality of your patch series is quite high. So I don't see much of a
reason to hold back the other patch series in case they are unrelated.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]