| Message ID | ffc8ffc6ede731b182d32a81d044428566acc625.1579253411.git.bert.wesarg@googlemail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] remote rename: rename branch.<name>.pushRemote config values too | expand |
Hi Bert, On Fri, 17 Jan 2020, Bert Wesarg wrote: > 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 Should we warn if remote.pushDefault = X? Other than that, I am still okay with the patch (even after the re-ordering of the enum ;-) Just one more suggestion: > 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 | 17 +++++++++++++++-- > t/t5505-remote.sh | 4 +++- > 2 files changed, 18 insertions(+), 3 deletions(-) > > diff --git a/builtin/remote.c b/builtin/remote.c > index 96bbe828fe..a74aac344f 100644 > --- a/builtin/remote.c > +++ b/builtin/remote.c > @@ -251,6 +251,7 @@ struct branch_info { > enum { > NO_REBASE, NORMAL_REBASE, INTERACTIVE_REBASE, REBASE_MERGES > } rebase; > + char *push_remote_name; > }; > > static struct string_list branch_list = STRING_LIST_INIT_NODUP; > @@ -269,7 +270,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 += 7; > @@ -282,6 +283,9 @@ static int config_read_branches(const char *key, const char *value, void *cb) > } else if (strip_suffix(key, ".rebase", &key_len)) { > name = xmemdupz(key, key_len); > type = REBASE; > + } else if (strip_suffix(key, ".pushremote", &key_len)) { > + name = xmemdupz(key, key_len); > + type = PUSH_REMOTE; > } else > return 0; > > @@ -305,7 +309,7 @@ static int config_read_branches(const char *key, const char *value, void *cb) > space = strchr(value, ' '); > } > string_list_append(&info->merge, xstrdup(value)); > - } else { > + } else if (type == REBASE) { > int v = git_parse_maybe_bool(value); > if (v >= 0) > info->rebase = v; > @@ -315,6 +319,10 @@ static int config_read_branches(const char *key, const char *value, void *cb) > info->rebase = REBASE_MERGES; > else if (!strcmp(value, "interactive")) > info->rebase = INTERACTIVE_REBASE; > + } else { > + if (info->push_remote_name) > + warning(_("more than one %s"), orig_key); > + info->push_remote_name = xstrdup(value); It makes me a bit nervous that this is an `else` without an `if (type == PUSH_REMOTE)`... Maybe do that, just to be on the safe side, even if another type is introduced in the future? Thanks, Dscho > } > } > return 0; > @@ -680,6 +688,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 > >
Hi, On Fri, Jan 17, 2020 at 12:50 PM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote: > > Hi Bert, > > On Fri, 17 Jan 2020, Bert Wesarg wrote: > > > 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 > > Should we warn if remote.pushDefault = X? AFAIU, the value of remote.pushDefault wont be renamed yet. So you suggest to issue a warning in case remote.pushDefault is X. But as X does not exists anymore after the rename, the value of remote.pushDefault is invalid. So why not rename it too? > > Other than that, I am still okay with the patch (even after the > re-ordering of the enum ;-) > > Just one more suggestion: > > > 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 | 17 +++++++++++++++-- > > t/t5505-remote.sh | 4 +++- > > 2 files changed, 18 insertions(+), 3 deletions(-) > > > > diff --git a/builtin/remote.c b/builtin/remote.c > > index 96bbe828fe..a74aac344f 100644 > > --- a/builtin/remote.c > > +++ b/builtin/remote.c > > @@ -251,6 +251,7 @@ struct branch_info { > > enum { > > NO_REBASE, NORMAL_REBASE, INTERACTIVE_REBASE, REBASE_MERGES > > } rebase; > > + char *push_remote_name; > > }; > > > > static struct string_list branch_list = STRING_LIST_INIT_NODUP; > > @@ -269,7 +270,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 += 7; > > @@ -282,6 +283,9 @@ static int config_read_branches(const char *key, const char *value, void *cb) > > } else if (strip_suffix(key, ".rebase", &key_len)) { > > name = xmemdupz(key, key_len); > > type = REBASE; > > + } else if (strip_suffix(key, ".pushremote", &key_len)) { > > + name = xmemdupz(key, key_len); > > + type = PUSH_REMOTE; > > } else > > return 0; > > > > @@ -305,7 +309,7 @@ static int config_read_branches(const char *key, const char *value, void *cb) > > space = strchr(value, ' '); > > } > > string_list_append(&info->merge, xstrdup(value)); > > - } else { > > + } else if (type == REBASE) { > > int v = git_parse_maybe_bool(value); > > if (v >= 0) > > info->rebase = v; > > @@ -315,6 +319,10 @@ static int config_read_branches(const char *key, const char *value, void *cb) > > info->rebase = REBASE_MERGES; > > else if (!strcmp(value, "interactive")) > > info->rebase = INTERACTIVE_REBASE; > > + } else { > > + if (info->push_remote_name) > > + warning(_("more than one %s"), orig_key); > > + info->push_remote_name = xstrdup(value); > > It makes me a bit nervous that this is an `else` without an `if (type == > PUSH_REMOTE)`... Maybe do that, just to be on the safe side, even if > another type is introduced in the future? I'm not a friend of this last 'else' either, but it was so to begin with. I'm all for changing it to an 'else if'. Though while it is impossible that 'type' has a value different than one from the enum, I would be even more comfortable with having BUG at the end. Bert > > Thanks, > Dscho >
Hi Bert, On Fri, 17 Jan 2020, Bert Wesarg wrote: > On Fri, Jan 17, 2020 at 12:50 PM Johannes Schindelin > <Johannes.Schindelin@gmx.de> wrote: > > > > Hi Bert, > > > > On Fri, 17 Jan 2020, Bert Wesarg wrote: > > > > > 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 > > > > Should we warn if remote.pushDefault = X? > > AFAIU, the value of remote.pushDefault wont be renamed yet. So you > suggest to issue a warning in case remote.pushDefault is X. But as X > does not exists anymore after the rename, the value of > remote.pushDefault is invalid. So why not rename it too? If this setting was usually a repository-specific one, I would suggest to change its value, too. But it is my understanding that this might be set in `~/.gitconfig` more often than not, so I recommend a warning instead. > > > @@ -305,7 +309,7 @@ static int config_read_branches(const char *key, const char *value, void *cb) > > > space = strchr(value, ' '); > > > } > > > string_list_append(&info->merge, xstrdup(value)); > > > - } else { > > > + } else if (type == REBASE) { > > > int v = git_parse_maybe_bool(value); > > > if (v >= 0) > > > info->rebase = v; > > > @@ -315,6 +319,10 @@ static int config_read_branches(const char *key, const char *value, void *cb) > > > info->rebase = REBASE_MERGES; > > > else if (!strcmp(value, "interactive")) > > > info->rebase = INTERACTIVE_REBASE; > > > + } else { > > > + if (info->push_remote_name) > > > + warning(_("more than one %s"), orig_key); > > > + info->push_remote_name = xstrdup(value); > > > > It makes me a bit nervous that this is an `else` without an `if (type == > > PUSH_REMOTE)`... Maybe do that, just to be on the safe side, even if > > another type is introduced in the future? > > I'm not a friend of this last 'else' either, but it was so to begin > with. I'm all for changing it to an 'else if'. Though while it is > impossible that 'type' has a value different than one from the enum, I > would be even more comfortable with having BUG at the end. My thinking was: even if this was a bare `else` before, why not fix that issue while we're already in the area? I like the `BUG` idea. Ciao, Dscho
Hi, On Fri, Jan 17, 2020 at 2:30 PM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote: > > Hi Bert, > > On Fri, 17 Jan 2020, Bert Wesarg wrote: > > > On Fri, Jan 17, 2020 at 12:50 PM Johannes Schindelin > > <Johannes.Schindelin@gmx.de> wrote: > > > > > > Hi Bert, > > > > > > On Fri, 17 Jan 2020, Bert Wesarg wrote: > > > > > > > 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 > > > > > > Should we warn if remote.pushDefault = X? > > > > AFAIU, the value of remote.pushDefault wont be renamed yet. So you > > suggest to issue a warning in case remote.pushDefault is X. But as X > > does not exists anymore after the rename, the value of > > remote.pushDefault is invalid. So why not rename it too? > > If this setting was usually a repository-specific one, I would suggest to > change its value, too. But it is my understanding that this might be set > in `~/.gitconfig` more often than not, so I recommend a warning instead. than why not rename it, if its a repository-specific setting and warn if it is a global one? If this is detectable at all. > > > > > @@ -305,7 +309,7 @@ static int config_read_branches(const char *key, const char *value, void *cb) > > > > space = strchr(value, ' '); > > > > } > > > > string_list_append(&info->merge, xstrdup(value)); > > > > - } else { > > > > + } else if (type == REBASE) { > > > > int v = git_parse_maybe_bool(value); > > > > if (v >= 0) > > > > info->rebase = v; > > > > @@ -315,6 +319,10 @@ static int config_read_branches(const char *key, const char *value, void *cb) > > > > info->rebase = REBASE_MERGES; > > > > else if (!strcmp(value, "interactive")) > > > > info->rebase = INTERACTIVE_REBASE; > > > > + } else { > > > > + if (info->push_remote_name) > > > > + warning(_("more than one %s"), orig_key); > > > > + info->push_remote_name = xstrdup(value); > > > > > > It makes me a bit nervous that this is an `else` without an `if (type == > > > PUSH_REMOTE)`... Maybe do that, just to be on the safe side, even if > > > another type is introduced in the future? > > > > I'm not a friend of this last 'else' either, but it was so to begin > > with. I'm all for changing it to an 'else if'. Though while it is > > impossible that 'type' has a value different than one from the enum, I > > would be even more comfortable with having BUG at the end. > > My thinking was: even if this was a bare `else` before, why not fix that > issue while we're already in the area? I like the `BUG` idea. Glad I can squash this into this one, instead of making it a single patch out of it. Bert > > Ciao, > Dscho
Bert Wesarg <bert.wesarg@googlemail.com> writes: > 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 This makes sense now. Thanks for an updated description. > Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com> > @@ -305,7 +309,7 @@ static int config_read_branches(const char *key, const char *value, void *cb) > space = strchr(value, ' '); > } > string_list_append(&info->merge, xstrdup(value)); > - } else { > + } else if (type == REBASE) { > int v = git_parse_maybe_bool(value); > if (v >= 0) > info->rebase = v; > @@ -315,6 +319,10 @@ static int config_read_branches(const char *key, const char *value, void *cb) > info->rebase = REBASE_MERGES; > else if (!strcmp(value, "interactive")) > info->rebase = INTERACTIVE_REBASE; > + } else { > + if (info->push_remote_name) > + warning(_("more than one %s"), orig_key); > + info->push_remote_name = xstrdup(value); > } This is perfectly fine for now, as it follows the existing "now we have handled X, and Y, so the remainder must be Z" mentality, but at some point we may want to make sure that we are protected against seeing an unexpected 'type', iow ... } else if (type == PUSH_REMOTE) { ... } else { BUG("unexpected type=%d", type); } as we learn more "type"s. Better yet, this if/elseif/ cascade may become clearer if it is rewritten to a switch statement. I was about to conclude this message with "but that is all outside the scope of this fix, so I'll queue it as-is " before noticing that you two seem to be leaning towards clean-up at the same time. If we are to clean up the code structure along these lines, I'd prefer to see it done as a preparatory patch before pushremote handling gets introduced. Taking some other clean-up ideas on this function, e.g.: * key += 7 should better be done without hardcoded length of "branch." * By leaving early, we can save one indentation level. * name does not have to be computed for each branch. the resulting body of the function might look more like this: if (!skip_prefix(key, "branch.", &key)) return 0; if (strip_suffix(key, ".remote", &key_len)) type = REMOTE; else if (strip_suffix(key, ".merge", &key_len)) type = MERGE; ... else return 0; name = xmemdupz(key, key_len); item = string_list_insert(&branch_list, name); ... switch (type) { case REMOTE: ... default: BUG("unhandled type %d", type); } Thanks.
Junio, On Fri, Jan 17, 2020 at 7:48 PM Junio C Hamano <gitster@pobox.com> wrote: > > @@ -305,7 +309,7 @@ static int config_read_branches(const char *key, const char *value, void *cb) > > space = strchr(value, ' '); > > } > > string_list_append(&info->merge, xstrdup(value)); > > - } else { > > + } else if (type == REBASE) { > > int v = git_parse_maybe_bool(value); > > if (v >= 0) > > info->rebase = v; > > @@ -315,6 +319,10 @@ static int config_read_branches(const char *key, const char *value, void *cb) > > info->rebase = REBASE_MERGES; > > else if (!strcmp(value, "interactive")) > > info->rebase = INTERACTIVE_REBASE; > > + } else { > > + if (info->push_remote_name) > > + warning(_("more than one %s"), orig_key); > > + info->push_remote_name = xstrdup(value); > > } > > This is perfectly fine for now, as it follows the existing "now we > have handled X, and Y, so the remainder must be Z" mentality, but at > some point we may want to make sure that we are protected against > seeing an unexpected 'type', iow > > ... > } else if (type == PUSH_REMOTE) { > ... > } else { > BUG("unexpected type=%d", type); > } > > as we learn more "type"s. Better yet, this if/elseif/ cascade may > become clearer if it is rewritten to a switch statement. > > I was about to conclude this message with "but that is all outside > the scope of this fix, so I'll queue it as-is " before noticing > that you two seem to be leaning towards clean-up at the same time. > If we are to clean up the code structure along these lines, I'd > prefer to see it done as a preparatory patch before pushremote > handling gets introduced. > > Taking some other clean-up ideas on this function, e.g.: > > * key += 7 should better be done without hardcoded length of "branch." > * By leaving early, we can save one indentation level. > * name does not have to be computed for each branch. > > the resulting body of the function might look more like this: > > if (!skip_prefix(key, "branch.", &key)) > return 0; > > if (strip_suffix(key, ".remote", &key_len)) > type = REMOTE; > else if (strip_suffix(key, ".merge", &key_len)) > type = MERGE; > ... > else > return 0; > name = xmemdupz(key, key_len); > item = string_list_insert(&branch_list, name); > ... > > switch (type) { > case REMOTE: > ... > default: > BUG("unhandled type %d", type); > } can you give me an heads up about your expected number of patches for this clean up. Rather detailed or just one? Thanks in advance. Best, Bert > > Thanks.
Bert Wesarg <bert.wesarg@googlemail.com> writes: > can you give me an heads up about your expected number of patches for > this clean up. Rather detailed or just one? That's up to the contributor X-<. I would expect that some folks can write it up as a single patch and still keep it easily understandable, and others might have trouble explaining what they did well to the others and may need to split them into a few pieces.
Hi Bert, On Fri, 17 Jan 2020, Bert Wesarg wrote: > On Fri, Jan 17, 2020 at 2:30 PM Johannes Schindelin > <Johannes.Schindelin@gmx.de> wrote: > > > > On Fri, 17 Jan 2020, Bert Wesarg wrote: > > > > > On Fri, Jan 17, 2020 at 12:50 PM Johannes Schindelin > > > <Johannes.Schindelin@gmx.de> wrote: > > > > > > > > On Fri, 17 Jan 2020, Bert Wesarg wrote: > > > > > > > > > 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 > > > > > > > > Should we warn if remote.pushDefault = X? > > > > > > AFAIU, the value of remote.pushDefault wont be renamed yet. So you > > > suggest to issue a warning in case remote.pushDefault is X. But as X > > > does not exists anymore after the rename, the value of > > > remote.pushDefault is invalid. So why not rename it too? > > > > If this setting was usually a repository-specific one, I would suggest to > > change its value, too. But it is my understanding that this might be set > > in `~/.gitconfig` more often than not, so I recommend a warning instead. > > than why not rename it, if its a repository-specific setting and warn > if it is a global one? If this is detectable at all. Sure, but you might need to re-parse the config to detect that (and you have to use `git_config_from_file()` to make sure that you know that you are looking at the repository config and not at anything else). Ciao, Dscho
Hi Dscho, On Mon, Jan 20, 2020 at 12:25 PM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote: > > Hi Bert, > > On Fri, 17 Jan 2020, Bert Wesarg wrote: > > > On Fri, Jan 17, 2020 at 2:30 PM Johannes Schindelin > > <Johannes.Schindelin@gmx.de> wrote: > > > > > > On Fri, 17 Jan 2020, Bert Wesarg wrote: > > > > > > > On Fri, Jan 17, 2020 at 12:50 PM Johannes Schindelin > > > > <Johannes.Schindelin@gmx.de> wrote: > > > > AFAIU, the value of remote.pushDefault wont be renamed yet. So you > > > > suggest to issue a warning in case remote.pushDefault is X. But as X > > > > does not exists anymore after the rename, the value of > > > > remote.pushDefault is invalid. So why not rename it too? > > > > > > If this setting was usually a repository-specific one, I would suggest to > > > change its value, too. But it is my understanding that this might be set > > > in `~/.gitconfig` more often than not, so I recommend a warning instead. > > > > than why not rename it, if its a repository-specific setting and warn > > if it is a global one? If this is detectable at all. > > Sure, but you might need to re-parse the config to detect that (and you > have to use `git_config_from_file()` to make sure that you know that you > are looking at the repository config and not at anything else). I found current_config_scope() which serves the purpose for me. Anything wrong with this approach? Best, Bert > > Ciao, > Dscho
Hi Bert, On Mon, 20 Jan 2020, Bert Wesarg wrote: > On Mon, Jan 20, 2020 at 12:25 PM Johannes Schindelin > <Johannes.Schindelin@gmx.de> wrote: > > > > Hi Bert, > > > > On Fri, 17 Jan 2020, Bert Wesarg wrote: > > > > > On Fri, Jan 17, 2020 at 2:30 PM Johannes Schindelin > > > <Johannes.Schindelin@gmx.de> wrote: > > > > > > > > On Fri, 17 Jan 2020, Bert Wesarg wrote: > > > > > > > > > On Fri, Jan 17, 2020 at 12:50 PM Johannes Schindelin > > > > > <Johannes.Schindelin@gmx.de> wrote: > > > > > AFAIU, the value of remote.pushDefault wont be renamed yet. So you > > > > > suggest to issue a warning in case remote.pushDefault is X. But as X > > > > > does not exists anymore after the rename, the value of > > > > > remote.pushDefault is invalid. So why not rename it too? > > > > > > > > If this setting was usually a repository-specific one, I would suggest to > > > > change its value, too. But it is my understanding that this might be set > > > > in `~/.gitconfig` more often than not, so I recommend a warning instead. > > > > > > than why not rename it, if its a repository-specific setting and warn > > > if it is a global one? If this is detectable at all. > > > > Sure, but you might need to re-parse the config to detect that (and you > > have to use `git_config_from_file()` to make sure that you know that you > > are looking at the repository config and not at anything else). > > I found current_config_scope() which serves the purpose for me. > Anything wrong with this approach? I guess you could go for that, even if it is not exactly elegant (and not thread-safe, but who cares about that in `git remote`...). It would also do way more work than you need. If I were to implement this, I would definitely go for `git_config_from_file(git_path("index"), ...)`. Ciao, Dscho
diff --git a/builtin/remote.c b/builtin/remote.c index 96bbe828fe..a74aac344f 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -251,6 +251,7 @@ struct branch_info { enum { NO_REBASE, NORMAL_REBASE, INTERACTIVE_REBASE, REBASE_MERGES } rebase; + char *push_remote_name; }; static struct string_list branch_list = STRING_LIST_INIT_NODUP; @@ -269,7 +270,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 += 7; @@ -282,6 +283,9 @@ static int config_read_branches(const char *key, const char *value, void *cb) } else if (strip_suffix(key, ".rebase", &key_len)) { name = xmemdupz(key, key_len); type = REBASE; + } else if (strip_suffix(key, ".pushremote", &key_len)) { + name = xmemdupz(key, key_len); + type = PUSH_REMOTE; } else return 0; @@ -305,7 +309,7 @@ static int config_read_branches(const char *key, const char *value, void *cb) space = strchr(value, ' '); } string_list_append(&info->merge, xstrdup(value)); - } else { + } else if (type == REBASE) { int v = git_parse_maybe_bool(value); if (v >= 0) info->rebase = v; @@ -315,6 +319,10 @@ static int config_read_branches(const char *key, const char *value, void *cb) info->rebase = REBASE_MERGES; else if (!strcmp(value, "interactive")) info->rebase = INTERACTIVE_REBASE; + } else { + if (info->push_remote_name) + warning(_("more than one %s"), orig_key); + info->push_remote_name = xstrdup(value); } } return 0; @@ -680,6 +688,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" ) '
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 | 17 +++++++++++++++-- t/t5505-remote.sh | 4 +++- 2 files changed, 18 insertions(+), 3 deletions(-)