Re: [PATCH 4/5] merge-ort: abort merge when trees have duplicate entries

[Junio C Hamano <gitster@pobox.com>]

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Elijah Newren <newren@gmail.com>
>
> Trees with duplicate entries are malformed; fsck reports "contains
> duplicate file entries" for them.  merge-ort has from the beginning
> assumed that we would never hit such trees.  It was written with the
> assumption that traverse_trees() calls collect_merge_info_callback() at
> most once per path.  The "sanity checks" in that callback (added in
> d2bc1994f363 (merge-ort: implement a very basic collect_merge_info(),
> 2020-12-13)) verify properties of each individual call but not that
> invariant.  The strmap_put() in setup_path_info() silently overwrites
> the entry from any prior call for the same path, because it assumed
> there would be no other path.  Unfortunately, supplemental data
> structures for various optimizations could still be tweaked before the
> extra paths were overwritten, and those data structures not matching
> expected state could trip various assertions.
>
> Change the return type of setup_path_info() from void to int to allow us
> to detect this case, and abort the merge with a clear error message when
> it occurs.

OK.

> @@ -1081,9 +1081,11 @@ static void setup_path_info(struct merge_options *opt,
>  			 */
>  			mi->is_null = 1;
>  	}
> -	strmap_put(&opt->priv->paths, fullpath, mi);
> +	if (strmap_put(&opt->priv->paths, fullpath, mi))
> +		return error(_("tree has duplicate entries for '%s'"), fullpath);

OK.  I was wondering what _other_ kind of malformed trees would the
updated code by this change is prepared to handle (most notably,
tree entries must be sorted, and one way to detect duplicate is to
remember one single path that we saw earlier, which would work as
long as the entries are sorted).  This "ah, we saw that path already"
approach is much more robust in that it does not have to depend on a
sorted tree.

Makes sense.