From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Duy Nguyen <pclouds@gmail.com>
Cc: Git Mailing List <git@vger.kernel.org>,
Junio C Hamano <gitster@pobox.com>, Jeff King <peff@peff.net>,
Christian Couder <christian.couder@gmail.com>,
Ben Peart <benpeart@microsoft.com>
Subject: Re: [PATCH] status: add a failing test showing a core.untrackedCache bug
Date: Mon, 25 Dec 2017 19:45:06 +0100 [thread overview]
Message-ID: <87wp1ar6j1.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <CACsJy8B1FNpq-AYJdcs_gVOxdPSnh-kNaeVykLSSDL1+EW9YjA@mail.gmail.com>
On Mon, Dec 25 2017, Duy Nguyen jotted:
> 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)
next prev parent reply other threads:[~2017-12-25 19:06 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-12-22 14:00 [PATCH] status: add a failing test showing a core.untrackedCache bug Ævar Arnfjörð Bjarmason
2017-12-25 11:26 ` Duy Nguyen
2017-12-25 18:45 ` Ævar Arnfjörð Bjarmason [this message]
2017-12-26 10:47 ` Duy Nguyen
2017-12-27 10:25 ` Duy Nguyen
2017-12-27 11:28 ` [PATCH 3/1] update-index doc: note a fixed bug in the untracked cache Ævar Arnfjörð Bjarmason
2017-12-27 18:07 ` Junio C Hamano
2017-12-27 17:50 ` [PATCH] status: add a failing test showing a core.untrackedCache bug Eric Sunshine
2017-12-27 18:09 ` Junio C Hamano
2017-12-27 18:50 ` Ævar Arnfjörð Bjarmason
2017-12-28 6:10 ` Duy Nguyen
2017-12-28 18:34 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87wp1ar6j1.fsf@evledraar.gmail.com \
--to=avarab@gmail.com \
--cc=benpeart@microsoft.com \
--cc=christian.couder@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=pclouds@gmail.com \
--cc=peff@peff.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).