Re: [PATCH v2 27/29] pack-objects: fix buggy warning about threads

[Junio C Hamano <gitster@pobox.com>]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Fix a buggy warning about threads under NO_PTHREADS=YesPlease. Due to
> re-using the delta_search_threads variable for both the state of the
> "pack.threads" config & the --threads option, setting "pack.threads"
> but not supplying --threads would trigger the warning for both
> "pack.threads" & --threads.
>
> Solve this bug by resetting the delta_search_threads variable in
> git_pack_config(), it might then be set by --threads again and be
> subsequently warned about, as the test I'm changing here asserts.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  builtin/pack-objects.c | 4 +++-
>  t/t5300-pack-object.sh | 3 +--
>  2 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 0fe35d1b5a..f1baf05dfe 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -2472,8 +2472,10 @@ static int git_pack_config(const char *k, const char *v, void *cb)
>  			die("invalid number of threads specified (%d)",
>  			    delta_search_threads);
>  #ifdef NO_PTHREADS
> -		if (delta_search_threads != 1)
> +		if (delta_search_threads != 1) {
>  			warning("no threads support, ignoring %s", k);
> +			delta_search_threads = 0;
> +		}
>  #endif
>  		return 0;
>  	}
> diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh
> index 1629fa80b0..0ec25c4966 100755
> --- a/t/t5300-pack-object.sh
> +++ b/t/t5300-pack-object.sh
> @@ -445,8 +445,7 @@ test_expect_success !PTHREADS,C_LOCALE_OUTPUT 'pack-objects --threads=N or pack.
>  	git -c pack.threads=2 pack-objects --stdout --all </dev/null >/dev/null 2>err &&
>  	cat err &&
>  	grep ^warning: err >warnings &&
> -	test_line_count = 2 warnings &&
> -	grep -F "no threads support, ignoring --threads" err &&
> +	test_line_count = 1 warnings &&
>  	grep -F "no threads support, ignoring pack.threads" err &&
>  	git -c pack.threads=2 pack-objects --threads=4 --stdout --all </dev/null >/dev/null 2>err &&
>  	grep ^warning: err >warnings &&

Commenting on both 26 and 27.

The usual way we document a known breakage to be fixed is to write a
test that checks for the desired outcome with test_expect_failure,
and when a patch corrects the behaviour we just flip it to expect
success instead.  On the other hand, when we document a behaviour
that is accepted/acceptable we would have a test that checks for the
then-accepted behaviour with test_expect_success, and a patch that
improves the behaviour would update the expectation.

This follows the second pattern, even though the log message for the
patches claim this is about an existing bug and its fix.

Now, I am not saying (at least not yet) that 26 & 27 violates the
established practice and they must be changed to expect seeing only
one line of output in warnings with test_expect_failure in patch 26
which is flipped to test_expect_success in patch 27.  Yes, it does
not follow the usual pattern, but it gives a good food-for-thought.

Perhaps our usual pattern may be suboptimal in illustrating in what
way the current behaviour is not desirable, and the way these two
patches document the current breakage and then documents the fixed
behaviour may be a better example to follow?  With our usual way, it
is not apparent until you actually run the tests with the current
code what the questionable current behaviour is, but with the way
patch 26 is written, we can tell that two warnings are given,
instead of one.

I dunno.  What do others think?