Re: [PATCH 4/4] remote: use remote_is_configured() for add and rename
Hi Tomas, On Wed, 17 Feb 2016, Thomas Gummerer wrote: > 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@host.com:team/repo.git.insteadOf myremote && > > Minor nit: I think we should use test_config here. Good point. -- snipsnap -- From: Johannes Schindelin 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 --- t/t5505-remote.sh | 5 + 1 file changed, 5 insertions(+) diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh index 94079a0..949725e 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' ' + test_config url@host.com:team/repo.git.insteadOf myremote && + git remote add myremote g...@host.com:team/repo.git +' + test_expect_success C_LOCALE_OUTPUT 'remote information for the origin' ' ( cd test && -- 2.7.1.windows.2 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] remote: use remote_is_configured() for add and rename
On 02/17, Johannes Schindelin wrote: > 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. Thanks for confirming and the test case so we don't break it again. > 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 > 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 > --- > 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@host.com:team/repo.git.insteadOf myremote && Minor nit: I think we should use test_config here. > + git remote add myremote g...@host.com:team/repo.git > +' > + > test_expect_success C_LOCALE_OUTPUT 'remote information for the origin' ' > ( > cd test && > -- > 2.7.1.windows.2 -- Thomas -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] remote: use remote_is_configured() for add and rename
Hi Peff & Thomas, On Mon, 15 Feb 2016, Jeff King wrote: > 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. 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 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 --- 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@host.com:team/repo.git.insteadOf myremote && + git remote add myremote g...@host.com:team/repo.git +' + test_expect_success C_LOCALE_OUTPUT 'remote information for the origin' ' ( cd test && -- 2.7.1.windows.2 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] remote: use remote_is_configured() for add and rename
On 02/15, Jeff King wrote: > 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. It took me a while too to convince myself there is nothing strange going on. But I could neither find anything in the history, nor could I think of any case that we could break. Thanks for your review! > -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] remote: use remote_is_configured() for add and rename
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. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html