From: Atharva Raykar <raykar.ath@gmail.com> To: Christian Couder <christian.couder@gmail.com> Cc: git <git@vger.kernel.org>, Junio C Hamano <gitster@pobox.com>, Shourya Shukla <shouryashukla.oo@gmail.com>, Prathamesh Chavan <pc44800@gmail.com> Subject: Re: [GSoC] [PATCH 2/2] submodule--helper: introduce add-config subcommand Date: Mon, 7 Jun 2021 16:54:27 +0530 [thread overview] Message-ID: <441792CB-259C-4635-8B00-9511E8D1FC3C@gmail.com> (raw) In-Reply-To: <CAP8UFD2kwSB60kF_cjs4JYSYeXzvjAtz0OuFOfTqEU7F+ijR-w@mail.gmail.com> On 07-Jun-2021, at 14:54, Christian Couder <christian.couder@gmail.com> wrote: > > On Sat, Jun 5, 2021 at 1:42 PM Atharva Raykar <raykar.ath@gmail.com> wrote: >> >> Add a new "add-config" subcommand to `git submodule--helper` with the >> goal of converting part of the shell code in git-submodule.sh related to >> `git submodule add` into C code. This new subcommand sets the >> configuration variables of a newly added submodule, by registering the >> url in local git config, as well as the submodule name and path in the >> .gitmodules file. It also sets 'submodule.<name>.active' to "true" if >> the submodule path has not already been covered by any pathspec >> specified in 'submodule.active'. >> >> This is meant to be a faithful conversion from shell to C, with only one >> minor change: A warning is emitted if no value is specified in >> 'submodule.active', ie, the config looks like: "[submodule] active\n". > > Maybe explaining a bit how this warning is useful could help reviewers > here. Especially what could happen if no value is specified in > 'submodule.active'? Will do. For now I'll leave an explanation here as well, so that those who might see this thread can know the motivation behind it. (I'll make it more concise in my cover letter of v2) Junio in his review of Shourya's patch[1] said: > When a user has "[submodule] active" in his or her > configuration file, it is a configuration error. When Git reads > "submodule.active" configuration variable to make a decision (like > the above code) and finds that the user has such an error, the user > would appreciate if the error is pointed out, so that it can be > corrected, rather than silently ignored. Git might set 'submodule.<name>.active = true' in the above case, and since it has a higher priority[2] than the 'submodule.active' switch, it would be useful to let the user know of this change, and the incorrect configuration that led to this behaviour, so that they may be able to remedy it, and avoid surprises later on. [1] https://lore.kernel.org/git/xmqqo8isxefz.fsf@gitster.c.googlers.com/ [2] https://git-scm.com/docs/gitsubmodules#_active_submodules >> The structure of the conditional to check if we need to set the 'active' >> toggle looks different from the shell version -- but behaves the same. >> The change was made to decrease code duplication. A comment has been >> added to explain that only one value of 'submodule.active' is obtained >> to check if we need to call is_submodule_active() at all. >> >> This is part of a series of changes that will result in all of >> 'submodule add' being converted to C. > > [...] > > >> + /* >> + * NEEDSWORK: In a multi-working-tree world this needs to be >> + * set in the per-worktree config. >> + */ >> + pathspec_key_exists = !git_config_get_string("submodule.active", >> + &submod_pathspec); >> + if (pathspec_key_exists && !submod_pathspec) >> + warning(_("The submodule.active configuration exists, but " >> + "no pathspec was specified. Setting the value of " >> + "submodule.%s.active to 'true'."), add_data->sm_name); > > It's not very clear that we will actually set > 'submodule.<name>.active' to true below as it depends on the result of > calling is_submodule_active() Hmm, I see the issue. Would it be more accurate to say this: "The submodule.active configuration exists, but no pathspec was specified. If the module is not already active, the value of 'submodule.<name>.active' will be set to 'true'." > , and anyway is_submodule_active() will > check again if "submodule.active" is set, which is wasteful. > > Maybe we could set a variable, called for example "activate" here, > with something like: > > if (pathspec_key_exists && !submod_pathspec) { > warning(...); > activate = 1; > } > > and below use a check like: > > if (!pathspec_key_exists || activate || > !is_submodule_active(the_repository, add_data->sm_path)) { Got it. Thanks for suggesting this improvement! > ... > >> + /* >> + * If submodule.active does not exist, we will activate this >> + * module unconditionally. >> + * >> + * Otherwise, we ask is_submodule_active(), which iterates >> + * through all the values of 'submodule.active' to determine >> + * if this module is already active. >> + */ >> + if (!pathspec_key_exists || >> + !is_submodule_active(the_repository, add_data->sm_path)) { >> + key = xstrfmt("submodule.%s.active", add_data->sm_name); >> + git_config_set_gently(key, "true"); >> + free(key); >> + } >> +} > > The rest looks good to me! Thanks! :^)
next prev parent reply other threads:[~2021-06-07 11:25 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 [this message] 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 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=441792CB-259C-4635-8B00-9511E8D1FC3C@gmail.com \ --to=raykar.ath@gmail.com \ --cc=christian.couder@gmail.com \ --cc=git@vger.kernel.org \ --cc=gitster@pobox.com \ --cc=pc44800@gmail.com \ --cc=shouryashukla.oo@gmail.com \ --subject='Re: [GSoC] [PATCH 2/2] submodule--helper: introduce add-config subcommand' \ /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
Code repositories for project(s) associated with this 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).