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

Hi John and Junio

On 04/01/2022 22:46, Junio C Hamano wrote:

John Cai [off-list ref] writes:

quoted

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.

quoted

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