diff mbox series

[v2,1/5] submodule: expose the '--for-status' option of summary

Message ID 20200806164102.6707-2-shouryashukla.oo@gmail.com (mailing list archive)
State Superseded
Headers show
Series submodule: port subcommand 'summary' from shell to C | expand

Commit Message

Shourya Shukla Aug. 6, 2020, 4:40 p.m. UTC
The 'for-status' option is used to compute the summary of submodule(s)
in a superproject by skipping the ignored submdules i.e., those with
'submodule.<name>.ignore' set to 'all' in the '.gitmodules' or
'.git/config', with the latter taking precedence over the former.

The option was introduced in d0f64dd44d (git-submodule summary:
--for-status option, 2008-04-12), refined in 3ba7407b8b (submodule
summary: ignore --for-status option, 2013-09-06) and finally perfected
in 927b26f87a (submodule: don't print status output with ignore=all,
2013-09-01). But, it was not mentioned in the 'git submodule'
Documentation.

Expose the '--for-status' option accepted by the command 'git submodule
summary'.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>
---
 Documentation/git-submodule.txt        | 8 +++++++-
 contrib/completion/git-completion.bash | 2 +-
 git-submodule.sh                       | 2 +-
 3 files changed, 9 insertions(+), 3 deletions(-)

Comments

Kaartic Sivaraam Aug. 8, 2020, 2:40 p.m. UTC | #1
On 06-08-2020 22:10, Shourya Shukla wrote:
> The 'for-status' option is used to compute the summary of submodule(s)
> in a superproject by skipping the ignored submdules i.e., those with
> 'submodule.<name>.ignore' set to 'all' in the '.gitmodules' or
> '.git/config', with the latter taking precedence over the former.
> 
> The option was introduced in d0f64dd44d (git-submodule summary:
> --for-status option, 2008-04-12), refined in 3ba7407b8b (submodule
> summary: ignore --for-status option, 2013-09-06) and finally perfected
> in 927b26f87a (submodule: don't print status output with ignore=all,
> 2013-09-01). But, it was not mentioned in the 'git submodule'
> Documentation.
> 
> Expose the '--for-status' option accepted by the command 'git submodule
> summary'.
>

I've had one concern about exposing '--for-status'. As of now, the name
of the option has no relation with the behaviour that we get as a
consequence. So long, the option has been internal and this wasn't a
problem. Now that we're considering to expose it in the docs, usage and
autocomplete, I would say it should be done after renaming it
appropriately given that it's easy to do now than later. As to name
suggestions, I really don't have any.

Also, as to whether exposing this would be useful at all, I really don't
know.
Christian Couder Aug. 8, 2020, 8:25 p.m. UTC | #2
Le sam. 8 août 2020 à 16:40, Kaartic Sivaraam
<kaartic.sivaraam@gmail.com> a écrit :
>
> On 06-08-2020 22:10, Shourya Shukla wrote:
> > The 'for-status' option is used to compute the summary of submodule(s)
> > in a superproject by skipping the ignored submdules i.e., those with
> > 'submodule.<name>.ignore' set to 'all' in the '.gitmodules' or
> > '.git/config', with the latter taking precedence over the former.

The above seems to suggest that a name like --skip-ignored could fit,
if we wanted to rename --for-status.

> > The option was introduced in d0f64dd44d (git-submodule summary:
> > --for-status option, 2008-04-12), refined in 3ba7407b8b (submodule
> > summary: ignore --for-status option, 2013-09-06) and finally perfected
> > in 927b26f87a (submodule: don't print status output with ignore=all,
> > 2013-09-01). But, it was not mentioned in the 'git submodule'
> > Documentation.

After this we would need to tell why it's a good idea to actually
document this option (and perhaps rename it if we are going to do
that). It could be a good idea, if it could help users to see a
summary without the ignored submodules.

So for example a possibly good justification could be that in a repo
with many ignored submodules it might be interesting for users to get
a summary that contains information only about the non-ignored
submodules.

An example output of `git submodule summary` both with and without
--for-status (or --skip-ignored) in an interesting case (where there
are many ignored submodule) could help convince people that it's a
possibly useful option, and that it's worth documenting.

> > Expose the '--for-status' option accepted by the command 'git submodule
> > summary'.
> >
>
> I've had one concern about exposing '--for-status'. As of now, the name
> of the option has no relation with the behaviour that we get as a
> consequence. So long, the option has been internal and this wasn't a
> problem. Now that we're considering to expose it in the docs, usage and
> autocomplete, I would say it should be done after renaming it
> appropriately given that it's easy to do now than later. As to name
> suggestions, I really don't have any.

Yeah, I agree that finding a good name and a good use case for the
option would surely help.

> Also, as to whether exposing this would be useful at all, I really don't
> know.
Junio C Hamano Aug. 8, 2020, 11:26 p.m. UTC | #3
Christian Couder <christian.couder@gmail.com> writes:

> Yeah, I agree that finding a good name and a good use case for the
> option would surely help.

That makes it sound like a solution looking for a problem.

The option was added and discussed in [*1*], and it was quite clear
that it was merely an implementation detail to show the same info as
"git submodule summary" in a format that would fit better in the
context of "git status".  I doubt that anything changed since then
in the past 12 years to make the option deserve more attention by
the end users, but what this patch (which is the first in a 5-patch
series) does may be worth doing if a later patch in the series
serves as that "good use case".

On the other hand, if there is no such "good use case" example in
the other changes in this series, the option can and should be kept
as an implementation detail of "git status", I would think.

Thanks.


[Reference]

*1*
https://lore.kernel.org/git/1205416085-23431-1-git-send-email-pkufranky@gmail.com/
diff mbox series

Patch

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 7e5f995f77..d944e4c817 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -190,7 +190,7 @@  set-url [--] <path> <newurl>::
 	automatically synchronize the submodule's new remote URL
 	configuration.
 
-summary [--cached|--files] [(-n|--summary-limit) <n>] [commit] [--] [<path>...]::
+summary [--cached|--files] [--for-status] [(-n|--summary-limit) <n>] [commit] [--] [<path>...]::
 	Show commit summary between the given commit (defaults to HEAD) and
 	working tree/index. For a submodule in question, a series of commits
 	in the submodule between the given super project commit and the
@@ -309,6 +309,12 @@  OPTIONS
 	compares the commit in the index with that in the submodule HEAD
 	when this option is used.
 
+--for-status::
+	This option is only valid for the summary command. This command
+	skips the submodules with `submodule.<name>.ignore` set to `all`
+	in the `.gitmodules` or `.git/config`. The configuration in
+	`.git/config` overrides the configuration in `.gitmodules`.
+
 -n::
 --summary-limit::
 	This option is only valid for the summary command.
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 0fdb5da83b..2b7b033c17 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -3059,7 +3059,7 @@  _git_submodule ()
 		__gitcomp "--default --branch"
 		;;
 	summary,--*)
-		__gitcomp "--cached --files --summary-limit"
+		__gitcomp "--cached --files --for-status --summary-limit"
 		;;
 	foreach,--*|sync,--*)
 		__gitcomp "--recursive"
diff --git a/git-submodule.sh b/git-submodule.sh
index 43eb6051d2..dda3fee167 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -13,7 +13,7 @@  USAGE="[--quiet] [--cached]
    or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] [-f|--force] [--checkout|--merge|--rebase] [--[no-]recommend-shallow] [--reference <repository>] [--recursive] [--[no-]single-branch] [--] [<path>...]
    or: $dashless [--quiet] set-branch (--default|--branch <branch>) [--] <path>
    or: $dashless [--quiet] set-url [--] <path> <newurl>
-   or: $dashless [--quiet] summary [--cached|--files] [--summary-limit <n>] [commit] [--] [<path>...]
+   or: $dashless [--quiet] summary [--cached|--files] [--for-status] [--summary-limit <n>] [commit] [--] [<path>...]
    or: $dashless [--quiet] foreach [--recursive] <command>
    or: $dashless [--quiet] sync [--recursive] [--] [<path>...]
    or: $dashless [--quiet] absorbgitdirs [--] [<path>...]"