git-mv: allow submodules and fsmonitor to work together
diff mbox series

Message ID 20180906203444.162213-1-sbeller@google.com
State New
Headers show
Series
  • git-mv: allow submodules and fsmonitor to work together
Related show

Commit Message

Stefan Beller Sept. 6, 2018, 8:34 p.m. UTC
It was reported that

  GIT_FSMONITOR_TEST=$PWD/t7519/fsmonitor-all ./t7411-submodule-config.sh

breaks as the .gitmodules file is modified and staged after the fsmonitor
considers it clean. Mark the .gitmodules file to be not clean before
staging.

Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Inspired-by: Ben Peart <benpeart@microsoft.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---

I am not quite sure if this is the correct approach and handling of the
fsmonitor API, but it unbreaks the test.

> Just naively adding mark_fsmonitor_invalid doesn't work, as then ...

Adding it before the staging, works.

Please double check!

Thanks,
Stefan

 submodule.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Ben Peart Sept. 10, 2018, 3:58 p.m. UTC | #1
On 9/6/2018 4:34 PM, Stefan Beller wrote:
> It was reported that
> 
>    GIT_FSMONITOR_TEST=$PWD/t7519/fsmonitor-all ./t7411-submodule-config.sh
> 
> breaks as the .gitmodules file is modified and staged after the fsmonitor
> considers it clean. Mark the .gitmodules file to be not clean before
> staging.
> 
> Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Inspired-by: Ben Peart <benpeart@microsoft.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> 
> I am not quite sure if this is the correct approach and handling of the
> fsmonitor API, but it unbreaks the test.
> 
>> Just naively adding mark_fsmonitor_invalid doesn't work, as then ...
> 
> Adding it before the staging, works.
> 
> Please double check!

I took a look at this bug/patch and wondered why add_file_to_index() 
wasn't properly handling the .gitmodules file.  On investigation, I 
chased it down to what looks like a faulty test in 
is_staging_gitmodules_ok().

I believe the following is a better patch for this bug:

diff --git a/submodule.c b/submodule.c
index 50cbf5f13e..1e7194af28 100644
--- a/submodule.c
+++ b/submodule.c
@@ -65,8 +65,7 @@ int is_staging_gitmodules_ok(struct index_state *istate)
         if ((pos >= 0) && (pos < istate->cache_nr)) {
                 struct stat st;
                 if (lstat(GITMODULES_FILE, &st) == 0 &&
-                   ie_match_stat(istate, istate->cache[pos], &st,
-                                 CE_MATCH_IGNORE_FSMONITOR) & DATA_CHANGED)
+                   ie_match_stat(istate, istate->cache[pos], &st, 0) & 
DATA_CHANGED)
                         return 0;
         }

Please double check but I just don't understand why the .gitmodules file 
should force the fsmonitor data to be ignored.  This flag was added to 
enable proper behavior in the preload_thread() logic and I don't believe 
it is appropriate here.

Ben

> 
> Thanks,
> Stefan
> 
>   submodule.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/submodule.c b/submodule.c
> index 50cbf5f13ed..56b0d5fe24e 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -22,6 +22,7 @@
>   #include "worktree.h"
>   #include "parse-options.h"
>   #include "object-store.h"
> +#include "fsmonitor.h"
>   
>   static int config_update_recurse_submodules = RECURSE_SUBMODULES_OFF;
>   static struct string_list changed_submodule_names = STRING_LIST_INIT_DUP;
> @@ -149,6 +150,15 @@ int remove_path_from_gitmodules(const char *path)
>   
>   void stage_updated_gitmodules(struct index_state *istate)
>   {
> +	struct cache_entry *ce;
> +	int pos;
> +
> +	pos = index_name_pos(istate, GITMODULES_FILE, strlen(GITMODULES_FILE));
> +	ce = (0 <= pos) ? istate->cache[pos] : NULL;
> +
> +	if (ce)
> +		mark_fsmonitor_invalid(istate, ce);
> +
>   	if (add_file_to_index(istate, GITMODULES_FILE, 0))
>   		die(_("staging updated .gitmodules failed"));
>   }
>

Patch
diff mbox series

diff --git a/submodule.c b/submodule.c
index 50cbf5f13ed..56b0d5fe24e 100644
--- a/submodule.c
+++ b/submodule.c
@@ -22,6 +22,7 @@ 
 #include "worktree.h"
 #include "parse-options.h"
 #include "object-store.h"
+#include "fsmonitor.h"
 
 static int config_update_recurse_submodules = RECURSE_SUBMODULES_OFF;
 static struct string_list changed_submodule_names = STRING_LIST_INIT_DUP;
@@ -149,6 +150,15 @@  int remove_path_from_gitmodules(const char *path)
 
 void stage_updated_gitmodules(struct index_state *istate)
 {
+	struct cache_entry *ce;
+	int pos;
+
+	pos = index_name_pos(istate, GITMODULES_FILE, strlen(GITMODULES_FILE));
+	ce = (0 <= pos) ? istate->cache[pos] : NULL;
+
+	if (ce)
+		mark_fsmonitor_invalid(istate, ce);
+
 	if (add_file_to_index(istate, GITMODULES_FILE, 0))
 		die(_("staging updated .gitmodules failed"));
 }