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

2018-10-09 Thread brian m. carlson
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

2018-10-08 Thread Eric Sunshine
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

2018-10-08 Thread Stefan Beller
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(, 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);


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

2018-10-08 Thread brian m. carlson
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(, 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(, line.buf);
}
fclose(out);