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

From: brian m. carlson <sandals@crustytoothpaste.net>
Date: Jul 26, 2020, 22:59 UTC
Message-ID: <20200726225910.GD6540@camp.crustytoothpaste.net>
In-Reply-To: <CAPig+cR+e9XGNCgtDMHUsaAgbKDi=-bztwtd64fVZj7S05ppKQ@mail.gmail.com>

On 2020-07-26 at 22:18:23, Eric Sunshine wrote:

Show 7 quoted lines

> 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.

Show 12 quoted lines

> 
> 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.

Show 16 quoted lines

> 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