patch, 4 partsgit remote improvements

> 95b567c7 ("use skip_prefix to avoid repeating strings") transformed
> calls using starts_with() and then skipping the length of the prefix to
> skip_prefix() calls.  In remote.c there are a few calls like:
> 
>   if (starts_with(foo, "bar"))
>       foo += 3
> 
> These calls weren't touched by the commit mentioned above, but can
> benefit from the same treatment to avoid magic numbers.

> This is definitely an improvement, but I think we can actually go a step
> further here, and use parse_config_key. Like:
>
> -       if (starts_with(key, "branch.")) {
> -               name = key + 7;
> -               subkey = strrchr(name, '.');
> -               if (!subkey)
> +       if (starts_with(key, "branch", &name, &namelen, &subkey)) {

> +               if (!name)
>                         return 0;
> -               branch = make_branch(name, subkey - name);
> +               branch = make_branch(name, namelen);
>                 if (!strcmp(subkey, ".remote")) {
>                         return git_config_string(&branch->remote_name, key, value);
>                 } else if (!strcmp(subkey, ".pushremote")) {

> On Mon, Feb 15, 2016 at 1:18 PM, Jeff King <peff@peff.net> wrote:
> > This is definitely an improvement, but I think we can actually go a step
> > further here, and use parse_config_key. Like:
> >
> > -       if (starts_with(key, "branch.")) {
> > -               name = key + 7;
> > -               subkey = strrchr(name, '.');
> > -               if (!subkey)
> > +       if (starts_with(key, "branch", &name, &namelen, &subkey)) {
> 
> I guess you meant: s/starts_with/parse_config_key/

> On Mon, Feb 15, 2016 at 06:42:27PM +0100, Thomas Gummerer wrote:
>
> > 95b567c7 ("use skip_prefix to avoid repeating strings") transformed
> > calls using starts_with() and then skipping the length of the prefix to
> > skip_prefix() calls.  In remote.c there are a few calls like:
> >
> >   if (starts_with(foo, "bar"))
> >       foo += 3
> >
> > These calls weren't touched by the commit mentioned above, but can
> > benefit from the same treatment to avoid magic numbers.
>
> This is definitely an improvement, but I think we can actually go a step
> further here, and use parse_config_key. Like:

> diff --git a/remote.c b/remote.c
> index 21e4ec3..8d2c3ca 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -318,15 +318,14 @@ static void read_branches_file(struct remote *remote)
>  static int handle_config(const char *key, const char *value, void *cb)
>  {
>  	const char *name;
> +	int namelen;
>  	const char *subkey;
>  	struct remote *remote;
>  	struct branch *branch;
> -	if (starts_with(key, "branch.")) {
> -		name = key + 7;
> -		subkey = strrchr(name, '.');
> -		if (!subkey)
> +	if (starts_with(key, "branch", &name, &namelen, &subkey)) {
> +		if (!name)
>  			return 0;
> -		branch = make_branch(name, subkey - name);
> +		branch = make_branch(name, namelen);
>  		if (!strcmp(subkey, ".remote")) {
>  			return git_config_string(&branch->remote_name, key, value);
>  		} else if (!strcmp(subkey, ".pushremote")) {
>
> and so on. The difference in lines of code isn't that great, but I think
> it makes the resulting code more obvious to read.
>
> -Peff

> The remote_is_configured() function allows checking whether a remote
> exists or not.  The function however only works if remote_get() wasn't
> called before calling it.  In addition, it only checks the configuration
> for remotes, but not remotes or branches files.
> 
> Make use of the origin member of struct remote instead, which indicates
> where the remote comes from.  It will be set to some value if the remote
> is configured in any file in the repository, but is initialized to 0 if
> the remote is only created in make_remote().

> diff --git a/remote.h b/remote.h
> index 4fd7a0f..7a5ee77 100644
> --- a/remote.h
> +++ b/remote.h
> @@ -5,7 +5,7 @@
>  #include "hashmap.h"
>  
>  enum {
> -	REMOTE_CONFIG,
> +	REMOTE_CONFIG = 1,
>  	REMOTE_REMOTES,
>  	REMOTE_BRANCHES
>  };

> On Mon, Feb 15, 2016 at 06:42:28PM +0100, Thomas Gummerer wrote:
>
> > The remote_is_configured() function allows checking whether a remote
> > exists or not.  The function however only works if remote_get() wasn't
> > called before calling it.  In addition, it only checks the configuration
> > for remotes, but not remotes or branches files.
> >
> > Make use of the origin member of struct remote instead, which indicates
> > where the remote comes from.  It will be set to some value if the remote
> > is configured in any file in the repository, but is initialized to 0 if
> > the remote is only created in make_remote().
>
> Makes sense. I wonder if we would want to give this an explicit slot in
> the enum. I.e.:
>
> > diff --git a/remote.h b/remote.h
> > index 4fd7a0f..7a5ee77 100644
> > --- a/remote.h
> > +++ b/remote.h
> > @@ -5,7 +5,7 @@
> >  #include "hashmap.h"
> >
> >  enum {
> > -	REMOTE_CONFIG,
> > +	REMOTE_CONFIG = 1,
> >  	REMOTE_REMOTES,
> >  	REMOTE_BRANCHES
> >  };
>
> Add in "REMOTE_UNCONFIGURED = 0" here. It makes no difference to
> correctness, but is perhaps documents what is going on a bit better.

> When converting the git remote command to a builtin in 211c89 ("Make
> git-remote a builtin"), a few calls to check if a remote exists were
> converted from:
>        if (!exists $remote->{$name}) {
>        	  [...]
> to:
>        remote = remote_get(argv[1]);
>        if (!remote)
>           [...]
> 
> The new check is not quite correct, because remote_get() never returns
> NULL if a name is given.  This leaves us with the somewhat cryptic error
> message "error: Could not remove config section 'remote.test'", if we
> are trying to remove a remote that does not exist, or a similar error if
> we try to rename a remote.
> 
> Use the remote_is_configured() function to check whether the remote
> actually exists, and die with a more sensible error message ("No such
> remote: $remotename") instead if it doesn't.
> 
> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> ---
>  builtin/remote.c  |  4 ++--
>  t/t5505-remote.sh | 18 ++++++++++++++++++
>  2 files changed, 20 insertions(+), 2 deletions(-)

> Both remote add and remote rename use a slightly different hand-rolled
> check if the remote exits.  The hand-rolled check may have some subtle
> cases in which it might fail to detect when a remote already exists.
> One such case was fixed in fb86e32 ("git remote: allow adding remotes
> agreeing with url.<...>.insteadOf").  Another case is when a remote is
> configured as follows:
> 
>   [remote "foo"]
>     vcs = bar
> 
> If we try to run `git remote add foo bar` with the above remote
> configuration, git segfaults.  This change fixes it.
> 
> In addition, git remote rename $existing foo with the configuration for
> foo as above silently succeeds, even though foo already exists,
> modifying its configuration.  With this patch it fails with "remote foo
> already exists".

> diff --git a/builtin/remote.c b/builtin/remote.c
> index 981c487..bd57f1b 100644
> --- a/builtin/remote.c
> +++ b/builtin/remote.c
> @@ -186,10 +186,7 @@ static int add(int argc, const char **argv)
>  	url = argv[1];
>  
>  	remote = remote_get(name);
> -	if (remote && (remote->url_nr > 1 ||
> -			(strcmp(name, remote->url[0]) &&
> -				strcmp(url, remote->url[0])) ||
> -			remote->fetch_refspec_nr))
> +	if (remote_is_configured(remote))
>  		die(_("remote %s already exists."), name);

> On Mon, Feb 15, 2016 at 06:42:30PM +0100, Thomas Gummerer wrote:
>
> > Both remote add and remote rename use a slightly different hand-rolled
> > check if the remote exits.  The hand-rolled check may have some subtle
> > cases in which it might fail to detect when a remote already exists.
> > One such case was fixed in fb86e32 ("git remote: allow adding remotes
> > agreeing with url.<...>.insteadOf").  Another case is when a remote is
> > configured as follows:
> >
> >   [remote "foo"]
> >     vcs = bar
> >
> > If we try to run `git remote add foo bar` with the above remote
> > configuration, git segfaults.  This change fixes it.
> >
> > In addition, git remote rename $existing foo with the configuration for
> > foo as above silently succeeds, even though foo already exists,
> > modifying its configuration.  With this patch it fails with "remote foo
> > already exists".
>
> Checking is_configured() certainly sounds like a better test, but...
>
> > diff --git a/builtin/remote.c b/builtin/remote.c
> > index 981c487..bd57f1b 100644
> > --- a/builtin/remote.c
> > +++ b/builtin/remote.c
> > @@ -186,10 +186,7 @@ static int add(int argc, const char **argv)
> >  	url = argv[1];
> >
> >  	remote = remote_get(name);
> > -	if (remote && (remote->url_nr > 1 ||
> > -			(strcmp(name, remote->url[0]) &&
> > -				strcmp(url, remote->url[0])) ||
> > -			remote->fetch_refspec_nr))
> > +	if (remote_is_configured(remote))
> >  		die(_("remote %s already exists."), name);
>
> This original is quite confusing. I thought at first that there was
> perhaps something going on with allowing repeated re-configuration of
> the same remote, as long as some parameters matched. I.e., I am
> wondering if there is a case here that does _not_ segfault, that we
> would be breaking.
>
> But reading over fb86e32dcc, I think I have convinced myself that it was
> merely an ad-hoc check for "is_configured", and using that function is a
> better replacement.

> On Mon, Feb 15, 2016 at 06:42:30PM +0100, Thomas Gummerer wrote:
> 
> > Both remote add and remote rename use a slightly different hand-rolled
> > check if the remote exits.  The hand-rolled check may have some subtle
> > cases in which it might fail to detect when a remote already exists.
> > One such case was fixed in fb86e32 ("git remote: allow adding remotes
> > agreeing with url.<...>.insteadOf").  Another case is when a remote is
> > configured as follows:
> > 
> >   [remote "foo"]
> >     vcs = bar
> > 
> > If we try to run `git remote add foo bar` with the above remote
> > configuration, git segfaults.  This change fixes it.
> > 
> > In addition, git remote rename $existing foo with the configuration for
> > foo as above silently succeeds, even though foo already exists,
> > modifying its configuration.  With this patch it fails with "remote foo
> > already exists".
> 
> Checking is_configured() certainly sounds like a better test, but...
> 
> > diff --git a/builtin/remote.c b/builtin/remote.c
> > index 981c487..bd57f1b 100644
> > --- a/builtin/remote.c
> > +++ b/builtin/remote.c
> > @@ -186,10 +186,7 @@ static int add(int argc, const char **argv)
> >  	url = argv[1];
> >  
> >  	remote = remote_get(name);
> > -	if (remote && (remote->url_nr > 1 ||
> > -			(strcmp(name, remote->url[0]) &&
> > -				strcmp(url, remote->url[0])) ||
> > -			remote->fetch_refspec_nr))
> > +	if (remote_is_configured(remote))
> >  		die(_("remote %s already exists."), name);
> 
> This original is quite confusing. I thought at first that there was
> perhaps something going on with allowing repeated re-configuration of
> the same remote, as long as some parameters matched. I.e., I am
> wondering if there is a case here that does _not_ segfault, that we
> would be breaking.
> 
> But reading over fb86e32dcc, I think I have convinced myself that it was
> merely an ad-hoc check for "is_configured", and using that function is a
> better replacement.

> Hi Peff & Thomas,
>
> On Mon, 15 Feb 2016, Jeff King wrote:
> > This original is quite confusing. I thought at first that there was
> > perhaps something going on with allowing repeated re-configuration of
> > the same remote, as long as some parameters matched. I.e., I am
> > wondering if there is a case here that does _not_ segfault, that we
> > would be breaking.
> >
> > But reading over fb86e32dcc, I think I have convinced myself that it was
> > merely an ad-hoc check for "is_configured", and using that function is a
> > better replacement.
>
> Yes, yes, yes. Y'all are absolutely correct. I shoulda added a test case
> right away, to make sure not only that what I fixed does not get broken in
> the future, but also to document *what* was fixed, exactly.

> So, belatedly, here goes a patch that verifies what that commit was
> supposed to fix, and yes, it passes with Thomas' changes (Junio, would you
> please apply this on top of tg/git-remote?):
>
> -- snipsnap --
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> Date: Wed, 17 Feb 2016 14:45:59 +0100
> Subject: [PATCH] t5505: 'remote add x y' should work when url.y.insteadOf = x
>
> This is the test missing from fb86e32 (git remote: allow adding
> remotes agreeing with url.<...>.insteadOf, 2014-12-23): we should
> allow adding a remote with the URL when it agrees with the
> url.<...>.insteadOf setting.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  t/t5505-remote.sh | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
> index 94079a0..19e8e34 100755
> --- a/t/t5505-remote.sh
> +++ b/t/t5505-remote.sh
> @@ -51,6 +51,11 @@ test_expect_success setup '
>  	git clone one test
>  '
>
> +test_expect_success 'add remote whose URL agrees with url.<...>.insteadOf' '
> +	git config url.git@host.com:team/repo.git.insteadOf myremote &&

> +	git remote add myremote git@host.com:team/repo.git
> +'
> +
>  test_expect_success C_LOCALE_OUTPUT 'remote information for the origin' '
>  	(
>  		cd test &&
> --
> 2.7.1.windows.2

> On 02/17, Johannes Schindelin wrote:
> >
> > diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
> > index 94079a0..19e8e34 100755
> > --- a/t/t5505-remote.sh
> > +++ b/t/t5505-remote.sh
> > @@ -51,6 +51,11 @@ test_expect_success setup '
> >  	git clone one test
> >  '
> >
> > +test_expect_success 'add remote whose URL agrees with url.<...>.insteadOf' '
> > +	git config url.git@host.com:team/repo.git.insteadOf myremote &&
> 
> Minor nit: I think we should use test_config here.