A bug in git-add with GIT_DIR?

3 messages, 2 authors, 2018-12-20 · open the first message on its own page

A bug in git-add with GIT_DIR?

From: Orgad Shaneh <hidden>
Date: 2018-12-20 08:37:36

Hi,

I played around with t5403-post-checkout-hook, and noticed that its
state is not exactly what I'd expect it to be.

The test setup is:
echo Data for commit0. >a &&
echo Data for commit0. >b &&
git update-index --add a &&
git update-index --add b &&
tree0=$(git write-tree) &&
commit0=$(echo setup | git commit-tree $tree0) &&
git update-ref refs/heads/master $commit0 &&
git clone ./. clone1 &&
git clone ./. clone2 &&
GIT_DIR=clone2/.git git branch new2 &&
echo Data for commit1. >clone2/b &&
GIT_DIR=clone2/.git git add clone2/b &&
GIT_DIR=clone2/.git git commit -m new2

Now, the line before the last one executes git add clone2/b with GIT_DIR set.

I'd expect that to add b inside clone2, but instead it adds an
inexistent clone2/clone2/b, and if I stop at this line, then the
status shows:

On branch master
Your branch is up to date with 'origin/master'.

Changes to be committed:
  (use "git reset HEAD <file>..." to unstage)

        new file:   clone2/b

Changes not staged for commit:
  (use "git add/rm <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

        modified:   b
        deleted:    clone2/b

Is this the intended behavior? It looks like that's not what the test
meant to do anyway...

And if I change it to (cd clone2 && git add b), then the commits look
reasonable, but step 6 fails.

- Orgad

Re: A bug in git-add with GIT_DIR?

From: Jeff King <hidden>
Date: 2018-12-20 15:18:55

On Thu, Dec 20, 2018 at 10:37:21AM +0200, Orgad Shaneh wrote:
I played around with t5403-post-checkout-hook, and noticed that its
state is not exactly what I'd expect it to be.

The test setup is:
echo Data for commit0. >a &&
echo Data for commit0. >b &&
git update-index --add a &&
git update-index --add b &&
tree0=$(git write-tree) &&
commit0=$(echo setup | git commit-tree $tree0) &&
git update-ref refs/heads/master $commit0 &&
git clone ./. clone1 &&
git clone ./. clone2 &&
GIT_DIR=clone2/.git git branch new2 &&
echo Data for commit1. >clone2/b &&
GIT_DIR=clone2/.git git add clone2/b &&
GIT_DIR=clone2/.git git commit -m new2

Now, the line before the last one executes git add clone2/b with GIT_DIR set.
When GIT_DIR is set but not GIT_WORK_TREE, the current directory is
taken as the working tree.

So that will find clone2/b (from the current directory, which is a real
file), and add an index entry with that path "clone2/b" and the sha1 of
that content.

But when commands are run from inside "clone2", they will naturally
treat "clone2" as the working tree. And since "clone2/b" does not exist
inside there, they will say "oops, it looks like this file has been
deleted".
I'd expect that to add b inside clone2, but instead it adds an
inexistent clone2/clone2/b, and if I stop at this line, then the
status shows:
Sort of. It never sees the path "clone2/clone2/b", but the path in the
index coupled with the working tree being inside clone2 means that it
would look for such a file.
On branch master
Your branch is up to date with 'origin/master'.

Changes to be committed:
  (use "git reset HEAD <file>..." to unstage)

        new file:   clone2/b

Changes not staged for commit:
  (use "git add/rm <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

        modified:   b
        deleted:    clone2/b

Is this the intended behavior? It looks like that's not what the test
meant to do anyway...
This is the expected behavior if you did "cd clone2 && git status".
Looking at the test, I don't think it quite meant to do this. It looks
like it predates "git -C", but for some reason did not want to "cd" in a
subshell.

I think it would be better written as:

  git -C clone2 add b &&
  git -C clone2 commit -m new2

or:

  (
	cd clone2 &&
	git add b &&
	git commit -m new2
  )

And ditto for all of the other uses of $GIT_DIR in that script. E.g.,
the ones that do:

  GIT_DIR=clone1/.git git checkout master

are likely writing the contents of clone1's master branch to the
_current_ directory (not the working tree in clone1).
And if I change it to (cd clone2 && git add b), then the commits look
reasonable, but step 6 fails.
You probably just need to update the other calls, too, so they all
match.

-Peff

Re: A bug in git-add with GIT_DIR?

From: Orgad Shaneh <hidden>
Date: 2018-12-20 15:21:32

On Thu, Dec 20, 2018 at 5:18 PM Jeff King [off-list ref] wrote:
On Thu, Dec 20, 2018 at 10:37:21AM +0200, Orgad Shaneh wrote:
quoted
I played around with t5403-post-checkout-hook, and noticed that its
state is not exactly what I'd expect it to be.

The test setup is:
echo Data for commit0. >a &&
echo Data for commit0. >b &&
git update-index --add a &&
git update-index --add b &&
tree0=$(git write-tree) &&
commit0=$(echo setup | git commit-tree $tree0) &&
git update-ref refs/heads/master $commit0 &&
git clone ./. clone1 &&
git clone ./. clone2 &&
GIT_DIR=clone2/.git git branch new2 &&
echo Data for commit1. >clone2/b &&
GIT_DIR=clone2/.git git add clone2/b &&
GIT_DIR=clone2/.git git commit -m new2

Now, the line before the last one executes git add clone2/b with GIT_DIR set.
When GIT_DIR is set but not GIT_WORK_TREE, the current directory is
taken as the working tree.

So that will find clone2/b (from the current directory, which is a real
file), and add an index entry with that path "clone2/b" and the sha1 of
that content.

But when commands are run from inside "clone2", they will naturally
treat "clone2" as the working tree. And since "clone2/b" does not exist
inside there, they will say "oops, it looks like this file has been
deleted".
quoted
I'd expect that to add b inside clone2, but instead it adds an
inexistent clone2/clone2/b, and if I stop at this line, then the
status shows:
Sort of. It never sees the path "clone2/clone2/b", but the path in the
index coupled with the working tree being inside clone2 means that it
would look for such a file.
quoted
On branch master
Your branch is up to date with 'origin/master'.

Changes to be committed:
  (use "git reset HEAD <file>..." to unstage)

        new file:   clone2/b

Changes not staged for commit:
  (use "git add/rm <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

        modified:   b
        deleted:    clone2/b

Is this the intended behavior? It looks like that's not what the test
meant to do anyway...
This is the expected behavior if you did "cd clone2 && git status".
Looking at the test, I don't think it quite meant to do this. It looks
like it predates "git -C", but for some reason did not want to "cd" in a
subshell.

I think it would be better written as:

  git -C clone2 add b &&
  git -C clone2 commit -m new2

or:

  (
        cd clone2 &&
        git add b &&
        git commit -m new2
  )

And ditto for all of the other uses of $GIT_DIR in that script. E.g.,
the ones that do:

  GIT_DIR=clone1/.git git checkout master

are likely writing the contents of clone1's master branch to the
_current_ directory (not the working tree in clone1).
quoted
And if I change it to (cd clone2 && git add b), then the commits look
reasonable, but step 6 fails.
You probably just need to update the other calls, too, so they all
match.

-Peff
Thanks. I'll refactor the tests and post a patch later.

- Orgad
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help