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
Attachment:
signature.asc
Description: PGP signature
