Re: [PATCH 1/3] send-pack: track errors for each ref

[Jeff King <peff@peff.net>]

On Sat, Nov 17, 2007 at 01:05:35PM -0500, Daniel Barkalow wrote:

> > +static void print_ref_status(char flag, const char *summary, struct ref *to, struct ref *from, const char *msg)
> 
> Isn't "from" always "to->peer_ref"? It'd be nice to make this function 
> unable to print something different from what we actually did. (Actually 
> it might be "to->deletion ? NULL : to->peer_ref", but that would also be 
> better to have as an explicit feature of how you display "to", rather than 
> implicit in the set of callers.

Yes, I also considered changing "from" to "show peer" which might have
been nicer. I am not opposed to such a cleanup, but again, not sure if
it worth it now that we are merged.

> > +static const char *status_abbrev(unsigned char sha1[20])
> > +{
> > +	const char *abbrev;
> > +	abbrev = find_unique_abbrev(sha1, DEFAULT_ABBREV);
> > +	return abbrev ? abbrev : sha1_to_hex(sha1);
> > +}
> 
> Maybe we should have a find_unique_abbrev()-like function that doesn't 
> mind if the requested object doesn't exist?

I agree, though I don't feel qualified to comment on what other places
that should be used (I was a bit surprised to find out that
find_unique_abbrev ever returned NULL, but changing the semantics at
this point is probably going to cause some subtle bug).

> > +		char quickref[83];
> Shouldn't this be 40 + 3 + 40 + 1?

Oops, yes. I think it should be hard to trigger (both commits would have
to either not be in your db, or not be unique to 40 digits). But clearly
it should be fixed, and it looks like Junio did.

It was a stupid cut-and-paste from Nicolas' fetch code, but it looks
like he correctly allocates 84 bytes for the "..." case.

> > +		char type;
> > +		const char *msg;
> > +
> > +		strcpy(quickref, status_abbrev(ref->old_sha1));
> > +		if (ref->nonfastforward) {
> > +			strcat(quickref, "...");
> > +			type = '+';
> > +			msg = " (forced update)";
> > +		}
> > +		else {
> 
> Coding style, IIRC.

Sorry, I don't see the style nit you're mentioning here.

-Peff