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

From: Pasha Bolokhov <pasha.bolokhov@gmail.com>
Date: May 28, 2014, 22:11 UTC
Message-ID: <CAKpPgvfvrdBr7oxyKg2G1XDHwS=Dzs9=K9MmQLtLzUx-Zi+1dA@mail.gmail.com>
In-Reply-To: <CACsJy8By-58wBoV8BydWbpDkpUG6jJhM16aO+Ed8J2ucq66VcQ@mail.gmail.com>

On Wed, May 28, 2014 at 5:36 AM, Duy Nguyen <pclouds@gmail.com> wrote:

Show 28 quoted lines

> On Tue, May 27, 2014 at 10:56 AM, Pasha Bolokhov
> <pasha.bolokhov@gmail.com> wrote:
>> @@ -1588,6 +1588,38 @@ void setup_standard_excludes(struct dir_struct *dir)
>>  {
>>         const char *path;
>>         char *xdg_path;
>> +       const char *r_git, *gitdir = get_git_dir();
>> +       char *n_git, *basename;
>> +       int len, i;
>> +
>> +       /*
>> +        * Add git directory to the ignores; do this only if
>> +        * GIT_DIR does not end with "/.git"
>> +        */
>> +       r_git = real_path(absolute_path(gitdir));
>> +       n_git = xmalloc(strlen(r_git) + 1 + 1);
>> +       normalize_path_copy(n_git, r_git);
>> +
>> +       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. 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--) ;
        basename = n_git + i + 1;
        if (strcmp(basename, ".git)) {

Show 19 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);
>> +               }
>> +       }
>> +       free(n_git);
>
> All this add-only code makes me think it may be nice to make it a
> separate function. A good function name could replace the comment near
> the beginning of the block.

Reasonable I'll send the all-updated patch including doc when ready

> --
> Duy