"Antonin Delpeuch via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
> From: Antonin Delpeuch <antonin@xxxxxxxxxxx>
>
> 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()).
> 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.
> 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?
> -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.
