* [PATCH 1/7] pull --rebase/remote rename: document and honor single-letter abbreviations rebase types
2020-01-21 9:24 ` [PATCH 0/7] remote rename: improve handling of configuration values Bert Wesarg
@ 2020-01-21 9:24 ` Bert Wesarg
2020-01-21 23:26 ` Junio C Hamano
2020-01-21 9:24 ` [PATCH 2/7] remote: clean-up by returning early to avoid one indentation Bert Wesarg
` (6 subsequent siblings)
7 siblings, 1 reply; 33+ messages in thread
From: Bert Wesarg @ 2020-01-21 9:24 UTC (permalink / raw)
To: git; +Cc: Bert Wesarg, Johannes Schindelin
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
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 */
--
2.24.1.497.g9abd7b20b4.dirty
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH 1/7] pull --rebase/remote rename: document and honor single-letter abbreviations rebase types
2020-01-21 9:24 ` [PATCH 1/7] pull --rebase/remote rename: document and honor single-letter abbreviations rebase types Bert Wesarg
@ 2020-01-21 23:26 ` Junio C Hamano
2020-01-22 7:34 ` Bert Wesarg
0 siblings, 1 reply; 33+ messages in thread
From: Junio C Hamano @ 2020-01-21 23:26 UTC (permalink / raw)
To: Bert Wesarg; +Cc: git, Johannes Schindelin
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 */
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH 1/7] pull --rebase/remote rename: document and honor single-letter abbreviations rebase types
2020-01-21 23:26 ` Junio C Hamano
@ 2020-01-22 7:34 ` Bert Wesarg
2020-01-22 19:43 ` Junio C Hamano
0 siblings, 1 reply; 33+ messages in thread
From: Bert Wesarg @ 2020-01-22 7:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List, Johannes Schindelin
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
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH 1/7] pull --rebase/remote rename: document and honor single-letter abbreviations rebase types
2020-01-22 7:34 ` Bert Wesarg
@ 2020-01-22 19:43 ` Junio C Hamano
0 siblings, 0 replies; 33+ messages in thread
From: Junio C Hamano @ 2020-01-22 19:43 UTC (permalink / raw)
To: Bert Wesarg; +Cc: Git Mailing List, Johannes Schindelin
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.
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 2/7] remote: clean-up by returning early to avoid one indentation
2020-01-21 9:24 ` [PATCH 0/7] remote rename: improve handling of configuration values Bert Wesarg
2020-01-21 9:24 ` [PATCH 1/7] pull --rebase/remote rename: document and honor single-letter abbreviations rebase types Bert Wesarg
@ 2020-01-21 9:24 ` Bert Wesarg
2020-01-23 23:02 ` Junio C Hamano
2020-01-21 9:24 ` [PATCH 3/7] remote: clean-up config callback Bert Wesarg
` (5 subsequent siblings)
7 siblings, 1 reply; 33+ messages in thread
From: Bert Wesarg @ 2020-01-21 9:24 UTC (permalink / raw)
To: git; +Cc: Bert Wesarg, Junio C Hamano
Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
---
Cc: Junio C Hamano <gitster@pobox.com>
---
builtin/remote.c | 86 +++++++++++++++++++++++++-----------------------
1 file changed, 44 insertions(+), 42 deletions(-)
diff --git a/builtin/remote.c b/builtin/remote.c
index 2830c4ab33..a8bdaca4f4 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -263,50 +263,52 @@ static const char *abbrev_ref(const char *name, const char *prefix)
static int config_read_branches(const char *key, const char *value, void *cb)
{
- if (starts_with(key, "branch.")) {
- const char *orig_key = key;
- char *name;
- struct string_list_item *item;
- struct branch_info *info;
- enum { REMOTE, MERGE, REBASE } type;
- size_t key_len;
-
- key += 7;
- if (strip_suffix(key, ".remote", &key_len)) {
- name = xmemdupz(key, key_len);
- type = REMOTE;
- } else if (strip_suffix(key, ".merge", &key_len)) {
- name = xmemdupz(key, key_len);
- type = MERGE;
- } else if (strip_suffix(key, ".rebase", &key_len)) {
- name = xmemdupz(key, key_len);
- type = REBASE;
- } else
- return 0;
+ if (!starts_with(key, "branch."))
+ return 0;
- item = string_list_insert(&branch_list, name);
+ const char *orig_key = key;
+ char *name;
+ struct string_list_item *item;
+ struct branch_info *info;
+ enum { REMOTE, MERGE, REBASE } type;
+ size_t key_len;
+
+ key += 7;
+ if (strip_suffix(key, ".remote", &key_len)) {
+ name = xmemdupz(key, key_len);
+ type = REMOTE;
+ } else if (strip_suffix(key, ".merge", &key_len)) {
+ name = xmemdupz(key, key_len);
+ type = MERGE;
+ } else if (strip_suffix(key, ".rebase", &key_len)) {
+ name = xmemdupz(key, key_len);
+ type = REBASE;
+ } else
+ return 0;
+
+ item = string_list_insert(&branch_list, name);
+
+ if (!item->util)
+ item->util = xcalloc(1, sizeof(struct branch_info));
+ info = item->util;
+ if (type == REMOTE) {
+ if (info->remote_name)
+ warning(_("more than one %s"), orig_key);
+ info->remote_name = xstrdup(value);
+ } else if (type == MERGE) {
+ char *space = strchr(value, ' ');
+ value = abbrev_branch(value);
+ while (space) {
+ char *merge;
+ merge = xstrndup(value, space - value);
+ string_list_append(&info->merge, merge);
+ value = abbrev_branch(space + 1);
+ space = strchr(value, ' ');
+ }
+ string_list_append(&info->merge, xstrdup(value));
+ } else
+ info->rebase = rebase_parse_value(value);
- if (!item->util)
- item->util = xcalloc(1, sizeof(struct branch_info));
- info = item->util;
- if (type == REMOTE) {
- if (info->remote_name)
- warning(_("more than one %s"), orig_key);
- info->remote_name = xstrdup(value);
- } else if (type == MERGE) {
- char *space = strchr(value, ' ');
- value = abbrev_branch(value);
- while (space) {
- char *merge;
- merge = xstrndup(value, space - value);
- string_list_append(&info->merge, merge);
- value = abbrev_branch(space + 1);
- space = strchr(value, ' ');
- }
- string_list_append(&info->merge, xstrdup(value));
- } else
- info->rebase = rebase_parse_value(value);
- }
return 0;
}
--
2.24.1.497.g9abd7b20b4.dirty
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH 2/7] remote: clean-up by returning early to avoid one indentation
2020-01-21 9:24 ` [PATCH 2/7] remote: clean-up by returning early to avoid one indentation Bert Wesarg
@ 2020-01-23 23:02 ` Junio C Hamano
0 siblings, 0 replies; 33+ messages in thread
From: Junio C Hamano @ 2020-01-23 23:02 UTC (permalink / raw)
To: Bert Wesarg; +Cc: git
Bert Wesarg <bert.wesarg@googlemail.com> writes:
> Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
>
> ---
> Cc: Junio C Hamano <gitster@pobox.com>
> ---
> builtin/remote.c | 86 +++++++++++++++++++++++++-----------------------
> 1 file changed, 44 insertions(+), 42 deletions(-)
>
> diff --git a/builtin/remote.c b/builtin/remote.c
> index 2830c4ab33..a8bdaca4f4 100644
> --- a/builtin/remote.c
> +++ b/builtin/remote.c
> @@ -263,50 +263,52 @@ static const char *abbrev_ref(const char *name, const char *prefix)
>
> static int config_read_branches(const char *key, const char *value, void *cb)
> {
> - if (starts_with(key, "branch.")) {
> - const char *orig_key = key;
> - char *name;
> - struct string_list_item *item;
> - struct branch_info *info;
> - enum { REMOTE, MERGE, REBASE } type;
> - size_t key_len;
> -
> - key += 7;
> - if (strip_suffix(key, ".remote", &key_len)) {
> - name = xmemdupz(key, key_len);
> - type = REMOTE;
> - } else if (strip_suffix(key, ".merge", &key_len)) {
> - name = xmemdupz(key, key_len);
> - type = MERGE;
> - } else if (strip_suffix(key, ".rebase", &key_len)) {
> - name = xmemdupz(key, key_len);
> - type = REBASE;
> - } else
> - return 0;
> + if (!starts_with(key, "branch."))
> + return 0;
That's way too early. We must have all decl/defn before the first
statement (see Documentation/CodingGuidelines).
> - item = string_list_insert(&branch_list, name);
> + const char *orig_key = key;
> + char *name;
> + struct string_list_item *item;
> + struct branch_info *info;
> + enum { REMOTE, MERGE, REBASE } type;
> + size_t key_len;
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 3/7] remote: clean-up config callback
2020-01-21 9:24 ` [PATCH 0/7] remote rename: improve handling of configuration values Bert Wesarg
2020-01-21 9:24 ` [PATCH 1/7] pull --rebase/remote rename: document and honor single-letter abbreviations rebase types Bert Wesarg
2020-01-21 9:24 ` [PATCH 2/7] remote: clean-up by returning early to avoid one indentation Bert Wesarg
@ 2020-01-21 9:24 ` Bert Wesarg
2020-01-21 9:24 ` [PATCH v3 4/7] remote rename: rename branch.<name>.pushRemote config values too Bert Wesarg
` (4 subsequent siblings)
7 siblings, 0 replies; 33+ messages in thread
From: Bert Wesarg @ 2020-01-21 9:24 UTC (permalink / raw)
To: git; +Cc: Bert Wesarg, Junio C Hamano, Johannes Schindelin
Some minor clean-ups in function `config_read_branches`:
* remove hardcoded length in `key += 7`
* call `xmemdupz` only once
* use a switch to handle the configuration type and add a `BUG()`
Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
---
Cc: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/remote.c | 28 +++++++++++++++++-----------
1 file changed, 17 insertions(+), 11 deletions(-)
diff --git a/builtin/remote.c b/builtin/remote.c
index a8bdaca4f4..9466e32b3d 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -273,29 +273,29 @@ static int config_read_branches(const char *key, const char *value, void *cb)
enum { REMOTE, MERGE, REBASE } type;
size_t key_len;
- key += 7;
- if (strip_suffix(key, ".remote", &key_len)) {
- name = xmemdupz(key, key_len);
+ key += strlen("branch.");
+ if (strip_suffix(key, ".remote", &key_len))
type = REMOTE;
- } else if (strip_suffix(key, ".merge", &key_len)) {
- name = xmemdupz(key, key_len);
+ else if (strip_suffix(key, ".merge", &key_len))
type = MERGE;
- } else if (strip_suffix(key, ".rebase", &key_len)) {
- name = xmemdupz(key, key_len);
+ else if (strip_suffix(key, ".rebase", &key_len))
type = REBASE;
- } else
+ else
return 0;
+ name = xmemdupz(key, key_len);
item = string_list_insert(&branch_list, name);
if (!item->util)
item->util = xcalloc(1, sizeof(struct branch_info));
info = item->util;
- if (type == REMOTE) {
+ switch (type) {
+ case REMOTE:
if (info->remote_name)
warning(_("more than one %s"), orig_key);
info->remote_name = xstrdup(value);
- } else if (type == MERGE) {
+ break;
+ case MERGE: {
char *space = strchr(value, ' ');
value = abbrev_branch(value);
while (space) {
@@ -306,8 +306,14 @@ static int config_read_branches(const char *key, const char *value, void *cb)
space = strchr(value, ' ');
}
string_list_append(&info->merge, xstrdup(value));
- } else
+ break;
+ }
+ case REBASE:
info->rebase = rebase_parse_value(value);
+ break;
+ default:
+ BUG("unexpected type=%d", type);
+ }
return 0;
}
--
2.24.1.497.g9abd7b20b4.dirty
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH v3 4/7] remote rename: rename branch.<name>.pushRemote config values too
2020-01-21 9:24 ` [PATCH 0/7] remote rename: improve handling of configuration values Bert Wesarg
` (2 preceding siblings ...)
2020-01-21 9:24 ` [PATCH 3/7] remote: clean-up config callback Bert Wesarg
@ 2020-01-21 9:24 ` Bert Wesarg
2020-01-21 9:24 ` [PATCH 5/7] [RFC] config: make `scope_name` global as `config_scope_name` Bert Wesarg
` (3 subsequent siblings)
7 siblings, 0 replies; 33+ messages in thread
From: Bert Wesarg @ 2020-01-21 9:24 UTC (permalink / raw)
To: git; +Cc: Bert Wesarg, Junio C Hamano, Johannes Schindelin
When renaming a remote with
git remote rename X Y
Git already renames any config values from
branch.<name>.remote = X
to
branch.<name>.remote = Y
As branch.<name>.pushRemote also names a remote, it now also renames
these config values from
branch.<name>.pushRemote = X
to
branch.<name>.pushRemote = Y
Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
---
Cc: Junio C Hamano <gitster@pobox.com>
Cc: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/remote.c | 15 ++++++++++++++-
t/t5505-remote.sh | 4 +++-
2 files changed, 17 insertions(+), 2 deletions(-)
diff --git a/builtin/remote.c b/builtin/remote.c
index 9466e32b3d..0cb930fe00 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -250,6 +250,7 @@ struct branch_info {
char *remote_name;
struct string_list merge;
enum rebase_type rebase;
+ char *push_remote_name;
};
static struct string_list branch_list = STRING_LIST_INIT_NODUP;
@@ -270,7 +271,7 @@ static int config_read_branches(const char *key, const char *value, void *cb)
char *name;
struct string_list_item *item;
struct branch_info *info;
- enum { REMOTE, MERGE, REBASE } type;
+ enum { REMOTE, MERGE, REBASE, PUSH_REMOTE } type;
size_t key_len;
key += strlen("branch.");
@@ -280,6 +281,8 @@ static int config_read_branches(const char *key, const char *value, void *cb)
type = MERGE;
else if (strip_suffix(key, ".rebase", &key_len))
type = REBASE;
+ if (strip_suffix(key, ".pushremote", &key_len))
+ type = PUSH_REMOTE;
else
return 0;
name = xmemdupz(key, key_len);
@@ -311,6 +314,11 @@ static int config_read_branches(const char *key, const char *value, void *cb)
case REBASE:
info->rebase = rebase_parse_value(value);
break;
+ case PUSH_REMOTE:
+ if (info->push_remote_name)
+ warning(_("more than one %s"), orig_key);
+ info->push_remote_name = xstrdup(value);
+ break;
default:
BUG("unexpected type=%d", type);
}
@@ -678,6 +686,11 @@ static int mv(int argc, const char **argv)
strbuf_addf(&buf, "branch.%s.remote", item->string);
git_config_set(buf.buf, rename.new_name);
}
+ if (info->push_remote_name && !strcmp(info->push_remote_name, rename.old_name)) {
+ strbuf_reset(&buf);
+ strbuf_addf(&buf, "branch.%s.pushremote", item->string);
+ git_config_set(buf.buf, rename.new_name);
+ }
}
if (!refspec_updated)
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 883b32efa0..59a1681636 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -737,12 +737,14 @@ test_expect_success 'rename a remote' '
git clone one four &&
(
cd four &&
+ git config branch.master.pushRemote origin &&
git remote rename origin upstream &&
test -z "$(git for-each-ref refs/remotes/origin)" &&
test "$(git symbolic-ref refs/remotes/upstream/HEAD)" = "refs/remotes/upstream/master" &&
test "$(git rev-parse upstream/master)" = "$(git rev-parse master)" &&
test "$(git config remote.upstream.fetch)" = "+refs/heads/*:refs/remotes/upstream/*" &&
- test "$(git config branch.master.remote)" = "upstream"
+ test "$(git config branch.master.remote)" = "upstream" &&
+ test "$(git config branch.master.pushRemote)" = "upstream"
)
'
--
2.24.1.497.g9abd7b20b4.dirty
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH 5/7] [RFC] config: make `scope_name` global as `config_scope_name`
2020-01-21 9:24 ` [PATCH 0/7] remote rename: improve handling of configuration values Bert Wesarg
` (3 preceding siblings ...)
2020-01-21 9:24 ` [PATCH v3 4/7] remote rename: rename branch.<name>.pushRemote config values too Bert Wesarg
@ 2020-01-21 9:24 ` Bert Wesarg
2020-01-22 0:12 ` Matt Rogers
2020-01-21 9:24 ` [PATCH 6/7] config: provide access to the current line number Bert Wesarg
` (2 subsequent siblings)
7 siblings, 1 reply; 33+ messages in thread
From: Bert Wesarg @ 2020-01-21 9:24 UTC (permalink / raw)
To: git; +Cc: Bert Wesarg, Matthew Rogers
Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
---
Will be replaced by Matthew Rogers.
Cc: Matthew Rogers <mattr94@gmail.com>
---
config.c | 16 ++++++++++++++++
config.h | 1 +
t/helper/test-config.c | 17 +----------------
3 files changed, 18 insertions(+), 16 deletions(-)
diff --git a/config.c b/config.c
index d75f88ca0c..4c461bb7a3 100644
--- a/config.c
+++ b/config.c
@@ -3317,6 +3317,22 @@ enum config_scope current_config_scope(void)
return current_parsing_scope;
}
+const char *config_scope_name(enum config_scope scope)
+{
+ switch (scope) {
+ case CONFIG_SCOPE_SYSTEM:
+ return "system";
+ case CONFIG_SCOPE_GLOBAL:
+ return "global";
+ case CONFIG_SCOPE_REPO:
+ return "repo";
+ case CONFIG_SCOPE_CMDLINE:
+ return "cmdline";
+ default:
+ return "unknown";
+ }
+}
+
int lookup_config(const char **mapping, int nr_mapping, const char *var)
{
int i;
diff --git a/config.h b/config.h
index 91fd4c5e96..c063f33ff6 100644
--- a/config.h
+++ b/config.h
@@ -301,6 +301,7 @@ enum config_scope {
CONFIG_SCOPE_REPO,
CONFIG_SCOPE_CMDLINE,
};
+const char *config_scope_name(enum config_scope scope);
enum config_scope current_config_scope(void);
const char *current_config_origin_type(void);
diff --git a/t/helper/test-config.c b/t/helper/test-config.c
index 214003d5b2..1e3bc7c8f4 100644
--- a/t/helper/test-config.c
+++ b/t/helper/test-config.c
@@ -37,21 +37,6 @@
*
*/
-static const char *scope_name(enum config_scope scope)
-{
- switch (scope) {
- case CONFIG_SCOPE_SYSTEM:
- return "system";
- case CONFIG_SCOPE_GLOBAL:
- return "global";
- case CONFIG_SCOPE_REPO:
- return "repo";
- case CONFIG_SCOPE_CMDLINE:
- return "cmdline";
- default:
- return "unknown";
- }
-}
static int iterate_cb(const char *var, const char *value, void *data)
{
static int nr;
@@ -63,7 +48,7 @@ static int iterate_cb(const char *var, const char *value, void *data)
printf("value=%s\n", value ? value : "(null)");
printf("origin=%s\n", current_config_origin_type());
printf("name=%s\n", current_config_name());
- printf("scope=%s\n", scope_name(current_config_scope()));
+ printf("scope=%s\n", config_scope_name(current_config_scope()));
return 0;
}
--
2.24.1.497.g9abd7b20b4.dirty
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH 5/7] [RFC] config: make `scope_name` global as `config_scope_name`
2020-01-21 9:24 ` [PATCH 5/7] [RFC] config: make `scope_name` global as `config_scope_name` Bert Wesarg
@ 2020-01-22 0:12 ` Matt Rogers
2020-01-22 7:37 ` Bert Wesarg
0 siblings, 1 reply; 33+ messages in thread
From: Matt Rogers @ 2020-01-22 0:12 UTC (permalink / raw)
To: Bert Wesarg; +Cc: git
Logos good to me...
As I'm a bit new, what would be the best way for me to work this into
my workflow?
On Tue, Jan 21, 2020 at 4:25 AM Bert Wesarg <bert.wesarg@googlemail.com> wrote:
>
> Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
> ---
> Will be replaced by Matthew Rogers.
>
> Cc: Matthew Rogers <mattr94@gmail.com>
> ---
> config.c | 16 ++++++++++++++++
> config.h | 1 +
> t/helper/test-config.c | 17 +----------------
> 3 files changed, 18 insertions(+), 16 deletions(-)
>
> diff --git a/config.c b/config.c
> index d75f88ca0c..4c461bb7a3 100644
> --- a/config.c
> +++ b/config.c
> @@ -3317,6 +3317,22 @@ enum config_scope current_config_scope(void)
> return current_parsing_scope;
> }
>
> +const char *config_scope_name(enum config_scope scope)
> +{
> + switch (scope) {
> + case CONFIG_SCOPE_SYSTEM:
> + return "system";
> + case CONFIG_SCOPE_GLOBAL:
> + return "global";
> + case CONFIG_SCOPE_REPO:
> + return "repo";
> + case CONFIG_SCOPE_CMDLINE:
> + return "cmdline";
> + default:
> + return "unknown";
> + }
> +}
> +
> int lookup_config(const char **mapping, int nr_mapping, const char *var)
> {
> int i;
> diff --git a/config.h b/config.h
> index 91fd4c5e96..c063f33ff6 100644
> --- a/config.h
> +++ b/config.h
> @@ -301,6 +301,7 @@ enum config_scope {
> CONFIG_SCOPE_REPO,
> CONFIG_SCOPE_CMDLINE,
> };
> +const char *config_scope_name(enum config_scope scope);
>
> enum config_scope current_config_scope(void);
> const char *current_config_origin_type(void);
> diff --git a/t/helper/test-config.c b/t/helper/test-config.c
> index 214003d5b2..1e3bc7c8f4 100644
> --- a/t/helper/test-config.c
> +++ b/t/helper/test-config.c
> @@ -37,21 +37,6 @@
> *
> */
>
> -static const char *scope_name(enum config_scope scope)
> -{
> - switch (scope) {
> - case CONFIG_SCOPE_SYSTEM:
> - return "system";
> - case CONFIG_SCOPE_GLOBAL:
> - return "global";
> - case CONFIG_SCOPE_REPO:
> - return "repo";
> - case CONFIG_SCOPE_CMDLINE:
> - return "cmdline";
> - default:
> - return "unknown";
> - }
> -}
> static int iterate_cb(const char *var, const char *value, void *data)
> {
> static int nr;
> @@ -63,7 +48,7 @@ static int iterate_cb(const char *var, const char *value, void *data)
> printf("value=%s\n", value ? value : "(null)");
> printf("origin=%s\n", current_config_origin_type());
> printf("name=%s\n", current_config_name());
> - printf("scope=%s\n", scope_name(current_config_scope()));
> + printf("scope=%s\n", config_scope_name(current_config_scope()));
>
> return 0;
> }
> --
> 2.24.1.497.g9abd7b20b4.dirty
>
--
Matthew Rogers
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH 5/7] [RFC] config: make `scope_name` global as `config_scope_name`
2020-01-22 0:12 ` Matt Rogers
@ 2020-01-22 7:37 ` Bert Wesarg
2020-01-23 1:30 ` Matt Rogers
0 siblings, 1 reply; 33+ messages in thread
From: Bert Wesarg @ 2020-01-22 7:37 UTC (permalink / raw)
To: Matt Rogers; +Cc: Git Mailing List
On Wed, Jan 22, 2020 at 1:12 AM Matt Rogers <mattr94@gmail.com> wrote:
>
> Logos good to me...
>
> As I'm a bit new, what would be the best way for me to work this into
> my workflow?
if you have done that change already locally, then you can ignore my
patch. I will wait for your re-roll and put my changes on top of
yours. If not, you could replace your patch with this one in your
series. Your call.
Bert
>
> On Tue, Jan 21, 2020 at 4:25 AM Bert Wesarg <bert.wesarg@googlemail.com> wrote:
> >
> > Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
> > ---
> > Will be replaced by Matthew Rogers.
> >
> > Cc: Matthew Rogers <mattr94@gmail.com>
> > ---
> > config.c | 16 ++++++++++++++++
> > config.h | 1 +
> > t/helper/test-config.c | 17 +----------------
> > 3 files changed, 18 insertions(+), 16 deletions(-)
> >
> > diff --git a/config.c b/config.c
> > index d75f88ca0c..4c461bb7a3 100644
> > --- a/config.c
> > +++ b/config.c
> > @@ -3317,6 +3317,22 @@ enum config_scope current_config_scope(void)
> > return current_parsing_scope;
> > }
> >
> > +const char *config_scope_name(enum config_scope scope)
> > +{
> > + switch (scope) {
> > + case CONFIG_SCOPE_SYSTEM:
> > + return "system";
> > + case CONFIG_SCOPE_GLOBAL:
> > + return "global";
> > + case CONFIG_SCOPE_REPO:
> > + return "repo";
> > + case CONFIG_SCOPE_CMDLINE:
> > + return "cmdline";
> > + default:
> > + return "unknown";
> > + }
> > +}
> > +
> > int lookup_config(const char **mapping, int nr_mapping, const char *var)
> > {
> > int i;
> > diff --git a/config.h b/config.h
> > index 91fd4c5e96..c063f33ff6 100644
> > --- a/config.h
> > +++ b/config.h
> > @@ -301,6 +301,7 @@ enum config_scope {
> > CONFIG_SCOPE_REPO,
> > CONFIG_SCOPE_CMDLINE,
> > };
> > +const char *config_scope_name(enum config_scope scope);
> >
> > enum config_scope current_config_scope(void);
> > const char *current_config_origin_type(void);
> > diff --git a/t/helper/test-config.c b/t/helper/test-config.c
> > index 214003d5b2..1e3bc7c8f4 100644
> > --- a/t/helper/test-config.c
> > +++ b/t/helper/test-config.c
> > @@ -37,21 +37,6 @@
> > *
> > */
> >
> > -static const char *scope_name(enum config_scope scope)
> > -{
> > - switch (scope) {
> > - case CONFIG_SCOPE_SYSTEM:
> > - return "system";
> > - case CONFIG_SCOPE_GLOBAL:
> > - return "global";
> > - case CONFIG_SCOPE_REPO:
> > - return "repo";
> > - case CONFIG_SCOPE_CMDLINE:
> > - return "cmdline";
> > - default:
> > - return "unknown";
> > - }
> > -}
> > static int iterate_cb(const char *var, const char *value, void *data)
> > {
> > static int nr;
> > @@ -63,7 +48,7 @@ static int iterate_cb(const char *var, const char *value, void *data)
> > printf("value=%s\n", value ? value : "(null)");
> > printf("origin=%s\n", current_config_origin_type());
> > printf("name=%s\n", current_config_name());
> > - printf("scope=%s\n", scope_name(current_config_scope()));
> > + printf("scope=%s\n", config_scope_name(current_config_scope()));
> >
> > return 0;
> > }
> > --
> > 2.24.1.497.g9abd7b20b4.dirty
> >
>
>
> --
> Matthew Rogers
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 6/7] config: provide access to the current line number
2020-01-21 9:24 ` [PATCH 0/7] remote rename: improve handling of configuration values Bert Wesarg
` (4 preceding siblings ...)
2020-01-21 9:24 ` [PATCH 5/7] [RFC] config: make `scope_name` global as `config_scope_name` Bert Wesarg
@ 2020-01-21 9:24 ` Bert Wesarg
2020-01-21 9:24 ` [PATCH 7/7] remote rename: gently handle remote.pushDefault config Bert Wesarg
2020-01-22 15:26 ` [PATCH 0/7] remote rename: improve handling of configuration values Bert Wesarg
7 siblings, 0 replies; 33+ messages in thread
From: Bert Wesarg @ 2020-01-21 9:24 UTC (permalink / raw)
To: git; +Cc: Bert Wesarg
Users are nowadays trained to see message from CLI tools in the form
<file>:<lno>: …
To be able to give such messages when notifying the user about
configurations in any config file, it is currently only possible to get
the file name (if the value originates from a file to begin with) via
`current_config_name()`. Now it is also possible to query the current line
number for the configuration.
Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
---
config.c | 8 ++++++++
config.h | 1 +
t/helper/test-config.c | 1 +
t/t1308-config-set.sh | 14 ++++++++++++--
4 files changed, 22 insertions(+), 2 deletions(-)
diff --git a/config.c b/config.c
index 4c461bb7a3..5d1d6b5871 100644
--- a/config.c
+++ b/config.c
@@ -3333,6 +3333,14 @@ const char *config_scope_name(enum config_scope scope)
}
}
+int current_config_line(void)
+{
+ if (current_config_kvi)
+ return current_config_kvi->linenr;
+ else
+ return cf->linenr;
+}
+
int lookup_config(const char **mapping, int nr_mapping, const char *var)
{
int i;
diff --git a/config.h b/config.h
index c063f33ff6..371f7f2dd0 100644
--- a/config.h
+++ b/config.h
@@ -306,6 +306,7 @@ const char *config_scope_name(enum config_scope scope);
enum config_scope current_config_scope(void);
const char *current_config_origin_type(void);
const char *current_config_name(void);
+int current_config_line(void);
/**
* Include Directives
diff --git a/t/helper/test-config.c b/t/helper/test-config.c
index 1e3bc7c8f4..234c722b48 100644
--- a/t/helper/test-config.c
+++ b/t/helper/test-config.c
@@ -48,6 +48,7 @@ static int iterate_cb(const char *var, const char *value, void *data)
printf("value=%s\n", value ? value : "(null)");
printf("origin=%s\n", current_config_origin_type());
printf("name=%s\n", current_config_name());
+ printf("lno=%d\n", current_config_line());
printf("scope=%s\n", config_scope_name(current_config_scope()));
return 0;
diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
index 7b4e1a63eb..9e36e7a590 100755
--- a/t/t1308-config-set.sh
+++ b/t/t1308-config-set.sh
@@ -238,8 +238,8 @@ test_expect_success 'error on modifying repo config without repo' '
cmdline_config="'foo.bar=from-cmdline'"
test_expect_success 'iteration shows correct origins' '
- echo "[foo]bar = from-repo" >.git/config &&
- echo "[foo]bar = from-home" >.gitconfig &&
+ printf "[ignore]\n\tthis = please\n[foo]bar = from-repo\n" >.git/config &&
+ printf "[foo]\n\tbar = from-home\n" >.gitconfig &&
if test_have_prereq MINGW
then
# Use Windows path (i.e. *not* $HOME)
@@ -253,18 +253,28 @@ test_expect_success 'iteration shows correct origins' '
value=from-home
origin=file
name=$HOME_GITCONFIG
+ lno=2
scope=global
+ key=ignore.this
+ value=please
+ origin=file
+ name=.git/config
+ lno=2
+ scope=repo
+
key=foo.bar
value=from-repo
origin=file
name=.git/config
+ lno=3
scope=repo
key=foo.bar
value=from-cmdline
origin=command line
name=
+ lno=-1
scope=cmdline
EOF
GIT_CONFIG_PARAMETERS=$cmdline_config test-tool config iterate >actual &&
--
2.24.1.497.g9abd7b20b4.dirty
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH 7/7] remote rename: gently handle remote.pushDefault config
2020-01-21 9:24 ` [PATCH 0/7] remote rename: improve handling of configuration values Bert Wesarg
` (5 preceding siblings ...)
2020-01-21 9:24 ` [PATCH 6/7] config: provide access to the current line number Bert Wesarg
@ 2020-01-21 9:24 ` Bert Wesarg
2020-01-23 23:03 ` Junio C Hamano
2020-01-22 15:26 ` [PATCH 0/7] remote rename: improve handling of configuration values Bert Wesarg
7 siblings, 1 reply; 33+ messages in thread
From: Bert Wesarg @ 2020-01-21 9:24 UTC (permalink / raw)
To: git; +Cc: Bert Wesarg, Junio C Hamano, Johannes Schindelin, Matthew Rogers
When renaming a remote with
git remote rename X Y
Git already renames any branch.<name>.remote and branch.<name>.pushRemote
values from X to Y.
However remote.pushDefault needs a more gentle approach, as this may be
set in a non-repo configuration file. In such a case only a warning is
printed, such as:
warning: The global configuration remote.pushDefault in:
$HOME/.gitconfig:35
now names the non-existent remote 'origin'
It is changed to remote.pushDefault = Y when set in a repo configuration
though.
Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
---
Matthew, you are in Cc because of your current work 'config: allow user to
know scope of config options'. I think I'm correct to assuming an ordering
of the enum config_scope.
Cc: Junio C Hamano <gitster@pobox.com>
Cc: Johannes Schindelin <johannes.schindelin@gmx.de>
Cc: Matthew Rogers <mattr94@gmail.com>
---
builtin/remote.c | 43 ++++++++++++++++++++++++++++++++++++++++
t/t5505-remote.sh | 50 ++++++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 92 insertions(+), 1 deletion(-)
diff --git a/builtin/remote.c b/builtin/remote.c
index 0cb930fe00..52172e523a 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -611,6 +611,29 @@ static int migrate_file(struct remote *remote)
return 0;
}
+struct push_default_info
+{
+ struct rename_info *rename;
+ enum config_scope scope;
+ struct strbuf* origin;
+ int linenr;
+};
+
+static int config_read_push_default(const char *key, const char *value,
+ void *cb)
+{
+ struct push_default_info* info = cb;
+ if (strcmp(key, "remote.pushdefault") || strcmp(value, info->rename->old_name))
+ return 0;
+
+ info->scope = current_config_scope();
+ strbuf_reset(info->origin);
+ strbuf_addstr(info->origin, current_config_name());
+ info->linenr = current_config_line();
+
+ return 0;
+}
+
static int mv(int argc, const char **argv)
{
struct option options[] = {
@@ -746,6 +769,26 @@ static int mv(int argc, const char **argv)
die(_("creating '%s' failed"), buf.buf);
}
string_list_clear(&remote_branches, 1);
+
+ struct push_default_info push_default;
+ push_default.rename = &rename;
+ push_default.scope = CONFIG_SCOPE_UNKNOWN;
+ push_default.origin = &buf;
+ git_config(config_read_push_default, &push_default);
+ if (push_default.scope >= CONFIG_SCOPE_CMDLINE)
+ ; /* pass */
+ else if (push_default.scope >= CONFIG_SCOPE_REPO) {
+ git_config_set("remote.pushDefault", rename.new_name);
+ } else if (push_default.scope >= CONFIG_SCOPE_SYSTEM) {
+ /* warn */
+ warning(_("The %s configuration remote.pushDefault in:\n"
+ "\t%s:%d\n"
+ "now names the non-existent remote '%s'"),
+ config_scope_name(push_default.scope),
+ push_default.origin->buf, push_default.linenr,
+ rename.old_name);
+ }
+
return 0;
}
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 59a1681636..3b84c7bf9b 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -737,6 +737,7 @@ test_expect_success 'rename a remote' '
git clone one four &&
(
cd four &&
+ test_config_global remote.pushDefault origin &&
git config branch.master.pushRemote origin &&
git remote rename origin upstream &&
test -z "$(git for-each-ref refs/remotes/origin)" &&
@@ -744,7 +745,54 @@ test_expect_success 'rename a remote' '
test "$(git rev-parse upstream/master)" = "$(git rev-parse master)" &&
test "$(git config remote.upstream.fetch)" = "+refs/heads/*:refs/remotes/upstream/*" &&
test "$(git config branch.master.remote)" = "upstream" &&
- test "$(git config branch.master.pushRemote)" = "upstream"
+ test "$(git config branch.master.pushRemote)" = "upstream" &&
+ test "$(git config --global remote.pushDefault)" = "origin"
+ )
+'
+
+test_expect_success 'rename a remote renames repo remote.pushDefault' '
+ git clone one four.1 &&
+ (
+ cd four.1 &&
+ git config remote.pushDefault origin &&
+ git remote rename origin upstream &&
+ test "$(git config remote.pushDefault)" = "upstream"
+ )
+'
+
+test_expect_success 'rename a remote keeps global remote.pushDefault' '
+ git clone one four.2 &&
+ (
+ cd four.2 &&
+ test_config_global remote.pushDefault origin &&
+ git config remote.pushDefault other &&
+ git remote rename origin upstream &&
+ test "$(git config --global remote.pushDefault)" = "origin" &&
+ test "$(git config remote.pushDefault)" = "other"
+ )
+'
+
+test_expect_success 'rename a remote renames repo remote.pushDefault but ignores global' '
+ git clone one four.3 &&
+ (
+ cd four.3 &&
+ test_config_global remote.pushDefault other &&
+ git config remote.pushDefault origin &&
+ git remote rename origin upstream &&
+ test "$(git config --global remote.pushDefault)" = "other" &&
+ test "$(git config remote.pushDefault)" = "upstream"
+ )
+'
+
+test_expect_success 'rename a remote renames repo remote.pushDefault but keeps global' '
+ git clone one four.4 &&
+ (
+ cd four.4 &&
+ test_config_global remote.pushDefault origin &&
+ git config remote.pushDefault origin &&
+ git remote rename origin upstream &&
+ test "$(git config --global remote.pushDefault)" = "origin" &&
+ test "$(git config remote.pushDefault)" = "upstream"
)
'
--
2.24.1.497.g9abd7b20b4.dirty
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH 7/7] remote rename: gently handle remote.pushDefault config
2020-01-21 9:24 ` [PATCH 7/7] remote rename: gently handle remote.pushDefault config Bert Wesarg
@ 2020-01-23 23:03 ` Junio C Hamano
2020-01-24 8:49 ` Bert Wesarg
0 siblings, 1 reply; 33+ messages in thread
From: Junio C Hamano @ 2020-01-23 23:03 UTC (permalink / raw)
To: Bert Wesarg; +Cc: git, Johannes Schindelin, Matthew Rogers
Bert Wesarg <bert.wesarg@googlemail.com> writes:
> @@ -746,6 +769,26 @@ static int mv(int argc, const char **argv)
> die(_("creating '%s' failed"), buf.buf);
> }
> string_list_clear(&remote_branches, 1);
> +
> + struct push_default_info push_default;
Likewise. decl-after-stmt is not allowed.
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH 7/7] remote rename: gently handle remote.pushDefault config
2020-01-23 23:03 ` Junio C Hamano
@ 2020-01-24 8:49 ` Bert Wesarg
0 siblings, 0 replies; 33+ messages in thread
From: Bert Wesarg @ 2020-01-24 8:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List, Johannes Schindelin, Matthew Rogers
On Fri, Jan 24, 2020 at 12:03 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Bert Wesarg <bert.wesarg@googlemail.com> writes:
>
> > @@ -746,6 +769,26 @@ static int mv(int argc, const char **argv)
> > die(_("creating '%s' failed"), buf.buf);
> > }
> > string_list_clear(&remote_branches, 1);
> > +
> > + struct push_default_info push_default;
>
> Likewise. decl-after-stmt is not allowed.
thanks. Its time for a re-roll.
Bert
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 0/7] remote rename: improve handling of configuration values
2020-01-21 9:24 ` [PATCH 0/7] remote rename: improve handling of configuration values Bert Wesarg
` (6 preceding siblings ...)
2020-01-21 9:24 ` [PATCH 7/7] remote rename: gently handle remote.pushDefault config Bert Wesarg
@ 2020-01-22 15:26 ` Bert Wesarg
7 siblings, 0 replies; 33+ messages in thread
From: Bert Wesarg @ 2020-01-22 15:26 UTC (permalink / raw)
To: Git Mailing List; +Cc: Johannes Schindelin, Junio C Hamano
All,
I think 'git remote remove X' needs similar improvements to
'handle.*.pushremote = X' and 'push.default = X'. Will be handled in
the re-roll.
Bert
On Tue, Jan 21, 2020 at 10:24 AM Bert Wesarg <bert.wesarg@googlemail.com> wrote:
>
> While fixing that 'git remote rename X Y' does not rename the values for
> 'branch.*.pushRemote', it opened the possibility to more improvements in
> this area:
>
> - 'remote rename' did not accept single-letter abbreviations for
> 'branch.*.rebase' like 'pull --rebase' does
>
> - minor clean-ups the config callback
>
> - patch 5 will be replaced by/rebased on Matthew's work in 'config: allow user to
> know scope of config options', once 'config_scope_name' is available
>
> - gently handling the rename of 'remote.pushDefault'
>
> Bert Wesarg (7):
> pull --rebase/remote rename: document and honor single-letter
> abbreviations rebase types
> remote: clean-up by returning early to avoid one indentation
> remote: clean-up config callback
> remote rename: rename branch.<name>.pushRemote config values too
> [RFC] config: make `scope_name` global as `config_scope_name`
> config: provide access to the current line number
> remote rename: gently handle remote.pushDefault config
>
> Documentation/config/branch.txt | 7 +-
> Documentation/config/pull.txt | 7 +-
> Makefile | 1 +
> builtin/pull.c | 29 +-----
> builtin/remote.c | 168 +++++++++++++++++++++-----------
> config.c | 24 +++++
> config.h | 2 +
> rebase.c | 24 +++++
> rebase.h | 15 +++
> t/helper/test-config.c | 18 +---
> t/t1308-config-set.sh | 14 ++-
> t/t5505-remote.sh | 52 +++++++++-
> 12 files changed, 254 insertions(+), 107 deletions(-)
> create mode 100644 rebase.c
> create mode 100644 rebase.h
>
> --
> 2.24.1.497.g9abd7b20b4.dirty
>
^ permalink raw reply [flat|nested] 33+ messages in thread