Re: [PATCH v3] builtin/pull.c: teach run_merge() to honor rebase.autostash config

From: Taylor Blau <me@ttaylorr.com>
Date: Jan 6, 2022, 17:14 UTC
Message-ID: <YdcjWG/67JlxTjfF@nand.local>
In-Reply-To: <YdcjCKTrK+W/FCw1@nand.local>

On Thu, Jan 06, 2022 at 12:12:40PM -0500, Taylor Blau wrote:

Show 23 quoted lines

> On Thu, Jan 06, 2022 at 10:02:26AM -0500, John Cai wrote:
> > diff --git a/builtin/pull.c b/builtin/pull.c
> > index 100cbf9fb8..8423e420ee 100644
> > --- a/builtin/pull.c
> > +++ b/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;
>
> This looks OK, and prefers the value of autostash given over options
> over the configured one. But it may be a little clearer to construct it
> that way explicitly (see the conditional "if (opt_rebase)" inside of
> cmd_pull()).

Oops, ignore this suggestion. run_merge() looks at the static variable opt_autostash, so you really do need to be manipulating it directly.

Nevermind: what you wrote here looks fine.

Thanks, Taylor