[RFC] describe: add option --dirty

[RFC] describe: add option --dirty

[Yasushi SHOJI]

when --dirty is given, git describe will check the working tree and
append "-dirty" to describe string if the tree is dirty.
---
I'm not sure this is good idea or the current way (using diff-index in
shell script) is more prefered.

one thing I found out is that we s/-/./g the out put of describe
before we append "-dirty".  this patch doesn't take care that. so with
this patch what we get is either

	v1.5.3-rc2-840-g1c0e2-dirty, or
	v1.5.3.rc2.840.g1c0e2.dirty

if we don't put more complecated sed command in GIT-VERSION-GEN.

anyway, comments welcome.

ps. another thing I don't like about this patch is that it changed to
pass prefix down to describe() as the last argument because I was lazy
to call cmd_diff_index() in describe().  if there is better way to do
it, please let me know.


 Documentation/git-describe.txt |    5 ++++-
 builtin-describe.c             |   29 ++++++++++++++++++++++-------
 2 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt
index f0bcb61..627c4d5 100644
--- a/Documentation/git-describe.txt
+++ b/Documentation/git-describe.txt
@@ -9,7 +9,7 @@ git-describe - Show the most recent tag that is reachable from a commit
 SYNOPSIS
 --------
 'git-describe' [--all] [--tags] [--contains] [--abbrev=<n>]
-               [--candidates=<n>] [--debug]
+               [--candidates=<n>] [--debug] [--dirty]
                <committish>...
 
 DESCRIPTION
@@ -53,6 +53,9 @@ OPTIONS
 	being employed to standard error.  The tag name will still
 	be printed to standard out.
 
+--dirty::
+	Append "-dirty" to describe string if working tree is dirty.
+
 EXAMPLES
 --------
 
diff --git a/builtin-describe.c b/builtin-describe.c
index e94f867..bebc16b 100644
--- a/builtin-describe.c
+++ b/builtin-describe.c
@@ -9,11 +9,12 @@
 #define MAX_TAGS	(FLAG_BITS - 1)
 
 static const char describe_usage[] =
-"git-describe [--all] [--tags] [--contains] [--abbrev=<n>] [--candidates] [--debug] <committish>*";
+"git-describe [--all] [--tags] [--contains] [--abbrev=<n>] [--candidates] [--debug] [--dirty] <committish>*";
 
 static int debug;	/* Display lots of verbose info */
 static int all;	/* Default to annotated tags only */
 static int tags;	/* But allow any tags if --tags is specified */
+static int check_dirty;	/* Append "-dirty" to describe string if working tree is dirty */
 static int abbrev = DEFAULT_ABBREV;
 static int max_candidates = 10;
 
@@ -125,7 +126,7 @@ static unsigned long finish_depth_computation(
 	return seen_commits;
 }
 
-static void describe(const char *arg, int last_one)
+static void describe(const char *arg, int last_one, const char *prefix)
 {
 	unsigned char sha1[20];
 	struct commit *cmit, *gave_up_on = NULL;
@@ -135,6 +136,7 @@ static void describe(const char *arg, int last_one)
 	struct possible_tag all_matches[MAX_TAGS];
 	unsigned int match_cnt = 0, annotated_cnt = 0, cur_match;
 	unsigned long seen_commits = 0;
+	char *dirty_string = "";
 
 	if (get_sha1(arg, sha1))
 		die("Not a valid object name %s", arg);
@@ -229,12 +231,23 @@ static void describe(const char *arg, int last_one)
 				sha1_to_hex(gave_up_on->object.sha1));
 		}
 	}
+	if (check_dirty) {
+		const char **args = xmalloc(5 * sizeof(char*));
+		args[0] = "diff-index";
+		args[1] = "--quiet";
+		args[2] = "--name-only";
+		args[3] = "HEAD";
+		args[4] = NULL;
+		if (cmd_diff_index(4, args, prefix))
+			dirty_string = "-dirty";
+	}
 	if (abbrev == 0)
-		printf("%s\n", all_matches[0].name->path );
+		printf("%s%s\n", all_matches[0].name->path, dirty_string);
 	else
-		printf("%s-%d-g%s\n", all_matches[0].name->path,
+		printf("%s-%d-g%s%s\n", all_matches[0].name->path,
 		       all_matches[0].depth,
-		       find_unique_abbrev(cmit->object.sha1, abbrev));
+		       find_unique_abbrev(cmit->object.sha1, abbrev),
+		       dirty_string);
 
 	if (!last_one)
 		clear_commit_marks(cmit, -1);
@@ -250,6 +263,8 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 
 		if (*arg != '-')
 			break;
+		else if (!strcmp(arg, "--dirty"))
+			check_dirty = 1;
 		else if (!strcmp(arg, "--contains"))
 			contains = 1;
 		else if (!strcmp(arg, "--debug"))
@@ -290,10 +305,10 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 	}
 
 	if (argc <= i)
-		describe("HEAD", 1);
+		describe("HEAD", 1, prefix);
 	else
 		while (i < argc) {
-			describe(argv[i], (i == argc - 1));
+			describe(argv[i], (i == argc - 1), prefix);
 			i++;
 		}
 
-- 
1.5.3.rc2.4.g726f9

Re: [RFC] describe: add option --dirty

[Junio C Hamano]

Yasushi SHOJI <yashi@atmark-techno.com> writes:

> when --dirty is given, git describe will check the working tree and
> append "-dirty" to describe string if the tree is dirty.
> ---
> I'm not sure this is good idea or the current way (using diff-index in
> shell script) is more prefered.

Hmph, this makes sense _ONLY_ for HEAD, doesn't it?

IOW, what should this output?

	$ git checkout v1.5.0 ;# detached HEAD
        $ git reset --hard  ;# clean slate
        $ echo >>Makefile ;# not anymore
        $ git describe --dirty v1.4.0^1

Should it say "v1.4.0-rc2-156-g0a8f4f0-dirty"?  The dirtiness
does not have anything to do with commit v1.4.0^1, so...

Re: [RFC] describe: add option --dirty

[Shawn O. Pearce]

Junio C Hamano <gitster@pobox.com> wrote:
> Yasushi SHOJI <yashi@atmark-techno.com> writes:
> 
> > when --dirty is given, git describe will check the working tree and
> > append "-dirty" to describe string if the tree is dirty.
> > ---
> > I'm not sure this is good idea or the current way (using diff-index in
> > shell script) is more prefered.
> 
> Hmph, this makes sense _ONLY_ for HEAD, doesn't it?
> 
> IOW, what should this output?
> 
> 	$ git checkout v1.5.0 ;# detached HEAD
>         $ git reset --hard  ;# clean slate
>         $ echo >>Makefile ;# not anymore
>         $ git describe --dirty v1.4.0^1
> 
> Should it say "v1.4.0-rc2-156-g0a8f4f0-dirty"?  The dirtiness
> does not have anything to do with commit v1.4.0^1, so...

Good catch.  I had that in my mind when I was reading the patch,
but failed to mention it.  I blame metze on #git, he interrupted
my train of thought.  ;-)


I think the answer is the user passes either --dirty OR one or
more commit-ish.  But not --dirty and a commit-ish.  In other words
you can either describe the working directory state, or a commit,
but not both at once.  Which also neatly solves my issue with
diff-index running more than once.

-- 
Shawn.

Re: [RFC] describe: add option --dirty

[Yasushi SHOJI]

At Mon, 23 Jul 2007 03:08:18 -0400,
Shawn O. Pearce wrote:
> 
> Junio C Hamano <gitster@pobox.com> wrote:
> > Yasushi SHOJI <yashi@atmark-techno.com> writes:
> > 
> > > when --dirty is given, git describe will check the working tree and
> > > append "-dirty" to describe string if the tree is dirty.
> > > ---
> > > I'm not sure this is good idea or the current way (using diff-index in
> > > shell script) is more prefered.
> > 
> > Hmph, this makes sense _ONLY_ for HEAD, doesn't it?
> > 
> > IOW, what should this output?
> > 
> > 	$ git checkout v1.5.0 ;# detached HEAD
> >         $ git reset --hard  ;# clean slate
> >         $ echo >>Makefile ;# not anymore
> >         $ git describe --dirty v1.4.0^1
> > 
> > Should it say "v1.4.0-rc2-156-g0a8f4f0-dirty"?  The dirtiness
> > does not have anything to do with commit v1.4.0^1, so...
> 
> Good catch.  I had that in my mind when I was reading the patch,
> but failed to mention it.  I blame metze on #git, he interrupted
> my train of thought.  ;-)

I knew the issue be failed to note about it.  Thanks.

> I think the answer is the user passes either --dirty OR one or
> more commit-ish.  But not --dirty and a commit-ish.  In other words
> you can either describe the working directory state, or a commit,
> but not both at once.  Which also neatly solves my issue with
> diff-index running more than once.

So the point is would it be worth implementing in usable form?

>From the comments I'd add an option "--workinig-tree" instead of
--dirty to describe the working tree.  because that, the special case,
is what we want after all,

synopsis would be:

	SYNOPSIS
	--------
	'git-describe' [--all] [--tags] [--contains] [--abbrev=<n>]
	               [--candidates=<n>] [--debug]
	               --working-tree | <committish>...
	    :
	    :
	--working-tree::
		Describe the working tree instead of committishes.  if the
		working tree is dirty, the describe string will have "-dirty"
		appended.
	
		As you can assume from the name, this option requires working
		tree; running it on a bare repository will fail.

what do you think?
-- 
         yashi

Re: [RFC] describe: add option --dirty

[Shawn O. Pearce]

Yasushi SHOJI <yashi@atmark-techno.com> wrote:
> From the comments I'd add an option "--workinig-tree" instead of
> --dirty to describe the working tree.  because that, the special case,
> is what we want after all,
> 
> synopsis would be:
> 
> 	SYNOPSIS
> 	--------
> 	'git-describe' [--all] [--tags] [--contains] [--abbrev=<n>]
> 	               [--candidates=<n>] [--debug]
> 	               --working-tree | <committish>...
> 	    :
> 	    :
> 	--working-tree::
> 		Describe the working tree instead of committishes.  if the
> 		working tree is dirty, the describe string will have "-dirty"
> 		appended.
> 	
> 		As you can assume from the name, this option requires working
> 		tree; running it on a bare repository will fail.
> 
> what do you think?

That's reasonable.  It seems like a lot of work in core Git just
to avoid a small chunk of shell, but I think almost everyone has
that same small chunk of shell in their build scripts...

-- 
Shawn.

Re: [RFC] describe: add option --dirty

[Yasushi SHOJI]

At Mon, 23 Jul 2007 03:58:34 -0400,
Shawn O. Pearce wrote:
> 
> Yasushi SHOJI <yashi@atmark-techno.com> wrote:
> > From the comments I'd add an option "--workinig-tree" instead of
> > --dirty to describe the working tree.  because that, the special case,
> > is what we want after all,
> > 
> > synopsis would be:
> > 
> > 	SYNOPSIS
> > 	--------
> > 	'git-describe' [--all] [--tags] [--contains] [--abbrev=<n>]
> > 	               [--candidates=<n>] [--debug]
> > 	               --working-tree | <committish>...
> > 	    :
> > 	    :
> > 	--working-tree::
> > 		Describe the working tree instead of committishes.  if the
> > 		working tree is dirty, the describe string will have "-dirty"
> > 		appended.
> > 	
> > 		As you can assume from the name, this option requires working
> > 		tree; running it on a bare repository will fail.
> > 
> > what do you think?
> 
> That's reasonable.  It seems like a lot of work in core Git just
> to avoid a small chunk of shell, but I think almost everyone has
> that same small chunk of shell in their build scripts...

it's not that much.  here it is.  hope you like it.


>From 25acf0fad4866b87998b51f1e66f540f2bcc5f0d Mon Sep 17 00:00:00 2001
From: Yasushi SHOJI <yashi@atmark-techno.com>
Date: Mon, 23 Jul 2007 17:49:11 +0900
Subject: [PATCH] describe: add a new option --working-tree

Many people like to use git-describe for version number, and yet
people always tack in the -dirty if the directory is dirty according
to diff-index. So this patch tries to scratch the itchy.

With --working-tree, the command will describe the working tree
instead of committishes. If the working tree is dirty, the describe
string will have "-dirty" appended.

As you can assume from the name, this option requires working tree.
running it on a bare repository will fail.

Signed-off-by: Yasushi SHOJI <yashi@atmark-techno.com>
---
 Documentation/git-describe.txt |   10 +++++++++-
 builtin-describe.c             |   38 +++++++++++++++++++++++++++++++-------
 2 files changed, 40 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt
index f0bcb61..d9b1550 100644
--- a/Documentation/git-describe.txt
+++ b/Documentation/git-describe.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 --------
 'git-describe' [--all] [--tags] [--contains] [--abbrev=<n>]
                [--candidates=<n>] [--debug]
-               <committish>...
+               --working-tree | <committish>...
 
 DESCRIPTION
 -----------
@@ -53,6 +53,14 @@ OPTIONS
 	being employed to standard error.  The tag name will still
 	be printed to standard out.
 
+--working-tree::
+	Describe the working tree instead of committishes.  if the
+	working tree is dirty, the describe string will have "-dirty"
+	appended.
+
+	As you can assume from the name, this option requires a
+	working tree. Running it on a bare repository will fail.
+
 EXAMPLES
 --------
 
diff --git a/builtin-describe.c b/builtin-describe.c
index e94f867..1c58480 100644
--- a/builtin-describe.c
+++ b/builtin-describe.c
@@ -9,11 +9,12 @@
 #define MAX_TAGS	(FLAG_BITS - 1)
 
 static const char describe_usage[] =
-"git-describe [--all] [--tags] [--contains] [--abbrev=<n>] [--candidates] [--debug] <committish>*";
+"git-describe [--all] [--tags] [--contains] [--abbrev=<n>] [--candidates] [--debug] --working-tree | <committish>*";
 
 static int debug;	/* Display lots of verbose info */
 static int all;	/* Default to annotated tags only */
 static int tags;	/* But allow any tags if --tags is specified */
+static int working_tree;/* describe the working tree instead of committish */
 static int abbrev = DEFAULT_ABBREV;
 static int max_candidates = 10;
 
@@ -125,7 +126,7 @@ static unsigned long finish_depth_computation(
 	return seen_commits;
 }
 
-static void describe(const char *arg, int last_one)
+static void describe(const char *arg, int last_one, int dirty)
 {
 	unsigned char sha1[20];
 	struct commit *cmit, *gave_up_on = NULL;
@@ -135,6 +136,7 @@ static void describe(const char *arg, int last_one)
 	struct possible_tag all_matches[MAX_TAGS];
 	unsigned int match_cnt = 0, annotated_cnt = 0, cur_match;
 	unsigned long seen_commits = 0;
+	char *dirty_string = dirty ? "-dirty" : "";
 
 	if (get_sha1(arg, sha1))
 		die("Not a valid object name %s", arg);
@@ -230,11 +232,12 @@ static void describe(const char *arg, int last_one)
 		}
 	}
 	if (abbrev == 0)
-		printf("%s\n", all_matches[0].name->path );
+		printf("%s%s\n", all_matches[0].name->path, dirty_string);
 	else
-		printf("%s-%d-g%s\n", all_matches[0].name->path,
+		printf("%s-%d-g%s%s\n", all_matches[0].name->path,
 		       all_matches[0].depth,
-		       find_unique_abbrev(cmit->object.sha1, abbrev));
+		       find_unique_abbrev(cmit->object.sha1, abbrev),
+		       dirty_string);
 
 	if (!last_one)
 		clear_commit_marks(cmit, -1);
@@ -244,12 +247,15 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 {
 	int i;
 	int contains = 0;
+	int dirty = 0;
 
 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];
 
 		if (*arg != '-')
 			break;
+		else if (!strcmp(arg, "--working-tree"))
+			working_tree = 1;
 		else if (!strcmp(arg, "--contains"))
 			contains = 1;
 		else if (!strcmp(arg, "--debug"))
@@ -276,6 +282,24 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 
 	save_commit_buffer = 0;
 
+	if (working_tree) {
+		const char **args;
+
+		if (!is_inside_work_tree() || is_inside_git_dir())
+			die("%s with --working-tree must be run in a working tree", argv[0]);
+		if (argc > i)
+			die("--working-tree doesn't take any committish\n");
+
+		args = xmalloc(5 * sizeof(char*));
+		args[0] = "diff-index";
+		args[1] = "--quiet";
+		args[2] = "--name-only";
+		args[3] = "HEAD";
+		args[4] = NULL;
+		if (cmd_diff_index(4, args, prefix))
+			dirty = 1;
+	}
+
 	if (contains) {
 		const char **args = xmalloc((4 + argc - i) * sizeof(char*));
 		args[0] = "name-rev";
@@ -290,10 +314,10 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 	}
 
 	if (argc <= i)
-		describe("HEAD", 1);
+		describe("HEAD", 1, dirty);
 	else
 		while (i < argc) {
-			describe(argv[i], (i == argc - 1));
+			describe(argv[i], (i == argc - 1), dirty);
 			i++;
 		}
 
-- 
1.5.3.rc2.4.g726f9

Re: [RFC] describe: add option --dirty

[Shawn O. Pearce]

Yasushi SHOJI <yashi@atmark-techno.com> wrote:
> when --dirty is given, git describe will check the working tree and
> append "-dirty" to describe string if the tree is dirty.
> ---
> I'm not sure this is good idea or the current way (using diff-index in
> shell script) is more prefered.

Yea, I'm actually torn on this.  A lot of people like the output of
git-describe for versions (where a lot is at least me!) and yet I
also always tack in the -dirty if the directory is dirty according
to diff-index.  So having this built right into git-describe is
actually quite handy.  It simplifies a little bit of build rule
logic.

> diff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt
> @@ -53,6 +53,9 @@ OPTIONS
>  	being employed to standard error.  The tag name will still
>  	be printed to standard out.
>  
> +--dirty::
> +	Append "-dirty" to describe string if working tree is dirty.
> +

It requires a working directory.  Running this in a bare repository
with --dirty won't work.  You might want to discuss that in the
documentation.

> diff --git a/builtin-describe.c b/builtin-describe.c
> @@ -229,12 +231,23 @@ static void describe(const char *arg, int last_one)
>  				sha1_to_hex(gave_up_on->object.sha1));
>  		}
>  	}
> +	if (check_dirty) {
> +		const char **args = xmalloc(5 * sizeof(char*));
> +		args[0] = "diff-index";
> +		args[1] = "--quiet";
> +		args[2] = "--name-only";
> +		args[3] = "HEAD";
> +		args[4] = NULL;
> +		if (cmd_diff_index(4, args, prefix))
> +			dirty_string = "-dirty";
> +	}

So if I describe two different commits at once in the same working
tree you are going to run diff-index twice?  That's not a great idea.
The outcome of diff-index won't change between those two commits.
Better to compute this up front before calling the describe()
function, and instead of passing in prefix pass in dirty_string.
Or just make it a global, like you did to the option flag.

>  	if (abbrev == 0)
> -		printf("%s\n", all_matches[0].name->path );
> +		printf("%s%s\n", all_matches[0].name->path, dirty_string);
>  	else
> -		printf("%s-%d-g%s\n", all_matches[0].name->path,
> +		printf("%s-%d-g%s%s\n", all_matches[0].name->path,
>  		       all_matches[0].depth,
> -		       find_unique_abbrev(cmit->object.sha1, abbrev));
> +		       find_unique_abbrev(cmit->object.sha1, abbrev),
> +		       dirty_string);
>  
>  	if (!last_one)
>  		clear_commit_marks(cmit, -1);

So if HEAD is exactly matching a tag you don't output the -dirty
suffix, even if the working tree is dirty?  That's counter to the
documentation above.  See l.150-154, we break out of the describe
function very quickly if there is a tag on the input commit.

-- 
Shawn.