Re: [PATCH v3 15/25] setup.c: detect $GIT_COMMON_DIR in is_git_directory()

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 27, 2014, 00:16 UTC
Message-ID: <xmqqr46p1kbo.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1392730814-19656-16-git-send-email-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

Show 25 quoted lines

> If the file "$GIT_DIR/commondir" exists, it contains the value of
> $GIT_COMMON_DIR.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  Documentation/gitrepository-layout.txt |  4 ++++
>  setup.c                                | 38 ++++++++++++++++++++++++++++------
>  strbuf.c                               |  8 +++++++
>  strbuf.h                               |  1 +
>  4 files changed, 45 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/gitrepository-layout.txt b/Documentation/gitrepository-layout.txt
> index aa03882..9bfe0f1 100644
> --- a/Documentation/gitrepository-layout.txt
> +++ b/Documentation/gitrepository-layout.txt
> @@ -211,6 +211,10 @@ shallow::
>  	and maintained by shallow clone mechanism.  See `--depth`
>  	option to linkgit:git-clone[1] and linkgit:git-fetch[1].
>  
> +commondir::
> +	If this file exists, $GIT_COMMON_DIR will be set to the path
> +	specified in this file if it is not set.
> +
>  modules::
>  	Contains the git-repositories of the submodules.

In a way similar to the "*Note*" in a very early part of this file describes the .git-file redirected repositories, we would need to mention that there now exist repositories borrowing from another repository via this commondir mechanism, and what the caveats are when using them (like, "do not ever nuke the original repository somebody else is borrowing from with 'rm -rf' when you think you are done with the original").

> +	if (file_exists(path.buf)) {
> +		if (strbuf_read_file(&data, path.buf, 0) <= 0)
> +			die_errno(_("failed to read %s"), path.buf);

Do we care about the case where we cannot tell if the file exists (e.g. stat() fails due to EPERM or something), or would it be not worth worrying about?

Show 5 quoted lines

> +		strbuf_chomp(&data);
> +		strbuf_reset(&path);
> +		if (!is_absolute_path(data.buf))
> +			strbuf_addf(&path, "%s/", gitdir);
> +		strbuf_addbuf(&path, &data);

OK, so commondir can be relative to the containing gitdir (e.g. /a/foo/.git/commondir with "../../bar/.git" would name /a/bar/.git as the common dir).

It needs to be documented in the repository-layout somehow.

Show 6 quoted lines

> @@ -188,14 +212,20 @@ int is_git_directory(const char *suspect)
>  	int ret = 0;
>  	size_t len;
>  
> -	strbuf_addstr(&path, suspect);
> +	strbuf_addf(&path, "%s/HEAD", suspect);

> +	if (validate_headref(path.buf))
> +		goto done;

Is there a reason why we want to check HEAD before other stuff? Just being curious, as I do not think of any (I am not saying that we shouldn't change the order).

Show 7 quoted lines

> +void strbuf_chomp(struct strbuf *sb)
> +{
> +	while (sb->len && (sb->buf[sb->len - 1] == '\n' ||
> +			   sb->buf[sb->len - 1] == '\r'))
> +		sb->len--;
> +	sb->buf[sb->len] = '\0';
> +}

It feels a bit yucky to ignore trailing \r on non-DOS filesystems (if it were also removing any whitespace, then I would sort of understand in the context of the expected caller of this function in this series---except that it would no longer be a "chomp"), but I'd let it pass.