{"thread":{"id":"25322","subject":"[PATCH v4 1/4] Introduce wholesame directory move detection in diffcore.","startedAt":"2010-10-03T20:42:05Z","lastAt":"2010-10-06T23:17:34Z","messageCount":25,"participants":["Yann Dirson","Sverre Rabbelier","Junio C Hamano","Ævar Arnfjörð Bjarmason","Jonathan Nieder","Andreas Ericsson"],"isPatch":true,"patchVersion":4,"patchTotal":4},"messages":[{"id":"152400","messageId":"1286138529-6780-1-git-send-email-ydirson@altern.org","threadId":"25322","inReplyTo":null,"subject":"[RFC PATCH v4 0/4] Detection of directory renames","fromName":"Yann Dirson","fromEmail":"ydirson@altern.org","sentAt":"2010-10-03T20:42:05Z","receivedAt":"2010-10-03T20:42:05Z","isPatch":true,"sender":{"key":"ydirson@altern.org","avatar":"https://avatars.githubusercontent.com/u/1190950?v=4"},"body":"At last a reroll of that old series.  The most prominent change is a\nsplit of the old --factorize-renames flag into --detect-dir-renames\nand --hide-dir-rename-details - hopefully following Junio's comments\ndated nearly 2 years ago.  While doing this, a couple of testcases got\nadded (as well as a list of more testcases to be written), and some\nminor optimizations were done, but no heavy surgery.\n\nI hope we can turn the simple detection feature to something\nacceptable for inclusion - that is, once unified diff output shows the\ndetected renames as annotations, and once the known major holes are\nplugged (both from FIXME and testcases).  For the \"hiding\" one OTOH, a\nmore precise description of what we want must probably be done before\nprogressing, but I suggest we keep that for later.\n\nAlso, maybe the --detect-dir-renames would get a good alias as \"-M -M\".\n\nDoes all of that look reasonable ?\n\nYann Dirson (4):\n  Introduce wholesame directory move detection in diffcore.\n  Add testcases for the --detect-dir-renames diffcore flag.\n  Allow hiding renames of individual files involved in a directory\n    rename.\n  Add testcases for the --hide-dir-rename-details diffcore flag.\n\n diff-lib.c                       |    6 +-\n diff.c                           |   12 +\n diff.h                           |    6 +\n diffcore-rename.c                |  316 +++++++++++++++++++++++++++-\n diffcore.h                       |    1 +\n t/t4046-diff-rename-factorize.sh |  433 ++++++++++++++++++++++++++++++++++++++\n tree-diff.c                      |    4 +-\n 7 files changed, 766 insertions(+), 12 deletions(-)\n create mode 100755 t/t4046-diff-rename-factorize.sh\n\n-- \n1.7.2.3\n"},{"id":"152399","messageId":"1286138529-6780-2-git-send-email-ydirson@altern.org","threadId":"25322","inReplyTo":"1286138529-6780-1-git-send-email-ydirson@altern.org","subject":"[PATCH v4 1/4] Introduce wholesame directory move detection in diffcore.","fromName":"Yann Dirson","fromEmail":"ydirson@altern.org","sentAt":"2010-10-03T20:42:06Z","receivedAt":"2010-10-03T20:42:06Z","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-dir-renames diffcore\nflag.\n\nThis is only the first step, adding the basic functionnality and\nadding support to raw diff output (and it breaks unified-diff output\nwhich does not know how to handle that stuff yet).\n\nEven the output format may not be kept as is.  For now both the result\nof \"mv a b\" and \"mv a/* b/\" are displayed as \"Rnnn a/ b/\", which is\nprobably not what we want.  \"Rnnn a/* b/\" could be a good choice for\nthe latter if we want them to be distinguished, and even if we want\nthem to look the same.\n\nOther future developements to be made on top of this include:\n* extension of unified-diff format to express this\n* application of such new diffs\n* teach git-svn (and others ?) to make use of that flag\n* merge correctly in case of addition into a moved dir\n* detect \"directory splits\" so merge can flag a conflict on file adds\n* use inexact dir renames to bump score of below-threshold renames\n  from/to same locations\n* add yours here\n---\n diff-lib.c        |    6 +-\n diff.c            |    5 +\n diff.h            |    3 +\n diffcore-rename.c |  253 +++++++++++++++++++++++++++++++++++++++++++++++++++--\n tree-diff.c       |    4 +-\n 5 files changed, 260 insertions(+), 11 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 392ce2b..f95a672 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_DIR_RENAMES))\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_DIR_RENAMES))\n \t\treturn 0;\n \n \tdiff_change(&revs->diffopt, oldmode, mode,\ndiff --git a/diff.c b/diff.c\nindex 71efa8e..47ef7ea 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-dir-renames\")) {\n+\t\tDIFF_OPT_SET(options, DETECT_DIR_RENAMES);\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 1fd44f5..40c548d 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_DIR_RENAMES  (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-dir-renames\\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..06a8b6c 100644\n--- a/diffcore-rename.c\n+++ b/diffcore-rename.c\n@@ -11,6 +11,7 @@\n static struct diff_rename_dst {\n \tstruct diff_filespec *two;\n \tstruct diff_filepair *pair;\n+\tint 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@@ -49,9 +50,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+static struct diff_rename_dst *locate_rename_dst_dir(struct diff_filespec *dir)\n+{\n+\t/* code mostly duplicated from locate_rename_dst - not sure we\n+\t * could merge them efficiently,though\n+\t */\n+\tint first, last;\n+\tint prefixlength = strlen(dir->path);\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(dir->path, 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 +414,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 +445,180 @@ static void record_if_better(struct diff_score m[], struct diff_score *o)\n \t\tm[worst] = *o;\n }\n \n+struct diff_dir_rename {\n+\tstruct diff_filespec *one;\n+\tstruct diff_filespec *two;\n+\tint discarded;\n+\tstruct diff_dir_rename* next;\n+};\n+\n+// FIXME: prevent possible overflow\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+ */\n+static const char* copy_dirname(char* dst, const char* src)\n+{\n+\tchar* lastslash = strrchr(src, '/');\n+\tif (!lastslash) {\n+\t\t*dst = '\\0';\n+\t\treturn dst;\n+\t}\n+\tstrncpy(dst, src, lastslash - src + 1);\n+\tdst[lastslash - src + 1] = '\\0';\n+\n+\t// if src ends with a \"/\" strip the last component\n+\tif (lastslash[1] == '\\0') {\n+\t\tlastslash = strrchr(dst, '/');\n+\t\tif (!lastslash)\n+\t\t\treturn strcpy(dst, \".\");\n+\t\tlastslash[1] = '\\0';\n+\t}\n+\n+\treturn dst;\n+}\n+\n+/*\n+ * Take all file renames recorded so far and check if they could cause\n+ * a bulk-rename to be detected.\n+ *\n+ *\n+ * FIXME: we could optimize the 100%-rename case by preventing\n+ * recursion to unfold what we know we would refold here.\n+ * FIXME: do we want to replace linked list with sorted array ?\n+ * FIXME: this prototype only properly handles renaming of dirs without\n+ * a subdir.\n+ * FIXME: leaks like hell.\n+ * FIXME: ideas to evaluate a similarity score, anyone ?\n+ *  10% * tree similarity + 90% * moved files similarity ?\n+ * FIXME: ideas to consider under-threshold moves as part of bulk move ?\n+ */\n+static struct diff_dir_rename* factorization_candidates = NULL;\n+static void diffcore_factorize_renames(void)\n+{\n+\tchar one_parent_path[PATH_MAX], two_parent_path[PATH_MAX];\n+\tint i;\n+\n+\tfor (i = 0; i < rename_dst_nr; i++) {\n+\t\tstruct diff_dir_rename* seen;\n+\n+\t\t// FIXME: what are those ?\n+\t\tif (!rename_dst[i].pair)\n+\t\t\tcontinue;\n+\t\t// dummy renames used by copy detection\n+\t\tif (!strcmp(rename_dst[i].pair->one->path, rename_dst[i].pair->two->path))\n+\t\t\tcontinue;\n+\n+\t\tcopy_dirname(one_parent_path, rename_dst[i].pair->one->path);\n+\t\tcopy_dirname(two_parent_path, rename_dst[i].pair->two->path);\n+\n+\t\t// simple rename with no directory change\n+\t\tif (!strcmp(one_parent_path, two_parent_path))\n+\t\t\tcontinue;\n+\n+\t\tstruct diff_filespec* one_parent = alloc_filespec(one_parent_path);\n+\t\tfill_filespec(one_parent, null_sha1 /*FIXME*/, S_IFDIR);\n+\n+\t\t// After this commit, are there any files still under one_parent_path ?\n+\t\tstruct diff_rename_dst* one_leftover = locate_rename_dst_dir(one_parent);\n+\t\tif (one_leftover) { // FIXME: should only be run if !seen\n+\t\t\t/* this might be a dir split, or files added\n+\t\t\t * after the bulk rename, or just an isolated\n+\t\t\t * rename */\n+\t\t\t/* FIXME: should we cache this information to avoid\n+\t\t\t * recomputing that result when many files trigger it ?\n+\t\t\t * See eg. git.git:761e742d692.\n+\t\t\t */\n+\t\t\tint two_idx, j, onep_len, maybe_dir_rename;\n+\n+\t\t\t// try to see if it is a file added after the bulk rename\n+\t\t\ttwo_idx = one_leftover - rename_dst;\n+\t\t\tonep_len = strlen(one_parent_path);\n+\t\t\tmaybe_dir_rename = 1;\n+\n+\t\t\t// check no leftover file was already here before\n+\t\t\tfor (j = two_idx; j < rename_dst_nr; j++) {\n+\t\t\t\tif (strncmp(rename_dst[j].two->path,\n+\t\t\t\t\t    one_parent_path, onep_len))\n+\t\t\t\t\tbreak; // exhausted directory in this direction\n+\t\t\t\tfprintf(stderr, \"[DBG] leftover file %s in %s\\n\",\n+\t\t\t\t\trename_dst[j].two->path, one_parent_path);\n+\t\t\t\tif (rename_dst[j].i_am_not_single || // those were already here\n+\t\t\t\t    (rename_dst[j].pair &&\n+\t\t\t\t     !strncmp(rename_dst[j].pair->one->path,\n+\t\t\t\t\t      one_parent_path, onep_len) && // renamed from here\n+\t\t\t\t     strncmp(rename_dst[j].two->path,\n+\t\t\t\t\t     one_parent_path, onep_len))) { // not to a subdir\n+\t\t\t\t\tmaybe_dir_rename = 0;\n+\t\t\t\t\tfprintf(stderr, \"[DBG] ... tells not a bulk rename\\n\");\n+\t\t\t\t\tbreak;\n+\t\t\t\t}\n+\t\t\t\tfprintf(stderr, \"[DBG] ... not believed to prevent bulk rename\\n\");\n+\t\t\t}\n+\t\t\tif (!maybe_dir_rename) continue;\n+\t\t\t// try the other direction (dup code)\n+\t\t\tfor (j = two_idx-1; j >= 0; j--) {\n+\t\t\t\tif (strncmp(rename_dst[j].two->path,\n+\t\t\t\t\t    one_parent_path, onep_len))\n+\t\t\t\t\tbreak; // exhausted directory in this direction\n+\t\t\t\tfprintf(stderr, \"[DBG] leftover file %s in %s\\n\",\n+\t\t\t\t\trename_dst[j].two->path, one_parent_path);\n+\t\t\t\tif (rename_dst[j].i_am_not_single || // those were already here\n+\t\t\t\t    (rename_dst[j].pair &&\n+\t\t\t\t     !strncmp(rename_dst[j].pair->one->path,\n+\t\t\t\t\t      one_parent_path, onep_len) && // renamed from here\n+\t\t\t\t     strncmp(rename_dst[j].two->path,\n+\t\t\t\t\t     one_parent_path, onep_len))) { // not to a subdir\n+\t\t\t\t\tmaybe_dir_rename = 0;\n+\t\t\t\t\tfprintf (stderr, \"[DBG] ... tells not a bulk rename\\n\");\n+\t\t\t\t\tbreak;\n+\t\t\t\t}\n+\t\t\t\tfprintf(stderr, \"[DBG] ... not believed to prevent bulk rename\\n\");\n+\t\t\t}\n+\t\t\tif (!maybe_dir_rename) continue;\n+\n+\t\t\t// here we are in the case where a directory\n+\t\t\t// content was completely moved, but files\n+\t\t\t// were added to it afterwards.  Proceed as\n+\t\t\t// for a simple bulk move.\n+\t\t}\n+\n+\t\t// already considered ?\n+\t\tfor (seen=factorization_candidates; seen; seen = seen->next)\n+\t\t\tif (!strcmp(seen->one->path, one_parent_path)) break;\n+\t\tif (!seen) {\n+\t\t\t// record potential dir rename\n+\t\t\tseen = xmalloc(sizeof(*seen));\n+\t\t\tseen->one = one_parent;\n+\t\t\tseen->two = alloc_filespec(two_parent_path);\n+\t\t\tfill_filespec(seen->two, null_sha1 /*FIXME*/, S_IFDIR);\n+\t\t\tseen->discarded = 0;\n+\t\t\tseen->next = factorization_candidates;\n+\t\t\tfactorization_candidates = seen;\n+\t\t\tfprintf(stderr, \"[DBG] %s -> %s suggests possible rename from %s to %s\\n\",\n+\t\t\t\trename_dst[i].pair->one->path,\n+\t\t\t\trename_dst[i].pair->two->path,\n+\t\t\t\tone_parent_path, two_parent_path);\n+\t\t\tcontinue;\n+\t\t}\n+\t\tif (seen->discarded)\n+\t\t\tcontinue;\n+\t\t// check that seen entry matches this rename\n+\t\tif (strcmp(two_parent_path, seen->two->path)) {\n+\t\t\tfprintf(stderr, \"[DBG] discarding dir %s from renames (split into %s and %s)\\n\",\n+\t\t\t\tone_parent_path, two_parent_path, seen->two->path);\n+\t\t\tseen->discarded = 1;\n+\t\t}\n+\n+\t\t/* all checks ok, we keep that entry */\n+\t}\n+\n+\treturn;\n+}\n+\n void diffcore_rename(struct diff_options *options)\n {\n \tint detect_rename = options->detect_rename;\n@@ -451,13 +656,22 @@ void diffcore_rename(struct diff_options *options)\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\telse {\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_DIR_RENAMES)) {\n+\t\t\t\t/* similarly, rename factorization 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\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 +723,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 +785,28 @@ 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_DIR_RENAMES))\n+\t\tdiffcore_factorize_renames();\n+\n \tDIFF_QUEUE_CLEAR(&outq);\n+\n+\t// Now turn non-discarded factorization_candidates into real renames\n+\tstruct diff_dir_rename* candidate;\n+\tfor (candidate=factorization_candidates; candidate; candidate = candidate->next) {\n+\t\tstruct diff_filepair* pair;\n+\t\tif (candidate->discarded) continue;\n+\t\t// visualize toplevel dir if needed - FIXME: wrong place for this ?\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}\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/tree-diff.c b/tree-diff.c\nindex cd659c6..ca0b84f 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_DIR_RENAMES) &&\n+\t    !hashcmp(sha1, sha2) && mode1 == mode2)\n \t\treturn 0;\n \n \t/*\n-- \n1.7.2.3\n"},{"id":"152404","messageId":"1286138529-6780-3-git-send-email-ydirson@altern.org","threadId":"25322","inReplyTo":"1286138529-6780-2-git-send-email-ydirson@altern.org","subject":"[PATCH v4 2/4] Add testcases for the --detect-dir-renames diffcore flag.","fromName":"Yann Dirson","fromEmail":"ydirson@altern.org","sentAt":"2010-10-03T20:42:07Z","receivedAt":"2010-10-03T20:42:07Z","isPatch":true,"sender":{"key":"ydirson@altern.org","avatar":"https://avatars.githubusercontent.com/u/1190950?v=4"},"body":"From: Yann Dirson <ydirson@free.fr>\n\nThis notably includes a couple of tests for cases known not to be\nworking correctly yet.\n---\n t/t4046-diff-rename-factorize.sh |  326 ++++++++++++++++++++++++++++++++++++++\n 1 files changed, 326 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..51b0b0b\n--- /dev/null\n+++ b/t/t4046-diff-rename-factorize.sh\n@@ -0,0 +1,326 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2008,2010 Yann Dirson\n+# Copyright (c) 2005 Junio C Hamano\n+#\n+\n+# TODO - missing tests:\n+# * two dirs or more moving all their files to a single dir\n+# * simultaneous bulkmove and rename\n+# * add a new file under a dir that was moved in the same commit\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 \\\n+    'commit the index.'  \\\n+    'git update-ref HEAD $(echo \"original empty commit\" | git commit-tree $(git write-tree))'\n+\n+mkdir a\n+echo >a/path0 'Line 1\n+Line 2\n+Line 3\n+Line 4\n+Line 5\n+Line 6\n+Line 7\n+Line 8\n+Line 9\n+Line 10\n+line 11\n+Line 12\n+Line 13\n+Line 14\n+Line 15\n+'\n+sed <a/path0 >a/path1 s/Line/Record/\n+sed <a/path0 >a/path2 s/Line/Stuff/\n+sed <a/path0 >a/path3 s/Line/Blurb/\n+\n+test_expect_success \\\n+    'update-index --add file inside a directory.' \\\n+    'git update-index --add a/path*'\n+\n+test_expect_success \\\n+    'commit the index.'  \\\n+    'git update-ref HEAD $(echo \"original set of files\" | git commit-tree $(git write-tree))'\n+\n+mv a b\n+test_expect_success \\\n+    'renamed the directory.' \\\n+    'git update-index --add --remove a/path0 a/path1 a/path2 a/path3 b/path*'\n+\n+test_expect_success \\\n+    'git diff-index --detect-dir-renames after directory move.' \\\n+    'git diff-index --detect-dir-renames HEAD >current'\n+grep -v \"^\\[DBG\\] \" <current >current.filtered\n+cat >expected <<\\EOF\n+:040000 040000 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 R100\ta/\tb/\n+:100644 100644 fdbec444a77953b1bcc899d9fabfa202e5e68f08 fdbec444a77953b1bcc899d9fabfa202e5e68f08 R100\ta/path0\tb/path0\n+:100644 100644 2f1f8d70c0fdad990819dfe37a31deb010805161 2f1f8d70c0fdad990819dfe37a31deb010805161 R100\ta/path1\tb/path1\n+:100644 100644 dbde7141d737c8aa0003672c1bc21ded48c6c3b9 dbde7141d737c8aa0003672c1bc21ded48c6c3b9 R100\ta/path2\tb/path2\n+:100644 100644 c6971ab9f08a6cd9c89a0f87d94ae347aad6144a c6971ab9f08a6cd9c89a0f87d94ae347aad6144a R100\ta/path3\tb/path3\n+EOF\n+\n+test_expect_success \\\n+    'validate the output for directory move.' \\\n+    'compare_diff_patch expected current.filtered'\n+\n+# now test non-100% renames\n+\n+echo 'Line 16' >> b/path0\n+mv b/path2 b/2path\n+rm b/path3\n+echo anything > b/path100\n+test_expect_success \\\n+    'edited dir contents.' \\\n+    'git update-index --add --remove b/* b/path2 b/path3'\n+\n+test_expect_success \\\n+    'git diff-index --detect-dir-renames after directory move and content changes.' \\\n+    'git diff-index --detect-dir-renames HEAD >current'\n+grep -v \"^\\[DBG\\] \" <current >current.filtered\n+cat >expected <<\\EOF\n+:040000 040000 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 R100\ta/\tb/\n+:100644 000000 c6971ab9f08a6cd9c89a0f87d94ae347aad6144a 0000000000000000000000000000000000000000 D\ta/path3\n+:100644 100644 dbde7141d737c8aa0003672c1bc21ded48c6c3b9 dbde7141d737c8aa0003672c1bc21ded48c6c3b9 R100\ta/path2\tb/2path\n+:100644 100644 fdbec444a77953b1bcc899d9fabfa202e5e68f08 4db595d12886f90e36765fc1732c17bccb836663 R093\ta/path0\tb/path0\n+:100644 100644 2f1f8d70c0fdad990819dfe37a31deb010805161 2f1f8d70c0fdad990819dfe37a31deb010805161 R100\ta/path1\tb/path1\n+:000000 100644 0000000000000000000000000000000000000000 1ba4650885513e62386fd3e23aeb45beeb67d3bb A\tb/path100\n+EOF\n+\n+test_expect_success \\\n+    'validate the output for directory move and content changes.' \\\n+    'compare_diff_patch expected current.filtered'\n+\n+git reset -q --hard\n+\n+# now test bulk moves that are not directory moves (get consensus before going further ?)\n+\n+mkdir c\n+for i in 0 1 2; do cp a/path$i c/apath$i; done\n+test_expect_success \\\n+    'add files into a new directory.' \\\n+    'git update-index --add c/apath*'\n+\n+test_expect_success \\\n+    'commit all this.'  \\\n+    'git commit -m \"first set of changes\"'\n+\n+mv c/* a/\n+test_expect_success \\\n+    'move all of the new dir contents into a preexisting dir.' \\\n+    'git update-index --add --remove a/* c/apath0 c/apath1 c/apath2'\n+\n+test_expect_success \\\n+    'git diff-index --detect-dir-renames without full-dir rename.' \\\n+    'git diff-index --detect-dir-renames HEAD >current'\n+grep -v \"^\\[DBG\\] \" <current >current.filtered\n+cat >expected <<\\EOF\n+:040000 040000 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 R100\tc/*\ta/\n+:100644 100644 fdbec444a77953b1bcc899d9fabfa202e5e68f08 fdbec444a77953b1bcc899d9fabfa202e5e68f08 R100\tc/apath0\ta/apath0\n+:100644 100644 2f1f8d70c0fdad990819dfe37a31deb010805161 2f1f8d70c0fdad990819dfe37a31deb010805161 R100\tc/apath1\ta/apath1\n+:100644 100644 dbde7141d737c8aa0003672c1bc21ded48c6c3b9 dbde7141d737c8aa0003672c1bc21ded48c6c3b9 R100\tc/apath2\ta/apath2\n+EOF\n+\n+test_expect_failure \\\n+    'validate the output for bulk rename without full-dir rename.' \\\n+    'compare_diff_patch expected current.filtered'\n+\n+git reset -q --hard\n+\n+# now test moves to toplevel\n+\n+mv c/* .\n+test_expect_success \\\n+    'move all of the new dir contents into toplevel.' \\\n+    'git update-index --add --remove apath* c/apath0 c/apath1 c/apath2'\n+\n+test_expect_success \\\n+    'git diff-index --detect-dir-renames files bulk-moved to toplevel.' \\\n+    'git diff-index --detect-dir-renames HEAD >current'\n+grep -v \"^\\[DBG\\] \" <current >current.filtered\n+cat >expected <<\\EOF\n+:040000 040000 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 R100\tc/*\t./\n+:100644 100644 fdbec444a77953b1bcc899d9fabfa202e5e68f08 fdbec444a77953b1bcc899d9fabfa202e5e68f08 R100\tc/apath0\tapath0\n+:100644 100644 2f1f8d70c0fdad990819dfe37a31deb010805161 2f1f8d70c0fdad990819dfe37a31deb010805161 R100\tc/apath1\tapath1\n+:100644 100644 dbde7141d737c8aa0003672c1bc21ded48c6c3b9 dbde7141d737c8aa0003672c1bc21ded48c6c3b9 R100\tc/apath2\tapath2\n+EOF\n+\n+test_expect_failure \\\n+    'validate the output for files bulk-moved to toplevel.' \\\n+    'compare_diff_patch expected current.filtered'\n+\n+git reset -q --hard\n+\n+# now test renaming with subdirs (does not take subdirs into account)\n+\n+mv c a/\n+test_expect_success \\\n+    'move the new dir as subdir of another.' \\\n+    'git update-index --add --remove a/c/* c/apath0 c/apath1 c/apath2'\n+\n+test_expect_success \\\n+    'commit all this.'  \\\n+    'git commit -m \"move as subdir\"'\n+\n+mv a b\n+echo foo >> b/c/apath0\n+test_expect_success \\\n+    'rename the directory with some changes.' \\\n+    'git update-index --add --remove a/path0 a/path1 a/path2 a/path3 a/c/apath0 a/c/apath1 a/c/apath2 b/path* b/c/apath*'\n+\n+test_expect_success \\\n+    'git diff-index --detect-dir-renames on a move including a subdir.' \\\n+    'git diff-index --detect-dir-renames HEAD >current'\n+grep -v \"^\\[DBG\\] \" <current >current.filtered\n+cat >expected <<\\EOF\n+:040000 040000 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 R100\ta/\tb/\n+:100644 100644 fdbec444a77953b1bcc899d9fabfa202e5e68f08 00084e5ea68b5ae339b7c4b429e4a70fe25d069b R096\ta/c/apath0\tb/c/apath0\n+:100644 100644 2f1f8d70c0fdad990819dfe37a31deb010805161 2f1f8d70c0fdad990819dfe37a31deb010805161 R100\ta/c/apath1\tb/c/apath1\n+:100644 100644 dbde7141d737c8aa0003672c1bc21ded48c6c3b9 dbde7141d737c8aa0003672c1bc21ded48c6c3b9 R100\ta/c/apath2\tb/c/apath2\n+:100644 100644 fdbec444a77953b1bcc899d9fabfa202e5e68f08 fdbec444a77953b1bcc899d9fabfa202e5e68f08 R100\ta/path0\tb/path0\n+:100644 100644 2f1f8d70c0fdad990819dfe37a31deb010805161 2f1f8d70c0fdad990819dfe37a31deb010805161 R100\ta/path1\tb/path1\n+:100644 100644 dbde7141d737c8aa0003672c1bc21ded48c6c3b9 dbde7141d737c8aa0003672c1bc21ded48c6c3b9 R100\ta/path2\tb/path2\n+:100644 100644 c6971ab9f08a6cd9c89a0f87d94ae347aad6144a c6971ab9f08a6cd9c89a0f87d94ae347aad6144a R100\ta/path3\tb/path3\n+EOF\n+\n+test_expect_failure \\\n+    'validate the output for a move including a subdir.' \\\n+    'compare_diff_patch expected current.filtered'\n+\n+git reset -q --hard\n+\n+# now check that moving all files but not subdirs is not mistaken for dir move\n+\n+mkdir b\n+mv a/path* b/\n+test_expect_success \\\n+    'rename files in the directory but not subdir.' \\\n+    'git update-index --add --remove a/path0 a/path1 a/path2 a/path3 b/path*'\n+\n+test_expect_success \\\n+    'git diff-index --detect-dir-renames on a move without a subdir.' \\\n+    'git diff-index --detect-dir-renames HEAD >current'\n+grep -v \"^\\[DBG\\] \" <current >current.filtered\n+cat >expected <<\\EOF\n+:100644 100644 fdbec444a77953b1bcc899d9fabfa202e5e68f08 fdbec444a77953b1bcc899d9fabfa202e5e68f08 R100\ta/path0\tb/path0\n+:100644 100644 2f1f8d70c0fdad990819dfe37a31deb010805161 2f1f8d70c0fdad990819dfe37a31deb010805161 R100\ta/path1\tb/path1\n+:100644 100644 dbde7141d737c8aa0003672c1bc21ded48c6c3b9 dbde7141d737c8aa0003672c1bc21ded48c6c3b9 R100\ta/path2\tb/path2\n+:100644 100644 c6971ab9f08a6cd9c89a0f87d94ae347aad6144a c6971ab9f08a6cd9c89a0f87d94ae347aad6144a R100\ta/path3\tb/path3\n+EOF\n+\n+test_expect_success \\\n+    'validate the output for a move without a subdir.' \\\n+    'compare_diff_patch expected current.filtered'\n+\n+git reset -q --hard\n+\n+# now check that moving subdirs into one dir and files into another is not mistaken for dir move\n+# (well, clearly it is ...)\n+\n+mv a/c b\n+mv a d\n+test_expect_success \\\n+    'rename subdir and files into different places.' \\\n+    'git update-index --add --remove a/path0 a/path1 a/path2 a/path3 a/c/apath0 a/c/apath1 a/c/apath2 d/path* b/apath*'\n+\n+test_expect_success \\\n+    'git diff-index --detect-dir-renames on a split of subdir and files into different places.' \\\n+    'git diff-index --detect-dir-renames HEAD >current'\n+grep -v \"^\\[DBG\\] \" <current >current.filtered\n+cat >expected <<\\EOF\n+:040000 040000 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 R100\ta/c/\tb/\n+:100644 100644 fdbec444a77953b1bcc899d9fabfa202e5e68f08 fdbec444a77953b1bcc899d9fabfa202e5e68f08 R100\ta/c/apath0\tb/apath0\n+:100644 100644 2f1f8d70c0fdad990819dfe37a31deb010805161 2f1f8d70c0fdad990819dfe37a31deb010805161 R100\ta/c/apath1\tb/apath1\n+:100644 100644 dbde7141d737c8aa0003672c1bc21ded48c6c3b9 dbde7141d737c8aa0003672c1bc21ded48c6c3b9 R100\ta/c/apath2\tb/apath2\n+:100644 100644 fdbec444a77953b1bcc899d9fabfa202e5e68f08 fdbec444a77953b1bcc899d9fabfa202e5e68f08 R100\ta/path0\td/path0\n+:100644 100644 2f1f8d70c0fdad990819dfe37a31deb010805161 2f1f8d70c0fdad990819dfe37a31deb010805161 R100\ta/path1\td/path1\n+:100644 100644 dbde7141d737c8aa0003672c1bc21ded48c6c3b9 dbde7141d737c8aa0003672c1bc21ded48c6c3b9 R100\ta/path2\td/path2\n+:100644 100644 c6971ab9f08a6cd9c89a0f87d94ae347aad6144a c6971ab9f08a6cd9c89a0f87d94ae347aad6144a R100\ta/path3\td/path3\n+EOF\n+\n+test_expect_failure \\\n+    'validate the output for a split of subdir and files into different places.' \\\n+    'compare_diff_patch expected current.filtered'\n+\n+# now test moving a dir with no files but only subdirs\n+# (only factorizes lowest-level directories - not a big deal, just not perfect)\n+\n+git reset -q --hard\n+mkdir a/b\n+mv a/path* a/b/\n+test_expect_success \\\n+    'setup the directory with only subdirs, no direct child files.' \\\n+    'git update-index --add --remove a/path0 a/path1 a/path2 a/path3 a/b/path*'\n+test_expect_success \\\n+    'commit all this.'  \\\n+    'git commit -m \"move all toplevel files down one level\"'\n+\n+test_expect_success \\\n+    'move this dir.'  \\\n+    'git mv a z'\n+\n+test_expect_success \\\n+    'git diff-index --detect-dir-renames with only subdirs' \\\n+    'git diff-index --detect-dir-renames HEAD >current'\n+grep -v \"^\\[DBG\\] \" <current >current.filtered\n+cat >expected <<\\EOF\n+:040000 040000 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 R100\ta/\tz/\n+:040000 040000 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 R100\ta/b/\tz/b/\n+:040000 040000 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 R100\ta/c/\tz/c/\n+:100644 100644 fdbec444a77953b1bcc899d9fabfa202e5e68f08 fdbec444a77953b1bcc899d9fabfa202e5e68f08 R100\ta/b/path0\tz/b/path0\n+:100644 100644 2f1f8d70c0fdad990819dfe37a31deb010805161 2f1f8d70c0fdad990819dfe37a31deb010805161 R100\ta/b/path1\tz/b/path1\n+:100644 100644 dbde7141d737c8aa0003672c1bc21ded48c6c3b9 dbde7141d737c8aa0003672c1bc21ded48c6c3b9 R100\ta/b/path2\tz/b/path2\n+:100644 100644 c6971ab9f08a6cd9c89a0f87d94ae347aad6144a c6971ab9f08a6cd9c89a0f87d94ae347aad6144a R100\ta/b/path3\tz/b/path3\n+:100644 100644 fdbec444a77953b1bcc899d9fabfa202e5e68f08 fdbec444a77953b1bcc899d9fabfa202e5e68f08 R100\ta/c/apath0\tz/c/apath0\n+:100644 100644 2f1f8d70c0fdad990819dfe37a31deb010805161 2f1f8d70c0fdad990819dfe37a31deb010805161 R100\ta/c/apath1\tz/c/apath1\n+:100644 100644 dbde7141d737c8aa0003672c1bc21ded48c6c3b9 dbde7141d737c8aa0003672c1bc21ded48c6c3b9 R100\ta/c/apath2\tz/c/apath2\n+EOF\n+\n+test_expect_failure \\\n+    'validate the output for a move with only subdirs.' \\\n+    'compare_diff_patch expected current.filtered'\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+git reset -q --hard HEAD~3\n+\n+mv a/* .\n+test_expect_success \\\n+    'rename the directory with some changes.' \\\n+    'git update-index --add --remove a/path0 a/path1 a/path2 a/path3 path*'\n+\n+test_expect_success \\\n+    'commit all this.'  \\\n+    'git commit -m \"move all files to toplevel\"'\n+\n+mkdir z\n+mv path* z/\n+test_expect_success \\\n+    'rename the directory with some changes.' \\\n+    'git update-index --add --remove path0 path1 path2 path3 z/path*'\n+\n+test_expect_success \\\n+    'git diff-index --detect-dir-renames everything from toplevel.' \\\n+    'git diff-index --detect-dir-renames HEAD >current'\n+grep -v \"^\\[DBG\\] \" <current >current.filtered\n+cat >expected <<\\EOF\n+:040000 040000 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 R100\t./*\tz/\n+:100644 100644 fdbec444a77953b1bcc899d9fabfa202e5e68f08 fdbec444a77953b1bcc899d9fabfa202e5e68f08 R100\tpath0\tz/path0\n+:100644 100644 2f1f8d70c0fdad990819dfe37a31deb010805161 2f1f8d70c0fdad990819dfe37a31deb010805161 R100\tpath1\tz/path1\n+:100644 100644 dbde7141d737c8aa0003672c1bc21ded48c6c3b9 dbde7141d737c8aa0003672c1bc21ded48c6c3b9 R100\tpath2\tz/path2\n+:100644 100644 c6971ab9f08a6cd9c89a0f87d94ae347aad6144a c6971ab9f08a6cd9c89a0f87d94ae347aad6144a R100\tpath3\tz/path3\n+EOF\n+\n+test_expect_failure \\\n+    'validate the output for a move of everything from toplevel.' \\\n+    'compare_diff_patch expected current.filtered'\n+\n+test_done\n-- \n1.7.2.3\n"},{"id":"152401","messageId":"1286138529-6780-4-git-send-email-ydirson@altern.org","threadId":"25322","inReplyTo":"1286138529-6780-3-git-send-email-ydirson@altern.org","subject":"[PATCH v4 3/4] Allow hiding renames of individual files involved in a directory rename.","fromName":"Yann Dirson","fromEmail":"ydirson@altern.org","sentAt":"2010-10-03T20:42:08Z","receivedAt":"2010-10-03T20:42:08Z","isPatch":true,"sender":{"key":"ydirson@altern.org","avatar":"https://avatars.githubusercontent.com/u/1190950?v=4"},"body":"From: Yann Dirson <ydirson@free.fr>\n\nOnce --detect-dir-renames has identified groups of bulk-moved files, and then\nthe --hide-dir-rename-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---\n diff.c            |    7 +++++\n diff.h            |    3 ++\n diffcore-rename.c |   67 ++++++++++++++++++++++++++++++++++++++++++++++++++--\n diffcore.h        |    1 +\n 4 files changed, 75 insertions(+), 3 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 47ef7ea..2731c33 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-dir-rename-details\")) {\n+\t\tDIFF_OPT_SET(options, HIDE_DIR_RENAME_DETAILS);\n+\t\tif (!DIFF_OPT_TST(options, DETECT_DIR_RENAMES))\n+\t\t\tDIFF_OPT_SET(options, DETECT_DIR_RENAMES);\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 40c548d..ae1e2e7 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_DIR_RENAMES  (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-dir-renames\\n\" \\\n \"                detect wholesale directory renames.\\n\" \\\n+\"  --hide-dir-rename-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 06a8b6c..89e0b53 100644\n--- a/diffcore-rename.c\n+++ b/diffcore-rename.c\n@@ -452,6 +452,34 @@ struct diff_dir_rename {\n \tstruct diff_dir_rename* next;\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_dir_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 // FIXME: prevent possible overflow\n /*\n  * Copy dirname of src into dst, suitable to append a filename without\n@@ -497,7 +525,7 @@ static const char* copy_dirname(char* dst, const char* src)\n  * FIXME: ideas to consider under-threshold moves as part of bulk move ?\n  */\n static struct diff_dir_rename* factorization_candidates = NULL;\n-static void diffcore_factorize_renames(void)\n+static void diffcore_factorize_renames(int opt_hide_renames)\n {\n \tchar one_parent_path[PATH_MAX], two_parent_path[PATH_MAX];\n \tint i;\n@@ -616,6 +644,38 @@ static void diffcore_factorize_renames(void)\n \t\t/* all checks ok, we keep that entry */\n \t}\n \n+\tif (opt_hide_renames) {\n+\t\t// flag as \"uninteresting\" those candidates hidden by dir move\n+\t\tstruct diff_dir_rename* candidate;\n+\t\tfor (candidate=factorization_candidates;\n+\t\t     candidate; candidate = candidate->next) {\n+\t\t\tint two_idx, i, one_len, two_len;\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\tstruct diff_rename_dst* two_sample =\n+\t\t\t\tlocate_rename_dst_dir(candidate->two);\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 \treturn;\n }\n \n@@ -788,7 +848,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_DIR_RENAMES))\n-\t\tdiffcore_factorize_renames();\n+\t\tdiffcore_factorize_renames(DIFF_OPT_TST(options, HIDE_DIR_RENAME_DETAILS));\n \n \tDIFF_QUEUE_CLEAR(&outq);\n \n@@ -821,7 +881,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 b8f1fde..beee596 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 };\n #define DIFF_PAIR_UNMERGED(p) ((p)->is_unmerged)\n \n-- \n1.7.2.3\n"},{"id":"152403","messageId":"1286138529-6780-5-git-send-email-ydirson@altern.org","threadId":"25322","inReplyTo":"1286138529-6780-4-git-send-email-ydirson@altern.org","subject":"[PATCH v4 4/4] Add testcases for the --hide-dir-rename-details diffcore flag.","fromName":"Yann Dirson","fromEmail":"ydirson@altern.org","sentAt":"2010-10-03T20:42:09Z","receivedAt":"2010-10-03T20:42:09Z","isPatch":true,"sender":{"key":"ydirson@altern.org","avatar":"https://avatars.githubusercontent.com/u/1190950?v=4"},"body":"From: Yann Dirson <ydirson@free.fr>\n\n---\n t/t4046-diff-rename-factorize.sh |  107 ++++++++++++++++++++++++++++++++++++++\n 1 files changed, 107 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t4046-diff-rename-factorize.sh b/t/t4046-diff-rename-factorize.sh\nindex 51b0b0b..d982658 100755\n--- a/t/t4046-diff-rename-factorize.sh\n+++ b/t/t4046-diff-rename-factorize.sh\n@@ -69,6 +69,18 @@ test_expect_success \\\n     'validate the output for directory move.' \\\n     'compare_diff_patch expected current.filtered'\n \n+test_expect_success \\\n+    'git diff-index --hide-dir-rename-details after directory move.' \\\n+    'git diff-index --hide-dir-rename-details HEAD >current'\n+grep -v \"^\\[DBG\\] \" <current >current.filtered\n+cat >expected <<\\EOF\n+:040000 040000 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 R100\ta/\tb/\n+EOF\n+\n+test_expect_success \\\n+    'validate the output for directory move.' \\\n+    'compare_diff_patch expected current.filtered'\n+\n # now test non-100% renames\n \n echo 'Line 16' >> b/path0\n@@ -96,6 +108,22 @@ test_expect_success \\\n     'validate the output for directory move and content changes.' \\\n     'compare_diff_patch expected current.filtered'\n \n+test_expect_success \\\n+    'git diff-index --hide-dir-rename-details after directory move and content changes.' \\\n+    'git diff-index --hide-dir-rename-details HEAD >current'\n+grep -v \"^\\[DBG\\] \" <current >current.filtered\n+cat >expected <<\\EOF\n+:040000 040000 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 R100\ta/\tb/\n+:100644 000000 c6971ab9f08a6cd9c89a0f87d94ae347aad6144a 0000000000000000000000000000000000000000 D\ta/path3\n+:100644 100644 dbde7141d737c8aa0003672c1bc21ded48c6c3b9 dbde7141d737c8aa0003672c1bc21ded48c6c3b9 R100\ta/path2\tb/2path\n+:100644 100644 fdbec444a77953b1bcc899d9fabfa202e5e68f08 4db595d12886f90e36765fc1732c17bccb836663 R093\ta/path0\tb/path0\n+:000000 100644 0000000000000000000000000000000000000000 1ba4650885513e62386fd3e23aeb45beeb67d3bb A\tb/path100\n+EOF\n+\n+test_expect_success \\\n+    'validate the output for directory move and content changes.' \\\n+    'compare_diff_patch expected current.filtered'\n+\n git reset -q --hard\n \n # now test bulk moves that are not directory moves (get consensus before going further ?)\n@@ -130,6 +158,18 @@ test_expect_failure \\\n     'validate the output for bulk rename without full-dir rename.' \\\n     'compare_diff_patch expected current.filtered'\n \n+test_expect_success \\\n+    'git diff-index --hide-dir-rename-details without full-dir rename.' \\\n+    'git diff-index --hide-dir-rename-details HEAD >current'\n+grep -v \"^\\[DBG\\] \" <current >current.filtered\n+cat >expected <<\\EOF\n+:040000 040000 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 R100\tc/*\ta/\n+EOF\n+\n+test_expect_failure \\\n+    'validate the output for bulk rename without full-dir rename.' \\\n+    'compare_diff_patch expected current.filtered'\n+\n git reset -q --hard\n \n # now test moves to toplevel\n@@ -154,6 +194,18 @@ test_expect_failure \\\n     'validate the output for files bulk-moved to toplevel.' \\\n     'compare_diff_patch expected current.filtered'\n \n+test_expect_success \\\n+    'git diff-index --hide-dir-rename-details files bulk-moved to toplevel.' \\\n+    'git diff-index --hide-dir-rename-details HEAD >current'\n+grep -v \"^\\[DBG\\] \" <current >current.filtered\n+cat >expected <<\\EOF\n+:040000 040000 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 R100\tc/*\t./\n+EOF\n+\n+test_expect_failure \\\n+    'validate the output for files bulk-moved to toplevel.' \\\n+    'compare_diff_patch expected current.filtered'\n+\n git reset -q --hard\n \n # now test renaming with subdirs (does not take subdirs into account)\n@@ -192,6 +244,19 @@ test_expect_failure \\\n     'validate the output for a move including a subdir.' \\\n     'compare_diff_patch expected current.filtered'\n \n+test_expect_success \\\n+    'git diff-index --hide-dir-rename-details on a move including a subdir.' \\\n+    'git diff-index --hide-dir-rename-details HEAD >current'\n+grep -v \"^\\[DBG\\] \" <current >current.filtered\n+cat >expected <<\\EOF\n+:040000 040000 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 R100\ta/\tb/\n+:100644 100644 fdbec444a77953b1bcc899d9fabfa202e5e68f08 00084e5ea68b5ae339b7c4b429e4a70fe25d069b R096\ta/c/apath0\tb/c/apath0\n+EOF\n+\n+test_expect_failure \\\n+    'validate the output for a move including a subdir.' \\\n+    'compare_diff_patch expected current.filtered'\n+\n git reset -q --hard\n \n # now check that moving all files but not subdirs is not mistaken for dir move\n@@ -217,6 +282,8 @@ test_expect_success \\\n     'validate the output for a move without a subdir.' \\\n     'compare_diff_patch expected current.filtered'\n \n+# no need to test for --hide-dir-rename-details here\n+\n git reset -q --hard\n \n # now check that moving subdirs into one dir and files into another is not mistaken for dir move\n@@ -247,6 +314,22 @@ test_expect_failure \\\n     'validate the output for a split of subdir and files into different places.' \\\n     'compare_diff_patch expected current.filtered'\n \n+test_expect_success \\\n+    'git diff-index --hide-dir-rename-details on a split of subdir and files into different places.' \\\n+    'git diff-index --hide-dir-rename-details HEAD >current'\n+grep -v \"^\\[DBG\\] \" <current >current.filtered\n+cat >expected <<\\EOF\n+:040000 040000 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 R100\ta/c/\tb/\n+:100644 100644 fdbec444a77953b1bcc899d9fabfa202e5e68f08 fdbec444a77953b1bcc899d9fabfa202e5e68f08 R100\ta/path0\td/path0\n+:100644 100644 2f1f8d70c0fdad990819dfe37a31deb010805161 2f1f8d70c0fdad990819dfe37a31deb010805161 R100\ta/path1\td/path1\n+:100644 100644 dbde7141d737c8aa0003672c1bc21ded48c6c3b9 dbde7141d737c8aa0003672c1bc21ded48c6c3b9 R100\ta/path2\td/path2\n+:100644 100644 c6971ab9f08a6cd9c89a0f87d94ae347aad6144a c6971ab9f08a6cd9c89a0f87d94ae347aad6144a R100\ta/path3\td/path3\n+EOF\n+\n+test_expect_failure \\\n+    'validate the output for a split of subdir and files into different places.' \\\n+    'compare_diff_patch expected current.filtered'\n+\n # now test moving a dir with no files but only subdirs\n # (only factorizes lowest-level directories - not a big deal, just not perfect)\n \n@@ -285,6 +368,18 @@ test_expect_failure \\\n     'validate the output for a move with only subdirs.' \\\n     'compare_diff_patch expected current.filtered'\n \n+test_expect_success \\\n+    'git diff-index --hide-dir-rename-details with only subdirs' \\\n+    'git diff-index --hide-dir-rename-details HEAD >current'\n+grep -v \"^\\[DBG\\] \" <current >current.filtered\n+cat >expected <<\\EOF\n+:040000 040000 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 R100\ta/\tz/\n+EOF\n+\n+test_expect_failure \\\n+    'validate the output for a move with only subdirs.' \\\n+    'compare_diff_patch expected current.filtered'\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@@ -323,4 +418,16 @@ test_expect_failure \\\n     'validate the output for a move of everything from toplevel.' \\\n     'compare_diff_patch expected current.filtered'\n \n+test_expect_success \\\n+    'git diff-index --hide-dir-rename-details everything from toplevel.' \\\n+    'git diff-index --hide-dir-rename-details HEAD >current'\n+grep -v \"^\\[DBG\\] \" <current >current.filtered\n+cat >expected <<\\EOF\n+:040000 040000 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 R100\t./*\tz/\n+EOF\n+\n+test_expect_failure \\\n+    'validate the output for a move of everything from toplevel.' \\\n+    'compare_diff_patch expected current.filtered'\n+\n test_done\n-- \n1.7.2.3\n"},{"id":"152424","messageId":"AANLkTikdD06x45yQsVjhk4NYZYKCXmYzOcioNA_-rZ5O@mail.gmail.com","threadId":"25322","inReplyTo":"1286138529-6780-5-git-send-email-ydirson@altern.org","subject":"Re: [PATCH v4 4/4] Add testcases for the --hide-dir-rename-details diffcore flag.","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2010-10-03T23:04:32Z","receivedAt":"2010-10-03T23:04:32Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Sun, Oct 3, 2010 at 22:42, Yann Dirson <ydirson@altern.org> wrote:\n> +:040000 040000 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 R100  a/      b/\n> +:100644 000000 c6971ab9f08a6cd9c89a0f87d94ae347aad6144a 0000000000000000000000000000000000000000 D     a/path3\n> +:100644 100644 dbde7141d737c8aa0003672c1bc21ded48c6c3b9 dbde7141d737c8aa0003672c1bc21ded48c6c3b9 R100  a/path2 b/2path\n> +:100644 100644 fdbec444a77953b1bcc899d9fabfa202e5e68f08 4db595d12886f90e36765fc1732c17bccb836663 R093  a/path0 b/path0\n> +:000000 100644 0000000000000000000000000000000000000000 1ba4650885513e62386fd3e23aeb45beeb67d3bb A     b/path100\n\nThere was a recent discussion that came to the conclusion that we\ndon't want to be comparing against hashes outside t0000. So either\ndon't use diff-index, or use 'git rev-parse' to get those objects\nfirst, and then compare against them symbolically (e.g., \":100644\n000000 $(path3) $(nullref)\", or such).\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"152425","messageId":"AANLkTi=S0AkNdajK5MM60HUxHPG1R=Tp_iL8S8Ds4P+j@mail.gmail.com","threadId":"25322","inReplyTo":"1286138529-6780-4-git-send-email-ydirson@altern.org","subject":"Re: [PATCH v4 3/4] Allow hiding renames of individual files involved in a directory rename.","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2010-10-03T23:06:13Z","receivedAt":"2010-10-03T23:06:13Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Sun, Oct 3, 2010 at 22:42, Yann Dirson <ydirson@altern.org> wrote:\n> This makes it much easier to a human reader to spot content changes\n> in a commit that also moves a whole subtree.\n\nI'd like to use this by default (but only for regular 'git diff', 'git\nlog', etc., not for 'git format-patch') if/when it gets merged, can\nhas config option?\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"152428","messageId":"7vr5g6etzf.fsf@alter.siamese.dyndns.org","threadId":"25322","inReplyTo":"AANLkTi=S0AkNdajK5MM60HUxHPG1R=Tp_iL8S8Ds4P+j@mail.gmail.com","subject":"Re: [PATCH v4 3/4] Allow hiding renames of individual files involved in a directory rename.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-10-03T23:28:52Z","receivedAt":"2010-10-03T23:28:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sverre Rabbelier <srabbelier@gmail.com> writes:\n\n> I'd like to use this by default (but only for regular 'git diff', 'git\n> log', etc., not for 'git format-patch') if/when it gets merged, can\n> has config option?\n\nHaving a configuration variable might be a good idea, as long as there is\na provision to avoid breaking scripted porcelains when the option is set.\n"},{"id":"152443","messageId":"AANLkTinhfxXPqapzdBkSehq9Qxv20=bxq9HwR8kQfPdn@mail.gmail.com","threadId":"25322","inReplyTo":"1286138529-6780-2-git-send-email-ydirson@altern.org","subject":"Re: [PATCH v4 1/4] Introduce wholesame directory move detection in diffcore.","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-10-04T02:59:20Z","receivedAt":"2010-10-04T02:59:20Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Sun, Oct 3, 2010 at 20:42, Yann Dirson <ydirson@altern.org> wrote:\n> This feature tries to group together files moving from and to\n> identical directories - the most common case being directory renames.\n\nYou should change the C++/C99 comments to C89 comments since we target\nthe latter.\n"},{"id":"152444","messageId":"AANLkTi=B-aJJ1Q3grmea_YWQCgBkaHoYj0Rym4rqY19C@mail.gmail.com","threadId":"25322","inReplyTo":"1286138529-6780-3-git-send-email-ydirson@altern.org","subject":"Re: [PATCH v4 2/4] Add testcases for the --detect-dir-renames diffcore flag.","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2010-10-04T03:03:35Z","receivedAt":"2010-10-04T03:03:35Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Sun, Oct 3, 2010 at 20:42, Yann Dirson <ydirson@altern.org> wrote:\n> From: Yann Dirson <ydirson@free.fr>\n>\n> This notably includes a couple of tests for cases known not to be\n> working correctly yet.\n> ---\n>  t/t4046-diff-rename-factorize.sh |  326 ++++++++++++++++++++++++++++++++++++++\n>  1 files changed, 326 insertions(+), 0 deletions(-)\n>  create mode 100755 t/t4046-diff-rename-factorize.sh\n>\n> diff --git a/t/t4046-diff-rename-factorize.sh b/t/t4046-diff-rename-factorize.sh\n> new file mode 100755\n> index 0000000..51b0b0b\n> --- /dev/null\n> +++ b/t/t4046-diff-rename-factorize.sh\n> @@ -0,0 +1,326 @@\n> +#!/bin/sh\n> +#\n> +# Copyright (c) 2008,2010 Yann Dirson\n> +# Copyright (c) 2005 Junio C Hamano\n> +#\n> +\n> +# TODO - missing tests:\n> +# * two dirs or more moving all their files to a single dir\n> +# * simultaneous bulkmove and rename\n> +# * add a new file under a dir that was moved in the same commit\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 \\\n> +    'commit the index.'  \\\n> +    'git update-ref HEAD $(echo \"original empty commit\" | git commit-tree $(git write-tree))'\n\nSince this is a new test it should use the nominal style:\n\n    test_expect_success 'commit the index.' '\n        git update-ref HEAD $(echo \"original empty commit\" | git\ncommit-tree $(git write-tree))\n    '\n\n> +mkdir a\n> +echo >a/path0 'Line 1\n> +Line 2\n> +Line 3\n> +Line 4\n> +Line 5\n> +Line 6\n> +Line 7\n> +Line 8\n> +Line 9\n> +Line 10\n> +line 11\n> +Line 12\n> +Line 13\n> +Line 14\n> +Line 15\n> +'\n> +sed <a/path0 >a/path1 s/Line/Record/\n> +sed <a/path0 >a/path2 s/Line/Stuff/\n> +sed <a/path0 >a/path3 s/Line/Blurb/\n\nShould be in a test, see \"Put all code inside test_expect_success and\nother assertions.\" in t/README.\n\n> +test_expect_success \\\n> +    'update-index --add file inside a directory.' \\\n> +    'git update-index --add a/path*'\n> +\n> +test_expect_success \\\n> +    'commit the index.'  \\\n> +    'git update-ref HEAD $(echo \"original set of files\" | git commit-tree $(git write-tree))'\n> +\n> +mv a b\n\nlikewise here, and throughout the test of the test.\n"},{"id":"152463","messageId":"20101004062054.GI24884@burratino","threadId":"25322","inReplyTo":"1286138529-6780-1-git-send-email-ydirson@altern.org","subject":"Re: [RFC PATCH v4 0/4] Detection of directory renames","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-10-04T06:20:54Z","receivedAt":"2010-10-04T06:20:54Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Yann Dirson wrote:\n\n> At last a reroll of that old series.\n\nv3: http://thread.gmane.org/gmane.comp.version-control.git/99780\nv2: http://thread.gmane.org/gmane.comp.version-control.git/99529\nv1: http://thread.gmane.org/gmane.comp.version-control.git/94612\n\nIn particular, from the cover letter to v1:\n\n| I often found myself lost when looking at a diff where a couple of\n| large dirs where renamed, and a handful of files were modified to take\n| the rename into account - not a rare situation, I'd say.  In such a\n| case, the diffs themselves are mostly hidden among numerous rename\n| entries.\n| \n| To that, I felt that git ought to know better, and could easily\n| present a directory rename as such.\n\nYes!\n\n> I hope we can turn the simple detection feature to something\n> acceptable for inclusion - that is, once unified diff output shows the\n> detected renames as annotations, and once the known major holes are\n> plugged (both from FIXME and testcases).\n\nOkay, I look forward to learning the output format.\n"},{"id":"152466","messageId":"AANLkTinw-ZH-RcqWaHY8pNJszc0oTrzdLarenn+FzMXw@mail.gmail.com","threadId":"25322","inReplyTo":"7vr5g6etzf.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v4 3/4] Allow hiding renames of individual files involved in a directory rename.","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2010-10-04T06:43:26Z","receivedAt":"2010-10-04T06:43:26Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Mon, Oct 4, 2010 at 01:28, Junio C Hamano <gitster@pobox.com> wrote:\n> Having a configuration variable might be a good idea, as long as there is\n> a provision to avoid breaking scripted porcelains when the option is set.\n\nI agree... maybe we need a generic way to have a config that only\naffects porcelain?\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"152481","messageId":"20101004072850.GJ24884@burratino","threadId":"25322","inReplyTo":"1286138529-6780-2-git-send-email-ydirson@altern.org","subject":"Re: [PATCH v4 1/4] Introduce wholesame directory move detection in diffcore.","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-10-04T07:28:50Z","receivedAt":"2010-10-04T07:28:50Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"(+cc: Duy and Baz, who commented before)\n\nYann Dirson wrote:\n\n> This feature tries to group together files moving from and to\n> identical directories - the most common case being directory renames.\n> \n> It is activated by the new --detect-dir-renames diffcore\n> flag.\n> \n> This is only the first step, adding the basic functionnality and\n> adding support to raw diff output (and it breaks unified-diff output\n> which does not know how to handle that stuff yet).\n[...]\n>                                                For now both the result\n> of \"mv a b\" and \"mv a/* b/\" are displayed as \"Rnnn a/ b/\", which is\n> probably not what we want.  \"Rnnn a/* b/\" could be a good choice for\n> the latter if we want them to be distinguished, and even if we want\n> them to look the same.\n\nExample?\n\n> Other future developements to be made on top of this include:\n> * extension of unified-diff format to express this\n[...]\n> ---\n\nOh, so this is for diff --raw only.\n\nFor the confused: the discussion from v3 perhaps explains why.  But:\n\n 1. Just like --word-diff, this could be used as a user-facing feature\n    for diffs that cannot be applied\n\n 2. Even if no one agrees on the proper diff format, it would be good\n    plumbing to have.\n\nv3: http://thread.gmane.org/gmane.comp.version-control.git/99780/focus=99782\nv2: http://thread.gmane.org/gmane.comp.version-control.git/99529/focus=99528\nv1: http://thread.gmane.org/gmane.comp.version-control.git/94612/focus=96807\n\nI kind of like the cover letter to v1:\n\n The idea is to add a new pass to diffcore-rename, to group file renames\n looking like a full directory move, and then to hide those file\n renames which do not carry any additionnal information.\n\n Here is a sample run:\n\n $ ./git-diff-tree --abbrev=6 ee491 --factorize-renames -r \n [DBG] possible rename from arm/ to bar/\n [DBG] possible rename from ppc/ to moved/\n [DBG] discarding dir rename of arm/, mixing moved/ and bar/\n [DBG] ppc/* -> moved/* makes ppc/sha1ppc.S -> moved/sha1ppc.S uninteresting\n [DBG] ppc/* -> moved/* makes ppc/sha1.c -> moved/sha1.c uninteresting\n ee491a42190ec6e716f46a55fa0a7f4e307f1629\n :040000 040000 000000... 000000... R100\tppc/\tmoved/\n :100644 100644 9e3ae0... 9e3ae0... R100\tarm/sha1.c\tbar/sha1.c\n :100644 100644 395264... 395264... R100\tarm/sha1.h\tbar/sha1.h\n :100644 100644 c3c51a... c065ee... R099\tppc/sha1.h\tmoved/sha1.h\n :100644 100644 8c1cb9... 8c1cb9... R100\tarm/sha1_arm.S\tmoved/sha1_arm.S\n\nPresumably this patch takes care of the first step (grouping potential\nfull-directory moves) and not the second (hiding the redundant file renames).\n\nNaïve review:\n\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_DIR_RENAMES))\n>  \t\t\t\tcontinue;\n\nHm, why?\n\n> @@ -338,7 +339,8 @@ static int show_modified(struct rev_info *revs,\n[...]\n\nLikewise.\n\n[...]\n> --- a/diffcore-rename.c\n> +++ b/diffcore-rename.c\n> @@ -11,6 +11,7 @@\n>  static struct diff_rename_dst {\n>  \tstruct diff_filespec *two;\n>  \tstruct diff_filepair *pair;\n> +\tint i_am_not_single:1; // does not look for a match, only here to be looked at\n>  } *rename_dst;\n\nWhat does single mean?\n\n> @@ -49,9 +50,36 @@ static struct diff_rename_dst *locate_rename_dst(struct diff_filespec *two,\n[...]\n>  \n> +static struct diff_rename_dst *locate_rename_dst_dir(struct diff_filespec *dir)\n> +{\n> +\t/* code mostly duplicated from locate_rename_dst - not sure we\n> +\t * could merge them efficiently,though\n> +\t */\n> +\tint first, last;\n> +\tint prefixlength = strlen(dir->path);\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(dir->path, 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\nBinary search --- this is just a way to index into the sorted list\nof rename_dsts, right?\n\nAt first I thought it was searching for a good rename destination.\nA comment (or overview in the log message) could help clarify.\n\n> @@ -386,8 +414,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\nWhat is this code path for?  (Sorry for the tedious questions.  My\nthinking is, if I cannot answer them without doing some legwork, how\ncould the person about to break your code who is only glancing for\na moment before plunging forward on an exciting new feature?)\n\n> @@ -414,6 +445,180 @@ static void record_if_better(struct diff_score m[], struct diff_score *o)\n>  \t\tm[worst] = *o;\n>  }\n>  \n> +struct diff_dir_rename {\n> +\tstruct diff_filespec *one;\n> +\tstruct diff_filespec *two;\n> +\tint discarded;\n> +\tstruct diff_dir_rename* next;\n> +};\n\nOkay.\n\n> +// FIXME: prevent possible overflow\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> + */\n> +static const char* copy_dirname(char* dst, const char* src)\n> +{\n> +\tchar* lastslash = strrchr(src, '/');\n> +\tif (!lastslash) {\n> +\t\t*dst = '\\0';\n> +\t\treturn dst;\n> +\t}\n> +\tstrncpy(dst, src, lastslash - src + 1);\n\nmemcpy?\n\n> +\tdst[lastslash - src + 1] = '\\0';\n> +\n> +\t// if src ends with a \"/\" strip the last component\n> +\tif (lastslash[1] == '\\0') {\n> +\t\tlastslash = strrchr(dst, '/');\n> +\t\tif (!lastslash)\n> +\t\t\treturn strcpy(dst, \".\");\n> +\t\tlastslash[1] = '\\0';\n> +\t}\n\nIt might be easier to read like this:\n\n/* Write name of the parent directory of src to dest. */\nstatic char *remove_last_component(char *dst, const char *src)\n{\n\tsize_t len = strlen(src);\n\tconst char *slash;\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\t*mempcpy(dst, src, slash - src) = '\\0';\n\treturn dst;\n}\n\nRequires the glibc-specific function memrchr(), but that is easy to\nimplement.  Compare strchrnul() [in git-compat-util.h].\n\n[...]\n> +static struct diff_dir_rename* factorization_candidates = NULL;\n> +static void diffcore_factorize_renames(void)\n\nMaybe this could be refactored into multiple functions?\n\n> @@ -451,13 +656,22 @@ void diffcore_rename(struct diff_options *options)\n>  \t\t\t\tp->one->rename_used++;\n>  \t\t\tregister_rename_src(p->one, p->score);\n>  \t\t}\n> +\t\telse {\n[...]\n> +\t\t\tif (DIFF_OPT_TST(options, DETECT_DIR_RENAMES)) {\n> +\t\t\t\t/* similarly, rename factorization 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\tlocate_rename_dst(p->two, 1)->i_am_not_single = 1;\n\nHuh?\n\n> @@ -509,6 +723,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\nOh, not single means \"not seeking a new relationship\".\n\n> @@ -569,7 +785,28 @@ 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_DIR_RENAMES))\n> +\t\tdiffcore_factorize_renames();\n\nHuh?  Factorize?\n\n>  \tDIFF_QUEUE_CLEAR(&outq);\n> +\n> +\t// Now turn non-discarded factorization_candidates into real renames\n> +\tstruct diff_dir_rename* candidate;\n> +\tfor (candidate=factorization_candidates; candidate; candidate = candidate->next) {\n> +\t\tstruct diff_filepair* pair;\n> +\t\tif (candidate->discarded) continue;\n> +\t\t// visualize toplevel dir if needed - FIXME: wrong place for this ?\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}\n\nLooks like I'll need to read diffcore_factorize_renames() after all. :(\n\nSorry, not much to say yet.  Hopefully some of that can be useful\nnonetheless.\n\nJonathan\n"},{"id":"152559","messageId":"20101004181929.GS4983@home.lan","threadId":"25322","inReplyTo":"AANLkTinhfxXPqapzdBkSehq9Qxv20=bxq9HwR8kQfPdn@mail.gmail.com","subject":"Re: [PATCH v4 1/4] Introduce wholesame directory move detection in diffcore.","fromName":"Yann Dirson","fromEmail":"ydirson@free.fr","sentAt":"2010-10-04T18:19:29Z","receivedAt":"2010-10-04T18:19:29Z","isPatch":true,"sender":{"key":"ydirson@free.fr","avatar":null},"body":"On Mon, Oct 04, 2010 at 02:59:20AM +0000, ?var Arnfj?r? Bjarmason wrote:\n> On Sun, Oct 3, 2010 at 20:42, Yann Dirson <ydirson@altern.org> wrote:\n> > This feature tries to group together files moving from and to\n> > identical directories - the most common case being directory renames.\n> \n> You should change the C++/C99 comments to C89 comments since we target\n> the latter.\n\nRight.  Most of them are here as reminders to either fix something or\nbe sure fixing it was considered before submission for inclusion -\nhopefully using unacceptable syntax helps here :)\n"},{"id":"152561","messageId":"20101004182136.GT4983@home.lan","threadId":"25322","inReplyTo":"AANLkTi=S0AkNdajK5MM60HUxHPG1R=Tp_iL8S8Ds4P+j@mail.gmail.com","subject":"Re: [PATCH v4 3/4] Allow hiding renames of individual files involved in a directory rename.","fromName":"Yann Dirson","fromEmail":"ydirson@free.fr","sentAt":"2010-10-04T18:21:36Z","receivedAt":"2010-10-04T18:21:36Z","isPatch":true,"sender":{"key":"ydirson@free.fr","avatar":null},"body":"On Mon, Oct 04, 2010 at 01:06:13AM +0200, Sverre Rabbelier wrote:\n> Heya,\n> \n> On Sun, Oct 3, 2010 at 22:42, Yann Dirson <ydirson@altern.org> wrote:\n> > This makes it much easier to a human reader to spot content changes\n> > in a commit that also moves a whole subtree.\n> \n> I'd like to use this by default (but only for regular 'git diff', 'git\n> log', etc., not for 'git format-patch') if/when it gets merged, can\n> has config option?\n\nSure - I was leaving that sort of beels and whistles for after the\ncore feature gets cold enough.\n"},{"id":"152562","messageId":"20101004183202.GU4983@home.lan","threadId":"25322","inReplyTo":"AANLkTi=B-aJJ1Q3grmea_YWQCgBkaHoYj0Rym4rqY19C@mail.gmail.com","subject":"Re: [PATCH v4 2/4] Add testcases for the --detect-dir-renames diffcore flag.","fromName":"Yann Dirson","fromEmail":"ydirson@free.fr","sentAt":"2010-10-04T18:32:02Z","receivedAt":"2010-10-04T18:32:02Z","isPatch":true,"sender":{"key":"ydirson@free.fr","avatar":null},"body":"On Mon, Oct 04, 2010 at 03:03:35AM +0000, ?var Arnfj?r? Bjarmason wrote:\n> > +mkdir a\n[...]\n> > +sed <a/path0 >a/path3 s/Line/Blurb/\n> \n> Should be in a test, see \"Put all code inside test_expect_success and\n> other assertions.\" in t/README.\n\nWhile I can see the value of having everything run from the test\nframework, I was thinking it would be even better to have a special\n\"test_setup\" clause, which could be as a first step just an alias to\ntest_expect_success, but could afterwards gain features such as \"not\ncounted as success, and counted specially when failed\", so we get\nbetter stats of the number of failures we get, and so that editor\noutlining/folding modes could be developped in a useful fashion.\n"},{"id":"152587","messageId":"20101004203241.GF6466@burratino","threadId":"25322","inReplyTo":"1286138529-6780-3-git-send-email-ydirson@altern.org","subject":"Re: [PATCH v4 2/4] Add testcases for the --detect-dir-renames diffcore flag.","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-10-04T20:32:41Z","receivedAt":"2010-10-04T20:32:41Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Yann Dirson wrote:\n\n> From: Yann Dirson <ydirson@free.fr>\n> Subject: Add testcases for the --detect-dir-renames diffcore flag\n\nProbably better for the test to be squashed with the patch that adds\nthat option.\n\n> --- /dev/null\n> +++ b/t/t4046-diff-rename-factorize.sh\n> @@ -0,0 +1,326 @@\n[...]\n> +test_expect_success \\\n> +    'commit the index.'  \\\n> +    'git update-ref HEAD $(echo \"original empty commit\" | git commit-tree $(git write-tree))'\n\nNit:\n\nNow test assertions tend to be written as\n\n\ttest_expect_success '...' '\n\t\tcommand &&\n\t\tcommand &&\n\t\t...\n\t'\n\nThe tabs to indent and opening ' at the end of the first line means\nless fuss in lining things up.  Making each test assertion include its\nsetup means tests don't pass if something gets messed up in the setup,\nand using multiline test assertions with && means there is no\nconfusion about what the tests were written to test.\n\nAs you mentioned, this has the unfortunate downside of messing with\nsyntax highlighting.  Maybe the common text editors need a new mode\nfor git-style test scripts?  Or maybe it would make sense to implement\n\n\ttest_begin_expecting_success '...'\n\t...\n\ttest_end\n\n--- it would certainly make quoting easier.\n\nIt might make sense to compute the tree, commit, etc one at a time\ninstead of this long one-liner.\n\n> +\n> +mkdir a\n> +echo >a/path0 'Line 1\n> +Line 2\n> +Line 3\n> +Line 4\n> +Line 5\n> +Line 6\n> +Line 7\n> +Line 8\n> +Line 9\n> +Line 10\n> +line 11\n> +Line 12\n> +Line 13\n> +Line 14\n> +Line 15\n> +'\n\nTo avoid quoting troubles and since these are innocuous commands, it\ncould be nice to put this before the first test assertion.  Or even\nbetter:\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\t...\n\n[...]\n> +test_expect_success \\\n> +    'git diff-index --detect-dir-renames after directory move.' \\\n> +    'git diff-index --detect-dir-renames HEAD >current'\n> +grep -v \"^\\[DBG\\] \" <current >current.filtered\n> +cat >expected <<\\EOF\n> +:040000 040000 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 R100\ta/\tb/\n> +:100644 100644 fdbec444a77953b1bcc899d9fabfa202e5e68f08 fdbec444a77953b1bcc899d9fabfa202e5e68f08 R100\ta/path0\tb/path0\n> +:100644 100644 2f1f8d70c0fdad990819dfe37a31deb010805161 2f1f8d70c0fdad990819dfe37a31deb010805161 R100\ta/path1\tb/path1\n> +:100644 100644 dbde7141d737c8aa0003672c1bc21ded48c6c3b9 dbde7141d737c8aa0003672c1bc21ded48c6c3b9 R100\ta/path2\tb/path2\n> +:100644 100644 c6971ab9f08a6cd9c89a0f87d94ae347aad6144a c6971ab9f08a6cd9c89a0f87d94ae347aad6144a R100\ta/path3\tb/path3\n> +EOF\n\nNit: although compare_diff_patch ensures the result is not dependent\non the hash function, these hard-coded hashes are still hard for a\nhuman to read.  Could they be computed instead?\n\n(Sorry, no thoughts about the actual content yet...)\n---\ndiff --git a/t/t4046-diff-rename-factorize.sh b/t/t4046-diff-rename-factorize.sh\nindex 51b0b0b..1e7badb 100755\n--- a/t/t4046-diff-rename-factorize.sh\n+++ b/t/t4046-diff-rename-factorize.sh\n@@ -15,312 +15,246 @@ test_description='Test rename factorization in diff engine.\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/diff-lib.sh\n \n-test_expect_success \\\n-    'commit the index.'  \\\n-    'git update-ref HEAD $(echo \"original empty commit\" | git commit-tree $(git write-tree))'\n-\n-mkdir a\n-echo >a/path0 'Line 1\n-Line 2\n-Line 3\n-Line 4\n-Line 5\n-Line 6\n-Line 7\n-Line 8\n-Line 9\n-Line 10\n-line 11\n-Line 12\n-Line 13\n-Line 14\n-Line 15\n+test_expect_success 'setup' '\n+\tzeroes=0000000000000000000000000000000000000000 &&\n+\n+\ttree=$(git write-tree) &&\n+\tcommit=$(echo \"original empty commit\" | git commit-tree $tree) &&\n+\tgit update-ref HEAD $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+\ttree=$(git write-tree) &&\n+\tcommit=$(echo \"original set of files\" | git commit-tree $tree) &&\n+\tgit update-ref HEAD $commit &&\n+\n+\tpath0_id=$(git rev-parse HEAD:a/path0) &&\n+\tpath1_id=$(git rev-parse HEAD:a/path1) &&\n+\tpath2_id=$(git rev-parse HEAD:a/path2) &&\n+\tpath3_id=$(git rev-parse HEAD:a/path3) &&\n+\n+\t: rename the directory &&\n+\tmv a b &&\n+\tgit update-index --add --remove a/path0 a/path1 a/path2 a/path3 b/path*\n '\n-sed <a/path0 >a/path1 s/Line/Record/\n-sed <a/path0 >a/path2 s/Line/Stuff/\n-sed <a/path0 >a/path3 s/Line/Blurb/\n-\n-test_expect_success \\\n-    'update-index --add file inside a directory.' \\\n-    'git update-index --add a/path*'\n-\n-test_expect_success \\\n-    'commit the index.'  \\\n-    'git update-ref HEAD $(echo \"original set of files\" | git commit-tree $(git write-tree))'\n-\n-mv a b\n-test_expect_success \\\n-    'renamed the directory.' \\\n-    'git update-index --add --remove a/path0 a/path1 a/path2 a/path3 b/path*'\n-\n-test_expect_success \\\n-    'git diff-index --detect-dir-renames after directory move.' \\\n-    'git diff-index --detect-dir-renames HEAD >current'\n-grep -v \"^\\[DBG\\] \" <current >current.filtered\n-cat >expected <<\\EOF\n-:040000 040000 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 R100\ta/\tb/\n-:100644 100644 fdbec444a77953b1bcc899d9fabfa202e5e68f08 fdbec444a77953b1bcc899d9fabfa202e5e68f08 R100\ta/path0\tb/path0\n-:100644 100644 2f1f8d70c0fdad990819dfe37a31deb010805161 2f1f8d70c0fdad990819dfe37a31deb010805161 R100\ta/path1\tb/path1\n-:100644 100644 dbde7141d737c8aa0003672c1bc21ded48c6c3b9 dbde7141d737c8aa0003672c1bc21ded48c6c3b9 R100\ta/path2\tb/path2\n-:100644 100644 c6971ab9f08a6cd9c89a0f87d94ae347aad6144a c6971ab9f08a6cd9c89a0f87d94ae347aad6144a R100\ta/path3\tb/path3\n-EOF\n-\n-test_expect_success \\\n-    'validate the output for directory move.' \\\n-    'compare_diff_patch expected current.filtered'\n-\n-# now test non-100% renames\n-\n-echo 'Line 16' >> b/path0\n-mv b/path2 b/2path\n-rm b/path3\n-echo anything > b/path100\n-test_expect_success \\\n-    'edited dir contents.' \\\n-    'git update-index --add --remove b/* b/path2 b/path3'\n-\n-test_expect_success \\\n-    'git diff-index --detect-dir-renames after directory move and content changes.' \\\n-    'git diff-index --detect-dir-renames HEAD >current'\n-grep -v \"^\\[DBG\\] \" <current >current.filtered\n-cat >expected <<\\EOF\n-:040000 040000 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 R100\ta/\tb/\n-:100644 000000 c6971ab9f08a6cd9c89a0f87d94ae347aad6144a 0000000000000000000000000000000000000000 D\ta/path3\n-:100644 100644 dbde7141d737c8aa0003672c1bc21ded48c6c3b9 dbde7141d737c8aa0003672c1bc21ded48c6c3b9 R100\ta/path2\tb/2path\n-:100644 100644 fdbec444a77953b1bcc899d9fabfa202e5e68f08 4db595d12886f90e36765fc1732c17bccb836663 R093\ta/path0\tb/path0\n-:100644 100644 2f1f8d70c0fdad990819dfe37a31deb010805161 2f1f8d70c0fdad990819dfe37a31deb010805161 R100\ta/path1\tb/path1\n-:000000 100644 0000000000000000000000000000000000000000 1ba4650885513e62386fd3e23aeb45beeb67d3bb A\tb/path100\n-EOF\n-\n-test_expect_success \\\n-    'validate the output for directory move and content changes.' \\\n-    'compare_diff_patch expected current.filtered'\n-\n-git reset -q --hard\n-\n-# now test bulk moves that are not directory moves (get consensus before going further ?)\n-\n-mkdir c\n-for i in 0 1 2; do cp a/path$i c/apath$i; done\n-test_expect_success \\\n-    'add files into a new directory.' \\\n-    'git update-index --add c/apath*'\n-\n-test_expect_success \\\n-    'commit all this.'  \\\n-    'git commit -m \"first set of changes\"'\n-\n-mv c/* a/\n-test_expect_success \\\n-    'move all of the new dir contents into a preexisting dir.' \\\n-    'git update-index --add --remove a/* c/apath0 c/apath1 c/apath2'\n-\n-test_expect_success \\\n-    'git diff-index --detect-dir-renames without full-dir rename.' \\\n-    'git diff-index --detect-dir-renames HEAD >current'\n-grep -v \"^\\[DBG\\] \" <current >current.filtered\n-cat >expected <<\\EOF\n-:040000 040000 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 R100\tc/*\ta/\n-:100644 100644 fdbec444a77953b1bcc899d9fabfa202e5e68f08 fdbec444a77953b1bcc899d9fabfa202e5e68f08 R100\tc/apath0\ta/apath0\n-:100644 100644 2f1f8d70c0fdad990819dfe37a31deb010805161 2f1f8d70c0fdad990819dfe37a31deb010805161 R100\tc/apath1\ta/apath1\n-:100644 100644 dbde7141d737c8aa0003672c1bc21ded48c6c3b9 dbde7141d737c8aa0003672c1bc21ded48c6c3b9 R100\tc/apath2\ta/apath2\n-EOF\n-\n-test_expect_failure \\\n-    'validate the output for bulk rename without full-dir rename.' \\\n-    'compare_diff_patch expected current.filtered'\n-\n-git reset -q --hard\n-\n-# now test moves to toplevel\n-\n-mv c/* .\n-test_expect_success \\\n-    'move all of the new dir contents into toplevel.' \\\n-    'git update-index --add --remove apath* c/apath0 c/apath1 c/apath2'\n-\n-test_expect_success \\\n-    'git diff-index --detect-dir-renames files bulk-moved to toplevel.' \\\n-    'git diff-index --detect-dir-renames HEAD >current'\n-grep -v \"^\\[DBG\\] \" <current >current.filtered\n-cat >expected <<\\EOF\n-:040000 040000 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 R100\tc/*\t./\n-:100644 100644 fdbec444a77953b1bcc899d9fabfa202e5e68f08 fdbec444a77953b1bcc899d9fabfa202e5e68f08 R100\tc/apath0\tapath0\n-:100644 100644 2f1f8d70c0fdad990819dfe37a31deb010805161 2f1f8d70c0fdad990819dfe37a31deb010805161 R100\tc/apath1\tapath1\n-:100644 100644 dbde7141d737c8aa0003672c1bc21ded48c6c3b9 dbde7141d737c8aa0003672c1bc21ded48c6c3b9 R100\tc/apath2\tapath2\n-EOF\n-\n-test_expect_failure \\\n-    'validate the output for files bulk-moved to toplevel.' \\\n-    'compare_diff_patch expected current.filtered'\n \n-git reset -q --hard\n-\n-# now test renaming with subdirs (does not take subdirs into account)\n-\n-mv c a/\n-test_expect_success \\\n-    'move the new dir as subdir of another.' \\\n-    'git update-index --add --remove a/c/* c/apath0 c/apath1 c/apath2'\n-\n-test_expect_success \\\n-    'commit all this.'  \\\n-    'git commit -m \"move as subdir\"'\n-\n-mv a b\n-echo foo >> b/c/apath0\n-test_expect_success \\\n-    'rename the directory with some changes.' \\\n-    'git update-index --add --remove a/path0 a/path1 a/path2 a/path3 a/c/apath0 a/c/apath1 a/c/apath2 b/path* b/c/apath*'\n-\n-test_expect_success \\\n-    'git diff-index --detect-dir-renames on a move including a subdir.' \\\n-    'git diff-index --detect-dir-renames HEAD >current'\n-grep -v \"^\\[DBG\\] \" <current >current.filtered\n-cat >expected <<\\EOF\n-:040000 040000 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 R100\ta/\tb/\n-:100644 100644 fdbec444a77953b1bcc899d9fabfa202e5e68f08 00084e5ea68b5ae339b7c4b429e4a70fe25d069b R096\ta/c/apath0\tb/c/apath0\n-:100644 100644 2f1f8d70c0fdad990819dfe37a31deb010805161 2f1f8d70c0fdad990819dfe37a31deb010805161 R100\ta/c/apath1\tb/c/apath1\n-:100644 100644 dbde7141d737c8aa0003672c1bc21ded48c6c3b9 dbde7141d737c8aa0003672c1bc21ded48c6c3b9 R100\ta/c/apath2\tb/c/apath2\n-:100644 100644 fdbec444a77953b1bcc899d9fabfa202e5e68f08 fdbec444a77953b1bcc899d9fabfa202e5e68f08 R100\ta/path0\tb/path0\n-:100644 100644 2f1f8d70c0fdad990819dfe37a31deb010805161 2f1f8d70c0fdad990819dfe37a31deb010805161 R100\ta/path1\tb/path1\n-:100644 100644 dbde7141d737c8aa0003672c1bc21ded48c6c3b9 dbde7141d737c8aa0003672c1bc21ded48c6c3b9 R100\ta/path2\tb/path2\n-:100644 100644 c6971ab9f08a6cd9c89a0f87d94ae347aad6144a c6971ab9f08a6cd9c89a0f87d94ae347aad6144a R100\ta/path3\tb/path3\n-EOF\n-\n-test_expect_failure \\\n-    'validate the output for a move including a subdir.' \\\n-    'compare_diff_patch expected current.filtered'\n+test_expect_success 'diff-index --detect-dir-renames after directory move.' '\n+\tcat >expected <<-EOF &&\n+\t:040000 040000 $zeroes $zeroes R100\ta/\tb/\n+\t:100644 100644 $path0_id $path0_id R100\ta/path0\tb/path0\n+\t:100644 100644 $path1_id $path1_id R100\ta/path1\tb/path1\n+\t:100644 100644 $path2_id $path2_id R100\ta/path2\tb/path2\n+\t:100644 100644 $path3_id $path3_id R100\ta/path3\tb/path3\n+\tEOF\n+\tgit diff-index --detect-dir-renames HEAD >current &&\n+\tgrep -v \"^\\[DBG\\] \" <current >current.filtered &&\n+\tcompare_diff_patch expected current.filtered\n+'\n \n-git reset -q --hard\n+test_expect_success 'setup non-100% rename' '\n+\techo \"Line 16\" >>b/path0 &&\n+\tmv b/path2 b/2path &&\n+\trm b/path3 &&\n+\techo anything >b/path100 &&\n+\tgit update-index --add --remove b/* b/path2 b/path3\n+'\n \n-# now check that moving all files but not subdirs is not mistaken for dir move\n+test_expect_success 'diff-index --detect-dir-renames after content changes.' '\n+\tpath0_id2=$(git rev-parse :b/path0) &&\n+\tpath100_id=$(git rev-parse :b/path100) &&\n+\tcat >expected <<-EOF &&\n+\t:040000 040000 $zeroes $zeroes R100\ta/\tb/\n+\t:100644 000000 $path3_id $zeroes D\ta/path3\n+\t:100644 100644 $path2_id $path2_id R100\ta/path2\tb/2path\n+\t:100644 100644 $path0_id $path0_id2 R093\ta/path0\tb/path0\n+\t:100644 100644 $path1_id $path1_id R100\ta/path1\tb/path1\n+\t:000000 100644 $zeroes $path100_id A\tb/path100\n+\tEOF\n+\tgit diff-index --detect-dir-renames HEAD >current &&\n+\tgrep -v \"^\\[DBG\\] \" <current >current.filtered &&\n+\tcompare_diff_patch expected current.filtered\n+'\n \n-mkdir b\n-mv a/path* b/\n-test_expect_success \\\n-    'rename files in the directory but not subdir.' \\\n-    'git update-index --add --remove a/path0 a/path1 a/path2 a/path3 b/path*'\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\n+\t\tdo\n+\t\t\tcp a/path$i c/apath$i ||\n+\t\t\texit\n+\t\tdone\n+\t) &&\n+\tgit update-index --add c/apath* &&\n+\tgit commit -m \"first set of changes\" &&\n+\n+\tmv c/* a/ &&\n+\tgit update-index --add --remove a/* c/apath0 c/apath1 c/apath2\n+'\n \n-test_expect_success \\\n-    'git diff-index --detect-dir-renames on a move without a subdir.' \\\n-    'git diff-index --detect-dir-renames HEAD >current'\n-grep -v \"^\\[DBG\\] \" <current >current.filtered\n-cat >expected <<\\EOF\n-:100644 100644 fdbec444a77953b1bcc899d9fabfa202e5e68f08 fdbec444a77953b1bcc899d9fabfa202e5e68f08 R100\ta/path0\tb/path0\n-:100644 100644 2f1f8d70c0fdad990819dfe37a31deb010805161 2f1f8d70c0fdad990819dfe37a31deb010805161 R100\ta/path1\tb/path1\n-:100644 100644 dbde7141d737c8aa0003672c1bc21ded48c6c3b9 dbde7141d737c8aa0003672c1bc21ded48c6c3b9 R100\ta/path2\tb/path2\n-:100644 100644 c6971ab9f08a6cd9c89a0f87d94ae347aad6144a c6971ab9f08a6cd9c89a0f87d94ae347aad6144a R100\ta/path3\tb/path3\n-EOF\n+test_expect_failure 'diff-index --detect-dir-renames without full-dir rename.' '\n+\tcat >expected <<-EOF &&\n+\t:040000 040000 $zeroes $zeroes R100\tc/*\ta/\n+\t:100644 100644 $path0_id $path0_id R100\tc/apath0\ta/apath0\n+\t:100644 100644 $path1_id $path1_id R100\tc/apath1\ta/apath1\n+\t:100644 100644 $path2_id $path2_id R100\tc/apath2\ta/apath2\n+\tEOF\n+\tgit diff-index --detect-dir-renames HEAD >current\n+\tgrep -v \"^\\[DBG\\] \" <current >current.filtered &&\n+\tcompare_diff_patch expected current.filtered\n+'\n \n-test_expect_success \\\n-    'validate the output for a move without a subdir.' \\\n-    'compare_diff_patch expected current.filtered'\n+test_expect_success 'setup bulk move to toplevel' '\n+\tgit reset -q --hard &&\n+\tmv c/* . &&\n+\tgit update-index --add --remove apath* c/apath0 c/apath1 c/apath2\n+'\n \n-git reset -q --hard\n+test_expect_failure 'diff-index --detect-dir-renames bulk move to toplevel.' '\n+\tcat >expected <<-EOF &&\n+\t:040000 040000 $zeroes $zeroes R100\tc/*\t./\n+\t:100644 100644 $path0_id $path0_id R100\tc/apath0\tapath0\n+\t:100644 100644 $path1_id $path1_id R100\tc/apath1\tapath1\n+\t:100644 100644 $path2_id $path2_id R100\tc/apath2\tapath2\n+\tEOF\n+\tgit diff-index --detect-dir-renames HEAD >current &&\n+\tgrep -v \"^\\[DBG\\] \" <current >current.filtered &&\n+\tcompare_diff_patch expected current.filtered\n+'\n \n-# now check that moving subdirs into one dir and files into another is not mistaken for dir move\n-# (well, clearly it is ...)\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+\tgit commit -m \"move as subdir\" &&\n \n-mv a/c b\n-mv a d\n-test_expect_success \\\n-    'rename subdir and files into different places.' \\\n-    'git update-index --add --remove a/path0 a/path1 a/path2 a/path3 a/c/apath0 a/c/apath1 a/c/apath2 d/path* b/apath*'\n+\tmv a b &&\n+\techo foo >>b/c/apath0 &&\n+\tgit update-index --add --remove a/path0 a/path1 a/path2 a/path3 \\\n+\t\t\ta/c/apath0 a/c/apath1 a/c/apath2 b/path* b/c/apath*\n+'\n \n-test_expect_success \\\n-    'git diff-index --detect-dir-renames on a split of subdir and files into different places.' \\\n-    'git diff-index --detect-dir-renames HEAD >current'\n-grep -v \"^\\[DBG\\] \" <current >current.filtered\n-cat >expected <<\\EOF\n-:040000 040000 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 R100\ta/c/\tb/\n-:100644 100644 fdbec444a77953b1bcc899d9fabfa202e5e68f08 fdbec444a77953b1bcc899d9fabfa202e5e68f08 R100\ta/c/apath0\tb/apath0\n-:100644 100644 2f1f8d70c0fdad990819dfe37a31deb010805161 2f1f8d70c0fdad990819dfe37a31deb010805161 R100\ta/c/apath1\tb/apath1\n-:100644 100644 dbde7141d737c8aa0003672c1bc21ded48c6c3b9 dbde7141d737c8aa0003672c1bc21ded48c6c3b9 R100\ta/c/apath2\tb/apath2\n-:100644 100644 fdbec444a77953b1bcc899d9fabfa202e5e68f08 fdbec444a77953b1bcc899d9fabfa202e5e68f08 R100\ta/path0\td/path0\n-:100644 100644 2f1f8d70c0fdad990819dfe37a31deb010805161 2f1f8d70c0fdad990819dfe37a31deb010805161 R100\ta/path1\td/path1\n-:100644 100644 dbde7141d737c8aa0003672c1bc21ded48c6c3b9 dbde7141d737c8aa0003672c1bc21ded48c6c3b9 R100\ta/path2\td/path2\n-:100644 100644 c6971ab9f08a6cd9c89a0f87d94ae347aad6144a c6971ab9f08a6cd9c89a0f87d94ae347aad6144a R100\ta/path3\td/path3\n-EOF\n+test_expect_failure 'diff-index --detect-dir-renames on a move including a subdir.' '\n+\tpath0_id2=$(git rev-parse :b/c/apath0) &&\n+\tcat >expected <<-EOF &&\n+\t:040000 040000 $zeroes $zeroes R100\ta/\tb/\n+\t:100644 100644 $path0_id $path0_id2 R096\ta/c/apath0\tb/c/apath0\n+\t:100644 100644 $path1_id $path1_id R100\ta/c/apath1\tb/c/apath1\n+\t:100644 100644 $path2_id $path2_id R100\ta/c/apath2\tb/c/apath2\n+\t:100644 100644 $path0_id $path0_id R100\ta/path0\tb/path0\n+\t:100644 100644 $path1_id $path1_id R100\ta/path1\tb/path1\n+\t:100644 100644 $path2_id $path2_id R100\ta/path2\tb/path2\n+\t:100644 100644 $path3_id $path3_id R100\ta/path3\tb/path3\n+\tEOF\n+\tgit diff-index --detect-dir-renames HEAD >current &&\n+\tgrep -v \"^\\[DBG\\] \" <current >current.filtered &&\n+\tcompare_diff_patch expected current.filtered\n+'\n \n-test_expect_failure \\\n-    'validate the output for a split of subdir and files into different places.' \\\n-    'compare_diff_patch expected current.filtered'\n+test_expect_success 'setup move without a subdir' '\n+\tgit reset -q --hard &&\n+\tmkdir b &&\n+\tmv a/path* b/ &&\n+\t: rename files in the directory but not subdir. &&\n+\tgit update-index --add --remove a/path0 a/path1 a/path2 a/path3 b/path*\n+'\n \n-# now test moving a dir with no files but only subdirs\n-# (only factorizes lowest-level directories - not a big deal, just not perfect)\n+test_expect_success 'moving files but not subdirs is not mistaken for dir move' '\n+\tcat >expected <<-EOF &&\n+\t:100644 100644 $path0_id $path0_id R100\ta/path0\tb/path0\n+\t:100644 100644 $path1_id $path1_id R100\ta/path1\tb/path1\n+\t:100644 100644 $path2_id $path2_id R100\ta/path2\tb/path2\n+\t:100644 100644 $path3_id $path3_id R100\ta/path3\tb/path3\n+\tEOF\n+\tgit diff-index --detect-dir-renames HEAD >current &&\n+\tgrep -v \"^\\[DBG\\] \" <current >current.filtered &&\n+\tcompare_diff_patch expected current.filtered\n+'\n \n-git reset -q --hard\n-mkdir a/b\n-mv a/path* a/b/\n-test_expect_success \\\n-    'setup the directory with only subdirs, no direct child files.' \\\n-    'git update-index --add --remove a/path0 a/path1 a/path2 a/path3 a/b/path*'\n-test_expect_success \\\n-    'commit all this.'  \\\n-    'git commit -m \"move all toplevel files down one level\"'\n+test_expect_success 'setup move of files and subdirs to different places' '\n+\tgit reset -q --hard &&\n+\tmv a/c b &&\n+\tmv a d &&\n+\tgit update-index --add --remove a/path0 a/path1 a/path2 a/path3 \\\n+\t\ta/c/apath0 a/c/apath1 a/c/apath2 d/path* b/apath*\n+'\n \n-test_expect_success \\\n-    'move this dir.'  \\\n-    'git mv a z'\n+test_expect_failure 'moving subdirs into one dir and files into another is not mistaken for dir move' '\n+\tcat >expected <<-EOF &&\n+\t:040000 040000 $zeroes $zeroes R100\ta/c/\tb/\n+\t:100644 100644 $path0_id $path0_id R100\ta/c/apath0\tb/apath0\n+\t:100644 100644 $path1_id $path1_id R100\ta/c/apath1\tb/apath1\n+\t:100644 100644 $path2_id $path2_id R100\ta/c/apath2\tb/apath2\n+\t:100644 100644 $path0_id $path0_id R100\ta/path0\td/path0\n+\t:100644 100644 $path1_id $path1_id R100\ta/path1\td/path1\n+\t:100644 100644 $path2_id $path2_id R100\ta/path2\td/path2\n+\t:100644 100644 $path3_id $path3_id R100\ta/path3\td/path3\n+\tEOF\n+\tgit diff-index --detect-dir-renames HEAD >current &&\n+\tgrep -v \"^\\[DBG\\] \" <current >current.filtered &&\n+\tcompare_diff_patch expected current.filtered\n+'\n \n-test_expect_success \\\n-    'git diff-index --detect-dir-renames with only subdirs' \\\n-    'git diff-index --detect-dir-renames HEAD >current'\n-grep -v \"^\\[DBG\\] \" <current >current.filtered\n-cat >expected <<\\EOF\n-:040000 040000 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 R100\ta/\tz/\n-:040000 040000 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 R100\ta/b/\tz/b/\n-:040000 040000 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 R100\ta/c/\tz/c/\n-:100644 100644 fdbec444a77953b1bcc899d9fabfa202e5e68f08 fdbec444a77953b1bcc899d9fabfa202e5e68f08 R100\ta/b/path0\tz/b/path0\n-:100644 100644 2f1f8d70c0fdad990819dfe37a31deb010805161 2f1f8d70c0fdad990819dfe37a31deb010805161 R100\ta/b/path1\tz/b/path1\n-:100644 100644 dbde7141d737c8aa0003672c1bc21ded48c6c3b9 dbde7141d737c8aa0003672c1bc21ded48c6c3b9 R100\ta/b/path2\tz/b/path2\n-:100644 100644 c6971ab9f08a6cd9c89a0f87d94ae347aad6144a c6971ab9f08a6cd9c89a0f87d94ae347aad6144a R100\ta/b/path3\tz/b/path3\n-:100644 100644 fdbec444a77953b1bcc899d9fabfa202e5e68f08 fdbec444a77953b1bcc899d9fabfa202e5e68f08 R100\ta/c/apath0\tz/c/apath0\n-:100644 100644 2f1f8d70c0fdad990819dfe37a31deb010805161 2f1f8d70c0fdad990819dfe37a31deb010805161 R100\ta/c/apath1\tz/c/apath1\n-:100644 100644 dbde7141d737c8aa0003672c1bc21ded48c6c3b9 dbde7141d737c8aa0003672c1bc21ded48c6c3b9 R100\ta/c/apath2\tz/c/apath2\n-EOF\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+\tgit commit -m \"move all toplevel files down one level\" &&\n \n-test_expect_failure \\\n-    'validate the output for a move with only subdirs.' \\\n-    'compare_diff_patch expected current.filtered'\n+\tgit mv a z\n+'\n \n+test_expect_failure 'moving a dir with no files' '\n+\tcat >expected <<-EOF &&\n+\t:040000 040000 $zeroes $zeroes R100\ta/\tz/\n+\t:040000 040000 $zeroes $zeroes R100\ta/b/\tz/b/\n+\t:040000 040000 $zeroes $zeroes R100\ta/c/\tz/c/\n+\t:100644 100644 $path0_id $path0_id R100\ta/b/path0\tz/b/path0\n+\t:100644 100644 $path1_id $path1_id R100\ta/b/path1\tz/b/path1\n+\t:100644 100644 $path2_id $path2_id R100\ta/b/path2\tz/b/path2\n+\t:100644 100644 $path3_id $path3_id R100\ta/b/path3\tz/b/path3\n+\t:100644 100644 $path0_id $path0_id R100\ta/c/apath0\tz/c/apath0\n+\t:100644 100644 $path1_id $path1_id R100\ta/c/apath1\tz/c/apath1\n+\t:100644 100644 $path2_id $path2_id R100\ta/c/apath2\tz/c/apath2\n+\tEOF\n+\tgit diff-index --detect-dir-renames HEAD >current &&\n+\tgrep -v \"^\\[DBG\\] \" <current >current.filtered &&\n+\tcompare_diff_patch expected current.filtered\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-git reset -q --hard HEAD~3\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+\tgit commit -m \"move all files to toplevel\" &&\n \n-mv a/* .\n-test_expect_success \\\n-    'rename the directory with some changes.' \\\n-    'git update-index --add --remove a/path0 a/path1 a/path2 a/path3 path*'\n+\tmkdir z &&\n+\tmv path* z/ &&\n+\tgit update-index --add --remove path0 path1 path2 path3 z/path*\n+'\n \n-test_expect_success \\\n-    'commit all this.'  \\\n-    'git commit -m \"move all files to toplevel\"'\n-\n-mkdir z\n-mv path* z/\n-test_expect_success \\\n-    'rename the directory with some changes.' \\\n-    'git update-index --add --remove path0 path1 path2 path3 z/path*'\n-\n-test_expect_success \\\n-    'git diff-index --detect-dir-renames everything from toplevel.' \\\n-    'git diff-index --detect-dir-renames HEAD >current'\n-grep -v \"^\\[DBG\\] \" <current >current.filtered\n-cat >expected <<\\EOF\n-:040000 040000 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 R100\t./*\tz/\n-:100644 100644 fdbec444a77953b1bcc899d9fabfa202e5e68f08 fdbec444a77953b1bcc899d9fabfa202e5e68f08 R100\tpath0\tz/path0\n-:100644 100644 2f1f8d70c0fdad990819dfe37a31deb010805161 2f1f8d70c0fdad990819dfe37a31deb010805161 R100\tpath1\tz/path1\n-:100644 100644 dbde7141d737c8aa0003672c1bc21ded48c6c3b9 dbde7141d737c8aa0003672c1bc21ded48c6c3b9 R100\tpath2\tz/path2\n-:100644 100644 c6971ab9f08a6cd9c89a0f87d94ae347aad6144a c6971ab9f08a6cd9c89a0f87d94ae347aad6144a R100\tpath3\tz/path3\n-EOF\n-\n-test_expect_failure \\\n-    'validate the output for a move of everything from toplevel.' \\\n-    'compare_diff_patch expected current.filtered'\n+test_expect_failure '--detect-dir-renames everything from toplevel.' '\n+\tcat >expected <<-EOF &&\n+\t:040000 040000 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 R100\t./*\tz/\n+\t:100644 100644 $path0_id $path0_id R100\tpath0\tz/path0\n+\t:100644 100644 $path1_id $path1_id R100\tpath1\tz/path1\n+\t:100644 100644 $path2_id $path2_id R100\tpath2\tz/path2\n+\t:100644 100644 $path3_id $path3_id R100\tpath3\tz/path3\n+\tEOF\n+\tgit diff-index --detect-dir-renames HEAD >current &&\n+\tgrep -v \"^\\[DBG\\] \" <current >current.filtered &&\n+\tcompare_diff_patch expected current.filtered\n+'\n \n test_done\n-- \n"},{"id":"152595","messageId":"20101004211359.GV4983@home.lan","threadId":"25322","inReplyTo":"20101004072850.GJ24884@burratino","subject":"Re: [PATCH v4 1/4] Introduce wholesame directory move detection in diffcore.","fromName":"Yann Dirson","fromEmail":"ydirson@free.fr","sentAt":"2010-10-04T21:13:59Z","receivedAt":"2010-10-04T21:13:59Z","isPatch":true,"sender":{"key":"ydirson@free.fr","avatar":null},"body":"On Mon, Oct 04, 2010 at 02:28:50AM -0500, Jonathan Nieder wrote:\n> > of \"mv a b\" and \"mv a/* b/\" are displayed as \"Rnnn a/ b/\", which is\n> > probably not what we want.  \"Rnnn a/* b/\" could be a good choice for\n> > the latter if we want them to be distinguished, and even if we want\n> > them to look the same.\n> \n> Example?\n\nIt's just that every time we detect a bulk rename (of all files of a\ndirectory) it is currently shown as a directory rename, even if they\nall get moved into an existing dir.  Technically, a \"directory rename\"\n(which I'd label \"mv a b\") is a special case of \"bulk rename\" (for\nwhich \"mv a/* b/\" is more correct), where the target directory is\nempty.  Currently the distinction between the two is not done, but\neverything is incorrectly advertized as a rename.  I shall fix that.\n\n\n> > Other future developements to be made on top of this include:\n> > * extension of unified-diff format to express this\n> [...]\n> > ---\n> \n> Oh, so this is for diff --raw only.\n\nRight, but that TODO list has not be revised in this iteration.\n--detect-dir-renames does not need much to gain unified-diff, it is\nmostly --hide-dir-rename-details which needs thougth and work - but as\nit is now targetted at humans only, we can start with whatever seems\nadequate and rewrite/refine as see fit.\n\n\n> For the confused: the discussion from v3 perhaps explains why.\n\nThanks for this work of putting everything into context - I should\nhave done that myself.\n\n\n>  $ ./git-diff-tree --abbrev=6 ee491 --factorize-renames -r \n>  [DBG] possible rename from arm/ to bar/\n>  [DBG] possible rename from ppc/ to moved/\n>  [DBG] discarding dir rename of arm/, mixing moved/ and bar/\n>  [DBG] ppc/* -> moved/* makes ppc/sha1ppc.S -> moved/sha1ppc.S uninteresting\n>  [DBG] ppc/* -> moved/* makes ppc/sha1.c -> moved/sha1.c uninteresting\n>  ee491a42190ec6e716f46a55fa0a7f4e307f1629\n>  :040000 040000 000000... 000000... R100\tppc/\tmoved/\n>  :100644 100644 9e3ae0... 9e3ae0... R100\tarm/sha1.c\tbar/sha1.c\n>  :100644 100644 395264... 395264... R100\tarm/sha1.h\tbar/sha1.h\n>  :100644 100644 c3c51a... c065ee... R099\tppc/sha1.h\tmoved/sha1.h\n>  :100644 100644 8c1cb9... 8c1cb9... R100\tarm/sha1_arm.S\tmoved/sha1_arm.S\n> \n> Presumably this patch takes care of the first step (grouping potential\n> full-directory moves) and not the second (hiding the redundant file renames).\n\nNo, it did hide the two moves labeled as \"uninteresting\" in the debug\ntraces above.  The ones left are one which is part of the dif rename\nbut was also modified, and the other ones (from arm/) are part of a\ndirectory split (see 3rd debug line).\n\n> Naïve review:\n> \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_DIR_RENAMES))\n> >  \t\t\t\tcontinue;\n> \n> Hm, why?\n> > @@ -338,7 +339,8 @@ static int show_modified(struct rev_info *revs,\n> [...]\n> \n> Likewise.\n\nThe code below that (calls to diff_change) was previously only used\nfor FIND_COPIES_HARDER, but we also need to go there for\nDETECT_DIR_RENAMES.  Removing those eg. make test \"validate the output\nfor a move without a subdir.\" fail.  I admit I have not re-analyzed\nthis part recently, those tests were already parts of the first\niterations.\n\n\n> [...]\n> > --- a/diffcore-rename.c\n> > +++ b/diffcore-rename.c\n> > @@ -11,6 +11,7 @@\n> >  static struct diff_rename_dst {\n> >  \tstruct diff_filespec *two;\n> >  \tstruct diff_filepair *pair;\n> > +\tint i_am_not_single:1; // does not look for a match, only here to be looked at\n> >  } *rename_dst;\n> \n> What does single mean?\n\n\"looking for a match\" :)\n\n> > @@ -451,13 +656,22 @@ void diffcore_rename(struct diff_options *options)\n> >  \t\t\t\tp->one->rename_used++;\n> >  \t\t\tregister_rename_src(p->one, p->score);\n> >  \t\t}\n> > +\t\telse {\n> [...]\n> > +\t\t\tif (DIFF_OPT_TST(options, DETECT_DIR_RENAMES)) {\n> > +\t\t\t\t/* similarly, rename factorization 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\tlocate_rename_dst(p->two, 1)->i_am_not_single = 1;\n> \n> Huh?\n\n> > @@ -386,8 +414,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> What is this code path for?  (Sorry for the tedious questions.  My\n> thinking is, if I cannot answer them without doing some legwork, how\n> could the person about to break your code who is only glancing for\n> a moment before plunging forward on an exciting new feature?)\n\nThose are definitely related to the above.  This mimics the\nFIND_COPIES_HARDER behaviour (which is adding untouched files from\n*src* tree to the rename_dst list so the -C algorithm works): here we\nneed to see all files from *dst* tree so we can tell if there was any\nfile left in a dir after a move.  This has a cost: we must ensure in\nother places that codepaths not intended for those extra files are ok.\n\nHm, thinking quickly about it, maybe we could use a separate list to\nstore them, and only run through them when explicitely needed, that\nwould cost less and avoid the polutions you pointed at.  I shall have\na look at that.\n\n\n> > @@ -49,9 +50,36 @@ static struct diff_rename_dst *locate_rename_dst(struct diff_filespec *two,\n> [...]\n> >  \n> > +static struct diff_rename_dst *locate_rename_dst_dir(struct diff_filespec *dir)\n> > +{\n> > +\t/* code mostly duplicated from locate_rename_dst - not sure we\n> > +\t * could merge them efficiently,though\n> > +\t */\n> > +\tint first, last;\n> > +\tint prefixlength = strlen(dir->path);\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(dir->path, 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> Binary search --- this is just a way to index into the sorted list\n> of rename_dsts, right?\n\nright.\n\n> At first I thought it was searching for a good rename destination.\n> A comment (or overview in the log message) could help clarify.\n\nOk.  More comments are surely good in this hairy piece of code :)\n\n\n> > +// FIXME: prevent possible overflow\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> > + */\n> > +static const char* copy_dirname(char* dst, const char* src)\n> > +{\n> > +\tchar* lastslash = strrchr(src, '/');\n> > +\tif (!lastslash) {\n> > +\t\t*dst = '\\0';\n> > +\t\treturn dst;\n> > +\t}\n> > +\tstrncpy(dst, src, lastslash - src + 1);\n> \n> memcpy?\n\nright\n\n> > +\tdst[lastslash - src + 1] = '\\0';\n> > +\n> > +\t// if src ends with a \"/\" strip the last component\n> > +\tif (lastslash[1] == '\\0') {\n> > +\t\tlastslash = strrchr(dst, '/');\n> > +\t\tif (!lastslash)\n> > +\t\t\treturn strcpy(dst, \".\");\n> > +\t\tlastslash[1] = '\\0';\n> > +\t}\n> \n> It might be easier to read like this:\n> \n> /* Write name of the parent directory of src to dest. */\n> static char *remove_last_component(char *dst, const char *src)\n> {\n> \tsize_t len = strlen(src);\n> \tconst char *slash;\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> \t*mempcpy(dst, src, slash - src) = '\\0';\n> \treturn dst;\n> }\n> \n> Requires the glibc-specific function memrchr(), but that is easy to\n> implement.  Compare strchrnul() [in git-compat-util.h].\n\nRight, that's much more elegant - and should also be more efficient in\nthe \"ends with /\" case.\n\n> [...]\n> > +static struct diff_dir_rename* factorization_candidates = NULL;\n> > +static void diffcore_factorize_renames(void)\n> \n> Maybe this could be refactored into multiple functions?\n> \n> > @@ -509,6 +723,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> Oh, not single means \"not seeking a new relationship\".\n> \n> > @@ -569,7 +785,28 @@ 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_DIR_RENAMES))\n> > +\t\tdiffcore_factorize_renames();\n> \n> Huh?  Factorize?\n\n\"--factorize-renames\" was how the single-option was called in the\nprevious series.  It has still not turned completely false, but sure\nusing \"bulk rename\" would be easier to read.\n\n\n> Looks like I'll need to read diffcore_factorize_renames() after all. :(\n\nHopefully that part is better commented :)\n\n\n> Sorry, not much to say yet.  Hopefully some of that can be useful\n> nonetheless.\n\nThanks for this review, that work is always useful.\n"},{"id":"152604","messageId":"20101004213703.GW4983@home.lan","threadId":"25322","inReplyTo":"20101004203241.GF6466@burratino","subject":"Re: [PATCH v4 2/4] Add testcases for the --detect-dir-renames diffcore flag.","fromName":"Yann Dirson","fromEmail":"ydirson@free.fr","sentAt":"2010-10-04T21:37:03Z","receivedAt":"2010-10-04T21:37:03Z","isPatch":true,"sender":{"key":"ydirson@free.fr","avatar":null},"body":"On Mon, Oct 04, 2010 at 03:32:41PM -0500, Jonathan Nieder wrote:\n> Yann Dirson wrote:\n> \n> > From: Yann Dirson <ydirson@free.fr>\n> > Subject: Add testcases for the --detect-dir-renames diffcore flag\n> \n> Probably better for the test to be squashed with the patch that adds\n> that option.\n\nI tend to agree, but we see quite a lot of patches split this way it\nseems.  Junio, any preference here ?\n\n\n> > --- /dev/null\n> > +++ b/t/t4046-diff-rename-factorize.sh\n> > @@ -0,0 +1,326 @@\n> [...]\n> > +test_expect_success \\\n> > +    'commit the index.'  \\\n> > +    'git update-ref HEAD $(echo \"original empty commit\" | git commit-tree $(git write-tree))'\n> \n> Nit:\n> \n> Now test assertions tend to be written as\n> \n> \ttest_expect_success '...' '\n> \t\tcommand &&\n> \t\tcommand &&\n> \t\t...\n> \t'\n> \n> The tabs to indent and opening ' at the end of the first line means\n> less fuss in lining things up.  Making each test assertion include its\n> setup means tests don't pass if something gets messed up in the setup,\n> and using multiline test assertions with && means there is no\n> confusion about what the tests were written to test.\n> \n> As you mentioned, this has the unfortunate downside of messing with\n> syntax highlighting.  Maybe the common text editors need a new mode\n> for git-style test scripts?\n\nThere used to be a mmm-mode thingie for emacs which made such things\neasier (multiple major modes with custom delimiters) - but that did\nnot handle quoting issues automagically either :)\n\nOne solution seen in some test scripts already is to define functions\noutside of the test_expect* clauses.\n\n\n>  Or maybe it would make sense to implement\n> \n> \ttest_begin_expecting_success '...'\n> \t...\n> \ttest_end\n> \n> --- it would certainly make quoting easier.\n\nNot sure it would be possible without a macro processor.  And not sure\na macro processor would be welcome by everyone here :)\n\n\n> It might make sense to compute the tree, commit, etc one at a time\n> instead of this long one-liner.\n\nIf moved into a function which would make it readable, yes.\n\n\n> To avoid quoting troubles and since these are innocuous commands, it\n> could be nice to put this before the first test assertion.  Or even\n> better:\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> \t...\n\nWell, when seeing for the first time such a construct, i tend to\nwonder how portable it is amont printf implementations.  But hey, I'm\nalways thrilled to discover parts of the standard tools I had missed\nuntil now :)\n\n\n> [...]\n> > +test_expect_success \\\n> > +    'git diff-index --detect-dir-renames after directory move.' \\\n> > +    'git diff-index --detect-dir-renames HEAD >current'\n> > +grep -v \"^\\[DBG\\] \" <current >current.filtered\n> > +cat >expected <<\\EOF\n> > +:040000 040000 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 R100\ta/\tb/\n> > +:100644 100644 fdbec444a77953b1bcc899d9fabfa202e5e68f08 fdbec444a77953b1bcc899d9fabfa202e5e68f08 R100\ta/path0\tb/path0\n> > +:100644 100644 2f1f8d70c0fdad990819dfe37a31deb010805161 2f1f8d70c0fdad990819dfe37a31deb010805161 R100\ta/path1\tb/path1\n> > +:100644 100644 dbde7141d737c8aa0003672c1bc21ded48c6c3b9 dbde7141d737c8aa0003672c1bc21ded48c6c3b9 R100\ta/path2\tb/path2\n> > +:100644 100644 c6971ab9f08a6cd9c89a0f87d94ae347aad6144a c6971ab9f08a6cd9c89a0f87d94ae347aad6144a R100\ta/path3\tb/path3\n> > +EOF\n> \n> Nit: although compare_diff_patch ensures the result is not dependent\n> on the hash function, these hard-coded hashes are still hard for a\n> human to read.  Could they be computed instead?\n\nWell, that would just make the test harder to read imho.  Using\nregexps would help for readability, but that would require another\ncompare_diff variant.  BTW, I should be using compare_diff_raw not\ncompare_diff_patch here.\n"},{"id":"152606","messageId":"20101004220923.GA9491@burratino","threadId":"25322","inReplyTo":"20101004213703.GW4983@home.lan","subject":"Re: [PATCH v4 2/4] Add testcases for the --detect-dir-renames diffcore flag.","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-10-04T22:09:23Z","receivedAt":"2010-10-04T22:09:23Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Yann Dirson wrote:\n> On Mon, Oct 04, 2010 at 03:32:41PM -0500, Jonathan Nieder wrote:\n\n>> It might make sense to compute the tree, commit, etc one at a time\n>> instead of this long one-liner.\n>\n> If moved into a function which would make it readable, yes.\n\nSomething like\n\n\tcommit_index () {\n\t\ttest_tick &&\n\t\ttree=$(git write-tree) &&\n\t\tcommit=$(\n\t\t\tprintf \"%s\\n\" \"$*\" |\n\t\t\tgit commit-tree $tree\n\t\t) &&\n\t\tgit update-ref HEAD $commit\n\t}\n\n? Maybe the following (similar to what you use in later tests)\nwould be even better, for more verbose output when running with -v.\n\n\tcommit_index () {\n\t\ttest_tick &&\n\t\tgit commit -m \"$*\"\n\t}\n\n>> \tprintf \"Line %s\\n\" 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 >a/path0 &&\n>> \t...\n>\n> Well, when seeing for the first time such a construct, i tend to\n> wonder how portable it is amont printf implementations.\n\nYes, it's portable.\n\n>> Nit: although compare_diff_patch ensures the result is not dependent\n>> on the hash function, these hard-coded hashes are still hard for a\n>> human to read.  Could they be computed instead?\n>\n> Well, that would just make the test harder to read imho.  Using\n> regexps would help for readability\n\nIf you had said \"writability\" I would agree with you here.  And that's\nan important concern, too.\n\nWhat I was suggesting looks like this:\n\n\tpath0_id2=$(git rev-parse :b/path0) &&\n\tpath100_id=$(git rev-parse :b/path100) &&\n\tcat >expected <<-EOF &&\n\t:040000 040000 $zeroes $zeroes R100\ta/\tb/\n\t:100644 000000 $path3_id $zeroes D\ta/path3\n\t:100644 100644 $path2_id $path2_id R100\ta/path2\tb/2path\n\t:100644 100644 $path0_id $path0_id2 R093\ta/path0\tb/path0\n\t:100644 100644 $path1_id $path1_id R100\ta/path1\tb/path1\n\t:000000 100644 $zeroes $path100_id A\tb/path100\n\tEOF\n\t...\n\nMaybe it would be better to do\n\n\tcat >expected <<-\\EOF &&\n\t:040000 040000 X X R100\ta/\tb/\n\t:100644 000000 X X D\ta/path3\n\t:100644 100644 X X R100\ta/path2\tb/2path\n\t:100644 100644 X X R093\ta/path0\tb/path0\n\t:100644 100644 X X R100\ta/path1\tb/path1\n\t:000000 100644 X X A\tb/path100\n\tEOF\n\nsince the hashes are not being checked, anyway.\n"},{"id":"152630","messageId":"20101005010620.GC9994@burratino","threadId":"25322","inReplyTo":"1286138529-6780-2-git-send-email-ydirson@altern.org","subject":"Re: [PATCH v4 1/4] Introduce wholesame directory move detection in diffcore.","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-10-05T01:06:20Z","receivedAt":"2010-10-05T01:06:20Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Yann Dirson wrote:\n\n> This feature tries to group together files moving from and to\n> identical directories - the most common case being directory renames.\n\nIntro for the uninitiated (e.g., me):\n\n1. Entry point for the existing rename detection.\n\nThe entry point to diffcore's rename translator is diffcore_rename(),\nwhich is used, naturally enough, by diffcore_std() if and only if\nthe detect_rename option is enabled.\n\n2. Semantics of the existing rename detection.\n\nWhen diffcore_std() is called, the caller has already queued up\na nice list of files to diff (see Documentation/technical/api-diff.txt).\nThe rename detection pass replaces the queue with a new list of\nfilepairs that reflects the detected renames, guided by the\ndetect_rename, rename_score, and rename_limit options.\n\n3. Implementation of the existing rename detection.\n\n I.   find rename candidates (all files, if detecting copies; added and\n       deleted files, if not)\n II.  find exact renames and copies\n III. for each target file not accounted for yet, find the best 4\n       candidate source files\n IV.  record rename winners, in order from most to least similar pair\n V.   record copy winners, in order from most to least similar pair\n VI.  queue up results\n\n4. Data maintained by the existing rename detection.\n\n rename_dst, a list of rename candidates from step I, sorted by name\n For each rename dest candidate: a filespec and a filepair.\n\n  filepair::one\t\tsource filespec\n  filepair::two\t\tdestination filespec\n  filepair::score\tsimilarity score\n\n rename_src, a list of rename candidates from step I, sorted by name\n For each rename source candidate: a filespec and a score\n\n  filespec::rename_used\tnumber of filespecs it's paired against\n\t\t\t(to distinguish renames from copies)\n\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_DIR_RENAMES))\n>  \t\t\t\tcontinue;\n>  \t\t}\n>  \t\toldmode = ce->ce_mode;\n\nSane.  diff-files and friends need to pass unchanged files to the\ndiffcore lib when directory renames are being considered, just like\nthey would with -C -C.\n\nOtherwise, we cannot distinguish between\n\n\ta/1 stays\n\ta/2 stays\n\ta/3 stays\n\ta/4 stays\n\t...\n\ta/1000 stays\n\ta/1001 -> b/1001\n\ta/1002 -> b/1002\n\nwhich is clearly not a directory rename, and\n\n\ta/1001 -> b/1001\n\ta/1002 -> b/1002\n\nwhich sure looks like one.\n\nReference: v0.99~515 (Diff overhaul, adding the other half of copy\ndetection, 2005-05-21).\n\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_DIR_RENAMES))\n>  \t\treturn 0;\n\nThe diff-index part.\n\nWould we also need something like v1.6.4-rc0~45^2 (Avoid \"diff-index\n--cached\" optimization under --find-copies-harder, 2009-05-22) in\norder to grok unchanged subtrees, or is that taken care of some other\nway?\n\n[...]\n> --- a/diffcore-rename.c\n> +++ b/diffcore-rename.c\n> @@ -11,6 +11,7 @@\n>  static struct diff_rename_dst {\n>  \tstruct diff_filespec *two;\n>  \tstruct diff_filepair *pair;\n> +\tint i_am_not_single:1; // does not look for a match, only here to be looked at\n>  } *rename_dst;\n\nIf we're looking for directory renames but not finding-copies-harder,\nthen unmodified files, while interesting and deserving of dst entries,\nshould not be considered candidates for tracing individual files.\n\nYes?  If so, maybe the less colorful\n\n\tunsigned rename_target_candidate:1;\n\nwould make that clearer (and more importantly, the new kind of dst\nentry should be mentioned in the commit log message).\n\n> @@ -49,9 +50,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\nAll new files are rename target candidates.\n\n>  \n> +static struct diff_rename_dst *locate_rename_dst_dir(struct diff_filespec *dir)\n> +{\n> +\t/* code mostly duplicated from locate_rename_dst - not sure we\n> +\t * could merge them efficiently,though\n> +\t */\n> +\tint first, last;\n> +\tint prefixlength = strlen(dir->path);\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(dir->path, dst->two->path, prefixlength);\n\nprefixcmp?\n\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\nHmm, so this is like locate_rename_dst(..., 0), except it matches\nprefixes.  Result is a rename_dst entry within that directory (why\nnot spend the time to find the first?).\n\n> +\n>  /* Table of rename/copy src files */\n>  static struct diff_rename_src {\n>  \tstruct diff_filespec *one;\n> @@ -386,8 +414,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\nPrevents the new feature from turning on --find-copies-harder in\nstep II.  Good.\n\n>  \n>  \t/* Find the renames */\n>  \ti = for_each_hash(&file_table, find_same_files);\n> @@ -414,6 +445,180 @@ static void record_if_better(struct diff_score m[], struct diff_score *o)\n>  \t\tm[worst] = *o;\n>  }\n>  \n> +struct diff_dir_rename {\n> +\tstruct diff_filespec *one;\n> +\tstruct diff_filespec *two;\n> +\tint discarded;\n> +\tstruct diff_dir_rename* next;\n> +};\n\nA linked list of renamed directories.  What functions maintain it?\n\n[...]\n> +static const char* copy_dirname(char* dst, const char* src)\n\nDiscussed already.\n\n[...]\n> +static struct diff_dir_rename* factorization_candidates = NULL;\n> +static void diffcore_factorize_renames(void)\n> +{\n> +\tchar one_parent_path[PATH_MAX], two_parent_path[PATH_MAX];\n> +\tint i;\n> +\n> +\tfor (i = 0; i < rename_dst_nr; i++) {\n\nFor each rename target candidate: ...\n\nWould it make sense to give the body of this loop its own function?\n\n> +\t\tstruct diff_dir_rename* seen;\n> +\n> +\t\t// FIXME: what are those ?\n> +\t\tif (!rename_dst[i].pair)\n> +\t\t\tcontinue;\n\nGenuine new files are not interesting to us (since they do not\nprovide evidence about directory renames, postive or negative).\n\n> +\t\t// dummy renames used by copy detection\n> +\t\tif (!strcmp(rename_dst[i].pair->one->path, rename_dst[i].pair->two->path))\n> +\t\t\tcontinue;\n\nWhether a file was copied is probably interesting to us, but the\ncorresponding filepair isn't.\n\n> +\n> +\t\tcopy_dirname(one_parent_path, rename_dst[i].pair->one->path);\n> +\t\tcopy_dirname(two_parent_path, rename_dst[i].pair->two->path);\n> +\n> +\t\t// simple rename with no directory change\n> +\t\tif (!strcmp(one_parent_path, two_parent_path))\n> +\t\t\tcontinue;\n\nFile renames within a flat directory are not interesting to us.\n\n> +\n> +\t\tstruct diff_filespec* one_parent = alloc_filespec(one_parent_path);\n> +\t\tfill_filespec(one_parent, null_sha1 /*FIXME*/, S_IFDIR);\n\nInitialize a filespec, for use in filepairs in case the containing\ndirectories are a source/target pair. (?)\n\n> +\n> +\t\t// After this commit, are there any files still under one_parent_path ?\n> +\t\tstruct diff_rename_dst* one_leftover = locate_rename_dst_dir(one_parent);\n> +\t\tif (one_leftover) { // FIXME: should only be run if !seen\n[...]\n> +\t\t}\n\nA candidate for a separate function, no?  The idea, if I understand\nit: too many unchanged files can disqualify a rename source.\n\n> +\n> +\t\t// already considered ?\n> +\t\tfor (seen=factorization_candidates; seen; seen = seen->next)\n> +\t\t\tif (!strcmp(seen->one->path, one_parent_path)) break;\n\nLookup.  A candidate for a separate function, I think.\n\n> +\t\tif (!seen) {\n> +\t\t\t// record potential dir rename\n> +\t\t\tseen = xmalloc(sizeof(*seen));\n> +\t\t\tseen->one = one_parent;\n> +\t\t\tseen->two = alloc_filespec(two_parent_path);\n> +\t\t\tfill_filespec(seen->two, null_sha1 /*FIXME*/, S_IFDIR);\n> +\t\t\tseen->discarded = 0;\n> +\t\t\tseen->next = factorization_candidates;\n> +\t\t\tfactorization_candidates = seen;\n> +\t\t\tfprintf(stderr, \"[DBG] %s -> %s suggests possible rename from %s to %s\\n\",\n> +\t\t\t\trename_dst[i].pair->one->path,\n> +\t\t\t\trename_dst[i].pair->two->path,\n> +\t\t\t\tone_parent_path, two_parent_path);\n\nInsertion.  Likewise.\n\n> +\t\t\tcontinue;\n> +\t\t}\n> +\t\tif (seen->discarded)\n> +\t\t\tcontinue;\n\nOne disqualification disqualifies a directory completely.\n\n> +\t\t// check that seen entry matches this rename\n> +\t\tif (strcmp(two_parent_path, seen->two->path)) {\n> +\t\t\tfprintf(stderr, \"[DBG] discarding dir %s from renames (split into %s and %s)\\n\",\n> +\t\t\t\tone_parent_path, two_parent_path, seen->two->path);\n> +\t\t\tseen->discarded = 1;\n> +\t\t}\n\nIf a directory split in two, it gets disqualified.  Ok.\n\n> +\n> +\t\t/* all checks ok, we keep that entry */\n> +\t}\n> +\n> +\treturn;\n> +}\n> +\n>  void diffcore_rename(struct diff_options *options)\n>  {\n>  \tint detect_rename = options->detect_rename;\n> @@ -451,13 +656,22 @@ void diffcore_rename(struct diff_options *options)\n[...]\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_DIR_RENAMES)) {\n> +\t\t\t\t/* similarly, rename factorization 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\tlocate_rename_dst(p->two, 1)->i_am_not_single = 1;\n\n... except when --find-copies-harder is being used, right?\n\nSo I suspect there needs to be a\n\n\tDIFF_OPT_TST(options, FIND_COPIES_HARDER)\n\nsomewhere around here.\n\n> @@ -509,6 +723,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\nPrevents the new feature from turning on --find-copies-harder in\nstep III.  Good.\n\n> @@ -569,7 +785,28 @@ 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_DIR_RENAMES))\n> +\t\tdiffcore_factorize_renames();\n> +\n>  \tDIFF_QUEUE_CLEAR(&outq);\n> +\n> +\t// Now turn non-discarded factorization_candidates into real renames\n> +\tstruct diff_dir_rename* candidate;\n> +\tfor (candidate=factorization_candidates; candidate; candidate = candidate->next) {\n> +\t\tstruct diff_filepair* pair;\n> +\t\tif (candidate->discarded) continue;\n> +\t\t// visualize toplevel dir if needed - FIXME: wrong place for this ?\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\nOutputting the discovered directory renames.\n\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;\n> diff --git a/tree-diff.c b/tree-diff.c\n> index cd659c6..ca0b84f 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_DIR_RENAMES) &&\n> +\t    !hashcmp(sha1, sha2) && mode1 == mode2)\n>  \t\treturn 0;\n\nRight, makes sense.\n\nConclusions:\n\n - the basic idea looks sane\n - the main function would benefit a lot from being split up a bit\n - would be nice to have an overview of the design (especially, a\n   quick description of the heuristics used) for the commit message\n\nLooks promising, and a lot simpler than I feared. :)\n\nThanks.\n"},{"id":"152633","messageId":"20101005014208.GE9994@burratino","threadId":"25322","inReplyTo":"1286138529-6780-1-git-send-email-ydirson@altern.org","subject":"Re: [RFC PATCH v4 0/4] Detection of directory renames","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-10-05T01:42:08Z","receivedAt":"2010-10-05T01:42:08Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Yann Dirson wrote:\n\n>                                       The most prominent change is a\n> split of the old --factorize-renames flag into --detect-dir-renames\n> and --hide-dir-rename-details - hopefully following Junio's comments\n> dated nearly 2 years ago.\n\nI suspect that makes the patches more readable, too.\n\n> I hope we can turn the simple detection feature to something\n> acceptable for inclusion - that is, once unified diff output shows the\n> detected renames as annotations, and once the known major holes are\n> plugged (both from FIXME and testcases).\n\n>From my point of view, I think it would be best to start with\nthe smallest usable piece, which is the raw format.  It probably\nmakes the most sense to error out when -u and --detect-dir-renames are\nused together.  Then unified diff could be reenabled in a separate\npatch series on top of this one.\n\nAnother nice feature might be to let the directory-move\ndetection feed back into file-move detection to make it more\naccurate.  Have you thought about this?  Would it be feasible,\nand if so, would it be useful?\n"},{"id":"152657","messageId":"4CAAEE36.4060000@op5.se","threadId":"25322","inReplyTo":"20101004213703.GW4983@home.lan","subject":"Re: [PATCH v4 2/4] Add testcases for the --detect-dir-renames diffcore flag.","fromName":"Andreas Ericsson","fromEmail":"ae@op5.se","sentAt":"2010-10-05T09:21:58Z","receivedAt":"2010-10-05T09:21:58Z","isPatch":true,"sender":{"key":"ae@op5.se","avatar":"https://gravatar.com/avatar/426e89595c75a8f5252dd0c989e5fabe5bcac616e68557427ad9aef6b0ca342a?d=mp&s=160"},"body":"On 10/04/2010 11:37 PM, Yann Dirson wrote:\n> On Mon, Oct 04, 2010 at 03:32:41PM -0500, Jonathan Nieder wrote:\n>> Yann Dirson wrote:\n>>\n>>> From: Yann Dirson<ydirson@free.fr>\n>>> Subject: Add testcases for the --detect-dir-renames diffcore flag\n>>\n>> Probably better for the test to be squashed with the patch that adds\n>> that option.\n> \n> I tend to agree, but we see quite a lot of patches split this way it\n> seems.  Junio, any preference here ?\n> \n\nPersonally, I prefer adding the test first with \"test_expect_failure\"\nand then changing it to test_expect_success in the patch that has the\nreal code changes. That way it's really easy to accept a testcase and\nstill keep altering how the test itself is fixed, which is usually a\nfar more complex and nitpickley process than showing that the bug is\nthere in the first place.\n\nI know Junio has voiced the same preference for patchseries before and\nespecially when the solution isn't obvious. Note that I obviously can't\nspeak for him in any particular case.\n\nPatches adding features should ofcourse have the code first and the\ntests after, since the tests in that case are only about preventing\nfuture breakage rather than showing that there is one now.\n\n-- \nAndreas Ericsson                   andreas.ericsson@op5.se\nOP5 AB                             www.op5.se\nTel: +46 8-230225                  Fax: +46 8-230231\n\nConsidering the successes of the wars on alcohol, poverty, drugs and\nterror, I think we should give some serious thought to declaring war\non peace.\n"},{"id":"152819","messageId":"20101006231308.GY4983@home.lan","threadId":"25322","inReplyTo":"20101005010620.GC9994@burratino","subject":"Re: [PATCH v4 1/4] Introduce wholesame directory move detection in diffcore.","fromName":"Yann Dirson","fromEmail":"ydirson@free.fr","sentAt":"2010-10-06T23:13:08Z","receivedAt":"2010-10-06T23:13:08Z","isPatch":true,"sender":{"key":"ydirson@free.fr","avatar":null},"body":"On Mon, Oct 04, 2010 at 08:06:20PM -0500, Jonathan Nieder wrote:\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_DIR_RENAMES))\n> >  \t\treturn 0;\n> \n> The diff-index part.\n> \n> Would we also need something like v1.6.4-rc0~45^2 (Avoid \"diff-index\n> --cached\" optimization under --find-copies-harder, 2009-05-22) in\n> order to grok unchanged subtrees, or is that taken care of some other\n> way?\n\nNote to self: look at that\n\n> [...]\n> > --- a/diffcore-rename.c\n> > +++ b/diffcore-rename.c\n> > @@ -11,6 +11,7 @@\n> >  static struct diff_rename_dst {\n> >  \tstruct diff_filespec *two;\n> >  \tstruct diff_filepair *pair;\n> > +\tint i_am_not_single:1; // does not look for a match, only here to be looked at\n> >  } *rename_dst;\n> \n> If we're looking for directory renames but not finding-copies-harder,\n> then unmodified files, while interesting and deserving of dst entries,\n> should not be considered candidates for tracing individual files.\n> \n> Yes?  If so, maybe the less colorful\n> \n> \tunsigned rename_target_candidate:1;\n> \n> would make that clearer\n\nRight - but maybe we can use a separate list for those non-candidates\nas I suggested elsewhere, to make things even more clear.\n\n> (and more importantly, the new kind of dst entry should be mentioned\n> in the commit log message).\n\nHm, the commit message is a bit long already, isn't that too much detail ?\n\n\n> > +static struct diff_rename_dst *locate_rename_dst_dir(struct diff_filespec *dir)\n> > +{\n> > +\t/* code mostly duplicated from locate_rename_dst - not sure we\n> > +\t * could merge them efficiently,though\n> > +\t */\n> > +\tint first, last;\n> > +\tint prefixlength = strlen(dir->path);\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(dir->path, dst->two->path, prefixlength);\n> \n> prefixcmp?\n\nSince prefixlength is constant accross the loop, strncmp spares\ncomparing every char with \\0 - we only do that once.  I guess it's\nworth keeping.\n\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> Hmm, so this is like locate_rename_dst(..., 0), except it matches\n> prefixes.  Result is a rename_dst entry within that directory (why\n> not spend the time to find the first?).\n\nWe just don't need the first.  In most cases, finding one entry just\ninvalidates the possiblity of a bulk move.  Then when we want all of\nthem, we have to scan the list in both directions - and yes, a clean\nway to avoid code duplication would be nice for those.\n\n> >  \n> >  \t/* Find the renames */\n> >  \ti = for_each_hash(&file_table, find_same_files);\n> > @@ -414,6 +445,180 @@ static void record_if_better(struct diff_score m[], struct diff_score *o)\n> >  \t\tm[worst] = *o;\n> >  }\n> >  \n> > +struct diff_dir_rename {\n> > +\tstruct diff_filespec *one;\n> > +\tstruct diff_filespec *two;\n> > +\tint discarded;\n> > +\tstruct diff_dir_rename* next;\n> > +};\n> \n> A linked list of renamed directories.  What functions maintain it?\n\nThat is just the type decl.  For this patch it is only used from\ndiffcore_factorize_renames() and diffcore_rename(), but the\n--hide-dir-rename-details part needs it for maybe_mark_uninteresting,\nwhich explains its position in the source file.\n\n> [...]\n> > +static struct diff_dir_rename* factorization_candidates = NULL;\n> > +static void diffcore_factorize_renames(void)\n> > +{\n> > +\tchar one_parent_path[PATH_MAX], two_parent_path[PATH_MAX];\n> > +\tint i;\n> > +\n> > +\tfor (i = 0; i < rename_dst_nr; i++) {\n> \n> For each rename target candidate: ...\n> \n> Would it make sense to give the body of this loop its own function?\n\nYes, that makes things more readable.\n\n> > +\t\tstruct diff_dir_rename* seen;\n> > +\n> > +\t\t// FIXME: what are those ?\n> > +\t\tif (!rename_dst[i].pair)\n> > +\t\t\tcontinue;\n> \n> Genuine new files are not interesting to us (since they do not\n> provide evidence about directory renames, postive or negative).\n\nWell, maybe except for Junio's idea of using bulk-rename detection to\nbump rename scores (we may want to look at that afterwards, but I fear\nit will require heavy surgery).\n\n\n> > +\t\t// dummy renames used by copy detection\n> > +\t\tif (!strcmp(rename_dst[i].pair->one->path, rename_dst[i].pair->two->path))\n> > +\t\t\tcontinue;\n> \n> Whether a file was copied is probably interesting to us, but the\n> corresponding filepair isn't.\n\nGood point: bulk-rename can already be made to direct move+copy\ndetection to select the correct dst for the move.\n\n> > +\n> > +\t\tstruct diff_filespec* one_parent = alloc_filespec(one_parent_path);\n> > +\t\tfill_filespec(one_parent, null_sha1 /*FIXME*/, S_IFDIR);\n> \n> Initialize a filespec, for use in filepairs in case the containing\n> directories are a source/target pair. (?)\n\nWe need a filespec for locate_rename_dst_dir, but it is silly, we can\njust pass it a path string, and initialize the filespec when really\nneeded.  Good catch.\n \n> > +\n> > +\t\t// After this commit, are there any files still under one_parent_path ?\n> > +\t\tstruct diff_rename_dst* one_leftover = locate_rename_dst_dir(one_parent);\n> > +\t\tif (one_leftover) { // FIXME: should only be run if !seen\n> [...]\n> > +\t\t}\n> \n> A candidate for a separate function, no?\n\nRight.\n\n> The idea, if I understand it: too many unchanged files can\n> disqualify a rename source.\n\nNot exactly: any file left in a dir disqualifies this dir for bulk-rename.\n\n> > +\n> > +\t\t// already considered ?\n> > +\t\tfor (seen=factorization_candidates; seen; seen = seen->next)\n> > +\t\t\tif (!strcmp(seen->one->path, one_parent_path)) break;\n> \n> Lookup.  A candidate for a separate function, I think.\n> \n> > +\t\tif (!seen) {\n> > +\t\t\t// record potential dir rename\n> > +\t\t\tseen = xmalloc(sizeof(*seen));\n> > +\t\t\tseen->one = one_parent;\n> > +\t\t\tseen->two = alloc_filespec(two_parent_path);\n> > +\t\t\tfill_filespec(seen->two, null_sha1 /*FIXME*/, S_IFDIR);\n> > +\t\t\tseen->discarded = 0;\n> > +\t\t\tseen->next = factorization_candidates;\n> > +\t\t\tfactorization_candidates = seen;\n> > +\t\t\tfprintf(stderr, \"[DBG] %s -> %s suggests possible rename from %s to %s\\n\",\n> > +\t\t\t\trename_dst[i].pair->one->path,\n> > +\t\t\t\trename_dst[i].pair->two->path,\n> > +\t\t\t\tone_parent_path, two_parent_path);\n> \n> Insertion.  Likewise.\n\nHm, it's only used once, and once the other splits are done the func\nfits on a screen (at least on my display ;)\n\nOTOH, if we decide for a sorted list, to allow for binary search to be\nmore efficient, then yes.\n\n> > +\n> > +\t\t/* all checks ok, we keep that entry */\n> > +\t}\n> > +\n> > +\treturn;\n> > +}\n> > +\n> >  void diffcore_rename(struct diff_options *options)\n> >  {\n> >  \tint detect_rename = options->detect_rename;\n> > @@ -451,13 +656,22 @@ void diffcore_rename(struct diff_options *options)\n> [...]\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_DIR_RENAMES)) {\n> > +\t\t\t\t/* similarly, rename factorization 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\tlocate_rename_dst(p->two, 1)->i_am_not_single = 1;\n> \n> ... except when --find-copies-harder is being used, right?\n\nI have not investigated interactions with copy detection yet.\n\n\n> > @@ -569,7 +785,28 @@ 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_DIR_RENAMES))\n> > +\t\tdiffcore_factorize_renames();\n> > +\n> >  \tDIFF_QUEUE_CLEAR(&outq);\n> > +\n> > +\t// Now turn non-discarded factorization_candidates into real renames\n> > +\tstruct diff_dir_rename* candidate;\n> > +\tfor (candidate=factorization_candidates; candidate; candidate = candidate->next) {\n> > +\t\tstruct diff_filepair* pair;\n> > +\t\tif (candidate->discarded) continue;\n> > +\t\t// visualize toplevel dir if needed - FIXME: wrong place for this ?\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> \n> Outputting the discovered directory renames.\n\nNot really outputting, just queueing for ouput here.\n\n\n> Conclusions:\n> \n>  - the basic idea looks sane\n>  - the main function would benefit a lot from being split up a bit\n>  - would be nice to have an overview of the design (especially, a\n>    quick description of the heuristics used) for the commit message\n\nThanks for this detailed review.  Pushing to\nhttp://repo.or.cz/w/git/ydirson.git a set of patches addressing those\nremarks and others from this thread.\n"},{"id":"152823","messageId":"20101006231734.GZ4983@home.lan","threadId":"25322","inReplyTo":"20101005014208.GE9994@burratino","subject":"Re: [RFC PATCH v4 0/4] Detection of directory renames","fromName":"Yann Dirson","fromEmail":"ydirson@free.fr","sentAt":"2010-10-06T23:17:34Z","receivedAt":"2010-10-06T23:17:34Z","isPatch":true,"sender":{"key":"ydirson@free.fr","avatar":null},"body":"On Mon, Oct 04, 2010 at 08:42:08PM -0500, Jonathan Nieder wrote:\n> >From my point of view, I think it would be best to start with\n> the smallest usable piece, which is the raw format.  It probably\n> makes the most sense to error out when -u and --detect-dir-renames are\n> used together.  Then unified diff could be reenabled in a separate\n> patch series on top of this one.\n\nWell, there's probably not much to do - in the same order of work than\nadding the check to error out :)\n\n> Another nice feature might be to let the directory-move\n> detection feed back into file-move detection to make it more\n> accurate.  Have you thought about this?  Would it be feasible,\n> and if so, would it be useful?\n\nThat was suggested by Junio at that time.  It would surely be useful\nin some cases, but it's not clear to me how frequently it would.  And\nI'd suspect that quite some design changes would be necessary: that\nwould be a 2-way influence, in that a nearly-complete bulkmove could\nbump the score of the latest rename that would make it a full\nbulkmove.  Probably not trivial to do right.\n"}]}