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

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

[Shourya Shukla]

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

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

[Shourya Shukla]

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(+)

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

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

[Shourya Shukla]

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(-)

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

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

[Christian Couder]

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 ...`

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

[Christian Couder]

Hi,

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

> 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.

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

[Denton Liu]

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

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

[Shourya Shukla]

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.

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

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

[Denton Liu]

Hi Shourya,

On Mon, Apr 27, 2020 at 12:49:20PM +0530, Shourya Shukla wrote:
> 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.

> 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