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

Re: [GSoC] [PATCH v2 6/9] submodule--helper: convert the bulk of cmd_add() to C

From
Đoàn Trần Công Danh <congdanhqx@gmail.com>
Date
Aug 6, 2021, 01:14 UTC
Message-ID
<YQyNDv3bzVZwpAVl@danh.dev>
In-Reply-To
<20210805074054.29916-7-raykar.ath@gmail.com>
On 2021-08-05 13:10:51+0530, Atharva Raykar <raykar.ath@gmail.com> wrote:
Show 119 quoted lines
> Introduce the 'add' subcommand to `submodule--helper.c` that does all
> the work 'submodule add' past the parsing of flags.
> 
> As with the previous conversions, this is meant to be a faithful
> conversion with no modification to the behaviour of `submodule add`.
> 
> Signed-off-by: Atharva Raykar <raykar.ath@gmail.com>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Helped-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
> Mentored-by: Shourya Shukla <periperidip@gmail.com>
> Based-on-patch-by: Shourya Shukla <periperidip@gmail.com>
> Based-on-patch-by: Prathamesh Chavan <pc44800@gmail.com>
> ---
>  builtin/submodule--helper.c | 160 ++++++++++++++++++++++++++++++++++++
>  git-submodule.sh            |  96 +---------------------
>  2 files changed, 162 insertions(+), 94 deletions(-)
> 
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 99aabf1078..05ae9ebe50 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -3046,6 +3046,165 @@ static int add_config(int argc, const char **argv, const char *prefix)
>  	return 0;
>  }
>  
> +static void die_on_index_match(const char *path, int force)
> +{
> +	struct pathspec ps;
> +	const char *args[] = { path, NULL };
> +	parse_pathspec(&ps, 0, PATHSPEC_PREFER_CWD, NULL, args);
> +
> +	if (read_cache_preload(NULL) < 0)
> +		die(_("index file corrupt"));
> +
> +	if (ps.nr) {
> +		int i;
> +		char *ps_matched = xcalloc(ps.nr, 1);
> +
> +		/* TODO: audit for interaction with sparse-index. */
> +		ensure_full_index(&the_index);
> +
> +		/*
> +		 * Since there is only one pathspec, we just need
> +		 * need to check ps_matched[0] to know if a cache
> +		 * entry matched.
> +		 */
> +		for (i = 0; i < active_nr; i++) {
> +			ce_path_match(&the_index, active_cache[i], &ps,
> +				      ps_matched);
> +
> +			if (ps_matched[0]) {
> +				if (!force)
> +					die(_("'%s' already exists in the index"),
> +					    path);
> +				if (!S_ISGITLINK(active_cache[i]->ce_mode))
> +					die(_("'%s' already exists in the index "
> +					      "and is not a submodule"), path);
> +				break;
> +			}
> +		}
> +		free(ps_matched);
> +	}
> +}
> +
> +static void die_on_repo_without_commits(const char *path)
> +{
> +	struct strbuf sb = STRBUF_INIT;
> +	strbuf_addstr(&sb, path);
> +	if (is_nonbare_repository_dir(&sb)) {
> +		struct object_id oid;
> +		if (resolve_gitlink_ref(path, "HEAD", &oid) < 0)
> +			die(_("'%s' does not have a commit checked out"), path);
> +	}
> +}
> +
> +static int module_add(int argc, const char **argv, const char *prefix)
> +{
> +	int force = 0, quiet = 0, progress = 0, dissociate = 0;
> +	struct add_data add_data = ADD_DATA_INIT;
> +
> +	struct option options[] = {
> +		OPT_STRING('b', "branch", &add_data.branch, N_("branch"),
> +			   N_("branch of repository to add as submodule")),
> +		OPT__FORCE(&force, N_("allow adding an otherwise ignored submodule path"),
> +			   PARSE_OPT_NOCOMPLETE),
> +		OPT__QUIET(&quiet, N_("print only error messages")),
> +		OPT_BOOL(0, "progress", &progress, N_("force cloning progress")),
> +		OPT_STRING(0, "reference", &add_data.reference_path, N_("repository"),
> +			   N_("reference repository")),
> +		OPT_BOOL(0, "dissociate", &dissociate, N_("borrow the objects from reference repositories")),
> +		OPT_STRING(0, "name", &add_data.sm_name, N_("name"),
> +			   N_("sets the submodule’s name to the given string "
> +			      "instead of defaulting to its path")),
> +		OPT_INTEGER(0, "depth", &add_data.depth, N_("depth for shallow clones")),
> +		OPT_END()
> +	};
> +
> +	const char *const usage[] = {
> +		N_("git submodule--helper add [<options>] [--] <repository> [<path>]"),
> +		NULL
> +	};
> +
> +	argc = parse_options(argc, argv, prefix, options, usage, 0);
> +
> +	if (!is_writing_gitmodules_ok())
> +		die(_("please make sure that the .gitmodules file is in the working tree"));
> +
> +	if (prefix && *prefix &&
> +	    add_data.reference_path && !is_absolute_path(add_data.reference_path))
> +		add_data.reference_path = xstrfmt("%s%s", prefix, add_data.reference_path);
> +
> +	if (argc == 0 || argc > 2)
> +		usage_with_options(usage, options);
> +
> +	add_data.repo = argv[0];
> +	if (argc == 1)
> +		add_data.sm_path = guess_dir_name_from_git_url(add_data.repo, 0, 0);
> +	else
> +		add_data.sm_path = xstrdup(argv[1]);
add_data.sm_path is allocated in this block (regardless of legs).
Show 40 quoted lines
> +	if (prefix && *prefix && !is_absolute_path(add_data.sm_path))
> +		add_data.sm_path = xstrfmt("%s%s", prefix, add_data.sm_path);
> +
> +	if (starts_with_dot_dot_slash(add_data.repo) ||
> +	    starts_with_dot_slash(add_data.repo)) {
> +		if (prefix)
> +			die(_("Relative path can only be used from the toplevel "
> +			      "of the working tree"));
> +
> +		/* dereference source url relative to parent's url */
> +		add_data.realrepo = compute_submodule_clone_url(add_data.repo, NULL, 1);
> +	} else if (is_dir_sep(add_data.repo[0]) || strchr(add_data.repo, ':')) {
> +		add_data.realrepo = add_data.repo;
> +	} else {
> +		die(_("repo URL: '%s' must be absolute or begin with ./|../"),
> +		    add_data.repo);
> +	}
> +
> +	/*
> +	 * normalize path:
> +	 * multiple //; leading ./; /./; /../;
> +	 */
> +	normalize_path_copy(add_data.sm_path, add_data.sm_path);
> +	strip_dir_trailing_slashes(add_data.sm_path);
> +
> +	die_on_index_match(add_data.sm_path, force);
> +	die_on_repo_without_commits(add_data.sm_path);
> +
> +	if (!force) {
> +		int exit_code = -1;
> +		struct strbuf sb = STRBUF_INIT;
> +		struct child_process cp = CHILD_PROCESS_INIT;
> +		cp.git_cmd = 1;
> +		cp.no_stdout = 1;
> +		strvec_pushl(&cp.args, "add", "--dry-run", "--ignore-missing",
> +			     "--no-warn-embedded-repo", add_data.sm_path, NULL);
> +		if ((exit_code = pipe_command(&cp, NULL, 0, NULL, 0, &sb, 0))) {
> +			strbuf_complete_line(&sb);
> +			fputs(sb.buf, stderr);
> +			return exit_code;
But, we don't free it when return from here.
Show 18 quoted lines
> +		}
> +		strbuf_release(&sb);
> +	}
> +
> +	if(!add_data.sm_name)
> +		add_data.sm_name = add_data.sm_path;
> +
> +	if (check_submodule_name(add_data.sm_name))
> +		die(_("'%s' is not a valid submodule name"), add_data.sm_name);
> +
> +	add_data.prefix = prefix;
> +	add_data.force = !!force;
> +	add_data.quiet = !!quiet;
> +	add_data.progress = !!progress;
> +	add_data.dissociate = !!dissociate;
> +
> +	if (add_submodule(&add_data))
> +		return 1;
And here.
> +	configure_added_submodule(&add_data);
> +	free(add_data.sm_path);
However, it will be free()-d here, is it intended?

I think we may use UNLEAK above (for now) because we will exit process after this function.

However, I anticipated we may need to do more stuffs after this function in the future.

Show 129 quoted lines
> +
> +	return 0;
> +}
> +
>  #define SUPPORT_SUPER_PREFIX (1<<0)
>  
>  struct cmd_struct {
> @@ -3060,6 +3219,7 @@ static struct cmd_struct commands[] = {
>  	{"clone", module_clone, 0},
>  	{"add-clone", add_clone, 0},
>  	{"add-config", add_config, 0},
> +	{"add", module_add, SUPPORT_SUPER_PREFIX},
>  	{"update-module-mode", module_update_module_mode, 0},
>  	{"update-clone", update_clone, 0},
>  	{"ensure-core-worktree", ensure_core_worktree, 0},
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 8c219ef382..1070540525 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -145,104 +145,12 @@ cmd_add()
>  		shift
>  	done
>  
> -	if ! git submodule--helper config --check-writeable >/dev/null 2>&1
> +	if test -z "$1"
>  	then
> -		 die "fatal: $(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" ||
> -		reference_path="$wt_prefix$reference_path"
> -
> -		reference="--reference=$reference_path"
> -	fi
> -
> -	repo=$1
> -	sm_path=$2
> -
> -	if test -z "$sm_path"; then
> -		sm_path=$(printf '%s\n' "$repo" |
> -			sed -e 's|/$||' -e 's|:*/*\.git$||' -e 's|.*[/:]||g')
> -	fi
> -
> -	if test -z "$repo" || test -z "$sm_path"; then
>  		usage
>  	fi
>  
> -	is_absolute_path "$sm_path" || sm_path="$wt_prefix$sm_path"
> -
> -	# assure repo is absolute or relative to parent
> -	case "$repo" in
> -	./*|../*)
> -		test -z "$wt_prefix" ||
> -		die "fatal: $(gettext "Relative path can only be used from the toplevel of the working tree")"
> -
> -		# dereference source url relative to parent's url
> -		realrepo=$(git submodule--helper resolve-relative-url "$repo") || exit
> -		;;
> -	*:*|/*)
> -		# absolute url
> -		realrepo=$repo
> -		;;
> -	*)
> -		die "fatal: $(eval_gettext "repo URL: '\$repo' must be absolute or begin with ./|../")"
> -	;;
> -	esac
> -
> -	# normalize path:
> -	# multiple //; leading ./; /./; /../; trailing /
> -	sm_path=$(printf '%s/\n' "$sm_path" |
> -		sed -e '
> -			s|//*|/|g
> -			s|^\(\./\)*||
> -			s|/\(\./\)*|/|g
> -			:start
> -			s|\([^/]*\)/\.\./||
> -			tstart
> -			s|/*$||
> -		')
> -	if test -z "$force"
> -	then
> -		git ls-files --error-unmatch "$sm_path" > /dev/null 2>&1 &&
> -		die "fatal: $(eval_gettext "'\$sm_path' already exists in the index")"
> -	else
> -		git ls-files -s "$sm_path" | sane_grep -v "^160000" > /dev/null 2>&1 &&
> -		die "fatal: $(eval_gettext "'\$sm_path' already exists in the index and is not a submodule")"
> -	fi
> -
> -	if test -d "$sm_path" &&
> -		test -z $(git -C "$sm_path" rev-parse --show-cdup 2>/dev/null)
> -	then
> -	    git -C "$sm_path" rev-parse --verify -q HEAD >/dev/null ||
> -	    die "fatal: $(eval_gettext "'\$sm_path' does not have a commit checked out")"
> -	fi
> -
> -	if test -z "$force"
> -	then
> -	    dryerr=$(git add --dry-run --ignore-missing --no-warn-embedded-repo "$sm_path" 2>&1 >/dev/null)
> -	    res=$?
> -	    if test $res -ne 0
> -	    then
> -		 echo >&2 "$dryerr"
> -		 exit $res
> -	    fi
> -	fi
> -
> -	if test -n "$custom_name"
> -	then
> -		sm_name="$custom_name"
> -	else
> -		sm_name="$sm_path"
> -	fi
> -
> -	if ! git submodule--helper check-name "$sm_name"
> -	then
> -		die "fatal: $(eval_gettext "'$sm_name' is not a valid submodule name")"
> -	fi
> -
> -	git submodule--helper add-clone ${GIT_QUIET:+--quiet} ${force:+"--force"} ${progress:+"--progress"} ${branch:+--branch "$branch"} --prefix "$wt_prefix" --path "$sm_path" --name "$sm_name" --url "$realrepo" ${reference:+"$reference"} ${dissociate:+"--dissociate"} ${depth:+"$depth"} || exit
> -	git submodule--helper add-config ${force:+--force} ${branch:+--branch "$branch"} --url "$repo" --resolved-url "$realrepo" --path "$sm_path" --name "$sm_name"
> +	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper add ${GIT_QUIET:+--quiet} ${force:+--force} ${progress:+"--progress"} ${branch:+--branch "$branch"} ${reference_path:+--reference "$reference_path"} ${dissociate:+--dissociate} ${custom_name:+--name "$custom_name"} ${depth:+"$depth"} -- "$@"
>  }
>  
>  #
> -- 
> 2.32.0
> 
-- 
Danh
Previous: Atharva RaykarNext: Atharva Raykar
Message 30 of 78 in “submodule: convert the rest of 'add' to C”
  1. Atharva RaykarAug 5, 2021
  2. [GSoC] [PATCH 1/8] submodule--helper: refactor resolve_relative_url() helperAtharva Raykar, Aug 5, 2021
  3. [GSoC] [PATCH 2/8] submodule--helper: remove repeated code in sync_submodule()Atharva Raykar, Aug 5, 2021
  4. Đoàn Trần Công DanhAug 6, 2021
  5. Christian CouderAug 6, 2021
  6. Atharva RaykarAug 6, 2021
  7. Junio C HamanoAug 6, 2021
  8. [GSoC] [PATCH 3/8] dir: libify and export helper functions from clone.cAtharva Raykar, Aug 5, 2021
  9. [GSoC] [PATCH 4/8] submodule--helper: remove constness of sm_pathAtharva Raykar, Aug 5, 2021
  10. [GSoC] [PATCH 5/8] submodule--helper: convert the bulk of cmd_add() to CAtharva Raykar, Aug 5, 2021
  11. [GSoC] [PATCH 6/8] submodule--helper: remove add-clone subcommandAtharva Raykar, Aug 5, 2021
  12. [GSoC] [PATCH 8/8] submodule--helper: remove resolve-relative-url subcommandAtharva Raykar, Aug 5, 2021
  13. [GSoC] [PATCH 7/8] submodule--helper: remove add-config subcommandAtharva Raykar, Aug 5, 2021
  14. [GSoC] [PATCH v2 0/9] submodule: convert the rest of 'add' to CAtharva Raykar, Aug 5, 2021
  15. [GSoC] [PATCH v2 1/9] submodule--helper: add options for compute_submodule_clone_url()Atharva Raykar, Aug 5, 2021
  16. Junio C HamanoAug 5, 2021
  17. [GSoC] [PATCH v2 2/9] submodule--helper: refactor resolve_relative_url() helperAtharva Raykar, Aug 5, 2021
  18. Junio C HamanoAug 5, 2021
  19. [GSoC] [PATCH v2 3/9] submodule--helper: remove repeated code in sync_submodule()Atharva Raykar, Aug 5, 2021
  20. Junio C HamanoAug 5, 2021
  21. [GSoC] [PATCH v2 4/9] dir: libify and export helper functions from clone.cAtharva Raykar, Aug 5, 2021
  22. Junio C HamanoAug 5, 2021
  23. Atharva RaykarAug 6, 2021
  24. Junio C HamanoAug 6, 2021
  25. Atharva RaykarAug 7, 2021
  26. [GSoC] [PATCH v2 5/9] submodule--helper: remove constness of sm_pathAtharva Raykar, Aug 5, 2021
  27. Junio C HamanoAug 5, 2021
  28. Atharva RaykarAug 6, 2021
  29. [GSoC] [PATCH v2 6/9] submodule--helper: convert the bulk of cmd_add() to CAtharva Raykar, Aug 5, 2021
  30. Đoàn Trần Công DanhAug 6, 2021
  31. Atharva RaykarAug 6, 2021
  32. [GSoC] [PATCH v2 7/9] submodule--helper: remove add-clone subcommandAtharva Raykar, Aug 5, 2021
  33. [GSoC] [PATCH v2 8/9] submodule--helper: remove add-config subcommandAtharva Raykar, Aug 5, 2021
  34. [GSoC] [PATCH v2 9/9] submodule--helper: remove resolve-relative-url subcommandAtharva Raykar, Aug 5, 2021
  35. [GSoC] [PATCH v3 0/8] submodule: convert the rest of 'add' to CAtharva Raykar, Aug 6, 2021
  36. [GSoC] [PATCH v3 1/8] submodule--helper: add options for compute_submodule_clone_url()Atharva Raykar, Aug 6, 2021
  37. [GSoC] [PATCH v3 2/8] submodule--helper: refactor resolve_relative_url() helperAtharva Raykar, Aug 6, 2021
  38. [GSoC] [PATCH v3 3/8] submodule--helper: remove repeated code in sync_submodule()Atharva Raykar, Aug 6, 2021
  39. [GSoC] [PATCH v3 4/8] dir: libify and export helper functions from clone.cAtharva Raykar, Aug 6, 2021
  40. [GSoC] [PATCH v3 5/8] submodule--helper: convert the bulk of cmd_add() to CAtharva Raykar, Aug 6, 2021
  41. [GSoC] [PATCH v3 6/8] submodule--helper: remove add-clone subcommandAtharva Raykar, Aug 6, 2021
  42. [GSoC] [PATCH v3 7/8] submodule--helper: remove add-config subcommandAtharva Raykar, Aug 6, 2021
  43. [GSoC] [PATCH v3 8/8] submodule--helper: remove resolve-relative-url subcommandAtharva Raykar, Aug 6, 2021
  44. [GSoC] [PATCH v4 0/8] submodule: convert the rest of 'add' to CAtharva Raykar, Aug 7, 2021
  45. [GSoC] [PATCH v4 1/8] submodule--helper: add options for compute_submodule_clone_url()Atharva Raykar, Aug 7, 2021
  46. Kaartic SivaraamAug 8, 2021
  47. Kaartic SivaraamAug 8, 2021
  48. Atharva RaykarAug 9, 2021
  49. Atharva RaykarAug 9, 2021
  50. Kaartic SivaraamAug 10, 2021
  51. [GSoC] [PATCH v4 2/8] submodule--helper: refactor resolve_relative_url() helperAtharva Raykar, Aug 7, 2021
  52. [GSoC] [PATCH v4 3/8] submodule--helper: remove repeated code in sync_submodule()Atharva Raykar, Aug 7, 2021
  53. Kaartic SivaraamAug 8, 2021
  54. Atharva RaykarAug 9, 2021
  55. [GSoC] [PATCH v4 4/8] dir: libify and export helper functions from clone.cAtharva Raykar, Aug 7, 2021
  56. Kaartic SivaraamAug 8, 2021
  57. Atharva RaykarAug 9, 2021
  58. Kaartic SivaraamAug 10, 2021
  59. Junio C HamanoAug 10, 2021
  60. Atharva RaykarAug 11, 2021
  61. [GSoC] [PATCH v4 5/8] submodule--helper: convert the bulk of cmd_add() to CAtharva Raykar, Aug 7, 2021
  62. [GSoC] [PATCH v4 6/8] submodule--helper: remove add-clone subcommandAtharva Raykar, Aug 7, 2021
  63. [GSoC] [PATCH v4 7/8] submodule--helper: remove add-config subcommandAtharva Raykar, Aug 7, 2021
  64. [GSoC] [PATCH v4 8/8] submodule--helper: remove resolve-relative-url subcommandAtharva Raykar, Aug 7, 2021
  65. Kaartic SivaraamAug 8, 2021
  66. [GSoC] [PATCH v5 0/9] submodule: convert the rest of 'add' to CAtharva Raykar, Aug 10, 2021
  67. [GSoC] [PATCH v5 1/9] submodule--helper: add options for compute_submodule_clone_url()Atharva Raykar, Aug 10, 2021
  68. Bagas SanjayaAug 11, 2021
  69. Atharva RaykarAug 11, 2021
  70. [GSoC] [PATCH v5 3/9] submodule--helper: remove repeated code in sync_submodule()Atharva Raykar, Aug 10, 2021
  71. [GSoC] [PATCH v5 2/9] submodule--helper: refactor resolve_relative_url() helperAtharva Raykar, Aug 10, 2021
  72. [GSoC] [PATCH v5 4/9] dir: libify and export helper functions from clone.cAtharva Raykar, Aug 10, 2021
  73. [GSoC] [PATCH v5 5/9] submodule--helper: convert the bulk of cmd_add() to CAtharva Raykar, Aug 10, 2021
  74. [GSoC] [PATCH v5 6/9] submodule--helper: remove add-clone subcommandAtharva Raykar, Aug 10, 2021
  75. [GSoC] [PATCH v5 9/9] submodule--helper: rename compute_submodule_clone_url()Atharva Raykar, Aug 10, 2021
  76. [GSoC] [PATCH v5 8/9] submodule--helper: remove resolve-relative-url subcommandAtharva Raykar, Aug 10, 2021
  77. [GSoC] [PATCH v5 7/9] submodule--helper: remove add-config subcommandAtharva Raykar, Aug 10, 2021
  78. Junio C HamanoSep 8, 2021

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.