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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 27, 2021, 19:49 UTC
Message-ID: <xmqqpmphx1yh.fsf@gitster.g>
In-Reply-To: <pull.1171.git.git.1640492910432.gitgitgadget@gmail.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.

Show 9 quoted lines

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

Show 10 quoted lines

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

Show 7 quoted lines

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

Show 5 quoted lines

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

Show 20 quoted lines

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