Re: [PATCH v2] remote rename: rename branch.<name>.pushRemote config values too

From: Bert Wesarg <bert.wesarg@googlemail.com>
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Cc: Git Mailing List <git@vger.kernel.org>,
	Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH v2] remote rename: rename branch.<name>.pushRemote config values too
Date: Fri, 17 Jan 2020 15:40:04 +0100	[thread overview]
Message-ID: <CAKPyHN0qY5odXi2wAv20D4nsNM0r4qO_8JOx9DHiGchJmS8cbw@mail.gmail.com> (raw)
In-Reply-To: <nycvar.QRO.7.76.6.2001171428170.46@tvgsbejvaqbjf.bet>

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