Re: [DONOTAPPLY PATCH 2/3] setup: warn about implicit worktree with $GIT_DIR
Jeff King writes: > I probably shouldn't have included this middle patch at all, because > the interesting thing is what happens when we do turn it off. Actually, I think the warning is the most important part. With the warning enabled, people should notice they are doing something potentially wrong and dangerous, so the warning essentially fixes the issue in the short term. I also think that changing the default behavior later makes sense (but I agree that replacing "in Git 2.0" with "in a future version of Git" is better, there's no urgency for this change and people start looking forward 2.0). -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- 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: [DONOTAPPLY PATCH 2/3] setup: warn about implicit worktree with $GIT_DIR
Jeff King wrote: > On Tue, Mar 26, 2013 at 01:21:42PM -0700, Jonathan Nieder wrote: >> If we want this warning, would something like the following do? >> >> warning: You have set GIT_DIR without setting GIT_WORK_TREE >> hint: In this case, GIT_WORK_TREE defaults to '.' >> hint: To suppress this message, set GIT_WORK_TREE='.' > > That can help by teaching people how GIT_DIR behaves in general. Yes, I think it would have helped in this case. If I understand correctly then for a while Richard was habitually setting GIT_DIR to mean "act on this repository" and thought the worktree was automatically being set to the containing directory. I think patch 3 is a bad direction to go because there will always be old scripts that follow what used to be the recommended way to use GIT_DIR. In the long term a warning like this that doesn't break them (or a fatal error that at least doesn't confuse them) might be a good way to go. Thanks for your thoughtful work, as always. Jonathan -- 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: [DONOTAPPLY PATCH 2/3] setup: warn about implicit worktree with $GIT_DIR
On Tue, Mar 26, 2013 at 01:21:42PM -0700, Jonathan Nieder wrote:
> Jeff King wrote:
>
> > --- a/setup.c
> > +++ b/setup.c
> > @@ -437,6 +437,23 @@ const char *read_gitfile(const char *path)
> > return path;
> > }
> >
> > +static const char warn_implicit_work_tree_msg[] =
> > +N_("You have set GIT_DIR (or used --git-dir) without specifying\n"
> > + "a working tree. In Git 2.0, the behavior will change
>
> Please no. I don't want git 2.0 to be delayed forever.
Please replace "2.0" with some future version, then. I just made up the
number. But...
> If we want this warning, would something like the following do?
>
> warning: You have set GIT_DIR without setting GIT_WORK_TREE
> hint: In this case, GIT_WORK_TREE defaults to '.'
> hint: To suppress this message, set GIT_WORK_TREE='.'
That can help by teaching people how GIT_DIR behaves in general. But the
warning and hint will be small consolation to somebody who runs
"GIT_DIR=foo.git git clean -f" and sees it for the first time.
If you want to argue that people would see the warning in earlier runs
of git, I can kind of buy that. Although the incident that triggered
this discussion probably wouldn't have (I would usually start a
git-clean session with "git clean" without "-f" or "git status", either
of which would have done equally well as this warning to notify the user
what was going on).
Like I said earlier, though, I'm not really sure this is the direction
we want to go. This series is more about seeing what the fallouts are. I
probably shouldn't have included this middle patch at all, because the
interesting thing is what happens when we do turn it off.
-Peff
--
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: [DONOTAPPLY PATCH 2/3] setup: warn about implicit worktree with $GIT_DIR
Jeff King wrote:
> --- a/setup.c
> +++ b/setup.c
> @@ -437,6 +437,23 @@ const char *read_gitfile(const char *path)
> return path;
> }
>
> +static const char warn_implicit_work_tree_msg[] =
> +N_("You have set GIT_DIR (or used --git-dir) without specifying\n"
> + "a working tree. In Git 2.0, the behavior will change
Please no. I don't want git 2.0 to be delayed forever.
If we want this warning, would something like the following do?
warning: You have set GIT_DIR without setting GIT_WORK_TREE
hint: In this case, GIT_WORK_TREE defaults to '.'
hint: To suppress this message, set GIT_WORK_TREE='.'
Thanks,
Jonathan
--
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
[DONOTAPPLY PATCH 2/3] setup: warn about implicit worktree with $GIT_DIR
It can be surprising to some users that pointing GIT_DIR to
a ".git" directory does not use the working tree that
surrounds the .git directory, but rather uses the current
working directory as the working tree.
Git has always worked this way, and for the most part it has
not been a big problem. However, given that one way of the
user finding this out is by having a destructive git command
impact an unexpected area of the filesystem, it would be
nice to default to something less surprising and likely to
cause problems (namely, having no working directory).
This breaks existing users of the feature, of course; they
can adapt by setting GIT_WORK_TREE explicitly to ".", but
they need to be told to do so. Therefore we'll start with a
deprecation period and a warning to give them time to fix
their scripts and workflows.
Signed-off-by: Jeff King
---
setup.c | 21 -
t/t1510-repo-setup.sh | 8 ++--
2 files changed, 26 insertions(+), 3 deletions(-)
diff --git a/setup.c b/setup.c
index 01c5476..afc245f 100644
--- a/setup.c
+++ b/setup.c
@@ -437,6 +437,23 @@ const char *read_gitfile(const char *path)
return path;
}
+static const char warn_implicit_work_tree_msg[] =
+N_("You have set GIT_DIR (or used --git-dir) without specifying\n"
+ "a working tree. In Git 2.0, the behavior will change from using current\n"
+ "working directory as the working tree to having no working tree at all.\n"
+ "If you wish to continue the current behavior, please set GIT_WORK_TREE\n"
+ "or core.worktree explicitly. See `git help git` for more details.");
+
+static void warn_implicit_work_tree(void)
+{
+ static int warn_once;
+
+ if (warn_once++)
+ return;
+
+ warning("%s", _(warn_implicit_work_tree_msg));
+}
+
static const char *setup_explicit_git_dir(const char *gitdirenv,
char *cwd, int len,
int *nongit_ok)
@@ -503,8 +520,10 @@ static const char *setup_explicit_git_dir(const char
*gitdirenv,
free(gitfile);
return NULL;
}
- else /* #2, #10 */
+ else { /* #2, #10 */
+ warn_implicit_work_tree();
set_git_work_tree(".");
+ }
/* set_git_work_tree() must have been called by now */
worktree = get_git_work_tree();
diff --git a/t/t1510-repo-setup.sh b/t/t1510-repo-setup.sh
index cf2ee78..0910de1 100755
--- a/t/t1510-repo-setup.sh
+++ b/t/t1510-repo-setup.sh
@@ -243,7 +243,9 @@ test_expect_success '#2: worktree defaults to cwd with
explicit GIT_DIR' '
test_expect_success '#2: worktree defaults to cwd with explicit GIT_DIR' '
try_repo 2 unset "$here/2/.git" unset "" unset \
"$here/2/.git" "$here/2" "$here/2" "(null)" \
- "$here/2/.git" "$here/2/sub" "$here/2/sub" "(null)"
+ "$here/2/.git" "$here/2/sub" "$here/2/sub" "(null)" \
+ 2>message &&
+ test_i18ngrep "warning:.*GIT_DIR" message
'
test_expect_success '#2b: relative GIT_DIR' '
@@ -378,7 +380,9 @@ test_expect_success '#10: GIT_DIR can point to gitfile' '
test_expect_success '#10: GIT_DIR can point to gitfile' '
try_repo 10 unset "$here/10/.git" unset gitfile unset \
"$here/10.git" "$here/10" "$here/10" "(null)" \
- "$here/10.git" "$here/10/sub" "$here/10/sub" "(null)"
+ "$here/10.git" "$here/10/sub" "$here/10/sub" "(null)" \
+ 2>message &&
+ test_i18ngrep "warning:.*GIT_DIR" message
'
test_expect_success '#10b: relative GIT_DIR can point to gitfile' '
--
1.8.2.13.g0f18d3c
--
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
