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

From: Philippe Blain <levraiphilippeblain@gmail.com>
Date: Jan 5, 2022, 03:58 UTC
Message-ID: <4fe0e850-1c30-da70-5535-e2a4105ba734@gmail.com>
In-Reply-To: <xmqqbl0r9l0l.fsf@gitster.g>

Hi Junio,

Le 2022-01-04 à 17:46, Junio C Hamano a écrit :

Show 32 quoted lines

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