{"thread":{"id":"25536","subject":"[PATCH v7 0/3] Detection of directory renames","startedAt":"2010-10-23T21:06:59Z","lastAt":"2010-10-28T21:42:40Z","messageCount":11,"participants":["Yann Dirson","Junio C Hamano"],"isPatch":true,"patchVersion":7,"patchTotal":3},"messages":[{"id":"154282","messageId":"1287868022-24872-1-git-send-email-ydirson@altern.org","threadId":"25536","inReplyTo":null,"subject":"[PATCH v7 0/3] Detection of directory renames","fromName":"Yann Dirson","fromEmail":"ydirson@altern.org","sentAt":"2010-10-23T21:06:59Z","receivedAt":"2010-10-23T21:06:59Z","isPatch":true,"sender":{"key":"ydirson@altern.org","avatar":"https://avatars.githubusercontent.com/u/1190950?v=4"},"body":"Changes since v6.1:\n* now using a sorted list for bulkmove_candidates, improved lookup code\n* added a bit of doc\n* fixed decl-after-stmt and code style issues (Junio)\n* fixed signedness of i_am_not_single bitfield (sparse)\n* removed useless filtering of debug messages in tests\n* avoid using \"git update-index\" where \"git mv\" and friends are more readable\n* added a testcase for move of a subdir, fixed the code\n* more naming normalisation\n* added short note in commit message about implementation\n* added Thanks-to's to commit messages (hope I did not forget anyone)\n* added more possible future uses to commit message\n* moved list of optimisation opportunities from FIXME's to commit message,\n  leaving only a handful of things as real FIXME tags (purposely kept as C99\n  comments)\n\n\nYann Dirson (3):\n  Introduce bulk-move detection in diffcore.\n  Add testcases for the --detect-bulk-moves diffcore flag.\n  [WIP] Allow hiding renames of individual files involved in a\n    directory rename.\n\n Documentation/diff-options.txt   |    4 +\n Documentation/gitdiffcore.txt    |   12 +\n diff-lib.c                       |    6 +-\n diff.c                           |   21 ++-\n diff.h                           |    6 +\n diffcore-rename.c                |  418 ++++++++++++++++++++++++++++++++++++-\n diffcore.h                       |    2 +\n t/t4046-diff-rename-factorize.sh |  296 +++++++++++++++++++++++++++\n tree-diff.c                      |    4 +-\n 9 files changed, 753 insertions(+), 16 deletions(-)\n create mode 100755 t/t4046-diff-rename-factorize.sh\n\n-- \n1.7.2.3\n"},{"id":"154283","messageId":"1287868022-24872-2-git-send-email-ydirson@altern.org","threadId":"25536","inReplyTo":"1287868022-24872-1-git-send-email-ydirson@altern.org","subject":"[PATCH v7 1/3] Introduce bulk-move detection in diffcore.","fromName":"Yann Dirson","fromEmail":"ydirson@altern.org","sentAt":"2010-10-23T21:07:00Z","receivedAt":"2010-10-23T21:07:00Z","isPatch":true,"sender":{"key":"ydirson@altern.org","avatar":"https://avatars.githubusercontent.com/u/1190950?v=4"},"body":"This feature tries to group together files moving from and to\nidentical directories - the most common case being directory renames.\n\nIt is activated by the new --detect-bulk-moves diffcore\nflag.\n\nThis is only the first step, adding the basic functionnality and\nadding support to raw diff output (and it is not supported in\nunified-diff output yet)\n\nIt is implemented as a new pass in diffcore-rename, occuring after the\nfile renames get detected, grouping those renames looking like a move\nof a full directory into some other place.\n\nThe output of raw diff is displayed as \"Rnnn a/* b/\".  Those cannot be\nconfused with renames of files named \"whatever/*\" with a literal star\ncharacter, from the full-zero SHA1's.\n\nPossible optimisations to this code include:\n* avoid use of i_am_not_single by using a separate list\n* use a more informative prefixcmp to avoid strcmp calls\n  eg. in discard_if_outside()\n* optimize for bulk insertions (avoid useless successive memmove's)\n\nOther future developements to be made on top of this include:\n* extension of unified-diff format to express this\n* detect inexact bulk-moves (where some files were not moved, or were\n  moved to a different place) - problem of computing a similarity score\n* display as such the special case of directory move/rename\n* application of such new diffs: issue a conflict, or just a warning ?\n* teach git-svn (and others ?) to make use of that flag\n* handle new conflict type \"bulk-move/add\"\n* detect \"directory splits\" as well\n* use inexact dir renames to bump score of below-threshold renames\n  from/to same locations\n* support other types of bluk-grouping, like prefixes (see eg. kernel\n  5d1e859c), and maybe config-specified patterns\n* add yours here\n\nThis patch has been improved by the following contributions:\n- Jonathan Nieder: better implementation of copy_dirname()\n- Jonathan Nieder: portable implementation of memrchr() in another patch\n- Junio C Hamano: split individual renames hiding under control of another flag\n- Junio C Hamano: coding style issues\n- Ævar Arnfjörð Bjarmason: Don't use C99 comments.\n- Jonathan Nieder: just too many other helpful suggestions to list them all\n\nThanks-to: Jonathan Nieder <jrnieder@gmail.com>\nThanks-to: Junio C Hamano <gitster@pobox.com>\nThanks-to: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\nSigned-off-by: Yann Dirson <ydirson@free.fr>\n---\n Documentation/diff-options.txt |    4 +\n Documentation/gitdiffcore.txt  |   12 ++\n diff-lib.c                     |    6 +-\n diff.c                         |   14 ++-\n diff.h                         |    3 +\n diffcore-rename.c              |  354 ++++++++++++++++++++++++++++++++++++++--\n diffcore.h                     |    1 +\n tree-diff.c                    |    4 +-\n 8 files changed, 383 insertions(+), 15 deletions(-)\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex e954af0..2d99511 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -243,6 +243,10 @@ endif::git-log[]\n \tdelete/add pair to be a rename if more than 90% of the file\n \thasn't changed.\n \n+--detect-bulk-moves::\n+\tDetect bulk move of all files of a directory into a\n+\tdifferent one.\n+\n -C[<n>]::\n \tDetect copies as well as renames.  See also `--find-copies-harder`.\n \tIf `n` is specified, it has the same meaning as for `-M<n>`.\ndiff --git a/Documentation/gitdiffcore.txt b/Documentation/gitdiffcore.txt\nindex 6af29a4..edeb159 100644\n--- a/Documentation/gitdiffcore.txt\n+++ b/Documentation/gitdiffcore.txt\n@@ -175,6 +175,18 @@ the expense of making it slower.  Without `\\--find-copies-harder`,\n 'git diff-{asterisk}' commands can detect copies only if the file that was\n copied happened to have been modified in the same changeset.\n \n+Bulk move of all files of a directory into a different one can get\n+detected using the `\\--detect-bulk-moves` option.  This adds an\n+additional pass on top of the results of per-file rename detection.\n+They are reported with NULL SHA1 id, in addition to the file renames:\n+\n+------------------------------------------------\n+:040000 040000 0000000... 0000000... R100\ta/*\tb/\n+:100644 100644 0123456... 1234567... R90\ta/file0\tb/file3\n+:100644 100644 0123456... 1234567... R100\ta/file1\tb/file1\n+:100644 100644 0123456... 1234567... R100\ta/file2\tb/file2\n+------------------------------------------------\n+\n \n diffcore-merge-broken: For Putting \"Complete Rewrites\" Back Together\n --------------------------------------------------------------------\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 392ce2b..5ec3ddc 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -208,7 +208,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t\t\t\t\t    ce_option, &dirty_submodule);\n \t\tif (!changed && !dirty_submodule) {\n \t\t\tce_mark_uptodate(ce);\n-\t\t\tif (!DIFF_OPT_TST(&revs->diffopt, FIND_COPIES_HARDER))\n+\t\t\tif (!DIFF_OPT_TST(&revs->diffopt, FIND_COPIES_HARDER) &&\n+\t\t\t    !DIFF_OPT_TST(&revs->diffopt, DETECT_BULK_MOVES))\n \t\t\t\tcontinue;\n \t\t}\n \t\toldmode = ce->ce_mode;\n@@ -338,7 +339,8 @@ static int show_modified(struct rev_info *revs,\n \n \toldmode = old->ce_mode;\n \tif (mode == oldmode && !hashcmp(sha1, old->sha1) && !dirty_submodule &&\n-\t    !DIFF_OPT_TST(&revs->diffopt, FIND_COPIES_HARDER))\n+\t    !DIFF_OPT_TST(&revs->diffopt, FIND_COPIES_HARDER) &&\n+\t    !DIFF_OPT_TST(&revs->diffopt, DETECT_BULK_MOVES))\n \t\treturn 0;\n \n \tdiff_change(&revs->diffopt, oldmode, mode,\ndiff --git a/diff.c b/diff.c\nindex 71efa8e..1d88281 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3188,6 +3188,11 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)\n \t\tDIFF_OPT_SET(options, REVERSE_DIFF);\n \telse if (!strcmp(arg, \"--find-copies-harder\"))\n \t\tDIFF_OPT_SET(options, FIND_COPIES_HARDER);\n+\telse if (!strcmp(arg, \"--detect-bulk-moves\")) {\n+\t\tDIFF_OPT_SET(options, DETECT_BULK_MOVES);\n+\t\tif (!options->detect_rename)\n+\t\t\toptions->detect_rename = DIFF_DETECT_RENAME;\n+\t}\n \telse if (!strcmp(arg, \"--follow\"))\n \t\tDIFF_OPT_SET(options, FOLLOW_RENAMES);\n \telse if (!strcmp(arg, \"--color\"))\n@@ -3466,7 +3471,14 @@ static void diff_flush_raw(struct diff_filepair *p, struct diff_options *opt)\n \tif (p->status == DIFF_STATUS_COPIED ||\n \t    p->status == DIFF_STATUS_RENAMED) {\n \t\tconst char *name_a, *name_b;\n-\t\tname_a = p->one->path;\n+\t\tif (p->is_bulkmove) {\n+\t\t\t/* append \"*\" to the first dirname */\n+\t\t\tchar buf[PATH_MAX];\n+\t\t\tchar *next = memccpy(buf, p->one->path, '\\0', PATH_MAX);\n+\t\t\tnext[-1] = '*'; *next = '\\0';\n+\t\t\tname_a = buf;\n+\t\t} else\n+\t\t\tname_a = p->one->path;\n \t\tname_b = p->two->path;\n \t\tstrip_prefix(opt->prefix_length, &name_a, &name_b);\n \t\twrite_name_quoted(name_a, opt->file, inter_name_termination);\ndiff --git a/diff.h b/diff.h\nindex 1fd44f5..b0d6fa6 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -78,6 +78,7 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct diff_options *opt, void *data)\n #define DIFF_OPT_IGNORE_UNTRACKED_IN_SUBMODULES (1 << 25)\n #define DIFF_OPT_IGNORE_DIRTY_SUBMODULES (1 << 26)\n #define DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG (1 << 27)\n+#define DIFF_OPT_DETECT_BULK_MOVES  (1 << 28)\n \n #define DIFF_OPT_TST(opts, flag)    ((opts)->flags & DIFF_OPT_##flag)\n #define DIFF_OPT_SET(opts, flag)    ((opts)->flags |= DIFF_OPT_##flag)\n@@ -265,6 +266,8 @@ extern void diffcore_fix_diff_index(struct diff_options *);\n \"  -C            detect copies.\\n\" \\\n \"  --find-copies-harder\\n\" \\\n \"                try unchanged files as candidate for copy detection.\\n\" \\\n+\"  --detect-bulk-moves\\n\" \\\n+\"                detect wholesale directory renames.\\n\" \\\n \"  -l<n>         limit rename attempts up to <n> paths.\\n\" \\\n \"  -O<file>      reorder diffs according to the <file>.\\n\" \\\n \"  -S<string>    find filepair whose only one side contains the string.\\n\" \\\ndiff --git a/diffcore-rename.c b/diffcore-rename.c\nindex df41be5..fa2ba7c 100644\n--- a/diffcore-rename.c\n+++ b/diffcore-rename.c\n@@ -11,9 +11,13 @@\n static struct diff_rename_dst {\n \tstruct diff_filespec *two;\n \tstruct diff_filepair *pair;\n+\tunsigned i_am_not_single:1; /* does not look for a match, only here to be looked at */\n } *rename_dst;\n static int rename_dst_nr, rename_dst_alloc;\n \n+/*\n+ * Do a binary search of \"two\" in \"rename_dst\", inserting it if not found.\n+ */\n static struct diff_rename_dst *locate_rename_dst(struct diff_filespec *two,\n \t\t\t\t\t\t int insert_ok)\n {\n@@ -49,9 +53,36 @@ static struct diff_rename_dst *locate_rename_dst(struct diff_filespec *two,\n \trename_dst[first].two = alloc_filespec(two->path);\n \tfill_filespec(rename_dst[first].two, two->sha1, two->mode);\n \trename_dst[first].pair = NULL;\n+\trename_dst[first].i_am_not_single = 0;\n \treturn &(rename_dst[first]);\n }\n \n+/*\n+ * Do a binary search in \"rename_dst\" of any entry under \"dirname\".\n+ */\n+static struct diff_rename_dst *locate_rename_dst_dir(const char *dirname)\n+{\n+\tint first, last;\n+\tint prefixlength = strlen(dirname);\n+\n+\tfirst = 0;\n+\tlast = rename_dst_nr;\n+\twhile (last > first) {\n+\t\tint next = (last + first) >> 1;\n+\t\tstruct diff_rename_dst *dst = &(rename_dst[next]);\n+\t\tint cmp = strncmp(dirname, dst->two->path, prefixlength);\n+\t\tif (!cmp)\n+\t\t\treturn dst;\n+\t\tif (cmp < 0) {\n+\t\t\tlast = next;\n+\t\t\tcontinue;\n+\t\t}\n+\t\tfirst = next+1;\n+\t}\n+\t/* not found */\n+\treturn NULL;\n+}\n+\n /* Table of rename/copy src files */\n static struct diff_rename_src {\n \tstruct diff_filespec *one;\n@@ -386,8 +417,11 @@ static int find_exact_renames(void)\n \tfor (i = 0; i < rename_src_nr; i++)\n \t\tinsert_file_table(&file_table, -1, i, rename_src[i].one);\n \n-\tfor (i = 0; i < rename_dst_nr; i++)\n+\tfor (i = 0; i < rename_dst_nr; i++) {\n+\t\tif (rename_dst[i].i_am_not_single)\n+\t\t\tcontinue;\n \t\tinsert_file_table(&file_table, 1, i, rename_dst[i].two);\n+\t}\n \n \t/* Find the renames */\n \ti = for_each_hash(&file_table, find_same_files);\n@@ -414,6 +448,270 @@ static void record_if_better(struct diff_score m[], struct diff_score *o)\n \t\tm[worst] = *o;\n }\n \n+static struct diff_bulk_rename {\n+\tstruct diff_filespec *one;\n+\tstruct diff_filespec *two;\n+\tint discarded;\n+} *bulkmove_candidates;\n+static int bulkmove_candidates_nr, bulkmove_candidates_alloc;\n+\n+/*\n+ * Do a binary search of \"one\" in \"bulkmove_candidate\", inserting it if not\n+ * found.\n+ * A good part was copy-pasted from locate_rename_dst().\n+ */\n+static struct diff_bulk_rename *locate_bulkmove_candidate(const char *one_path,\n+\t\t\t\t\t\t\t  const char *two_path)\n+{\n+\tint first, last;\n+\n+\tfirst = 0;\n+\tlast = bulkmove_candidates_nr;\n+\twhile (last > first) {\n+\t\tint next = (last + first) >> 1;\n+\t\tstruct diff_bulk_rename *candidate = &(bulkmove_candidates[next]);\n+\t\t/* primary sort key on one_path, secondary on two_path */\n+\t\tint cmp = strcmp(one_path, candidate->one->path);\n+\t\tif (!cmp)\n+\t\t\tcmp = strcmp(two_path, candidate->two->path);\n+\t\tif (!cmp)\n+\t\t\treturn candidate;\n+\t\tif (cmp < 0) {\n+\t\t\tlast = next;\n+\t\t\tcontinue;\n+\t\t}\n+\t\tfirst = next+1;\n+\t}\n+\t/* not found */\n+\t/* insert to make it at \"first\" */\n+\tif (bulkmove_candidates_alloc <= bulkmove_candidates_nr) {\n+\t\tbulkmove_candidates_alloc = alloc_nr(bulkmove_candidates_alloc);\n+\t\tbulkmove_candidates = xrealloc(bulkmove_candidates,\n+\t\t\t\t      bulkmove_candidates_alloc * sizeof(*bulkmove_candidates));\n+\t}\n+\tbulkmove_candidates_nr++;\n+\tif (first < bulkmove_candidates_nr)\n+\t\tmemmove(bulkmove_candidates + first + 1, bulkmove_candidates + first,\n+\t\t\t(bulkmove_candidates_nr - first - 1) * sizeof(*bulkmove_candidates));\n+\n+\tbulkmove_candidates[first].one = alloc_filespec(one_path);\n+\tfill_filespec(bulkmove_candidates[first].one, null_sha1, S_IFDIR);\n+\tbulkmove_candidates[first].two = alloc_filespec(two_path);\n+\tfill_filespec(bulkmove_candidates[first].two, null_sha1, S_IFDIR);\n+\tbulkmove_candidates[first].discarded = 0;\n+\treturn &(bulkmove_candidates[first]);\n+}\n+\n+/*\n+ * Copy dirname of src into dst, suitable to append a filename without\n+ * an additional \"/\".\n+ * Only handles relative paths since there is no absolute path in a git repo.\n+ * Writes \"\" when there is no \"/\" in src.\n+ * May overwrite more chars than really needed, if src ends with a \"/\".\n+ * Supports in-place modification of src by passing dst == src.\n+ */\n+static const char *copy_dirname(char *dst, const char *src)\n+{\n+\tsize_t len = strlen(src);\n+\tconst char *slash;\n+\tchar *end;\n+\n+\tif (len > 0 && src[len - 1] == '/')\n+\t\t/* Trailing slash.  Ignore it. */\n+\t\tlen--;\n+\n+\tslash = memrchr(src, '/', len);\n+\tif (!slash) {\n+\t\t*dst = '\\0';\n+\t\treturn dst;\n+\t}\n+\n+\tend = mempcpy(dst, src, slash - src + 1);\n+\t*end = '\\0';\n+\treturn dst;\n+}\n+\n+// FIXME: leaks like hell.\n+/* See if the fact that one_leftover exists under one_parent_path in\n+ * dst tree should disqualify one_parent_path from bulkmove eligibility.\n+ * Return 1 if it disqualifies, 0 if it is OK.\n+ */\n+static int dispatched_to_different_dirs(const char *one_parent_path)\n+{\n+\t/* this might be a dir split, or files added\n+\t * after the bulk move, or just an isolated\n+\t * rename */\n+\tint two_idx, j, onep_len, maybe_dir_rename;\n+\tstruct diff_rename_dst *one_leftover =\n+\t\tone_leftover = locate_rename_dst_dir(one_parent_path);\n+\n+\tif (!one_leftover)\n+\t\treturn 0;\n+\n+\t/* try to see if it is a file added after the bulk move */\n+\ttwo_idx = one_leftover - rename_dst;\n+\tonep_len = strlen(one_parent_path);\n+\tmaybe_dir_rename = 1;\n+\n+\t/* check no leftover file was already here before */\n+\tfor (j = two_idx; j < rename_dst_nr; j++) {\n+\t\tif (strncmp(rename_dst[j].two->path,\n+\t\t\t    one_parent_path, onep_len))\n+\t\t\tbreak; /* exhausted directory in this direction */\n+\t\tfprintf(stderr, \"[DBG] leftover file %s in %s\\n\",\n+\t\t\trename_dst[j].two->path, one_parent_path);\n+\t\tif (rename_dst[j].i_am_not_single || /* those were already here */\n+\t\t    (rename_dst[j].pair &&\n+\t\t     !strncmp(rename_dst[j].pair->one->path,\n+\t\t\t      one_parent_path, onep_len) && /* renamed from here */\n+\t\t     strncmp(rename_dst[j].two->path,\n+\t\t\t     one_parent_path, onep_len))) { /* not to a subdir */\n+\t\t\tmaybe_dir_rename = 0;\n+\t\t\tfprintf(stderr, \"[DBG] ... tells not a bulk move\\n\");\n+\t\t\tbreak;\n+\t\t}\n+\t\tfprintf(stderr, \"[DBG] ... not believed to prevent bulk move\\n\");\n+\t}\n+\tif (!maybe_dir_rename)\n+\t\treturn 1;\n+\t/* try the other direction (dup code) */\n+\tfor (j = two_idx-1; j >= 0; j--) {\n+\t\tif (strncmp(rename_dst[j].two->path,\n+\t\t\t    one_parent_path, onep_len))\n+\t\t\tbreak; /* exhausted directory in this direction */\n+\t\tfprintf(stderr, \"[DBG] leftover file %s in '%s'\\n\",\n+\t\t\trename_dst[j].two->path, one_parent_path);\n+\t\tif (rename_dst[j].i_am_not_single || /* those were already here */\n+\t\t    (rename_dst[j].pair &&\n+\t\t     !strncmp(rename_dst[j].pair->one->path,\n+\t\t\t      one_parent_path, onep_len) && /* renamed from here */\n+\t\t     strncmp(rename_dst[j].two->path,\n+\t\t\t     one_parent_path, onep_len))) { /* not to a subdir */\n+\t\t\tmaybe_dir_rename = 0;\n+\t\t\tfprintf(stderr, \"[DBG] ... tells not a bulk move\\n\");\n+\t\t\tbreak;\n+\t\t}\n+\t\tfprintf(stderr, \"[DBG] ... not believed to prevent bulk move\\n\");\n+\t}\n+\tif (!maybe_dir_rename)\n+\t\treturn 1;\n+\n+\t/* Here we are in the case where a directory\n+\t * content was completely moved, but files\n+\t * were added to it afterwards.  Proceed as\n+\t * for a simple bulk move. */\n+\treturn 0;\n+}\n+\n+/*\n+ * Assumes candidate->one is a subdir of seen->one, mark 'seen' as\n+ * discarded if candidate->two is outside seen->two.  Also mark\n+ * 'candidate' itself as discarded if the conflict implies so.\n+ *\n+ * Return 1 if 'seen' was discarded\n+ */\n+static int discard_if_outside(struct diff_bulk_rename *candidate,\n+\t\t\t struct diff_bulk_rename *seen) {\n+\tif (!prefixcmp(candidate->two->path, seen->two->path)) {\n+\t\tfprintf(stderr, \"[DBG]  'dstpair' conforts 'seen'\\n\");\n+\t\treturn 0;\n+\t} else {\n+\t\tfprintf(stderr, \"[DBG] discarding %s -> %s from bulk moves (split into %s and %s)\\n\",\n+\t\t\tseen->one->path, seen->two->path,\n+\t\t\tcandidate->two->path, seen->two->path);\n+\t\tseen->discarded = 1;\n+\t\t/* Need to discard dstpair as well, unless moving from\n+\t\t * a strict subdir of seen->one or to a strict subdir\n+\t\t * of seen->two */\n+\t\tif (!strcmp(seen->one->path, candidate->one->path) &&\n+\t\t    prefixcmp(seen->two->path, candidate->two->path)) {\n+\t\t\tfprintf(stderr, \"[DBG] ... and not adding self\\n\");\n+\t\t\tcandidate->discarded = 1;\n+\t\t}\n+\t\treturn 1;\n+\t}\n+}\n+\n+/*\n+ * Check if the rename specified by \"dstpair\" could cause a\n+ * bulk move to be detected, record it in bulkmove_candidates if yes.\n+ */\n+static void check_one_bulk_move(struct diff_filepair *dstpair)\n+{\n+\tchar one_parent_path[PATH_MAX], two_parent_path[PATH_MAX];\n+\n+\t/* genuine new files (or believed to be so) */\n+\tif (!dstpair)\n+\t\treturn;\n+\t/* dummy renames used by copy detection */\n+\tif (!strcmp(dstpair->one->path, dstpair->two->path))\n+\t\treturn;\n+\n+\tcopy_dirname(one_parent_path, dstpair->one->path);\n+\tcopy_dirname(two_parent_path, dstpair->two->path);\n+\n+\t/* simple rename with no directory change */\n+\tif (!strcmp(one_parent_path, two_parent_path))\n+\t\treturn;\n+\n+\tfprintf(stderr, \"[] %s -> %s ?\\n\", dstpair->one->path, dstpair->two->path);\n+\n+\t/* loop up one_parent_path over successive parents */\n+\t// FIXME: also loop over two_parent_path prefixes\n+\tdo {\n+\t\tstruct diff_bulk_rename *seen;\n+\t\tint old_nr = bulkmove_candidates_nr;\n+\t\tstruct diff_bulk_rename *candidate =\n+\t\t\tlocate_bulkmove_candidate(one_parent_path, two_parent_path);\n+\t\tfprintf(stderr, \"[[]] %s ...\\n\", one_parent_path);\n+\t\tif (old_nr == bulkmove_candidates_nr) {\n+\t\t\tfprintf(stderr, \"[DBG]  already seen\\n\");\n+\t\t\treturn;\n+\t\t}\n+\n+\t\t/* After this commit, are there any files still under one_parent_path ?\n+\t\t * Any file left would disqualifies this dir for bulk move.\n+\t\t */\n+\t\tif (dispatched_to_different_dirs(one_parent_path)) {\n+\t\t\t// FIXME: check overlap with discard_if_outside()\n+\t\t\tcandidate->discarded = 1;\n+\t\t\treturn;\n+\t\t}\n+\n+\t\t/* walk up for one_parent_path prefixes */\n+\t\tfor (seen = candidate-1; (seen >= bulkmove_candidates) &&\n+\t\t\t     !prefixcmp(one_parent_path, seen->one->path); seen--) {\n+\t\t\tfprintf(stderr, \"[DBG]  ? %s -> %s\\n\", seen->one->path, seen->two->path);\n+\t\t\t/* subdir of \"seen\" dest dir ? */\n+\t\t\tif (discard_if_outside(candidate, seen))\n+\t\t\t\tcontinue;\n+\t\t}\n+\t\t/* look down for other moves from one_parent_path */\n+\t\tseen = candidate + 1;\n+\t\tif (seen != bulkmove_candidates + bulkmove_candidates_nr &&\n+\t\t    !strcmp(one_parent_path, seen->one->path)) {\n+\t\t\tfprintf(stderr, \"[DBG]  ? %s -> %s (2)\\n\", seen->one->path, seen->two->path);\n+\t\t\t/* subdir of \"seen\" dest dir ? */\n+\t\t\tif (discard_if_outside(candidate, seen))\n+\t\t\t\tcontinue;\n+\t\t}\n+\n+\t\t/* next parent if any */\n+\t\tcopy_dirname(one_parent_path, one_parent_path);\n+\t} while (*one_parent_path);\n+}\n+\n+/*\n+ * Take all file renames recorded so far and check if they could cause\n+ * a bulk move to be detected.\n+ */\n+static void diffcore_bulk_moves(void)\n+{\n+\tint i;\n+\tfor (i = 0; i < rename_dst_nr; i++)\n+\t\tcheck_one_bulk_move(rename_dst[i].pair);\n+}\n+\n void diffcore_rename(struct diff_options *options)\n {\n \tint detect_rename = options->detect_rename;\n@@ -424,6 +722,7 @@ void diffcore_rename(struct diff_options *options)\n \tstruct diff_score *mx;\n \tint i, j, rename_count;\n \tint num_create, num_src, dst_cnt;\n+\tstruct diff_bulk_rename *candidate;\n \n \tif (!minimum_score)\n \t\tminimum_score = DEFAULT_RENAME_SCORE;\n@@ -438,8 +737,7 @@ void diffcore_rename(struct diff_options *options)\n \t\t\t\tcontinue; /* not interested */\n \t\t\telse\n \t\t\t\tlocate_rename_dst(p->two, 1);\n-\t\t}\n-\t\telse if (!DIFF_FILE_VALID(p->two)) {\n+\t\t} else if (!DIFF_FILE_VALID(p->two)) {\n \t\t\t/*\n \t\t\t * If the source is a broken \"delete\", and\n \t\t\t * they did not really want to get broken,\n@@ -450,14 +748,23 @@ void diffcore_rename(struct diff_options *options)\n \t\t\tif (p->broken_pair && !p->score)\n \t\t\t\tp->one->rename_used++;\n \t\t\tregister_rename_src(p->one, p->score);\n-\t\t}\n-\t\telse if (detect_rename == DIFF_DETECT_COPY) {\n-\t\t\t/*\n-\t\t\t * Increment the \"rename_used\" score by\n-\t\t\t * one, to indicate ourselves as a user.\n-\t\t\t */\n-\t\t\tp->one->rename_used++;\n-\t\t\tregister_rename_src(p->one, p->score);\n+\t\t} else {\n+\t\t\tif (detect_rename == DIFF_DETECT_COPY) {\n+\t\t\t\t/*\n+\t\t\t\t * Increment the \"rename_used\" score by\n+\t\t\t\t * one, to indicate ourselves as a user.\n+\t\t\t\t */\n+\t\t\t\tp->one->rename_used++;\n+\t\t\t\tregister_rename_src(p->one, p->score);\n+\t\t\t}\n+\t\t\tif (DIFF_OPT_TST(options, DETECT_BULK_MOVES)) {\n+\t\t\t\t/* similarly, bulk move detection needs to\n+\t\t\t\t * see all files from second tree, but we don't\n+\t\t\t\t * want them to be matched against single sources.\n+\t\t\t\t */\n+\t\t\t\t// FIXME: check interaction with --find-copies-harder\n+\t\t\t\tlocate_rename_dst(p->two, 1)->i_am_not_single = 1;\n+\t\t\t}\n \t\t}\n \t}\n \tif (rename_dst_nr == 0 || rename_src_nr == 0)\n@@ -509,6 +816,8 @@ void diffcore_rename(struct diff_options *options)\n \n \t\tif (rename_dst[i].pair)\n \t\t\tcontinue; /* dealt with exact match already. */\n+\t\tif (rename_dst[i].i_am_not_single)\n+\t\t\tcontinue; /* not looking for a match. */\n \n \t\tm = &mx[dst_cnt * NUM_CANDIDATE_PER_DST];\n \t\tfor (j = 0; j < NUM_CANDIDATE_PER_DST; j++)\n@@ -569,7 +878,30 @@ void diffcore_rename(struct diff_options *options)\n \t/* At this point, we have found some renames and copies and they\n \t * are recorded in rename_dst.  The original list is still in *q.\n \t */\n+\n+\t/* Now possibly factorize those renames and copies. */\n+\tif (DIFF_OPT_TST(options, DETECT_BULK_MOVES))\n+\t\tdiffcore_bulk_moves();\n+\n \tDIFF_QUEUE_CLEAR(&outq);\n+\n+\t/* Now turn non-discarded bulkmove_candidates into real renames */\n+\tfor (candidate = bulkmove_candidates;\n+\t     candidate < bulkmove_candidates + bulkmove_candidates_nr; candidate++) {\n+\t\tstruct diff_filepair* pair;\n+\t\tif (candidate->discarded)\n+\t\t\tcontinue;\n+\t\t/* visualize toplevel dir if needed */\n+\t\tif (!*candidate->one->path)\n+\t\t\tcandidate->one->path = \"./\";\n+\t\tif (!*candidate->two->path)\n+\t\t\tcandidate->two->path = \"./\";\n+\t\tpair = diff_queue(&outq, candidate->one, candidate->two);\n+\t\tpair->score = MAX_SCORE;\n+\t\tpair->renamed_pair = 1;\n+\t\tpair->is_bulkmove = 1;\n+\t}\n+\n \tfor (i = 0; i < q->nr; i++) {\n \t\tstruct diff_filepair *p = q->queue[i];\n \t\tstruct diff_filepair *pair_to_free = NULL;\ndiff --git a/diffcore.h b/diffcore.h\nindex b8f1fde..6dab95b 100644\n--- a/diffcore.h\n+++ b/diffcore.h\n@@ -69,6 +69,7 @@ struct diff_filepair {\n \tunsigned broken_pair : 1;\n \tunsigned renamed_pair : 1;\n \tunsigned is_unmerged : 1;\n+\tunsigned is_bulkmove : 1;\n };\n #define DIFF_PAIR_UNMERGED(p) ((p)->is_unmerged)\n \ndiff --git a/tree-diff.c b/tree-diff.c\nindex cd659c6..5d9f123 100644\n--- a/tree-diff.c\n+++ b/tree-diff.c\n@@ -49,7 +49,9 @@ static int compare_tree_entry(struct tree_desc *t1, struct tree_desc *t2, const\n \t\tshow_entry(opt, \"+\", t2, base, baselen);\n \t\treturn 1;\n \t}\n-\tif (!DIFF_OPT_TST(opt, FIND_COPIES_HARDER) && !hashcmp(sha1, sha2) && mode1 == mode2)\n+\tif (!DIFF_OPT_TST(opt, FIND_COPIES_HARDER) &&\n+\t    !DIFF_OPT_TST(opt, DETECT_BULK_MOVES) &&\n+\t    !hashcmp(sha1, sha2) && mode1 == mode2)\n \t\treturn 0;\n \n \t/*\n-- \n1.7.2.3\n"},{"id":"154285","messageId":"1287868022-24872-3-git-send-email-ydirson@altern.org","threadId":"25536","inReplyTo":"1287868022-24872-1-git-send-email-ydirson@altern.org","subject":"[PATCH v7 2/3] Add testcases for the --detect-bulk-moves diffcore flag.","fromName":"Yann Dirson","fromEmail":"ydirson@altern.org","sentAt":"2010-10-23T21:07:01Z","receivedAt":"2010-10-23T21:07:01Z","isPatch":true,"sender":{"key":"ydirson@altern.org","avatar":"https://avatars.githubusercontent.com/u/1190950?v=4"},"body":"This notably includes a couple of tests for cases known not to be\nworking correctly yet.\n\nThis patch has been improved by the following contributions:\n- Jonathan Nieder: reworked style of test script\n- Jonathan Nieder: use \"git commit\" in test instead of only plumbing,\n  and use test_tick\n- Sverre Rabbelier: anonymize hashes\n\nThanks-to: Jonathan Nieder <jrnieder@gmail.com>\nThanks-to: Sverre Rabbelier <srabbelier@gmail.com>\nSigned-off-by: Yann Dirson <ydirson@free.fr>\n---\n t/t4046-diff-rename-factorize.sh |  296 ++++++++++++++++++++++++++++++++++++++\n 1 files changed, 296 insertions(+), 0 deletions(-)\n create mode 100755 t/t4046-diff-rename-factorize.sh\n\ndiff --git a/t/t4046-diff-rename-factorize.sh b/t/t4046-diff-rename-factorize.sh\nnew file mode 100755\nindex 0000000..88e5b5b\n--- /dev/null\n+++ b/t/t4046-diff-rename-factorize.sh\n@@ -0,0 +1,296 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2008,2010 Yann Dirson\n+# Copyright (c) 2005 Junio C Hamano\n+#\n+\n+# TODO for dir renames:\n+# * two dirs or more moving all their files to a single dir\n+# * simultaneous bulkmove and rename\n+\n+test_description='Test rename factorization in diff engine.\n+\n+'\n+. ./test-lib.sh\n+. \"$TEST_DIRECTORY\"/diff-lib.sh\n+\n+test_expect_success 'setup' '\n+\tgit commit --allow-empty -m \"original empty commit\"\n+\n+\tmkdir a &&\n+\tprintf \"Line %s\\n\" 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 >a/path0 &&\n+\tsed <a/path0 >a/path1 s/Line/Record/ &&\n+\tsed <a/path0 >a/path2 s/Line/Stuff/ &&\n+\tsed <a/path0 >a/path3 s/Line/Blurb/ &&\n+\n+\tgit update-index --add a/path* &&\n+\ttest_tick &&\n+\tgit commit -m \"original set of files\"\n+\n+\t: rename the directory &&\n+\tgit mv a b\n+'\n+test_expect_success 'diff-index --detect-bulk-moves after directory move.' '\n+\tcat >expected <<-EOF &&\n+\t:040000 040000 X X R#\ta/*\tb/\n+\t:100644 100644 X X R#\ta/path0\tb/path0\n+\t:100644 100644 X X R#\ta/path1\tb/path1\n+\t:100644 100644 X X R#\ta/path2\tb/path2\n+\t:100644 100644 X X R#\ta/path3\tb/path3\n+\tEOF\n+\tgit diff-index --detect-bulk-moves HEAD >current &&\n+\tcompare_diff_raw expected current\n+'\n+\n+test_expect_success 'setup non-100% rename' '\n+\techo \"Line 16\" >>b/path0 &&\n+\tgit mv b/path2 b/2path &&\n+\tgit rm -f b/path3 &&\n+\techo anything >b/path100 &&\n+\tgit add b/path100\n+'\n+test_expect_success 'diff-index --detect-bulk-moves after content changes.' '\n+\tcat >expected <<-EOF &&\n+\t:040000 040000 X X R#\ta/*\tb/\n+\t:100644 000000 X X D#\ta/path3\n+\t:100644 100644 X X R#\ta/path2\tb/2path\n+\t:100644 100644 X X R#\ta/path0\tb/path0\n+\t:100644 100644 X X R#\ta/path1\tb/path1\n+\t:000000 100644 X X A#\tb/path100\n+\tEOF\n+\tgit diff-index --detect-bulk-moves HEAD >current &&\n+\tcompare_diff_raw expected current\n+'\n+\n+test_expect_success 'setup bulk move that is not directory move' '\n+\tgit reset -q --hard &&\n+\n+\tmkdir c &&\n+\t(\n+\t\tfor i in 0 1 2; do\n+\t\t\tcp a/path$i c/apath$i || exit\n+\t\tdone\n+\t) &&\n+\tgit update-index --add c/apath* &&\n+\ttest_tick &&\n+\tgit commit -m \"first set of changes\" &&\n+\n+\tgit mv c/* a/\n+'\n+test_expect_success 'diff-index --detect-bulk-moves without full-dir rename.' '\n+\tcat >expected <<-EOF &&\n+\t:040000 040000 X X R#\tc/*\ta/\n+\t:100644 100644 X X R#\tc/apath0\ta/apath0\n+\t:100644 100644 X X R#\tc/apath1\ta/apath1\n+\t:100644 100644 X X R#\tc/apath2\ta/apath2\n+\tEOF\n+\tgit diff-index --detect-bulk-moves HEAD >current\n+\tcompare_diff_raw expected current\n+'\n+\n+test_expect_success 'setup bulk move with new file in source dir' '\n+\techo > c/anotherpath \"How much wood?\" &&\n+\tgit update-index --add c/another*\n+'\n+test_expect_success 'diff-index --detect-bulk-moves with new file in source dir.' '\n+\tcat >expected <<-EOF &&\n+\t:040000 040000 X X R#\tc/*\ta/\n+\t:100644 100644 X X R#\tc/apath0\ta/apath0\n+\t:100644 100644 X X R#\tc/apath1\ta/apath1\n+\t:100644 100644 X X R#\tc/apath2\ta/apath2\n+\t:000000 100644 X X A#\tc/anotherpath\n+\tEOF\n+\tgit diff-index --detect-bulk-moves HEAD >current\n+\tcompare_diff_raw expected current\n+'\n+\n+test_expect_success 'setup bulk move with interfering copy' '\n+\trm c/anotherpath &&\n+\tgit update-index --remove c/anotherpath &&\n+\tmkdir b &&\n+\tcp a/apath0 b/apath9 &&\n+\techo >> a/apath0 \"more\" &&\n+\tgit update-index --add a/apath0 b/apath9\n+'\n+# scores select the \"wrong\" one as \"moved\" (only a suboptimal detection)\n+test_expect_failure 'diff-index --detect-bulk-moves with interfering copy.' '\n+\tcat >expected <<-EOF &&\n+\t:040000 040000 X X R#\tc/*\ta/\n+\t:100644 100644 X X R#\tc/apath0\ta/apath0\n+\t:100644 100644 X X R#\tc/apath1\ta/apath1\n+\t:100644 100644 X X R#\tc/apath2\ta/apath2\n+\t:100644 100644 X X C#\tc/apath0\tb/apath9\n+\tEOF\n+\tgit diff-index --detect-bulk-moves HEAD >current\n+\tcompare_diff_raw expected current\n+'\n+\n+test_expect_success 'setup bulk move to toplevel' '\n+\tgit reset -q --hard &&\n+\tgit mv c/* .\n+'\n+test_expect_success 'diff-index --detect-bulk-moves bulk move to toplevel.' '\n+\tcat >expected <<-EOF &&\n+\t:040000 040000 X X R#\tc/*\t./\n+\t:100644 100644 X X R#\tc/apath0\tapath0\n+\t:100644 100644 X X R#\tc/apath1\tapath1\n+\t:100644 100644 X X R#\tc/apath2\tapath2\n+\tEOF\n+\tgit diff-index --detect-bulk-moves HEAD >current &&\n+\tcompare_diff_raw expected current\n+'\n+\n+test_expect_success 'setup move including a subdir, with some content changes' '\n+\tgit reset -q --hard &&\n+\tmv c a/ &&\n+\tgit update-index --add --remove a/c/* c/apath0 c/apath1 c/apath2 &&\n+\ttest_tick &&\n+\tgit commit -m \"move as subdir\" &&\n+\n+\tgit mv a b &&\n+\techo foo >>b/c/apath0 &&\n+\tgit update-index --add b/c/apath*\n+'\n+test_expect_success 'diff-index --detect-bulk-moves on a move including a subdir.' '\n+\tcat >expected <<-EOF &&\n+\t:040000 040000 X X R#\ta/*\tb/\n+\t:040000 040000 X X R#\ta/c/*\tb/c/\n+\t:100644 100644 X X R#\ta/c/apath0\tb/c/apath0\n+\t:100644 100644 X X R#\ta/c/apath1\tb/c/apath1\n+\t:100644 100644 X X R#\ta/c/apath2\tb/c/apath2\n+\t:100644 100644 X X R#\ta/path0\tb/path0\n+\t:100644 100644 X X R#\ta/path1\tb/path1\n+\t:100644 100644 X X R#\ta/path2\tb/path2\n+\t:100644 100644 X X R#\ta/path3\tb/path3\n+\tEOF\n+\tgit diff-index --detect-bulk-moves HEAD >current &&\n+\tcompare_diff_raw expected current\n+'\n+\n+test_expect_success 'setup move of only a subdir' '\n+\tgit reset -q --hard &&\n+\tgit mv a/c a/d &&\n+\t: rename a subdirectory of a/.\n+'\n+test_expect_success 'moving a subdir only' '\n+\tcat >expected <<-EOF &&\n+\t:040000 040000 X X R#\ta/c/*\ta/d/\n+\t:100644 100644 X X R#\ta/c/apath0\ta/d/apath0\n+\t:100644 100644 X X R#\ta/c/apath1\ta/d/apath1\n+\t:100644 100644 X X R#\ta/c/apath2\ta/d/apath2\n+\tEOF\n+\tgit diff-index --detect-bulk-moves HEAD >current &&\n+\tcompare_diff_raw expected current\n+'\n+\n+test_expect_success 'setup move without a subdir' '\n+\tgit reset -q --hard &&\n+\tmkdir b &&\n+\t: rename files in the directory but not subdir. &&\n+\tgit mv a/path* b/\n+'\n+test_expect_success 'moving files but not subdirs is not mistaken for dir move' '\n+\tcat >expected <<-EOF &&\n+\t:100644 100644 X X R#\ta/path0\tb/path0\n+\t:100644 100644 X X R#\ta/path1\tb/path1\n+\t:100644 100644 X X R#\ta/path2\tb/path2\n+\t:100644 100644 X X R#\ta/path3\tb/path3\n+\tEOF\n+\tgit diff-index --detect-bulk-moves HEAD >current &&\n+\tcompare_diff_raw expected current\n+'\n+\n+test_expect_success 'setup move of files and subdirs to different places' '\n+\tgit reset -q --hard &&\n+\tgit mv a/c b &&\n+\tgit mv a d\n+'\n+test_expect_success 'moving subdirs into one dir and files into another is not mistaken for dir move' '\n+\tcat >expected <<-EOF &&\n+\t:040000 040000 X X R#\ta/c/*\tb/\n+\t:100644 100644 X X R#\ta/c/apath0\tb/apath0\n+\t:100644 100644 X X R#\ta/c/apath1\tb/apath1\n+\t:100644 100644 X X R#\ta/c/apath2\tb/apath2\n+\t:100644 100644 X X R#\ta/path0\td/path0\n+\t:100644 100644 X X R#\ta/path1\td/path1\n+\t:100644 100644 X X R#\ta/path2\td/path2\n+\t:100644 100644 X X R#\ta/path3\td/path3\n+\tEOF\n+\tgit diff-index --detect-bulk-moves HEAD >current &&\n+\tcompare_diff_raw expected current\n+'\n+\n+# the same with different ordering\n+test_expect_success 'setup move of files and subdirs to different places' '\n+\tgit mv d 0\n+'\n+test_expect_success 'moving subdirs into one dir and files into another is not mistaken for dir move' '\n+\tcat >expected <<-EOF &&\n+\t:040000 040000 X X R#\ta/c/*\tb/\n+\t:100644 100644 X X R#\ta/path0\t0/path0\n+\t:100644 100644 X X R#\ta/path1\t0/path1\n+\t:100644 100644 X X R#\ta/path2\t0/path2\n+\t:100644 100644 X X R#\ta/path3\t0/path3\n+\t:100644 100644 X X R#\ta/c/apath0\tb/apath0\n+\t:100644 100644 X X R#\ta/c/apath1\tb/apath1\n+\t:100644 100644 X X R#\ta/c/apath2\tb/apath2\n+\tEOF\n+\tgit diff-index --detect-bulk-moves HEAD >current &&\n+\tcompare_diff_raw expected current\n+'\n+\n+test_expect_success 'setup move of dir with only subdirs' '\n+\tgit reset -q --hard &&\n+\tmkdir a/b &&\n+\tmv a/path* a/b/ &&\n+\tgit update-index --add --remove a/path0 a/path1 a/path2 a/path3 a/b/path* &&\n+\ttest_tick &&\n+\tgit commit -m \"move all toplevel files down one level\" &&\n+\n+\tgit mv a z\n+'\n+# TODO: only a suboptimal non-detection\n+test_expect_failure 'moving a dir with no direct children files' '\n+\tcat >expected <<-EOF &&\n+\t:040000 040000 X X R#\ta/*\tz/\n+\t:040000 040000 X X R#\ta/b/*\tz/b/\n+\t:040000 040000 X X R#\ta/c/*\tz/c/\n+\t:100644 100644 X X R#\ta/b/path0\tz/b/path0\n+\t:100644 100644 X X R#\ta/b/path1\tz/b/path1\n+\t:100644 100644 X X R#\ta/b/path2\tz/b/path2\n+\t:100644 100644 X X R#\ta/b/path3\tz/b/path3\n+\t:100644 100644 X X R#\ta/c/apath0\tz/c/apath0\n+\t:100644 100644 X X R#\ta/c/apath1\tz/c/apath1\n+\t:100644 100644 X X R#\ta/c/apath2\tz/c/apath2\n+\tEOF\n+\tgit diff-index --detect-bulk-moves HEAD >current &&\n+\tcompare_diff_raw expected current\n+'\n+# now test moving all files from toplevel into subdir (does not hides file moves) (needs consensus on syntax)\n+# Note: this is a special case of move of a dir into one of its own subdirs, which in\n+# turn is a variant of new files/dirs being added into a dir after all its contents\n+# are moved away\n+\n+test_expect_success 'setup move from toplevel to subdir' '\n+\tgit reset -q --hard HEAD~3 &&\n+\tmv a/* . &&\n+\tgit update-index --add --remove a/path0 a/path1 a/path2 a/path3 path* &&\n+\ttest_tick &&\n+\tgit commit -m \"move all files to toplevel\" &&\n+\n+\tmkdir z &&\n+\tgit mv path* z/\n+'\n+test_expect_success '--detect-bulk-moves everything from toplevel.' '\n+\tcat >expected <<-EOF &&\n+\t:040000 040000 X X R#\t./*\tz/\n+\t:100644 100644 X X R#\tpath0\tz/path0\n+\t:100644 100644 X X R#\tpath1\tz/path1\n+\t:100644 100644 X X R#\tpath2\tz/path2\n+\t:100644 100644 X X R#\tpath3\tz/path3\n+\tEOF\n+\tgit diff-index --detect-bulk-moves HEAD >current &&\n+\tcompare_diff_raw expected current\n+'\n+\n+test_done\n-- \n1.7.2.3\n"},{"id":"154284","messageId":"1287868022-24872-4-git-send-email-ydirson@altern.org","threadId":"25536","inReplyTo":"1287868022-24872-1-git-send-email-ydirson@altern.org","subject":"[PATCH v7 3/3] [WIP] Allow hiding renames of individual files involved in a directory rename.","fromName":"Yann Dirson","fromEmail":"ydirson@altern.org","sentAt":"2010-10-23T21:07:02Z","receivedAt":"2010-10-23T21:07:02Z","isPatch":true,"sender":{"key":"ydirson@altern.org","avatar":"https://avatars.githubusercontent.com/u/1190950?v=4"},"body":"Once has identified groups of bulk-moved files, and then\nthe --hide-bulk-move-details flag hides those of the individual renames\nwhich carry no other information (further name change, or content changes).\n\nThis makes it much easier to a human reader to spot content changes\nin a commit that also moves a whole subtree.\n\nImportant note: unified diff output is not currently useful, since the \"bulk move\"\nheaders are not yet added by --detect-bulk-moves, but the redundant renames are\nreally removed.\n\nSigned-off-by: Yann Dirson <ydirson@free.fr>\n---\n diff.c            |    7 +++++\n diff.h            |    3 ++\n diffcore-rename.c |   68 ++++++++++++++++++++++++++++++++++++++++++++++++++--\n diffcore.h        |    1 +\n 4 files changed, 76 insertions(+), 3 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 1d88281..3cc7a55 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3193,6 +3193,13 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)\n \t\tif (!options->detect_rename)\n \t\t\toptions->detect_rename = DIFF_DETECT_RENAME;\n \t}\n+\telse if (!strcmp(arg, \"--hide-bulk-move-details\")) {\n+\t\tDIFF_OPT_SET(options, HIDE_DIR_RENAME_DETAILS);\n+\t\tif (!DIFF_OPT_TST(options, DETECT_BULK_MOVES))\n+\t\t\tDIFF_OPT_SET(options, DETECT_BULK_MOVES);\n+\t\tif (!options->detect_rename)\n+\t\t\toptions->detect_rename = DIFF_DETECT_RENAME;\n+\t}\n \telse if (!strcmp(arg, \"--follow\"))\n \t\tDIFF_OPT_SET(options, FOLLOW_RENAMES);\n \telse if (!strcmp(arg, \"--color\"))\ndiff --git a/diff.h b/diff.h\nindex b0d6fa6..7f132d0 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -79,6 +79,7 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct diff_options *opt, void *data)\n #define DIFF_OPT_IGNORE_DIRTY_SUBMODULES (1 << 26)\n #define DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG (1 << 27)\n #define DIFF_OPT_DETECT_BULK_MOVES  (1 << 28)\n+#define DIFF_OPT_HIDE_DIR_RENAME_DETAILS (1 << 29)\n \n #define DIFF_OPT_TST(opts, flag)    ((opts)->flags & DIFF_OPT_##flag)\n #define DIFF_OPT_SET(opts, flag)    ((opts)->flags |= DIFF_OPT_##flag)\n@@ -268,6 +269,8 @@ extern void diffcore_fix_diff_index(struct diff_options *);\n \"                try unchanged files as candidate for copy detection.\\n\" \\\n \"  --detect-bulk-moves\\n\" \\\n \"                detect wholesale directory renames.\\n\" \\\n+\"  --hide-bulk-move-details\\n\" \\\n+\"                hide renames of individual files in a directory rename.\\n\" \\\n \"  -l<n>         limit rename attempts up to <n> paths.\\n\" \\\n \"  -O<file>      reorder diffs according to the <file>.\\n\" \\\n \"  -S<string>    find filepair whose only one side contains the string.\\n\" \\\ndiff --git a/diffcore-rename.c b/diffcore-rename.c\nindex fa2ba7c..3a68807 100644\n--- a/diffcore-rename.c\n+++ b/diffcore-rename.c\n@@ -503,6 +503,34 @@ static struct diff_bulk_rename *locate_bulkmove_candidate(const char *one_path,\n }\n \n /*\n+ * Marks as such file_rename if it is made uninteresting by dir_rename.\n+ * Returns -1 if the file_rename is outside of the range in which given\n+ * renames concerned by dir_rename are to be found (ie. end of loop),\n+ * 0 otherwise.\n+ */\n+static int maybe_mark_uninteresting(struct diff_rename_dst *file_rename,\n+\t\t\t\t    struct diff_bulk_rename *dir_rename,\n+\t\t\t\t    int one_len, int two_len)\n+{\n+\tif (!file_rename->pair) /* file add */\n+\t\treturn 0;\n+\tif (strncmp(file_rename->two->path,\n+\t\t    dir_rename->two->path, two_len))\n+\t\treturn -1;\n+\tif (strncmp(file_rename->pair->one->path,\n+\t\t    dir_rename->one->path, one_len) ||\n+\t    !basename_same(file_rename->pair->one, file_rename->two) ||\n+\t    file_rename->pair->score != MAX_SCORE)\n+\t\treturn 0;\n+\n+\tfile_rename->pair->uninteresting_rename = 1;\n+\tfprintf(stderr, \"[DBG] %s* -> %s* makes %s -> %s uninteresting\\n\",\n+\t\tdir_rename->one->path, dir_rename->two->path,\n+\t\tfile_rename->pair->one->path, file_rename->two->path);\n+\treturn 0;\n+}\n+\n+/*\n  * Copy dirname of src into dst, suitable to append a filename without\n  * an additional \"/\".\n  * Only handles relative paths since there is no absolute path in a git repo.\n@@ -705,11 +733,44 @@ static void check_one_bulk_move(struct diff_filepair *dstpair)\n  * Take all file renames recorded so far and check if they could cause\n  * a bulk move to be detected.\n  */\n-static void diffcore_bulk_moves(void)\n+static void diffcore_bulk_moves(int opt_hide_renames)\n {\n \tint i;\n \tfor (i = 0; i < rename_dst_nr; i++)\n \t\tcheck_one_bulk_move(rename_dst[i].pair);\n+\n+\tif (opt_hide_renames) {\n+\t\t/* flag as \"uninteresting\" those candidates hidden by dir move */\n+\t\tstruct diff_bulk_rename *candidate;\n+\t\tfor (candidate = bulkmove_candidates;\n+\t\t     candidate < bulkmove_candidates + bulkmove_candidates_nr;\n+\t\t     candidate++) {\n+\t\t\tint two_idx, i, one_len, two_len;\n+\t\t\tstruct diff_rename_dst *two_sample;\n+\t\t\tif (candidate->discarded)\n+\t\t\t\tcontinue;\n+\n+\t\t\t/* bisect to any entry within candidate dst dir */\n+\t\t\ttwo_sample = locate_rename_dst_dir(candidate->two->path);\n+\t\t\tif (!two_sample) {\n+\t\t\t\tdie(\"PANIC: %s candidate of rename not in target tree (from %s)\\n\",\n+\t\t\t\t    candidate->two->path, candidate->one->path);\n+\t\t\t}\n+\t\t\ttwo_idx = two_sample - rename_dst;\n+\n+\t\t\t/* now remove extraneous 100% files inside. */\n+\t\t\tone_len = strlen(candidate->one->path);\n+\t\t\ttwo_len = strlen(candidate->two->path);\n+\t\t\tfor (i = two_idx; i < rename_dst_nr; i++)\n+\t\t\t\tif (maybe_mark_uninteresting(rename_dst+i, candidate,\n+\t\t\t\t\t\t\t     one_len, two_len) < 0)\n+\t\t\t\t\tbreak;\n+\t\t\tfor (i = two_idx-1; i >= 0; i--)\n+\t\t\t\tif (maybe_mark_uninteresting(rename_dst+i, candidate,\n+\t\t\t\t\t\t\t     one_len, two_len) < 0)\n+\t\t\t\t\tbreak;\n+\t\t}\n+\t}\n }\n \n void diffcore_rename(struct diff_options *options)\n@@ -881,7 +942,7 @@ void diffcore_rename(struct diff_options *options)\n \n \t/* Now possibly factorize those renames and copies. */\n \tif (DIFF_OPT_TST(options, DETECT_BULK_MOVES))\n-\t\tdiffcore_bulk_moves();\n+\t\tdiffcore_bulk_moves(DIFF_OPT_TST(options, HIDE_DIR_RENAME_DETAILS));\n \n \tDIFF_QUEUE_CLEAR(&outq);\n \n@@ -916,7 +977,8 @@ void diffcore_rename(struct diff_options *options)\n \t\t\tstruct diff_rename_dst *dst =\n \t\t\t\tlocate_rename_dst(p->two, 0);\n \t\t\tif (dst && dst->pair) {\n-\t\t\t\tdiff_q(&outq, dst->pair);\n+\t\t\t\tif (!dst->pair->uninteresting_rename)\n+\t\t\t\t\tdiff_q(&outq, dst->pair);\n \t\t\t\tpair_to_free = p;\n \t\t\t}\n \t\t\telse\ndiff --git a/diffcore.h b/diffcore.h\nindex 6dab95b..a4eb8e1 100644\n--- a/diffcore.h\n+++ b/diffcore.h\n@@ -69,6 +69,7 @@ struct diff_filepair {\n \tunsigned broken_pair : 1;\n \tunsigned renamed_pair : 1;\n \tunsigned is_unmerged : 1;\n+\tunsigned uninteresting_rename : 1;\n \tunsigned is_bulkmove : 1;\n };\n #define DIFF_PAIR_UNMERGED(p) ((p)->is_unmerged)\n-- \n1.7.2.3\n"},{"id":"154349","messageId":"1287954634-14201-1-git-send-email-ydirson@altern.org","threadId":"25536","inReplyTo":"1287868022-24872-1-git-send-email-ydirson@altern.org","subject":"[PATCH] [RFC] Add --detect-bulk-moves output to unidiff format.","fromName":"Yann Dirson","fromEmail":"ydirson@altern.org","sentAt":"2010-10-24T21:10:34Z","receivedAt":"2010-10-24T21:10:34Z","isPatch":true,"sender":{"key":"ydirson@altern.org","avatar":"https://avatars.githubusercontent.com/u/1190950?v=4"},"body":"Signed-off-by: Yann Dirson <ydirson@altern.org>\n---\n\nThis patch is a complement to the v7 \"Detection of directory renames\"\nseries.  It produces output like the sample below (taken from kernel\ntree).  Does that feel right ?  Is it worth moving the output\nproduction to a separate func filling an strbuf ?\n\n|commit 86c6d4d0acc64543a485c11e197d0bd2c5ae0bb2\n|Author: Ralf Baechle <ralf@linux-mips.org>\n|Date:   Thu Aug 5 13:25:55 2010 +0100\n|\n|    MIPS: PNX833x: Move code one directory level up.\n|    \n|    It was sharing the nxp directory but no code with pnx8550 and will fit\n|    better into the new platform makefile scheme, if moved.\n|    \n|    Signed-off-by: Ralf Baechle <ralf@linux-mips.org>\n|\n|diff --git --detect-bulk-moves arch/mips/nxp/pnx833x/common/ arch/mips/pnx833x/common/\n|similarity index 100%\n|bulk move from arch/mips/nxp/pnx833x/common/*\n|bulk move to arch/mips/pnx833x/common/\n|diff --git --detect-bulk-moves arch/mips/nxp/pnx833x/stb22x/ arch/mips/pnx833x/stb22x/\n|similarity index 100%\n|bulk move from arch/mips/nxp/pnx833x/stb22x/*\n|bulk move to arch/mips/pnx833x/stb22x/\n|diff --git a/arch/mips/Makefile b/arch/mips/Makefile\n|index 2ba20d9..0556bc4 100644\n[...]\n\n diff.c |   48 +++++++++++++++++++++++++++++++++++++-----------\n 1 files changed, 37 insertions(+), 11 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 1d88281..8f2f67f 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2659,6 +2659,7 @@ static void run_diff_cmd(const char *pgm,\n \tconst char *xfrm_msg = NULL;\n \tint complete_rewrite = (p->status == DIFF_STATUS_MODIFIED) && p->score;\n \tint must_show_header = 0;\n+\tint use_color;\n \n \tif (!DIFF_OPT_TST(o, ALLOW_EXTERNAL))\n \t\tpgm = NULL;\n@@ -2668,14 +2669,36 @@ static void run_diff_cmd(const char *pgm,\n \t\t\tpgm = drv->external;\n \t}\n \n+\t/*\n+\t * don't use colors when the header is intended for an\n+\t * external diff driver\n+\t */\n+\tuse_color = DIFF_OPT_TST(o, COLOR_DIFF) && !pgm;\n+\n+\tif (p->is_bulkmove) {\n+\t\tconst char *set = diff_get_color(use_color, DIFF_METAINFO);\n+\t\tconst char *reset = diff_get_color(use_color, DIFF_RESET);\n+\t\tstruct strbuf *msgbuf;\n+\t\tchar *line_prefix = \"\";\n+\n+\t\tif (o->output_prefix) {\n+\t\t\tmsgbuf = o->output_prefix(o, o->output_prefix_data);\n+\t\t\tline_prefix = msgbuf->buf;\n+\t\t}\n+\t\tfprintf(o->file, \"%s%sdiff --git --detect-bulk-moves %s %s%s\\n\",\n+\t\t\tline_prefix, set, one->path, two->path, reset);\n+\t\tfprintf(o->file, \"%s%ssimilarity index %d%%%s\\n\",\n+\t\t\tline_prefix, set, similarity_index(p), reset);\n+\t\tfprintf(o->file, \"%s%sbulk move from %s*%s\\n\",\n+\t\t\tline_prefix, set, one->path, reset);\n+\t\tfprintf(o->file, \"%s%sbulk move to %s%s\\n\",\n+\t\t\tline_prefix, set, two->path, reset);\n+\t\treturn;\n+\t}\n+\n \tif (msg) {\n-\t\t/*\n-\t\t * don't use colors when the header is intended for an\n-\t\t * external diff driver\n-\t\t */\n \t\tfill_metainfo(msg, name, other, one, two, o, p,\n-\t\t\t      &must_show_header,\n-\t\t\t      DIFF_OPT_TST(o, COLOR_DIFF) && !pgm);\n+\t\t\t      &must_show_header, use_color);\n \t\txfrm_msg = msg->len ? msg->buf : NULL;\n \t}\n \n@@ -2748,8 +2771,10 @@ static void run_diff(struct diff_filepair *p, struct diff_options *o)\n \t\treturn;\n \t}\n \n-\tdiff_fill_sha1_info(one);\n-\tdiff_fill_sha1_info(two);\n+\tif (!p->is_bulkmove) {\n+\t\tdiff_fill_sha1_info(one);\n+\t\tdiff_fill_sha1_info(two);\n+\t}\n \n \tif (!pgm &&\n \t    DIFF_FILE_VALID(one) && DIFF_FILE_VALID(two) &&\n@@ -3529,9 +3554,10 @@ static void diff_flush_patch(struct diff_filepair *p, struct diff_options *o)\n \tif (diff_unmodified_pair(p))\n \t\treturn;\n \n-\tif ((DIFF_FILE_VALID(p->one) && S_ISDIR(p->one->mode)) ||\n-\t    (DIFF_FILE_VALID(p->two) && S_ISDIR(p->two->mode)))\n-\t\treturn; /* no tree diffs in patch format */\n+\tif (!p->is_bulkmove &&\n+\t    ((DIFF_FILE_VALID(p->one) && S_ISDIR(p->one->mode)) ||\n+\t     (DIFF_FILE_VALID(p->two) && S_ISDIR(p->two->mode))))\n+\t\treturn; /* no tree diffs in patch format, except for bulk moves */\n \n \trun_diff(p, o);\n }\n-- \n1.7.2.3\n"},{"id":"154391","messageId":"7veibeitip.fsf@alter.siamese.dyndns.org","threadId":"25536","inReplyTo":"1287868022-24872-2-git-send-email-ydirson@altern.org","subject":"Re: [PATCH v7 1/3] Introduce bulk-move detection in diffcore.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-10-25T08:08:46Z","receivedAt":"2010-10-25T08:08:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Yann Dirson <ydirson@altern.org> writes:\n\n> The output of raw diff is displayed as \"Rnnn a/* b/\".  Those cannot be\n> confused with renames of files named \"whatever/*\" with a literal star\n> character, from the full-zero SHA1's.\n\nI do not particularly like this asterisk here.  It is ambiguous with the\ncase where you renamed a directory whose name is '*' below a/ to a new\ndirectory whose name is 'b', isn't it?  Using 0{40} as a differentiator\nlike you did is probably a good idea, but then I do not see a sound reason\nyou would need nor want that asterisk.\n\n> +Bulk move of all files of a directory into a different one can get\n> +detected using the `\\--detect-bulk-moves` option.  This adds an\n> +additional pass on top of the results of per-file rename detection.\n> +They are reported with NULL SHA1 id, in addition to the file renames:\n> +\n> +------------------------------------------------\n> +:040000 040000 0000000... 0000000... R100\ta/*\tb/\n> +:100644 100644 0123456... 1234567... R90\ta/file0\tb/file3\n> +:100644 100644 0123456... 1234567... R100\ta/file1\tb/file1\n> +:100644 100644 0123456... 1234567... R100\ta/file2\tb/file2\n\nThis is kind of interesting.  Let's address two issues that should be\nuncontroversial:\n\n (1) please do not use a/ and b/, as the reader can easily be confused and\n     mistakenly think you are referring to the pre-/post- indicator we use\n     when writing textual diffs (e.g. \"diff --git a/frotz b/frotz\").  You\n     are illustrating rename between a directory to another directory,\n     both of which are tracked paths.\n\n (2) The last two lines are R100 but with different object names between\n     pre- and post-images, which is wrong and unnecessarily puzzling.\n     Please make them match.\n\nThese obvious two complaints behind us, there is one more interesting\nthing in the above, which is _not_ a complaint.\n\nWhat about renaming of a/file0 to b/file3?  Is this part of \"all files\nfrom directory A moved to directory B\"?  IOW, is the goal of this series\nto use the \"A/* -> B/\" to label the change as bulk directory rename, if\nthe preimage has A/{1,2,3} and the postimage has their moved contents in\nB/{one,two,three}?\n\nI am wondering about the utility of such an extra information.  If there\nwere no \"a/file0 -> b/file3\" entry in the example, I would imagine that we\ncould use this \"a/* -> b/\" information to move \"a/file5\" to \"b/file5\" when\nrebasing this patch to apply to a different preimage that had files other\nthan file{1,2} in directory \"a\", and I would further imagine that might be\na wonderful thing.\n\nBut if the new \"a/* -> b/\" hint only gives \"things from A/ have migrated\nto B/ but I can give no information on what name they took under their new\nhome\", that may probably reduce the utility of this feature.\n\nI dunno.\n"},{"id":"154426","messageId":"20101025201227.GB3347@home.lan","threadId":"25536","inReplyTo":"7veibeitip.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v7 1/3] Introduce bulk-move detection in diffcore.","fromName":"Yann Dirson","fromEmail":"ydirson@free.fr","sentAt":"2010-10-25T20:12:27Z","receivedAt":"2010-10-25T20:12:27Z","isPatch":true,"sender":{"key":"ydirson@free.fr","avatar":null},"body":"On Mon, Oct 25, 2010 at 01:08:46AM -0700, Junio C Hamano wrote:\n> Yann Dirson <ydirson@altern.org> writes:\n> \n> > The output of raw diff is displayed as \"Rnnn a/* b/\".  Those cannot be\n> > confused with renames of files named \"whatever/*\" with a literal star\n> > character, from the full-zero SHA1's.\n> \n> I do not particularly like this asterisk here.  It is ambiguous with the\n> case where you renamed a directory whose name is '*' below a/ to a new\n> directory whose name is 'b', isn't it?  Using 0{40} as a differentiator\n> like you did is probably a good idea, but then I do not see a sound reason\n> you would need nor want that asterisk.\n\nIt all boils down to one of the original motivations of this patch:\nallowing exporters like git-svn to provide the right level of\ninformation on directory renames, instead of only telling svn that the\ngit-svn user has renamed all individual files separately.\n\nIt is only later that I realized that the larger picture was about\nbulk moves, and directory renames were only a subset of those.  So to\nbe able here to provide the right level of information to git-svn, we\nneed a separate syntax for the special case of dir renaming - and it\nappeared natural to me to use something as close as possible to the\nusual syntax used for the operation:\n\ngit mv foo bar => rename directory \"foo\" to \"bar\"\ngit mv foo/* bar/ => move all contents of \"foo\" to pre-existing \"bar\"\n\n(I would still add trailing slashed to the first form for the sake of\nambiguity, obviously).\n\nNow I'm not 100% happy with using 0{40} as differentiator - eg. we may\nsome day find out that it is indeed useful to output the tree hashes\nthere.\n\nOTOH, the quoting rules for diff output are quite minimalist, I don't\nknow whether adding \"*\" as a character that requires quotes around the\nfilename would be acceptable: then we'll be able to differentiate\n\"foo/*\" from \"foo/\"* or \"foo\"/* (quotes here for emphasis, for\ninteraction with other possible special chars) - the current code\nwould have a strong bias towards \"foo/\"*, but neither seem very sexy\nto me (but since they would only appear in \"pathologic\" situations, do\nwe care ?)\n\n[off-topic] while playing with (un)quoted filenames I noticed spaces\ndo not cause quoting, which can cause its own problems, like (not)\nmaking a visual distinction between file \"foo\" and file \"foo \" in diff\noutput - not that it would be good practice, but someone can always\nshoot itself in the foot, and it appears we have not thought yet on\nhow to help him not to shoot himself in this particular way.\n\n\n> > +Bulk move of all files of a directory into a different one can get\n> > +detected using the `\\--detect-bulk-moves` option.  This adds an\n> > +additional pass on top of the results of per-file rename detection.\n> > +They are reported with NULL SHA1 id, in addition to the file renames:\n> > +\n> > +------------------------------------------------\n> > +:040000 040000 0000000... 0000000... R100\ta/*\tb/\n> > +:100644 100644 0123456... 1234567... R90\ta/file0\tb/file3\n> > +:100644 100644 0123456... 1234567... R100\ta/file1\tb/file1\n> > +:100644 100644 0123456... 1234567... R100\ta/file2\tb/file2\n> \n> This is kind of interesting.  Let's address two issues that should be\n> uncontroversial:\n\nOK.\n\n> These obvious two complaints behind us, there is one more interesting\n> thing in the above, which is _not_ a complaint.\n> \n> What about renaming of a/file0 to b/file3?  Is this part of \"all files\n> from directory A moved to directory B\"?\n\nThe move+rename is \"part of\" the bulk rename in that the file is moved\nfrom/two the same dirs as its siblings, but I acknowledge that it is\nmore than that.\n\n>  IOW, is the goal of this series\n> to use the \"A/* -> B/\" to label the change as bulk directory rename, if\n> the preimage has A/{1,2,3} and the postimage has their moved contents in\n> B/{one,two,three}?\n\nYes.  But --hide-bulk-move-details would not hide them, as they would\nnot be strictly included in the bulk move.  Desite their name change,\nthey are however a confirmation that the contents of A/ was move to B/.\n\n\n> I am wondering about the utility of such an extra information.  If there\n> were no \"a/file0 -> b/file3\" entry in the example, I would imagine that we\n> could use this \"a/* -> b/\" information to move \"a/file5\" to \"b/file5\" when\n> rebasing this patch to apply to a different preimage that had files other\n> than file{1,2} in directory \"a\", and I would further imagine that might be\n> a wonderful thing.\n\nI imagined that as well, and that situation would not be a problem:\nsince \"a/file0 -> b/file3\" would be there in the rebased patch,\n\"apply\" would be able to spot the possible conflict.\n\nOTOH, I had the vision of \"merge does automatic moves\" when starting\nthis project, but got convinced on-list that there are always cases\nwhere the \"automatic move\" on merge would be wrong, and that we should\nreport a conflict instead.\n\nThat would mostly shift the problem to: how do we provide support for\neasily solving this type of problem (regardless of how we want to\nrepresent them, which may not be trivial either).\n\n\n> But if the new \"a/* -> b/\" hint only gives \"things from A/ have migrated\n> to B/ but I can give no information on what name they took under their new\n> home\", that may probably reduce the utility of this feature.\n\nIt would still mean that \"ALL things from A/ have migrated\", and does\nnot imply anything more, just like the fact that a file was renamed\ndoes not prevent us to say that while moving it was also modified.\n\n-- \nYann\n"},{"id":"154458","messageId":"20101026181314.GA5695@home.lan","threadId":"25536","inReplyTo":"20101025201227.GB3347@home.lan","subject":"Bulk move and some of its close relatives","fromName":"Yann Dirson","fromEmail":"ydirson@free.fr","sentAt":"2010-10-26T18:13:14Z","receivedAt":"2010-10-26T18:13:14Z","isPatch":false,"sender":{"key":"ydirson@free.fr","avatar":null},"body":"On Mon, Oct 25, 2010 at 10:12:27PM +0200, Yann Dirson wrote:\n> It is only later that I realized that the larger picture was about\n> bulk moves, and directory renames were only a subset of those.\n\nI had a couple of thoughts about this dir-rename -> bulk-moves shift\ntoday while riding to/from work.  So here are some of the fresh air I\ngot...\n\nIs a bulk-move finally so special ?  There is a close relative to it\nwhich may be of interest too: bulk deletion.  If we implement\ndetection of bulk deletion, we can build on it to get:\n\n* detection of a new type of conflict: addition in a branch of a new\n  file in a dir which is deleted in the other branch (quite similar to\n  the one we were talking about, of a new file in a dir that got moved in\n  other branch)\n* split a good part of the current bulk-move patch in a standalone\n  patch, which is something I wondered how to do\n* probably a much better base for directory-split detection\n\n\nThen, since a bulk-move is made of a bulk deletion, and addition\nsomewhere else of those deleted files, it became tempting to\ncontemplate the concept of \"bulk addition\".  At first sight it seems\nawkward, I confess, especially \"bulk addition into a preexisting dir\",\nwhich is a bit hard to define with precision.  OTOH, \"bulk addition of\na brand new dir\" could bring goodies as well:\n\n* new conflict type: creation in both branches of a new directory with\n  same name\n\nThat confict type made me think of another one.  Some of you may have\nnoticed in the v7 commit message a suggestion for future developments\nlabelled \"support other types of bluk-grouping, like prefixes, and\nmaybe config-specified patterns\".  Now if we get the capability of\nspecifying how we define a \"group of files\" with something other than\nthe directory hierarchy (let's say eg. that in t/ every \"t[0-9]{4}-\"\nis a prefix), the \"bulk-add/bulk-add\" conflict generated by 2 tests\nwith same numeric ID would be detected as a conflict.\n\nAnother grouping example would be by file suffix - source-code reorgs\neg. bulk-moving header files into their own dir are not rare either.\n\n\nDoes that sound sane ?\n-- \nYann\n"},{"id":"154477","messageId":"20101027090501.71980573@chalon.bertin.fr","threadId":"25536","inReplyTo":"20101026181314.GA5695@home.lan","subject":"Re: Bulk move and some of its close relatives","fromName":"Yann Dirson","fromEmail":"ydirson@free.fr","sentAt":"2010-10-27T07:05:01Z","receivedAt":"2010-10-27T07:05:01Z","isPatch":false,"sender":{"key":"ydirson@free.fr","avatar":null},"body":"Oh, and another I forgot to mention is detection of bulk copies.  It is\nsomewhat tricky: while --find-copies-harder will properly work in\ncombination with --detect-bulk-copies, for the standard case we will\nneed to use find-copies-harder for those cases where not all of the\ncopied files have been modified.\n\nFor this, maybe it could help to store the unmodified src files in a\nseparate list to avoid pref degradation for the common case (similar to\nmy plans to avoid the need of the i_am_not_single flag in the current\nbulkmove series by using a separte list for dst files).\n\n-- \nYann\n"},{"id":"154672","messageId":"7vr5fa9ij3.fsf@alter.siamese.dyndns.org","threadId":"25536","inReplyTo":"20101025201227.GB3347@home.lan","subject":"Re: [PATCH v7 1/3] Introduce bulk-move detection in diffcore.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-10-28T20:20:00Z","receivedAt":"2010-10-28T20:20:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Yann Dirson <ydirson@free.fr> writes:\n\n> OTOH, the quoting rules for diff output are quite minimalist, I don't\n> know whether adding \"*\" as a character that requires quotes around the\n> filename would be acceptable.\n\nI suspect that would be an unacceptable entry to a slippery slope.\n\n>> IOW, is the goal of this series\n>> to use the \"A/* -> B/\" to label the change as bulk directory rename, if\n>> the preimage has A/{1,2,3} and the postimage has their moved contents in\n>> B/{one,two,three}?\n>\n> Yes.  But --hide-bulk-move-details would not hide them, as they would\n> not be strictly included in the bulk move.  Desite their name change,\n> they are however a confirmation that the contents of A/ was move to B/.\n>\n>> I am wondering about the utility of such an extra information.  If there\n>> were no \"a/file0 -> b/file3\" entry in the example, I would imagine that we\n>> could use this \"a/* -> b/\" information to move \"a/file5\" to \"b/file5\" when\n>> rebasing this patch to apply to a different preimage that had files other\n>> than file{1,2} in directory \"a\", and I would further imagine that might be\n>> a wonderful thing.\n>\n> I imagined that as well, and that situation would not be a problem:\n> since \"a/file0 -> b/file3\" would be there in the rebased patch,\n> \"apply\" would be able to spot the possible conflict.\n>\n> OTOH, I had the vision of \"merge does automatic moves\" when starting\n> this project, but got convinced on-list that there are always cases\n> where the \"automatic move\" on merge would be wrong, and that we should\n> report a conflict instead.\n>\n> That would mostly shift the problem to...\n\nThat would mostly make this patch not worth worrying about, wouldn't it?\nWhat's the point of spending extra cycles to say \"many things have moved\nin the same direction\" without turning it into a usable information?\n"},{"id":"154680","messageId":"20101028214240.GF3347@home.lan","threadId":"25536","inReplyTo":"7vr5fa9ij3.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v7 1/3] Introduce bulk-move detection in diffcore.","fromName":"Yann Dirson","fromEmail":"ydirson@free.fr","sentAt":"2010-10-28T21:42:40Z","receivedAt":"2010-10-28T21:42:40Z","isPatch":true,"sender":{"key":"ydirson@free.fr","avatar":null},"body":"On Thu, Oct 28, 2010 at 01:20:00PM -0700, Junio C Hamano wrote:\n> Yann Dirson <ydirson@free.fr> writes:\n> \n> > OTOH, the quoting rules for diff output are quite minimalist, I don't\n> > know whether adding \"*\" as a character that requires quotes around the\n> > filename would be acceptable.\n> \n> I suspect that would be an unacceptable entry to a slippery slope.\n\nOK.  I have thought of only 2 remaining options:\n\n* keep using 0{40} sha1 as differentiator\n  => will prevent us to ever use this sha1 for something useful\n* introducing a new change-type letter\n  => another slippery slope, with at least bulk removals and bulk\n  copies in sight, with already too few letters to keep them meaningful\n  to human reader\n* introducing a \"bulk\" flag\n  => needs extension of the raw format\n  Would could want to extend the <letter><score> group, for things like\n  \"RB100\" (bulk rename), \"CB090\" (imperfect bulk copy), \"DB096\" (imperfect\n  bulk removal, meaning there were some files left in source directory)\n\n> >> IOW, is the goal of this series\n> >> to use the \"A/* -> B/\" to label the change as bulk directory rename, if\n> >> the preimage has A/{1,2,3} and the postimage has their moved contents in\n> >> B/{one,two,three}?\n> >\n> > Yes.  But --hide-bulk-move-details would not hide them, as they would\n> > not be strictly included in the bulk move.  Desite their name change,\n> > they are however a confirmation that the contents of A/ was move to B/.\n> >\n> >> I am wondering about the utility of such an extra information.  If there\n> >> were no \"a/file0 -> b/file3\" entry in the example, I would imagine that we\n> >> could use this \"a/* -> b/\" information to move \"a/file5\" to \"b/file5\" when\n> >> rebasing this patch to apply to a different preimage that had files other\n> >> than file{1,2} in directory \"a\", and I would further imagine that might be\n> >> a wonderful thing.\n> >\n> > I imagined that as well, and that situation would not be a problem:\n> > since \"a/file0 -> b/file3\" would be there in the rebased patch,\n> > \"apply\" would be able to spot the possible conflict.\n> >\n> > OTOH, I had the vision of \"merge does automatic moves\" when starting\n> > this project, but got convinced on-list that there are always cases\n> > where the \"automatic move\" on merge would be wrong, and that we should\n> > report a conflict instead.\n> >\n> > That would mostly shift the problem to...\n> \n> That would mostly make this patch not worth worrying about, wouldn't it?\n\n(assuming \"that\" refers to not doing automatic moves on merge)\n\nNo, at least the \"avoid hiding useful info among less-relevant stuff\"\nprovided by the human-targetted --hide-bulk-move-details would stay\nregardless of what we decide here.\n\n> What's the point of spending extra cycles to say \"many things have moved\n> in the same direction\" without turning it into a usable information?\n\nIf we issue a conflict, we have already gone farther than where we are\nnow.  Deciding to have this particular type of conflict auto-resolved\nin the way useful to a wider audience could be done (having \"adds\"\nfollow the bulkmove), with obviously the option of not auto-resolving,\nand possibly, the option of auto-resolving \"adds\" as they are\n(ie. keep current behaviour).\n\nBut my feeling is \"automatically solving the conflict\" would be be\nonly marginally different from applying a patch generated with (what\nin current series is) --hide-bulk-move-details where you cannot know\nwhich individual files were moved in the patch author's tree, and has\nno info to decide if his own added files are at the right place or\nneed to follow the move (which I understood to be your point in [1]).\n\nOr did I miss your point ?\n\n[1] http://marc.info/?l=git&m=122610146506413\n\n-- \nYann\n"}]}