Re: [PATCH v8 1/5] Introduce bulk-move detection in diffcore.

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Oct 30, 2010, 00:26 UTC
Message-ID: <20101030002600.GC21410@burratino>
In-Reply-To: <20101029211852.GB5695@home.lan>

Yann Dirson wrote:

> Hm, I fear too much granularity would become meaningless :)

[...]

>                 If the code needs to be easier to understand, I'd
> rather add some more doc, than added commits that are basically
> "useless for bisect".

Yes, the example list was probably a little overboard. Even so, the idea was to maintain bisectability by introducing a few tests at a time. That way, the fallout of a subfeature can easily be found with "git bisect", among other benefits[1].

I might try it out some time soon; don't worry about it if you don't like the idea. :)

> On Thu, Oct 28, 2010 at 08:45:40PM -0500, Jonathan Nieder wrote:

>> 1. introduce DETECT_DIRECTORY_RENAMES flag and hidden UI for it.
>
> What do you mean by "hidden UI" ?

PARSE_OPT_HIDDEN (i.e., a commandline option not tempting readers by listing in -h).

Show 5 quoted lines

>> Is the debugging output infrequent enough to just use a function
>> unconditionally?
>
> You mean, keep funccalls even with DEBUG_BULKMOVE is not set ?  No,
> there are too many traces for that.

Yep, that's what I meant.  Alas.

Will the debugging output still be useful once the code is known to work reliably? If not, I guess we can remove it at that point (avoiding the need to worry about patches that may introduce

	debug_bulkmove(expression that does not even
		       typecheck);

down the line).

Show 11 quoted lines

> Ah, sure the dst==src case can be improved.  But I'm not sure
> factorizing writing NUL is worth the cost of re-computing where to put
> it when using mempcpy would avoid.  Wouldn't the following be more
> adequate ?
> 
> 	if (dst != src) {
> 		end = mempcpy(dst, src, slash - src + 1);
> 		*end = '\0';
> 	} else
> 		dst[slash - src + 1] = '\0';
> 	return dst;

I doubt the difference is measurable. But that looks fine (micronit: I suppose one should put the dst == src case first in that case).

Jonathan

[1] The example that set me on this path to madness: http://thread.gmane.org/gmane.comp.version-control.git/151086/focus=158913