| Message ID | 5a8791ef1e262d2078a4ca26b87bfbd777bd4432.1579209398.git.bert.wesarg@googlemail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | remote: rename also remotes in the branch.<name>.pushRemote config | expand |
Bert Wesarg <bert.wesarg@googlemail.com> writes: > Subject: Re: [PATCH] remote: rename also remotes in the branch.<name>.pushRemote config > Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com> This is way under-explained. It is not even clear what "also" in "remote: rename also remotes..." refers to. It hints that something that is not "remotes" is renamed, but the readers do not know what it is. It is not clear when that said renaming is done, either. Is it supposed to be a bugfix? If so, readers would need the issue being addressed described, perhaps like so: When X is done, Y is renamed, but at the same time Z should also be renamed, but it is not. > --- > Cc: Johannes Schindelin <johannes.schindelin@gmx.de> > --- > builtin/remote.c | 16 ++++++++++++++-- > t/t5505-remote.sh | 4 +++- > 2 files changed, 17 insertions(+), 3 deletions(-) > > diff --git a/builtin/remote.c b/builtin/remote.c > index 96bbe828fe..ddceba868a 100644 > --- a/builtin/remote.c > +++ b/builtin/remote.c > @@ -246,7 +246,7 @@ static int add(int argc, const char **argv) > } > > struct branch_info { > - char *remote_name; > + char *remote_name, *push_remote_name; > struct string_list merge; > enum { > NO_REBASE, NORMAL_REBASE, INTERACTIVE_REBASE, REBASE_MERGES > @@ -269,13 +269,16 @@ 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, PUSH_REMOTE, MERGE, REBASE } type; Is there a reason why this new one has to come between REMOTE and MERGE? Otherwise, the usual convention is either to add alphabetically (if the list is sorted alphabetically from the beginning) or add to the end (otherwise). > 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, ".pushremote", &key_len)) { > + name = xmemdupz(key, key_len); > + type = PUSH_REMOTE; > } else if (strip_suffix(key, ".merge", &key_len)) { > name = xmemdupz(key, key_len); > type = MERGE; > @@ -294,6 +297,10 @@ static int config_read_branches(const char *key, const char *value, void *cb) > if (info->remote_name) > warning(_("more than one %s"), orig_key); > info->remote_name = xstrdup(value); > + } else if (type == PUSH_REMOTE) { > + if (info->push_remote_name) > + warning(_("more than one %s"), orig_key); > + info->push_remote_name = xstrdup(value); > } else if (type == MERGE) { > char *space = strchr(value, ' '); > value = abbrev_branch(value); > @@ -680,6 +687,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" > ) > ' OK, so the issue the patch wants to address is git remote rename X Y ought to rename branch.X.Z (for any value of Z) to branch.Y.Z but it forgot to do so for Z==pushRemote? If that is the case, I have to wonder if the patch is making the situation better or even worse. Shouldn't we be not even caring what Z is by not having to list these specific keys? I dunno.
Hi Bert, On Thu, 16 Jan 2020, Bert Wesarg wrote: > Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com> > --- > Cc: Johannes Schindelin <johannes.schindelin@gmx.de> > --- > builtin/remote.c | 16 ++++++++++++++-- > t/t5505-remote.sh | 4 +++- > 2 files changed, 17 insertions(+), 3 deletions(-) The patch looks obviously good to me. I would have wished for slightly more information in the commit message, but it is okay as it is. Thanks, Dscho > > diff --git a/builtin/remote.c b/builtin/remote.c > index 96bbe828fe..ddceba868a 100644 > --- a/builtin/remote.c > +++ b/builtin/remote.c > @@ -246,7 +246,7 @@ static int add(int argc, const char **argv) > } > > struct branch_info { > - char *remote_name; > + char *remote_name, *push_remote_name; > struct string_list merge; > enum { > NO_REBASE, NORMAL_REBASE, INTERACTIVE_REBASE, REBASE_MERGES > @@ -269,13 +269,16 @@ 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, PUSH_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, ".pushremote", &key_len)) { > + name = xmemdupz(key, key_len); > + type = PUSH_REMOTE; > } else if (strip_suffix(key, ".merge", &key_len)) { > name = xmemdupz(key, key_len); > type = MERGE; > @@ -294,6 +297,10 @@ static int config_read_branches(const char *key, const char *value, void *cb) > if (info->remote_name) > warning(_("more than one %s"), orig_key); > info->remote_name = xstrdup(value); > + } else if (type == PUSH_REMOTE) { > + if (info->push_remote_name) > + warning(_("more than one %s"), orig_key); > + info->push_remote_name = xstrdup(value); > } else if (type == MERGE) { > char *space = strchr(value, ' '); > value = abbrev_branch(value); > @@ -680,6 +687,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 Junio, On Thu, 16 Jan 2020, Junio C Hamano wrote: > Bert Wesarg <bert.wesarg@googlemail.com> writes: > > > 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" > > ) > > ' > > OK, so the issue the patch wants to address is > > git remote rename X Y > > ought to rename branch.X.Z (for any value of Z) to branch.Y.Z but it > forgot to do so for Z==pushRemote? > > If that is the case, I have to wonder if the patch is making the > situation better or even worse. Shouldn't we be not even caring > what Z is by not having to list these specific keys? I dunno. I think what the code does is not rename branch.X.Z to branch.Y.Z, but for every `branch.B.pushRemote` whose value is `X`, it will now be set to `Y`. So we really should not touch, say, `branch.X.description` because it has nothing to do with any renamed remote. And yes, I agree, the commit message would have made for a splendid place to explain something like this. Ciao, Dscho
diff --git a/builtin/remote.c b/builtin/remote.c index 96bbe828fe..ddceba868a 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -246,7 +246,7 @@ static int add(int argc, const char **argv) } struct branch_info { - char *remote_name; + char *remote_name, *push_remote_name; struct string_list merge; enum { NO_REBASE, NORMAL_REBASE, INTERACTIVE_REBASE, REBASE_MERGES @@ -269,13 +269,16 @@ 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, PUSH_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, ".pushremote", &key_len)) { + name = xmemdupz(key, key_len); + type = PUSH_REMOTE; } else if (strip_suffix(key, ".merge", &key_len)) { name = xmemdupz(key, key_len); type = MERGE; @@ -294,6 +297,10 @@ static int config_read_branches(const char *key, const char *value, void *cb) if (info->remote_name) warning(_("more than one %s"), orig_key); info->remote_name = xstrdup(value); + } else if (type == PUSH_REMOTE) { + if (info->push_remote_name) + warning(_("more than one %s"), orig_key); + info->push_remote_name = xstrdup(value); } else if (type == MERGE) { char *space = strchr(value, ' '); value = abbrev_branch(value); @@ -680,6 +687,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" ) '
Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com> --- Cc: Johannes Schindelin <johannes.schindelin@gmx.de> --- builtin/remote.c | 16 ++++++++++++++-- t/t5505-remote.sh | 4 +++- 2 files changed, 17 insertions(+), 3 deletions(-)