On 8/25/20, Kaartic Sivaraam <kaartic.sivaraam@xxxxxxxxx> wrote:
> 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.
Understood. I will correct this part. BTW, you surely are talking
about error() on
the first 2 lines? I think fprintf(stderr, _()) is OK for them otherwise they
will be prefixed by 'error:' which will not be in line with the shell version.
