Re: [PATCH,RFC] Implement 'git rm --if-missing'

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 16, 2008, 18:48 UTC
Message-ID: <7vtzepr7g5.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <1216231250-21141-1-git-send-email-ciaran.mccreesh@googlemail.com>

Ciaran McCreesh <ciaran.mccreesh@googlemail.com> writes:

> git rm --if-missing will only remove files if they've already been removed from
> disk.

This probably is a borderline with feaping creaturism. What's the use of it in a real workflow that you need this for?

"git add -u" may be too broad in that it also adds anything modified, but so is --if-missing too broad in that it removes anything removed, and if you are going to limit by giving pathspecs _anyway_, then...

Old timers might just do:

	git diff --name-only --diff-filter=D |
        git update-index --remove --stdin

;-)

Show 10 quoted lines

> diff --git a/builtin-rm.c b/builtin-rm.c
> index 22c9bd1..4b89705 100644
> --- a/builtin-rm.c
> +++ b/builtin-rm.c
> @@ -125,7 +125,7 @@ static int check_local_mod(unsigned char *head, int index_only)
>  static struct lock_file lock_file;
>  
>  static int show_only = 0, force = 0, index_only = 0, recursive = 0, quiet = 0;
> -static int ignore_unmatch = 0;
> +static int ignore_unmatch = 0, if_missing = 0;

Not your fault in entirety, but we should drop these " = 0" initializations for static variables in a clean-up patch.

Show 7 quoted lines

>  static struct option builtin_rm_options[] = {
>  	OPT__DRY_RUN(&show_only),
> @@ -135,6 +135,7 @@ static struct option builtin_rm_options[] = {
>  	OPT_BOOLEAN('r', NULL,             &recursive,  "allow recursive removal"),
>  	OPT_BOOLEAN( 0 , "ignore-unmatch", &ignore_unmatch,
>  				"exit with a zero status even if nothing matched"),
> +	OPT_BOOLEAN( 0 , "if-missing",     &if_missing, "only remove missing files"),

Perhaps the command should error out if some of the named files still exist in the working tree?

Show 10 quoted lines

> @@ -168,6 +169,12 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
>  		struct cache_entry *ce = active_cache[i];
>  		if (!match_pathspec(pathspec, ce->name, ce_namelen(ce), 0, seen))
>  			continue;
> +		if (if_missing)
> +		{
> +			struct stat st;
> +			if ((lstat(ce->name, &st) == 0) || (errno != ENOENT))
> +				continue;
> +		}

 (1) (Style).  Opening brace comes on the same line as "if ()".

 (2) (Design). How should this new option interact with --cached mode of
     operation?

 (3) (Design). Shouldn't "git rm --if-missing" without any pathspec remove
     all missing paths from the index?

 (4) If lstat fails due to I/O error or something, you do not continue and
     add that path you did not get ENOENT for to the kill-list.  Is that
     desirable?

 (5) I wonder if lstat() is enough here.

     Consider:

	- current commit has "kernel" symlink to "linux-2.6/" directory but
          you want to remove kernel and move directory linux-2.6 to it, so:

          - you run "rm kernel; mv linux-2.6 kernel"

	  - then you run "git rm --if-missing -- kernel"

     What should the command do?