git reset -- path weirdness

git reset -- path weirdness

[Björn Steinbrink]

Hi,

I noticed some weirdness with git reset when a path is given. Basically
it seems to cycle the file through 3 states: unstaged, unmerged,
deleted(!) which is IMHO weird at best. A bisection showed that the
behaviour was introduced with the shell -> conversion of git-reset.

Extra annoyance is caused by the fact that "git status" tells you to add
the file when it is in the unmerged state, but that doesn't do anything.

Log of "git reset" switching through the states:

doener@atjola:git (master) $ git status
# On branch master
nothing to commit (working directory clean)

doener@atjola:git (master) $ git reset -- builtin-reset.c
builtin-reset.c: needs merge

doener@atjola:git (master) $ git status
# On branch master
# Changed but not updated:
#   (use "git add <file>..." to update what will be committed)
#
#       unmerged:   builtin-reset.c
#
no changes added to commit (use "git add" and/or "git commit -a")

doener@atjola:git (master) $ git reset -- builtin-reset.c

doener@atjola:git (master) $ git status
# On branch master
# Changes to be committed:
#   (use "git reset HEAD <file>..." to unstage)
#
#       deleted:    builtin-reset.c
#
# Untracked files:
#   (use "git add <file>..." to include in what will be committed)
#
#       builtin-reset.c

doener@atjola:git (master) $ git reset -- builtin-reset.c

doener@atjola:git (master) $ git status
# On branch master
nothing to commit (working directory clean)


Thanks,
Björn

Re: git reset -- path weirdness

[Johannes Schindelin]

Hi,

On Sat, 3 Nov 2007, Bj?rn Steinbrink wrote:

> I noticed some weirdness with git reset when a path is given. Basically 
> it seems to cycle the file through 3 states: unstaged, unmerged, 
> deleted(!) which is IMHO weird at best. A bisection showed that the 
> behaviour was introduced with the shell -> conversion of git-reset.

Indeed.  I'm on it.

Ciao,
Dscho

[PATCH] git-reset: do not be confused if there is nothing to reset

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3065 bytes --]


The purpose of the function update_index_from_diff() (which is the
callback function we give do_diff_cache()) is to update those index
entries which differ from the given commit.

Since do_diff_cache() plays games with the in-memory index, this function
discarded the cache and reread it.

Then, back in the function read_from_tree() we wrote the index.

Of course, this broke down when there were no changes and
update_index_from_diff() was not called, and therefore the mangled index
was not discarded.

The solution is to move the index writing into the function
update_index_from_diff().

Noticed by Björn Steinbrink.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin-reset.c  |   24 ++++++++++++++++++------
 t/t7102-reset.sh |    7 +++++++
 2 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/builtin-reset.c b/builtin-reset.c
index e1dc31e..5467e36 100644
--- a/builtin-reset.c
+++ b/builtin-reset.c
@@ -113,10 +113,17 @@ static int update_index_refresh(void)
 	return run_command_v_opt(argv_update_index, RUN_GIT_CMD);
 }
 
+struct update_cb_data {
+	int index_fd;
+	struct lock_file *lock;
+	int exit_code;
+};
+
 static void update_index_from_diff(struct diff_queue_struct *q,
 		struct diff_options *opt, void *data)
 {
 	int i;
+	struct update_cb_data *cb = data;
 
 	/* do_diff_cache() mangled the index */
 	discard_cache();
@@ -133,29 +140,34 @@ static void update_index_from_diff(struct diff_queue_struct *q,
 		} else
 			remove_file_from_cache(one->path);
 	}
+
+	cb->exit_code = write_cache(cb->index_fd, active_cache, active_nr) ||
+		close(cb->index_fd) ||
+		commit_locked_index(cb->lock);
 }
 
 static int read_from_tree(const char *prefix, const char **argv,
 		unsigned char *tree_sha1)
 {
-        struct lock_file *lock = xcalloc(1, sizeof(struct lock_file));
-	int index_fd;
 	struct diff_options opt;
+	struct update_cb_data cb;
 
 	memset(&opt, 0, sizeof(opt));
 	diff_tree_setup_paths(get_pathspec(prefix, (const char **)argv), &opt);
 	opt.output_format = DIFF_FORMAT_CALLBACK;
 	opt.format_callback = update_index_from_diff;
+	opt.format_callback_data = &cb;
 
-	index_fd = hold_locked_index(lock, 1);
+	cb.lock = xcalloc(1, sizeof(struct lock_file));
+	cb.index_fd = hold_locked_index(cb.lock, 1);
+	cb.exit_code = 0;
 	read_cache();
 	if (do_diff_cache(tree_sha1, &opt))
 		return 1;
 	diffcore_std(&opt);
 	diff_flush(&opt);
-	return write_cache(index_fd, active_cache, active_nr) ||
-		close(index_fd) ||
-		commit_locked_index(lock);
+
+	return cb.exit_code;
 }
 
 static void prepend_reflog_action(const char *action, char *buf, size_t size)
diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
index f64b1cb..cea9afb 100755
--- a/t/t7102-reset.sh
+++ b/t/t7102-reset.sh
@@ -402,4 +402,11 @@ test_expect_success 'test resetting the index at give paths' '
 
 '
 
+test_expect_success 'resetting an unmodified path is a no-op' '
+	git reset --hard &&
+	git reset -- file1 &&
+	git diff-files --exit-code &&
+	git diff-index --cached --exit-code HEAD
+'
+
 test_done
-- 
1.5.3.5.1505.gd778c