fsmonitor inline / testing cleanup #767
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nipunn1313
force-pushed
the
fsmonitor
branch
3 times, most recently
from
e46d7e5 to
5985210
Compare
|
/submit |
|
Submitted as pull.767.git.1603303474.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Taylor Blau wrote (reply to this): |
|
User |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
|
|
||
| /* 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); |
There was a problem hiding this comment.
On the Git mailing list, Taylor Blau wrote (reply to this):
On Wed, Oct 21, 2020 at 06:04:33PM +0000, Alex Vandiver via GitGitGadget wrote:
> From: Alex Vandiver <alexmv@dropbox.com>
>
> These were inline'd when they were first introduced, presumably as an
> optimization for cases when they were called in tight loops. This
> complicates using these functions, as untracked_cache_invalidate_path
> is defined in dir.h.
>
> Leave the inline'ing up to the compiler's decision, for ease of use.
Letting the compiler inline these is fine, but...
> diff --git a/fsmonitor.h b/fsmonitor.h
> index 739318ab6d..6249020692 100644
> --- a/fsmonitor.h
> +++ b/fsmonitor.h
> @@ -49,14 +49,7 @@ void refresh_fsmonitor(struct index_state *istate);
> * called any time the cache entry has been updated to reflect the
> * current state of the file on disk.
> */
> -static inline void mark_fsmonitor_valid(struct index_state *istate, struct cache_entry *ce)
> -{
> - if (core_fsmonitor && !(ce->ce_flags & CE_FSMONITOR_VALID)) {
> - istate->cache_changed = 1;
> - ce->ce_flags |= CE_FSMONITOR_VALID;
> - trace_printf_key(&trace_fsmonitor, "mark_fsmonitor_clean '%s'", ce->name);
> - }
> -}
> +extern void mark_fsmonitor_valid(struct index_state *istate, struct cache_entry *ce);
>
> /*
> * Clear the given cache entry's CE_FSMONITOR_VALID bit and invalidate
> @@ -65,13 +58,6 @@ static inline void mark_fsmonitor_valid(struct index_state *istate, struct cache
> * trigger an lstat() or invalidate the untracked cache for the
> * corresponding directory
> */
> -static inline void mark_fsmonitor_invalid(struct index_state *istate, struct cache_entry *ce)
> -{
> - if (core_fsmonitor) {
> - ce->ce_flags &= ~CE_FSMONITOR_VALID;
> - untracked_cache_invalidate_path(istate, ce->name, 1);
> - trace_printf_key(&trace_fsmonitor, "mark_fsmonitor_invalid '%s'", ce->name);
> - }
> -}
> +extern void mark_fsmonitor_invalid(struct index_state *istate, struct cache_entry *ce);
>
> #endif
Any reason that these need to be externed explicitly? Note that these
functions are already externed by default since you haven't said
otherwise (and for no other reason than this'd be the only explicitly
externed function in fsmonitor.h).
Thanks,
Taylor
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Taylor Blau <me@ttaylorr.com> writes:
>> +extern void mark_fsmonitor_invalid(struct index_state *istate, struct cache_entry *ce);
> ...
> Any reason that these need to be externed explicitly? Note that these
> functions are already externed by default since you haven't said
> otherwise (and for no other reason than this'd be the only explicitly
> externed function in fsmonitor.h).
Possibly due to the recent discussion?
https://lore.kernel.org/git/xmqqtuv3ryhr.fsf_-_@gitster.c.googlers.com/
There was a problem hiding this comment.
On the Git mailing list, Taylor Blau wrote (reply to this):
On Wed, Oct 21, 2020 at 02:24:22PM -0700, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
> > Any reason that these need to be externed explicitly? Note that these
> > functions are already externed by default since you haven't said
> > otherwise (and for no other reason than this'd be the only explicitly
> > externed function in fsmonitor.h).
>
> Possibly due to the recent discussion?
>
> https://lore.kernel.org/git/xmqqtuv3ryhr.fsf_-_@gitster.c.googlers.com/
Ah, thanks. I remember the thread, but I wasn't sure where the
discussion ended up. After re-reading it, it sounds like new function
declarations in header files should be prefixed with 'extern' (making
this patch correct as it already is).
Tangential to this discussion: are you still expecting a tree-wide
change to start use extern everywhere?
Thanks,
Taylor
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Taylor Blau <me@ttaylorr.com> writes:
> Tangential to this discussion: are you still expecting a tree-wide
> change to start use extern everywhere?
I think before we start opening the tree for new topics is the best
time to do so, if we were to follow through, but after we have dealt
with brown-paper-bag fixes to the release. The Makefile tweak for
the skip-dashed thing is the only one for 2.29, I think, so ...
Thanks.
There was a problem hiding this comment.
On the Git mailing list, Nipunn Koorapati wrote (reply to this):
> Letting the compiler inline these is fine, but...
>
> Any reason that these need to be externed explicitly? Note that these
> functions are already externed by default since you haven't said
> otherwise (and for no other reason than this'd be the only explicitly
> externed function in fsmonitor.h).
Did not have a reason or strong opinion here. It was this way, because this was
the way alexmv used it originally - but it does compile in either manner. The
thread Junio linked does seem to indicate preference for extern to avoid
confusion.
--Nipunn
|
This branch is now known as |
|
This patch series was integrated into seen via git@0329cdd. |
added 2 commits
October 22, 2020 00:18
These were inline'd when they were first introduced, presumably as an optimization for cases when they were called in tight loops. This complicates using these functions, as untracked_cache_invalidate_path is defined in dir.h. Leave the inline'ing up to the compiler's decision, for ease of use. Signed-off-by: Alex Vandiver <alexmv@dropbox.com> Signed-off-by: Nipunn Koorapati <nipunn@dropbox.com>
After displaying one very long line, summarize the contents of that
line. The tests do not currently rely on any content except the first
line ("no fsmonitor" / "fsmonitor last update").
Signed-off-by: Alex Vandiver <alexmv@dropbox.com>
Signed-off-by: Nipunn Koorapati <nipunn@dropbox.com>
|
On the Git mailing list, Nipunn Koorapati wrote (reply to this): |
|
User |
|
/submit |
|
Submitted as pull.767.v2.git.1603326066.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
This patch series was integrated into seen via git@0f00a96. |
|
On the Git mailing list, Taylor Blau wrote (reply to this): |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): |
|
On the Git mailing list, Taylor Blau 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@7803745. |
|
This patch series was integrated into seen via git@152d49d. |
|
On the Git mailing list, Nipunn Koorapati wrote (reply to this): |
|
This patch series was integrated into seen via git@0eaf313. |
|
This patch series was integrated into seen via git@3830ded. |
|
This patch series was integrated into seen via git@7398e42. |
|
This patch series was integrated into seen via git@8377462. |
|
This patch series was integrated into seen via git@6689f5b. |
|
This patch series was integrated into seen via git@e82be5f. |
|
This patch series was integrated into seen via git@b48426e. |
|
This patch series was integrated into seen via git@765fca7. |
|
This patch series was integrated into seen via git@74718a3. |
|
This patch series was integrated into seen via git@38c3f2a. |
|
This patch series was integrated into seen via git@4f79d43. |
|
This patch series was integrated into seen via git@a1ea2a2. |
|
This patch series was integrated into seen via git@71084aa. |
|
This patch series was integrated into seen via git@5222df7. |
|
This patch series was integrated into seen via git@4bf2d47. |
|
This patch series was integrated into seen via git@87827c4. |
|
This patch series was integrated into seen via git@3db4720. |
|
This patch series was integrated into seen via git@e8be73c. |
|
This patch series was integrated into seen via git@41b8a5e. |
|
This patch series was integrated into seen via git@14e53e3. |
|
This patch series was integrated into seen via git@809e811. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
UPDATE SINCE v1
Credit to alexmv again - I'm rebasing these changes from a couple years ago for contribution.
Full comments are available here -
https://public-inbox.org/git/01ad47b4-aa5e-461a-270b-dd60032afbd1@gmail.com/
To summarize the relevant points
Re: Inlining mark_fsmonitor_[in]valid
peartben said
Re test-dump-fsmonitor
peartben suggested keeping the +- syntax as well as the summary counts
dscho suggested dumping the invalid entries
cc: Taylor Blau me@ttaylorr.com
cc: Nipunn Koorapati nipunn1313@gmail.com