| Message ID | c48fbf984ea42e7c13d56db015dc63c2495f5f5f.1660831231.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Make rebase.autostash default | expand |
On 2022-08-18 at 14:00:30, Sergio via GitGitGadget wrote: > From: Sergio <sergeikrivonos@gmail.com> > > Signed-off-by: Sergio <sergeikrivonos@gmail.com> Typically you'll want to explain in the commit message why this is a valuable change. For example, I don't have this option set and don't use it, and I always stash my changes manually before rebasing. You should tell me why the user will benefit from this setting defaulting to enabled, since I personally don't see a need for it. You may also want to discuss why any pitfalls of stash, such as unadded changes being added after a stash pop, are not problematic here and why this behaviour won't be more surprising or annoying to experienced users who are used to seeing an error message instead. This isn't to say that the change is bad or we shouldn't accept it, only that I (and others) need help understanding why it's a good change to make.
"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".
diff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt index f19bd0e0407..bc952327140 100644 --- a/Documentation/config/rebase.txt +++ b/Documentation/config/rebase.txt @@ -19,7 +19,7 @@ rebase.autoStash:: successful rebase might result in non-trivial conflicts. This option can be overridden by the `--no-autostash` and `--autostash` options of linkgit:git-rebase[1]. - Defaults to false. + Defaults to true. rebase.updateRefs:: If set to true enable `--update-refs` option by default. 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); return 0; } else if (!strcmp(var, "submodule.recurse")) { recurse_submodules = git_config_bool(var, value) ? 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; +} + int git_config_string(const char **dest, const char *var, const char *value) { if (!value) diff --git a/config.h b/config.h index ca994d77147..d236bb0a326 100644 --- a/config.h +++ b/config.h @@ -242,6 +242,14 @@ int git_config_bool_or_int(const char *, const char *, int *); */ int git_config_bool(const char *, const char *); +/** + * Parse a string into a boolean value, respecting keywords like "true" and + * "false". Integer values are converted into true/false values (when they + * are non-zero or zero, respectively). Other values results in default. If + * parsing is successful, the return value is the result. + */ +int git_config_bool_or_default(const char *, const char *, int); + /** * Allocates and copies the value string into the `dest` parameter; if no * string is given, prints an error message and returns -1.