git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v2 2/7] merge-ort: maintain expected invariant for priv member

From
Derrick Stolee <stolee@gmail.com>
Date
Jun 28, 2024, 02:09 UTC
Message-ID
<0df65763-9419-4ed4-ba28-a82ea1ac6dcb@gmail.com>
In-Reply-To
<d1adec6d10556e247c21f94420879724fa2c6436.1718766019.git.gitgitgadget@gmail.com>
On 6/18/24 11:00 PM, Elijah Newren via GitGitGadget wrote:
Show 15 quoted lines
> From: Elijah Newren <newren@gmail.com>
> 
> 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.
"one of these problems" is key here.
Show 7 quoted lines
> @@ -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.

Show 8 quoted lines
> @@ -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

Previous: Elijah Newren via GitGitGadgetNext: Elijah Newren via GitGitGadget
Message 19 of 34 in “Fix and improve some error codepaths in merge-ort”
  1. 0/7 Fix and improve some error codepaths in merge-ortElijah Newren via GitGitGadget, Jun 13, 2024
  2. 1/7 merge-ort: extract handling of priv member into reusable functionElijah Newren via GitGitGadget, Jun 13, 2024
  3. Junio C HamanoJun 13, 2024
  4. Taylor BlauJun 13, 2024
  5. 2/7 merge-ort: maintain expected invariant for priv memberElijah Newren via GitGitGadget, Jun 13, 2024
  6. Taylor BlauJun 13, 2024
  7. Elijah NewrenJun 19, 2024
  8. 3/7 merge-ort: fix type of local 'clean' var in handle_content_merge()Elijah Newren via GitGitGadget, Jun 13, 2024
  9. 4/7 merge-ort: clearer propagation of failure-to-function from merge_submoduleElijah Newren via GitGitGadget, Jun 13, 2024
  10. 5/7 merge-ort: loosen commented requirementsElijah Newren via GitGitGadget, Jun 13, 2024
  11. 6/7 merge-ort: upon merge abort, only show messages causing the abortElijah Newren via GitGitGadget, Jun 13, 2024
  12. Eric SunshineJun 14, 2024
  13. Elijah NewrenJun 19, 2024
  14. 7/7 merge-ort: convert more error() cases to path_msg()Elijah Newren via GitGitGadget, Jun 13, 2024
  15. Taylor BlauJun 13, 2024
  16. 0/7 Fix and improve some error codepaths in merge-ortElijah Newren via GitGitGadget, Jun 19, 2024
  17. 1/7 merge-ort: extract handling of priv member into reusable functionElijah Newren via GitGitGadget, Jun 19, 2024
  18. 2/7 merge-ort: maintain expected invariant for priv memberElijah Newren via GitGitGadget, Jun 19, 2024
  19. Derrick StoleeJun 28, 2024
  20. 3/7 merge-ort: fix type of local 'clean' var in handle_content_merge()Elijah Newren via GitGitGadget, Jun 19, 2024
  21. Derrick StoleeJun 28, 2024
  22. 4/7 merge-ort: clearer propagation of failure-to-function from merge_submoduleElijah Newren via GitGitGadget, Jun 19, 2024
  23. Derrick StoleeJun 28, 2024
  24. Elijah NewrenJun 28, 2024
  25. Derrick StoleeJun 28, 2024
  26. 5/7 merge-ort: loosen commented requirementsElijah Newren via GitGitGadget, Jun 19, 2024
  27. 6/7 merge-ort: upon merge abort, only show messages causing the abortElijah Newren via GitGitGadget, Jun 19, 2024
  28. 7/7 merge-ort: convert more error() cases to path_msg()Elijah Newren via GitGitGadget, Jun 19, 2024
  29. Jeff KingJul 2, 2024
  30. Elijah NewrenJul 3, 2024
  31. Junio C HamanoJul 3, 2024
  32. Jeff KingJul 6, 2024
  33. Derrick StoleeJun 28, 2024
  34. Junio C HamanoJun 28, 2024

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.