`git describe --dirty` doesn't consider untracked files to be dirty

`git describe --dirty` doesn't consider untracked files to be dirty

[Ash Holland]

Hi,

There seems to be a discrepancy between how `git describe --dirty` is
documented and how it actually behaves. The documentation describes
the --dirty flag like this:

> If the working tree has local modification "-dirty" is appended to it.

but certain kinds of "local modification", namely untracked files,
don't cause "-dirty" to be included.

Please could this be fixed, either in the documentation or in git describe?

thanks,
Ash
she/they

Re: `git describe --dirty` doesn't consider untracked files to be dirty

[Raymond E. Pasco]

On Mon Sep 7, 2020 at 5:04 AM EDT, Ash Holland wrote:
> There seems to be a discrepancy between how `git describe --dirty` is
> documented and how it actually behaves. The documentation describes
> the --dirty flag like this:
>
> > If the working tree has local modification "-dirty" is appended to it.
>
> but certain kinds of "local modification", namely untracked files,
> don't cause "-dirty" to be included.

I think the documentation here could be made clearer, but I'm not sure
of the precise wording that would be best.

I wonder if describe should have an option that considers the presence
of untracked (but not ignored, i.e. anything that would be flagged by
status) files to count as a dirty worktree. Implementing this option
might be the lazy way to make the documentation easier to rewrite.

Re: `git describe --dirty` doesn't consider untracked files to be dirty

[Junio C Hamano]

Ash Holland <ash@sorrel.sh> writes:

> There seems to be a discrepancy between how `git describe --dirty` is
> documented and how it actually behaves. The documentation describes
> the --dirty flag like this:
>
>> If the working tree has local modification "-dirty" is appended to it.

Not limited to what "describe" does, whenever we mention "local
modification", we only mean modification to tracked contents,
because by definition we do not detect or track "modifications" to
anything that is not tracked.  Untracked paths may have been
modified multiple times, but since they are not even added, we do
not notice nor care.

That is to say that the documentation and the code are consistent
with each other.

Having said all that, a source that was forgotten to be added, yet
affects the built product by a build rule with wildcard e.g.
"compile all *.c files and link them into a single binary", would
happen in real life, so from that point of view, appending "-dirty"
only when there is a local modification may not be all that useful,
and tweaking the "--dirty" option to also pay attention to untracked
(but not ignored) might have merit.  

I do not think this is something we want to hide behind a
configuration knob, but I am undecided between (1) declare that this
is a bug and change the behaviour of "--dirty" and (2) declare that
we discovered another useful behaviour and add a new option next to
"--dirty".

Thanks.

Re: `git describe --dirty` doesn't consider untracked files to be dirty

[Aaron Schrab]

At 09:33 -0700 08 Sep 2020, Junio C Hamano <gitster@pobox.com> wrote:
>Ash Holland <ash@sorrel.sh> writes:
>
>> There seems to be a discrepancy between how `git describe --dirty` is
>> documented and how it actually behaves. The documentation describes
>> the --dirty flag like this:
>>
>>> If the working tree has local modification "-dirty" is appended to it.
>
>Not limited to what "describe" does, whenever we mention "local
>modification", we only mean modification to tracked contents,
>because by definition we do not detect or track "modifications" to
>anything that is not tracked.  Untracked paths may have been
>modified multiple times, but since they are not even added, we do
>not notice nor care.

It's perhaps worth noting that submodules are already considered dirty 
when untracked files are added:

$ git diff vim/bundle/fugitive

$ echo foo >vim/bundle/fugitive/foo

$ git diff vim/bundle/fugitive
diff --git i/vim/bundle/fugitive w/vim/bundle/fugitive
--- i/vim/bundle/fugitive
+++ w/vim/bundle/fugitive
@@ -1 +1 @@
-Subproject commit caf3b1d5696e8d39a905e48f1e89d8c0c565168c
+Subproject commit caf3b1d5696e8d39a905e48f1e89d8c0c565168c-dirty

Re: `git describe --dirty` doesn't consider untracked files to be dirty

[Junio C Hamano]

Aaron Schrab <aaron@schrab.com> writes:

> It's perhaps worth noting that submodules are already considered dirty
> when untracked files are added:
>
> $ git diff vim/bundle/fugitive
>
> $ echo foo >vim/bundle/fugitive/foo
>
> $ git diff vim/bundle/fugitive
> diff --git i/vim/bundle/fugitive w/vim/bundle/fugitive
> --- i/vim/bundle/fugitive
> +++ w/vim/bundle/fugitive
> @@ -1 +1 @@
> -Subproject commit caf3b1d5696e8d39a905e48f1e89d8c0c565168c
> +Subproject commit caf3b1d5696e8d39a905e48f1e89d8c0c565168c-dirty

It gives one vote for (1) to the part you did not quote from the
message you are responding to, which was:

>> I do not think this is something we want to hide behind a
>> configuration knob, but I am undecided between (1) declare that this
>> is a bug and change the behaviour of "--dirty" and (2) declare that
>> we discovered another useful behaviour and add a new option next to
>> "--dirty".

I tend to agree the consistency with that behaviour would be more
useful.  The discrepanthy shows the relative age of features and how
our thinking has changed over time ;-)

Re: `git describe --dirty` doesn't consider untracked files to be dirty

[Junio C Hamano]

Aaron Schrab <aaron@schrab.com> writes:

> It's perhaps worth noting that submodules are already considered dirty
> when untracked files are added:
>
> $ git diff vim/bundle/fugitive
>
> $ echo foo >vim/bundle/fugitive/foo
>
> $ git diff vim/bundle/fugitive
> diff --git i/vim/bundle/fugitive w/vim/bundle/fugitive
> --- i/vim/bundle/fugitive
> +++ w/vim/bundle/fugitive
> @@ -1 +1 @@
> -Subproject commit caf3b1d5696e8d39a905e48f1e89d8c0c565168c
> +Subproject commit caf3b1d5696e8d39a905e48f1e89d8c0c565168c-dirty

In other words, if we do this in the state:

  $ git -C vim/bundle/fugitive describe --dirty

the submodule directory is not reported as dirty.

This is worth fixing.  I am leaning towards saying that `diff` is
wrong in this case, but I am OK to consider unifying the behaviour
the other way and making `describe --dirty` more strict.

Thanks.

Re: `git describe --dirty` doesn't consider untracked files to be dirty

[Ash Holland]

On Sat, 19 Sep 2020 at 19:12, Junio C Hamano <gitster@pobox.com> wrote:
> This is worth fixing.  I am leaning towards saying that `diff` is
> wrong in this case, but I am OK to consider unifying the behaviour
> the other way and making `describe --dirty` more strict.

fwiw, my preference would be for the second behaviour; I have a release
script which complains at me if I've forgotten to commit something, and
to avoid making a release with new uncommitted files I currently have to
use both `git describe --dirty` (to check for modifications to tracked
files) and also `git ls-files --others --exclude-standard` (to check for
untracked files).

maybe there's a better plumbing command I should be using in a script,
but your example of the wildcard build rule also would suggest that
`describe` should be changed, not `diff`:

> Having said all that, a source that was forgotten to be added, yet
> affects the built product by a build rule with wildcard e.g.
> "compile all *.c files and link them into a single binary", would
> happen in real life, so from that point of view, appending "-dirty"
> only when there is a local modification may not be all that useful,
> and tweaking the "--dirty" option to also pay attention to untracked
> (but not ignored) might have merit.

lastly, by appeal to `git clean`'s documentation: "Remove untracked
files from the working tree"

if you clean a repository by removing untracked files, then untracked
files surely make the working tree dirty :)

Re: `git describe --dirty` doesn't consider untracked files to be dirty

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Aaron Schrab <aaron@schrab.com> writes:
>
>> It's perhaps worth noting that submodules are already considered dirty
>> when untracked files are added:
>>
>> $ git diff vim/bundle/fugitive
>>
>> $ echo foo >vim/bundle/fugitive/foo
>>
>> $ git diff vim/bundle/fugitive
>> diff --git i/vim/bundle/fugitive w/vim/bundle/fugitive
>> --- i/vim/bundle/fugitive
>> +++ w/vim/bundle/fugitive
>> @@ -1 +1 @@
>> -Subproject commit caf3b1d5696e8d39a905e48f1e89d8c0c565168c
>> +Subproject commit caf3b1d5696e8d39a905e48f1e89d8c0c565168c-dirty
>
> In other words, if we do this in the state:
>
>   $ git -C vim/bundle/fugitive describe --dirty
>
> the submodule directory is not reported as dirty.
>
> This is worth fixing.  I am leaning towards saying that `diff` is
> wrong in this case, but I am OK to consider unifying the behaviour
> the other way and making `describe --dirty` more strict.

"git diff" family of commands know the "--ignore-submodules=<what>"
option, and it seems that by default they do not ignore "untracked".

This seems to be what causes its output fail to pretend as if output
from "git describe --dirty" in the submodule directory were used on
the working-tree side of the comparison and leads to this
inconsistency.  Obviously we can tweak the default of "diff" family
of commands to ignore untracked paths in submodules and that would
make them consistent with "git describe --dirty", but that would not
give us a new way to tweak behaviour of "git describe" like we can
do with "git diff --ignore-submodules=<what>".

The current "untracked files do not count as part of dirtiness"
default behaviour of "git describe --dirty" is relied upon by
people's existing scripts, and changing it from under them would
cause unnecessary breakage.  But that does not have to stop us from
teaching "git describe --dirty" an optional "--ignore=<what>"
option, similar to what "diff --ignore-submodules=<what>" option
does to the submodules.

The first step would be to allow those who want their "git describe
--dirty --ignore=none" (untracked files are counted as dirtiness, to
be consistent with how "git diff" sees submodule directories by
default) to use presence of untracked files as dirty.  This is a
safe first step and can be done without breaking any existing users.

After that materializes and users gain experience, we may want to
discuss if we want to change the default behaviour of "git describe
--dirty" or what value the future default should be, how bad the
compatibility breakage would be if we change the default, and what
the transition plan and schedule looks like.  But we do not have to
do such a longer-term planning before the first step happens.

Re: `git describe --dirty` doesn't consider untracked files to be dirty

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> The first step would be to allow those who want their "git describe
> --dirty --ignore=none" (untracked files are counted as dirtiness, to
> be consistent with how "git diff" sees submodule directories by
> default) to use presence of untracked files as dirty.  This is a
> safe first step and can be done without breaking any existing users.

It is worse than I thought.  There is zero-th step we need to have,
to fix "git describe --dirty" itself.

Because the command internally uses "diff-index", and by default it
considers that a submodule with untracked path *is* dirty.  Because
of that, you get an inexplicable inconsistent behaviour.

 * If you start from a pristine checkout, and then add an untracked
   path to the current project, "git describe --dirty" won't give
   the -dirty suffix.

 * But if you add an untracked path in its submodule, the command
   does give you the -dirty suffix.

A fix, without the first-step to give the command configurable
definition of what makes a repository 'dirty', would probably look
like the attached untested patch.  A fix to "diff" machinery to make
"--ignore-submodules=untracked" the default would also make "describe"
internally consistent, too.

 builtin/describe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/describe.c b/builtin/describe.c
index 7668591d57..af08d7d8cf 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -45,7 +45,7 @@ static struct commit_names commit_names;
 
 /* diff-index command arguments to check if working tree is dirty. */
 static const char *diff_index_args[] = {
-	"diff-index", "--quiet", "HEAD", "--", NULL
+	"diff-index", "--quiet", "--ignore-submodules=untracked", "HEAD", "--", NULL
 };
 
 struct commit_name {