name-rev: deprecate --stdin in favor of --annotate-text

Commit Message

John Cai Dec. 26, 2021, 4:28 a.m. UTC

From: John Cai <[e-mail]>

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.

Signed-off-by: "John Cai" <[e-mail]>
---
    name-rev: deprecate --stdin in favor of --anotate-text
    
    Introduce a --anontate-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, the goal is to rename it to
    --anotate-text.
    
    This is the first step in a process of eventually fully deprecating
    --stdin. This change also adds a warning to --stdin warning that it will
    be removed in the future.
    
    See https://lore.kernel.org/git/xmqqsfuh1pxz.fsf@gitster.g/ for
    discussion.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1171%2Fjohn-cai%2Fjc%2Fdeprecate-name-rev-stdin-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1171/john-cai/jc/deprecate-name-rev-stdin-v1
Pull-Request: https://github.com/git/git/pull/1171

 Documentation/git-name-rev.txt | 29 ++++++++++++++++++++++++++++-
 builtin/name-rev.c             | 17 +++++++++++++----
 2 files changed, 41 insertions(+), 5 deletions(-)


base-commit: 2ae0a9cb8298185a94e5998086f380a355dd8907

Comments

Junio C Hamano Dec. 27, 2021, 7:49 p.m. UTC | #1

"John Cai via GitGitGadget" <[e-mail]> 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 <[e-mail]>
>
> 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.

John Cai Dec. 28, 2021, 5:13 a.m. UTC | #2

> On Dec 27, 2021, at 11:49 AM, Junio C Hamano <[e-mail]> wrote:
> 
> "John Cai via GitGitGadget" <[e-mail]> 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 <[e-mail]>
>> 
>> 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.

Would this also mean that deprecating --stdin wouldn’t be **too** disruptive to existing users and scripts?

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

Junio C Hamano Dec. 31, 2021, 8:16 a.m. UTC | #3

John Cai <[e-mail]> writes:

>> 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.
>
> Would this also mean that deprecating --stdin wouldn’t be **too**
> disruptive to existing users and scripts?

That probably is a lot of wishful thinking.  Most likely, it is an
indication that most text input files that go into the command have
no overly long lines ;-)  Loss of or change in behaviour of the
command would be disruptive for the users who are content with the
limitation that they cannot feed lines wider than 2k.

Johannes Schindelin Jan. 1, 2022, 10:59 p.m. UTC | #4

Hi Junio,

On Mon, 27 Dec 2021, Junio C Hamano wrote:

> "John Cai via GitGitGadget" <[e-mail]> 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.

Thank you!

> > From: John Cai <[e-mail]>
> >
> > 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 `--annotate-stdin` is better, especially since people who
might look for a replacement for `--stdin` in the documentation are much
more likely to find what they're looking for.

In addition to this, I also agree with Junio on the deprecation needing to
be done carefully.

Ciao,
Dscho

Patch

diff --git a/Documentation/git-name-rev.txt b/Documentation/git-name-rev.txt
index 5cb0eb0855f..64d59c6ee1a 100644
--- a/Documentation/git-name-rev.txt
+++ b/Documentation/git-name-rev.txt
@@ -43,10 +43,37 @@  OPTIONS
 	List all commits reachable from all refs
 
 --stdin::
+	This option is deprecated in favor of 'git name-rev --annotate-text'.
+	They are functionally equivalent.
+
+--annotate-text::
 	Transform stdin by substituting all the 40-character SHA-1
 	hexes (say $hex) with "$hex ($rev_name)".  When used with
 	--name-only, substitute with "$rev_name", omitting $hex
-	altogether.  Intended for the scripter's use.
+	altogether.
+
+	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
+
+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
+
+An abbreviated revision 2ae0a9cb82 will not be substituted.
+The full name is master,
+while its tree object is 70d105cc79e63b81cfdcb08a15297c23e60b07ad
+----------
 
 --name-only::
 	Instead of printing both the SHA-1 and the name, print only
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index 27f60153a6c..73b244a5aac 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -527,7 +527,7 @@  static void name_rev_line(char *p, struct name_ref_data *data)
 int cmd_name_rev(int argc, const char **argv, const char *prefix)
 {
 	struct object_array revs = OBJECT_ARRAY_INIT;
-	int all = 0, transform_stdin = 0, allow_undefined = 1, always = 0, peel_tag = 0;
+	int all = 0, annotate_text = 0, transform_stdin = 0, allow_undefined = 1, always = 0, peel_tag = 0;
 	struct name_ref_data data = { 0, 0, STRING_LIST_INIT_NODUP, STRING_LIST_INIT_NODUP };
 	struct option opts[] = {
 		OPT_BOOL(0, "name-only", &data.name_only, N_("print only ref-based names (no object names)")),
@@ -539,6 +539,7 @@  int cmd_name_rev(int argc, const char **argv, const char *prefix)
 		OPT_GROUP(""),
 		OPT_BOOL(0, "all", &all, N_("list all commits reachable from all refs")),
 		OPT_BOOL(0, "stdin", &transform_stdin, N_("read from stdin")),
+		OPT_BOOL(0, "annotate-text", &annotate_text, N_("annotate text text from stdin")),
 		OPT_BOOL(0, "undefined", &allow_undefined, N_("allow to print `undefined` names (default)")),
 		OPT_BOOL(0, "always",     &always,
 			   N_("show abbreviated commit object as fallback")),
@@ -554,11 +555,19 @@  int cmd_name_rev(int argc, const char **argv, const char *prefix)
 	init_commit_rev_name(&rev_names);
 	git_config(git_default_config, NULL);
 	argc = parse_options(argc, argv, prefix, opts, name_rev_usage, 0);
-	if (all + transform_stdin + !!argc > 1) {
+
+	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;
+	}
+
+	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)) {