[v2] remote rename: rename branch.<name>.pushRemote config values too
diff mbox series

Message ID ffc8ffc6ede731b182d32a81d044428566acc625.1579253411.git.bert.wesarg@googlemail.com
State New
Headers show
Series
  • [v2] remote rename: rename branch.<name>.pushRemote config values too
Related show

Commit Message

Bert Wesarg Jan. 17, 2020, 9:33 a.m. UTC
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(-)

Comments

Johannes Schindelin Jan. 17, 2020, 11:50 a.m. UTC | #1
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
>
>
Bert Wesarg Jan. 17, 2020, 12:37 p.m. UTC | #2
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
>
Johannes Schindelin Jan. 17, 2020, 1:30 p.m. UTC | #3
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
Bert Wesarg Jan. 17, 2020, 2:40 p.m. UTC | #4
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
Junio C Hamano Jan. 17, 2020, 6:48 p.m. UTC | #5
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.
Bert Wesarg Jan. 17, 2020, 8:20 p.m. UTC | #6
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.
Junio C Hamano Jan. 17, 2020, 9:24 p.m. UTC | #7
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.
Johannes Schindelin Jan. 20, 2020, 11:25 a.m. UTC | #8
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
Bert Wesarg Jan. 20, 2020, 1:14 p.m. UTC | #9
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
Johannes Schindelin Jan. 20, 2020, 1:51 p.m. UTC | #10
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

Patch
diff mbox series

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"
 	)
 '