threads / patch / 37484

patchsetup.c: set workdir when gitdir is not default

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

## tl;dr

5 messages between Sep 3, 2014 and Sep 4, 2014. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

Max Kirillov· Sep 3, 2014, 22:42 UTC · lore

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(-)
Show changes to 2 files +10 −1

setup.c, t/t0002-gitfile.sh

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
Eric Sunshine· Sep 4, 2014, 10:44 UTC · re: Max Kirillov · lore

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

On Wed, Sep 3, 2014 at 6:42 PM, Max Kirillov <max@max630.net> wrote:
Show 43 quoted lines
> 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
    )
Show 5 quoted lines
> +'
> +
>  test_done
> --
> 2.0.1.1697.g73c6810
Max Kirillov· Sep 4, 2014, 14:23 UTC · re: Eric Sunshine · lore

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

On Thu, Sep 04, 2014 at 06:44:08AM -0400, Eric Sunshine wrote:
Show 8 quoted lines
> > +       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
Duy Nguyen· Sep 4, 2014, 10:53 UTC · re: Max Kirillov · lore

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

On Thu, Sep 4, 2014 at 5:42 AM, Max Kirillov <max@max630.net> wrote:
Show 8 quoted lines
> 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.

Show 8 quoted lines
> -       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
Max Kirillov· Sep 4, 2014, 14:20 UTC · re: Duy Nguyen · lore

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

On Thu, Sep 04, 2014 at 05:53:34PM +0700, Duy Nguyen wrote:
Show 8 quoted lines
> 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

← back to recent threads