From: Junio C Hamano <gitster@pobox.com> To: Shourya Shukla <shouryashukla.oo@gmail.com> Cc: git@vger.kernel.org, christian.couder@gmail.com, kaartic.sivaraam@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: [PATCH v2 2/3] submodule: port submodule subcommand 'add' from shell to C Date: Thu, 08 Oct 2020 22:09:55 -0700 Message-ID: <xmqqd01sugrg.fsf@gitster.c.googlers.com> (raw) In-Reply-To: <20201007074538.25891-3-shouryashukla.oo@gmail.com> (Shourya Shukla's message of "Wed, 7 Oct 2020 13:15:37 +0530") Shourya Shukla <shouryashukla.oo@gmail.com> writes: > +static void config_added_submodule(struct add_data *info) > +{ > + char *key, *var = NULL; > + struct child_process cp = CHILD_PROCESS_INIT; > + > + key = xstrfmt("submodule.%s.url", info->sm_name); > + git_config_set_gently(key, info->realrepo); > + free(key); > + > + cp.git_cmd = 1; > + strvec_pushl(&cp.args, "add", "--no-warn-embedded-repo", NULL); > + if (info->force) > + strvec_push(&cp.args, "--force"); > + strvec_pushl(&cp.args, "--", info->sm_path, ".gitmodules", NULL); Hmph, you lost me. I think this ought to correspond to this part of the original: git add --no-warn-embedded-repo $force "$sm_path" || die "$(eval_gettext "Failed to add submodule '\$sm_path'")" I can see that adding "--" before $sm_path may be an improvement, but why do we also add .gitmodules here, and ... > + key = xstrfmt("submodule.%s.path", info->sm_name); > + git_config_set_in_file_gently(".gitmodules", key, info->sm_path); > + free(key); > + key = xstrfmt("submodule.%s.url", info->sm_name); > + git_config_set_in_file_gently(".gitmodules", key, info->repo); > + free(key); > + key = xstrfmt("submodule.%s.branch", info->sm_name); > + if (info->branch) > + git_config_set_in_file_gently(".gitmodules", key, info->branch); > + free(key); ... perform quite a lot of configuration writing, before actually spawning the "git add" and make sure it succeeds? The original won't futz with any of these .gitmodules entries if "git add" of the $sm_path fails and that is a good discipline to follow. > + if (run_command(&cp)) > + die(_("failed to add submodule '%s'"), info->sm_path); > + > + /* > + * NEEDSWORK: In a multi-working-tree world, this needs to be > + * set in the per-worktree config. > + */ > + if (!git_config_get_string("submodule.active", &var) && var) { What if this were a valueless true ("[submodule] active\n" without "= true")? Wouldn't get_string() fail? > + /* > + * If the submodule being adding isn't already covered by the > + * current configured pathspec, set the submodule's active flag > + */ > + if (!is_submodule_active(the_repository, info->sm_path)) { > + key = xstrfmt("submodule.%s.active", info->sm_name); > + git_config_set_gently(key, "true"); > + free(key); > + } > + } else { > + key = xstrfmt("submodule.%s.active", info->sm_name); > + git_config_set_gently(key, "true"); > + free(key); > + } > +} > + > + ... > + 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; > + config_added_submodule(&info); Whew. This was way too big to be reviewed in a single sitting. I do not know offhand if there is a better way to structure the changes into a more digestible pieces to help prevent reviewers from overlooking potential mistakes, though. Thanks.
next prev parent reply other threads:[~2020-10-09 5:10 UTC|newest] Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-10-07 7:45 [PATCH v2 0/3] submodule: port subcommand add " Shourya Shukla 2020-10-07 7:45 ` [PATCH v2 1/3] dir: change the scope of function 'directory_exists_in_index()' Shourya Shukla 2020-10-07 18:05 ` Junio C Hamano 2020-10-12 10:11 ` Shourya Shukla 2020-11-18 23:25 ` Emily Shaffer 2020-10-07 7:45 ` [PATCH v2 2/3] submodule: port submodule subcommand 'add' from shell to C Shourya Shukla 2020-10-07 18:37 ` Junio C Hamano 2020-10-07 22:19 ` Junio C Hamano 2020-10-08 17:19 ` Junio C Hamano 2020-10-09 5:09 ` Junio C Hamano [this message] 2020-11-18 23:13 ` Jonathan Tan 2020-11-19 7:44 ` Ævar Arnfjörð Bjarmason 2020-11-19 12:38 ` Johannes Schindelin 2020-11-19 20:37 ` Junio C Hamano 2020-10-07 7:45 ` [PATCH v2 3/3] t7400: add test to check 'submodule add' for tracked paths Shourya Shukla 2020-11-19 0:03 ` [PATCH v2 0/3] submodule: port subcommand add from shell to C Josh Steadmon
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=xmqqd01sugrg.fsf@gitster.c.googlers.com \ --to=gitster@pobox.com \ --cc=Johannes.Schindelin@gmx.de \ --cc=chriscool@tuxfamily.org \ --cc=christian.couder@gmail.com \ --cc=git@vger.kernel.org \ --cc=kaartic.sivaraam@gmail.com \ --cc=liu.denton@gmail.com \ --cc=pc44800@gmail.com \ --cc=shouryashukla.oo@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
git@vger.kernel.org list mirror (unofficial, one of many) This inbox may be cloned and mirrored by anyone: git clone --mirror https://public-inbox.org/git git clone --mirror http://ou63pmih66umazou.onion/git git clone --mirror http://czquwvybam4bgbro.onion/git git clone --mirror http://hjrcffqmbrq6wope.onion/git # If you have public-inbox 1.1+ installed, you may # initialize and index your mirror using the following commands: public-inbox-init -V1 git git/ https://public-inbox.org/git \ git@vger.kernel.org public-inbox-index git Example config snippet for mirrors. Newsgroups are available over NNTP: nntp://news.public-inbox.org/inbox.comp.version-control.git nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git nntp://news.gmane.io/gmane.comp.version-control.git note: .onion URLs require Tor: https://www.torproject.org/ code repositories for the project(s) associated with this inbox: https://80x24.org/mirrors/git.git AGPL code for this site: git clone https://public-inbox.org/public-inbox.git