merge-recursive: improve add_cacheinfo error handling

Four closely related changes all with the purpose of fixing error handling in this function: - fix reported function name in add_cacheinfo error messages - differentiate between the two error messages - abort early when we hit the error (stop ignoring return code) - mark a test which was hitting this error as failing until we get the right fix In more detail... In commit 0424138d5715 ("Fix bogus error message from merge-recursive error path", 2007-04-01), it was noted that the name of the function which the error message claimed it was reported from did not match the actual function name. This was changed to something closer to the real function name, but it still didn't match the actual function name. Fix the reported name to match. Second, the two errors in this function had identical messages, preventing us from knowing which error had been triggered. Add a couple words to the second error message to differentiate the two. Next, make sure callers do not ignore the return code so that it will stop processing further entries (processing further entries could result in more output which could cause the error to scroll off the screen, or at least be missed by the user) and make it clear the error is the cause of the early abort. These errors should never be triggered in production; if either one is, it represents a bug in the calling path somewhere and is likely to have resulted in mis-merged content. The combination of ignoring of the return code and continuing to print other standard messages after hitting the error resulted in the following bug report from Junio: "...the command pretends that everything went well and merged cleanly in that path...[Behaving] in a buggy and unexplainable way is bad enough, doing so silently is unexcusable." Fix this. Finally, there was one test in the testsuite that did hit this error path, but was passing anyway. This would have been easy to miss since it had a test_must_fail and thus could have failed for the wrong reason, but in a separate testing step I added an intentional NULL-dereference to the codepath where these error messages are printed in order to flush out such cases. I could modify that test to explicitly check for this error and fail the test if it is hit, but since this test operates in a bit of a gray area and needed other changes, I went for a different fix. The gray area this test operates in is the following: If the merge of a certain file results in the same version of the file that existed in HEAD, but there are dirty modifications to the file, is that an error with a "Refusing to overwrite existing file" expected, or a case where the merge should succeed since we shouldn't have to touch the dirty file anyway? Recent discussion on the list leaned towards saying it should be a success. Therefore, change the expected behavior of this test to match. As a side effect, this makes the failed-due-to-hitting-add_cacheinfo-error very clear, and we can mark the test as test_expect_failure. A subsequent commit will implement the necessary changes to get this test to pass again. Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Elijah Newren committed Apr 19, 2018 at 10:58 UTC fd53b7ffd1bd499d24fa48b43a79703355bde512
2 files changed +11 -9
merge-recursive.c
+8 -5
@@ -316,7 +316,7 @@ static int add_cacheinfo(struct merge_options *o,
316
317 ce = make_cache_entry(mode, oid ? oid->hash : null_sha1, path, stage, 0);
318 if (!ce)
319 - return err(o, _("addinfo_cache failed for path '%s'"), path);
319 + return err(o, _("add_cacheinfo failed for path '%s'; merge aborting."), path);
320
321 ret = add_cache_entry(ce, options);
322 if (refresh) {
@@ -324,7 +324,7 @@ static int add_cacheinfo(struct merge_options *o,
324
325 nce = refresh_cache_entry(ce, CE_MATCH_REFRESH | CE_MATCH_IGNORE_MISSING);
326 if (!nce)
327 - return err(o, _("addinfo_cache failed for path '%s'"), path);
327 + return err(o, _("add_cacheinfo failed to refresh for path '%s'; merge aborting."), path);
328 if (nce != ce)
329 ret = add_cache_entry(nce, options);
330 }
@@ -942,7 +942,9 @@ static int update_file_flags(struct merge_options *o,
942 }
943 update_index:
944 if (!ret && update_cache)
945 - add_cacheinfo(o, mode, oid, path, 0, update_wd, ADD_CACHE_OK_TO_ADD);
945 + if (add_cacheinfo(o, mode, oid, path, 0, update_wd,
946 + ADD_CACHE_OK_TO_ADD))
947 + return -1;
948 return ret;
949 }
950
@@ -2783,8 +2785,9 @@ static int merge_content(struct merge_options *o,
2785 */
2786 path_renamed_outside_HEAD = !path2 || !strcmp(path, path2);
2787 if (!path_renamed_outside_HEAD) {
2786 - add_cacheinfo(o, mfi.mode, &mfi.oid, path,
2787 - 0, (!o->call_depth), 0);
2788 + if (add_cacheinfo(o, mfi.mode, &mfi.oid, path,
2789 + 0, (!o->call_depth), 0))
2790 + return -1;
2791 return mfi.clean;
2792 }
2793 } else
t/t3501-revert-cherry-pick.sh
+3 -4
@@ -141,7 +141,7 @@ test_expect_success 'cherry-pick "-" works with arguments' '
141 test_cmp expect actual
142 '
143
144 -test_expect_success 'cherry-pick works with dirty renamed file' '
144 +test_expect_failure 'cherry-pick works with dirty renamed file' '
145 test_commit to-rename &&
146 git checkout -b unrelated &&
147 test_commit unrelated &&
@@ -150,9 +150,8 @@ test_expect_success 'cherry-pick works with dirty renamed file' '
150 test_tick &&
151 git commit -m renamed &&
152 echo modified >renamed &&
153 - test_must_fail git cherry-pick refs/heads/unrelated >out &&
154 - test_i18ngrep "Refusing to lose dirty file at renamed" out &&
155 - test $(git rev-parse :0:renamed) = $(git rev-parse HEAD^:to-rename.t) &&
153 + git cherry-pick refs/heads/unrelated >out &&
154 + test $(git rev-parse :0:renamed) = $(git rev-parse HEAD~2:to-rename.t) &&
155 grep -q "^modified$" renamed
156 '
157