Re: [PATCH 1/2] for_each_bisect_ref(): don't trim refnames

From: Jeff King <peff@peff.net>
Date: Jun 14, 2017, 09:22 UTC
Message-ID: <20170614092256.c3fmfcokuwbbcvbz@sigill.intra.peff.net>
In-Reply-To: <3615deefe90bebe746618b04c055a466a442f85b.1497430232.git.mhagger@alum.mit.edu>

On Wed, Jun 14, 2017 at 11:07:26AM +0200, Michael Haggerty wrote:

Show 14 quoted lines

> `for_each_bisect_ref()` is called by `for_each_bad_bisect_ref()` with
> a term "bad". This used to make it call `for_each_ref_in_submodule()`
> with a prefix "refs/bisect/bad". But the latter is the name of the
> reference that is being sought, so the empty string was being passed
> to the callback as the trimmed refname. Moreover, this questionable
> practice was turned into an error by
> 
>     b9c8e7f2fb prefix_ref_iterator: don't trim too much, 2017-05-22
> 
> It makes more sense (and agrees better with the documentation of
> `--bisect`) for the callers to receive the full reference names. So
> 
> * Add a new function, `for_each_fullref_in_submodule()`, to the refs
>   API.

You might want to mention that this is really just a hole in the existing API. We have for_each_ref_in_submodule() and for_each_fullref_in(), but not the missing link.

I don't think that makes it any more or less correct, but I thought at first you had to invent a new function totally.

Show 9 quoted lines

> * Change `for_each_bad_bisect_ref()` to call the new function rather
>   than `for_each_ref_in_submodule()`.
> 
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  refs.c     | 12 ++++++++++++
>  refs.h     |  5 ++++-
>  revision.c |  2 +-
>  3 files changed, 17 insertions(+), 2 deletions(-)

The change itself looks fine to me.

Since we obviously don't have even a single test for "--bisect", that might be worth adding.

-Peff