From: Atharva Raykar <raykar.ath@gmail.com>
To: "Đoàn Trần Công Danh" <congdanhqx@gmail.com>
Cc: git <git@vger.kernel.org>,
Christian Couder <christian.couder@gmail.com>,
Shourya Shukla <shouryashukla.oo@gmail.com>,
Prathamesh Chavan <pc44800@gmail.com>
Subject: Re: [GSoC] [PATCH v2 1/2] submodule--helper: introduce add-clone subcommand
Date: Wed, 9 Jun 2021 16:01:40 +0530 [thread overview]
Message-ID: <3B9B2CD5-2B99-4BF3-B348-5766F1CEB6D7@gmail.com> (raw)
In-Reply-To: <YL9jTFAoEBP+mDA2@danh.dev>
On 08-Jun-2021, at 18:02, Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote:
>
>> [...]
>> +static char *get_next_line(char *const begin, const char *const end)
>> +{
>> + char *pos = begin;
>> + while (pos != end && *pos++ != '\n');
>> + return pos;
>> +}
>
> On my first glance, this function looks like a reinvention of strchr(3).
> Except that, this function also has a second parameter for "end".
> Maybe it has a specical use-case?
>
>> +
>> +static void show_fetch_remotes(FILE *output, const char *sm_name, const char *git_dir_path)
>> +{
>> + struct child_process cp_remote = CHILD_PROCESS_INIT;
>> + struct strbuf sb_remote_out = STRBUF_INIT;
>> +
>> + cp_remote.git_cmd = 1;
>> + strvec_pushf(&cp_remote.env_array,
>> + "GIT_DIR=%s", git_dir_path);
>> + strvec_push(&cp_remote.env_array, "GIT_WORK_TREE=.");
>> + strvec_pushl(&cp_remote.args, "remote", "-v", NULL);
>> + if (!capture_command(&cp_remote, &sb_remote_out, 0)) {
>> + char *line;
>> + char *begin = sb_remote_out.buf;
>> + char *end = sb_remote_out.buf + sb_remote_out.len;
>> + while (begin != end && (line = get_next_line(begin, end))) {
>
> And this is the only use-case. Because you also want to check if you
> reached the last token or not. I guess you really meant to write:
>
> while ((line = strchr(begin, '\n')) != NULL) {
>
> Anyway, I would name the "line" variable as "nextline"
>
>> + int namelen = 0, urllen = 0, taillen = 0;
>> + char *name = parse_token(&begin, line, &namelen);
>> + char *url = parse_token(&begin, line, &urllen);
>> + char *tail = parse_token(&begin, line, &taillen);
>> + if (!memcmp(tail, "(fetch)", 7))
>> + fprintf(output, " %.*s\t%.*s\n",
>> + namelen, name, urllen, url);
>
> I think this whole block is better replaced with strip_suffix_mem and
> fprintf.
>
> Overral I would replace the block inside capture_command with:
>
> -----8<-----
> char *nextline;
> char *line = sb_remote_out.buf;
> while ((nextline = strchr(line, '\n')) != NULL) {
> size_t len = nextline - line;
> if (strip_suffix_mem(line, &len, "(fetch)"))
> fprintf(output, " %.*s\n", (int)len, line);
> line = nextline + 1;
> }
> ---->8-----
>
> And get rid of parse_token and get_next_line functions.
That looks much simpler. Thanks!
I realised that all the token parsing I do is not really necessary.
What I really want to do is "If this line ends with '(fetch)',
print it, but without the '(fetch)'", and I think your version
captures that succinctly.
>> + }
>> + }
>> +
>> + strbuf_release(&sb_remote_out);
>> +}
>> +
>> +static int add_submodule(const struct add_data *add_data)
>> +{
>> + char *submod_gitdir_path;
>> + /* perhaps the path already exists and is already a git repo, else clone it */
>> + if (is_directory(add_data->sm_path)) {
>> + submod_gitdir_path = xstrfmt("%s/.git", add_data->sm_path);
>> + if (is_directory(submod_gitdir_path) || file_exists(submod_gitdir_path))
>> + printf(_("Adding existing path at '%s' to index\n"),
>> + add_data->sm_path);
>> + else
>> + die(_("'%s' already exists and is not a valid git repo"),
>> + add_data->sm_path);
>> + free(submod_gitdir_path);
>> + } else {
>> + struct strvec clone_args = STRVEC_INIT;
>> + struct child_process cp = CHILD_PROCESS_INIT;
>> + submod_gitdir_path = xstrfmt(".git/modules/%s", add_data->sm_name);
>> +
>> + if (is_directory(submod_gitdir_path)) {
>> + if (!add_data->force) {
>> + error(_("A git directory for '%s' is found "
>> + "locally with remote(s):"), add_data->sm_name);
>
> We don't capitalise first character of error message.
> IOW, downcase "A git".
Got it.
> Well, it's bug-for-bug with shell implementation, so it doesn't matter much, anyway.
While it is meant to be a faithful implementation, I think this
is a good opportunity to make minor style fixes.
>> + show_fetch_remotes(stderr, add_data->sm_name,
>> + submod_gitdir_path);
>> + fprintf(stderr,
>> + _("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 if you are unsure what this means, choose "
>> + "another name with the '--name' option.\n"),
>> + add_data->realrepo);
>
> Is there any reason we can't use "error" here?
The message in its entirety looks like this:
error: A git directory for 'test' is found locally with remote(s):
origin git@github.com:tfidfwastaken/abc.git
If you want to reuse this local git directory instead of cloning again from
git@github.com:tfidfwastaken/test.git
use the '--force' option. If the local git directory is not the correct repo
or if you are unsure what this means, choose another name with the '--name' option.
Since the 'error:' is already there in the first line, having it
prepended before 'If you want to reuse...' felt redundant to me.
Besides, it's more of an informational message about what a user
can do next, rather than a message that signifies an error.
If there is a preferred convention or label for such messages,
I can use that. The shell version did not have any such thing though.
>> [...]
>> + struct option options[] = {
>> + OPT_STRING('b', "branch", &add_data.branch,
>> + N_("branch"),
>> + N_("branch of repository to checkout on cloning")),
>> + OPT_STRING(0, "prefix", &add_data.prefix,
>> + N_("path"),
>> + N_("alternative anchor for relative paths")),
>> + OPT_STRING(0, "path", &add_data.sm_path,
>> + N_("path"),
>> + N_("where the new submodule will be cloned to")),
>> + OPT_STRING(0, "name", &add_data.sm_name,
>> + N_("string"),
>> + N_("name of the new submodule")),
>> + OPT_STRING(0, "url", &add_data.realrepo,
>> + N_("string"),
>> + N_("url where to clone the submodule from")),
>> + OPT_STRING(0, "reference", &add_data.reference_path,
>> + N_("repo"),
>> + N_("reference repository")),
>> + OPT_BOOL(0, "dissociate", &dissociate,
>> + N_("use --reference only while cloning")),
>> + OPT_INTEGER(0, "depth", &add_data.depth,
>> + N_("depth for shallow clones")),
>> + OPT_BOOL(0, "progress", &progress,
>> + N_("force cloning progress")),
>> + OPT_BOOL('f', "force", &force,
>> + N_("allow adding an otherwise ignored submodule path")),
>
> We have OPT__FORCE, too.
Will switch over to that.
>> + OPT__QUIET(&quiet, "Suppress output for cloning a submodule"),
>
> And, please downcase "Suppress".
OK.
>> + OPT_END()
>> + };
>> +
>> + const char *const usage[] = {
>> + N_("git submodule--helper add-clone [--prefix=<path>] [--quiet] [--force] "
>> + "[--reference <repository>] [--depth <depth>] [-b|--branch <branch>]"
>> + "[--progress] [--dissociate] --url <url> --path <path> --name <name>"),
>
> I think it's too crowded here, I guess it's better to write:
>
> N_("git submodule--helper add-clone [<options>...] --url <url> --path <path> --name <name>"),
OK. It shouldn't be an issue to shorten it, because this is not
user-facing, and is only ever used within 'cmd_add()'.
>> + NULL
>> + };
>> +
>> + argc = parse_options(argc, argv, prefix, options, usage, 0);
>
> From above usage, I think url, path, name is required, should we have a check for them, here?
We could. The reason why I was not too rigorous about this is
because I plan to eliminate the shell interface for this helper
eventually and call add-clone from within C, in the next few
patches.
But this is a small ask, and I can just add a quick check just
to be extra safe, so I'll do it.
>> +
>> + add_data.progress = !!progress;
>> + add_data.dissociate = !!dissociate;
>> + add_data.force = !!force;
>> + add_data.quiet = !!quiet;
>> +
>> + if (add_submodule(&add_data))
>> + return 1;
>> +
>> + return 0;
>> +}
>> +
>> #define SUPPORT_SUPER_PREFIX (1<<0)
>>
>> struct cmd_struct {
>> @@ -2757,6 +2955,7 @@ static struct cmd_struct commands[] = {
>> {"list", module_list, 0},
>> {"name", module_name, 0},
>> {"clone", module_clone, 0},
>> + {"add-clone", add_clone, 0},
>> {"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 4678378424..f71e1e5495 100755
>> --- a/git-submodule.sh
>> +++ b/git-submodule.sh
>> @@ -241,43 +241,7 @@ cmd_add()
>> die "$(eval_gettext "'$sm_name' is not a valid submodule name")"
>> fi
>>
>> - # perhaps the path exists and is already a git repo, else clone it
>> - if test -e "$sm_path"
>> - then
>> - if test -d "$sm_path"/.git || test -f "$sm_path"/.git
>> - then
>> - eval_gettextln "Adding existing repo at '\$sm_path' to the index"
>> - else
>> - die "$(eval_gettext "'\$sm_path' already exists and is not a valid git repo")"
>> - fi
>> -
>> - else
>> - if test -d ".git/modules/$sm_name"
>> - then
>> - if test -z "$force"
>> - then
>> - eval_gettextln >&2 "A git directory for '\$sm_name' is found locally with remote(s):"
>> - GIT_DIR=".git/modules/$sm_name" GIT_WORK_TREE=. git remote -v | grep '(fetch)' | sed -e s,^," ", -e s,' (fetch)',, >&2
>> - die "$(eval_gettextln "\
>> -If you want to reuse this local git directory instead of cloning again from
>> - \$realrepo
>> -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.")"
>> - else
>> - eval_gettextln "Reactivating local git directory for submodule '\$sm_name'."
>> - fi
>> - fi
>> - git submodule--helper clone ${GIT_QUIET:+--quiet} ${progress:+"--progress"} --prefix "$wt_prefix" --path "$sm_path" --name "$sm_name" --url "$realrepo" ${reference:+"$reference"} ${dissociate:+"--dissociate"} ${depth:+"$depth"} || exit
>> - (
>> - sanitize_submodule_env
>> - cd "$sm_path" &&
>> - # ash fails to wordsplit ${branch:+-b "$branch"...}
>> - case "$branch" in
>> - '') git checkout -f -q ;;
>> - ?*) git checkout -f -q -B "$branch" "origin/$branch" ;;
>> - esac
>> - ) || die "$(eval_gettext "Unable to checkout submodule '\$sm_path'")"
>> - 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 config submodule."$sm_name".url "$realrepo"
>>
>> git add --no-warn-embedded-repo $force "$sm_path" ||
>> --
>> 2.31.1
>>
>
> --
> Danh
Thanks for the comments!
next prev parent reply other threads:[~2021-06-09 10:33 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-05 11:39 [GSoC] [PATCH 0/2] submodule--helper: introduce subcommands for sh to C conversion Atharva Raykar
2021-06-05 11:39 ` [GSoC] [PATCH 1/2] submodule--helper: introduce add-clone subcommand Atharva Raykar
2021-06-06 3:38 ` Bagas Sanjaya
2021-06-06 9:06 ` Christian Couder
2021-06-05 11:39 ` [GSoC] [PATCH 2/2] submodule--helper: introduce add-config subcommand Atharva Raykar
2021-06-07 9:24 ` Christian Couder
2021-06-07 11:24 ` Atharva Raykar
2021-06-08 9:56 ` [GSoC] [PATCH v2 0/2] submodule--helper: introduce subcommands for sh to C conversion Atharva Raykar
2021-06-08 9:56 ` [GSoC] [PATCH v2 1/2] submodule--helper: introduce add-clone subcommand Atharva Raykar
2021-06-08 12:32 ` Đoàn Trần Công Danh
2021-06-09 10:31 ` Atharva Raykar [this message]
2021-06-09 13:06 ` Đoàn Trần Công Danh
2021-06-09 13:10 ` Atharva Raykar
2021-06-09 4:24 ` Junio C Hamano
2021-06-09 10:31 ` Atharva Raykar
2021-06-08 9:56 ` [GSoC] [PATCH v2 2/2] submodule--helper: introduce add-config subcommand Atharva Raykar
2021-06-10 8:39 ` [GSoC] [PATCH v3 0/2] submodule--helper: introduce subcommands for sh to C conversion Atharva Raykar
2021-06-10 8:39 ` [PATCH v3 1/2] submodule--helper: introduce add-clone subcommand Atharva Raykar
2021-06-11 6:10 ` Junio C Hamano
2021-06-11 7:32 ` Atharva Raykar
2021-06-11 7:59 ` Junio C Hamano
2021-06-10 8:39 ` [PATCH v3 2/2] submodule--helper: introduce add-config subcommand Atharva Raykar
2021-06-14 12:51 ` [GSoC] [PATCH v4 0/3] submodule--helper: introduce subcommands for sh to C conversion Atharva Raykar
2021-06-14 12:51 ` [PATCH v4 1/3] submodule--helper: refactor module_clone() Atharva Raykar
2021-06-15 3:51 ` Junio C Hamano
2021-06-15 9:03 ` Atharva Raykar
2021-06-14 12:51 ` [PATCH v4 2/3] submodule--helper: introduce add-clone subcommand Atharva Raykar
2021-06-14 12:51 ` [PATCH v4 3/3] submodule--helper: introduce add-config subcommand Atharva Raykar
2021-06-14 19:51 ` Rafael Silva
2021-06-14 20:12 ` Eric Sunshine
2021-06-15 9:37 ` Rafael Silva
2021-06-15 7:09 ` Atharva Raykar
2021-06-15 9:38 ` [GSoC] [PATCH v5 0/3] submodule--helper: introduce subcommands for sh to C conversion Atharva Raykar
2021-06-15 9:38 ` [PATCH v5 1/3] submodule--helper: refactor module_clone() Atharva Raykar
2021-06-15 9:38 ` [PATCH v5 2/3] submodule--helper: introduce add-clone subcommand Atharva Raykar
2021-06-15 9:38 ` [PATCH v5 3/3] submodule--helper: introduce add-config subcommand Atharva Raykar
2021-06-15 14:57 ` [GSoC] [PATCH v6 0/3] submodule--helper: introduce subcommands for sh to C conversion Atharva Raykar
2021-06-15 14:57 ` [PATCH v6 1/3] submodule--helper: refactor module_clone() Atharva Raykar
2021-06-15 14:57 ` [PATCH v6 2/3] submodule--helper: introduce add-clone subcommand Atharva Raykar
2021-06-15 14:57 ` [PATCH v6 3/3] submodule--helper: introduce add-config subcommand Atharva Raykar
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=3B9B2CD5-2B99-4BF3-B348-5766F1CEB6D7@gmail.com \
--to=raykar.ath@gmail.com \
--cc=christian.couder@gmail.com \
--cc=congdanhqx@gmail.com \
--cc=git@vger.kernel.org \
--cc=pc44800@gmail.com \
--cc=shouryashukla.oo@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).