| Message ID | 20201204061623.1170745-12-felipe.contreras@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pull: default warning improvements | expand |
On Thu, Dec 3, 2020 at 10:16 PM Felipe Contreras <felipe.contreras@gmail.com> wrote: > > We want --ff-only to make sense only when no --merge or --rebase option > is specified. A lot of git commands have opposite options, and git allows them both to be specified with the last one winning. Thus, much like git log --patch --no-patch mean show logs without patches and git log --no-patch --patch means show logs with patches, I would similarly expect the following two commands to have opposite behavior: git pull --merge --no-ff git pull --no-ff --merge > Currently --rebase already ignores --ff-only (or any other --ff option), > but --merge fails. > > Make it so --ff-only is only considered in the default mode. > > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> > --- > builtin/pull.c | 4 ++++ > t/t5520-pull.sh | 2 +- > 2 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/builtin/pull.c b/builtin/pull.c > index f54ff36b57..ebf2ac687b 100644 > --- a/builtin/pull.c > +++ b/builtin/pull.c > @@ -1037,6 +1037,10 @@ int cmd_pull(int argc, const char **argv, const char *prefix) > } > } > > + /* Disable --ff-only when --merge is specified */ > + if (!can_ff && !default_mode && !opt_rebase && opt_ff && !strcmp(opt_ff, "--ff-only")) > + opt_ff = NULL; > + > if (opt_rebase) { > int ret = 0; > if ((recurse_submodules == RECURSE_SUBMODULES_ON || > diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh > index fdd1f79b06..eec6224fb0 100755 > --- a/t/t5520-pull.sh > +++ b/t/t5520-pull.sh > @@ -843,7 +843,7 @@ test_expect_success 'git pull non-fast-forward (ff-only)' ' > test_must_fail git pull > ' > > -test_expect_failure 'git pull non-fast-forward with merge (ff-only)' ' > +test_expect_success 'git pull non-fast-forward with merge (ff-only)' ' > test_when_finished "git checkout master && git branch -D other test" && > test_config pull.ff only && > git checkout -b other master^ && > -- > 2.29.2 >
On Fri, Dec 4, 2020 at 5:39 PM Elijah Newren <newren@gmail.com> wrote: > > On Thu, Dec 3, 2020 at 10:16 PM Felipe Contreras > <felipe.contreras@gmail.com> wrote: > > > > We want --ff-only to make sense only when no --merge or --rebase option > > is specified. > > A lot of git commands have opposite options, and git allows them both > to be specified with the last one winning. Thus, much like > git log --patch --no-patch > mean show logs without patches and > git log --no-patch --patch > means show logs with patches, I would similarly expect the following > two commands to have opposite behavior: > git pull --merge --no-ff > git pull --no-ff --merge Good point. Although I presume you meant --ff-only. Taking that into consideration it may be possible to make --ff-only work straightforwardly. Further changes to the code are needed though. In the meantime I'm sending a quick patch on top of this series. Cheers.
On Fri, Dec 4, 2020 at 10:01 PM Felipe Contreras <felipe.contreras@gmail.com> wrote: > > On Fri, Dec 4, 2020 at 5:39 PM Elijah Newren <newren@gmail.com> wrote: > > > > On Thu, Dec 3, 2020 at 10:16 PM Felipe Contreras > > <felipe.contreras@gmail.com> wrote: > > > > > > We want --ff-only to make sense only when no --merge or --rebase option > > > is specified. > > > > A lot of git commands have opposite options, and git allows them both > > to be specified with the last one winning. Thus, much like > > git log --patch --no-patch > > mean show logs without patches and > > git log --no-patch --patch > > means show logs with patches, I would similarly expect the following > > two commands to have opposite behavior: > > git pull --merge --no-ff > > git pull --no-ff --merge > > Good point. > > Although I presume you meant --ff-only. > > Taking that into consideration it may be possible to make --ff-only > work straightforwardly. > > Further changes to the code are needed though. In the meantime I'm > sending a quick patch on top of this series. Nevermind. I have implemented the changes properly and v3 is ready with some interesting improvements, but it is still not straightforward. Currently "git pull --ff-only --merge" fails with: fatal: Not possible to fast-forward, aborting. With the proposed change --merge would override --ff-only and make the pull essentially --no-ff. That's still a semantic change. So there's no way around that. Cheers.
diff --git a/builtin/pull.c b/builtin/pull.c index f54ff36b57..ebf2ac687b 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -1037,6 +1037,10 @@ int cmd_pull(int argc, const char **argv, const char *prefix) } } + /* Disable --ff-only when --merge is specified */ + if (!can_ff && !default_mode && !opt_rebase && opt_ff && !strcmp(opt_ff, "--ff-only")) + opt_ff = NULL; + if (opt_rebase) { int ret = 0; if ((recurse_submodules == RECURSE_SUBMODULES_ON || diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh index fdd1f79b06..eec6224fb0 100755 --- a/t/t5520-pull.sh +++ b/t/t5520-pull.sh @@ -843,7 +843,7 @@ test_expect_success 'git pull non-fast-forward (ff-only)' ' test_must_fail git pull ' -test_expect_failure 'git pull non-fast-forward with merge (ff-only)' ' +test_expect_success 'git pull non-fast-forward with merge (ff-only)' ' test_when_finished "git checkout master && git branch -D other test" && test_config pull.ff only && git checkout -b other master^ &&
We want --ff-only to make sense only when no --merge or --rebase option is specified. Currently --rebase already ignores --ff-only (or any other --ff option), but --merge fails. Make it so --ff-only is only considered in the default mode. Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> --- builtin/pull.c | 4 ++++ t/t5520-pull.sh | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-)