diff mbox series

[2/3] fsmonitor: add assertion that fsmonitor is valid to check_removed

Message ID dda5b537a3f0706ebf933e2b2efd996267e9d9b1.1615760258.git.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series teach git to respect fsmonitor in diff-index | expand

Commit Message

Nipunn Koorapati March 14, 2021, 10:17 p.m. UTC 
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.

Signed-off-by: Nipunn Koorapati <nipunn@dropbox.com>
---
 diff-lib.c  | 18 +++++++++++-------
 fsmonitor.h | 11 +++++++++++
 2 files changed, 22 insertions(+), 7 deletions(-)

Comments

Eric Sunshine March 14, 2021, 10:35 p.m. UTC | #1 
On Sun, Mar 14, 2021 at 6:19 PM Nipunn Koorapati via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> Validate that fsmonitor is valid to futureproof against bugs where
> check_removed might be called from places that haven't refreshed.
>
> Signed-off-by: Nipunn Koorapati <nipunn@dropbox.com>
> ---
> diff --git a/fsmonitor.h b/fsmonitor.h
> @@ -49,6 +49,17 @@ void refresh_fsmonitor(struct index_state *istate);
> +/*
> + * 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;
> +}

Unusual 4-space indentation rather than typical 1-tab.
Nipunn Koorapati March 15, 2021, 10:01 p.m. UTC | #2 
> Unusual 4-space indentation rather than typical 1-tab.

Thanks for identifying - will fix in the next patch series. Perhaps in
a separate patch we could add a unit test that validates the codebase
for such style?
Eric Sunshine March 15, 2021, 10:51 p.m. UTC | #3 
On Mon, Mar 15, 2021 at 6:01 PM Nipunn Koorapati <nipunn1313@gmail.com> wrote:
> > Unusual 4-space indentation rather than typical 1-tab.
>
> Thanks for identifying - will fix in the next patch series. Perhaps in
> a separate patch we could add a unit test that validates the codebase
> for such style?

My guess is that a test which complains about existing style
violations would not be particularly helpful since it's output likely
would be noisy due to existing style violations. (For the same reason,
we wouldn't want to complain about style violations in tests either
since existing test scripts are full of violations.) What is more
interesting than identifying existing violations is identifying
violations before they make it into the codebase. For instance, you
could run your patches through `checkpatch.pl`[1] before submitting
them.

[1]: https://github.com/torvalds/linux/blob/master/scripts/checkpatch.pl
diff mbox series

Patch

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..c12f10117544 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