Re: [PATCH 02/14] builtin/repack: replace hard-coded constant

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Oct 8, 2018, 23:01 UTC
Message-ID: <CAPig+cS5VdYq1ZOrP9oPwpXZ3ZmSV3_QdQAHiz5fV1Tc1uEF9A@mail.gmail.com>
In-Reply-To: <CAGZ79kYvW2PFdLfvd3W_t6rPs=oMBDxgMEgDRLtC4MHyo6MXVQ@mail.gmail.com>

On Mon, Oct 8, 2018 at 6:27 PM Stefan Beller <sbeller@google.com> wrote:

Show 15 quoted lines

> On Mon, Oct 8, 2018 at 2:57 PM brian m. carlson
> <sandals@crustytoothpaste.net> wrote:
> > -               if (line.len != 40)
> > -                       die("repack: Expecting 40 character sha1 lines only from pack-objects.");
> > +               if (line.len != the_hash_algo->hexsz)
> > +                       die("repack: Expecting full hex object ID lines only from pack-objects.");
>
> This is untranslated as it is plumbing? If so, maybe
>
>     if (is_sha1(the_hash_algo)
>         die("repack: Expecting 40 character sh...
>     else
>         die(repack: Expecting full hex object ID in %s, of length %d",
>             the_hash_algo->name,
>             the_hash_algo->hexsz);

Special-casing for SHA-1 seems overkill for an error message. A script expecting this particular error condition and this particular error message would be fragile indeed.