| Message ID | 20181008215701.779099-3-sandals@crustytoothpaste.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Hash function transition part 15 | expand |
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);
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.
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.
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);
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> --- builtin/repack.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)