From: Shourya Shukla <shouryashukla.oo@gmail.com>
To: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Cc: Junio C Hamano <gitster@pobox.com>,
git@vger.kernel.org, christian.couder@gmail.com,
johannes.schindelin@gmx.de, liu.denton@gmail.com,
Prathamesh Chavan <pc44800@gmail.com>,
Christian Couder <chriscool@tuxfamily.org>,
Stefan Beller <stefanbeller@gmail.com>
Subject: Re: [GSoC][PATCH] submodule: port submodule subcommand 'add' from shell to C
Date: Wed, 26 Aug 2020 14:57:41 +0530 [thread overview]
Message-ID: <CAP6+3T2FbjKc35QYiDmaezzKbkrxEOcBqzirm032_tTU2foZ=Q@mail.gmail.com> (raw)
In-Reply-To: <43337924c09119d43c74fdad3f00d4dab76edb51.camel@gmail.com>
On 8/25/20, Kaartic Sivaraam <kaartic.sivaraam@gmail.com> 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.
next prev parent reply other threads:[~2020-08-26 9:27 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-24 9:03 [GSoC][PATCH] submodule: port submodule subcommand 'add' from shell to C Shourya Shukla
2020-08-24 18:35 ` Junio C Hamano
2020-08-24 20:30 ` Kaartic Sivaraam
2020-08-24 20:46 ` Junio C Hamano
2020-08-26 9:27 ` Shourya Shukla [this message]
2020-08-26 10:54 ` Kaartic Sivaraam
2020-08-26 9:15 ` Shourya Shukla
2020-08-30 19:58 ` Kaartic Sivaraam
2020-08-31 13:04 ` Shourya Shukla
2020-09-01 20:35 ` Kaartic Sivaraam
2020-09-02 12:04 ` Shourya Shukla
2020-09-03 8:46 ` Kaartic Sivaraam
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAP6+3T2FbjKc35QYiDmaezzKbkrxEOcBqzirm032_tTU2foZ=Q@mail.gmail.com' \
--to=shouryashukla.oo@gmail.com \
--cc=chriscool@tuxfamily.org \
--cc=christian.couder@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=johannes.schindelin@gmx.de \
--cc=kaartic.sivaraam@gmail.com \
--cc=liu.denton@gmail.com \
--cc=pc44800@gmail.com \
--cc=stefanbeller@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).