Re: [PATCH v2 4/9] Export the discover_git_directory() function
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Mar 3, 2017, 14:49 UTC
- Message-ID
- <alpine.DEB.2.20.1703031511490.3767@virtualbox>
- In-Reply-To
- <20170303044518.rdtyvgs7kqe7fkpf@sigill.intra.peff.net>
Hi Peff,
On Thu, 2 Mar 2017, Jeff King wrote:
Show 13 quoted lines
> 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.
Show 6 quoted lines
> > +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.
Show 30 quoted lines
> > + 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`.
Show 6 quoted lines
> 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.
Show 10 quoted lines
> 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