Re: [PATCH 1/2] Make rebase.autostash default

[Junio C Hamano <gitster@pobox.com>]

"Sergio via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Sergio <sergeikrivonos@gmail.com>

Neither the cover letter nor the proposed log message even attempt
to justify why this is a good change.

Probably that is because it is not justifiable.  I do not think it
is a good idea to change the default, either.

> diff --git a/builtin/pull.c b/builtin/pull.c
> index 403a24d7ca6..333d6a232a7 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -362,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);
> +		config_autostash = git_config_bool_or_default(var, value, 1);

This is wrong.  What this says is "if the user has rebase.autostash,
attempt to interpret its value as a Boolean, and store it in this
variable.  If the value cannot be read as a Boolean, pretend as if
true was given".

That does not set the default to a configuration variable.  The
default is the value used when the user does *NOT* specify
rebase.autostash anywhere, but anything the code does inside the
block guarded by that strcmp() cannot affect that case.

If it were a good idea to make the variable default to true, the
place to do so would probably be

        diff --git i/builtin/pull.c w/builtin/pull.c
        index 403a24d7ca..0bb8421dfc 100644
        --- i/builtin/pull.c
        +++ w/builtin/pull.c
        @@ -87,7 +87,7 @@ static char *opt_ff;
         static char *opt_verify_signatures;
         static char *opt_verify;
         static int opt_autostash = -1;
        -static int config_autostash;
        +static int config_autostash = 1; /* default to true */
         static int check_trust_level = 1;
         static struct strvec opt_strategies = STRVEC_INIT;
         static struct strvec opt_strategy_opts = STRVEC_INIT;

> diff --git a/config.c b/config.c
> index e8ebef77d5c..c4f6da3547e 100644
> --- a/config.c
> +++ b/config.c
> @@ -1437,6 +1437,14 @@ int git_config_bool(const char *name, const char *value)
>  	return v;
>  }
>  
> +int git_config_bool_or_default(const char *name, const char *value, int default_value)
> +{
> +	int v = git_parse_maybe_bool(value);
> +	if (v < 0)
> +		v = default_value;
> +	return v;
> +}

And this is not a useful helper function.  At least, this is not
useful for this particular case.  We have tristate Booleans that
take yes/no/auto, and 

	git_config_bool_or_default(name, value, 2);

can take "name.value=auto" and turn it into 2 (instead of 0=no
1=yes), but because the helper takes *any* garbage that is not a
Boolean and gives the same default_value, the value does not have to
be "auto" here, which makes the helper pretty much useless.

The patch is incomplete.  It only changes "git pull" but does not
do anything to "git rebase".