Re: [PATCH v3 1/4] status: add option to show ignored files differently

Jameson Miller [off-list ref] writes:

Teach the status command more flexibility in how ignored files are
reported. Currently, the reporting of ignored files and untracked
files are linked. You cannot control how ignored files are reported
independently of how untracked files are reported (i.e. `all` vs
`normal`). This makes it impossible to show untracked files with the
`all` option, but show ignored files with the `normal` option.

This work 1) adds the ability to control the reporting of ignored
files independently of untracked files and 2) introduces the concept
of status reporting ignored paths that explicitly match an ignored
pattern. ...

When status is set to report matching ignored files, it has the
following behavior. Ignored files and directories that explicitly
match an exclude pattern are reported. If an ignored directory matches
an exclude pattern, then the path of the directory is returned. If a
directory does not match an exclude pattern, but all of its contents
are ignored, then the contained files are reported instead of the
directory.

Thanks for an updated log message.  Very nicely explained.

We used to use "show_ignored_in_status" variable that can be either
0 (when no --ignored is given, or an explicit --no-ignored is given,
on the command line) or 1 (when --ignored is given on the command
line).

We still allow "--ignored" without value for backward compatibility,
but 

    $ git status -uall --ignored \*.c

may trigger die() from handle_ignored_arg().  I wonder if this is
something we care about.

	... goes and digs ...

OK, because of PARSE_OPT_OPTARG, the above is safe; "*.c" is not
mistaken as the value given to the --ignored option.  It also means
that

    $ git status -uno --ignored matching

would not mean what a naïve reader may expect and does not error
out, even though

    $ git status -uno --ignored=matching

would.  Which is something we eventually might care about, but that
is how parse-options PARSE_OPT_OPTARG works, and I consider "fixing"
it is totally out of the scope of this series (e.g. the next option
"--ignore-submodules" below shares exactly the same issue).

 		{ OPTION_STRING, 0, "ignore-submodules", &ignore_submodule_arg, N_("when"),
 		  N_("ignore changes to submodules, optional when: all, dirty, untracked. (Default: all)"),
 		  PARSE_OPT_OPTARG, NULL, (intptr_t)"all" },

Now we no longer use show_ignored_files that was a boolean yes/no,
and instead use an enum show_ignored_mode, we'd better spell this
out like so:

	if (s->show_ignored_mode == SHOW_NO_IGNORED) {

 		dir.flags |= DIR_SHOW_IGNORED_TOO;
-	else
+
+		if (s->show_ignored_mode == SHOW_MATCHING_IGNORED)
+			dir.flags |= DIR_SHOW_IGNORED_TOO_MODE_MATCHING;

Mental note: the old "show_ignored_files is true" case is now split
into two, i.e. traditional vs matching.  When matching is used,
dir.flags gets a new bit, i.e. DIR_SHOW_IGNORED_TOO_MODE_MATCHING.
Lack of that bit means dir.c helpers should behave as before.

Likewise, i.e.

	if (s->show_ignored_mode == SHOW_NO_IGNORED)

 			wt_longstatus_print_other(s, &s->ignored, _("Ignored files"), "add -f");
 		if (advice_status_u_option && 2000 < s->untracked_in_ms) {
 			status_printf_ln(s, GIT_COLOR_NORMAL, "%s", "");

It somehow feels superfluous to check both TOO and TOO_MODE_MATCHING
here (IOW, wouldn't the latter be superset of the former?  Is there
a valid case where the latter is set but not the former?), but it is
perhaps OK as belt-and-suspenders sanity.

+			/*
+			 * This is an excluded directory and we are
+			 * showing ignored paths that match an exclude
+			 * pattern.  (e.g. show directory as ignored
+			 * only if it matches an exclude pattern).
+			 * This path will either be 'path_excluded`
+			 * (if we are showing empty directories or if
+			 * the directory is not empty), or will be
+			 * 'path_none' (empty directory, and we are
+			 * not showing empty directories).
+			 */

The only caller of treat_directory() calls us when the path we are
seeing (sans trailing '/' that was added immediately before we were
called) passed "is_excluded()" check in the caller, so at this
point, the directory in question is being excluded because it
matched an exclude pattern.  IOW, if we are a/b/c, a pattern such as
"a/b/*" may have said we are excluded and made the call to this
function.  Do we need to ensure that a pattern like "a/*" is not
what excluded us?  Or IGNORED_TOO_MODE_MATCHING also should show
"a/b/c" when a pattern "a/*" matched it as "ignored because it
matched an exclude pattern"?

+			if (!(dir->flags & DIR_HIDE_EMPTY_DIRECTORIES))
+				return path_excluded;
+
+			if (read_directory_recursive(dir, istate, dirname, len,
+						     untracked, 1, 1, pathspec) == path_excluded)
+				return path_excluded;
+
+			return path_none;
+		}

... I guess we won't get into such a situation because this codepath
will return only _excluded or _none when we are handing "a/b" which
is already excluded "a/*", and does not cause the caller to recurse
into "a/b/c" in the first place.  So the updated code looks fine.