Re: [PATCH v2 4/9] Export the discover_git_directory() function

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

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