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

2014-02-28 Thread Duy Nguyen
On Thu, Feb 27, 2014 at 7:16 AM, Junio C Hamano  wrote:
>> + 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?

In that case we assume (incorrectly) that the repository is complete.
Following operations would fail. So not too bad, I think.

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

Yes, it's reordered so that worktree signature (e.g. HEAD) is checked
first, against $GIT_DIR. Then non-worktree signatures ("refs" and
"objects") are checked against $GIT_COMMON_DIR (or $GIT_DIR still if
$GIT_DIR/commondir does not exist). Notice "path" is reset to
$GIT_COMMON_DIR just after checking HEAD. I should probably add a
comment about this separation.
-- 
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


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

2014-02-26 Thread Junio C Hamano
Nguyễn Thái Ngọc Duy   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 
> ---
>  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.
--
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