Re: [PATCH v2] merge-recursive: honor diff.algorithm
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jul 10, 2024, 17:58 UTC
- Message-ID
- <xmqqmsmpw2mp.fsf@gitster.g>
- In-Reply-To
- <pull.1743.v2.git.git.1720551701648.gitgitgadget@gmail.com>
"Antonin Delpeuch via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 8 quoted lines
> From: Antonin Delpeuch <antonin@delpeuch.eu> > > The documentation claims that "recursive defaults to the diff.algorithm > config setting", but this is currently not the case. This fixes it, > ensuring that diff.algorithm is used when -Xdiff-algorithm is not > supplied. This affects the following porcelain commands: "merge", > "rebase", "cherry-pick", "pull", "stash", "log", "am" and "checkout". > It also affects the "merge-tree" ancillary interrogator.
Unfortunate.
Since be733e12 (Merge branch 'en/merge-tree', 2022-07-14), merge-tree is no longer an interrogator but works as an manipulator. As it is meant to be used as a building block that gives a reliable and repeatable output, I am tempted to say it should be categorized as a plumbing, but second opinions do count. Elijah Cc'ed as it was his "fault" to add "--write-tree" mode to the command and forgetting to update command-list.txt ;-)
But I agree with the direction of this patch and the structure of the solution (i.e. have two variants of init_*_options()).
Show 7 quoted lines
> This change refactors the initialization of merge options to introduce > two functions, "init_merge_ui_options" and "init_merge_basic_options" > instead of just one "init_merge_options". This design follows the > approach used in diff.c, providing initialization methods for > porcelain and plumbing commands respectively. Thanks to that, the > "replay" and "merge-recursive" plumbing commands remain unaffected by > diff.algorithm.
In other words, these two are the only ones that use the _basic variant.
I am unsure (read: do not take this as my recommendation to change your patch) which one merge-tree should use, but other than that, nicely done.
Show 13 quoted lines
> diff --git a/log-tree.c b/log-tree.c > index 101079e8200..5d8fb6ff8df 100644 > --- a/log-tree.c > +++ b/log-tree.c > @@ -1025,7 +1025,7 @@ static int do_remerge_diff(struct rev_info *opt, > struct strbuf parent2_desc = STRBUF_INIT; > > /* Setup merge options */ > - init_merge_options(&o, the_repository); > + init_ui_merge_options(&o, the_repository); > o.show_rename_progress = 0; > o.record_conflict_msgs_as_headers = 1; > o.msg_header_prefix = "remerge";
Isn't log-tree shared with things like "git diff-tree" porcelain?
Show 20 quoted lines
> -static void merge_recursive_config(struct merge_options *opt)
> +static void merge_recursive_config(struct merge_options *opt, int ui)
> {
> char *value = NULL;
> int renormalize = 0;
> @@ -3930,11 +3930,20 @@ static void merge_recursive_config(struct merge_options *opt)
> } /* avoid erroring on values from future versions of git */
> free(value);
> }
> + if (ui) {
> + if (!git_config_get_string("diff.algorithm", &value)) {
> + long diff_algorithm = parse_algorithm_value(value);
> + if (diff_algorithm < 0)
> + die(_("unknown value for config '%s': %s"), "diff.algorithm", value);
> + opt->xdl_opts = (opt->xdl_opts & ~XDF_DIFF_ALGORITHM_MASK) | diff_algorithm;
> + free(value);
> + }
> + }
> git_config(git_xmerge_config, NULL);
> }This looks sensible. Even though we have a single merge_recursive() that is internally callable, depending on the callers, they may or may not want to be affected by configuration.
As to the tests, it felt a bit unnatural and error prone to make t7615 depend on material that appears to be made only for t3515 (by naming the directory as such).
We have not done "a test-material directory that is shared among multiple tests" in t/, but we have plenty of "test helpers that are shared across multiple tests" named lib-foo.sh. I wonder if doing something like
... in t/lib-histogram-merge-history.sh ...
# prepare history for merges that depend on diff.algorithm
setup_history_for_histogram () {
cat >file.c <<\EOF &&
... contents of base.c ...
EOF
git add file.c &&
git commit -m c0 &&
git tag c0 && cat >file.c <<\EOF &&
... contents of ours.c ...
EOF
...
git tag c2
} and make the setup step in t3515 (and t7615) use that shared set-up function like so:
. ./test-lib.sh . "$TEST_DIRECTORY/test-lib-histogram-merge/history.sh"
test_expect_success setup ' setup_history_for_histogram '
may be cleaner? I am mostly afraid of mistakes like "now we are done with the area 3515 covered let's remove all the traces of it, like t3515-cherry-pick-diff.sh and t3515/ directory", breaking an seemingly unrelated t7615.
Even better. Can't we save the scarce resource that is test number and make these not about "I test cherry-pick" and "I test merge"? You are testing how mergy operations are affected by the choice of diff.algorithm, so perhaps create a single test file and name it after that single shared aspect of the tests you are adding? Perhaps t/t7615-diff-algo-with-mergy-operations.sh that has all three of these:
* the setup_history_for_histogram() helper function as described above;
* the test for cherry-pick in this patch;
* the test for merge in this patch.
Thanks.