[RFC PATCH] change default for status.submoduleSummary to true

[RFC PATCH] change default for status.submoduleSummary to true

[Stefan Beller]

A user complained about the workflow with submodules:
> Re submodules pain, I've seen a lot of people get confused about
> how and when to commit submodule changes. The main thing missing
> in the related UIs is some way to summarize the subproject commit
> diff in a human readable way. Maybe last log message would be better
> than just sha?

We could advise all the confused users to turn on
status.submoduleSummary.  However there is no downside from turning
it on by default apart from a slight change in behavior and bit
longer output of git-status and the help in git-commit.

Signed-off-by: Stefan Beller <sbeller@google.com>
---

 Maybe we can merge this early after 2.13, so we have a longer time frame
 in which people may react to this change of a default?
 
 Thanks,
 Stefan

 Documentation/config.txt | 2 +-
 builtin/commit.c         | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1df1965457..34d4735414 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2984,7 +2984,7 @@ This variable can be overridden with the -u|--untracked-files option
 of linkgit:git-status[1] and linkgit:git-commit[1].
 
 status.submoduleSummary::
-	Defaults to false.
+	Defaults to true.
 	If this is set to a non zero number or true (identical to -1 or an
 	unlimited number), the submodule summary will be enabled and a
 	summary of commits for modified submodules will be shown (see
diff --git a/builtin/commit.c b/builtin/commit.c
index 4e288bc513..833a651013 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1388,6 +1388,7 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 
 	s.ignore_submodule_arg = ignore_submodule_arg;
 	s.status_format = status_format;
+	s.submodule_summary = -1;
 	s.verbose = verbose;
 
 	wt_status_collect(&s);
@@ -1650,6 +1651,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	status_init_config(&s, git_commit_config);
 	status_format = STATUS_FORMAT_NONE; /* Ignore status.short */
 	s.colopts = 0;
+	s.submodule_summary = -1;
 
 	if (get_sha1("HEAD", oid.hash))
 		current_head = NULL;
-- 
2.12.2.511.g2abb8caf66

Re: [RFC PATCH] change default for status.submoduleSummary to true

[Jacob Keller]

On Wed, Mar 29, 2017 at 6:20 PM, Stefan Beller <sbeller@google.com> wrote:
> A user complained about the workflow with submodules:
>> Re submodules pain, I've seen a lot of people get confused about
>> how and when to commit submodule changes. The main thing missing
>> in the related UIs is some way to summarize the subproject commit
>> diff in a human readable way. Maybe last log message would be better
>> than just sha?
>
> We could advise all the confused users to turn on
> status.submoduleSummary.  However there is no downside from turning
> it on by default apart from a slight change in behavior and bit
> longer output of git-status and the help in git-commit.
>

Makes sense to me.

Thanks,
Jake

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>
>  Maybe we can merge this early after 2.13, so we have a longer time frame
>  in which people may react to this change of a default?
>
>  Thanks,
>  Stefan
>
>  Documentation/config.txt | 2 +-
>  builtin/commit.c         | 2 ++
>  2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 1df1965457..34d4735414 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2984,7 +2984,7 @@ This variable can be overridden with the -u|--untracked-files option
>  of linkgit:git-status[1] and linkgit:git-commit[1].
>
>  status.submoduleSummary::
> -       Defaults to false.
> +       Defaults to true.
>         If this is set to a non zero number or true (identical to -1 or an
>         unlimited number), the submodule summary will be enabled and a
>         summary of commits for modified submodules will be shown (see
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 4e288bc513..833a651013 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1388,6 +1388,7 @@ int cmd_status(int argc, const char **argv, const char *prefix)
>
>         s.ignore_submodule_arg = ignore_submodule_arg;
>         s.status_format = status_format;
> +       s.submodule_summary = -1;
>         s.verbose = verbose;
>
>         wt_status_collect(&s);
> @@ -1650,6 +1651,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>         status_init_config(&s, git_commit_config);
>         status_format = STATUS_FORMAT_NONE; /* Ignore status.short */
>         s.colopts = 0;
> +       s.submodule_summary = -1;
>
>         if (get_sha1("HEAD", oid.hash))
>                 current_head = NULL;
> --
> 2.12.2.511.g2abb8caf66
>

Re: [RFC PATCH] change default for status.submoduleSummary to true

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> A user complained about the workflow with submodules:
>> Re submodules pain, I've seen a lot of people get confused about
>> how and when to commit submodule changes. The main thing missing
>> in the related UIs is some way to summarize the subproject commit
>> diff in a human readable way. Maybe last log message would be better
>> than just sha?
>
> We could advise all the confused users to turn on
> status.submoduleSummary.  However there is no downside from turning
> it on by default apart from a slight change in behavior and bit
> longer output of git-status and the help in git-commit.

Is "there is no downside" substantiated or just hand-waving?
Any pros-and-cons analysis, e.g. performance implications, etc.?

Re: [RFC PATCH] change default for status.submoduleSummary to true

[Stefan Beller]

On Wed, Mar 29, 2017 at 11:04 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> A user complained about the workflow with submodules:
>>> Re submodules pain, I've seen a lot of people get confused about
>>> how and when to commit submodule changes. The main thing missing
>>> in the related UIs is some way to summarize the subproject commit
>>> diff in a human readable way. Maybe last log message would be better
>>> than just sha?
>>
>> We could advise all the confused users to turn on
>> status.submoduleSummary.  However there is no downside from turning
>> it on by default apart from a slight change in behavior and bit
>> longer output of git-status and the help in git-commit.
>
> Is "there is no downside" substantiated or just hand-waving?

It's the best kind of hand waving.

> Any pros-and-cons analysis, e.g. performance implications, etc.?

Performance will be terrible. The submodule summary is gathered by
running "git submodule summary", which is shell (that is slower than
the C code in status).

In gerrit (which has 6 submodules, that I modified for the test by adding
one commit on top of each), we find

$ time git -c status.submoduleSummary=false status
...
real 0m0.043s

$ time git -c status.submoduleSummary=true status
...
real 0m0.359s

So it is slower by an order of magnitude.
Maybe it is not the right time to propose this patch then, but we'
rather want to wait until "git submodule summary" is converted to C.
(Maybe we can even parallelize the data gathering before output.)

Even for git.git (which has no submodules), we have

$ time git -c status.submoduleSummary=false status
...
real 0m0.014s
$ time git -c status.submoduleSummary=true status
...
real 0m0.125s

So additionally to porting that part to C, we may want to condition it
to a new function "int do_we_have_submodules_at_all(void)",
which has a similar heuristic as the once proposed patch
to recurse into submodules all the time
https://public-inbox.org/git/20161006193725.31553-3-sbeller@google.com/

Now that I explained why this patch is unacceptable as-is,
let's think of potential upsides:

git-status is the one command to deliver a summary on the state
of a repo with the granularity on the commit
(e.g. it reports the relation to its upstream tracking branch)
and file level (which files are change, untracked).

And I think a submodule is a weird between-state of these
two as it is seen like a file in the superproject, but it also
has commits, which are the smallest unit of logical change.

So maybe we'd want to report just a number in git-status,
which indicates how much the current detached head
is behind/before the remote tracking branch, and if it can be
fast forwarded.

Thanks,
Stefan