{"thread":{"id":"27147","subject":"[PATCH 1/2] diffcore: make addremove/modified/unmerged return a filepair","startedAt":"2011-04-21T01:07:45Z","lastAt":"2011-04-21T01:11:19Z","messageCount":2,"participants":["Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"166170","messageId":"7vei4wza1q.fsf@alter.siamese.dyndns.org","threadId":"27147","inReplyTo":null,"subject":"[PATCH 1/2] diffcore: make addremove/modified/unmerged return a filepair","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-04-21T01:07:45Z","receivedAt":"2011-04-21T01:07:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The return value from the underlying diff_queue() function these helper\nfunctions wrap are meant to be further manipulated by the caller, but\nthe helpers discarded it.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * Strictly speaking, I only need this patch for diff_unmerged(), but \n   fixed all the three helpers for consistency.\n\n diff.c     |   29 ++++++++++++++++++-----------\n diff.h     |   10 +++++-----\n revision.c |    6 ++++--\n 3 files changed, 27 insertions(+), 18 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 9a5c77c..41b91cb 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4225,15 +4225,16 @@ static int is_submodule_ignored(const char *path, struct diff_options *options)\n \treturn ignored;\n }\n \n-void diff_addremove(struct diff_options *options,\n+struct diff_filepair *diff_addremove(struct diff_options *options,\n \t\t    int addremove, unsigned mode,\n \t\t    const unsigned char *sha1,\n \t\t    const char *concatpath, unsigned dirty_submodule)\n {\n \tstruct diff_filespec *one, *two;\n+\tstruct diff_filepair *elem;\n \n \tif (S_ISGITLINK(mode) && is_submodule_ignored(concatpath, options))\n-\t\treturn;\n+\t\treturn NULL;\n \n \t/* This may look odd, but it is a preparation for\n \t * feeding \"there are unchanged files which should\n@@ -4253,7 +4254,7 @@ void diff_addremove(struct diff_options *options,\n \n \tif (options->prefix &&\n \t    strncmp(concatpath, options->prefix, options->prefix_length))\n-\t\treturn;\n+\t\treturn NULL;\n \n \tone = alloc_filespec(concatpath);\n \ttwo = alloc_filespec(concatpath);\n@@ -4265,12 +4266,13 @@ void diff_addremove(struct diff_options *options,\n \t\ttwo->dirty_submodule = dirty_submodule;\n \t}\n \n-\tdiff_queue(&diff_queued_diff, one, two);\n+\telem = diff_queue(&diff_queued_diff, one, two);\n \tif (!DIFF_OPT_TST(options, DIFF_FROM_CONTENTS))\n \t\tDIFF_OPT_SET(options, HAS_CHANGES);\n+\treturn elem;\n }\n \n-void diff_change(struct diff_options *options,\n+struct diff_filepair *diff_change(struct diff_options *options,\n \t\t unsigned old_mode, unsigned new_mode,\n \t\t const unsigned char *old_sha1,\n \t\t const unsigned char *new_sha1,\n@@ -4278,10 +4280,11 @@ void diff_change(struct diff_options *options,\n \t\t unsigned old_dirty_submodule, unsigned new_dirty_submodule)\n {\n \tstruct diff_filespec *one, *two;\n+\tstruct diff_filepair *elem;\n \n \tif (S_ISGITLINK(old_mode) && S_ISGITLINK(new_mode) &&\n \t    is_submodule_ignored(concatpath, options))\n-\t\treturn;\n+\t\treturn NULL;\n \n \tif (DIFF_OPT_TST(options, REVERSE_DIFF)) {\n \t\tunsigned tmp;\n@@ -4294,7 +4297,7 @@ void diff_change(struct diff_options *options,\n \n \tif (options->prefix &&\n \t    strncmp(concatpath, options->prefix, options->prefix_length))\n-\t\treturn;\n+\t\treturn NULL;\n \n \tone = alloc_filespec(concatpath);\n \ttwo = alloc_filespec(concatpath);\n@@ -4303,25 +4306,29 @@ void diff_change(struct diff_options *options,\n \tone->dirty_submodule = old_dirty_submodule;\n \ttwo->dirty_submodule = new_dirty_submodule;\n \n-\tdiff_queue(&diff_queued_diff, one, two);\n+\telem = diff_queue(&diff_queued_diff, one, two);\n \tif (!DIFF_OPT_TST(options, DIFF_FROM_CONTENTS))\n \t\tDIFF_OPT_SET(options, HAS_CHANGES);\n+\treturn elem;\n }\n \n-void diff_unmerge(struct diff_options *options,\n+struct diff_filepair *diff_unmerge(struct diff_options *options,\n \t\t  const char *path,\n \t\t  unsigned mode, const unsigned char *sha1)\n {\n \tstruct diff_filespec *one, *two;\n+\tstruct diff_filepair *elem;\n \n \tif (options->prefix &&\n \t    strncmp(path, options->prefix, options->prefix_length))\n-\t\treturn;\n+\t\treturn NULL;\n \n \tone = alloc_filespec(path);\n \ttwo = alloc_filespec(path);\n \tfill_filespec(one, sha1, mode);\n-\tdiff_queue(&diff_queued_diff, one, two)->is_unmerged = 1;\n+\telem = diff_queue(&diff_queued_diff, one, two);\n+\telem->is_unmerged = 1;\n+\treturn elem;\n }\n \n static char *run_textconv(const char *pgm, struct diff_filespec *spec,\ndiff --git a/diff.h b/diff.h\nindex bf2f44d..d71965f 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -13,14 +13,14 @@ struct strbuf;\n struct diff_filespec;\n struct userdiff_driver;\n \n-typedef void (*change_fn_t)(struct diff_options *options,\n+typedef struct diff_filepair *(*change_fn_t)(struct diff_options *options,\n \t\t unsigned old_mode, unsigned new_mode,\n \t\t const unsigned char *old_sha1,\n \t\t const unsigned char *new_sha1,\n \t\t const char *fullpath,\n \t\t unsigned old_dirty_submodule, unsigned new_dirty_submodule);\n \n-typedef void (*add_remove_fn_t)(struct diff_options *options,\n+typedef struct diff_filepair *(*add_remove_fn_t)(struct diff_options *options,\n \t\t    int addremove, unsigned mode,\n \t\t    const unsigned char *sha1,\n \t\t    const char *fullpath, unsigned dirty_submodule);\n@@ -196,20 +196,20 @@ extern void diff_tree_combined_merge(const unsigned char *sha1, int, struct rev_\n \n void diff_set_mnemonic_prefix(struct diff_options *options, const char *a, const char *b);\n \n-extern void diff_addremove(struct diff_options *,\n+extern struct diff_filepair *diff_addremove(struct diff_options *,\n \t\t\t   int addremove,\n \t\t\t   unsigned mode,\n \t\t\t   const unsigned char *sha1,\n \t\t\t   const char *fullpath, unsigned dirty_submodule);\n \n-extern void diff_change(struct diff_options *,\n+extern struct diff_filepair *diff_change(struct diff_options *,\n \t\t\tunsigned mode1, unsigned mode2,\n \t\t\tconst unsigned char *sha1,\n \t\t\tconst unsigned char *sha2,\n \t\t\tconst char *fullpath,\n \t\t\tunsigned dirty_submodule1, unsigned dirty_submodule2);\n \n-extern void diff_unmerge(struct diff_options *,\n+extern struct diff_filepair *diff_unmerge(struct diff_options *,\n \t\t\t const char *path,\n \t\t\t unsigned mode,\n \t\t\t const unsigned char *sha1);\ndiff --git a/revision.c b/revision.c\nindex b1c1890..614e5d9 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -276,7 +276,7 @@ static int everybody_uninteresting(struct commit_list *orig)\n  */\n static int tree_difference = REV_TREE_SAME;\n \n-static void file_add_remove(struct diff_options *options,\n+static struct diff_filepair *file_add_remove(struct diff_options *options,\n \t\t    int addremove, unsigned mode,\n \t\t    const unsigned char *sha1,\n \t\t    const char *fullpath, unsigned dirty_submodule)\n@@ -286,9 +286,10 @@ static void file_add_remove(struct diff_options *options,\n \ttree_difference |= diff;\n \tif (tree_difference == REV_TREE_DIFFERENT)\n \t\tDIFF_OPT_SET(options, HAS_CHANGES);\n+\treturn NULL;\n }\n \n-static void file_change(struct diff_options *options,\n+static struct diff_filepair *file_change(struct diff_options *options,\n \t\t unsigned old_mode, unsigned new_mode,\n \t\t const unsigned char *old_sha1,\n \t\t const unsigned char *new_sha1,\n@@ -297,6 +298,7 @@ static void file_change(struct diff_options *options,\n {\n \ttree_difference = REV_TREE_DIFFERENT;\n \tDIFF_OPT_SET(options, HAS_CHANGES);\n+\treturn NULL;\n }\n \n static int rev_compare_tree(struct rev_info *revs, struct commit *parent, struct commit *commit)\n-- \n1.7.5.rc3\n"},{"id":"166171","messageId":"7vaafkz9vs.fsf@alter.siamese.dyndns.org","threadId":"27147","inReplyTo":"7vei4wza1q.fsf@alter.siamese.dyndns.org","subject":"[PATCH 2/2] fix \"add -u\" that sometimes failes to resolve unmerged paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-04-21T01:11:19Z","receivedAt":"2011-04-21T01:11:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"git add -u\" updates the index with the updated contents from the working\ntree by internally running \"diff-files\" to grab the set of paths that are\ndifferent from the index, and updates the paths that are modified, and\ndeletes the paths that are deleted.\n\nIt ignored the diff-files output that indicated a path is unmerged, and\nrelied on the fact that an unmerged path is followed by the result of\ncomparison between stage #2 (ours) and the working tree and used that to\nupdate or delete such a path when it is used to record the resolution of a\nconflict.  As the result, when a path did not have stage #2 (e.g. \"we\ndeleted while the other side added\"), these unmerged stages were left\nbehind, instead of recording what the user resolved in the working tree.\n\nThis teaches the diff-files machinery to record if an unmerged path has a\ncorresponding file in the working tree, teaches \"add -u\" codepath to tell\nthe diff-files not to show the extra comparison with stage #2 (i.e. we get\nonly the \"unmerged\" report), and upon seeing an unmerged path, either\nupdate (when the file exists in the working tree) or delete (otherwise).\n\nThe changes to the test vector in t2200 illustrates the nature of the bug\nand the fix.  The test expected stage #1 and #3 entries be left behind,\nbut it was codifying the buggy behaviour.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * Storing the working tree information in an unmerged index entry feels\n   somewhat dirty, as it breaks the expectation that \"U\" entries would\n   show zero mode and 0{40} SHA-1 people might have gained from the\n   traditional behaviour.\n\n   Considering that a similar change was made to diff-index in e9c8409\n   (diff-index --cached --raw: show tree entry on the LHS for unmerged\n   entries., 2007-01-05) and later in d1f2d7e (Make run_diff_index() use\n   unpack_trees(), not read_tree(), 2008-01-19) and as the result of these\n   changes, \"diff-index --cached $tree\" shows the information from the\n   index on the preimage side ever since, and nobody complained when or\n   after the change was made, I am hoping that nobody relies on the\n   current behaviour of \"diff --raw\" family for the unmerged paths.\n\n   If anything, the updated behaviour of \"diff-files\" is more consistent\n   with the other parts of the system.  When showing the working tree\n   status in the diff --raw output, we do not show its object name, but we\n   do show its mode.  We didn't show its mode when the corresponding index\n   entry is unmerged before this fix.\n\n   When showing an unmerged index entry in the diff --raw output as a\n   single entry, we do not show its mode nor object name, as it stands for\n   \"fate not determined yet\".  That is not something this patch changes.\n\n builtin/add.c         |   39 +++++++++++++++++----------------------\n diff-lib.c            |    9 +++++++--\n t/t2200-add-update.sh |   24 +++++++-----------------\n 3 files changed, 31 insertions(+), 41 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex 56a4e0a..c85a3ac 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -27,6 +27,21 @@ struct update_callback_data\n \tint add_errors;\n };\n \n+static int fix_unmerged_status(struct diff_filepair *p,\n+\t\t\t       struct update_callback_data *data)\n+{\n+\tif (p->status != DIFF_STATUS_UNMERGED)\n+\t\treturn p->status;\n+\tif (!(data->flags & ADD_CACHE_IGNORE_REMOVAL) && !p->two->mode)\n+\t\t/*\n+\t\t * This is not an explicit add request,\n+\t\t * and the path is missing from the working tree.\n+\t\t */\n+\t\treturn DIFF_STATUS_DELETED;\n+\telse\n+\t\treturn DIFF_STATUS_MODIFIED;\n+}\n+\n static void update_callback(struct diff_queue_struct *q,\n \t\t\t    struct diff_options *opt, void *cbdata)\n {\n@@ -36,30 +51,9 @@ static void update_callback(struct diff_queue_struct *q,\n \tfor (i = 0; i < q->nr; i++) {\n \t\tstruct diff_filepair *p = q->queue[i];\n \t\tconst char *path = p->one->path;\n-\t\tswitch (p->status) {\n+\t\tswitch (fix_unmerged_status(p, data)) {\n \t\tdefault:\n \t\t\tdie(\"unexpected diff status %c\", p->status);\n-\t\tcase DIFF_STATUS_UNMERGED:\n-\t\t\t/*\n-\t\t\t * ADD_CACHE_IGNORE_REMOVAL is unset if \"git\n-\t\t\t * add -u\" is calling us, In such a case, a\n-\t\t\t * missing work tree file needs to be removed\n-\t\t\t * if there is an unmerged entry at stage #2,\n-\t\t\t * but such a diff record is followed by\n-\t\t\t * another with DIFF_STATUS_DELETED (and if\n-\t\t\t * there is no stage #2, we won't see DELETED\n-\t\t\t * nor MODIFIED).  We can simply continue\n-\t\t\t * either way.\n-\t\t\t */\n-\t\t\tif (!(data->flags & ADD_CACHE_IGNORE_REMOVAL))\n-\t\t\t\tcontinue;\n-\t\t\t/*\n-\t\t\t * Otherwise, it is \"git add path\" is asking\n-\t\t\t * to explicitly add it; we fall through.  A\n-\t\t\t * missing work tree file is an error and is\n-\t\t\t * caught by add_file_to_index() in such a\n-\t\t\t * case.\n-\t\t\t */\n \t\tcase DIFF_STATUS_MODIFIED:\n \t\tcase DIFF_STATUS_TYPE_CHANGED:\n \t\t\tif (add_file_to_index(&the_index, path, data->flags)) {\n@@ -92,6 +86,7 @@ int add_files_to_cache(const char *prefix, const char **pathspec, int flags)\n \tdata.flags = flags;\n \tdata.add_errors = 0;\n \trev.diffopt.format_callback_data = &data;\n+\trev.max_count = 0; /* do not show extra M/D after U */\n \trun_diff_files(&rev, DIFF_RACY_IS_MODIFIED);\n \treturn !!data.add_errors;\n }\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 392ce2b..f0ce473 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -111,7 +111,9 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \n \t\tif (ce_stage(ce)) {\n \t\t\tstruct combine_diff_path *dpath;\n+\t\t\tstruct diff_filepair *dp;\n \t\t\tint num_compare_stages = 0;\n+\t\t\tunsigned int wt_mode = 0;\n \t\t\tsize_t path_len;\n \n \t\t\tpath_len = ce_namelen(ce);\n@@ -129,7 +131,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \n \t\t\tchanged = check_removed(ce, &st);\n \t\t\tif (!changed)\n-\t\t\t\tdpath->mode = ce_mode_from_stat(ce, st.st_mode);\n+\t\t\t\twt_mode = ce_mode_from_stat(ce, st.st_mode);\n \t\t\telse {\n \t\t\t\tif (changed < 0) {\n \t\t\t\t\tperror(ce->name);\n@@ -137,7 +139,9 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t\t\t}\n \t\t\t\tif (silent_on_removed)\n \t\t\t\t\tcontinue;\n+\t\t\t\twt_mode = 0;\n \t\t\t}\n+\t\t\tdpath->mode = wt_mode;\n \n \t\t\twhile (i < entries) {\n \t\t\t\tstruct cache_entry *nce = active_cache[i];\n@@ -183,7 +187,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t\t * Show the diff for the 'ce' if we found the one\n \t\t\t * from the desired stage.\n \t\t\t */\n-\t\t\tdiff_unmerge(&revs->diffopt, ce->name, 0, null_sha1);\n+\t\t\tdp = diff_unmerge(&revs->diffopt, ce->name, 0, null_sha1);\n+\t\t\tdp->two->mode = wt_mode;\n \t\t\tif (ce_stage(ce) != diff_unmerged_stage)\n \t\t\t\tcontinue;\n \t\t}\ndiff --git a/t/t2200-add-update.sh b/t/t2200-add-update.sh\nindex 2ad2819..d3bb9fa 100755\n--- a/t/t2200-add-update.sh\n+++ b/t/t2200-add-update.sh\n@@ -149,31 +149,21 @@ test_expect_success 'add -u resolves unmerged paths' '\n \techo 3 >path1 &&\n \techo 2 >path3 &&\n \techo 2 >path5 &&\n-\tgit add -u &&\n-\tgit ls-files -s path1 path2 path3 path4 path5 path6 >actual &&\n-\t{\n-\t\techo \"100644 $three 0\tpath1\"\n-\t\techo \"100644 $one 1\tpath3\"\n-\t\techo \"100644 $one 1\tpath4\"\n-\t\techo \"100644 $one 3\tpath5\"\n-\t\techo \"100644 $one 3\tpath6\"\n-\t} >expect &&\n-\ttest_cmp expect actual &&\n \n-\t# Bonus tests.  Explicit resolving\n-\tgit add path3 path5 &&\n+\t# Explicit resolving by adding removed paths should fail\n \ttest_must_fail git add path4 &&\n \ttest_must_fail git add path6 &&\n-\tgit rm path4 &&\n-\tgit rm path6 &&\n \n-\tgit ls-files -s \"path?\" >actual &&\n+\t# \"add -u\" should notice removals no matter what stages\n+\t# the index entries are in.\n+\tgit add -u &&\n+\tgit ls-files -s path1 path2 path3 path4 path5 path6 >actual &&\n \t{\n \t\techo \"100644 $three 0\tpath1\"\n \t\techo \"100644 $two 0\tpath3\"\n \t\techo \"100644 $two 0\tpath5\"\n-\t} >expect\n-\n+\t} >expect &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success '\"add -u non-existent\" should fail' '\n"}]}