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

[Yann Dirson <ydirson@free.fr>]

On Thu, Oct 28, 2010 at 08:45:40PM -0500, Jonathan Nieder wrote:
> Yann Dirson wrote:
> 
> > Possible optimisations to this code include:
> > * avoid use of i_am_not_single by using a separate list
> 
> I think that would help code clarity, too. It is tempting to try to
> split this patch into micropatches:

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

A number of the steps you suggest (notably 8 and 9) would cause the
code to be plain wrong at some points, since those are part of what
makes the algorithm (appear to be) correct.  This would not be a
problem only because that code would not be reachable throught any
means, right ?  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".

I'm much more tempted to split into fully-functionnal patches that do
adds reachable code paths (eg. bulk removal - although it's much more
than just a split of the patch).

> 1. introduce DETECT_DIRECTORY_RENAMES flag and hidden UI for it.

What do you mean by "hidden UI" ?

[...]
> 8. disqualify directories with stragglers left behind.
> 
> 9. disqualify directories for which the contents are not unanimous
>    about where to go.

[...]

> Trivial comments on the patch:
> 
> [...]
> > +++ b/diffcore-rename.c
> > @@ -6,14 +6,34 @@
> >  #include "diffcore.h"
> >  #include "hash.h"
> >  
> > +#define DEBUG_BULKMOVE 0
> > +
> > +#if DEBUG_BULKMOVE
> > +#define debug_bulkmove(args) __debug_bulkmove args
> > +void __debug_bulkmove(const char *fmt, ...)
> > +{
> > +	va_list ap;
> > +	va_start(ap, fmt);
> > +	fprintf(stderr, "[DBG] ");
> > +	vfprintf(stderr, fmt, ap);
> > +	va_end(ap);
> > +}
> > +#else
> > +#define debug_bulkmove(args) do { /*nothing */ } while (0)
> > +#endif
> 
> 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.

> [...]
> > + * Supports in-place modification of src by passing dst == src.
> > + */
> > +static const char *copy_dirname(char *dst, const char *src)
> [...]
> > +	end = mempcpy(dst, src, slash - src + 1);
> 
> I suppose this should read:
> 
> 	if (dst != src)
> 		memcpy(dst, src, slash - src + 1);
> 	dst[slash - src + 1] = '\0';
> 	return dst;

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;

> Style: '{' for functions goes in column 0.

> Can get some depth reduction by dropping the else here (since in
> the trivial case we have already returned).

Right, thanks.

> > +static void diffcore_bulk_moves(void)
> > +{
> > +	int i;
> > +	for (i = 0; i < rename_dst_nr; i++)
> > +		check_one_bulk_move(rename_dst[i].pair);
> > +}
> 
> Yay. :)

Isn't that nice and pretty :)