From: Jonathan Nieder <jrnieder@gmail.com>
To: Jeff King <peff@peff.net>
Cc: "Cedric Gava" <gava.c@free.fr>,
git@vger.kernel.org, "Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>
Subject: Re: [PATCH] init: don't set core.worktree when initializing /.git
Date: Tue, 31 Mar 2015 12:14:20 -0700 [thread overview]
Message-ID: <20150331191420.GE22844@google.com> (raw)
In-Reply-To: <20150331183423.GD19206@peff.net>
Jeff King wrote:
> No tests, as we would need to be able to write to "/" to do
> so.
t1509-root-worktree.sh is supposed to test the repository-at-/ case.
But I wouldn't be surprised if it's bitrotted, since people don't set
up a throwaway chroot or VM for tests too often.
[...]
> The current behavior isn't _wrong_, in the sense that it's OK to set
> core.worktree when we don't need to. But I think it is unnecessarily
> confusing to users who expect to be able to move .git directories
> around, because it usually just works. So I'd call this an extremely
> minor bug.
This belongs in the commit message.
[...]
> --- a/builtin/init-db.c
> +++ b/builtin/init-db.c
> @@ -182,6 +182,21 @@ static int git_init_db_config(const char *k, const char *v, void *cb)
> return 0;
> }
>
> +/*
> + * If the git_dir is not directly inside the working tree, then git will not
> + * find it by default, and we need to set the worktree explicitly.
> + */
> +static int needs_work_tree_config(const char *git_dir, const char *work_tree)
> +{
> + /* special case that is missed by the general rule below */
(optional) I'd leave out this comment --- it seems obvious enough in
context and the purpose of the comment is unobvious without looking
at the history.
> + if (!strcmp(work_tree, "/") && !strcmp(git_dir, "/.git"))
> + return 0;
> + if (skip_prefix(git_dir, work_tree, &git_dir) &&
> + !strcmp(git_dir, "/.git"))
> + return 0;
work_tree has been cleaned up with real_path, so in the normal case it
contains getcwd() output which will not end with a / unless it has to.
The only exception I can see is when git hits the MAXDEPTH limit for
symlink resolution (5 nested symlinks), in which case we take what we
find instead of erroring out, which looks like a bug.
We have called set_git_dir_init so git_dir has been cleaned up by
real_path in the same way. Good.
With or without the commit message, comment, and test improvements
mentioned above,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Thanks.
next prev parent reply other threads:[~2015-03-31 19:14 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-03-31 9:25 Forcing git top-level Cedric Gava
2015-03-31 18:15 ` Jeff King
2015-03-31 18:34 ` [PATCH] init: don't set core.worktree when initializing /.git Jeff King
2015-03-31 19:14 ` Jonathan Nieder [this message]
2015-04-02 18:37 ` [PATCH v2] " Jeff King
2015-04-02 18:49 ` Jonathan Nieder
2015-04-03 10:08 ` [PATCH] t1509: update prepare script to be able to run t1509 in chroot again Nguyễn Thái Ngọc Duy
2015-04-03 12:01 ` Jeff King
2015-04-03 12:14 ` Duy Nguyen
2015-04-11 19:58 ` Junio C Hamano
2015-04-18 11:22 ` [PATCH v2] " Nguyễn Thái Ngọc Duy
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20150331191420.GE22844@google.com \
--to=jrnieder@gmail.com \
--cc=gava.c@free.fr \
--cc=git@vger.kernel.org \
--cc=pclouds@gmail.com \
--cc=peff@peff.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).