On 10/21/21 4:40 PM, Junio C Hamano wrote:
"Jeff Hostetler via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
From: Jeff Hostetler <jeffhost@xxxxxxxxxxxxx>
Signed-off-by: Jeff Hostetler <jeffhost@xxxxxxxxxxxxx>
---
fsmonitor.c | 37 ++++++++++++++++++++++++++++++-------
1 file changed, 30 insertions(+), 7 deletions(-)
diff --git a/fsmonitor.c b/fsmonitor.c
index ab9bfc60b34..ec4c46407c5 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -301,9 +301,25 @@ void refresh_fsmonitor(struct index_state *istate)
core_fsmonitor, query_success ? "success" : "failure");
}
- /* a fsmonitor process can return '/' to indicate all entries are invalid */
+ /*
+ * The response from FSMonitor (excluding the header token) is
+ * either:
+ *
+ * [a] a (possibly empty) list of NUL delimited relative
+ * pathnames of changed paths. This list can contain
+ * files and directories. Directories have a trailing
+ * slash.
+ *
+ * [b] a single '/' to indicate the provider had no
+ * information and that we should consider everything
+ * invalid. We call this a trivial response.
+ */
if (query_success && query_result.buf[bol] != '/') {
- /* Mark all entries returned by the monitor as dirty */
+ /*
+ * Mark all pathnames returned by the monitor as dirty.
+ *
+ * This updates both the cache-entries and the untracked-cache.
+ */
Not a problem this patch introduces, but we only checked that the
query result begins with a slash, not "we did receive a trivial
response", but the "else" clause of this statement pretends as if we
did.
It is a shame that we do have fsmonitor_is_trivial_response()
function defined, but its interface is not capable of helping us
here.
Or is fsmonitor_is_trivial_response() already good to do this, and
reliance of [bol] this code has is the source of confusion? I
notice that when we have last update token and makes a call to
query_fsmonitor() with HOOK_INTERFACE_VERSION1, nobody updates bol
(hence it stays 0), and with HOOK_INTERFACE_VERSION2, bol is at the
NUL that terminates the initial string of the query result, after
which presumably has either '/' NUL (trivial) or list of paths.
I am not sure about the VERSION1 case, but at least in the VERSION2
case, making sure that the last three bytes are NUL slash NUL like
fsmonitor_is_trivial_response() does and the half check the above is
doing (i.e. the byte after the NUL is slash, without making sure
about the length of the whole response or what follows the slash is
NUL), seems "close enough" (meaning: the check in this code is a
sloppy attempt to reinvent what _is_trivial_response() function
already does).
So, would it make sense to rewrite the condition to
if (query_success &&
!fsmonitor_is_trivial_response(&query_result)) {
here? Or perhaps
if (query_success &&
!(query_result.len == bol + 3 &&
query_result[bol] == '/' && !query_result[bol+1])) {
which would be open coding the _is_trivial_response() function.
Yes, there is an opportunity here to clean this up.
The original code was a little sloppy -- just testing buf[bol] for
slash and assuming that that was sufficient. Technically, no response
should begin with slash (since the set of reported modified files are
relative to the root directory), so my _is_trivial_ function could be
simplified a little.
I'll refactor this.
Thanks
Jeff
