git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Philippe Blain <levraiphilippeblain@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: John Cai <johncai86@gmail.com>,
	git@vger.kernel.org, Tilman Vogel <tilman.vogel@web.de>
Subject: Re: [PATCH 1/1] builtin/pull.c: use config value of autostash
Date: Thu, 13 Jan 2022 22:14:51 -0500	[thread overview]
Message-ID: <8d19623b-4b11-8ecc-8f27-17ad4ba4167a@gmail.com> (raw)
In-Reply-To: <xmqqee5bfaof.fsf@gitster.g>

Hi Junio,

Le 2022-01-13 à 19:00, Junio C Hamano a écrit :
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> Here is how I would explain your change.
> 
> This topic is in "Expecting an ack or two" state for some time.
> 
>   - Philippe, are you OK with the attached patch?  If so throw your
>     Signed-off-by to this thread.

I'm not 100% OK since as I remarked to John in [1], I don't think all 4
tests are necessary, 3 out of 4 are duplicates of existing tests, and I
would have put the new test in t5520 with 'git pull's other "autostash"
tests. I hoped that John would incorporate my suggestions in a v2, but he
seems to be busy, so I'm including an updated patch at the end of this email.
I only slightly edited the commit message you wrote, so thanks for that.
'pb/pull-rebase-autostash-fix' could be replaced by the patch below,
I would think.

>   - John, if you find Philippe's implementation a good idea (I think
>     it is, as it is simpler and cleaner) after reading and
>     understanding it, please throw an Acked-by or Reviewed-by to this
>     thread.
> 


Thanks,

Philippe.

[1] https://lore.kernel.org/git/fe0b7337-3005-d09c-a3b6-65a100799676@gmail.com/

----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----
 From 28edde4e302e14c900e314268e4eeaeadc240bcb Mon Sep 17 00:00:00 2001
From: Philippe Blain <levraiphilippeblain@gmail.com>
Date: Thu, 13 Jan 2022 21:58:05 -0500
Subject: [PATCH] pull --rebase: honor rebase.autostash when fast-forwarding

"pull --rebase" internally uses the merge machinery when the other
history is a descendant of ours (i.e. perform fast-forward).  This
came from [1], where the discussion was started from a feature
request to do so.  It is a bit hard to read the rationale behind it
in the discussion, but it seems that it was an established fact for
everybody involved that does not even need to be mentioned that
fast-forwarding done with "rebase" was much undesirable than done
with "merge", and more importantly, the result left by "merge" is as
good as (or better than) that by "rebase".

Except for one thing.  Because "git merge" does not (and should not)
honor rebase.autostash, "git pull" needs to read it and forward it
when we use "git merge" as a (hopefully better) substitute for "git
rebase" during the fast-forwarding.  But we forgot to do so (we only
add "--[no-]autostash" to the "git merge" command when "git pull" itself
was invoked with "--[no-]autostash" command line option.

Make sure "git merge" is run with "--autostash" when
rebase.autostash is set and used to fast-forward the history on
behalf of "git rebase".  Incidentally this change also takes care of
the case where

  - "git pull --rebase" (without other command line options) is run
  - "rebase.autostash" is not set
  - The history fast-forwards

In such a case, "git merge" is run with an explicit "--no-autostash"
to prevent it from honoring merge.autostash configuration, which is
what we want.  After all, we want the "git merge" to pretend as if
it is "git rebase" while being used for this purpose.

[1] https://lore.kernel.org/git/xmqqa8cfbkeq.fsf_-_@gitster.mtv.corp.google.com/

Reported-by: Tilman Vogel <tilman.vogel@web.de>
Co-authored-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
  builtin/pull.c  |  7 +++----
  t/t5520-pull.sh | 13 +++++++++++++
  2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 1cfaf9f343..9f8a8dd716 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -1036,14 +1036,13 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
  		oidclr(&orig_head);
  
  	if (opt_rebase) {
-		int autostash = config_autostash;
-		if (opt_autostash != -1)
-			autostash = opt_autostash;
+		if (opt_autostash == -1)
+			opt_autostash = config_autostash;
  
  		if (is_null_oid(&orig_head) && !is_cache_unborn())
  			die(_("Updating an unborn branch with changes added to the index."));
  
-		if (!autostash)
+		if (!opt_autostash)
  			require_clean_work_tree(the_repository,
  				N_("pull with rebase"),
  				_("please commit or stash them."), 1, 0);
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 93ecfcdd24..3e784f18a6 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -330,6 +330,19 @@ test_expect_success '--rebase --autostash fast forward' '
  	test_cmp_rev HEAD to-rebase-ff
  '
  
+test_expect_success '--rebase with rebase.autostash succeeds on ff' '
+	test_when_finished "rm -fr src dst actual" &&
+	git init src &&
+	test_commit -C src "initial" file "content" &&
+	git clone src dst &&
+	test_commit -C src --printf "more_content" file "more content\ncontent\n" &&
+	echo "dirty" >>dst/file &&
+	test_config -C dst rebase.autostash true &&
+	git -C dst pull --rebase  >actual 2>&1 &&
+	grep -q "Fast-forward" actual &&
+	grep -q "Applied autostash." actual
+'
+
  test_expect_success '--rebase with conflicts shows advice' '
  	test_when_finished "git rebase --abort; git checkout -f to-rebase" &&
  	git checkout -b seq &&

base-commit: e9d7761bb94f20acc98824275e317fa82436c25d
prerequisite-patch-id: 3c6b4be75d7a634bf45f0264b3f04216818a0816
-- 
2.29.2

  reply	other threads:[~2022-01-14  3:14 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-01-04 21:45 [PATCH 0/1] Fix bug in pull --rebase not recognizing rebase.autostash John Cai
2022-01-04 21:45 ` [PATCH 1/1] builtin/pull.c: use config value of autostash John Cai
2022-01-04 22:46   ` Junio C Hamano
2022-01-05  3:58     ` Philippe Blain
2022-01-06 19:11       ` Junio C Hamano
2022-01-14  0:00         ` Junio C Hamano
2022-01-14  3:14           ` Philippe Blain [this message]
2022-01-14 14:09             ` John Cai
2022-01-14 19:40             ` Junio C Hamano
2022-01-14 23:33               ` Philippe Blain
2022-01-05 11:21     ` Phillip Wood
2022-01-05  3:40   ` Philippe Blain
2022-01-05  4:02     ` Philippe Blain
2022-01-05 15:50   ` Johannes Schindelin
2022-01-04 23:32 ` [PATCH 0/1] Fix bug in pull --rebase not recognizing rebase.autostash Philippe Blain

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=8d19623b-4b11-8ecc-8f27-17ad4ba4167a@gmail.com \
    --to=levraiphilippeblain@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=johncai86@gmail.com \
    --cc=tilman.vogel@web.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).