Re: [PATCH 1/1] ls-refs.c: minimize number of refs visited

[Jeff King <peff@peff.net>]

On Tue, Jan 19, 2021 at 02:09:04PM -0500, Taylor Blau wrote:

> > What does "delimited by /" mean?
> 
> Ah, I just meant that it looks for the longest common prefix where it
> will only split at '/' characters. But, that's not right at all:
> find_longest_prefixes_1() will happily split anywhere there is a
> difference.

Right. We thought in early revisions of the ref-filter work that we
might have to split on path components, but it turns out that the
underlying ref code is happy to take arbitrary prefixes. And it's that
code which defines our strategy; Even if the ls-refs code wanted to
allow only full path components, it should be using the limiting from
for_each_ref_in() only as an optimization, and applying its own
filtering to the output.

> > Without really understanding the longest common prefix code in
> > ref-filter.c, my intuitive concern is that the specifics of glob
> > matching and special treatment of '/' may bite us. I suppose we'll be
> > fine because ls-refs has its own matching logic. So long as
> > for_each_fullref_in_prefixes yield enough prefixes, the end result
> > would remain the same.
> 
> Right. We can ignore the concern about '/' (seeing my comment above),
> and note that find_longest_prefixes_1() breaks on glob metacharacters,
> so we'll only match or overmatch the desired set (and we'll never
> undermatch).
> 
> I made sure to write in the second patch downthread that
> ls-refs.c:send_ref() correctly handles receiving too many refs (and it
> discards ones that it doesn't want).

Yeah, I think the glob-handling is OK for that reason. We'd have a
smaller prefix, which means seeing more refs. So it's never a
correctness issue, but only a potential optimization one (we consider
more refs in ls-refs.c than we might otherwise). But I think even that s
impossible, because the glob characters are not valid in refnames. So we
would never see one at all in a string which is meant to be a pure
prefix.

> > I think my approach would be to expose the new
> > for_each_fullref_in_prefixes iterator you propose through test-tool,
> > and unit test it so we can be sure it handles both contexts
> > (for-each-refs with globs and special '/', and ls-refs without any
> > special character behavior) correctly.
> >
> > I may be overly cautious here, take this with a grain of salt because
> > I am not an experienced Git contributor. On that topic, apologies if
> > I'm botching my inline replies in this email.
> 
> I do appreciate your caution, but I'm not sure exposing a test-tool is
> necessary, since we already test this behavior extensively in t6300 (and
> now t5701, t5702 and t5704, too).

Agreed. test-tool is fine when we have no way to easily feed data to a
unit. But in this case, the prefix code is easy to test via the
for-each-ref plumbing. We'd want to make sure the new setting in ls-refs
is covered, too, but I agree that t5701 covers that well (regular
fetches make sure we don't send too few refs, but those ones check that
we limited the refs in the expected way).

-Peff