Re: [PATCH] Revert "setup: set env $GIT_WORK_TREE when work tree is set, like $GIT_DIR"

[Junio C Hamano]

Duy Nguyen  writes:

> On Tue, Dec 22, 2015 at 1:31 AM, Junio C Hamano  wrote:
>> Junio C Hamano  writes:
>>
>>> Nguyễn Thái Ngọc Duy   writes:
>>>
 This commit has caused three regression reports so far. All of them are
 about spawning git subprocesses, where the new presence of GIT_WORK_TREE
 either changes command behaviour (git-init or git-clone), or how
 repo/worktree is detected (from aliases), with or without $GIT_DIR.
 The original bug will be re-fixed another way.

 Signed-off-by: Nguyễn Thái Ngọc Duy 
 ---
  On Thu, Dec 3, 2015 at 12:09 AM, Junio C Hamano  wrote:
  > OK, when/if you decide that our first step should be a revert of
  > d95138e please send in a patch to do so with a brief write-up of a
  > follow-up plan.

  Three reports to me are enough. And I obviously could not push the
  fix out fast enough. So if you want to revert it, here's the patch on
  maint.
>>
>> Also, can you reference these three reports for future reference?
>
> http://article.gmane.org/gmane.comp.version-control.git/281608
> http://article.gmane.org/gmane.comp.version-control.git/281979
> http://article.gmane.org/gmane.comp.version-control.git/282691
>
> The last one is not confirmed by the reporter yet. But I'm pretty sure
> i'll trigger the special case "when GIT_WORK_TREE is set but GIT_DIR
> is not" in setup code

Thanks, I'll leave these breadcrumbs in the log message for future
reference.

I think the last sentence of the original commit is telling how this
bug came about.  "It does not harm if $GIT_WORK_TREE is set while
$GIT_DIR is not." forgets to consider the possibility that scripts
may be relying on the "Go to the top of the working tree and setting
GIT_DIR would give you a reasonable environment".  That is true if
GIT_WORK_TREE is not set, and these scripts weren't getting the
environment exported [*1*].  These scripts now have to unset
GIT_WORK_TREE themselves (or set it to their $cwd if they are indeed
at the top), just in case the process that calls them exports it
X-<.

Thanks.


[Footnote]

*1* If the end user has GIT_WORK_TREE in the environment, even if
Git stops exporting it by reverting d95138e, such a script may
break.  So in that sense, d95138e did not quite change the rule
of the game for these scripts, but made it more obvious when
these scripts were written sloppily.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Revert "setup: set env $GIT_WORK_TREE when work tree is set, like $GIT_DIR"

[Junio C Hamano]

Junio C Hamano  writes:

> Nguyễn Thái Ngọc Duy   writes:
>
>> This commit has caused three regression reports so far. All of them are
>> about spawning git subprocesses, where the new presence of GIT_WORK_TREE
>> either changes command behaviour (git-init or git-clone), or how
>> repo/worktree is detected (from aliases), with or without $GIT_DIR.
>> The original bug will be re-fixed another way.
>>
>> Signed-off-by: Nguyễn Thái Ngọc Duy 
>> ---
>>  On Thu, Dec 3, 2015 at 12:09 AM, Junio C Hamano  wrote:
>>  > OK, when/if you decide that our first step should be a revert of
>>  > d95138e please send in a patch to do so with a brief write-up of a
>>  > follow-up plan.
>>
>>  Three reports to me are enough. And I obviously could not push the
>>  fix out fast enough. So if you want to revert it, here's the patch on
>>  maint.

Also, can you reference these three reports for future reference?

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Revert "setup: set env $GIT_WORK_TREE when work tree is set, like $GIT_DIR"

[Duy Nguyen]

On Tue, Dec 22, 2015 at 1:31 AM, Junio C Hamano  wrote:
> Junio C Hamano  writes:
>
>> Nguyễn Thái Ngọc Duy   writes:
>>
>>> This commit has caused three regression reports so far. All of them are
>>> about spawning git subprocesses, where the new presence of GIT_WORK_TREE
>>> either changes command behaviour (git-init or git-clone), or how
>>> repo/worktree is detected (from aliases), with or without $GIT_DIR.
>>> The original bug will be re-fixed another way.
>>>
>>> Signed-off-by: Nguyễn Thái Ngọc Duy 
>>> ---
>>>  On Thu, Dec 3, 2015 at 12:09 AM, Junio C Hamano  wrote:
>>>  > OK, when/if you decide that our first step should be a revert of
>>>  > d95138e please send in a patch to do so with a brief write-up of a
>>>  > follow-up plan.
>>>
>>>  Three reports to me are enough. And I obviously could not push the
>>>  fix out fast enough. So if you want to revert it, here's the patch on
>>>  maint.
>
> Also, can you reference these three reports for future reference?

http://article.gmane.org/gmane.comp.version-control.git/281608
http://article.gmane.org/gmane.comp.version-control.git/281979
http://article.gmane.org/gmane.comp.version-control.git/282691

The last one is not confirmed by the reporter yet. But I'm pretty sure
i'll trigger the special case "when GIT_WORK_TREE is set but GIT_DIR
is not" in setup code
-- 
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] Revert "setup: set env $GIT_WORK_TREE when work tree is set, like $GIT_DIR"

[Nguyễn Thái Ngọc Duy]

This commit has caused three regression reports so far. All of them are
about spawning git subprocesses, where the new presence of GIT_WORK_TREE
either changes command behaviour (git-init or git-clone), or how
repo/worktree is detected (from aliases), with or without $GIT_DIR.
The original bug will be re-fixed another way.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 On Thu, Dec 3, 2015 at 12:09 AM, Junio C Hamano  wrote:
 > OK, when/if you decide that our first step should be a revert of
 > d95138e please send in a patch to do so with a brief write-up of a
 > follow-up plan.

 Three reports to me are enough. And I obviously could not push the
 fix out fast enough. So if you want to revert it, here's the patch on
 maint.

 environment.c  | 2 --
 t/t0002-gitfile.sh | 2 +-
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/environment.c b/environment.c
index 23a38e4..f1a2a49 100644
--- a/environment.c
+++ b/environment.c
@@ -238,8 +238,6 @@ void set_git_work_tree(const char *new_work_tree)
}
git_work_tree_initialized = 1;
work_tree = xstrdup(real_path(new_work_tree));
-   if (setenv(GIT_WORK_TREE_ENVIRONMENT, work_tree, 1))
-   die("could not set GIT_WORK_TREE to '%s'", work_tree);
 }
 
 const char *get_git_work_tree(void)
diff --git a/t/t0002-gitfile.sh b/t/t0002-gitfile.sh
index 9670e8c..3afe012 100755
--- a/t/t0002-gitfile.sh
+++ b/t/t0002-gitfile.sh
@@ -99,7 +99,7 @@ test_expect_success 'check rev-list' '
test "$SHA" = "$(git rev-list HEAD)"
 '
 
-test_expect_success 'setup_git_dir twice in subdir' '
+test_expect_failure 'setup_git_dir twice in subdir' '
git init sgd &&
(
cd sgd &&
-- 
2.3.0.rc1.137.g477eb31

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Revert "setup: set env $GIT_WORK_TREE when work tree is set, like $GIT_DIR"

[Junio C Hamano]

Nguyễn Thái Ngọc Duy   writes:

> This commit has caused three regression reports so far. All of them are
> about spawning git subprocesses, where the new presence of GIT_WORK_TREE
> either changes command behaviour (git-init or git-clone), or how
> repo/worktree is detected (from aliases), with or without $GIT_DIR.
> The original bug will be re-fixed another way.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  On Thu, Dec 3, 2015 at 12:09 AM, Junio C Hamano  wrote:
>  > OK, when/if you decide that our first step should be a revert of
>  > d95138e please send in a patch to do so with a brief write-up of a
>  > follow-up plan.
>
>  Three reports to me are enough. And I obviously could not push the
>  fix out fast enough. So if you want to revert it, here's the patch on
>  maint.

The timing of this is a bit unfortunate, as d95138e is a regression
since v2.5.1 throughout v2.6 series.  I'll push this out on maint
and onwards for now.  I do not think we added new things that rely
on the post-d95138e "broken" behaviour in-tree, but this reversion
might introduce another unexpected regression X-<.  We'll see.

Thanks.

>
>  environment.c  | 2 --
>  t/t0002-gitfile.sh | 2 +-
>  2 files changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/environment.c b/environment.c
> index 23a38e4..f1a2a49 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -238,8 +238,6 @@ void set_git_work_tree(const char *new_work_tree)
>   }
>   git_work_tree_initialized = 1;
>   work_tree = xstrdup(real_path(new_work_tree));
> - if (setenv(GIT_WORK_TREE_ENVIRONMENT, work_tree, 1))
> - die("could not set GIT_WORK_TREE to '%s'", work_tree);
>  }
>  
>  const char *get_git_work_tree(void)
> diff --git a/t/t0002-gitfile.sh b/t/t0002-gitfile.sh
> index 9670e8c..3afe012 100755
> --- a/t/t0002-gitfile.sh
> +++ b/t/t0002-gitfile.sh
> @@ -99,7 +99,7 @@ test_expect_success 'check rev-list' '
>   test "$SHA" = "$(git rev-list HEAD)"
>  '
>  
> -test_expect_success 'setup_git_dir twice in subdir' '
> +test_expect_failure 'setup_git_dir twice in subdir' '
>   git init sgd &&
>   (
>   cd sgd &&
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html