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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 6, 2019, 04:40 UTC
Message-ID: <xmqqd0e5vcq9.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <8d9f1fbc18346144a0c866a59891b652dcfe9b7f.1572895605.git.gitgitgadget@gmail.com>

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

Show 8 quoted lines

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

Show 5 quoted lines

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

Show 6 quoted lines

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

Show 13 quoted lines

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

Show 28 quoted lines

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

Show 28 quoted lines

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