{"thread":{"id":"16125","subject":"[PATCH 1/3] Fix error in diff_filepair::status documentation.","startedAt":"2008-11-01T22:03:08Z","lastAt":"2008-11-08T00:50:19Z","messageCount":15,"participants":["Yann Dirson","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"94596","messageId":"20081101215739.1116.59319.stgit@gandelf.nowhere.earth","threadId":"16125","inReplyTo":null,"subject":"[PATCH v3 0/3] Detection of directory renames","fromName":"Yann Dirson","fromEmail":"ydirson@altern.org","sentAt":"2008-11-01T22:03:08Z","receivedAt":"2008-11-01T22:03:08Z","isPatch":true,"sender":{"key":"ydirson@altern.org","avatar":"https://avatars.githubusercontent.com/u/1190950?v=4"},"body":"This new version fixes handling of moves to and from tree toplevel,\nadds supports for detecting bulk moves of files into a subdirectory of\ntheir original dir, and adds a couple of other testcases showing what\nstill has to be done to properly handle moves of directories with\nsubdirs.\n\n-- \nYann Dirson    <ydirson@altern.org> |\nDebian-related: <dirson@debian.org> |   Support Debian GNU/Linux:\n                                    |  Freedom, Power, Stability, Gratis\n     http://ydirson.free.fr/        | Check <http://www.debian.org/>\n"},{"id":"94595","messageId":"20081101220314.1116.79924.stgit@gandelf.nowhere.earth","threadId":"16125","inReplyTo":"20081101215739.1116.59319.stgit@gandelf.nowhere.earth","subject":"[PATCH 1/3] Fix error in diff_filepair::status documentation.","fromName":"Yann Dirson","fromEmail":"ydirson@altern.org","sentAt":"2008-11-01T22:03:14Z","receivedAt":"2008-11-01T22:03:14Z","isPatch":true,"sender":{"key":"ydirson@altern.org","avatar":"https://avatars.githubusercontent.com/u/1190950?v=4"},"body":"Signed-off-by: Yann Dirson <ydirson@altern.org>\n---\n\n diffcore.h |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/diffcore.h b/diffcore.h\nindex 713cca7..05d0898 100644\n--- a/diffcore.h\n+++ b/diffcore.h\n@@ -62,7 +62,7 @@ struct diff_filepair {\n \tstruct diff_filespec *one;\n \tstruct diff_filespec *two;\n \tunsigned short int score;\n-\tchar status; /* M C R N D U (see Documentation/diff-format.txt) */\n+\tchar status; /* M C R A D U etc. (see DIFF_STATUS_* in diff.h) */\n \tunsigned broken_pair : 1;\n \tunsigned renamed_pair : 1;\n \tunsigned is_unmerged : 1;\n"},{"id":"94598","messageId":"20081101220319.1116.50509.stgit@gandelf.nowhere.earth","threadId":"16125","inReplyTo":"20081101215739.1116.59319.stgit@gandelf.nowhere.earth","subject":"[PATCH 2/3] Introduce rename factorization in diffcore.","fromName":"Yann Dirson","fromEmail":"ydirson@altern.org","sentAt":"2008-11-01T22:03:20Z","receivedAt":"2008-11-01T22:03:20Z","isPatch":true,"sender":{"key":"ydirson@altern.org","avatar":"https://avatars.githubusercontent.com/u/1190950?v=4"},"body":"Rename factorization tries to group together files moving from and to\nidentical directories - the most common case being directory renames.\nWe do that by first identifying groups of bulk-moved files, and then\nhiding those of the individual renames which carry no other\ninformation (further name change, or content changes).\nThis feature is activated by the new --factorize-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\n diff-lib.c        |    6 +\n diff.c            |    5 +\n diff.h            |    3 +\n diffcore-rename.c |  301 +++++++++++++++++++++++++++++++++++++++++++++++++++--\n diffcore.h        |    1 \n tree-diff.c       |    4 +\n 6 files changed, 307 insertions(+), 13 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex ae96c64..dcc4c2c 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -179,7 +179,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\tchanged = ce_match_stat(ce, &st, ce_option);\n \t\tif (!changed) {\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, FACTORIZE_RENAMES))\n \t\t\t\tcontinue;\n \t\t}\n \t\toldmode = ce->ce_mode;\n@@ -310,7 +311,8 @@ static int show_modified(struct oneway_unpack_data *cbdata,\n \n \toldmode = old->ce_mode;\n \tif (mode == oldmode && !hashcmp(sha1, old->sha1) &&\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, FACTORIZE_RENAMES))\n \t\treturn 0;\n \n \tdiff_change(&revs->diffopt, oldmode, mode,\ndiff --git a/diff.c b/diff.c\nindex e368fef..f91fcf6 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2437,6 +2437,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, \"--factorize-renames\")) {\n+\t\tDIFF_OPT_SET(options, FACTORIZE_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 a49d865..db1658b 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -65,6 +65,7 @@ typedef void (*diff_format_fn_t)(struct diff_queue_struct *q,\n #define DIFF_OPT_IGNORE_SUBMODULES   (1 << 18)\n #define DIFF_OPT_DIRSTAT_CUMULATIVE  (1 << 19)\n #define DIFF_OPT_DIRSTAT_BY_FILE     (1 << 20)\n+#define DIFF_OPT_FACTORIZE_RENAMES   (1 << 21)\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 #define DIFF_OPT_CLR(opts, flag)    ((opts)->flags &= ~DIFF_OPT_##flag)\n@@ -220,6 +221,8 @@ extern void diffcore_std(struct diff_options *);\n \"  -C            detect copies.\\n\" \\\n \"  --find-copies-harder\\n\" \\\n \"                try unchanged files as candidate for copy detection.\\n\" \\\n+\"  --factorize-renames\\n\" \\\n+\"                factorize renames of all files of a directory.\\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 168a95b..ab6149e 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@@ -262,7 +290,7 @@ static int find_identical_files(struct file_similarity *src,\n \t\t\tint score;\n \t\t\tstruct diff_filespec *source = p->filespec;\n \n-\t\t\t/* False hash collission? */\n+\t\t\t/* False hash collision? */\n \t\t\tif (hashcmp(source->sha1, target->sha1))\n \t\t\t\tcontinue;\n \t\t\t/* Non-regular files? If so, the modes must match! */\n@@ -381,8 +409,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@@ -409,6 +440,223 @@ 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+/*\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+ * an additional \"/\".\n+ * Only handles relative paths since there is no relative 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+ * 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 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+ */\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\tstruct diff_filespec* one_parent = alloc_filespec(one_parent_path);\n+\t\tfill_filespec(one_parent, null_sha1 /*FIXME*/, S_IFDIR);\n+\n+\t\tstruct diff_rename_dst* one_leftover =\n+\t\t\tlocate_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\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\t rename_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\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\t rename_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       rename_dst[i].pair->one->path,\n+\t\t\t       rename_dst[i].pair->two->path,\n+\t\t\t       one_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 split of %s from renames (into %s and %s)\\n\",\n+\t\t\t       one_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+\t// turn candidates into real renames\n+\tstruct diff_dir_rename* candidate;\n+\tfor (candidate=factorization_candidates; candidate; candidate = candidate->next) {\n+\t\tint two_idx, i, one_len, two_len;\n+\t\tif (candidate->discarded)\n+\t\t\tcontinue;\n+\n+\t\t// bisect to an entry within candidate dst dir\n+\t\tstruct diff_rename_dst* two_sample =\n+\t\t\tlocate_rename_dst_dir(candidate->two);\n+\t\tif (!two_sample) {\n+\t\t\tdie (\"PANIC: %s candidate of rename not in target tree (from %s)\\n\",\n+\t\t\t     candidate->two->path, candidate->one->path);\n+\t\t}\n+\t\ttwo_idx = two_sample - rename_dst;\n+\n+\t\t// now remove extraneous 100% files inside.\n+\t\tone_len = strlen(candidate->one->path);\n+\t\ttwo_len = strlen(candidate->two->path);\n+\t\tfor (i = two_idx; i < rename_dst_nr; i++)\n+\t\t\tif (maybe_mark_uninteresting (rename_dst+i, candidate,\n+\t\t\t\t\t\t      one_len, two_len) < 0)\n+\t\t\t\tbreak;\n+\t\tfor (i = two_idx-1; i >= 0; i--)\n+\t\t\tif (maybe_mark_uninteresting (rename_dst+i, candidate,\n+\t\t\t\t\t\t      one_len, two_len) < 0)\n+\t\t\t\tbreak;\n+\t}\n+\n+\treturn;\n+}\n+\n void diffcore_rename(struct diff_options *options)\n {\n \tint detect_rename = options->detect_rename;\n@@ -446,13 +694,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, FACTORIZE_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@@ -504,6 +761,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@@ -561,8 +820,29 @@ 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, FACTORIZE_RENAMES))\n+\t\tdiffcore_factorize_renames();\n+\n \toutq.queue = NULL;\n \toutq.nr = outq.alloc = 0;\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;\n@@ -577,7 +857,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 05d0898..ce781ae 100644\n--- a/diffcore.h\n+++ b/diffcore.h\n@@ -66,6 +66,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 \ndiff --git a/tree-diff.c b/tree-diff.c\nindex 9f67af6..872f757 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, FACTORIZE_RENAMES) &&\n+\t    !hashcmp(sha1, sha2) && mode1 == mode2)\n \t\treturn 0;\n \n \t/*\n"},{"id":"94597","messageId":"20081101220325.1116.81935.stgit@gandelf.nowhere.earth","threadId":"16125","inReplyTo":"20081101215739.1116.59319.stgit@gandelf.nowhere.earth","subject":"[PATCH 3/3] Add testcases for the --factorize-renames diffcore flag.","fromName":"Yann Dirson","fromEmail":"ydirson@altern.org","sentAt":"2008-11-01T22:03:25Z","receivedAt":"2008-11-01T22:03:25Z","isPatch":true,"sender":{"key":"ydirson@altern.org","avatar":"https://avatars.githubusercontent.com/u/1190950?v=4"},"body":"This notably includes a couple of tests for cases known not to be\nworking correctly yet.\n---\n\n t/t4030-diff-rename-factorize.sh |  259 ++++++++++++++++++++++++++++++++++++++\n 1 files changed, 259 insertions(+), 0 deletions(-)\n create mode 100755 t/t4030-diff-rename-factorize.sh\n\ndiff --git a/t/t4030-diff-rename-factorize.sh b/t/t4030-diff-rename-factorize.sh\nnew file mode 100755\nindex 0000000..302a7ab\n--- /dev/null\n+++ b/t/t4030-diff-rename-factorize.sh\n@@ -0,0 +1,259 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2008 Yann Dirson\n+# Copyright (c) 2005 Junio C Hamano\n+#\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 --factorize-renames after directory move.' \\\n+    'git diff-index --factorize-renames 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 current.filtered expected'\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 --factorize-renames after directory move and content changes.' \\\n+    'git diff-index --factorize-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+: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 current.filtered expected'\n+\n+git reset --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 --factorize-renames without full-dir rename.' \\\n+    'git diff-index --factorize-renames 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 current.filtered expected'\n+\n+git reset --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 --factorize-renames files bulk-moved to toplevel.' \\\n+    'git diff-index --factorize-renames 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 current.filtered expected'\n+\n+git reset --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 --factorize-renames on a move including a subdir.' \\\n+    'git diff-index --factorize-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+EOF\n+\n+test_expect_failure \\\n+    'validate the output for a move including a subdir.' \\\n+    'compare_diff_patch current.filtered expected'\n+\n+git reset --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 --factorize-renames on a move without a subdir.' \\\n+    'git diff-index --factorize-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 current.filtered expected'\n+\n+git reset --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 --factorize-renames on a split of subdir and files into different places.' \\\n+    'git diff-index --factorize-renames HEAD >current'\n+grep -v \"^\\[DBG\\] \" <current >current.filtered\n+cat >expected <<\\EOF\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+:040000 040000 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 R100\ta/c/\tb/\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 current.filtered expected'\n+\n+# now test moving all files from toplevel into subdir (does not hides file moves) (needs consensus on syntax)\n+# Note: this is as special case of move of a dir into one of its own subdirs, which in\n+# turn is a special case of new files/dirs being added into a dir after all its contents\n+# are moved away\n+\n+git reset --hard HEAD~2\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 --factorize-renames everything from toplevel.' \\\n+    'git diff-index --factorize-renames 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 current.filtered expected'\n+\n+test_done\n"},{"id":"94635","messageId":"7vod0y3ahc.fsf@gitster.siamese.dyndns.org","threadId":"16125","inReplyTo":"20081101220314.1116.79924.stgit@gandelf.nowhere.earth","subject":"Re: [PATCH 1/3] Fix error in diff_filepair::status documentation.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-11-02T06:31:11Z","receivedAt":"2008-11-02T06:31:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Yann Dirson <ydirson@altern.org> writes:\n\n> Signed-off-by: Yann Dirson <ydirson@altern.org>\n> ---\n>\n>  diffcore.h |    2 +-\n>  1 files changed, 1 insertions(+), 1 deletions(-)\n>\n> diff --git a/diffcore.h b/diffcore.h\n> index 713cca7..05d0898 100644\n> --- a/diffcore.h\n> +++ b/diffcore.h\n> @@ -62,7 +62,7 @@ struct diff_filepair {\n>  \tstruct diff_filespec *one;\n>  \tstruct diff_filespec *two;\n>  \tunsigned short int score;\n> -\tchar status; /* M C R N D U (see Documentation/diff-format.txt) */\n> +\tchar status; /* M C R A D U etc. (see DIFF_STATUS_* in diff.h) */\n\nI think you spotted the right bug and fixed it in a wrong way.  The status\nletters are user visible, and \"git grep status Documentation/*diff*\"\nmention it in handful places without listing its repertoire, which _is_\nthe bug.\n"},{"id":"95087","messageId":"7vy6zwgx3i.fsf@gitster.siamese.dyndns.org","threadId":"16125","inReplyTo":"20081101220319.1116.50509.stgit@gandelf.nowhere.earth","subject":"Re: [PATCH 2/3] Introduce rename factorization in diffcore.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-11-07T01:10:09Z","receivedAt":"2008-11-07T01:10:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Yann Dirson <ydirson@altern.org> writes:\n\n> Rename factorization tries to group together files moving from and to\n> identical directories - the most common case being directory renames.\n> We do that by first identifying groups of bulk-moved files, and then\n> hiding those of the individual renames which carry no other\n> information (further name change, or content changes).\n> This feature is activated by the new --factorize-renames diffcore\n> flag.\n\nI have a mixed feeling about this one, primarily because I cannot\nvisualize how a useful output should look like.  Unless you rename one\ndirectory to another without any content changes, you would have to say\n\"this directory changed to that, and among the paths underneath them, this\nfile have this content change in addition\".\n\nA related feature that would benefit from something like your change\nwithout any downside/complication of output format issues is to boost\nrename similarity score of a path when its neighbouring paths are moved to\nthe same location.  E.g. when you see:\n\n - three files a/{1,2,3} deleted;\n - three files b/{1,2,3} created;\n - (a/1 => b/1) and (a/2 => b/2) are similar enough;\n - (a/3 => b/3) are not similar enough.\n\nwe currently detect only two renames and leave deletion of a/3 and\ncreation of b/3 unpaired.  You should be able to help them paired up by\nnoticing that the entire a/* goes away (for that, reading the full\npostimage like you do in your patch helps) and boost the similarity score\nbetween these two.\n\nAlthough I do not offhand think a good format to show the information you\nare trying to capture in the textual diff output, one thing that would be\nhelped by the grouping of renames like you do would be process_renames()\nin merge_recursive.c.  This is especially so when you have added a new\npath in a directory that has been moved by the other branch you are\nmerging.  For this usage, there is no \"textual output format\" issues.  It\ndoes not even have to be expressed by replacing individual entries from\ndiffq with entries that represent a whole subtree --- you could for\nexample keep what diffq.queue records intact, and add a separate list of\ndirectory renames as a hint for users like process_renames() to use.\n"},{"id":"95139","messageId":"20081107193957.GI5158@nan92-1-81-57-214-146.fbx.proxad.net","threadId":"16125","inReplyTo":"7vy6zwgx3i.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/3] Introduce rename factorization in diffcore.","fromName":"Yann Dirson","fromEmail":"ydirson@altern.org","sentAt":"2008-11-07T19:39:57Z","receivedAt":"2008-11-07T19:39:57Z","isPatch":true,"sender":{"key":"ydirson@altern.org","avatar":"https://avatars.githubusercontent.com/u/1190950?v=4"},"body":"On Thu, Nov 06, 2008 at 05:10:09PM -0800, Junio C Hamano wrote:\n> Yann Dirson <ydirson@altern.org> writes:\n> \n> > Rename factorization tries to group together files moving from and to\n> > identical directories - the most common case being directory renames.\n> > We do that by first identifying groups of bulk-moved files, and then\n> > hiding those of the individual renames which carry no other\n> > information (further name change, or content changes).\n> > This feature is activated by the new --factorize-renames diffcore\n> > flag.\n> \n> I have a mixed feeling about this one, primarily because I cannot\n> visualize how a useful output should look like.  Unless you rename one\n> directory to another without any content changes, you would have to say\n> \"this directory changed to that, and among the paths underneath them, this\n> file have this content change in addition\".\n\nRight.  This is something I have already thought about, without taking\nthe time to finalize.  So let's have a try.\n\nThe direct transposition of the raw output of a dir rename with a\nchange would yield something like:\n\n|diff --git a/ppc/ b/foo\n|similarity index 100%\n|rename from ppc/\n|rename to foo/\n|diff --git a/ppc/sha1.h b/foo/sha1.h\n|--- ppc/sha1.h\n|+++ foo/sha1.h\n|...\n\nThis would have the usefulness I'm looking for, in that the content\nmodifications would not be \"hidden\" among a whole lot af individual\nfile rename hunks.\n\nBut if we want to make this more generally useful (eg. allow other\ntools to apply such patches), we need to be careful.  A typical patch\nfile can usually be seen as a sequence of changes which can be applied\nindependently (at least in order).  It is obvious that this is not\ntrue for the above output.  So we could also consider making this\npatch output sequential with something like:\n\n|diff --git a/ppc/ b/foo\n|similarity index 100%\n|rename from ppc/\n|rename to foo/\n|diff --git a/foo/sha1.h b/foo/sha1.h\n|--- foo/sha1.h\n|+++ foo/sha1.h\n|...\n\nHowever, such output would possibly be confusing, eg. when looking at\nit from gitk and looking for commits which modify ppc/sha1.h.\n\nA solution to this could be to add an annotation such as:\n\n|diff --git a/ppc/ b/foo\n|similarity index 100%\n|rename from ppc/\n|rename to foo/\n|diff --git a/foo/sha1.h b/foo/sha1.h\n|--- foo/sha1.h (previously ppc/sha1.h)\n|+++ foo/sha1.h\n|...\n\nThat would make the \"git diff\" format diverge a bit more from the\nstandard, but since a full-fledged git patch already cannot be used by\nstandard patch tools, it should not be a big issue - we should just be\ncareful about choosing a suitable format if we go that way.\n\nBut as we already change the patch format, we could also simply say\nthat we don't care about the patch being \"litterally splittable\", add\na \"this is a non-splittable whole\" note at the beginning of the patch\noutput, and go with the litteral solution shown as first example.\n\nHow do you feel wrt this ?\n\n\n> A related feature that would benefit from something like your change\n> without any downside/complication of output format issues is to boost\n> rename similarity score of a path when its neighbouring paths are moved to\n> the same location.\n\nYes, that could be triggered by distinct switches.\n\nDoing this would be related to detecting \"directory splits\" (ie. when\nsome files are really not moved, or moved into several dirs instead of\nbeing all moved to the same target dir).  We can start by detecting\ndir-splits, then shift the scoring threshold and do another pass on\nsplit dirs.\n"},{"id":"95141","messageId":"7v3ai3gugt.fsf@gitster.siamese.dyndns.org","threadId":"16125","inReplyTo":"20081107193957.GI5158@nan92-1-81-57-214-146.fbx.proxad.net","subject":"Re: [PATCH 2/3] Introduce rename factorization in diffcore.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-11-07T20:19:14Z","receivedAt":"2008-11-07T20:19:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Yann Dirson <ydirson@altern.org> writes:\n\n> The direct transposition of the raw output of a dir rename with a\n> change would yield something like:\n>\n> |diff --git a/ppc/ b/foo\n> |similarity index 100%\n> |rename from ppc/\n> |rename to foo/\n> |diff --git a/ppc/sha1.h b/foo/sha1.h\n> |--- ppc/sha1.h\n> |+++ foo/sha1.h\n> |...\n>\n> This would have the usefulness I'm looking for, in that the content\n> modifications would not be \"hidden\" among a whole lot af individual\n> file rename hunks.\n\nI am afraid that this is totally unacceptable, as you yourself mentioned,\nthe end result is unapplicable with any existing tool and would confuse\nviewers like gitk and gitweb.\n\nI think it would be a much better approach to emit a hint that talks about\ndirectory rename in a format that does not fool usual \"patch\" application\ntools.  E.g.\n\n|directory moved with similarity index 82%\n|rename from ppc\n|rename from foo\n|diff --git a/ppc/sha1.h b/foo/sha1.h\n|similarity index 85%\n|rename from ppc/sha1.h\n|rename to foo/sha1.h\n|index  9b34f76..8fce4b7 100644\n|--- ppc/sha1.h\n|+++ foo/sha1.h\n|@@ ...\n\nIOW, do not add anything that begins with \"diff --git\" if it is not a\npatch.\n"},{"id":"95143","messageId":"20081107203952.GJ5158@nan92-1-81-57-214-146.fbx.proxad.net","threadId":"16125","inReplyTo":"7v3ai3gugt.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/3] Introduce rename factorization in diffcore.","fromName":"Yann Dirson","fromEmail":"ydirson@altern.org","sentAt":"2008-11-07T20:39:52Z","receivedAt":"2008-11-07T20:39:52Z","isPatch":true,"sender":{"key":"ydirson@altern.org","avatar":"https://avatars.githubusercontent.com/u/1190950?v=4"},"body":"On Fri, Nov 07, 2008 at 12:19:14PM -0800, Junio C Hamano wrote:\n> I am afraid that this is totally unacceptable, as you yourself mentioned,\n> the end result is unapplicable with any existing tool and would confuse\n> viewers like gitk and gitweb.\n\nWell, other tools will still have to be taught about a new syntax, if\nthey're going to use the new flag - just like it was for --rename.\n\nThat makes me think that it could be interesting to have a header\ntelling which diff extensions it is using.  It could even be\ninteresting to come with a cross-vc convention, which would ease the\ninterchange of modern patch files (I suppose some other tools already\nhave their own extensions for file renames).\n\nWhat about some hint like the following ?\n\n|extended diff features: git:rename git:binary git:filetypes\n|extended diff features: git:unixperms git:dir-rename\n\nMaybe we could leave \"filetype\" and \"unixperms\" out, since these do\nnot replace an information that would otherwise be present in a\nstandard diff anyway, and would just be ignored by a tool not\nsupporting them.\n\n\n> I think it would be a much better approach to emit a hint that talks about\n> directory rename in a format that does not fool usual \"patch\" application\n> tools.  E.g.\n> \n> |directory moved with similarity index 82%\n> |rename from ppc\n> |rename from foo\n> |diff --git a/ppc/sha1.h b/foo/sha1.h\n> |similarity index 85%\n> |rename from ppc/sha1.h\n> |rename to foo/sha1.h\n> |index  9b34f76..8fce4b7 100644\n> |--- ppc/sha1.h\n> |+++ foo/sha1.h\n> |@@ ...\n\nSo you're favoring the \"patch is not textually splittable approach\".\nDon't you think it would be good to add some sort of hint about this\nas a patch header ?\n\n> IOW, do not add anything that begins with \"diff --git\" if it is not a\n> patch.\n\nOK.\n"},{"id":"95146","messageId":"7vskq3fdgz.fsf@gitster.siamese.dyndns.org","threadId":"16125","inReplyTo":"20081107203952.GJ5158@nan92-1-81-57-214-146.fbx.proxad.net","subject":"Re: [PATCH 2/3] Introduce rename factorization in diffcore.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-11-07T21:11:40Z","receivedAt":"2008-11-07T21:11:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Yann Dirson <ydirson@altern.org> writes:\n\n> On Fri, Nov 07, 2008 at 12:19:14PM -0800, Junio C Hamano wrote:\n>> I am afraid that this is totally unacceptable, as you yourself mentioned,\n>> the end result is unapplicable with any existing tool and would confuse\n>> viewers like gitk and gitweb.\n>\n> Well, other tools will still have to be taught about a new syntax, if\n> they're going to use the new flag - just like it was for --rename.\n\nYou are mistaken.  For a patch, you are dealing with two different\nparties: producer and consumer.  If you are adding new feature to the\nproducer, the output format should be desigend to allow the consumer tell\nthat it is something it does not know how to handle.\n\nMarking a non patch with \"diff --git\" to trigger the logic to signal the\nbeginning of a patch to git-apply (and perhaps other tools) is a non\nstarter.\n\nAnd for this \"we are giving a patch that your git-apply can apply and gitk\ncan show, but by the way we also think the whole directory foo moved to\nnew location bar\" is merely an additional information.  You should still\nbe able to apply the patch with tools that are unaware of this new\ndirectory move detection feature.\n"},{"id":"95169","messageId":"20081107221224.GK5158@nan92-1-81-57-214-146.fbx.proxad.net","threadId":"16125","inReplyTo":"7vskq3fdgz.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/3] Introduce rename factorization in diffcore.","fromName":"Yann Dirson","fromEmail":"ydirson@altern.org","sentAt":"2008-11-07T22:12:24Z","receivedAt":"2008-11-07T22:12:24Z","isPatch":true,"sender":{"key":"ydirson@altern.org","avatar":"https://avatars.githubusercontent.com/u/1190950?v=4"},"body":"On Fri, Nov 07, 2008 at 01:11:40PM -0800, Junio C Hamano wrote:\n> Yann Dirson <ydirson@altern.org> writes:\n> \n> > On Fri, Nov 07, 2008 at 12:19:14PM -0800, Junio C Hamano wrote:\n> >> I am afraid that this is totally unacceptable, as you yourself mentioned,\n> >> the end result is unapplicable with any existing tool and would confuse\n> >> viewers like gitk and gitweb.\n> >\n> > Well, other tools will still have to be taught about a new syntax, if\n> > they're going to use the new flag - just like it was for --rename.\n> \n> You are mistaken.  For a patch, you are dealing with two different\n> parties: producer and consumer.  If you are adding new feature to the\n> producer, the output format should be desigend to allow the consumer tell\n> that it is something it does not know how to handle.\n\nAgreed.\n\n> Marking a non patch with \"diff --git\" to trigger the logic to signal the\n> beginning of a patch to git-apply (and perhaps other tools) is a non\n> starter.\n\nThis is not what I meant.\n\n> And for this \"we are giving a patch that your git-apply can apply and gitk\n> can show, but by the way we also think the whole directory foo moved to\n> new location bar\" is merely an additional information.  You should still\n> be able to apply the patch with tools that are unaware of this new\n> directory move detection feature.\n\nI hope I just miss your point.  Letting unaware tools handle such a\npatch \"the right way\" would imply just adding the information \"dir foo\nmoved to bar\", and not removing the individual file moves, which goes\nin the way of the exact reason why I have started to work on this.\n\nCompare this to the addition of the \"file rename\" feature (correct me\nif I'm wrong): it was added without bothering whether plain old\n\"patch\" can deal with it, and when feeding a git diff to patch(1) I\ncannot expect it to DTRT (indeed it applies the content change but not\nthe rename part, without complaining, and an unsuspecting user would\njust be shot in the foot): we are precisely *not* able to apply the\npatch with tools that are unaware of this new file rename feature.\n\nThis is precisely to limit this problem in the future that I proposed\nthis \"diff extension\" stuff in my last mail: limit how the \"backward\ncompatibility\" argument can cripple innovation.\n"},{"id":"95177","messageId":"7vhc6jdrvs.fsf@gitster.siamese.dyndns.org","threadId":"16125","inReplyTo":"20081107221224.GK5158@nan92-1-81-57-214-146.fbx.proxad.net","subject":"Re: [PATCH 2/3] Introduce rename factorization in diffcore.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-11-07T23:43:19Z","receivedAt":"2008-11-07T23:43:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Yann Dirson <ydirson@altern.org> writes:\n\n> I hope I just miss your point.  Letting unaware tools handle such a\n> patch \"the right way\" would imply just adding the information \"dir foo\n> moved to bar\", and not removing the individual file moves, which goes\n> in the way of the exact reason why I have started to work on this.\n\nIf your change is to move a/{1,2,3} to b/{1,2,3} and without content\nchange to a/{1,2} to b/{1,2}, then do you want to say \"a/ moved to b/\nand by the way here is the content change from a/3 to b/3\" without saying\nanything about a/{1,2} and b/{1,2}?\n\nTwo points.\n\n * I do not think it is a good idea to begin with.  If you are to apply\n   such a patch (with git-apply that is updated with your patch to\n   understand that notation) to the exact tree that has only {1,2,3} under\n   a/, you would get an expected result.  But if the recipient of your\n   patch has a/4 (or lacks a/2), there is no cue in the patch that\n   automatically moving a/4 to b/4 may or may not be what is sane (or the\n   patch is unapplicable in general).\n\n * If you give at least the names of paths that were moved without any\n   content changes as I suggested, at least the recipient of your patch\n   can catch the case where his tree is structurally different from what\n   you used to prepare the patch for by noticing the a/2 in the patch that\n   he does not have.\n\nIn addition, if you keep the movements for the paths whose contents did\nnot change, existing tools are perfectly capable of applying (or showing)\nthe output.  I seriously doubt that keeping 4 lines per perfectly moved\npaths is too much a price to pay to keep backward compatibility.\n\n> Compare this to the addition of the \"file rename\" feature (correct me\n> if I'm wrong): it was added without bothering whether plain old\n> \"patch\" can deal with it,...\n\nSorry, but that's an old history whie git-diff output format was rapidly\nbeing developed, when we did not have that many users, and when we did not\nhave an old version of git-apply that did not understand the new feature\nin majority of user's hands.\n\nWe do not have that kind of luxury anymore.  git is much more widespread\nnow and the majority of people use pre-1.6.1 git now (including me ;-)).\n"},{"id":"95181","messageId":"20081108002952.GC4030@nan92-1-81-57-214-146.fbx.proxad.net","threadId":"16125","inReplyTo":"7vhc6jdrvs.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/3] Introduce rename factorization in diffcore.","fromName":"Yann Dirson","fromEmail":"ydirson@altern.org","sentAt":"2008-11-08T00:29:52Z","receivedAt":"2008-11-08T00:29:52Z","isPatch":true,"sender":{"key":"ydirson@altern.org","avatar":"https://avatars.githubusercontent.com/u/1190950?v=4"},"body":"On Fri, Nov 07, 2008 at 03:43:19PM -0800, Junio C Hamano wrote:\n> Yann Dirson <ydirson@altern.org> writes:\n> \n> > I hope I just miss your point.  Letting unaware tools handle such a\n> > patch \"the right way\" would imply just adding the information \"dir foo\n> > moved to bar\", and not removing the individual file moves, which goes\n> > in the way of the exact reason why I have started to work on this.\n> \n> If your change is to move a/{1,2,3} to b/{1,2,3} and without content\n> change to a/{1,2} to b/{1,2}, then do you want to say \"a/ moved to b/\n> and by the way here is the content change from a/3 to b/3\" without saying\n> anything about a/{1,2} and b/{1,2}?\n> \n> Two points.\n> \n>  * I do not think it is a good idea to begin with.  If you are to apply\n>    such a patch (with git-apply that is updated with your patch to\n>    understand that notation) to the exact tree that has only {1,2,3} under\n>    a/, you would get an expected result.  But if the recipient of your\n>    patch has a/4 (or lacks a/2), there is no cue in the patch that\n>    automatically moving a/4 to b/4 may or may not be what is sane (or the\n>    patch is unapplicable in general).\n\nSure in theory.  But in practice I do not remember one time when, if\nall files from one dir are moved in one branch, the files added on\nanother in the same dir were not bound to be moved as well.\n\nAnway, if we feel git-apply should not decide without the user\nknowing, we can make it refuse by default, with options to do either\nway, and one option to ask for each doubtful file instead.\n\n\n>  * If you give at least the names of paths that were moved without any\n>    content changes as I suggested, at least the recipient of your patch\n>    can catch the case where his tree is structurally different from what\n>    you used to prepare the patch for by noticing the a/2 in the patch that\n>    he does not have.\n\nRight.\n\n> In addition, if you keep the movements for the paths whose contents did\n> not change, existing tools are perfectly capable of applying (or showing)\n> the output.  I seriously doubt that keeping 4 lines per perfectly moved\n> paths is too much a price to pay to keep backward compatibility.\n\nOK, so I realize we need 2 things here: one format for diff-exporting\nwith complete info, and one for human viewing (which is, again, the\nprimary reason why I needed this feature, so I'm not very keen on\nletting all this work finally not being useful for me :).  Commands\nfor saving/mailing patches could issue a bold warning if the user\nspecifies the for-human-viewing flag.\n\n\n> > Compare this to the addition of the \"file rename\" feature (correct me\n> > if I'm wrong): it was added without bothering whether plain old\n> > \"patch\" can deal with it,...\n> \n> Sorry, but that's an old history whie git-diff output format was rapidly\n> being developed, when we did not have that many users, and when we did not\n> have an old version of git-apply that did not understand the new feature\n> in majority of user's hands.\n> \n> We do not have that kind of luxury anymore.  git is much more widespread\n> now and the majority of people use pre-1.6.1 git now (including me ;-)).\n\nI was talking about exchanging patches with the non-git part of the\nworld.  The point is that eg. GNU patch still happily accepts\ngit-generated files but produces nonsense using some, exactly because\nit ignores meaningful data which (by design ?) appear to it to be\nlegal to ignore.\n"},{"id":"95182","messageId":"7vd4h7dowd.fsf@gitster.siamese.dyndns.org","threadId":"16125","inReplyTo":"20081108002952.GC4030@nan92-1-81-57-214-146.fbx.proxad.net","subject":"Re: [PATCH 2/3] Introduce rename factorization in diffcore.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-11-08T00:47:46Z","receivedAt":"2008-11-08T00:47:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Yann Dirson <ydirson@altern.org> writes:\n\n> ...  Commands\n> for saving/mailing patches could issue a bold warning if the user\n> specifies the for-human-viewing flag.\n\nIf it is a warning to the user who produces the patch, and not but a\nwarning in the patch text for the user who receives it, it is not useful.\n\n> I was talking about exchanging patches with the non-git part of the\n> world.\n\nRenaming patch needs manual massaging if you want to use GNU patch and\nthat is not a new issue.  People know that.\n\nThe problem is in your example output were that it would break _existing\ngit tools_ with a backward incompatible change, when you did not have to.\nThat's the difference.\n"},{"id":"95183","messageId":"20081108005019.GL5158@nan92-1-81-57-214-146.fbx.proxad.net","threadId":"16125","inReplyTo":"7vd4h7dowd.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/3] Introduce rename factorization in diffcore.","fromName":"Yann Dirson","fromEmail":"ydirson@altern.org","sentAt":"2008-11-08T00:50:19Z","receivedAt":"2008-11-08T00:50:19Z","isPatch":true,"sender":{"key":"ydirson@altern.org","avatar":"https://avatars.githubusercontent.com/u/1190950?v=4"},"body":"On Fri, Nov 07, 2008 at 04:47:46PM -0800, Junio C Hamano wrote:\n> Yann Dirson <ydirson@altern.org> writes:\n> \n> > ...  Commands\n> > for saving/mailing patches could issue a bold warning if the user\n> > specifies the for-human-viewing flag.\n> \n> If it is a warning to the user who produces the patch, and not but a\n> warning in the patch text for the user who receives it, it is not useful.\n\nOK to get everyone warned :)\n"}]}