Junio,
On Fri, Jan 17, 2020 at 7:48 PM Junio C Hamano <gitster@xxxxxxxxx> 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.
