Re: [PATCH] remote: rename also remotes in the branch.<name>.pushRemote config
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 16, 2020, 23:14 UTC
- Message-ID
- <xmqq7e1rf0zg.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <5a8791ef1e262d2078a4ca26b87bfbd777bd4432.1579209398.git.bert.wesarg@googlemail.com>
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.
Show 26 quoted lines
> ---
> 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).
Show 54 quoted lines
> 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.