On 2020-07-26 at 22:18:23, Eric Sunshine wrote:
> This error message would be more helpful if it provided more context,
> such as the name it tried looking up. For instance:
> 
>     return error(_("unrecognized bundle header hash algorithm: "
>         "@object-format=%s"), arg);
> 
> or something.

I've improved it somewhat.

> The reader has to think through this unconditional NUL-termination
> more carefully than would be the case if...
> ... you just moved this strbuf_rtrim() call above the capability check
> conditional.

That is indeed much easier.

> 
> This conditional will become fragile when bundle version v4 is
> introduced and the git-bundle invocation somehow triggers v4 to be
> assigned to 'default_version', in which case:
> 
>     git bundle --version=3 ...
> 
> will complain:
> 
>     cannot write bundle version 3 with algorithm sha256
> 
> which is clearly wrong and misleading.

I've changed this variable to min_version.  There is one place we use it
as the default, but I think it's easy to change that if we want it to be
something different in the future, and all the rest of the uses are of a
minimum version.

> Do you still need the "head -n 4 ... &&" check at the very top of this
> list? Is that providing something that we don't get from the new code
> which uses test_cmp with 'expect' and 'actual' files?

Removed.

> In my earlier review when I suggested using an "expect" file and
> converting the object ID to some literal string such as "OID", the
> example I gave did indeed also use literal "message", though my use of
> "message" was meant as a placeholder that you would fill in with the
> real values, like this:
> 
>     -OID updated by origin
>     OID refs/heads/master
> 
> I probably should have been clearer about "message" being a
> placeholder (since I was too lazy to look up the actual values). I
> suppose the generic "message" you use here is no worse than the
> original code with its 'grep' invocations which didn't care about the
> message either. It makes me a bit uncomfortable for the test to
> unnecessarily be loose like this when it doesn't have to be, but it's
> not necessarily worth a re-roll.

Fixed.
-- 
brian m. carlson: Houston, Texas, US