Re: [PATCH] name-rev: deprecate --stdin in favor of --annotate-text

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

"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:

As name-rev is Dscho's brainchild, I think it is benefitial to ask input
from him, so I added an address to the CC: list.

> From: John Cai <johncai86@gmail.com>
>
> Introduce a --annotate-text that is functionally equivalent of --stdin.
> --stdin does not behave as --stdin in other subcommands, such as
> pack-objects whereby it takes one argument per line. Since --stdin can
> be a confusing and misleading name, rename it to --annotate-text.
>
> This change adds a warning to --stdin warning that it will be removed in
> the future.

I know I've suggested the name, but 'text' in --annotate-text is a
low value word in an option name where every byte is precious.
"Annotate" is very good to convey what is done to the object of the
verb, but "text" stresses the wrong thing (we do not annotate
binary,o we annotate text) without saying where the text comes from
(i.e.  standard input).  Perhaps "--annotate-stdin" would be a much
better name.

I agree that letting "--stdin" to deviate so much from an emulation
of "xargs git name-rev" was indeed a mistake that caused the
confusion that led to the other thread.  If the mode had a better
name from the day 1, the thread would have been avoided.

What I am not sure about is how much this transition would hurt
existing users and scripts.

> +	For example:
> ++
> +----------
> +$ cat sample.txt
> +
> +An abbreviated revision 2ae0a9cb82 will not be substituted.
> +The full name after substitution is 2ae0a9cb8298185a94e5998086f380a355dd8907,
> +while its tree object is 70d105cc79e63b81cfdcb08a15297c23e60b07ad
> +
> +$ git name-rev --annotate-text < sample.txt

Lose SP between the redirection operator '<' and its file 'sample.txt'.

> +
> +An abbreviated revision 2ae0a9cb82 will not be substituted.
> +The full name after substitution is 2ae0a9cb8298185a94e5998086f380a355dd8907
> +(master),
> +while its tree object is 70d105cc79e63b81cfdcb08a15297c23e60b07ad
> +
> +$ git name-rev --name-only --annotate-text < sample.txt

Ditto.

> +	if (transform_stdin) {
> +		warning("--stdin is deprecated. Please use --annotate-text instead, "
> +					"which is functionally equivalent.\n"
> +					"This option will be removed in a future release.");
> +		annotate_text = 1;

I guess that is sensible.  To squelch the warning, they can switch
to the new option.

> +	}
> +
> +	if (all + annotate_text + !!argc > 1) {
>  		error("Specify either a list, or --all, not both!");
>  		usage_with_options(name_rev_usage, opts);
>  	}
> -	if (all || transform_stdin)
> +	if (all || annotate_text)
>  		cutoff = 0;
>  
>  	for (; argc; argc--, argv++) {
> @@ -613,7 +622,7 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix)
>  	for_each_ref(name_ref, &data);
>  	name_tips();
>  
> -	if (transform_stdin) {
> +	if (annotate_text) {
>  		char buffer[2048];
>  
>  		while (!feof(stdin)) {

Not a suggestion to change anything in this patch, but just an
observation.

 - If the mode is useful enough for many users, certainly somebody
   would have rewritten this loop to lift the line-length limit, but
   nobody noticed and complained about this 2k limit for the past 17
   years.  I am not sure if that is an indication that nobody uses
   the option in its current form.

 - A low hanging fruit, if we do not go full strbuf_getline(), at
   least we should narrow the scope of buffer[] array.