Re: [Outreachy] [PATCH v3] diff: do not show submodule with untracked files as "-dirty"

From: Junio C Hamano <gitster@pobox.com>
To: Sangeeta Jain <sangunb09@gmail.com>
Cc: git@vger.kernel.org, phillip.wood123@gmail.com,
	kaartic.sivaraam@gmail.com, sunshine@sunshineco.com
Subject: Re: [Outreachy] [PATCH v3] diff: do not show submodule with untracked files as "-dirty"
Date: Thu, 22 Oct 2020 11:07:37 -0700	[thread overview]
Message-ID: <xmqqft66m8wm.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <20201022112201.51459-1-sangunb09@gmail.com> (Sangeeta Jain's message of "Thu, 22 Oct 2020 16:52:01 +0530")

Sangeeta Jain <sangunb09@gmail.com> writes:

> From: sangu09 <sangunb09@gmail.com>

Oops?

> Git diff reports a submodule directory as -dirty even when there are
> only untracked files in the submodule directory.
> This is inconsistent with what `git describe --dirty` says when run in
> the submodule directory in that state.

Nicely written.

> This patch makes `git diff HEAD` consistent with `git describe --dirty`
> when a submodule contains untracked files by making
> `--ignore-submodules=untracked` the default.

Instead of explaining "This patch makes...", it is more common to
give an order to the codebase, i.e. "Make...".

> Also, to avoid `ignoreSubmodules` in user config being overwritten,
> I have made added a flag in diff_flags to keep a track whether
> `handle_ignore_submodules_arg` is called before or not.

Well, that is not "Also"; it is an integral part of making
"--ignore-submodules=untracked" the _default_.  We need to make sure
that end-user configuration would be honored, and we need to make
sure that the command line option would be honored, too.

I'd suggest to follow the excellent first paragraph above with
something like the following.

	Make `--ignore-submodules=untracked` the default for `git
	diff` when there is no configuration variable or command
	line option, so that the command would not give '-dirty'
	suffix to a submodule whose working tree has untracked
	files, to make it consistent with `git describe --dirty`
	that is run in the submodule working tree.

> @@ -4585,6 +4585,9 @@ void repo_diff_setup(struct repository *r, struct diff_options *options)
>  		DIFF_XDL_SET(options, INDENT_HEURISTIC);
>  
>  	options->orderfile = diff_order_file_cfg;
> +	
> +	if (!options->flags.ignore_submodule_set)

This new boolean is meant to be set only when non-default
ignore-submodules option is given either from the command line or
from the configuration---when the bit is unset, we are told to do
whatever it is that is the default for us.

> +		options->flags.ignore_untracked_in_submodules = 1;

And the default is to flip only this bit on.  Do we need to turn off
other bits that submodule.c::handle_ignore_submodules_arg() function
touches?  [*1*]

> diff --git a/diff.h b/diff.h
> index 11de52e9e9..1e18e6b1c3 100644
> --- a/diff.h
> +++ b/diff.h
> @@ -178,6 +178,7 @@ struct diff_flags {
>  	unsigned diff_from_contents;
>  	unsigned dirty_submodules;
>  	unsigned ignore_untracked_in_submodules;
> +	unsigned ignore_submodule_set;
>  	unsigned ignore_dirty_submodules;
>  	unsigned override_submodule_config;
>  	unsigned dirstat_by_line;
> diff --git a/submodule.c b/submodule.c
> index b3bb59f066..a7956e1539 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -420,6 +420,7 @@ const char *submodule_strategy_to_string(const struct submodule_update_strategy
>  void handle_ignore_submodules_arg(struct diff_options *diffopt,
>  				  const char *arg)
>  {
> +	diffopt->flags.ignore_submodule_set = 1;

I like the general idea of having one bit that is set if and only if
the command line or configuration told us specifically what to do,
so that we can dictate the default after they were taken care of.

But I am not sure if this is a good implementation of that idea.  

Case in point.  I was wondering if the most future-proof way to
answer the question I asked (marked with [*1*] above) was to avoid
flipping the bits in options->flags ourselves, but to make a call

	handle_ignore_submodules_arg(&options, "untracked");

in repo_diff_setup().  When such an improvement is made after this
patch lands, the assumption that only the end-user preference will
call this function no longer holds.

Even without anticipating such a change in the future, there already
is a callsite of this function in wt-status.c that hands a hardcoded
string "dirty" to it.  That is *not* caused by an end-user request
(be it a configuration variable or a command line option), so in a
sense, the assumption is already broken.

I wonder, if we can do things in a more natural way (at least the
natural way in this codebase).  Usually we do things in this order:

 - initialize the status and option variables to their default state.

 - read the configuration files to allow the state of these
   variables to be modified from their default state.

 - parse the command line arguments to further allow the state of
   these variables to be modified.

and then use the final state of these variables.  That way, we do
not even need the extra bit that is only required if we did things
in an unnatural way, which is

 - read the config; remember if any bits were toggled
 - parse the command line; remember if any bits were toggled
 - only if bits weren't toggled in the above, set the default

I dunno.

> @@ -1677,7 +1678,9 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
>  
>  	strvec_pushl(&cp.args, "status", "--porcelain=2", NULL);
>  	if (ignore_untracked)
> -		strvec_push(&cp.args, "-uno");
> +		strvec_push (&cp.args, "-uno");

Noise.

> +	else
> +    	strvec_push (&cp.args, "--ignore-submodules=none");

Misindented.