Re: [PATCH v5] pull --rebase: add --[no-]autostash flag

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 7, 2016, 23:01 UTC
Message-ID: <xmqqy49tnb8x.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1457354245-26871-1-git-send-email-mehul.jain2029@gmail.com>

Mehul Jain <mehul.jain2029@gmail.com> writes:

Show 9 quoted lines

> If rebase.autoStash configuration variable is set, there is no way to
> override it for "git pull --rebase" from the command line.
>
> Teach "git pull --rebase" the --[no-]autostash command line flag which
> overrides the current value of rebase.autoStash, if set. As "git rebase"
> understands the --[no-]autostash option, it's just a matter of passing
> the option to underlying "git rebase" when "git pull --rebase" is called.
>
> Also introduce a callback function to read config variables.

I was expecting you not to do this (it does not have to be in the scope of this topic), but if you do so, it would be preferrable to do it as a preparatory clean-up patch--the existing code already makes a separate get_bool() call to read a single config while it uses git_config(git_default_config) to read the other variables, and introduction of git_pull_config() is a clean-up that is independent of your new feature.

And then on top of that cleaned-up codebase, add this new feature.

Show 16 quoted lines

> diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
> index a62a2a6..393ac7d 100644
> --- a/Documentation/git-pull.txt
> +++ b/Documentation/git-pull.txt
> @@ -128,6 +128,15 @@ unless you have read linkgit:git-rebase[1] carefully.
>  --no-rebase::
>  	Override earlier --rebase.
>  
> +--autostash::
> +--no-autostash::
> +	Before starting rebase, stash local modifications away (see
> +	linkgit:git-stash.txt[1]) if needed, and apply the stash when
> +	done (this option is only valid when '--rebase' is used).
> ++
> +'--no-autostash' is useful to override the 'rebase.autoStash'
> +configuration variable (see linkgit:git-config[1]).

Don't we use `typewriter font` when referring to literal command, option and variable names in the documentation? The description for `--rebase` that comes immediately before this hunk seems to support that observation.

Show 9 quoted lines

> @@ -306,6 +310,18 @@ static enum rebase_type config_get_rebase(void)
>  }
>  
>  /**
> + * Read config variables.
> + */
> +static int git_pull_config(const char *var, const char *value, void *cb)
> +{
> +	if (!strcmp(var,"rebase.autostash")) {

Style: SP after a comma.  There may be the same problem elsewhere on
the new lines, but I won't point out all of them.

Other than that, looking better.

Thanks.