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

[Brandon Williams]

On 08/02, Brandon Williams wrote:
> On 07/31, Stefan Beller wrote:
> > 
> > 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.
> 
> I can drop the pos < active_nr requirement.

>From our conversation offline i'll leave this just in case there is some
subtle reason why it exists.  Also makes it more of a 1:1 conversion.

-- 
Brandon Williams

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

[Brandon Williams]

On 07/31, Stefan Beller wrote:
> On Tue, Jul 18, 2017 at 12:05 PM, Brandon Williams  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
> 

Will change.

> > submodule-config parsing.
> >
> > Signed-off-by: Brandon Williams 
> > ---
> >  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(_index))
> > die(_("Please stage your changes to .gitmodules or stash 
> > them to proceed"));
> > strbuf_addf(_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(_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, ) == 0 &&
> ce_match_stat(istate->cache[pos], , 0) & DATA_CHANGED);
>   }
> 
> > @@ -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", ) == 0 &&
> > -   ce_match_stat(active_cache[pos], , 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.

I can drop the pos < active_nr 

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

[Stefan Beller]

On Tue, Jul 18, 2017 at 12:05 PM, Brandon Williams  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

> submodule-config parsing.
>
> Signed-off-by: Brandon Williams 
> ---
>  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(_index))
> die(_("Please stage your changes to .gitmodules or stash them 
> to proceed"));
> strbuf_addf(_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(_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, ) == 0 &&
ce_match_stat(istate->cache[pos], , 0) & DATA_CHANGED);
  }

> @@ -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", ) == 0 &&
> -   ce_match_stat(active_cache[pos], , 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.

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

[Brandon Williams]

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
submodule-config parsing.

Signed-off-by: Brandon Williams 
---
 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(_index))
die(_("Please stage your changes to .gitmodules or stash them 
to proceed"));
strbuf_addf(_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(_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)) {
+   struct stat st;
+   if (lstat(GITMODULES_FILE, ) == 0 &&
+   ce_match_stat(istate->cache[pos], , 0) & DATA_CHANGED)
+   return 0;
+   }
+
+   return 1;
 }
 
 /*
@@ -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", ) == 0 &&
-   ce_match_stat(active_cache[pos], , 0) & 
DATA_CHANGED)
-   gitmodules_is_modified = 1;
}
 
if (!gitmodules_is_unmerged)
diff --git a/submodule.h b/submodule.h
index 29a1ecd19..b14660585 100644
--- a/submodule.h
+++ b/submodule.h
@@ -33,7 +33,7 @@ struct submodule_update_strategy {
 };
 #define SUBMODULE_UPDATE_STRATEGY_INIT {SM_UPDATE_UNSPECIFIED, NULL}
 
-extern int is_staging_gitmodules_ok(void);
+extern int is_staging_gitmodules_ok(const struct index_state *istate);
 extern int update_path_in_gitmodules(const char *oldpath, const char *newpath);
 extern int remove_path_from_gitmodules(const char *path);
 extern void stage_updated_gitmodules(void);
-- 
2.13.2.932.g7449e964c-goog