@matheusbsbs / matheus-bagliano / commits / 75973b2cb5
Fork 0

Fix "add -u" that sometimes fails to resolve unmerged paths

"git add -u" updates the index with the updated contents from the working tree by internally running "diff-files" to grab the set of paths that are different from the index. Then it updates the index entries for the paths that are modified in the working tree, and deletes the index entries for the paths that are deleted in the working tree. It ignored the output from the diff-files that indicated that a path is unmerged. For these paths, it instead relied on the fact that an unmerged path is followed by the result of comparison between stage #2 (ours) and the working tree, and used that to update or delete such a path when it is used to record the resolution of a conflict. As the result, when a path did not have stage #2 (e.g. "we deleted while the other side added"), these unmerged stages were left behind, instead of recording what the user resolved in the working tree. Since we recently fixed "diff-files" to indicate if the corresponding path exists on the working tree for an unmerged path, we do not have to rely on the comparison with stage #2 anymore. We can instead tell the diff-files not to compare with higher stages, and use the unmerged output to update the index to reflect the state of the working tree. The changes to the test vector in t2200 illustrates the nature of the bug and the fix. The test expected stage #1 and #3 entries be left behind, but it was codifying the buggy behaviour. Signed-off-by: Junio C Hamano <gitster@pobox.com>

Junio C Hamano committed Apr 20, 2011 at 18:11 UTC 75973b2cb58bf2b3038c5c214fc0a1b96d6868fe
2 files changed +30 -39
builtin/add.c
+23 -22
@@ -27,6 +27,27 @@ struct update_callback_data
27 int add_errors;
28 };
29
30 +static int fix_unmerged_status(struct diff_filepair *p,
31 + struct update_callback_data *data)
32 +{
33 + if (p->status != DIFF_STATUS_UNMERGED)
34 + return p->status;
35 + if (!(data->flags & ADD_CACHE_IGNORE_REMOVAL) && !p->two->mode)
36 + /*
37 + * This is not an explicit add request, and the
38 + * path is missing from the working tree (deleted)
39 + */
40 + return DIFF_STATUS_DELETED;
41 + else
42 + /*
43 + * Either an explicit add request, or path exists
44 + * in the working tree. An attempt to explicitly
45 + * add a path that does not exist in the working tree
46 + * will be caught as an error by the caller immediately.
47 + */
48 + return DIFF_STATUS_MODIFIED;
49 +}
50 +
51 static void update_callback(struct diff_queue_struct *q,
52 struct diff_options *opt, void *cbdata)
53 {
@@ -36,30 +57,9 @@ static void update_callback(struct diff_queue_struct *q,
57 for (i = 0; i < q->nr; i++) {
58 struct diff_filepair *p = q->queue[i];
59 const char *path = p->one->path;
39 - switch (p->status) {
60 + switch (fix_unmerged_status(p, data)) {
61 default:
62 die("unexpected diff status %c", p->status);
42 - case DIFF_STATUS_UNMERGED:
43 - /*
44 - * ADD_CACHE_IGNORE_REMOVAL is unset if "git
45 - * add -u" is calling us, In such a case, a
46 - * missing work tree file needs to be removed
47 - * if there is an unmerged entry at stage #2,
48 - * but such a diff record is followed by
49 - * another with DIFF_STATUS_DELETED (and if
50 - * there is no stage #2, we won't see DELETED
51 - * nor MODIFIED). We can simply continue
52 - * either way.
53 - */
54 - if (!(data->flags & ADD_CACHE_IGNORE_REMOVAL))
55 - continue;
56 - /*
57 - * Otherwise, it is "git add path" is asking
58 - * to explicitly add it; we fall through. A
59 - * missing work tree file is an error and is
60 - * caught by add_file_to_index() in such a
61 - * case.
62 - */
63 case DIFF_STATUS_MODIFIED:
64 case DIFF_STATUS_TYPE_CHANGED:
65 if (add_file_to_index(&the_index, path, data->flags)) {
@@ -92,6 +92,7 @@ int add_files_to_cache(const char *prefix, const char **pathspec, int flags)
92 data.flags = flags;
93 data.add_errors = 0;
94 rev.diffopt.format_callback_data = &data;
95 + rev.max_count = 0; /* do not compare unmerged paths with stage #2 */
96 run_diff_files(&rev, DIFF_RACY_IS_MODIFIED);
97 return !!data.add_errors;
98 }
t/t2200-add-update.sh
+7 -17
@@ -149,31 +149,21 @@ test_expect_success 'add -u resolves unmerged paths' '
149 echo 3 >path1 &&
150 echo 2 >path3 &&
151 echo 2 >path5 &&
152 - git add -u &&
153 - git ls-files -s path1 path2 path3 path4 path5 path6 >actual &&
154 - {
155 - echo "100644 $three 0 path1"
156 - echo "100644 $one 1 path3"
157 - echo "100644 $one 1 path4"
158 - echo "100644 $one 3 path5"
159 - echo "100644 $one 3 path6"
160 - } >expect &&
161 - test_cmp expect actual &&
152
163 - # Bonus tests. Explicit resolving
164 - git add path3 path5 &&
153 + # Explicit resolving by adding removed paths should fail
154 test_must_fail git add path4 &&
155 test_must_fail git add path6 &&
167 - git rm path4 &&
168 - git rm path6 &&
156
170 - git ls-files -s "path?" >actual &&
157 + # "add -u" should notice removals no matter what stages
158 + # the index entries are in.
159 + git add -u &&
160 + git ls-files -s path1 path2 path3 path4 path5 path6 >actual &&
161 {
162 echo "100644 $three 0 path1"
163 echo "100644 $two 0 path3"
164 echo "100644 $two 0 path5"
175 - } >expect
176 -
165 + } >expect &&
166 + test_cmp expect actual
167 '
168
169 test_expect_success '"add -u non-existent" should fail' '