Thread (57 messages) flat view 57 messages, 6 authors, 2017-08-02

Re: [PATCH v3 07/10] submodule: check for unstaged .gitmodules outside of config parsing

From: Stefan Beller <hidden>
Date: 2017-07-31 23:41:47

On Tue, Jul 18, 2017 at 12:05 PM, Brandon Williams [off-list ref] wrote:
Teach 'is_staging_gitmodules_ok()' to be able to determine in the
'.gitmodules' file has unstaged changes based on the passed in index
instead of relying on a global varible which is set during the
variable
quoted hunk ↗ jump to hunk
submodule-config parsing.

Signed-off-by: Brandon Williams <redacted>
---
 builtin/mv.c |  2 +-
 builtin/rm.c |  2 +-
 submodule.c  | 32 +++++++++++++++++---------------
 submodule.h  |  2 +-
 4 files changed, 20 insertions(+), 18 deletions(-)
diff --git a/builtin/mv.c b/builtin/mv.c
index dcf6736b5..94fbaaa5d 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -81,7 +81,7 @@ static void prepare_move_submodule(const char *src, int first,
        struct strbuf submodule_dotgit = STRBUF_INIT;
        if (!S_ISGITLINK(active_cache[first]->ce_mode))
                die(_("Directory %s is in index and no submodule?"), src);
-       if (!is_staging_gitmodules_ok())
+       if (!is_staging_gitmodules_ok(&the_index))
                die(_("Please stage your changes to .gitmodules or stash them to proceed"));
        strbuf_addf(&submodule_dotgit, "%s/.git", src);
        *submodule_gitfile = read_gitfile(submodule_dotgit.buf);
diff --git a/builtin/rm.c b/builtin/rm.c
index 52826d137..4057e73fa 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -286,7 +286,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
                list.entry[list.nr].name = xstrdup(ce->name);
                list.entry[list.nr].is_submodule = S_ISGITLINK(ce->ce_mode);
                if (list.entry[list.nr++].is_submodule &&
-                   !is_staging_gitmodules_ok())
+                   !is_staging_gitmodules_ok(&the_index))
                        die (_("Please stage your changes to .gitmodules or stash them to proceed"));
        }
diff --git a/submodule.c b/submodule.c
index b1965290f..46ec04d7c 100644
--- a/submodule.c
+++ b/submodule.c
@@ -37,18 +37,25 @@ static struct oid_array ref_tips_after_fetch;
 static int gitmodules_is_unmerged;

 /*
- * This flag is set if the .gitmodules file had unstaged modifications on
- * startup. This must be checked before allowing modifications to the
- * .gitmodules file with the intention to stage them later, because when
- * continuing we would stage the modifications the user didn't stage herself
- * too. That might change in a future version when we learn to stage the
- * changes we do ourselves without staging any previous modifications.
+ * Check if the .gitmodules file has unstaged modifications.  This must be
+ * checked before allowing modifications to the .gitmodules file with the
+ * intention to stage them later, because when continuing we would stage the
+ * modifications the user didn't stage herself too. That might change in a
+ * future version when we learn to stage the changes we do ourselves without
+ * staging any previous modifications.
  */
-static int gitmodules_is_modified;
-
-int is_staging_gitmodules_ok(void)
+int is_staging_gitmodules_ok(const struct index_state *istate)
 {
-       return !gitmodules_is_modified;
+       int pos = index_name_pos(istate, GITMODULES_FILE, strlen(GITMODULES_FILE));
+
+       if ((pos >= 0) && (pos < istate->cache_nr)) {
Why do we need the second check (pos < istate->cache_nr) ?

I would have assumed the first one suffices,
it might read better if turned around:


    if (pos < 0)
        return 1;

    return (lstat(GITMODULES_FILE, &st) == 0 &&
        ce_match_stat(istate->cache[pos], &st, 0) & DATA_CHANGED);
  }
quoted hunk ↗ jump to hunk
@@ -231,11 +238,6 @@ void gitmodules_config(void)
                                    !memcmp(ce->name, ".gitmodules", 11))
                                        gitmodules_is_unmerged = 1;
                        }
-               } else if (pos < active_nr) {
-                       struct stat st;
-                       if (lstat(".gitmodules", &st) == 0 &&
-                           ce_match_stat(active_cache[pos], &st, 0) & DATA_CHANGED)
-                               gitmodules_is_modified = 1;
                }
So this is where the check "pos < active_nr" is coming from,
introduced in 5fee995244 (submodule.c: add .gitmodules staging
helper functions, 2013-07-30) as well as d4e98b581b (Submodules:
Don't parse .gitmodules when it contains, merge conflicts, 2011-05-14).

If I am reading the docs for cache_name_pos correctly, we would
not need to check for the index exceeding active_cache,
but checking for the index not being out of bounds seems
to be wide spread.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help