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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

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.

> @@ -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));
>  		}
>  	}

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