Re: [PATCH v2 27/29] pack-objects: fix buggy warning about threads
- From
Junio C Hamano <gitster@pobox.com>
- Date
- May 15, 2017, 08:59 UTC
- Message-ID
- <xmqqfug6v6l3.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20170513231509.7834-28-avarab@gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 46 quoted lines
> 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?