Git development
 help / color / mirror / Atom feed
* tracking repository
@ 2008-03-15 19:35 kenneth johansson
  2008-03-16  2:42 ` Junio C Hamano
  0 siblings, 1 reply; 16+ messages in thread
From: kenneth johansson @ 2008-03-15 19:35 UTC (permalink / raw)
  To: git

I was trying to create a repository used only to track different linux git 
repositories. The goal with this was to maximize object sharing and having 
one local copy of the data. 

But I think I managed to paint myself into a corner. Here was my initial 
setup. 

create a directory 
"mkdir linux"

create a git data base
"cd linux; git --bare init"

Add a few linux repositories with
"git --bare remote add [name] [url]"

then download the objects/branches with
"git remote update"

This works great and it will track all changes in the remote repositories 
without me having to worry about it aborting due to merge issues with my 
local branch or remote  doing rebase on some branch.

The problem is that it is useless :( I can't find any way to use a 
repository with only remotes in it. Is there a way to make a clone of a 
remote branch in a repository ??

Now it is not entirely useless since I can reuse the objects downloaded by 
setting GIT_OBJECT_DIRECTORY. This works quite well until "git gc --prune" 
is used. I leave it up to the reader to figure out what happens then :(

So I guess there should be some warning about using GIT_OBJECT_DIRECTORY 
that points to the same object store for different repositories. It's 
obvious but still a warning in the man page could be helpful.

GIT_ALTERNATE_OBJECT_DIRECTORIES works much better. The only potential 
problem I see is if I set this is set in my environment when I login and 
then do operation on my tracking repository's it will now point into it's 
own object directory. I have not tried that yet.

The downside of only using the objects is that I need to setup the remotes 
again in my clone. 

Now has anybody tried to do something similar? is there a better way?

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: tracking repository
  2008-03-15 19:35 tracking repository kenneth johansson
@ 2008-03-16  2:42 ` Junio C Hamano
  2008-03-16 20:02   ` kenneth johansson
  0 siblings, 1 reply; 16+ messages in thread
From: Junio C Hamano @ 2008-03-16  2:42 UTC (permalink / raw)
  To: kenneth johansson; +Cc: git

kenneth johansson <ken@kenjo.org> writes:

> This works great and it will track all changes in the remote repositories 
> without me having to worry about it aborting due to merge issues with my 
> local branch or remote  doing rebase on some branch.
>
> The problem is that it is useless :( I can't find any way to use a 
> repository with only remotes in it. Is there a way to make a clone of a 
> remote branch in a repository ??

Usually a clone with a work tree ("git clone $elsewhere") is configured to
keep copies of branches at the remote in remotes/origin in order to track
them, and that is done by having this in its .git/config:

	[remote "origin"]
		url = $elsewhere
		fetch = +refs/heads/*:refs/remotes/origin/*
	[branch "master"]
        	remote = origin
                merge = refs/heads/master

This lets you to have your own work on your own "master", and have changes
on the other end merged when you "git pull" from there, while keeping
track of other branches on the other end in remotes/origin/ namespace.

You do not want to have any of your own work in this repository, however,
so there is no reason to separate the remote ones in remotes/origin/
namespace.  You would want "mirroring".

You can have in your $GIT_DIR/config something like this:

        [remote "origin"]
		url = $elsewhere
                fetch = +refs/heads/*:refs/heads/*

You can edit the configuration file yourself to read like above, and then
"git fetch" will keep a copy of remote "master" branch in your local
"master" (and similarly to all the branches over there).

Modern git allows this setup via "git remote add --mirror"; it is merely a
convenience wrapper and it is perfectly fine to edit the configuration
file yourself without using it.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: tracking repository
  2008-03-16  2:42 ` Junio C Hamano
@ 2008-03-16 20:02   ` kenneth johansson
  2008-03-16 20:38     ` Junio C Hamano
  0 siblings, 1 reply; 16+ messages in thread
From: kenneth johansson @ 2008-03-16 20:02 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

On Sat, 2008-03-15 at 19:42 -0700, Junio C Hamano wrote:

> You do not want to have any of your own work in this repository, however,
> so there is no reason to separate the remote ones in remotes/origin/
> namespace.  You would want "mirroring".
> 
> You can have in your $GIT_DIR/config something like this:
> 
>         [remote "origin"]
> 		url = $elsewhere
>                 fetch = +refs/heads/*:refs/heads/*

> Modern git allows this setup via "git remote add --mirror"; it is merely a
> convenience wrapper and it is perfectly fine to edit the configuration
> file yourself without using it.

I tried using the option --mirror but then the config end up in a way
that 
all remote repositories master branch maps to exactly the same name. 

However changing the config file manually I did get one that works more
or 
less as I intended. 

So with the below config I can create an empty directory and in that do 
"git --bare init"
copy in the config file and any objects I have laying around. 
then simply put a cron job that once a day do a
"git remote update"

the resulting repository is then possible to clone. And as long as no
repacking is done the object data will be shared. But to share data even
after a repack I guess I need to use GIT_ALTERNATE_OBJECT_DIRECTORIES
for my local clone. And then I need to be very careful when doing any
pruning on the download repository since my local clone could need data
that is no longer needed in the download repository. 

What would a safe procedure be ?? copy all data in the object hierarchy
from the download repository to my local clones then do a gc in them
starting from the download repository. 

------------
[core]
	repositoryformatversion = 0
	filemode = true
	bare = true
[remote "linus"]
	url =
git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git
	fetch = +refs/heads/*:refs/heads/*
[remote "stable_2.6.12"]
	url =
git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-2.6.12.y.git
	fetch = +refs/heads/*:refs/heads/stable_2.6.12_*
[remote "stable_2.6.13"]
	url =
git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-2.6.13.y.git
	fetch = +refs/heads/*:refs/heads/stable_2.6.13_*
[remote "stable_2.6.14"]
	url =
git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-2.6.14.y.git
	fetch = +refs/heads/*:refs/heads/stable_2.6.14_*
[remote "stable_2.6.15"]
	url =
git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-2.6.15.y.git
	fetch = +refs/heads/*:refs/heads/stable_2.6.15_*
[remote "stable_2.6.16"]
	url =
git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-2.6.16.y.git
	fetch = +refs/heads/*:refs/heads/stable_2.6.16_*
[remote "stable_2.6.17"]
	url =
git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-2.6.17.y.git
	fetch = +refs/heads/*:refs/heads/stable_2.6.17_*
[remote "stable_2.6.18"]
	url =
git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-2.6.18.y.git
	fetch = +refs/heads/*:refs/heads/stable_2.6.18_*
[remote "stable_2.6.19"]
	url =
git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-2.6.19.y.git
	fetch = +refs/heads/*:refs/heads/stable_2.6.19_*
[remote "stable_2.6.20"]
	url =
git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-2.6.20.y.git
	fetch = +refs/heads/*:refs/heads/stable_2.6.20_*
[remote "stable_2.6.21"]
	url =
git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-2.6.21.y.git
	fetch = +refs/heads/*:refs/heads/stable_2.6.21_*
[remote "stable_2.6.22"]
	url =
git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-2.6.22.y.git
	fetch = +refs/heads/*:refs/heads/stable_2.6.22_*
[remote "stable_2.6.23"]
	url =
git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-2.6.23.y.git
	fetch = +refs/heads/*:refs/heads/stable_2.6.23_*
[remote "stable_2.6.24"]
	url =
git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-2.6.24.y.git
	fetch = +refs/heads/*:refs/heads/stable_2.6.24_*

------------

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: tracking repository
  2008-03-16 20:02   ` kenneth johansson
@ 2008-03-16 20:38     ` Junio C Hamano
  2008-03-16 21:28       ` Daniel Barkalow
  0 siblings, 1 reply; 16+ messages in thread
From: Junio C Hamano @ 2008-03-16 20:38 UTC (permalink / raw)
  To: Daniel Barkalow; +Cc: git, kenneth johansson

kenneth johansson <ken@kenjo.org> writes:

> git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git
> 	fetch = +refs/heads/*:refs/heads/*
> [remote "stable_2.6.12"]
> 	url =
> git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-2.6.12.y.git
> 	fetch = +refs/heads/*:refs/heads/stable_2.6.12_*

Daniel, I think we are looking at a regression.  The latter style, * at
the end but not immediately following a slash, should never have worked.
Wildcard expansion function should be erroring out when it sees something
like this.

Once we fix that regression, the above would stop working (in)correctly.
Rewrite it to something like this right now will make it keep working:

 	fetch = +refs/heads/*:refs/heads/stable_2.6.12/*

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: tracking repository
  2008-03-16 20:38     ` Junio C Hamano
@ 2008-03-16 21:28       ` Daniel Barkalow
  2008-03-16 21:57         ` Junio C Hamano
  2008-03-17  0:35         ` Junio C Hamano
  0 siblings, 2 replies; 16+ messages in thread
From: Daniel Barkalow @ 2008-03-16 21:28 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, kenneth johansson

On Sun, 16 Mar 2008, Junio C Hamano wrote:

> kenneth johansson <ken@kenjo.org> writes:
> 
> > git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git
> > 	fetch = +refs/heads/*:refs/heads/*
> > [remote "stable_2.6.12"]
> > 	url =
> > git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-2.6.12.y.git
> > 	fetch = +refs/heads/*:refs/heads/stable_2.6.12_*
> 
> Daniel, I think we are looking at a regression.  The latter style, * at
> the end but not immediately following a slash, should never have worked.
> Wildcard expansion function should be erroring out when it sees something
> like this.

I'm not sure any older code actually enforced this, either

We don't currently have any concept of an invalid refspec; we just have 
things that fall back to not being patterns and not being possible to 
match (due to one or the other side being invalid as a ref name).

Here's a patch to make the pattern logic require a slash before the *:
---------
commit 7aa15c359bcfc7a3c87345435b81ef41e1f59800
Author: Daniel Barkalow <barkalow@iabervon.org>
Date:   Sun Mar 16 17:26:41 2008 -0400

    Require / before * in pattern refspecs
    
    We don't want to have "+refs/heads/*:refs/heads/something_*" match
    "refs/heads/master" to "refs/heads/something_master".
    
    Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>

diff --git a/remote.c b/remote.c
index f3f7375..fffde34 100644
--- a/remote.c
+++ b/remote.c
@@ -404,18 +404,17 @@ struct refspec *parse_ref_spec(int nr_refspec, const char **refspec)
 			rs[i].force = 1;
 			sp++;
 		}
-		gp = strchr(sp, '*');
+		gp = strstr(sp, "/*");
 		ep = strchr(sp, ':');
 		if (gp && ep && gp > ep)
 			gp = NULL;
 		if (ep) {
 			if (ep[1]) {
-				const char *glob = strchr(ep + 1, '*');
+				const char *glob = strstr(ep + 1, "/*");
 				if (!glob)
 					gp = NULL;
 				if (gp)
-					rs[i].dst = xstrndup(ep + 1,
-							     glob - ep - 1);
+					rs[i].dst = xstrndup(ep + 1, glob - ep);
 				else
 					rs[i].dst = xstrdup(ep + 1);
 			}
@@ -424,7 +423,7 @@ struct refspec *parse_ref_spec(int nr_refspec, const char **refspec)
 		}
 		if (gp) {
 			rs[i].pattern = 1;
-			ep = gp;
+			ep = gp + 1;
 		}
 		rs[i].src = xstrndup(sp, ep - sp);
 	}

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: tracking repository
  2008-03-16 21:28       ` Daniel Barkalow
@ 2008-03-16 21:57         ` Junio C Hamano
  2008-03-16 22:18           ` Daniel Barkalow
  2008-03-17  0:35         ` Junio C Hamano
  1 sibling, 1 reply; 16+ messages in thread
From: Junio C Hamano @ 2008-03-16 21:57 UTC (permalink / raw)
  To: Daniel Barkalow; +Cc: git, kenneth johansson

Daniel Barkalow <barkalow@iabervon.org> writes:

> I'm not sure any older code actually enforced this, either

I am fairly sure the old code was written with the intention in mind (I
wrote it, in other words).  It meant to accept refs/<anything>/* and no
other wildcard.

Does your patch require * to be at the end?

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: tracking repository
  2008-03-16 21:57         ` Junio C Hamano
@ 2008-03-16 22:18           ` Daniel Barkalow
  2008-03-16 22:30             ` Junio C Hamano
  0 siblings, 1 reply; 16+ messages in thread
From: Daniel Barkalow @ 2008-03-16 22:18 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, kenneth johansson

On Sun, 16 Mar 2008, Junio C Hamano wrote:

> Daniel Barkalow <barkalow@iabervon.org> writes:
> 
> > I'm not sure any older code actually enforced this, either
> 
> I am fairly sure the old code was written with the intention in mind (I
> wrote it, in other words).  It meant to accept refs/<anything>/* and no
> other wildcard.

I know that's all it was supposed to accept, but I don't remember seeing 
anything to enforce that. Actually, I don't now remember what the old code 
looked like at all, so I might be wrong about that.

Is "refs/*:refs/*" (mirror everything, including weird stuff) supposed to 
be prohibited?

> Does your patch require * to be at the end?

Looks like it just ignores anything after a *. Want checks for that as 
well?

	-Daniel
*This .sig left intentionally blank*

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: tracking repository
  2008-03-16 22:18           ` Daniel Barkalow
@ 2008-03-16 22:30             ` Junio C Hamano
  2008-03-16 23:01               ` Junio C Hamano
  0 siblings, 1 reply; 16+ messages in thread
From: Junio C Hamano @ 2008-03-16 22:30 UTC (permalink / raw)
  To: Daniel Barkalow; +Cc: git, kenneth johansson

Daniel Barkalow <barkalow@iabervon.org> writes:

> Is "refs/*:refs/*" (mirror everything, including weird stuff) supposed to 
> be prohibited?

No.  In fact "remote add --mirror" actively creates such.  See my other
message about design level issues.

>> Does your patch require * to be at the end?
>
> Looks like it just ignores anything after a *. Want checks for that as 
> well?

Surely.  Starting strict and making it looser later is much easier than
starting loose and incoherent and having to deal with the resulting mess
the code appears to allow people to make in their configuration.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: tracking repository
  2008-03-16 22:30             ` Junio C Hamano
@ 2008-03-16 23:01               ` Junio C Hamano
  2008-03-16 23:11                 ` Daniel Barkalow
  0 siblings, 1 reply; 16+ messages in thread
From: Junio C Hamano @ 2008-03-16 23:01 UTC (permalink / raw)
  To: Daniel Barkalow; +Cc: Johannes Schindelin, git, kenneth johansson

Junio C Hamano <gitster@pobox.com> writes:

> Daniel Barkalow <barkalow@iabervon.org> writes:
>
>> Is "refs/*:refs/*" (mirror everything, including weird stuff) supposed to 
>> be prohibited?
>
> No.  In fact "remote add --mirror" actively creates such.  See my other
> message about design level issues.

I think something like this is needed.  It still has an independent issue
that this is now called by "git remote show" or "git remote prune", and it
will die with a nonsense "refusing to create" error message, though.

The error, as far as I can tell, is half about a misconfigured config
(e.g. "fetch = refs/heads/*:refs/remotes/[]?/*") and half about screwy
remote repository (e.g. a misnamed "[]?" branch on the remote end can try
to update a broken "refs/remotes/origin/[]?" even the configuration is a
perfectly valid "fetch = refs/heads/*:refs/remotes/origin/*").  It may
make sense to reword the error message to "ignoring" from "refusing" and
do just that without dying here.  I dunno.

 remote.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/remote.c b/remote.c
index f3f7375..fbcb03c 100644
--- a/remote.c
+++ b/remote.c
@@ -1007,9 +1007,12 @@ int get_fetch_map(const struct ref *remote_refs,
 	}
 
 	for (rm = ref_map; rm; rm = rm->next) {
-		if (rm->peer_ref && check_ref_format(rm->peer_ref->name + 5))
-			die("* refusing to create funny ref '%s' locally",
-			    rm->peer_ref->name);
+		if (rm->peer_ref) {
+			int st = check_ref_format(rm->peer_ref->name + 5);
+			if (st && st != CHECK_REF_FORMAT_ONELEVEL)
+				die("* refusing to create funny ref '%s'"
+				    " locally", rm->peer_ref->name);
+		}
 	}
 
 	if (ref_map)

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: tracking repository
  2008-03-16 23:01               ` Junio C Hamano
@ 2008-03-16 23:11                 ` Daniel Barkalow
  2008-03-17  0:17                   ` Junio C Hamano
  0 siblings, 1 reply; 16+ messages in thread
From: Daniel Barkalow @ 2008-03-16 23:11 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Johannes Schindelin, git, kenneth johansson

On Sun, 16 Mar 2008, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Daniel Barkalow <barkalow@iabervon.org> writes:
> >
> >> Is "refs/*:refs/*" (mirror everything, including weird stuff) supposed to 
> >> be prohibited?
> >
> > No.  In fact "remote add --mirror" actively creates such.  See my other
> > message about design level issues.
> 
> I think something like this is needed.  It still has an independent issue
> that this is now called by "git remote show" or "git remote prune", and it
> will die with a nonsense "refusing to create" error message, though.
> 
> The error, as far as I can tell, is half about a misconfigured config
> (e.g. "fetch = refs/heads/*:refs/remotes/[]?/*") and half about screwy
> remote repository (e.g. a misnamed "[]?" branch on the remote end can try
> to update a broken "refs/remotes/origin/[]?" even the configuration is a
> perfectly valid "fetch = refs/heads/*:refs/remotes/origin/*").  It may
> make sense to reword the error message to "ignoring" from "refusing" and
> do just that without dying here.  I dunno.

Yeah, I think that's right. (And this patch is also right)

	-Daniel
*This .sig left intentionally blank*

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: tracking repository
  2008-03-16 23:11                 ` Daniel Barkalow
@ 2008-03-17  0:17                   ` Junio C Hamano
  0 siblings, 0 replies; 16+ messages in thread
From: Junio C Hamano @ 2008-03-17  0:17 UTC (permalink / raw)
  To: Daniel Barkalow; +Cc: Johannes Schindelin, git, kenneth johansson

Daniel Barkalow <barkalow@iabervon.org> writes:

> On Sun, 16 Mar 2008, Junio C Hamano wrote:
> ...
>> ...  It still has an independent issue
>> that this is now called by "git remote show" or "git remote prune", and it
>> will die with a nonsense "refusing to create" error message, though.
>> 
>> The error, as far as I can tell, is half about a misconfigured config
>> (e.g. "fetch = refs/heads/*:refs/remotes/[]?/*") and half about screwy
>> remote repository (e.g. a misnamed "[]?" branch on the remote end can try
>> to update a broken "refs/remotes/origin/[]?" even the configuration is a
>> perfectly valid "fetch = refs/heads/*:refs/remotes/origin/*").  It may
>> make sense to reword the error message to "ignoring" from "refusing" and
>> do just that without dying here.  I dunno.
>
> Yeah, I think that's right. (And this patch is also right)

Which means that an error checking (i.e. dying) needs to be added to
whatever reads from config to find "refs/heads/*:refs/remotes/[]?/*" to
cover the first half.  That's an configuration error and we should not
just say "ignoring" but actively urge the user to correct, no?

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: tracking repository
  2008-03-16 21:28       ` Daniel Barkalow
  2008-03-16 21:57         ` Junio C Hamano
@ 2008-03-17  0:35         ` Junio C Hamano
  2008-03-17  2:13           ` Daniel Barkalow
  2008-03-17  2:37           ` Daniel Barkalow
  1 sibling, 2 replies; 16+ messages in thread
From: Junio C Hamano @ 2008-03-17  0:35 UTC (permalink / raw)
  To: Daniel Barkalow; +Cc: git, kenneth johansson

Daniel Barkalow <barkalow@iabervon.org> writes:

> We don't currently have any concept of an invalid refspec;

We don't? or just that parse_ref_spec() does not detect one?

> ... we just have 
> things that fall back to not being patterns and not being possible to 
> match (due to one or the other side being invalid as a ref name).

I am afraid that is an invitation for more bugs and confusions.

It probably is not too late to fix this; users would rather want to see
their misconfigurations clearly flagged as such, rather than the code
letting bogosity through silently and doing something that does not
exactly match what they configured.

> diff --git a/remote.c b/remote.c
> index f3f7375..fffde34 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -404,18 +404,17 @@ struct refspec *parse_ref_spec(int nr_refspec, const char **refspec)
>  			rs[i].force = 1;
>  			sp++;
>  		}
> -		gp = strchr(sp, '*');
> +		gp = strstr(sp, "/*");
>  		ep = strchr(sp, ':');
>  		if (gp && ep && gp > ep)
>  			gp = NULL;

How would this trigger?  We find * (or /*) but that is the one on the LHS,
which means the spec was like "refs/heads/foobar:refs/remotes/origin/*",
and it makes me wonder if we should mark this as an configuration error.

Did erroring out on "gp && ep && gp > ep" here have issues (i.e. reject a
valid configuration)?

>  		if (ep) {
>  			if (ep[1]) {
> -				const char *glob = strchr(ep + 1, '*');
> +				const char *glob = strstr(ep + 1, "/*");
>  				if (!glob)
>  					gp = NULL;
>  				if (gp)
> -					rs[i].dst = xstrndup(ep + 1,
> -							     glob - ep - 1);
> +					rs[i].dst = xstrndup(ep + 1, glob - ep);

This truncates "refs/heads/*:refs/remotes/origin/*/bar" as if it did not
have "/bar" without any error indication.  The same questions apply.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: tracking repository
  2008-03-17  0:35         ` Junio C Hamano
@ 2008-03-17  2:13           ` Daniel Barkalow
  2008-03-17  2:37           ` Daniel Barkalow
  1 sibling, 0 replies; 16+ messages in thread
From: Daniel Barkalow @ 2008-03-17  2:13 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, kenneth johansson

On Sun, 16 Mar 2008, Junio C Hamano wrote:

> Daniel Barkalow <barkalow@iabervon.org> writes:
> 
> > We don't currently have any concept of an invalid refspec;
> 
> We don't? or just that parse_ref_spec() does not detect one?

I don't think we ever formalized anything; it just makes sure not to 
actually create anything bad, and doesn't give any feedback on the 
configuration.

> > ... we just have 
> > things that fall back to not being patterns and not being possible to 
> > match (due to one or the other side being invalid as a ref name).
> 
> I am afraid that is an invitation for more bugs and confusions.
> 
> It probably is not too late to fix this; users would rather want to see
> their misconfigurations clearly flagged as such, rather than the code
> letting bogosity through silently and doing something that does not
> exactly match what they configured.

I think I wasn't sure that aborting on invalid input wouldn't cause worse 
problems. There were actually a number of tests, IIRC, that required that 
certain configurations silently did nothing, which I mostly left alone.

Also, it's possible that we're parsing refspecs because we're using "git 
remote" to modify the configuration to replace an invalid refspec, and it 
would be unfortunate to die() because the remote has an invalid refspec.
 
> > diff --git a/remote.c b/remote.c
> > index f3f7375..fffde34 100644
> > --- a/remote.c
> > +++ b/remote.c
> > @@ -404,18 +404,17 @@ struct refspec *parse_ref_spec(int nr_refspec, const char **refspec)
> >  			rs[i].force = 1;
> >  			sp++;
> >  		}
> > -		gp = strchr(sp, '*');
> > +		gp = strstr(sp, "/*");
> >  		ep = strchr(sp, ':');
> >  		if (gp && ep && gp > ep)
> >  			gp = NULL;
> 
> How would this trigger?  We find * (or /*) but that is the one on the LHS,
> which means the spec was like "refs/heads/foobar:refs/remotes/origin/*",
> and it makes me wonder if we should mark this as an configuration error.

It's that case (there's a *, but not until after the :); I think the 
history was that the code first just looked for <a>:<b>, and used it for 
non-pattern matches, and then started using <a>/*:<b>/* as a pattern 
first, and then I made the C version match that shell version.

> Did erroring out on "gp && ep && gp > ep" here have issues (i.e. reject a
> valid configuration)?

Nope.

> >  		if (ep) {
> >  			if (ep[1]) {
> > -				const char *glob = strchr(ep + 1, '*');
> > +				const char *glob = strstr(ep + 1, "/*");
> >  				if (!glob)
> >  					gp = NULL;
> >  				if (gp)
> > -					rs[i].dst = xstrndup(ep + 1,
> > -							     glob - ep - 1);
> > +					rs[i].dst = xstrndup(ep + 1, glob - ep);
> 
> This truncates "refs/heads/*:refs/remotes/origin/*/bar" as if it did not
> have "/bar" without any error indication.  The same questions apply.

That one was just an oversight. I was just glad I didn't have to make it 
actually support that refspec and have it do the obvious (but annoying to 
implement) thing.

	-Daniel
*This .sig left intentionally blank*

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: tracking repository
  2008-03-17  0:35         ` Junio C Hamano
  2008-03-17  2:13           ` Daniel Barkalow
@ 2008-03-17  2:37           ` Daniel Barkalow
  2008-03-17  7:48             ` Junio C Hamano
  1 sibling, 1 reply; 16+ messages in thread
From: Daniel Barkalow @ 2008-03-17  2:37 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, kenneth johansson

On Sun, 16 Mar 2008, Junio C Hamano wrote:

> Daniel Barkalow <barkalow@iabervon.org> writes:
> 
> > We don't currently have any concept of an invalid refspec;
> 
> We don't? or just that parse_ref_spec() does not detect one?
> 
> > ... we just have 
> > things that fall back to not being patterns and not being possible to 
> > match (due to one or the other side being invalid as a ref name).
> 
> I am afraid that is an invitation for more bugs and confusions.

Yeah, we're definitely too lenient. t3200-branch has been using the 
refspec "=" since July without anybody noticing that it's wrong.

	-Daniel
*This .sig left intentionally blank*

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: tracking repository
  2008-03-17  2:37           ` Daniel Barkalow
@ 2008-03-17  7:48             ` Junio C Hamano
  2008-03-17 16:23               ` Daniel Barkalow
  0 siblings, 1 reply; 16+ messages in thread
From: Junio C Hamano @ 2008-03-17  7:48 UTC (permalink / raw)
  To: Daniel Barkalow; +Cc: Jay Soffian, Johannes Schindelin, git, kenneth johansson

Daniel Barkalow <barkalow@iabervon.org> writes:

> On Sun, 16 Mar 2008, Junio C Hamano wrote:
>
>> Daniel Barkalow <barkalow@iabervon.org> writes:
>> 
>> > We don't currently have any concept of an invalid refspec;
>> 
>> We don't? or just that parse_ref_spec() does not detect one?
>> 
>> > ... we just have 
>> > things that fall back to not being patterns and not being possible to 
>> > match (due to one or the other side being invalid as a ref name).
>> 
>> I am afraid that is an invitation for more bugs and confusions.
>
> Yeah, we're definitely too lenient. t3200-branch has been using the 
> refspec "=" since July without anybody noticing that it's wrong.

You mean these that came in 6f084a5 (branch --track: code cleanup and
saner handling of local branches, 2007-07-10), right?

Will you fix them while you come up with a patch to tighten the parsing?
Fixing these does seem to trigger problems in later parts of the test
sequence.

---

 t/t3200-branch.sh |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 38a90ad..48b8a45 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -155,10 +155,10 @@ test_expect_success 'test tracking setup via config' \
 
 test_expect_success 'avoid ambiguous track' '
 	git config branch.autosetupmerge true &&
-	git config remote.ambi1.url = lalala &&
-	git config remote.ambi1.fetch = refs/heads/lalala:refs/heads/master &&
-	git config remote.ambi2.url = lilili &&
-	git config remote.ambi2.fetch = refs/heads/lilili:refs/heads/master &&
+	git config remote.ambi1.url lalala &&
+	git config remote.ambi1.fetch refs/heads/lalala:refs/heads/master &&
+	git config remote.ambi2.url lilili &&
+	git config remote.ambi2.fetch refs/heads/lilili:refs/heads/master &&
 	git branch all1 master &&
 	test -z "$(git config branch.all1.merge)"
 '

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: tracking repository
  2008-03-17  7:48             ` Junio C Hamano
@ 2008-03-17 16:23               ` Daniel Barkalow
  0 siblings, 0 replies; 16+ messages in thread
From: Daniel Barkalow @ 2008-03-17 16:23 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jay Soffian, Johannes Schindelin, git, kenneth johansson

On Mon, 17 Mar 2008, Junio C Hamano wrote:

> Daniel Barkalow <barkalow@iabervon.org> writes:
> 
> > On Sun, 16 Mar 2008, Junio C Hamano wrote:
> >
> >> Daniel Barkalow <barkalow@iabervon.org> writes:
> >> 
> >> > We don't currently have any concept of an invalid refspec;
> >> 
> >> We don't? or just that parse_ref_spec() does not detect one?
> >> 
> >> > ... we just have 
> >> > things that fall back to not being patterns and not being possible to 
> >> > match (due to one or the other side being invalid as a ref name).
> >> 
> >> I am afraid that is an invitation for more bugs and confusions.
> >
> > Yeah, we're definitely too lenient. t3200-branch has been using the 
> > refspec "=" since July without anybody noticing that it's wrong.
> 
> You mean these that came in 6f084a5 (branch --track: code cleanup and
> saner handling of local branches, 2007-07-10), right?

Yup. That actually makes me wonder if git-config should complain if it 
doesn't do anything due to the value_regex not matching anything.

> Will you fix them while you come up with a patch to tighten the parsing?
> Fixing these does seem to trigger problems in later parts of the test
> sequence.

Yeah, we just need to remove those options once that case finishes (or I 
suppose that test could go at the end).

	-Daniel
*This .sig left intentionally blank*

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2008-03-17 16:24 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-03-15 19:35 tracking repository kenneth johansson
2008-03-16  2:42 ` Junio C Hamano
2008-03-16 20:02   ` kenneth johansson
2008-03-16 20:38     ` Junio C Hamano
2008-03-16 21:28       ` Daniel Barkalow
2008-03-16 21:57         ` Junio C Hamano
2008-03-16 22:18           ` Daniel Barkalow
2008-03-16 22:30             ` Junio C Hamano
2008-03-16 23:01               ` Junio C Hamano
2008-03-16 23:11                 ` Daniel Barkalow
2008-03-17  0:17                   ` Junio C Hamano
2008-03-17  0:35         ` Junio C Hamano
2008-03-17  2:13           ` Daniel Barkalow
2008-03-17  2:37           ` Daniel Barkalow
2008-03-17  7:48             ` Junio C Hamano
2008-03-17 16:23               ` Daniel Barkalow

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox