[PATCH 0/1] Fix bug in pull --rebase not recognizing rebase.autostash

[PATCH 0/1] Fix bug in pull --rebase not recognizing rebase.autostash

[John Cai]

NOTE: this is my first patch I'm submitting through git send-email. Please let
me know if there is some convention that I'm missing. thank you in advance :)

A bug in pull.c causes merge and rebase functions to ignore
rebase.autostash if it is only set in the config.

There are a couple of different scenarios that we need to be mindful of:

--autostash passed in through command line
$ git pull --autostash

merge/rebase should get --autostashed passed through

--rebase passed in, rebase.autostash set in config
$ git config rebase.autostash true
$ git pull --rebase

merge/rebase should get --autostash from config

--no-autostash passed in
$ git pull --no-autostash

--no-autostash should be passed into merge/rebase

rebase.autostash set but --rebase not used
$ git config rebase.autostash true
$ git pull

--autostash should not be passed into merge but not rebase

This change adjusts variable names to make it more clear which autostash
setting it is modifying, and ensures --autostash is passed into the
merge/rebase where appropriate.

John Cai (1):
  builtin/pull.c: use config value of autostash

 builtin/pull.c          | 15 ++++++------
 t/t5521-pull-options.sh | 51 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+), 7 deletions(-)

-- 
2.34.1

[PATCH 1/1] builtin/pull.c: use config value of autostash

[John Cai]

A bug in pull.c causes merge and rebase functions to ignore
rebase.autostash if it is only set in the config.

There are a couple of different scenarios that we need to be mindful of:
1. --autostash passed in through command line
$ git pull --autostash
merge/rebase should get --autostashed passed through

2. --rebase passed in, rebase.autostash set in config
$ git config rebase.autostash true
$ git pull --rebase

merge/rebase should get --autostash from config

3. --no-autostash passed in
$ git pull --no-autostash
--no-autostash should be passed into merge/rebase

4. rebase.autostash set but --rebase not used

$ git config rebase.autostash true
$ git pull
--autostash should not be passed into merge but not rebase

This change adjusts variable names to make it more clear which autostash
setting it is modifying, and ensures --autostash is passed into the
merge/rebase where appropriate.

Reported-by: "Tilman Vogel" <tilman.vogel@web.de>
Co-authored-by: "Philippe Blain" <levraiphilippeblain@gmail.com>
Signed-Off-by: "John Cai" <johncai86@gmail.com>
---
 builtin/pull.c          | 15 ++++++------
 t/t5521-pull-options.sh | 51 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+), 7 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 100cbf9fb8..fb700c2d39 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -86,7 +86,8 @@ static char *opt_ff;
 static char *opt_verify_signatures;
 static char *opt_verify;
 static int opt_autostash = -1;
-static int config_autostash;
+static int rebase_autostash = 0;
+static int cfg_rebase_autostash;
 static int check_trust_level = 1;
 static struct strvec opt_strategies = STRVEC_INIT;
 static struct strvec opt_strategy_opts = STRVEC_INIT;
@@ -361,7 +362,7 @@ static int git_pull_config(const char *var, const char *value, void *cb)
 	int status;
 
 	if (!strcmp(var, "rebase.autostash")) {
-		config_autostash = git_config_bool(var, value);
+		cfg_rebase_autostash = git_config_bool(var, value);
 		return 0;
 	} else if (!strcmp(var, "submodule.recurse")) {
 		recurse_submodules = git_config_bool(var, value) ?
@@ -689,7 +690,7 @@ static int run_merge(void)
 		strvec_push(&args, opt_gpg_sign);
 	if (opt_autostash == 0)
 		strvec_push(&args, "--no-autostash");
-	else if (opt_autostash == 1)
+	else if (rebase_autostash == 1 || opt_autostash == 1)
 		strvec_push(&args, "--autostash");
 	if (opt_allow_unrelated_histories > 0)
 		strvec_push(&args, "--allow-unrelated-histories");
@@ -901,7 +902,7 @@ static int run_rebase(const struct object_id *newbase,
 		strvec_push(&args, opt_signoff);
 	if (opt_autostash == 0)
 		strvec_push(&args, "--no-autostash");
-	else if (opt_autostash == 1)
+	else if (rebase_autostash == 1)
 		strvec_push(&args, "--autostash");
 	if (opt_verify_signatures &&
 	    !strcmp(opt_verify_signatures, "--verify-signatures"))
@@ -1038,14 +1039,14 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 		oidclr(&orig_head);
 
 	if (opt_rebase) {
-		int autostash = config_autostash;
+		rebase_autostash = cfg_rebase_autostash;
 		if (opt_autostash != -1)
-			autostash = opt_autostash;
+			rebase_autostash = opt_autostash;
 
 		if (is_null_oid(&orig_head) && !is_cache_unborn())
 			die(_("Updating an unborn branch with changes added to the index."));
 
-		if (!autostash)
+		if (!rebase_autostash)
 			require_clean_work_tree(the_repository,
 				N_("pull with rebase"),
 				_("please commit or stash them."), 1, 0);
diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
index 66cfcb09c5..28f551db8e 100755
--- a/t/t5521-pull-options.sh
+++ b/t/t5521-pull-options.sh
@@ -251,5 +251,56 @@ test_expect_success 'git pull --no-verify --verify passed to merge' '
 	test_commit -C src two &&
 	test_must_fail git -C dst pull --no-ff --no-verify --verify
 '
+test_expect_success 'git pull --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 &&
+	git -C dst pull --rebase --autostash >actual 2>&1 &&
+	grep -q "Fast-forward" actual &&
+	grep -q "Applied autostash." actual
+'
+
+test_expect_success 'git pull --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 'git pull --rebase --autostash succeeds on non-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 "src_content" file "src content\ncontent\n" &&
+	test_commit -C dst --append "dst_content" file "dst content" &&
+	echo "dirty" >>dst/file &&
+	git -C dst pull --rebase --autostash >actual 2>&1 &&
+	grep -q "Successfully rebased and updated refs/heads/main." actual &&
+	grep -q "Applied autostash." actual
+'
+
+test_expect_success 'git pull --rebase with rebase.autostash succeeds on non-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 "src_content" file "src content\ncontent\n" &&
+	test_commit -C dst --append "dst_content" file "dst content" &&
+	echo "dirty" >>dst/file &&
+	test_config -C dst rebase.autostash true &&
+	git -C dst pull --rebase >actual 2>&1 &&
+	grep -q "Successfully rebased and updated refs/heads/main." actual &&
+	grep -q "Applied autostash." actual
+'
 
 test_done
-- 
2.34.1

Re: [PATCH 1/1] builtin/pull.c: use config value of autostash

[Junio C Hamano]

John Cai <johncai86@gmail.com> writes:

> diff --git a/builtin/pull.c b/builtin/pull.c
> index 100cbf9fb8..fb700c2d39 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -86,7 +86,8 @@ static char *opt_ff;
>  static char *opt_verify_signatures;
>  static char *opt_verify;
>  static int opt_autostash = -1;
> -static int config_autostash;
> +static int rebase_autostash = 0;
> +static int cfg_rebase_autostash;

Do not explicitly initialize statics to '0' (or NULL for that matter).

But more importantly, I have a feeling that you are making a piece
of code that is already hard to read impossible to follow by adding
yet another variable.

> diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
> index 66cfcb09c5..28f551db8e 100755
> --- a/t/t5521-pull-options.sh
> +++ b/t/t5521-pull-options.sh
> @@ -251,5 +251,56 @@ test_expect_success 'git pull --no-verify --verify passed to merge' '
>  	test_commit -C src two &&
>  	test_must_fail git -C dst pull --no-ff --no-verify --verify
>  '
> +test_expect_success 'git pull --rebase --autostash succeeds on ff' '

Missing blank line between tests.


I thought that the root cause of the problem is that run_merge() is
called even when rebase was asked for (either via pull.rebase=true
configuration or "pull --rebase" option), when the other side is a
descendant of HEAD.  The basic idea behind that behaviour may be
sound (i.e. if we do not have any of our own development on top of
their history, rebase vs merge shouldn't make any difference while
fast-forwarding HEAD to their history), except that rebase vs merge
look at different configuration variables.

I wonder if the following two-liner patch is a simpler fix that is
easier to understand.  run_merge() decides if we should pass the
"--[no-]autostash" option based on the value of opt_autostash, and
we know the value of rebase.autostash in config_autostash variable.

It appears to pass all four tests your patch adds.

 builtin/pull.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git c/builtin/pull.c w/builtin/pull.c
index 100cbf9fb8..d459a91a64 100644
--- c/builtin/pull.c
+++ w/builtin/pull.c
@@ -1133,7 +1133,14 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 			die(_("cannot rebase with locally recorded submodule modifications"));
 
 		if (can_ff) {
-			/* we can fast-forward this without invoking rebase */
+			/*
+			 * We can fast-forward without invoking
+			 * rebase, by calling run_merge().  But we
+			 * have to allow rebase.autostash=true to kick
+			 * in.
+			 */
+			if (opt_autostash < 0)
+				opt_autostash = config_autostash;
 			opt_ff = "--ff-only";
 			ret = run_merge();
 		} else {

Re: [PATCH 1/1] builtin/pull.c: use config value of autostash

[Philippe Blain]

Hi Junio,

Le 2022-01-04 à 17:46, Junio C Hamano a écrit :
> 
> I wonder if the following two-liner patch is a simpler fix that is
> easier to understand.  run_merge() decides if we should pass the
> "--[no-]autostash" option based on the value of opt_autostash, and
> we know the value of rebase.autostash in config_autostash variable.
> 
> It appears to pass all four tests your patch adds.
> 
>   builtin/pull.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git c/builtin/pull.c w/builtin/pull.c
> index 100cbf9fb8..d459a91a64 100644
> --- c/builtin/pull.c
> +++ w/builtin/pull.c
> @@ -1133,7 +1133,14 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>   			die(_("cannot rebase with locally recorded submodule modifications"));
>   
>   		if (can_ff) {
> -			/* we can fast-forward this without invoking rebase */
> +			/*
> +			 * We can fast-forward without invoking
> +			 * rebase, by calling run_merge().  But we
> +			 * have to allow rebase.autostash=true to kick
> +			 * in.
> +			 */
> +			if (opt_autostash < 0)
> +				opt_autostash = config_autostash;
>   			opt_ff = "--ff-only";
>   			ret = run_merge();
>   		} else {
> 

We already have a quite useless 'int autostash' local variable in cmd_pull
a few lines up, so an equivalent fix, I think, would be the following:

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

Thanks,
Philippe.

Re: [PATCH 1/1] builtin/pull.c: use config value of autostash

[Junio C Hamano]

Philippe Blain <levraiphilippeblain@gmail.com> writes:

> We already have a quite useless 'int autostash' local variable in cmd_pull
> a few lines up, so an equivalent fix, I think, would be the following:
>
> 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);

OK, so the idea is to make opt_autostash the _primary_ thing that
matters when deciding to do the auto-stashing.  We may make it
affected by the value we read from the configuration, if there is no
command line option that sets it.

If there is no place that cares about what is in config_autostash
(which is rebase.autostash; nobody reads merge.autostash in this
command) other than this part, I think that is an even cleaner
approach than what I sent.  I very much like it.

Here is how I would explain your change.

----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----
Subject: pull --rebase: honor rebase.autostash even 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" was
invoked with "--[no-]autostash" command line option, so that "git merge"
can honor merge.autostash in such a case).

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/

Re: [PATCH 1/1] builtin/pull.c: use config value of autostash

[Junio C Hamano]

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.

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

----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----
From: Philippe Blain <levraiphilippeblain@gmail.com>
Date: Tue, 4 Jan 2022 22:58:32 -0500
Subject: [PATCH] pull --rebase: honor rebase.autostash even 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" was
invoked with "--[no-]autostash" command line option, so that "git merge"
can honor merge.autostash in such a case).

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/
---
 builtin/pull.c          |  7 +++---
 t/t5521-pull-options.sh | 52 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 55 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/t5521-pull-options.sh b/t/t5521-pull-options.sh
index 66cfcb09c5..4046a74cad 100755
--- a/t/t5521-pull-options.sh
+++ b/t/t5521-pull-options.sh
@@ -252,4 +252,56 @@ test_expect_success 'git pull --no-verify --verify passed to merge' '
 	test_must_fail git -C dst pull --no-ff --no-verify --verify
 '
 
+test_expect_success 'git pull --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 &&
+	git -C dst pull --rebase --autostash >actual 2>&1 &&
+	grep -q "Fast-forward" actual &&
+	grep -q "Applied autostash." actual
+'
+
+test_expect_success 'git pull --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 'git pull --rebase --autostash succeeds on non-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 "src_content" file "src content\ncontent\n" &&
+	test_commit -C dst --append "dst_content" file "dst content" &&
+	echo "dirty" >>dst/file &&
+	git -C dst pull --rebase --autostash >actual 2>&1 &&
+	grep -q "Successfully rebased and updated refs/heads/main." actual &&
+	grep -q "Applied autostash." actual
+'
+
+test_expect_success 'git pull --rebase with rebase.autostash succeeds on non-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 "src_content" file "src content\ncontent\n" &&
+	test_commit -C dst --append "dst_content" file "dst content" &&
+	echo "dirty" >>dst/file &&
+	test_config -C dst rebase.autostash true &&
+	git -C dst pull --rebase >actual 2>&1 &&
+	grep -q "Successfully rebased and updated refs/heads/main." actual &&
+	grep -q "Applied autostash." actual
+'
+
 test_done
-- 
2.35.0-rc0-182-gd99ee8e0c1

Re: [PATCH 1/1] builtin/pull.c: use config value of autostash

[Philippe Blain]

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

Re: [PATCH 1/1] builtin/pull.c: use config value of autostash

[John Cai]

> On Jan 13, 2022, at 10:14 PM, Philippe Blain <levraiphilippeblain@gmail.com> wrote:
> 
> 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.

I agree-think Philippe’s implementation is a better solution.

I don’t have the bandwidth these couple of days to reroll the patch so if someone else can take over that would be great!

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

Re: [PATCH 1/1] builtin/pull.c: use config value of autostash

[Junio C Hamano]

Philippe Blain <levraiphilippeblain@gmail.com> writes:

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

Was about to say "Will replace the in-tree version with this. Thanks",
before I realized that your message is "text/plain; format=flawed" X-<.

I think I fixed it up correctly.  I'll pick this version up from the
list and replace what's in-tree with it.

Thanks.

----- >8 ---------- >8 ---------- >8 ---------- >8 ---------- >8 -----
From: Philippe Blain <levraiphilippeblain@gmail.com>
Date: Thu, 13 Jan 2022 22:14:51 -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>
Helped-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..081808009b 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 &&
-- 
2.35.0-rc0-182-g7444c100dc

Re: [PATCH 1/1] builtin/pull.c: use config value of autostash

[Philippe Blain]

Hi Junio,

Le 2022-01-14 à 14:40, Junio C Hamano a écrit :
> Philippe Blain <levraiphilippeblain@gmail.com> writes:
> 
>> 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.
> 
> Was about to say "Will replace the in-tree version with this. Thanks",
> before I realized that your message is "text/plain; format=flawed" X-<.

I'm sorry for that. I was in a rush and *tought* that I had Thunderbird configured
correctly so that pasting the patch would work out. It seems not. I'll
be more careful next time:)

> 
> I think I fixed it up correctly.  I'll pick this version up from the
> list and replace what's in-tree with it.
> 
> Thanks.

Thanks!

Philippe.

Re: [PATCH 1/1] builtin/pull.c: use config value of autostash

[Phillip Wood]

Hi John and Junio

On 04/01/2022 22:46, Junio C Hamano wrote:
> John Cai <johncai86@gmail.com> writes:
> 
>> diff --git a/builtin/pull.c b/builtin/pull.c
>> index 100cbf9fb8..fb700c2d39 100644
>> --- a/builtin/pull.c
>> +++ b/builtin/pull.c
>> @@ -86,7 +86,8 @@ static char *opt_ff;
>>   static char *opt_verify_signatures;
>>   static char *opt_verify;
>>   static int opt_autostash = -1;
>> -static int config_autostash;
>> +static int rebase_autostash = 0;
>> +static int cfg_rebase_autostash;
> 
> Do not explicitly initialize statics to '0' (or NULL for that matter).
> 
> But more importantly, I have a feeling that you are making a piece
> of code that is already hard to read impossible to follow by adding
> yet another variable.
> 
>> diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
>> index 66cfcb09c5..28f551db8e 100755
>> --- a/t/t5521-pull-options.sh
>> +++ b/t/t5521-pull-options.sh
>> @@ -251,5 +251,56 @@ test_expect_success 'git pull --no-verify --verify passed to merge' '
>>   	test_commit -C src two &&
>>   	test_must_fail git -C dst pull --no-ff --no-verify --verify
>>   '
>> +test_expect_success 'git pull --rebase --autostash succeeds on ff' '
> 
> Missing blank line between tests.
> 
> 
> I thought that the root cause of the problem is that run_merge() is
> called even when rebase was asked for (either via pull.rebase=true
> configuration or "pull --rebase" option), when the other side is a
> descendant of HEAD.  The basic idea behind that behaviour may be
> sound (i.e. if we do not have any of our own development on top of
> their history, rebase vs merge shouldn't make any difference while
> fast-forwarding HEAD to their history), except that rebase vs merge
> look at different configuration variables.
> 
> I wonder if the following two-liner patch is a simpler fix that is
> easier to understand.  run_merge() decides if we should pass the
> "--[no-]autostash" option based on the value of opt_autostash, and
> we know the value of rebase.autostash in config_autostash variable.
> 
> It appears to pass all four tests your patch adds.

I think this is a better approach - it's simpler and it is clear that we 
only use rebase.autostash when a rebase was requested.

Best Wishes

Phillip

>   builtin/pull.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git c/builtin/pull.c w/builtin/pull.c
> index 100cbf9fb8..d459a91a64 100644
> --- c/builtin/pull.c
> +++ w/builtin/pull.c
> @@ -1133,7 +1133,14 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>   			die(_("cannot rebase with locally recorded submodule modifications"));
>   
>   		if (can_ff) {
> -			/* we can fast-forward this without invoking rebase */
> +			/*
> +			 * We can fast-forward without invoking
> +			 * rebase, by calling run_merge().  But we
> +			 * have to allow rebase.autostash=true to kick
> +			 * in.
> +			 */
> +			if (opt_autostash < 0)
> +				opt_autostash = config_autostash;
>   			opt_ff = "--ff-only";
>   			ret = run_merge();
>   		} else {
> 

Re: [PATCH 1/1] builtin/pull.c: use config value of autostash

[Philippe Blain]

Hi John,

Le 2022-01-04 à 16:45, John Cai a écrit :
> A bug in pull.c causes merge and rebase functions to ignore
> rebase.autostash if it is only set in the config.

The reported bug only affects fast-forwards as far as I understand, so I
don't think "merge and rebase" is the best wording here. Also, 'functions' is
not super clear. The actual functions in the code are 'run_rebase'
and 'run_merge', if that is what you are referring to. If you
mean the different underlying "modes" of 'git pull', I'd phrase
it more like "the underlying 'merge' or 'rebase' invocations"
or something like that - but again, only the underlying 'git merge'
is affected, and only for fast-forwards.

> 
> There are a couple of different scenarios that we need to be mindful of:
> 1. --autostash passed in through command line
> $ git pull --autostash
> merge/rebase should get --autostashed passed through
> 
> 2. --rebase passed in, rebase.autostash set in config
> $ git config rebase.autostash true
> $ git pull --rebase
> 
> merge/rebase should get --autostash from config
> 
> 3. --no-autostash passed in
> $ git pull --no-autostash
> --no-autostash should be passed into merge/rebase
> 
> 4. rebase.autostash set but --rebase not used
> 
> $ git config rebase.autostash true
> $ git pull
> --autostash should not be passed into merge but not rebase

Usually we start the commit message by a description of the current behaviour,
so in the case of a bugfix like here, a description of the exact behaviour
that triggers the bug. As Tilman reported, this only affects fast-forwards,
so that should be mentioned in your commit message.
While what you wrote is not wrong per se (although I'm not sure what you meant
with point 4), almost all of the behaviour is
correct, apart from the (rebase + ff) case, so I would focus on that.

> 
> This change adjusts variable names to make it more clear which autostash
> setting it is modifying, and ensures --autostash is passed into the
> merge/rebase where appropriate.

As Junio already pointed out, I'm not sure the changes you propose
are really clearer... I agree that adding yet another variable is unneeded.

> 
> Reported-by: "Tilman Vogel" <tilman.vogel@web.de>
> Co-authored-by: "Philippe Blain" <levraiphilippeblain@gmail.com>

As I remarked in the other thread, I'd prefer if you remove that trailer.

> Signed-Off-by: "John Cai" <johncai86@gmail.com>
> ---
>   builtin/pull.c          | 15 ++++++------
>   t/t5521-pull-options.sh | 51 +++++++++++++++++++++++++++++++++++++++++

The existing tests for 'git pull --autostash' are in t5520, so I think it
might make more sense to add any new tests there instead of t5521.

> diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
> index 66cfcb09c5..28f551db8e 100755
> --- a/t/t5521-pull-options.sh
> +++ b/t/t5521-pull-options.sh
> @@ -251,5 +251,56 @@ test_expect_success 'git pull --no-verify --verify passed to merge' '
>   	test_commit -C src two &&
>   	test_must_fail git -C dst pull --no-ff --no-verify --verify
>   '
> +test_expect_success 'git pull --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 &&
> +	git -C dst pull --rebase --autostash >actual 2>&1 &&
> +	grep -q "Fast-forward" actual &&
> +	grep -q "Applied autostash." actual
> +'

This seems to test the same thing as  t5520's "--rebase --autostash fast forward",
so I don't think it's necessary to add this one.

> +
> +test_expect_success 'git pull --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
> +'

OK, this is the actual test that was failing.

> +
> +test_expect_success 'git pull --rebase --autostash succeeds on non-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 "src_content" file "src content\ncontent\n" &&
> +	test_commit -C dst --append "dst_content" file "dst content" &&
> +	echo "dirty" >>dst/file &&
> +	git -C dst pull --rebase --autostash >actual 2>&1 &&
> +	grep -q "Successfully rebased and updated refs/heads/main." actual &&
> +	grep -q "Applied autostash." actual
> +'

This seems to test the same thing as t5520's "pull --rebase --autostash & rebase.autostash unset"

> +
> +test_expect_success 'git pull --rebase with rebase.autostash succeeds on non-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 "src_content" file "src content\ncontent\n" &&
> +	test_commit -C dst --append "dst_content" file "dst content" &&
> +	echo "dirty" >>dst/file &&
> +	test_config -C dst rebase.autostash true &&
> +	git -C dst pull --rebase >actual 2>&1 &&
> +	grep -q "Successfully rebased and updated refs/heads/main." actual &&
> +	grep -q "Applied autostash." actual
> +'

This seems to test the same thing as t5520's
"pull --rebase succeeds with dirty working directory and rebase.autostash set".


Thanks for working on this ! :)

Philippe.

Re: [PATCH 1/1] builtin/pull.c: use config value of autostash

[Philippe Blain]

Hi again John,

Le 2022-01-04 à 22:40, Philippe Blain a écrit :
> Hi John,
> 
> Le 2022-01-04 à 16:45, John Cai a écrit :
> 
> Usually we start the commit message by a description of the current behaviour,
> so in the case of a bugfix like here, a description of the exact behaviour
> that triggers the bug. As Tilman reported, this only affects fast-forwards,
> so that should be mentioned in your commit message.
> While what you wrote is not wrong per se (although I'm not sure what you meant
> with point 4), almost all of the behaviour is
> correct, apart from the (rebase + ff) case, so I would focus on that.

I forgot to mention in my previous mail, but in the same vein, I think your
commit message title could reflect a little more the bug you are fixing, maybe
something like

pull: honor 'rebase.autostash' if rebase fast-forwards

or something similar.

Re: [PATCH 1/1] builtin/pull.c: use config value of autostash

[Johannes Schindelin]

Hi John,

On Tue, 4 Jan 2022, John Cai wrote:

> Reported-by: "Tilman Vogel" <tilman.vogel@web.de>
> Co-authored-by: "Philippe Blain" <levraiphilippeblain@gmail.com>
> Signed-Off-by: "John Cai" <johncai86@gmail.com>

We spell the 'o' in 'Signed-off-by' with a lower-case 'o'. That's what
`git commit -s` does automatically.

Was this the problem why you stopped using GitGitGadget? It would also
have helped you avoid the frowned-upon cover letter for single patch
contributions.

The entire point of GitGitGadget is to _not_ force contributors to know
about all these things.

Ciao,
Dscho

Re: [PATCH 0/1] Fix bug in pull --rebase not recognizing rebase.autostash

[Philippe Blain]

Hi John,

Le 2022-01-04 à 16:45, John Cai a écrit :
> NOTE: this is my first patch I'm submitting through git send-email. Please let
> me know if there is some convention that I'm missing. thank you in advance :)

Usually, for one-patch series as this one, it's preferred in this project to not
send a separate cover letter, but instead to include the cover letter material below
the three dash line. You can do that manually when using 'git send-email --annotate',
or you can use the 'git notes' command along with git format-patch's '--notes'
arguments to include it automatically.

Cheers,
Philippe.