threads / bug / 65158

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

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

## tl;dr

14 messages between Mar 6, 2026 and Mar 11, 2026.

replies: 13people: 5as markdown or json

Junio C Hamano· Mar 6, 2026, 23:10 UTC · lore

"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;
Tian Yuchen· Mar 9, 2026, 17:10 UTC · re: Junio C Hamano · lore

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

Hi Junio,
On 3/7/26 07:10, Junio C Hamano wrote:
Show 9 quoted lines
> "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
Tian Yuchen· Mar 9, 2026, 17:27 UTC · re: Junio C Hamano · lore

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

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
D. Ben Knoble· Mar 9, 2026, 22:00 UTC · re: Tian Yuchen · lore

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

On Mon, Mar 9, 2026 at 1:28 PM Tian Yuchen <cat@malon.dev> wrote:
Show 30 quoted lines
>
> 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
Tian Yuchen· Mar 10, 2026, 04:55 UTC · re: D. Ben Knoble · lore

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

Hi Ben,
On 3/10/26 06:00, D. Ben Knoble wrote:
Show 7 quoted lines
> 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
Junio C Hamano· Mar 9, 2026, 22:17 UTC · re: Tian Yuchen · lore

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

Tian Yuchen <cat@malon.dev> writes:
Show 16 quoted lines
> 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.

Tian Yuchen· Mar 10, 2026, 04:51 UTC · re: Junio C Hamano · lore

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

Hi Junio,
Show 6 quoted lines
> 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?

Show 10 quoted lines
> 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
Tian Yuchen· Mar 10, 2026, 05:30 UTC · re: Tian Yuchen · lore

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

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
Oswald Buddenhagen· Mar 10, 2026, 09:15 UTC · re: Tian Yuchen · lore

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

On Tue, Mar 10, 2026 at 01:30:32PM +0800, Tian Yuchen wrote:
Show 7 quoted lines
>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.
Tian Yuchen· Mar 10, 2026, 09:43 UTC · re: Oswald Buddenhagen · lore

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

On 3/10/26 17:15, Oswald Buddenhagen wrote:
Show 9 quoted lines
> 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
Junio C Hamano· Mar 10, 2026, 13:14 UTC · re: Oswald Buddenhagen · lore

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

Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
Show 13 quoted lines
>>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.
Tian Yuchen· Mar 10, 2026, 09:50 UTC · re: Tian Yuchen · lore

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

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
Jean-Noël AVILA· Mar 10, 2026, 17:31 UTC · re: Tian Yuchen · lore

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

On Monday, 9 March 2026 18:27:19 CET Tian Yuchen wrote:
Show 11 quoted lines
> 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>]`
?
Show 8 quoted lines
> 
> 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
Show 7 quoted lines
> --- 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.
Tian Yuchen· Mar 11, 2026, 04:33 UTC · re: Jean-Noël AVILA · lore

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

On 3/11/26 01:31, Jean-Noël AVILA wrote:
Show 5 quoted lines
> 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.
Show 34 quoted lines
>> 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

← back to recent threads