Forcing git top-level

Hello

I’ve copied a .git folder located at the root (/) of a filesystem, into another directory (/home/mydir). If I issue a git rev-parse —show-toplevel I got "/"... I would like to change the top-level to point to /home/mydir.

I’ve also tried to bare clone the original .git folder, but the new git repo stills points it top-level to "/".

Thank you for your answer

Best regards Cedric

On Tue, Mar 31, 2015 at 11:25:58AM +0200, Cedric Gava wrote:

Try running "git config --unset core.worktree" in the .git dir.

It looks like "git init" will write a core.worktree entry in this case, even though it isn't technically needed. I think it is due to these lines in builtin/init-db.c:

                if (!starts_with(git_dir, work_tree) ||
                    strcmp(git_dir + strlen(work_tree), "/.git")) {
                        git_config_set("core.worktree", work_tree);
                }

The check returns a false positive for the root directory because we only have one slash (i.e., appending "/.git" to our worktree would be "//.git" in this case).

-Peff

If you create a git repository in the root directory with "git init /", we erroneously write a core.worktree entry. That can be surprising if you later move the .git directory to another path (which usually moves the relative working tree, but is foiled if there is an explicit worktree set).

The problem is that we check whether core.worktree is necessary by seeing if we can make the git_dir by concatenating "/.git" onto the working tree. That would lead to "//.git" in this instance, but we actually have "/.git" (without the doubled slash).

We can fix this by special-casing the root directory. I also split the logic out into its own function to make the conditional a bit more readable (and used skip_prefix, which I think makes it a little more obvious what is going on).

No tests, as we would need to be able to write to "/" to do so. I did manually confirm that:

  sudo git init /
  cd /
  git rev-parse --show-toplevel
  git config core.worktree

Still finds the top-level correctly (as "/"), and does not set any core.worktree variable.

Signed-off-by: Jeff King <peff@peff.net>
---
The current behavior isn't _wrong_, in the sense that it's OK to set
core.worktree when we don't need to. But I think it is unnecessarily
confusing to users who expect to be able to move .git directories
around, because it usually just works. So I'd call this an extremely
minor bug.

 builtin/init-db.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

Jeff King wrote:

t1509-root-worktree.sh is supposed to test the repository-at-/ case. But I wouldn't be surprised if it's bitrotted, since people don't set up a throwaway chroot or VM for tests too often.

[...]

This belongs in the commit message.

[...]

(optional) I'd leave out this comment --- it seems obvious enough in context and the purpose of the comment is unobvious without looking at the history.

work_tree has been cleaned up with real_path, so in the normal case it contains getcwd() output which will not end with a / unless it has to. The only exception I can see is when git hits the MAXDEPTH limit for symlink resolution (5 nested symlinks), in which case we take what we find instead of erroring out, which looks like a bug.

We have called set_git_dir_init so git_dir has been cleaned up by real_path in the same way. Good.

With or without the commit message, comment, and test improvements
mentioned above,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.

On Tue, Mar 31, 2015 at 12:14:20PM -0700, Jonathan Nieder wrote:

Thanks, I had thought we had such a test script, but for some reason had trouble finding it earlier. I couldn't get it to run using the included t1509/prepare-chroot.sh helper; the resulting chroot was missing several tools we rely on in test-lib.sh (perl, expr, tr, and mv).

I gave up for now; I don't think rescuing that script is worth the effort for this minor bug.

It's mostly a restatement of what is already in the first paragraph of the commit message. I folded it into that paragraph.

OK, dropped. Here's the re-roll.

-- >8 --
Subject: [PATCH] init: don't set core.worktree when initializing /.git

If you create a git repository in the root directory with "git init /", we erroneously write a core.worktree entry. This isn't _wrong_, in the sense that it's OK to set core.worktree when we don't need to. But it is unnecessarily surprising if you later move the .git directory to another path (which usually moves the relative working tree, but is foiled if there is an explicit worktree set).

The problem is that we check whether core.worktree is necessary by seeing if we can make the git_dir by concatenating "/.git" onto the working tree. That would lead to "//.git" in this instance, but we actually have "/.git" (without the doubled slash).

We can fix this by special-casing the root directory. I also split the logic out into its own function to make the conditional a bit more readable (and used skip_prefix, which I think makes it a little more obvious what is going on).

No tests, as we would need to be able to write to "/" to do so. I did manually confirm that:

  sudo git init /
  cd /
  git rev-parse --show-toplevel
  git config core.worktree

still finds the top-level correctly (as "/"), and does not set any core.worktree variable.

Signed-off-by: Jeff King <peff@peff.net>
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
---
 builtin/init-db.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

Jeff King wrote:

Thanks.

[...]

This is confusing in the light of t1509-root-worktree.sh existing, but anyone curious can dig up this mailing list thread so I don't mind. :)

[...]

Yep - still looks good.

Regards, Jonathan

Tested on Gentoo and OpenSUSE 13.1, both x86-64

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 On Wed, Apr 1, 2015 at 2:14 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
 > Jeff King wrote:
 >
 >> No tests, as we would need to be able to write to "/" to do
 >> so.
 >
 > t1509-root-worktree.sh is supposed to test the repository-at-/ case.
 > But I wouldn't be surprised if it's bitrotted, since people don't set
 > up a throwaway chroot or VM for tests too often.

 Can't leave it rotting. Either fix it or kill it. This is the first
 option. Good news is the test passes, nothing else is broken. Bad
 news is it does not detect the core.worktree breakage, but this on
 top would verify that Jeff's patch works

  diff --git a/t/t1509-root-worktree.sh b/t/t1509-root-worktree.sh
  index b6977d4..17ff4ce 100755
  --- a/t/t1509-root-worktree.sh
  +++ b/t/t1509-root-worktree.sh
  @@ -224,6 +224,10 @@ test_expect_success 'setup' '
   	test_cmp expected result
   '
   
  +test_expect_success 'no core.worktree after git init' '
  +	test "`git config core.worktree`" = ""
  +'
  +
   test_vars 'auto gitdir, root' ".git" "/" ""
   test_foobar_root
 

 t/t1509/prepare-chroot.sh | 35 ++++++++++++++++++++++++++---------
 1 file changed, 26 insertions(+), 9 deletions(-)

On Fri, Apr 03, 2015 at 05:08:57PM +0700, Nguyễn Thái Ngọc Duy wrote:

This line complains for me. $UID is set in my bash login shell, but not in the dash shell for this script. Maybe "id -u" instead?

It is nice not to mknod here, as it requires root. But making /dev/null a regular file seems a bit flaky. We will constantly overwrite it with the output of each test, and then pipe that output back into the next test. The current set of tests in t1509 does not seem to mind, though.

Re-preparing an already-made chroot makes this:

  PERL_PATH=/bin/true'/usr/bin/perl'

Did you want "PERL_PATH=.*" as your regex?

I found the "in root" here (and in the original) confusing. Do you mean "as root"? I wonder if it would make sense to just show:

  sudo chroot $R /bin/su - $(id -nu)

as the sample command.

Aside from the nits above, I did get it to run t1509 with this. I can't say I'm incredibly excited about the whole thing, if only because it is clear that nobody is going to run it regularly (so regressions will likely go unnoticed, which is the whole point of the test script). But perhaps it is better than nothing, and it is not hurting anyone to sit there and bitrot again. ;)

-Peff

On Fri, Apr 3, 2015 at 7:01 PM, Jeff King <peff@peff.net> wrote:

To be honest, I didn't run it often in the last 5 years. The code it protects seems not broken during this time and probably so for a foreseeable future. So I don't mind if you just kill t1509 and related scripts.

Duy Nguyen <pclouds@gmail.com> writes:

Yeah, but as long as you two worked out to make it run again on at least two systems (i.e. yours), it would be good not to let that effort go to waste. Care to throw a v2 at me with $(id -u) and other changes Peff mentioned?

Thanks.

Tested on Gentoo and OpenSUSE 13.1, both x86-64

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 On Sun, Apr 12, 2015 at 2:58 AM, Junio C Hamano <gitster@pobox.com> wrote:
 > Duy Nguyen <pclouds@gmail.com> writes:
 >
 >> On Fri, Apr 3, 2015 at 7:01 PM, Jeff King <peff@peff.net> wrote:
 >>> Aside from the nits above, I did get it to run t1509 with this. I can't
 >>> say I'm incredibly excited about the whole thing, if only because it is
 >>> clear that nobody is going to run it regularly (so regressions will
 >>> likely go unnoticed, which is the whole point of the test script).  But
 >>> perhaps it is better than nothing, and it is not hurting anyone to sit
 >>> there and bitrot again. ;)
 >>
 >> To be honest, I didn't run it often in the last 5 years. The code it
 >> protects seems not broken during this time and probably so for a
 >> foreseeable future. So I don't mind if you just kill t1509 and related
 >> scripts.
 >
 > Yeah, but as long as you two worked out to make it run again on at
 > least two systems (i.e. yours), it would be good not to let that
 > effort go to waste. Care to throw a v2 at me with $(id -u) and other
 > changes Peff mentioned?

 thrown (a little bit late because $DAYJOB consumed much of my energy)

 t/t1509/prepare-chroot.sh | 38 +++++++++++++++++++++++++++++---------
 1 file changed, 29 insertions(+), 9 deletions(-)