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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Oct 29, 2010, 01:45 UTC
Message-ID: <20101029014540.GB28984@burratino>
In-Reply-To: <1288303712-14662-2-git-send-email-ydirson@altern.org>

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:

1. introduce DETECT_DIRECTORY_RENAMES flag and hidden UI for it.

2. if detecting bulk relocations, do not simplify input for
   diffcore by omitting unchanged files (just like when
   finding copies harder).

3. introduce "rename target candidate" flag in rename_dsts.
    - if detecting copies, all diff targets are potential rename
      targets

    - if detecting bulk relocations but not copies, all diff
      targets are rename_dsts, but only file creation diff
      targets are potential rename targets

    - if detecting neither bulk reloc nor copies, only file
      creation diff targets are rename_dsts (and all are
      potential rename targets).

   Alternatively, rename_dsts that are not potential rename targets
   could be put in a different list (which is simpler and probably
   faster, while using a little more memory).

4. honor rename_target_candidate flag (not needed if the
   non-candidates get their own list).

5. introduce a list of potential directory relocations and functions
   to manipulate it.

6. pass the (empty) list of relocated directories back to diffcore.

7. populate the list of directory relocation candidates.  If a file
   has been renamed to go from directory a/ to directory b/, we have a
   directory rename candidate.

8. disqualify directories with stragglers left behind.

9. disqualify directories for which the contents are not unanimous
   about where to go.

10. add documentation and stop hiding the UI.

Trivial comments on the patch:

[...]

Show 20 quoted lines

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

[...]

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

[...]

> +static int discard_if_outside(struct diff_bulk_rename *candidate,
> +			 struct diff_bulk_rename *seen) {

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

> +	if (!prefixcmp(candidate->two->path, seen->two->path)) {
> +		debug_bulkmove((" 'dstpair' conforts 'seen'\n"));
> +		return 0;
> +	} else {

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

Show 6 quoted lines

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