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

[Junio C Hamano <gitster@pobox.com>]

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

> 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?

> +		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.

> @@ -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).

> +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.