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

From: Bert Wesarg <bert.wesarg@googlemail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Git Mailing List <git@vger.kernel.org>,
	Johannes Schindelin <johannes.schindelin@gmx.de>
Subject: Re: [PATCH v2] remote rename: rename branch.<name>.pushRemote config values too
Date: Fri, 17 Jan 2020 21:20:35 +0100	[thread overview]
Message-ID: <CAKPyHN2RwpWhBkTUwERSns0B0Vg7r7CTmhe8p0mZRNeNw4Aagw@mail.gmail.com> (raw)
In-Reply-To: <xmqqeevxex7a.fsf@gitster-ct.c.googlers.com>

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.