| Message ID | afd326c5011b09d89b6354817c1913d85142c335.1616016143.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 0ec9949f78e348250f55ba0343344ec8c606be11 |
| Headers | show |
| Series | teach git to respect fsmonitor in diff-index | expand |
"Nipunn Koorapati via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Nipunn Koorapati <nipunn@dropbox.com> > > Validate that fsmonitor is valid to futureproof against bugs where > check_removed might be called from places that haven't refreshed. Isn't this the other way around, wrt to the previous step? At least, "pass around istate throughout the callchain in the diff-lib.c file" change should stand alone and come much earlier in the series (perhaps as step #1). Then "call refresh_fsmonitor from run_diff_index() and make sure in check_removed() that fsmonitor does not have bogus VALID bit" assertion should come on top, as a single step, I would think. > Signed-off-by: Nipunn Koorapati <nipunn@dropbox.com> > --- > diff-lib.c | 18 +++++++++++------- > fsmonitor.h | 11 +++++++++++ > 2 files changed, 22 insertions(+), 7 deletions(-) > > diff --git a/diff-lib.c b/diff-lib.c > index 3fb538ad18e9..e5a58c9259cf 100644 > --- a/diff-lib.c > +++ b/diff-lib.c > @@ -28,8 +28,9 @@ > * exists for ce that is a submodule -- it is a submodule that is not > * checked out). Return negative for an error. > */ > -static int check_removed(const struct cache_entry *ce, struct stat *st) > +static int check_removed(const struct index_state *istate, const struct cache_entry *ce, struct stat *st) > { > + assert(is_fsmonitor_refreshed(istate)); > if (!(ce->ce_flags & CE_FSMONITOR_VALID) && lstat(ce->name, st) < 0) { > if (!is_missing_file_error(errno)) > return -1; > @@ -136,7 +137,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option) > memset(&(dpath->parent[0]), 0, > sizeof(struct combine_diff_parent)*5); > > - changed = check_removed(ce, &st); > + changed = check_removed(istate, ce, &st); > if (!changed) > wt_mode = ce_mode_from_stat(ce, st.st_mode); > else { > @@ -216,7 +217,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option) > } else { > struct stat st; > > - changed = check_removed(ce, &st); > + changed = check_removed(istate, ce, &st); > if (changed) { > if (changed < 0) { > perror(ce->name); > @@ -278,7 +279,8 @@ static void diff_index_show_file(struct rev_info *revs, > oid, oid_valid, ce->name, dirty_submodule); > } > > -static int get_stat_data(const struct cache_entry *ce, > +static int get_stat_data(const struct index_state *istate, > + const struct cache_entry *ce, > const struct object_id **oidp, > unsigned int *modep, > int cached, int match_missing, > @@ -290,7 +292,7 @@ static int get_stat_data(const struct cache_entry *ce, > if (!cached && !ce_uptodate(ce)) { > int changed; > struct stat st; > - changed = check_removed(ce, &st); > + changed = check_removed(istate, ce, &st); > if (changed < 0) > return -1; > else if (changed) { > @@ -321,12 +323,13 @@ static void show_new_file(struct rev_info *revs, > const struct object_id *oid; > unsigned int mode; > unsigned dirty_submodule = 0; > + struct index_state *istate = revs->diffopt.repo->index; > > /* > * New file in the index: it might actually be different in > * the working tree. > */ > - if (get_stat_data(new_file, &oid, &mode, cached, match_missing, > + if (get_stat_data(istate, new_file, &oid, &mode, cached, match_missing, > &dirty_submodule, &revs->diffopt) < 0) > return; > > @@ -342,8 +345,9 @@ static int show_modified(struct rev_info *revs, > unsigned int mode, oldmode; > const struct object_id *oid; > unsigned dirty_submodule = 0; > + struct index_state *istate = revs->diffopt.repo->index; > > - if (get_stat_data(new_entry, &oid, &mode, cached, match_missing, > + if (get_stat_data(istate, new_entry, &oid, &mode, cached, match_missing, > &dirty_submodule, &revs->diffopt) < 0) { > if (report_missing) > diff_index_show_file(revs, "-", old_entry, > diff --git a/fsmonitor.h b/fsmonitor.h > index 7f1794b90b00..f20d72631d76 100644 > --- a/fsmonitor.h > +++ b/fsmonitor.h > @@ -49,6 +49,17 @@ void refresh_fsmonitor(struct index_state *istate); > */ > int fsmonitor_is_trivial_response(const struct strbuf *query_result); > > +/* > + * Check if refresh_fsmonitor has been called at least once. > + * refresh_fsmonitor is idempotent. Returns true if fsmonitor is > + * not enabled (since the state will be "fresh" w/ CE_FSMONITOR_VALID unset) > + * This version is useful for assertions > + */ > +static inline int is_fsmonitor_refreshed(const struct index_state *istate) > +{ > + return !core_fsmonitor || istate->fsmonitor_has_run_once; > +} > + > /* > * Set the given cache entries CE_FSMONITOR_VALID bit. This should be > * called any time the cache entry has been updated to reflect the
On Thu, Mar 18, 2021 at 1:47 PM Junio C Hamano <gitster@pobox.com> wrote: > Isn't this the other way around, wrt to the previous step? > > At least, "pass around istate throughout the callchain in the > diff-lib.c file" change should stand alone and come much earlier in > the series (perhaps as step #1). Then "call refresh_fsmonitor from > run_diff_index() and make sure in check_removed() that fsmonitor > does not have bogus VALID bit" assertion should come on top, as a > single step, I would think. Just to make sure I understand - it sounds like you're recommending I split this diff up into two parts - one which passes istate through the callstack, and a second which provides this assertion. It also sounds like you're recommending reordering the series to 1 - pass istate around callstack 2 - add is_fsmonitor_refreshed and use it to assert fsmonitor is refreshed in check_removed in prep for usage 3 - use fsmonitor bit to save a call to lstat 4 - Add perf benchmark test This makes sense to me - and I can execute the factoring in the next patch series iteration --Nipunn
diff --git a/diff-lib.c b/diff-lib.c index 3fb538ad18e9..e5a58c9259cf 100644 --- a/diff-lib.c +++ b/diff-lib.c @@ -28,8 +28,9 @@ * exists for ce that is a submodule -- it is a submodule that is not * checked out). Return negative for an error. */ -static int check_removed(const struct cache_entry *ce, struct stat *st) +static int check_removed(const struct index_state *istate, const struct cache_entry *ce, struct stat *st) { + assert(is_fsmonitor_refreshed(istate)); if (!(ce->ce_flags & CE_FSMONITOR_VALID) && lstat(ce->name, st) < 0) { if (!is_missing_file_error(errno)) return -1; @@ -136,7 +137,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option) memset(&(dpath->parent[0]), 0, sizeof(struct combine_diff_parent)*5); - changed = check_removed(ce, &st); + changed = check_removed(istate, ce, &st); if (!changed) wt_mode = ce_mode_from_stat(ce, st.st_mode); else { @@ -216,7 +217,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option) } else { struct stat st; - changed = check_removed(ce, &st); + changed = check_removed(istate, ce, &st); if (changed) { if (changed < 0) { perror(ce->name); @@ -278,7 +279,8 @@ static void diff_index_show_file(struct rev_info *revs, oid, oid_valid, ce->name, dirty_submodule); } -static int get_stat_data(const struct cache_entry *ce, +static int get_stat_data(const struct index_state *istate, + const struct cache_entry *ce, const struct object_id **oidp, unsigned int *modep, int cached, int match_missing, @@ -290,7 +292,7 @@ static int get_stat_data(const struct cache_entry *ce, if (!cached && !ce_uptodate(ce)) { int changed; struct stat st; - changed = check_removed(ce, &st); + changed = check_removed(istate, ce, &st); if (changed < 0) return -1; else if (changed) { @@ -321,12 +323,13 @@ static void show_new_file(struct rev_info *revs, const struct object_id *oid; unsigned int mode; unsigned dirty_submodule = 0; + struct index_state *istate = revs->diffopt.repo->index; /* * New file in the index: it might actually be different in * the working tree. */ - if (get_stat_data(new_file, &oid, &mode, cached, match_missing, + if (get_stat_data(istate, new_file, &oid, &mode, cached, match_missing, &dirty_submodule, &revs->diffopt) < 0) return; @@ -342,8 +345,9 @@ static int show_modified(struct rev_info *revs, unsigned int mode, oldmode; const struct object_id *oid; unsigned dirty_submodule = 0; + struct index_state *istate = revs->diffopt.repo->index; - if (get_stat_data(new_entry, &oid, &mode, cached, match_missing, + if (get_stat_data(istate, new_entry, &oid, &mode, cached, match_missing, &dirty_submodule, &revs->diffopt) < 0) { if (report_missing) diff_index_show_file(revs, "-", old_entry, diff --git a/fsmonitor.h b/fsmonitor.h index 7f1794b90b00..f20d72631d76 100644 --- a/fsmonitor.h +++ b/fsmonitor.h @@ -49,6 +49,17 @@ void refresh_fsmonitor(struct index_state *istate); */ int fsmonitor_is_trivial_response(const struct strbuf *query_result); +/* + * Check if refresh_fsmonitor has been called at least once. + * refresh_fsmonitor is idempotent. Returns true if fsmonitor is + * not enabled (since the state will be "fresh" w/ CE_FSMONITOR_VALID unset) + * This version is useful for assertions + */ +static inline int is_fsmonitor_refreshed(const struct index_state *istate) +{ + return !core_fsmonitor || istate->fsmonitor_has_run_once; +} + /* * Set the given cache entries CE_FSMONITOR_VALID bit. This should be * called any time the cache entry has been updated to reflect the