Re: [PATCH 2/8] rm: support the --pathspec-from-file option

From: Junio C Hamano <gitster@pobox.com>
To: "Alexandr Miloslavskiy via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org,
	Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com>,
	Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
Subject: Re: [PATCH 2/8] rm: support the --pathspec-from-file option
Date: Tue, 21 Jan 2020 11:36:26 -0800	[thread overview]
Message-ID: <xmqqftg8a9fp.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <5611e3ae326bb7f61abf870e3b2851226b6af1d8.1579190965.git.gitgitgadget@gmail.com> (Alexandr Miloslavskiy via GitGitGadget's message of "Thu, 16 Jan 2020 16:09:19 +0000")

"Alexandr Miloslavskiy via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
>
> Decisions taken for simplicity:
> 1) It is not allowed to pass pathspec in both args and file.
>
> `if (!argc)` block was adapted to work with --pathspec-from-file. For
> that, I also had to parse pathspec earlier. Now it happens before
> `read_cache()` / `hold_locked_index()` / `setup_work_tree()`, which
> sounds fine to me.

That is not an explanation nor justification.

> In case of empty pathspec, there is now a clear error message instead
> of showing usage.

Hmph, "git rm --pathspec-from-file=/dev/null" would say "nothing
specified, nothing removed" and it makes perfect sense, but I am not
sure "git rm" that gives the same message is better than the output
by usage_with_options(builtin_rm_usage, builtin_rm_options).

> -'git rm' [-f | --force] [-n] [-r] [--cached] [--ignore-unmatch] [--quiet] [--] <pathspec>...
> +'git rm' [-f | --force] [-n] [-r] [--cached] [--ignore-unmatch]
> +	  [--quiet] [--pathspec-from-file=<file> [--pathspec-file-nul]]
> +	  [--] [<pathspec>...]

OK.

> +--pathspec-from-file=<file>::
> +	Pathspec is passed in `<file>` instead of commandline args. If
> +	`<file>` is exactly `-` then standard input is used. Pathspec
> +	elements are separated by LF or CR/LF. Pathspec elements can be
> +	quoted as explained for the configuration variable `core.quotePath`
> +	(see linkgit:git-config[1]). See also `--pathspec-file-nul` and
> +	global `--literal-pathspecs`.
> +
> +--pathspec-file-nul::
> +	Only meaningful with `--pathspec-from-file`. Pathspec elements are
> +	separated with NUL character and all other characters are taken
> +	literally (including newlines and quotes).
> +

OK.

> diff --git a/builtin/rm.c b/builtin/rm.c
> index 19ce95a901..8e40795751 100644
> --- a/builtin/rm.c
> +++ b/builtin/rm.c
> @@ -235,7 +235,8 @@ static int check_local_mod(struct object_id *head, int index_only)
>  }
>  
>  static int show_only = 0, force = 0, index_only = 0, recursive = 0, quiet = 0;
> -static int ignore_unmatch = 0;
> +static int ignore_unmatch = 0, pathspec_file_nul = 0;
> +static char *pathspec_from_file = NULL;

We may want to clean these "explicitly initialize to 0/NULL" up at
some point.  The clean-up itself would not be in the scope of this
patch, of course, but not making it worse is something this patch
can do to help.

> @@ -259,8 +262,24 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
>  
>  	argc = parse_options(argc, argv, prefix, builtin_rm_options,
>  			     builtin_rm_usage, 0);
> -	if (!argc)
> -		usage_with_options(builtin_rm_usage, builtin_rm_options);
> +
> +	parse_pathspec(&pathspec, 0,
> +		       PATHSPEC_PREFER_CWD,
> +		       prefix, argv);
> +
> +	if (pathspec_from_file) {
> +		if (pathspec.nr)
> +			die(_("--pathspec-from-file is incompatible with pathspec arguments"));
> +
> +		parse_pathspec_file(&pathspec, 0,
> +				    PATHSPEC_PREFER_CWD,
> +				    prefix, pathspec_from_file, pathspec_file_nul);
> +	} else if (pathspec_file_nul) {
> +		die(_("--pathspec-file-nul requires --pathspec-from-file"));
> +	}
> +
> +	if (!pathspec.nr)
> +		die(_("Nothing specified, nothing removed"));

I wonder if doing these in this order instead would make more sense
without making unnecessary behaviour change.

    - parse the options (which would make pathspec_f_f available to
      us)

    - if pathspec_f_f is given, call parse_pathspec_file()

    - otherwise complain if pathspec_file_nul is set

    - otherwise check argc and give the usage_with_options()

I dunno.

Thanks.