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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jul 8, 2007, 18:10 UTC
Message-ID: <Pine.LNX.4.64.0707081855300.4248@racer.site>
In-Reply-To: <vpqfy3yajbj.fsf_-_@bauges.imag.fr>

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.

Show 16 quoted lines

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

Show 12 quoted lines

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

Show 10 quoted lines

> +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 "{ .. }".

Show 9 quoted lines

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

Show 16 quoted lines

> @@ -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