Re: [PATCH v7 09/10] submodule: support reading .gitmodules when it's not in the working tree
On Tue, 30 Oct 2018 10:57:09 +0100 (STD) Johannes Schindelin wrote: > Hi Antonio, > Hi Johannes, > On Thu, 25 Oct 2018, Antonio Ospite wrote: > > > diff --git a/t/t7418-submodule-sparse-gitmodules.sh > > b/t/t7418-submodule-sparse-gitmodules.sh > > new file mode 100755 [...] > > + echo "$PWD/submodule" >expect && > > Would you mind squashing this fixup in? > > -- snip -- > diff --git a/t/t7418-submodule-sparse-gitmodules.sh > b/t/t7418-submodule-sparse-gitmodules.sh > index 21a86b89c6cb..3f7f27188313 100755 > --- a/t/t7418-submodule-sparse-gitmodules.sh > +++ b/t/t7418-submodule-sparse-gitmodules.sh > @@ -55,7 +55,7 @@ test_expect_success 'initialising submodule when the > gitmodules config is not ch > test_must_fail git -C super config submodule.submodule.url && > git -C super submodule init && > git -C super config submodule.submodule.url >actual && > - echo "$PWD/submodule" >expect && > + echo "$(pwd)/submodule" >expect && > test_cmp expect actual > ' > > -- snap -- > > On Windows, `$PWD` and `$(pwd)` are *not* synonymous. The former > reflects the "Unix path" which is understood by the Bash script (and > only by the Bash script, *not* by `git.exe`!) while the latter refers to > the actual Windows path. > I see, this is also mentioned in t/README, I had overlooked that part. Thank you for reporting. > Without this fix, your new test case will fail on Windows all the time, > see e.g. > https://git-for-windows.visualstudio.com/git/_build/results?buildId=22913=logs > Junio, what is the plan for 'ao/submodule-wo-gitmodules-checked-out'? I see it's not in next yet; do you want me to resend the whole series with this fixup in or would it be less overhead for you to apply it directly to patch 9/10 from v7 of the series? Thank you, Antonio P.S. I was wondering if it is worth having patchset versions mentioned somewhere in pu/, maybe in merge commits if not in branch names? Or at least in whats-cooking.txt next to the date. -- 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?
[PATCH v7 03/10] t7411: merge tests 5 and 6
Tests 5 and 6 check for the effects of the same commit, merge the two tests to make it more straightforward to clean things up after the test has finished. The cleanup will be added in a future commit. Signed-off-by: Antonio Ospite --- t/t7411-submodule-config.sh | 18 +- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh index 0bde5850ac..f2cd1f4a2c 100755 --- a/t/t7411-submodule-config.sh +++ b/t/t7411-submodule-config.sh @@ -82,29 +82,21 @@ Submodule name: 'a' for path 'b' Submodule name: 'submodule' for path 'submodule' EOF -test_expect_success 'error in one submodule config lets continue' ' +test_expect_success 'error in history of one submodule config lets continue, stderr message contains blob ref' ' (cd super && cp .gitmodules .gitmodules.bak && echo " value = \"" >>.gitmodules && git add .gitmodules && mv .gitmodules.bak .gitmodules && git commit -m "add error" && - test-tool submodule-config \ - HEAD b \ - HEAD submodule \ - >actual && - test_cmp expect_error actual - ) -' - -test_expect_success 'error message contains blob reference' ' - (cd super && sha1=$(git rev-parse HEAD) && test-tool submodule-config \ HEAD b \ HEAD submodule \ - 2>actual_err && - test_i18ngrep "submodule-blob $sha1:.gitmodules" actual_err >/dev/null + >actual \ + 2>actual_stderr && + test_cmp expect_error actual && + test_i18ngrep "submodule-blob $sha1:.gitmodules" actual_stderr >/dev/null ) ' -- 2.19.1
[PATCH v7 08/10] submodule: add a helper to check if it is safe to write to .gitmodules
Introduce a helper function named is_writing_gitmodules_ok() to verify
that the .gitmodules file is safe to write.
The function name follows the scheme of is_staging_gitmodules_ok().
The two symbolic constants GITMODULES_INDEX and GITMODULES_HEAD are used
to get help from the C preprocessor in preventing typos, especially for
future users.
This is in preparation for a future change which teaches git how to read
.gitmodules from the index or from the current branch if the file is not
available in the working tree.
The rationale behind the check is that writing to .gitmodules requires
the file to be present in the working tree, unless a brand new
.gitmodules is being created (in which case the .gitmodules file would
not exist at all: neither in the working tree nor in the index or in the
current branch).
Expose the functionality also via a "submodule-helper config
--check-writeable" command, as git scripts may want to perform the check
before modifying submodules configuration.
Signed-off-by: Antonio Ospite
---
builtin/submodule--helper.c | 24 +++-
cache.h | 2 ++
submodule.c | 18 ++
submodule.h | 1 +
t/t7411-submodule-config.sh | 31 +++
5 files changed, 75 insertions(+), 1 deletion(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 9af6626e32..b272a78801 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2128,6 +2128,28 @@ static int check_name(int argc, const char **argv, const
char *prefix)
static int module_config(int argc, const char **argv, const char *prefix)
{
+ enum {
+ CHECK_WRITEABLE = 1
+ } command = 0;
+
+ struct option module_config_options[] = {
+ OPT_CMDMODE(0, "check-writeable", ,
+ N_("check if it is safe to write to the .gitmodules
file"),
+ CHECK_WRITEABLE),
+ OPT_END()
+ };
+ const char *const git_submodule_helper_usage[] = {
+ N_("git submodule--helper config name [value]"),
+ N_("git submodule--helper config --check-writeable"),
+ NULL
+ };
+
+ argc = parse_options(argc, argv, prefix, module_config_options,
+git_submodule_helper_usage, PARSE_OPT_KEEP_ARGV0);
+
+ if (argc == 1 && command == CHECK_WRITEABLE)
+ return is_writing_gitmodules_ok() ? 0 : -1;
+
/* Equivalent to ACTION_GET in builtin/config.c */
if (argc == 2)
return print_config_from_gitmodules(the_repository, argv[1]);
@@ -2136,7 +2158,7 @@ static int module_config(int argc, const char **argv,
const char *prefix)
if (argc == 3)
return config_set_in_gitmodules_file_gently(argv[1], argv[2]);
- die("submodule--helper config takes 1 or 2 arguments: name [value]");
+ usage_with_options(git_submodule_helper_usage, module_config_options);
}
#define SUPPORT_SUPER_PREFIX (1<<0)
diff --git a/cache.h b/cache.h
index 59c8a93046..4c1f827c54 100644
--- a/cache.h
+++ b/cache.h
@@ -486,6 +486,8 @@ 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_INDEX ":.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.c b/submodule.c
index 24a49eae61..9516685478 100644
--- a/submodule.c
+++ b/submodule.c
@@ -51,6 +51,24 @@ int is_gitmodules_unmerged(const struct index_state *istate)
return 0;
}
+/*
+ * Check if the .gitmodules file is safe to write.
+ *
+ * Writing to the .gitmodules file requires that the file exists in the
+ * working tree or, if it doesn't, that a brand new .gitmodules file is going
+ * to be created (i.e. it's neither in the index nor in the current branch).
+ *
+ * It is not safe to write to .gitmodules if it's not in the working tree but
+ * it is in the index or in the current branch, because writing new values
+ * (and staging them) would blindly overwrite ALL the old content.
+ */
+int is_writing_gitmodules_ok(void)
+{
+ struct object_id oid;
+ return file_exists(GITMODULES_FILE) ||
+ (get_oid(GITMODULES_INDEX, ) < 0 &&
get_oid(GITMODULES_HEAD, ) < 0);
+}
+
/*
* Check if the .gitmodules file has unstaged modifications. This must be
* checked before allowing modifications to the .gitmodules file with the
diff --git a/submodule.h b/submodule.h
index 4826601ff2..ea6e115f16 1006
[PATCH v7 10/10] t/helper: add test-submodule-nested-repo-config
Add a test tool to exercise config_from_gitmodules(), in particular for
the case of nested submodules.
Add also a test to document that reading the submoudles config of nested
submodules does not work yet when the .gitmodules file is not in the
working tree but it still in the index.
This is because the git API does not always make it possible access the
object store of an arbitrary repository (see get_oid() usage in
config_from_gitmodules()).
When this git limitation gets fixed the aforementioned use case will be
supported too.
Signed-off-by: Antonio Ospite
---
Makefile | 1 +
t/helper/test-submodule-nested-repo-config.c | 30 +
t/helper/test-tool.c | 1 +
t/helper/test-tool.h | 1 +
t/t7411-submodule-config.sh | 34
5 files changed, 67 insertions(+)
create mode 100644 t/helper/test-submodule-nested-repo-config.c
diff --git a/Makefile b/Makefile
index d18ab0fe78..d8f4dfdb3e 100644
--- a/Makefile
+++ b/Makefile
@@ -741,6 +741,7 @@ TEST_BUILTINS_OBJS += test-sigchain.o
TEST_BUILTINS_OBJS += test-strcmp-offset.o
TEST_BUILTINS_OBJS += test-string-list.o
TEST_BUILTINS_OBJS += test-submodule-config.o
+TEST_BUILTINS_OBJS += test-submodule-nested-repo-config.o
TEST_BUILTINS_OBJS += test-subprocess.o
TEST_BUILTINS_OBJS += test-urlmatch-normalization.o
TEST_BUILTINS_OBJS += test-wildmatch.o
diff --git a/t/helper/test-submodule-nested-repo-config.c
b/t/helper/test-submodule-nested-repo-config.c
new file mode 100644
index 00..a31e2a9bea
--- /dev/null
+++ b/t/helper/test-submodule-nested-repo-config.c
@@ -0,0 +1,30 @@
+#include "test-tool.h"
+#include "submodule-config.h"
+
+static void die_usage(int argc, const char **argv, const char *msg)
+{
+ fprintf(stderr, "%s\n", msg);
+ fprintf(stderr, "Usage: %s \n", argv[0]);
+ exit(1);
+}
+
+int cmd__submodule_nested_repo_config(int argc, const char **argv)
+{
+ struct repository submodule;
+
+ if (argc < 3)
+ die_usage(argc, argv, "Wrong number of arguments.");
+
+ setup_git_directory();
+
+ if (repo_submodule_init(, the_repository, argv[1])) {
+ die_usage(argc, argv, "Submodule not found.");
+ }
+
+ /* Read the config of _child_ submodules. */
+ print_config_from_gitmodules(, argv[2]);
+
+ submodule_free(the_repository);
+
+ return 0;
+}
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index 6b5836dc1b..ca5c5ede6c 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -46,6 +46,7 @@ static struct test_cmd cmds[] = {
{ "strcmp-offset", cmd__strcmp_offset },
{ "string-list", cmd__string_list },
{ "submodule-config", cmd__submodule_config },
+ { "submodule-nested-repo-config", cmd__submodule_nested_repo_config },
{ "subprocess", cmd__subprocess },
{ "urlmatch-normalization", cmd__urlmatch_normalization },
{ "wildmatch", cmd__wildmatch },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index e4890566da..500e3684e1 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -42,6 +42,7 @@ int cmd__sigchain(int argc, const char **argv);
int cmd__strcmp_offset(int argc, const char **argv);
int cmd__string_list(int argc, const char **argv);
int cmd__submodule_config(int argc, const char **argv);
+int cmd__submodule_nested_repo_config(int argc, const char **argv);
int cmd__subprocess(int argc, const char **argv);
int cmd__urlmatch_normalization(int argc, const char **argv);
int cmd__wildmatch(int argc, const char **argv);
diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
index 2cfabb18bc..89690b7adb 100755
--- a/t/t7411-submodule-config.sh
+++ b/t/t7411-submodule-config.sh
@@ -216,4 +216,38 @@ test_expect_success 'reading submodules config from the
current branch when .git
)
'
+test_expect_success 'reading nested submodules config' '
+ (cd super &&
+ git init submodule/nested_submodule &&
+ echo "a" >submodule/nested_submodule/a &&
+ git -C submodule/nested_submodule add a &&
+ git -C submodule/nested_submodule commit -m "add a" &&
+ git -C submodule submodule add ./nested_submodule &&
+ git -C submodule add nested_submodule &&
+ git -C submodule commit -m "added nested_submodule" &&
+ git add submodule &&
+ git commit -m "updated submodule" &&
+ echo "./nested_submodule" >expect &&
+ test-tool submodule-nested-repo-config \
+ submod
[PATCH v7 02/10] 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
---
submodule-config.c | 12
submodule-config.h | 1 +
submodule.c| 10 +++---
3 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/submodule-config.c b/submodule-config.c
index e36d4e2271..329c0b44f6 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -717,6 +717,18 @@ int print_config_from_gitmodules(struct repository *repo,
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 031747ccf8..4dc9b0771c 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -49,6 +49,7 @@ const struct submodule *submodule_from_path(struct repository
*r,
const char *path);
void submodule_free(struct repository *r);
int print_config_from_gitmodules(struct repository *repo, const char *key);
+int config_set_in_gitmodules_file_gently(const char *key, const char *value);
/*
* Returns 0 if the name is syntactically acceptable as a submodule "name"
diff --git a/submodule.c b/submodule.c
index d9d3046689..24a49eae61 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(, "submodule.");
strbuf_addstr(, submodule->name);
strbuf_addstr(, ".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();
- return -1;
- }
+ ret = config_set_in_gitmodules_file_gently(entry.buf, newpath);
strbuf_release();
- return 0;
+ return ret;
}
/*
--
2.19.1
[PATCH v7 06/10] 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
---
git-submodule.sh | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/git-submodule.sh b/git-submodule.sh
index 1b568e29b9..0805fadf47 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.19.1
[PATCH v7 01/10] submodule: add a print_config_from_gitmodules() helper
Add a new print_config_from_gitmodules() helper function to print values
from .gitmodules just like "git config -f .gitmodules" would.
This will be used by a new submodule--helper subcommand to be able to
access the .gitmodules file in a more controlled way.
Signed-off-by: Antonio Ospite
---
submodule-config.c | 25 +
submodule-config.h | 1 +
2 files changed, 26 insertions(+)
diff --git a/submodule-config.c b/submodule-config.c
index b132f7a80b..e36d4e2271 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -692,6 +692,31 @@ void submodule_free(struct repository *r)
submodule_cache_clear(r->submodule_cache);
}
+static int config_print_callback(const char *var, const char *value, void
*cb_data)
+{
+ char *wanted_key = cb_data;
+
+ if (!strcmp(wanted_key, var))
+ printf("%s\n", value);
+
+ return 0;
+}
+
+int print_config_from_gitmodules(struct repository *repo, const char *key)
+{
+ int ret;
+ char *store_key;
+
+ ret = git_config_parse_key(key, _key, NULL);
+ if (ret < 0)
+ return CONFIG_INVALID_KEY;
+
+ config_from_gitmodules(config_print_callback, repo, 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..031747ccf8 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -48,6 +48,7 @@ const struct submodule *submodule_from_path(struct repository
*r,
const struct object_id
*commit_or_tree,
const char *path);
void submodule_free(struct repository *r);
+int print_config_from_gitmodules(struct repository *repo, const char *key);
/*
* Returns 0 if the name is syntactically acceptable as a submodule "name"
--
2.19.1
[PATCH v7 07/10] t7506: clean up .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 intends 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 --- 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 943708fb04..08629a6e70 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.19.1
[PATCH v7 04/10] t7411: be nicer to future tests and really clean things up
Tests 5 and 7 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. Remove both the commits as soon as they are not needed anymore. Signed-off-by: Antonio Ospite --- t/t7411-submodule-config.sh | 7 +-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh index f2cd1f4a2c..b1f3c6489b 100755 --- a/t/t7411-submodule-config.sh +++ b/t/t7411-submodule-config.sh @@ -83,6 +83,8 @@ Submodule name: 'submodule' for path 'submodule' EOF test_expect_success 'error in history of one submodule config lets continue, stderr message contains blob ref' ' + ORIG=$(git -C super rev-parse HEAD) && + test_when_finished "git -C super reset --hard $ORIG" && (cd super && cp .gitmodules .gitmodules.bak && echo " value = \"" >>.gitmodules && @@ -115,6 +117,8 @@ test_expect_success 'using different treeishs works' ' ' test_expect_success 'error in history in fetchrecursesubmodule lets continue' ' + ORIG=$(git -C super rev-parse HEAD) && + test_when_finished "git -C super reset --hard $ORIG" && (cd super && git config -f .gitmodules \ submodule.submodule.fetchrecursesubmodules blabla && @@ -126,8 +130,7 @@ test_expect_success 'error in history in fetchrecursesubmodule lets continue' ' HEAD b \ HEAD submodule \ >actual && - test_cmp expect_error actual && - git reset --hard HEAD^ + test_cmp expect_error actual ) ' -- 2.19.1
[PATCH v7 00/10] Make submodules work if .gitmodules is not checked out
Hi, this series teaches git to try and read the .gitmodules file from the index (:.gitmodules) or from the current branch (HEAD:.gitmodules) when the file is not readily available in the working tree. This can be used, together with sparse checkouts, to enable submodule usage with programs like vcsh[1] which manage multiple repositories with their working trees sharing the same path. [1] https://github.com/RichiH/vcsh In the previous series there were some comments about not using the enum in patch 8, but I decided to leave the code as it was as I still think that it make sense to use an enum there, and have the default value unnamed; during the discussion I pointed out that other code in git do the same. In this series I am addressing the comments by Stefan Beller about the tests in patch 9. If the new tests look OK, I'd say we try moving the series to "next" and see what happens? I am available to address any further concerns in follow-up patches. v6 of the series is here: https://public-inbox.org/git/20181005130601.15879-1-...@ao2.it/ v5 of the series is here: https://public-inbox.org/git/20180917140940.3839-1-...@ao2.it/ v4 of the series is here: https://public-inbox.org/git/20180824132951.8000-1-...@ao2.it/ v3 of the series is here: https://public-inbox.org/git/20180814110525.17801-1-...@ao2.it/ v2 of the series is here: https://public-inbox.org/git/20180802134634.10300-1-...@ao2.it/ v1 of the series, with some background info, is here: https://public-inbox.org/git/20180514105823.8378-1-...@ao2.it/ Changes since v6: * Renamed t7416-submodule-sparse-gitmodules.sh to t7418-submodule-sparse-gitmodules.sh because t7416 was already taken. This has been already taken care of by Junio in "pu". * Improved tests in t7418: now, instead of just testing the return value of "git submodule ..." commands when .gitmodules is not in the working tree, the actual use case is checked in each test, with pre- and post-conditions. Thank you, Antonio Antonio Ospite (10): submodule: add a print_config_from_gitmodules() helper submodule: factor out a config_set_in_gitmodules_file_gently function t7411: merge tests 5 and 6 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 t7506: clean up .gitmodules properly before setting up new scenario submodule: add a helper to check if it is safe to write to .gitmodules submodule: support reading .gitmodules when it's not in the working tree t/helper: add test-submodule-nested-repo-config Makefile | 1 + builtin/grep.c | 17 ++- builtin/submodule--helper.c | 40 ++ cache.h | 2 + git-submodule.sh | 13 +- submodule-config.c | 68 - submodule-config.h | 2 + submodule.c | 28 +++- submodule.h | 1 + t/helper/test-submodule-nested-repo-config.c | 30 t/helper/test-tool.c | 1 + t/helper/test-tool.h | 1 + t/t7411-submodule-config.sh | 141 +-- t/t7418-submodule-sparse-gitmodules.sh | 122 t/t7506-status-submodule.sh | 3 +- t/t7814-grep-recurse-submodules.sh | 16 +++ 16 files changed, 454 insertions(+), 32 deletions(-) create mode 100644 t/helper/test-submodule-nested-repo-config.c create mode 100755 t/t7418-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?
[PATCH v7 09/10] submodule: support reading .gitmodules when it's not in the working tree
When the .gitmodules file is not available in the working tree, try
using the content from the index and 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.
Add a similar check also in git-submodule.sh::cmd_add() to anticipate
the eventual failure of the "git submodule add" command when .gitmodules
is not safely writeable; this prevents the command from leaving the
repository in a spurious state (e.g. the submodule repository was cloned
but .gitmodules was not updated because
config_set_in_gitmodules_file_gently failed).
Moreover, since config_from_gitmodules() now accesses the global object
store, it is necessary to protect all code paths which call the function
against concurrent access to the global object store. Currently this
only happens in builtin/grep.c::grep_submodules(), so call
grep_read_lock() before invoking code involving
config_from_gitmodules().
Finally, add t7418-submodule-sparse-gitmodules.sh to verify that reading
from .gitmodules succeeds and that writing to it fails when the file is
not checked out.
NOTE: there is one rare case where this new feature does not work
properly yet: nested submodules without .gitmodules in their working
tree. This has been documented with a warning and a test_expect_failure
item in t7814, and in this case the current behavior is not altered: no
config is read.
Signed-off-by: Antonio Ospite
---
builtin/grep.c | 17 +++-
builtin/submodule--helper.c| 6 +-
git-submodule.sh | 5 +
submodule-config.c | 31 ++-
t/t7411-submodule-config.sh| 26 +-
t/t7418-submodule-sparse-gitmodules.sh | 122 +
t/t7814-grep-recurse-submodules.sh | 16
7 files changed, 216 insertions(+), 7 deletions(-)
create mode 100755 t/t7418-submodule-sparse-gitmodules.sh
diff --git a/builtin/grep.c b/builtin/grep.c
index d8508ddf79..56e4a11052 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -422,11 +422,23 @@ static int grep_submodule(struct grep_opt *opt, struct
repository *superproject,
struct repository submodule;
int hit;
- if (!is_submodule_active(superproject, path))
+ /*
+* NEEDSWORK: submodules functions need to be protected because they
+* access the object store via config_from_gitmodules(): the latter
+* uses get_oid() which, for now, relies on the global the_repository
+* object.
+*/
+ grep_read_lock();
+
+ if (!is_submodule_active(superproject, path)) {
+ grep_read_unlock();
return 0;
+ }
- if (repo_submodule_init(, superproject, path))
+ if (repo_submodule_init(, superproject, path)) {
+ grep_read_unlock();
return 0;
+ }
repo_read_gitmodules();
@@ -440,7 +452,6 @@ static int grep_submodule(struct grep_opt *opt, struct
repository *superproject,
* store is no longer global and instead is a member of the repository
* object.
*/
- grep_read_lock();
add_to_alternates_memory(submodule.objects->objectdir);
grep_read_unlock();
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index b272a78801..05ce666142 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2155,8 +2155,12 @@ static int module_config(int argc, const char **argv,
const char *prefix)
return print_config_from_gitmodules(the_repository, argv[1]);
/* Equivalent to ACTION_SET in builtin/config.c */
- if (argc == 3)
+ if (argc == 3) {
+ if (!is_writing_gitmodules_ok())
+ die(_("please make sure that the .gitmodules file is in
the working tree"));
+
return config_set_in_gitmodules_file_gently(argv[1], argv[2]);
+ }
usage_with_options(git_submodule_helper_usage, module_config_options);
}
diff --git a/git-submodule.sh b/git-submodule.sh
index 0805fadf47..f5124bbf23 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -159,6 +159,11 @@ cmd_add()
shift
done
+ if ! git submodule--helper config --check-writeable >/dev/null 2>&1
+ then
+die "$(eval_gettext "please make sure that the .gitmodules
file is in the working tree")"
+ fi
+
if test -n "$reference_path"
then
is_absolute_path "$reference_path" ||
diff --git a/su
[PATCH v7 05/10] 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
---
builtin/submodule--helper.c | 14 ++
t/t7411-submodule-config.sh | 27 +++
2 files changed, 41 insertions(+)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 80474c3ff5..9af6626e32 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2126,6 +2126,19 @@ static int check_name(int argc, const char **argv, const
char *prefix)
return 0;
}
+static int module_config(int argc, const char **argv, const char *prefix)
+{
+ /* Equivalent to ACTION_GET in builtin/config.c */
+ if (argc == 2)
+ return print_config_from_gitmodules(the_repository, argv[1]);
+
+ /* Equivalent to ACTION_SET in builtin/config.c */
+ if (argc == 3)
+ return config_set_in_gitmodules_file_gently(argv[1], argv[2]);
+
+ die("submodule--helper config takes 1 or 2 arguments: name [value]");
+}
+
#define SUPPORT_SUPER_PREFIX (1<<0)
struct cmd_struct {
@@ -2155,6 +2168,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 b1f3c6489b..791245f18d 100755
--- a/t/t7411-submodule-config.sh
+++ b/t/t7411-submodule-config.sh
@@ -134,4 +134,31 @@ test_expect_success 'error in history in
fetchrecursesubmodule lets continue' '
)
'
+test_expect_success 'reading submodules config with "submodule--helper
config"' '
+ (cd super &&
+ echo "../submodule" >expect &&
+ git submodule--helper config submodule.submodule.url >actual &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'writing submodules config with "submodule--helper
config"' '
+ (cd super &&
+ echo "new_url" >expect &&
+ git submodule--helper config submodule.submodule.url "new_url"
&&
+ git submodule--helper config submodule.submodule.url >actual &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'overwriting unstaged submodules config with
"submodule--helper config"' '
+ test_when_finished "git -C super checkout .gitmodules" &&
+ (cd super &&
+ echo "newer_url" >expect &&
+ git submodule--helper config submodule.submodule.url
"newer_url" &&
+ git submodule--helper config submodule.submodule.url >actual &&
+ test_cmp expect actual
+ )
+'
+
test_done
--
2.19.1
Re: [PATCH v6 00/10] Make submodules work if .gitmodules is not checked out
On Thu, 25 Oct 2018 17:40:47 +0900 Junio C Hamano wrote: > Antonio Ospite writes: > > > this series teaches git to try and read the .gitmodules file from the > > index (:.gitmodules) or from the current branch (HEAD:.gitmodules) when > > the file is not readily available in the working tree. > > What you said in [*1*] the discussion on [09/10] sounded like you > are preparing an update of the series, so the topic is marked as > "Expecting a reroll" in the recent "What's cooking" report. At > least one topic now depends on the enhancement this topic makes, so > I'd like to know what the current status and ETA of the reroll would > be, in order to sort-of act as a traffic cop. > Hi Junio, I can send a v7 later today. It will only contain the improvements to 7416-submodule-sparse-gitmodules.sh as discussed in [*1*], it won't contain changes to patch 8 as motivated in https://public-inbox.org/git/20181008143709.dfcc845ab393c9caea660...@ao2.it/ I will also leave patch 10 unchanged, improvements can be made in follow-up patches. BTW, what is the new topic which depends on this one? Thank you, Antonio > [Reference] > > *1* > http://public-inbox.org/git/20181010205645.e1529eff9099805029b1d...@ao2.it/ -- 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?
Re: [PATCH v6 09/10] submodule: support reading .gitmodules when it's not in the working tree
On Mon, 8 Oct 2018 15:19:00 -0700 Stefan Beller wrote: > > +test_expect_success 'not writing gitmodules config file when it is not > > checked out' ' > > +test_must_fail git -C super submodule--helper config > > submodule.submodule.url newurl > > This only checks the exit code, do we also want to check for > > test_path_is_missing .gitmodules ? > OK, I agree, let's re-check also *after* we tried and failed to set a config value, just to be sure that the code does not get accidentally changed in the future to create the file. I'll add the check. > > +test_expect_success 'initialising submodule when the gitmodules config is > > not checked out' ' > > + git -C super submodule init > > +' > > + > > +test_expect_success 'showing submodule summary when the gitmodules config > > is not checked out' ' > > + git -C super submodule summary > > +' > > Same for these, is the exit code enough, or do we want to look at > specific things? > Except for the "summary" test which was not even exercising the config_from_gitmodule path, checking exist status should be sufficient to verify that "submodule--helper config" does not fail, but we can surely do better. I will add checks to confirm that not only the commands exited without errors but they also achieved the desired effect, to validate the actual high-level use case advertised by the test file. This should be more future-proof. And I think I'll merge the summary and the update tests. > > + > > +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" > > + ) && > > + git -C super submodule update > > git status would want to be clean afterwards? Mmh, this should have been "submodule update --remote" in the first place to have any effect, I'll take the chance and rewrite this test in a different way and also check the effect of the update operation, and the repository status. I'll be something like this: ORIG_SUBMODULE=$(git -C submodule rev-parse HEAD) ORIG_UPSTREAM=$(git -C upstream rev-parse HEAD) ORIG_SUPER=$(git -C super rev-parse HEAD) test_expect_success 're-updating submodule when the gitmodules config is not checked out' ' test_when_finished "git -C submodule reset --hard $ORIG_SUBMODULE; git -C upstream reset --hard $ORIG_UPSTREAM; git -C super reset --hard $ORIG_SUPER; git -C upstream submodule update --remote; git -C super pull; git -C super submodule update --remote" && (cd submodule && echo file2 >file2 && git add file2 && test_tick && git commit -m "add file2 to submodule" ) && (cd upstream && git submodule update --remote && git add submodule && test_tick && git commit -m "Update submodule" ) && git -C super pull && # The --for-status options reads the gitmdoules config git -C super submodule summary --for-status >actual && cat >expect <<-\EOF && * submodule 951c301...a939200 (1): < add file2 to submodule EOF test_cmp expect actual && # Test that the update actually succeeds test_path_is_missing super/submodule/file2 && git -C super submodule update && test_cmp submodule/file2 super/submodule/file2 && git -C super status --short >output && test_must_be_empty output ' Maybe a little overkill? The "upstream" repo will be added in test 1 to better clarify the roles of the involved repositories. The commit ids should be stable because of test_tick, shouldn't they? Thanks for the comments, they helped improving the quality of the tests once again. I'll wait a few days before sending a v7, hopefully someone will find time to take another look at patch 9 and comment also on patch 10, and give an opinion on the "mergeability" status of the whole patchset. Ciao 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?
Re: [PATCH v6 08/10] submodule: add a helper to check if it is safe to write to .gitmodules
On Sun, 07 Oct 2018 08:44:20 +0900
Junio C Hamano wrote:
> Stefan Beller writes:
>
> >> static int module_config(int argc, const char **argv, const char *prefix)
> >> {
> >> + enum {
> >> + CHECK_WRITEABLE = 1
> >> + } command = 0;
> >
> > Can we have the default named? Then we would only use states
> > from within the enum?
>
> Why? Do we use a half-intelligent "switch () { case ...: ... }"
> checker that would otherwise complain if we handled "case 0" in such
> a switch statement, or something like that?
>
> Are we going to gain a lot more enum members, by the way? At this
> point, this looks more like a
>
> unsigned check_writable = 0; /* default is not to check */
>
> to me.
Hi,
the CHECK_WRITEABLE operation is alternative to the get/set ones, not
an addition, so I can see the rationale behind Stefan's suggestion:
either have named enums members for all command "modes" or for none of
them; however other users of enum+OPT_CMDMODE seems to think like the
enum is for commands passed as *options* and the unnamed default is for
actions derived from *arguments*. I don't have a strong opinion on this
matter, tho, so just tell me what you prefer and I'll do it for v7.
Using an enum was to have a more explicit syntax in case other commands
were going to be added in the future (I imagine "--stage" or
"--list-all" as possible additions), and does not affect the generated
code, so I though it was worth it.
Anyways, these are really details, let's concentrate on patches 9 and
10 which deserve much more attention. :)
Thanks 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?
Re: [PATCH v6 00/10] Make submodules work if .gitmodules is not checked out
On Fri, 5 Oct 2018 15:05:51 +0200 Antonio Ospite wrote: [...] > t/t7416-submodule-sparse-gitmodules.sh | 78 ++ > 16 files changed, 410 insertions(+), 32 deletions(-) > create mode 100755 t/t7416-submodule-sparse-gitmodules.sh I just saw that t7416 and t7417 have been added in the latest stable release, I'll wait some days before sending a v7 which renames the newly added test to t/t7418-submodule-sparse-gitmodules.sh 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?
Re: [PATCH v6 08/10] submodule: add a helper to check if it is safe to write to .gitmodules
On Fri, 5 Oct 2018 16:50:10 -0700
Stefan Beller wrote:
> > static int module_config(int argc, const char **argv, const char *prefix)
> > {
> > + enum {
> > + CHECK_WRITEABLE = 1
> > + } command = 0;
>
> Can we have the default named? Then we would only use states
> from within the enum?
The default would mean:
"no command passed as a CLI *option*"
I copied this style from builtin/bisect--helper.c::cmd_bisect__helper()
and it's also used in builtin/rebase--helper.c
I can add a name for the default enum value but I am not sure what it
should be: NO_COMMAND_OPTION, COMMAND_DEFAULT, MODE_DEFAULT?
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?
Re: [PATCH] builtin/grep.c: remote superflous submodule code
On Fri, 5 Oct 2018 15:45:57 -0700 Stefan Beller wrote: > In f9ee2fcdfa (grep: recurse in-process using 'struct repository', > 2017-08-02), we introduced a call to repo_read_gitmodules in builtin/grep > to simplify the submodule handling. > > After ff6f1f564c4 (submodule-config: lazy-load a repository's .gitmodules > file, 2017-08-03) this is no longer necessary, but that commit did not > cleanup the whole tree, but just show cased the new way how to deal with > submodules in ls-files. > > Cleanup the only remaining caller to repo_read_gitmodules outside of > submodule.c > > Signed-off-by: Stefan Beller Not sure if I am entitled to formally ack it, but: Acked-by: Antonio Ospite > --- > > Antonio Ospite writes: > > BTW, with Stefan Beller we also identified some unneeded code which > > could have been removed to alleviate the issue, but that would not have > > solved it completely; so, I am not removing the unnecessary call to > > repo_read_gitmodules() builtin/grep.c in this series, possibly this can > > become a stand-alone change. > > Here is the stand-alone change. > Thank you for sending it. 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?
[PATCH v6 09/10] submodule: support reading .gitmodules when it's not in the working tree
When the .gitmodules file is not available in the working tree, try
using the content from the index and 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.
Add a similar check also in git-submodule.sh::cmd_add() to anticipate
the eventual failure of the "git submodule add" command when .gitmodules
is not safely writeable; this prevents the command from leaving the
repository in a spurious state (e.g. the submodule repository was cloned
but .gitmodules was not updated because
config_set_in_gitmodules_file_gently failed).
Moreover, since config_from_gitmodules() now accesses the global object
store, it is necessary to protect all code paths which call the function
against concurrent access to the global object store. Currently this
only happens in builtin/grep.c::grep_submodules(), so call
grep_read_lock() before invoking code involving
config_from_gitmodules().
Finally, add t7416-submodule-sparse-gitmodules.sh to verify that reading
from .gitmodules succeeds and that writing to it fails when the file is
not checked out.
NOTE: there is one rare case where this new feature does not work
properly yet: nested submodules without .gitmodules in their working
tree. This has been documented with a warning and a test_expect_failure
item in t7814, and in this case the current behavior is not altered: no
config is read.
Signed-off-by: Antonio Ospite
---
builtin/grep.c | 17 +-
builtin/submodule--helper.c| 6 +-
git-submodule.sh | 5 ++
submodule-config.c | 31 +-
t/t7411-submodule-config.sh| 26 -
t/t7416-submodule-sparse-gitmodules.sh | 78 ++
t/t7814-grep-recurse-submodules.sh | 16 ++
7 files changed, 172 insertions(+), 7 deletions(-)
create mode 100755 t/t7416-submodule-sparse-gitmodules.sh
diff --git a/builtin/grep.c b/builtin/grep.c
index 601f801158..7da8fef31a 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -421,11 +421,23 @@ static int grep_submodule(struct grep_opt *opt, struct
repository *superproject,
struct repository submodule;
int hit;
- if (!is_submodule_active(superproject, path))
+ /*
+* NEEDSWORK: submodules functions need to be protected because they
+* access the object store via config_from_gitmodules(): the latter
+* uses get_oid() which, for now, relies on the global the_repository
+* object.
+*/
+ grep_read_lock();
+
+ if (!is_submodule_active(superproject, path)) {
+ grep_read_unlock();
return 0;
+ }
- if (repo_submodule_init(, superproject, path))
+ if (repo_submodule_init(, superproject, path)) {
+ grep_read_unlock();
return 0;
+ }
repo_read_gitmodules();
@@ -439,7 +451,6 @@ static int grep_submodule(struct grep_opt *opt, struct
repository *superproject,
* store is no longer global and instead is a member of the repository
* object.
*/
- grep_read_lock();
add_to_alternates_memory(submodule.objects->objectdir);
grep_read_unlock();
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 28f3ccca6d..5f8a804a6e 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2154,8 +2154,12 @@ static int module_config(int argc, const char **argv,
const char *prefix)
return print_config_from_gitmodules(the_repository, argv[1]);
/* Equivalent to ACTION_SET in builtin/config.c */
- if (argc == 3)
+ if (argc == 3) {
+ if (!is_writing_gitmodules_ok())
+ die(_("please make sure that the .gitmodules file is in
the working tree"));
+
return config_set_in_gitmodules_file_gently(argv[1], argv[2]);
+ }
usage_with_options(git_submodule_helper_usage, module_config_options);
}
diff --git a/git-submodule.sh b/git-submodule.sh
index 0805fadf47..f5124bbf23 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -159,6 +159,11 @@ cmd_add()
shift
done
+ if ! git submodule--helper config --check-writeable >/dev/null 2>&1
+ then
+die "$(eval_gettext "please make sure that the .gitmodules
file is in the working tree")"
+ fi
+
if test -n "$reference_path"
then
is_absolute_path "$reference_path" ||
diff --git a/su
[PATCH v6 08/10] submodule: add a helper to check if it is safe to write to .gitmodules
Introduce a helper function named is_writing_gitmodules_ok() to verify
that the .gitmodules file is safe to write.
The function name follows the scheme of is_staging_gitmodules_ok().
The two symbolic constants GITMODULES_INDEX and GITMODULES_HEAD are used
to get help from the C preprocessor in preventing typos, especially for
future users.
This is in preparation for a future change which teaches git how to read
.gitmodules from the index or from the current branch if the file is not
available in the working tree.
The rationale behind the check is that writing to .gitmodules requires
the file to be present in the working tree, unless a brand new
.gitmodules is being created (in which case the .gitmodules file would
not exist at all: neither in the working tree nor in the index or in the
current branch).
Expose the functionality also via a "submodule-helper config
--check-writeable" command, as git scripts may want to perform the check
before modifying submodules configuration.
Signed-off-by: Antonio Ospite
---
builtin/submodule--helper.c | 24 +++-
cache.h | 2 ++
submodule.c | 18 ++
submodule.h | 1 +
t/t7411-submodule-config.sh | 31 +++
5 files changed, 75 insertions(+), 1 deletion(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index e1bdca8f0b..28f3ccca6d 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2127,6 +2127,28 @@ static int check_name(int argc, const char **argv, const
char *prefix)
static int module_config(int argc, const char **argv, const char *prefix)
{
+ enum {
+ CHECK_WRITEABLE = 1
+ } command = 0;
+
+ struct option module_config_options[] = {
+ OPT_CMDMODE(0, "check-writeable", ,
+ N_("check if it is safe to write to the .gitmodules
file"),
+ CHECK_WRITEABLE),
+ OPT_END()
+ };
+ const char *const git_submodule_helper_usage[] = {
+ N_("git submodule--helper config name [value]"),
+ N_("git submodule--helper config --check-writeable"),
+ NULL
+ };
+
+ argc = parse_options(argc, argv, prefix, module_config_options,
+git_submodule_helper_usage, PARSE_OPT_KEEP_ARGV0);
+
+ if (argc == 1 && command == CHECK_WRITEABLE)
+ return is_writing_gitmodules_ok() ? 0 : -1;
+
/* Equivalent to ACTION_GET in builtin/config.c */
if (argc == 2)
return print_config_from_gitmodules(the_repository, argv[1]);
@@ -2135,7 +2157,7 @@ static int module_config(int argc, const char **argv,
const char *prefix)
if (argc == 3)
return config_set_in_gitmodules_file_gently(argv[1], argv[2]);
- die("submodule--helper config takes 1 or 2 arguments: name [value]");
+ usage_with_options(git_submodule_helper_usage, module_config_options);
}
#define SUPPORT_SUPER_PREFIX (1<<0)
diff --git a/cache.h b/cache.h
index d508f3d4f8..2d495fc800 100644
--- a/cache.h
+++ b/cache.h
@@ -486,6 +486,8 @@ 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_INDEX ":.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.c b/submodule.c
index 3b23e76e55..bd2506c5ba 100644
--- a/submodule.c
+++ b/submodule.c
@@ -51,6 +51,24 @@ int is_gitmodules_unmerged(const struct index_state *istate)
return 0;
}
+/*
+ * Check if the .gitmodules file is safe to write.
+ *
+ * Writing to the .gitmodules file requires that the file exists in the
+ * working tree or, if it doesn't, that a brand new .gitmodules file is going
+ * to be created (i.e. it's neither in the index nor in the current branch).
+ *
+ * It is not safe to write to .gitmodules if it's not in the working tree but
+ * it is in the index or in the current branch, because writing new values
+ * (and staging them) would blindly overwrite ALL the old content.
+ */
+int is_writing_gitmodules_ok(void)
+{
+ struct object_id oid;
+ return file_exists(GITMODULES_FILE) ||
+ (get_oid(GITMODULES_INDEX, ) < 0 &&
get_oid(GITMODULES_HEAD, ) < 0);
+}
+
/*
* Check if the .gitmodules file has unstaged modifications. This must be
* checked before allowing modifications to the .gitmodules file with the
diff --git a/submodule.h b/submodule.h
index e452919aa4..7a22f71cb9 1006
[PATCH v6 05/10] 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
---
builtin/submodule--helper.c | 14 ++
t/t7411-submodule-config.sh | 27 +++
2 files changed, 41 insertions(+)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 40844870cf..e1bdca8f0b 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2125,6 +2125,19 @@ static int check_name(int argc, const char **argv, const
char *prefix)
return 0;
}
+static int module_config(int argc, const char **argv, const char *prefix)
+{
+ /* Equivalent to ACTION_GET in builtin/config.c */
+ if (argc == 2)
+ return print_config_from_gitmodules(the_repository, argv[1]);
+
+ /* Equivalent to ACTION_SET in builtin/config.c */
+ if (argc == 3)
+ return config_set_in_gitmodules_file_gently(argv[1], argv[2]);
+
+ die("submodule--helper config takes 1 or 2 arguments: name [value]");
+}
+
#define SUPPORT_SUPER_PREFIX (1<<0)
struct cmd_struct {
@@ -2154,6 +2167,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 b1f3c6489b..791245f18d 100755
--- a/t/t7411-submodule-config.sh
+++ b/t/t7411-submodule-config.sh
@@ -134,4 +134,31 @@ test_expect_success 'error in history in
fetchrecursesubmodule lets continue' '
)
'
+test_expect_success 'reading submodules config with "submodule--helper
config"' '
+ (cd super &&
+ echo "../submodule" >expect &&
+ git submodule--helper config submodule.submodule.url >actual &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'writing submodules config with "submodule--helper
config"' '
+ (cd super &&
+ echo "new_url" >expect &&
+ git submodule--helper config submodule.submodule.url "new_url"
&&
+ git submodule--helper config submodule.submodule.url >actual &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'overwriting unstaged submodules config with
"submodule--helper config"' '
+ test_when_finished "git -C super checkout .gitmodules" &&
+ (cd super &&
+ echo "newer_url" >expect &&
+ git submodule--helper config submodule.submodule.url
"newer_url" &&
+ git submodule--helper config submodule.submodule.url >actual &&
+ test_cmp expect actual
+ )
+'
+
test_done
--
2.19.0
[PATCH v6 10/10] t/helper: add test-submodule-nested-repo-config
Add a test tool to exercise config_from_gitmodules(), in particular for
the case of nested submodules.
Add also a test to document that reading the submoudles config of nested
submodules does not work yet when the .gitmodules file is not in the
working tree but it still in the index.
This is because the git API does not always make it possible access the
object store of an arbitrary repository (see get_oid() usage in
config_from_gitmodules()).
When this git limitation gets fixed the aforementioned use case will be
supported too.
Signed-off-by: Antonio Ospite
---
Makefile | 1 +
t/helper/test-submodule-nested-repo-config.c | 30 +
t/helper/test-tool.c | 1 +
t/helper/test-tool.h | 1 +
t/t7411-submodule-config.sh | 34
5 files changed, 67 insertions(+)
create mode 100644 t/helper/test-submodule-nested-repo-config.c
diff --git a/Makefile b/Makefile
index 13e1c52478..fe8587cd8c 100644
--- a/Makefile
+++ b/Makefile
@@ -737,6 +737,7 @@ TEST_BUILTINS_OBJS += test-sigchain.o
TEST_BUILTINS_OBJS += test-strcmp-offset.o
TEST_BUILTINS_OBJS += test-string-list.o
TEST_BUILTINS_OBJS += test-submodule-config.o
+TEST_BUILTINS_OBJS += test-submodule-nested-repo-config.o
TEST_BUILTINS_OBJS += test-subprocess.o
TEST_BUILTINS_OBJS += test-urlmatch-normalization.o
TEST_BUILTINS_OBJS += test-wildmatch.o
diff --git a/t/helper/test-submodule-nested-repo-config.c
b/t/helper/test-submodule-nested-repo-config.c
new file mode 100644
index 00..a31e2a9bea
--- /dev/null
+++ b/t/helper/test-submodule-nested-repo-config.c
@@ -0,0 +1,30 @@
+#include "test-tool.h"
+#include "submodule-config.h"
+
+static void die_usage(int argc, const char **argv, const char *msg)
+{
+ fprintf(stderr, "%s\n", msg);
+ fprintf(stderr, "Usage: %s \n", argv[0]);
+ exit(1);
+}
+
+int cmd__submodule_nested_repo_config(int argc, const char **argv)
+{
+ struct repository submodule;
+
+ if (argc < 3)
+ die_usage(argc, argv, "Wrong number of arguments.");
+
+ setup_git_directory();
+
+ if (repo_submodule_init(, the_repository, argv[1])) {
+ die_usage(argc, argv, "Submodule not found.");
+ }
+
+ /* Read the config of _child_ submodules. */
+ print_config_from_gitmodules(, argv[2]);
+
+ submodule_free(the_repository);
+
+ return 0;
+}
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index b87a8c1f22..3b473bccd8 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -42,6 +42,7 @@ static struct test_cmd cmds[] = {
{ "strcmp-offset", cmd__strcmp_offset },
{ "string-list", cmd__string_list },
{ "submodule-config", cmd__submodule_config },
+ { "submodule-nested-repo-config", cmd__submodule_nested_repo_config },
{ "subprocess", cmd__subprocess },
{ "urlmatch-normalization", cmd__urlmatch_normalization },
{ "wildmatch", cmd__wildmatch },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index e074957279..3ca351230c 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -38,6 +38,7 @@ int cmd__sigchain(int argc, const char **argv);
int cmd__strcmp_offset(int argc, const char **argv);
int cmd__string_list(int argc, const char **argv);
int cmd__submodule_config(int argc, const char **argv);
+int cmd__submodule_nested_repo_config(int argc, const char **argv);
int cmd__subprocess(int argc, const char **argv);
int cmd__urlmatch_normalization(int argc, const char **argv);
int cmd__wildmatch(int argc, const char **argv);
diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
index 2cfabb18bc..89690b7adb 100755
--- a/t/t7411-submodule-config.sh
+++ b/t/t7411-submodule-config.sh
@@ -216,4 +216,38 @@ test_expect_success 'reading submodules config from the
current branch when .git
)
'
+test_expect_success 'reading nested submodules config' '
+ (cd super &&
+ git init submodule/nested_submodule &&
+ echo "a" >submodule/nested_submodule/a &&
+ git -C submodule/nested_submodule add a &&
+ git -C submodule/nested_submodule commit -m "add a" &&
+ git -C submodule submodule add ./nested_submodule &&
+ git -C submodule add nested_submodule &&
+ git -C submodule commit -m "added nested_submodule" &&
+ git add submodule &&
+ git commit -m "updated submodule" &&
+ echo "./nested_submodule" >expect &&
+ test-tool submodule-nested-repo-config \
+ submod
[PATCH v6 02/10] 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
---
submodule-config.c | 12
submodule-config.h | 1 +
submodule.c| 10 +++---
3 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/submodule-config.c b/submodule-config.c
index 823bc76812..8659d97e06 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -707,6 +707,18 @@ int print_config_from_gitmodules(struct repository *repo,
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 031747ccf8..4dc9b0771c 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -49,6 +49,7 @@ const struct submodule *submodule_from_path(struct repository
*r,
const char *path);
void submodule_free(struct repository *r);
int print_config_from_gitmodules(struct repository *repo, const char *key);
+int config_set_in_gitmodules_file_gently(const char *key, const char *value);
/*
* Returns 0 if the name is syntactically acceptable as a submodule "name"
diff --git a/submodule.c b/submodule.c
index b53cb6e9c4..3b23e76e55 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(, "submodule.");
strbuf_addstr(, submodule->name);
strbuf_addstr(, ".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();
- return -1;
- }
+ ret = config_set_in_gitmodules_file_gently(entry.buf, newpath);
strbuf_release();
- return 0;
+ return ret;
}
/*
--
2.19.0
[PATCH v6 04/10] t7411: be nicer to future tests and really clean things up
Tests 5 and 7 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. Remove both the commits as soon as they are not needed anymore. Signed-off-by: Antonio Ospite --- t/t7411-submodule-config.sh | 7 +-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh index f2cd1f4a2c..b1f3c6489b 100755 --- a/t/t7411-submodule-config.sh +++ b/t/t7411-submodule-config.sh @@ -83,6 +83,8 @@ Submodule name: 'submodule' for path 'submodule' EOF test_expect_success 'error in history of one submodule config lets continue, stderr message contains blob ref' ' + ORIG=$(git -C super rev-parse HEAD) && + test_when_finished "git -C super reset --hard $ORIG" && (cd super && cp .gitmodules .gitmodules.bak && echo " value = \"" >>.gitmodules && @@ -115,6 +117,8 @@ test_expect_success 'using different treeishs works' ' ' test_expect_success 'error in history in fetchrecursesubmodule lets continue' ' + ORIG=$(git -C super rev-parse HEAD) && + test_when_finished "git -C super reset --hard $ORIG" && (cd super && git config -f .gitmodules \ submodule.submodule.fetchrecursesubmodules blabla && @@ -126,8 +130,7 @@ test_expect_success 'error in history in fetchrecursesubmodule lets continue' ' HEAD b \ HEAD submodule \ >actual && - test_cmp expect_error actual && - git reset --hard HEAD^ + test_cmp expect_error actual ) ' -- 2.19.0
[PATCH v6 01/10] submodule: add a print_config_from_gitmodules() helper
Add a new print_config_from_gitmodules() helper function to print values
from .gitmodules just like "git config -f .gitmodules" would.
This will be used by a new submodule--helper subcommand to be able to
access the .gitmodules file in a more controlled way.
Signed-off-by: Antonio Ospite
---
submodule-config.c | 25 +
submodule-config.h | 1 +
2 files changed, 26 insertions(+)
diff --git a/submodule-config.c b/submodule-config.c
index e04ba756d9..823bc76812 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 *var, const char *value, void
*cb_data)
+{
+ char *wanted_key = cb_data;
+
+ if (!strcmp(wanted_key, var))
+ printf("%s\n", value);
+
+ return 0;
+}
+
+int print_config_from_gitmodules(struct repository *repo, const char *key)
+{
+ int ret;
+ char *store_key;
+
+ ret = git_config_parse_key(key, _key, NULL);
+ if (ret < 0)
+ return CONFIG_INVALID_KEY;
+
+ config_from_gitmodules(config_print_callback, repo, 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..031747ccf8 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -48,6 +48,7 @@ const struct submodule *submodule_from_path(struct repository
*r,
const struct object_id
*commit_or_tree,
const char *path);
void submodule_free(struct repository *r);
+int print_config_from_gitmodules(struct repository *repo, const char *key);
/*
* Returns 0 if the name is syntactically acceptable as a submodule "name"
--
2.19.0
[PATCH v6 07/10] t7506: clean up .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 intends 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 --- 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 943708fb04..08629a6e70 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.19.0
[PATCH v6 06/10] 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
---
git-submodule.sh | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/git-submodule.sh b/git-submodule.sh
index 1b568e29b9..0805fadf47 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.19.0
[PATCH v6 00/10] Make submodules work if .gitmodules is not checked out
Hi, this series teaches git to try and read the .gitmodules file from the index (:.gitmodules) or from the current branch (HEAD:.gitmodules) when the file is not readily available in the working tree. This can be used, together with sparse checkouts, to enable submodule usage with programs like vcsh[1] which manage multiple repositories with their working trees sharing the same path. [1] https://github.com/RichiH/vcsh Thanks to SZEDER Gábor we found out that the changes in patch 9 could allow to access the object store in an inconsistent way when using multi-threading in "git grep --recurse-submodules", this has been dealt with in this revision. BTW, with Stefan Beller we also identified some unneeded code which could have been removed to alleviate the issue, but that would not have solved it completely; so, I am not removing the unnecessary call to repo_read_gitmodules() builtin/grep.c in this series, possibly this can become a stand-alone change. The problems from above also made me investigate what implications the current use of a global object store had on my new addition, and now I know that there is one case which I cannot support just yet: nested submodules without .gitmodules in their working tree. This case has been documented with a warning and test_expect_failure items in tests, and hitting the unsupported case does not alter the current behavior of git. Apart form patch 9 and 10 there are no major changes to the previous iteration. IMHO we are in a place where the problem has been analyzed with enough depth, the limitations have been identified and dealt with in a way that should not affect current users nor diminish the usefulness of the new code. v5 of the series is here: https://public-inbox.org/git/20180917140940.3839-1-...@ao2.it/ v4 of the series is here: https://public-inbox.org/git/20180824132951.8000-1-...@ao2.it/ v3 of the series is here: https://public-inbox.org/git/20180814110525.17801-1-...@ao2.it/ v2 of the series is here: https://public-inbox.org/git/20180802134634.10300-1-...@ao2.it/ v1 of the series, with some background info, is here: https://public-inbox.org/git/20180514105823.8378-1-...@ao2.it/ Changes since v5: * print_config_from_gitmodules() in patch 1 now accepts a struct repository argument. * submodule--helper config in patch 5 has been adjusted to use the new signature of print_config_from_gitmodules(). * In patch 9 the grep read lock in builtin/grep.c now covers all code paths involving config_from_gitmodules(). FTR git-grep is the only place where config_from_gitmodules() is called from multi-threaded code. * Patch 9 also documents the rare case that cannot be supported just yet, and adds a warning to the user. * In patch 9, config_from_gitmodules() now does not read any config when the config source is not specified.(I added a catch-all "else" block) This match more closely the behavior of the old code using git_config_from_file. * Added a new test tool in patch 10 to exercise config_read_config() in a more direct way, passing an arbitrary repository. Admittedly, patch 10 performs a similar test to the one added to t7814 in patch 9, so I'd be OK with dropping patch 10 if it is too specific. Thank you, Antonio Antonio Ospite (10): submodule: add a print_config_from_gitmodules() helper submodule: factor out a config_set_in_gitmodules_file_gently function t7411: merge tests 5 and 6 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 t7506: clean up .gitmodules properly before setting up new scenario submodule: add a helper to check if it is safe to write to .gitmodules submodule: support reading .gitmodules when it's not in the working tree t/helper: add test-submodule-nested-repo-config Makefile | 1 + builtin/grep.c | 17 ++- builtin/submodule--helper.c | 40 ++ cache.h | 2 + git-submodule.sh | 13 +- submodule-config.c | 68 - submodule-config.h | 2 + submodule.c | 28 +++- submodule.h | 1 + t/helper/test-submodule-nested-repo-config.c | 30 t/helper/test-tool.c | 1 + t/helper/test-tool.h | 1 + t/t7411-submodule-config.sh | 141 +-- t/t7416-submodule-sparse-gitmodules.sh | 78 ++ t/t7506-status-submodule.sh | 3 +- t/t7814-grep-recurse-submodules.sh | 16 +++ 16 files changed, 410 insertions(+), 32 deletions(-) create mode 100644 t/helper/test-submodule-nested-repo-config.c create
[PATCH v6 03/10] t7411: merge tests 5 and 6
Tests 5 and 6 check for the effects of the same commit, merge the two tests to make it more straightforward to clean things up after the test has finished. The cleanup will be added in a future commit. Signed-off-by: Antonio Ospite --- t/t7411-submodule-config.sh | 18 +- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh index 0bde5850ac..f2cd1f4a2c 100755 --- a/t/t7411-submodule-config.sh +++ b/t/t7411-submodule-config.sh @@ -82,29 +82,21 @@ Submodule name: 'a' for path 'b' Submodule name: 'submodule' for path 'submodule' EOF -test_expect_success 'error in one submodule config lets continue' ' +test_expect_success 'error in history of one submodule config lets continue, stderr message contains blob ref' ' (cd super && cp .gitmodules .gitmodules.bak && echo " value = \"" >>.gitmodules && git add .gitmodules && mv .gitmodules.bak .gitmodules && git commit -m "add error" && - test-tool submodule-config \ - HEAD b \ - HEAD submodule \ - >actual && - test_cmp expect_error actual - ) -' - -test_expect_success 'error message contains blob reference' ' - (cd super && sha1=$(git rev-parse HEAD) && test-tool submodule-config \ HEAD b \ HEAD submodule \ - 2>actual_err && - test_i18ngrep "submodule-blob $sha1:.gitmodules" actual_err >/dev/null + >actual \ + 2>actual_stderr && + test_cmp expect_error actual && + test_i18ngrep "submodule-blob $sha1:.gitmodules" actual_stderr >/dev/null ) ' -- 2.19.0
Re: [PATCH v5 9/9] submodule: support reading .gitmodules when it's not in the working tree
On Thu, 27 Sep 2018 11:00:52 -0700 Stefan Beller wrote: > On Thu, Sep 27, 2018 at 7:44 AM Antonio Ospite wrote: [...] > > OK, so the plan for v6 is: > > > > - avoid the corruption issues spotted by Gábor by removing the call > > to repo_read_gitmodules in builtin/grep.c (this still does not fix > > the potential problem with nested submodules). > > Actually that is not enough to fix the inconsistent access to the object store: the functions is_submodule_active() and repo_submodule_init() too end up calling config_from_gitmodules() and need protecting as well, so I am going to put them under the git read lock and leave repo_read_gitmodules() in place for now. Removing unneeded code can go in a possible stand-alone patch. > > - add a new test-tool which better exercises the new > > config_from_gitmodules code, > > Sounds good. > > > > > - add also a test_expect_failure test to document the use case that > > cannot be supported yet: nested submodules without .gitmodules in > > their working tree. > > Personally I would want to live in a world where we don't *have* to nor > *want* to support submodules without .gitmodules in the respective > superproject. > Just to double check: are you referring to *nested* submodules in the sentence above? I am asking because the whole point of this patchset is to *enable* the use of submodules without .gitmodules in the working tree of the superproject. :) It's just that current limitations in git do not allow to support this for *nested* submodules yet. > We did support some use cases historically that I would make sure to > continue to support, but I am not sure how much effort we want to spend > on supporting further use cases of incomplete submodules. > > Feel free to do so, as such tests help to document the boundaries. > Let's see how v6 turns out. 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?
Re: [PATCH v5 9/9] submodule: support reading .gitmodules when it's not in the working tree
On Fri, 21 Sep 2018 09:19:45 -0700 Junio C Hamano wrote: > Antonio Ospite writes: > > > Protecting the problematic submodules function could work for now, but > > I'd like to have more comments, my proposal is: > > > > diff --git a/builtin/grep.c b/builtin/grep.c > > index 601f801158..52b45de749 100644 > > --- a/builtin/grep.c > > +++ b/builtin/grep.c > > @@ -427,6 +427,11 @@ static int grep_submodule(struct grep_opt *opt, struct > > repository *superproject, > > if (repo_submodule_init(, superproject, path)) > > return 0; > > > > + grep_read_lock(); > > + /* > > +* NEEDSWORK: repo_read_gitmodules accesses the object store which > > is > > +* global, thus it needs to be protected. > > +*/ > > repo_read_gitmodules(); > > > > /* > > @@ -439,7 +444,6 @@ static int grep_submodule(struct grep_opt *opt, struct > > repository *superproject, > > * store is no longer global and instead is a member of the > > repository > > * object. > > */ > > - grep_read_lock(); > > add_to_alternates_memory(submodule.objects->objectdir); > > grep_read_unlock(); > > I think this is in line with how the grep codepath protects itself > when doing anything that accesses the object store. > Thanks for the comment. However, after confirming with Stefan Beller, I think we are going to solve the corruption issue by removing this call to repo_read_gitmodules(), which is not strictly necessary: https://public-inbox.org/git/CAGZ79kZaomuE3p1puznM1x+hu-w4O+ZqeGUODBDj=-r3z1h...@mail.gmail.com/ 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?
Re: [PATCH v5 9/9] submodule: support reading .gitmodules when it's not in the working tree
Hi Stefan, On Mon, 24 Sep 2018 14:00:50 -0700 Stefan Beller wrote: > On Mon, Sep 24, 2018 at 3:20 AM Antonio Ospite wrote: > [...] > > This is a limitation of the object store in git, there is no equivalent > > of get_oid() to get the oid from a specific repository and this affects > > config_with_options too when the config source is a blob. > > Not yet, as there is a big push to pass-through an object-store object > or similar recently and rely less on global variables. > I am not sure I get to this code, though. > If you end up touching get_oid() please CC me. > > This does not affect commands called via "git -C submodule_dir cmd" > > because in that case the chdir happens before the_repository is set up, > > for instance "git-submodule $SOMETHING --recursive" commands seem to > > change the working directory before the recursion. > > For this it may be worth looking into the option >--super-prefix= > Currently for internal use only. Set a prefix which gives a > path from above a repository down to its root. One use is > to give submodules context about the superproject that > invoked it. > My comment wanted to highlight that there are NO problems in the mentioned cases: - git -C submodule_dir cmd - git submodule cmd --recursive Are you suggesting to look into super-prefix for any reason in particular? [...] > > The test suite passes even after removing repo_read_gitmodules() > > entirely from builtin/grep.c, but I am still not confident that I get > > all the implication of why that call was originally added in commit > > f9ee2fcdfa (grep: recurse in-process using 'struct repository', > > 2017-08-02). > > If you checkout that commit and remove the call to repo_read_gitmodules > and then call git-grep in a superproject with nested submodules, you > get a segfault. > > On master (and deleting out that line) you do not get the segfault, > I think praise goes to ff6f1f564c4 (submodule-config: lazy-load a > repository's .gitmodules file, 2017-08-03) which happened shortly > after f9ee2fcdfa. > > It showcased that it worked by converting ls-files, but left out grep. > > So I think based on ff6f1f564c4 it is safe to remove all calls to > repo_read_gitmodules. > Thanks for confirming. > > Anyways, even if we removed the call we would prevent the problem from > > happening in the test suite, but not in the real world, in case non-leaf > > submodules without .gitmodules in their working tree. > > Quite frankly I think grep was just overlooked in review of > https://public-inbox.org/git/20170803182000.179328-14-bmw...@google.com/ > OK, so the plan for v6 is: - avoid the corruption issues spotted by Gábor by removing the call to repo_read_gitmodules in builtin/grep.c (this still does not fix the potential problem with nested submodules). - add a new test-tool which better exercises the new config_from_gitmodules code, - add also a test_expect_failure test to document the use case that cannot be supported yet: nested submodules without .gitmodules in their working tree. 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?
Re: [PATCH v5 1/9] submodule: add a print_config_from_gitmodules() helper
On Mon, 17 Sep 2018 16:09:32 +0200
Antonio Ospite wrote:
> Add a new print_config_from_gitmodules() helper function to print values
> from .gitmodules just like "git config -f .gitmodules" would.
>
[...]
> +int print_config_from_gitmodules(const char *key)
I am thinking about adding a "struct repository" argument to this
function
> +{
> + int ret;
> + char *store_key;
> +
> + ret = git_config_parse_key(key, _key, NULL);
> + if (ret < 0)
> + return CONFIG_INVALID_KEY;
> +
> + config_from_gitmodules(config_print_callback, the_repository,
> store_key);
And use it here, to avoid another usage of "the_repository" when it's
not strictly necessary.
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?
Re: [PATCH v5 9/9] submodule: support reading .gitmodules when it's not in the working tree
On Tue, 18 Sep 2018 19:12:57 +0200
SZEDER Gábor wrote:
[...]
> On Mon, Sep 17, 2018 at 04:09:40PM +0200, Antonio Ospite wrote:
> > When the .gitmodules file is not available in the working tree, try
> > using the content from the index and from the current branch.
>
> "from the index and from the current branch" of which repository?
>
I took a look, some comments below.
> > diff --git a/submodule-config.c b/submodule-config.c
> > index 61a555e920..bdb1d0e2c9 100644
> > --- a/submodule-config.c
> > +++ b/submodule-config.c
>
> > @@ -603,8 +604,21 @@ static void submodule_cache_check_init(struct
> > repository *repo)
> > static void config_from_gitmodules(config_fn_t fn, struct repository
> > *repo, void *data)
> > {
[...]
> > + file = repo_worktree_path(repo, GITMODULES_FILE);
> > + if (file_exists(file))
> > + config_source.file = file;
> > + else if (get_oid(GITMODULES_INDEX, ) >= 0)
> > + config_source.blob = GITMODULES_INDEX;
> > + else if (get_oid(GITMODULES_HEAD, ) >= 0)
> > + config_source.blob = GITMODULES_HEAD;
> > +
>
> The repository used in t7814 contains nested submodules, which means
> that config_from_gitmodules() is invoked three times.
>
> Now, the first two of those calls look at the superproject and at
> 'submodule', and find the existing files '.../trash
> directory.t7814-grep-recurse-submodules/.gitmodules' and '.../trash
> directory.t7814-grep-recurse-submodules/submodule/.gitmodules',
> respectively. So far so good.
>
> The third call, however, looks at the nested submodule at
> 'submodule/sub', which doesn't contain a '.gitmodules' file. So this
> function goes on with the second condition and calls
> get_oid(GITMODULES_INDEX, ), which then appears to find the blob
> in the _superproject's_ index.
>
You are correct.
This is a limitation of the object store in git, there is no equivalent
of get_oid() to get the oid from a specific repository and this affects
config_with_options too when the config source is a blob.
This does not affect commands called via "git -C submodule_dir cmd"
because in that case the chdir happens before the_repository is set up,
for instance "git-submodule $SOMETHING --recursive" commands seem to
change the working directory before the recursion.
> > + config_with_options(fn, data, _source, );
> > +
> > free(file);
> > }
> > }
> I'm no expert on submodules, but my gut feeling says that this can't
> be right. But if it _is_ right, then I would say that the commit
> message should explain in detail, why it is right.
>
I agree it isn't right, I didn't consider the case of nested
submodules, but even if I did the current git design does not
allow to correctly solve the problem: "read config from blob of an
arbitrary repository".
So what to do for the time being?
The issue is there but in a "normal" scenario it is not causing any real
harm because:
1. currently it is exposed "only" by git grep and nested submodules.
2. the new mechanism works fine when reading the submodule config for
the root repository.
3. the new mechanism does not "usually" impact non-leaf nested
submodules, because the .gitmodules file is "normally" there.
4. git grep never *uses* the GITMODULES_INDEX erroneously read
from the root project when scanning the _leaf_ nested submodule
because there are no further submodules down the line, the
following check fails in builtin/grep.c:
if (recurse_submodules && S_ISGITLINK(entry.mode) ...
...
In fact, because of 4. the test suite passes even if the gitmodule
config is not correct for the leaf submodule.
Actually 4. makes me think that the repo_read_gitmodules() call in
builtin/grep.c might not be strictly necessary, its purpose seems to be
to *anticipate* reading the config of *child* submodules while still
processing the *current* submodule, the config for the *current*
submodule was already read from the superproject by the immediately
preceding repo_submodule_init(), via:
repo_submodule_init()
submodule_from_path()
gitmodules_read_check()
repo_read_gitmodules()
And this would happen anyway also for child submodules down the
recursion path if we removed repo_read_gitmodules() in builtin/grep.c,
the operation would be not protected by grep_read_lock() tho.
The test suite passes even after removing repo_read_gitmodules()
entirely from builtin/grep.c, but I am still not confident that I get
all the implication of why that call was originally added in commit
f9ee2fcdfa (grep: recurse in-process using 'struct repository',
2
Re: [PATCH v5 9/9] submodule: support reading .gitmodules when it's not in the working tree
On Tue, 18 Sep 2018 19:12:57 +0200
SZEDER Gábor wrote:
> Hi Antonio,
>
> it appears that this patch (and its previous versions as well) is
> responsible for triggering occasional test failures in
> 't7814-grep-recurse-submodules.sh', more frequently, about once in
> every ten runs, on macOS on Travis CI, less frequently, about once in
> a couple of hundred runs on Linux on my machine.
>
Thanks a lot for testing Gábor, it's really appreciated.
> The reason for the failure is memory corruption manifesting in various
> ways: segfault, malloc() or use after free() errors from libc, corrupt
> loose object, invalid ref, bogus output, etc.
>
> Applying the following patch makes t7814 fail almost every time,
> though sometimes that loop has to iterate over 1000 times until that
> 'git grep' finally fails... so good luck with debugging ;)
[...]
I managed to capture some traces of the segfaults using this variation:
diff --git a/t/t7814-grep-recurse-submodules.sh
b/t/t7814-grep-recurse-submodules.sh
index 7184113b9b..56e87c3f8a 100755
--- a/t/t7814-grep-recurse-submodules.sh
+++ b/t/t7814-grep-recurse-submodules.sh
@@ -337,6 +337,10 @@ test_expect_success 'grep --recurse-submodules should pass
the pattern type alon
test_must_fail git -c grep.patternType=fixed grep --recurse-submodules
-e "(.|.)[\d]" &&
# Basic
+ for i in $(test_seq 0 2000)
+ do
+ debug --debugger="gdb --silent -ex run -ex quit
--return-child-result --args" git grep --recurse-submodules 1 >/dev/null ||
return 1
+ done &&
git grep -G --recurse-submodules -e "(.|.)[\d]" >actual &&
cat >expect <<-\EOF &&
a:(1|2)d(3|4)
Running t7814 with --run="1,6,22" is enough to observe the issue.
FWICS these corruptions are caused by concurrent accesses to the object
store.
The issue is caused by these facts:
1. git grep uses threads;
2. git grep reads submodules config with repo_read_gitmodules;
3. repo_read_gitmodules calls config_from_gitmodules
4. the changes in patch 9 in this series make config_from_gitmodules
use the object store, which apparently is not mt-safe, while the
previous use of git_config_from_file() was.
> On first look I didn't notice anything that is obviously wrong in this
> patch and could be responsible for the memory corruption, but there is
> one thing I found strange, though:
>
>
> On Mon, Sep 17, 2018 at 04:09:40PM +0200, Antonio Ospite wrote:
> > When the .gitmodules file is not available in the working tree, try
> > using the content from the index and from the current branch.
>
> "from the index and from the current branch" of which repository?
>
[...]
> > diff --git a/submodule-config.c b/submodule-config.c
> > index 61a555e920..bdb1d0e2c9 100644
> > --- a/submodule-config.c
> > +++ b/submodule-config.c
>
> > @@ -603,8 +604,21 @@ 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_INDEX, ) >= 0)
> > + config_source.blob = GITMODULES_INDEX;
>
> The repository used in t7814 contains nested submodules, which means
> that config_from_gitmodules() is invoked three times.
>
> Now, the first two of those calls look at the superproject and at
> 'submodule', and find the existing files '.../trash
> directory.t7814-grep-recurse-submodules/.gitmodules' and '.../trash
> directory.t7814-grep-recurse-submodules/submodule/.gitmodules',
> respectively. So far so good.
>
> The third call, however, looks at the nested submodule at
> 'submodule/sub', which doesn't contain a '.gitmodules' file. So this
> function goes on with the second condition and calls
> get_oid(GITMODULES_INDEX, ), which then appears to find the blob
> in the _superproject's_ index.
>
> I'm no expert on submodules, but my gut feeling says that this can't
> be right. But if it _is_ right, then I would say that the commit
> message should explain in detail, why it is right.
>
I'll think about that too.
> Any
[PATCH v5 0/9] Make submodules work if .gitmodules is not checked out
Hi, this series teaches git to try and read the .gitmodules file from the index (:.gitmodules) or from the current branch (HEAD:.gitmodules) when the file is not readily available in the working tree. This can be used, together with sparse checkouts, to enable submodule usage with programs like vcsh[1] which manage multiple repositories with their working trees sharing the same path. [1] https://github.com/RichiH/vcsh v4 of the series is here: https://public-inbox.org/git/20180824132951.8000-1-...@ao2.it/ v3 of the series is here: https://public-inbox.org/git/20180814110525.17801-1-...@ao2.it/ v2 of the series is here: https://public-inbox.org/git/20180802134634.10300-1-...@ao2.it/ v1 of the series, with some background info, is here: https://public-inbox.org/git/20180514105823.8378-1-...@ao2.it/ v5 only contains some small cosmetic fixes suggested by Ævar Arnfjörð Bjarmason, I ignored the comment about the memory leak in patch 1 because I think there is no leak in how git_config_parse_key is used: https://public-inbox.org/git/87wosfesxl@evledraar.gmail.com/ Changes since v4: * Improve names of local variables in patch 1. * Move new functions definitions in patches 1 and 2 before a pre-existing comment instead of after it, as the comment does not apply to the new functions. I am repeating the previous changelog below for your convenience, as v3 is what is currently in pu/ao/submodule-wo-gitmodules-checked-out. Changes between v3 and v4: * Improve robustness of current tests in t7411-submodule-config.sh: - merge two tests that check for effects of the same commit. - reset to a well defined point in history when exiting the tests. * Fix style issues in new tests added to t7411-submodule-config.sh: - use test_when_finished in new tests. - name the output file 'expect' instead of 'expected'. * Add a new "submodule--helper config --check-writeable" command. * Use "s--h config --check-wrteable" in git-submodule.sh to share the code with the C implementation instead of duplicating the safety check in shell script. * Add the ability to read .gitmodules from the index and then fall-back to the current branch if the file is not in the index. Add also more tests to validate all the possible scenarios. * Fix style issues in t7416-submodule-sparse-gitmodules.sh: - name the output file 'expect' instead of 'expected'. - remove white space after the redirection operator. - indent the HEREDOC block. - use "git -C super" instead of a subshell when there is only one command in the test. * Remove a stale file named 'new' which erroneously slipped in a commit. * Update some comments and commit messages. Thank you, Antonio Antonio Ospite (9): submodule: add a print_config_from_gitmodules() helper submodule: factor out a config_set_in_gitmodules_file_gently function t7411: merge tests 5 and 6 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 t7506: clean up .gitmodules properly before setting up new scenario submodule: add a helper to check if it is safe to write to .gitmodules submodule: support reading .gitmodules when it's not in the working tree builtin/submodule--helper.c| 40 + cache.h| 2 + git-submodule.sh | 13 ++- submodule-config.c | 55 - submodule-config.h | 2 + submodule.c| 28 +-- submodule.h| 1 + t/t7411-submodule-config.sh| 107 + t/t7416-submodule-sparse-gitmodules.sh | 78 ++ t/t7506-status-submodule.sh| 3 +- 10 files changed, 300 insertions(+), 29 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?
[PATCH v5 1/9] submodule: add a print_config_from_gitmodules() helper
Add a new print_config_from_gitmodules() helper function to print values
from .gitmodules just like "git config -f .gitmodules" would.
This will be used by a new submodule--helper subcommand to be able to
access the .gitmodules file in a more controlled way.
Signed-off-by: Antonio Ospite
---
submodule-config.c | 25 +
submodule-config.h | 1 +
2 files changed, 26 insertions(+)
diff --git a/submodule-config.c b/submodule-config.c
index fc2c41b947..f70b7f1baf 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 *var, const char *value, void
*cb_data)
+{
+ char *wanted_key = cb_data;
+
+ if (!strcmp(wanted_key, var))
+ 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, _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..dd7f1b9a46 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -48,6 +48,7 @@ const struct submodule *submodule_from_path(struct repository
*r,
const struct object_id
*commit_or_tree,
const char *path);
void submodule_free(struct repository *r);
+int print_config_from_gitmodules(const char *key);
/*
* Returns 0 if the name is syntactically acceptable as a submodule "name"
--
2.19.0
[PATCH v5 7/9] t7506: clean up .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 intends 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 --- 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 943708fb04..08629a6e70 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.19.0
[PATCH v5 5/9] 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
---
builtin/submodule--helper.c | 14 ++
t/t7411-submodule-config.sh | 27 +++
2 files changed, 41 insertions(+)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index f6fb8991f3..80f939cd9e 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2003,6 +2003,19 @@ static int check_name(int argc, const char **argv, const
char *prefix)
return 0;
}
+static int module_config(int argc, const char **argv, const char *prefix)
+{
+ /* 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]);
+
+ die("submodule--helper config takes 1 or 2 arguments: name [value]");
+}
+
#define SUPPORT_SUPER_PREFIX (1<<0)
struct cmd_struct {
@@ -2030,6 +2043,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 b1f3c6489b..791245f18d 100755
--- a/t/t7411-submodule-config.sh
+++ b/t/t7411-submodule-config.sh
@@ -134,4 +134,31 @@ test_expect_success 'error in history in
fetchrecursesubmodule lets continue' '
)
'
+test_expect_success 'reading submodules config with "submodule--helper
config"' '
+ (cd super &&
+ echo "../submodule" >expect &&
+ git submodule--helper config submodule.submodule.url >actual &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'writing submodules config with "submodule--helper
config"' '
+ (cd super &&
+ echo "new_url" >expect &&
+ git submodule--helper config submodule.submodule.url "new_url"
&&
+ git submodule--helper config submodule.submodule.url >actual &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'overwriting unstaged submodules config with
"submodule--helper config"' '
+ test_when_finished "git -C super checkout .gitmodules" &&
+ (cd super &&
+ echo "newer_url" >expect &&
+ git submodule--helper config submodule.submodule.url
"newer_url" &&
+ git submodule--helper config submodule.submodule.url >actual &&
+ test_cmp expect actual
+ )
+'
+
test_done
--
2.19.0
[PATCH v5 4/9] t7411: be nicer to future tests and really clean things up
Tests 5 and 7 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. Remove both the commits as soon as they are not needed anymore. Signed-off-by: Antonio Ospite --- t/t7411-submodule-config.sh | 7 +-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh index f2cd1f4a2c..b1f3c6489b 100755 --- a/t/t7411-submodule-config.sh +++ b/t/t7411-submodule-config.sh @@ -83,6 +83,8 @@ Submodule name: 'submodule' for path 'submodule' EOF test_expect_success 'error in history of one submodule config lets continue, stderr message contains blob ref' ' + ORIG=$(git -C super rev-parse HEAD) && + test_when_finished "git -C super reset --hard $ORIG" && (cd super && cp .gitmodules .gitmodules.bak && echo " value = \"" >>.gitmodules && @@ -115,6 +117,8 @@ test_expect_success 'using different treeishs works' ' ' test_expect_success 'error in history in fetchrecursesubmodule lets continue' ' + ORIG=$(git -C super rev-parse HEAD) && + test_when_finished "git -C super reset --hard $ORIG" && (cd super && git config -f .gitmodules \ submodule.submodule.fetchrecursesubmodules blabla && @@ -126,8 +130,7 @@ test_expect_success 'error in history in fetchrecursesubmodule lets continue' ' HEAD b \ HEAD submodule \ >actual && - test_cmp expect_error actual && - git reset --hard HEAD^ + test_cmp expect_error actual ) ' -- 2.19.0
[PATCH v5 9/9] submodule: support reading .gitmodules when it's not in the working tree
When the .gitmodules file is not available in the working tree, try
using the content from the index and 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.
Add a similar check also in git-submodule.sh::cmd_add() to anticipate
the eventual failure of the "git submodule add" command when .gitmodules
is not safely writeable; this prevents the command from leaving the
repository in a spurious state (e.g. the submodule repository was cloned
but .gitmodules was not updated because
config_set_in_gitmodules_file_gently failed).
Finally, add t7416-submodule-sparse-gitmodules.sh to verify that reading
from .gitmodules succeeds and that writing to it fails when the file is
not checked out.
Signed-off-by: Antonio Ospite
---
builtin/submodule--helper.c| 6 +-
git-submodule.sh | 5 ++
submodule-config.c | 18 +-
t/t7411-submodule-config.sh| 26 -
t/t7416-submodule-sparse-gitmodules.sh | 78 ++
5 files changed, 129 insertions(+), 4 deletions(-)
create mode 100755 t/t7416-submodule-sparse-gitmodules.sh
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index bd14f57d00..f72f6ee58a 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2032,8 +2032,12 @@ 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) {
+ if (!is_writing_gitmodules_ok())
+ die(_("please make sure that the .gitmodules file is in
the working tree"));
+
return config_set_in_gitmodules_file_gently(argv[1], argv[2]);
+ }
usage_with_options(git_submodule_helper_usage, module_config_options);
}
diff --git a/git-submodule.sh b/git-submodule.sh
index 25b9bc58cb..bff855f54a 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -159,6 +159,11 @@ cmd_add()
shift
done
+ if ! git submodule--helper config --check-writeable >/dev/null 2>&1
+ then
+die "$(eval_gettext "please make sure that the .gitmodules
file is in the working tree")"
+ fi
+
if test -n "$reference_path"
then
is_absolute_path "$reference_path" ||
diff --git a/submodule-config.c b/submodule-config.c
index 61a555e920..bdb1d0e2c9 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,21 @@ 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_INDEX, ) >= 0)
+ config_source.blob = GITMODULES_INDEX;
+ else if (get_oid(GITMODULES_HEAD, ) >= 0)
+ config_source.blob = GITMODULES_HEAD;
+
+ config_with_options(fn, data, _source, );
+
free(file);
}
}
diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
index 45953f9300..2cfabb18bc 100755
--- a/t/t7411-submodule-config.sh
+++ b/t/t7411-submodule-config.sh
@@ -134,7 +134,7 @@ test_expect_success 'error in history in
fetchrecursesubmodule lets continue' '
)
'
-test_expect_success 'reading submodules config with "submodule--helper
config"' '
+test_expect_success 'reading submodules config from the working tree with
"submodule--helper config"' '
(cd super &&
echo "../submodule" >expect &&
git submodule--helper config submodule.submodule.url >actual &&
@@ -192,4 +192,28 @@ test_expect_success 'non-writeable .git
[PATCH v5 2/9] 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
---
submodule-config.c | 12
submodule-config.h | 1 +
submodule.c| 10 +++---
3 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/submodule-config.c b/submodule-config.c
index f70b7f1baf..61a555e920 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 dd7f1b9a46..3921927aa1 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -49,6 +49,7 @@ const struct submodule *submodule_from_path(struct repository
*r,
const char *path);
void submodule_free(struct repository *r);
int print_config_from_gitmodules(const char *key);
+int config_set_in_gitmodules_file_gently(const char *key, const char *value);
/*
* Returns 0 if the name is syntactically acceptable as a submodule "name"
diff --git a/submodule.c b/submodule.c
index a2b266fbfa..2e97032f86 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(, "submodule.");
strbuf_addstr(, submodule->name);
strbuf_addstr(, ".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();
- return -1;
- }
+ ret = config_set_in_gitmodules_file_gently(entry.buf, newpath);
strbuf_release();
- return 0;
+ return ret;
}
/*
--
2.19.0
[PATCH v5 3/9] t7411: merge tests 5 and 6
Tests 5 and 6 check for the effects of the same commit, merge the two tests to make it more straightforward to clean things up after the test has finished. The cleanup will be added in a future commit. Signed-off-by: Antonio Ospite --- t/t7411-submodule-config.sh | 18 +- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh index 0bde5850ac..f2cd1f4a2c 100755 --- a/t/t7411-submodule-config.sh +++ b/t/t7411-submodule-config.sh @@ -82,29 +82,21 @@ Submodule name: 'a' for path 'b' Submodule name: 'submodule' for path 'submodule' EOF -test_expect_success 'error in one submodule config lets continue' ' +test_expect_success 'error in history of one submodule config lets continue, stderr message contains blob ref' ' (cd super && cp .gitmodules .gitmodules.bak && echo " value = \"" >>.gitmodules && git add .gitmodules && mv .gitmodules.bak .gitmodules && git commit -m "add error" && - test-tool submodule-config \ - HEAD b \ - HEAD submodule \ - >actual && - test_cmp expect_error actual - ) -' - -test_expect_success 'error message contains blob reference' ' - (cd super && sha1=$(git rev-parse HEAD) && test-tool submodule-config \ HEAD b \ HEAD submodule \ - 2>actual_err && - test_i18ngrep "submodule-blob $sha1:.gitmodules" actual_err >/dev/null + >actual \ + 2>actual_stderr && + test_cmp expect_error actual && + test_i18ngrep "submodule-blob $sha1:.gitmodules" actual_stderr >/dev/null ) ' -- 2.19.0
[PATCH v5 8/9] submodule: add a helper to check if it is safe to write to .gitmodules
Introduce a helper function named is_writing_gitmodules_ok() to verify
that the .gitmodules file is safe to write.
The function name follows the scheme of is_staging_gitmodules_ok().
The two symbolic constants GITMODULES_INDEX and GITMODULES_HEAD are used
to get help from the C preprocessor in preventing typos, especially for
future users.
This is in preparation for a future change which teaches git how to read
.gitmodules from the index or from the current branch if the file is not
available in the working tree.
The rationale behind the check is that writing to .gitmodules requires
the file to be present in the working tree, unless a brand new
.gitmodules is being created (in which case the .gitmodules file would
not exist at all: neither in the working tree nor in the index or in the
current branch).
Expose the functionality also via a "submodule-helper config
--check-writeable" command, as git scripts may want to perform the check
before modifying submodules configuration.
Signed-off-by: Antonio Ospite
---
builtin/submodule--helper.c | 24 +++-
cache.h | 2 ++
submodule.c | 18 ++
submodule.h | 1 +
t/t7411-submodule-config.sh | 31 +++
5 files changed, 75 insertions(+), 1 deletion(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 80f939cd9e..bd14f57d00 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2005,6 +2005,28 @@ static int check_name(int argc, const char **argv, const
char *prefix)
static int module_config(int argc, const char **argv, const char *prefix)
{
+ enum {
+ CHECK_WRITEABLE = 1
+ } command = 0;
+
+ struct option module_config_options[] = {
+ OPT_CMDMODE(0, "check-writeable", ,
+ N_("check if it is safe to write to the .gitmodules
file"),
+ CHECK_WRITEABLE),
+ OPT_END()
+ };
+ const char *const git_submodule_helper_usage[] = {
+ N_("git submodule--helper config name [value]"),
+ N_("git submodule--helper config --check-writeable"),
+ NULL
+ };
+
+ argc = parse_options(argc, argv, prefix, module_config_options,
+git_submodule_helper_usage, PARSE_OPT_KEEP_ARGV0);
+
+ if (argc == 1 && command == CHECK_WRITEABLE)
+ return is_writing_gitmodules_ok() ? 0 : -1;
+
/* Equivalent to ACTION_GET in builtin/config.c */
if (argc == 2)
return print_config_from_gitmodules(argv[1]);
@@ -2013,7 +2035,7 @@ static int module_config(int argc, const char **argv,
const char *prefix)
if (argc == 3)
return config_set_in_gitmodules_file_gently(argv[1], argv[2]);
- die("submodule--helper config takes 1 or 2 arguments: name [value]");
+ usage_with_options(git_submodule_helper_usage, module_config_options);
}
#define SUPPORT_SUPER_PREFIX (1<<0)
diff --git a/cache.h b/cache.h
index 4d014541ab..33723d2a32 100644
--- a/cache.h
+++ b/cache.h
@@ -486,6 +486,8 @@ 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_INDEX ":.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.c b/submodule.c
index 2e97032f86..2b7082b2db 100644
--- a/submodule.c
+++ b/submodule.c
@@ -50,6 +50,24 @@ int is_gitmodules_unmerged(const struct index_state *istate)
return 0;
}
+/*
+ * Check if the .gitmodules file is safe to write.
+ *
+ * Writing to the .gitmodules file requires that the file exists in the
+ * working tree or, if it doesn't, that a brand new .gitmodules file is going
+ * to be created (i.e. it's neither in the index nor in the current branch).
+ *
+ * It is not safe to write to .gitmodules if it's not in the working tree but
+ * it is in the index or in the current branch, because writing new values
+ * (and staging them) would blindly overwrite ALL the old content.
+ */
+int is_writing_gitmodules_ok(void)
+{
+ struct object_id oid;
+ return file_exists(GITMODULES_FILE) ||
+ (get_oid(GITMODULES_INDEX, ) < 0 &&
get_oid(GITMODULES_HEAD, ) < 0);
+}
+
/*
* Check if the .gitmodules file has unstaged modifications. This must be
* checked before allowing modifications to the .gitmodules file with the
diff --git a/submodule.h b/submodule.h
index e452919aa4..7a22f71cb9 100644
--- a/submodule.h
++
[PATCH v5 6/9] 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
---
git-submodule.sh | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/git-submodule.sh b/git-submodule.sh
index 1cb2c0a31b..25b9bc58cb 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.19.0
Re: What's cooking in git.git (Sep 2018, #03; Fri, 14)
On Fri, 14 Sep 2018 14:56:36 -0700 Junio C Hamano wrote: [...] > * ao/submodule-wo-gitmodules-checked-out (2018-08-14) 7 commits > - submodule: support reading .gitmodules even when it's not checked out > - t7506: clean up .gitmodules properly before setting up new scenario > - submodule: use the 'submodule--helper config' command > - submodule--helper: add a new 'config' subcommand > - t7411: be nicer to future tests and really clean things up > - submodule: factor out a config_set_in_gitmodules_file_gently function > - submodule: add a print_config_from_gitmodules() helper > > The submodule support has been updated to read from the blob at > HEAD:.gitmodules when the .gitmodules file is missing from the > working tree. > > Expecting a reroll. > > I find the design a bit iffy in that our usual "missing in the > working tree? let's use the latest blob" fallback is to take it > from the index, not from the HEAD. > Hi Junio, I did send a v4 addressing this: https://public-inbox.org/git/20180824132951.8000-1-...@ao2.it/ In case you missed it I might as well send a v5 with some minor cosmetic touches suggested by Ævar: https://public-inbox.org/git/87wosfesxl@evledraar.gmail.com/ 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?
Re: [PATCH v4 1/9] submodule: add a print_config_from_gitmodules() helper
On Fri, 24 Aug 2018 18:52:51 +0200 Antonio Ospite wrote: [...] > I'll wait for other comments to see if a v5 is really needed. > Ping. In case someone missed v4. 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?
Re: [PATCH v4 1/9] submodule: add a print_config_from_gitmodules() helper
On Fri, 24 Aug 2018 16:32:38 +0200
Ævar Arnfjörð Bjarmason wrote:
>
> On Fri, Aug 24 2018, Antonio Ospite wrote:
[...]
> > +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;
> > +}
>
> No problem with the code itself, but I'd find this a lot easier to read
> in context if it was:
>
> key_ -> var
> value_ -> value
> key -> wanted_key, perhaps?
>
> I.e. the rest of the file uses the convention of the
> config_from_gitmodules callbacks getting "var" and "value",
> respectively.
>
> We don't use this convention of suffixing variables with "_" if they're
> arguments to the function anywhere else.
>
I was new to git when I firstly wrote the code, I picked up the style
by copying from builtin/config.c (collect_config() and
show_all_config()) because I was focusing more on the functionality and
didn't bother to harmonize the style with the destination file.
> > +int print_config_from_gitmodules(const char *key)
> > +{
> > + int ret;
> > + char *store_key;
> > +
> > + ret = git_config_parse_key(key, _key, NULL);
> > + if (ret < 0)
> > + return CONFIG_INVALID_KEY;
>
> Isn't this a memory leak? I.e. we should free() and return here, no?
>
It is true that git_config_parse_key_1() allocates some storage even
for an invalid key, and uses the space to lowercase the key as the
parsing progresses, however it also frees the memory in the *error*
path.
So unless I am missing something I don't think there is a leak here.
[...]
> > diff --git a/submodule-config.h b/submodule-config.h
> > index dc7278eea4..ed40e9a478 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);
> >
> > +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
>
> Another style nit: Makes more sense to put this new function right
> underneath "submodule_free" above, instead of under 1/2 of the functions
> that have a big comment describing how they work, since that comment
> doesn't apply to this new function.
You are probably right, IIRC the function was firstly written before
check_submodule_name() was there. And I didn't think of moving it up
when I rebased after check_submodule_name() was added.
Your same remark may apply to the new function added in patch 02.
I'll wait for other comments to see if a v5 is really needed.
Thanks for the review Ævar.
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?
[PATCH v4 1/9] submodule: add a print_config_from_gitmodules() helper
Add a new print_config_from_gitmodules() helper function to print values
from .gitmodules just like "git config -f .gitmodules" would.
This will be used by a new submodule--helper subcommand to be able to
access the .gitmodules file in a more controlled way.
Signed-off-by: Antonio Ospite
---
submodule-config.c | 25 +
submodule-config.h | 2 ++
2 files changed, 27 insertions(+)
diff --git a/submodule-config.c b/submodule-config.c
index fc2c41b947..eef96c4198 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, _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..ed40e9a478 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);
+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
[PATCH v4 7/9] t7506: clean up .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 intends 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 --- 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 943708fb04..08629a6e70 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
[PATCH v4 3/9] t7411: merge tests 5 and 6
Tests 5 and 6 check for the effects of the same commit, merge the two tests to make it more straightforward to clean things up after the test has finished. The cleanup will be added in a future commit. Signed-off-by: Antonio Ospite --- t/t7411-submodule-config.sh | 18 +- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh index 0bde5850ac..f2cd1f4a2c 100755 --- a/t/t7411-submodule-config.sh +++ b/t/t7411-submodule-config.sh @@ -82,29 +82,21 @@ Submodule name: 'a' for path 'b' Submodule name: 'submodule' for path 'submodule' EOF -test_expect_success 'error in one submodule config lets continue' ' +test_expect_success 'error in history of one submodule config lets continue, stderr message contains blob ref' ' (cd super && cp .gitmodules .gitmodules.bak && echo " value = \"" >>.gitmodules && git add .gitmodules && mv .gitmodules.bak .gitmodules && git commit -m "add error" && - test-tool submodule-config \ - HEAD b \ - HEAD submodule \ - >actual && - test_cmp expect_error actual - ) -' - -test_expect_success 'error message contains blob reference' ' - (cd super && sha1=$(git rev-parse HEAD) && test-tool submodule-config \ HEAD b \ HEAD submodule \ - 2>actual_err && - test_i18ngrep "submodule-blob $sha1:.gitmodules" actual_err >/dev/null + >actual \ + 2>actual_stderr && + test_cmp expect_error actual && + test_i18ngrep "submodule-blob $sha1:.gitmodules" actual_stderr >/dev/null ) ' -- 2.18.0
[PATCH v4 5/9] 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
---
builtin/submodule--helper.c | 14 ++
t/t7411-submodule-config.sh | 27 +++
2 files changed, 41 insertions(+)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 2bcc70fdfe..a461a1107c 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2028,6 +2028,19 @@ 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)
+{
+ /* 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]);
+
+ die("submodule--helper config takes 1 or 2 arguments: name [value]");
+}
+
#define SUPPORT_SUPER_PREFIX (1<<0)
struct cmd_struct {
@@ -2056,6 +2069,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 b1f3c6489b..791245f18d 100755
--- a/t/t7411-submodule-config.sh
+++ b/t/t7411-submodule-config.sh
@@ -134,4 +134,31 @@ test_expect_success 'error in history in
fetchrecursesubmodule lets continue' '
)
'
+test_expect_success 'reading submodules config with "submodule--helper
config"' '
+ (cd super &&
+ echo "../submodule" >expect &&
+ git submodule--helper config submodule.submodule.url >actual &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'writing submodules config with "submodule--helper
config"' '
+ (cd super &&
+ echo "new_url" >expect &&
+ git submodule--helper config submodule.submodule.url "new_url"
&&
+ git submodule--helper config submodule.submodule.url >actual &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'overwriting unstaged submodules config with
"submodule--helper config"' '
+ test_when_finished "git -C super checkout .gitmodules" &&
+ (cd super &&
+ echo "newer_url" >expect &&
+ git submodule--helper config submodule.submodule.url
"newer_url" &&
+ git submodule--helper config submodule.submodule.url >actual &&
+ test_cmp expect actual
+ )
+'
+
test_done
--
2.18.0
[PATCH v4 0/9] Make submodules work if .gitmodules is not checked out
Hi, this series teaches git to try and read the .gitmodules file from the index (:.gitmodules) and the current branch (HEAD:.gitmodules) when it is not readily available in the working tree. This can be used, together with sparse checkouts, to enable submodule usage with programs like vcsh[1] which manage multiple repositories with their working trees sharing the same path. [1] https://github.com/RichiH/vcsh In v4 patches 1, 2, 5, 6, and 7 are basically the same as in the previous series; the review can concentrate on patches 3, 4, 8, and 9. v3 of the series is here: https://public-inbox.org/git/20180814110525.17801-1-...@ao2.it/ v2 of the series is here: https://public-inbox.org/git/20180802134634.10300-1-...@ao2.it/ v1 of the series, with some background info, is here: https://public-inbox.org/git/20180514105823.8378-1-...@ao2.it/ Changes since v3: * Improve robustness of current tests in t7411-submodule-config.sh: - merge two tests that check for effects of the same commit. - reset to a well defined point in history when exiting the tests. * Fix style issues in new tests added to t7411-submodule-config.sh: - use test_when_finished in new tests. - name the output file 'expect' instead of 'expected'. * Add a new "submodule--helper config --check-writeable" command. * Use "s--h config --check-wrteable" in git-submodule.sh to share the code with the C implementation instead of duplicating the safety check in shell script. * Add the ability to read .gitmodules from the index and then fall-back to the current branch if the file is not in the index. Add also more tests to validate all the possible scenarios. * Fix style issues in t7416-submodule-sparse-gitmodules.sh: - name the output file 'expect' instead of 'expected'. - remove white space after the redirection operator. - indent the HEREDOC block. - use "git -C super" instead of a subshell when there is only one command in the test. * Remove a stale file named 'new' which erroneusly slipped in a commit. * Update some comments and commit messages. Thanks, Antonio Antonio Ospite (9): submodule: add a print_config_from_gitmodules() helper submodule: factor out a config_set_in_gitmodules_file_gently function t7411: merge tests 5 and 6 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 t7506: clean up .gitmodules properly before setting up new scenario submodule: add a helper to check if it is safe to write to .gitmodules submodule: support reading .gitmodules when it's not in the working tree builtin/submodule--helper.c| 40 + cache.h| 2 + git-submodule.sh | 13 ++- submodule-config.c | 55 - submodule-config.h | 3 + submodule.c| 28 +-- submodule.h| 1 + t/t7411-submodule-config.sh| 107 + t/t7416-submodule-sparse-gitmodules.sh | 78 ++ t/t7506-status-submodule.sh| 3 +- 10 files changed, 301 insertions(+), 29 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?
[PATCH v4 8/9] submodule: add a helper to check if it is safe to write to .gitmodules
Introduce a helper function named is_writing_gitmodules_ok() to verify
that the .gitmodules file is safe to write.
The function name follows the scheme of is_staging_gitmodules_ok().
The two symbolic constants GITMODULES_INDEX and GITMODULES_HEAD are used
to get help from the C preprocessor in preventing typos, especially for
future users.
This is in preparation for a future change which teaches git how to read
.gitmodules from the index or from the current branch if the file is not
available in the working tree.
The rationale behind the check is that writing to .gitmodules requires
the file to be present in the working tree, unless a brand new
.gitmodules is being created (in which case the .gitmodules file would
not exist at all: neither in the working tree nor in the index or in the
current branch).
Expose the functionality also via a "submodule-helper config
--check-writeable" command, as git scripts may want to perform the check
before modifying submodules configuration.
Signed-off-by: Antonio Ospite
---
builtin/submodule--helper.c | 24 +++-
cache.h | 2 ++
submodule.c | 18 ++
submodule.h | 1 +
t/t7411-submodule-config.sh | 31 +++
5 files changed, 75 insertions(+), 1 deletion(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index a461a1107c..1bb168e814 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2030,6 +2030,28 @@ 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)
{
+ enum {
+ CHECK_WRITEABLE = 1
+ } command = 0;
+
+ struct option module_config_options[] = {
+ OPT_CMDMODE(0, "check-writeable", ,
+ N_("check if it is safe to write to the .gitmodules
file"),
+ CHECK_WRITEABLE),
+ OPT_END()
+ };
+ const char *const git_submodule_helper_usage[] = {
+ N_("git submodule--helper config name [value]"),
+ N_("git submodule--helper config --check-writeable"),
+ NULL
+ };
+
+ argc = parse_options(argc, argv, prefix, module_config_options,
+git_submodule_helper_usage, PARSE_OPT_KEEP_ARGV0);
+
+ if (argc == 1 && command == CHECK_WRITEABLE)
+ return is_writing_gitmodules_ok() ? 0 : -1;
+
/* Equivalent to ACTION_GET in builtin/config.c */
if (argc == 2)
return print_config_from_gitmodules(argv[1]);
@@ -2038,7 +2060,7 @@ static int module_config(int argc, const char **argv,
const char *prefix)
if (argc == 3)
return config_set_in_gitmodules_file_gently(argv[1], argv[2]);
- die("submodule--helper config takes 1 or 2 arguments: name [value]");
+ usage_with_options(git_submodule_helper_usage, module_config_options);
}
#define SUPPORT_SUPER_PREFIX (1<<0)
diff --git a/cache.h b/cache.h
index b1fd3d58ab..e3fe1e6191 100644
--- a/cache.h
+++ b/cache.h
@@ -486,6 +486,8 @@ 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_INDEX ":.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.c b/submodule.c
index 36a9465acb..d875b512df 100644
--- a/submodule.c
+++ b/submodule.c
@@ -50,6 +50,24 @@ int is_gitmodules_unmerged(const struct index_state *istate)
return 0;
}
+/*
+ * Check if the .gitmodules file is safe to write.
+ *
+ * Writing to the .gitmodules file requires that the file exists in the
+ * working tree or, if it doesn't, that a brand new .gitmodules file is going
+ * to be created (i.e. it's neither in the index nor in the current branch).
+ *
+ * It is not safe to write to .gitmodules if it's not in the working tree but
+ * it is in the index or in the current branch, because writing new values
+ * (and staging them) would blindly overwrite ALL the old content.
+ */
+int is_writing_gitmodules_ok(void)
+{
+ struct object_id oid;
+ return file_exists(GITMODULES_FILE) ||
+ (get_oid(GITMODULES_INDEX, ) < 0 &&
get_oid(GITMODULES_HEAD, ) < 0);
+}
+
/*
* Check if the .gitmodules file has unstaged modifications. This must be
* checked before allowing modifications to the .gitmodules file with the
diff --git a/submodule.h b/submodule.h
index 7d476cefa7..5106d4597a 1006
[PATCH v4 6/9] 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
---
git-submodule.sh | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/git-submodule.sh b/git-submodule.sh
index f7fd80345c..9e47dc9574 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
[PATCH v4 2/9] 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
---
submodule-config.c | 12
submodule-config.h | 1 +
submodule.c| 10 +++---
3 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/submodule-config.c b/submodule-config.c
index eef96c4198..b7ef055c63 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 ed40e9a478..9957bcbbfa 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);
int print_config_from_gitmodules(const char *key);
+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 50cbf5f13e..36a9465acb 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(, "submodule.");
strbuf_addstr(, submodule->name);
strbuf_addstr(, ".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();
- return -1;
- }
+ ret = config_set_in_gitmodules_file_gently(entry.buf, newpath);
strbuf_release();
- return 0;
+ return ret;
}
/*
--
2.18.0
[PATCH v4 9/9] submodule: support reading .gitmodules when it's not in the working tree
When the .gitmodules file is not available in the working tree, try
using the content from the index and 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.
Add a similar check also in git-submodule.sh::cmd_add() to anticipate
the eventual failure of the "git submodule add" command when .gitmodules
is not safely writeable; this prevents the command from leaving the
repository in a spurious state (e.g. the submodule repository was cloned
but .gitmodules was not updated because
config_set_in_gitmodules_file_gently failed).
Finally, add t7416-submodule-sparse-gitmodules.sh to verify that reading
from .gitmodules succeeds and that writing to it fails when the file is
not checked out.
Signed-off-by: Antonio Ospite
---
builtin/submodule--helper.c| 6 +-
git-submodule.sh | 5 ++
submodule-config.c | 18 +-
t/t7411-submodule-config.sh| 26 -
t/t7416-submodule-sparse-gitmodules.sh | 78 ++
5 files changed, 129 insertions(+), 4 deletions(-)
create mode 100755 t/t7416-submodule-sparse-gitmodules.sh
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 1bb168e814..6bc44d87dc 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2057,8 +2057,12 @@ 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) {
+ if (!is_writing_gitmodules_ok())
+ die(_("please make sure that the .gitmodules file is in
the working tree"));
+
return config_set_in_gitmodules_file_gently(argv[1], argv[2]);
+ }
usage_with_options(git_submodule_helper_usage, module_config_options);
}
diff --git a/git-submodule.sh b/git-submodule.sh
index 9e47dc9574..0e44487135 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -159,6 +159,11 @@ cmd_add()
shift
done
+ if ! git submodule--helper config --check-writeable >/dev/null 2>&1
+ then
+die "$(eval_gettext "please make sure that the .gitmodules
file is in the working tree")"
+ fi
+
if test -n "$reference_path"
then
is_absolute_path "$reference_path" ||
diff --git a/submodule-config.c b/submodule-config.c
index b7ef055c63..edba68ac85 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,21 @@ 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_INDEX, ) >= 0)
+ config_source.blob = GITMODULES_INDEX;
+ else if (get_oid(GITMODULES_HEAD, ) >= 0)
+ config_source.blob = GITMODULES_HEAD;
+
+ config_with_options(fn, data, _source, );
+
free(file);
}
}
diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
index 45953f9300..2cfabb18bc 100755
--- a/t/t7411-submodule-config.sh
+++ b/t/t7411-submodule-config.sh
@@ -134,7 +134,7 @@ test_expect_success 'error in history in
fetchrecursesubmodule lets continue' '
)
'
-test_expect_success 'reading submodules config with "submodule--helper
config"' '
+test_expect_success 'reading submodules config from the working tree with
"submodule--helper config"' '
(cd super &&
echo "../submodule" >expect &&
git submodule--helper config submodule.submodule.url >actual &&
@@ -192,4 +192,28 @@ test_expect_success 'non-writeable .git
[PATCH v4 4/9] t7411: be nicer to future tests and really clean things up
Tests 5 and 7 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. Remove both the commits as soon as they are not needed anymore. Signed-off-by: Antonio Ospite --- t/t7411-submodule-config.sh | 7 +-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh index f2cd1f4a2c..b1f3c6489b 100755 --- a/t/t7411-submodule-config.sh +++ b/t/t7411-submodule-config.sh @@ -83,6 +83,8 @@ Submodule name: 'submodule' for path 'submodule' EOF test_expect_success 'error in history of one submodule config lets continue, stderr message contains blob ref' ' + ORIG=$(git -C super rev-parse HEAD) && + test_when_finished "git -C super reset --hard $ORIG" && (cd super && cp .gitmodules .gitmodules.bak && echo " value = \"" >>.gitmodules && @@ -115,6 +117,8 @@ test_expect_success 'using different treeishs works' ' ' test_expect_success 'error in history in fetchrecursesubmodule lets continue' ' + ORIG=$(git -C super rev-parse HEAD) && + test_when_finished "git -C super reset --hard $ORIG" && (cd super && git config -f .gitmodules \ submodule.submodule.fetchrecursesubmodules blabla && @@ -126,8 +130,7 @@ test_expect_success 'error in history in fetchrecursesubmodule lets continue' ' HEAD b \ HEAD submodule \ >actual && - test_cmp expect_error actual && - git reset --hard HEAD^ + test_cmp expect_error actual ) ' -- 2.18.0
Re: [PATCH v3 7/7] submodule: support reading .gitmodules even when it's not checked out
On Wed, 22 Aug 2018 08:29:25 -0700 Junio C Hamano wrote: > Antonio Ospite writes: > [...] > >> > > + else if (get_oid(GITMODULES_HEAD, ) >= 0) > >> > > + config_source.blob = GITMODULES_HEAD; > >> > > >> Would using ":.gitmodules" instead of "HEAD:.gitmodules" be enough? > > Yeah, either "instead of", or "in addition" (i.e. "try the index > version in addition, before falling further back to the HEAD > version"), would be more consistent with the remainder of the system > (or, at least where the remainder of the system wants to go). > OK, I now tested with both "rm .gitmodules" and "git rm .gitmodules" and I see why one would want to try _both_ ":.gitmodules" and "HEAD:.gitmodules". I'll go with "in addition" then, adding tests for both the scenarios. > >> If so, what name should I use instead of GITMODULES_HEAD? > >> GITMODULES_BLOB is already taken for something different, maybe > >> GITMODULES_REF or GITMODULES_OBJECT? > > I do not know why you want to refrain from spelling them out as > "HEAD:.gitmodules" and ":.gitmodules"; at least to me the extra > layer of names do not look like they are making the code easier > to understand that much. > This is in the spirit of commit 4c0eeafe47 (cache.h: add GITMODULES_FILE macro, 2017-08-02), IIRC this was done mainly to get help from the preprocessor to spot typos: I caught myself writing ".gitmdoules" several times; GITMDOULES_FILE would not compile. If this makes sense I'll use GITMODULES_INDEX and GITMODULES_HEAD. 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?
Re: [PATCH v3 7/7] submodule: support reading .gitmodules even when it's not checked out
On Mon, 20 Aug 2018 23:37:55 +0200
Antonio Ospite wrote:
> On Tue, 14 Aug 2018 13:36:17 -0700
> Junio C Hamano wrote:
>
> > Antonio Ospite writes:
[...]
> > >
> > > + # For more details about this check, see
> > > + # builtin/submodule--helper.c::module_config()
> > > + if test ! -e .gitmodules && git cat-file -e HEAD:.gitmodules >
> > > /dev/null 2>&1
> >
[...]
> > More importantly, I think it is better to add a submodule--helper
> > subcommand that exposes the check in question, as the code is
> > already written ;-) That approach will guarantee that the logic and
> > the message stay the same between here and in the C code. Then you
> > do not even need these two line comment.
> >
[...]
> Does the interface suggested in the patch annotation sound acceptable?
>
> To recap:
>
> - add an is_gitmodules_safely_writeable() helper;
> - expose a "submodule--helper config --is-safely-writeable"
> subcommand for git-submodule.sh to use.
>
Maybe "submodule--helper config --check-writeable" could be a better
name to avoid confusion between the boolean return value of the C
function (0: false, 1: true) and the exit status returned to the shell
(0: safe to write, !0: unsafe).
I'll use the following to map the returned value, as I saw that in
other places in the code base:
if (argc == 1 && command == CHECK_WRITEABLE)
return is_gitmodules_safely_writeable() ? 0 : -1;
I am assuming a command flag to the "config" subcommand is OK instead
of a brand new subcommand.
> [...]
> > > @@ -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, ) >= 0)
> > > + config_source.blob = GITMODULES_HEAD;
> >
> > What is the reason why we fall back directly to HEAD when working
> > tree file does not exist? I thought that our usual fallback was to
> > the version in the index for other things like .gitignore/attribute
> > and this codepath look like an oddball. Are you trying to handle
> > the case where we are in a bare repository without any file checked
> > out (and there is not even the index)?
> >
>
> My use case is about *reading* .gitmodules when it's ignored in a sparse
> checkout, in this scenario there are usually no staged changes
> to .gitmodules, so I basically just didn't care about the index.
>
> Would using ":.gitmodules" instead of "HEAD:.gitmodules" be enough?
>
[...]
>
> If so, what name should I use instead of GITMODULES_HEAD?
> GITMODULES_BLOB is already taken for something different, maybe
> GITMODULES_REF or GITMODULES_OBJECT?
>
If using ":.gitmodules" is good enough I could rename the current use
of GITMODULES_BLOB in fsck.c to GITMODULES_NONBLOB and use
GITMODULES_BLOB for ":.gitmodules" after all.
This is to avoid preprocessor clashes with the symbolic constant
GITMODULES_BLOB currently used in in fsck.c.
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?
Re: [PATCH v3 7/7] submodule: support reading .gitmodules even when it's not checked out
On Tue, 14 Aug 2018 13:36:17 -0700
Junio C Hamano wrote:
> Antonio Ospite writes:
>
> > /* 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.
>
> Hmph, "the file is missing" certainly is a condition we would want
> to notice, but wouldn't we in general want to prevent us from
> overwriting any local modification, where "missing" is merely a very
> special case of local modification? I am wondering if we would want
> to stop if .gitmodules file exists both in the working tree and in
> the index, and the contents of them differ, or something like that.
>
TTBOMK checking the index status (with something like
is_staging_gitmodules_ok()) when the .gitmodules file *exists* in the
working tree would break calling "git submodule add" multiple times
before committing the changes.
> > diff --git a/git-submodule.sh b/git-submodule.sh
> > index ff258e2e8c..b1cb187227 100755
> > --- a/git-submodule.sh
> > +++ b/git-submodule.sh
> > @@ -159,6 +159,13 @@ cmd_add()
> > shift
> > done
> >
> > + # For more details about this check, see
> > + # builtin/submodule--helper.c::module_config()
> > + if test ! -e .gitmodules && git cat-file -e HEAD:.gitmodules >
> > /dev/null 2>&1
>
> No SP between redirection '>' and its target '/dev/null'.
>
> More importantly, I think it is better to add a submodule--helper
> subcommand that exposes the check in question, as the code is
> already written ;-) That approach will guarantee that the logic and
> the message stay the same between here and in the C code. Then you
> do not even need these two line comment.
>
Yeah I anticipated this concern in the patch annotation, but I was
hoping that it would be OK to have this as a followup change.
I guess I can do it for v4 instead.
Does the interface suggested in the patch annotation sound acceptable?
To recap:
- add an is_gitmodules_safely_writeable() helper;
- expose a "submodule--helper config --is-safely-writeable"
subcommand for git-submodule.sh to use.
[...]
> > @@ -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, ) >= 0)
> > + config_source.blob = GITMODULES_HEAD;
>
> What is the reason why we fall back directly to HEAD when working
> tree file does not exist? I thought that our usual fallback was to
> the version in the index for other things like .gitignore/attribute
> and this codepath look like an oddball. Are you trying to handle
> the case where we are in a bare repository without any file checked
> out (and there is not even the index)?
>
My use case is about *reading* .gitmodules when it's ignored in a sparse
checkout, in this scenario there are usually no staged changes
to .gitmodules, so I basically just didn't care about the index.
Would using ":.gitmodules" instead of "HEAD:.gitmodules" be enough?
By reading man gitrevisions(7) and after a quick test with "git
cat-file blob :.gitmodules" it looks like this would be more in line
with your suggestion, still covering my use case.
If so, what name should I use instead of GITMODULES_HEAD?
GITMODULES_BLOB is already taken for something different, maybe
GITMODULES_REF or GITMODULES_OBJECT?
> > diff --git a/t/t7416-submodule-sparse-gitmodules.sh
> > b/t/t7416-submodule-sparse-gitmodules.sh
> > new file mode 100755
> > index 00..5341e9b012
> > --- /dev/null
> > +++ b/t/t7416-submodule-sparse-gitmodules.sh
> > @@ -0,0 +1,90 @@
> > +#!/bin/sh
> > +#
> > +# Copyright (C) 2018 Antonio Ospite
> > +#
> >
Re: [PATCH v3 4/7] submodule--helper: add a new 'config' subcommand
On Tue, 14 Aug 2018 10:10:58 -0700 Brandon Williams wrote: > On 08/14, Antonio Ospite wrote: > > 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 > > --- > > builtin/submodule--helper.c | 14 ++ > > > new | 0 > > Looks like you may have accidentally left in an empty file "new" it should > probably be removed from this commit before it gets merged. > Yeah, I had added it to test "git cat-file -e new" for a later patch and then I must have messed up some rebase. Thanks for pointing it out. 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?
Re: [PATCH v3 3/7] t7411: be nicer to future tests and really clean things up
On Tue, 14 Aug 2018 13:16:38 -0700 Junio C Hamano wrote: > Antonio Ospite writes: > [...] > > test_expect_success 'error message contains blob reference' ' > > + # Remove the error introduced in the previous test. > > + # It is not needed in the following tests. > > + test_when_finished "git -C super reset --hard HEAD^" && > > Hmm, that is ugly. Depending on where in the subshell the previous > test failed, you'd still be taking us to an unexpected place. > Imagine if "git commit -m 'add error'" failed, for example, in the > test before this one. > > I am wondering if the proper fix is to merge the previous one and > this one into a single test. The combined test would > > - remember where the HEAD in super is and arrange to come back > to it when test is done > - break .gitmodules and commit it > - run test-tool and check its output > - also check its error output > > in a single test_expect_success. > I will try that. > > @@ -123,6 +126,7 @@ test_expect_success 'using different treeishs works' ' > > ' > > > > test_expect_success 'error in history in fetchrecursesubmodule lets > > continue' ' > > + test_when_finished "git -C super reset --hard HEAD^" && > > (cd super && > > git config -f .gitmodules \ > > submodule.submodule.fetchrecursesubmodules blabla && > > @@ -134,8 +138,7 @@ test_expect_success 'error in history in > > fetchrecursesubmodule lets continue' ' > > HEAD b \ > > HEAD submodule \ > > >actual && > > - test_cmp expect_error actual && > > - git reset --hard HEAD^ > > + test_cmp expect_error actual > > ) > > ' > > If we want to be more robust, you'd probably need to find a better > anchoring point than HEAD, which can be pointing different commit > depending on where in the subshell the process is hit with ^C, > i.e. > > ORIG=$(git -C super rev-parse HEAD) && > test_when_finished "git -C super reset --hard $ORIG" && > ( > cd super && > ... > I see, ORIG is set and evaluated immediately but the value will be used only at a later time. I remember that you raised concerns also in the previous review round but I didn't quite get what you meant, now I think I do. > The patch is still an improvement compared to the current code, > where a broken test-tool that does not produce expected output in > the file 'actual' is guaranteed to leave us at a commit that we do > not expect to be at, but not entirely satisfactory. I can do a v4 with these fixes since there are also some comments about other patches. 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?
[PATCH v3 6/7] t7506: clean up .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 intends 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 --- 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 943708fb04..08629a6e70 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
[PATCH v3 1/7] submodule: add a print_config_from_gitmodules() helper
Add a new print_config_from_gitmodules() helper function to print values
from .gitmodules just like "git config -f .gitmodules" would.
This will be used by a new submodule--helper subcommand to be able to
access the .gitmodules file in a more controlled way.
Signed-off-by: Antonio Ospite
---
submodule-config.c | 25 +
submodule-config.h | 2 ++
2 files changed, 27 insertions(+)
diff --git a/submodule-config.c b/submodule-config.c
index fc2c41b947..eef96c4198 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, _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..ed40e9a478 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);
+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
[PATCH v3 0/7] Make submodules work if .gitmodules is not checked out
Hi, this series teaches git to try and read the .gitmodules file from the current branch (HEAD:.gitmodules) when it's not readily available in the working tree. This can be used, along with sparse checkouts, to enable submodule usage with programs like vcsh[1] which manage multiple repositories with their working trees sharing the same path. [1] https://github.com/RichiH/vcsh This is v3 of the series from: https://public-inbox.org/git/20180802134634.10300-1-...@ao2.it/ The cover letter of the first proposal contains more background: https://public-inbox.org/git/20180514105823.8378-1-...@ao2.it/ Changes since v2: * Removed the extern keyword from the public declaration of print_config_from_gitmodules() and config_set_in_gitmodules_file_gently() * Used test_when_finished in t/t7411-submodule-config.sh and remove the problematic commits as soon as they are not needed anymore. * Restructured the code in module_config to avoid an unreachable section, the code now dies as a fallback if the arguments are not supported, as suggested by Jeff. * Dropped patches and tests about "submodule--helper config --stage" as they are not strictly needed for now and there is no immediate benefit from them. * Added a check to git-submodule.sh::cdm_add to make it fail earlier if the .gitmodules file is not "safely writeable". This also fixes one of the new tests which was previously marked as "test_expect_failure". * Fixed a broken &&-chain in a subshell in one of the new tests, the issue was exposed by a recent change in master. * Dropped a note about "git rm" and "git mv", it was intended as a personal reminder and not for the general public. * Squashed t7416-submodule-sparse-gitmodules.sh in the same commit of the code it exercises. * Dropped the two unrelated patches from v2: - 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 as they are orthogonal to this series. I will send them as a standalone series. * Minor wording fixes here and there. The series looks a lot cleaner and more to the point, thanks for the review. Ciao, Antonio Antonio Ospite (7): 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 t7506: clean up .gitmodules properly before setting up new scenario submodule: support reading .gitmodules even when it's not checked out builtin/submodule--helper.c| 29 + cache.h| 1 + git-submodule.sh | 15 +++-- new| 0 submodule-config.c | 53 ++- submodule-config.h | 3 + submodule.c| 10 +-- t/t7411-submodule-config.sh| 33 +- t/t7416-submodule-sparse-gitmodules.sh | 90 ++ t/t7506-status-submodule.sh| 3 +- 10 files changed, 221 insertions(+), 16 deletions(-) create mode 100644 new 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?
[PATCH v3 2/7] 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
---
submodule-config.c | 12
submodule-config.h | 1 +
submodule.c| 10 +++---
3 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/submodule-config.c b/submodule-config.c
index eef96c4198..b7ef055c63 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 ed40e9a478..9957bcbbfa 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);
int print_config_from_gitmodules(const char *key);
+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 6e14547e9e..fd95cb76b3 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(, "submodule.");
strbuf_addstr(, submodule->name);
strbuf_addstr(, ".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();
- return -1;
- }
+ ret = config_set_in_gitmodules_file_gently(entry.buf, newpath);
strbuf_release();
- return 0;
+ return ret;
}
/*
--
2.18.0
[PATCH v3 4/7] 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
---
builtin/submodule--helper.c | 14 ++
new | 0
t/t7411-submodule-config.sh | 26 ++
3 files changed, 40 insertions(+)
create mode 100644 new
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index a3c4564c6c..7481d03b63 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2029,6 +2029,19 @@ 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)
+{
+ /* 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]);
+
+ die("submodule--helper config takes 1 or 2 arguments: name [value]");
+}
+
#define SUPPORT_SUPER_PREFIX (1<<0)
struct cmd_struct {
@@ -2057,6 +2070,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/new b/new
new file mode 100644
index 00..e69de29bb2
diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
index c6b6cf6fae..4afb6f152e 100755
--- a/t/t7411-submodule-config.sh
+++ b/t/t7411-submodule-config.sh
@@ -142,4 +142,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
[PATCH v3 5/7] 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
---
git-submodule.sh | 8
1 file changed, 4 insertions(+), 4 deletions(-)
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
[PATCH v3 7/7] 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.
Add a similar check also in git-submodule.sh::cmd_add() to anticipate
the eventual failure of the "git submodule add" command when .gitmodules
is not safely writeable; this prevents the command from leaving the
repository in a spurious state (e.g. the submodule repository was cloned
but .gitmodules was not updated because
config_set_in_gitmodules_file_gently failed).
Finally, add t7416-submodule-sparse-gitmodules.sh to verify that reading
from .gitmodules succeeds and that writing to it fails when the file is
not checked out.
Signed-off-by: Antonio Ospite
---
Maybe the check in config_set_in_gitmodules_file_gently and
git-submodule.sh::cmd_add() can share some code:
- add an is_gitmodules_safely_writeable() helper
- expose a "submodule--helper config --is-safely-writeable" subcommand
But for now I preferred to keep the changes with v2 to a minimum to avoid
blocking the series.
If adding a new helper is preferred I can do a v4 or send a follow-up patch.
Thank you,
Antonio
builtin/submodule--helper.c| 17 -
cache.h| 1 +
git-submodule.sh | 7 ++
submodule-config.c | 16 -
t/t7416-submodule-sparse-gitmodules.sh | 90 ++
5 files changed, 128 insertions(+), 3 deletions(-)
create mode 100755 t/t7416-submodule-sparse-gitmodules.sh
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 7481d03b63..c0370a756b 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2036,8 +2036,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 check still makes it possible to create a brand new
+* .gitmodules when it is safe to do so: when neither
+* GITMODULES_FILE nor GITMODULES_HEAD exist.
+*/
+ if (!file_exists(GITMODULES_FILE) && get_oid(GITMODULES_HEAD,
) >= 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]);
+ }
die("submodule--helper config takes 1 or 2 arguments: name [value]");
}
diff --git a/cache.h b/cache.h
index 8dc7134f00..900f9e09e5 100644
--- a/cache.h
+++ b/cache.h
@@ -486,6 +486,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/git-submodule.sh b/git-submodule.sh
index ff258e2e8c..b1cb187227 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -159,6 +159,13 @@ cmd_add()
shift
done
+ # For more details about this check, see
+ # builtin/submodule--helper.c::module_config()
+ if test ! -e .gitmodules && git cat-file -e HEAD:.gitmodules >
/dev/null 2>&1
+ 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" ||
diff --git a/submodule-config.c b/submodule-config.c
index b7ef055c63..088dabb56f 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
[PATCH v3 3/7] 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. Remove both the commits as soon as they are not needed anymore. The error introduced in test 5 is also required by test 6, so the two commits from above are removed respectively in tests 6 and 8. Signed-off-by: Antonio Ospite --- t/t7411-submodule-config.sh | 7 +-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh index 0bde5850ac..c6b6cf6fae 100755 --- a/t/t7411-submodule-config.sh +++ b/t/t7411-submodule-config.sh @@ -98,6 +98,9 @@ test_expect_success 'error in one submodule config lets continue' ' ' test_expect_success 'error message contains blob reference' ' + # Remove the error introduced in the previous test. + # It is not needed in the following tests. + test_when_finished "git -C super reset --hard HEAD^" && (cd super && sha1=$(git rev-parse HEAD) && test-tool submodule-config \ @@ -123,6 +126,7 @@ test_expect_success 'using different treeishs works' ' ' test_expect_success 'error in history in fetchrecursesubmodule lets continue' ' + test_when_finished "git -C super reset --hard HEAD^" && (cd super && git config -f .gitmodules \ submodule.submodule.fetchrecursesubmodules blabla && @@ -134,8 +138,7 @@ test_expect_success 'error in history in fetchrecursesubmodule lets continue' ' HEAD b \ HEAD submodule \ >actual && - test_cmp expect_error actual && - git reset --hard HEAD^ + test_cmp expect_error actual ) ' -- 2.18.0
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 wrote: > On Thu, Aug 2, 2018 at 9:41 AM SZEDER Gábor wrote: > > [...] > > > > > > 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. > > > 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? > 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?
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 wrote: > On Thu, Aug 2, 2018 at 6:47 AM Antonio Ospite wrote: > > [...] > > +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?
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 wrote: > On Thu, Aug 2, 2018 at 6:47 AM Antonio Ospite 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 > > --- [...] > > +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. [...] > > +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: 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 th
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 wrote: > Antonio Ospite 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?
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 wrote:
> Antonio Ospite 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.
> 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?
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 wrote: > Antonio Ospite 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. > The original 's--h config []' is quite understandable. It > is like "git config []", 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. > 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. > > 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?
Re: [RFC PATCH v2 04/12] submodule--helper: add a new 'config' subcommand
On Thu, 2 Aug 2018 15:20:33 -0400
Jeff King wrote:
> 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?
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 wrote: > On Thu, Aug 2, 2018 at 6:47 AM Antonio Ospite 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. > > > > 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. > > - 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 < M .gitmodules > D submod > EOF > cat >expect.both_deleted< 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?
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 wrote: > On Thu, Aug 2, 2018 at 6:47 AM Antonio Ospite wrote: > > > > The is_empty_file() function can be generally useful, move it to dir.c > > and make it public. > > > > Signed-off-by: Antonio Ospite > > 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?
[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
---
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;
+}
+
#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
[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 --- 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 diff --git a/t/t7416-submodule-sparse-gitmodules.sh b/t/t7416-submodule-sparse-gitmodules.sh new file mode 100755 index 00..3c7a53316b --- /dev/null +++ b/t/t7416-submodule-sparse-gitmodules.sh @@ -0,0 +1,112 @@ +#!/bin/sh +# +# Copyright (C) 2018 Antonio Ospite +# + +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
[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 --- 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
[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
---
builtin/am.c | 15 ---
dir.c| 16
dir.h| 1 +
3 files changed, 17 insertions(+), 15 deletions(-)
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, ) < 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, ) < 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
[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
---
builtin/submodule--helper.c | 54 +++--
t/t7411-submodule-config.sh | 35
2 files changed, 87 insertions(+), 2 deletions(-)
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", _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(_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(_index);
+
+ if (write_locked_index(_index, _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.su
[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 --- git-submodule.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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
[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
---
submodule-config.c | 12
submodule-config.h | 1 +
submodule.c| 10 +++---
3 files changed, 16 insertions(+), 7 deletions(-)
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(, "submodule.");
strbuf_addstr(, submodule->name);
strbuf_addstr(, ".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();
- return -1;
- }
+ ret = config_set_in_gitmodules_file_gently(entry.buf, newpath);
strbuf_release();
- return 0;
+ return ret;
}
/*
--
2.18.0
[RFC PATCH v2 01/12] submodule: add a print_config_from_gitmodules() helper
This will be used to print values just like "git config -f .gitmodules"
would.
Signed-off-by: Antonio Ospite
---
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, _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
[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.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
[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
---
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(-)
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
[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 --- 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(-) 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
[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
---
builtin/submodule--helper.c | 17 -
cache.h | 1 +
submodule-config.c | 16 ++--
3 files changed, 31 insertions(+), 3 deletions(-)
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,
) >= 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, ) >= 0)
+ config_source.blob = GITMODULES_HEAD;
+
+ config_with_options(fn, data, _source, );
+
free(file);
}
}
--
2.18.0
[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
---
submodule.c| 10 --
t/t3600-rm.sh | 32
t/t7400-submodule-basic.sh | 11 +++
3 files changed, 35 insertions(+), 18 deletions(-)
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(_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 popula
Re: [PATCH v2 5/6] submodule-config: pass repository as argument to config_from_gitmodules
On Tue, 26 Jun 2018 13:15:33 -0700 Junio C Hamano wrote: > Antonio Ospite writes: > > > Generlize config_from_gitmodules to accept a repository as an argument. > > generalize??? > Of course I was going to miss a typo in the first word of the commit message :| If this is the only change, I'd ask you to amend it when applying the patch, if it's not too much trouble. If instead I have to add also the comments about the new public functions in submodule-config.c, as you asked for patch 2/6, I can send a v3 and fix the typo there. 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?
Re: [PATCH v2 2/6] submodule-config: add helper function to get 'fetch' config from .gitmodules
On Tue, 26 Jun 2018 13:11:40 -0700
Junio C Hamano wrote:
> Antonio Ospite writes:
>
> > Add a helper function to make it clearer that retrieving 'fetch'
> > configuration from the .gitmodules file is a special case supported
> > solely for backward compatibility purposes.
> > ...
>
> Then perhaps the new public function deserves a comment stating
> that?
>
Hi Junio,
a comment about that is added to submodule-config.h in patch 4/6 in
place of the comment about config_from_gitmodules.
I can add a note here as well if that one is not enough.
[...]
> > +void fetch_config_from_gitmodules(int *max_children, int
> > *recurse_submodules)
> > +{
> > + struct fetch_config config = {
> > + .max_children = max_children,
> > + .recurse_submodules = recurse_submodules
> > + };
>
> We started using designated initializers some time ago, and use of
> it improves readability of something like this ;-)
>
Ah, TBH I didn't even consider whether it was allowed in git code, I
just used the construct out of habit.
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?
[PATCH v2 4/6] submodule-config: make 'config_from_gitmodules' private
Now that 'config_from_gitmodules' is not used in the open, it can be
marked as private.
Hopefully this will prevent its usage for retrieving arbitrary
configuration form the '.gitmodules' file.
Signed-off-by: Antonio Ospite
---
submodule-config.c | 8
submodule-config.h | 12 +---
2 files changed, 9 insertions(+), 11 deletions(-)
diff --git a/submodule-config.c b/submodule-config.c
index 9a2b13d8b..cd1f1e06a 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -673,14 +673,14 @@ void submodule_free(struct repository *r)
}
/*
- * Note: This function exists solely to maintain backward compatibility with
- * 'fetch' and 'update_clone' storing configuration in '.gitmodules' and should
- * NOT be used anywhere else.
+ * Note: This function is private for a reason, the '.gitmodules' file should
+ * not be used as as a mechanism to retrieve arbitrary configuration stored in
+ * the repository.
*
* Runs the provided config function on the '.gitmodules' file found in the
* working directory.
*/
-void config_from_gitmodules(config_fn_t fn, void *data)
+static void config_from_gitmodules(config_fn_t fn, void *data)
{
if (the_repository->worktree) {
char *file = repo_worktree_path(the_repository,
GITMODULES_FILE);
diff --git a/submodule-config.h b/submodule-config.h
index b6f19d0d4..dc7278eea 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -57,15 +57,13 @@ void submodule_free(struct repository *r);
int check_submodule_name(const char *name);
/*
- * Note: This function exists solely to maintain backward compatibility with
- * 'fetch' and 'update_clone' storing configuration in '.gitmodules' and should
- * NOT be used anywhere else.
+ * Note: these helper functions exist solely to maintain backward
+ * compatibility with 'fetch' and 'update_clone' storing configuration in
+ * '.gitmodules'.
*
- * Runs the provided config function on the '.gitmodules' file found in the
- * working directory.
+ * New helpers to retrieve arbitrary configuration from the '.gitmodules' file
+ * should NOT be added.
*/
-extern void config_from_gitmodules(config_fn_t fn, void *data);
-
extern void fetch_config_from_gitmodules(int *max_children, int
*recurse_submodules);
extern void update_clone_config_from_gitmodules(int *max_jobs);
--
2.18.0
[PATCH v2 1/6] config: move config_from_gitmodules to submodule-config.c
The .gitmodules file is not meant as a place to store arbitrary
configuration to distribute with the repository.
Move config_from_gitmodules() out of config.c and into
submodule-config.c to make it even clearer that it is not a mechanism to
retrieve arbitrary configuration from the .gitmodules file.
Signed-off-by: Antonio Ospite
---
config.c | 17 -
config.h | 10 --
submodule-config.c | 17 +
submodule-config.h | 11 +++
4 files changed, 28 insertions(+), 27 deletions(-)
diff --git a/config.c b/config.c
index a0a6ae198..fa78b1ff9 100644
--- a/config.c
+++ b/config.c
@@ -2172,23 +2172,6 @@ int git_config_get_pathname(const char *key, const char
**dest)
return repo_config_get_pathname(the_repository, key, dest);
}
-/*
- * Note: This function exists solely to maintain backward compatibility with
- * 'fetch' and 'update_clone' storing configuration in '.gitmodules' and should
- * NOT be used anywhere else.
- *
- * Runs the provided config function on the '.gitmodules' file found in the
- * working directory.
- */
-void config_from_gitmodules(config_fn_t fn, void *data)
-{
- if (the_repository->worktree) {
- char *file = repo_worktree_path(the_repository,
GITMODULES_FILE);
- git_config_from_file(fn, file, data);
- free(file);
- }
-}
-
int git_config_get_expiry(const char *key, const char **output)
{
int ret = git_config_get_string_const(key, output);
diff --git a/config.h b/config.h
index 626d4654b..b95bb7649 100644
--- a/config.h
+++ b/config.h
@@ -215,16 +215,6 @@ extern int repo_config_get_maybe_bool(struct repository
*repo,
extern int repo_config_get_pathname(struct repository *repo,
const char *key, const char **dest);
-/*
- * Note: This function exists solely to maintain backward compatibility with
- * 'fetch' and 'update_clone' storing configuration in '.gitmodules' and should
- * NOT be used anywhere else.
- *
- * Runs the provided config function on the '.gitmodules' file found in the
- * working directory.
- */
-extern void config_from_gitmodules(config_fn_t fn, void *data);
-
extern int git_config_get_value(const char *key, const char **value);
extern const struct string_list *git_config_get_value_multi(const char *key);
extern void git_config_clear(void);
diff --git a/submodule-config.c b/submodule-config.c
index 388ef1f89..b431555db 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -671,3 +671,20 @@ void submodule_free(struct repository *r)
if (r->submodule_cache)
submodule_cache_clear(r->submodule_cache);
}
+
+/*
+ * Note: This function exists solely to maintain backward compatibility with
+ * 'fetch' and 'update_clone' storing configuration in '.gitmodules' and should
+ * NOT be used anywhere else.
+ *
+ * Runs the provided config function on the '.gitmodules' file found in the
+ * working directory.
+ */
+void config_from_gitmodules(config_fn_t fn, void *data)
+{
+ if (the_repository->worktree) {
+ char *file = repo_worktree_path(the_repository,
GITMODULES_FILE);
+ git_config_from_file(fn, file, data);
+ free(file);
+ }
+}
diff --git a/submodule-config.h b/submodule-config.h
index ca1f94e2d..5148801f4 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -2,6 +2,7 @@
#define SUBMODULE_CONFIG_CACHE_H
#include "cache.h"
+#include "config.h"
#include "hashmap.h"
#include "submodule.h"
#include "strbuf.h"
@@ -55,4 +56,14 @@ void submodule_free(struct repository *r);
*/
int check_submodule_name(const char *name);
+/*
+ * Note: This function exists solely to maintain backward compatibility with
+ * 'fetch' and 'update_clone' storing configuration in '.gitmodules' and should
+ * NOT be used anywhere else.
+ *
+ * Runs the provided config function on the '.gitmodules' file found in the
+ * working directory.
+ */
+extern void config_from_gitmodules(config_fn_t fn, void *data);
+
#endif /* SUBMODULE_CONFIG_H */
--
2.18.0
[PATCH v2 6/6] submodule-config: reuse config_from_gitmodules in repo_read_gitmodules
Reuse config_from_gitmodules in repo_read_gitmodules to remove some
duplication and also have a single point where the .gitmodules file is
read.
The change does not introduce any new behavior, the same gitmodules_cb
config callback is still used, which only deals with configuration
specific to submodules.
The check about the repo's worktree is removed from repo_read_gitmodules
because it's already performed in config_from_gitmodules.
The config_from_gitmodules function is moved up in the file —unchanged—
before its users to avoid a forward declaration.
Signed-off-by: Antonio Ospite
---
submodule-config.c | 50 +++---
1 file changed, 21 insertions(+), 29 deletions(-)
diff --git a/submodule-config.c b/submodule-config.c
index 602c46af2..77421a497 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -591,6 +591,23 @@ static void submodule_cache_check_init(struct repository
*repo)
submodule_cache_init(repo->submodule_cache);
}
+/*
+ * Note: This function is private for a reason, the '.gitmodules' file should
+ * not be used as as a mechanism to retrieve arbitrary configuration stored in
+ * the repository.
+ *
+ * Runs the provided config function on the '.gitmodules' file found in the
+ * working directory.
+ */
+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);
+ free(file);
+ }
+}
+
static int gitmodules_cb(const char *var, const char *value, void *data)
{
struct repository *repo = data;
@@ -608,19 +625,11 @@ void repo_read_gitmodules(struct repository *repo)
{
submodule_cache_check_init(repo);
- if (repo->worktree) {
- char *gitmodules;
-
- if (repo_read_index(repo) < 0)
- return;
-
- gitmodules = repo_worktree_path(repo, GITMODULES_FILE);
-
- if (!is_gitmodules_unmerged(repo->index))
- git_config_from_file(gitmodules_cb, gitmodules, repo);
+ if (repo_read_index(repo) < 0)
+ return;
- free(gitmodules);
- }
+ if (!is_gitmodules_unmerged(repo->index))
+ config_from_gitmodules(gitmodules_cb, repo, repo);
repo->submodule_cache->gitmodules_read = 1;
}
@@ -672,23 +681,6 @@ void submodule_free(struct repository *r)
submodule_cache_clear(r->submodule_cache);
}
-/*
- * Note: This function is private for a reason, the '.gitmodules' file should
- * not be used as as a mechanism to retrieve arbitrary configuration stored in
- * the repository.
- *
- * Runs the provided config function on the '.gitmodules' file found in the
- * working directory.
- */
-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);
- free(file);
- }
-}
-
struct fetch_config {
int *max_children;
int *recurse_submodules;
--
2.18.0
[PATCH v2 3/6] submodule-config: add helper to get 'update-clone' config from .gitmodules
Add a helper function to make it clearer that retrieving 'update-clone'
configuration from the .gitmodules file is a special case supported
solely for backward compatibility purposes.
This change removes one direct use of 'config_from_gitmodules' for
options not strictly related to submodules: "submodule.fetchjobs" does
not describe a property of a submodule, but a behavior of other commands
when dealing with submodules, so it does not really belong to the
.gitmodules file.
This is in the effort to communicate better that .gitmodules is not to
be used as a mechanism to store arbitrary configuration in the
repository that any command can retrieve.
Signed-off-by: Antonio Ospite
---
builtin/submodule--helper.c | 8
submodule-config.c | 14 ++
submodule-config.h | 1 +
3 files changed, 19 insertions(+), 4 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 20ae9191c..110a47eca 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1706,8 +1706,8 @@ static int update_clone_task_finished(int result,
return 0;
}
-static int gitmodules_update_clone_config(const char *var, const char *value,
- void *cb)
+static int git_update_clone_config(const char *var, const char *value,
+ void *cb)
{
int *max_jobs = cb;
if (!strcmp(var, "submodule.fetchjobs"))
@@ -1757,8 +1757,8 @@ static int update_clone(int argc, const char **argv,
const char *prefix)
};
suc.prefix = prefix;
- config_from_gitmodules(gitmodules_update_clone_config, _jobs);
- git_config(gitmodules_update_clone_config, _jobs);
+ update_clone_config_from_gitmodules(_jobs);
+ git_config(git_update_clone_config, _jobs);
argc = parse_options(argc, argv, prefix, module_update_clone_options,
git_submodule_helper_usage, 0);
diff --git a/submodule-config.c b/submodule-config.c
index f44d6a777..9a2b13d8b 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -716,3 +716,17 @@ void fetch_config_from_gitmodules(int *max_children, int
*recurse_submodules)
};
config_from_gitmodules(gitmodules_fetch_config, );
}
+
+static int gitmodules_update_clone_config(const char *var, const char *value,
+ void *cb)
+{
+ int *max_jobs = cb;
+ if (!strcmp(var, "submodule.fetchjobs"))
+ *max_jobs = parse_submodule_fetchjobs(var, value);
+ return 0;
+}
+
+void update_clone_config_from_gitmodules(int *max_jobs)
+{
+ config_from_gitmodules(gitmodules_update_clone_config, _jobs);
+}
diff --git a/submodule-config.h b/submodule-config.h
index cff297a75..b6f19d0d4 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -67,5 +67,6 @@ int check_submodule_name(const char *name);
extern void config_from_gitmodules(config_fn_t fn, void *data);
extern void fetch_config_from_gitmodules(int *max_children, int
*recurse_submodules);
+extern void update_clone_config_from_gitmodules(int *max_jobs);
#endif /* SUBMODULE_CONFIG_H */
--
2.18.0
[PATCH v2 2/6] submodule-config: add helper function to get 'fetch' config from .gitmodules
Add a helper function to make it clearer that retrieving 'fetch'
configuration from the .gitmodules file is a special case supported
solely for backward compatibility purposes.
This change removes one direct use of 'config_from_gitmodules' in code
not strictly related to submodules, in the effort to communicate better
that .gitmodules is not to be used as a mechanism to store arbitrary
configuration in the repository that any command can retrieve.
Signed-off-by: Antonio Ospite
---
builtin/fetch.c| 15 +--
submodule-config.c | 28
submodule-config.h | 2 ++
3 files changed, 31 insertions(+), 14 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index ea5b9669a..92a5d235d 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -93,19 +93,6 @@ static int git_fetch_config(const char *k, const char *v,
void *cb)
return git_default_config(k, v, cb);
}
-static int gitmodules_fetch_config(const char *var, const char *value, void
*cb)
-{
- if (!strcmp(var, "submodule.fetchjobs")) {
- max_children = parse_submodule_fetchjobs(var, value);
- return 0;
- } else if (!strcmp(var, "fetch.recursesubmodules")) {
- recurse_submodules = parse_fetch_recurse_submodules_arg(var,
value);
- return 0;
- }
-
- return 0;
-}
-
static int parse_refmap_arg(const struct option *opt, const char *arg, int
unset)
{
/*
@@ -1433,7 +1420,7 @@ int cmd_fetch(int argc, const char **argv, const char
*prefix)
for (i = 1; i < argc; i++)
strbuf_addf(_rla, " %s", argv[i]);
- config_from_gitmodules(gitmodules_fetch_config, NULL);
+ fetch_config_from_gitmodules(_children, _submodules);
git_config(git_fetch_config, NULL);
argc = parse_options(argc, argv, prefix,
diff --git a/submodule-config.c b/submodule-config.c
index b431555db..f44d6a777 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -688,3 +688,31 @@ void config_from_gitmodules(config_fn_t fn, void *data)
free(file);
}
}
+
+struct fetch_config {
+ int *max_children;
+ int *recurse_submodules;
+};
+
+static int gitmodules_fetch_config(const char *var, const char *value, void
*cb)
+{
+ struct fetch_config *config = cb;
+ if (!strcmp(var, "submodule.fetchjobs")) {
+ *(config->max_children) = parse_submodule_fetchjobs(var, value);
+ return 0;
+ } else if (!strcmp(var, "fetch.recursesubmodules")) {
+ *(config->recurse_submodules) =
parse_fetch_recurse_submodules_arg(var, value);
+ return 0;
+ }
+
+ return 0;
+}
+
+void fetch_config_from_gitmodules(int *max_children, int *recurse_submodules)
+{
+ struct fetch_config config = {
+ .max_children = max_children,
+ .recurse_submodules = recurse_submodules
+ };
+ config_from_gitmodules(gitmodules_fetch_config, );
+}
diff --git a/submodule-config.h b/submodule-config.h
index 5148801f4..cff297a75 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -66,4 +66,6 @@ int check_submodule_name(const char *name);
*/
extern void config_from_gitmodules(config_fn_t fn, void *data);
+extern void fetch_config_from_gitmodules(int *max_children, int
*recurse_submodules);
+
#endif /* SUBMODULE_CONFIG_H */
--
2.18.0
