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