[RFC][PATCH] Re: git-rm isn't the inverse action of git-add

Matthieu Moy [off-list ref] writes:

quoted

 - if (HEAD == index && index == version) unversion and unlink

Just to be more precise:

   - if (HEAD == index && index == version) unversion and
       * if (--cached is not given) unlink
       * else do nothing

quoted

 - else if (HEAD == index || index == version) unversion
 - else print message and do nothing

Would you consider that a sane behaviour?

[...]

I'll try writting patch for that if people agree that this is saner
that the current behavior.

Here's a first attempt (I'm still not familiar with the git codebase,
so the patch is probably not so good).

Note: currently, git-rm still shows those "rm '...'" messages on
stdout. AAUI, they were actually useful at a time when git-rm didn't
actually remove the files, and people actually ran the "rm" commands
after. They can probably be removed now, but that's another topic.

From f4f4aa047b2b9050d968704d1f2db07b2a1a79cc Mon Sep 17 00:00:00 2001

From: Matthieu Moy <redacted>
Date: Sun, 8 Jul 2007 19:27:44 +0200
Subject: [PATCH] Make git-rm obey in more circumstances.

In the previous behavior of git-rm, git refused to do anything in case of
a difference between the file on disk, the index, and the HEAD. As a
result, the -f flag is forced even for simple senarios like:

$ git add foo
# oops, I didn't want to version it
$ git rm -f [--cached] foo
# foo is deleted on disk if --cached isn't provided.

This patch proposes a saner behavior. When there are no difference at all
between file, index and HEAD, the file is removed both from the index and
the tree, as before.

Otherwise, if the index matches either the file on disk or the HEAD, the
file is removed from the index, but the file is kept on disk, it may
contain important data.

Otherwise, that's an error, and git-rm aborts.

The above senario becomes

$ git add foo
$ git rm foo
# back to the initial state.

Signed-off-by: Matthieu Moy <redacted>
---
 Documentation/git-rm.txt |    9 ++++---
 builtin-rm.c             |   55 ++++++++++++++++++++++++++++++++++++---------
 2 files changed, 49 insertions(+), 15 deletions(-)

diff --git a/Documentation/git-rm.txt b/Documentation/git-rm.txt
index 78f45dc..180671c 100644
--- a/Documentation/git-rm.txt
+++ b/Documentation/git-rm.txt

@@ -11,10 +11,11 @@ SYNOPSIS
 
 DESCRIPTION
 -----------
-Remove files from the working tree and from the index.  The
-files have to be identical to the tip of the branch, and no
-updates to its contents must have been placed in the staging
-area (aka index).
+Remove files from the working tree and from the index. The content
+placed in the staging area (aka index) must match either the content
+of the file on disk, or the tip of the branch. If it matches only one
+of them, the file is kept on disk for safety, but is still removed
+from the index.
 
 
 OPTIONS

diff --git a/builtin-rm.c b/builtin-rm.c
index 4a0bd93..d2a8998 100644
--- a/builtin-rm.c
+++ b/builtin-rm.c

@@ -12,18 +12,27 @@
 static const char builtin_rm_usage[] =
 "git-rm [-f] [-n] [-r] [--cached] [--ignore-unmatch] [--quiet] [--] <file>...";
 
+struct file_info {
+	const char *name;
+	int local_changes;
+	int staged_changes;
+};
+
 static struct {
 	int nr, alloc;
-	const char **name;
+	struct file_info * files;
 } list;
 
 static void add_list(const char *name)
 {
 	if (list.nr >= list.alloc) {
 		list.alloc = alloc_nr(list.alloc);
-		list.name = xrealloc(list.name, list.alloc * sizeof(const char *));
+		list.files = xrealloc(list.files, list.alloc * sizeof(const char *));
 	}
-	list.name[list.nr++] = name;
+	list.files[list.nr].name = name;
+	list.files[list.nr].local_changes  = 0;
+	list.files[list.nr].staged_changes = 0;
+	list.nr++;
 }
 
 static int remove_file(const char *name)

@@ -46,6 +55,26 @@ static int remove_file(const char *name)
 	return ret;
 }
 
+static int remove_file_maybe(const struct file_info fi, int quiet)
+{
+	const char *path = fi.name;
+	if (!fi.local_changes && !fi.staged_changes) {
+		/* The file matches either the index or the HEAD.
+		 * It's content exists somewhere else, it's safe to
+		 * delete it.
+		 */
+		return remove_file(path);
+	} else {
+		if (!quiet)
+			fprintf(stderr, 
+				"note: file '%s' not removed "
+				"(doesn't match %s).\n",
+				path,
+				fi.local_changes?"the index":"HEAD");
+		return 0;
+	}
+}
+
 static int check_local_mod(unsigned char *head)
 {
 	/* items in list are already sorted in the cache order,

@@ -62,7 +91,7 @@ static int check_local_mod(unsigned char *head)
 		struct stat st;
 		int pos;
 		struct cache_entry *ce;
-		const char *name = list.name[i];
+		const char *name = list.files[i].name;
 		unsigned char sha1[20];
 		unsigned mode;
 

@@ -87,13 +116,17 @@ static int check_local_mod(unsigned char *head)
 			continue;
 		}
 		if (ce_match_stat(ce, &st, 0))
-			errs = error("'%s' has local modifications "
-				     "(hint: try -f)", ce->name);
+			list.files[i].local_changes = 1;
+
 		if (no_head
 		     || get_tree_entry(head, name, sha1, &mode)
 		     || ce->ce_mode != create_ce_mode(mode)
 		     || hashcmp(ce->sha1, sha1))
-			errs = error("'%s' has changes staged in the index "
+			list.files[i].staged_changes = 1;
+
+		if (list.files[i].local_changes && 
+		    list.files[i].staged_changes)
+			errs = error("'%s' doesn't match neither HEAD nor the index "
 				     "(hint: try -f)", name);
 	}
 	return errs;

@@ -201,7 +234,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 	 * the index unless all of them succeed.
 	 */
 	for (i = 0; i < list.nr; i++) {
-		const char *path = list.name[i];
+		const char *path = list.files[i].name;
 		if (!quiet)
 			printf("rm '%s'\n", path);
 

@@ -224,13 +257,13 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 	if (!index_only) {
 		int removed = 0;
 		for (i = 0; i < list.nr; i++) {
-			const char *path = list.name[i];
-			if (!remove_file(path)) {
+			if (!remove_file_maybe(list.files[i], quiet)) {
 				removed = 1;
 				continue;
 			}
 			if (!removed)
-				die("git-rm: %s: %s", path, strerror(errno));
+				die("git-rm: %s: %s", 
+				    list.files[i].name, strerror(errno));
 		}
 	}
 

-- 
1.5.3.rc0.63.gc956-dirty



-- 
Matthieu

Hi,

On Sun, 8 Jul 2007, Matthieu Moy wrote:

Subject: [PATCH] Make git-rm obey in more circumstances.

This is not really a good patch title.  Since it only obeys your 
particular understanding of what it should do.  You are changing 
semantics, and you should say so.

In the previous behavior of git-rm, git refused to do anything in case 
of a difference between the file on disk, the index, and the HEAD. As a 
result, the -f flag is forced even for simple senarios like:

$ git add foo
# oops, I didn't want to version it
$ git rm -f [--cached] foo
# foo is deleted on disk if --cached isn't provided.

This patch proposes a saner behavior. When there are no difference at 
all between file, index and HEAD, the file is removed both from the 
index and the tree, as before.

Otherwise, if the index matches either the file on disk or the HEAD, the 
file is removed from the index, but the file is kept on disk, it may 
contain important data.

However, if some of the files are of the first kind, and some are of the 
second kind, you happily apply with mixed strategies.  IMO that is wrong.

 static struct {
 	int nr, alloc;
-	const char **name;
+	struct file_info * files;
 } list;
 
 static void add_list(const char *name)
 {
 	if (list.nr >= list.alloc) {
 		list.alloc = alloc_nr(list.alloc);
-		list.name = xrealloc(list.name, list.alloc * sizeof(const char *));
+		list.files = xrealloc(list.files, list.alloc * sizeof(const char *));

This is wrong, too.  Yes, it works.  But it really should be 
"sizeof(struct file_info *)".  Remember, code is also documentation.

+static int remove_file_maybe(const struct file_info fi, int quiet)
+{
+	const char *path = fi.name;
+	if (!fi.local_changes && !fi.staged_changes) {
+		/* The file matches either the index or the HEAD.
+		 * It's content exists somewhere else, it's safe to
+		 * delete it.
+		 */
+		return remove_file(path);
+	} else {

Superfluous "{ .. }".

+		if (!quiet)
+			fprintf(stderr, 
+				"note: file '%s' not removed "
+				"(doesn't match %s).\n",
+				path,
+				fi.local_changes?"the index":"HEAD");
+		return 0;
+	}
+}

I suspect that this case does never fail. 0 means success for 
remove_file().  Not good.  You should at least have a way to ensure that 
it removed the files from the working tree from a script.  Otherwise there 
is not much point in returning a value to begin with.

Style: the old code set and used "path" for readability.  You should do 
the same (with "file", probably).

Additionally, since this changes semantics, you better provide test cases 
to show what is expected to work, and _ensure_ that it actually works.

Ciao,
Dscho