[PATCH] setup.c: set workdir when gitdir is not default

[PATCH] setup.c: set workdir when gitdir is not default

[Max Kirillov]

When gitfile is used, git sets GIT_DIR environment variable for
subsequent commands, and that commands start working in mode "GIT_DIR
set, workdir current", which is incorrect for the case when git runs
from subdirectory of repository. This can be observed at least for
running aliases - git fails with message "internal error: work tree has
already been set"

Fix by setting GIT_WORK_TREE environment also.

Add test which demonstrates problem with alias.

Signed-off-by: Max Kirillov <max@max630.net>
---
 setup.c            | 4 +++-
 t/t0002-gitfile.sh | 7 +++++++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/setup.c b/setup.c
index 0a22f8b..bcf4e31 100644
--- a/setup.c
+++ b/setup.c
@@ -508,8 +508,10 @@ static const char *setup_discovered_git_dir(const char *gitdir,
 
 	/* #0, #1, #5, #8, #9, #12, #13 */
 	set_git_work_tree(".");
-	if (strcmp(gitdir, DEFAULT_GIT_DIR_ENVIRONMENT))
+	if (strcmp(gitdir, DEFAULT_GIT_DIR_ENVIRONMENT)) {
 		set_git_dir(gitdir);
+		setenv(GIT_WORK_TREE_ENVIRONMENT, get_git_work_tree(), 1);
+	}
 	inside_git_dir = 0;
 	inside_work_tree = 1;
 	if (offset == len)
diff --git a/t/t0002-gitfile.sh b/t/t0002-gitfile.sh
index 37e9396..428cfdc 100755
--- a/t/t0002-gitfile.sh
+++ b/t/t0002-gitfile.sh
@@ -99,4 +99,11 @@ test_expect_success 'check rev-list' '
 	test "$SHA" = "$(git rev-list HEAD)"
 '
 
+test_expect_success 'check alias call from subdirectory' '
+	git config alias.testalias "rev-parse HEAD" &&
+	mkdir -p subdir &&
+	cd subdir &&
+	git testalias
+'
+
 test_done
-- 
2.0.1.1697.g73c6810

Re: [PATCH] setup.c: set workdir when gitdir is not default

[Eric Sunshine]

On Wed, Sep 3, 2014 at 6:42 PM, Max Kirillov <max@max630.net> wrote:
> When gitfile is used, git sets GIT_DIR environment variable for
> subsequent commands, and that commands start working in mode "GIT_DIR
> set, workdir current", which is incorrect for the case when git runs
> from subdirectory of repository. This can be observed at least for
> running aliases - git fails with message "internal error: work tree has
> already been set"
>
> Fix by setting GIT_WORK_TREE environment also.
>
> Add test which demonstrates problem with alias.
>
> Signed-off-by: Max Kirillov <max@max630.net>
> ---
>  setup.c            | 4 +++-
>  t/t0002-gitfile.sh | 7 +++++++
>  2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/setup.c b/setup.c
> index 0a22f8b..bcf4e31 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -508,8 +508,10 @@ static const char *setup_discovered_git_dir(const char *gitdir,
>
>         /* #0, #1, #5, #8, #9, #12, #13 */
>         set_git_work_tree(".");
> -       if (strcmp(gitdir, DEFAULT_GIT_DIR_ENVIRONMENT))
> +       if (strcmp(gitdir, DEFAULT_GIT_DIR_ENVIRONMENT)) {
>                 set_git_dir(gitdir);
> +               setenv(GIT_WORK_TREE_ENVIRONMENT, get_git_work_tree(), 1);
> +       }
>         inside_git_dir = 0;
>         inside_work_tree = 1;
>         if (offset == len)
> diff --git a/t/t0002-gitfile.sh b/t/t0002-gitfile.sh
> index 37e9396..428cfdc 100755
> --- a/t/t0002-gitfile.sh
> +++ b/t/t0002-gitfile.sh
> @@ -99,4 +99,11 @@ test_expect_success 'check rev-list' '
>         test "$SHA" = "$(git rev-list HEAD)"
>  '
>
> +test_expect_success 'check alias call from subdirectory' '
> +       git config alias.testalias "rev-parse HEAD" &&

Perhaps use test_config here?

> +       mkdir -p subdir &&
> +       cd subdir &&
> +       git testalias

If a new test is added following this one, it will be run from within
'subdir', which might come as a surprise as the author of the new
test. Wrapping the 'cd' and following code in a subshell is a good way
to avoid the problem.

    mkdir -p subdir &&
    (
        cd subdir &&
        git testalias
    )

> +'
> +
>  test_done
> --
> 2.0.1.1697.g73c6810

Re: [PATCH] setup.c: set workdir when gitdir is not default

[Duy Nguyen]

On Thu, Sep 4, 2014 at 5:42 AM, Max Kirillov <max@max630.net> wrote:
> diff --git a/setup.c b/setup.c
> index 0a22f8b..bcf4e31 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -508,8 +508,10 @@ static const char *setup_discovered_git_dir(const char *gitdir,
>
>         /* #0, #1, #5, #8, #9, #12, #13 */
>         set_git_work_tree(".");

I wonder if we should setenv(GIT_WORK_TREE_) from inside this function
instead. A quick glance over 'git grep set_git_work_tree' gives me the
impression that it's safe to do so, and could cover future bugs
similar to this.

> -       if (strcmp(gitdir, DEFAULT_GIT_DIR_ENVIRONMENT))
> +       if (strcmp(gitdir, DEFAULT_GIT_DIR_ENVIRONMENT)) {
>                 set_git_dir(gitdir);
> +               setenv(GIT_WORK_TREE_ENVIRONMENT, get_git_work_tree(), 1);
> +       }
>         inside_git_dir = 0;
>         inside_work_tree = 1;
>         if (offset == len)
-- 
Duy

Re: [PATCH] setup.c: set workdir when gitdir is not default

[Max Kirillov]

On Thu, Sep 04, 2014 at 05:53:34PM +0700, Duy Nguyen wrote:
> On Thu, Sep 4, 2014 at 5:42 AM, Max Kirillov <max@max630.net> wrote:
>>         /* #0, #1, #5, #8, #9, #12, #13 */
>>         set_git_work_tree(".");
> 
> I wonder if we should setenv(GIT_WORK_TREE_) from inside this function
> instead. A quick glance over 'git grep set_git_work_tree' gives me the
> impression that it's safe to do so, and could cover future bugs
> similar to this.

Actually, I've been thinking about just the oppozite: remove
the setenv(GIT_DIR) from the discovery code at all. Because
backend performs the same discovery, and it would find the
same git directory itself. And leave environment variables to
be only set by user.

Currently environment variables are used for both purposes:
user can use them for hacky stuff, and git itself use them
for passing internal information. But, as we can see,
setting the variables can have bigger effect than expected.
When they are set by user, it's ok, let him try, fail and
set some else variable to adapt for his needs. But for git
code there is the discovery routines, lets just use them
always, it will cause a couple of extra (very few) IO calls
but should always give same (correct) result

-- 
Max

Re: [PATCH] setup.c: set workdir when gitdir is not default

[Max Kirillov]

On Thu, Sep 04, 2014 at 06:44:08AM -0400, Eric Sunshine wrote:
> > +       mkdir -p subdir &&
> > +       cd subdir &&
> > +       git testalias
> 
> If a new test is added following this one, it will be run from within
> 'subdir', which might come as a surprise as the author of the new
> test. Wrapping the 'cd' and following code in a subshell is a good way
> to avoid the problem.

Sure, my mistake. Will fix this and the other issue you
mentioned. Thank you.

-- 
Max