| Message ID | 20210721134650.1866387-1-felipe.contreras@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] pull: introduce --merge option | expand |
On Wed, Jul 21, 2021 at 6:47 AM Felipe Contreras <felipe.contreras@gmail.com> wrote: > > Users need to specify if they want to either --merge or --rebase, but > unfortunately the former is missing. Ack. I think it's just historical, because long long ago it used to be that 'git pull' always merged unless told otherwise with --rebase. Linus
Linus Torvalds <torvalds@linux-foundation.org> writes: > On Wed, Jul 21, 2021 at 6:47 AM Felipe Contreras > <felipe.contreras@gmail.com> wrote: >> >> Users need to specify if they want to either --merge or --rebase, but >> unfortunately the former is missing. > > Ack. I think it's just historical, because long long ago it used to be > that 'git pull' always merged unless told otherwise with --rebase. The "--no-rebase" option, which is documented as a synonym for "--rebase=false", has been there, but the implementation is buggy in some corner cases, which has been worked on recently in a separate thread. I do not think it is too bad to add "--merge" as yet another synonym for "--rebase=false".
On Wed, Jul 21, 2021 at 11:11 AM Junio C Hamano <gitster@pobox.com> wrote: > > The "--no-rebase" option, which is documented as a synonym for > "--rebase=false", has been there, but the implementation is buggy in > some corner cases, which has been worked on recently in a separate > thread. I do not think it is too bad to add "--merge" as yet > another synonym for "--rebase=false". It's convenient to have the one-letter option `git pull -r` to override the configuration and do a rebase. I'd really like to have a similar one-letter option `git pull -m` to override the configuration and do a merge. That would also alleviate a lot of the desire for a separate `git update` (i.e. "fetch and rebase") command. Junio, would you be willing to accept adding -m without adding --merge also? -Alex
Alex Henrie wrote: > On Wed, Jul 21, 2021 at 11:11 AM Junio C Hamano <gitster@pobox.com> wrote: > > > > The "--no-rebase" option, which is documented as a synonym for > > "--rebase=false", has been there, but the implementation is buggy in > > some corner cases, which has been worked on recently in a separate > > thread. I do not think it is too bad to add "--merge" as yet > > another synonym for "--rebase=false". > > It's convenient to have the one-letter option `git pull -r` to > override the configuration and do a rebase. I'd really like to have a > similar one-letter option `git pull -m` to override the configuration > and do a merge. That would also alleviate a lot of the desire for a > separate `git update` (i.e. "fetch and rebase") command. My proposed `git update` is not "fetch and rebase", but fetch and fast-forward. Morevoer, `git pull -m` would still merge with the wrong order of the parents. On the other hand `git update --merge` would merge them with the correct order. I'm not sure what -m would alleviate.
Junio C Hamano wrote: > Linus Torvalds <torvalds@linux-foundation.org> writes: > > > On Wed, Jul 21, 2021 at 6:47 AM Felipe Contreras > > <felipe.contreras@gmail.com> wrote: > >> > >> Users need to specify if they want to either --merge or --rebase, but > >> unfortunately the former is missing. > > > > Ack. I think it's just historical, because long long ago it used to be > > that 'git pull' always merged unless told otherwise with --rebase. > > The "--no-rebase" option, which is documented as a synonym for > "--rebase=false", has been there, but the implementation is buggy in > some corner cases, which has been worked on recently in a separate > thread. I do not think it is too bad to add "--merge" as yet > another synonym for "--rebase=false". Any particular reason why this is not a topic? [1] [1] https://lore.kernel.org/git/xmqq35s0fj9o.fsf@gitster.g/
Alex Henrie <alexhenrie24@gmail.com> writes:
> Junio, would you be willing to accept adding -m without adding --merge also?
My gut feeling is that "-m" without "--merge" in the context of
"pull" is extremely unlikely to fly well.
As "git pull" is a "git fetch" followed by a "git merge" (or "git
rebase"), it takes the union of common command line options from
both phases, and "git merge" takes "-m 'message'" which is an option
fairly familiar to users (since it comes from "git commit"). Even
if we are never going to pass "-m message" from "git pull" down to
underlying "git merge", squatting on short and common "-m" would be
a bad idea.
On Tue, Jul 27, 2021 at 2:45 AM Junio C Hamano <gitster@pobox.com> wrote: > > Alex Henrie <alexhenrie24@gmail.com> writes: > > > Junio, would you be willing to accept adding -m without adding --merge also? > > My gut feeling is that "-m" without "--merge" in the context of > "pull" is extremely unlikely to fly well. > > As "git pull" is a "git fetch" followed by a "git merge" (or "git > rebase"), it takes the union of common command line options from > both phases, and "git merge" takes "-m 'message'" which is an option > fairly familiar to users (since it comes from "git commit"). Even > if we are never going to pass "-m message" from "git pull" down to > underlying "git merge", squatting on short and common "-m" would be > a bad idea. Thanks for the explanation. I forgot that "-m" usually means "message". That does seem like a good reason to not use "-m" for "merge". -Alex
Alex Henrie wrote: > On Tue, Jul 27, 2021 at 2:45 AM Junio C Hamano <gitster@pobox.com> wrote: > > > > Alex Henrie <alexhenrie24@gmail.com> writes: > > > > > Junio, would you be willing to accept adding -m without adding --merge also? > > > > My gut feeling is that "-m" without "--merge" in the context of > > "pull" is extremely unlikely to fly well. > > > > As "git pull" is a "git fetch" followed by a "git merge" (or "git > > rebase"), it takes the union of common command line options from > > both phases, and "git merge" takes "-m 'message'" which is an option > > fairly familiar to users (since it comes from "git commit"). Even > > if we are never going to pass "-m message" from "git pull" down to > > underlying "git merge", squatting on short and common "-m" would be > > a bad idea. > > Thanks for the explanation. I forgot that "-m" usually means > "message". That does seem like a good reason to not use "-m" for > "merge". It means --merge plenty of times: * git restore -m * git checkout -m * git rebase -m * git diff -m * git read-tree -m * git diff-tree -m
On 7/27/21 6:48 PM, Felipe Contreras wrote: > Alex Henrie wrote: >> On Tue, Jul 27, 2021 at 2:45 AM Junio C Hamano <gitster@pobox.com> wrote: >>> >>> Alex Henrie <alexhenrie24@gmail.com> writes: >>> >>>> Junio, would you be willing to accept adding -m without adding --merge also? >>> >>> My gut feeling is that "-m" without "--merge" in the context of >>> "pull" is extremely unlikely to fly well. >>> >>> As "git pull" is a "git fetch" followed by a "git merge" (or "git >>> rebase"), it takes the union of common command line options from >>> both phases, and "git merge" takes "-m 'message'" which is an option >>> fairly familiar to users (since it comes from "git commit"). Even >>> if we are never going to pass "-m message" from "git pull" down to >>> underlying "git merge", squatting on short and common "-m" would be >>> a bad idea. >> >> Thanks for the explanation. I forgot that "-m" usually means >> "message". That does seem like a good reason to not use "-m" for >> "merge". > > It means --merge plenty of times: > > * git restore -m > * git checkout -m > * git rebase -m > * git diff -m > * git read-tree -m > * git diff-tree -m Add to Felipes list: * git switch -m and maybe git cherry-pick -m where -m does not mean "merge" itself but is used to determine the parent of the merge (when picking merge commits) to base on. Other examples of where -m has different meaning than merge: * git am -m (message-id) * git branch -m (move branch) I would rephrase the question as to what would I expect `git pull -m` to do, if I had never heard of it before. In the case of fast-forwarding and rebasing trying to add a merge commit message with -m would not even make sense. Only in the case of trying to create a merge commit by issuing git pull this would make sense. So if we could agree on that being not the most used scenario, I think -m would be a great short option for --merge.
Matthias Baumgarten <matthias.baumgarten@aixigo.com> writes: > Add to Felipes list: > > * git switch -m > > and maybe git cherry-pick -m where -m does not mean "merge" itself but > is used to determine the parent of the merge (when picking merge > commits) to base on. > > Other examples of where -m has different meaning than merge: > > * git am -m (message-id) > * git branch -m (move branch) > > I would rephrase the question as to what would I expect `git pull -m` > to do, if I had never heard of it before. In the case of > fast-forwarding and rebasing trying to add a merge commit message with > -m would not even make sense. Only in the case of trying to create a > merge commit by issuing git pull this would make sense. So if we could > agree on that being not the most used scenario, I think -m would be a > great short option for --merge. I am afraid that you are misinterpreting what I said, comparing apples and oranges, and drawing a wrong conclusion. When I said "-m" would not fly well as a short-hand for "--merge" in the context of "pull", I didn't mean "nobody would think 'm' stands for 'merge'", and I didn't mean "more people would think 'm' stands for 'message' more than 'merge'". The reason why I find it problematic is because it can be ambiguous. When we step back and think about your "switch -m" and its synonym "checkout -m", we realize that these commands fundamentally never take "--message", as there is no place to record such a message (they do not create a commit after all), after they switch to a different branch while carrying the local modification forward by performing a (possibly conflicting) content-level merge. That is why we can give their "merge" operation a short-and-sweet "m" without confusing our users. So contrasting "switch" having "-m" that means "merge" with "pull" that can conceivably take both "merge" and "message" is not a comparison you can draw useful conclusion from.
On 7/28/21 7:18 PM, Junio C Hamano wrote: > I am afraid that you are misinterpreting what I said, comparing > apples and oranges, and drawing a wrong conclusion > > When I said "-m" would not fly well as a short-hand for "--merge" in > the context of "pull", I didn't mean "nobody would think 'm' stands > for 'merge'", and I didn't mean "more people would think 'm' stands > for 'message' more than 'merge'". The reason why I find it > problematic is because it can be ambiguous > > When we step back and think about your "switch -m" and its synonym > "checkout -m", we realize that these commands fundamentally never > take "--message", as there is no place to record such a message > (they do not create a commit after all), after they switch to a > different branch while carrying the local modification forward by > performing a (possibly conflicting) content-level merge. That is > why we can give their "merge" operation a short-and-sweet "m" > without confusing our users. So contrasting "switch" having "-m" > that means "merge" with "pull" that can conceivably take both > "merge" and "message" is not a comparison you can draw useful > conclusion from. I must confess that this comparison is indeed not a valid one. Maybe -m isn't as great as I thought it was.
Junio C Hamano wrote: > Matthias Baumgarten <matthias.baumgarten@aixigo.com> writes: > > > Add to Felipes list: > > > > * git switch -m > > > > and maybe git cherry-pick -m where -m does not mean "merge" itself but > > is used to determine the parent of the merge (when picking merge > > commits) to base on. > > > > Other examples of where -m has different meaning than merge: > > > > * git am -m (message-id) > > * git branch -m (move branch) > > > > I would rephrase the question as to what would I expect `git pull -m` > > to do, if I had never heard of it before. In the case of > > fast-forwarding and rebasing trying to add a merge commit message with > > -m would not even make sense. Only in the case of trying to create a > > merge commit by issuing git pull this would make sense. So if we could > > agree on that being not the most used scenario, I think -m would be a > > great short option for --merge. > > I am afraid that you are misinterpreting what I said, comparing > apples and oranges, and drawing a wrong conclusion. > > When I said "-m" would not fly well as a short-hand for "--merge" in > the context of "pull", I didn't mean "nobody would think 'm' stands > for 'merge'", and I didn't mean "more people would think 'm' stands > for 'message' more than 'merge'". The reason why I find it > problematic is because it can be ambiguous. The question shouldn't be "can it be ambiguous?", the question should be "is it ambiguous?". The *main* purpose of `git pull` is to integrate remote changes, and the first question asked is "how?". git pull --merge|-m git pull --rebase|-r So I don't see why it is ambiguous. The fact that a tiny minority of users might find a command (any command) ambiguous is not valid reason for its inexistence. By that rationale `git pull` shouldn't exist at all, because many find it ambiguous that it's not the symmetric command opposed to `git push`. The vast majority of users shouldn't suffer because of the confusion of a tiny few. The tiny few can simply look at the documentation.
diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt index 5c3fb67c01..c7a1f676cc 100644 --- a/Documentation/git-pull.txt +++ b/Documentation/git-pull.txt @@ -134,6 +134,12 @@ unless you have read linkgit:git-rebase[1] carefully. --no-rebase:: Override earlier --rebase. +-m:: +--merge:: + Do a merge. ++ +Alias for --no-rebase. + Options related to fetching ~~~~~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/builtin/pull.c b/builtin/pull.c index 3e13f81084..0d76b54186 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -129,6 +129,8 @@ static struct option pull_options[] = { "(false|true|merges|preserve|interactive)", N_("incorporate changes by rebasing rather than merging"), PARSE_OPT_OPTARG, parse_opt_rebase), + OPT_SET_INT('m', "merge", &opt_rebase, + N_("incorporate changes by merging"), REBASE_FALSE), OPT_PASSTHRU('n', NULL, &opt_diffstat, NULL, N_("do not show a diffstat at the end of the merge"), PARSE_OPT_NOARG | PARSE_OPT_NONEG), @@ -936,7 +938,7 @@ static void show_advice_pull_non_ff(void) " git config pull.ff only # fast-forward only\n" "\n" "You can replace \"git config\" with \"git config --global\" to set a default\n" - "preference for all repositories. You can also pass --rebase, --no-rebase,\n" + "preference for all repositories. You can also pass --rebase, --merge,\n" "or --ff-only on the command line to override the configured default per\n" "invocation.\n")); } diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh index e2c0c51022..f299a82405 100755 --- a/t/t5520-pull.sh +++ b/t/t5520-pull.sh @@ -452,6 +452,13 @@ test_expect_success 'pull.rebase' ' test_cmp expect actual ' +test_expect_success 'pull.rebase with --merge' ' + git reset --hard before-rebase && + test_config pull.rebase true && + git pull --merge . copy && + test_cmp_rev HEAD^2 copy +' + test_expect_success 'pull --autostash & pull.rebase=true' ' test_config pull.rebase true && test_pull_autostash 1 --autostash
Users need to specify if they want to either --merge or --rebase, but unfortunately the former is missing. In many discussions regarding `git pull` --merge is often mentioned, but for one reason or another the patches have not been merged. Let's just go ahead and do it. Now we can update the default warning to say --merge instead of the non-intuitive --no-rebase. Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> --- Since v1 I've removed any hint of deprecation (even though we obviously want to do that), and added an actual test (I don't seem to find a simialr one for --no-rebase). Range-diff against v1: 1: 80d7866599 < -: ---------- pull: introduce --merge option -: ---------- > 1: 5969fc4455 pull: introduce --merge option Documentation/git-pull.txt | 6 ++++++ builtin/pull.c | 4 +++- t/t5520-pull.sh | 7 +++++++ 3 files changed, 16 insertions(+), 1 deletion(-)