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

From: Jeff King <peff@peff.net>
Date: Nov 18, 2007, 00:13 UTC
Message-ID: <20071118001312.GB4000@sigill.intra.peff.net>
In-Reply-To: <Pine.LNX.4.64.0711171217200.12193@iabervon.org>

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

Show 7 quoted lines

> > +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.

Show 9 quoted lines

> > +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.

Show 12 quoted lines

> > +		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