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

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

From
Denton Liu <liu.denton@gmail.com>
Date
May 14, 2020, 10:10 UTC
Message-ID
<20200514101013.GA28018@generichostname>
In-Reply-To
<20200513201737.55778-1-shouryashukla.oo@gmail.com>
Hi Shourya,
On Thu, May 14, 2020 at 01:47:36AM +0530, Shourya Shukla wrote:
Show 38 quoted lines
> Convert submodule subcommand 'set-branch' to a builtin. Port 'set-branch'
> to 'submodule--helper.c' and call the latter via 'git-submodule.sh'.
> 
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
> Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>
> ---
> Here is another conversion, this time it is 'set-branch'. It passes all
> the tests in t7419. I am aware that there are some repetitve parts in
> the conversion as well as variables which can be named better. I would
> love everyone's suggestion on this and how this can be made better.
> 
> The extra '$branch' on line 752 was because of Christian's help after
> reference from TLDP's Parameter Subsitution documentation:
> https://www.tldp.org/LDP/abs/html/parameter-substitution.html
> 
> Similarly, I had to change a coouple of other lines in the shell version
> so as to make it compatible with the C version.
> 
> Thank you so much Christian and Kaartic for the mentoring, this wouldn't
> have been possible otherwise :)
> 
>  builtin/submodule--helper.c | 58 +++++++++++++++++++++++++++++++++++++
>  git-submodule.sh            | 31 ++------------------
>  2 files changed, 60 insertions(+), 29 deletions(-)
> 
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index f50745a03f..5a8815b76e 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2284,6 +2284,63 @@ static int module_set_url(int argc, const char **argv, const char *prefix)
>  	return 0;
>  }
>  
> +static int module_set_branch(int argc, const char **argv, const char *prefix)
> +{
> +	int quiet = 0, opt_branch = 0, opt_default = 0;
> +	const char *newbranch;
nit: I would call this new_branch
Show 7 quoted lines
> +	const char *path;
> +	char *config_name;
> +
> +	struct option options[] = {
> +		OPT__QUIET(&quiet, N_("Suppress output for setting default tracking branch of a submodule")),
> +		OPT_BOOL(0, "default", &opt_default, N_("Set the default tracking branch to master")),
> +		OPT_BOOL(0, "branch", &opt_branch, N_("Set the default tracking branch to the one specified")),

This should use OPT_STRING and accept a string argument instead of using the implicit command-line ordering.

Show 12 quoted lines
> +		OPT_END()
> +	};
> +	const char *const usage[] = {
> +		N_("git submodule--helper set-branch [--quiet] (-d|--default) <path>"),
> +		N_("git submodule--helper set-branch [--quiet] (-b|--branch) <branch> <path>"),
> +		NULL
> +	};
> +
> +	argc = parse_options(argc, argv, prefix, options, usage, 0);
> +
> +	if ((!opt_branch && !opt_default))
> +		die(_("At least one of --branch and --default required"));

Error messages in Git are generally written without capitalising the first letter of the sentence.

> +
> +	if (opt_branch) {
> +		if (opt_default)
> +			die(_("--branch and --default do not make sense"));

This error message should be qualified, perhaps with something like "do not make sense together".

> +
> +		newbranch = argv[0];
> +		path = argv[1];

These assignments are incorrect since we haven't check argc yet. Also, they're redundant since you have the assignments in the if statement below.

Also, if you do the OPT_STRING thing as described above, you can do the `path = ...` outside of the if-statement since it'll be common to both -d and -b.

Show 8 quoted lines
> +
> +		if (argc != 2 || !(newbranch = argv[0]) || !(path = argv[1]))
> +			usage_with_options(usage, options);
> +
> +		config_name = xstrfmt("submodule.%s.branch", path);
> +		config_set_in_gitmodules_file_gently(config_name, newbranch);
> +
> +		printf(_("Default tracking branch set to '%s' successfully\n"), newbranch);

The original function did not print anything. We shouldn't alter the behaviour if we're just porting it over so we should delete this.

Show 15 quoted lines
> +		free(config_name);
> +	}
> +
> +	if (opt_default) {
> +		path = argv[0];
> +
> +		if (argc != 1 || !(path = argv[0]))
> +			usage_with_options(usage, options);
> +
> +		config_name = xstrfmt("submodule.%s.branch", path);
> +		config_set_in_gitmodules_file_gently(config_name, NULL);
> +
> +		printf(_("Default tracking branch set to 'master' successfully\n"));
> +		free(config_name);
> +	}

The same arguments for the above apply to this case too. Actually, the only place where they both really differ is in the call to config_set_in_gitmodules_file_gently(). Can you do all of your argument checks together? Something like

	if (!!new_branch == opt_default)
		usage_with_options(usage, options);

Then the call to config_set_in_gitmodules_file_gently() could look like this:

	config_name = xstrfmt("submodule.%s.branch", path);
	config_set_in_gitmodules_file_gently(config_name, new_branch ? new_branch : NULL);
	free(config_name);
and we wouldn't need the ifs at all.
Show 68 quoted lines
> +
> +	return 0;
> +}
> +
>  #define SUPPORT_SUPER_PREFIX (1<<0)
>  
>  struct cmd_struct {
> @@ -2315,6 +2372,7 @@ static struct cmd_struct commands[] = {
>  	{"check-name", check_name, 0},
>  	{"config", module_config, 0},
>  	{"set-url", module_set_url, 0},
> +	{"set-branch", module_set_branch, 0},
>  };
>  
>  int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 39ebdf25b5..2438ef576e 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -719,7 +719,6 @@ cmd_update()
>  # $@ = requested path
>  #
>  cmd_set_branch() {
> -	unset_branch=false
>  	branch=
>  
>  	while test $# -ne 0
> @@ -729,7 +728,7 @@ cmd_set_branch() {
>  			# we don't do anything with this but we need to accept it
>  			;;
>  		-d|--default)
> -			unset_branch=true
> +			default=1
>  			;;
>  		-b|--branch)
>  			case "$2" in '') usage ;; esac
> @@ -750,33 +749,7 @@ cmd_set_branch() {
>  		shift
>  	done
>  
> -	if test $# -ne 1
> -	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
> -
> -	test -n "$branch"; has_branch=$?
> -	test "$unset_branch" = true; has_unset_branch=$?
> -
> -	if test $((!$has_branch != !$has_unset_branch)) -eq 0
> -	then
> -		usage
> -	fi
> -
> -	if test $has_branch -eq 0
> -	then
> -		git submodule--helper config submodule."$name".branch "$branch"
> -	else
> -		git submodule--helper config --unset submodule."$name".branch
> -	fi
> +	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper set-branch ${GIT_QUIET:+--quiet} ${branch:+--branch $branch} ${default:+--default} -- "$@"
The shell script portion looks good.
Thanks,
Denton
Show 6 quoted lines
>  }
>  
>  #
> -- 
> 2.26.2
>
Previous: Kaartic SivaraamNext: Shourya Shukla
Message 11 of 14 in “submodule: port subcommand 'set-branch' from shell to C”
  1. 1/2 submodule: port subcommand 'set-branch' from shell to CShourya Shukla, May 13, 2020
  2. 2/2 submodule: Add 'quiet' option in subcommand 'set-branch'Shourya Shukla, May 13, 2020
  3. Denton LiuMay 14, 2020
  4. Shourya ShuklaMay 16, 2020
  5. Denton LiuMay 16, 2020
  6. Shourya ShuklaMay 16, 2020
  7. Denton LiuMay 16, 2020
  8. Kaartic SivaraamMay 14, 2020
  9. Junio C HamanoMay 17, 2020
  10. Kaartic SivaraamMay 17, 2020
  11. Denton LiuMay 14, 2020
  12. Shourya ShuklaMay 16, 2020
  13. Denton LiuMay 16, 2020
  14. Đoàn Trần Công DanhMay 17, 2020

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

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