Re: [PATCH 03/11] merge-ort: add a function for initializing our special attr_index

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Fri, Mar 05 2021, Elijah Newren via GitGitGadget wrote:

> From: Elijah Newren <newren@gmail.com>
>
> Add a function which can be called to populate the attr_index with the
> appropriate .gitattributes contents when necessary.  Make it return
> early if the attr_index is already initialized or if we are not
> renormalizing files.
>
> NOTE 1: Even if the user has a working copy or a real index (which is
> not a given as merge-ort can be used in bare repositories), we
> explicitly ignore any .gitattributes file from either of these
> locations.  merge-ort can be used to merge two branches that are
> unrelated to HEAD, so .gitattributes from the working copy and current
> index should not be considered relevant.
>
> NOTE 2: Since we are in the middle of merging, there is a risk that
> .gitattributes itself is conflicted...leaving us with an ill-defined
> situation about how to perform the rest of the merge.  It could be that
> the .gitattributes file does not even exist on one of the sides of the
> merge, or that it has been modified on both sides.  If it's been
> modified on both sides, it's possible that it could itself be merged
> cleanly, though it's also possible that it only merges cleanly if you
> use the right version of the .gitattributes file to drive the merge.  It
> gets kind of complicated.  The only test we ever had that attempted to
> test behavior in this area was seemingly unaware of the undefined
> behavior, but knew the test wouldn't work for lack of attribute handling
> support, marked it as test_expect_failure from the beginning, but
> managed to fail for several reasons unrelated to attribute handling.
> See commit 6f6e7cfb52 ("t6038: remove problematic test", 2020-08-03) for
> details.  So there are probably various ways to improve what
> initialize_attr_index() picks in the case of a conflicted .gitattributes
> but for now I just implemented something simple -- look for whatever
> .gitattributes file we can find in any of the higher order stages and
> use it.
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  merge-ort.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 61 insertions(+)
>
> diff --git a/merge-ort.c b/merge-ort.c
> index d91b66a052b6..028d1adcd2c9 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -988,6 +988,67 @@ static int merge_submodule(struct merge_options *opt,
>  	return 0;
>  }
>  
> +MAYBE_UNUSED

As with the lst series you had I also think this is better squashed with
04/11.

> +static void initialize_attr_index(struct merge_options *opt)
> +{
> +	/*
> +	 * The renormalize_buffer() functions require attributes, and
> +	 * annoyingly those can only be read from the working tree or from
> +	 * an index_state.  merge-ort doesn't have an index_state, so we
> +	 * generate a fake one containing only attribute information.
> +	 */
> +	struct merged_info *mi;
> +	struct index_state *attr_index = &opt->priv->attr_index;
> +	struct cache_entry *ce;
> +
> +	if (!opt->renormalize)
> +		return;
> +
> +	if (attr_index->initialized)
> +		return;

Will comment on this in 04/11.

> +	attr_index->initialized = 1;
> +
> +	mi = strmap_get(&opt->priv->paths, GITATTRIBUTES_FILE);
> +	if (!mi)
> +		return;
> +
> +	if (mi->clean) {
> +		int len = strlen(GITATTRIBUTES_FILE);
> +		ce = make_empty_cache_entry(attr_index, len);
> +		ce->ce_mode = create_ce_mode(mi->result.mode);
> +		ce->ce_flags = create_ce_flags(0);
> +		ce->ce_namelen = len;
> +		oidcpy(&ce->oid, &mi->result.oid);
> +		memcpy(ce->name, GITATTRIBUTES_FILE, len);
> +		add_index_entry(attr_index, ce,
> +				ADD_CACHE_OK_TO_ADD | ADD_CACHE_OK_TO_REPLACE);
> +		get_stream_filter(attr_index, GITATTRIBUTES_FILE, &ce->oid);
> +	}
> +	else {

Style nit: } else {

> +		int stage, len;
> +		struct conflict_info *ci;
> +
> +		ASSIGN_AND_VERIFY_CI(ci, mi);
> +		for (stage=0; stage<3; ++stage) {

Style nit: stage < 3

Style nit: I find just stage++ to be more readable in for-loops, makes
no difference to the compiler, just more idiomatic.

> +			unsigned stage_mask = (1 << stage);
> +
> +			if (!(ci->filemask & stage_mask))
> +				continue;
> +			len = strlen(GITATTRIBUTES_FILE);
> +			ce = make_empty_cache_entry(attr_index, len);
> +			ce->ce_mode = create_ce_mode(ci->stages[stage].mode);
> +			ce->ce_flags = create_ce_flags(stage);
> +			ce->ce_namelen = len;
> +			oidcpy(&ce->oid, &ci->stages[stage].oid);
> +			memcpy(ce->name, GITATTRIBUTES_FILE, len);
> +			add_index_entry(attr_index, ce,
> +					ADD_CACHE_OK_TO_ADD | ADD_CACHE_OK_TO_REPLACE);
> +			get_stream_filter(attr_index, GITATTRIBUTES_FILE,
> +					  &ce->oid);
> +		}
> +	}
> +}
> +