Re: [GSoC][PATCH] submodule: port submodule subcommand 'add' from shell to C

> On Mon, 2020-08-24 at 11:35 -0700, Junio C Hamano wrote:
>> Now it is a different question if the original is correct to begin
>> with ;-).
>>
>
> By looking at commit message of 619acfc78c (submodule add: extend force
> flag to add existing repos, 2016-10-06), I'm assuming it's correct.
> There are chances I might be missing something, though.
>
> Speaking of correctness, I'm surprised how the port passed the
> following test t7400.63 despite the incorrect check.
>
> -- 8< --
> $ ./t7400-submodule-basic.sh
> ... snip ...
> ok 62 - add submodules without specifying an explicit path
> ok 63 - add should fail when path is used by a file
> ok 64 - add should fail when path is used by an existing directory
> ... snip ...
> -- >8 --
>
> Most likely it passed because it slipped through the incorrect check
> and failed later in the code[1]. That's not good, of course.
>
>> > 	}
>> >
>> > Is this part correct? I am not very sure about this. This particular
>> > part is not covered in any test or test script, so, I do not have a
>> > solid method of knowing the correctness of this segment.
>> > Feedback and reviews are appreciated.
>
>
>> +static int add_submodule(struct add_data *info)
>> +{
>> +	/* perhaps the path exists and is already a git repo, else clone it */
>> +	if (is_directory(info->sm_path)) {
>> +		char *sub_git_path = xstrfmt("%s/.git", info->sm_path);
>> +		if (is_directory(sub_git_path) || file_exists(sub_git_path))
>> +			printf(_("Adding existing repo at '%s' to the index\n"),
>> +				 info->sm_path);
>> +		else
>> +			die(_("'%s' already exists and is not a valid git repo"),
>> +			      info->sm_path);
>> +		free(sub_git_path);
>> +	} else {
>> +		struct strvec clone_args = STRVEC_INIT;
>> +		struct child_process cp = CHILD_PROCESS_INIT;
>> +		char *submodule_git_dir = xstrfmt(".git/modules/%s", info->sm_name);
>> +
>> +		if (is_directory(submodule_git_dir)) {
>> +			if (!info->force) {
>> +				struct child_process cp_rem = CHILD_PROCESS_INIT;
>> +				struct strbuf sb_rem = STRBUF_INIT;
>> +				cp_rem.git_cmd = 1;
>> +				fprintf(stderr, _("A git directory for '%s' is "
>> +					"found locally with remote(s):\n"),
>> +					info->sm_name);
>> +				strvec_pushf(&cp_rem.env_array,
>> +					     "GIT_DIR=%s", submodule_git_dir);
>> +				strvec_push(&cp_rem.env_array, "GIT_WORK_TREE=.");
>> +				strvec_pushl(&cp_rem.args, "remote", "-v", NULL);
>> +				if (!capture_command(&cp_rem, &sb_rem, 0)) {
>> +					modify_remote_v(&sb_rem);
>> +				}
>> +				error(_("If you want to reuse this local git "
>> +				      "directory instead of cloning again from\n "
>> +				      "  %s\n"
>> +				      "use the '--force' option. If the local "
>> +				      "git directory is not the correct repo\n"
>> +				      "or you are unsure what this means choose "
>> +				      "another name with the '--name' option."),
>> +				      info->realrepo);
>> +				return 1;
>> +			} else {
>> +				printf(_("Reactivating local git directory for "
>> +					 "submodule '%s'."), info->sm_path);
>> +			}
>> +		}
>> +		free(submodule_git_dir);
>
> This part results in a difference in error message in shell and C
> versions.
>
> -- 8< --
> $ # Shell version
> $ git submodule add ../subm1 sub
> A git directory for 'sub' is found locally with remote(s):
>   origin        /me/subm1
> If you want to reuse this local git directory instead of cloning again from
>   /me/subm1
> use the '--force' option. If the local git directory is not the correct
> repo
> or you are unsure what this means choose another name with the '--name'
> option.
> $
> $ # C version
> $ git submodule add ../subm1 sub
> A git directory for 'sub' is found locally with remote(s):
>   origin        /me/subm1
> error: If you want to reuse this local git directory instead of cloning
> again from
>    /me/subm1
> use the '--force' option. If the local git directory is not the correct
> repo
> or you are unsure what this means choose another name with the '--name'
> option.
> -- >8 --
>
> Note how the third line is oddly prefixed by a `error` unlike the rest
> of the lines. It would be nice if we could weed out that inconsistency.
> We could probably use `advise()` for printing the last four lines and
> `error()` for the lines above them.