From fd53b7ffd1bd499d24fa48b43a79703355bde512 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Thu, 19 Apr 2018 10:58:16 -0700 Subject: [PATCH] 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 Signed-off-by: Junio C Hamano --- merge-recursive.c | 13 ++++++++----- t/t3501-revert-cherry-pick.sh | 7 +++---- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/merge-recursive.c b/merge-recursive.c index d079d6783b..d3771e3626 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -316,7 +316,7 @@ static int add_cacheinfo(struct merge_options *o, ce = make_cache_entry(mode, oid ? oid->hash : null_sha1, path, stage, 0); if (!ce) - return err(o, _("addinfo_cache failed for path '%s'"), path); + return err(o, _("add_cacheinfo failed for path '%s'; merge aborting."), path); ret = add_cache_entry(ce, options); if (refresh) { @@ -324,7 +324,7 @@ static int add_cacheinfo(struct merge_options *o, nce = refresh_cache_entry(ce, CE_MATCH_REFRESH | CE_MATCH_IGNORE_MISSING); if (!nce) - return err(o, _("addinfo_cache failed for path '%s'"), path); + return err(o, _("add_cacheinfo failed to refresh for path '%s'; merge aborting."), path); if (nce != ce) ret = add_cache_entry(nce, options); } @@ -942,7 +942,9 @@ static int update_file_flags(struct merge_options *o, } update_index: if (!ret && update_cache) - add_cacheinfo(o, mode, oid, path, 0, update_wd, ADD_CACHE_OK_TO_ADD); + if (add_cacheinfo(o, mode, oid, path, 0, update_wd, + ADD_CACHE_OK_TO_ADD)) + return -1; return ret; } @@ -2783,8 +2785,9 @@ static int merge_content(struct merge_options *o, */ path_renamed_outside_HEAD = !path2 || !strcmp(path, path2); if (!path_renamed_outside_HEAD) { - add_cacheinfo(o, mfi.mode, &mfi.oid, path, - 0, (!o->call_depth), 0); + if (add_cacheinfo(o, mfi.mode, &mfi.oid, path, + 0, (!o->call_depth), 0)) + return -1; return mfi.clean; } } else diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh index c9a1f783f5..3871807d09 100755 --- a/t/t3501-revert-cherry-pick.sh +++ b/t/t3501-revert-cherry-pick.sh @@ -141,7 +141,7 @@ test_expect_success 'cherry-pick "-" works with arguments' ' test_cmp expect actual ' -test_expect_success 'cherry-pick works with dirty renamed file' ' +test_expect_failure 'cherry-pick works with dirty renamed file' ' test_commit to-rename && git checkout -b unrelated && test_commit unrelated && @@ -150,9 +150,8 @@ test_expect_success 'cherry-pick works with dirty renamed file' ' test_tick && git commit -m renamed && echo modified >renamed && - test_must_fail git cherry-pick refs/heads/unrelated >out && - test_i18ngrep "Refusing to lose dirty file at renamed" out && - test $(git rev-parse :0:renamed) = $(git rev-parse HEAD^:to-rename.t) && + git cherry-pick refs/heads/unrelated >out && + test $(git rev-parse :0:renamed) = $(git rev-parse HEAD~2:to-rename.t) && grep -q "^modified$" renamed '