Re: [PATCH v7 4/7] submodule: allow do_submodule_path to work if given gitdir directly
On Thu, Aug 18, 2016 at 11:46 AM, Junio C Hamanowrote: > Jacob Keller writes: > >> From: Jacob Keller >> >> Currently, do_submodule_path relies on read_gitfile, which will die() if >> it can't read from the specified gitfile. Unfortunately, this means that >> do_submodule_path will not work when given the path to a submodule which >> is checked out directly, such as a newly added submodule which you >> cloned and then "git submodule add". > > Hmm, are you sure about that? > > A call to read_gitfile() turns into a call to read_gitfile_gently() > with the return_error_code parameter set to NULL. The function does > a stat(2), and if the given path is not a file (e.g. we created the > submodule working tree and repository in-place ourselves, instead of > cloning an existing project from elsewhere, in which case we would > see a directory there), it says READ_GIT_FILE_ERR_NOT_A_FILE and > returns NULL, because that is not a fatal error condition. The same > thing happens if path does not yet exist. > > This caller is given , prepares "/.git" in buf, and > calls read_gitfile(). If it returns a non-NULL, it replaces what is > in buf and continues, but if it returns a NULL (i.e. the two cases I > mentioned in the above paragraph), it continues with "/.git". > > While I do not think changing it to resolve_gitdir() is wrong per-se, > I am not sure if it is necessary. > > I must be misreading something in the existing code. > No I think you're correct. I was under the assumption that it would die here. I think it's probably better to stick with read_gitfile() here, it should work. The main issue is what happens after this needs to be fixed. I'll investigate this more. Thanks, Jake -- 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 v7 4/7] submodule: allow do_submodule_path to work if given gitdir directly
Jacob Kellerwrites: > From: Jacob Keller > > Currently, do_submodule_path relies on read_gitfile, which will die() if > it can't read from the specified gitfile. Unfortunately, this means that > do_submodule_path will not work when given the path to a submodule which > is checked out directly, such as a newly added submodule which you > cloned and then "git submodule add". Hmm, are you sure about that? A call to read_gitfile() turns into a call to read_gitfile_gently() with the return_error_code parameter set to NULL. The function does a stat(2), and if the given path is not a file (e.g. we created the submodule working tree and repository in-place ourselves, instead of cloning an existing project from elsewhere, in which case we would see a directory there), it says READ_GIT_FILE_ERR_NOT_A_FILE and returns NULL, because that is not a fatal error condition. The same thing happens if path does not yet exist. This caller is given , prepares "/.git" in buf, and calls read_gitfile(). If it returns a non-NULL, it replaces what is in buf and continues, but if it returns a NULL (i.e. the two cases I mentioned in the above paragraph), it continues with "/.git". While I do not think changing it to resolve_gitdir() is wrong per-se, I am not sure if it is necessary. I must be misreading something in the existing code. > Instead, replace the call with > resolve_gitdir. This first checks to see if we've been given a gitdir > already. > > Because resolve_gitdir may return the same buffer it was passed, we have > to check for this case as well, since strbuf_reset() will not work as > expected here, and indeed is not necessary. > > Signed-off-by: Jacob Keller > --- > path.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/path.c b/path.c > index 17551c483476..d1af029152a2 100644 > --- a/path.c > +++ b/path.c > @@ -477,8 +477,8 @@ static void do_submodule_path(struct strbuf *buf, const > char *path, > strbuf_complete(buf, '/'); > strbuf_addstr(buf, ".git"); > > - git_dir = read_gitfile(buf->buf); > - if (git_dir) { > + git_dir = resolve_gitdir(buf->buf); > + if (git_dir && git_dir != buf->buf) { > strbuf_reset(buf); > strbuf_addstr(buf, git_dir); > } -- 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 v7 4/7] submodule: allow do_submodule_path to work if given gitdir directly
On Wed, Aug 17, 2016 at 5:51 PM, Jacob Kellerwrote: > From: Jacob Keller > > Currently, do_submodule_path relies on read_gitfile, which will die() if > it can't read from the specified gitfile. Unfortunately, this means that > do_submodule_path will not work when given the path to a submodule which > is checked out directly, such as a newly added submodule which you > cloned and then "git submodule add". Instead, replace the call with > resolve_gitdir. This first checks to see if we've been given a gitdir > already. > > Because resolve_gitdir may return the same buffer it was passed, we have > to check for this case as well, since strbuf_reset() will not work as > expected here, and indeed is not necessary. > > Signed-off-by: Jacob Keller > --- > path.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) Looks good, Thanks, Stefan -- 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 v7 4/7] submodule: allow do_submodule_path to work if given gitdir directly
From: Jacob KellerCurrently, do_submodule_path relies on read_gitfile, which will die() if it can't read from the specified gitfile. Unfortunately, this means that do_submodule_path will not work when given the path to a submodule which is checked out directly, such as a newly added submodule which you cloned and then "git submodule add". Instead, replace the call with resolve_gitdir. This first checks to see if we've been given a gitdir already. Because resolve_gitdir may return the same buffer it was passed, we have to check for this case as well, since strbuf_reset() will not work as expected here, and indeed is not necessary. Signed-off-by: Jacob Keller --- path.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/path.c b/path.c index 17551c483476..d1af029152a2 100644 --- a/path.c +++ b/path.c @@ -477,8 +477,8 @@ static void do_submodule_path(struct strbuf *buf, const char *path, strbuf_complete(buf, '/'); strbuf_addstr(buf, ".git"); - git_dir = read_gitfile(buf->buf); - if (git_dir) { + git_dir = resolve_gitdir(buf->buf); + if (git_dir && git_dir != buf->buf) { strbuf_reset(buf); strbuf_addstr(buf, git_dir); } -- 2.10.0.rc0.217.g609f9e8.dirty -- 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