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

[Junio C Hamano <gitster@pobox.com>]

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.