git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Stefan Beller <sbeller@google.com>
Date
Jul 31, 2017, 23:41 UTC
Message-ID
<CAGZ79kYWd5WdwkC74+AdRt0anTUtngD7jt=aJv7iLCLf-TAfbA@mail.gmail.com>
In-Reply-To
<20170718190527.78049-8-bmwill@google.com>
On Tue, Jul 18, 2017 at 12:05 PM, Brandon Williams <bmwill@google.com> 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
Show 66 quoted lines
> submodule-config parsing.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
>  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);
  }
Show 10 quoted lines
> @@ -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.

Previous: Brandon WilliamsNext: Brandon Williams
Message 53 of 68 in “Convert grep to recurse in-process”
  1. 0/3 Convert grep to recurse in-processBrandon Williams, Jul 11, 2017
  2. 1/3 repo_read_index: don't discard the indexBrandon Williams, Jul 11, 2017
  3. Jonathan NiederJul 11, 2017
  4. Brandon WilliamsJul 12, 2017
  5. Stefan BellerJul 11, 2017
  6. Brandon WilliamsJul 12, 2017
  7. 2/3 setup: have the_repository use the_indexBrandon Williams, Jul 11, 2017
  8. Jonathan NiederJul 12, 2017
  9. Stefan BellerJul 12, 2017
  10. Brandon WilliamsJul 12, 2017
  11. Junio C HamanoJul 12, 2017
  12. Brandon WilliamsJul 12, 2017
  13. Junio C HamanoJul 12, 2017
  14. Jonathan NiederJul 12, 2017
  15. Junio C HamanoJul 12, 2017
  16. Junio C HamanoJul 18, 2017
  17. 3/3 grep: recurse in-process using 'struct repository'Brandon Williams, Jul 11, 2017
  18. Jacob KellerJul 11, 2017
  19. Brandon WilliamsJul 12, 2017
  20. Stefan BellerJul 12, 2017
  21. Brandon WilliamsJul 12, 2017
  22. Jonathan NiederJul 12, 2017
  23. Brandon WilliamsJul 12, 2017
  24. Jeff KingJul 12, 2017
  25. Brandon WilliamsJul 12, 2017
  26. Jeff KingJul 12, 2017
  27. Jonathan NiederJul 12, 2017
  28. Jeff KingJul 12, 2017
  29. Jonathan NiederJul 12, 2017
  30. Stefan BellerJul 12, 2017
  31. Jeff KingJul 12, 2017
  32. 0/3 Convert grep to recurse in-processBrandon Williams, Jul 14, 2017
  33. 1/3 repo_read_index: don't discard the indexBrandon Williams, Jul 14, 2017
  34. 2/3 repository: have the_repository use the_indexBrandon Williams, Jul 14, 2017
  35. 3/3 grep: recurse in-process using 'struct repository'Brandon Williams, Jul 14, 2017
  36. 00/10 Convert grep to recurse in-processBrandon Williams, Jul 18, 2017
  37. 01/10 repo_read_index: don't discard the indexBrandon Williams, Jul 18, 2017
  38. 02/10 repository: have the_repository use the_indexBrandon Williams, Jul 18, 2017
  39. 04/10 config: add config_from_gitmodulesBrandon Williams, Jul 18, 2017
  40. 03/10 cache.h: add GITMODULES_FILE macroBrandon Williams, Jul 18, 2017
  41. convert any hard coded .gitmodules file string to the MACROStefan Beller, Jul 31, 2017
  42. Jeff HostetlerAug 1, 2017
  43. Stefan BellerAug 1, 2017
  44. Junio C HamanoAug 1, 2017
  45. Brandon WilliamsAug 2, 2017
  46. Brandon WilliamsAug 2, 2017
  47. 08/10 submodule: check for unmerged .gitmodules outside of config parsingBrandon Williams, Jul 18, 2017
  48. 09/10 submodule: merge repo_read_gitmodules and gitmodules_configBrandon Williams, Jul 18, 2017
  49. 10/10 grep: recurse in-process using 'struct repository'Brandon Williams, Jul 18, 2017
  50. 05/10 submodule: remove submodule.fetchjobs from submodule-config parsingBrandon Williams, Jul 18, 2017
  51. 06/10 submodule: remove fetch.recursesubmodules from submodule-config parsingBrandon Williams, Jul 18, 2017
  52. 07/10 submodule: check for unstaged .gitmodules outside of config parsingBrandon Williams, Jul 18, 2017
  53. Stefan BellerJul 31, 2017
  54. Brandon WilliamsAug 2, 2017
  55. Brandon WilliamsAug 2, 2017
  56. Junio C HamanoJul 18, 2017
  57. Brandon WilliamsJul 18, 2017
  58. 00/10 Convert grep to recurse in-processBrandon Williams, Aug 2, 2017
  59. 01/10 repo_read_index: don't discard the indexBrandon Williams, Aug 2, 2017
  60. 02/10 repository: have the_repository use the_indexBrandon Williams, Aug 2, 2017
  61. 04/10 config: add config_from_gitmodulesBrandon Williams, Aug 2, 2017
  62. 06/10 submodule: remove fetch.recursesubmodules from submodule-config parsingBrandon Williams, Aug 2, 2017
  63. 03/10 cache.h: add GITMODULES_FILE macroBrandon Williams, Aug 2, 2017
  64. 08/10 submodule: check for unmerged .gitmodules outside of config parsingBrandon Williams, Aug 2, 2017
  65. 09/10 submodule: merge repo_read_gitmodules and gitmodules_configBrandon Williams, Aug 2, 2017
  66. 10/10 grep: recurse in-process using 'struct repository'Brandon Williams, Aug 2, 2017
  67. 05/10 submodule: remove submodule.fetchjobs from submodule-config parsingBrandon Williams, Aug 2, 2017
  68. 07/10 submodule: check for unstaged .gitmodules outside of config parsingBrandon Williams, Aug 2, 2017

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.