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

On Thu, May 29, 2014 at 5:11 AM, Pasha Bolokhov
[off-list ref] wrote:

quoted

quoted

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

        basename = n_git + i + 1;
        if (strcmp(basename, ".git)) {

quoted

quoted

+               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