Hi Peff,
On Thu, 2 Mar 2017, Jeff King wrote:
> On Fri, Mar 03, 2017 at 03:04:15AM +0100, Johannes Schindelin wrote:
>
> > diff --git a/cache.h b/cache.h
> > index 80b6372cf76..a104b76c02e 100644
> > --- a/cache.h
> > +++ b/cache.h
> > @@ -518,6 +518,7 @@ extern void set_git_work_tree(const char *tree);
> > #define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES"
> >
> > extern void setup_work_tree(void);
> > +extern const char *discover_git_directory(struct strbuf *gitdir);
>
> Perhaps it's worth adding a short docstring describing the function.
Okay.
> > +const char *discover_git_directory(struct strbuf *gitdir)
> > +{
> > + struct strbuf dir = STRBUF_INIT;
> > + int len;
>
> Nit: please use size_t for storing strbuf lengths.
Okay.
> > + if (strbuf_getcwd(&dir))
> > + return NULL;
> > +
> > + len = dir.len;
> > + if (discover_git_directory_1(&dir, gitdir) < 0) {
> > + strbuf_release(&dir);
> > + return NULL;
> > + }
> > +
> > + if (dir.len < len && !is_absolute_path(gitdir->buf)) {
> > + strbuf_addch(&dir, '/');
> > + strbuf_insert(gitdir, 0, dir.buf, dir.len);
> > + }
> > + strbuf_release(&dir);
>
> I was confused by two things here.
>
> One is that because I was wondering whether "gitdir" was supposed to be
> passed empty or not, it wasn't clear that this insert is doing the right
> thing. If there _is_ something in it, then discover_git_directory_1()
> would just append to it, which makes sense. But then this insert blindly
> sticks the absolute-path bit at the front of the string, leaving
> whatever was originally there spliced into the middle of the path.
> Doing:
>
> size_t base = gitdir->len;
> ...
> strbuf_insert(gitdir, base, dir.buf, dir.len);
>
> would solve that.
And of course the is_absolute_path() call also needs to offset `gitdir->buf
+ base`.
> It's probably not that likely for somebody to do:
>
> strbuf_addstr(&buf, "my git dir is ");
> discover_git_directory(&buf);
>
> but since it's not much effort, it might be worth making it work.
Plus, I have no assert()s in place to ensure any expectation to the
contrary. So I fixed it as you suggested.
> The second is that I don't quite understand why we only make the result
> absolute when we walked upwards. In git.git, the result of the function
> in various directories is:
>
> - ".git", when we're at the root of the worktree
> - "/home/peff/git/.git, when we're in "t/"
> - "." when we're already in ".git"
> - "/home/peff/git/.git/." when we're in ".git/objects"
> > I'm not sure if some of those cases are not behaving as intended, or
> there's some reason for the differences that I don't quite understand.
Yes, some of those cases do not behave as intended: it is true that
setup_git_directory() sets git_dir to "/home/peff/git/.git" when we
(actually, you, given that my home directory is different) are in "t/",
but setup_git_directory_gently_1() would set gitdir to ".git" and dir to
"/home/peff/git". But the current directory is still what cwd says it is:
"/home/peff/git/t".
I added a comment:
/*
* The returned gitdir is relative to dir, and if dir does not reflect
* the current working directory, we simply make the gitdir
* absolute.
*/
And thanks: you reminded me of another issue I wanted to address but
forgot: if gitdir is ".", I do not want the resulting absolute path to
have a trailing "/.". I fixed that, too.
Ciao,
Dscho
