Re: [PATCH v2 2/3] submodule: port submodule subcommand 'add' from shell to C

[Junio C Hamano <gitster@pobox.com>]

Shourya Shukla <shouryashukla.oo@gmail.com> writes:

> +static void config_added_submodule(struct add_data *info)
> +{

This one I may take a look at later, but won't review in this
message.

> +}
> +
> +static int module_add(int argc, const char **argv, const char *prefix)
> +{
> +	const char *branch = NULL, *custom_name = NULL, *realrepo = NULL;
> +	const char *reference_path = NULL, *repo = NULL, *name = NULL;
> +	char *path;
> +	int force = 0, quiet = 0, depth = -1, progress = 0, dissociate = 0;
> +	struct add_data info = ADD_DATA_INIT;
> +	struct strbuf sb = STRBUF_INIT;
> +
> +	struct option options[] = {
> +		OPT_STRING('b', "branch", &branch, N_("branch"),
> +			   N_("branch of repository to add as submodule")),
> +		OPT_BOOL('f', "force", &force, N_("allow adding an otherwise "
> +						  "ignored submodule path")),
> +		OPT__QUIET(&quiet, N_("print only error messages")),
> +		OPT_BOOL(0, "progress", &progress, N_("force cloning progress")),
> +		OPT_STRING(0, "reference", &reference_path, N_("repository"),
> +			   N_("reference repository")),
> +		OPT_BOOL(0, "dissociate", &dissociate, N_("borrow the objects from reference repositories")),
> +		OPT_STRING(0, "name", &custom_name, N_("name"),
> +			   N_("sets the submodule’s name to the given string "
> +			      "instead of defaulting to its path")),
> +		OPT_INTEGER(0, "depth", &depth, N_("depth for shallow clones")),
> +		OPT_END()
> +	};
> +
> +	const char *const usage[] = {
> +		N_("git submodule--helper add [<options>] [--] [<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 (reference_path && !is_absolute_path(reference_path) && prefix)

Checking "*prefix" lets us avoid an unnecessary allocation, i.e.

	if (prefix && *prefix &&
	    reference_path && !is_absolute_path(reference_path))

> +		reference_path = xstrfmt("%s%s", prefix, reference_path);
> +
> +	if (argc == 0 || argc > 2) {

Nice that you are checking excess args, which the original didn't do.

> +		usage_with_options(usage, options);
> +	} else if (argc == 1) {
> +		repo = argv[0];
> +		path = guess_dir_name(repo);

We've reviewed the function already.  Good.

> +	} else {
> +		repo = argv[0];
> +		path = xstrdup(argv[1]);

OK.  So after this if/else if/else cascade, path is an allocated
piece of memory we could later free() whichever branch is taken.

> +	}
> +
> +	if (!is_absolute_path(path) && prefix)
> +		path = xstrfmt("%s%s", prefix, path);

This also makes path freeable, but the original path is leaked.

	if (prefix && *prefix && !is_absolute_path(path)) {
		free(path);
		path = xstrfmt(...);
	}

Is there a reason (does not have to be a strong reason) why we use
'path', not 'sm_path', as the variable name that corresponds to
$sm_path in the original, by the way?

> +	/* assure repo is absolute or relative to parent */
> +	if (starts_with_dot_dot_slash(repo) || starts_with_dot_slash(repo)) {
> +		char *remote = get_default_remote();
> +		char *remoteurl;
> +		struct strbuf sb = STRBUF_INIT;
> +
> +		if (prefix)
> +			die(_("relative path can only be used from the toplevel "
> +			      "of the working tree"));

This is 'git submodule--helper resolve-relative-url "$repo"' in the
original.

> +		/* dereference source url relative to parent's url */
> +		strbuf_addf(&sb, "remote.%s.url", remote);
> +		if (git_config_get_string(sb.buf, &remoteurl))
> +			remoteurl = xgetcwd();
> +		realrepo = relative_url(remoteurl, repo, NULL);

And this is copied-and-pasted from resolve_relative_url() function
found in builtins/submodule--helper.c.

relative_url() returns an allocated memory so we can free() realrepo
if we took this branch.

> +		free(remoteurl);
> +		free(remote);
> +	} else if (is_dir_sep(repo[0]) || strchr(repo, ':')) {
> +		realrepo = repo;

This repo came from argv[0] so we cannot free realrepo if we took
this branch.  Are we willing to leak realrepo we obtained from the
other branch?

> +	} else {
> +		die(_("repo URL: '%s' must be absolute or begin with ./|../"),
> +		      repo);
> +	}
> +
> +	/*
> +	 * normalize path:
> +	 * multiple //; leading ./; /./; /../;
> +	 */
> +	normalize_path_copy(path, path);

It's nice that a handy (almost) equivalent helper is already
available ;-)

> +	/* strip trailing '/' */
> +	if (is_dir_sep(path[strlen(path) -1]))
> +		path[strlen(path) - 1] = '\0';

The original dealt with multiple trailing '/' but this one does not.
Shouldn't it loop starting at the end?

> +	if (check_sm_exists(force, path))
> +		return 1;

OK.  I think we reviewed the function.  Seeing it in the context of
the calling site makes us realize that it has a wrong name.  "check
submodule exists" sounds as if we expect a submodule to exist at the
path, and it is an error for a submodule not to be there, but that
is not what this caller (which is the only caller of the helper)
wants to check.  And more importantly, the helper reacts to anything
sitting at the path, not just submoudle.

So what does the helper really do?  I think it checks if it is OK to
create a submodule there.  IOW, "exists" part of the name is what
makes it a misnomer.  Perhaps "can_create_submodule()"?

> +	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);
> +	}
> +
> +	if (!force) {
> +		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", path, NULL);
> +		if (pipe_command(&cp, NULL, 0, NULL, 0, &sb, 0)) {
> +			fprintf(stderr, _("%s"), sb.buf);

Sorry, but I cannot guess what this _("%s") is trying to achieve.
Shouldn't it be
			strbuf_complete_line(&sb);
			fputs(sb.buf, stderr);
instead?

> +			return 1;

The original honors the exit code from the dry-run and relays it to
the user.  Is this a regression, or nobody care what exit status
they get as long as it is not zero?

> +		}
> +		strbuf_release(&sb);
> +	}
> +
> +	name = custom_name ? custom_name : path;
> +	if (check_submodule_name(name))
> +		die(_("'%s' is not a valid submodule name"), name);
> +
> +	info.prefix = prefix;
> +	info.sm_name = name;
> +	info.sm_path = path;
> +	info.repo = repo;
> +	info.realrepo = realrepo;
> +	info.reference_path = reference_path;
> +	info.branch = branch;
> +	info.depth = depth;
> +	info.progress = !!progress;
> +	info.dissociate = !!dissociate;
> +	info.force = !!force;
> +	info.quiet = !!quiet;
> +
> +	if (add_submodule(&info))
> +		return 1;

I think we've reviewed this funciton already.

> +	config_added_submodule(&info);
> +
> +	free(path);

Looking a bit uneven wrt to leak handling.

> +	return 0;
> +}

Thanks.