Re: [PATCH 3/5] reset: support the --pathspec-from-file option

[Junio C Hamano <gitster@pobox.com>]

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

> From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
>
> This option solves the problem of commandline length limit for UI's
> built on top of git. Plumbing commands are not always a good fit, for
> two major reasons:
> 1) Some UI's serve as assistants that help user run git commands. In
>    this case, replacing familiar commands with plumbing commands will
>    confuse most users.

"UI's that serve as assistants that help user run git commands" does
not have to avoid plumbing commands at all.  Only the ones that "show"
the commands that are run on behalf of the users (perhaps so that the
users can learn from such examples?) do, and I think I learned that
it was your motivating use case from an earlier discussion.  Perhaps

	UIs that help users to formulate git commands to run need to
	present Porcelain commands to be used, as it is not reasonable
	to demonstrate arcane combination of plumbing commands as an
	example for their interactive use.

would probably be more readable without bending what you wanted to
say too much?

> 2) Some UI's have started and grown with porcelain commands. Replacing
>    existing logic with plumbing commands could be cumbersome and prone
>    to various new problems.

There is not a lot of sympathy for such argument ;-)

> 2) It is not allowed to pass some refspecs in args and others in file.

Did you mean refspec, not pathspec?

> 3) New options do not have shorthands to avoid shorthand conflicts.
>
> Also add new '--pathspec-file-null' switch that mirrors '-z' used in
> various places. Some porcelain commands, such as `git commit`, already
> use '-z', therefore it needed a new unambiguous name.

I do not understand this part.  Wouldn't it natural to expect that
"-z", when used with "--pathspec-from-file", tell us that the named
file is NUL delimited collection of records?  What's different about
"--pathspec-file-null" that it is confusing to all it "-z"?

I actually do not mind not having "-z" and using only
"--pathspec-file-null".  A new option with long name that feels
similar to existing "-z" but does sufficiently different things so
that it needs a different name, however, feels counter-productive
for the purpose of using it in the UI that produces commands to be
learned by end-users.

> +--pathspec-from-file=<file>::
> +	Read `<pathspec>` from `<file>` instead. If `<file>` is exactly `-`
> +	then read from standard input. Pathspecs are separated by LF or
> +	CR/LF. Pathspecs can be quoted as explained for the configuration

When I invented the terms "pathspec", "refspec", etc. for this
project, I meant them to be collective nouns.  A pathspec is a set
of pathspec elements, each of which is usually given as a separate
argument on the command line.  So "Read pathspec from file" is good
(not "Read pathspecs from file").

And "Pathspec elements" are separated by LF or CR/LF.

A tangent.  Since we do not allow NUL in a pathspec element, we do
not even need the "-z" option.  When we read pathspec from a file,
we can take any of CRLF, LF or NUL as the separator.

Ah, sorry, that would not help very much.  With "-z" we are allowing
to express pathspec elements inside which there are embedded LF or
CR/LF.  Sorry about the noise.

> +--pathspec-file-null::
> +	Only meaningful with `--pathspec-from-file`. Pathspecs are
> +	separated with NUL character and are not expected to be quoted.

OK.

> +	if (pathspec_from_file) {
> +		if (patch_mode)
> +			die(_("--pathspec-from-file is incompatible with --patch"));
> +
> +		if (pathspec.nr)
> +			die(_("--pathspec-from-file is incompatible with path arguments"));

Shouldn't the error message say pathspec arguments instead?

> diff --git a/t/t7107-reset-pathspec-file.sh b/t/t7107-reset-pathspec-file.sh
> new file mode 100755
> index 0000000000..cf7f085ad5
> --- /dev/null
> +++ b/t/t7107-reset-pathspec-file.sh
> @@ -0,0 +1,126 @@
> +#!/bin/sh
> +
> +test_description='reset --pathspec-from-file'
> +
> +. ./test-lib.sh
> +
> +cat > expect.a <<EOF

Style:

 - Modern test scripts strive to perform these set-up procedure
   inside (the first) test_expect_success.

 - No SP between the redirection operator and its source/destination
   filename.

 - Quote end-of-here-document (EOF in the above) if you do not rely
   on parameter interpolation inside the here document; this helps
   readers by telling them that what is presented is used verbatim.

> + D fileA.t
> +EOF
> +
> +cat > expect.ab <<EOF
> + D fileA.t
> + D fileB.t
> +EOF
> +
> +cat > expect.a_bc_d <<EOF
> +D  fileA.t
> + D fileB.t
> + D fileC.t
> +D  fileD.t
> +EOF
> +
> +test_expect_success setup '
> +	echo A >fileA.t &&
> +	echo B >fileB.t &&
> +	echo C >fileC.t &&
> +	echo D >fileD.t &&
> +	git add . &&
> +	git commit --include . -m "Commit" &&
> +	checkpoint=$(git rev-parse --verify HEAD)
> +'
> +
> +restore_checkpoint () {
> +	git reset --hard "$checkpoint"
> +}

Hmm, wouldn't it be cleaner to use a lightweight tag or something to
keep checkpoint, instead of a variable that is hard to examine when
tests break and needs debugging?

> +verify_state () {
> +	git status --porcelain -- fileA.t fileB.t fileC.t fileD.t >actual &&
> +	test_cmp "$1" actual
> +}
> +
> +test_expect_success '--pathspec-from-file from stdin' '
> +	restore_checkpoint &&
> +
> +	git rm fileA.t &&
> +	echo fileA.t | git reset --pathspec-from-file=- &&
> +	verify_state expect.a
> +'
> +
> +test_expect_success '--pathspec-from-file from file' '
> +	restore_checkpoint &&
> +
> +	git rm fileA.t &&
> +	echo fileA.t >list &&
> +	git reset --pathspec-from-file=list &&
> +
> +	verify_state expect.a
> +'
> +
> +test_expect_success 'NUL delimiters' '
> +	restore_checkpoint &&
> +
> +	git rm fileA.t fileB.t &&
> +	printf fileA.tQfileB.t | q_to_nul | git reset --pathspec-from-file=- --pathspec-file-null &&

This feeds "fileA.t<NUL>fileB.t" without <NUL> after "fileB.t" to
the command.  Intended?

Rather, perhaps

	printf "%s\0" fileA.t fileB.t

without q-to-nul, once you use printf anyway?

If you truly mean "delimiter" (as opposed to "terminator"),

	printf "fileA.t\0fileB.t"

can also lose " | q_to_nul".

Thanks.