[PATCH 0/4] Attr fixes

[PATCH 0/4] Attr fixes

[Elijah Newren via GitGitGadget]

This fixes a few issues surrounding .gitattributes files and usage of the
merge machinery outside of "git merge". All were issues I found and fixed
while working on merge-ort.

Patches 1, 3, and 4 are definitely fixes. Patch 2 only touches a testcase
and might be a fix, or might just change it to a different kind of
brokenness -- either way, it leaves the affected testcase as
test_expect_failure. I'm kind of curious what is correct expected behavior
for that and similar testcases. I probably won't implement it, and I'm
worried it might be rife with multi-layered corner cases and no good way to
define correct behavior for all cases.

Elijah Newren (4):
  t6038: make tests fail for the right reason
  t6038: fix test with obviously incorrect expectations
  merge: make merge.renormalize work for all uses of merge machinery
  checkout: support renormalization with checkout -m <paths>

 builtin/checkout.c         | 18 ++++++------------
 builtin/merge.c            |  7 +++----
 merge-recursive.c          |  3 +++
 t/t6038-merge-text-auto.sh | 20 ++++++++++----------
 4 files changed, 22 insertions(+), 26 deletions(-)


base-commit: 47ae905ffb98cc4d4fd90083da6bc8dab55d9ecc
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-825%2Fnewren%2Fattr-fixes-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-825/newren/attr-fixes-v1
Pull-Request: https://github.com/git/git/pull/825
-- 
gitgitgadget

[PATCH 1/4] t6038: make tests fail for the right reason

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

t6038 had a pair of tests that were expected to fail, but weren't
failing for the expected reason.  Both were meant to do a merge that
could be done cleanly after renormalization, but were supposed to fail
for lack of renormalization.  Unfortunately, both tests has staged
changes, and checkout -m would abort due to the presence of those staged
changes before even attempting a merge.

Fix this first issue by utilizing git-restore instead of git-checkout,
so that the index is left alone and just the working directory gets the
changes we want.

However, there is a second issue with these tests.  Technically, they
just wanted to verify that after renormalization, no conflicts would be
present.  This could have been checked for by grepping for a lack of
conflict markers, but the test instead tried to compare the working
directory files to an expected result.  Unfortunately, the setting of
"text=auto" without setting core.eol to any value meant that the content
of the file (in particular, the line endings) would be
platform-dependent and the tests could only pass on some platforms.
Replace the existing comparison with a call to 'git diff --no-index
--ignore-cr-at-eol' to verify that the contents, other than possible
carriage returns in the file, match the expected results and in
particular that the file has no conflicts from the checkout -m
operation.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t6038-merge-text-auto.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t6038-merge-text-auto.sh b/t/t6038-merge-text-auto.sh
index 5e8d5fa50c..27cea15533 100755
--- a/t/t6038-merge-text-auto.sh
+++ b/t/t6038-merge-text-auto.sh
@@ -168,9 +168,9 @@ test_expect_failure 'checkout -m after setting text=auto' '
 	git rm -fr . &&
 	rm -f .gitattributes &&
 	git reset --hard initial &&
-	git checkout a -- . &&
+	git restore --source=a -- . &&
 	git checkout -m b &&
-	compare_files expected file
+	git diff --no-index --ignore-cr-at-eol expected file
 '
 
 test_expect_failure 'checkout -m addition of text=auto' '
@@ -183,9 +183,9 @@ test_expect_failure 'checkout -m addition of text=auto' '
 	git rm -fr . &&
 	rm -f .gitattributes file &&
 	git reset --hard initial &&
-	git checkout b -- . &&
+	git restore --source=b -- . &&
 	git checkout -m a &&
-	compare_files expected file
+	git diff --no-index --ignore-cr-at-eol expected file
 '
 
 test_expect_failure 'cherry-pick patch from after text=auto was added' '
-- 
gitgitgadget

[PATCH 2/4] t6038: fix test with obviously incorrect expectations

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

t6038.11, 'cherry-pick patch from after text=auto' was set up so that on
a branch with no .gitattributes file, you cherry-picked a patch from a
branch that had a .gitattributes file (containing '* text=auto').
Further, the two branches had a file which differed only in line
endings.  In this situation, correct behavior is not well defined:
should the .gitattributes file affect the merge or not?

If the .gitattributes file on the other branch should not affect the
merge, then we would have a content conflict with all three stages
different (the merge base didn't match either side).

If the .gitattributes file from the other branch should affect the
merge, then we would expect the line endings to be normalized to LF so
that the versions from both sides match, and then the cherry-pick has no
conflicts and can succeed.  After the cherry-pick, the line endings in
the file will change from CRLF to LF.

This test had an expectation that the line endings would remain CRLF.
Further, it expected an error message that was built assuming
cherry-pick was the old scripted version, because cherry-pick no longer
uses the error message that was encoded in this test.  So, although I
don't know what correct behavior for this test is, I know that this test
was not testing for it.

Since I have no idea which of the two cases above provides correct
behavior, let's just assume for now it's the case where we assume that
.gitattributes affects the merge and update it accordingly.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t6038-merge-text-auto.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t6038-merge-text-auto.sh b/t/t6038-merge-text-auto.sh
index 27cea15533..39413d5b48 100755
--- a/t/t6038-merge-text-auto.sh
+++ b/t/t6038-merge-text-auto.sh
@@ -189,7 +189,7 @@ test_expect_failure 'checkout -m addition of text=auto' '
 '
 
 test_expect_failure 'cherry-pick patch from after text=auto was added' '
-	append_cr <<-\EOF >expected &&
+	cat <<-\EOF >expected &&
 	first line
 	same line
 	EOF
@@ -197,9 +197,9 @@ test_expect_failure 'cherry-pick patch from after text=auto was added' '
 	git config merge.renormalize true &&
 	git rm -fr . &&
 	git reset --hard b &&
-	test_must_fail git cherry-pick a >err 2>&1 &&
-	grep "[Nn]othing added" err &&
-	compare_files expected file
+	git cherry-pick a &&
+	git cat-file -p HEAD:file >actual &&
+	compare_files expected actual
 '
 
 test_expect_success 'Test delete/normalize conflict' '
-- 
gitgitgadget

[PATCH 3/4] merge: make merge.renormalize work for all uses of merge machinery

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

The 'merge' command is not the only one that does merges; other commands
like checkout -m or rebase do as well.  Unfortunately, the only area of
the code that checked for the "merge.renormalize" config setting was in
builtin/merge.c, meaning it could only affect merges performed by the
"merge" command.  Move the handling of this config setting to
merge_recursive_config() so that other commands can benefit from it as
well.  Fixes a few tests in t6038.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/checkout.c         | 7 -------
 builtin/merge.c            | 7 +++----
 merge-recursive.c          | 3 +++
 t/t6038-merge-text-auto.sh | 4 ++--
 4 files changed, 8 insertions(+), 13 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index af849c644f..18c49034c4 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -771,13 +771,6 @@ static int merge_working_tree(const struct checkout_opts *opts,
 			 */
 
 			add_files_to_cache(NULL, NULL, 0);
-			/*
-			 * NEEDSWORK: carrying over local changes
-			 * when branches have different end-of-line
-			 * normalization (or clean+smudge rules) is
-			 * a pain; plumb in an option to set
-			 * o.renormalize?
-			 */
 			init_merge_options(&o, the_repository);
 			o.verbosity = 0;
 			work = write_in_core_index_as_tree(the_repository);
diff --git a/builtin/merge.c b/builtin/merge.c
index 7da707bf55..52f03ea715 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -72,7 +72,7 @@ static const char **xopts;
 static size_t xopts_nr, xopts_alloc;
 static const char *branch;
 static char *branch_mergeoptions;
-static int option_renormalize;
+static int option_renormalize = -1;
 static int verbosity;
 static int allow_rerere_auto;
 static int abort_current_merge;
@@ -621,8 +621,6 @@ static int git_merge_config(const char *k, const char *v, void *cb)
 		return git_config_string(&pull_octopus, k, v);
 	else if (!strcmp(k, "commit.cleanup"))
 		return git_config_string(&cleanup_arg, k, v);
-	else if (!strcmp(k, "merge.renormalize"))
-		option_renormalize = git_config_bool(k, v);
 	else if (!strcmp(k, "merge.ff")) {
 		int boolval = git_parse_maybe_bool(v);
 		if (0 <= boolval) {
@@ -721,7 +719,8 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
 		if (!strcmp(strategy, "subtree"))
 			o.subtree_shift = "";
 
-		o.renormalize = option_renormalize;
+		if (option_renormalize != -1)
+			o.renormalize = option_renormalize;
 		o.show_rename_progress =
 			show_progress == -1 ? isatty(2) : show_progress;
 
diff --git a/merge-recursive.c b/merge-recursive.c
index 36948eafb7..a1c8b36ddb 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -3791,9 +3791,12 @@ int merge_recursive_generic(struct merge_options *opt,
 static void merge_recursive_config(struct merge_options *opt)
 {
 	char *value = NULL;
+	int renormalize = 0;
 	git_config_get_int("merge.verbosity", &opt->verbosity);
 	git_config_get_int("diff.renamelimit", &opt->rename_limit);
 	git_config_get_int("merge.renamelimit", &opt->rename_limit);
+	git_config_get_bool("merge.renormalize", &renormalize);
+	opt->renormalize = renormalize;
 	if (!git_config_get_string("diff.renames", &value)) {
 		opt->detect_renames = git_config_rename("diff.renames", value);
 		free(value);
diff --git a/t/t6038-merge-text-auto.sh b/t/t6038-merge-text-auto.sh
index 39413d5b48..8cff0d45a1 100755
--- a/t/t6038-merge-text-auto.sh
+++ b/t/t6038-merge-text-auto.sh
@@ -158,7 +158,7 @@ test_expect_success 'Detect LF/CRLF conflict from addition of text=auto' '
 	compare_files expected file.fuzzy
 '
 
-test_expect_failure 'checkout -m after setting text=auto' '
+test_expect_success 'checkout -m after setting text=auto' '
 	cat <<-\EOF >expected &&
 	first line
 	same line
@@ -173,7 +173,7 @@ test_expect_failure 'checkout -m after setting text=auto' '
 	git diff --no-index --ignore-cr-at-eol expected file
 '
 
-test_expect_failure 'checkout -m addition of text=auto' '
+test_expect_success 'checkout -m addition of text=auto' '
 	cat <<-\EOF >expected &&
 	first line
 	same line
-- 
gitgitgadget

[PATCH 4/4] checkout: support renormalization with checkout -m <paths>

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/checkout.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 18c49034c4..2837195491 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -239,6 +239,8 @@ static int checkout_merged(int pos, const struct checkout *state, int *nr_checko
 	mmbuffer_t result_buf;
 	struct object_id threeway[3];
 	unsigned mode = 0;
+	struct ll_merge_options ll_opts;
+	int renormalize = 0;
 
 	memset(threeway, 0, sizeof(threeway));
 	while (pos < active_nr) {
@@ -259,13 +261,12 @@ static int checkout_merged(int pos, const struct checkout *state, int *nr_checko
 	read_mmblob(&ours, &threeway[1]);
 	read_mmblob(&theirs, &threeway[2]);
 
-	/*
-	 * NEEDSWORK: re-create conflicts from merges with
-	 * merge.renormalize set, too
-	 */
+	memset(&ll_opts, 0, sizeof(ll_opts));
+	git_config_get_bool("merge.renormalize", &renormalize);
+	ll_opts.renormalize = renormalize;
 	status = ll_merge(&result_buf, path, &ancestor, "base",
 			  &ours, "ours", &theirs, "theirs",
-			  state->istate, NULL);
+			  state->istate, &ll_opts);
 	free(ancestor.ptr);
 	free(ours.ptr);
 	free(theirs.ptr);
-- 
gitgitgadget

Re: [PATCH 1/4] t6038: make tests fail for the right reason

[Junio C Hamano]

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> t6038 had a pair of tests that were expected to fail, but weren't
> failing for the expected reason.  Both were meant to do a merge that
> could be done cleanly after renormalization, but were supposed to fail
> for lack of renormalization.  Unfortunately, both tests has staged
> changes, and checkout -m would abort due to the presence of those staged
> changes before even attempting a merge.
>
> Fix this first issue by utilizing git-restore instead of git-checkout,
> so that the index is left alone and just the working directory gets the
> changes we want.

Nicely analysed.

> However, there is a second issue with these tests.  Technically, they
> just wanted to verify that after renormalization, no conflicts would be
> present.  This could have been checked for by grepping for a lack of
> conflict markers, but the test instead tried to compare the working
> directory files to an expected result.  Unfortunately, the setting of
> "text=auto" without setting core.eol to any value meant that the content
> of the file (in particular, the line endings) would be
> platform-dependent and the tests could only pass on some platforms.

OK.

> Replace the existing comparison with a call to 'git diff --no-index
> --ignore-cr-at-eol' to verify that the contents, other than possible
> carriage returns in the file, match the expected results and in
> particular that the file has no conflicts from the checkout -m
> operation.

Makes sense.

Thanks.

> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  t/t6038-merge-text-auto.sh | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/t/t6038-merge-text-auto.sh b/t/t6038-merge-text-auto.sh
> index 5e8d5fa50c..27cea15533 100755
> --- a/t/t6038-merge-text-auto.sh
> +++ b/t/t6038-merge-text-auto.sh
> @@ -168,9 +168,9 @@ test_expect_failure 'checkout -m after setting text=auto' '
>  	git rm -fr . &&
>  	rm -f .gitattributes &&
>  	git reset --hard initial &&
> -	git checkout a -- . &&
> +	git restore --source=a -- . &&
>  	git checkout -m b &&
> -	compare_files expected file
> +	git diff --no-index --ignore-cr-at-eol expected file
>  '
>  
>  test_expect_failure 'checkout -m addition of text=auto' '
> @@ -183,9 +183,9 @@ test_expect_failure 'checkout -m addition of text=auto' '
>  	git rm -fr . &&
>  	rm -f .gitattributes file &&
>  	git reset --hard initial &&
> -	git checkout b -- . &&
> +	git restore --source=b -- . &&
>  	git checkout -m a &&
> -	compare_files expected file
> +	git diff --no-index --ignore-cr-at-eol expected file
>  '
>  
>  test_expect_failure 'cherry-pick patch from after text=auto was added' '

Re: [PATCH 1/4] t6038: make tests fail for the right reason

[Eric Sunshine]

On Sun, Aug 2, 2020 at 2:33 AM Elijah Newren via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> t6038 had a pair of tests that were expected to fail, but weren't
> failing for the expected reason.  Both were meant to do a merge that
> could be done cleanly after renormalization, but were supposed to fail
> for lack of renormalization.  Unfortunately, both tests has staged

s/has/had/
...or...
s/has/have/

> changes, and checkout -m would abort due to the presence of those staged
> changes before even attempting a merge.
>
> Fix this first issue by utilizing git-restore instead of git-checkout,
> so that the index is left alone and just the working directory gets the
> changes we want.
>
> However, there is a second issue with these tests.  Technically, they
> just wanted to verify that after renormalization, no conflicts would be
> present.  This could have been checked for by grepping for a lack of
> conflict markers, but the test instead tried to compare the working
> directory files to an expected result.  Unfortunately, the setting of
> "text=auto" without setting core.eol to any value meant that the content
> of the file (in particular, the line endings) would be
> platform-dependent and the tests could only pass on some platforms.
> Replace the existing comparison with a call to 'git diff --no-index
> --ignore-cr-at-eol' to verify that the contents, other than possible
> carriage returns in the file, match the expected results and in
> particular that the file has no conflicts from the checkout -m
> operation.
>
> Signed-off-by: Elijah Newren <newren@gmail.com>

Re: [PATCH 3/4] merge: make merge.renormalize work for all uses of merge machinery

[Junio C Hamano]

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Elijah Newren <newren@gmail.com>
>
> The 'merge' command is not the only one that does merges; other commands
> like checkout -m or rebase do as well.  Unfortunately, the only area of
> the code that checked for the "merge.renormalize" config setting was in
> builtin/merge.c, meaning it could only affect merges performed by the
> "merge" command.  Move the handling of this config setting to
> merge_recursive_config() so that other commands can benefit from it as
> well.  Fixes a few tests in t6038.

Makes sense, but.

> @@ -771,13 +771,6 @@ static int merge_working_tree(const struct checkout_opts *opts,
>  			 */
>  
>  			add_files_to_cache(NULL, NULL, 0);
> -			/*
> -			 * NEEDSWORK: carrying over local changes
> -			 * when branches have different end-of-line
> -			 * normalization (or clean+smudge rules) is
> -			 * a pain; plumb in an option to set
> -			 * o.renormalize?
> -			 */
>  			init_merge_options(&o, the_repository);

Now init_merge_options() takes care of setting o.renormalize, which
does exactly what the comment wanted to see happen.

> diff --git a/builtin/merge.c b/builtin/merge.c
> index 7da707bf55..52f03ea715 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -72,7 +72,7 @@ static const char **xopts;
>  static size_t xopts_nr, xopts_alloc;
>  static const char *branch;
>  static char *branch_mergeoptions;
> -static int option_renormalize;
> +static int option_renormalize = -1;

Do we still need this file-scope static variable at all?

> @@ -621,8 +621,6 @@ static int git_merge_config(const char *k, const char *v, void *cb)
>  		return git_config_string(&pull_octopus, k, v);
>  	else if (!strcmp(k, "commit.cleanup"))
>  		return git_config_string(&cleanup_arg, k, v);
> -	else if (!strcmp(k, "merge.renormalize"))
> -		option_renormalize = git_config_bool(k, v);

We no longer set value to option_renormalize in git_merge_config()
that is used only from cmd_merge().

> @@ -721,7 +719,8 @@ static int try_merge_strategy(const char *stra
>  		if (!strcmp(strategy, "subtree"))
>  			o.subtree_shift = "";
>  
> -		o.renormalize = option_renormalize;
> +		if (option_renormalize != -1)
> +			o.renormalize = option_renormalize;

And if somebody sets option_renormalize, then (and only then) we
override o.renormalize.  One line before the precontext of this hunk
is a call to init_merge_options() that would have set o.renormalize
by reading merge.renormalize configuration.  So there must be another
place where option_renormalize comes from other than that configuration
variable.

But I do not see the variable mentioned anywhere else in the
original   The assignment to it in git_merge_config() you just got
rid of was the only one that used to assign to it.

> diff --git a/merge-recursive.c b/merge-recursive.c
> index 36948eafb7..a1c8b36ddb 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -3791,9 +3791,12 @@ int merge_recursive_generic(struct merge_options *opt,
>  static void merge_recursive_config(struct merge_options *opt)
>  {
>  	char *value = NULL;
> +	int renormalize = 0;
>  	git_config_get_int("merge.verbosity", &opt->verbosity);
>  	git_config_get_int("diff.renamelimit", &opt->rename_limit);
>  	git_config_get_int("merge.renamelimit", &opt->rename_limit);
> +	git_config_get_bool("merge.renormalize", &renormalize);
> +	opt->renormalize = renormalize;
>  	if (!git_config_get_string("diff.renames", &value)) {
>  		opt->detect_renames = git_config_rename("diff.renames", value);
>  		free(value);

OK.

IOW, wouldn't the attached be a no-op code clean-up on top?

 builtin/merge.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 52f03ea715..74829a838e 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -72,7 +72,6 @@ static const char **xopts;
 static size_t xopts_nr, xopts_alloc;
 static const char *branch;
 static char *branch_mergeoptions;
-static int option_renormalize = -1;
 static int verbosity;
 static int allow_rerere_auto;
 static int abort_current_merge;
@@ -719,8 +718,6 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
 		if (!strcmp(strategy, "subtree"))
 			o.subtree_shift = "";
 
-		if (option_renormalize != -1)
-			o.renormalize = option_renormalize;
 		o.show_rename_progress =
 			show_progress == -1 ? isatty(2) : show_progress;
 

Re: [PATCH 2/4] t6038: fix test with obviously incorrect expectations

[Junio C Hamano]

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Elijah Newren <newren@gmail.com>
>
> t6038.11, 'cherry-pick patch from after text=auto' was set up so that on
> a branch with no .gitattributes file, you cherry-picked a patch from a
> branch that had a .gitattributes file (containing '* text=auto').
> Further, the two branches had a file which differed only in line
> endings.  In this situation, correct behavior is not well defined:
> should the .gitattributes file affect the merge or not?

I'd say that it is probably more intuitive to expect whatever
attributes set on the currently checked out and receiving the
cherry-picked change would take effect, but I do agree with you that
this is not well defined.  I think "changing attributes in the
middle may produce unexpected/undefined results---validate it when
you cross such a boundary" is a prudent advice we should give to the
users.

Would it make more sense not to test ill defined behaviour at all
instead, I wonder?

Thanks.

Re: [PATCH 1/4] t6038: make tests fail for the right reason

[Elijah Newren]

On Sun, Aug 2, 2020 at 12:11 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Sun, Aug 2, 2020 at 2:33 AM Elijah Newren via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > t6038 had a pair of tests that were expected to fail, but weren't
> > failing for the expected reason.  Both were meant to do a merge that
> > could be done cleanly after renormalization, but were supposed to fail
> > for lack of renormalization.  Unfortunately, both tests has staged
>
> s/has/had/
> ...or...
> s/has/have/

Thanks, will fix in next reroll.

Re: [PATCH 3/4] merge: make merge.renormalize work for all uses of merge machinery

[Elijah Newren]

On Sun, Aug 2, 2020 at 12:23 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Elijah Newren <newren@gmail.com>
> >
> > The 'merge' command is not the only one that does merges; other commands
> > like checkout -m or rebase do as well.  Unfortunately, the only area of
> > the code that checked for the "merge.renormalize" config setting was in
> > builtin/merge.c, meaning it could only affect merges performed by the
> > "merge" command.  Move the handling of this config setting to
> > merge_recursive_config() so that other commands can benefit from it as
> > well.  Fixes a few tests in t6038.
>
> Makes sense, but.
>
> > @@ -771,13 +771,6 @@ static int merge_working_tree(const struct checkout_opts *opts,
> >                        */
> >
> >                       add_files_to_cache(NULL, NULL, 0);
> > -                     /*
> > -                      * NEEDSWORK: carrying over local changes
> > -                      * when branches have different end-of-line
> > -                      * normalization (or clean+smudge rules) is
> > -                      * a pain; plumb in an option to set
> > -                      * o.renormalize?
> > -                      */
> >                       init_merge_options(&o, the_repository);
>
> Now init_merge_options() takes care of setting o.renormalize, which
> does exactly what the comment wanted to see happen.
>
> > diff --git a/builtin/merge.c b/builtin/merge.c
> > index 7da707bf55..52f03ea715 100644
> > --- a/builtin/merge.c
> > +++ b/builtin/merge.c
> > @@ -72,7 +72,7 @@ static const char **xopts;
> >  static size_t xopts_nr, xopts_alloc;
> >  static const char *branch;
> >  static char *branch_mergeoptions;
> > -static int option_renormalize;
> > +static int option_renormalize = -1;
>
> Do we still need this file-scope static variable at all?
>
> > @@ -621,8 +621,6 @@ static int git_merge_config(const char *k, const char *v, void *cb)
> >               return git_config_string(&pull_octopus, k, v);
> >       else if (!strcmp(k, "commit.cleanup"))
> >               return git_config_string(&cleanup_arg, k, v);
> > -     else if (!strcmp(k, "merge.renormalize"))
> > -             option_renormalize = git_config_bool(k, v);
>
> We no longer set value to option_renormalize in git_merge_config()
> that is used only from cmd_merge().
>
> > @@ -721,7 +719,8 @@ static int try_merge_strategy(const char *stra
> >               if (!strcmp(strategy, "subtree"))
> >                       o.subtree_shift = "";
> >
> > -             o.renormalize = option_renormalize;
> > +             if (option_renormalize != -1)
> > +                     o.renormalize = option_renormalize;
>
> And if somebody sets option_renormalize, then (and only then) we
> override o.renormalize.  One line before the precontext of this hunk
> is a call to init_merge_options() that would have set o.renormalize
> by reading merge.renormalize configuration.  So there must be another
> place where option_renormalize comes from other than that configuration
> variable.
>
> But I do not see the variable mentioned anywhere else in the
> original   The assignment to it in git_merge_config() you just got
> rid of was the only one that used to assign to it.
>
> > diff --git a/merge-recursive.c b/merge-recursive.c
> > index 36948eafb7..a1c8b36ddb 100644
> > --- a/merge-recursive.c
> > +++ b/merge-recursive.c
> > @@ -3791,9 +3791,12 @@ int merge_recursive_generic(struct merge_options *opt,
> >  static void merge_recursive_config(struct merge_options *opt)
> >  {
> >       char *value = NULL;
> > +     int renormalize = 0;
> >       git_config_get_int("merge.verbosity", &opt->verbosity);
> >       git_config_get_int("diff.renamelimit", &opt->rename_limit);
> >       git_config_get_int("merge.renamelimit", &opt->rename_limit);
> > +     git_config_get_bool("merge.renormalize", &renormalize);
> > +     opt->renormalize = renormalize;
> >       if (!git_config_get_string("diff.renames", &value)) {
> >               opt->detect_renames = git_config_rename("diff.renames", value);
> >               free(value);
>
> OK.
>
> IOW, wouldn't the attached be a no-op code clean-up on top?
>
>  builtin/merge.c | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 52f03ea715..74829a838e 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -72,7 +72,6 @@ static const char **xopts;
>  static size_t xopts_nr, xopts_alloc;
>  static const char *branch;
>  static char *branch_mergeoptions;
> -static int option_renormalize = -1;
>  static int verbosity;
>  static int allow_rerere_auto;
>  static int abort_current_merge;
> @@ -719,8 +718,6 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
>                 if (!strcmp(strategy, "subtree"))
>                         o.subtree_shift = "";
>
> -               if (option_renormalize != -1)
> -                       o.renormalize = option_renormalize;
>                 o.show_rename_progress =
>                         show_progress == -1 ? isatty(2) : show_progress;
>

Indeed; I'll squash that in for my next iteration.

Re: [PATCH 2/4] t6038: fix test with obviously incorrect expectations

[Elijah Newren]

On Sun, Aug 2, 2020 at 12:57 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Elijah Newren <newren@gmail.com>
> >
> > t6038.11, 'cherry-pick patch from after text=auto' was set up so that on
> > a branch with no .gitattributes file, you cherry-picked a patch from a
> > branch that had a .gitattributes file (containing '* text=auto').
> > Further, the two branches had a file which differed only in line
> > endings.  In this situation, correct behavior is not well defined:
> > should the .gitattributes file affect the merge or not?
>
> I'd say that it is probably more intuitive to expect whatever
> attributes set on the currently checked out and receiving the
> cherry-picked change would take effect, but I do agree with you that
> this is not well defined.

Turns out this question is kind of important since merge-ort does not
naturally get to take advantage of the existing infrastructure for
attribute handling; I have to implement some stuff to trick it into
using it, and the stuff I implement makes it glaringly obvious what
choice is being made.  Your answer here doesn't really help me since
it's not even applicable in some cases.  Anyway, I'll list a bunch of
questions I'm facing with it, but if you want you can skip to the next
quoted block of text and ignore this topic until I post the relevant
patches for merge-ort, if you want.

My questions about attribute handling in merges and which version(s)
of .gitattributes should be used for three-way content merges:

What if you don't have any version checked out, and are doing a merge
in a bare-repo or are just redoing a merge (on some other branch)
without touching the working directory or index just so you can view
that other merge?  In that case, your answer doesn't even apply and I
need to implement something else.

Also, what if you were doing a merge in a dirty working tree, where
your .gitattributes was locally modified?  Should the local
.gitattributes file override the .gitattributes file recorded in
history for how those versions are merged (which seems somewhat
suggested by your answer)?  Even if it makes the merge not
reproducible by others?

Are we okay with merging one way resulting in no conflicts while
merges the other way (with the order of parents reversed) yielding
conflicts due to use of different .gitattributes files?  Also, there
can be differences in what the user sees in a single merge while
resolving, due to 'git checkout -m $CONFLICTED_PATH'.  This happens in
a few cases...explored in the next two paragraphs:

What if the first parent had a .gitattributes file and the merge base
did too (with same contents), but the second parent didn't?  Do we use
the .gitattributes from before the merge, despite the fact that the
merge is going to delete the .gitattributes?  If there are any
conflicts and the users messes up a single file and needs to redo it,
then a 'git checkout -m $CONFLICTED_PATH' later might give them a
different result.

Assuming there is no .gitattributes file in the first parent and none
locally before merging, but the second parent did have a
.gitattributes file, if we only pay attention to the .gitattributes
file from the local working directory or the first parent, then we
again run into a case where the merge may produce a different result
than a subsequent 'git checkout -m $CONFLICTED_PATH'.

What if the .gitattributes file itself needed to be merged?  If it
merges cleanly, should we use that clean merged version of
.gitattributes to define the settings for all three-way content merges
in the remainder of the merge?  If it doesn't merge cleanly...should
we just pick the one from the first parent?  From the second?  Throw a
merge conflict stating that .gitattributes can't be merged and thus we
don't know how to do content merging on any other conflicted path?
(And what if the .gitattributes file only cleanly merges depending on
if it's merged with one of the two .gitattributes settings from one of
its parents?)

> I think "changing attributes in the
> middle may produce unexpected/undefined results---validate it when
> you cross such a boundary" is a prudent advice we should give to the
> users.

Makes sense; especially given all the cases above.

> Would it make more sense not to test ill defined behaviour at all
> instead, I wonder?

I'd be happy to toss the test and punt this conversation until I post
the relevant patches for merge-ort, but we might not be kicking the
can all that far down the road...

Re: [PATCH 2/4] t6038: fix test with obviously incorrect expectations

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

>> I'd say that it is probably more intuitive to expect whatever
>> attributes set on the currently checked out and receiving the
>> cherry-picked change would take effect, but I do agree with you that
>> this is not well defined.
> ...
> What if you don't have any version checked out, and are doing a merge
> in a bare-repo or are just redoing a merge (on some other branch)
> without touching the working directory or index just so you can view
> that other merge?

The "receiving" is the keyword in my "I'd say".  Whey you view the
result of merge, the merge may be symmetric, but when initiating a
merge, you have a clear distinction between the target and other: I
am merging these other side branches into this one.

But as I said, I think it is not well defined whose attribute should
be used.  Some might even dream that .gitattributes from the tips
being merged are somehow magically merged first and then the other
paths' merges should use that resulting merged .gitattributes.

> Also, what if you were doing a merge in a dirty working tree, where
> your .gitattributes was locally modified?  Should the local
> .gitattributes file override the .gitattributes file recorded in
> history for how those versions are merged (which seems somewhat
> suggested by your answer)?

Yes, that is quite deliberate outcome from "when doing a merge, the
person who is merging is fully aware into which branch others are
merged into".

[PATCH v2 0/4] Attr fixes

[Elijah Newren via GitGitGadget]

This fixes a few issues surrounding .gitattributes files and usage of the
merge machinery outside of "git merge". All were issues I found and fixed
while working on merge-ort.

Changes since v1:

 * Made the fixes suggested by Eric and Junio
 * Just ripped out the test in patch 2 that was testing undefined behavior
   (especially since it was a test_expect_failure, and clearly was testing
   multiple things wrong), as suggested by Junio.

Elijah Newren (4):
  t6038: make tests fail for the right reason
  t6038: remove problematic test
  merge: make merge.renormalize work for all uses of merge machinery
  checkout: support renormalization with checkout -m <paths>

 builtin/checkout.c         | 18 ++++++------------
 builtin/merge.c            |  4 ----
 merge-recursive.c          |  3 +++
 t/t6038-merge-text-auto.sh | 26 ++++++--------------------
 4 files changed, 15 insertions(+), 36 deletions(-)


base-commit: 47ae905ffb98cc4d4fd90083da6bc8dab55d9ecc
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-825%2Fnewren%2Fattr-fixes-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-825/newren/attr-fixes-v2
Pull-Request: https://github.com/git/git/pull/825

Range-diff vs v1:

 1:  3619118175 ! 1:  21033c4c14 t6038: make tests fail for the right reason
     @@ Commit message
          t6038 had a pair of tests that were expected to fail, but weren't
          failing for the expected reason.  Both were meant to do a merge that
          could be done cleanly after renormalization, but were supposed to fail
     -    for lack of renormalization.  Unfortunately, both tests has staged
     +    for lack of renormalization.  Unfortunately, both tests had staged
          changes, and checkout -m would abort due to the presence of those staged
          changes before even attempting a merge.
      
 2:  83a50f7e0b ! 2:  305fe534c5 t6038: fix test with obviously incorrect expectations
     @@ Metadata
      Author: Elijah Newren <newren@gmail.com>
      
       ## Commit message ##
     -    t6038: fix test with obviously incorrect expectations
     +    t6038: remove problematic test
      
     -    t6038.11, 'cherry-pick patch from after text=auto' was set up so that on
     -    a branch with no .gitattributes file, you cherry-picked a patch from a
     -    branch that had a .gitattributes file (containing '* text=auto').
     -    Further, the two branches had a file which differed only in line
     -    endings.  In this situation, correct behavior is not well defined:
     -    should the .gitattributes file affect the merge or not?
     +    t6038.11, 'cherry-pick patch from after text=auto' was a test of
     +    undefined behavior.  To make matters worse, while there are a couple
     +    possible correct answers, this test was coded to only check for an
     +    obviously incorrect answer.  And the final cherry on top is that the
     +    test is marked test_expect_failure, meaning it can't provide much value,
     +    other than possibly confusing future folks who come along and try to
     +    work on attributes and look at existing tests.  Because of all these
     +    problems, just remove the test.
     +
     +    But for any future code spelunkers, here's my understanding of the two
     +    possible correct answers:
     +
     +    This test was set up so that on a branch with no .gitattributes file,
     +    you cherry-picked a patch from a branch that had a .gitattributes file
     +    (containing '* text=auto').  Further, the two branches had a file which
     +    differed only in line endings.  In this situation, correct behavior is
     +    not well defined: should the .gitattributes file affect the merge or
     +    not?
      
          If the .gitattributes file on the other branch should not affect the
          merge, then we would have a content conflict with all three stages
          different (the merge base didn't match either side).
      
          If the .gitattributes file from the other branch should affect the
     -    merge, then we would expect the line endings to be normalized to LF so
     -    that the versions from both sides match, and then the cherry-pick has no
     -    conflicts and can succeed.  After the cherry-pick, the line endings in
     -    the file will change from CRLF to LF.
     -
     -    This test had an expectation that the line endings would remain CRLF.
     -    Further, it expected an error message that was built assuming
     -    cherry-pick was the old scripted version, because cherry-pick no longer
     -    uses the error message that was encoded in this test.  So, although I
     -    don't know what correct behavior for this test is, I know that this test
     -    was not testing for it.
     -
     -    Since I have no idea which of the two cases above provides correct
     -    behavior, let's just assume for now it's the case where we assume that
     -    .gitattributes affects the merge and update it accordingly.
     +    merge, then we would expect the line endings to be normalized to LF for
     +    the version to be recorded in the repository.  This would mean that when
     +    doing a three-way content merge on the file that differed in line
     +    endings, that the three-way content merge would see that the versions on
     +    both sides matched and so the cherry-pick has no conflicts and can
     +    succeed.  The line endings in the file as recorded in the repository
     +    will change from CRLF to LF.  The version checked out in the working
     +    copy will depend on the platform (since there's no eol attribute defined
     +    for the file).
     +
     +    Also, as a final side note, this test expected an error message that was
     +    built assuming cherry-pick was the old scripted version, because
     +    cherry-pick no longer uses the error message that was encoded in this
     +    test.  So it was wrong for yet another reason.
     +
     +    Given that the handling of .gitattributes is not well defined and this
     +    test was obviously broken and could do nothing but confuse future
     +    readers, just remove it.
      
          Signed-off-by: Elijah Newren <newren@gmail.com>
      
       ## t/t6038-merge-text-auto.sh ##
      @@ t/t6038-merge-text-auto.sh: test_expect_failure 'checkout -m addition of text=auto' '
     + 	git diff --no-index --ignore-cr-at-eol expected file
       '
       
     - test_expect_failure 'cherry-pick patch from after text=auto was added' '
     +-test_expect_failure 'cherry-pick patch from after text=auto was added' '
      -	append_cr <<-\EOF >expected &&
     -+	cat <<-\EOF >expected &&
     - 	first line
     - 	same line
     - 	EOF
     -@@ t/t6038-merge-text-auto.sh: test_expect_failure 'cherry-pick patch from after text=auto was added' '
     - 	git config merge.renormalize true &&
     - 	git rm -fr . &&
     - 	git reset --hard b &&
     +-	first line
     +-	same line
     +-	EOF
     +-
     +-	git config merge.renormalize true &&
     +-	git rm -fr . &&
     +-	git reset --hard b &&
      -	test_must_fail git cherry-pick a >err 2>&1 &&
      -	grep "[Nn]othing added" err &&
      -	compare_files expected file
     -+	git cherry-pick a &&
     -+	git cat-file -p HEAD:file >actual &&
     -+	compare_files expected actual
     - '
     - 
     +-'
     +-
       test_expect_success 'Test delete/normalize conflict' '
     + 	git checkout -f side &&
     + 	git rm -fr . &&
 3:  08c8080b31 ! 3:  379a87ea82 merge: make merge.renormalize work for all uses of merge machinery
     @@ builtin/merge.c: static const char **xopts;
       static const char *branch;
       static char *branch_mergeoptions;
      -static int option_renormalize;
     -+static int option_renormalize = -1;
       static int verbosity;
       static int allow_rerere_auto;
       static int abort_current_merge;
     @@ builtin/merge.c: static int try_merge_strategy(const char *strategy, struct comm
       			o.subtree_shift = "";
       
      -		o.renormalize = option_renormalize;
     -+		if (option_renormalize != -1)
     -+			o.renormalize = option_renormalize;
       		o.show_rename_progress =
       			show_progress == -1 ? isatty(2) : show_progress;
       
 4:  fcc7ea3add = 4:  36e08a75a3 checkout: support renormalization with checkout -m <paths>

-- 
gitgitgadget

[PATCH v2 1/4] t6038: make tests fail for the right reason

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

t6038 had a pair of tests that were expected to fail, but weren't
failing for the expected reason.  Both were meant to do a merge that
could be done cleanly after renormalization, but were supposed to fail
for lack of renormalization.  Unfortunately, both tests had staged
changes, and checkout -m would abort due to the presence of those staged
changes before even attempting a merge.

Fix this first issue by utilizing git-restore instead of git-checkout,
so that the index is left alone and just the working directory gets the
changes we want.

However, there is a second issue with these tests.  Technically, they
just wanted to verify that after renormalization, no conflicts would be
present.  This could have been checked for by grepping for a lack of
conflict markers, but the test instead tried to compare the working
directory files to an expected result.  Unfortunately, the setting of
"text=auto" without setting core.eol to any value meant that the content
of the file (in particular, the line endings) would be
platform-dependent and the tests could only pass on some platforms.
Replace the existing comparison with a call to 'git diff --no-index
--ignore-cr-at-eol' to verify that the contents, other than possible
carriage returns in the file, match the expected results and in
particular that the file has no conflicts from the checkout -m
operation.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t6038-merge-text-auto.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t6038-merge-text-auto.sh b/t/t6038-merge-text-auto.sh
index 5e8d5fa50c..27cea15533 100755
--- a/t/t6038-merge-text-auto.sh
+++ b/t/t6038-merge-text-auto.sh
@@ -168,9 +168,9 @@ test_expect_failure 'checkout -m after setting text=auto' '
 	git rm -fr . &&
 	rm -f .gitattributes &&
 	git reset --hard initial &&
-	git checkout a -- . &&
+	git restore --source=a -- . &&
 	git checkout -m b &&
-	compare_files expected file
+	git diff --no-index --ignore-cr-at-eol expected file
 '
 
 test_expect_failure 'checkout -m addition of text=auto' '
@@ -183,9 +183,9 @@ test_expect_failure 'checkout -m addition of text=auto' '
 	git rm -fr . &&
 	rm -f .gitattributes file &&
 	git reset --hard initial &&
-	git checkout b -- . &&
+	git restore --source=b -- . &&
 	git checkout -m a &&
-	compare_files expected file
+	git diff --no-index --ignore-cr-at-eol expected file
 '
 
 test_expect_failure 'cherry-pick patch from after text=auto was added' '
-- 
gitgitgadget

[PATCH v2 2/4] t6038: remove problematic test

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

t6038.11, 'cherry-pick patch from after text=auto' was a test of
undefined behavior.  To make matters worse, while there are a couple
possible correct answers, this test was coded to only check for an
obviously incorrect answer.  And the final cherry on top is that the
test is marked test_expect_failure, meaning it can't provide much value,
other than possibly confusing future folks who come along and try to
work on attributes and look at existing tests.  Because of all these
problems, just remove the test.

But for any future code spelunkers, here's my understanding of the two
possible correct answers:

This test was set up so that on a branch with no .gitattributes file,
you cherry-picked a patch from a branch that had a .gitattributes file
(containing '* text=auto').  Further, the two branches had a file which
differed only in line endings.  In this situation, correct behavior is
not well defined: should the .gitattributes file affect the merge or
not?

If the .gitattributes file on the other branch should not affect the
merge, then we would have a content conflict with all three stages
different (the merge base didn't match either side).

If the .gitattributes file from the other branch should affect the
merge, then we would expect the line endings to be normalized to LF for
the version to be recorded in the repository.  This would mean that when
doing a three-way content merge on the file that differed in line
endings, that the three-way content merge would see that the versions on
both sides matched and so the cherry-pick has no conflicts and can
succeed.  The line endings in the file as recorded in the repository
will change from CRLF to LF.  The version checked out in the working
copy will depend on the platform (since there's no eol attribute defined
for the file).

Also, as a final side note, this test expected an error message that was
built assuming cherry-pick was the old scripted version, because
cherry-pick no longer uses the error message that was encoded in this
test.  So it was wrong for yet another reason.

Given that the handling of .gitattributes is not well defined and this
test was obviously broken and could do nothing but confuse future
readers, just remove it.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t6038-merge-text-auto.sh | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/t/t6038-merge-text-auto.sh b/t/t6038-merge-text-auto.sh
index 27cea15533..9337745793 100755
--- a/t/t6038-merge-text-auto.sh
+++ b/t/t6038-merge-text-auto.sh
@@ -188,20 +188,6 @@ test_expect_failure 'checkout -m addition of text=auto' '
 	git diff --no-index --ignore-cr-at-eol expected file
 '
 
-test_expect_failure 'cherry-pick patch from after text=auto was added' '
-	append_cr <<-\EOF >expected &&
-	first line
-	same line
-	EOF
-
-	git config merge.renormalize true &&
-	git rm -fr . &&
-	git reset --hard b &&
-	test_must_fail git cherry-pick a >err 2>&1 &&
-	grep "[Nn]othing added" err &&
-	compare_files expected file
-'
-
 test_expect_success 'Test delete/normalize conflict' '
 	git checkout -f side &&
 	git rm -fr . &&
-- 
gitgitgadget

[PATCH v2 3/4] merge: make merge.renormalize work for all uses of merge machinery

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

The 'merge' command is not the only one that does merges; other commands
like checkout -m or rebase do as well.  Unfortunately, the only area of
the code that checked for the "merge.renormalize" config setting was in
builtin/merge.c, meaning it could only affect merges performed by the
"merge" command.  Move the handling of this config setting to
merge_recursive_config() so that other commands can benefit from it as
well.  Fixes a few tests in t6038.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/checkout.c         | 7 -------
 builtin/merge.c            | 4 ----
 merge-recursive.c          | 3 +++
 t/t6038-merge-text-auto.sh | 4 ++--
 4 files changed, 5 insertions(+), 13 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index af849c644f..18c49034c4 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -771,13 +771,6 @@ static int merge_working_tree(const struct checkout_opts *opts,
 			 */
 
 			add_files_to_cache(NULL, NULL, 0);
-			/*
-			 * NEEDSWORK: carrying over local changes
-			 * when branches have different end-of-line
-			 * normalization (or clean+smudge rules) is
-			 * a pain; plumb in an option to set
-			 * o.renormalize?
-			 */
 			init_merge_options(&o, the_repository);
 			o.verbosity = 0;
 			work = write_in_core_index_as_tree(the_repository);
diff --git a/builtin/merge.c b/builtin/merge.c
index 7da707bf55..74829a838e 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -72,7 +72,6 @@ static const char **xopts;
 static size_t xopts_nr, xopts_alloc;
 static const char *branch;
 static char *branch_mergeoptions;
-static int option_renormalize;
 static int verbosity;
 static int allow_rerere_auto;
 static int abort_current_merge;
@@ -621,8 +620,6 @@ static int git_merge_config(const char *k, const char *v, void *cb)
 		return git_config_string(&pull_octopus, k, v);
 	else if (!strcmp(k, "commit.cleanup"))
 		return git_config_string(&cleanup_arg, k, v);
-	else if (!strcmp(k, "merge.renormalize"))
-		option_renormalize = git_config_bool(k, v);
 	else if (!strcmp(k, "merge.ff")) {
 		int boolval = git_parse_maybe_bool(v);
 		if (0 <= boolval) {
@@ -721,7 +718,6 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
 		if (!strcmp(strategy, "subtree"))
 			o.subtree_shift = "";
 
-		o.renormalize = option_renormalize;
 		o.show_rename_progress =
 			show_progress == -1 ? isatty(2) : show_progress;
 
diff --git a/merge-recursive.c b/merge-recursive.c
index 36948eafb7..a1c8b36ddb 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -3791,9 +3791,12 @@ int merge_recursive_generic(struct merge_options *opt,
 static void merge_recursive_config(struct merge_options *opt)
 {
 	char *value = NULL;
+	int renormalize = 0;
 	git_config_get_int("merge.verbosity", &opt->verbosity);
 	git_config_get_int("diff.renamelimit", &opt->rename_limit);
 	git_config_get_int("merge.renamelimit", &opt->rename_limit);
+	git_config_get_bool("merge.renormalize", &renormalize);
+	opt->renormalize = renormalize;
 	if (!git_config_get_string("diff.renames", &value)) {
 		opt->detect_renames = git_config_rename("diff.renames", value);
 		free(value);
diff --git a/t/t6038-merge-text-auto.sh b/t/t6038-merge-text-auto.sh
index 9337745793..89c86d4e56 100755
--- a/t/t6038-merge-text-auto.sh
+++ b/t/t6038-merge-text-auto.sh
@@ -158,7 +158,7 @@ test_expect_success 'Detect LF/CRLF conflict from addition of text=auto' '
 	compare_files expected file.fuzzy
 '
 
-test_expect_failure 'checkout -m after setting text=auto' '
+test_expect_success 'checkout -m after setting text=auto' '
 	cat <<-\EOF >expected &&
 	first line
 	same line
@@ -173,7 +173,7 @@ test_expect_failure 'checkout -m after setting text=auto' '
 	git diff --no-index --ignore-cr-at-eol expected file
 '
 
-test_expect_failure 'checkout -m addition of text=auto' '
+test_expect_success 'checkout -m addition of text=auto' '
 	cat <<-\EOF >expected &&
 	first line
 	same line
-- 
gitgitgadget

[PATCH v2 4/4] checkout: support renormalization with checkout -m <paths>

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/checkout.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 18c49034c4..2837195491 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -239,6 +239,8 @@ static int checkout_merged(int pos, const struct checkout *state, int *nr_checko
 	mmbuffer_t result_buf;
 	struct object_id threeway[3];
 	unsigned mode = 0;
+	struct ll_merge_options ll_opts;
+	int renormalize = 0;
 
 	memset(threeway, 0, sizeof(threeway));
 	while (pos < active_nr) {
@@ -259,13 +261,12 @@ static int checkout_merged(int pos, const struct checkout *state, int *nr_checko
 	read_mmblob(&ours, &threeway[1]);
 	read_mmblob(&theirs, &threeway[2]);
 
-	/*
-	 * NEEDSWORK: re-create conflicts from merges with
-	 * merge.renormalize set, too
-	 */
+	memset(&ll_opts, 0, sizeof(ll_opts));
+	git_config_get_bool("merge.renormalize", &renormalize);
+	ll_opts.renormalize = renormalize;
 	status = ll_merge(&result_buf, path, &ancestor, "base",
 			  &ours, "ours", &theirs, "theirs",
-			  state->istate, NULL);
+			  state->istate, &ll_opts);
 	free(ancestor.ptr);
 	free(ours.ptr);
 	free(theirs.ptr);
-- 
gitgitgadget