Re: [PATCH 0/3] Remotes library, take 3

Daniel Barkalow <barkalow@iabervon.org> writes:

Thanks.

        diff --git a/remote.c b/remote.c
        index 32a0acf..1dd2e77 100644
        --- a/remote.c
        +++ b/remote.c
        @@ -189,12 +189,14 @@ struct remote *remote_get(const char *name)
                if (!name)
                        name = default_remote_name;
                ret = make_remote(name, 0);
        -	if (*name == '/')
        -		add_uri(ret, name);
        -	if (!ret->uri)
        -		read_remotes_file(ret);
        +	if (name[0] != '/') {
        +		if (!ret->uri)
        +			read_remotes_file(ret);
        +		if (!ret->uri)
        +			read_branches_file(ret);
        +	}
                if (!ret->uri)
        -		read_branches_file(ret);
        +		add_uri(ret, name);
                if (!ret->uri)
                        return NULL;
                return ret;

This is more similar to the original from builtin-push.c than your previous round, but it is still not identical.

The differences should not matter in real life, but I think we need to make it clear what the differences are to warn users. Here is my reading of the change (please correct me).

Earlier.

  - A name that does not begin with a slash could be a remote
    shorthand.  Check remotes, config and branches in this order
    and stop once a match is found.

  - Otherwise use the name as a literal URI.

This patch.

  - Config always wins.

  - A name that does not begin with a slash could be found in
    remotes or branches; check them in this order.

  - Otherwise use it as is.

Theoretically people _could_ have had a config like

	[remote "/pub"]
		url = blah

but it would never have matched. This ``broken'' config file suddenly start to interfere when somebody does:

	$ git push /pub

Also people may have had a remotes and config of the same name, and currently what is defined in config is ignored, but with the new code, config takes precedence. Which is unarguably good, but still a change I should remember to write down in the release notes, hence prefer to have it clearly described in the commit log message.

We probably would not care about the first difference, but it is easy enough to guard against, I think. Perhaps with this patch?

-- >8 -- parsing remotes: forbid "remote./foo.variable" and fix segfault

Historically we did not pay attention to a remote shorthand defined in the config file whose name starts with a slash (because we always took such a string as a literal localfile URL), but the new organization of the parsing code makes config always take precedence. It does not make much sense to define such a remote shorthand, so protect ourselves against it.

Also, the code forgot that a config variable could be spelled without a value to denote a boolean set to true, in which case the config parser passes a NULL in value parameter, and run xstrdup() on it without checking. Fix this.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---
 remote.c |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)