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

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

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?

[...]

"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

At 16:48 -0700 07 Apr 2013, Jonathan Nieder [off-list ref] wrote:

Hi Aaron,

Thanks for the feedback.

Aaron Schrab wrote:

quoted

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.

quoted

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

At 16:51 -0700 07 Apr 2013, Jonathan Nieder [off-list ref] wrote:

quoted

-	char *ref_git;
+	char *ref_git, *repo;

[...]

quoted

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

At 20:06 -0400 07 Apr 2013, I wrote:

At 16:48 -0700 07 Apr 2013, Jonathan Nieder [off-list ref] wrote:

quoted

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.

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

Aaron Schrab wrote:

[...]

+	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