An annoying "Bug" that we would probably leave as-is

An annoying "Bug" that we would probably leave as-is

[Junio C Hamano]

"git show -U" does not complain.  In an ideal world, it should say
"-U wants a number", just like "git show -Unan" does.

Unfortunately t/t4013/ actually has tests that break if we start
tightening the command line parser for this.

This falls into the "if it hurts, do not do it" category that
somebody might be taking advantage of out there that we might be
better off leaving them broken.



 diff.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git c/diff.c w/diff.c
index a1961526c0..ff79a41cfc 100644
--- c/diff.c
+++ w/diff.c
@@ -5581,15 +5581,14 @@ static int diff_opt_unified(const struct option *opt,
 			    const char *arg, int unset)
 {
 	struct diff_options *options = opt->value;
-	char *s;
+	char *s = NULL;
 
 	BUG_ON_OPT_NEG(unset);
 
-	if (arg) {
+	if (arg)
 		options->context = strtol(arg, &s, 10);
-		if (*s)
-			return error(_("%s expects a numerical value"), "--unified");
-	}
+	if (!s || *s)
+		return error(_("%s expects a numerical value"), "--unified");
 	enable_patch_output(&options->output_format);
 
 	return 0;

Re: An annoying "Bug" that we would probably leave as-is

[Tian Yuchen]

Hi Junio,

On 3/7/26 07:10, Junio C Hamano wrote:
> "git show -U" does not complain.  In an ideal world, it should say
> "-U wants a number", just like "git show -Unan" does.
> 
> Unfortunately t/t4013/ actually has tests that break if we start
> tightening the command line parser for this.
> 
> This falls into the "if it hurts, do not do it" category that
> somebody might be taking advantage of out there that we might be
> better off leaving them broken.

Out of curiosity, I applied your patch and ran t4013 to see exactly what 
would break. It turns out it's 'not ok 157 - git diff -U initial..side'.

The suite explicitly relies on 'git diff -U' (without a number) to
implicitly default to 3 lines of context, behaving identically to '-u'.
With your patch, it correctly but fatally throws 'error: --unified
expects a numerical value' instead of generating the diff, causing the
test to fail.

I looked at diff-context-options.adoc, and it says:

`-U<n>`::
`--unified=<n>`::
	Generate diffs with _<n>_ lines of context. Defaults to `diff.context`
	or 3 if the config option is unset.

I believe this description needs revision, as in actual practice (as you 
described), the number following '-U' is not strictly required.

(I was taken aback, especially when you mentioned “somebody might be 
taking advantage of out there.” It made me realize how an inadvertent 
little bug or minor error can become a daily habit for many people, or 
even foundations of millions of scripts. Just a thought. ;)

Regards,

Yuchen

[PATCH v1] diff: document -U without <n> as using default context

[Tian Yuchen]

The documentation for '-U<n>' implies that the numeric value '<n>' is
mandatory. However, the command line parser has historically accepted
'-U' without a number.

Strictly requiring a number for '-U' would break existing tests
(e.g., in 't4013') and likely disrupt user scripts relying on this
undocumented behavior.

Since we are retaining this fallback behavior for backward compatibility,
update the documentation to explicitly state that '<n>' can be omitted
for the short option '-U'.

Signed-off-by: Tian Yuchen <cat@malon.dev>
---
 Documentation/diff-context-options.adoc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/diff-context-options.adoc b/Documentation/diff-context-options.adoc
index e161260358..655496ec3a 100644
--- a/Documentation/diff-context-options.adoc
+++ b/Documentation/diff-context-options.adoc
@@ -1,4 +1,4 @@
-`-U<n>`::
+`-U[<n>]`::
 `--unified=<n>`::
 	Generate diffs with _<n>_ lines of context. Defaults to `diff.context`
 	or 3 if the config option is unset.
-- 
2.43.0

Re: [PATCH v1] diff: document -U without <n> as using default context

[D. Ben Knoble]

On Mon, Mar 9, 2026 at 1:28 PM Tian Yuchen <cat@malon.dev> wrote:
>
> The documentation for '-U<n>' implies that the numeric value '<n>' is
> mandatory. However, the command line parser has historically accepted
> '-U' without a number.
>
> Strictly requiring a number for '-U' would break existing tests
> (e.g., in 't4013') and likely disrupt user scripts relying on this
> undocumented behavior.
>
> Since we are retaining this fallback behavior for backward compatibility,
> update the documentation to explicitly state that '<n>' can be omitted
> for the short option '-U'.
>
> Signed-off-by: Tian Yuchen <cat@malon.dev>
> ---
>  Documentation/diff-context-options.adoc | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/diff-context-options.adoc b/Documentation/diff-context-options.adoc
> index e161260358..655496ec3a 100644
> --- a/Documentation/diff-context-options.adoc
> +++ b/Documentation/diff-context-options.adoc
> @@ -1,4 +1,4 @@
> -`-U<n>`::
> +`-U[<n>]`::
>  `--unified=<n>`::
>         Generate diffs with _<n>_ lines of context. Defaults to `diff.context`
>         or 3 if the config option is unset.
> --
> 2.43.0

I was curious about the way we indicate this kind of optionality for
single-letter options, so:

    git grep -e '-[[:alnum:]]\[' Documentation

which finds many hits of this pattern. Cool. Adding -A1, we see that
it is also common to document the long-form like

    --unified[=<n>]

which you may want to add to this patch. (I haven't really considered
the rest of it very well, although it does seem worth updating the
syntax to match what some of our tests exercise.)

Which makes me notice: 3 is the default if the config option is unset
_or_ if <n> is not provided. Is there a better wording to indicate
that? Maybe the simplest tweak is to clarify that <n> is the thing
which defaults to… (since a first read might leave the reader
wondering "what defaults to diff.context or 3? ah, it's probably n…").
But a glance at other docs makes this pattern seem common while some
do say "if <n> is specified…", so idk.

BTW I noticed these docs are duplicated between
Documentation/diff-{,context-}options.adoc

-- 
D. Ben Knoble

Re: [PATCH v1] diff: document -U without <n> as using default context

[Junio C Hamano]

Tian Yuchen <cat@malon.dev> writes:

> The documentation for '-U<n>' implies that the numeric value '<n>' is
> mandatory. However, the command line parser has historically accepted
> '-U' without a number.
>
> Strictly requiring a number for '-U' would break existing tests
> (e.g., in 't4013') and likely disrupt user scripts relying on this
> undocumented behavior.
>
> Since we are retaining this fallback behavior for backward compatibility,
> update the documentation to explicitly state that '<n>' can be omitted
> for the short option '-U'.
>
> Signed-off-by: Tian Yuchen <cat@malon.dev>
> ---
>  Documentation/diff-context-options.adoc | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

I am moderately nagative.

It is not like we are _encouraging_ users to omit <n> from -U<n>,
but it is not errored out only due to a bug.  Who would the new text
help?  Users would wonder why <n> is not optional in --unified=<n>,
the other way to spell the same thing.

If we want to be explicit, we should probably do this instead:

    `-U<n>`::
    `--unified=<n>`::
            Generate diffs with _<n>_ lines of context. Defaults to `diff.context`
            or 3 if the config option is unset (`-U` without '<n>' is accepted
            as a silent synonym for `-p` due to a historical accident).

which would tell readers what happens when '<n>' is omitted and why
we allow such an inconsistency.

Re: [PATCH v1] diff: document -U without <n> as using default context

[Tian Yuchen]

Hi Junio,

> I am moderately nagative.
> 
> It is not like we are _encouraging_ users to omit <n> from -U<n>,
> but it is not errored out only due to a bug.  Who would the new text
> help?  Users would wonder why <n> is not optional in --unified=<n>,
> the other way to spell the same thing.

Indeed.

What I was actually thinking earlier was: for bugs like this, which most 
likely come from misoperations, typos, or inconsistencies from *ancient* 
standards, should we acknowledge them as

“yes it is a bug/historical issue”

or

“*special behavior* we defined”?

I'm not as familiar with the Git codebase as you are, so I couldn't make 
a definitive call. I chose the second approach, but I now agree the 
first makes more sense. In other words, users shouldn't be *encouraged* 
to engage in such behavior, right?

> If we want to be explicit, we should probably do this instead:
> 
>      `-U<n>`::
>      `--unified=<n>`::
>              Generate diffs with _<n>_ lines of context. Defaults to `diff.context`
>              or 3 if the config option is unset (`-U` without '<n>' is accepted
>              as a silent synonym for `-p` due to a historical accident).
> 
> which would tell readers what happens when '<n>' is omitted and why
> we allow such an inconsistency.

That makes sense. I'll make the changes accordingly.

Regards,

Yuchen

Re: [PATCH v1] diff: document -U without <n> as using default context

[Tian Yuchen]

Hi Ben,

On 3/10/26 06:00, D. Ben Knoble wrote:

> Which makes me notice: 3 is the default if the config option is unset
> _or_ if <n> is not provided. Is there a better wording to indicate
> that? Maybe the simplest tweak is to clarify that <n> is the thing
> which defaults to… (since a first read might leave the reader
> wondering "what defaults to diff.context or 3? ah, it's probably n…").
> But a glance at other docs makes this pattern seem common while some
> do say "if <n> is specified…", so idk.

Indeed, the current wording isn't clear enough. I'll make corrections 
wherever it's needed. I think Junio provides a good template that's 
great with just a few tweaks ;)

Thanks for the review!

Yuchen

[PATCH v2] diff: document -U without <n> as using default context

[Tian Yuchen]

The documentation for '-U<n>' implies that the numeric value '<n>' is
mandatory. However, the command line parser has historically accepted
'-U' without a number.

Strictly requiring a number for '-U' would break existing tests
(e.g., in 't4013') and likely disrupt user scripts relying on this
undocumented behavior.

Since we are retaining this fallback behavior for backward compatibility,
update the documentation to explicitly state that '<n>' can be omitted
for the short option '-U'.

Signed-off-by: Tian Yuchen <cat@malon.dev>
---
 Documentation/diff-context-options.adoc | 6 ++++--
 Documentation/diff-options.adoc         | 6 ++++--
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/Documentation/diff-context-options.adoc b/Documentation/diff-context-options.adoc
index e161260358..42d29a0c00 100644
--- a/Documentation/diff-context-options.adoc
+++ b/Documentation/diff-context-options.adoc
@@ -1,7 +1,9 @@
 `-U<n>`::
 `--unified=<n>`::
-	Generate diffs with _<n>_ lines of context. Defaults to `diff.context`
-	or 3 if the config option is unset.
+	Generate diffs with _<n>_ lines of context. The number of context
+	lines defaults to `diff.context` or 3 if the config option is unset.
+	(`-U` without `<n>` is accepted as a silent synonym for `-p` due
+	to a historical accident).
 
 `--inter-hunk-context=<n>`::
 	Show the context between diff hunks, up to the specified _<number>_
diff --git a/Documentation/diff-options.adoc b/Documentation/diff-options.adoc
index 9cdad6f72a..40502639a2 100644
--- a/Documentation/diff-options.adoc
+++ b/Documentation/diff-options.adoc
@@ -127,8 +127,10 @@ endif::git-log[]
 
 `-U<n>`::
 `--unified=<n>`::
-	Generate diffs with _<n>_ lines of context instead of
-	the usual three.
+	Generate diffs with _<n>_ lines of context. The number of context
+	lines defaults to `diff.context` or 3 if the config option is unset.
+	(`-U` without `<n>` is accepted as a silent synonym for `-p` due
+	to a historical accident).	
 ifndef::git-format-patch[]
 	Implies `--patch`.
 endif::git-format-patch[]
-- 
2.43.0

Re: [PATCH v2] diff: document -U without <n> as using default context

[Oswald Buddenhagen]

On Tue, Mar 10, 2026 at 01:30:32PM +0800, Tian Yuchen wrote:
>The documentation for '-U<n>' implies that the numeric value '<n>' is
>mandatory. However, the command line parser has historically accepted
>'-U' without a number.
>
>Strictly requiring a number for '-U' would break existing tests
>(e.g., in 't4013') and likely disrupt user scripts relying on this
>undocumented behavior.


>Since we are retaining this fallback behavior for backward compatibility,
>update the documentation to explicitly state that '<n>' can be omitted
>for the short option '-U'.
>
i'd replace that with:

Hence we retain this fallback behavior for backward compatibility, but 
document it as such.

>+	(`-U` without `<n>` is accepted as a silent synonym for `-p` due
>+	to a historical accident).
> 
"silently accepted as a synonym" would be much more natural.

Re: [PATCH v2] diff: document -U without <n> as using default context

[Tian Yuchen]

On 3/10/26 17:15, Oswald Buddenhagen wrote:

> i'd replace that with:
> 
> Hence we retain this fallback behavior for backward compatibility, but 
> document it as such.
> 
>> +    (`-U` without `<n>` is accepted as a silent synonym for `-p` due
>> +    to a historical accident).
>>
> "silently accepted as a synonym" would be much more natural.

Thank you for pointing out. This change does make the message read more 
smoothly.

Will send v3 shortly.

Yuchen

[PATCH v3] diff: document -U without <n> as using default context

[Tian Yuchen]

The documentation for '-U<n>' implies that the numeric value '<n>' is
mandatory. However, the command line parser has historically accepted
'-U' without a number.

Strictly requiring a number for '-U' would break existing tests
(e.g., in 't4013') and likely disrupt user scripts relying on this
undocumented behavior.

Hence we retain this fallback behavior for backward compatibility, but
document it as such.

Signed-off-by: Tian Yuchen <cat@malon.dev>
---
 Documentation/diff-context-options.adoc | 6 ++++--
 Documentation/diff-options.adoc         | 6 ++++--
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/Documentation/diff-context-options.adoc b/Documentation/diff-context-options.adoc
index e161260358..306abfd9ab 100644
--- a/Documentation/diff-context-options.adoc
+++ b/Documentation/diff-context-options.adoc
@@ -1,7 +1,9 @@
 `-U<n>`::
 `--unified=<n>`::
-	Generate diffs with _<n>_ lines of context. Defaults to `diff.context`
-	or 3 if the config option is unset.
+	Generate diffs with _<n>_ lines of context. The number of context
+	lines defaults to `diff.context` or 3 if the config option is unset.
+	(`-U` without `<n>` is silently accepted as a synonym for `-p` due
+	to a historical accident).
 
 `--inter-hunk-context=<n>`::
 	Show the context between diff hunks, up to the specified _<number>_
diff --git a/Documentation/diff-options.adoc b/Documentation/diff-options.adoc
index 9cdad6f72a..e1f20bedf6 100644
--- a/Documentation/diff-options.adoc
+++ b/Documentation/diff-options.adoc
@@ -127,8 +127,10 @@ endif::git-log[]
 
 `-U<n>`::
 `--unified=<n>`::
-	Generate diffs with _<n>_ lines of context instead of
-	the usual three.
+	Generate diffs with _<n>_ lines of context. The number of context
+	lines defaults to `diff.context` or 3 if the config option is unset.
+	(`-U` without `<n>` is silently accepted as a synonym for `-p` due
+	to a historical accident).	
 ifndef::git-format-patch[]
 	Implies `--patch`.
 endif::git-format-patch[]
-- 
2.43.0

Re: [PATCH v2] diff: document -U without <n> as using default context

[Junio C Hamano]

Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:

>>Since we are retaining this fallback behavior for backward compatibility,
>>update the documentation to explicitly state that '<n>' can be omitted
>>for the short option '-U'.
>>
> i'd replace that with:
>
> Hence we retain this fallback behavior for backward compatibility, but 
> document it as such.
>
>>+	(`-U` without `<n>` is accepted as a silent synonym for `-p` due
>>+	to a historical accident).
>> 
> "silently accepted as a synonym" would be much more natural.

An excellent suggestion.  Thanks.

Re: [PATCH v1] diff: document -U without <n> as using default context

[Jean-Noël AVILA]

On Monday, 9 March 2026 18:27:19 CET Tian Yuchen wrote:
> The documentation for '-U<n>' implies that the numeric value '<n>' is
> mandatory. However, the command line parser has historically accepted
> '-U' without a number.
> 
> Strictly requiring a number for '-U' would break existing tests
> (e.g., in 't4013') and likely disrupt user scripts relying on this
> undocumented behavior.
> 
> Since we are retaining this fallback behavior for backward compatibility,
> update the documentation to explicitly state that '<n>' can be omitted
> for the short option '-U'.

In this case, isn't the long option also changed to optional number, such as:

`--unified[=<n>]`

?

> 
> Signed-off-by: Tian Yuchen <cat@malon.dev>
> ---
>  Documentation/diff-context-options.adoc | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/diff-context-options.adoc
> b/Documentation/diff-context-options.adoc index e161260358..655496ec3a 
100644
> --- a/Documentation/diff-context-options.adoc
> +++ b/Documentation/diff-context-options.adoc
> @@ -1,4 +1,4 @@
> -`-U<n>`::
> +`-U[<n>]`::
>  `--unified=<n>`::
>  	Generate diffs with _<n>_ lines of context. Defaults to 
`diff.context`
>  	or 3 if the config option is unset.

Re: [PATCH v1] diff: document -U without <n> as using default context

[Tian Yuchen]

On 3/11/26 01:31, Jean-Noël AVILA wrote:

> In this case, isn't the long option also changed to optional number, such as:
> 
> `--unified[=<n>]`
> 
> ?

Indeed. Your statement aligns with what Junio said earlier.

>> The documentation for '-U<n>' implies that the numeric value '<n>' is
>> mandatory. However, the command line parser has historically accepted
>> '-U' without a number.
>>
>> Strictly requiring a number for '-U' would break existing tests
>> (e.g., in 't4013') and likely disrupt user scripts relying on this
>> undocumented behavior.
>>
>> Since we are retaining this fallback behavior for backward compatibility,
>> update the documentation to explicitly state that '<n>' can be omitted
>> for the short option '-U'.
>>
>> Signed-off-by: Tian Yuchen <cat@malon.dev>
>> ---
>>  Documentation/diff-context-options.adoc | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> I am moderately nagative.
> 
> It is not like we are _encouraging_ users to omit <n> from -U<n>,
> but it is not errored out only due to a bug.  Who would the new text
> help?  Users would wonder why <n> is not optional in --unified=<n>,
> the other way to spell the same thing.
> 
> If we want to be explicit, we should probably do this instead:
> 
>     `-U<n>`::
>     `--unified=<n>`::
>             Generate diffs with _<n>_ lines of context. Defaults to `diff.context`
>             or 3 if the config option is unset (`-U` without '<n>' is accepted
>             as a silent synonym for `-p` due to a historical accident).
> 
> which would tell readers what happens when '<n>' is omitted and why
> we allow such an inconsistency.

I have already fixed this in the v3 patch.

Thank you,

Yuchen