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:
[...]
> +++ 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).
> +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. :)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html