[PATCH] Add an "-i" option to git-reset, to confirm a reset.

[PATCH] Add an "-i" option to git-reset, to confirm a reset.

[Kelvie Wong]

It shows a diffstat, and asks the user if they would like to continue, or
show a full diff of the things getting reset.

I know that many times, I do a reset --hard thinking I had commited a file
already, but it turns out that I hadn't; and so this makes sure I don't
lose any work when the caffeine wears off.

Maybe it should also be made that only hard resets take this option, as
I cannot see this being useful in other places.

Signed-off-by: Kelvie Wong <kelvie@ieee.org>
---
 Documentation/git-reset.txt |    4 +++
 builtin-reset.c             |   46 ++++++++++++++++++++++++++++++++++++++----
 2 files changed, 45 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index 050e4ea..0323d9d 100644
--- a/Documentation/git-reset.txt
+++ b/Documentation/git-reset.txt
@@ -48,6 +48,10 @@ OPTIONS
 -q::
 	Be quiet, only report errors.
 
+-i::
+	Show what is about to be reset, and ask for confirmation before doing
+	so.
+
 <commit>::
 	Commit to make the current HEAD.
 
diff --git a/builtin-reset.c b/builtin-reset.c
index 713c2d5..1086817 100644
--- a/builtin-reset.c
+++ b/builtin-reset.c
@@ -18,7 +18,8 @@
 #include "tree.h"
 
 static const char builtin_reset_usage[] =
-"git-reset [--mixed | --soft | --hard] [-q] [<commit-ish>] [ [--] <paths>...]";
+"git-reset [--mixed | --soft | --hard] [-q] [-i] [<commit-ish>] [ [--] "
+"<paths>...]";
 
 static char *args_to_str(const char **argv)
 {
@@ -56,17 +57,47 @@ static int unmerged_files(void)
 	return 0;
 }
 
-static int reset_index_file(const unsigned char *sha1, int is_hard_reset)
+static int reset_index_file(const unsigned char *sha1, int is_hard_reset,
+			    int confirm_reset)
 {
 	int i = 0;
 	const char *args[6];
+	struct strbuf buf;
+	char result = 0;
+	const char *ref = sha1_to_hex(sha1);
+	const char *diffstat_args[] = { "diff", "--stat", ref, NULL };
+	const char *diff_args[] = { "diff", ref, NULL };
 
 	args[i++] = "read-tree";
 	args[i++] = "-v";
 	args[i++] = "--reset";
+
+	/* Show the user what is about to be reset, and in more detail, if they
+	 * like. */
+	if(confirm_reset) {
+		printf("The following files will be reset:\n");
+		run_command_v_opt(diffstat_args, RUN_GIT_CMD);
+		strbuf_init(&buf, 0);
+		while(result != 'y') {
+			printf("Continue? ((y)es/(n)o/view (d)iff)\n");
+			strbuf_getline(&buf, stdin, '\n');
+			result = tolower(buf.buf[0]);
+			switch(result) {
+			case 'd':
+				run_command_v_opt(diff_args, RUN_GIT_CMD);
+				break;
+			case 'n':
+				return 1;
+				break;
+			};
+		}
+		strbuf_release(&buf);
+        }
+
+
 	if (is_hard_reset)
 		args[i++] = "-u";
-	args[i++] = sha1_to_hex(sha1);
+	args[i++] = ref;
 	args[i] = NULL;
 
 	return run_command_v_opt(args, RUN_GIT_CMD);
@@ -181,7 +212,8 @@ static const char *reset_type_names[] = { "mixed", "soft", "hard", NULL };
 
 int cmd_reset(int argc, const char **argv, const char *prefix)
 {
-	int i = 1, reset_type = NONE, update_ref_status = 0, quiet = 0;
+	int i = 1, reset_type = NONE, update_ref_status = 0, quiet = 0,
+		confirm = 0;
 	const char *rev = "HEAD";
 	unsigned char sha1[20], *orig = NULL, sha1_orig[20],
 				*old_orig = NULL, sha1_old_orig[20];
@@ -210,6 +242,10 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 			quiet = 1;
 			i++;
 		}
+		else if (!strcmp(argv[i], "-i")) {
+			confirm = 1;
+			i++;
+		}
 		else
 			break;
 	}
@@ -251,7 +287,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 		if (is_merge() || unmerged_files())
 			die("Cannot do a soft reset in the middle of a merge.");
 	}
-	else if (reset_index_file(sha1, (reset_type == HARD)))
+	else if (reset_index_file(sha1, (reset_type == HARD), confirm))
 		die("Could not reset index file to revision '%s'.", rev);
 
 	/* Any resets update HEAD to the head being switched to,
-- 
1.5.4-rc0.GIT

Re: [PATCH] Add an "-i" option to git-reset, to confirm a reset.

[Johannes Schindelin]

Hi,

On Sat, 15 Dec 2007, Kelvie Wong wrote:

> It shows a diffstat, and asks the user if they would like to continue, 
> or show a full diff of the things getting reset.
> 
> I know that many times, I do a reset --hard thinking I had commited a 
> file already, but it turns out that I hadn't; and so this makes sure I 
> don't lose any work when the caffeine wears off.
> 
> Maybe it should also be made that only hard resets take this option, as 
> I cannot see this being useful in other places.

I am slightly negative on this patch.  Not only do I think that it is both 
easier and more natural to run diff/status/an-alias to see what a reset 
would do, but the patch only handles the index_file part (missing the -- 
<file> part AFAICT).

Besides, the code style is incompatible with the surrounding code.

Ciao,
Dscho

Re: [PATCH] Add an "-i" option to git-reset, to confirm a reset.

[Kelvie Wong]

On Dec 15, 2007 7:35 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Sat, 15 Dec 2007, Kelvie Wong wrote:
>
> > It shows a diffstat, and asks the user if they would like to continue,
> > or show a full diff of the things getting reset.
> >
> > I know that many times, I do a reset --hard thinking I had commited a
> > file already, but it turns out that I hadn't; and so this makes sure I
> > don't lose any work when the caffeine wears off.
> >
> > Maybe it should also be made that only hard resets take this option, as
> > I cannot see this being useful in other places.
>
> I am slightly negative on this patch.  Not only do I think that it is both
> easier and more natural to run diff/status/an-alias to see what a reset
> would do, but the patch only handles the index_file part (missing the --
> <file> part AFAICT).
>
> Besides, the code style is incompatible with the surrounding code.
>
> Ciao,
> Dscho
>
>
[forgot to hit Reply To All again, sorry!]

Ah, you're completely right about the index_file part (this is
actually the first time I've looked at the git-code :P)

Hrm.. I should have just used a shell script wrapper instead it seems.

I do think something like this would be nice though.

w.r.t. the style, you were referring to just the array initializers
right?  Or was there something else I did that doesn't look right?
-- 
Kelvie Wong

Re: [PATCH] Add an "-i" option to git-reset, to confirm a reset.

[Junio C Hamano]

"Kelvie Wong" <kelvie@ieee.org> writes:

> On Dec 15, 2007 7:35 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> ..
>> I am slightly negative on this patch.  Not only do I think that it is both
>> easier and more natural to run diff/status/an-alias to see what a reset
>> would do, but the patch only handles the index_file part (missing the --
>> <file> part AFAICT).

I am in principle very negative on additional option that does the same
thing as what the users on odd occasions can run a separate command
themselves to achieve, and I think "reset -i" falls into that category.

And I am negative on this "-i" not just because I think that would be
only in "odd occasions" (i.e. rare), but because I think it would not
help much.  Either you are sure about resetting, in which case you would
not even use "-i" option (and not get this safety), or you are unsure,
in which case you can do "git status" or whatever commands that are
already available.

> w.r.t. the style, you were referring to just the array initializers
> right?  Or was there something else I did that doesn't look right?

I spotted only two classes.

+
+	/* Show the user what is about to be reset, and in more detail, if they
+	 * like. */

	/*
         * Show the user what is about to be reset, and in more detail,
         * if they like.
         */

+	if(confirm_reset) {

	if (confirm_reset) {

Re: [PATCH] Add an "-i" option to git-reset, to confirm a reset.

[Kelvie Wong]

On Dec 16, 2007 12:09 PM, Junio C Hamano <gitster@pobox.com> wrote:
> "Kelvie Wong" <kelvie@ieee.org> writes:
>
> > On Dec 15, 2007 7:35 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > ..
> >> I am slightly negative on this patch.  Not only do I think that it is both
> >> easier and more natural to run diff/status/an-alias to see what a reset
> >> would do, but the patch only handles the index_file part (missing the --
> >> <file> part AFAICT).
>
> I am in principle very negative on additional option that does the same
> thing as what the users on odd occasions can run a separate command
> themselves to achieve, and I think "reset -i" falls into that category.
>
> And I am negative on this "-i" not just because I think that would be
> only in "odd occasions" (i.e. rare), but because I think it would not
> help much.  Either you are sure about resetting, in which case you would
> not even use "-i" option (and not get this safety), or you are unsure,
> in which case you can do "git status" or whatever commands that are
> already available.
>
> > w.r.t. the style, you were referring to just the array initializers
> > right?  Or was there something else I did that doesn't look right?
>
> I spotted only two classes.
>
> +
> +       /* Show the user what is about to be reset, and in more detail, if they
> +        * like. */
>
>         /*
>          * Show the user what is about to be reset, and in more detail,
>          * if they like.
>          */
>
> +       if(confirm_reset) {
>
>         if (confirm_reset) {
>

I guess this is just for people like me who are used to the "M-x
vc-revert" function in emacs, which shows you exactly what changes get
reverted before doing so.

But I guess it is quite trivial to make an alias to do the same when
invoking git-reset (or checkout) directly.

-- 
Kelvie Wong