New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Builtin FSMonitor Part 2 #1041
Builtin FSMonitor Part 2 #1041
Conversation
/submit |
Submitted as pull.1041.git.1631822063.gitgitgadget@gmail.com To fetch this version into
To fetch this version to local tag
|
fsmonitor.c
Outdated
@@ -301,9 +301,25 @@ void refresh_fsmonitor(struct index_state *istate) | |||
core_fsmonitor, query_success ? "success" : "failure"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the Git mailing list, Bagas Sanjaya wrote (reply to this):
On 17/09/21 02.54, Jeff Hostetler via GitGitGadget wrote:
> - /* If we're going to check every file, ensure we save the results */
> + /*
> + * If we're going to check every file, ensure we save
> + * the results.
> + */
Why did you split the comment above?
--
An old man doll... just what I always wanted! - Clara
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Bagas Sanjaya <bagasdotme@gmail.com> writes:
> On 17/09/21 02.54, Jeff Hostetler via GitGitGadget wrote:
>> - /* If we're going to check every file, ensure we save the results */
>> + /*
>> + * If we're going to check every file, ensure we save
>> + * the results.
>> + */
>
> Why did you split the comment above?
I would guess that the reason why it is done is because the original
line is overly long it (extends to 84 columns, if I am counting
correctly).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the Git mailing list, Jeff Hostetler wrote (reply to this):
On 9/17/21 2:44 AM, Junio C Hamano wrote:
> Bagas Sanjaya <bagasdotme@gmail.com> writes:
>
>> On 17/09/21 02.54, Jeff Hostetler via GitGitGadget wrote:
>>> - /* If we're going to check every file, ensure we save the results */
>>> + /*
>>> + * If we're going to check every file, ensure we save
>>> + * the results.
>>> + */
>>
>> Why did you split the comment above?
>
> I would guess that the reason why it is done is because the original
> line is overly long it (extends to 84 columns, if I am counting
> correctly).
>
Yes, I just wrapped it because it was too long and the commit
was focused on improving other nearby comments (and no code),
so it seemed like a good opportunity to cleanup this one too.
Jeff
User |
0f445ea
to
05881a6
Compare
User |
fb5251e
to
7c22ce5
Compare
The pull request has 713 commits. The max allowed is 30. Please split the patch series into multiple pull requests. Also consider squashing related commits. |
There are merge commits in this Pull Request:
Please rebase the branch and force-push. |
/submit |
Submitted as pull.1041.v2.git.1633614772.gitgitgadget@gmail.com To fetch this version into
To fetch this version to local tag
|
@@ -897,6 +897,8 @@ LIB_OBJS += fetch-pack.o | |||
LIB_OBJS += fmt-merge-msg.o |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):
On Thu, Oct 07 2021, Jeff Hostetler via GitGitGadget wrote:
Good to see this move forward!
This bit:
> --- a/repo-settings.c
> +++ b/repo-settings.c
> @@ -20,6 +20,8 @@ void prepare_repo_settings(struct repository *r)
> if (r->settings.initialized++)
> return;
>
> + r->settings.fsmonitor = NULL; /* lazy loaded */
> +
> /* Defaults */
> r->settings.index_version = -1;
> r->settings.core_untracked_cache = UNTRACKED_CACHE_KEEP;
> diff --git a/repository.h b/repository.h
Is carried forward from v1, but with 3050b6dfc75 (repo-settings.c:
simplify the setup, 2021-09-21) isn't needed. It's init'd to 0/NULL
already, but was -1 before.
So this hunk can go, and its presence makes for confusing reading
without that history, is it set before somehow? No, just working around
older code that's no longer there.
But also: For untracked_cache_setting and fetch_negotiation_setting
we've got an embedded enum in the struct, but this...
> index a057653981c..89a1873ade7 100644
> --- a/repository.h
> +++ b/repository.h
> @@ -4,6 +4,7 @@
> #include "path.h"
>
> struct config_set;
> +struct fsmonitor_settings;
> struct git_hash_algo;
> struct index_state;
> struct lock_file;
> @@ -34,6 +35,8 @@ struct repo_settings {
> int command_requires_full_index;
> int sparse_index;
>
> + struct fsmonitor_settings *fsmonitor; /* lazy loaded */
> +
> int index_version;
> enum untracked_cache_setting core_untracked_cache;
>
Is a pointer to a struct that has an "enum fsmonitor_mode mode", and the
code in fsmonitor-settings.c seems to be repeating the patterns we had
in repo-settings.c pre-3050b6dfc75, e.g. checking whether a bool config
variable exists *and* is true, v.s. checking if it exists (presumably an
explicit false wants to override something).
I haven't looked carefully, but between that & the "char *hook_path"
being something that'll need to be made to use Emily's hook config
series sooner than later, can't we read/setup the initial config in
"repo_cfg_bool"?
The relevant commit message just says:
Move FSMonitor config settings to a new `struct fsmonitor_settings`
structure. Add a lazily-loaded pointer to `struct repo_settings`.
Create `fsm_settings__get_*()` getters to lazily look up fsmonitor-
related config settings.[...]
Which I think can be paraphrased as "Add scaffolding to repo-settings.c
but do config loading differently than everything there (lazily),
because...", except the "because" is missing :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the Git mailing list, Jeff Hostetler wrote (reply to this):
On 10/7/21 12:59 PM, Ævar Arnfjörð Bjarmason wrote:
>
> On Thu, Oct 07 2021, Jeff Hostetler via GitGitGadget wrote:
>
> Good to see this move forward!
>
> This bit:
>
>> --- a/repo-settings.c
>> +++ b/repo-settings.c
>> @@ -20,6 +20,8 @@ void prepare_repo_settings(struct repository *r)
>> if (r->settings.initialized++)
>> return;
>>
>> + r->settings.fsmonitor = NULL; /* lazy loaded */
>> +
>> /* Defaults */
>> r->settings.index_version = -1;
>> r->settings.core_untracked_cache = UNTRACKED_CACHE_KEEP;
>> diff --git a/repository.h b/repository.h
>
> Is carried forward from v1, but with 3050b6dfc75 (repo-settings.c:
> simplify the setup, 2021-09-21) isn't needed. It's init'd to 0/NULL
> already, but was -1 before.
>
> So this hunk can go, and its presence makes for confusing reading
> without that history, is it set before somehow? No, just working around
> older code that's no longer there.
Um, yeah, if the structure is now zero-filled on init
rather than the weird -1, then we don't need this here.
I'll remove in my next version. Thanks!
>
> But also: For untracked_cache_setting and fetch_negotiation_setting
> we've got an embedded enum in the struct, but this...
>
>> index a057653981c..89a1873ade7 100644
>> --- a/repository.h
>> +++ b/repository.h
>> @@ -4,6 +4,7 @@
>> #include "path.h"
>>
>> struct config_set;
>> +struct fsmonitor_settings;
>> struct git_hash_algo;
>> struct index_state;
>> struct lock_file;
>> @@ -34,6 +35,8 @@ struct repo_settings {
>> int command_requires_full_index;
>> int sparse_index;
>>
>> + struct fsmonitor_settings *fsmonitor; /* lazy loaded */
>> +
>> int index_version;
>> enum untracked_cache_setting core_untracked_cache;
>>
>
> Is a pointer to a struct that has an "enum fsmonitor_mode mode", and the
> code in fsmonitor-settings.c seems to be repeating the patterns we had
> in repo-settings.c pre-3050b6dfc75, e.g. checking whether a bool config
> variable exists *and* is true, v.s. checking if it exists (presumably an
> explicit false wants to override something).
My usage is a little more complicated because the historical value
had double duty. And I only want to set the hook value when not
using the builtin and when the repo itself is compatible with being
monitored. Later in the series we'll disallow remote worktrees,
for example. I wanted to hide some of those details in my
fsmonitor-settings.c and keep them out of repo-settings.c. And
to avoid paying for that when not needed, I made is a lazy-load
and hid the values behind an opaque type with access functions.
>
> I haven't looked carefully, but between that & the "char *hook_path"
> being something that'll need to be made to use Emily's hook config
> series sooner than later, can't we read/setup the initial config in
> "repo_cfg_bool"?
It's been a while since I looked at Emily's series. I'll have to
revisit that later. I do wonder if the existing fsmonitor hook
is in the same class as the other hooks or whether it is a one-off
and should be separate, but again I need to review that work first.
>
> The relevant commit message just says:
>
> Move FSMonitor config settings to a new `struct fsmonitor_settings`
> structure. Add a lazily-loaded pointer to `struct repo_settings`.
> Create `fsm_settings__get_*()` getters to lazily look up fsmonitor-
> related config settings.[...]
>
> Which I think can be paraphrased as "Add scaffolding to repo-settings.c
> but do config loading differently than everything there (lazily),
> because...", except the "because" is missing :)
>
Let me reword the commit message and try to explain what I was
going for. Thanks!
Jeff
User |
User |
7c22ce5
to
45a86ce
Compare
/submit |
Submitted as pull.1041.v3.git.1634157107.gitgitgadget@gmail.com To fetch this version into
To fetch this version to local tag
|
This branch is now known as |
This patch series was integrated into seen via git@a2db160. |
There was a status update in the "New Topics" section about the branch Built-in fsmonitor (part 2). |
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):
|
This patch series was integrated into seen via git@152d87c. |
This patch series was integrated into seen via git@83ac2a5. |
This patch series was integrated into seen via git@1358e8b. |
This patch series was integrated into seen via git@ff2299e. |
There was a status update in the "Cooking" section about the branch Built-in fsmonitor (part 2). Breaks check-docs lint. |
This patch series was integrated into seen via git@1b66cd6. |
45a86ce
to
507020b
Compare
This patch series was integrated into seen via git@8b30694. |
This patch series was integrated into seen via git@f10ffa3. |
/submit |
1 similar comment
/submit |
Submitted as pull.1041.v9.git.1648231393.gitgitgadget@gmail.com To fetch this version into
To fetch this version to local tag
|
On the Git mailing list, wrote (reply to this):
|
On the Git mailing list, Jeff Hostetler wrote (reply to this):
|
On the Git mailing list, Junio C Hamano wrote (reply to this):
|
This patch series was integrated into seen via git@ed85f76. |
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):
|
This patch series was integrated into seen via git@fb62166. |
On the Git mailing list, Jeff Hostetler wrote (reply to this):
|
On the Git mailing list, Jeff Hostetler wrote (reply to this):
|
This patch series was integrated into seen via git@8757070. |
There was a status update in the "Cooking" section about the branch Built-in fsmonitor (part 2). Will merge to 'next'? source: <pull.1041.v9.git.1648231393.gitgitgadget@gmail.com> |
This patch series was integrated into seen via git@7346297. |
This patch series was integrated into seen via git@9a78878. |
This patch series was integrated into seen via git@016d502. |
There was a status update in the "Cooking" section about the branch Built-in fsmonitor (part 2). Will merge to 'next'? source: <pull.1041.v9.git.1648231393.gitgitgadget@gmail.com> |
This patch series was integrated into seen via git@f8881e7. |
This patch series was integrated into seen via git@8815074. |
This patch series was integrated into next via git@72cda55. |
This patch series was integrated into seen via git@f6c2ea0. |
This patch series was integrated into seen via git@439c1e6. |
This patch series was integrated into master via git@439c1e6. |
This patch series was integrated into next via git@439c1e6. |
Closed via 439c1e6. |
Here is V9 of Part 2 of my builtin FSMonitor series. This version addresses bash
style issues in t7527 raised on V8. These changes do not require a new version
of Part 3.
Here is a range-diff from V8 to V9 relative to
715d08a9e5 (The eighth batch, 2022-02-25)
.cc: Bagas Sanjaya bagasdotme@gmail.com
cc: Ævar Arnfjörð Bjarmason avarab@gmail.com
cc: Jeff Hostetler git@jeffhostetler.com
cc: Eric Sunshine sunshine@sunshineco.com
cc: Johannes Schindelin Johannes.Schindelin@gmx.de
cc: Tao Klerks tao@klerks.biz
cc: rsbecker@nexbridge.com