threads / bug / 50073

A bug in git-add with GIT_DIR?

Subject: A bug in git-add with GIT_DIR?

## tl;dr

3 messages between Dec 20, 2018 and Dec 20, 2018.

replies: 2people: 2as markdown or json

Orgad Shaneh· Dec 20, 2018, 08:37 UTC · lore
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
Jeff King· Dec 20, 2018, 15:18 UTC · re: Orgad Shaneh · lore

Re: A bug in git-add with GIT_DIR?

On Thu, Dec 20, 2018 at 10:37:21AM +0200, Orgad Shaneh wrote:
Show 19 quoted lines
> 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.

Show 17 quoted lines
> 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
Orgad Shaneh· Dec 20, 2018, 15:21 UTC · re: Jeff King · lore

Re: A bug in git-add with GIT_DIR?

On Thu, Dec 20, 2018 at 5:18 PM Jeff King <peff@peff.net> wrote:
Show 94 quoted lines
>
> 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
Thanks. I'll refactor the tests and post a patch later.
- Orgad

← back to recent threads