overswitched command lines
(1) By anonymous on 2021-02-02 17:22:16 [source]
You don't do it, but there's always one. (;
~/src/trunk-fossil$ ./fossil diff -N -v
unrecognized command-line option or missing argument: -N
[https://fossil-scm.org/fossil/file?ci=trunk&name=src%2Fdiffcmd.c&ln=894-896]
There is also a second probem:
~/src/trunk-fossil$ ./fossil diff --verbose -v
unrecognized command-line option or missing argument: -v
~/src/trunk-fossil$ ./fossil diff -v --verbose
unrecognized command-line option or missing argument: --verbose
~/src/trunk-fossil$ ./fossil diff -v -v
unrecognized command-line option or missing argument: -v
Thank you for your work.
(2) By Stephan Beal (stephan) on 2021-02-03 03:55:13 in reply to 1 [link] [source]
~/src/trunk-fossil$ ./fossil diff -N -v
As those code you linked to indicates, that deprecated -N flag is only valid if -verbose is not specified. Note that the help text for both -N and -v are nearly identical:
--new-file|-N Show complete text of added and deleted files
...
-v|--verbose Output complete text of added or deleted files
The bug there is that flag being marked as deprecated only in the code, not in the help text.
There is also a second probem:
Those aren't bugs - they're a side effect of how the CLI arg parser works, has always worked, and works for all arguments (not just -v -verbose). When the app-level code checks for a flag, that flag is consumed from the flag list, but only the first form (short or long) found in the flag list is consumed. After consuming all of its known flags, the app-level code asks, "are there any additional flags we have not consumed?" If so, it's an error.
The exact error text:
unrecognized command-line option or missing argument: -v
is a bit vague because the CLI parser does not know, by itself, whether -v requires a value or not - it's up to the app-level code to indicate whether that's the case when it looks up the flag in the first place.
(3) By anonymous on 2021-02-03 17:40:34 in reply to 2 [link] [source]
Hi!
First of all: I jumped in here in a hurry. Hurry and rudeness are probably not good companions. /: Sorry for that.
It was not my intention to embarrass anyone but rather to shift the focus to the user side.
As a newbie and SCM occasional user, I looked at `fossil diff --help` from top to bottom.
... The "-N" or "--new-file" option causes the complete text of added or deleted files to be displayed. ... --internal|-i Use internal diff logic --new-file|-N Show complete text of added and deleted files --numstat Show only the number of lines delete and added --side-by-side|-y Side-by-side diff --strip-trailing-cr Strip trailing CR --tclsh PATH Tcl/Tk used for --tk (default: "tclsh") --tk Launch a Tcl/Tk GUI for display --to VERSION Select VERSION as target for the diff --undo Diff against the "undo" buffer --unified Unified diff -v|--verbose Output complete text of added or deleted files
.o° "-N" does what I want it to do, and for good measure I'll throw in a "-v" as well.
The message "unrecognized command-line option or missing argument: -N" is not clear from the user's point of view. Only that something went wrong and the desired goal was not achieved.
The tolerated "--new-file|-N" can be seen as a plus in usability when coming from the 'standard' diff. (As an oblique user request:) Would the additional introduction of the short forms "-u|--unified" and possibly "--brief|-q" be worth considering?
Thanks for decoding the hidden question (bug or feature?) in the second part. I understood the detailed explanations from the developer's point of view (hopefully) and also the desire to get things done in an efficient manner.
The downside: "Users" sometimes do weird things, whether intentionally or by accident. They don't necessarily know about the internals, but notice that fossil is more accommodating here and there on the CLI. For example, slipping through incomplete commands when uniqueness could be determined, or passing of --long-args without the second preceding minus. In the case of the nonsensical redundancy of switches, it would rather leave the user out in the cold.
I would like to apologize again. Due to the partially used translator, there may be linguistic inconsistencies or suspected rudeness by picking some terms.
---
To pass the redundant --new-file|-N option in combination with -v.
Index: src/diffcmd.c
==================================================================
--- src/diffcmd.c
+++ src/diffcmd.c
@@ -892,11 +892,14 @@
diffFlags = diff_options();
verboseFlag = find_option("verbose","v",0)!=0;
if( !verboseFlag ){
verboseFlag = find_option("new-file","N",0)!=0; /* deprecated */
}
- if( verboseFlag ) diffFlags |= DIFF_VERBOSE;
+ if( verboseFlag ){
+ diffFlags |= DIFF_VERBOSE;
+ find_option("new-file","N",0);
+ }
if( againstUndo && ( zFrom!=0 || zTo!=0 || zCheckin!=0 || zBranch!=0) ){
fossil_fatal("cannot use --undo together with --from, --to, --checkin,"
" or --branch");
}
if( zBranch ){
Adjustment for -u as an alternative to diff --unified. (--brief|-q was omitted because the desired order in the help documentation would probably change.)
Index: src/diff.c
==================================================================
--- src/diff.c
+++ src/diff.c
@@ -1992,11 +1992,11 @@
** --invert Invert the diff DIFF_INVERT
** -n|--linenum Show line numbers DIFF_LINENO
** --noopt Disable optimization DIFF_NOOPT
** --numstat Show change counts DIFF_NUMSTAT
** --strip-trailing-cr Strip trailing CR DIFF_STRIP_EOLCR
-** --unified Unified diff. ~DIFF_SIDEBYSIDE
+** -u|--unified Unified diff. ~DIFF_SIDEBYSIDE
** -w|--ignore-all-space Ignore all whitespaces DIFF_IGNORE_ALLWS
** -W|--width N N character lines. DIFF_WIDTH_MASK
** -y|--side-by-side Side-by-side diff. DIFF_SIDEBYSIDE
** -Z|--ignore-trailing-space Ignore eol-whitespaces DIFF_IGNORE_EOLWS
*/
@@ -2015,11 +2015,11 @@
}
if( find_option("side-by-side","y",0)!=0 ) diffFlags |= DIFF_SIDEBYSIDE;
if( find_option("yy",0,0)!=0 ){
diffFlags |= DIFF_SIDEBYSIDE | DIFF_SLOW_SBS;
}
- if( find_option("unified",0,0)!=0 ) diffFlags &= ~DIFF_SIDEBYSIDE;
+ if( find_option("unified","u",0)!=0 ) diffFlags &= ~DIFF_SIDEBYSIDE;
if( (z = find_option("context","c",1))!=0 && (f = atoi(z))>=0 ){
if( f > DIFF_CONTEXT_MASK ) f = DIFF_CONTEXT_MASK;
diffFlags |= f + DIFF_CONTEXT_EX;
}
if( (z = find_option("width","W",1))!=0 && (f = atoi(z))>0 ){
Index: src/diffcmd.c
==================================================================
--- src/diffcmd.c
+++ src/diffcmd.c
@@ -855,11 +855,11 @@
** --strip-trailing-cr Strip trailing CR
** --tclsh PATH Tcl/Tk used for --tk (default: "tclsh")
** --tk Launch a Tcl/Tk GUI for display
** --to VERSION Select VERSION as target for the diff
** --undo Diff against the "undo" buffer
-** --unified Unified diff
+** -u|--unified Unified diff
** -v|--verbose Output complete text of added or deleted files
** -w|--ignore-all-space Ignore white space when comparing lines
** -W|--width N Width of lines in side-by-side diff
** -Z|--ignore-trailing-space Ignore changes to end-of-line whitespace
*/
(4) By Stephan Beal (stephan) on 2021-02-04 04:11:19 in reply to 3 [link] [source]
The downside: "Users" sometimes do weird things, whether intentionally or by accident...."
And yet you're the first, in many years, to report that combination ;).
Thanks for decoding the hidden question (bug or feature?) in the second part.
It's a little bit of both: feature in that it allows fossil to report that "something went wrong with the CLI handling," and to do so at a level which doesn't require that the app be explicitly told every single combination of flags for every command in advance. The bug, however, is that it cannot accurately report the nature of such errors nor know whether the offending flag expects a value or not.
Would the additional introduction of the short forms "-u|--unified" and possibly "--brief|-q" be worth considering?
i'm going to defer that one to... someone else. i'm hesitant to go adding flags willy-nilly to the diff command.
i'm not personally particularly keen on this change:
+ if( verboseFlag ){
+ diffFlags |= DIFF_VERBOSE;
+ find_option("new-file","N",0);
+ }
because it encourages a combination of flags which is meaningless. If it does not fail (as it currently does) then users may well expect that -v -N behaves differently that just -v or just -N, which isn't the case.
i will, however, update the help text to note that --new-file is just an alias for --verbose.
(5) By Andy Bradford (andybradford) on 2021-02-04 15:22:31 in reply to 4 [link] [source]
> i will, however, update the help text to note that --new-file is just > an alias for --verbose. Maybe another apropos change should also move the -v help text above the -N help text so it is seen first as users read the help from top to bottom? Andy
(6) By Stephan Beal (stephan) on 2021-02-05 03:56:05 in reply to 5 [link] [source]
Maybe another apropos change should also move ...
Done: moved -N down below -v, rather than moving -v up. (It just looked better to me that way.)
Sidebar: that command's help text uses a mix of:
-X|--long-X ....
--long-Y|-Y ....
and now i'm wondering if it would be worth the effort to go through the 300kb+ of help text and change that inconsistency, always preferring the short flag first (to follow the GNU tools conventions).
$ fossil help --everything | wc
8161 45312 323484
(7) By anonymous on 2021-02-05 23:32:11 in reply to 6 [link] [source]
Thanks for your work, Stephan. (:
A similar construct, like this one, might also be a possibility. file.c&ln=2334-2335
---
A big thanks also to jamsek for restructuring the help to -s|--long-form.
It's quite complicated not to step on anyone's toes. I've been working on some formulations here for quite a while. In any case, not meant badly and found along the way.
At the risk of creating discontent, there are still minor inconsistencies. Sometimes the long form "dryrun" vs. "dry-run" is also used with some commands. (files: smtp.c, tag.c, unversioned.c)
Just for quicker locating:
--- src/export.c +++ src/export.c @@ -1816,3 +1816,3 @@ ** piping it into "git fast-import". -** --force|-f Do the export even if nothing has changed +** -f|--force Do the export even if nothing has changed ** --if-mirrored No-op if the mirror does not already exist. @@ -1824,4 +1824,4 @@ ** this option is omitted. -** --quiet|-q Reduce output. Repeat for even less output. -** --verbose|-v More output. +** -q|--quiet Reduce output. Repeat for even less output. +** -v|--verbose More output. **--- src/file.c +++ src/file.c @@ -2292,3 +2292,3 @@ ** fossil-conventional glob list file. -** -v|-verbose Outputs extra information about its globs +** -v|--verbose Outputs extra information about its globs ** and each file it touches.
--- src/main.c +++ src/main.c @@ -1276,3 +1276,3 @@ ** -** Usage: %fossil version ?-v|-verbose? +** Usage: %fossil version ?-v|--verbose? **
--- src/purge.c +++ src/purge.c @@ -510,3 +510,3 @@ ** --explain Make no changes, but show what would happen. -** --dry-run An alias for --explain +** -n|--dry-run An alias for --explain */ @@ -523,3 +523,3 @@ n = (int)strlen(zSubcmd); - if( find_option("explain",0,0)!=0 || find_option("dry-run",0,0)!=0 ){ + if( find_option("explain",0,0)!=0 || find_option("dry-run","n",0)!=0 ){ purgeFlags |= PURGE_EXPLAIN_ONLY; @@ -551,3 +551,3 @@ int vid; - if( find_option("explain",0,0)!=0 || find_option("dry-run",0,0)!=0 ){ + if( find_option("explain",0,0)!=0 || find_option("dry-run","n",0)!=0 ){ purgeFlags |= PURGE_EXPLAIN_ONLY;
(8) By Stephan Beal (stephan) on 2021-02-06 06:32:40 in reply to 7 [link] [source]
Sometimes the long form "dryrun" vs. "dry-run" is also used with some commands.
That's unfortunate historical baggage we can't eliminate without risking breaking scripts. We can add the dry-run form everywhere which uses dryrun, though (and i thought that Warren did that a year or two ago).
Just for quicker locating:
i'm swapping the short/long pairs which were missed now but adding new flag aliases is not currently on my radar. In particular:
+ if( find_option("explain",0,0)!=0 || find_option("dry-run","n",0)!=0 ){
-n has a different meaning in some contexts, so adding it as a dry-run alias needs to be closely looked at before it's implemented (and it's currently way too early for that in my time zone ;).
And just FYI:
-** Usage: %fossil version ?-v|-verbose?
-foo and --foo are 100% equivalent in fossil. Nonetheless, i'll patch this for consistency's sake.
(9) By anonymous on 2021-02-08 17:15:44 in reply to 8 [link] [source]
Hi,
after all the pain, a big thanks for this.
-foo and --foo are 100% equivalent in fossil. Nonetheless, i'll patch this for consistency's sake.
This was known. Even if it was unnecessary, it now looks nicer in the documentation which is also otherwise quite well maintained.