diff mbox series

[1/7] pull --rebase/remote rename: document and honor single-letter abbreviations rebase types

Message ID f9da9aac7edf6f682592592fe8f450a5801fb012.1579598053.git.bert.wesarg@googlemail.com (mailing list archive)
State New, archived
Headers show
Series remote rename: improve handling of configuration values | expand

Commit Message

Bert Wesarg Jan. 21, 2020, 9:24 a.m. UTC 
When 46af44b07d (pull --rebase=<type>: allow single-letter abbreviations
for the type, 2018-08-04) landed in Git, it had the side effect that
not only 'pull --rebase=<type>' accepted the single-letter abbreviations
but also the 'pull.rebase' and 'branch.<name>.rebase' configurations.

Secondly, 'git remote rename' did not honor these single-letter
abbreviations when reading the 'branch.*.rebase' configurations.

We now document the single-letter abbreviations and both code places
share a common function to parse the values of 'git pull --rebase=*',
'pull.rebase', and 'branches.*.rebase'.

The only functional change is the handling of the `branch_info::rebase`
value. Before it was an unsigned enum, thus the truth value could be
checked with `branch_info::rebase != 0`. But `enum rebase_type` is
signed, thus the truth value must now be checked with
`branch_info::rebase >= REBASE_TRUE`.

Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>

---
Cc: Johannes Schindelin <Johannes.Schindelin@gmx.de>

In case this is considered a BUG, then sharing the function is nevertheless
a good thing. The function could than learn a new flag, indicating whether
the single-letter abbreviations are accepted or not.
---
 Documentation/config/branch.txt |  7 ++++---
 Documentation/config/pull.txt   |  7 ++++---
 Makefile                        |  1 +
 builtin/pull.c                  | 29 ++++-------------------------
 builtin/remote.c                | 26 ++++++++------------------
 rebase.c                        | 24 ++++++++++++++++++++++++
 rebase.h                        | 15 +++++++++++++++
 7 files changed, 60 insertions(+), 49 deletions(-)
 create mode 100644 rebase.c
 create mode 100644 rebase.h

Comments

Junio C Hamano Jan. 21, 2020, 11:26 p.m. UTC | #1 
Bert Wesarg <bert.wesarg@googlemail.com> writes:

> When 46af44b07d (pull --rebase=<type>: allow single-letter abbreviations
> for the type, 2018-08-04) landed in Git, it had the side effect that
> not only 'pull --rebase=<type>' accepted the single-letter abbreviations
> but also the 'pull.rebase' and 'branch.<name>.rebase' configurations.
>
> Secondly, 'git remote rename' did not honor these single-letter
> abbreviations when reading the 'branch.*.rebase' configurations.

Hmph, do you mean s/Secondly/However/ instead?

> The only functional change is the handling of the `branch_info::rebase`
> value. Before it was an unsigned enum, thus the truth value could be
> checked with `branch_info::rebase != 0`. But `enum rebase_type` is
> signed, thus the truth value must now be checked with
> `branch_info::rebase >= REBASE_TRUE`.

I think there is another hidden one, but I do not know offhand the
implications of the change.  It could well be benign.

>  /**
>   * Parses the value of --rebase. If value is a false value, returns
>   * REBASE_FALSE. If value is a true value, returns REBASE_TRUE. If value is
> @@ -45,22 +37,9 @@ enum rebase_type {
>  static enum rebase_type parse_config_rebase(const char *key, const char *value,
>  		int fatal)
>  {
> -	int v = git_parse_maybe_bool(value);
> -
> -	if (!v)
> -		return REBASE_FALSE;
> -	else if (v > 0)
> -		return REBASE_TRUE;
> -	else if (!strcmp(value, "preserve") || !strcmp(value, "p"))
> -		return REBASE_PRESERVE;
> -	else if (!strcmp(value, "merges") || !strcmp(value, "m"))
> -		return REBASE_MERGES;
> -	else if (!strcmp(value, "interactive") || !strcmp(value, "i"))
> -		return REBASE_INTERACTIVE;
> -	/*
> -	 * Please update _git_config() in git-completion.bash when you
> -	 * add new rebase modes.
> -	 */

I see all of the above, including the "Please update" comment, has
become rebase_parse_value(), which is very good.

> diff --git a/builtin/remote.c b/builtin/remote.c
> index 96bbe828fe..2830c4ab33 100644
> --- a/builtin/remote.c
> +++ b/builtin/remote.c
> @@ -6,6 +6,7 @@
> ...
> -	enum {
> -		NO_REBASE, NORMAL_REBASE, INTERACTIVE_REBASE, REBASE_MERGES
> -	} rebase;
> +	enum rebase_type rebase;

Good to see the duplicate go.

> @@ -305,17 +304,8 @@ static int config_read_branches(const char *key, const char *value, void *cb)
>  				space = strchr(value, ' ');
>  			}
>  			string_list_append(&info->merge, xstrdup(value));
> -		} else {
> -			int v = git_parse_maybe_bool(value);
> -			if (v >= 0)
> -				info->rebase = v;
> -			else if (!strcmp(value, "preserve"))
> -				info->rebase = NORMAL_REBASE;
> -			else if (!strcmp(value, "merges"))
> -				info->rebase = REBASE_MERGES;
> -			else if (!strcmp(value, "interactive"))
> -				info->rebase = INTERACTIVE_REBASE;
> -		}
> +		} else
> +			info->rebase = rebase_parse_value(value);

Here, we never had info->rebase == REBASE_INVALID.  The field was
left intact when the configuration file had a rebase type that is
not known to this version of git.  Now it has become possible that
info->rebase to be REBASE_INVALID.  Would the code after this part
returns be prepared to handle it, and if so how?  At least I think
it deserves a comment here, or in rebase_parse_value(), to say (1)
that unknown rebase value is treated as false for most of the code
that do not need to differentiate between false and unknown, and (2)
that assigning a negative value to REBASE_INVALID and always
checking if the value is the same or greater than REBASE_TRUE helps
to maintain the convention.


> diff --git a/rebase.h b/rebase.h
> new file mode 100644
> index 0000000000..cc723d4748
> --- /dev/null
> +++ b/rebase.h
> @@ -0,0 +1,15 @@
> +#ifndef REBASE_H
> +#define REBASE_H
> +
> +enum rebase_type {
> +	REBASE_INVALID = -1,
> +	REBASE_FALSE = 0,
> +	REBASE_TRUE,
> +	REBASE_PRESERVE,
> +	REBASE_MERGES,
> +	REBASE_INTERACTIVE
> +};
> +
> +enum rebase_type rebase_parse_value(const char *value);
> +
> +#endif /* REBASE */
Bert Wesarg Jan. 22, 2020, 7:34 a.m. UTC | #2 
Dear Junio,

On Wed, Jan 22, 2020 at 12:26 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Bert Wesarg <bert.wesarg@googlemail.com> writes:
>
> > When 46af44b07d (pull --rebase=<type>: allow single-letter abbreviations
> > for the type, 2018-08-04) landed in Git, it had the side effect that
> > not only 'pull --rebase=<type>' accepted the single-letter abbreviations
> > but also the 'pull.rebase' and 'branch.<name>.rebase' configurations.
> >
> > Secondly, 'git remote rename' did not honor these single-letter
> > abbreviations when reading the 'branch.*.rebase' configurations.
>
> Hmph, do you mean s/Secondly/However/ instead?

thanks, that now reads smoothly.

> > @@ -305,17 +304,8 @@ static int config_read_branches(const char *key, const char *value, void *cb)
> >                               space = strchr(value, ' ');
> >                       }
> >                       string_list_append(&info->merge, xstrdup(value));
> > -             } else {
> > -                     int v = git_parse_maybe_bool(value);
> > -                     if (v >= 0)
> > -                             info->rebase = v;
> > -                     else if (!strcmp(value, "preserve"))
> > -                             info->rebase = NORMAL_REBASE;
> > -                     else if (!strcmp(value, "merges"))
> > -                             info->rebase = REBASE_MERGES;
> > -                     else if (!strcmp(value, "interactive"))
> > -                             info->rebase = INTERACTIVE_REBASE;
> > -             }
> > +             } else
> > +                     info->rebase = rebase_parse_value(value);
>
> Here, we never had info->rebase == REBASE_INVALID.  The field was
> left intact when the configuration file had a rebase type that is
> not known to this version of git.  Now it has become possible that
> info->rebase to be REBASE_INVALID.  Would the code after this part
> returns be prepared to handle it, and if so how?  At least I think
> it deserves a comment here, or in rebase_parse_value(), to say (1)
> that unknown rebase value is treated as false for most of the code
> that do not need to differentiate between false and unknown, and (2)
> that assigning a negative value to REBASE_INVALID and always
> checking if the value is the same or greater than REBASE_TRUE helps
> to maintain the convention.

Its true that we never had 'info->rebase == REBASE_INVALID', but the
previous code also considered unknown values as false. 'info' is
allocated with 'xcalloc', thus 'info->rebase' defaults to false. Thus
it remains false.

While my change may set 'info->rebase' implicitly to 'REBASE_INVALID'
I also changed all truth value checks to '>= REBASE_TRUE'. Therefore,
(and I must admit) incidentally, I did not introduced a function
change. Both versions handle unknown '.rebase' values as false.

If this is the expected behavior, I will add a comment to the line,
with that finding. If not, I will map 'REBASE_INVALID' to
'REBASE_TRUE' in that case.

Bert
Junio C Hamano Jan. 22, 2020, 7:43 p.m. UTC | #3 
Bert Wesarg <bert.wesarg@googlemail.com> writes:

> Dear Junio,
>
> On Wed, Jan 22, 2020 at 12:26 AM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Bert Wesarg <bert.wesarg@googlemail.com> writes:
>>
>> > When 46af44b07d (pull --rebase=<type>: allow single-letter abbreviations
>> > for the type, 2018-08-04) landed in Git, it had the side effect that
>> > not only 'pull --rebase=<type>' accepted the single-letter abbreviations
>> > but also the 'pull.rebase' and 'branch.<name>.rebase' configurations.
>> >
>> > Secondly, 'git remote rename' did not honor these single-letter
>> > abbreviations when reading the 'branch.*.rebase' configurations.
>>
>> Hmph, do you mean s/Secondly/However/ instead?
>
> thanks, that now reads smoothly.
>
>> > @@ -305,17 +304,8 @@ static int config_read_branches(const char *key, const char *value, void *cb)
>> >                               space = strchr(value, ' ');
>> >                       }
>> >                       string_list_append(&info->merge, xstrdup(value));
>> > -             } else {
>> > -                     int v = git_parse_maybe_bool(value);
>> > -                     if (v >= 0)
>> > -                             info->rebase = v;
>> > -                     else if (!strcmp(value, "preserve"))
>> > -                             info->rebase = NORMAL_REBASE;
>> > -                     else if (!strcmp(value, "merges"))
>> > -                             info->rebase = REBASE_MERGES;
>> > -                     else if (!strcmp(value, "interactive"))
>> > -                             info->rebase = INTERACTIVE_REBASE;
>> > -             }
>> > +             } else
>> > +                     info->rebase = rebase_parse_value(value);
>>
>> Here, we never had info->rebase == REBASE_INVALID.  The field was
>> left intact when the configuration file had a rebase type that is
>> not known to this version of git.  Now it has become possible that
>> info->rebase to be REBASE_INVALID.  Would the code after this part
>> returns be prepared to handle it, and if so how?  At least I think
>> it deserves a comment here, or in rebase_parse_value(), to say (1)
>> that unknown rebase value is treated as false for most of the code
>> that do not need to differentiate between false and unknown, and (2)
>> that assigning a negative value to REBASE_INVALID and always
>> checking if the value is the same or greater than REBASE_TRUE helps
>> to maintain the convention.
>
> Its true that we never had 'info->rebase == REBASE_INVALID', but the
> previous code also considered unknown values as false. 'info' is
> allocated with 'xcalloc', thus 'info->rebase' defaults to false. Thus
> it remains false.

Yes, that is why I was not opposed to the new code.  It was just
that it was not clear, without some comments I suggested in the
latter half of my paragraph you responded above, why it is correct
to unconditionally assign to info->rebase and the code the control
reaches after this part gets executed does not need any adjustment
and simply "works".

Thinking about it again, I think the two points I thought need
highlighting in the above belong to the in-code comment for the new
helper rebase_parse_value().

    *** in rebase.h ***
    enum rebase_type {
            REBASE_INVALID = -1,
            REBASE_FALSE = 0,
            REBASE_TRUE,
            REBASE_PRESERVE,
            REBASE_MERGES,
            REBASE_INTERACTIVE
    };

    /*
     * Parses textual value for pull.rebase, branch.<name>.rebase, etc.
     * Unrecognised value yields REBASE_INVALID, which traditionally is
     * treated the same way as REBASE_FALSE.
     *
     * The callers that care if (any) rebase is requested should say
     *   if (REBASE_TRUE <= rebase_parse_value(string))
     *
     * The callers that want to differenciate an unrecognised value and
     * false can do so by treating _INVALID and _FALSE differently.
     */
    enum rebase_type rebase_parse_value(const char *value);

or something like that, perhaps.
diff mbox series

Patch

diff --git a/Documentation/config/branch.txt b/Documentation/config/branch.txt
index a592d522a7..cc5f3249fc 100644
--- a/Documentation/config/branch.txt
+++ b/Documentation/config/branch.txt
@@ -81,15 +81,16 @@  branch.<name>.rebase::
 	"git pull" is run. See "pull.rebase" for doing this in a non
 	branch-specific manner.
 +
-When `merges`, pass the `--rebase-merges` option to 'git rebase'
+When `merges` (or just 'm'), pass the `--rebase-merges` option to 'git rebase'
 so that the local merge commits are included in the rebase (see
 linkgit:git-rebase[1] for details).
 +
-When `preserve` (deprecated in favor of `merges`), also pass
+When `preserve` (or just 'p', deprecated in favor of `merges`), also pass
 `--preserve-merges` along to 'git rebase' so that locally committed merge
 commits will not be flattened by running 'git pull'.
 +
-When the value is `interactive`, the rebase is run in interactive mode.
+When the value is `interactive` (or just 'i'), the rebase is run in interactive
+mode.
 +
 *NOTE*: this is a possibly dangerous operation; do *not* use
 it unless you understand the implications (see linkgit:git-rebase[1]
diff --git a/Documentation/config/pull.txt b/Documentation/config/pull.txt
index b87cab31b3..5404830609 100644
--- a/Documentation/config/pull.txt
+++ b/Documentation/config/pull.txt
@@ -14,15 +14,16 @@  pull.rebase::
 	pull" is run. See "branch.<name>.rebase" for setting this on a
 	per-branch basis.
 +
-When `merges`, pass the `--rebase-merges` option to 'git rebase'
+When `merges` (or just 'm'), pass the `--rebase-merges` option to 'git rebase'
 so that the local merge commits are included in the rebase (see
 linkgit:git-rebase[1] for details).
 +
-When `preserve` (deprecated in favor of `merges`), also pass
+When `preserve` (or just 'p', deprecated in favor of `merges`), also pass
 `--preserve-merges` along to 'git rebase' so that locally committed merge
 commits will not be flattened by running 'git pull'.
 +
-When the value is `interactive`, the rebase is run in interactive mode.
+When the value is `interactive` (or just 'i'), the rebase is run in interactive
+mode.
 +
 *NOTE*: this is a possibly dangerous operation; do *not* use
 it unless you understand the implications (see linkgit:git-rebase[1]
diff --git a/Makefile b/Makefile
index 09f98b777c..96ced97bff 100644
--- a/Makefile
+++ b/Makefile
@@ -954,6 +954,7 @@  LIB_OBJS += quote.o
 LIB_OBJS += range-diff.o
 LIB_OBJS += reachable.o
 LIB_OBJS += read-cache.o
+LIB_OBJS += rebase.o
 LIB_OBJS += rebase-interactive.o
 LIB_OBJS += reflog-walk.o
 LIB_OBJS += refs.o
diff --git a/builtin/pull.c b/builtin/pull.c
index d25ff13a60..888181c07c 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -15,6 +15,7 @@ 
 #include "sha1-array.h"
 #include "remote.h"
 #include "dir.h"
+#include "rebase.h"
 #include "refs.h"
 #include "refspec.h"
 #include "revision.h"
@@ -26,15 +27,6 @@ 
 #include "commit-reach.h"
 #include "sequencer.h"
 
-enum rebase_type {
-	REBASE_INVALID = -1,
-	REBASE_FALSE = 0,
-	REBASE_TRUE,
-	REBASE_PRESERVE,
-	REBASE_MERGES,
-	REBASE_INTERACTIVE
-};
-
 /**
  * Parses the value of --rebase. If value is a false value, returns
  * REBASE_FALSE. If value is a true value, returns REBASE_TRUE. If value is
@@ -45,22 +37,9 @@  enum rebase_type {
 static enum rebase_type parse_config_rebase(const char *key, const char *value,
 		int fatal)
 {
-	int v = git_parse_maybe_bool(value);
-
-	if (!v)
-		return REBASE_FALSE;
-	else if (v > 0)
-		return REBASE_TRUE;
-	else if (!strcmp(value, "preserve") || !strcmp(value, "p"))
-		return REBASE_PRESERVE;
-	else if (!strcmp(value, "merges") || !strcmp(value, "m"))
-		return REBASE_MERGES;
-	else if (!strcmp(value, "interactive") || !strcmp(value, "i"))
-		return REBASE_INTERACTIVE;
-	/*
-	 * Please update _git_config() in git-completion.bash when you
-	 * add new rebase modes.
-	 */
+	enum rebase_type v = rebase_parse_value(value);
+	if (v != REBASE_INVALID)
+		return v;
 
 	if (fatal)
 		die(_("Invalid value for %s: %s"), key, value);
diff --git a/builtin/remote.c b/builtin/remote.c
index 96bbe828fe..2830c4ab33 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -6,6 +6,7 @@ 
 #include "string-list.h"
 #include "strbuf.h"
 #include "run-command.h"
+#include "rebase.h"
 #include "refs.h"
 #include "refspec.h"
 #include "object-store.h"
@@ -248,9 +249,7 @@  static int add(int argc, const char **argv)
 struct branch_info {
 	char *remote_name;
 	struct string_list merge;
-	enum {
-		NO_REBASE, NORMAL_REBASE, INTERACTIVE_REBASE, REBASE_MERGES
-	} rebase;
+	enum rebase_type rebase;
 };
 
 static struct string_list branch_list = STRING_LIST_INIT_NODUP;
@@ -305,17 +304,8 @@  static int config_read_branches(const char *key, const char *value, void *cb)
 				space = strchr(value, ' ');
 			}
 			string_list_append(&info->merge, xstrdup(value));
-		} else {
-			int v = git_parse_maybe_bool(value);
-			if (v >= 0)
-				info->rebase = v;
-			else if (!strcmp(value, "preserve"))
-				info->rebase = NORMAL_REBASE;
-			else if (!strcmp(value, "merges"))
-				info->rebase = REBASE_MERGES;
-			else if (!strcmp(value, "interactive"))
-				info->rebase = INTERACTIVE_REBASE;
-		}
+		} else
+			info->rebase = rebase_parse_value(value);
 	}
 	return 0;
 }
@@ -943,7 +933,7 @@  static int add_local_to_show_info(struct string_list_item *branch_item, void *cb
 		return 0;
 	if ((n = strlen(branch_item->string)) > show_info->width)
 		show_info->width = n;
-	if (branch_info->rebase)
+	if (branch_info->rebase >= REBASE_TRUE)
 		show_info->any_rebase = 1;
 
 	item = string_list_insert(show_info->list, branch_item->string);
@@ -960,16 +950,16 @@  static int show_local_info_item(struct string_list_item *item, void *cb_data)
 	int width = show_info->width + 4;
 	int i;
 
-	if (branch_info->rebase && branch_info->merge.nr > 1) {
+	if (branch_info->rebase >= REBASE_TRUE && branch_info->merge.nr > 1) {
 		error(_("invalid branch.%s.merge; cannot rebase onto > 1 branch"),
 			item->string);
 		return 0;
 	}
 
 	printf("    %-*s ", show_info->width, item->string);
-	if (branch_info->rebase) {
+	if (branch_info->rebase >= REBASE_TRUE) {
 		const char *msg;
-		if (branch_info->rebase == INTERACTIVE_REBASE)
+		if (branch_info->rebase == REBASE_INTERACTIVE)
 			msg = _("rebases interactively onto remote %s");
 		else if (branch_info->rebase == REBASE_MERGES)
 			msg = _("rebases interactively (with merges) onto "
diff --git a/rebase.c b/rebase.c
new file mode 100644
index 0000000000..a9ab27205a
--- /dev/null
+++ b/rebase.c
@@ -0,0 +1,24 @@ 
+#include "rebase.h"
+#include "config.h"
+
+enum rebase_type rebase_parse_value(const char *value)
+{
+	int v = git_parse_maybe_bool(value);
+
+	if (!v)
+		return REBASE_FALSE;
+	else if (v > 0)
+		return REBASE_TRUE;
+	else if (!strcmp(value, "preserve") || !strcmp(value, "p"))
+		return REBASE_PRESERVE;
+	else if (!strcmp(value, "merges") || !strcmp(value, "m"))
+		return REBASE_MERGES;
+	else if (!strcmp(value, "interactive") || !strcmp(value, "i"))
+		return REBASE_INTERACTIVE;
+	/*
+	 * Please update _git_config() in git-completion.bash when you
+	 * add new rebase modes.
+	 */
+
+	return REBASE_INVALID;
+}
diff --git a/rebase.h b/rebase.h
new file mode 100644
index 0000000000..cc723d4748
--- /dev/null
+++ b/rebase.h
@@ -0,0 +1,15 @@ 
+#ifndef REBASE_H
+#define REBASE_H
+
+enum rebase_type {
+	REBASE_INVALID = -1,
+	REBASE_FALSE = 0,
+	REBASE_TRUE,
+	REBASE_PRESERVE,
+	REBASE_MERGES,
+	REBASE_INTERACTIVE
+};
+
+enum rebase_type rebase_parse_value(const char *value);
+
+#endif /* REBASE */