threads / rfc / 49010

RFC v2, 12 partssubmodule: add a print_config_from_gitmodules() helper

Subject: [RFC PATCH v2 01/12] submodule: add a print_config_from_gitmodules() helper

## tl;dr

38 messages between Aug 2, 2018 and Aug 9, 2018. Diffs are folded; open one to read it.

replies: 37people: 5as markdown or json

Antonio Ospite· Aug 2, 2018, 13:46 UTC · lore

[RFC PATCH v2 00/12] Make submodules work if .gitmodules is not checked out

Hi,

this is a second version of the RFC from https://public-inbox.org/git/20180514105823.8378-1-ao2@ao2.it/

Please refer to the cover letter of the previous series for the motivation and background of the changes.

Patches from 01 to 08 should be rather uncontroversial, as they do not introduce any change of behavior but just prepare the ground for patches 09 and 10.

The last two patches, 11 and 12 are not strictly related to the topic, it's just an idea I came up with while reading the code for the series; I could send them separately if you think they are valid, or drop them.

The behavior of accessing HEAD:.gitmodules have been analyzed with the following test matrix:

Predicates:
  G = .gitmodules exists
  H = HEAD:.gitmodules exists
Operations:
  read = git submodule--helper config name
  write = git submodule--helper config name value
  stage = git submodule--helper config --stage
!G and !H:
  - read: nop
  - write: set value to .gitmodules (we are creating a brand new
    .gitmodules)
  - stage: exit with error, staging requires .gitmodules (like git
    add/rm)
G and !H:
  - read: get value from from .gitmodules
  - write: set value to .gitmodules
  - stage: stage .gitmodules
!G and H:
  - read: get the value from HEAD:.gitmodules
  - write: exit with error, setting a value requires .gitmodules
  - stage: exit with error, staging requires .gitmodules
  
G and H:
  - read: get value from from .gitmodules
  - write: set value to .gitmodules
  - stage: stage .gitmodules

Note that "git rm" and "git mv" will keep working when !G just as t3600-rm.sh and t7001-mv.sh mandate, I am not sure if they should fail if "!G and H".

In my previous attempt I also tried to check for the skip-worktree bit, but I am not sure if this is needed.

My intuition suggested the following behavior to cover the case of a manually crafted .gitmodules which could be committed and override the hidden one:

S = .gitmodules has the skip-worktree bit set
S:
  - read: get value from .gitmodules if G or from HEAD:.gitmodules if H
  - write: exit with error, .gitmodules is not supposed to be changed
  - stage: exit with error, .gitmodules is not supposed to be changed

However it looks like git gives the user quite some freedom to add, stage, commit, remove files even when they have the skip-worktree bit set (which was a little surprising to me TBH) so I am leaving this out for now.

Changes since v1:
  * Added a helper to print config values from the gitmodules
    configuration, the code is now in submoudle-config.c instead of
    buildtin/submodule--helper.c
  * Renamed config_gitmodules_set to
    config_set_in_gitmodules_file_gently and put it in
    submodule-config.c instead of submodule.c
    The naming follows the style of git config functions, and also takes
    into account Stefan Beller's comment about the final purpose of the
    function: we only write gitmodules config to the actual .gitmodules
    file, so we may as well reflect that in the function name,
  * Started using "working tree" instead of "work tree" as the former is
    the official term for the set of checked out files.
  * Start referring to HEAD as the "current branch" instead of "the
    index", the former should be more accurate.
  * Renamed GITMODULES_BLOB to GITMODULES_HEAD because GITMODULES_BLOB
    was added in commit 59e7b080 ("fsck: detect gitmodules files",
    2018-05-02)
  * Do not check for the skip-worktree bit on .gitmodules anymore, just
    check for file existence when appropriate.
  * Renamed t7415-submodule-sparse-gitmodules.sh to
    t7416-submodule-sparse-gitmodules.sh because t7415 was taken by
    t7415-submodule-names.sh
  * Made "git mv" and "git rm" work again when .gitmodules does not
    exist.
  * Removed checks about the  skip-worktree bit.
Thanks,
   Antonio
Antonio Ospite (12):
  submodule: add a print_config_from_gitmodules() helper
  submodule: factor out a config_set_in_gitmodules_file_gently function
  t7411: be nicer to future tests and really clean things up
  submodule--helper: add a new 'config' subcommand
  submodule: use the 'submodule--helper config' command
  submodule--helper: add a '--stage' option to the 'config' sub command
  submodule: use 'submodule--helper config --stage' to stage .gitmodules
  t7506: cleanup .gitmodules properly before setting up new scenario
  submodule: support reading .gitmodules even when it's not checked out
  t7416: add new test about HEAD:.gitmodules and not existing
    .gitmodules
  dir: move is_empty_file() from builtin/am.c to dir.c and make it
    public
  submodule: remove the .gitmodules file when it is empty
 builtin/am.c                           |  15 ----
 builtin/submodule--helper.c            |  82 ++++++++++++++++++
 cache.h                                |   1 +
 dir.c                                  |  16 ++++
 dir.h                                  |   1 +
 git-submodule.sh                       |  10 +--
 submodule-config.c                     |  53 +++++++++++-
 submodule-config.h                     |   3 +
 submodule.c                            |  20 +++--
 t/t3600-rm.sh                          |  32 +++----
 t/t7400-submodule-basic.sh             |  11 +++
 t/t7411-submodule-config.sh            |  65 +++++++++++++-
 t/t7416-submodule-sparse-gitmodules.sh | 112 +++++++++++++++++++++++++
 t/t7506-status-submodule.sh            |   3 +-
 14 files changed, 375 insertions(+), 49 deletions(-)
 create mode 100755 t/t7416-submodule-sparse-gitmodules.sh
-- 
Antonio Ospite
https://ao2.it
https://twitter.com/ao2it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
Antonio Ospite· Aug 2, 2018, 13:46 UTC · re: Antonio Ospite · lore

This will be used to print values just like "git config -f .gitmodules" would.

Signed-off-by: Antonio Ospite <ao2@ao2.it>
---
 submodule-config.c | 25 +++++++++++++++++++++++++
 submodule-config.h |  2 ++
 2 files changed, 27 insertions(+)
Show changes to 2 files +27 −0

submodule-config.c, submodule-config.h

diff --git a/submodule-config.c b/submodule-config.c
index 2a7259ba8b..6f6f5f9960 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -682,6 +682,31 @@ void submodule_free(struct repository *r)
 		submodule_cache_clear(r->submodule_cache);
 }
 
+static int config_print_callback(const char *key_, const char *value_, void *cb_data)
+{
+	char *key = cb_data;
+
+	if (!strcmp(key, key_))
+		printf("%s\n", value_);
+
+	return 0;
+}
+
+int print_config_from_gitmodules(const char *key)
+{
+	int ret;
+	char *store_key;
+
+	ret = git_config_parse_key(key, &store_key, NULL);
+	if (ret < 0)
+		return CONFIG_INVALID_KEY;
+
+	config_from_gitmodules(config_print_callback, the_repository, store_key);
+
+	free(store_key);
+	return 0;
+}
+
 struct fetch_config {
 	int *max_children;
 	int *recurse_submodules;
diff --git a/submodule-config.h b/submodule-config.h
index dc7278eea4..6fec3caadd 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -56,6 +56,8 @@ void submodule_free(struct repository *r);
  */
 int check_submodule_name(const char *name);
 
+extern int print_config_from_gitmodules(const char *key);
+
 /*
  * Note: these helper functions exist solely to maintain backward
  * compatibility with 'fetch' and 'update_clone' storing configuration in

-- 
2.18.0
Stefan Beller· Aug 2, 2018, 18:05 UTC · re: Antonio Ospite · lore

Re: [RFC PATCH v2 01/12] submodule: add a print_config_from_gitmodules() helper

On Thu, Aug 2, 2018 at 6:47 AM Antonio Ospite <ao2@ao2.it> wrote:
Show 55 quoted lines
>
> This will be used to print values just like "git config -f .gitmodules"
> would.
>
> Signed-off-by: Antonio Ospite <ao2@ao2.it>
> ---
>  submodule-config.c | 25 +++++++++++++++++++++++++
>  submodule-config.h |  2 ++
>  2 files changed, 27 insertions(+)
>
> diff --git a/submodule-config.c b/submodule-config.c
> index 2a7259ba8b..6f6f5f9960 100644
> --- a/submodule-config.c
> +++ b/submodule-config.c
> @@ -682,6 +682,31 @@ void submodule_free(struct repository *r)
>                 submodule_cache_clear(r->submodule_cache);
>  }
>
> +static int config_print_callback(const char *key_, const char *value_, void *cb_data)
> +{
> +       char *key = cb_data;
> +
> +       if (!strcmp(key, key_))
> +               printf("%s\n", value_);
> +
> +       return 0;
> +}
> +
> +int print_config_from_gitmodules(const char *key)
> +{
> +       int ret;
> +       char *store_key;
> +
> +       ret = git_config_parse_key(key, &store_key, NULL);
> +       if (ret < 0)
> +               return CONFIG_INVALID_KEY;
> +
> +       config_from_gitmodules(config_print_callback, the_repository, store_key);
> +
> +       free(store_key);
> +       return 0;
> +}
> +
>  struct fetch_config {
>         int *max_children;
>         int *recurse_submodules;
> diff --git a/submodule-config.h b/submodule-config.h
> index dc7278eea4..6fec3caadd 100644
> --- a/submodule-config.h
> +++ b/submodule-config.h
> @@ -56,6 +56,8 @@ void submodule_free(struct repository *r);
>   */
>  int check_submodule_name(const char *name);
>
> +extern int print_config_from_gitmodules(const char *key);

The only real pushback for this patch I'd have is lack of documentation in public functions, though this is pretty self explanatory; so I'd be fine for lacking the docs here.

In case a resend is needed, please drop the extern keyword here.

Thanks, Stefan

Antonio Ospite· Aug 9, 2018, 10:17 UTC · re: Stefan Beller · lore

Re: [RFC PATCH v2 01/12] submodule: add a print_config_from_gitmodules() helper

On Thu, 2 Aug 2018 11:05:02 -0700 Stefan Beller <sbeller@google.com> wrote:

> On Thu, Aug 2, 2018 at 6:47 AM Antonio Ospite <ao2@ao2.it> wrote:
> >
[...]
Show 8 quoted lines
> > +extern int print_config_from_gitmodules(const char *key);
> 
> The only real pushback for this patch I'd have is lack of documentation
> in public functions, though this is pretty self explanatory; so I'd be fine
> for lacking the docs here.
> 
> In case a resend is needed, please drop the extern keyword here.
>

I'll drop the extern keyword also for the public function added in patch 02 then.

Thanks,
   Antonio
-- 
Antonio Ospite
https://ao2.it
https://twitter.com/ao2it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
Antonio Ospite· Aug 2, 2018, 13:46 UTC · re: Antonio Ospite · lore

[RFC PATCH v2 10/12] t7416: add new test about HEAD:.gitmodules and not existing .gitmodules

git submodule commands can now access .gitmodules from the current branch even when it's not in the working tree, add some tests for that scenario.

Signed-off-by: Antonio Ospite <ao2@ao2.it>
---

For the test files I used the most used style in other tests, Stefan suggested to avoid subshells and use "git -C" but subshells make the test look cleaner IMHO.

 t/t7416-submodule-sparse-gitmodules.sh | 112 +++++++++++++++++++++++++
 1 file changed, 112 insertions(+)
 create mode 100755 t/t7416-submodule-sparse-gitmodules.sh
Show changes to t/t7416-submodule-sparse-gitmodules.sh +112 −0
diff --git a/t/t7416-submodule-sparse-gitmodules.sh b/t/t7416-submodule-sparse-gitmodules.sh
new file mode 100755
index 0000000000..3c7a53316b
--- /dev/null
+++ b/t/t7416-submodule-sparse-gitmodules.sh
@@ -0,0 +1,112 @@
+#!/bin/sh
+#
+# Copyright (C) 2018  Antonio Ospite <ao2@ao2.it>
+#
+
+test_description=' Test reading/writing .gitmodules is not in the working tree
+
+This test verifies that, when .gitmodules is in the current branch but is not
+in the working tree reading from it still works but writing to it does not.
+
+The test setup uses a sparse checkout, but the same scenario can be set up
+also by committing .gitmodules and then just removing it from the filesystem.
+
+NOTE: "git mv" and "git rm" are still supposed to work even without
+a .gitmodules file, as stated in the t3600-rm.sh and t7001-mv.sh tests.
+'
+
+. ./test-lib.sh
+
+test_expect_success 'sparse checkout setup which hides .gitmodules' '
+	echo file > file &&
+	git add file &&
+	test_tick &&
+	git commit -m upstream &&
+	git clone . super &&
+	git clone super submodule &&
+	git clone super new_submodule &&
+	(cd super &&
+		git submodule add ../submodule
+		test_tick &&
+		git commit -m submodule &&
+		cat >.git/info/sparse-checkout <<\EOF &&
+/*
+!/.gitmodules
+EOF
+		git config core.sparsecheckout true &&
+		git read-tree -m -u HEAD &&
+		test ! -e .gitmodules
+	)
+'
+
+test_expect_success 'reading gitmodules config file when it is not checked out' '
+	(cd super &&
+		echo "../submodule" >expected &&
+		git submodule--helper config submodule.submodule.url >actual &&
+		test_cmp expected actual
+	)
+'
+
+test_expect_success 'not writing gitmodules config file when it is not checked out' '
+	(cd super &&
+		test_must_fail git submodule--helper config submodule.submodule.url newurl
+	)
+'
+
+test_expect_success 'not staging gitmodules config when it is not checked out' '
+	(cd super &&
+		test_must_fail git submodule--helper config --stage
+	)
+'
+
+test_expect_success 'initialising submodule when the gitmodules config is not checked out' '
+	(cd super &&
+		git submodule init
+	)
+'
+
+test_expect_success 'showing submodule summary when the gitmodules config is not checked out' '
+	(cd super &&
+		git submodule summary
+	)
+'
+
+test_expect_success 'updating submodule when the gitmodules config is not checked out' '
+	(cd submodule &&
+		echo file2 >file2 &&
+		git add file2 &&
+		git commit -m "add file2 to submodule"
+	) &&
+	(cd super &&
+		git submodule update
+	)
+'
+
+test_expect_success 'not adding submodules when the gitmodules config is not checked out' '
+	(cd super &&
+		test_must_fail git submodule add ../new_submodule
+	)
+'
+
+# "git add" in the test above fails as expected, however it still leaves the
+# cloned tree in there and adds a config entry to .git/config. This is because
+# no cleanup is done by cmd_add in git-submodule.sh when "git
+# submodule--helper config" fails to add a new config setting.
+#
+# If we added the following commands to the test above:
+#
+#   rm -rf .git/modules/new_submodule &&
+#   git reset HEAD new_submodule &&
+#   rm -rf new_submodule
+#
+# then the repository would be in a clean state and the test below would pass.
+#
+# Maybe cmd_add should do the cleanup from above itself when failing to add
+# a submodule.
+test_expect_failure 'init submodule after adding failed when the gitmodules config is not checked out' '
+	(cd super &&
+		git submodule init
+	)
+'
+
+test_done

-- 
2.18.0
Stefan Beller· Aug 2, 2018, 20:43 UTC · re: Antonio Ospite · lore

Re: [RFC PATCH v2 10/12] t7416: add new test about HEAD:.gitmodules and not existing .gitmodules

On Thu, Aug 2, 2018 at 6:47 AM Antonio Ospite <ao2@ao2.it> wrote:
Show 11 quoted lines
>
> git submodule commands can now access .gitmodules from the current
> branch even when it's not in the working tree, add some tests for that
> scenario.
>
> Signed-off-by: Antonio Ospite <ao2@ao2.it>
> ---
>
> For the test files I used the most used style in other tests, Stefan suggested
> to avoid subshells and use "git -C" but subshells make the test look cleaner
> IMHO.
Oh well. Let's not dive into a style argument, so let me focus on the tests.

IMHO it would be nice if (at least partially) these tests are in the same patch that added the reading from HEAD:.gitmodules (although I like short patches, too).

Show 26 quoted lines
>
>  t/t7416-submodule-sparse-gitmodules.sh | 112 +++++++++++++++++++++++++
>  1 file changed, 112 insertions(+)
>  create mode 100755 t/t7416-submodule-sparse-gitmodules.sh
>
> diff --git a/t/t7416-submodule-sparse-gitmodules.sh b/t/t7416-submodule-sparse-gitmodules.sh
> new file mode 100755
> index 0000000000..3c7a53316b
> --- /dev/null
> +++ b/t/t7416-submodule-sparse-gitmodules.sh
> @@ -0,0 +1,112 @@
> +#!/bin/sh
> +#
> +# Copyright (C) 2018  Antonio Ospite <ao2@ao2.it>
> +#
> +
> +test_description=' Test reading/writing .gitmodules is not in the working tree
> +
> +This test verifies that, when .gitmodules is in the current branch but is not
> +in the working tree reading from it still works but writing to it does not.
> +
> +The test setup uses a sparse checkout, but the same scenario can be set up
> +also by committing .gitmodules and then just removing it from the filesystem.
> +
> +NOTE: "git mv" and "git rm" are still supposed to work even without
> +a .gitmodules file, as stated in the t3600-rm.sh and t7001-mv.sh tests.

"supposed to work" != "tested that it works" ? I am not sure what the NOTE wants to tell me? (Should I review those tests to double check them now? or do we just want to tell future readers of this test there are other tangent tests to this?)

Show 39 quoted lines
> +test_expect_success 'initialising submodule when the gitmodules config is not checked out' '
> +       (cd super &&
> +               git submodule init
> +       )
> +'
> +
> +test_expect_success 'showing submodule summary when the gitmodules config is not checked out' '
> +       (cd super &&
> +               git submodule summary
> +       )
> +'
> +
> +test_expect_success 'updating submodule when the gitmodules config is not checked out' '
> +       (cd submodule &&
> +               echo file2 >file2 &&
> +               git add file2 &&
> +               git commit -m "add file2 to submodule"
> +       ) &&
> +       (cd super &&
> +               git submodule update
> +       )
> +'
> +
> +test_expect_success 'not adding submodules when the gitmodules config is not checked out' '
> +       (cd super &&
> +               test_must_fail git submodule add ../new_submodule
> +       )
> +'
> +
> +# "git add" in the test above fails as expected, however it still leaves the
> +# cloned tree in there and adds a config entry to .git/config. This is because
> +# no cleanup is done by cmd_add in git-submodule.sh when "git
> +# submodule--helper config" fails to add a new config setting.
> +#
> +# If we added the following commands to the test above:
> +#
> +#   rm -rf .git/modules/new_submodule &&
> +#   git reset HEAD new_submodule &&
> +#   rm -rf new_submodule

Alternatively we could check for the existence of .gitmodules before starting all these things?

I think it is okay to not clean up if we check all "regular" or rather expected things such as a non-writable .gitmodules file before actually doing it. (This is similar to 'checkout' that walks the whole tree and checks if the checkout is possible given the dirtyness of the tree, to either abort early or pull through completely. In catastrophic problems such as a full disk we'd still die in the middle of work)

Show 6 quoted lines
> +#
> +# then the repository would be in a clean state and the test below would pass.
> +#
> +# Maybe cmd_add should do the cleanup from above itself when failing to add
> +# a submodule.
> +test_expect_failure 'init submodule after adding failed when the gitmodules config is not checked out' '

So this comment and test is about explaining why we can fail mid way through, which we could not before unless we had the catastrophic event.

I think we should check for a "writable" .gitmodules file at the beginning, which is if (G || (!G && !H)) [using the notation from the cover letter]?

Show 9 quoted lines
> +       (cd super &&
> +               git submodule init
> +       )
> +'
> +
> +test_done
> --
> 2.18.0
>
Antonio Ospite· Aug 9, 2018, 09:14 UTC · re: Stefan Beller · lore

Re: [RFC PATCH v2 10/12] t7416: add new test about HEAD:.gitmodules and not existing .gitmodules

On Thu, 2 Aug 2018 13:43:05 -0700 Stefan Beller <sbeller@google.com> wrote:

Show 8 quoted lines
> On Thu, Aug 2, 2018 at 6:47 AM Antonio Ospite <ao2@ao2.it> wrote:
> >
> > git submodule commands can now access .gitmodules from the current
> > branch even when it's not in the working tree, add some tests for that
> > scenario.
> >
> > Signed-off-by: Antonio Ospite <ao2@ao2.it>
> > ---
[...]
> > +NOTE: "git mv" and "git rm" are still supposed to work even without
> > +a .gitmodules file, as stated in the t3600-rm.sh and t7001-mv.sh tests.
> 
> "supposed to work" != "tested that it works" ?

"git mv submod new_submod" and "git rm submod" are actually expected to work without the .gitmodules file, and there are tests about that in t3600-rm.sh and t7001-mv.sh:

t3600-rm.sh:
  'rm does not complain when no .gitmodules file is found'
t7001-mv.sh:
  'git mv moves a submodule with a .git directory and no .gitmodules'  
  'mv does not complain when no .gitmodules file is found'
> I am not sure what the NOTE wants to tell me? (Should I review those
> tests to double check them now? or do we just want to tell future readers
> of this test there are other tangent tests to this?)
>

Admittedly the NOTE is not useful without any context: during the development of "submodule--helper config --stage" I initially assumed that "git mv" and "git rm" should fail if .gitmodules was not available, because these commands modify .gitmodules and I added code for that in stage_updated_gitmodules().

But then later I found out that my assumption was wrong and that git has tests to verify that these operations on submodules succeed even when .gitmodules does not exist, which was a little of a surprise to me.

So I removed all my code that was conflicting with git assumptions, and added the NOTE. However I guess that was primarily a note to myself, and it should have not slipped in the public patches.

I think I will remove the note, it can be confusing and does not really add anything, and even less considering that "submodule--helper config --stage" is going to be dropped.

[...]
Show 20 quoted lines
> > +test_expect_success 'not adding submodules when the gitmodules config is not checked out' '
> > +       (cd super &&
> > +               test_must_fail git submodule add ../new_submodule
> > +       )
> > +'
> > +
> > +# "git add" in the test above fails as expected, however it still leaves the
> > +# cloned tree in there and adds a config entry to .git/config. This is because
> > +# no cleanup is done by cmd_add in git-submodule.sh when "git
> > +# submodule--helper config" fails to add a new config setting.
> > +#
> > +# If we added the following commands to the test above:
> > +#
> > +#   rm -rf .git/modules/new_submodule &&
> > +#   git reset HEAD new_submodule &&
> > +#   rm -rf new_submodule
> 
> Alternatively we could check for the existence of .gitmodules
> before starting all these things?
>
You mean in cmd_add(), before doing anything?

The following would anticipates the same check which makes "git submodule add" fail:

Show changes to git-submodule.sh +5 −0
diff --git a/git-submodule.sh b/git-submodule.sh
index ff258e2e8c..b261175143 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -159,6 +159,11 @@ cmd_add()
                shift
        done

+       if test ! -e .gitmodules && git cat-file -e HEAD:.gitmodules
+       then
+                die "$(eval_gettext "please make sure that the .gitmodules file in the current branch is checked out")"
+       fi
+
        if test -n "$reference_path"
        then
                is_absolute_path "$reference_path" ||

This refers to .gitmodules explicitly but we said that we do not care
about that for now, if opaque access was ever needed in the future,
something like "submodule--helper config --is-writeable" could be added.

> I think it is okay to not clean up if we check all "regular" or rather expected
> things such as a non-writable .gitmodules file before actually doing it.
> (This is similar to 'checkout' that walks the whole tree and checks if the
> checkout is possible given the dirtyness of the tree, to either abort early
> or pull through completely. In catastrophic problems such as a full disk
> we'd still die in the middle of work)
> 
> > +#
> > +# then the repository would be in a clean state and the test below would pass.
> > +#
> > +# Maybe cmd_add should do the cleanup from above itself when failing to add
> > +# a submodule.
> > +test_expect_failure 'init submodule after adding failed when the gitmodules config is not checked out' '
> 
> So this comment and test is about explaining why we can fail mid way through,
> which we could not before unless we had the catastrophic event.
> 
> I think we should check for a "writable" .gitmodules file at the beginning,
> which is if (G || (!G && !H)) [using the notation from the cover letter]?
> 
> > +       (cd super &&
> > +               git submodule init

With the change from above this last test passes.

BTW the check I am using here and in the code of submodule--helper,
corresponds indeed to the boolean expression you mentioned, but
simplified and negated.

Thanks,
   Antonio

-- 
Antonio Ospite
https://ao2.it
https://twitter.com/ao2it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
Antonio Ospite· Aug 2, 2018, 13:46 UTC · re: Antonio Ospite · lore

[RFC PATCH v2 07/12] submodule: use 'submodule--helper config --stage' to stage .gitmodules

Use 'git submodule--helper config --stage' in git-submodule.sh to remove the last place where the .gitmodules file is mentioned explicitly by its file path.

Signed-off-by: Antonio Ospite <ao2@ao2.it>
---
 git-submodule.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to git-submodule.sh +1 −1
diff --git a/git-submodule.sh b/git-submodule.sh
index ff258e2e8c..f20c37d92d 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -289,7 +289,7 @@ or you are unsure what this means choose another name with the '--name' option."
 	then
 		git submodule--helper config submodule."$sm_name".branch "$branch"
 	fi &&
-	git add --force .gitmodules ||
+	git submodule--helper config --stage ||
 	die "$(eval_gettext "Failed to register submodule '\$sm_path'")"
 
 	# NEEDSWORK: In a multi-working-tree world, this needs to be

-- 
2.18.0
Antonio Ospite· Aug 2, 2018, 13:46 UTC · re: Antonio Ospite · lore

[RFC PATCH v2 08/12] t7506: cleanup .gitmodules properly before setting up new scenario

In t/t7506-status-submodule.sh at some point a new scenario is set up to test different things, in particular new submodules are added which are meant to completely replace the previous ones.

However before calling the "git submodule add" commands for the new layout, the .gitmodules file is removed only from the working tree still leaving the previous content in current branch.

This can break if, in the future, "git submodule add" starts differentiating between the following two cases:

  - .gitmodules is not in the working tree but it is in the current
    branch (it may not be safe to add new submodules in this case);
  - .gitmodules is neither in the working tree nor anywhere in the
    current branch (it is safe to add new submodules).

Since the test means to get rid of .gitmodules anyways, let's completely remove it from the current branch, to actually start afresh in the new scenario.

This is more future-proof and does not break current tests.
Signed-off-by: Antonio Ospite <ao2@ao2.it>
---
 t/t7506-status-submodule.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Show changes to t/t7506-status-submodule.sh +2 −1
diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh
index b4b74dbe29..af91ba92ff 100755
--- a/t/t7506-status-submodule.sh
+++ b/t/t7506-status-submodule.sh
@@ -325,7 +325,8 @@ test_expect_success 'setup superproject with untracked file in nested submodule'
 	(
 		cd super &&
 		git clean -dfx &&
-		rm .gitmodules &&
+		git rm .gitmodules &&
+		git commit -m "remove .gitmodules" &&
 		git submodule add -f ./sub1 &&
 		git submodule add -f ./sub2 &&
 		git submodule add -f ./sub1 sub3 &&

-- 
2.18.0
Stefan Beller· Aug 2, 2018, 19:11 UTC · re: Antonio Ospite · lore

Re: [RFC PATCH v2 08/12] t7506: cleanup .gitmodules properly before setting up new scenario

On Thu, Aug 2, 2018 at 6:47 AM Antonio Ospite <ao2@ao2.it> wrote:
Show 23 quoted lines
>
> In t/t7506-status-submodule.sh at some point a new scenario is set up to
> test different things, in particular new submodules are added which are
> meant to completely replace the previous ones.
>
> However before calling the "git submodule add" commands for the new
> layout, the .gitmodules file is removed only from the working tree still
> leaving the previous content in current branch.
>
> This can break if, in the future, "git submodule add" starts
> differentiating between the following two cases:
>
>   - .gitmodules is not in the working tree but it is in the current
>     branch (it may not be safe to add new submodules in this case);
>
>   - .gitmodules is neither in the working tree nor anywhere in the
>     current branch (it is safe to add new submodules).
>
> Since the test means to get rid of .gitmodules anyways, let's completely
> remove it from the current branch, to actually start afresh in the new
> scenario.
>
> This is more future-proof and does not break current tests.
Makes sense.

Thanks, Stefan

Show 23 quoted lines
>
> Signed-off-by: Antonio Ospite <ao2@ao2.it>
> ---
>  t/t7506-status-submodule.sh | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh
> index b4b74dbe29..af91ba92ff 100755
> --- a/t/t7506-status-submodule.sh
> +++ b/t/t7506-status-submodule.sh
> @@ -325,7 +325,8 @@ test_expect_success 'setup superproject with untracked file in nested submodule'
>         (
>                 cd super &&
>                 git clean -dfx &&
> -               rm .gitmodules &&
> +               git rm .gitmodules &&
> +               git commit -m "remove .gitmodules" &&
>                 git submodule add -f ./sub1 &&
>                 git submodule add -f ./sub2 &&
>                 git submodule add -f ./sub1 sub3 &&
> --
> 2.18.0
>
Antonio Ospite· Aug 2, 2018, 13:46 UTC · re: Antonio Ospite · lore

[RFC PATCH v2 12/12] submodule: remove the .gitmodules file when it is empty

In particular this makes it possible to really clean things up when removing the last submodule with "git rm".

The rationale is that if git creates .gitmodules when adding the first submodule it should also remove it when removing the last submodule.

Signed-off-by: Antonio Ospite <ao2@ao2.it>
---
 submodule.c                | 10 ++++++++--
 t/t3600-rm.sh              | 32 ++++++++++++++++----------------
 t/t7400-submodule-basic.sh | 11 +++++++++++
 3 files changed, 35 insertions(+), 18 deletions(-)
Show changes to 3 files +35 −18

submodule.c, t/t3600-rm.sh, t/t7400-submodule-basic.sh

diff --git a/submodule.c b/submodule.c
index 2af09068d7..8cfa82231d 100644
--- a/submodule.c
+++ b/submodule.c
@@ -145,8 +145,14 @@ int remove_path_from_gitmodules(const char *path)
 
 void stage_updated_gitmodules(struct index_state *istate)
 {
-	if (add_file_to_index(istate, GITMODULES_FILE, 0))
-		die(_("staging updated .gitmodules failed"));
+	if (is_empty_file(GITMODULES_FILE)) {
+		if (remove_path(GITMODULES_FILE) < 0)
+			die(_("removing .empty gitmodules failed"));
+		remove_file_from_index(&the_index, GITMODULES_FILE);
+	} else {
+		if (add_file_to_index(istate, GITMODULES_FILE, 0))
+			die(_("staging updated .gitmodules failed"));
+	}
 }
 
 /* TODO: remove this function, use repo_submodule_init instead. */
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index b8fbdefcdc..bac2054f51 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -296,7 +296,7 @@ test_expect_success 'rm removes empty submodules from work tree' '
 	git rm submod &&
 	test ! -e submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect actual &&
+	test_cmp expect.both_deleted actual &&
 	test_must_fail git config -f .gitmodules submodule.sub.url &&
 	test_must_fail git config -f .gitmodules submodule.sub.path
 '
@@ -307,7 +307,7 @@ test_expect_success 'rm removes removed submodule from index and .gitmodules' '
 	rm -rf submod &&
 	git rm submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect actual &&
+	test_cmp expect.both_deleted actual &&
 	test_must_fail git config -f .gitmodules submodule.sub.url &&
 	test_must_fail git config -f .gitmodules submodule.sub.path
 '
@@ -318,7 +318,7 @@ test_expect_success 'rm removes work tree of unmodified submodules' '
 	git rm submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect actual &&
+	test_cmp expect.both_deleted actual &&
 	test_must_fail git config -f .gitmodules submodule.sub.url &&
 	test_must_fail git config -f .gitmodules submodule.sub.path
 '
@@ -329,7 +329,7 @@ test_expect_success 'rm removes a submodule with a trailing /' '
 	git rm submod/ &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect actual
+	test_cmp expect.both_deleted actual
 '
 
 test_expect_success 'rm fails when given a file with a trailing /' '
@@ -352,7 +352,7 @@ test_expect_success 'rm of a populated submodule with different HEAD fails unles
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect actual &&
+	test_cmp expect.both_deleted actual &&
 	test_must_fail git config -f .gitmodules submodule.sub.url &&
 	test_must_fail git config -f .gitmodules submodule.sub.path
 '
@@ -433,7 +433,7 @@ test_expect_success 'rm of a populated submodule with modifications fails unless
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect actual
+	test_cmp expect.both_deleted actual
 '
 
 test_expect_success 'rm of a populated submodule with untracked files fails unless forced' '
@@ -448,7 +448,7 @@ test_expect_success 'rm of a populated submodule with untracked files fails unle
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect actual
+	test_cmp expect.both_deleted actual
 '
 
 test_expect_success 'setup submodule conflict' '
@@ -485,7 +485,7 @@ test_expect_success 'rm removes work tree of unmodified conflicted submodule' '
 	git rm submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect actual
+	test_cmp expect.both_deleted actual
 '
 
 test_expect_success 'rm of a conflicted populated submodule with different HEAD fails unless forced' '
@@ -502,7 +502,7 @@ test_expect_success 'rm of a conflicted populated submodule with different HEAD
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect actual &&
+	test_cmp expect.both_deleted actual &&
 	test_must_fail git config -f .gitmodules submodule.sub.url &&
 	test_must_fail git config -f .gitmodules submodule.sub.path
 '
@@ -521,7 +521,7 @@ test_expect_success 'rm of a conflicted populated submodule with modifications f
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect actual &&
+	test_cmp expect.both_deleted actual &&
 	test_must_fail git config -f .gitmodules submodule.sub.url &&
 	test_must_fail git config -f .gitmodules submodule.sub.path
 '
@@ -540,7 +540,7 @@ test_expect_success 'rm of a conflicted populated submodule with untracked files
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect actual
+	test_cmp expect.both_deleted actual
 '
 
 test_expect_success 'rm of a conflicted populated submodule with a .git directory fails even when forced' '
@@ -574,7 +574,7 @@ test_expect_success 'rm of a conflicted unpopulated submodule succeeds' '
 	git rm submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect actual
+	test_cmp expect.both_deleted actual
 '
 
 test_expect_success 'rm of a populated submodule with a .git directory migrates git dir' '
@@ -618,7 +618,7 @@ test_expect_success 'rm recursively removes work tree of unmodified submodules'
 	git rm submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect actual
+	test_cmp expect.both_deleted actual
 '
 
 test_expect_success 'rm of a populated nested submodule with different nested HEAD fails unless forced' '
@@ -633,7 +633,7 @@ test_expect_success 'rm of a populated nested submodule with different nested HE
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect actual
+	test_cmp expect.both_deleted actual
 '
 
 test_expect_success 'rm of a populated nested submodule with nested modifications fails unless forced' '
@@ -648,7 +648,7 @@ test_expect_success 'rm of a populated nested submodule with nested modification
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect actual
+	test_cmp expect.both_deleted actual
 '
 
 test_expect_success 'rm of a populated nested submodule with nested untracked files fails unless forced' '
@@ -663,7 +663,7 @@ test_expect_success 'rm of a populated nested submodule with nested untracked fi
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect actual
+	test_cmp expect.both_deleted actual
 '
 
 test_expect_success "rm absorbs submodule's nested .git directory" '
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 48fd14fae6..2bb42a4c8f 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -99,6 +99,17 @@ inspect() {
 	)
 }
 
+test_expect_success 'removal of last submodule also removes empty .gitmodules' '
+	(
+		cd addtest &&
+		test ! -d .git/modules &&
+		git submodule add -q "$submodurl" first_submod &&
+		test -e .gitmodules &&
+		git rm -f first_submod &&
+		test ! -e .gitmodules
+	)
+'
+
 test_expect_success 'submodule add' '
 	echo "refs/heads/master" >expect &&
 	>empty &&

-- 
2.18.0
Stefan Beller· Aug 2, 2018, 21:15 UTC · re: Antonio Ospite · lore

Re: [RFC PATCH v2 12/12] submodule: remove the .gitmodules file when it is empty

On Thu, Aug 2, 2018 at 6:47 AM Antonio Ospite <ao2@ao2.it> wrote:
>
> In particular this makes it possible to really clean things up when
> removing the last submodule with "git rm".

This sentence is a continuation of the subject line, and I had to reread it to follow along.

>
> The rationale is that if git creates .gitmodules when adding the first
> submodule it should also remove it when removing the last submodule.

I agree with this sentiment. It seems slightly odd to me to have this tied in the same patch series that changes .gitmodules reading behavior as I could think of this feature as orthogonal to what this series achieved up to patch 10.

> -       test_cmp expect actual &&
> +       test_cmp expect.both_deleted actual &&

This seems to be the re-occuring pattern in t3600, and given that we have

  cat >expect <<EOF
  M  .gitmodules
  D  submod
  EOF
  cat >expect.both_deleted<<EOF
  D  .gitmodules
  D  submod
  EOF

with no other writing of expect in the range, this seems to be correct. Maybe worth testing that we do not delete a .gitmodules file if we have more than one submodule? (But I would expect this to be covered implicitly somewhere in the test suite. If so that would be worth mentioning in the commit message instead of writing a test -- just looking quickly we do have " git rm --cached submodule2" in t7406 which might be sufficient?)

Show 13 quoted lines
>  test_expect_success "rm absorbs submodule's nested .git directory" '
> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> index 48fd14fae6..2bb42a4c8f 100755
> --- a/t/t7400-submodule-basic.sh
> +++ b/t/t7400-submodule-basic.sh
> @@ -99,6 +99,17 @@ inspect() {
>         )
>  }
>
> +test_expect_success 'removal of last submodule also removes empty .gitmodules' '
> +       (
> +               cd addtest &&
> +               test ! -d .git/modules &&
test_path_is_missing ?
> +               git submodule add -q "$submodurl" first_submod &&
> +               test -e .gitmodules &&
test_path_is_file
> +               git rm -f first_submod &&
Do we need to force it here?
> +               test ! -e .gitmodules
test_path_is_missing

Thanks for this series! Overall it was a pleasant read, though I had some comments.

Thanks, Stefan

Antonio Ospite· Aug 3, 2018, 08:57 UTC · re: Stefan Beller · lore

Re: [RFC PATCH v2 12/12] submodule: remove the .gitmodules file when it is empty

On Thu, 2 Aug 2018 14:15:54 -0700 Stefan Beller <sbeller@google.com> wrote:

Show 8 quoted lines
> On Thu, Aug 2, 2018 at 6:47 AM Antonio Ospite <ao2@ao2.it> wrote:
> >
> > In particular this makes it possible to really clean things up when
> > removing the last submodule with "git rm".
> 
> This sentence is a continuation of the subject line, and I had to reread
> it to follow along.
>
OK, I'll fix that.
Show 9 quoted lines
> >
> > The rationale is that if git creates .gitmodules when adding the first
> > submodule it should also remove it when removing the last submodule.
> 
> I agree with this sentiment. It seems slightly odd to me to have this tied
> in the same patch series that changes .gitmodules reading behavior
> as I could think of this feature as orthogonal to what this series achieved
> up to patch 10.
>

I will send this as a separate series, I briefly mentioned this possibility in the cover letter.

Show 22 quoted lines
> > -       test_cmp expect actual &&
> > +       test_cmp expect.both_deleted actual &&
> 
> This seems to be the re-occuring pattern in t3600, and given that
> we have
> 
>   cat >expect <<EOF
>   M  .gitmodules
>   D  submod
>   EOF
>   cat >expect.both_deleted<<EOF
>   D  .gitmodules
>   D  submod
>   EOF
> 
> with no other writing of expect in the range, this seems to be correct.
> Maybe worth testing that we do not delete a .gitmodules file if we have
> more than one submodule? (But I would expect this to be covered implicitly
> somewhere in the test suite. If so that would be worth mentioning in the
> commit message instead of writing a test -- just looking quickly we
> do have " git rm --cached submodule2" in t7406 which might be sufficient?)
>

I think I will remove the new test in t7400 as the changes to t3600 should cover that case already.

I will check about the case of more than one submodule and I'll add a comment to the commit message.

[...]
> Thanks for this series!
> Overall it was a pleasant read, though I had some comments.
> 
Thanks,
   Antonio
-- 
Antonio Ospite
https://ao2.it
https://twitter.com/ao2it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
Antonio Ospite· Aug 2, 2018, 13:46 UTC · re: Antonio Ospite · lore

[RFC PATCH v2 11/12] dir: move is_empty_file() from builtin/am.c to dir.c and make it public

The is_empty_file() function can be generally useful, move it to dir.c and make it public.

Signed-off-by: Antonio Ospite <ao2@ao2.it>
---
 builtin/am.c | 15 ---------------
 dir.c        | 16 ++++++++++++++++
 dir.h        |  1 +
 3 files changed, 17 insertions(+), 15 deletions(-)
Show changes to 3 files +17 −15

builtin/am.c, dir.c, dir.h

diff --git a/builtin/am.c b/builtin/am.c
index 6273ea5195..0c04312a50 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -33,21 +33,6 @@
 #include "string-list.h"
 #include "packfile.h"
 
-/**
- * Returns 1 if the file is empty or does not exist, 0 otherwise.
- */
-static int is_empty_file(const char *filename)
-{
-	struct stat st;
-
-	if (stat(filename, &st) < 0) {
-		if (errno == ENOENT)
-			return 1;
-		die_errno(_("could not stat %s"), filename);
-	}
-
-	return !st.st_size;
-}
 
 /**
  * Returns the length of the first line of msg.
diff --git a/dir.c b/dir.c
index 21e6f2520a..c1f7b90256 100644
--- a/dir.c
+++ b/dir.c
@@ -2412,6 +2412,22 @@ int is_empty_dir(const char *path)
 	return ret;
 }
 
+/**
+ * Returns 1 if the file is empty or does not exist, 0 otherwise.
+ */
+int is_empty_file(const char *filename)
+{
+	struct stat st;
+
+	if (stat(filename, &st) < 0) {
+		if (errno == ENOENT)
+			return 1;
+		die_errno(_("could not stat %s"), filename);
+	}
+
+	return !st.st_size;
+}
+
 static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up)
 {
 	DIR *dir;
diff --git a/dir.h b/dir.h
index f5fdedbab2..e45aa3f459 100644
--- a/dir.h
+++ b/dir.h
@@ -281,6 +281,7 @@ static inline int is_dot_or_dotdot(const char *name)
 }
 
 extern int is_empty_dir(const char *dir);
+extern int is_empty_file(const char *filename);
 
 extern void setup_standard_excludes(struct dir_struct *dir);
 

-- 
2.18.0
Stefan Beller· Aug 2, 2018, 20:50 UTC · re: Antonio Ospite · lore

Re: [RFC PATCH v2 11/12] dir: move is_empty_file() from builtin/am.c to dir.c and make it public

On Thu, Aug 2, 2018 at 6:47 AM Antonio Ospite <ao2@ao2.it> wrote:
Show 5 quoted lines
>
> The is_empty_file() function can be generally useful, move it to dir.c
> and make it public.
>
> Signed-off-by: Antonio Ospite <ao2@ao2.it>
Makes sense,

Thanks, Stefan

Show 8 quoted lines
> +++ b/dir.c
> @@ -2412,6 +2412,22 @@ int is_empty_dir(const char *path)
>         return ret;
>  }
>
> +/**
> + * Returns 1 if the file is empty or does not exist, 0 otherwise.
> + */

Please move the comment to the header instead. /* possibly as a oneliner ? */

Antonio Ospite· Aug 3, 2018, 08:49 UTC · re: Stefan Beller · lore

Re: [RFC PATCH v2 11/12] dir: move is_empty_file() from builtin/am.c to dir.c and make it public

On Thu, 2 Aug 2018 13:50:55 -0700 Stefan Beller <sbeller@google.com> wrote:

Show 23 quoted lines
> On Thu, Aug 2, 2018 at 6:47 AM Antonio Ospite <ao2@ao2.it> wrote:
> >
> > The is_empty_file() function can be generally useful, move it to dir.c
> > and make it public.
> >
> > Signed-off-by: Antonio Ospite <ao2@ao2.it>
> 
> Makes sense,
> 
> Thanks,
> Stefan
> 
> > +++ b/dir.c
> > @@ -2412,6 +2412,22 @@ int is_empty_dir(const char *path)
> >         return ret;
> >  }
> >
> > +/**
> > + * Returns 1 if the file is empty or does not exist, 0 otherwise.
> > + */
> 
> Please move the comment to the header instead.
> /* possibly as a oneliner ? */
Will do.
Thanks,
   Antonio
-- 
Antonio Ospite
https://ao2.it
https://twitter.com/ao2it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
Antonio Ospite· Aug 2, 2018, 13:46 UTC · re: Antonio Ospite · lore

[RFC PATCH v2 03/12] t7411: be nicer to future tests and really clean things up

Tests 5 and 8 in t/t7411-submodule-config.sh add two commits with invalid lines in .gitmodules but then only the second commit is removed.

This may affect future subsequent tests if they assume that the .gitmodules file has no errors.

Since those commits are not needed anymore remove both of them.

The modified line is in the last test of the file, so this does not change the current behavior, it only affects future tests.

Signed-off-by: Antonio Ospite <ao2@ao2.it>
---

Note that test_when_finished is not used here, both to keep the current style and also because it does not work in sub-shells.

 t/t7411-submodule-config.sh | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
Show changes to t/t7411-submodule-config.sh +3 −1
diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
index 0bde5850ac..248da0bc4f 100755
--- a/t/t7411-submodule-config.sh
+++ b/t/t7411-submodule-config.sh
@@ -135,7 +135,9 @@ test_expect_success 'error in history in fetchrecursesubmodule lets continue' '
 			HEAD submodule \
 				>actual &&
 		test_cmp expect_error actual  &&
-		git reset --hard HEAD^
+		# Remove both the commits which add errors to .gitmodules,
+		# the one from this test and the one from a previous test.
+		git reset --hard HEAD~2
 	)
 '
 

-- 
2.18.0
SZEDER Gábor· Aug 2, 2018, 16:40 UTC · re: Antonio Ospite · lore

Re: [RFC PATCH v2 03/12] t7411: be nicer to future tests and really clean things up

Show 16 quoted lines
> Tests 5 and 8 in t/t7411-submodule-config.sh add two commits with
> invalid lines in .gitmodules but then only the second commit is removed.
> 
> This may affect future subsequent tests if they assume that the
> .gitmodules file has no errors.
> 
> Since those commits are not needed anymore remove both of them.
> 
> The modified line is in the last test of the file, so this does not
> change the current behavior, it only affects future tests.
> 
> Signed-off-by: Antonio Ospite <ao2@ao2.it>
> ---
> 
> Note that test_when_finished is not used here, both to keep the current style
> and also because it does not work in sub-shells.
That's true, but I think that this:
  test_when_finished git -C super reset --hard HEAD~2
at the very beginning of the test should work.
Show 22 quoted lines
>  t/t7411-submodule-config.sh | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
> index 0bde5850ac..248da0bc4f 100755
> --- a/t/t7411-submodule-config.sh
> +++ b/t/t7411-submodule-config.sh
> @@ -135,7 +135,9 @@ test_expect_success 'error in history in fetchrecursesubmodule lets continue' '
>  			HEAD submodule \
>  				>actual &&
>  		test_cmp expect_error actual  &&
> -		git reset --hard HEAD^
> +		# Remove both the commits which add errors to .gitmodules,
> +		# the one from this test and the one from a previous test.
> +		git reset --hard HEAD~2
>  	)
>  '
>  
> -- 
> 2.18.0
> 
>
Stefan Beller· Aug 2, 2018, 18:15 UTC · re: SZEDER Gábor · lore

Re: [RFC PATCH v2 03/12] t7411: be nicer to future tests and really clean things up

On Thu, Aug 2, 2018 at 9:41 AM SZEDER Gábor <szeder.dev@gmail.com> wrote:
Show 23 quoted lines
>
> > Tests 5 and 8 in t/t7411-submodule-config.sh add two commits with
> > invalid lines in .gitmodules but then only the second commit is removed.
> >
> > This may affect future subsequent tests if they assume that the
> > .gitmodules file has no errors.
> >
> > Since those commits are not needed anymore remove both of them.
> >
> > The modified line is in the last test of the file, so this does not
> > change the current behavior, it only affects future tests.
> >
> > Signed-off-by: Antonio Ospite <ao2@ao2.it>
> > ---
> >
> > Note that test_when_finished is not used here, both to keep the current style
> > and also because it does not work in sub-shells.
>
> That's true, but I think that this:
>
>   test_when_finished git -C super reset --hard HEAD~2
>
> at the very beginning of the test should work.

Yeah that is a better way to do it. Even better would be to have 2 of these for both tests 5 and 8, such that each of them could be skipped individually and any following tests still work fine.

Show 15 quoted lines
> >  t/t7411-submodule-config.sh | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
> > index 0bde5850ac..248da0bc4f 100755
> > --- a/t/t7411-submodule-config.sh
> > +++ b/t/t7411-submodule-config.sh
> > @@ -135,7 +135,9 @@ test_expect_success 'error in history in fetchrecursesubmodule lets continue' '
> >                       HEAD submodule \
> >                               >actual &&
> >               test_cmp expect_error actual  &&
> > -             git reset --hard HEAD^
> > +             # Remove both the commits which add errors to .gitmodules,
> > +             # the one from this test and the one from a previous test.
> > +             git reset --hard HEAD~2

I am a bit hesitant to removing the commits though, as it is expected to have potentially broken history and submodules still working.

The config --unset already fixes the gitmodules file, so I think we can rather do

    git commit -a -m 'now the .gitmodules file is fixed at HEAD \
        but has a messy history'

But as I have only read up to here, not knowing what the future tests will bring this is all speculation at this point.

Antonio Ospite· Aug 9, 2018, 13:59 UTC · re: Stefan Beller · lore

Re: [RFC PATCH v2 03/12] t7411: be nicer to future tests and really clean things up

On Thu, 2 Aug 2018 11:15:03 -0700 Stefan Beller <sbeller@google.com> wrote:

> On Thu, Aug 2, 2018 at 9:41 AM SZEDER Gábor <szeder.dev@gmail.com> wrote:
> >
[...]
Show 15 quoted lines
> > >
> > > Note that test_when_finished is not used here, both to keep the current style
> > > and also because it does not work in sub-shells.
> >
> > That's true, but I think that this:
> >
> >   test_when_finished git -C super reset --hard HEAD~2
> >
> > at the very beginning of the test should work.
> 
> Yeah that is a better way to do it.
> Even better would be to have 2 of these for both tests 5 and 8,
> such that each of them could be skipped individually and any following
> tests still work fine.
>
Test 6 also relies on the error introduced in test 5.

So the options would be either to remove one commit at the time in test 6 and 8 (with a comment in test 6 to note that the commit is from the previous test), or to remove both the commits in test 8. I am going to go with the former, using test_when_finished.

Show 19 quoted lines
> > >  t/t7411-submodule-config.sh | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
> > > index 0bde5850ac..248da0bc4f 100755
> > > --- a/t/t7411-submodule-config.sh
> > > +++ b/t/t7411-submodule-config.sh
> > > @@ -135,7 +135,9 @@ test_expect_success 'error in history in fetchrecursesubmodule lets continue' '
> > >                       HEAD submodule \
> > >                               >actual &&
> > >               test_cmp expect_error actual  &&
> > > -             git reset --hard HEAD^
> > > +             # Remove both the commits which add errors to .gitmodules,
> > > +             # the one from this test and the one from a previous test.
> > > +             git reset --hard HEAD~2
> 
> I am a bit hesitant to removing the commits though, as it is expected to have
> potentially broken history and submodules still working.
>

The commits which are removed only affected .gitmoudles, no "submodule init" nor "submoudle update" is ever called after they are added, so I don't know what problems there could be.

Would a revert be any different?
Show 8 quoted lines
> The config --unset already fixes the gitmodules file,
> so I think we can rather do
> 
>     git commit -a -m 'now the .gitmodules file is fixed at HEAD \
>         but has a messy history'
> 
> But as I have only read up to here, not knowing what the future tests will
> bring this is all speculation at this point.

IIUC the "config --unset" is used to cause the error, not to fix it, I am not sure I understand this point.

Thanks,
   Antonio
-- 
Antonio Ospite
https://ao2.it
https://twitter.com/ao2it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
Junio C Hamano· Aug 2, 2018, 18:44 UTC · re: SZEDER Gábor · lore

Re: [RFC PATCH v2 03/12] t7411: be nicer to future tests and really clean things up

SZEDER Gábor <szeder.dev@gmail.com> writes:
Show 22 quoted lines
>> Tests 5 and 8 in t/t7411-submodule-config.sh add two commits with
>> invalid lines in .gitmodules but then only the second commit is removed.
>> 
>> This may affect future subsequent tests if they assume that the
>> .gitmodules file has no errors.
>> 
>> Since those commits are not needed anymore remove both of them.
>> 
>> The modified line is in the last test of the file, so this does not
>> change the current behavior, it only affects future tests.
>> 
>> Signed-off-by: Antonio Ospite <ao2@ao2.it>
>> ---
>> 
>> Note that test_when_finished is not used here, both to keep the current style
>> and also because it does not work in sub-shells.
>
> That's true, but I think that this:
>
>   test_when_finished git -C super reset --hard HEAD~2
>
> at the very beginning of the test should work.

Assuming that the operations to create these two extra commits always succeed, yes, that would be a more robust and preferrable option.

I don't know if that assumption hold true offhand, though. Don't we have a more stable point to anchor that going-back-to commit to?

Antonio Ospite· Aug 2, 2018, 13:46 UTC · re: Antonio Ospite · lore

[RFC PATCH v2 09/12] submodule: support reading .gitmodules even when it's not checked out

When the .gitmodules file is not available in the working tree, try using HEAD:.gitmodules from the current branch. This covers the case when the file is part of the repository but for some reason it is not checked out, for example because of a sparse checkout.

This makes it possible to use at least the 'git submodule' commands which *read* the gitmodules configuration file without fully populating the working tree.

Writing to .gitmodules will still require that the file is checked out, so check for that before calling config_set_in_gitmodules_file_gently.

Signed-off-by: Antonio Ospite <ao2@ao2.it>
---
 builtin/submodule--helper.c | 17 ++++++++++++++++-
 cache.h                     |  1 +
 submodule-config.c          | 16 ++++++++++++++--
 3 files changed, 31 insertions(+), 3 deletions(-)
Show changes to 3 files +31 −3

builtin/submodule--helper.c, cache.h, submodule-config.c

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index c388f4ee6f..616d5de0a9 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2089,8 +2089,23 @@ static int module_config(int argc, const char **argv, const char *prefix)
 		return print_config_from_gitmodules(argv[1]);
 
 	/* Equivalent to ACTION_SET in builtin/config.c */
-	if (argc == 3)
+	if (argc == 3) {
+		struct object_id oid;
+
+		/*
+		 * If the .gitmodules file is not in the working tree but it
+		 * is in the current branch, stop, as writing new values and
+		 * staging them would blindly overwrite ALL the old content.
+		 *
+		 * This still makes it possible to create a brand new
+		 * .gitmodules when neither GITMODULES_FILE nor
+		 * GITMODULES_HEAD exist.
+		 */
+		if (!file_exists(GITMODULES_FILE) && get_oid(GITMODULES_HEAD, &oid) >= 0)
+			die(_("please make sure that the .gitmodules file in the current branch is checked out"));
+
 		return config_set_in_gitmodules_file_gently(argv[1], argv[2]);
+	}
 
 	return 0;
 }
diff --git a/cache.h b/cache.h
index 8b447652a7..8f75cafbb6 100644
--- a/cache.h
+++ b/cache.h
@@ -424,6 +424,7 @@ static inline enum object_type object_type(unsigned int mode)
 #define INFOATTRIBUTES_FILE "info/attributes"
 #define ATTRIBUTE_MACRO_PREFIX "[attr]"
 #define GITMODULES_FILE ".gitmodules"
+#define GITMODULES_HEAD "HEAD:.gitmodules"
 #define GIT_NOTES_REF_ENVIRONMENT "GIT_NOTES_REF"
 #define GIT_NOTES_DEFAULT_REF "refs/notes/commits"
 #define GIT_NOTES_DISPLAY_REF_ENVIRONMENT "GIT_NOTES_DISPLAY_REF"
diff --git a/submodule-config.c b/submodule-config.c
index 702d40dd6b..cf08264220 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -1,4 +1,5 @@
 #include "cache.h"
+#include "dir.h"
 #include "repository.h"
 #include "config.h"
 #include "submodule-config.h"
@@ -603,8 +604,19 @@ static void submodule_cache_check_init(struct repository *repo)
 static void config_from_gitmodules(config_fn_t fn, struct repository *repo, void *data)
 {
 	if (repo->worktree) {
-		char *file = repo_worktree_path(repo, GITMODULES_FILE);
-		git_config_from_file(fn, file, data);
+		struct git_config_source config_source = { 0 };
+		const struct config_options opts = { 0 };
+		struct object_id oid;
+		char *file;
+
+		file = repo_worktree_path(repo, GITMODULES_FILE);
+		if (file_exists(file))
+			config_source.file = file;
+		else if (get_oid(GITMODULES_HEAD, &oid) >= 0)
+			config_source.blob = GITMODULES_HEAD;
+
+		config_with_options(fn, data, &config_source, &opts);
+
 		free(file);
 	}
 }

-- 
2.18.0
Stefan Beller· Aug 2, 2018, 20:27 UTC · re: Antonio Ospite · lore

Re: [RFC PATCH v2 09/12] submodule: support reading .gitmodules even when it's not checked out

On Thu, Aug 2, 2018 at 6:47 AM Antonio Ospite <ao2@ao2.it> wrote:
Show 9 quoted lines
>
> When the .gitmodules file is not available in the working tree, try
> using HEAD:.gitmodules from the current branch. This covers the case
> when the file is part of the repository but for some reason it is not
> checked out, for example because of a sparse checkout.
>
> This makes it possible to use at least the 'git submodule' commands
> which *read* the gitmodules configuration file without fully populating
> the working tree.

The reading part shows how nice our internal config system is, once everything is put in place.

> Writing to .gitmodules will still require that the file is checked out,
> so check for that before calling config_set_in_gitmodules_file_gently.
Antonio Ospite· Aug 2, 2018, 13:46 UTC · re: Antonio Ospite · lore

[RFC PATCH v2 02/12] submodule: factor out a config_set_in_gitmodules_file_gently function

Introduce a new config_set_in_gitmodules_file_gently() function to write config values to the .gitmodules file.

This is in preparation for a future change which will use the function to write to the .gitmodules file in a more controlled way instead of using "git config -f .gitmodules".

The purpose of the change is mainly to centralize the code that writes to the .gitmodules file to avoid some duplication.

The naming follows git_config_set_in_file_gently() but the git_ prefix is removed to communicate that this is not a generic git-config API.

Signed-off-by: Antonio Ospite <ao2@ao2.it>
---
 submodule-config.c | 12 ++++++++++++
 submodule-config.h |  1 +
 submodule.c        | 10 +++-------
 3 files changed, 16 insertions(+), 7 deletions(-)
Show changes to 3 files +16 −7

submodule-config.c, submodule-config.h, submodule.c

diff --git a/submodule-config.c b/submodule-config.c
index 6f6f5f9960..702d40dd6b 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -707,6 +707,18 @@ int print_config_from_gitmodules(const char *key)
 	return 0;
 }
 
+int config_set_in_gitmodules_file_gently(const char *key, const char *value)
+{
+	int ret;
+
+	ret = git_config_set_in_file_gently(GITMODULES_FILE, key, value);
+	if (ret < 0)
+		/* Maybe the user already did that, don't error out here */
+		warning(_("Could not update .gitmodules entry %s"), key);
+
+	return ret;
+}
+
 struct fetch_config {
 	int *max_children;
 	int *recurse_submodules;
diff --git a/submodule-config.h b/submodule-config.h
index 6fec3caadd..074e74a01c 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -57,6 +57,7 @@ void submodule_free(struct repository *r);
 int check_submodule_name(const char *name);
 
 extern int print_config_from_gitmodules(const char *key);
+extern int config_set_in_gitmodules_file_gently(const char *key, const char *value);
 
 /*
  * Note: these helper functions exist solely to maintain backward
diff --git a/submodule.c b/submodule.c
index 2a6381864e..2af09068d7 100644
--- a/submodule.c
+++ b/submodule.c
@@ -89,6 +89,7 @@ int update_path_in_gitmodules(const char *oldpath, const char *newpath)
 {
 	struct strbuf entry = STRBUF_INIT;
 	const struct submodule *submodule;
+	int ret;
 
 	if (!file_exists(GITMODULES_FILE)) /* Do nothing without .gitmodules */
 		return -1;
@@ -104,14 +105,9 @@ int update_path_in_gitmodules(const char *oldpath, const char *newpath)
 	strbuf_addstr(&entry, "submodule.");
 	strbuf_addstr(&entry, submodule->name);
 	strbuf_addstr(&entry, ".path");
-	if (git_config_set_in_file_gently(GITMODULES_FILE, entry.buf, newpath) < 0) {
-		/* Maybe the user already did that, don't error out here */
-		warning(_("Could not update .gitmodules entry %s"), entry.buf);
-		strbuf_release(&entry);
-		return -1;
-	}
+	ret = config_set_in_gitmodules_file_gently(entry.buf, newpath);
 	strbuf_release(&entry);
-	return 0;
+	return ret;
 }
 
 /*

-- 
2.18.0
Antonio Ospite· Aug 2, 2018, 13:46 UTC · re: Antonio Ospite · lore

[RFC PATCH v2 05/12] submodule: use the 'submodule--helper config' command

Use the 'submodule--helper config' command in git-submodules.sh to avoid referring explicitly to .gitmodules by the hardcoded file path.

This makes it possible to access the submodules configuration in a more controlled way.

Signed-off-by: Antonio Ospite <ao2@ao2.it>
---

Note that there are other instances of "git config -f .gitmodules" in test files, but I am not touching those for now.

 git-submodule.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
Show changes to git-submodule.sh +4 −4
diff --git a/git-submodule.sh b/git-submodule.sh
index 8b5ad59bde..ff258e2e8c 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -72,7 +72,7 @@ get_submodule_config () {
 	value=$(git config submodule."$name"."$option")
 	if test -z "$value"
 	then
-		value=$(git config -f .gitmodules submodule."$name"."$option")
+		value=$(git submodule--helper config submodule."$name"."$option")
 	fi
 	printf '%s' "${value:-$default}"
 }
@@ -283,11 +283,11 @@ or you are unsure what this means choose another name with the '--name' option."
 	git add --no-warn-embedded-repo $force "$sm_path" ||
 	die "$(eval_gettext "Failed to add submodule '\$sm_path'")"
 
-	git config -f .gitmodules submodule."$sm_name".path "$sm_path" &&
-	git config -f .gitmodules submodule."$sm_name".url "$repo" &&
+	git submodule--helper config submodule."$sm_name".path "$sm_path" &&
+	git submodule--helper config submodule."$sm_name".url "$repo" &&
 	if test -n "$branch"
 	then
-		git config -f .gitmodules submodule."$sm_name".branch "$branch"
+		git submodule--helper config submodule."$sm_name".branch "$branch"
 	fi &&
 	git add --force .gitmodules ||
 	die "$(eval_gettext "Failed to register submodule '\$sm_path'")"

-- 
2.18.0
Stefan Beller· Aug 2, 2018, 18:59 UTC · re: Antonio Ospite · lore

Re: [RFC PATCH v2 05/12] submodule: use the 'submodule--helper config' command

On Thu, Aug 2, 2018 at 6:47 AM Antonio Ospite <ao2@ao2.it> wrote:
>
> Use the 'submodule--helper config' command in git-submodules.sh to avoid
> referring explicitly to .gitmodules by the hardcoded file path.
ccol! This is the corner stone to the work of the previous patches. Nicely done!
Show 8 quoted lines
> This makes it possible to access the submodules configuration in a more
> controlled way.
>
> Signed-off-by: Antonio Ospite <ao2@ao2.it>
> ---
>
> Note that there are other instances of "git config -f .gitmodules" in test
> files, but I am not touching those for now.

I just checked and this converts all of git-submodule.sh which would have been my expectation as that is the real product.

The tests are fine to use "git config -f .gitmodules" as there we want to setup a specific environment to be tested? So I would think even future patches in this series will not touch test files for such a conversion as in here.

Stefan
Antonio Ospite· Aug 2, 2018, 13:46 UTC · re: Antonio Ospite · lore

[RFC PATCH v2 04/12] submodule--helper: add a new 'config' subcommand

Add a new 'config' subcommand to 'submodule--helper', this extra level of indirection makes it possible to add some flexibility to how the submodules configuration is handled.

Signed-off-by: Antonio Ospite <ao2@ao2.it>
---

Note that the tests follow the predominant style in the file: subshell and cd right at the start of the sub-shell opening.

 builtin/submodule--helper.c | 17 +++++++++++++++++
 t/t7411-submodule-config.sh | 26 ++++++++++++++++++++++++++
 2 files changed, 43 insertions(+)
Show changes to 2 files +43 −0

builtin/submodule--helper.c, t/t7411-submodule-config.sh

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index a3c4564c6c..14f0845d30 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2029,6 +2029,22 @@ static int connect_gitdir_workingtree(int argc, const char **argv, const char *p
 	return 0;
 }
 
+static int module_config(int argc, const char **argv, const char *prefix)
+{
+	if (argc < 2 || argc > 3)
+		die("submodule--helper config takes 1 or 2 arguments: name [value]");
+
+	/* Equivalent to ACTION_GET in builtin/config.c */
+	if (argc == 2)
+		return print_config_from_gitmodules(argv[1]);
+
+	/* Equivalent to ACTION_SET in builtin/config.c */
+	if (argc == 3)
+		return config_set_in_gitmodules_file_gently(argv[1], argv[2]);
+
+	return 0;
+}
+
 #define SUPPORT_SUPER_PREFIX (1<<0)
 
 struct cmd_struct {
@@ -2057,6 +2073,7 @@ static struct cmd_struct commands[] = {
 	{"absorb-git-dirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
 	{"is-active", is_active, 0},
 	{"check-name", check_name, 0},
+	{"config", module_config, 0},
 };
 
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
index 248da0bc4f..bbca4b421d 100755
--- a/t/t7411-submodule-config.sh
+++ b/t/t7411-submodule-config.sh
@@ -141,4 +141,30 @@ test_expect_success 'error in history in fetchrecursesubmodule lets continue' '
 	)
 '
 
+test_expect_success 'reading submodules config with "submodule--helper config"' '
+	(cd super &&
+		echo "../submodule" >expected &&
+		git submodule--helper config submodule.submodule.url >actual &&
+		test_cmp expected actual
+	)
+'
+
+test_expect_success 'writing submodules config with "submodule--helper config"' '
+	(cd super &&
+		echo "new_url" >expected &&
+		git submodule--helper config submodule.submodule.url "new_url" &&
+		git submodule--helper config submodule.submodule.url >actual &&
+		test_cmp expected actual
+	)
+'
+
+test_expect_success 'overwriting unstaged submodules config with "submodule--helper config"' '
+	(cd super &&
+		echo "newer_url" >expected &&
+		git submodule--helper config submodule.submodule.url "newer_url" &&
+		git submodule--helper config submodule.submodule.url >actual &&
+		test_cmp expected actual
+	)
+'
+
 test_done

-- 
2.18.0
Stefan Beller· Aug 2, 2018, 18:47 UTC · re: Antonio Ospite · lore

Re: [RFC PATCH v2 04/12] submodule--helper: add a new 'config' subcommand

On Thu, Aug 2, 2018 at 6:47 AM Antonio Ospite <ao2@ao2.it> wrote:
Show 37 quoted lines
>
> Add a new 'config' subcommand to 'submodule--helper', this extra level
> of indirection makes it possible to add some flexibility to how the
> submodules configuration is handled.
>
> Signed-off-by: Antonio Ospite <ao2@ao2.it>
> ---
>
> Note that the tests follow the predominant style in the file: subshell and cd
> right at the start of the sub-shell opening.
>
>  builtin/submodule--helper.c | 17 +++++++++++++++++
>  t/t7411-submodule-config.sh | 26 ++++++++++++++++++++++++++
>  2 files changed, 43 insertions(+)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index a3c4564c6c..14f0845d30 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2029,6 +2029,22 @@ static int connect_gitdir_workingtree(int argc, const char **argv, const char *p
>         return 0;
>  }
>
> +static int module_config(int argc, const char **argv, const char *prefix)
> +{
> +       if (argc < 2 || argc > 3)
> +               die("submodule--helper config takes 1 or 2 arguments: name [value]");
> +
> +       /* Equivalent to ACTION_GET in builtin/config.c */
> +       if (argc == 2)
> +               return print_config_from_gitmodules(argv[1]);
> +
> +       /* Equivalent to ACTION_SET in builtin/config.c */
> +       if (argc == 3)
> +               return config_set_in_gitmodules_file_gently(argv[1], argv[2]);
> +
> +       return 0;

Technically we cannot reach this point here? Maybe it would be more defensive to

    BUG("How did we get here?");
or at least return something !=0 ?
>
> +test_expect_success 'reading submodules config with "submodule--helper config"' '

I shortly wondered if it would make sense to put these tests at the beginning of either this or a new file, as the functionality is rather fundamental. (I never thought about it, but actually it is better to go from common basic to more exotic tests as you scroll down the file), but this place is ok, if you choose to do so.

Thanks, Stefan

Jeff King· Aug 2, 2018, 19:20 UTC · re: Stefan Beller · lore

Re: [RFC PATCH v2 04/12] submodule--helper: add a new 'config' subcommand

On Thu, Aug 02, 2018 at 11:47:30AM -0700, Stefan Beller wrote:
Show 21 quoted lines
> > +static int module_config(int argc, const char **argv, const char *prefix)
> > +{
> > +       if (argc < 2 || argc > 3)
> > +               die("submodule--helper config takes 1 or 2 arguments: name [value]");
> > +
> > +       /* Equivalent to ACTION_GET in builtin/config.c */
> > +       if (argc == 2)
> > +               return print_config_from_gitmodules(argv[1]);
> > +
> > +       /* Equivalent to ACTION_SET in builtin/config.c */
> > +       if (argc == 3)
> > +               return config_set_in_gitmodules_file_gently(argv[1], argv[2]);
> > +
> > +       return 0;
> 
> Technically we cannot reach this point here?
> Maybe it would be more defensive to
> 
>     BUG("How did we get here?");
> 
> or at least return something !=0 ?

When I find myself reaching for a BUG(), it is often a good time to see if we can restructure the code so that the logic more naturally falls out. Here the issue is that the first if conditional repeats the "else" for the other two. So I think we could just write:

  if (argc == 2)
	return ...
  if (argc == 3)
	return ...
  die("need 1 or 2 arguments");
-Peff
Antonio Ospite· Aug 3, 2018, 10:21 UTC · re: Jeff King · lore

Re: [RFC PATCH v2 04/12] submodule--helper: add a new 'config' subcommand

On Thu, 2 Aug 2018 15:20:33 -0400 Jeff King <peff@peff.net> wrote:

Show 36 quoted lines
> On Thu, Aug 02, 2018 at 11:47:30AM -0700, Stefan Beller wrote:
> 
> > > +static int module_config(int argc, const char **argv, const char *prefix)
> > > +{
> > > +       if (argc < 2 || argc > 3)
> > > +               die("submodule--helper config takes 1 or 2 arguments: name [value]");
> > > +
> > > +       /* Equivalent to ACTION_GET in builtin/config.c */
> > > +       if (argc == 2)
> > > +               return print_config_from_gitmodules(argv[1]);
> > > +
> > > +       /* Equivalent to ACTION_SET in builtin/config.c */
> > > +       if (argc == 3)
> > > +               return config_set_in_gitmodules_file_gently(argv[1], argv[2]);
> > > +
> > > +       return 0;
> > 
> > Technically we cannot reach this point here?
> > Maybe it would be more defensive to
> > 
> >     BUG("How did we get here?");
> > 
> > or at least return something !=0 ?
> 
> When I find myself reaching for a BUG(), it is often a good time to see
> if we can restructure the code so that the logic more naturally falls
> out.  Here the issue is that the first if conditional repeats the "else"
> for the other two. So I think we could just write:
> 
>   if (argc == 2)
> 	return ...
>   if (argc == 3)
> 	return ...
> 
>   die("need 1 or 2 arguments");
> 
Hi Jeff,

I like that, I'll see how this plays out after patch 06 which adds another option, and decide whether to use this style; validating arguments beforehand may still look a little cleaner.

Thanks for the comment.
Ciao,
   Antonio
-- 
Antonio Ospite
https://ao2.it
https://twitter.com/ao2it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
Antonio Ospite· Aug 2, 2018, 13:46 UTC · re: Antonio Ospite · lore

[RFC PATCH v2 06/12] submodule--helper: add a '--stage' option to the 'config' sub command

Add a --stage option to the 'submodule--helper config' command so that the .gitmodules file can be staged without referring to it explicitly by its file path.

In practice the config will still be written to .gitmodules, there are no plans to change the file path, but having this level of indirection makes it possible to perform additional checks before staging the file.

Note that "git submodule--helper config --stage" will fail when .gitmodules does not exist, this is to be consistent with how "git add" and "git rm" work: if the file to be staged does not exist they will complain.

Signed-off-by: Antonio Ospite <ao2@ao2.it>
---
 builtin/submodule--helper.c | 54 +++++++++++++++++++++++++++++++++++--
 t/t7411-submodule-config.sh | 35 ++++++++++++++++++++++++
 2 files changed, 87 insertions(+), 2 deletions(-)
Show changes to 2 files +87 −2

builtin/submodule--helper.c, t/t7411-submodule-config.sh

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 14f0845d30..c388f4ee6f 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -5,6 +5,7 @@
 #include "parse-options.h"
 #include "quote.h"
 #include "pathspec.h"
+#include "lockfile.h"
 #include "dir.h"
 #include "submodule.h"
 #include "submodule-config.h"
@@ -2031,8 +2032,57 @@ static int connect_gitdir_workingtree(int argc, const char **argv, const char *p
 
 static int module_config(int argc, const char **argv, const char *prefix)
 {
-	if (argc < 2 || argc > 3)
-		die("submodule--helper config takes 1 or 2 arguments: name [value]");
+	int stage_gitmodules = 0;
+
+	struct option module_config_options[] = {
+		OPT_BOOL(0, "stage", &stage_gitmodules,
+			    N_("stage the .gitmodules file content")),
+		OPT_END()
+	};
+	const char *const git_submodule_helper_usage[] = {
+		N_("git submodule--helper config name [value]"),
+		N_("git submodule--helper config --stage"),
+		NULL
+	};
+
+	argc = parse_options(argc, argv, prefix, module_config_options,
+			     git_submodule_helper_usage, PARSE_OPT_KEEP_ARGV0);
+
+	if ((stage_gitmodules && argc > 1) ||
+	    (!stage_gitmodules && (argc < 2 || argc > 3)))
+		usage_with_options(git_submodule_helper_usage, module_config_options);
+
+	/*
+	 * Equivalent to "git add --force .gitmodules".
+	 *
+	 * Silently override already staged changes, to support consecutive
+	 * invocations of "git submodule add".
+	 */
+	if (stage_gitmodules) {
+		static struct lock_file lock_file;
+
+		hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);
+
+		if (read_cache() < 0)
+			die(_("index file corrupt"));
+
+		/*
+		 * Do not use is_staging_gitmodules_ok() here to make it
+		 * possible to stage multiple changes before committing them,
+		 * in particular this covers the case of consecutive calls of
+		 * "git submodule add".
+		 */
+		if (!file_exists(GITMODULES_FILE))
+			die(_("%s does not exists"), GITMODULES_FILE);
+
+		stage_updated_gitmodules(&the_index);
+
+		if (write_locked_index(&the_index, &lock_file,
+				       COMMIT_LOCK | SKIP_IF_UNCHANGED))
+			die(_("Unable to write new index file"));
+
+		return 0;
+	}
 
 	/* Equivalent to ACTION_GET in builtin/config.c */
 	if (argc == 2)
diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
index bbca4b421d..8ff6ed444a 100755
--- a/t/t7411-submodule-config.sh
+++ b/t/t7411-submodule-config.sh
@@ -167,4 +167,39 @@ test_expect_success 'overwriting unstaged submodules config with "submodule--hel
 	)
 '
 
+test_expect_success 'staging submodules config with "submodule--helper config"' '
+	(cd super &&
+		: >expected &&
+		git diff --name-only --cached >actual &&
+		test_cmp expected actual &&
+		git submodule--helper config --stage &&
+		echo ".gitmodules" >expected &&
+		git diff --name-only --cached >actual &&
+		test_cmp expected actual
+	)
+'
+
+test_expect_success 'overwriting staged submodules config with "submodule--helper config"' '
+	(cd super &&
+		echo "even_newer_url" >expected &&
+		git submodule--helper config submodule.submodule.url "even_newer_url" &&
+		git submodule--helper config submodule.submodule.url >actual &&
+		test_cmp expected actual
+	)
+'
+
+test_expect_success 're-staging overridden submodule config with "submodule--helper config"' '
+	(cd super &&
+		echo ".gitmodules" >expected &&
+		git diff --name-only --cached >actual &&
+		test_cmp expected actual &&
+		echo "bogus config" >.gitmodules &&
+		git submodule--helper config --stage &&
+		git diff --name-only --cached >actual &&
+		test_cmp expected actual &&
+		git reset HEAD .gitmodules &&
+		git checkout .gitmodules
+	)
+'
+
 test_done

-- 
2.18.0
Junio C Hamano· Aug 2, 2018, 18:57 UTC · re: Antonio Ospite · lore

Re: [RFC PATCH v2 06/12] submodule--helper: add a '--stage' option to the 'config' sub command

Antonio Ospite <ao2@ao2.it> writes:
> Add a --stage option to the 'submodule--helper config' command so that
> the .gitmodules file can be staged without referring to it explicitly by
> its file path.
Sorry, but I have no clue what the above is trying to say.

The original 's--h config <name> [<value>]' is quite understandable. It is like "git config <name> [<value>]", i.e. either get the current value for the name, or set a new value to the name.

What does this 's--h config --stage' that does not take any option do? "git add .gitmodules"? Something else? In what meaning is the word "stage" used? Is it used as a verb "to stage"?

In a series that lets us use the data in the .gitmodules file without materializing the file in the working tree, I would have expected that you would want an option to specify which .gitmodules among (1) the one in the working tree (i.e. the only option we currently have), (2) the one in the index, and (3) the one in the HEAD, and when I saw the title, I would have expected that

	git submodule--helper config --stage name

may be a way to read from the .gitmodules in the index to find the value for name (however, we we follow the option naming convention in gitcli.txt, it should be called --cached, I would think).

> In practice the config will still be written to .gitmodules, there are
> no plans to change the file path, but having this level of indirection
> makes it possible to perform additional checks before staging the file.
Again, a claim without explanation or justification.
If you are planning to something like
 - prepare trial contents in .gitmodules.new file
 - do whatever "additional checks" on .gitmodules.new
 - add the contents to it to the index as a new .gitmodules blob

Then you do not need such an option. "submodule--helper" is purely a helper for scripts, and not for human consumption, so scripts can just hash-object the blob contents out and update-index --cacheinfo to register the blob at any location of choice.

But as I said, this step is way under-explained, so my guess above may not match what you really wanted to do.

Antonio Ospite· Aug 3, 2018, 11:03 UTC · re: Junio C Hamano · lore

Re: [RFC PATCH v2 06/12] submodule--helper: add a '--stage' option to the 'config' sub command

On Thu, 02 Aug 2018 11:57:14 -0700 Junio C Hamano <gitster@pobox.com> wrote:

Show 8 quoted lines
> Antonio Ospite <ao2@ao2.it> writes:
> 
> > Add a --stage option to the 'submodule--helper config' command so that
> > the .gitmodules file can be staged without referring to it explicitly by
> > its file path.
> 
> Sorry, but I have no clue what the above is trying to say.
>

You got me :), this was one of the changes I was having the more doubts about, basically about the user interface, and it must have showed in the commit message.

Show 8 quoted lines
> The original 's--h config <name> [<value>]' is quite understandable.  It
> is like "git config <name> [<value>]", i.e. either get the current value
> for the name, or  set a new value to the name.
> 
> What does this 's--h config --stage' that does not take any option
> do?  "git add .gitmodules"?  Something else?  In what meaning is the
> word "stage" used?  Is it used as a verb "to stage"?
>

"s--h config --stage" is meant to replace "git add -f .gitmodules" as shown in patch 07, maybe the two patches might have been merged to give more context, but I kept the changes separate to follow how things were done with patches 04 and 05.

The rationale behind the change is to close the circle started with 04 and 05 and stop referring to .gitmodules explicitly by file path in git commands. The change is not strictly needed for the series, it's for consistency sake.

The name "stage" is meant as a verb and was inspired by the "git stage" command and by the stage_updated_gitmodules() function, I used an option flag with no arguments because I saw this has been done for verbs before, e.g. "git config --list". I did not know about "gitcli.txt", sorry, I'll check it out.

Maybe a better name could be "submodule--helper config --add"?
The commit message clearly needs improvements.
Show 13 quoted lines
> In a series that lets us use the data in the .gitmodules file without
> materializing the file in the working tree, I would have expected
> that you would want an option to specify which .gitmodules among
> (1) the one in the working tree (i.e. the only option we currently
> have), (2) the one in the index, and (3) the one in the HEAD, and
> when I saw the title, I would have expected that
> 
> 	git submodule--helper config --stage name
> 
> may be a way to read from the .gitmodules in the index to find the
> value for name (however, we we follow the option naming convention
> in gitcli.txt, it should be called --cached, I would think).
>

For my use case the automatic behavior of falling back to HEAD:.gitmodules is enough, so I focused on that to have the functionality merged.

The option you suggest may be useful but I'd leave that as a possible future addition in case someone else needed it.

Show 19 quoted lines
> > In practice the config will still be written to .gitmodules, there are
> > no plans to change the file path, but having this level of indirection
> > makes it possible to perform additional checks before staging the file.
> 
> Again, a claim without explanation or justification.
> 
> If you are planning to something like
> 
>  - prepare trial contents in .gitmodules.new file
>  - do whatever "additional checks" on .gitmodules.new
>  - add the contents to it to the index as a new .gitmodules blob
> 
> Then you do not need such an option.  "submodule--helper" is purely
> a helper for scripts, and not for human consumption, so scripts can
> just hash-object the blob contents out and update-index --cacheinfo
> to register the blob at any location of choice.
> 
> But as I said, this step is way under-explained, so my guess above
> may not match what you really wanted to do.

As stated, the motivation for now is just syntactical: remove hardcoded references to .gitmodules in scripts.

No new "additional checks" are added as of now, the commit message is misleading indeed.

Thanks,
   Antonio
-- 
Antonio Ospite
https://ao2.it
https://twitter.com/ao2it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
Junio C Hamano· Aug 3, 2018, 16:24 UTC · re: Antonio Ospite · lore

Re: [RFC PATCH v2 06/12] submodule--helper: add a '--stage' option to the 'config' sub command

Antonio Ospite <ao2@ao2.it> writes:
> The rationale behind the change is to close the circle started with 04
> and 05 and stop referring to .gitmodules explicitly by file path in git
> commands. The change is not strictly needed for the series, it's for
> consistency sake.

Sorry, but I am still not quite sure what consistency you are referring to.

I also do not see a reason why we want to stop referring to .gitmodules explicitly by name. We do not hide the fact that in-tree .gitignore and .gitattributes files are used to hold the metainformation about the project tree, saying that it is an implementation detail. Is there a good reason why .gitmodules should be different from these other two?

Antonio Ospite· Aug 6, 2018, 10:58 UTC · re: Junio C Hamano · lore

Re: [RFC PATCH v2 06/12] submodule--helper: add a '--stage' option to the 'config' sub command

On Fri, 03 Aug 2018 09:24:14 -0700 Junio C Hamano <gitster@pobox.com> wrote:

Show 10 quoted lines
> Antonio Ospite <ao2@ao2.it> writes:
> 
> > The rationale behind the change is to close the circle started with 04
> > and 05 and stop referring to .gitmodules explicitly by file path in git
> > commands. The change is not strictly needed for the series, it's for
> > consistency sake.
> 
> Sorry, but I am still not quite sure what consistency you are
> referring to.
>
Hi Junio,

to recap: in previous patches we removed all instances of "git config -f .gitmodules" (read/write operations), because in that case encapsulating access to .gitmodules would directly enable us to add new functionality (i.e.: read from HEAD:.gitmodules).

So, to me, it looked more organic to remove also the last explicit reference to .gitmodules in git-submodule.sh ("git add -f .gitmodules", the "stage" operation), even if in this case no new behavior about staging is going to be added for the time being. that is: the change is not strictly necessary.

The consistency I was referring to is merely in the mechanism used to deal with .gitmodules: by always using "submodule--helper config".

As a side argument: in some previous discussion Stefan mentioned the possibility that, in the future, he may be interested to explore different locations for submodules configuration, like a special ref, to prevent .gitmodules from being in the working tree at all, not even for writing submodule configuration.

Having an opaque "stage submodule configuration" operation would prepare for that too, but as I said this is not my immediate goal.

Show 6 quoted lines
> I also do not see a reason why we want to stop referring to
> .gitmodules explicitly by name.  We do not hide the fact that
> in-tree .gitignore and .gitattributes files are used to hold the
> metainformation about the project tree, saying that it is an
> implementation detail.  Is there a good reason why .gitmodules
> should be different from these other two?

Not sure about that, but one difference I can see between .gitignore/.gitattributes and .gitmodules is that I got the impression that editing the latter by hand is strongly discouraged, if that is indeed the case a layer of indirection can make sense IMHO to make the actual file path less relevant.

Having said that I can drop patches 06/07 if this can help moving things forward; or if I had some success in convincing you I can improve the user interface (any advice?) and the commit message.

Thank you,
   Antonio
-- 
Antonio Ospite
https://ao2.it
https://twitter.com/ao2it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
Junio C Hamano· Aug 6, 2018, 17:38 UTC · re: Antonio Ospite · lore

Re: [RFC PATCH v2 06/12] submodule--helper: add a '--stage' option to the 'config' sub command

Antonio Ospite <ao2@ao2.it> writes:
Show 12 quoted lines
>> I also do not see a reason why we want to stop referring to
>> .gitmodules explicitly by name.  We do not hide the fact that
>> in-tree .gitignore and .gitattributes files are used to hold the
>> metainformation about the project tree, saying that it is an
>> implementation detail.  Is there a good reason why .gitmodules
>> should be different from these other two?
>
> Not sure about that, but one difference I can see
> between .gitignore/.gitattributes and .gitmodules is that I got the
> impression that editing the latter by hand is strongly discouraged, if
> that is indeed the case a layer of indirection can make sense IMHO to
> make the actual file path less relevant.

I do not think we discourage hand editing of .gitmodules more than others, say .gitignore; and I do not see a sane reason to do so.

"If you commit broken .gitmodules and let another person clone it, submodules will not be checked out correctly" is *not* a sane reason, as exactly the same thing can be said for incorrect checkout of files with broken .gitattributes.

Quite honestly, I just want to get over with this minor detail that won't help any scripts (after all submodule--helper is not meant to be used by humans) and focus on other parts of the patch series.

Antonio Ospite· Aug 7, 2018, 09:19 UTC · re: Junio C Hamano · lore

Re: [RFC PATCH v2 06/12] submodule--helper: add a '--stage' option to the 'config' sub command

On Mon, 06 Aug 2018 10:38:20 -0700 Junio C Hamano <gitster@pobox.com> wrote:

Show 23 quoted lines
> Antonio Ospite <ao2@ao2.it> writes:
> 
> >> I also do not see a reason why we want to stop referring to
> >> .gitmodules explicitly by name.  We do not hide the fact that
> >> in-tree .gitignore and .gitattributes files are used to hold the
> >> metainformation about the project tree, saying that it is an
> >> implementation detail.  Is there a good reason why .gitmodules
> >> should be different from these other two?
> >
> > Not sure about that, but one difference I can see
> > between .gitignore/.gitattributes and .gitmodules is that I got the
> > impression that editing the latter by hand is strongly discouraged, if
> > that is indeed the case a layer of indirection can make sense IMHO to
> > make the actual file path less relevant.
> 
> I do not think we discourage hand editing of .gitmodules more than
> others, say .gitignore; and I do not see a sane reason to do so.
> 
> "If you commit broken .gitmodules and let another person clone it,
> submodules will not be checked out correctly" is *not* a sane
> reason, as exactly the same thing can be said for incorrect checkout
> of files with broken .gitattributes.
>

OK, I see, maybe I got the wrong impression because while .gitignore/.gitattributes are expected to be hand edited, in my limited usage of submodules I never had to edit .gitmodules manually since git did it for me; I guess that does not necessarily mean it's discouraged and it may even be necessary for more advanced usages.

> Quite honestly, I just want to get over with this minor detail that
> won't help any scripts (after all submodule--helper is not meant to
> be used by humans) and focus on other parts of the patch series.
Agreed, let's drop 06 and 07 then.
Thanks,
   Antonio
-- 
Antonio Ospite
https://ao2.it
https://twitter.com/ao2it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
Stefan Beller· Aug 6, 2018, 18:19 UTC · re: Antonio Ospite · lore

Re: [RFC PATCH v2 06/12] submodule--helper: add a '--stage' option to the 'config' sub command

Show 12 quoted lines
> The consistency I was referring to is merely in the mechanism used to
> deal with .gitmodules: by always using "submodule--helper config".
>
> As a side argument: in some previous discussion Stefan mentioned the
> possibility that, in the future, he may be interested to explore
> different locations for submodules configuration, like a special ref,
> to prevent .gitmodules from being in the working tree at all, not even
> for writing submodule configuration.
>
> Having an opaque "stage submodule configuration" operation would
> prepare for that too, but as I said this is not my immediate goal.
>
Thanks for demonstrating that this will be easy to achieve in the future!
> Having said that I can drop patches 06/07 if this can help moving things
> forward; or if I had some success in convincing you I can improve the
> user interface (any advice?) and the commit message.
>

I think dropping the patches would be a good idea, given that this is not your immediate goal.

Thanks, Stefan

← back to recent threads