Re: [PATCH 1/2] clone: Fix error message for reference repository
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 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. -- 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
[PATCH 1/2] clone: Fix error message for reference repository
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
---
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
--
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 1/2] clone: Fix error message for reference repository
Aaron Schrab 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. -- 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 1/2] clone: Fix error message for reference repository
At 10:57 -0700 08 Apr 2013, Junio C Hamano 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). -- 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 1/2] clone: Fix error message for reference repository
Aaron Schrab writes: > At 08:30 -0700 08 Apr 2013, Junio C Hamano 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. -- 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 1/2] clone: Fix error message for reference repository
At 08:30 -0700 08 Apr 2013, Junio C Hamano 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. -- 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 1/2] clone: Fix error message for reference repository
Aaron Schrab writes: > At 06:58 -0700 08 Apr 2013, Junio C Hamano 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. -- 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 1/2] clone: Fix error message for reference repository
At 06:58 -0700 08 Apr 2013, Junio C Hamano 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. -- 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 1/2] clone: Fix error message for reference repository
Aaron Schrab writes: > At 16:48 -0700 07 Apr 2013, Jonathan Nieder 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? -- 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 1/2] clone: Fix error message for reference repository
At 20:06 -0400 07 Apr 2013, I wrote: At 16:48 -0700 07 Apr 2013, Jonathan Nieder 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. -- 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 1/2] clone: Fix error message for reference repository
At 16:48 -0700 07 Apr 2013, Jonathan Nieder 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.
--
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 1/2] clone: Fix error message for reference repository
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
--
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
[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
---
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
--
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
