[02/14] builtin/repack: replace hard-coded constant
diff mbox series

Message ID 20181008215701.779099-3-sandals@crustytoothpaste.net
State New
Headers show
Series
  • Hash function transition part 15
Related show

Commit Message

brian m. carlson Oct. 8, 2018, 9:56 p.m. UTC
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 builtin/repack.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Stefan Beller Oct. 8, 2018, 10:27 p.m. UTC | #1
On Mon, Oct 8, 2018 at 2:57 PM brian m. carlson
<sandals@crustytoothpaste.net> wrote:
>
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
>  builtin/repack.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/repack.c b/builtin/repack.c
> index c6a7943d5c..e77859062d 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -407,8 +407,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>
>         out = xfdopen(cmd.out, "r");
>         while (strbuf_getline_lf(&line, out) != EOF) {
> -               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);
Eric Sunshine Oct. 8, 2018, 11:01 p.m. UTC | #2
On Mon, Oct 8, 2018 at 6:27 PM Stefan Beller <sbeller@google.com> wrote:
> 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.
brian m. carlson Oct. 9, 2018, 11 p.m. UTC | #3
On Mon, Oct 08, 2018 at 07:01:27PM -0400, Eric Sunshine wrote:
> On Mon, Oct 8, 2018 at 6:27 PM Stefan Beller <sbeller@google.com> wrote:
> > 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.

Yeah, I don't think a special case is needed here.  Moreover, since we
just invoked pack-objects ourselves and are now reading the output of
it, seeing this error message means that someone has broken Git in a
significant way.  The end user should never see this message.

Patch
diff mbox series

diff --git a/builtin/repack.c b/builtin/repack.c
index c6a7943d5c..e77859062d 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -407,8 +407,8 @@  int cmd_repack(int argc, const char **argv, const char *prefix)
 
 	out = xfdopen(cmd.out, "r");
 	while (strbuf_getline_lf(&line, out) != EOF) {
-		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.");
 		string_list_append(&names, line.buf);
 	}
 	fclose(out);