Re: [PATCH] quote_path: fix collapsing of relative paths

From: Jeff King <peff@peff.net>
Date: Dec 3, 2007, 06:05 UTC
Message-ID: <20071203060557.GA13011@coredump.intra.peff.net>
In-Reply-To: <7vabosqq1l.fsf@gitster.siamese.dyndns.org>

On Sun, Dec 02, 2007 at 09:56:54PM -0800, Junio C Hamano wrote:

> Somehow I feel it would be simpler and less error prone if you atually
> count to set len to the length if you got negative.
> [...]
> So perhaps this is easier to read?

Yes, I actually wrote the exact same patch but discarded it in favor of what I sent, because I wanted a minimal change (and I assumed it had been implemented that way to avoid the extra strlen call). But that is just premature micro-optimization, and I think the version you posted is much less subtle.

> The part that follows the patch, there is a line that subtracts a small
> number (off+1) from len --- while it won't have a wraparound issue, it
> feels a bit ugly to rely on the "magic -1" value to stay "magic
> negative" if small positive integers are subtracted from it.

I hadn't considered that, but another good reason to just set len in the first place.

> Also the reason the updated condition to the while loop does not have to
> check NUL termination upon negative len is because both (prefix[off] !=
> NUL) and (prefix[off] == in[off]) are checked there, which some may find
> subtle.

Yes, I found it subtle, too, which is why I documented it in the commit message. :)

Show 8 quoted lines

> -	if (len > 0)
> -		strbuf_grow(out, len);
> -	strbuf_setlen(out, 0);
> +	if (len < 0)
> +		len = strlen(in);
>  
> +	strbuf_grow(out, len);
> +	strbuf_setlen(out, 0);

I wonder if this 'grow' is really all that useful, since we are now going to overallocate in some cases. Perhaps we should just let strbuf do its job and grow as necessary. But it likely doesn't matter either way.

-Peff