threads / rfc / 53246

RFC patch, 2 partssubmodule: port 'set-url' from shell to C

Subject: [RFC][PATCH 0/2] submodule: port 'set-url' from shell to C

## tl;dr

8 messages between Apr 16, 2020 and Apr 27, 2020. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

Shourya Shukla· Apr 16, 2020, 21:04 UTC · lore
Hello all,

This is my very first attempt at the conversion of subcommand 'set-url' from shell to C, thus making the subcommand a builtin.

I have based my conversion by looking at the way other subcommands have been implemented in 'submodule--helper.c' as well as the subcommand 'set-url' in 'remote.c'. The approach I have taken is as follows:

1. Create a helper function 'update_url_in_gitmodules()' in 'submodule.c'
   to update the URL of an entry in '.gitmodules'.
2. Port the function 'cmd_set_url()' in 'git-submodule.sh' to 'module_set_url()'
   in 'submodule--helper.c'.
The issues I am facing are:
1. The patch fails test #2 in t7420, i.e., the test to verify the working of 'set-url'
   subcommand.
2. Though not an issue affecting the patch, but the 'usage' prompt of 'git submodule'
   does not show the subcommands 'set-url' and 'set-branch'.

Also, I am aware that the patch does not support the '--quiet' option. This is something I intended to add after ensuring that the patch works in the first place.

I understand that the patch is in a weak condition right now. How can this patch be improved? Any guidance from the community would be appreciated! :)

Thanks, Shourya Shukla

Shourya Shukla (2):
  submodule.c: update URL in .gitmodules using update_url_in_gitmodules
  submodule: port subcommand 'set-url' from shell to C
 builtin/submodule--helper.c | 38 +++++++++++++++++++++++++++++++++++++
 git-submodule.sh            | 23 ++--------------------
 submodule.c                 | 33 ++++++++++++++++++++++++++++++++
 submodule.h                 |  2 ++
 4 files changed, 75 insertions(+), 21 deletions(-)
-- 
2.20.1
Shourya Shukla· Apr 16, 2020, 21:04 UTC · re: Shourya Shukla · lore

[PATCH 1/2] submodule.c: update URL in .gitmodules using update_url_in_gitmodules

Create a helper function update_url_in_gitmodules() to update URL of an entry in .gitmodules. Later on, we want to use this function to aid in the conversion of 'set-url' from shell to C.

Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>
---
 submodule.c | 33 +++++++++++++++++++++++++++++++++
 submodule.h |  2 ++
 2 files changed, 35 insertions(+)
Show changes to 2 files +35 −0

submodule.c, submodule.h

diff --git a/submodule.c b/submodule.c
index c3aadf3fff..0b599dc4e1 100644
--- a/submodule.c
+++ b/submodule.c
@@ -126,6 +126,39 @@ int update_path_in_gitmodules(const char *oldpath, const char *newpath)
 	return ret;
 }
 
+/*
+ * Try to update the "url" entry in the "submodule.<name>" section of the
+ * .gitmodules file. Return 0 only if a .gitmodules file was found, a section
+ * with the correct url=<oldurl> setting was found and we could update it.
+ */
+int update_url_in_gitmodules(const char *path, const char *newurl)
+{
+	struct strbuf entry = STRBUF_INIT;
+	const struct submodule *submodule;
+	int ret;
+
+	/* Do nothing without .gitmodules */
+	if (!file_exists(GITMODULES_FILE))
+		return -1;
+
+	if (is_gitmodules_unmerged(the_repository->index))
+		die(_("Cannot change unmerged .gitmodules, resolve merge conflicts first"));
+
+	submodule = submodule_from_path(the_repository, &null_oid, path);
+	if (!submodule || !submodule->name) {
+		warning(_("Could not find section in .gitmodules where path=%s"), path);
+		return -1;
+	}
+	
+	strbuf_addstr(&entry, "submodule.");
+	strbuf_addstr(&entry, submodule->name);
+	strbuf_addstr(&entry, ".url");
+	ret = config_set_in_gitmodules_file_gently(entry.buf, newurl);
+	strbuf_release(&entry);
+	
+	return ret;
+}
+
 /*
  * Try to remove the "submodule.<name>" section from .gitmodules where the given
  * path is configured. Return 0 only if a .gitmodules file was found, a section
diff --git a/submodule.h b/submodule.h
index 4dad649f94..ea09480433 100644
--- a/submodule.h
+++ b/submodule.h
@@ -49,6 +49,8 @@ void set_diffopt_flags_from_submodule_config(struct diff_options *,
 					     const char *path);
 int git_default_submodule_config(const char *var, const char *value, void *cb);
 
+int update_url_in_gitmodules(const char* path, const char *newurl);
+
 struct option;
 int option_parse_recurse_submodules_worktree_updater(const struct option *opt,
 						     const char *arg, int unset);

-- 
2.20.1
Shourya Shukla· Apr 16, 2020, 21:04 UTC · re: Shourya Shukla · lore

[PATCH 2/2] submodule: port subcommand 'set-url' from shell to C

This aims to convert submodule subcommand 'set-url' to a builtin. 'set-url' is ported to 'submodule--helper.c' and the latter is called via 'git-submodule.sh'

Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>
---
 builtin/submodule--helper.c | 38 +++++++++++++++++++++++++++++++++++++
 git-submodule.sh            | 23 ++--------------------
 2 files changed, 40 insertions(+), 21 deletions(-)
Show changes to 2 files +40 −21

builtin/submodule--helper.c, git-submodule.sh

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 1a4b391c88..10e774a5c9 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2246,6 +2246,43 @@ static int module_config(int argc, const char **argv, const char *prefix)
 	usage_with_options(git_submodule_helper_usage, module_config_options);
 }
 
+static int module_set_url(int argc, const char **argv, const char *prefix)
+{
+	const char *newurl = NULL;
+	const char *path = NULL;
+
+	struct pathspec pathspec;
+	struct module_list list;
+
+	int quiet = 0;
+
+	struct option module_set_url_options[] = {
+		OPT__QUIET(&quiet, N_("Suppress output for setting url of a submodule")),
+		OPT_END()
+	};
+
+	const char *const git_submodule_helper_usage[] = {
+		N_("git submodule [--quiet] set-url [--] <path> <newurl>"),
+		NULL
+	};
+
+	argc = parse_options(argc, argv, NULL, module_set_url_options,
+			     git_submodule_helper_usage, 0);
+
+	path = argv[1];
+	newurl = argv[2];
+
+	if(!path || !newurl){
+		usage_with_options(git_submodule_helper_usage,module_set_url_options);
+		return 1;
+	}
+
+	if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)
+		return 1;
+
+	return update_url_in_gitmodules(path, newurl);
+}
+
 #define SUPPORT_SUPER_PREFIX (1<<0)
 
 struct cmd_struct {
@@ -2276,6 +2313,7 @@ static struct cmd_struct commands[] = {
 	{"is-active", is_active, 0},
 	{"check-name", check_name, 0},
 	{"config", module_config, 0},
+	{"set-url", module_set_url, 0},
 };
 
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
diff --git a/git-submodule.sh b/git-submodule.sh
index 89f915cae9..f0304c3e19 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -783,6 +783,7 @@ cmd_set_branch() {
 # $@ = requested path, requested url
 #
 cmd_set_url() {
+
 	while test $# -ne 0
 	do
 		case "$1" in
@@ -803,27 +804,7 @@ cmd_set_url() {
 		shift
 	done
 
-	if test $# -ne 2
-	then
-		usage
-	fi
-
-	# we can't use `git submodule--helper name` here because internally, it
-	# hashes the path so a trailing slash could lead to an unintentional no match
-	name="$(git submodule--helper list "$1" | cut -f2)"
-	if test -z "$name"
-	then
-		exit 1
-	fi
-
-	url="$2"
-	if test -z "$url"
-	then
-		exit 1
-	fi
-
-	git submodule--helper config submodule."$name".url "$url"
-	git submodule--helper sync ${GIT_QUIET:+--quiet} "$name"
+	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper set-url ${GIT_QUIET:+--quiet} -- "$@"
 }
 
 #

-- 
2.20.1
Christian Couder· Apr 17, 2020, 22:13 UTC · re: Shourya Shukla · lore

Re: [PATCH 2/2] submodule: port subcommand 'set-url' from shell to C

On Thu, Apr 16, 2020 at 11:05 PM Shourya Shukla <shouryashukla.oo@gmail.com> wrote:

> -       git submodule--helper config submodule."$name".url "$url"
> -       git submodule--helper sync ${GIT_QUIET:+--quiet} "$name"
> +       git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper set-url ${GIT_QUIET:+--quiet} -- "$@"

Maybe I missed something but I see nothing in the C code that does what the `git submodule--helper sync ...` call does. Maybe for now it would work better if that call was kept after the the call to `git ... submodule--helper set-url ...`

Christian Couder· Apr 17, 2020, 22:09 UTC · re: Shourya Shukla · lore

Re: [RFC][PATCH 0/2] submodule: port 'set-url' from shell to C

Hi,

On Thu, Apr 16, 2020 at 11:05 PM Shourya Shukla <shouryashukla.oo@gmail.com> wrote:

Show 9 quoted lines
> This is my very first attempt at the conversion of subcommand 'set-url' from shell
> to C, thus making the subcommand a builtin.
>
> I have based my conversion by looking at the way other subcommands have been implemented
> in 'submodule--helper.c' as well as the subcommand 'set-url' in 'remote.c'. The approach
> I have taken is as follows:
>
> 1. Create a helper function 'update_url_in_gitmodules()' in 'submodule.c'
>    to update the URL of an entry in '.gitmodules'.
We usually don't add a function that is unused in the commit that adds it.
> 2. Port the function 'cmd_set_url()' in 'git-submodule.sh' to 'module_set_url()'
>    in 'submodule--helper.c'.
So it seems to me that you could squash the two commits into one.

Thanks, Christian.

Denton Liu· Apr 20, 2020, 08:19 UTC · re: Shourya Shukla · lore

Re: [RFC][PATCH 0/2] submodule: port 'set-url' from shell to C

Hi Shourya,
On Fri, Apr 17, 2020 at 02:34:54AM +0530, Shourya Shukla wrote:
> The issues I am facing are:
> 
> 1. The patch fails test #2 in t7420, i.e., the test to verify the working of 'set-url'
>    subcommand.

The 'set-url' command implicitly runs sync once it is changed. I would go further than what Christian suggests and just call sync_submodule() (in C) at the end of module_set_url().

> 2. Though not an issue affecting the patch, but the 'usage' prompt of 'git submodule'
>    does not show the subcommands 'set-url' and 'set-branch'.

Hmmm, the first of the pair, 'set-branch', was introduced in v2.22.0. It seems like you're running an older version of Git, 2.20.1. Stupid question but are you sure you're running the correct git? For me this runs correctly on the latest 'master':

	$ ./bin-wrappers/git submodule -h
	usage: git submodule [--quiet] [--cached]
	   or: git submodule [--quiet] add [-b <branch>] [-f|--force] [--name <name>] [--reference <repository>] [--] <repository> [<path>]
	   or: git submodule [--quiet] status [--cached] [--recursive] [--] [<path>...]
	   or: git submodule [--quiet] init [--] [<path>...]
	   or: git submodule [--quiet] deinit [-f|--force] (--all| [--] <path>...)
	   or: git submodule [--quiet] update [--init] [--remote] [-N|--no-fetch] [-f|--force] [--checkout|--merge|--rebase] [--[no-]recommend-shallow] [--reference <repository>] [--recursive] [--[no-]single-branch] [--] [<path>...]
	   or: git submodule [--quiet] set-branch (--default|--branch <branch>) [--] <path>
	   or: git submodule [--quiet] set-url [--] <path> <newurl>
	   or: git submodule [--quiet] summary [--cached|--files] [--summary-limit <n>] [commit] [--] [<path>...]
	   or: git submodule [--quiet] foreach [--recursive] <command>
	   or: git submodule [--quiet] sync [--recursive] [--] [<path>...]
	   or: git submodule [--quiet] absorbgitdirs [--] [<path>...]
Thanks,
Denton
Shourya Shukla· Apr 27, 2020, 07:19 UTC · re: Denton Liu · lore

Re: [RFC][PATCH 0/2] submodule: port 'set-url' from shell to C

On 20/04 04:19, Denton Liu wrote:
Show 6 quoted lines
> > 1. The patch fails test #2 in t7420, i.e., the test to verify the working of 'set-url'
> >    subcommand.
> 
> The 'set-url' command implicitly runs sync once it is changed. I would
> go further than what Christian suggests and just call sync_submodule()
> (in C) at the end of module_set_url().

I have implemented this, yet running the test still gives an error. The function 'init_pathspec_item' in 'pathspec.c' tends to fail. Going further deep, the function 'prefix_path_gently' fails. I think this is happening because of the relative path outside the superproject for '../newsubmodule' and hence it throws a problem with prefixing.

The exact function responsible here is 'normalize_path_copy' in 'path.c'.

On doing '-i -v' while running the test. The problem comes down to:
fatal: ../newsubmodule: '../newsubmodule' is outside repository at '<path>/git/t/trash directory.t7420-submodule-set-url/super'

What exactly is wrong here and how should this problem and similar ones (if encountered) be approached? Do we follow certain procedures when debugging problems at such an intricate level?

Regards, Shourya Shukla

Denton Liu· Apr 27, 2020, 10:17 UTC · re: Shourya Shukla · lore

Re: [RFC][PATCH 0/2] submodule: port 'set-url' from shell to C

Hi Shourya,
On Mon, Apr 27, 2020 at 12:49:20PM +0530, Shourya Shukla wrote:
Show 13 quoted lines
> On 20/04 04:19, Denton Liu wrote:
> > > 1. The patch fails test #2 in t7420, i.e., the test to verify the working of 'set-url'
> > >    subcommand.
> > 
> > The 'set-url' command implicitly runs sync once it is changed. I would
> > go further than what Christian suggests and just call sync_submodule()
> > (in C) at the end of module_set_url().
> 
> I have implemented this, yet running the test still gives an error. The
> function 'init_pathspec_item' in 'pathspec.c' tends to fail. Going
> further deep, the function 'prefix_path_gently' fails. I think this is
> happening because of the relative path outside the superproject for
> '../newsubmodule' and hence it throws a problem with prefixing.

I can't think of a reason why Git should be trying to do anything with the path `../newsubmodule`. The set-url part should only write that string into the `.gitmodules` and the sync part should only write into `.git/config`. Neither of these steps should actually access the submodule repo in any way.

Show 6 quoted lines
> The exact function responsible here is 'normalize_path_copy' in
> 'path.c'.
> 
> On doing '-i -v' while running the test. The problem comes down to:
> 
> fatal: ../newsubmodule: '../newsubmodule' is outside repository at '<path>/git/t/trash directory.t7420-submodule-set-url/super'

Perhaps it's failing at the `git submodule update --remote` part? It's hard to tell without more information. One thing you can do is try running the test with `-x` which would enable shell tracing. This gives even more information on the test that's running, including exactly which command is failing.

> What exactly is wrong here and how should this problem
> and similar ones (if encountered) be approached? Do we follow
> certain procedures when debugging problems at such an intricate level?

I can't tell exactly what's wrong from the information you've provided. Do you have link to the code you're working on? Alternatively, it might be a good idea to post a RFC patch onto this list for wider discussion.

Some general debugging techniques that I've found helpful include
	* debug prints using fprintf(stderr, ...)
	* `-x` for test scripts (as mentioned above)
	* `debug` in test scripts (which launches GDB)
	* using `GIT_DEBUGGER=1 ./bin-wrappers/git <subcommand>` (which
	  also launches GDB)

although I'm not quite sure how applicable they are to your case. Maybe it would be helpful for you to grab a stack trace of where the die() is being called in GDB to understand the cause of the problem?

-Denton
> Regards,
> Shourya Shukla

← back to recent threads