Skip to content

Conversation

newren
Copy link
Contributor

@newren newren commented Aug 2, 2020

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.

@newren newren force-pushed the attr-fixes branch 3 times, most recently from b893e51 to fcc7ea3 Compare August 2, 2020 05:36
@newren
Copy link
Contributor Author

newren commented Aug 2, 2020

/submit

@gitgitgadget-git
Copy link

EOF
git config merge.renormalize true &&
git rm -fr . &&

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"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' '

EOF
git config merge.renormalize true &&
git rm -fr . &&

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Eric Sunshine wrote (reply to this):

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>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Elijah Newren wrote (reply to this):

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.

* branch, leaving the working tree as the
* merged version, but skipping unmerged
* entries in the index.
*/

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"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;
 

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Elijah Newren wrote (reply to this):

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.

EOF
git config merge.renormalize true &&
git rm -fr . &&

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Elijah Newren wrote (reply to this):

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...

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

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".

@gitgitgadget-git
Copy link

This branch is now known as en/eol-attrs-gotchas.

@gitgitgadget-git
Copy link

This patch series was integrated into seen via e0fd819.

@gitgitgadget-git gitgitgadget-git bot added the seen label Aug 2, 2020
newren added 4 commits August 3, 2020 08:56
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>
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>
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>
Signed-off-by: Elijah Newren <newren@gmail.com>
@newren
Copy link
Contributor Author

newren commented Aug 3, 2020

/submit

@gitgitgadget-git
Copy link

@gitgitgadget-git
Copy link

This patch series was integrated into seen via c565477.

@gitgitgadget-git
Copy link

This patch series was integrated into seen via c227195.

@gitgitgadget-git
Copy link

This patch series was integrated into seen via dc4bba4.

@gitgitgadget-git
Copy link

This patch series was integrated into seen via b1de6d1.

@gitgitgadget-git
Copy link

This patch series was integrated into seen via f16958b.

@gitgitgadget-git
Copy link

This patch series was integrated into next via 46e73f4.

@gitgitgadget-git gitgitgadget-git bot added the next label Aug 5, 2020
@atabaytebrizl

This comment has been minimized.

@gitgitgadget-git
Copy link

This patch series was integrated into seen via 2bca63b.

@gitgitgadget-git
Copy link

This patch series was integrated into seen via 4339259.

@gitgitgadget-git
Copy link

This patch series was integrated into next via 4339259.

@gitgitgadget-git
Copy link

This patch series was integrated into master via 4339259.

@gitgitgadget-git
Copy link

Closed via 4339259.

@newren newren deleted the attr-fixes branch August 12, 2020 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants