[PATCH 0/3] Remotes library, take 3

[PATCH 0/3] Remotes library, take 3

[Daniel Barkalow]

This series is the same as the previous version, except that it matches 
the current behavior of builtin-push with respect to treating names as 
literal URIs.

	-Daniel
*This .sig left intentionally blank*

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

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> writes:

> This series is the same as the previous version, except that it matches 
> the current behavior of builtin-push with respect to treating names as 
> literal URIs.

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(-)

diff --git a/remote.c b/remote.c
index 1dd2e77..05df196 100644
--- a/remote.c
+++ b/remote.c
@@ -149,7 +149,24 @@ static int handle_config(const char *key, const char *value)
 	subkey = strrchr(name, '.');
 	if (!subkey)
 		return error("Config with no key for remote %s", name);
+	if (*subkey == '/')
+		return error("Config remote shorthand cannot begin with '/': %s", name);
 	remote = make_remote(name, subkey - name);
+	if (!value) {
+		/* if we ever have a boolean variable, e.g. "remote.*.disabled"
+		 * [remote "frotz"]
+		 *	disabled
+		 * is a valid way to set it to true; we get NULL in value so
+		 * we need to handle it here.
+		 *
+		 * if (!strcmp(subkey, ".disabled")) {
+		 * 	val = git_config_bool(key, value);
+		 *	return 0;
+		 * } else
+		 *
+		 */
+		return error("Config with no value for remote %s", name);
+	}
 	if (!strcmp(subkey, ".url")) {
 		add_uri(remote, xstrdup(value));
 	} else if (!strcmp(subkey, ".push")) {

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

[Daniel Barkalow]

On Sat, 12 May 2007, Junio C Hamano wrote:

> Daniel Barkalow <barkalow@iabervon.org> writes:
> 
> > This series is the same as the previous version, except that it matches 
> > the current behavior of builtin-push with respect to treating names as 
> > literal URIs.
> 
> 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

This is true, and I missed it before. Feel free to mention it in the 
appropriate commit message.

> 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.

This is in the message for [1/3] already, along with the other change: it 
will use the current branch's remote, if there is one, instead of "origin" 
if no repository is given on the command line.

I was only claiming here (incorrectly, it turns out) that the configured 
remote vs. literal URI behavior is the same with this series.

> We probably would not care about the first difference, but it is
> easy enough to guard against, I think.  Perhaps with this patch?
> 
> -- >8 --
> diff --git a/remote.c b/remote.c
> index 1dd2e77..05df196 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -149,7 +149,24 @@ static int handle_config(const char *key, const char *value)
>  	subkey = strrchr(name, '.');
>  	if (!subkey)
>  		return error("Config with no key for remote %s", name);
> +	if (*subkey == '/')
> +		return error("Config remote shorthand cannot begin with '/': %s", name);

Maybe just return? If we change the behavior to give an error in this 
situation, we might as well make the config file actually take effect 
instead.

	-Daniel
*This .sig left intentionally blank*