Re: [PATCH] status: add a failing test showing a core.untrackedCache bug

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Dec 25, 2017, 18:45 UTC
Message-ID: <87wp1ar6j1.fsf@evledraar.gmail.com>
In-Reply-To: <CACsJy8B1FNpq-AYJdcs_gVOxdPSnh-kNaeVykLSSDL1+EW9YjA@mail.gmail.com>

On Mon, Dec 25 2017, Duy Nguyen jotted:

Show 12 quoted lines

> On Fri, Dec 22, 2017 at 9:00 PM, Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> The untracked cache gets confused when a directory is swapped out for
>> a symlink to another directory. Whatever files are inside the target
>> of the symlink will be incorrectly shown as untracked. This issue does
>> not happen if the symlink links to another file, only if it links to
>> another directory.
>
> Sounds about right (I completely forgot about dir symlinks). Since
> I've been away for some time and have not caught up (probably cannot)
> with the mailing list yet, is anyone working on this? It may be
> easiest to just detect symlinksand disable  the cache for now.

I haven't yet, I wanted to see what you had to say about it, i.e. whether it was a "do'h here's a fix" or if it was more involved than that.

Being completely unfamiliar with this code, I hacked up [1] to add some ad-hoc tracing to this. It has some bugs and doesn't actually work, but is injecting something into valid_cached_dir() and checking if the directory in question is a symlink the right approach?

Although surely a better solution is to just see that y is a symlink to x, and use the data we get for x.

I also see that the the untracked_cache_dir struct has a stat_data field which contains a subset of what we get from stat(), but it doesn't have st_mode, so you can't see from that if the thing was a symlink (but it could be added).

Is that the right approach? I.e. saving away whether it was a symlink and if it changes invalidate the cache, although it could be a symlink to something else, so may it needs to be keyed on st_ino (but that may be chagned in-place?).

1.

    diff --git a/dir.c b/dir.c
    index 3c54366a17..8afe068c72 100644
    --- a/dir.c
    +++ b/dir.c
    @@ -1730,10 +1730,13 @@ static int valid_cached_dir(struct dir_struct *dir,
                                int check_only)
     {
            struct stat st;
    +       struct stat st2;

            if (!untracked)
                    return 0;

    +       fprintf(stderr, "Checking <%s>\n", path->buf);
    +
            /*
             * With fsmonitor, we can trust the untracked cache's valid field.
             */
    @@ -1742,6 +1745,7 @@ static int valid_cached_dir(struct dir_struct *dir,
                    if (stat(path->len ? path->buf : ".", &st)) {
                            invalidate_directory(dir->untracked, untracked);
                            memset(&untracked->stat_data, 0, sizeof(untracked->stat_data));
    +                       fprintf(stderr, "Ret #1 = 0\n");
                            return 0;
                    }
                    if (!untracked->valid ||
    @@ -1749,12 +1753,14 @@ static int valid_cached_dir(struct dir_struct *dir,
                            if (untracked->valid)
                                    invalidate_directory(dir->untracked, untracked);
                            fill_stat_data(&untracked->stat_data, &st);
    +                       fprintf(stderr, "Ret #2 = 0\n");
                            return 0;
                    }
            }

            if (untracked->check_only != !!check_only) {
                    invalidate_directory(dir->untracked, untracked);
    +               fprintf(stderr, "Ret #3 = 0\n");
                    return 0;
            }

    @@ -1772,6 +1778,28 @@ static int valid_cached_dir(struct dir_struct *dir,
            } else
                    prep_exclude(dir, istate, path->buf, path->len);

    +       if (path->len && path->buf[path->len - 1] == '/') {
    +               struct strbuf dirbuf = STRBUF_INIT;
    +               strbuf_add(&dirbuf, path->buf, path->len - 1);
    +               fprintf(stderr, "Just dir = <%s>\n", dirbuf.buf);
    +
    +               if (lstat(dirbuf.buf, &st2)) {
    +                       fprintf(stderr, "Ret #4 = 0\n");
    +                       return 0;
    +               } else if (S_ISLNK(st2.st_mode)) {
    +                       invalidate_directory(dir->untracked, untracked);
    +                       memset(&untracked->stat_data, 0, sizeof(untracked->stat_data));
    +                       fill_stat_data(&untracked->stat_data, &st);
    +                       fprintf(stderr, "Is link = <%s>\n", dirbuf.buf);
    +                       return 0;
    +               } else {
    +                       fprintf(stderr, "Is not link = <%s> but <%d>\n", dirbuf.buf, st2.st_mode);
    +               }
    +       }
    +
    +       fprintf(stderr, "Falling through for <%s>\n", path->buf);
    +
    +
            /* hopefully prep_exclude() haven't invalidated this entry... */
            return untracked->valid;
     }
    @@ -1783,9 +1811,12 @@ static int open_cached_dir(struct cached_dir *cdir,
                               struct strbuf *path,
                               int check_only)
     {
    +       int valid;
            memset(cdir, 0, sizeof(*cdir));
            cdir->untracked = untracked;
    -       if (valid_cached_dir(dir, untracked, istate, path, check_only))
    +       valid = valid_cached_dir(dir, untracked, istate, path, check_only);
    +       fprintf(stderr, "Checked <%s>, valid? <%d>\n", path->buf, valid);
    +       if (valid)
                    return 0;
            cdir->fdir = opendir(path->len ? path->buf : ".");
            if (dir->untracked)