mbox series

[v2,00/10] Complete merge-ort implementation...almost

Message ID pull.973.v2.git.git.1615271086.gitgitgadget@gmail.com (mailing list archive)
Headers show
Series Complete merge-ort implementation...almost | expand

Message

Philippe Blain via GitGitGadget March 9, 2021, 6:24 a.m. UTC
This series (nearly) completes the merge-ort implementation, cleaning up
testsuite failures. The exceptions are some t6423 failures being addressed
independently[1].

Changes since v1, to address feedback from Ævar:

 * Squashed patches 3 & 4
 * Various style fixes
 * Check attr_index->initialized before calling initialize_attr_index()
   instead of inside initialize_attr_index()
 * Simplify GIT_TEST_MERGE_ALGORITHM comparison to only have if-then block

Stuff not included in v2:

 * Ævar suggested patching the test expectation on the 4 known failing tests
   so that all tests passed under GIT_TEST_MERGE_ALGORITHM=ort. Seems
   reasonable, but the semantic conflicts with other series might make it
   more trouble than it's worth and other series will fix all 4 tests.
   Leaving as-is for now to avoid putting more burden on Junio; see
   https://lore.kernel.org/git/CABPp-BHeR6m4-M=nSX5NZtA2js3E3EVbAyDSMtp3-rN2QnUjqw@mail.gmail.com/
 * Ævar noted a few bigger cleanups to surrounding code that could also be
   done, even if orthogonal to this series. I'll leave those for other
   series to address.

[1] See
https://lore.kernel.org/git/pull.845.git.1614484707.gitgitgadget@gmail.com/
and batch 10 and 12 from
https://github.com/gitgitgadget/git/pulls?q=is%3Apr+author%3Anewren+Optimization+batch

Elijah Newren (10):
  merge-ort: use STABLE_QSORT instead of QSORT where required
  merge-ort: add a special minimal index just for renormalization
  merge-ort: have ll_merge() use a special attr_index for
    renormalization
  merge-ort: let renormalization change modify/delete into clean delete
  merge-ort: support subtree shifting
  t6428: new test for SKIP_WORKTREE handling and conflicts
  merge-ort: implement CE_SKIP_WORKTREE handling with conflicted entries
  t: mark several submodule merging tests as fixed under merge-ort
  merge-ort: write $GIT_DIR/AUTO_MERGE whenever we hit a conflict
  merge-recursive: add a bunch of FIXME comments documenting known bugs

 branch.c                                      |   1 +
 builtin/rebase.c                              |   1 +
 merge-ort.c                                   | 229 ++++++++++++++++--
 merge-recursive.c                             |  37 +++
 path.c                                        |   1 +
 path.h                                        |   2 +
 sequencer.c                                   |   5 +
 t/t3512-cherry-pick-submodule.sh              |   7 +-
 t/t3513-revert-submodule.sh                   |   5 +-
 t/t5572-pull-submodule.sh                     |   7 +-
 t/t6428-merge-conflicts-sparse.sh             | 158 ++++++++++++
 t/t6437-submodule-merge.sh                    |   5 +-
 t/t6438-submodule-directory-file-conflicts.sh |   7 +-
 13 files changed, 440 insertions(+), 25 deletions(-)
 create mode 100755 t/t6428-merge-conflicts-sparse.sh


base-commit: f01623b2c9d14207e497b21ebc6b3ec4afaf4b46
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-973%2Fnewren%2Fort-remainder-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-973/newren/ort-remainder-v2
Pull-Request: https://github.com/git/git/pull/973

Range-diff vs v1:

  1:  3ca16a5e3466 =  1:  3ca16a5e3466 merge-ort: use STABLE_QSORT instead of QSORT where required
  2:  24454e67b186 =  2:  24454e67b186 merge-ort: add a special minimal index just for renormalization
  3:  815af5d30ebd !  3:  386cb3230b67 merge-ort: add a function for initializing our special attr_index
     @@ Metadata
      Author: Elijah Newren <newren@gmail.com>
      
       ## Commit message ##
     -    merge-ort: add a function for initializing our special attr_index
     +    merge-ort: have ll_merge() use a special attr_index for renormalization
      
     -    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.
     +    ll_merge() needs an index when renormalization is requested.  Create one
     +    specifically for just this purpose with just the one needed entry.  This
     +    fixes t6418.4 and t6418.5 under GIT_TEST_MERGE_ALGORITHM=ort.
      
          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
     @@ Commit message
          Signed-off-by: Elijah Newren <newren@gmail.com>
      
       ## merge-ort.c ##
     +@@ merge-ort.c: static void clear_or_reinit_internal_opts(struct merge_options_internal *opti,
     + 	string_list_clear(&opti->paths_to_free, 0);
     + 	opti->paths_to_free.strdup_strings = 0;
     + 
     +-	if (opti->attr_index.cache_nr)
     ++	if (opti->attr_index.cache_nr) /* true iff opt->renormalize */
     + 		discard_index(&opti->attr_index);
     + 
     + 	/* Free memory used by various renames maps */
      @@ merge-ort.c: static int merge_submodule(struct merge_options *opt,
       	return 0;
       }
       
     -+MAYBE_UNUSED
      +static void initialize_attr_index(struct merge_options *opt)
      +{
      +	/*
     @@ merge-ort.c: static int merge_submodule(struct merge_options *opt,
      +	struct index_state *attr_index = &opt->priv->attr_index;
      +	struct cache_entry *ce;
      +
     -+	if (!opt->renormalize)
     -+		return;
     ++	attr_index->initialized = 1;
      +
     -+	if (attr_index->initialized)
     ++	if (!opt->renormalize)
      +		return;
     -+	attr_index->initialized = 1;
      +
      +	mi = strmap_get(&opt->priv->paths, GITATTRIBUTES_FILE);
      +	if (!mi)
     @@ merge-ort.c: static int merge_submodule(struct merge_options *opt,
      +		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 {
     ++	} else {
      +		int stage, len;
      +		struct conflict_info *ci;
      +
      +		ASSIGN_AND_VERIFY_CI(ci, mi);
     -+		for (stage=0; stage<3; ++stage) {
     ++		for (stage = 0; stage < 3; stage++) {
      +			unsigned stage_mask = (1 << stage);
      +
      +			if (!(ci->filemask & stage_mask))
     @@ merge-ort.c: static int merge_submodule(struct merge_options *opt,
       static int merge_3way(struct merge_options *opt,
       		      const char *path,
       		      const struct object_id *o,
     +@@ merge-ort.c: static int merge_3way(struct merge_options *opt,
     + 	char *base, *name1, *name2;
     + 	int merge_status;
     + 
     ++	if (!opt->priv->attr_index.initialized)
     ++		initialize_attr_index(opt);
     ++
     + 	ll_opts.renormalize = opt->renormalize;
     + 	ll_opts.extra_marker_size = extra_marker_size;
     + 	ll_opts.xdl_opts = opt->xdl_opts;
     +@@ merge-ort.c: static int merge_3way(struct merge_options *opt,
     + 
     + 	merge_status = ll_merge(result_buf, path, &orig, base,
     + 				&src1, name1, &src2, name2,
     +-				opt->repo->index, &ll_opts);
     ++				&opt->priv->attr_index, &ll_opts);
     + 
     + 	free(base);
     + 	free(name1);
  4:  cb035ac5fe4a <  -:  ------------ merge-ort: have ll_merge() calls use the attr_index for renormalization
  5:  b70ef4d7000a !  4:  7fcabded5016 merge-ort: let renormalization change modify/delete into clean delete
     @@ merge-ort.c: static int string_list_df_name_compare(const char *one, const char
      +	int ret = 0; /* assume changed for safety */
      +	const struct index_state *idx = &opt->priv->attr_index;
      +
     -+	initialize_attr_index(opt);
     ++	if (!idx->initialized)
     ++		initialize_attr_index(opt);
      +
      +	if (base->mode != side->mode)
      +		return 0;
  6:  d04ddabde124 =  5:  e21eea71e707 merge-ort: support subtree shifting
  7:  6ccb24b557fc =  6:  d1d8c017b23f t6428: new test for SKIP_WORKTREE handling and conflicts
  8:  100c0187bdfe =  7:  90a57ade629d merge-ort: implement CE_SKIP_WORKTREE handling with conflicted entries
  9:  95a6c0abe558 !  8:  fcce88c5ac3d t: mark several submodule merging tests as fixed under merge-ort
     @@ t/t3512-cherry-pick-submodule.sh: export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
       
      -KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1
      -KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1
     -+if test "$GIT_TEST_MERGE_ALGORITHM" = ort
     ++if test "$GIT_TEST_MERGE_ALGORITHM" != ort
      +then
     -+	:  # No special additional KNOWN_FAILURE knobs to set
     -+else
      +	KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1
      +	KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1
      +fi
     @@ t/t3513-revert-submodule.sh: git_revert () {
       }
       
      -KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1
     -+if test "$GIT_TEST_MERGE_ALGORITHM" = ort
     ++if test "$GIT_TEST_MERGE_ALGORITHM" != ort
      +then
     -+	:  # No special additional KNOWN_FAILURE knobs to set
     -+else
      +	KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1
      +fi
       test_submodule_switch_func "git_revert"
     @@ t/t5572-pull-submodule.sh: git_pull_noff () {
       
      -KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1
      -KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1
     -+if test "$GIT_TEST_MERGE_ALGORITHM" = ort
     ++if test "$GIT_TEST_MERGE_ALGORITHM" != ort
      +then
     -+	:  # No special additional KNOWN_FAILURE knobs to set
     -+else
      +	KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1
      +	KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1
      +fi
     @@ t/t6438-submodule-directory-file-conflicts.sh: test_submodule_switch "merge --ff
       
      -KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1
      -KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1
     -+if test "$GIT_TEST_MERGE_ALGORITHM" = ort
     ++if test "$GIT_TEST_MERGE_ALGORITHM" != ort
      +then
     -+	:  # No special additional KNOWN_FAILURE knobs to set
     -+else
      +	KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1
      +	KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1
      +fi
 10:  d8c6eb39aa7c =  9:  93b241c2f213 merge-ort: write $GIT_DIR/AUTO_MERGE whenever we hit a conflict
 11:  0409118d4ff7 = 10:  a9a95bb0391f merge-recursive: add a bunch of FIXME comments documenting known bugs

Comments

Derrick Stolee March 11, 2021, 3:20 p.m. UTC | #1
On 3/9/2021 1:24 AM, Elijah Newren via GitGitGadget wrote:
> This series (nearly) completes the merge-ort implementation, cleaning up
> testsuite failures. The exceptions are some t6423 failures being addressed
> independently[1].

I like that most of the patches here are small and straight-forward, and
are backed up by tests.

I'm concerned about the coupling of the "index with one entry" mechanism.
I'd like to see that built differently by creating methods that can
operate on an attributes file directly. If this method extraction is truly
too difficult, then I'm willing to concede this point. It is worth some
effort to try, though.

Thanks,
-Stolee
Elijah Newren March 11, 2021, 4:42 p.m. UTC | #2
On Thu, Mar 11, 2021 at 7:20 AM Derrick Stolee <stolee@gmail.com> wrote:
>
> On 3/9/2021 1:24 AM, Elijah Newren via GitGitGadget wrote:
> > This series (nearly) completes the merge-ort implementation, cleaning up
> > testsuite failures. The exceptions are some t6423 failures being addressed
> > independently[1].
>
> I like that most of the patches here are small and straight-forward, and
> are backed up by tests.
>
> I'm concerned about the coupling of the "index with one entry" mechanism.
> I'd like to see that built differently by creating methods that can
> operate on an attributes file directly. If this method extraction is truly
> too difficult, then I'm willing to concede this point. It is worth some
> effort to try, though.

Oh, I agree, I hate the "index with one entry" mechanism to activate
the renormalization code.  It feels like a capitulation on merge-ort's
design and is such a hack.  I tried to figure out how to do it a
different way, and failed pretty hard.  I was close to the point of
throwing my hands in the air and leaving the renormalization tests
failing ("who uses that stuff anyway?"), but it was the _only_ test
that merge-recursive passed that merge-ort didn't, so I kept trying
anyway.  At some point I realized that a hack along these lines might
work.  Although, to be honest, I still don't understand this code or
why my patches work -- in particular, the get_stream_filter() call has
me flummoxed.  It returns a value, but I throw it away -- yet the code
fails unless I call it.  What is it actually doing?  Clearly it has
some kind of side-effect somewhere, but reading the code didn't seem
to help me.  I discovered the function and others via various
debugging and trial-and-error attempts, and ended up finding out that
just calling that function is enough.  I know it works, but I don't
know why.  I understand everything else in merge-ort; I hate having a
magic corner here.  I'm not happy with it.

I'm certain that with enough effort the renormalization stuff could be
fixed and made callable without an istate.  Perhaps there's even a
simple way to extract the relevant logic and I'm just being dense, but
I got very lost in that code and I'm a bit afraid of it now.
Elijah Newren March 17, 2021, 9:42 p.m. UTC | #3
On Mon, Mar 8, 2021 at 10:24 PM Elijah Newren via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> This series (nearly) completes the merge-ort implementation, cleaning up
> testsuite failures. The exceptions are some t6423 failures being addressed
> independently[1].

This series was subsumed into
https://lore.kernel.org/git/pull.905.v2.git.1616016485.gitgitgadget@gmail.com/.
So, ignore this topic and look for the changes over there.

(Reasons for the curious: This series had fixed t6409 and t6418 and
the submodule TODO passes, while the other series fixed t6423.  The
other series could not have been included when this was submitted
because it depended on the not-yet-submitted ort-perf-batch-10.  Why
not just wait?  Well Ævar wanted a way to test his changes against
merge-ort, this early submission provided that and helped him find a
bug -- see https://lore.kernel.org/git/20210308150650.18626-1-avarab@gmail.com/.
Now that we can fix all the tests, I'm withdrawing this one and
including it with the other fixes.)