| Message ID | 20210609192842.696646-5-felipe.contreras@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Make diff3 the default conflict style | expand |
On 09/06/2021 20:28, Felipe Contreras wrote: > Currently both merge.conflictStyle and `git commit --merge > --conflict=diff3` don't work together, since the former wrongly > overrides the later. > > The way merge configurations are handled is not correct. > It should be possible to do git_config(merge_recursive_config, ...) just > like we can with git_diff_basic_config and others. It would be helpful to explain what the problem with merge_recursive_config() actually is rather than just saying "it should be possible ..." > Therefore builtins like `git merge` can't call this function at the > right time. > > We shuffle the functions a little bit so at least merge_recursive_config > doesn't call git_xmerge_config directly and thus override previous > configurations. Rather than papering of the problem, how difficult would it be to add a field to ll_merge_options and pass the conflict style with that rather than fiddling with the order that we set a global variable. Does this change affect 'am/apply -3'? - Do they still read the config setting properly? Best Wishes Phillip > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> > --- > builtin/merge-recursive.c | 3 +++ > builtin/merge.c | 4 ++++ > merge-recursive.c | 2 +- > sequencer.c | 5 +++++ > t/t6440-config-conflict-markers.sh | 31 ++++++++++++++++++++++++++++++ > 5 files changed, 44 insertions(+), 1 deletion(-) > > diff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c > index a4bfd8fc51..80f9279b4c 100644 > --- a/builtin/merge-recursive.c > +++ b/builtin/merge-recursive.c > @@ -4,6 +4,7 @@ > #include "tag.h" > #include "merge-recursive.h" > #include "xdiff-interface.h" > +#include "config.h" > > static const char builtin_merge_recursive_usage[] = > "git %s <base>... -- <head> <remote> ..."; > @@ -30,6 +31,8 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix) > char *better1, *better2; > struct commit *result; > > + git_config(git_xmerge_config, NULL); > + > init_merge_options(&o, the_repository); > if (argv[0] && ends_with(argv[0], "-subtree")) > o.subtree_shift = ""; > diff --git a/builtin/merge.c b/builtin/merge.c > index eddb8ae70d..7aa3dbb111 100644 > --- a/builtin/merge.c > +++ b/builtin/merge.c > @@ -43,6 +43,7 @@ > #include "commit-reach.h" > #include "wt-status.h" > #include "commit-graph.h" > +#include "xdiff-interface.h" > > #define DEFAULT_TWOHEAD (1<<0) > #define DEFAULT_OCTOPUS (1<<1) > @@ -659,6 +660,9 @@ static int git_merge_config(const char *k, const char *v, void *cb) > if (status) > return status; > status = git_gpg_config(k, v, NULL); > + if (status) > + return status; > + status = git_xmerge_config(k, v, NULL); > if (status) > return status; > return git_diff_ui_config(k, v, cb); > diff --git a/merge-recursive.c b/merge-recursive.c > index d146bb116f..10e6e1e4d1 100644 > --- a/merge-recursive.c > +++ b/merge-recursive.c > @@ -3845,7 +3845,7 @@ static void merge_recursive_config(struct merge_options *opt) > } /* avoid erroring on values from future versions of git */ > free(value); > } > - git_config(git_xmerge_config, NULL); > + git_config(git_default_config, NULL); > } > > void init_merge_options(struct merge_options *opt, > diff --git a/sequencer.c b/sequencer.c > index 0bec01cf38..9e2bdca0f6 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -34,6 +34,7 @@ > #include "commit-reach.h" > #include "rebase-interactive.h" > #include "reset.h" > +#include "xdiff-interface.h" > > #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION" > > @@ -224,6 +225,10 @@ static int git_sequencer_config(const char *k, const char *v, void *cb) > if (status) > return status; > > + status = git_xmerge_config(k, v, NULL); > + if (status) > + return status; > + > return git_diff_basic_config(k, v, NULL); > } > > diff --git a/t/t6440-config-conflict-markers.sh b/t/t6440-config-conflict-markers.sh > index 44f79ac91b..485ad0eee0 100755 > --- a/t/t6440-config-conflict-markers.sh > +++ b/t/t6440-config-conflict-markers.sh > @@ -89,4 +89,35 @@ test_expect_success 'notes' ' > ) > ' > > +test_expect_success 'checkout' ' > + test_create_repo checkout && > + ( > + test_commit checkout && > + > + fill a b c d e >content && > + git add content && > + git commit -m initial && > + > + git checkout -b simple master && > + fill a c e >content && > + git commit -a -m simple && > + > + fill b d >content && > + git checkout --merge master && > + ! grep -E "\|+" content && > + > + git config merge.conflictstyle merge && > + > + git checkout -f simple && > + fill b d >content && > + git checkout --merge --conflict=diff3 master && > + grep -E "\|+" content && > + > + git checkout -f simple && > + fill b d >content && > + git checkout --merge --conflict=merge master && > + ! grep -E "\|+" content > + ) > +' > + > test_done >
Phillip Wood wrote: > On 09/06/2021 20:28, Felipe Contreras wrote: > > Currently both merge.conflictStyle and `git commit --merge > > --conflict=diff3` don't work together, since the former wrongly > > overrides the later. > > > > The way merge configurations are handled is not correct. > > It should be possible to do git_config(merge_recursive_config, ...) just > > like we can with git_diff_basic_config and others. > > It would be helpful to explain what the problem with > merge_recursive_config() actually is rather than just saying "it should > be possible ..." The problem is that you can't do this: git_config(merge_recursive_config, NULL); As it was explained. That is the problem. I don't know how that's not clear. > > Therefore builtins like `git merge` can't call this function at the > > right time. > > > > We shuffle the functions a little bit so at least merge_recursive_config > > doesn't call git_xmerge_config directly and thus override previous > > configurations. > > Rather than papering of the problem, how difficult would it be to add a > field to ll_merge_options and pass the conflict style with that rather > than fiddling with the order that we set a global variable. Probably not that difficult, but then we also need a parser that converts from "diff3" to whatever values we decide in that new field. We would need a new parse_config_conflict_style() function. And that function will be only used by `git checkout` and nothing else. So I don't think there's much value in it. That problem whoever, is orthogonal to this series. > Does this change affect 'am/apply -3'? - Do they still read the config > setting properly? Good question. I'll have to add more tests to make sure that works properly. Cheers.
On 10/06/2021 15:11, Felipe Contreras wrote: > Phillip Wood wrote: >> On 09/06/2021 20:28, Felipe Contreras wrote: >>> Currently both merge.conflictStyle and `git commit --merge >>> --conflict=diff3` don't work together, since the former wrongly >>> overrides the later. >>> >>> The way merge configurations are handled is not correct. >>> It should be possible to do git_config(merge_recursive_config, ...) just >>> like we can with git_diff_basic_config and others. >> >> It would be helpful to explain what the problem with >> merge_recursive_config() actually is rather than just saying "it should >> be possible ..." > > The problem is that you can't do this: > > git_config(merge_recursive_config, NULL); > > As it was explained. You do not explain why you cannot do that > That is the problem. I don't know how that's not clear. > >>> Therefore builtins like `git merge` can't call this function at the >>> right time. >> > >>> We shuffle the functions a little bit so at least merge_recursive_config >>> doesn't call git_xmerge_config directly and thus override previous >>> configurations. >> >> Rather than papering of the problem, how difficult would it be to add a >> field to ll_merge_options and pass the conflict style with that rather >> than fiddling with the order that we set a global variable. > > Probably not that difficult, but then we also need a parser that > converts from "diff3" to whatever values we decide in that new field. We > would need a new parse_config_conflict_style() function. > And that function will be only used by `git checkout` and nothing else. > So I don't think there's much value in it. It would allow us to add a --conflict option to all the mergey commands in the future and would be much easier to reason about than the approach of juggling where we call git_xmerge_config(). This patch requires us to audit all the code paths that end up in merge_recursive_config() to make sure they now call git_xmerge_config() themselves. You don't seem to have done that as you don't know if am/apply are affected or not. Best Wishes Phillip > That problem whoever, is orthogonal to this series. > >> Does this change affect 'am/apply -3'? - Do they still read the config >> setting properly? > > Good question. I'll have to add more tests to make sure that works > properly. > > Cheers. >
Phillip Wood wrote: > On 10/06/2021 15:11, Felipe Contreras wrote: > > Phillip Wood wrote: > >> On 09/06/2021 20:28, Felipe Contreras wrote: > >>> Currently both merge.conflictStyle and `git commit --merge > >>> --conflict=diff3` don't work together, since the former wrongly > >>> overrides the later. > >>> > >>> The way merge configurations are handled is not correct. > >>> It should be possible to do git_config(merge_recursive_config, ...) just > >>> like we can with git_diff_basic_config and others. > >> > >> It would be helpful to explain what the problem with > >> merge_recursive_config() actually is rather than just saying "it should > >> be possible ..." > > > > The problem is that you can't do this: > > > > git_config(merge_recursive_config, NULL); > > > > As it was explained. > > You do not explain why you cannot do that % git grep merge_recursive_config static void merge_recursive_config(struct merge_options *opt) For starters it's a static function. Second, clearly the type of functions git_config() receives are not `void (*)(struct merge_options *)`. I mean, I do see value in explaning as much detail as needed in the commit message, but it shouldn't be a lesson on git's codebase: git_config() is a standard thing, and it's even mentioned in the user manual. https://git-scm.com/docs/user-manual#birdview-on-the-source-code > > That is the problem. I don't know how that's not clear. > > > >>> Therefore builtins like `git merge` can't call this function at the > >>> right time. > >> > > >>> We shuffle the functions a little bit so at least merge_recursive_config > >>> doesn't call git_xmerge_config directly and thus override previous > >>> configurations. > >> > >> Rather than papering of the problem, how difficult would it be to add a > >> field to ll_merge_options and pass the conflict style with that rather > >> than fiddling with the order that we set a global variable. > > > > Probably not that difficult, but then we also need a parser that > > converts from "diff3" to whatever values we decide in that new field. We > > would need a new parse_config_conflict_style() function. > > And that function will be only used by `git checkout` and nothing else. > > So I don't think there's much value in it. > > It would allow us to add a --conflict option to all the mergey commands > in the future and would be much easier to reason about than the approach > of juggling where we call git_xmerge_config(). Feel free to write a patch for that. > This patch requires us to audit all the code paths that end up in > merge_recursive_config() to make sure they now call > git_xmerge_config() themselves. You don't seem to have done that as > you don't know if am/apply are affected or not. That is a separate issue, that I already mentioned... > > That problem whoever, is orthogonal to this series. > > > >> Does this change affect 'am/apply -3'? - Do they still read the config > >> setting properly? > > > > Good question. I'll have to add more tests to make sure that works > > properly. here.
On 10/06/2021 17:32, Felipe Contreras wrote: > Phillip Wood wrote: >> On 10/06/2021 15:11, Felipe Contreras wrote: >>> Phillip Wood wrote: >>>> On 09/06/2021 20:28, Felipe Contreras wrote: >>>>> Currently both merge.conflictStyle and `git commit --merge >>>>> --conflict=diff3` don't work together, since the former wrongly >>>>> overrides the later. >>>>> >>>>> The way merge configurations are handled is not correct. >>>>> It should be possible to do git_config(merge_recursive_config, ...) just >>>>> like we can with git_diff_basic_config and others. >>>> >>>> It would be helpful to explain what the problem with >>>> merge_recursive_config() actually is rather than just saying "it should >>>> be possible ..." >>> >>> The problem is that you can't do this: >>> >>> git_config(merge_recursive_config, NULL); >>> >>> As it was explained. >> >> You do not explain why you cannot do that > > % git grep merge_recursive_config > static void merge_recursive_config(struct merge_options *opt) > > For starters it's a static function. > > Second, clearly the type of functions git_config() receives are not > `void (*)(struct merge_options *)`. > > I mean, I do see value in explaning as much detail as needed in the > commit message, but it shouldn't be a lesson on git's codebase: > git_config() is a standard thing, and it's even mentioned in the user > manual. I'm not asking for a lesson on git's config system, I'm asking you to add a sentence to the commit message to explain what the problem is rather than just saying "you should be able to do this but you can't". > https://git-scm.com/docs/user-manual#birdview-on-the-source-code > >>> That is the problem. I don't know how that's not clear. >>> >>>>> Therefore builtins like `git merge` can't call this function at the >>>>> right time. >>>> > >>>>> We shuffle the functions a little bit so at least merge_recursive_config >>>>> doesn't call git_xmerge_config directly and thus override previous >>>>> configurations. >>>> >>>> Rather than papering of the problem, how difficult would it be to add a >>>> field to ll_merge_options and pass the conflict style with that rather >>>> than fiddling with the order that we set a global variable. >>> >>> Probably not that difficult, but then we also need a parser that >>> converts from "diff3" to whatever values we decide in that new field. We >>> would need a new parse_config_conflict_style() function. >>> And that function will be only used by `git checkout` and nothing else. >>> So I don't think there's much value in it. >> >> It would allow us to add a --conflict option to all the mergey commands >> in the future and would be much easier to reason about than the approach >> of juggling where we call git_xmerge_config(). > > Feel free to write a patch for that. > >> This patch requires us to audit all the code paths that end up in >> merge_recursive_config() to make sure they now call >> git_xmerge_config() themselves. You don't seem to have done that as >> you don't know if am/apply are affected or not. > > That is a separate issue, that I already mentioned... I know you are going to add some tests but the point is that when making a change like this you need to actively audit all the callers of init_merge_options() and ensure that they are now calling git_config(git_xmerge_config) and base you tests on what you find while doing that. Have you run 'grep init_merge_options()' to see where it is being called? Best Wishes Phillip >>> That problem whoever, is orthogonal to this series. >>> >>>> Does this change affect 'am/apply -3'? - Do they still read the config >>>> setting properly? >>> >>> Good question. I'll have to add more tests to make sure that works >>> properly. > > here. >
On 09/06/2021 20:28, Felipe Contreras wrote: >[...] > diff --git a/merge-recursive.c b/merge-recursive.c > index d146bb116f..10e6e1e4d1 100644 > --- a/merge-recursive.c > +++ b/merge-recursive.c > @@ -3845,7 +3845,7 @@ static void merge_recursive_config(struct merge_options *opt) > } /* avoid erroring on values from future versions of git */ > free(value); > } > - git_config(git_xmerge_config, NULL); > + git_config(git_default_config, NULL); Now that all callers are required to call git_config(git_xmerge_config) before calling init_merge_options() this line can be deleted. Best Wishes Phillip > } > > void init_merge_options(struct merge_options *opt, > diff --git a/sequencer.c b/sequencer.c > index 0bec01cf38..9e2bdca0f6 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -34,6 +34,7 @@ > #include "commit-reach.h" > #include "rebase-interactive.h" > #include "reset.h" > +#include "xdiff-interface.h" > > #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION" > > @@ -224,6 +225,10 @@ static int git_sequencer_config(const char *k, const char *v, void *cb) > if (status) > return status; > > + status = git_xmerge_config(k, v, NULL); > + if (status) > + return status; > + > return git_diff_basic_config(k, v, NULL); > } > > diff --git a/t/t6440-config-conflict-markers.sh b/t/t6440-config-conflict-markers.sh > index 44f79ac91b..485ad0eee0 100755 > --- a/t/t6440-config-conflict-markers.sh > +++ b/t/t6440-config-conflict-markers.sh > @@ -89,4 +89,35 @@ test_expect_success 'notes' ' > ) > ' > > +test_expect_success 'checkout' ' > + test_create_repo checkout && > + ( > + test_commit checkout && > + > + fill a b c d e >content && > + git add content && > + git commit -m initial && > + > + git checkout -b simple master && > + fill a c e >content && > + git commit -a -m simple && > + > + fill b d >content && > + git checkout --merge master && > + ! grep -E "\|+" content && > + > + git config merge.conflictstyle merge && > + > + git checkout -f simple && > + fill b d >content && > + git checkout --merge --conflict=diff3 master && > + grep -E "\|+" content && > + > + git checkout -f simple && > + fill b d >content && > + git checkout --merge --conflict=merge master && > + ! grep -E "\|+" content > + ) > +' > + > test_done >
Phillip Wood wrote: > On 10/06/2021 17:32, Felipe Contreras wrote: > > Phillip Wood wrote: > >> On 10/06/2021 15:11, Felipe Contreras wrote: > >>> Phillip Wood wrote: > >>>> On 09/06/2021 20:28, Felipe Contreras wrote: > >>>>> Currently both merge.conflictStyle and `git commit --merge > >>>>> --conflict=diff3` don't work together, since the former wrongly > >>>>> overrides the later. > >>>>> > >>>>> The way merge configurations are handled is not correct. > >>>>> It should be possible to do git_config(merge_recursive_config, ...) just > >>>>> like we can with git_diff_basic_config and others. > >>>> > >>>> It would be helpful to explain what the problem with > >>>> merge_recursive_config() actually is rather than just saying "it should > >>>> be possible ..." > >>> > >>> The problem is that you can't do this: > >>> > >>> git_config(merge_recursive_config, NULL); > >>> > >>> As it was explained. > >> > >> You do not explain why you cannot do that > > > > % git grep merge_recursive_config > > static void merge_recursive_config(struct merge_options *opt) > > > > For starters it's a static function. > > > > Second, clearly the type of functions git_config() receives are not > > `void (*)(struct merge_options *)`. > > > > I mean, I do see value in explaning as much detail as needed in the > > commit message, but it shouldn't be a lesson on git's codebase: > > git_config() is a standard thing, and it's even mentioned in the user > > manual. > > I'm not asking for a lesson on git's config system, I'm asking you to > add a sentence to the commit message to explain what the problem is > rather than just saying "you should be able to do this but you can't". Yes, but why? What possible reason could there be for this to fail? Can you list two? git_config(merge_recursive_config, NULL); But more importantly, why does it matter? How would it change the patch? > > https://git-scm.com/docs/user-manual#birdview-on-the-source-code > > > >>> That is the problem. I don't know how that's not clear. > >>> > >>>>> Therefore builtins like `git merge` can't call this function at the > >>>>> right time. > >>>> > > >>>>> We shuffle the functions a little bit so at least merge_recursive_config > >>>>> doesn't call git_xmerge_config directly and thus override previous > >>>>> configurations. > >>>> > >>>> Rather than papering of the problem, how difficult would it be to add a > >>>> field to ll_merge_options and pass the conflict style with that rather > >>>> than fiddling with the order that we set a global variable. > >>> > >>> Probably not that difficult, but then we also need a parser that > >>> converts from "diff3" to whatever values we decide in that new field. We > >>> would need a new parse_config_conflict_style() function. > >>> And that function will be only used by `git checkout` and nothing else. > >>> So I don't think there's much value in it. > >> > >> It would allow us to add a --conflict option to all the mergey commands > >> in the future and would be much easier to reason about than the approach > >> of juggling where we call git_xmerge_config(). > > > > Feel free to write a patch for that. > > > >> This patch requires us to audit all the code paths that end up in > >> merge_recursive_config() to make sure they now call > >> git_xmerge_config() themselves. You don't seem to have done that as > >> you don't know if am/apply are affected or not. > > > > That is a separate issue, that I already mentioned... > > I know you are going to add some tests but the point is that when making > a change like this you need to actively audit all the callers of > init_merge_options() and ensure that they are now calling > git_config(git_xmerge_config) and base you tests on what you find while > doing that. And what makes you think I haven't? If developers were perfect there would be no need for code review. > Have you run 'grep init_merge_options()' to see where it is > being called? Now that is insulting. Cheers.
diff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c index a4bfd8fc51..80f9279b4c 100644 --- a/builtin/merge-recursive.c +++ b/builtin/merge-recursive.c @@ -4,6 +4,7 @@ #include "tag.h" #include "merge-recursive.h" #include "xdiff-interface.h" +#include "config.h" static const char builtin_merge_recursive_usage[] = "git %s <base>... -- <head> <remote> ..."; @@ -30,6 +31,8 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix) char *better1, *better2; struct commit *result; + git_config(git_xmerge_config, NULL); + init_merge_options(&o, the_repository); if (argv[0] && ends_with(argv[0], "-subtree")) o.subtree_shift = ""; diff --git a/builtin/merge.c b/builtin/merge.c index eddb8ae70d..7aa3dbb111 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -43,6 +43,7 @@ #include "commit-reach.h" #include "wt-status.h" #include "commit-graph.h" +#include "xdiff-interface.h" #define DEFAULT_TWOHEAD (1<<0) #define DEFAULT_OCTOPUS (1<<1) @@ -659,6 +660,9 @@ static int git_merge_config(const char *k, const char *v, void *cb) if (status) return status; status = git_gpg_config(k, v, NULL); + if (status) + return status; + status = git_xmerge_config(k, v, NULL); if (status) return status; return git_diff_ui_config(k, v, cb); diff --git a/merge-recursive.c b/merge-recursive.c index d146bb116f..10e6e1e4d1 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -3845,7 +3845,7 @@ static void merge_recursive_config(struct merge_options *opt) } /* avoid erroring on values from future versions of git */ free(value); } - git_config(git_xmerge_config, NULL); + git_config(git_default_config, NULL); } void init_merge_options(struct merge_options *opt, diff --git a/sequencer.c b/sequencer.c index 0bec01cf38..9e2bdca0f6 100644 --- a/sequencer.c +++ b/sequencer.c @@ -34,6 +34,7 @@ #include "commit-reach.h" #include "rebase-interactive.h" #include "reset.h" +#include "xdiff-interface.h" #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION" @@ -224,6 +225,10 @@ static int git_sequencer_config(const char *k, const char *v, void *cb) if (status) return status; + status = git_xmerge_config(k, v, NULL); + if (status) + return status; + return git_diff_basic_config(k, v, NULL); } diff --git a/t/t6440-config-conflict-markers.sh b/t/t6440-config-conflict-markers.sh index 44f79ac91b..485ad0eee0 100755 --- a/t/t6440-config-conflict-markers.sh +++ b/t/t6440-config-conflict-markers.sh @@ -89,4 +89,35 @@ test_expect_success 'notes' ' ) ' +test_expect_success 'checkout' ' + test_create_repo checkout && + ( + test_commit checkout && + + fill a b c d e >content && + git add content && + git commit -m initial && + + git checkout -b simple master && + fill a c e >content && + git commit -a -m simple && + + fill b d >content && + git checkout --merge master && + ! grep -E "\|+" content && + + git config merge.conflictstyle merge && + + git checkout -f simple && + fill b d >content && + git checkout --merge --conflict=diff3 master && + grep -E "\|+" content && + + git checkout -f simple && + fill b d >content && + git checkout --merge --conflict=merge master && + ! grep -E "\|+" content + ) +' + test_done
Currently both merge.conflictStyle and `git commit --merge --conflict=diff3` don't work together, since the former wrongly overrides the later. The way merge configurations are handled is not correct. It should be possible to do git_config(merge_recursive_config, ...) just like we can with git_diff_basic_config and others. Therefore builtins like `git merge` can't call this function at the right time. We shuffle the functions a little bit so at least merge_recursive_config doesn't call git_xmerge_config directly and thus override previous configurations. Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> --- builtin/merge-recursive.c | 3 +++ builtin/merge.c | 4 ++++ merge-recursive.c | 2 +- sequencer.c | 5 +++++ t/t6440-config-conflict-markers.sh | 31 ++++++++++++++++++++++++++++++ 5 files changed, 44 insertions(+), 1 deletion(-)