"John Cai via GitGitGadget" [off-list ref] 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 <redacted>
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.
quoted hunk
+ }
+
+ 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.
On Dec 27, 2021, at 11:49 AM, Junio C Hamano [off-list ref] wrote:
"John Cai via GitGitGadget" [off-list ref] 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
From: John Cai <redacted>
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.
quoted
+ 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'.
quoted
+
+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.
quoted
+ 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
+ }
+
+ 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.
John Cai [off-list ref] writes:
quoted
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.
Hi Junio,
On Mon, 27 Dec 2021, Junio C Hamano wrote:
"John Cai via GitGitGadget" [off-list ref] 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!
quoted
From: John Cai <redacted>
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