Re: [PATCH 02/14] builtin/repack: replace hard-coded constant
On Mon, Oct 08, 2018 at 07:01:27PM -0400, Eric Sunshine wrote: > On Mon, Oct 8, 2018 at 6:27 PM Stefan Beller wrote: > > On Mon, Oct 8, 2018 at 2:57 PM brian m. carlson > > 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. -- brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204 signature.asc Description: PGP signature
Re: [PATCH 02/14] builtin/repack: replace hard-coded constant
On Mon, Oct 8, 2018 at 6:27 PM Stefan Beller wrote: > On Mon, Oct 8, 2018 at 2:57 PM brian m. carlson > 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.
Re: [PATCH 02/14] builtin/repack: replace hard-coded constant
On Mon, Oct 8, 2018 at 2:57 PM brian m. carlson wrote: > > Signed-off-by: brian m. carlson > --- > 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);