Re: [PATCH v4] Add an explicit GIT_DIR to the list of excludes

From: Pasha Bolokhov <pasha.bolokhov@gmail.com>
Date: Jun 3, 2014, 20:55 UTC
Message-ID: <CAKpPgvcv10-DNM4xetfmcckG6ancfEn_y7i8EJ3mWQLaELDdUQ@mail.gmail.com>
In-Reply-To: <CACsJy8Dy2quFrXynH7g5DzNMikwkOiyJZGMoSqLyjxzzvRreYQ@mail.gmail.com>

On Thu, May 29, 2014 at 4:42 AM, Duy Nguyen <pclouds@gmail.com> wrote:

Show 17 quoted lines

>>>> +               const char *worktree = get_git_work_tree();
>>>> +
>>>> +               if (worktree == NULL ||
>>>> +                   dir_inside_of(n_git, get_git_work_tree()) >= 0) {
>>>> +                       struct exclude_list *el = add_exclude_list(dir, EXC_CMDL,
>>>> +                                                       "GIT_DIR setup");
>>>> +
>>>> +                       /* append a trailing slash to exclude directories */
>>>> +                       n_git[len] = '/';
>>>> +                       n_git[len + 1] = '\0';
>>>> +                       add_exclude(basename, "", 0, el, 0);
>
> Hmm.. I overlooked this bit before. So if  $GIT_DIR is /something/foo,
> we set to ignore "foo/". Because we know n_git must be part of
> (normalized) get_git_work_tree() at this point, could we path n_git +
> strlen(get_git_work_tree()) to add_exclude() instead of basename? Full
> path makes sure we don't accidentally exclude too much.

I guess so. In fact, dir_inside_of() already returns the relative position, can reuse that (however, that function doesn't include the leading path, making it a relative path; but it's not difficult to work around). The only uncovered situation is when GIT_DIR=WORK_TREE. But that's user's fault and I don't think we need to guarantee that GIT_DIR will be excluded then

Show 8 quoted lines

>
> The case when $GIT_DIR points to a _file_ seems uncovered.
> setup_git_directory() will transform the file to the directory
> internally and we never know the .git file's path (so we can't exclude
> it). So people could accidentally add the .git file in, then remove it
> from from work tree and suddenly the work tree becomes repo-less. It's
> not as bad as .git _directory_ because we don't lose valuable data. I
> don't know if you want to cover this too.

That's right, there is no way of knowing what the original .git file was. I guess the only way to work around this problem is to modify "read_gitfile()" so it saves the name of the original file. Then we can add both that .git-file and GIT_DIR to the exclude list. Not a big problem with me, but need to see what you guys think