Fix and improve some error codepaths in merge-ort
cover Fix and improve some error codepaths in merge-ort
This series started as a just a fix for the abort hit in merge-ort when custom merge drivers error out (see
https://lore.kernel.org/git/[email protected]/).
However, while working on that, I found a few other issues around error codepaths in merge-ort. So this series:
* Patches 1-2: fix the reported abort problem
* Patches 3-4: make code in handle_content_merges() easier to handle when we hit errors
* Patch 5: fix a misleading comment
* Patches 6-7: make error handling (immediate print vs. letting callers get the error information) more consistent
The last two patches change the behavior slightly for error codepaths, and there's a question about whether we should show only the error messages that caused an early termination of the merge, or if we should also show any conflict messages for other paths that were handled before we hit the early termination. These patches made a decision but feel free to take those last two patches as more of an RFC.
Elijah Newren (7):
merge-ort: extract handling of priv member into reusable function
merge-ort: maintain expected invariant for priv member
merge-ort: fix type of local 'clean' var in handle_content_merge()
merge-ort: clearer propagation of failure-to-function from
merge_submodule
merge-ort: loosen commented requirements
merge-ort: upon merge abort, only show messages causing the abort
merge-ort: convert more error() cases to path_msg()
merge-ort.c | 167 +++++++++++++++++++++++++++++++----------- t/t6406-merge-attr.sh | 42 ++++++++++- 2 files changed, 164 insertions(+), 45 deletions(-)
base-commit: 8d94cfb54504f2ec9edc7ca3eb5c29a3dd3675ae
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1748%2Fnewren%2Ffix-error-cases-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1748/newren/fix-error-cases-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1748
Signature · 1 line
gitgitgadget
PATCH 1/7 merge-ort: extract handling of priv member into reusable function
From: Elijah Newren <[email protected]>
In preparation for a subsequent commit which will ensure we do not forget to maintain our invariants for the priv member in error codepaths, extract the necessary functionality out into a separate function. This change is cosmetic at this point, and introduces no changes beyond an extra assertion sanity check.
--- merge-ort.c | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-)
merge-ort.c +23 -5
@@ -5000,6 +5000,28 @@ static void merge_check_renames_reusable(struct merge_result *result,/*** Function Grouping: merge_incore_*() and their internal variants ***/ +static void move_opt_priv_to_result_priv (struct merge_options *opt, +struct merge_result *result) +{ +/* +* opt->priv and result->priv are a bit weird. opt->priv contains +* information that we can re-use in subsequent merge operations to +* enable our cached renames optimization. The best way to provide +* that to subsequent merges is putting it in result->priv. +* However, putting it directly there would mean retrofitting lots +* of functions in this file to also take a merge_result pointer, +* which is ugly and annoying. So, we just make sure at the end of +* the merge (the outer merge if there are internal recursive ones) +* to move it. +*/ +assert (opt->priv && !result->priv); +if (!opt->priv->call_depth) { + result->priv = opt->priv; + result->_properly_initialized =RESULT_INITIALIZED ; + opt->priv =NULL ; + } +} +/* * Originally from merge_trees_internal(); heavily adapted, though. */
@@ -5060,11 +5082,7 @@ static void merge_ort_nonrecursive_internal(struct merge_options *opt,/* existence of conflicted entries implies unclean */ result->clean &=strmap_empty (&opt->priv->conflicted); } -if (!opt->priv->call_depth) { - result->priv = opt->priv; - result->_properly_initialized =RESULT_INITIALIZED ; - opt->priv =NULL ; - } +move_opt_priv_to_result_priv (opt, result); }/*
Signature · 1 line
gitgitgadget
Re: [PATCH 1/7] merge-ort: extract handling of priv member into reusable function
Quoted 26 lines · Elijah Newren via GitGitGadget
merge-ort.c +22
-0+static void move_opt_priv_to_result_priv (struct merge_options *opt, +struct merge_result *result) +{ +/* +* opt->priv and result->priv are a bit weird. opt->priv contains +* information that we can re-use in subsequent merge operations to +* enable our cached renames optimization. The best way to provide +* that to subsequent merges is putting it in result->priv. +* However, putting it directly there would mean retrofitting lots +* of functions in this file to also take a merge_result pointer, +* which is ugly and annoying. So, we just make sure at the end of +* the merge (the outer merge if there are internal recursive ones) +* to move it. +*/ +assert (opt->priv && !result->priv); +if (!opt->priv->call_depth) { + result->priv = opt->priv; + result->_properly_initialized =RESULT_INITIALIZED ; + opt->priv =NULL ; + } +} +/* * Originally from merge_trees_internal(); heavily adapted, though. */
merge-ort.c static void merge_ort_nonrecursive_internal(struct merge_options *opt, · line 5086
Quoted 10 lines
merge-ort.c +1
-5@@ -5060,11 +5082,7 @@ static void merge_ort_nonrecursive_internal(struct merge_options *opt,/* existence of conflicted entries implies unclean */ result->clean &=strmap_empty (&opt->priv->conflicted); } -if (!opt->priv->call_depth) { - result->priv = opt->priv; - result->_properly_initialized =RESULT_INITIALIZED ; - opt->priv =NULL ; - } +move_opt_priv_to_result_priv (opt, result); }
I have a feeling that making it the caller's responsibility to check "are we doing the outermost merge?" and not the callee's problem would result in a better code organization. If we write
if (!opt->priv->call_depth) move_opt_priv_to_result_priv(opt, result);
then for this call site, it is still crystal clear that this will happen only at the outermost level. The new caller you add in the next step would also be simpler to reason about.
You have the assert() to make sure callers do not call the "move" helper at a wrong place already, and if the organization in this patch somehow comes from a desire that the "move" is done only at the outermost level (and "or immediately before an error causes the whole thing to abort" after the next patch), it does not have to be a silent "if call_depth is not zero we return silently", but another assert() that insists that the callers are allowed to call it under these two specific conditions.
Thanks.
Re: [PATCH 1/7] merge-ort: extract handling of priv member into reusable function
Quoted 43 lines · Junio C Hamano
Quoted 26 lines · Elijah Newren via GitGitGadget
merge-ort.c +22
-0+static void move_opt_priv_to_result_priv (struct merge_options *opt, +struct merge_result *result) +{ +/* +* opt->priv and result->priv are a bit weird. opt->priv contains +* information that we can re-use in subsequent merge operations to +* enable our cached renames optimization. The best way to provide +* that to subsequent merges is putting it in result->priv. +* However, putting it directly there would mean retrofitting lots +* of functions in this file to also take a merge_result pointer, +* which is ugly and annoying. So, we just make sure at the end of +* the merge (the outer merge if there are internal recursive ones) +* to move it. +*/ +assert (opt->priv && !result->priv); +if (!opt->priv->call_depth) { + result->priv = opt->priv; + result->_properly_initialized =RESULT_INITIALIZED ; + opt->priv =NULL ; + } +} +/* * Originally from merge_trees_internal(); heavily adapted, though. */ merge-ort.c static void merge_ort_nonrecursive_internal(struct merge_options *opt, · line 5086
Quoted 10 lines
merge-ort.c +1
-5@@ -5060,11 +5082,7 @@ static void merge_ort_nonrecursive_internal(struct merge_options *opt,/* existence of conflicted entries implies unclean */ result->clean &=strmap_empty (&opt->priv->conflicted); } -if (!opt->priv->call_depth) { - result->priv = opt->priv; - result->_properly_initialized =RESULT_INITIALIZED ; - opt->priv =NULL ; - } +move_opt_priv_to_result_priv (opt, result); }I have a feeling that making it the caller's responsibility to check "are we doing the outermost merge?" and not the callee's problem would result in a better code organization. If we write
Quoted 6 lines
if (!opt->priv->call_depth) move_opt_priv_to_result_priv(opt, result);then for this call site, it is still crystal clear that this will happen only at the outermost level. The new caller you add in the next step would also be simpler to reason about.
I had the same thought. Calling the function move_opt_priv_to_result_priv() seems to indicate that reuslt->priv will definitely be updated to opt->priv.
Another approach would be to rename the function maybe_move_opt_priv_to_result_priv() and have it be a noop if opt->priv->call_depth is non-zero.
But this is all fairly philosophical ;-). I do not really mind or feel strongly either way whatsoever.
Thanks,
Taylor
PATCH 2/7 merge-ort: maintain expected invariant for priv member
In reply to Elijah Newren via GitGitGadgetFrom: Elijah Newren <[email protected]>
The calling convention for the merge machinery is
One call to init_merge_options()
One or more calls to merge_incore_[non]recursive()
One call to merge_finalize()
(possibly indirectly via merge_switch_to_result())
Both merge_switch_to_result() and merge_finalize() expect
opt->priv == NULL && result->priv != NULL
which is supposed to be set up by our move_opt_priv_to_result_priv() function. However, two codepath dealing with error cases did not execute this necessary logic, which could result in assertion failures (or, if assertions were compiled out, could result in segfaults). Fix the oversight and add a test that would have caught one of these problems.
While at it, also tighten an existing test for a non-recursive merge to verify that it fails correctly, i.e. with the expected exit code rather than with an assertion failure.
--- merge-ort.c | 3 ++- t/t6406-merge-attr.sh | 42 +++++++++++++++++++++++++++++++++++++++++- 2 files changed, 43 insertions(+), 2 deletions(-)
merge-ort.c +2 -1
@@ -5015,7 +5015,7 @@ static void move_opt_priv_to_result_priv(struct merge_options *opt,* to move it. */ assert (opt->priv && !result->priv); -if (!opt->priv->call_depth) { +if (!opt->priv->call_depth || result->clean <0 ) { result->priv = opt->priv; result->_properly_initialized =RESULT_INITIALIZED ; opt->priv =NULL ;
@@ -5052,6 +5052,7 @@ static void merge_ort_nonrecursive_internal(struct merge_options *opt,oid_to_hex (&side1->object.oid), oid_to_hex(&side2->object.oid)); result->clean =-1 ; +move_opt_priv_to_result_priv (opt, result);return ; }trace2_region_leave ("merge" ,"collect_merge_info" , opt->repo);
t/t6406-merge-attr.sh +41 -1
52 lines not on this page · raw message
Signature · 1 line
gitgitgadget
Re: [PATCH 2/7] merge-ort: maintain expected invariant for priv member
Quoted 11 lines · Elijah Newren via GitGitGadget
From: Elijah Newren <[email protected]>
The calling convention for the merge machinery is
One call toinit_merge_options()
One or more calls to merge_incore_[non]recursive()
One call tomerge_finalize()
(possibly indirectly viamerge_switch_to_result())
Bothmerge_switch_to_result()andmerge_finalize()expect
opt->priv== NULL &&result->priv!= NULL
which is supposed to be set up by ourmove_opt_priv_to_result_priv()function. However, two codepath dealing with error cases did not
s/codepath/&s/ ?
Quoted 8 lines
execute this necessary logic, which could result in assertion failures (or, if assertions were compiled out, could result in segfaults). Fix the oversight and add a test that would have caught one of these problems.
While at it, also tighten an existing test for a non-recursive merge to verify that it fails correctly, i.e. with the expected exit code rather than with an assertion failure.
I suspect that this test was flaky, too, since if the assertion errors were compiled out and it died via a segfault, the test would have failed outright as test_must_fail does not allow for segfaults typically.
So I'm glad to see us tightening up that area of the test suite.
Quoted 51 lines
Reported-by Matt Cree <[email protected]>Signed-off-by Elijah Newren <[email protected]>--- merge-ort.c | 3 ++- t/t6406-merge-attr.sh | 42 +++++++++++++++++++++++++++++++++++++++++- 2 files changed, 43 insertions(+), 2 deletions(-)merge-ort.c +2
-1@@ -5015,7 +5015,7 @@ static void move_opt_priv_to_result_priv(struct merge_options *opt,* to move it. */ assert (opt->priv && !result->priv); -if (!opt->priv->call_depth) { +if (!opt->priv->call_depth || result->clean <0 ) { result->priv = opt->priv; result->_properly_initialized =RESULT_INITIALIZED ; opt->priv =NULL ;@@ -5052,6 +5052,7 @@ static void merge_ort_nonrecursive_internal(struct merge_options *opt,oid_to_hex (&side1->object.oid), oid_to_hex(&side2->object.oid)); result->clean =-1 ; +move_opt_priv_to_result_priv (opt, result);return ; }trace2_region_leave ("merge" ,"collect_merge_info" , opt->repo);t/t6406-merge-attr.sh +7
-1@@ -185,7 +185,7 @@ test_expect_success !WINDOWS 'custom merge driver that is killed with a signal' >./please-abort && echo "* merge=custom" >.gitattributes && - test_must_fail git merge main 2>err && + test_expect_code 2 git merge main 2>err && grep "^error: failed to execute internal merge" err && git ls-files -u >output && git diff --name-only HEAD >>output &&@@ -261,4 +261,44 @@ test_expect_success 'binary files with union attribute' ' grep -i "warning.*cannot merge.*HEAD vs. bin-main" output ' +test_expect_success !WINDOWS 'custom merge driver that is killed with a signal on recursive merge' ' + test_when_finished "rm -f output please-abort" && + test_when_finished "git checkout side" && + + git reset --hard anchor && +
t/t6406-merge-attr.sh test_expect_success 'binary files with union attribute' ' · line 279
Quoted 10 lines
t/t6406-merge-attr.sh +10
-0+ git checkout -b base-a main^ && + echo base-a >text && + git commit -m base-a text && + + git checkout -b base-b main^ && + echo base-b >text && + git commit -m base-b text && + + git checkout -b recursive-a base-a && + test_must_fail git merge base-b &&
Here and below, do you care about the particular exit code of merging as above? IOW, should these be test_expect_code's as well?
Thanks,
Taylor
Re: [PATCH 2/7] merge-ort: maintain expected invariant for priv member
Quoted 14 lines · Taylor Blau
Quoted 11 lines · Elijah Newren via GitGitGadget
From: Elijah Newren <[email protected]>
The calling convention for the merge machinery is
One call toinit_merge_options()
One or more calls to merge_incore_[non]recursive()
One call tomerge_finalize()
(possibly indirectly viamerge_switch_to_result())
Bothmerge_switch_to_result()andmerge_finalize()expect
opt->priv== NULL &&result->priv!= NULL
which is supposed to be set up by ourmove_opt_priv_to_result_priv()function. However, two codepath dealing with error cases did not
Quoted 1 line
s/codepath/&s/ ?
Indeed.
Quoted 8 lines
Quoted 8 lines
execute this necessary logic, which could result in assertion failures (or, if assertions were compiled out, could result in segfaults). Fix the oversight and add a test that would have caught one of these problems.
While at it, also tighten an existing test for a non-recursive merge to verify that it fails correctly, i.e. with the expected exit code rather than with an assertion failure.
It turns out my logic here was faulty; if a
test_must_fail git ARGS...
command is run and git hits an assertion failure, the test_must_fail will fail with
test_must_fail: died by signal 6: git ARGS...
similar to how it would fail if git were to segfault.
However, I still like the idea of testing the exit status here because of this comment in builtin/merge.c:
/*
* The backend exits with 1 when conflicts are
* left to be resolved, with 2 when it does not
* handle the given merge at all.
*/
and this is one of the few tests in the testsuite where we are explicitly testing a case where the merge is neither success nor conflicts, but failed-to-handle.
Quoted 5 lines
I suspect that this test was flaky, too, since if the assertion errors were compiled out and it died via a segfault, the test would have failed outright as
test_must_faildoes not allow for segfaults typically.So I'm glad to see us tightening up that area of the test suite.
[...]
Quoted 4 lines
Quoted 1 line
+
test_must_failgit merge base-b &&Here and below, do you care about the particular exit code of merging as above? IOW, should these be
test_expect_code's as well?
Both of these comments led me on the path to check something out and discover my error above, which will lead to slightly different changes to the patch.
PATCH 3/7 merge-ort: fix type of local 'clean' var in handle_content_merge()
In reply to Elijah Newren via GitGitGadgetFrom: Elijah Newren <[email protected]>
handle_content_merge() returns an int. Every caller of handle_content_merge() expects an int. However, we declare a local variable 'clean' that we use for the return value to be unsigned. To make matters worse, we also assign 'clean' the return value of merge_submodule() in one codepath, which is defined to return an int. It seems that the only reason to have 'clean' be unsigned was to allow a cutesy bit manipulation operation to be well-defined. Fix the type of the 'clean' local in handle_content_merge().
--- merge-ort.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
merge-ort.c +3 -2
@@ -2109,7 +2109,7 @@ static int handle_content_merge(struct merge_options *opt,* merges, which happens for example with rename/rename(2to1) and * rename/add conflicts. */ -unsigned clean =1 ; +int clean =1 ;/* * handle_content_merge() needs both files to be of the same type, i.e.
@@ -2184,7 +2184,8 @@ static int handle_content_merge(struct merge_options *opt,free (result_buf.ptr);if (ret)return -1 ; - clean &= (merge_status ==0 ); +if (merge_status >0 ) + clean =0 ;path_msg (opt,INFO_AUTO_MERGING ,1 , path,NULL ,NULL ,NULL ,_ ("Auto-merging %s" ), path); }else if (S_ISGITLINK (a->mode)) {
Signature · 1 line
gitgitgadget
PATCH 4/7 merge-ort: clearer propagation of failure-to-function from merge_submodule
In reply to Elijah Newren via GitGitGadgetFrom: Elijah Newren <[email protected]>
The 'clean' member variable is somewhat of a tri-state (1 = clean, 0 = conflicted, -1 = failure-to-determine), but we often like to think of it as binary (ignoring the possibility of a negative value) and use constructs like '!clean' to reflect this. However, these constructs can make codepaths more difficult to understand, unless we handle the negative case early and return pre-emptively; do that in handle_content_merge() to make the code a bit easier to read.
--- merge-ort.c | 2 ++ 1 file changed, 2 insertions(+)
merge-ort.c +2 -0
@@ -2193,6 +2193,8 @@ static int handle_content_merge(struct merge_options *opt, clean =merge_submodule (opt, pathnames[0 ], two_way ?null_oid () : &o->oid, &a->oid, &b->oid, &result->oid); +if (clean <0 ) +return -1 ;if (opt->priv->call_depth && two_way && !clean) { result->mode = o->mode;oidcpy (&result->oid, &o->oid);
Signature · 1 line
gitgitgadget
PATCH 5/7 merge-ort: loosen commented requirements
In reply to Elijah Newren via GitGitGadgetFrom: Elijah Newren <[email protected]>
The comment above type_short_descriptions claimed that the order had to match what was found in the conflict_info_and_types enum. Since type_short_descriptions uses designated initializers, the order should not actually matter; I am guessing that positional initializers may have been under consideration when that comment was added, but the comment was not updated when designated initializers were chosen.
--- merge-ort.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
merge-ort.c +1 -1
@@ -553,7 +553,7 @@ enum conflict_and_info_types {* Short description of conflict type, relied upon by external tools. * * We can add more entries, but DO NOT change any of these strings. Also, -* Order MUST match conflict_info_and_types. +* please ensure the order matches what is used in conflict_info_and_types. */ static const char *type_short_descriptions[] = {/*** "Simple" conflicts and informational messages ***/
Signature · 1 line
gitgitgadget
PATCH 6/7 merge-ort: upon merge abort, only show messages causing the abort
In reply to Elijah Newren via GitGitGadgetFrom: Elijah Newren <[email protected]>
When something goes wrong enough that we need to abort early and not even attempt merging the remaining files, it probably does not make sense to report conflicts messages for the subset of files we processed before hitting the fatal error. Instead, only show the messages associated with paths where we hit the fatal error. Also, print these messages to stderr rather than stdout.
--- merge-ort.c | 76 ++++++++++++++++++++++++++++++++++++----------------- 1 file changed, 52 insertions(+), 24 deletions(-)
merge-ort.c +52 -24
@@ -543,10 +543,24 @@ enum conflict_and_info_types {CONFLICT_SUBMODULE_HISTORY_NOT_AVAILABLE ,CONFLICT_SUBMODULE_MAY_HAVE_REWINDS ,CONFLICT_SUBMODULE_NULL_MERGE_BASE , -CONFLICT_SUBMODULE_CORRUPT , + +/* INSERT NEW ENTRIES HERE */ + +/* +* Keep this entry after all regular conflict and info types; only +* errors (failures causing immediate abort of the merge) should +* come after this. +*/ +NB_REGULAR_CONFLICT_TYPES , + +/* +* Something is seriously wrong; cannot even perform merge; +* Keep this group _last_ other than NB_CONFLICT_TYPES +*/ +ERROR_SUBMODULE_CORRUPT ,/* Keep this entry _last_ in the list */ - NB_CONFLICT_TYPES, + NB_TOTAL_TYPES, };/*
@@ -597,8 +611,10 @@ static const char *type_short_descriptions[] = { "CONFLICT (submodule may have rewinds)", [CONFLICT_SUBMODULE_NULL_MERGE_BASE ] = "CONFLICT (submodule lacks merge base)", - [CONFLICT_SUBMODULE_CORRUPT ] = -"CONFLICT (submodule corrupt)" + +/* Something is seriously wrong; cannot even perform merge */ + [ERROR_SUBMODULE_CORRUPT ] = + "ERROR (submodule corrupt)", };struct logical_conflict_info {
@@ -762,7 +778,8 @@ static void path_msg(struct merge_options *opt,/* Sanity checks */ assert (omittable_hint == - !starts_with (type_short_descriptions[type],"CONFLICT" ) || + (!starts_with (type_short_descriptions[type],"CONFLICT" ) && + !starts_with (type_short_descriptions[type],"ERROR" )) || type ==CONFLICT_DIR_RENAME_SUGGESTED );if (opt->record_conflict_msgs_as_headers && omittable_hint)return ;/* Do not record mere hints in headers */
@@ -1817,9 +1834,9 @@ static int merge_submodule(struct merge_options *opt,/* check whether both changes are forward */ ret2 =repo_in_merge_bases (&subrepo, commit_o, commit_a);if (ret2 <0 ) { -path_msg (opt,CONFLICT_SUBMODULE_CORRUPT ,0 , +path_msg (opt,ERROR_SUBMODULE_CORRUPT ,0 , path,NULL ,NULL ,NULL , -_ ("Failed to merge submodule %s " +_ ("error: failed to merge submodule %s " "(repository corrupt)" ), path); ret =-1 ;
@@ -1828,9 +1845,9 @@ static int merge_submodule(struct merge_options *opt,if (ret2 >0 ) ret2 =repo_in_merge_bases (&subrepo, commit_o, commit_b);if (ret2 <0 ) { -path_msg (opt,CONFLICT_SUBMODULE_CORRUPT ,0 , +path_msg (opt,ERROR_SUBMODULE_CORRUPT ,0 , path,NULL ,NULL ,NULL , -_ ("Failed to merge submodule %s " +_ ("error: failed to merge submodule %s " "(repository corrupt)" ), path); ret =-1 ;
@@ -1848,7 +1865,7 @@ static int merge_submodule(struct merge_options *opt,/* Case #1: a is contained in b or vice versa */ ret2 =repo_in_merge_bases (&subrepo, commit_a, commit_b);if (ret2 <0 ) { - path_msg(opt, CONFLICT_SUBMODULE_CORRUPT, 0, + path_msg(opt, ERROR_SUBMODULE_CORRUPT, 0, path, NULL, NULL, NULL, _("Failed to merge submodule %s " "(repository corrupt)"),
@@ -1867,9 +1884,9 @@ static int merge_submodule(struct merge_options *opt, } ret2 =repo_in_merge_bases (&subrepo, commit_b, commit_a);if (ret2 <0 ) { -path_msg (opt,CONFLICT_SUBMODULE_CORRUPT ,0 , +path_msg (opt,ERROR_SUBMODULE_CORRUPT ,0 , path,NULL ,NULL ,NULL , -_ ("Failed to merge submodule %s " +_ ("error: failed to merge submodule %s " "(repository corrupt)" ), path); ret =-1 ;
@@ -1901,9 +1918,9 @@ static int merge_submodule(struct merge_options *opt, &merges);switch (parent_count) {case -1 : -path_msg (opt,CONFLICT_SUBMODULE_CORRUPT ,0 , +path_msg (opt,ERROR_SUBMODULE_CORRUPT ,0 , path,NULL ,NULL ,NULL , -_ ("Failed to merge submodule %s " +_ ("error: failed to merge submodule %s " "(repository corrupt)" ), path); ret =-1 ;
@@ -4646,6 +4663,7 @@ void merge_display_update_messages(struct merge_options *opt,struct hashmap_iter iter;struct strmap_entry *e;struct string_list olist =STRING_LIST_INIT_NODUP ; +FILE *o = stdout;if (opt->record_conflict_msgs_as_headers)BUG ("Either display conflict messages or record them as headers, not both" );
@@ -4662,6 +4680,10 @@ void merge_display_update_messages(struct merge_options *opt, }string_list_sort (&olist); +/* Print to stderr if we hit errors rather than just conflicts */ +if (result->clean <0 ) + o = stderr; +/* Iterate over the items, printing them */ for (int path_nr =0 ; path_nr < olist.nr; ++path_nr) {struct string_list *conflicts = olist.items[path_nr].util;
@@ -4669,25 +4691,31 @@ void merge_display_update_messages(struct merge_options *opt,struct logical_conflict_info *info = conflicts->items[i].util; +/* On failure, ignore regular conflict types */ +if (result->clean <0 && + info->type <NB_REGULAR_CONFLICT_TYPES ) +continue ; +if (detailed) { -printf ("%lu" , (unsigned long )info->paths.nr); -putchar ('\0' ); +fprintf (o,"%lu" , (unsigned long )info->paths.nr); +fputc ('\0' , o);for (int n =0 ; n < info->paths.nr; n++) { -fputs (info->paths.v[n], stdout); -putchar ('\0' ); +fputs (info->paths.v[n], o); +fputc ('\0' , o); } -fputs (type_short_descriptions[info->type], - stdout); -putchar ('\0' ); +fputs (type_short_descriptions[info->type], o); +fputc ('\0' , o); } -puts (conflicts->items[i].string); +fputs (conflicts->items[i].string, o); +fputc ('\n' , o);if (detailed) -putchar ('\0' ); +fputc ('\0' , o); } }string_list_clear (&olist,0 ); -print_submodule_conflict_suggestion (&opti->conflicted_submodules); +if (result->clean >=0 ) +print_submodule_conflict_suggestion (&opti->conflicted_submodules);/* Also include needed rename limit adjustment now */ diff_warn_rename_limit("merge.renamelimit",
Signature · 1 line
gitgitgadget
Re: [PATCH 6/7] merge-ort: upon merge abort, only show messages causing the abort
Quoted 11 lines · Elijah Newren via GitGitGadget
When something goes wrong enough that we need to abort early and not even attempt merging the remaining files, it probably does not make sense to report conflicts messages for the subset of files we processed before hitting the fatal error. Instead, only show the messages associated with paths where we hit the fatal error. Also, print these messages to stderr rather than stdout.
Signed-off-by Elijah Newren <[email protected]>---
merge-ort.c enum conflict_and_info_types { · line 548
Quoted 7 lines
merge-ort.c +6
-1@@ -543,10 +543,24 @@ enum conflict_and_info_types { - CONFLICT_SUBMODULE_CORRUPT, + +/* INSERT NEW ENTRIES HERE */ +/* +* Something is seriously wrong; cannot even perform merge; +* Keep this group _last_ other than NB_CONFLICT_TYPES +*/
I'm probably missing something obvious, but here the new comment talks about NB_CONFLICT_TYPES...
merge-ort.c enum conflict_and_info_types { · line 563
Quoted 5 lines
merge-ort.c +2
-1+ERROR_SUBMODULE_CORRUPT ,/* Keep this entry _last_ in the list */ - NB_CONFLICT_TYPES, + NB_TOTAL_TYPES,
... but NB_CONFLICT_TYPES gets removed here.
merge-ort.c static int merge_submodule(struct merge_options *opt, · line 1866
Quoted 6 lines
merge-ort.c +1
-1@@ -1828,9 +1845,9 @@ static int merge_submodule(struct merge_options *opt, - _("Failed to merge submodule %s " + _("error: failed to merge submodule %s "@@ -1848,7 +1865,7 @@ static int merge_submodule(struct merge_options *opt,_ ("Failed to merge submodule %s " "(repository corrupt)"),
Do you also want to apply the same "error: failed..." transformation to this error message as you did to other error messages?
Re: [PATCH 6/7] merge-ort: upon merge abort, only show messages causing the abort
Quoted 33 lines · Eric Sunshine
Quoted 11 lines · Elijah Newren via GitGitGadget
When something goes wrong enough that we need to abort early and not even attempt merging the remaining files, it probably does not make sense to report conflicts messages for the subset of files we processed before hitting the fatal error. Instead, only show the messages associated with paths where we hit the fatal error. Also, print these messages to stderr rather than stdout.
Signed-off-by Elijah Newren <[email protected]>---
merge-ort.c enum conflict_and_info_types { · line 548
Quoted 7 lines
merge-ort.c +6
-1@@ -543,10 +543,24 @@ enum conflict_and_info_types { - CONFLICT_SUBMODULE_CORRUPT, + +/* INSERT NEW ENTRIES HERE */ +/* +* Something is seriously wrong; cannot even perform merge; +* Keep this group _last_ other than NB_CONFLICT_TYPES +*/ I'm probably missing something obvious, but here the new comment talks about
NB_CONFLICT_TYPES...merge-ort.c enum conflict_and_info_types { · line 563
Quoted 5 lines
merge-ort.c +2
-1+ERROR_SUBMODULE_CORRUPT ,/* Keep this entry _last_ in the list */ - NB_CONFLICT_TYPES, + NB_TOTAL_TYPES,... but
NB_CONFLICT_TYPESgets removed here.
Quoted 9 lines
merge-ort.c static int merge_submodule(struct merge_options *opt, · line 1866
Quoted 6 lines
merge-ort.c +1
-1@@ -1828,9 +1845,9 @@ static int merge_submodule(struct merge_options *opt, - _("Failed to merge submodule %s " + _("error: failed to merge submodule %s "@@ -1848,7 +1865,7 @@ static int merge_submodule(struct merge_options *opt,_ ("Failed to merge submodule %s " "(repository corrupt)"),Do you also want to apply the same "error: failed..." transformation to this error message as you did to other error messages?
Doh, I tried to re-read through my patches after making numerous additional tweaks to try to catch stuff like this, but I clearly missed a few cases here. Thanks for catching both of these issues; I'll fix them up.
PATCH 7/7 merge-ort: convert more error() cases to path_msg()
In reply to Elijah Newren via GitGitGadgetFrom: Elijah Newren <[email protected]>
merge_submodule() stores errors using path_msg(), whereas other call sites make use of the error() function. This is inconsistent, and moving towards path_msg() seems more friendly for libification efforts since it will allow the caller to determine whether the error messages need to be printed.
Note that this deferred handling of error messages changes the error message in a recursive merge from
error: failed to execute internal merge
to
From inner merge: error: failed to execute internal merge
which provides a little more information about the error which may be useful. Since the recursive merge strategy still only shows the older error, we had to adjust the new testcase introduced a few commits ago to just search for the older message somewhere in the output.
--- merge-ort.c | 53 +++++++++++++++++++++++++++++++++---------- t/t6406-merge-attr.sh | 2 +- 2 files changed, 42 insertions(+), 13 deletions(-)
merge-ort.c +41 -12
@@ -558,6 +558,10 @@ enum conflict_and_info_types {* Keep this group _last_ other than NB_CONFLICT_TYPES */ ERROR_SUBMODULE_CORRUPT , +ERROR_THREEWAY_CONTENT_MERGE_FAILED , +ERROR_OBJECT_WRITE_FAILED , +ERROR_OBJECT_READ_FAILED , +ERROR_OBJECT_NOT_A_BLOB ,/* Keep this entry _last_ in the list */ NB_TOTAL_TYPES,
@@ -615,6 +619,14 @@ static const char *type_short_descriptions[] = {/* Something is seriously wrong; cannot even perform merge */ [ERROR_SUBMODULE_CORRUPT ] = "ERROR (submodule corrupt)", + [ERROR_THREEWAY_CONTENT_MERGE_FAILED ] = + "ERROR (three-way content merge failed)", + [ERROR_OBJECT_WRITE_FAILED ] = + "ERROR (object write failed)", + [ERROR_OBJECT_READ_FAILED ] = + "ERROR (object read failed)", + [ERROR_OBJECT_NOT_A_BLOB ] = + "ERROR (object is not a blob)", };struct logical_conflict_info {
@@ -2190,15 +2202,24 @@ static int handle_content_merge(struct merge_options *opt, pathnames, extra_marker_size, &result_buf); -if ((merge_status <0 ) || !result_buf.ptr) - ret =error (_ ("failed to execute internal merge" )); +if ((merge_status <0 ) || !result_buf.ptr) { +path_msg (opt,ERROR_THREEWAY_CONTENT_MERGE_FAILED ,0 , + pathnames[0 ], pathnames[1 ], pathnames[2 ],NULL , +_ ("error: failed to execute internal merge for %s" ), + path); + ret =-1 ; + }if (!ret &&write_object_file (result_buf.ptr, result_buf.size, -OBJ_BLOB , &result->oid)) - ret =error (_ ("unable to add %s to database" ), path); - +OBJ_BLOB , &result->oid)) { +path_msg (opt,ERROR_OBJECT_WRITE_FAILED ,0 , + pathnames[0 ], pathnames[1 ], pathnames[2 ],NULL , +_ ("error: unable to add %s to database" ), path); + ret =-1 ; + }free (result_buf.ptr); +if (ret)return -1 ;if (merge_status >0 )
@@ -3577,18 +3598,26 @@ static int sort_dirs_next_to_their_children(const char *one, const char *two)return c1 - c2; } -static int read_oid_strbuf (const struct object_id *oid, -struct strbuf *dst) +static int read_oid_strbuf (struct merge_options *opt, +const struct object_id *oid, +struct strbuf *dst, +const char *path) {void *buf;enum object_type type;unsigned long size; buf =repo_read_object_file (the_repository, oid, &type, &size); -if (!buf) -return error (_ ("cannot read object %s" ),oid_to_hex (oid)); +if (!buf) { +path_msg (opt,ERROR_OBJECT_READ_FAILED ,0 , + path,NULL ,NULL ,NULL , +_ ("error: cannot read object %s" ),oid_to_hex (oid)); +return -1 ; + }if (type !=OBJ_BLOB ) {free (buf); -return error (_ ("object %s is not a blob" ),oid_to_hex (oid)); +path_msg (opt,ERROR_OBJECT_NOT_A_BLOB ,0 , + path,NULL ,NULL ,NULL , +_ ("error: object %s is not a blob" ),oid_to_hex (oid)); }strbuf_attach (dst, buf, size, size +1 );return 0 ;
@@ -3612,8 +3641,8 @@ static int blob_unchanged(struct merge_options *opt,if (oideq (&base->oid, &side->oid))return 1 ; -if (read_oid_strbuf (&base->oid, &basebuf) || -read_oid_strbuf (&side->oid, &sidebuf)) +if (read_oid_strbuf (opt, &base->oid, &basebuf, path) || +read_oid_strbuf (opt, &side->oid, &sidebuf, path)) goto error_return;/* * Note: binary | is used so that both renormalizations are
t/t6406-merge-attr.sh +1 -1
@@ -295,7 +295,7 @@ test_expect_success !WINDOWS 'custom merge driver that is killed with a signal o >./please-abort && echo "* merge=custom" >.gitattributes && test_expect_code 2 git merge recursive-a 2>err && - grep "^error: failed to execute internal merge" err && + grep "error: failed to execute internal merge" err && git ls-files -u >output && git diff --name-only HEAD >>output && test_must_be_empty output
Signature · 1 line
gitgitgadget
Quoted 10 lines · Elijah Newren via GitGitGadget
Elijah Newren (7):
merge-ort: extract handling of priv member into reusable function
merge-ort: maintain expected invariant for priv member
merge-ort: fix type of local 'clean' var inhandle_content_merge()
merge-ort: clearer propagation of failure-to-function from
merge_submodule
merge-ort: loosen commented requirements
merge-ort: upon merge abort, only show messages causing the abort
merge-ort: convert moreerror()cases topath_msg()
Quoted 3 lines
merge-ort.c | 167 +++++++++++++++++++++++++++++++----------- t/t6406-merge-attr.sh | 42 ++++++++++- 2 files changed, 164 insertions(+), 45 deletions(-)
Very nice. I had a couple of minor thoughts on the earlier patches, but I agree with the substantive ones that printing only messages related to paths that we were processing when something went horribly wrong is a good idea.
I don't think that either of my comments alone merit a reroll, and I'd be happy to see this version move along as-is. But I'm equally happy if you want to fix a typo here or there or do some bikeshedding ;-).
Thanks for fixing this issue.
Thanks,
Taylor
v2 cover Fix and improve some error codepaths in merge-ort
In reply to Elijah Newren via GitGitGadgetChanges since v1:
* Remove the conditional outside of move_opt_priv_to_result_priv() for patches 1 & 2
* Fix the consistency issues in patch 6 (refer to updated enum value in comment, use consistent prefix on error messages)
* Fix typo in patch 2
* Fix incorrect claim in commit message of patch 2 and provide more detail about which merges we are interested in checking for a very specific exit code.
This series started as a just a fix for the abort hit in merge-ort when custom merge drivers error out (see
https://lore.kernel.org/git/[email protected]/).
However, while working on that, I found a few other issues around error codepaths in merge-ort. So this series:
* Patches 1-2: fix the reported abort problem
* Patches 3-4: make code in handle_content_merges() easier to handle when we hit errors
* Patch 5: fix a misleading comment
* Patches 6-7: make error handling (immediate print vs. letting callers get the error information) more consistent
The last two patches change the behavior slightly for error codepaths, and there's a question about whether we should show only the error messages that caused an early termination of the merge, or if we should also show any conflict messages for other paths that were handled before we hit the early termination. These patches made a decision but feel free to take those last two patches as more of an RFC.
Elijah Newren (7):
merge-ort: extract handling of priv member into reusable function
merge-ort: maintain expected invariant for priv member
merge-ort: fix type of local 'clean' var in handle_content_merge()
merge-ort: clearer propagation of failure-to-function from
merge_submodule
merge-ort: loosen commented requirements
merge-ort: upon merge abort, only show messages causing the abort
merge-ort: convert more error() cases to path_msg()
merge-ort.c | 168 +++++++++++++++++++++++++++++++----------- t/t6406-merge-attr.sh | 42 ++++++++++- 2 files changed, 164 insertions(+), 46 deletions(-)
base-commit: 8d94cfb54504f2ec9edc7ca3eb5c29a3dd3675ae
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1748%2Fnewren%2Ffix-error-cases-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1748/newren/fix-error-cases-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1748
Range-diff vs v1:
1: d4ba1fccd91 ! 1: 5c50c0b3aad merge-ort: extract handling of priv member into reusable function
@@ merge-ort.c: static void merge_check_renames_reusable(struct merge_result *resul
+ * to move it.
+ */
+ assert(opt->priv && !result->priv);
-+ if (!opt->priv->call_depth) {
-+ result->priv = opt->priv;
-+ result->_properly_initialized = RESULT_INITIALIZED;
-+ opt->priv = NULL;
-+ }
++ result->priv = opt->priv;
++ result->_properly_initialized = RESULT_INITIALIZED;
++ opt->priv = NULL;
+}
+
/*
@@ merge-ort.c: static void merge_ort_nonrecursive_internal(struct merge_options *o
- result->_properly_initialized = RESULT_INITIALIZED;
- opt->priv = NULL;
- }
-+ move_opt_priv_to_result_priv(opt, result);
++ if (!opt->priv->call_depth)
++ move_opt_priv_to_result_priv(opt, result);
}
/* 2: 17c97301baa ! 2: d1adec6d105 merge-ort: maintain expected invariant for priv member
@@ Commit message
Both merge_switch_to_result() and merge_finalize() expect
opt->priv == NULL && result->priv != NULL
which is supposed to be set up by our move_opt_priv_to_result_priv()
- function. However, two codepath dealing with error cases did not
+ function. However, two codepaths dealing with error cases did not
execute this necessary logic, which could result in assertion failures (or, if assertions were compiled out, could result in segfaults). Fix the oversight and add a test that would have caught one of these problems.
While at it, also tighten an existing test for a non-recursive merge
- to verify that it fails correctly, i.e. with the expected exit code
- rather than with an assertion failure.
+ to verify that it fails with appropriate status. Most merge tests in
+ the testsuite check either for success or conflicts; those testing for
+ neither are rare and it is good to ensure they support the invariant
+ assumed by builtin/merge.c in this comment:
+ /*
+ * The backend exits with 1 when conflicts are
+ * left to be resolved, with 2 when it does not
+ * handle the given merge at all.
+ */
+ So, explicitly check for the exit status of 2 in these cases.
## merge-ort.c ##
-@@ merge-ort.c: static void move_opt_priv_to_result_priv(struct merge_options *opt,
- * to move it.
- */
- assert(opt->priv && !result->priv);
-- if (!opt->priv->call_depth) {
-+ if (!opt->priv->call_depth || result->clean < 0) {
- result->priv = opt->priv;
- result->_properly_initialized = RESULT_INITIALIZED;
- opt->priv = NULL;
@@ merge-ort.c: static void merge_ort_nonrecursive_internal(struct merge_options *opt,
oid_to_hex(&side1->object.oid), oid_to_hex(&side2->object.oid));
@@ merge-ort.c: static void merge_ort_nonrecursive_internal(struct merge_options *o
return;
}
trace2_region_leave("merge", "collect_merge_info", opt->repo);
+@@ merge-ort.c: static void merge_ort_nonrecursive_internal(struct merge_options *opt,
+ /* existence of conflicted entries implies unclean */
+ result->clean &= strmap_empty(&opt->priv->conflicted);
+ }
+- if (!opt->priv->call_depth)
++ if (!opt->priv->call_depth || result->clean < 0)
+ move_opt_priv_to_result_priv(opt, result);
+ }
+
## t/t6406-merge-attr.sh ##
@@ t/t6406-merge-attr.sh: test_expect_success !WINDOWS 'custom merge driver that is killed with a signal'
3: 23bb3386114 = 3: 034b91db1d2 merge-ort: fix type of local 'clean' var in handle_content_merge() 4: 2789c58cd3f = 4: 2813a15b48b merge-ort: clearer propagation of failure-to-function from merge_submodule 5: 975fbddf305 = 5: 750acae4dba merge-ort: loosen commented requirements 6: 71391b18c1a ! 6: 6756956d0c7 merge-ort: upon merge abort, only show messages causing the abort
@@ merge-ort.c: enum conflict_and_info_types {
+
+ /*
+ * Something is seriously wrong; cannot even perform merge;
-+ * Keep this group _last_ other than NB_CONFLICT_TYPES
++ * Keep this group _last_ other than NB_TOTAL_TYPES
+ */
+ ERROR_SUBMODULE_CORRUPT,
@@ merge-ort.c: static int merge_submodule(struct merge_options *opt,
- path_msg(opt, CONFLICT_SUBMODULE_CORRUPT, 0,
+ path_msg(opt, ERROR_SUBMODULE_CORRUPT, 0,
path, NULL, NULL, NULL,
- _("Failed to merge submodule %s "
+- _("Failed to merge submodule %s "
++ _("error: failed to merge submodule %s "
"(repository corrupt)"),
+ path);
+ ret = -1;
@@ merge-ort.c: static int merge_submodule(struct merge_options *opt,
}
ret2 = repo_in_merge_bases(&subrepo, commit_b, commit_a);
7: 32ae44b6260 ! 7: 500433edf49 merge-ort: convert more error() cases to path_msg()
@@ Commit message
## merge-ort.c ##
@@ merge-ort.c: enum conflict_and_info_types {
- * Keep this group _last_ other than NB_CONFLICT_TYPES
+ * Keep this group _last_ other than NB_TOTAL_TYPES
*/
ERROR_SUBMODULE_CORRUPT,
+ ERROR_THREEWAY_CONTENT_MERGE_FAILED,
Signature · 1 line
gitgitgadget
PATCH v2 1/7 merge-ort: extract handling of priv member into reusable function
From: Elijah Newren <[email protected]>
In preparation for a subsequent commit which will ensure we do not forget to maintain our invariants for the priv member in error codepaths, extract the necessary functionality out into a separate function. This change is cosmetic at this point, and introduces no changes beyond an extra assertion sanity check.
--- merge-ort.c | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-)
merge-ort.c +22 -5
@@ -5000,6 +5000,26 @@ static void merge_check_renames_reusable(struct merge_result *result,/*** Function Grouping: merge_incore_*() and their internal variants ***/ +static void move_opt_priv_to_result_priv (struct merge_options *opt, +struct merge_result *result) +{ +/* +* opt->priv and result->priv are a bit weird. opt->priv contains +* information that we can re-use in subsequent merge operations to +* enable our cached renames optimization. The best way to provide +* that to subsequent merges is putting it in result->priv. +* However, putting it directly there would mean retrofitting lots +* of functions in this file to also take a merge_result pointer, +* which is ugly and annoying. So, we just make sure at the end of +* the merge (the outer merge if there are internal recursive ones) +* to move it. +*/ +assert (opt->priv && !result->priv); + result->priv = opt->priv; + result->_properly_initialized =RESULT_INITIALIZED ; + opt->priv =NULL ; +} +/* * Originally from merge_trees_internal(); heavily adapted, though. */
@@ -5060,11 +5080,8 @@ static void merge_ort_nonrecursive_internal(struct merge_options *opt,/* existence of conflicted entries implies unclean */ result->clean &=strmap_empty (&opt->priv->conflicted); } -if (!opt->priv->call_depth) { - result->priv = opt->priv; - result->_properly_initialized =RESULT_INITIALIZED ; - opt->priv =NULL ; - } +if (!opt->priv->call_depth) +move_opt_priv_to_result_priv (opt, result); }/*
Signature · 1 line
gitgitgadget
PATCH v2 2/7 merge-ort: maintain expected invariant for priv member
In reply to Elijah Newren via GitGitGadgetFrom: Elijah Newren <[email protected]>
The calling convention for the merge machinery is
One call to init_merge_options()
One or more calls to merge_incore_[non]recursive()
One call to merge_finalize()
(possibly indirectly via merge_switch_to_result())
Both merge_switch_to_result() and merge_finalize() expect
opt->priv == NULL && result->priv != NULL
which is supposed to be set up by our move_opt_priv_to_result_priv() function. However, two codepaths dealing with error cases did not execute this necessary logic, which could result in assertion failures (or, if assertions were compiled out, could result in segfaults). Fix the oversight and add a test that would have caught one of these problems.
While at it, also tighten an existing test for a non-recursive merge to verify that it fails with appropriate status. Most merge tests in the testsuite check either for success or conflicts; those testing for neither are rare and it is good to ensure they support the invariant assumed by builtin/merge.c in this comment:
/*
* The backend exits with 1 when conflicts are
* left to be resolved, with 2 when it does not
* handle the given merge at all.
*/
So, explicitly check for the exit status of 2 in these cases.
--- merge-ort.c | 3 ++- t/t6406-merge-attr.sh | 42 +++++++++++++++++++++++++++++++++++++++++- 2 files changed, 43 insertions(+), 2 deletions(-)
merge-ort.c +2 -1
@@ -5050,6 +5050,7 @@ static void merge_ort_nonrecursive_internal(struct merge_options *opt,oid_to_hex (&side1->object.oid), oid_to_hex(&side2->object.oid)); result->clean =-1 ; +move_opt_priv_to_result_priv (opt, result);return ; }trace2_region_leave ("merge" ,"collect_merge_info" , opt->repo);
@@ -5080,7 +5081,7 @@ static void merge_ort_nonrecursive_internal(struct merge_options *opt,/* existence of conflicted entries implies unclean */ result->clean &=strmap_empty (&opt->priv->conflicted); } -if (!opt->priv->call_depth) +if (!opt->priv->call_depth || result->clean <0 )move_opt_priv_to_result_priv (opt, result); }
t/t6406-merge-attr.sh +41 -1
52 lines not on this page · raw message
Signature · 1 line
gitgitgadget
Re: [PATCH v2 2/7] merge-ort: maintain expected invariant for priv member
Quoted 8 lines · Elijah Newren via GitGitGadget
From: Elijah Newren <[email protected]>
The calling convention for the merge machinery is
One call toinit_merge_options()
One or more calls to merge_incore_[non]recursive()
One call tomerge_finalize()
(possibly indirectly viamerge_switch_to_result())
Bothmerge_switch_to_result()andmerge_finalize()expect
Quoted 7 lines
opt->priv == NULL && result->priv != NULLwhich is supposed to be set up by our
move_opt_priv_to_result_priv()function. However, two codepaths dealing with error cases did not execute this necessary logic, which could result in assertion failures (or, if assertions were compiled out, could result in segfaults). Fix the oversight and add a test that would have caught one of these problems.
"one of these problems" is key here.
merge-ort.c static void merge_ort_nonrecursive_internal(struct merge_options *opt, · line 5055
Quoted 7 lines
merge-ort.c +1
-0@@ -5050,6 +5050,7 @@ static void merge_ort_nonrecursive_internal(struct merge_options *opt,oid_to_hex (&side1->object.oid), oid_to_hex(&side2->object.oid)); result->clean =-1 ; +move_opt_priv_to_result_priv (opt, result);return ; } > trace2_region_leave("merge", "collect_merge_info", opt->repo);
Removing this line does not cause your test script to fail. This is understandable as this case only happens during a parse failure, so it would be unreasonable to generate a test case that only fails for such a reason.
merge-ort.c static void merge_ort_nonrecursive_internal(struct merge_options *opt, · line 5086
Quoted 8 lines
merge-ort.c +1
-1@@ -5080,7 +5081,7 @@ static void merge_ort_nonrecursive_internal(struct merge_options *opt,/* existence of conflicted entries implies unclean */ result->clean &=strmap_empty (&opt->priv->conflicted); } -if (!opt->priv->call_depth) +if (!opt->priv->call_depth || result->clean <0 )move_opt_priv_to_result_priv (opt, result); }
Removing this change does get caught by the new test.
Thanks,
-Stolee
PATCH v2 3/7 merge-ort: fix type of local 'clean' var in handle_content_merge()
In reply to Elijah Newren via GitGitGadgetFrom: Elijah Newren <[email protected]>
handle_content_merge() returns an int. Every caller of handle_content_merge() expects an int. However, we declare a local variable 'clean' that we use for the return value to be unsigned. To make matters worse, we also assign 'clean' the return value of merge_submodule() in one codepath, which is defined to return an int. It seems that the only reason to have 'clean' be unsigned was to allow a cutesy bit manipulation operation to be well-defined. Fix the type of the 'clean' local in handle_content_merge().
--- merge-ort.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
merge-ort.c +3 -2
@@ -2109,7 +2109,7 @@ static int handle_content_merge(struct merge_options *opt,* merges, which happens for example with rename/rename(2to1) and * rename/add conflicts. */ -unsigned clean =1 ; +int clean =1 ;/* * handle_content_merge() needs both files to be of the same type, i.e.
@@ -2184,7 +2184,8 @@ static int handle_content_merge(struct merge_options *opt,free (result_buf.ptr);if (ret)return -1 ; - clean &= (merge_status ==0 ); +if (merge_status >0 ) + clean =0 ;path_msg (opt,INFO_AUTO_MERGING ,1 , path,NULL ,NULL ,NULL ,_ ("Auto-merging %s" ), path); }else if (S_ISGITLINK (a->mode)) {
Signature · 1 line
gitgitgadget
Re: [PATCH v2 3/7] merge-ort: fix type of local 'clean' var in handle_content_merge()
Quoted 12 lines · Elijah Newren via GitGitGadget
From: Elijah Newren <[email protected]>
handle_content_merge()returns an int. Every caller ofhandle_content_merge()expects an int. However, we declare a local variable 'clean' that we use for the return value to be unsigned. To make matters worse, we also assign 'clean' the return value ofmerge_submodule()in one codepath, which is defined to return an int. It seems that the only reason to have 'clean' be unsigned was to allow a cutesy bit manipulation operation to be well-defined. Fix the type of the 'clean' local inhandle_content_merge().
Review static int handle_content_merge(struct merge_options *opt, · line 2191
Quoted 9 lines
(quoted diff) +2
-1@@ -2184,7 +2184,8 @@ static int handle_content_merge(struct merge_options *opt, free(result_buf.ptr); if (ret) return -1; - clean &= (merge_status == 0); + if (merge_status > 0) + clean = 0; path_msg(opt, INFO_AUTO_MERGING, 1, path, NULL, NULL, NULL, _("Auto-merging %s"), path); } else if (S_ISGITLINK(a->mode)) {
Even after this removal of this cute bitflip, there is one more subtle use still in the code:
merge-ort.c +2 -1
@@ -2629,7 +2629,8 @@ static char *check_for_directory_rename(struct merge_options *opt, new_path =handle_path_level_conflicts (opt, path, side_index, rename_info, &collisions[side_index]); - *clean_merge &= (new_path !=NULL ); +if (*clean_merge && !new_path) + *clean_merge =0 ;return new_path; }
I had to think very carefully about this cleverness to be sure that this conversion is right (and I'm only mostly sure). When (new_path != NULL) is false, then we definitely set *clean_merge to zero. Otherwise, we set it to 1 (but only when it was already 1 or -1). Technically, this does change the behavior by not squashing -1 into 1, but that is less likely to be an existing value of *clean_merge.
There are other uses of "clean &= collect_renames(...)" but that appears to be fine because collect_renames() never results in an abort state (returning -1).
Thanks,
-Stolee
PATCH v2 4/7 merge-ort: clearer propagation of failure-to-function from merge_submodule
In reply to Elijah Newren via GitGitGadgetFrom: Elijah Newren <[email protected]>
The 'clean' member variable is somewhat of a tri-state (1 = clean, 0 = conflicted, -1 = failure-to-determine), but we often like to think of it as binary (ignoring the possibility of a negative value) and use constructs like '!clean' to reflect this. However, these constructs can make codepaths more difficult to understand, unless we handle the negative case early and return pre-emptively; do that in handle_content_merge() to make the code a bit easier to read.
--- merge-ort.c | 2 ++ 1 file changed, 2 insertions(+)
merge-ort.c +2 -0
@@ -2193,6 +2193,8 @@ static int handle_content_merge(struct merge_options *opt, clean =merge_submodule (opt, pathnames[0 ], two_way ?null_oid () : &o->oid, &a->oid, &b->oid, &result->oid); +if (clean <0 ) +return -1 ;if (opt->priv->call_depth && two_way && !clean) { result->mode = o->mode;oidcpy (&result->oid, &o->oid);
Signature · 1 line
gitgitgadget
Re: [PATCH v2 4/7] merge-ort: clearer propagation of failure-to-function from merge_submodule
Quoted 9 lines · Elijah Newren via GitGitGadget
From: Elijah Newren <[email protected]>
The 'clean' member variable is somewhat of a tri-state (1 = clean, 0 = conflicted, -1 = failure-to-determine), but we often like to think of it as binary (ignoring the possibility of a negative value) and use constructs like '!clean' to reflect this. However, these constructs can make codepaths more difficult to understand, unless we handle the negative case early and return pre-emptively; do that in
handle_content_merge()to make the code a bit easier to read.
This patch is correct and valuable.
Would it be valuable to go a bit further and turn 'clean' into an enum that reflects these states? Perhaps that would prevent future changes from slipping into this mistake.
Thanks,
-Stolee
Re: [PATCH v2 4/7] merge-ort: clearer propagation of failure-to-function from merge_submodule
Quoted 12 lines · Derrick Stolee
Quoted 9 lines · Elijah Newren via GitGitGadget
From: Elijah Newren <[email protected]>
The 'clean' member variable is somewhat of a tri-state (1 = clean, 0 = conflicted, -1 = failure-to-determine), but we often like to think of it as binary (ignoring the possibility of a negative value) and use constructs like '!clean' to reflect this. However, these constructs can make codepaths more difficult to understand, unless we handle the negative case early and return pre-emptively; do that in
handle_content_merge()to make the code a bit easier to read.
Quoted 5 lines
This patch is correct and valuable.
Would it be valuable to go a bit further and turn 'clean' into an enum that reflects these states? Perhaps that would prevent future changes from slipping into this mistake.
That may make sense to investigate, but I suspect it may be a bigger change and would recommend making such a clean up a separate series.
Also, I'm curious if it makes sense to finish off replacing recursive with ort first; as long as recursive exists, it has the same problem and in fact was the source of using a tri-state 'clean' variable and thus would need the same cleanup. But if we replace recursive with ort (making explicit requests for 'recursive' be handled by 'ort', as originally designed and intended), that cuts the number of sites needing this cleanup in half.
Re: [PATCH v2 4/7] merge-ort: clearer propagation of failure-to-function from merge_submodule
Quoted 22 lines · Elijah Newren
Quoted 12 lines · Derrick Stolee
Quoted 9 lines · Elijah Newren via GitGitGadget
From: Elijah Newren <[email protected]>
The 'clean' member variable is somewhat of a tri-state (1 = clean, 0 = conflicted, -1 = failure-to-determine), but we often like to think of it as binary (ignoring the possibility of a negative value) and use constructs like '!clean' to reflect this. However, these constructs can make codepaths more difficult to understand, unless we handle the negative case early and return pre-emptively; do that in
handle_content_merge()to make the code a bit easier to read.Quoted 5 lines
This patch is correct and valuable.
Would it be valuable to go a bit further and turn 'clean' into an enum that reflects these states? Perhaps that would prevent future changes from slipping into this mistake.
That may make sense to investigate, but I suspect it may be a bigger change and would recommend making such a clean up a separate series.
Quoted 7 lines
Also, I'm curious if it makes sense to finish off replacing recursive with ort first; as long as recursive exists, it has the same problem and in fact was the source of using a tri-state 'clean' variable and thus would need the same cleanup. But if we replace recursive with ort (making explicit requests for 'recursive' be handled by 'ort', as originally designed and intended), that cuts the number of sites needing this cleanup in half.
Your fast response to this message means that I didn't see this when I mentioned it in my closing of the review (in response to your cover letter).
Reducing the size of the conversion would definitely be good to do, and then you could also consider using the existing ll_merge_result enum, though it technically has four states.
Thanks,
-Stolee
PATCH v2 5/7 merge-ort: loosen commented requirements
In reply to Elijah Newren via GitGitGadgetFrom: Elijah Newren <[email protected]>
The comment above type_short_descriptions claimed that the order had to match what was found in the conflict_info_and_types enum. Since type_short_descriptions uses designated initializers, the order should not actually matter; I am guessing that positional initializers may have been under consideration when that comment was added, but the comment was not updated when designated initializers were chosen.
--- merge-ort.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
merge-ort.c +1 -1
@@ -553,7 +553,7 @@ enum conflict_and_info_types {* Short description of conflict type, relied upon by external tools. * * We can add more entries, but DO NOT change any of these strings. Also, -* Order MUST match conflict_info_and_types. +* please ensure the order matches what is used in conflict_info_and_types. */ static const char *type_short_descriptions[] = {/*** "Simple" conflicts and informational messages ***/
Signature · 1 line
gitgitgadget
PATCH v2 6/7 merge-ort: upon merge abort, only show messages causing the abort
In reply to Elijah Newren via GitGitGadgetFrom: Elijah Newren <[email protected]>
When something goes wrong enough that we need to abort early and not even attempt merging the remaining files, it probably does not make sense to report conflicts messages for the subset of files we processed before hitting the fatal error. Instead, only show the messages associated with paths where we hit the fatal error. Also, print these messages to stderr rather than stdout.
--- merge-ort.c | 78 ++++++++++++++++++++++++++++++++++++----------------- 1 file changed, 53 insertions(+), 25 deletions(-)
merge-ort.c +53 -25
@@ -543,10 +543,24 @@ enum conflict_and_info_types {CONFLICT_SUBMODULE_HISTORY_NOT_AVAILABLE ,CONFLICT_SUBMODULE_MAY_HAVE_REWINDS ,CONFLICT_SUBMODULE_NULL_MERGE_BASE , -CONFLICT_SUBMODULE_CORRUPT , + +/* INSERT NEW ENTRIES HERE */ + +/* +* Keep this entry after all regular conflict and info types; only +* errors (failures causing immediate abort of the merge) should +* come after this. +*/ +NB_REGULAR_CONFLICT_TYPES , + +/* +* Something is seriously wrong; cannot even perform merge; +* Keep this group _last_ other than NB_TOTAL_TYPES +*/ +ERROR_SUBMODULE_CORRUPT ,/* Keep this entry _last_ in the list */ - NB_CONFLICT_TYPES, + NB_TOTAL_TYPES, };/*
@@ -597,8 +611,10 @@ static const char *type_short_descriptions[] = { "CONFLICT (submodule may have rewinds)", [CONFLICT_SUBMODULE_NULL_MERGE_BASE ] = "CONFLICT (submodule lacks merge base)", - [CONFLICT_SUBMODULE_CORRUPT ] = -"CONFLICT (submodule corrupt)" + +/* Something is seriously wrong; cannot even perform merge */ + [ERROR_SUBMODULE_CORRUPT ] = + "ERROR (submodule corrupt)", };struct logical_conflict_info {
@@ -762,7 +778,8 @@ static void path_msg(struct merge_options *opt,/* Sanity checks */ assert (omittable_hint == - !starts_with (type_short_descriptions[type],"CONFLICT" ) || + (!starts_with (type_short_descriptions[type],"CONFLICT" ) && + !starts_with (type_short_descriptions[type],"ERROR" )) || type ==CONFLICT_DIR_RENAME_SUGGESTED );if (opt->record_conflict_msgs_as_headers && omittable_hint)return ;/* Do not record mere hints in headers */
@@ -1817,9 +1834,9 @@ static int merge_submodule(struct merge_options *opt,/* check whether both changes are forward */ ret2 =repo_in_merge_bases (&subrepo, commit_o, commit_a);if (ret2 <0 ) { -path_msg (opt,CONFLICT_SUBMODULE_CORRUPT ,0 , +path_msg (opt,ERROR_SUBMODULE_CORRUPT ,0 , path,NULL ,NULL ,NULL , -_ ("Failed to merge submodule %s " +_ ("error: failed to merge submodule %s " "(repository corrupt)" ), path); ret =-1 ;
@@ -1828,9 +1845,9 @@ static int merge_submodule(struct merge_options *opt,if (ret2 >0 ) ret2 =repo_in_merge_bases (&subrepo, commit_o, commit_b);if (ret2 <0 ) { -path_msg (opt,CONFLICT_SUBMODULE_CORRUPT ,0 , +path_msg (opt,ERROR_SUBMODULE_CORRUPT ,0 , path,NULL ,NULL ,NULL , -_ ("Failed to merge submodule %s " +_ ("error: failed to merge submodule %s " "(repository corrupt)" ), path); ret =-1 ;
@@ -1848,9 +1865,9 @@ static int merge_submodule(struct merge_options *opt,/* Case #1: a is contained in b or vice versa */ ret2 =repo_in_merge_bases (&subrepo, commit_a, commit_b);if (ret2 <0 ) { -path_msg (opt,CONFLICT_SUBMODULE_CORRUPT ,0 , +path_msg (opt,ERROR_SUBMODULE_CORRUPT ,0 , path,NULL ,NULL ,NULL , -_ ("Failed to merge submodule %s " +_ ("error: failed to merge submodule %s " "(repository corrupt)" ), path); ret =-1 ;
@@ -1867,9 +1884,9 @@ static int merge_submodule(struct merge_options *opt, } ret2 =repo_in_merge_bases (&subrepo, commit_b, commit_a);if (ret2 <0 ) { -path_msg (opt,CONFLICT_SUBMODULE_CORRUPT ,0 , +path_msg (opt,ERROR_SUBMODULE_CORRUPT ,0 , path,NULL ,NULL ,NULL , -_ ("Failed to merge submodule %s " +_ ("error: failed to merge submodule %s " "(repository corrupt)" ), path); ret =-1 ;
@@ -1901,9 +1918,9 @@ static int merge_submodule(struct merge_options *opt, &merges);switch (parent_count) {case -1 : -path_msg (opt,CONFLICT_SUBMODULE_CORRUPT ,0 , +path_msg (opt,ERROR_SUBMODULE_CORRUPT ,0 , path,NULL ,NULL ,NULL , -_ ("Failed to merge submodule %s " +_ ("error: failed to merge submodule %s " "(repository corrupt)" ), path); ret =-1 ;
@@ -4646,6 +4663,7 @@ void merge_display_update_messages(struct merge_options *opt,struct hashmap_iter iter;struct strmap_entry *e;struct string_list olist =STRING_LIST_INIT_NODUP ; +FILE *o = stdout;if (opt->record_conflict_msgs_as_headers)BUG ("Either display conflict messages or record them as headers, not both" );
@@ -4662,6 +4680,10 @@ void merge_display_update_messages(struct merge_options *opt, }string_list_sort (&olist); +/* Print to stderr if we hit errors rather than just conflicts */ +if (result->clean <0 ) + o = stderr; +/* Iterate over the items, printing them */ for (int path_nr =0 ; path_nr < olist.nr; ++path_nr) {struct string_list *conflicts = olist.items[path_nr].util;
@@ -4669,25 +4691,31 @@ void merge_display_update_messages(struct merge_options *opt,struct logical_conflict_info *info = conflicts->items[i].util; +/* On failure, ignore regular conflict types */ +if (result->clean <0 && + info->type <NB_REGULAR_CONFLICT_TYPES ) +continue ; +if (detailed) { -printf ("%lu" , (unsigned long )info->paths.nr); -putchar ('\0' ); +fprintf (o,"%lu" , (unsigned long )info->paths.nr); +fputc ('\0' , o);for (int n =0 ; n < info->paths.nr; n++) { -fputs (info->paths.v[n], stdout); -putchar ('\0' ); +fputs (info->paths.v[n], o); +fputc ('\0' , o); } -fputs (type_short_descriptions[info->type], - stdout); -putchar ('\0' ); +fputs (type_short_descriptions[info->type], o); +fputc ('\0' , o); } -puts (conflicts->items[i].string); +fputs (conflicts->items[i].string, o); +fputc ('\n' , o);if (detailed) -putchar ('\0' ); +fputc ('\0' , o); } }string_list_clear (&olist,0 ); -print_submodule_conflict_suggestion (&opti->conflicted_submodules); +if (result->clean >=0 ) +print_submodule_conflict_suggestion (&opti->conflicted_submodules);/* Also include needed rename limit adjustment now */ diff_warn_rename_limit("merge.renamelimit",
Signature · 1 line
gitgitgadget
PATCH v2 7/7 merge-ort: convert more error() cases to path_msg()
In reply to Elijah Newren via GitGitGadgetFrom: Elijah Newren <[email protected]>
merge_submodule() stores errors using path_msg(), whereas other call sites make use of the error() function. This is inconsistent, and moving towards path_msg() seems more friendly for libification efforts since it will allow the caller to determine whether the error messages need to be printed.
Note that this deferred handling of error messages changes the error message in a recursive merge from
error: failed to execute internal merge
to
From inner merge: error: failed to execute internal merge
which provides a little more information about the error which may be useful. Since the recursive merge strategy still only shows the older error, we had to adjust the new testcase introduced a few commits ago to just search for the older message somewhere in the output.
--- merge-ort.c | 53 +++++++++++++++++++++++++++++++++---------- t/t6406-merge-attr.sh | 2 +- 2 files changed, 42 insertions(+), 13 deletions(-)
merge-ort.c +41 -12
@@ -558,6 +558,10 @@ enum conflict_and_info_types {* Keep this group _last_ other than NB_TOTAL_TYPES */ ERROR_SUBMODULE_CORRUPT , +ERROR_THREEWAY_CONTENT_MERGE_FAILED , +ERROR_OBJECT_WRITE_FAILED , +ERROR_OBJECT_READ_FAILED , +ERROR_OBJECT_NOT_A_BLOB ,/* Keep this entry _last_ in the list */ NB_TOTAL_TYPES,
@@ -615,6 +619,14 @@ static const char *type_short_descriptions[] = {/* Something is seriously wrong; cannot even perform merge */ [ERROR_SUBMODULE_CORRUPT ] = "ERROR (submodule corrupt)", + [ERROR_THREEWAY_CONTENT_MERGE_FAILED ] = + "ERROR (three-way content merge failed)", + [ERROR_OBJECT_WRITE_FAILED ] = + "ERROR (object write failed)", + [ERROR_OBJECT_READ_FAILED ] = + "ERROR (object read failed)", + [ERROR_OBJECT_NOT_A_BLOB ] = + "ERROR (object is not a blob)", };struct logical_conflict_info {
@@ -2190,15 +2202,24 @@ static int handle_content_merge(struct merge_options *opt, pathnames, extra_marker_size, &result_buf); -if ((merge_status <0 ) || !result_buf.ptr) - ret =error (_ ("failed to execute internal merge" )); +if ((merge_status <0 ) || !result_buf.ptr) { +path_msg (opt,ERROR_THREEWAY_CONTENT_MERGE_FAILED ,0 , + pathnames[0 ], pathnames[1 ], pathnames[2 ],NULL , +_ ("error: failed to execute internal merge for %s" ), + path); + ret =-1 ; + }if (!ret &&write_object_file (result_buf.ptr, result_buf.size, -OBJ_BLOB , &result->oid)) - ret =error (_ ("unable to add %s to database" ), path); - +OBJ_BLOB , &result->oid)) { +path_msg (opt,ERROR_OBJECT_WRITE_FAILED ,0 , + pathnames[0 ], pathnames[1 ], pathnames[2 ],NULL , +_ ("error: unable to add %s to database" ), path); + ret =-1 ; + }free (result_buf.ptr); +if (ret)return -1 ;if (merge_status >0 )
@@ -3577,18 +3598,26 @@ static int sort_dirs_next_to_their_children(const char *one, const char *two)return c1 - c2; } -static int read_oid_strbuf (const struct object_id *oid, -struct strbuf *dst) +static int read_oid_strbuf (struct merge_options *opt, +const struct object_id *oid, +struct strbuf *dst, +const char *path) {void *buf;enum object_type type;unsigned long size; buf =repo_read_object_file (the_repository, oid, &type, &size); -if (!buf) -return error (_ ("cannot read object %s" ),oid_to_hex (oid)); +if (!buf) { +path_msg (opt,ERROR_OBJECT_READ_FAILED ,0 , + path,NULL ,NULL ,NULL , +_ ("error: cannot read object %s" ),oid_to_hex (oid)); +return -1 ; + }if (type !=OBJ_BLOB ) {free (buf); -return error (_ ("object %s is not a blob" ),oid_to_hex (oid)); +path_msg (opt,ERROR_OBJECT_NOT_A_BLOB ,0 , + path,NULL ,NULL ,NULL , +_ ("error: object %s is not a blob" ),oid_to_hex (oid)); }strbuf_attach (dst, buf, size, size +1 );return 0 ;
@@ -3612,8 +3641,8 @@ static int blob_unchanged(struct merge_options *opt,if (oideq (&base->oid, &side->oid))return 1 ; -if (read_oid_strbuf (&base->oid, &basebuf) || -read_oid_strbuf (&side->oid, &sidebuf)) +if (read_oid_strbuf (opt, &base->oid, &basebuf, path) || +read_oid_strbuf (opt, &side->oid, &sidebuf, path)) goto error_return;/* * Note: binary | is used so that both renormalizations are
t/t6406-merge-attr.sh +1 -1
@@ -295,7 +295,7 @@ test_expect_success !WINDOWS 'custom merge driver that is killed with a signal o >./please-abort && echo "* merge=custom" >.gitattributes && test_expect_code 2 git merge recursive-a 2>err && - grep "^error: failed to execute internal merge" err && + grep "error: failed to execute internal merge" err && git ls-files -u >output && git diff --name-only HEAD >>output && test_must_be_empty output
Signature · 1 line
gitgitgadget
Re: [PATCH v2 7/7] merge-ort: convert more error() cases to path_msg()
Quoted 16 lines · Elijah Newren via GitGitGadget
merge-ort.c +9
-2+static int read_oid_strbuf (struct merge_options *opt, +const struct object_id *oid, +struct strbuf *dst, +const char *path) {void *buf;enum object_type type;unsigned long size; buf =repo_read_object_file (the_repository, oid, &type, &size); -if (!buf) -return error (_ ("cannot read object %s" ),oid_to_hex (oid)); +if (!buf) { +path_msg (opt,ERROR_OBJECT_READ_FAILED ,0 , + path,NULL ,NULL ,NULL , +_ ("error: cannot read object %s" ),oid_to_hex (oid)); +return -1 ;
merge-ort.c static int sort_dirs_next_to_their_children(const char *one, const char *two) · line 3623
Quoted 10 lines
merge-ort.c +4
-1+ }if (type !=OBJ_BLOB ) {free (buf); -return error (_ ("object %s is not a blob" ),oid_to_hex (oid)); +path_msg (opt,ERROR_OBJECT_NOT_A_BLOB ,0 , + path,NULL ,NULL ,NULL , +_ ("error: object %s is not a blob" ),oid_to_hex (oid)); }strbuf_attach (dst, buf, size, size +1 );return 0 ;
This loses the early return in the "type != OBJ_BLOB" code path. So we free(buf), but then continue on to the strbuf_attach() call on the dangling pointer. Should it "return -1" like the earlier conditional?
-Peff
Re: [PATCH v2 7/7] merge-ort: convert more error() cases to path_msg()
Quoted 30 lines · Jeff King
Quoted 16 lines · Elijah Newren via GitGitGadget
merge-ort.c +9
-2+static int read_oid_strbuf (struct merge_options *opt, +const struct object_id *oid, +struct strbuf *dst, +const char *path) {void *buf;enum object_type type;unsigned long size; buf =repo_read_object_file (the_repository, oid, &type, &size); -if (!buf) -return error (_ ("cannot read object %s" ),oid_to_hex (oid)); +if (!buf) { +path_msg (opt,ERROR_OBJECT_READ_FAILED ,0 , + path,NULL ,NULL ,NULL , +_ ("error: cannot read object %s" ),oid_to_hex (oid)); +return -1 ;merge-ort.c static int sort_dirs_next_to_their_children(const char *one, const char *two) · line 3623
Quoted 10 lines
merge-ort.c +4
-1+ }if (type !=OBJ_BLOB ) {free (buf); -return error (_ ("object %s is not a blob" ),oid_to_hex (oid)); +path_msg (opt,ERROR_OBJECT_NOT_A_BLOB ,0 , + path,NULL ,NULL ,NULL , +_ ("error: object %s is not a blob" ),oid_to_hex (oid)); }strbuf_attach (dst, buf, size, size +1 );return 0 ;
Quoted 5 lines
This loses the early return in the "type !=
OBJ_BLOB" code path. So we free(buf), but then continue on to thestrbuf_attach()call on the dangling pointer. Should it "return -1" like the earlier conditional?-Peff
Oops! That's embarrassing. Thanks for catching; I'll send in a patch on top since this is already in next.
Re: [PATCH v2 7/7] merge-ort: convert more error() cases to path_msg()
Quoted 2 lines · Elijah Newren
Oops! That's embarrassing. Thanks for catching; I'll send in a patch on top since this is already in next.
Thanks.
Re: [PATCH v2 7/7] merge-ort: convert more error() cases to path_msg()
In reply to Elijah NewrenQuoted 10 lines · Elijah Newren
Quoted 10 lines
Quoted 9 lines
if (type != OBJ_BLOB) { free(buf);merge-ort.c +3
-1-return error (_ ("object %s is not a blob" ),oid_to_hex (oid)); +path_msg (opt,ERROR_OBJECT_NOT_A_BLOB ,0 , + path,NULL ,NULL ,NULL , +_ ("error: object %s is not a blob" ),oid_to_hex (oid)); }strbuf_attach (dst, buf, size, size +1 );return 0 ;
Quoted 6 lines
Quoted 3 lines
This loses the early return in the "type !=
OBJ_BLOB" code path. So we free(buf), but then continue on to thestrbuf_attach()call on the dangling pointer. Should it "return -1" like the earlier conditional?Oops! That's embarrassing. Thanks for catching; I'll send in a patch on top since this is already in next.
Yeah, I only caught it because Coverity flagged it when it hit 'next'. It is quite subtle. :)
-Peff
Quoted 6 lines · Elijah Newren via GitGitGadget
This series started as a just a fix for the abort hit in merge-ort when custom merge drivers error out (see
https://lore.kernel.org/git/[email protected]/).
However, while working on that, I found a few other issues around error codepaths in merge-ort. So this series:
Quoted 6 lines
* Patches 1-2: fix the reported abort problem
* Patches 3-4: make code inhandle_content_merges()easier to handle when we hit errors
* Patch 5: fix a misleading comment
* Patches 6-7: make error handling (immediate print vs. letting callers get the error information) more consistent
I saw this in Junio's email about series requiring review, so I took a look despite missing v1. Stepping through each commit in my local copy helped make sure that these changes were correct in their proper context.
Quoted 6 lines
The last two patches change the behavior slightly for error codepaths, and there's a question about whether we should show only the error messages that caused an early termination of the merge, or if we should also show any conflict messages for other paths that were handled before we hit the early termination. These patches made a decision but feel free to take those last two patches as more of an RFC.
I also support this change in the final two patches.
One thing I mentioned in an earlier reply was "why not use an enum in this tri-state 'clean' variable?" and then I tried to make just such a patch. There were two problems:
1. There were hundreds of changes that would be difficult to do in small bits (but maybe doable).
2. There is already an 'enum ll_merge_result' in merge-ll.h that is silently passed back from merge_3way() in merge-ort.c. While it's technically four states, maybe it would suffice to use that enum instead of creating a new one.
But I'll leave that as something to think about another time. It does not change the merit of this series. I left a note about another "&=" case that wasn't touched, but it's not wrong as-is.
Thanks,
-Stolee
Quoted 11 lines · Derrick Stolee
I saw this in Junio's email about series requiring review, so I took a look despite missing v1. Stepping through each commit in my local copy helped make sure that these changes were correct in their proper context.
Quoted 6 lines
The last two patches change the behavior slightly for error codepaths, and there's a question about whether we should show only the error messages that caused an early termination of the merge, or if we should also show any conflict messages for other paths that were handled before we hit the early termination. These patches made a decision but feel free to take those last two patches as more of an RFC.
Quoted 8 lines
I also support this change in the final two patches.
One thing I mentioned in an earlier reply was "why not use an enum in this tri-state 'clean' variable?" and then I tried to make just such a patch. ...
... But I'll leave that as something to think about another time. It does not change the merit of this series. I left a note about another "&=" case that wasn't touched, but it's not wrong as-is.
Thanks for having done a thorough review, including exploration of an alternative. Really appreciated.