bug in git-revert

bug in git-revert

[Jeff King]

In 9509af6 (Make git-revert & git-cherry-pick a builtin), you introduced
the following code:

   252          } else {
   253                  struct wt_status s;
   254
   255                  if (get_sha1("HEAD", head))
   256                          die ("You do not have a valid HEAD");
   257                  wt_status_prepare(&s);
   258                  if (s.commitable || s.workdir_dirty)
   259                          die ("Dirty index: cannot %s", me);
   260                  discard_cache();
   261          }

(where the die condition was later changed to allow a dirty workdir). Am
I crazy, or is it impossible to trigger either of those conditions?
wt_status_prepare is about setting up the object to do the actual diff.
At that point it hasn't actually looked at the index or file contents,
and so commitable is _always_ 0. And if it did do the right thing, it
would have spewed a bunch of stuff to stdout.

I think it should be fixable by something like the patch below, but it
feels a little hack-ish. Should there perhaps be an equivalent to
"wt_status_print_updated" that just finds the commitable flag?

diff --git a/builtin-revert.c b/builtin-revert.c
index b6dee6a..64f381c 100644
--- a/builtin-revert.c
+++ b/builtin-revert.c
@@ -279,6 +279,8 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 		if (get_sha1("HEAD", head))
 			die ("You do not have a valid HEAD");
 		wt_status_prepare(&s);
+		s.fp = NULL;
+		wt_status_print_updated(&s);
 		if (s.commitable)
 			die ("Dirty index: cannot %s", me);
 		discard_cache();
diff --git a/color.c b/color.c
index 12a6453..6dba762 100644
--- a/color.c
+++ b/color.c
@@ -160,6 +160,8 @@ static int color_vfprintf(FILE *fp, const char *color, const char *fmt,
 {
 	int r = 0;
 
+	if (!fp)
+		return 0;
 	if (*color)
 		r += fprintf(fp, "%s", color);
 	r += vfprintf(fp, fmt, args);
diff --git a/wt-status.c b/wt-status.c
index 32d780a..bf0934a 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -169,7 +169,8 @@ static void wt_status_print_filepair(struct wt_status *s,
 	default:
 		die("bug: unhandled diff status %c", p->status);
 	}
-	fprintf(s->fp, "\n");
+	if (s->fp)
+		fprintf(s->fp, "\n");
 	strbuf_release(&onebuf);
 	strbuf_release(&twobuf);
 }
@@ -238,7 +239,7 @@ static void wt_status_print_initial(struct wt_status *s)
 	strbuf_release(&buf);
 }
 
-static void wt_status_print_updated(struct wt_status *s)
+void wt_status_print_updated(struct wt_status *s)
 {
 	struct rev_info rev;
 	init_revisions(&rev, NULL);
@@ -369,8 +370,10 @@ void wt_status_print(struct wt_status *s)
 	if (s->verbose && !s->is_initial)
 		wt_status_print_verbose(s);
 	if (!s->commitable) {
-		if (s->amend)
-			fprintf(s->fp, "# No changes\n");
+		if (s->amend) {
+			if (s->fp)
+				fprintf(s->fp, "# No changes\n");
+		}
 		else if (s->nowarn)
 			; /* nothing */
 		else if (s->workdir_dirty)
diff --git a/wt-status.h b/wt-status.h
index 02afaa6..7d40a57 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -32,5 +32,6 @@ int wt_status_use_color;
 int wt_status_relative_paths;
 void wt_status_prepare(struct wt_status *s);
 void wt_status_print(struct wt_status *s);
+void wt_status_print_updated(struct wt_status *s);
 
 #endif /* STATUS_H */

Re: bug in git-revert

[Johannes Schindelin]

Hi,

On Sun, 2 Mar 2008, Jeff King wrote:

> In 9509af6 (Make git-revert & git-cherry-pick a builtin), you introduced
> the following code:
> 
>    252          } else {
>    253                  struct wt_status s;
>    254
>    255                  if (get_sha1("HEAD", head))
>    256                          die ("You do not have a valid HEAD");
>    257                  wt_status_prepare(&s);
>    258                  if (s.commitable || s.workdir_dirty)
>    259                          die ("Dirty index: cannot %s", me);
>    260                  discard_cache();
>    261          }
> 
> (where the die condition was later changed to allow a dirty workdir). Am
> I crazy, or is it impossible to trigger either of those conditions?
> wt_status_prepare is about setting up the object to do the actual diff.
> At that point it hasn't actually looked at the index or file contents,
> and so commitable is _always_ 0.

Well, then this is my mistake.  I haven't looked closely at what 
wt_status_prepare() does (or did).

Ciao,
Dscho

Re: bug in git-revert

[Jeff King]

On Sun, Mar 02, 2008 at 01:44:49AM -0500, Jeff King wrote:

> --- a/builtin-revert.c
> +++ b/builtin-revert.c
> @@ -279,6 +279,8 @@ static int revert_or_cherry_pick(int argc, const char **argv)
>  		if (get_sha1("HEAD", head))
>  			die ("You do not have a valid HEAD");
>  		wt_status_prepare(&s);
> +		s.fp = NULL;
> +		wt_status_print_updated(&s);
>  		if (s.commitable)
>  			die ("Dirty index: cannot %s", me);
>  		discard_cache();

Actually, this isn't right. It needs to call wt_status_print(&s), since
it does some magic that wt_status_print_updated() requires to run
correctly.

-Peff

[PATCH] revert: actually check for a dirty index

[Jeff King]

The previous code mistakenly used wt_status_prepare to check
whether the index had anything commitable in it; however,
that function is just an init function, and will never
report a dirty index.

The correct way with wt_status_* would be to call
wt_status_print with the output pointing to /dev/null or
similar. However, that does extra work by both examining the
working tree and spewing status information to nowhere.

Instead, let's just implement the useful subset of
wt_status_print as an "is_index_dirty" function.

Signed-off-by: Jeff King <peff@peff.net>
---
I said:
> I think it should be fixable by something like the patch below, but it
> feels a little hack-ish. Should there perhaps be an equivalent to
> "wt_status_print_updated" that just finds the commitable flag?

This is much nicer, and more efficient to boot.

 builtin-revert.c              |   34 ++++++++++++++++++++++++++++++----
 t/t3501-revert-cherry-pick.sh |    9 +++++++++
 2 files changed, 39 insertions(+), 4 deletions(-)

diff --git a/builtin-revert.c b/builtin-revert.c
index b6dee6a..15ab969 100644
--- a/builtin-revert.c
+++ b/builtin-revert.c
@@ -9,6 +9,9 @@
 #include "utf8.h"
 #include "parse-options.h"
 #include "cache-tree.h"
+#include "diff.h"
+#include "diffcore.h"
+#include "revision.h"
 
 /*
  * This implements the builtins revert and cherry-pick.
@@ -246,6 +249,30 @@ static char *help_msg(const unsigned char *sha1)
 	return helpbuf;
 }
 
+static void index_is_dirty_cb(struct diff_queue_struct *q,
+		struct diff_options *options,
+		void *data)
+{
+	int *is_dirty = data;
+	if (q->nr > 0)
+		*is_dirty = 1;
+}
+
+static int index_is_dirty(void)
+{
+	struct rev_info rev;
+	int is_dirty = 0;
+
+	init_revisions(&rev, NULL);
+	setup_revisions(0, NULL, &rev, "HEAD");
+	rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
+	rev.diffopt.format_callback = index_is_dirty_cb;
+	rev.diffopt.format_callback_data = &is_dirty;
+	run_diff_index(&rev, 1);
+
+	return is_dirty;
+}
+
 static int revert_or_cherry_pick(int argc, const char **argv)
 {
 	unsigned char head[20];
@@ -274,12 +301,11 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 		if (write_cache_as_tree(head, 0, NULL))
 			die ("Your index file is unmerged.");
 	} else {
-		struct wt_status s;
-
 		if (get_sha1("HEAD", head))
 			die ("You do not have a valid HEAD");
-		wt_status_prepare(&s);
-		if (s.commitable)
+		if(read_cache() < 0)
+			die("could not read the index");
+		if (index_is_dirty())
 			die ("Dirty index: cannot %s", me);
 		discard_cache();
 	}
diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
index 2dbe04f..6da2128 100755
--- a/t/t3501-revert-cherry-pick.sh
+++ b/t/t3501-revert-cherry-pick.sh
@@ -59,4 +59,13 @@ test_expect_success 'revert after renaming branch' '
 
 '
 
+test_expect_success 'revert forbidden on dirty working tree' '
+
+	echo content >extra_file &&
+	git add extra_file &&
+	test_must_fail git revert HEAD 2>errors &&
+	grep "Dirty index" errors
+
+'
+
 test_done
-- 
1.5.4.3.365.gc1491.dirty

Re: [PATCH] revert: actually check for a dirty index

[Alex Riesen]

Jeff King, Sun, Mar 02, 2008 08:22:52 +0100:
> +static int index_is_dirty(void)
> +{
> +	struct rev_info rev;
> +	int is_dirty = 0;
> +
> +	init_revisions(&rev, NULL);
> +	setup_revisions(0, NULL, &rev, "HEAD");
> +	rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
> +	rev.diffopt.format_callback = index_is_dirty_cb;
> +	rev.diffopt.format_callback_data = &is_dirty;
> +	run_diff_index(&rev, 1);
> +
> +	return is_dirty;
> +}

Wouldn't something like what built-commit does more effective?
It does:

	struct rev_info rev;
	unsigned char sha1[20];
	const char *parent = "HEAD";

	if (!active_nr && read_cache() < 0)
		die("Cannot read index");

	if (amend)
		parent = "HEAD^1";

	if (get_sha1(parent, sha1))
		commitable = !!active_nr;
	else {
		init_revisions(&rev, "");
		rev.abbrev = 0;
		setup_revisions(0, NULL, &rev, parent);
		DIFF_OPT_SET(&rev.diffopt, QUIET);
		DIFF_OPT_SET(&rev.diffopt, EXIT_WITH_STATUS);
		run_diff_index(&rev, 1 /* cached */);

		commitable = !!DIFF_OPT_TST(&rev.diffopt, HAS_CHANGES);
	}

Diff-index in this case will stop as soon as the first difference
found, while just using the output method will enforce finding all the
differences, which is just a waste of time, if all you need is just to
know if index is different to HEAD.

Re: [PATCH] revert: actually check for a dirty index

[Jeff King]

On Sun, Mar 02, 2008 at 11:37:53AM +0100, Alex Riesen wrote:

> Wouldn't something like what built-commit does more effective?

I assumed git-commit used wt_status_print, and it does, but only for
some code paths. When possible, it uses the more efficient version you
mentioned.

So here is my patch respun with the same method. I considered making a
globally available "is_index_dirty" function, but I think there are a
lot of assumptions that make such a function more trouble than the 6
lines it saves. E.g., "dirty versus what?", "which index?", "has the
index been loaded already?"

Thanks Alex.

-Peff

-- >8 --
revert: actually check for a dirty index

The previous code mistakenly used wt_status_prepare to check
whether the index had anything commitable in it; however,
that function is just an init function, and will never
report a dirty index.

The correct way with wt_status_* would be to call
wt_status_print with the output pointing to /dev/null or
similar. However, that does extra work by both examining the
working tree and spewing status information to nowhere.

Instead, let's just implement the useful subset of
wt_status_print as an "is_index_dirty" function.
---
 builtin-revert.c              |   20 ++++++++++++++++----
 t/t3501-revert-cherry-pick.sh |    9 +++++++++
 2 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/builtin-revert.c b/builtin-revert.c
index b6dee6a..1989f96 100644
--- a/builtin-revert.c
+++ b/builtin-revert.c
@@ -9,6 +9,8 @@
 #include "utf8.h"
 #include "parse-options.h"
 #include "cache-tree.h"
+#include "diff.h"
+#include "revision.h"
 
 /*
  * This implements the builtins revert and cherry-pick.
@@ -246,6 +248,17 @@ static char *help_msg(const unsigned char *sha1)
 	return helpbuf;
 }
 
+static int index_is_dirty(void)
+{
+	struct rev_info rev;
+	init_revisions(&rev, NULL);
+	setup_revisions(0, NULL, &rev, "HEAD");
+	DIFF_OPT_SET(&rev.diffopt, QUIET);
+	DIFF_OPT_SET(&rev.diffopt, EXIT_WITH_STATUS);
+	run_diff_index(&rev, 1);
+	return !!DIFF_OPT_TST(&rev.diffopt, HAS_CHANGES);
+}
+
 static int revert_or_cherry_pick(int argc, const char **argv)
 {
 	unsigned char head[20];
@@ -274,12 +287,11 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 		if (write_cache_as_tree(head, 0, NULL))
 			die ("Your index file is unmerged.");
 	} else {
-		struct wt_status s;
-
 		if (get_sha1("HEAD", head))
 			die ("You do not have a valid HEAD");
-		wt_status_prepare(&s);
-		if (s.commitable)
+		if(read_cache() < 0)
+			die("could not read the index");
+		if (index_is_dirty())
 			die ("Dirty index: cannot %s", me);
 		discard_cache();
 	}
diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
index 2dbe04f..6da2128 100755
--- a/t/t3501-revert-cherry-pick.sh
+++ b/t/t3501-revert-cherry-pick.sh
@@ -59,4 +59,13 @@ test_expect_success 'revert after renaming branch' '
 
 '
 
+test_expect_success 'revert forbidden on dirty working tree' '
+
+	echo content >extra_file &&
+	git add extra_file &&
+	test_must_fail git revert HEAD 2>errors &&
+	grep "Dirty index" errors
+
+'
+
 test_done
-- 
1.5.4.3.366.gb96b1.dirty

Re: [PATCH] revert: actually check for a dirty index

[Junio C Hamano]

Looks good.  I think we want to have the fix in 1.5.4 maintenance series
as well, so I'll backport it to 'maint' and merge it up to 'master'.