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

From: John Cai <[email protected]>

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.

builtin/name-rev.c +4 -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++) {

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

Quoted 9 lines

From: John Cai <[email protected]>

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.

Documentation/git-name-rev.txt OPTIONS · line 64

Quoted 10 lines

Documentation/git-name-rev.txt +10 -0

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

Documentation/git-name-rev.txt OPTIONS · line 71

Quoted 7 lines

Documentation/git-name-rev.txt +7 -0

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

builtin/name-rev.c int cmd_name_rev(int argc, const char **argv, const char *prefix) · line 563

Quoted 5 lines

builtin/name-rev.c +5 -0

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

Quoted 12 lines

builtin/name-rev.c +3 -0

+	}
+
+	if (all + annotate_text + !!argc > 1) {

		error("Specify either a list, or --all, not both!");
		usage_with_options(name_rev_usage, opts);
	}

builtin/name-rev.c +1 -1

-	if (all || transform_stdin)
+	if (all || annotate_text)

		cutoff = 0;

	for (; argc; argc--, argv++) {

builtin/name-rev.c int cmd_name_rev(int argc, const char **argv, const char *prefix) · line 628

Quoted 8 lines

builtin/name-rev.c +1 -1

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

Quoted 8 lines

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?

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

Quoted 9 lines

From: John Cai <[email protected]>

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.

From: John Cai <[email protected]>

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" <[email protected]>

---
 Documentation/git-name-rev.txt | 29 ++++++++++++++++++++++++++++-
 builtin/name-rev.c             | 19 ++++++++++++++-----
 2 files changed, 42 insertions(+), 6 deletions(-)

From: John Cai <[email protected]>

Introduce a --annotate-text that is functionally equivalent of --stdin.

From: John Cai <[email protected]>

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" <[email protected]>

---
 Documentation/git-name-rev.txt       | 29 +++++++++++++++++++++++++++-
 builtin/name-rev.c                   | 19 +++++++++++++-----
 t/t3412-rebase-root.sh               |  2 +-
 t/t4202-log.sh                       |  2 +-
 t/t6007-rev-list-cherry-pick-file.sh | 26 ++++++++++++-------------
 t/t6012-rev-list-simplify.sh         |  2 +-
 t/t6111-rev-list-treesame.sh         |  3 ++-
 t/t6120-describe.sh                  |  9 +++++++--
 8 files changed, 67 insertions(+), 25 deletions(-)

Documentation/git-name-rev.txt +0 -0

index 5cb0eb0855f..ee11b2ca9e7 100644
@@ -43,10 +43,37 @@ OPTIONS
 	List all commits reachable from all refs
 

Documentation/git-name-rev.txt +23 -0

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

builtin/name-rev.c +14 -5

index 27f60153a6c..21370afdaf9 100644
@@ -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_stdin = 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-stdin", &annotate_stdin, N_("annotate 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-stdin instead, "
+					"which is functionally equivalent.\n"
+					"This option will be removed in a future release.");
+		annotate_stdin = 1;
+	}
+
+	if (all + annotate_stdin + !!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_stdin)
 		cutoff = 0;
 
 	for (; argc; argc--, argv++) {

@@ -613,8 +622,8 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix)
 	for_each_ref(name_ref, &data);
 	name_tips();
 
-	if (transform_stdin) {
-		char buffer[2048];
+	if (annotate_stdin) {
+		struct strbuf sb = STRBUF_INIT;
 
 		while (!feof(stdin)) {
 			char *p = fgets(buffer, sizeof(buffer), stdin);

t/t3412-rebase-root.sh +1 -1

index 19c6f4acbf6..1e9f7833dd6 100755
@@ -11,7 +11,7 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
 log_with_names () {
 	git rev-list --topo-order --parents --pretty="tformat:%s" HEAD |
-	git name-rev --stdin --name-only --refs=refs/heads/$1
+	git name-rev --annotate-stdin --name-only --refs=refs/heads/$1
 }
 
 

t/t4202-log.sh +1 -1

index 50495598619..dc884107de4 100755
@@ -659,7 +659,7 @@ EOF
 
 test_expect_success 'log --graph with full output' '
 	git log --graph --date-order --pretty=short |
-		git name-rev --name-only --stdin |
+		git name-rev --name-only --annotate-stdin |
 		sed "s/Merge:.*/Merge: A B/;s/ *\$//" >actual &&
 	test_cmp expect actual
 '

t/t6007-rev-list-cherry-pick-file.sh +13 -13

index aebe4b69e13..6f3e5439771 100755
@@ -58,7 +58,7 @@ EOF
 
 test_expect_success '--left-right' '
 	git rev-list --left-right B...C > actual &&
-	git name-rev --stdin --name-only --refs="*tags/*" \
+	git name-rev --annotate-stdin --name-only --refs="*tags/*" \
 		< actual > actual.named &&
 	test_cmp expect actual.named
 '

@@ -78,14 +78,14 @@ EOF
 
 test_expect_success '--cherry-pick bar does not come up empty' '
 	git rev-list --left-right --cherry-pick B...C -- bar > actual &&
-	git name-rev --stdin --name-only --refs="*tags/*" \
+	git name-rev --annotate-stdin --name-only --refs="*tags/*" \
 		< actual > actual.named &&
 	test_cmp expect actual.named
 '
 
 test_expect_success 'bar does not come up empty' '
 	git rev-list --left-right B...C -- bar > actual &&
-	git name-rev --stdin --name-only --refs="*tags/*" \
+	git name-rev --annotate-stdin --name-only --refs="*tags/*" \
 		< actual > actual.named &&
 	test_cmp expect actual.named
 '

@@ -97,14 +97,14 @@ EOF
 
 test_expect_success '--cherry-pick bar does not come up empty (II)' '
 	git rev-list --left-right --cherry-pick F...E -- bar > actual &&
-	git name-rev --stdin --name-only --refs="*tags/*" \
+	git name-rev --annotate-stdin --name-only --refs="*tags/*" \
 		< actual > actual.named &&
 	test_cmp expect actual.named
 '
 
 test_expect_success 'name-rev multiple --refs combine inclusive' '
 	git rev-list --left-right --cherry-pick F...E -- bar >actual &&
-	git name-rev --stdin --name-only --refs="*tags/F" --refs="*tags/E" \
+	git name-rev --annotate-stdin --name-only --refs="*tags/F" --refs="*tags/E" \
 		<actual >actual.named &&
 	test_cmp expect actual.named
 '

@@ -116,7 +116,7 @@ EOF
 test_expect_success 'name-rev --refs excludes non-matched patterns' '
 	git rev-list --left-right --right-only --cherry-pick F...E -- bar >>expect &&
 	git rev-list --left-right --cherry-pick F...E -- bar >actual &&
-	git name-rev --stdin --name-only --refs="*tags/F" \
+	git name-rev --annotate-stdin --name-only --refs="*tags/F" \
 		<actual >actual.named &&
 	test_cmp expect actual.named
 '

@@ -128,14 +128,14 @@ EOF
 test_expect_success 'name-rev --exclude excludes matched patterns' '
 	git rev-list --left-right --right-only --cherry-pick F...E -- bar >>expect &&
 	git rev-list --left-right --cherry-pick F...E -- bar >actual &&
-	git name-rev --stdin --name-only --refs="*tags/*" --exclude="*E" \
+	git name-rev --annotate-stdin --name-only --refs="*tags/*" --exclude="*E" \
 		<actual >actual.named &&
 	test_cmp expect actual.named
 '
 
 test_expect_success 'name-rev --no-refs clears the refs list' '
 	git rev-list --left-right --cherry-pick F...E -- bar >expect &&
-	git name-rev --stdin --name-only --refs="*tags/F" --refs="*tags/E" --no-refs --refs="*tags/G" \
+	git name-rev --annotate-stdin --name-only --refs="*tags/F" --refs="*tags/E" --no-refs --refs="*tags/G" \
 		<expect >actual &&
 	test_cmp expect actual
 '

@@ -149,7 +149,7 @@ EOF
 
 test_expect_success '--cherry-mark' '
 	git rev-list --cherry-mark F...E -- bar > actual &&
-	git name-rev --stdin --name-only --refs="*tags/*" \
+	git name-rev --annotate-stdin --name-only --refs="*tags/*" \
 		< actual > actual.named &&
 	test_cmp expect actual.named
 '

@@ -163,7 +163,7 @@ EOF
 
 test_expect_success '--cherry-mark --left-right' '
 	git rev-list --cherry-mark --left-right F...E -- bar > actual &&
-	git name-rev --stdin --name-only --refs="*tags/*" \
+	git name-rev --annotate-stdin --name-only --refs="*tags/*" \
 		< actual > actual.named &&
 	test_cmp expect actual.named
 '

@@ -174,14 +174,14 @@ EOF
 
 test_expect_success '--cherry-pick --right-only' '
 	git rev-list --cherry-pick --right-only F...E -- bar > actual &&
-	git name-rev --stdin --name-only --refs="*tags/*" \
+	git name-rev --annotate-stdin --name-only --refs="*tags/*" \
 		< actual > actual.named &&
 	test_cmp expect actual.named
 '
 
 test_expect_success '--cherry-pick --left-only' '
 	git rev-list --cherry-pick --left-only E...F -- bar > actual &&
-	git name-rev --stdin --name-only --refs="*tags/*" \
+	git name-rev --annotate-stdin --name-only --refs="*tags/*" \
 		< actual > actual.named &&
 	test_cmp expect actual.named
 '

@@ -193,7 +193,7 @@ EOF
 
 test_expect_success '--cherry' '
 	git rev-list --cherry F...E -- bar > actual &&
-	git name-rev --stdin --name-only --refs="*tags/*" \
+	git name-rev --annotate-stdin --name-only --refs="*tags/*" \
 		< actual > actual.named &&
 	test_cmp expect actual.named
 '

t/t6012-rev-list-simplify.sh +1 -1

index 4f7fa8b6c03..4fedc614fa6 100755
@@ -12,7 +12,7 @@ note () {
 }
 
 unnote () {
-	git name-rev --tags --stdin | sed -e "s|$OID_REGEX (tags/\([^)]*\)) |\1 |g"
+	git name-rev --tags --annotate-stdin | sed -e "s|$OID_REGEX (tags/\([^)]*\)) |\1 |g"
 }
 
 #

t/t6111-rev-list-treesame.sh +2 -1

index e07b6070e0e..90ff1416400 100755
@@ -23,7 +23,8 @@ note () {
 }
 
 unnote () {
-	git name-rev --tags --stdin | sed -e "s|$OID_REGEX (tags/\([^)]*\))\([ 	]\)|\1\2|g"
+	git name-rev --tags --annotate-stdin | \
+	sed -e "s|$OID_REGEX (tags/\([^)]*\))\([ 	]\)|\1\2|g"
 }
 
 test_expect_success setup '

t/t6120-describe.sh +7 -2

index d8af2bb9d2b..9781b92aedd 100755
@@ -270,7 +270,7 @@ test_expect_success 'name-rev --all' '
 	test_cmp expect actual
 '
 
-test_expect_success 'name-rev --stdin' '
+test_expect_success 'name-rev --annotate-stdin' '
 	>expect.unsorted &&
 	for rev in $(git rev-list --all)
 	do

@@ -278,11 +278,16 @@ test_expect_success 'name-rev --stdin' '
 		echo "$rev ($name)" >>expect.unsorted || return 1
 	done &&
 	sort <expect.unsorted >expect &&
-	git rev-list --all | git name-rev --stdin >actual.unsorted &&
+	git rev-list --all | git name-rev --annotate-stdin >actual.unsorted &&
 	sort <actual.unsorted >actual &&
 	test_cmp expect actual
 '
 
+test_expect_success 'name-rev --stdin deprecated' "
+	git rev-list --all | git name-rev --stdin 2>actual &&
+	grep -E 'warning: --stdin is deprecated' actual
+"
+
 test_expect_success 'describe --contains with the exact tags' '
 	echo "A^0" >expect &&
 	tag_object=$(git rev-parse refs/tags/A) &&

Quoted 4 lines

--stdin::

Documentation/git-name-rev.txt +3 -0

+	This option is deprecated in favor of 'git name-rev --annotate-stdin'.
+	They are functionally equivalent.
+

Documentation/git-name-rev.txt OPTIONS · line 53

Quoted 6 lines

+--annotate-stdin::

Documentation/git-name-rev.txt +1 -1

 	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.

Is there a preferred order for the old/new variant documentation?

Documentation/git-name-rev.txt OPTIONS · line 53

Quoted 11 lines · John Cai via GitGitGadget

From: John Cai <[email protected]>
--stdin::

Documentation/git-name-rev.txt +5 -1

+     This option is deprecated in favor of 'git name-rev --annotate-stdin'.
+     They are functionally equivalent.
+
+--annotate-stdin::
      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.

Is there a preferred order for the old/new variant documentation?

It struck me that the docs should offer the new (now 'correct') variant first.

I didn't see anything about deprecation in the CodingGuidlines or SubmittingPatches (only a quick search though)

From: John Cai <[email protected]>

Using a buffer limited to 2048 is unnecessarily limiting. Switch to using a string buffer to read in stdin for annotation.

Signed-off-by "John Cai" <[email protected]>

---
 builtin/name-rev.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

builtin/name-rev.c +0 -0

index 21370afdaf9..85993bc2b38 100644

From: John Cai <[email protected]>
[...]

builtin/name-rev.c +1 -1

index 27f60153a6c..21370afdaf9 100644
@@ -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_stdin = 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)")),

builtin/name-rev.c +11 -2

+		OPT_BOOL(0, "annotate-stdin", &annotate_stdin, N_("annotate 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-stdin instead, "
+					"which is functionally equivalent.\n"
+					"This option will be removed in a future release.");
+		annotate_stdin = 1;
+	}
+
+	if (all + annotate_stdin + !!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_stdin)
  		cutoff = 0;
 
  	for (; argc; argc--, argv++) {

builtin/name-rev.c +0 -0

  		while (!feof(stdin)) {
  			char *p = fgets(buffer, sizeof(buffer), stdin);

t/t3412-rebase-root.sh +1 -1

index 19c6f4acbf6..1e9f7833dd6 100755
@@ -11,7 +11,7 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
  log_with_names () {
  	git rev-list --topo-order --parents --pretty="tformat:%s" HEAD |
-	git name-rev --stdin --name-only --refs=refs/heads/$1
+	git name-rev --annotate-stdin --name-only --refs=refs/heads/$1
  }
 
 

t/t4202-log.sh +1 -1

index 50495598619..dc884107de4 100755
@@ -659,7 +659,7 @@ EOF
 
  test_expect_success 'log --graph with full output' '
  	git log --graph --date-order --pretty=short |
-		git name-rev --name-only --stdin |
+		git name-rev --name-only --annotate-stdin |
  		sed "s/Merge:.*/Merge: A B/;s/ *\$//" >actual &&
  	test_cmp expect actual
  '

t/t6007-rev-list-cherry-pick-file.sh +13 -13

index aebe4b69e13..6f3e5439771 100755
@@ -58,7 +58,7 @@ EOF
 
  test_expect_success '--left-right' '
  	git rev-list --left-right B...C > actual &&
-	git name-rev --stdin --name-only --refs="*tags/*" \
+	git name-rev --annotate-stdin --name-only --refs="*tags/*" \
  		< actual > actual.named &&
  	test_cmp expect actual.named
  '

@@ -78,14 +78,14 @@ EOF
 
  test_expect_success '--cherry-pick bar does not come up empty' '
  	git rev-list --left-right --cherry-pick B...C -- bar > actual &&
-	git name-rev --stdin --name-only --refs="*tags/*" \
+	git name-rev --annotate-stdin --name-only --refs="*tags/*" \
  		< actual > actual.named &&
  	test_cmp expect actual.named
  '
 
  test_expect_success 'bar does not come up empty' '
  	git rev-list --left-right B...C -- bar > actual &&
-	git name-rev --stdin --name-only --refs="*tags/*" \
+	git name-rev --annotate-stdin --name-only --refs="*tags/*" \
  		< actual > actual.named &&
  	test_cmp expect actual.named
  '

@@ -97,14 +97,14 @@ EOF
 
  test_expect_success '--cherry-pick bar does not come up empty (II)' '
  	git rev-list --left-right --cherry-pick F...E -- bar > actual &&
-	git name-rev --stdin --name-only --refs="*tags/*" \
+	git name-rev --annotate-stdin --name-only --refs="*tags/*" \
  		< actual > actual.named &&
  	test_cmp expect actual.named
  '
 
  test_expect_success 'name-rev multiple --refs combine inclusive' '
  	git rev-list --left-right --cherry-pick F...E -- bar >actual &&
-	git name-rev --stdin --name-only --refs="*tags/F" --refs="*tags/E" \
+	git name-rev --annotate-stdin --name-only --refs="*tags/F" --refs="*tags/E" \
  		<actual >actual.named &&
  	test_cmp expect actual.named
  '

@@ -116,7 +116,7 @@ EOF
  test_expect_success 'name-rev --refs excludes non-matched patterns' '
  	git rev-list --left-right --right-only --cherry-pick F...E -- bar >>expect &&
  	git rev-list --left-right --cherry-pick F...E -- bar >actual &&
-	git name-rev --stdin --name-only --refs="*tags/F" \
+	git name-rev --annotate-stdin --name-only --refs="*tags/F" \
  		<actual >actual.named &&
  	test_cmp expect actual.named
  '

@@ -128,14 +128,14 @@ EOF
  test_expect_success 'name-rev --exclude excludes matched patterns' '
  	git rev-list --left-right --right-only --cherry-pick F...E -- bar >>expect &&
  	git rev-list --left-right --cherry-pick F...E -- bar >actual &&
-	git name-rev --stdin --name-only --refs="*tags/*" --exclude="*E" \
+	git name-rev --annotate-stdin --name-only --refs="*tags/*" --exclude="*E" \
  		<actual >actual.named &&
  	test_cmp expect actual.named
  '
 
  test_expect_success 'name-rev --no-refs clears the refs list' '
  	git rev-list --left-right --cherry-pick F...E -- bar >expect &&
-	git name-rev --stdin --name-only --refs="*tags/F" --refs="*tags/E" --no-refs --refs="*tags/G" \
+	git name-rev --annotate-stdin --name-only --refs="*tags/F" --refs="*tags/E" --no-refs --refs="*tags/G" \
  		<expect >actual &&
  	test_cmp expect actual
  '

@@ -149,7 +149,7 @@ EOF
 
  test_expect_success '--cherry-mark' '
  	git rev-list --cherry-mark F...E -- bar > actual &&
-	git name-rev --stdin --name-only --refs="*tags/*" \
+	git name-rev --annotate-stdin --name-only --refs="*tags/*" \
  		< actual > actual.named &&
  	test_cmp expect actual.named
  '

@@ -163,7 +163,7 @@ EOF
 
  test_expect_success '--cherry-mark --left-right' '
  	git rev-list --cherry-mark --left-right F...E -- bar > actual &&
-	git name-rev --stdin --name-only --refs="*tags/*" \
+	git name-rev --annotate-stdin --name-only --refs="*tags/*" \
  		< actual > actual.named &&
  	test_cmp expect actual.named
  '

@@ -174,14 +174,14 @@ EOF
 
  test_expect_success '--cherry-pick --right-only' '
  	git rev-list --cherry-pick --right-only F...E -- bar > actual &&
-	git name-rev --stdin --name-only --refs="*tags/*" \
+	git name-rev --annotate-stdin --name-only --refs="*tags/*" \
  		< actual > actual.named &&
  	test_cmp expect actual.named
  '
 
  test_expect_success '--cherry-pick --left-only' '
  	git rev-list --cherry-pick --left-only E...F -- bar > actual &&
-	git name-rev --stdin --name-only --refs="*tags/*" \
+	git name-rev --annotate-stdin --name-only --refs="*tags/*" \
  		< actual > actual.named &&
  	test_cmp expect actual.named
  '

@@ -193,7 +193,7 @@ EOF
 
  test_expect_success '--cherry' '
  	git rev-list --cherry F...E -- bar > actual &&
-	git name-rev --stdin --name-only --refs="*tags/*" \
+	git name-rev --annotate-stdin --name-only --refs="*tags/*" \
  		< actual > actual.named &&
  	test_cmp expect actual.named
  '

t/t6012-rev-list-simplify.sh +1 -1

index 4f7fa8b6c03..4fedc614fa6 100755
@@ -12,7 +12,7 @@ note () {
  }
 
  unnote () {
-	git name-rev --tags --stdin | sed -e "s|$OID_REGEX (tags/\([^)]*\)) |\1 |g"
+	git name-rev --tags --annotate-stdin | sed -e "s|$OID_REGEX (tags/\([^)]*\)) |\1 |g"
  }
 
  #

t/t6111-rev-list-treesame.sh +2 -1

index e07b6070e0e..90ff1416400 100755
@@ -23,7 +23,8 @@ note () {
  }
 
  unnote () {
-	git name-rev --tags --stdin | sed -e "s|$OID_REGEX (tags/\([^)]*\))\([ 	]\)|\1\2|g"
+	git name-rev --tags --annotate-stdin | \
+	sed -e "s|$OID_REGEX (tags/\([^)]*\))\([ 	]\)|\1\2|g"
  }
 
  test_expect_success setup '

t/t6120-describe.sh +7 -2

index d8af2bb9d2b..9781b92aedd 100755
@@ -270,7 +270,7 @@ test_expect_success 'name-rev --all' '
  	test_cmp expect actual
  '
 
-test_expect_success 'name-rev --stdin' '
+test_expect_success 'name-rev --annotate-stdin' '
  	>expect.unsorted &&
  	for rev in $(git rev-list --all)
  	do

@@ -278,11 +278,16 @@ test_expect_success 'name-rev --stdin' '
  		echo "$rev ($name)" >>expect.unsorted || return 1
  	done &&
  	sort <expect.unsorted >expect &&
-	git rev-list --all | git name-rev --stdin >actual.unsorted &&
+	git rev-list --all | git name-rev --annotate-stdin >actual.unsorted &&
  	sort <actual.unsorted >actual &&
  	test_cmp expect actual
  '
 
+test_expect_success 'name-rev --stdin deprecated' "
+	git rev-list --all | git name-rev --stdin 2>actual &&
+	grep -E 'warning: --stdin is deprecated' actual
+"
+
  test_expect_success 'describe --contains with the exact tags' '
  	echo "A^0" >expect &&
  	tag_object=$(git rev-parse refs/tags/A) &&

builtin/name-rev.c int cmd_name_rev(int argc, const char **argv, const char *prefix) · line 626

Quoted 7 lines

builtin/name-rev.c +2 -1

@@ -613,8 +622,8 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix)
  	for_each_ref(name_ref, &data);
  	name_tips();
  -	if (transform_stdin) {
-		char buffer[2048];
+	if (annotate_stdin) {
+		struct strbuf sb = STRBUF_INIT;

I think this hunk belongs in the next patch. Before posting a patch series I find it helpful to run

    git rebase --keep-base -x'make -j4 git && cd t && prove -j4 <tests
    I think might fail> :: --root=/dev/shm'

to check that the individual patches compile and pass the relevant tests. I've never got round to trying it but git-test[1] also lets you test all the commits in a series

builtin/name-rev.c int cmd_name_rev(int argc, const char **argv, const char *prefix) · line 541

Quoted 4 lines

builtin/name-rev.c +0 -0

@@ -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")),

If the intention is to deprecate this option then it might be worth marking it as PARSE_OPT_HIDDEN so that it is not shown by 'git name-rev -h'. (You need to change OPT_BOOL to OPT_BOOL_F to pass the flag)

From: John Cai <[email protected]>

Using a buffer limited to 2048 is unnecessarily limiting. Switch to using a string buffer to read in stdin for annotation.

Signed-off-by "John Cai" <[email protected]>

---
builtin/name-rev.c | 9 ++++-----

1 file changed, 4 insertions(+), 5 deletions(-)

builtin/name-rev.c +0 -0

index 21370afdaf9..d16b4ca0b66 100644

builtin/name-rev.c +2 -0

+			name_rev_line(sb.buf, &data);
 		}
+		strbuf_release(&sb);
 	} else if (all) {
 		int i, max;
 

Signature · 1 line

gitgitgadget

Documentation/git-name-rev.txt +13 -0

+	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-stdin <sample.txt
+

Introduce a --annotate-stdin 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 --annotate-stdin.

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/[email protected]/ for discussion.

changes since v5:

* fixed documentation example indentation (again)
* moved initialization of strbuf from 1/2 to 2/2

From: John Cai <[email protected]>

Introduce a --annotate-stdin 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-stdin.

This change adds a warning to --stdin warning that it will be removed in the future.

Signed-off-by "John Cai" <[email protected]>

---
 Documentation/git-name-rev.txt       | 30 ++++++++++++++++++++++++++--
 builtin/name-rev.c                   | 19 +++++++++++++-----
 t/t3412-rebase-root.sh               |  2 +-
 t/t4202-log.sh                       |  2 +-
 t/t6007-rev-list-cherry-pick-file.sh | 26 ++++++++++++------------
 t/t6012-rev-list-simplify.sh         |  2 +-
 t/t6111-rev-list-treesame.sh         |  3 ++-
 t/t6120-describe.sh                  |  9 +++++++--
 8 files changed, 67 insertions(+), 26 deletions(-)

Documentation/git-name-rev.txt +28 -2

index 5cb0eb0855f..ec8a27ce8bf 100644
@@ -42,11 +42,37 @@ OPTIONS
 --all::
 	List all commits reachable from all refs
 
---stdin::
+--annotate-stdin::
 	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-stdin <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-stdin <sample.txt
+
+An abbreviated revision 2ae0a9cb82 will not be substituted.
+The full name after substitution is master,
+while its tree object is 70d105cc79e63b81cfdcb08a15297c23e60b07ad
+-----------
+
+--stdin::
+	This option is deprecated in favor of 'git name-rev --annotate-stdin'.
+	They are functionally equivalent.
 
 --name-only::
 	Instead of printing both the SHA-1 and the name, print only

builtin/name-rev.c +1 -1

index 27f60153a6c..8baf5b52d0b 100644
@@ -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_stdin = 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)")),

builtin/name-rev.c +11 -3

 		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-stdin instead, "
+					"which is functionally equivalent.\n"
+					"This option will be removed in a future release.");
+		annotate_stdin = 1;
+	}
+
+	if (all + annotate_stdin + !!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_stdin)
 		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_stdin) {
 		char buffer[2048];
 
 		while (!feof(stdin)) {

t/t3412-rebase-root.sh +1 -1

index 19c6f4acbf6..1e9f7833dd6 100755
@@ -11,7 +11,7 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
 log_with_names () {
 	git rev-list --topo-order --parents --pretty="tformat:%s" HEAD |
-	git name-rev --stdin --name-only --refs=refs/heads/$1
+	git name-rev --annotate-stdin --name-only --refs=refs/heads/$1
 }
 
 

t/t4202-log.sh +1 -1

index 50495598619..dc884107de4 100755
@@ -659,7 +659,7 @@ EOF
 
 test_expect_success 'log --graph with full output' '
 	git log --graph --date-order --pretty=short |
-		git name-rev --name-only --stdin |
+		git name-rev --name-only --annotate-stdin |
 		sed "s/Merge:.*/Merge: A B/;s/ *\$//" >actual &&
 	test_cmp expect actual
 '

t/t6007-rev-list-cherry-pick-file.sh +13 -13

index aebe4b69e13..6f3e5439771 100755
@@ -58,7 +58,7 @@ EOF
 
 test_expect_success '--left-right' '
 	git rev-list --left-right B...C > actual &&
-	git name-rev --stdin --name-only --refs="*tags/*" \
+	git name-rev --annotate-stdin --name-only --refs="*tags/*" \
 		< actual > actual.named &&
 	test_cmp expect actual.named
 '

@@ -78,14 +78,14 @@ EOF
 
 test_expect_success '--cherry-pick bar does not come up empty' '
 	git rev-list --left-right --cherry-pick B...C -- bar > actual &&
-	git name-rev --stdin --name-only --refs="*tags/*" \
+	git name-rev --annotate-stdin --name-only --refs="*tags/*" \
 		< actual > actual.named &&
 	test_cmp expect actual.named
 '
 
 test_expect_success 'bar does not come up empty' '
 	git rev-list --left-right B...C -- bar > actual &&
-	git name-rev --stdin --name-only --refs="*tags/*" \
+	git name-rev --annotate-stdin --name-only --refs="*tags/*" \
 		< actual > actual.named &&
 	test_cmp expect actual.named
 '

@@ -97,14 +97,14 @@ EOF
 
 test_expect_success '--cherry-pick bar does not come up empty (II)' '
 	git rev-list --left-right --cherry-pick F...E -- bar > actual &&
-	git name-rev --stdin --name-only --refs="*tags/*" \
+	git name-rev --annotate-stdin --name-only --refs="*tags/*" \
 		< actual > actual.named &&
 	test_cmp expect actual.named
 '
 
 test_expect_success 'name-rev multiple --refs combine inclusive' '
 	git rev-list --left-right --cherry-pick F...E -- bar >actual &&
-	git name-rev --stdin --name-only --refs="*tags/F" --refs="*tags/E" \
+	git name-rev --annotate-stdin --name-only --refs="*tags/F" --refs="*tags/E" \
 		<actual >actual.named &&
 	test_cmp expect actual.named
 '

@@ -116,7 +116,7 @@ EOF
 test_expect_success 'name-rev --refs excludes non-matched patterns' '
 	git rev-list --left-right --right-only --cherry-pick F...E -- bar >>expect &&
 	git rev-list --left-right --cherry-pick F...E -- bar >actual &&
-	git name-rev --stdin --name-only --refs="*tags/F" \
+	git name-rev --annotate-stdin --name-only --refs="*tags/F" \
 		<actual >actual.named &&
 	test_cmp expect actual.named
 '

@@ -128,14 +128,14 @@ EOF
 test_expect_success 'name-rev --exclude excludes matched patterns' '
 	git rev-list --left-right --right-only --cherry-pick F...E -- bar >>expect &&
 	git rev-list --left-right --cherry-pick F...E -- bar >actual &&
-	git name-rev --stdin --name-only --refs="*tags/*" --exclude="*E" \
+	git name-rev --annotate-stdin --name-only --refs="*tags/*" --exclude="*E" \
 		<actual >actual.named &&
 	test_cmp expect actual.named
 '
 
 test_expect_success 'name-rev --no-refs clears the refs list' '
 	git rev-list --left-right --cherry-pick F...E -- bar >expect &&
-	git name-rev --stdin --name-only --refs="*tags/F" --refs="*tags/E" --no-refs --refs="*tags/G" \
+	git name-rev --annotate-stdin --name-only --refs="*tags/F" --refs="*tags/E" --no-refs --refs="*tags/G" \
 		<expect >actual &&
 	test_cmp expect actual
 '

@@ -149,7 +149,7 @@ EOF
 
 test_expect_success '--cherry-mark' '
 	git rev-list --cherry-mark F...E -- bar > actual &&
-	git name-rev --stdin --name-only --refs="*tags/*" \
+	git name-rev --annotate-stdin --name-only --refs="*tags/*" \
 		< actual > actual.named &&
 	test_cmp expect actual.named
 '

@@ -163,7 +163,7 @@ EOF
 
 test_expect_success '--cherry-mark --left-right' '
 	git rev-list --cherry-mark --left-right F...E -- bar > actual &&
-	git name-rev --stdin --name-only --refs="*tags/*" \
+	git name-rev --annotate-stdin --name-only --refs="*tags/*" \
 		< actual > actual.named &&
 	test_cmp expect actual.named
 '

@@ -174,14 +174,14 @@ EOF
 
 test_expect_success '--cherry-pick --right-only' '
 	git rev-list --cherry-pick --right-only F...E -- bar > actual &&
-	git name-rev --stdin --name-only --refs="*tags/*" \
+	git name-rev --annotate-stdin --name-only --refs="*tags/*" \
 		< actual > actual.named &&
 	test_cmp expect actual.named
 '
 
 test_expect_success '--cherry-pick --left-only' '
 	git rev-list --cherry-pick --left-only E...F -- bar > actual &&
-	git name-rev --stdin --name-only --refs="*tags/*" \
+	git name-rev --annotate-stdin --name-only --refs="*tags/*" \
 		< actual > actual.named &&
 	test_cmp expect actual.named
 '

@@ -193,7 +193,7 @@ EOF
 
 test_expect_success '--cherry' '
 	git rev-list --cherry F...E -- bar > actual &&
-	git name-rev --stdin --name-only --refs="*tags/*" \
+	git name-rev --annotate-stdin --name-only --refs="*tags/*" \
 		< actual > actual.named &&
 	test_cmp expect actual.named
 '

t/t6012-rev-list-simplify.sh +1 -1

index 4f7fa8b6c03..4fedc614fa6 100755
@@ -12,7 +12,7 @@ note () {
 }
 
 unnote () {
-	git name-rev --tags --stdin | sed -e "s|$OID_REGEX (tags/\([^)]*\)) |\1 |g"
+	git name-rev --tags --annotate-stdin | sed -e "s|$OID_REGEX (tags/\([^)]*\)) |\1 |g"
 }
 
 #

t/t6111-rev-list-treesame.sh +2 -1

index e07b6070e0e..90ff1416400 100755
@@ -23,7 +23,8 @@ note () {
 }
 
 unnote () {
-	git name-rev --tags --stdin | sed -e "s|$OID_REGEX (tags/\([^)]*\))\([ 	]\)|\1\2|g"
+	git name-rev --tags --annotate-stdin | \
+	sed -e "s|$OID_REGEX (tags/\([^)]*\))\([ 	]\)|\1\2|g"
 }
 
 test_expect_success setup '

t/t6120-describe.sh +7 -2

index d8af2bb9d2b..9781b92aedd 100755
@@ -270,7 +270,7 @@ test_expect_success 'name-rev --all' '
 	test_cmp expect actual
 '
 
-test_expect_success 'name-rev --stdin' '
+test_expect_success 'name-rev --annotate-stdin' '
 	>expect.unsorted &&
 	for rev in $(git rev-list --all)
 	do

@@ -278,11 +278,16 @@ test_expect_success 'name-rev --stdin' '
 		echo "$rev ($name)" >>expect.unsorted || return 1
 	done &&
 	sort <expect.unsorted >expect &&
-	git rev-list --all | git name-rev --stdin >actual.unsorted &&
+	git rev-list --all | git name-rev --annotate-stdin >actual.unsorted &&
 	sort <actual.unsorted >actual &&
 	test_cmp expect actual
 '
 
+test_expect_success 'name-rev --stdin deprecated' "
+	git rev-list --all | git name-rev --stdin 2>actual &&
+	grep -E 'warning: --stdin is deprecated' actual
+"
+
 test_expect_success 'describe --contains with the exact tags' '
 	echo "A^0" >expect &&
 	tag_object=$(git rev-parse refs/tags/A) &&

Signature · 1 line

gitgitgadget

Hi Junio,

...

builtin/name-rev.c int cmd_name_rev(int argc, const char **argv, const char *prefix) · line 543

Quoted 2 lines

builtin/name-rev.c +1 -0

+		OPT_BOOL(0, "annotate-stdin", &annotate_stdin, N_("annotate text from stdin")),
 		OPT_BOOL(0, "undefined", &allow_undefined, N_("allow to print `undefined` names (default)")),

I’ve changed this back to a non hidden bool. I believe this should be the last thing needed on this one. Let me know if anything else needs adjustment, thanks!

Quoted 3 lines · John Cai

Hi Junio,

...

Please trim quotes that are irrelevant for understanding what you want to say in your message to save recipient's time to find what you wrote.

Quoted 5 lines

builtin/name-rev.c int cmd_name_rev(int argc, const char **argv, const char *prefix) · line 543

Quoted 2 lines

builtin/name-rev.c +1 -0

+		OPT_BOOL(0, "annotate-stdin", &annotate_stdin, N_("annotate text from stdin")),
 		OPT_BOOL(0, "undefined", &allow_undefined, N_("allow to print `undefined` names (default)")),

I’ve changed this back to a non hidden bool. I believe this should be the last thing needed on this one. Let me know if anything else needs adjustment, thanks!

As I said, my mention of hidden vs non-hidden was not my preferrence of non-hidden over hidden, but was soliciting opinions from others, so I was a bit surprised to see a reroll to change only that thing and nothing else so soon.

But if it is your preference to leave it unhidden for now, that is fine as well. I have no strong preference over the matter either way.

I see only Dscho on the CC list, but I have this feeling that he wasn't the one who suggested to hide the old option. Whoever it was, if we can get an ack from them, that would be nicer.

Thanks for working on this.

Quoted 5 lines

As I said, my mention of hidden vs non-hidden was not my preferrence of non-hidden over hidden, but was soliciting opinions from others, so I was a bit surprised to see a reroll to change only that thing and nothing else so soon.

Quoted 3 lines

But if it is your preference to leave it unhidden for now, that is fine as well. I have no strong preference over the matter either way.

I think it makes sense in this first iteration to keep it non-hidden for visibility purposes. Maybe in the next release we can make it hidden before we move it altogether.

Quoted 9 lines · John Cai

Quoted 8 lines

As I said, my mention of hidden vs non-hidden was not my preferrence of non-hidden over hidden, but was soliciting opinions from others, so I was a bit surprised to see a reroll to change only that thing and nothing else so soon.

But if it is your preference to leave it unhidden for now, that is fine as well. I have no strong preference over the matter either way.

Quoted 3 lines

I think it makes sense in this first iteration to keep it non-hidden for visibility purposes. Maybe in the next release we can make it hidden before we move it altogether.

I think that is a sensible way forward.

From: John Cai <[email protected]>

Using a buffer limited to 2048 is unnecessarily limiting. Switch to using a string buffer to read in stdin for annotation.

Signed-off-by "John Cai" <[email protected]>

---
 builtin/name-rev.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

builtin/name-rev.c +1 -3

index 8baf5b52d0b..138e3c30a2b 100644
@@ -623,14 +623,13 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix)
 	name_tips();
 
 	if (annotate_stdin) {
-		char buffer[2048];
+		struct strbuf sb = STRBUF_INIT;
 
-		while (!feof(stdin)) {
-			char *p = fgets(buffer, sizeof(buffer), stdin);