On Wed, Jun 14, 2017 at 10:36:43AM +0200, Michael Haggerty wrote:
> The code for `git log --bisect` is questionable. It calls
> `for_each_ref_in_submodule()` with prefix "refs/bisect/bad", which is
> the actual name (not a prefix) of the reference that it is interested
> in. So the callback is called with the empty string as path, and that in
> turn is passed to a variety of functions, like `ref_excluded()`,
> `get_reference()`, `add_rev_cmdline()`, and `add_pending_oid()`. I'm not
> sure whom to ping; the code in question was introduced eons ago:
>
> ad3f9a71a8 Add '--bisect' revision machinery argument, 2009-10-27
>
> It seems to me that we should add a `for_each_fullref_in_submodule()`
> and call that instead. I'll submit a patch doing that, though I'm not
> certain that no new problems will arise from the callbacks getting full
> rather than trimmed reference names (also for "refs/bisect/good").
I doubt that would be a problem. The current values are nonsensical (an
empty string for bad, and "-$sha1" for good. They're mostly used in the
cmdline and pending lists. It would affect things like --exclude or
--source. I doubt anybody cares, but if they do IMHO the full names
would be a vast improvement.
Another option would be for this code to ask for:
for_each_ref_in("refs/bisect", ...);
and then match the various names in the callback. That gives sane short
names ("bad" and "good-$sha1"). But I think the full names are a much
better outcome. Some code (though note code I'd expect to use with
--bisect) assumes that the contents of the "name" field for pending
objects can be used to look up the object again. That's definitely not
true of the nonsense we produce now, but it would also not be true of
"bad" (because we don't DWIM refs/bisect).
> Another possible orthogonal "fix" is to make the refs side tolerate
> being asked to trim a refname down to the empty string, while still
> refusing to trim even more than that. I'll also submit a patch to that
> effect.
Even though the resulting "name" is silly, it does seem possible that
some caller would want to ask for all of refs/foo, even if it didn't
know if that was a single ref or a hierarchy. I do agree that any such
caller should probably be using for_each_fullref_in, though.
> Either of the patches fix the issue that was reported and pass the whole
> test suite (except for t1308, which seems to be broken in master for
> unrelated reasons).
It's broken if you use autoconf. See the patch I posted a few hours ago:
http://public-inbox.org/git/20170614053018.pbeftfyz2md4o73h@xxxxxxxxxxxxxxxxxxxxx/
-Peff