[PATCH] merge-ort: accept rename settings given without a value

[PATCH] merge-ort: accept rename settings given without a value

[Thomas Bachem via GitGitGadget]

From: Thomas Bachem <mail@thomasbachem.com>

"git -c merge.renames merge" fails with "missing value for
'merge.renames'", although git(1) says that "git -c foo.bar" sets
foo.bar to true. The same happens with diff.renames and
merge.directoryRenames, and with a setting written without
"= <value>" in a config file, which git-config(1) also documents as
true. Every command that merges through merge-ort fails this way.

merge-ort reads these settings with repo_config_get_string(), which
rejects a missing value. Read them with repo_config_get_value()
instead. git_config_rename() and git_parse_maybe_bool() already take
a missing value as true.

Assisted-by: Claude Fable 5.1
Signed-off-by: Thomas Bachem <mail@thomasbachem.com>
---
    merge-ort: accept rename settings given without a value

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2284%2Fthomasbachem%2Fmerge-ort-valueless-renames-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2284/thomasbachem/merge-ort-valueless-renames-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2284

 merge-ort.c                               | 20 ++++++++------------
 t/t6423-merge-rename-directories.sh       | 17 +++++++++++++++++
 t/t6434-merge-recursive-rename-options.sh | 12 ++++++++++++
 3 files changed, 37 insertions(+), 12 deletions(-)

diff --git a/merge-ort.c b/merge-ort.c
index c410a5d353..24ddd97696 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -5453,31 +5453,27 @@ void merge_incore_recursive(struct merge_options *opt,
 static void merge_recursive_config(struct merge_options *opt, int ui)
 {
 	char *value = NULL;
+	const char *v;
 	int renormalize = 0;
 	repo_config_get_int(opt->repo, "merge.verbosity", &opt->verbosity);
 	repo_config_get_int(opt->repo, "diff.renamelimit", &opt->rename_limit);
 	repo_config_get_int(opt->repo, "merge.renamelimit", &opt->rename_limit);
 	repo_config_get_bool(opt->repo, "merge.renormalize", &renormalize);
 	opt->renormalize = renormalize;
-	if (!repo_config_get_string(opt->repo, "diff.renames", &value)) {
-		opt->detect_renames = git_config_rename("diff.renames", value);
-		free(value);
-	}
-	if (!repo_config_get_string(opt->repo, "merge.renames", &value)) {
-		opt->detect_renames = git_config_rename("merge.renames", value);
-		free(value);
-	}
-	if (!repo_config_get_string(opt->repo, "merge.directoryrenames", &value)) {
-		int boolval = git_parse_maybe_bool(value);
+	if (!repo_config_get_value(opt->repo, "diff.renames", &v))
+		opt->detect_renames = git_config_rename("diff.renames", v);
+	if (!repo_config_get_value(opt->repo, "merge.renames", &v))
+		opt->detect_renames = git_config_rename("merge.renames", v);
+	if (!repo_config_get_value(opt->repo, "merge.directoryrenames", &v)) {
+		int boolval = git_parse_maybe_bool(v);
 		if (0 <= boolval) {
 			opt->detect_directory_renames = boolval ?
 				MERGE_DIRECTORY_RENAMES_TRUE :
 				MERGE_DIRECTORY_RENAMES_NONE;
-		} else if (!strcasecmp(value, "conflict")) {
+		} else if (!strcasecmp(v, "conflict")) {
 			opt->detect_directory_renames =
 				MERGE_DIRECTORY_RENAMES_CONFLICT;
 		} /* avoid erroring on values from future versions of git */
-		free(value);
 	}
 	if (ui) {
 		if (!repo_config_get_string(opt->repo, "diff.algorithm", &value)) {
diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh
index 24d4c5447b..da145c150c 100755
--- a/t/t6423-merge-rename-directories.sh
+++ b/t/t6423-merge-rename-directories.sh
@@ -97,6 +97,23 @@ test_expect_success '1a: Simple directory rename detection' '
 	)
 '
 
+test_expect_success '1a (valueless): merge.directoryRenames without a value means true' '
+	mkdir valueless &&
+	(
+		cd valueless &&
+		test_setup_1a &&
+		cd 1a &&
+
+		git checkout A^0 &&
+
+		git -c merge.directoryRenames merge -s recursive B^0 &&
+
+		git rev-parse >actual HEAD:y/d HEAD:y/e/f &&
+		git rev-parse >expect B:z/d B:z/e/f &&
+		test_cmp expect actual
+	)
+'
+
 # Testcase 1b, Merge a directory with another
 #   Commit O: z/{b,c},   y/d
 #   Commit A: z/{b,c,e}, y/d
diff --git a/t/t6434-merge-recursive-rename-options.sh b/t/t6434-merge-recursive-rename-options.sh
index 5a6f74839c..427de2ba95 100755
--- a/t/t6434-merge-recursive-rename-options.sh
+++ b/t/t6434-merge-recursive-rename-options.sh
@@ -332,4 +332,16 @@ test_expect_success 'merge.renames overrides diff.renames' '
 	$check_50
 '
 
+test_expect_success 'diff.renames without a value means true' '
+	git read-tree --reset -u HEAD &&
+	test_must_fail git -c diff.renames merge-recursive $tail &&
+	$check_50
+'
+
+test_expect_success 'merge.renames without a value means true' '
+	git read-tree --reset -u HEAD &&
+	test_must_fail git -c diff.renames=false -c merge.renames merge-recursive $tail &&
+	$check_50
+'
+
 test_done

base-commit: 6de20f6092dcf9bdb1c8efe03db4b70c82b423dd
-- 
gitgitgadget

Re: [PATCH] merge-ort: accept rename settings given without a value

[Junio C Hamano]

"Thomas Bachem via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Thomas Bachem <mail@thomasbachem.com>
>
> "git -c merge.renames merge" fails with "missing value for
> 'merge.renames'", although git(1) says that "git -c foo.bar" sets
> foo.bar to true. The same happens with diff.renames and
> merge.directoryRenames, and with a setting written without
> "= <value>" in a config file, which git-config(1) also documents as
> true. Every command that merges through merge-ort fails this way.

Searching the list archive for "valueless true" tells us that we had
corrected a handful of instances of config_get_string() with a call
to config_get_value() in the past half year.

And this patch seems to be in line with the other efforts to fix the
same misuse.

> +	if (!repo_config_get_value(opt->repo, "diff.renames", &v))
> +		opt->detect_renames = git_config_rename("diff.renames", v);
> +	if (!repo_config_get_value(opt->repo, "merge.renames", &v))
> +		opt->detect_renames = git_config_rename("merge.renames", v);
> +	if (!repo_config_get_value(opt->repo, "merge.directoryrenames", &v)) {
> +		int boolval = git_parse_maybe_bool(v);
>  		if (0 <= boolval) {
>  			opt->detect_directory_renames = boolval ?
>  				MERGE_DIRECTORY_RENAMES_TRUE :
>  				MERGE_DIRECTORY_RENAMES_NONE;
> -		} else if (!strcasecmp(value, "conflict")) {
> +		} else if (!strcasecmp(v, "conflict")) {
>  			opt->detect_directory_renames =
>  				MERGE_DIRECTORY_RENAMES_CONFLICT;
>  		} /* avoid erroring on values from future versions of git */
> -		free(value);
>  	}

OK.  Since get_value(), unlike get_string(), lets us borrow the
value as "const char *", once we used to drive git_config_rename() or
compared with fixed string to turn it into an enum value, we can simply
forget without calling free(), which is simpler.

Nicely done.