From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Kevin Willford via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org,
Kevin Willford <Kevin.Willford@microsoft.com>,
Kevin Willford <Kevin.Willford@microsoft.com>
Subject: Re: [PATCH v2 1/4] fsmonitor: change last update timestamp on the index_state to opaque token
Date: Wed, 5 Feb 2020 13:49:17 +0100 (CET) [thread overview]
Message-ID: <nycvar.QRO.7.76.6.2002051344020.3718@tvgsbejvaqbjf.bet> (raw)
In-Reply-To: <679bf4e0dd16f7d6f4ba1f05af50cca24b5abee4.1579793207.git.gitgitgadget@gmail.com>
Hi Kevin,
On Thu, 23 Jan 2020, Kevin Willford via GitGitGadget wrote:
> diff --git a/fsmonitor.c b/fsmonitor.c
> index 868cca01e2..9860587225 100644
> --- a/fsmonitor.c
> +++ b/fsmonitor.c
> @@ -89,11 +98,12 @@ void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)
> BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)",
> (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
>
> - put_be32(&hdr_version, INDEX_EXTENSION_VERSION);
> + put_be32(&hdr_version, INDEX_EXTENSION_VERSION2);
> strbuf_add(sb, &hdr_version, sizeof(uint32_t));
>
> - put_be64(&tm, istate->fsmonitor_last_update);
> - strbuf_add(sb, &tm, sizeof(uint64_t));
> + strbuf_addstr(sb, istate->fsmonitor_last_update);
> + strbuf_addch(sb, 0); /* Want to keep a NUL */
I have a slight preference to use '\0' here, which my brain somehow reads
as `NUL`.
> +
> fixup = sb->len;
> strbuf_add(sb, &ewah_size, sizeof(uint32_t)); /* we'll fix this up later */
>
> @@ -110,9 +120,9 @@ void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)
> }
>
> /*
> - * Call the query-fsmonitor hook passing the time of the last saved results.
> + * Call the query-fsmonitor hook passing the last update token of the saved results.
> */
> -static int query_fsmonitor(int version, uint64_t last_update, struct strbuf *query_result)
> +static int query_fsmonitor(int version, const char *last_update, struct strbuf *query_result)
> {
> struct child_process cp = CHILD_PROCESS_INIT;
>
> @@ -121,7 +131,7 @@ static int query_fsmonitor(int version, uint64_t last_update, struct strbuf *que
>
> argv_array_push(&cp.args, core_fsmonitor);
> argv_array_pushf(&cp.args, "%d", version);
> - argv_array_pushf(&cp.args, "%" PRIuMAX, (uintmax_t)last_update);
> + argv_array_pushf(&cp.args, "%s", last_update);
Maybe `argv_array_push(&cp.args, last_update)`?
> cp.use_shell = 1;
> cp.dir = get_git_work_tree();
>
> @@ -151,6 +161,7 @@ void refresh_fsmonitor(struct index_state *istate)
> int query_success = 0;
> size_t bol; /* beginning of line */
> uint64_t last_update;
> + struct strbuf last_update_token = STRBUF_INIT;
> char *buf;
> unsigned int i;
>
> @@ -164,6 +175,7 @@ void refresh_fsmonitor(struct index_state *istate)
> * should be inclusive to ensure we don't miss potential changes.
> */
> last_update = getnanotime();
> + strbuf_addf(&last_update_token, "%"PRIu64"", last_update);
>
> /*
> * If we have a last update time, call query_fsmonitor for the set of
> @@ -217,18 +229,21 @@ void refresh_fsmonitor(struct index_state *istate)
> }
> strbuf_release(&query_result);
>
> - /* Now that we've updated istate, save the last_update time */
> - istate->fsmonitor_last_update = last_update;
> + /* Now that we've updated istate, save the last_update_token */
> + FREE_AND_NULL(istate->fsmonitor_last_update);
> + istate->fsmonitor_last_update = strbuf_detach(&last_update_token, NULL);
I see quite a few `strbuf_detach()` calls in this patch, and could imagine
that this is a good indicator that the `fsmonitor_last_update` attribute
of `struct index_state` could be a `struct strbuf` instead, that is
`strbuf_reset()`ed and `strbuf_addf()`ed to, rather than having the
strbufs as local variables.
Other than that, this patch looks very good to me.
Thanks!
Dscho
> }
>
> void add_fsmonitor(struct index_state *istate)
> {
> unsigned int i;
> + struct strbuf last_update = STRBUF_INIT;
>
> if (!istate->fsmonitor_last_update) {
> trace_printf_key(&trace_fsmonitor, "add fsmonitor");
> istate->cache_changed |= FSMONITOR_CHANGED;
> - istate->fsmonitor_last_update = getnanotime();
> + strbuf_addf(&last_update, "%"PRIu64"", getnanotime());
> + istate->fsmonitor_last_update = strbuf_detach(&last_update, NULL);
>
> /* reset the fsmonitor state */
> for (i = 0; i < istate->cache_nr; i++)
> @@ -250,7 +265,7 @@ void remove_fsmonitor(struct index_state *istate)
> if (istate->fsmonitor_last_update) {
> trace_printf_key(&trace_fsmonitor, "remove fsmonitor");
> istate->cache_changed |= FSMONITOR_CHANGED;
> - istate->fsmonitor_last_update = 0;
> + FREE_AND_NULL(istate->fsmonitor_last_update);
> }
> }
>
> diff --git a/t/helper/test-dump-fsmonitor.c b/t/helper/test-dump-fsmonitor.c
> index 2786f47088..975f0ac890 100644
> --- a/t/helper/test-dump-fsmonitor.c
> +++ b/t/helper/test-dump-fsmonitor.c
> @@ -13,7 +13,7 @@ int cmd__dump_fsmonitor(int ac, const char **av)
> printf("no fsmonitor\n");
> return 0;
> }
> - printf("fsmonitor last update %"PRIuMAX"\n", (uintmax_t)istate->fsmonitor_last_update);
> + printf("fsmonitor last update %s\n", istate->fsmonitor_last_update);
>
> for (i = 0; i < istate->cache_nr; i++)
> printf((istate->cache[i]->ce_flags & CE_FSMONITOR_VALID) ? "+" : "-");
> --
> gitgitgadget
>
>
next prev parent reply other threads:[~2020-02-05 12:49 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-01-07 19:04 [PATCH 0/4] fsmonitor: start using an opaque token for last update Kevin Willford via GitGitGadget
2020-01-07 19:04 ` [PATCH 1/4] fsmonitor: change last update timestamp on the index_state to opaque token Kevin Willford via GitGitGadget
2020-01-07 19:04 ` [PATCH 2/4] fsmonitor: handle version 2 of the hooks that will use " Kevin Willford via GitGitGadget
2020-01-07 19:04 ` [PATCH 3/4] fsmonitor: add fsmonitor hook scripts for version 2 Kevin Willford via GitGitGadget
2020-01-07 19:04 ` [PATCH 4/4] fsmonitor: update documentation for hook version and watchman hooks Kevin Willford via GitGitGadget
2020-01-23 15:26 ` [PATCH v2 0/4] fsmonitor: start using an opaque token for last update Kevin Willford via GitGitGadget
2020-01-23 15:26 ` [PATCH v2 1/4] fsmonitor: change last update timestamp on the index_state to opaque token Kevin Willford via GitGitGadget
2020-02-05 12:49 ` Johannes Schindelin [this message]
2020-01-23 15:26 ` [PATCH v2 2/4] fsmonitor: handle version 2 of the hooks that will use " Kevin Willford via GitGitGadget
2020-02-05 13:11 ` Johannes Schindelin
2020-01-23 15:26 ` [PATCH v2 3/4] fsmonitor: add fsmonitor hook scripts for version 2 Kevin Willford via GitGitGadget
2020-01-23 15:26 ` [PATCH v2 4/4] fsmonitor: update documentation for hook version and watchman hooks Kevin Willford via GitGitGadget
2020-02-05 13:15 ` [PATCH v2 0/4] fsmonitor: start using an opaque token for last update Johannes Schindelin
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=nycvar.QRO.7.76.6.2002051344020.3718@tvgsbejvaqbjf.bet \
--to=johannes.schindelin@gmx.de \
--cc=Kevin.Willford@microsoft.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
/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).