[PATCH RFC] git describe without refs distinguishes dirty working tree

[PATCH RFC] git describe without refs distinguishes dirty working tree

[Jean Privat]

git describe without refs still works on HEAD but append "-dirty" if the
working three is dirty. If the working tree is clean, nothing is appended.

Previous behavior can be achieved by specifying "HEAD".

Many build scripts use `git describe` to produce a version number.
However, additional git commands are required to distinguish builds done
with a clean tree from builds done with a dirty tree.
This patch helps the writing of these scripts since `git describe` do
the intended thing.

This new behavior could affect existing scripts by producing version
number like v1.0.4-14-g2414721-dirty-dirty.
These scripts could be easily fixed by explicitly using HEAD when calling
`git describe` and works with any version of git.

Signed-off-by: Jean Privat <jean@pryen.org>
---

Initially, I wanted to add an option `--worktree` that works on HEAD and
appends "-dirty" when the working tree is dirty. After rethink I
realized that users (me included) should prefer to describe the working
tree by default, and only describe HEAD if HEAD was explicitly specified.

Note that documentation of `git describe` did not mentioned the behavior
of the command when no committish is specified.
However, since it is still a behavior change. If the patch is accepted,
it could target version 1.7.
---
 Documentation/git-describe.txt |    5 ++++-
 builtin-describe.c             |   18 +++++++++++++++++-
 t/t6120-describe.sh            |    8 ++++++++
 3 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt
index b231dbb..c49ecc8 100644
--- a/Documentation/git-describe.txt
+++ b/Documentation/git-describe.txt
@@ -8,7 +8,7 @@ git-describe - Show the most recent tag that is
reachable from a commit

 SYNOPSIS
 --------
-'git describe' [--all] [--tags] [--contains] [--abbrev=<n>] <committish>...
+'git describe' [--all] [--tags] [--contains] [--abbrev=<n>] [<committish>...]

 DESCRIPTION
 -----------
@@ -18,6 +18,9 @@ shown.  Otherwise, it suffixes the tag name with the number of
 additional commits on top of the tagged object and the
 abbreviated object name of the most recent commit.

+Without any committish, `git describe` opperates on HEAD and
+appends "-dirty" if the working tree is dirty.
+
 By default (without --all or --tags) `git describe` only shows
 annotated tags.  For more information about creating annotated tags
 see the -a and -s options to linkgit:git-tag[1].
diff --git a/builtin-describe.c b/builtin-describe.c
index df67a73..4105d9c 100644
--- a/builtin-describe.c
+++ b/builtin-describe.c
@@ -5,6 +5,7 @@
 #include "builtin.h"
 #include "exec_cmd.h"
 #include "parse-options.h"
+#include "diff.h"

 #define SEEN		(1u<<0)
 #define MAX_TAGS	(FLAG_BITS - 1)
@@ -23,6 +24,13 @@ static int max_candidates = 10;
 static int found_names;
 static const char *pattern;
 static int always;
+static int dirty; /* Is working tree dirty? */
+
+/* diff-index command arguments to check if working tree is dirty. */
+static const char *diff_index_args[] = {
+	"diff-index", "--quiet", "HEAD", "--", NULL
+};
+

 struct commit_name {
 	struct tag *tag;
@@ -208,6 +216,8 @@ static void describe(const char *arg, int last_one)
 		display_name(n);
 		if (longformat)
 			show_suffix(0, n->tag ? n->tag->tagged->sha1 : sha1);
+		if (dirty)
+			printf("-dirty");
 		printf("\n");
 		return;
 	}
@@ -265,7 +275,10 @@ static void describe(const char *arg, int last_one)
 	if (!match_cnt) {
 		const unsigned char *sha1 = cmit->object.sha1;
 		if (always) {
-			printf("%s\n", find_unique_abbrev(sha1, abbrev));
+			printf("%s", find_unique_abbrev(sha1, abbrev));
+			if (dirty)
+				printf("-dirty");
+			printf("\n");
 			return;
 		}
 		die("cannot describe '%s'", sha1_to_hex(sha1));
@@ -300,6 +313,8 @@ static void describe(const char *arg, int last_one)
 	display_name(all_matches[0].name);
 	if (abbrev)
 		show_suffix(all_matches[0].depth, cmit->object.sha1);
+	if (dirty)
+		printf("-dirty");
 	printf("\n");

 	if (!last_one)
@@ -360,6 +375,7 @@ int cmd_describe(int argc, const char **argv,
const char *prefix)
 	}

 	if (argc == 0) {
+		dirty = cmd_diff_index(ARRAY_SIZE(diff_index_args) - 1,
diff_index_args, prefix);
 		describe("HEAD", 1);
 	} else {
 		while (argc-- > 0) {
diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index 8c7e081..8938fc6 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -123,6 +123,14 @@ test_expect_success 'rename tag Q back to A' '
 test_expect_success 'pack tag refs' 'git pack-refs'
 check_describe A-* HEAD

+test_expect_success 'set-up dirty working tree' '
+	echo >>file
+'
+
+check_describe "A-*[0-9a-f]-dirty"
+
+check_describe "A-*[0-9a-f]" HEAD
+
 test_expect_success 'set-up matching pattern tests' '
 	git tag -a -m test-annotated test-annotated &&
 	echo >>file &&
-- 
1.6.5

Re: [PATCH RFC] git describe without refs distinguishes dirty working tree

[Junio C Hamano]

Jean Privat <jean@pryen.org> writes:

> ...
> This new behavior could affect existing scripts by producing version
> number like v1.0.4-14-g2414721-dirty-dirty.
> These scripts could be easily fixed by explicitly using HEAD when calling
> `git describe` and works with any version of git.
>
> Signed-off-by: Jean Privat <jean@pryen.org>
> ---
>
> Initially, I wanted to add an option `--worktree` that works on HEAD and
> appends "-dirty" when the working tree is dirty. After rethink I
> realized that users (me included) should prefer to describe the working
> tree by default, and only describe HEAD if HEAD was explicitly specified.
>
> Note that documentation of `git describe` did not mentioned the behavior
> of the command when no committish is specified.
> However, since it is still a behavior change. If the patch is accepted,
> it could target version 1.7.
> ---
>  Documentation/git-describe.txt |    5 ++++-
>  builtin-describe.c             |   18 +++++++++++++++++-
>  t/t6120-describe.sh            |    8 ++++++++
>  3 files changed, 29 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt
> index b231dbb..c49ecc8 100644
> --- a/Documentation/git-describe.txt
> +++ b/Documentation/git-describe.txt
> @@ -8,7 +8,7 @@ git-describe - Show the most recent tag that is
> reachable from a commit

Here is an indication of a linewrapped and broken patch that discourages
me to look further.

As to the new _ability_, I think it would make sense to reduce the need
for an extra invocation of "is the work tree dirty" and this addition is a
welcomed one in that sense.  However, as you already are aware of, this
will break existing scripts; it should not trigger for them.

How about "describe --dirty" and "describe --dirty=-mod" (the latter
creates v1.6.5-15-gc274db7-mod"), possibly with a short version of options
if this proves to be useful and frequently used from interactive sessions?

I personally think this does not deserve to have a short option (as you
said in the log message, it is primarily a way to make up a version number
string, and give interactive users a sense of where in the history he is.
If you want to know if your tree is dirty, depending on the reason _why_
you want to know it and what you want to do with the information after
learning your tree is dirty, "status", "diff --stat", "add -i" are more
appropriate and useful tools) but you (and others) may bring up use cases
that I didn't think of when I wrote the beginning of this sentence ;-)

Re: [PATCH RFC] git describe without refs distinguishes dirty working tree

[Shawn O. Pearce]

Jean Privat <jean@pryen.org> wrote:
> git describe without refs still works on HEAD but append "-dirty" if the
> working three is dirty. If the working tree is clean, nothing is appended.
> 
> Previous behavior can be achieved by specifying "HEAD".
> 
> Many build scripts use `git describe` to produce a version number.
> However, additional git commands are required to distinguish builds done
> with a clean tree from builds done with a dirty tree.
> This patch helps the writing of these scripts since `git describe` do
> the intended thing.
> 
> This new behavior could affect existing scripts by producing version
> number like v1.0.4-14-g2414721-dirty-dirty.
> These scripts could be easily fixed by explicitly using HEAD when calling
> `git describe` and works with any version of git.
> 
> Signed-off-by: Jean Privat <jean@pryen.org>

Yea, I think this is reasonable and sane thing to do.

Acked-by: Shawn O. Pearce <spearce@spearce.org>

> Note that documentation of `git describe` did not mentioned the behavior
> of the command when no committish is specified.
> However, since it is still a behavior change. If the patch is accepted,
> it could target version 1.7.

Yea, 1.7 may be safer for this, but also many scripts are based
on the GIT-VERSION-GEN in git.git which uses HEAD as an explicit
argument.  So they might actually be OK with this change.

-- 
Shawn.

Re: [PATCH RFC] git describe without refs distinguishes dirty working tree

[Junio C Hamano]

"Shawn O. Pearce" <spearce@spearce.org> writes:

> Yea, 1.7 may be safer for this, but also many scripts are based
> on the GIT-VERSION-GEN in git.git which uses HEAD as an explicit
> argument.  So they might actually be OK with this change.

Well, from my cursory look of its scripts/setlocalversion, even the Linux
kernel is not safe X-<.

I still haven't heard anything that helps me to decide which way the
default should be.  The only concrete thing I have heard against the
change of the default is that it will break existing setup, but I haven't
heard anything concrete for the change yet.

Re: [PATCH RFC] git describe without refs distinguishes dirty working tree

[Jean Privat]

> I still haven't heard anything that helps me to decide which way the
> default should be.  The only concrete thing I have heard against the
> change of the default is that it will break existing setup, but I haven't
> heard anything concrete for the change yet.

As a said in the comment part of the initial message, I initially
planed to add a --worktree option that means "I want you to describe
my working tree". Knowing that from my naive point of view, the
description of the working tree is what build script wanted :
description of HEAD (on which is based the working tree) + saying that
if the working tree is dirty or not (done manually by scripts).
Moreover, in my naive view with the "--worktree" option, no refs where
allowed (i.e. describe the working tree xor describe some commit
references).

Then, I realized that for some other git commands that can work both
on the working tree and on an arbitrary commit reference, the default
was to work on the working tree and require an explicit HEAD to work
on the HEAD commit. Thus it makes sense to me that "git describe"
alone should describe the working tree and that "git describe HEAD"
should describe the HEAD commit.
Moreover, since build scripts use "git describe" not to describe an
arbitrary commit but to describe the working tree, it is nice to make
what thez really want the default.

Currently, the breaking of scripts is the only reason that makes me
not entirely convinced that default should be to describe the working
tree. But I think that if "git describe" could be redesigned,
describing the working tree by default should be the right thing.
I also think that the breakage should not be so important:
* it could be fixed without requiring/checking a specific git version
* it appears only for people that build on a dirty tree
* the effect may be only to produce a strange version number
* git describe is porcelain :)
* 1.7 may break other things, so some fixing me be required anyway :).

If git people prefers that default remains HEAD, I have no real strong
objection (I still have to resend the patch because of line breaking
anyway).

> How about "describe --dirty" and "describe --dirty=-mod" (the latter
> creates v1.6.5-15-gc274db7-mod")

May be better than "--wortree" (especially because of the value part),
but what happen with
 $ git describe --dirty v1.2.1
should it show an error, output "v1.2.1" anyway, or output
"v1.2.1-dirty" if the working tree is different from v1.2.1 ?

-J

Re: [PATCH RFC] git describe without refs distinguishes dirty working tree

[Shawn O. Pearce]

Jean Privat <jean@pryen.org> wrote:
> > I still haven't heard anything that helps me to decide which way the
> > default should be.  The only concrete thing I have heard against the
> > change of the default is that it will break existing setup, but I haven't
> > heard anything concrete for the change yet.
...
> Then, I realized that for some other git commands that can work both
> on the working tree and on an arbitrary commit reference, the default
> was to work on the working tree and require an explicit HEAD to work
> on the HEAD commit. Thus it makes sense to me that "git describe"
> alone should describe the working tree and that "git describe HEAD"
> should describe the HEAD commit.

Yup.  That's my take on it too.  This default of "no argument means
describe the working tree" matches with tools like `git diff`,
`git checkout`, `git status`, `git blame` with no revision arguments.

We are being blasted by users for being inconsistent in our UI in too
many places.  Here's yet another.  We need to start standardizing
on a more consistent UI model.  If that model means we need to
use a "--worktree" flag to mean "against the working tree" then
we should start doing that also to `git status`, `git checkout`,
`git blame`, and `git diff`.

>  $ git describe --dirty v1.2.1
> should it show an error, output "v1.2.1" anyway, or output
> "v1.2.1-dirty" if the working tree is different from v1.2.1 ?

IMHO, that should be a fatal usage error, if we go that approach.
I would also argue `git describe --dirty HEAD` is equally fatal.

-- 
Shawn.

Re: [PATCH RFC] git describe without refs distinguishes dirty working tree

[Junio C Hamano]

Jean Privat <jean@pryen.org> writes:

>> I still haven't heard anything that helps me to decide which way the
>> default should be.  The only concrete thing I have heard against the
>> change of the default is that it will break existing setup, but I haven't
>> heard anything concrete for the change yet.
>
> As a said in the comment part of the initial message, I initially
> planed to add a --worktree option that means "I want you to describe
> my working tree". Knowing that from my naive point of view, the
> description of the working tree is what build script wanted :
> description of HEAD (on which is based the working tree) + saying that
> if the working tree is dirty or not (done manually by scripts).
> Moreover, in my naive view with the "--worktree" option, no refs where
> allowed (i.e. describe the working tree xor describe some commit
> references).

I do not think it is naive at all.  "git describe --worktree HEAD^" won't
make any sense in that world view; by definition the work tree is not
derived from anything but HEAD.

> Then, I realized that for some other git commands that can work both
> on the working tree and on an arbitrary commit reference, the default
> was to work on the working tree and require an explicit HEAD to work
> on the HEAD commit.

But the thing is, "git describe" without arguments already works on HEAD
and describes it, and people depend on the behaviour.

I originally thought this _might_ break and hoped it won't be a big issue,
but now I've seen that even the kernel would break (it runs the command
without saying "HEAD"), I do not want to risk breaking other projects I
may not even heard of.  Some people might have copied our GIT-VERSION-GEN
(that says "HEAD"), but I would not bet against that many many more people
would have copied the use of "git describe" from the kernel build tree
than from us.  After all, they are more famous and established than we
are.  [*1*]

One line of argument that I would have found reasonable to defend the
change of the default is that "The --worktree (or --dirty) option is
cumbersome to type.  And in _this_ workflow (that I haven't imagined, but
still would be *very* valid and useful one---so anybody who argues along
this line needs to fill in the blank here), it is the best solution to ask
the "git describe" command about the state of the work tree from the
command line very often, and here is why (here is another blank to fill
in), because there is no other way to get at the information, and/or the
info given by other existing commands are suboptimal for these reasons
(here is another blank to fill in)."

Then we would need to weigh benefits (for the interactive use of a very
useful and often used form of the command) against downsides (for people
having to update their existing scripts).

For use by scripts, the argument against having to give an extra option to
get the new behaviour becomes much weaker than for interactive use, even
if the new behaviour may be more common.  The point of scripting is that
you can write it once and forget about it; if it requires "--worktree" in
order to omit an extra "diff --index HEAD", it is not such a big deal.

And it is important to realize that "can write it once and forget about
it" cuts both ways.  It should apply not only to the people who write
their scripts after this patch of yours is applied to git, but should
equally apply to the people who wrote their scripts long time ago,
expecting that they can safely forget about them once they wrote them,
relying on the existing behaviour of the command.  Changing the defaults
for them means we lied to them and they have to update their scripts.  In
other words, they cannot script and forget about it anymore.

An attitude to change the default lightly like that, be it in 1.7.0 or
not, will then later come back and haunt your scripts you write while
assuming that the behaviour your patch will bring in will stay forever.
Allowing such a casual attitude will lead to other people changing the
default equally casually in the future.

It is my job to say no, even when I 100% agree that the new behaviour
would have been the default one if we were inventing "git describe" from
scratch and there were no existing users [*2*].  We do not live in an
ideal world, and 1.7.0 is not a blank check to change everything in sight.

>> How about "describe --dirty" and "describe --dirty=-mod" (the latter
>> creates v1.6.5-15-gc274db7-mod")
>
> May be better than "--wortree" (especially because of the value part),
> but what happen with
>  $ git describe --dirty v1.2.1
> should it show an error, output "v1.2.1" anyway, or output
> "v1.2.1-dirty" if the working tree is different from v1.2.1 ?

I am actually fine with your --worktree option, especially after seeing
your much more clear (i.e. "the state of my work tree") explanation in the
beginning of your response.

But I think "git describe" working in this "describe the version in the
work tree in the point in history" mode should reject any explicit
revision argument; by definition the work tree is not derived from
anything but HEAD.


[Footnote]

*1* In addition to the "it would break the kernel" issue, there are some
projects that expect users to tweak files they ship before building
(e.g. makefiles and config.h and the like).  For these people, any and all
builds would be -dirty.  I expect these projects, when migrating to git,
would either update their build procedures (moving config.h to
config.h-sample or something), but another solution for them would be to
use "git describe" without the -dirty bits.  So they are another class of
people who may not want -dirty by default.

*2* It personally hurts in a case like this, but that is what maintainers
have to do.  The maintainer's job is not to be loved, but is largely to
protect existing users from the second system syndrome.

Re: [PATCH RFC] git describe without refs distinguishes dirty working tree

[Nicolas Pitre]

On Fri, 16 Oct 2009, Junio C Hamano wrote:

> But the thing is, "git describe" without arguments already works on HEAD
> and describes it, and people depend on the behaviour.
> 
> I originally thought this _might_ break and hoped it won't be a big issue,
> but now I've seen that even the kernel would break (it runs the command
> without saying "HEAD"), I do not want to risk breaking other projects I
> may not even heard of.  Some people might have copied our GIT-VERSION-GEN
> (that says "HEAD"), but I would not bet against that many many more people
> would have copied the use of "git describe" from the kernel build tree
> than from us.

OpenOCD is another project that just started using Git and copied 
describe usage from Linux.


Nicolas