git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
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!

:^)

  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).