threads / bug / 12419

bug in git-revert

Subject: bug in git-revert

## tl;dr

7 messages between Mar 2, 2008 and Mar 3, 2008.

replies: 6people: 4as markdown or json

Jeff King· Mar 2, 2008, 06:44 UTC · lore

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 */
Johannes Schindelin· Mar 2, 2008, 07:00 UTC · re: Jeff King · lore

Re: bug in git-revert

Hi,
On Sun, 2 Mar 2008, Jeff King wrote:
Show 19 quoted lines
> 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

Jeff King· Mar 2, 2008, 07:02 UTC · re: Jeff King · lore

Re: bug in git-revert

On Sun, Mar 02, 2008 at 01:44:49AM -0500, Jeff King wrote:
Show 11 quoted lines
> --- 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
Jeff King· Mar 2, 2008, 07:22 UTC · re: Jeff King · lore

[PATCH] 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.

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
Alex Riesen· Mar 2, 2008, 10:37 UTC · re: Jeff King · lore

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

Jeff King, Sun, Mar 02, 2008 08:22:52 +0100:
Show 14 quoted lines
> +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.

Jeff King· Mar 3, 2008, 06:30 UTC · re: Alex Riesen · lore

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

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
Junio C Hamano· Mar 3, 2008, 07:11 UTC · re: Jeff King · lore

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

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'.

← back to recent threads