Re: [PATCH 2/4] add smudge-to-file and clean-from-file filter configuration

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 16, 2016, 21:57 UTC
Message-ID: <xmqqy4643ig1.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160616203259.5886-3-joeyh@joeyh.name>

Joey Hess <joeyh@joeyh.name> writes:

Show 23 quoted lines

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 2e1b2e4..bbb9296 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1299,14 +1299,29 @@ format.useAutoBase::
>  	format-patch by default.
>  
>  filter.<driver>.clean::
> -	The command which is used to convert the content of a worktree
> -	file to a blob upon checkin.  See linkgit:gitattributes[5] for
> -	details.
> +	The command which is used on checkin to convert the content of
> +	a worktree file (provided on stdin) to a blob (written to stdout).
> +	See linkgit:gitattributes[5] for details.
>  
>  filter.<driver>.smudge::
> -	The command which is used to convert the content of a blob
> -	object to a worktree file upon checkout.  See
> -	linkgit:gitattributes[5] for details.
> +	The command which is used on checkout to convert the content of a
> +	blob object (provided on stdin) to a worktree file (written to
> +	stdout).
> +	See linkgit:gitattributes[5] for details.

A "filter" by definition reads from its standard input and writes the result to its standard output, so I do not know if this change is necessary. If you are going to do this, in documentation (i.e. not code), please spell standard input/output out, and avoid stdin/stdout.

There is what we would want to fix, though. "worktree file" should be spelled "working tree file". This used not to matter before "git worktree" was invented (before that we used these two terms interchangeably), but these days the distinction matters.

> +filter.<driver>.clean-from-file::

Documentation/config.txt hopefully lists all the configuration, but I do not see anything that uses 'words-joined-with-dash' format. Please do not invent new out-of-convention names.

> +	Optional command which is used on checkin to convert the content
> +	of a worktree file, which can be read from disk, to a blob
> +	(written to stdout).

I am not sure why we want to say "which can be read from disk".

I think what a reader needs to be told (but this paragraph is not telling her) in order to understand what you meant to say is:

	cleanFromFile is asked to produce the "cleaned" content to
	its standard output (to be stored in the object database),
	but unlike clean, it does not work as a filter and is not
	given a file descriptor to read the working tree contents
	from.  Instead, it is told the path for which the contents
	need to be generated as the first parameter on its command
	line.

(the above is deliberately made verbose and is not meant as a suggestion to be literally used in your updated documentation).

With that understanding, the reader may be able to guess that "can be read from disk" is a permission for her cleanFromFile filter (i.e. it does not necessarily have to read from it and produced its output by some other magic); otherwise it can be misread as if the content of a working tree file is deposited on the disk to enable the filter to read it, but the location of that on-disk file is somewhere unspecified by this paragraph.

Show 7 quoted lines

> +	Only used when filter.<driver>.clean is also configured.
> +	See linkgit:gitattributes[5] for details.
> +
> +filter.<driver>.smudge-to-file::
> +	Optional command which is used to convert the content of a blob
> +	object (provided on stdin) to a worktree file, writing directly
> +	to the file.

A similar comment applies. With ", writing directly to the file." replaced with something like ". The command is expected to write to the working tree file at path given as the first parameter on the command line." it would be sufficient (i.e. it is clear enough that it does not give the smudged contents via its standard output, and no need to say "unlike smudge filter, ..." like we needed to for the "clean" side).