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

From: Duy Nguyen <pclouds@gmail.com>
Date: May 29, 2014, 11:42 UTC
Message-ID: <CACsJy8Dy2quFrXynH7g5DzNMikwkOiyJZGMoSqLyjxzzvRreYQ@mail.gmail.com>
In-Reply-To: <CAKpPgvfvrdBr7oxyKg2G1XDHwS=Dzs9=K9MmQLtLzUx-Zi+1dA@mail.gmail.com>

On Thu, May 29, 2014 at 5:11 AM, Pasha Bolokhov <pasha.bolokhov@gmail.com> wrote:

Show 14 quoted lines

>>> +       len = strlen(n_git); /* real_path() has stripped trailing slash */
>>> +       for (i = len - 1; i > 0 && !is_dir_sep(n_git[i]); i--) ;
>>> +       basename = n_git + i;
>>> +       if (is_dir_sep(*basename))
>>> +               basename++;
>>> +       if (strcmp(basename, ".git")) {
>>
>> I think normalize_path_copy makes sure that dir sep is '/', so this
>> code may be simplified to "if (strcmp(n_git, .git") && (len == 4 ||
>> strcmp(n_git + len - 5, "/.git")))"?
>
> Then if "n_git" is "/ab"  =>  coredump. But I agree there is logic to
> this (if we check len >= 4 first). However, we still need the
> basename.

Ah I missed this at add_exclude()

> So I've just shortened it a bit, what do you think: (notice
> the condition "i >= 0" btw)
>
>         for (i = len - 1; i >= 0 && n_git[i] != '/'; i--) ;

There's basename() that does this for you. A compat version is provided for Windows port so no portability worries.

Show 15 quoted lines

>         basename = n_git + i + 1;
>         if (strcmp(basename, ".git)) {
>
>>
>>> +               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.

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.

-- 
Duy