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 13:37:47 +0100	[thread overview]
Message-ID: <CAKPyHN0eTa9LC35oqsy0Dce0qpOJAx159HR+QyguDt_NZ2he_w@mail.gmail.com> (raw)
In-Reply-To: <nycvar.QRO.7.76.6.2001171245300.46@tvgsbejvaqbjf.bet>

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
>