Re: [PATCH] format-patch: correct documentation of --thread without an argument

From: Glen Choo <chooglen@google.com>
Date: Apr 3, 2023, 17:48 UTC
Message-ID: <kl6lpm8kzwjz.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <20230403040724.642513-1-alexhenrie24@gmail.com>

Alex Henrie <alexhenrie24@gmail.com> writes:

> In Git, almost all command line flags unconditionally override the
> corresponding config option.[1] Add a test to confirm that this is the
> case for `git format-patch --thread`.

[...]

Show 12 quoted lines

> diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
> index dfcc7da4c2..ed299e077d 100644
> --- a/Documentation/git-format-patch.txt
> +++ b/Documentation/git-format-patch.txt
> @@ -173,8 +173,7 @@ series, where the head is chosen from the cover letter, the
>  threading makes every mail a reply to the previous one.
>  +
>  The default is `--no-thread`, unless the `format.thread` configuration
> -is set.  If `--thread` is specified without a style, it defaults to the
> -style specified by `format.thread` if any, or else `shallow`.
> +is set.  `--thread` without an argument is equivalent to `--thread=shallow`.
>  +

Perhaps more important than the test is that you're fixing blatantly wrong documentation :)

This leads to the questions of:

a) what is the intended behavior is supposed to be: the documented one,
 or the actual one?
b) if it is supposed to be the documented one, when did it break?

AFAICT, this has _always_ been broken since it was introduced way way back in 30984ed2e9 (format-patch: support deep threading, 2009-02-19).

		else if (!strcmp(argv[i], "--thread")
			|| !strcmp(argv[i], "--thread=shallow"))
			thread = THREAD_SHALLOW;
		else if (!strcmp(argv[i], "--thread=deep"))
			thread = THREAD_DEEP;

Given how long this has been broken for without anyone noticing, we should probably just document whatever everyone has been using instead of wondering what the original intended behavior is.

Show 15 quoted lines

>  Beware that the default for 'git send-email' is to thread emails
>  itself.  If you want `git format-patch` to take care of threading, you
> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> index 8c3d06622a..b27a72f78a 100755
> --- a/t/t4014-format-patch.sh
> +++ b/t/t4014-format-patch.sh
> @@ -470,6 +470,11 @@ test_expect_success 'thread' '
>  	check_threading expect.thread --thread main
>  '
>  
> +test_expect_success '--thread overrides format.thread=deep' '
> +	test_config format.thread deep &&
> +	check_threading expect.thread --thread main
> +'
> +

The previous test is entirely in the context lines, and we can see that "check_threading expect.thread --thread main" passes regardless of the value of "format.thread". Nice.

Thanks!

Reviewed-By: Glen Choo <chooglen@google.com>