Re: [PATCH v4 31/39] bundle: add new version for use with SHA-256

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