[PATCH 1/2] clone: Fix error message for reference repository

[PATCH 1/2] clone: Fix error message for reference repository

[Aaron Schrab]

Do not report an argument to clone's --reference option is not a local
directory.  Nothing checks for the actual directory so we have no way to
know if whether or not exists.  Telling the user that a directory doesn't
exist when that isn't actually known may lead him or her on the wrong
path to finding the problem.

Signed-off-by: Aaron Schrab <aaron@schrab.com>
---
 builtin/clone.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index f9c380e..0a1e0bf 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -241,7 +241,7 @@ static int add_one_reference(struct string_list_item *item, void *cb_data)
 		free(ref_git);
 		ref_git = ref_git_git;
 	} else if (!is_directory(mkpath("%s/objects", ref_git)))
-		die(_("reference repository '%s' is not a local directory."),
+		die(_("reference repository '%s' is not a local repository."),
 		    item->string);
 
 	strbuf_addf(&alternate, "%s/objects", ref_git);
-- 
1.8.2.677.g7422c62

[PATCH 2/2] clone: Allow repo using gitfile as a reference

[Aaron Schrab]

Try reading gitfile files when processing --reference options to clone.
This will allow, among other things, using a submodule checked out with
a recent version of git as a reference repository without requiring the
user to have internal knowledge of submodule layout.

Signed-off-by: Aaron Schrab <aaron@schrab.com>
---
 builtin/clone.c            | 13 ++++++++++---
 t/t5700-clone-reference.sh |  7 +++++++
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 0a1e0bf..376ded8 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -231,12 +231,19 @@ static void strip_trailing_slashes(char *dir)
 
 static int add_one_reference(struct string_list_item *item, void *cb_data)
 {
-	char *ref_git;
+	char *ref_git, *repo;
 	struct strbuf alternate = STRBUF_INIT;
 
-	/* Beware: real_path() and mkpath() return static buffer */
+	/* Beware: read_gitfile(), real_path() and mkpath() return static buffer */
 	ref_git = xstrdup(real_path(item->string));
-	if (is_directory(mkpath("%s/.git/objects", ref_git))) {
+
+	repo = (char *)read_gitfile(mkpath("%s/.git", ref_git));
+	if (repo) {
+		free(ref_git);
+		ref_git = xstrdup(repo);
+	}
+
+	if (!repo && is_directory(mkpath("%s/.git/objects", ref_git))) {
 		char *ref_git_git = mkpathdup("%s/.git", ref_git);
 		free(ref_git);
 		ref_git = ref_git_git;
diff --git a/t/t5700-clone-reference.sh b/t/t5700-clone-reference.sh
index 2a7b78b..7a9044c 100755
--- a/t/t5700-clone-reference.sh
+++ b/t/t5700-clone-reference.sh
@@ -185,4 +185,11 @@ test_expect_success 'fetch with incomplete alternates' '
 	! grep " want $tag_object" "$U.K"
 '
 
+test_expect_success 'clone using repo with gitfile as a reference' '
+	git clone --separate-git-dir=L A M &&
+	git clone --reference=M A N &&
+	echo "$base_dir/L/objects" > expected &&
+	test_cmp expected "$base_dir/N/.git/objects/info/alternates"
+'
+
 test_done
-- 
1.8.2.677.g7422c62

Re: [PATCH 2/2] clone: Allow repo using gitfile as a reference

[Jonathan Nieder]

Aaron Schrab wrote:

> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -231,12 +231,19 @@ static void strip_trailing_slashes(char *dir)
>  
>  static int add_one_reference(struct string_list_item *item, void *cb_data)
>  {
> -	char *ref_git;
> +	char *ref_git, *repo;
[...]
> +	repo = (char *)read_gitfile(mkpath("%s/.git", ref_git));

Why not make repo a "const char *" and avoid the cast?  The above
would seem to make it too tempting to treat the return value from
read_gitfile() as a mutable buffer instead of a real_path string that
should be copied asap.

Hope that helps,
Jonathan

Re: [PATCH 2/2] clone: Allow repo using gitfile as a reference

[Aaron Schrab]

At 16:51 -0700 07 Apr 2013, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> -	char *ref_git;
>> +	char *ref_git, *repo;
>[...]
>> +	repo = (char *)read_gitfile(mkpath("%s/.git", ref_git));
>
>Why not make repo a "const char *" and avoid the cast?  The above
>would seem to make it too tempting to treat the return value from
>read_gitfile() as a mutable buffer instead of a real_path string that
>should be copied asap.

Good catch.  I'll fix that in the next version.

Thanks.

Re: [PATCH 2/2] clone: Allow repo using gitfile as a reference

[Junio C Hamano]

[ADMINISTRIVIA: please do not redirect a direct reply to you to
other people using Mail-Followup-To.]

Aaron Schrab <aaron@schrab.com> writes:

> At 16:51 -0700 07 Apr 2013, Jonathan Nieder <jrnieder@gmail.com> wrote:
>>> -	char *ref_git;
>>> +	char *ref_git, *repo;
>>[...]
>>> +	repo = (char *)read_gitfile(mkpath("%s/.git", ref_git));
>>
>>Why not make repo a "const char *" and avoid the cast?  The above
>>would seem to make it too tempting to treat the return value from
>>read_gitfile() as a mutable buffer instead of a real_path string that
>>should be copied asap.
>
> Good catch.  I'll fix that in the next version.

Thanks.  The patch otherwise looks good to me.

Re: [PATCH 2/2] clone: Allow repo using gitfile as a reference

[Aaron Schrab]

At 11:00 -0700 08 Apr 2013, Junio C Hamano <gitster@pobox.com> wrote:
>Aaron Schrab <aaron@schrab.com> writes:
>> Good catch.  I'll fix that in the next version.
>
>Thanks.  The patch otherwise looks good to me.

Great, I'll plan to send version 2 of this series later today.

[PATCH v2 0/2] Using gitfile repository with clone --reference

[Aaron Schrab]

Here's the promised second version of this series.

The diff in the first patch is unchanged, but I have made significant
changes to the commit message to hopefully to a better job of describing
why I think the old error message is bad.

For the second patch I've eliminated the need to do a cast.

Although I'm sending these as a series, the changes are independent both
textually and semantically.

[PATCH 1/2] clone: Fix error message for reference repository

[Aaron Schrab]

Do not report that an argument to clone's --reference option is not a
local directory.  Nothing checks for the existence or type of the path
as supplied by the user; checks are only done for particular contents of
the supposed directory, so we have no way to know the status of the
supplied path.  Telling the user that a directory doesn't exist when
that isn't actually known may lead him or her on the wrong path to
finding the problem.

Instead just state that the entered path is not a local repository which
is really all that is known about it.  It could be more helpful to state
the actual paths which were checked, but I believe that giving a good
description of that would be too verbose for a simple error message and
would be too dependent on implementation details.

Signed-off-by: Aaron Schrab <aaron@schrab.com>
---
 builtin/clone.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index f9c380e..0a1e0bf 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -241,7 +241,7 @@ static int add_one_reference(struct string_list_item *item, void *cb_data)
 		free(ref_git);
 		ref_git = ref_git_git;
 	} else if (!is_directory(mkpath("%s/objects", ref_git)))
-		die(_("reference repository '%s' is not a local directory."),
+		die(_("reference repository '%s' is not a local repository."),
 		    item->string);
 
 	strbuf_addf(&alternate, "%s/objects", ref_git);
-- 
1.7.10.4

Re: [PATCH 1/2] clone: Fix error message for reference repository

[Jonathan Nieder]

Aaron Schrab wrote:

> Do not report that an argument to clone's --reference option is not a
> local directory.  Nothing checks for the existence or type of the path
> as supplied by the user; checks are only done for particular contents of
> the supposed directory, so we have no way to know the status of the
> supplied path.

Yes, makes sense.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

My only remaining qualm is that a person could be confused by the
message after trying to pass in --reference=file:///path/to/repo, but
I guess that trial and error would eventually lead such a person in
the right direction.

[PATCH 2/2] clone: Allow repo using gitfile as a reference

[Aaron Schrab]

Try reading gitfile files when processing --reference options to clone.
This will allow, among other things, using a submodule checked out with
a recent version of git as a reference repository without requiring the
user to have internal knowledge of submodule layout.

Signed-off-by: Aaron Schrab <aaron@schrab.com>
---
 builtin/clone.c            |   12 ++++++++++--
 t/t5700-clone-reference.sh |    7 +++++++
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 0a1e0bf..0dc0791 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -232,11 +232,19 @@ static void strip_trailing_slashes(char *dir)
 static int add_one_reference(struct string_list_item *item, void *cb_data)
 {
 	char *ref_git;
+	const char *repo;
 	struct strbuf alternate = STRBUF_INIT;
 
-	/* Beware: real_path() and mkpath() return static buffer */
+	/* Beware: read_gitfile(), real_path() and mkpath() return static buffer */
 	ref_git = xstrdup(real_path(item->string));
-	if (is_directory(mkpath("%s/.git/objects", ref_git))) {
+
+	repo = read_gitfile(mkpath("%s/.git", ref_git));
+	if (repo) {
+		free(ref_git);
+		ref_git = xstrdup(repo);
+	}
+
+	if (!repo && is_directory(mkpath("%s/.git/objects", ref_git))) {
 		char *ref_git_git = mkpathdup("%s/.git", ref_git);
 		free(ref_git);
 		ref_git = ref_git_git;
diff --git a/t/t5700-clone-reference.sh b/t/t5700-clone-reference.sh
index 2a7b78b..7a9044c 100755
--- a/t/t5700-clone-reference.sh
+++ b/t/t5700-clone-reference.sh
@@ -185,4 +185,11 @@ test_expect_success 'fetch with incomplete alternates' '
 	! grep " want $tag_object" "$U.K"
 '
 
+test_expect_success 'clone using repo with gitfile as a reference' '
+	git clone --separate-git-dir=L A M &&
+	git clone --reference=M A N &&
+	echo "$base_dir/L/objects" > expected &&
+	test_cmp expected "$base_dir/N/.git/objects/info/alternates"
+'
+
 test_done
-- 
1.7.10.4

Re: [PATCH 2/2] clone: Allow repo using gitfile as a reference

[Jonathan Nieder]

Aaron Schrab wrote:

> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -232,11 +232,19 @@ static void strip_trailing_slashes(char *dir)
>  static int add_one_reference(struct string_list_item *item, void *cb_data)
>  {
>  	char *ref_git;
> +	const char *repo;
>  	struct strbuf alternate = STRBUF_INIT;
>  
> -	/* Beware: real_path() and mkpath() return static buffer */
> +	/* Beware: read_gitfile(), real_path() and mkpath() return static buffer */
>  	ref_git = xstrdup(real_path(item->string));
> -	if (is_directory(mkpath("%s/.git/objects", ref_git))) {
> +
> +	repo = read_gitfile(mkpath("%s/.git", ref_git));
[...]
> +++ b/t/t5700-clone-reference.sh
> @@ -185,4 +185,11 @@ test_expect_success 'fetch with incomplete alternates' '
>  	! grep " want $tag_object" "$U.K"
>  '
>  
> +test_expect_success 'clone using repo with gitfile as a reference' '
> +	git clone --separate-git-dir=L A M &&
> +	git clone --reference=M A N &&

What should happen if I pass --reference=M/.git?

> +	echo "$base_dir/L/objects" > expected &&

The usual style in tests is to include no space after >redirection
operators.

With those two changes,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Re: [PATCH 2/2] clone: Allow repo using gitfile as a reference

[Aaron Schrab]

At 17:24 -0700 08 Apr 2013, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> +test_expect_success 'clone using repo with gitfile as a reference' '
>> +	git clone --separate-git-dir=L A M &&
>> +	git clone --reference=M A N &&
>
>What should happen if I pass --reference=M/.git?

That isn't supported and I wouldn't expect it to work.  The --reference 
option is documented as taking the location of a repository as the 
argument and I wouldn't consider a .git file to be a repository.  I also 
can't think of a reason that it would be very useful since it should be 
simple to just refer to the directory containing the .git file.  But if 
others disagree, I could be convinced to add support for that.

I also wouldn't consider it breakage if that use would start working, so 
I don't see a point in adding a test to check that that usage fails.

>> +	echo "$base_dir/L/objects" > expected &&
>
>The usual style in tests is to include no space after >redirection
>operators.

Fixed for the next version, pending further comments.

Re: [PATCH 2/2] clone: Allow repo using gitfile as a reference

[Junio C Hamano]

Aaron Schrab <aaron@schrab.com> writes:

> At 17:24 -0700 08 Apr 2013, Jonathan Nieder <jrnieder@gmail.com> wrote:
>>> +test_expect_success 'clone using repo with gitfile as a reference' '
>>> +	git clone --separate-git-dir=L A M &&
>>> +	git clone --reference=M A N &&
>>
>>What should happen if I pass --reference=M/.git?
>
> That isn't supported and I wouldn't expect it to work.  The
> --reference option is documented as taking the location of a
> repository as the argument and I wouldn't consider a .git file to be a
> repository.  I also can't think of a reason that it would be very
> useful since it should be simple to just refer to the directory
> containing the .git file.  But if others disagree, I could be
> convinced to add support for that.

If M/.git weren't a gitfile that points elsewhere, that request
ought to work, no?  A gitfile is the moral equilvalent of a symbolic
link, meant to help people on platforms and filesystems that lack
symbolic links, so in that sense, not supporting the case goes
against the whole reason why we have added support for gitfile in
the first place, I think.

Re: [PATCH 2/2] clone: Allow repo using gitfile as a reference

[Aaron Schrab]

At 09:47 -0700 09 Apr 2013, Junio C Hamano <junio@pobox.com> wrote:
>Aaron Schrab <aaron@schrab.com> writes:
>> But if others disagree, I could be convinced to add support for that.
>
>If M/.git weren't a gitfile that points elsewhere, that request
>ought to work, no?  A gitfile is the moral equilvalent of a symbolic
>link, meant to help people on platforms and filesystems that lack
>symbolic links, so in that sense, not supporting the case goes
>against the whole reason why we have added support for gitfile in
>the first place, I think.

OK, I'm convinced.  I'll modify it to support that as well.

[PATCH v3 0/2] Using gitfile repository with clone --reference

[Aaron Schrab]

Here's the third version of my series for dealing with gitfiles in clone
--reference.

The first patch is unchanged from the previous version except for the
addition of a Reviewed-by line.

The second patch has been modified so that it now supports having a .git
file supplied as the argument to the option directly rather than only
dealing with that if the containing directory was supplied.  This makes
the first patch from the series more important, since it would make even
less sense to complain that the path isn't a directory when a
non-directory is acceptable.

I've also fixed the minor style issue in the test script from the previous
versions.

[PATCH v3 1/2] clone: Fix error message for reference repository

[Aaron Schrab]

Do not report that an argument to clone's --reference option is not a
local directory.  Nothing checks for the existence or type of the path
as supplied by the user; checks are only done for particular contents of
the supposed directory, so we have no way to know the status of the
supplied path.  Telling the user that a directory doesn't exist when
that isn't actually known may lead him or her on the wrong path to
finding the problem.

Instead just state that the entered path is not a local repository which
is really all that is known about it.  It could be more helpful to state
the actual paths which were checked, but I believe that giving a good
description of that would be too verbose for a simple error message and
would be too dependent on implementation details.

Signed-off-by: Aaron Schrab <aaron@schrab.com>
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

diff --git a/builtin/clone.c b/builtin/clone.c
index f9c380e..0a1e0bf 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -241,7 +241,7 @@ static int add_one_reference(struct string_list_item *item, void *cb_data)
 		free(ref_git);
 		ref_git = ref_git_git;
 	} else if (!is_directory(mkpath("%s/objects", ref_git)))
-		die(_("reference repository '%s' is not a local directory."),
+		die(_("reference repository '%s' is not a local repository."),
 		    item->string);
 
 	strbuf_addf(&alternate, "%s/objects", ref_git);
-- 
1.8.2.677.g9202ef0

[PATCH v3 2/2] clone: Allow repo using gitfile as a reference

[Aaron Schrab]

Try reading gitfile files when processing --reference options to clone.
This will allow, among other things, using a submodule checked out with
a recent version of git as a reference repository without requiring the
user to have internal knowledge of submodule layout.

Signed-off-by: Aaron Schrab <aaron@schrab.com>

diff --git a/builtin/clone.c b/builtin/clone.c
index 0a1e0bf..58fee98 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -232,11 +232,21 @@ static void strip_trailing_slashes(char *dir)
 static int add_one_reference(struct string_list_item *item, void *cb_data)
 {
 	char *ref_git;
+	const char *repo;
 	struct strbuf alternate = STRBUF_INIT;
 
-	/* Beware: real_path() and mkpath() return static buffer */
+	/* Beware: read_gitfile(), real_path() and mkpath() return static buffer */
 	ref_git = xstrdup(real_path(item->string));
-	if (is_directory(mkpath("%s/.git/objects", ref_git))) {
+
+	repo = read_gitfile(ref_git);
+	if (!repo)
+		repo = read_gitfile(mkpath("%s/.git", ref_git));
+	if (repo) {
+		free(ref_git);
+		ref_git = xstrdup(repo);
+	}
+
+	if (!repo && is_directory(mkpath("%s/.git/objects", ref_git))) {
 		char *ref_git_git = mkpathdup("%s/.git", ref_git);
 		free(ref_git);
 		ref_git = ref_git_git;
diff --git a/t/t5700-clone-reference.sh b/t/t5700-clone-reference.sh
index 2a7b78b..719d778 100755
--- a/t/t5700-clone-reference.sh
+++ b/t/t5700-clone-reference.sh
@@ -185,4 +185,17 @@ test_expect_success 'fetch with incomplete alternates' '
 	! grep " want $tag_object" "$U.K"
 '
 
+test_expect_success 'clone using repo with gitfile as a reference' '
+	git clone --separate-git-dir=L A M &&
+	git clone --reference=M A N &&
+	echo "$base_dir/L/objects" >expected &&
+	test_cmp expected "$base_dir/N/.git/objects/info/alternates"
+'
+
+test_expect_success 'clone using repo pointed at by gitfile as reference' '
+	git clone --reference=M/.git A O &&
+	echo "$base_dir/L/objects" >expected &&
+	test_cmp expected "$base_dir/O/.git/objects/info/alternates"
+'
+
 test_done
-- 
1.8.2.677.g9202ef0

Re: [PATCH v3 0/2] Using gitfile repository with clone --reference

[Junio C Hamano]

Aaron Schrab <aaron@schrab.com> writes:

> Here's the third version of my series for dealing with gitfiles in clone
> --reference.
>
> The first patch is unchanged from the previous version except for the
> addition of a Reviewed-by line.
>
> The second patch has been modified so that it now supports having a .git
> file supplied as the argument to the option directly rather than only
> dealing with that if the containing directory was supplied.  This makes
> the first patch from the series more important, since it would make even
> less sense to complain that the path isn't a directory when a
> non-directory is acceptable.
>
> I've also fixed the minor style issue in the test script from the previous
> versions.

Thanks, will queue.

Re: [PATCH 1/2] clone: Fix error message for reference repository

[Jonathan Nieder]

Hi Aaron,

Aaron Schrab wrote:

> Do not report an argument to clone's --reference option is not a local
> directory.  Nothing checks for the actual directory so we have no way to
> know if whether or not exists.  Telling the user that a directory doesn't
> exist when that isn't actually known may lead him or her on the wrong
> path to finding the problem.

I don't understand the above explanation.  Could you give an example?

[...]
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -241,7 +241,7 @@ static int add_one_reference(struct string_list_item *item, void *cb_data)
>  		free(ref_git);
>  		ref_git = ref_git_git;
>  	} else if (!is_directory(mkpath("%s/objects", ref_git)))
> -		die(_("reference repository '%s' is not a local directory."),
> +		die(_("reference repository '%s' is not a local repository."),

"is_directory" calls stat and checks if its target is a directory.  Is
the problem that "/path/to/repo.git" might be a directory but
"/path/to/repo.git/objects" may not?

Would it make sense for the message to say something like the
following?

	fatal: alternate object store '/path/to/repo.git/objects' is not a local directory

Thanks and hope that helps,
Jonathan

Re: [PATCH 1/2] clone: Fix error message for reference repository

[Aaron Schrab]

At 16:48 -0700 07 Apr 2013, Jonathan Nieder <jrnieder@gmail.com> wrote:
>Hi Aaron,

Thanks for the feedback.

>Aaron Schrab wrote:
>
>> Do not report an argument to clone's --reference option is not a local
>> directory.  Nothing checks for the actual directory so we have no way to
>> know if whether or not exists.  Telling the user that a directory doesn't
>> exist when that isn't actually known may lead him or her on the wrong
>> path to finding the problem.
>
>I don't understand the above explanation.  Could you give an example?

I originally noticed this while trying to use a submodule as a reference 
repository.  Since that submodule was first checked out using a recent 
version of git it used a .git file rather than having a .git directory.  
This caused the checks to fail, and the misleading error message had me 
checking for a typo in the path which I'd supplied.

I'll attempt to clarify that message in the next version.

>> --- a/builtin/clone.c
>> +++ b/builtin/clone.c
>> @@ -241,7 +241,7 @@ static int add_one_reference(struct string_list_item *item, void *cb_data)
>>  		free(ref_git);
>>  		ref_git = ref_git_git;
>>  	} else if (!is_directory(mkpath("%s/objects", ref_git)))
>> -		die(_("reference repository '%s' is not a local directory."),
>> +		die(_("reference repository '%s' is not a local repository."),
>
>"is_directory" calls stat and checks if its target is a directory.  Is
>the problem that "/path/to/repo.git" might be a directory but
>"/path/to/repo.git/objects" may not?

In my case the issue was that /path/to/repo is a directory, but 
/path/to/repo/.git/objects (which is checked shortly before the above 
context) didn't exist since /path/to/repo/.git is a file.

>Would it make sense for the message to say something like the
>following?
>
>	fatal: alternate object store '/path/to/repo.git/objects' is not a local directory

That would also avoid lying to the user.  But if combined with the 
second patch in this series it could cause confusion for a different 
reason.  Once .git files are honored, the path reported there may have 
no relation to the path supplied by the user.

Re: [PATCH 1/2] clone: Fix error message for reference repository

[Aaron Schrab]

At 20:06 -0400 07 Apr 2013, I wrote:
>At 16:48 -0700 07 Apr 2013, Jonathan Nieder <jrnieder@gmail.com> wrote:
>>Would it make sense for the message to say something like the
>>following?
>>
>>	fatal: alternate object store '/path/to/repo.git/objects' is not a local directory
>
>That would also avoid lying to the user.  But if combined with the 
>second patch in this series it could cause confusion for a different 
>reason.  Once .git files are honored, the path reported there may have 
>no relation to the path supplied by the user.

Thinking on this further, even without the companion patch there's 
another issue.  The problem isn't just that 
/path/supplied/by/user/objects isn't a directory.  It's that neither 
that nor /path/supplied/by/user/.git/objects is a directory.  And in 
many cases it's the latter that the user would be expecting to have been 
used.  Reporting on just the last name checked isn't really a good 
description of what's going on.

Re: [PATCH 1/2] clone: Fix error message for reference repository

[Junio C Hamano]

Aaron Schrab <aaron@schrab.com> writes:

> At 16:48 -0700 07 Apr 2013, Jonathan Nieder <jrnieder@gmail.com> wrote:
>
>>> Do not report an argument to clone's --reference option is not a local
>>> directory.  Nothing checks for the actual directory so we have no way to
>>> know if whether or not exists.  Telling the user that a directory doesn't
>>> exist when that isn't actually known may lead him or her on the wrong
>>> path to finding the problem.
>>
>>I don't understand the above explanation.  Could you give an example?
>
> I originally noticed this while trying to use a submodule as a
> reference repository.

I do agree that it would be nice to dereference .git gitfile when we
deal with --reference argument, but you do not want to use in-tree
repository of a submodule working tree.  What happens when you have
to check out a version of the containing superproject that did not
have the submodule you are borrowing from?  The directory will
disappear, leaving the borrowing repository still pointing at it
with its .git/objects/info/alternates file, no?

Re: [PATCH 1/2] clone: Fix error message for reference repository

[Aaron Schrab]

At 06:58 -0700 08 Apr 2013, Junio C Hamano <gitster@pobox.com> wrote:
>I do agree that it would be nice to dereference .git gitfile when we
>deal with --reference argument, but you do not want to use in-tree
>repository of a submodule working tree.  What happens when you have
>to check out a version of the containing superproject that did not
>have the submodule you are borrowing from?  The directory will
>disappear, leaving the borrowing repository still pointing at it
>with its .git/objects/info/alternates file, no?

No, submodule directories don't get removed when you checkout a version 
which didn't contain that submodule.  I believe that there are plans to 
change that for submodules which store the repository data under the 
containing project's .git directory; but removing the submodule working 
tree would not affect a repository using that submodule as a reference, 
since the reading of the .git file is only done during the initial 
clone.  I don't think that the risk of such a repository being deleted 
or moved is substantially higher than for any other repository.

Re: [PATCH 1/2] clone: Fix error message for reference repository

[Junio C Hamano]

Aaron Schrab <aaron@schrab.com> writes:

> At 06:58 -0700 08 Apr 2013, Junio C Hamano <gitster@pobox.com> wrote:
>>I do agree that it would be nice to dereference .git gitfile when we
>>deal with --reference argument, but you do not want to use in-tree
>>repository of a submodule working tree.  What happens when you have
>>to check out a version of the containing superproject that did not
>>have the submodule you are borrowing from?  The directory will
>>disappear, leaving the borrowing repository still pointing at it
>>with its .git/objects/info/alternates file, no?
>
> No, submodule directories don't get removed when you checkout a
> version which didn't contain that submodule.  

In the old world order, we did not use .git gitfile.

The version of the superproject had a submodule at dirA/ and
dirA/.git used to be a real directory.  "clone --reference
/path/to/super/dirA/.git" can borrow objects from there, and will
write /path/to/super/dirA/.git/objects (which is a real object
store) to the resulting repository's objects/info/alternates.

You switch to a version of the superproject with a plain file at
dirA/ or there is nothing at dirA.  The checkout will fail and you
need to manually rectify the situation [*1*], but after that is
done, you do not have any repository at /path/to/super/dirA/.git
anymore.

That was the reason why I recommended against the practice.

In the new world order, we use dirA/.git gitfile.

"clone --reference /path/to/super/dirA/.git" does not anticipate .git
could be a gitfile, but it can be fixed to dereference it and point
at "/path/to/super/.git/modules/moduleA", which will stay there
across branch switching at the supermodule level.

"clone" has to store /path/to/super/.git/modules/moduleA in
$GIT_DIR/objects/info/alternates of the new repository by
dereferencing the value given to --reference.  By doing so, what is
in the working tree of the superproject would not matter at the time
of access in the new repository.

So you are right that we do not remove in the new world order, but
then --reference can be given to point at the real location ;-)


[Footnote]

*1* ... for which fundamental fix was made to use dirA/.git gitfile
in the submodule working tree in the new world order.

Re: [PATCH 1/2] clone: Fix error message for reference repository

[Aaron Schrab]

At 08:30 -0700 08 Apr 2013, Junio C Hamano <gitster@pobox.com> wrote:
>You switch to a version of the superproject with a plain file at
>dirA/ or there is nothing at dirA.  The checkout will fail and you
>need to manually rectify the situation [*1*], but after that is
>done, you do not have any repository at /path/to/super/dirA/.git
>anymore.
>
>That was the reason why I recommended against the practice.

So you're essentially saying you don't want to support using a new-world 
submodule as a reference because using an old-world submodule as such is 
likely to be problematic?  Even though the type of submodule that is 
actually likely to cause problems would currently be accepted as a 
reference repository?  That seems somewhat perverse to me.

Also, nothing in this series is strictly about submodules; that just 
happens to be what I was working with when I noticed the issue.  It 
would apply to any repository created with --separate-git-dir, although 
submodules are likely to be the most common occurrence by far.

>So you are right that we do not remove in the new world order, but
>then --reference can be given to point at the real location ;-)

Yes, that's definitely a possibility.  But I think that the location of 
the work tree for a repository is much more likely to come to a user's 
mind than the location of a non-bare repository.  Especially when 
dealing with submodules where the repository location was decided for 
the user, and is somewhat of an implementation detail that the user 
shouldn't need to care about.

Re: [PATCH 1/2] clone: Fix error message for reference repository

[Junio C Hamano]

Aaron Schrab <aaron@schrab.com> writes:

> At 08:30 -0700 08 Apr 2013, Junio C Hamano <gitster@pobox.com> wrote:
>>You switch to a version of the superproject with a plain file at
>>dirA/ or there is nothing at dirA.  The checkout will fail and you
>>need to manually rectify the situation [*1*], but after that is
>>done, you do not have any repository at /path/to/super/dirA/.git
>>anymore.
>>
>>That was the reason why I recommended against the practice.
>
> So you're essentially saying you don't want to support using a
> new-world submodule as a reference because using an old-world
> submodule as such is likely to be problematic?

In general I am in favor of resolving a gitfile given to --reference
when clone interprets it, and have it use the location of the real
underlying object store when it grabs objects not in there from the
origin and store the location of the real underlying object store in
the objects/info/alternates of the newly created repository.  But
that is not limited to the gitfile used at the root level of a
submodule checkout.

Blindly using .git at the root level of submodule checkout as a
reference is what I was recommending against as a general
precaution.  You may be dealing with an old-style submodule
checkout.

Re: [PATCH 1/2] clone: Fix error message for reference repository

[Aaron Schrab]

At 10:57 -0700 08 Apr 2013, Junio C Hamano <gitster@pobox.com> wrote:
>In general I am in favor of resolving a gitfile given to --reference
>when clone interprets it, and have it use the location of the real
>underlying object store when it grabs objects not in there from the
>origin and store the location of the real underlying object store in
>the objects/info/alternates of the newly created repository.  But
>that is not limited to the gitfile used at the root level of a
>submodule checkout.

Yes, I agree that it's not limited to submodules.  The commit message 
for the second part of this series only mentioned submodules because I 
suspect that is by far the most common use of gitfiles.  The commit 
message for the first didn't even mention submodules at all, they were 
only brought up because I was asked about what lead to me having an 
issue.

>Blindly using .git at the root level of submodule checkout as a
>reference is what I was recommending against as a general
>precaution.

I agree with that.  But I still don't think it's relevant to this patch 
series.

>You may be dealing with an old-style submodule checkout.

No, the submodule in question was done with the new style.  If it were 
an old-style checkout my attempt to clone using that as a reference 
would have worked without issue (at least at clone time).

Re: [PATCH 1/2] clone: Fix error message for reference repository

[Junio C Hamano]

Aaron Schrab <aaron@schrab.com> writes:

>>You may be dealing with an old-style submodule checkout.
>
> No, the submodule in question was done with the new style.

I know that and I wasn't talking about _your_ particular case.

I just wanted to make sure people who are reading this thread from
sidelines (or finding with search engine later) applied that pattern
blindly.