git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Junio C Hamano <gitster@pobox.com>
Cc: Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org
Subject: Re: [PATCH 4/5] add -p: avoid ambiguous signed/unsigned comparison
Date: Tue, 16 Aug 2022 11:56:02 +0200 (CEST)	[thread overview]
Message-ID: <r1461365-9np6-n988-0288-9ps4o944qqp4@tzk.qr> (raw)
In-Reply-To: <xmqq7d3gm1bl.fsf@gitster.g>

Hi Junio,

On Wed, 10 Aug 2022, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > In the interactive `add` operation, users can choose to jump to specific
> > hunks, and Git will present the hunk list in that case. To avoid showing
> > too many lines at once, only a maximum of 21 hunks are shown, skipping
> > the "mode change" pseudo hunk.
> >
> > The comparison performed to skip the "mode change" pseudo hunk (if any)
> > compares a signed integer `i` to the unsigned value `mode_change` (which
> > can be 0 or 1 because it is a 1-bit type).
> >
> > According to section 6.3.1.8 of the C99 standard (see e.g.
> > https://www.open-std.org/jtc1/sc22/WG14/www/docs/n1256.pdf), what should
> > happen is an automatic conversion of the "lesser" type to the "greater"
> > type, but since the types differ in signedness, it is ill-defined what
> > is the correct "usual arithmetic conversion".
> >
> > Which means that Visual C's behavior can (and does) differ from GCC's:
> > When compiling Git using the latter, `add -p`'s `goto` command shows no
> > hunks by default because it casts a negative start offset to a pretty
> > large unsigned value, breaking the "goto hunk" test case in
> > `t3701-add-interactive.sh`.
> >
> > Let's avoid that by converting the unsigned bit explicitly to a signed
> > integer.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
>
> This looks more like a fix to a general problem, not limited to
> windows or cmake, that we had since 9254bdfb (built-in add -p:
> implement the 'g' ("goto") command, 2019-12-13).
>
> Please pull this out of the series and let's have it reviewed
> separately.

The scope of this patch series is to fix running the tests in Visual
Studio when building using CMake.

Pulling out this patch would break that patch series because it would
leave that breakage in place.

Except if you are asking to put this patch series on the back burner and
prioritize the patch that fixes an ambiguous implicit cast between signed
and unsigned data types?

However, that would mean that I'd now have to address all of those
implicit casts, which is unfortunately a larger amount of work than I can
justify to take on.

Therefore I move that in this instance, the perfect is the enemy of the
good, and that the patch should remain within this patch series, even if
the larger-scoped project to avoid any implicit signed/unsigned casts
remains unaddressed.

BTW I would have expected your review to ask the (in hindsight, obvious)
question why the test suite still passes even with `vs-test` exercising
the code that is compiled using Visual C?

The answer to that would have been that the `vs-test` job of our CI runs
defines `NO_PERL`, and t3701 is skipped completely if that is the case.

Ciao,
Dscho

  reply	other threads:[~2022-08-16 10:40 UTC|newest]

Thread overview: 58+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-08-10 15:02 [PATCH 0/5] Some fixes and an improvement for using CTest on Windows Johannes Schindelin via GitGitGadget
2022-08-10 15:02 ` [PATCH 1/5] cmake: align CTest definition with Git's CI runs Johannes Schindelin via GitGitGadget
2022-08-10 17:48   ` Junio C Hamano
2022-08-16 10:11     ` Johannes Schindelin
2022-08-16 15:15       ` Junio C Hamano
2022-08-19 13:57         ` Johannes Schindelin
2022-08-11 11:18   ` Ævar Arnfjörð Bjarmason
2022-08-10 15:02 ` [PATCH 2/5] cmake: copy the merge tools for testing Johannes Schindelin via GitGitGadget
2022-08-10 15:02 ` [PATCH 3/5] tests: explicitly skip `chmod` calls on Windows Johannes Schindelin via GitGitGadget
2022-08-11 11:22   ` Ævar Arnfjörð Bjarmason
2022-08-22 10:19     ` Johannes Schindelin
2022-08-23  7:34       ` Johannes Schindelin
2022-08-10 15:02 ` [PATCH 4/5] add -p: avoid ambiguous signed/unsigned comparison Johannes Schindelin via GitGitGadget
2022-08-10 17:54   ` Junio C Hamano
2022-08-16  9:56     ` Johannes Schindelin [this message]
2022-08-16 15:10       ` Junio C Hamano
2022-08-19 14:52         ` Johannes Schindelin
2022-08-11 12:49   ` Phillip Wood
2022-08-16 10:00     ` Johannes Schindelin
2022-08-16 14:23       ` Phillip Wood
2022-08-19 14:07         ` Johannes Schindelin
2022-08-10 15:02 ` [PATCH 5/5] cmake: avoid editing t/test-lib.sh Johannes Schindelin via GitGitGadget
2022-08-11 11:35   ` Ævar Arnfjörð Bjarmason
2022-10-18 14:02     ` Johannes Schindelin
2022-08-11 12:58   ` Phillip Wood
2022-08-16 10:09     ` Johannes Schindelin
2022-08-16 14:27       ` Phillip Wood
2022-08-23  8:30 ` [PATCH v2 0/5] Some fixes and an improvement for using CTest on Windows Johannes Schindelin via GitGitGadget
2022-08-23  8:30   ` [PATCH v2 1/5] cmake: make it easier to diagnose regressions in CTest runs Johannes Schindelin via GitGitGadget
2022-09-07 22:10     ` Victoria Dye
2022-10-18 14:02       ` Johannes Schindelin
2022-09-08  7:22     ` Ævar Arnfjörð Bjarmason
2022-09-28  6:55       ` Eric Sunshine
2022-08-23  8:31   ` [PATCH v2 2/5] cmake: copy the merge tools for testing Johannes Schindelin via GitGitGadget
2022-08-23  8:31   ` [PATCH v2 3/5] add -p: avoid ambiguous signed/unsigned comparison Johannes Schindelin via GitGitGadget
2022-08-23  8:31   ` [PATCH v2 4/5] cmake: avoid editing t/test-lib.sh Johannes Schindelin via GitGitGadget
2022-09-08  7:39     ` Ævar Arnfjörð Bjarmason
2022-10-18 14:03       ` Johannes Schindelin
2022-10-18 15:09         ` Ævar Arnfjörð Bjarmason
2022-09-08 23:37     ` Victoria Dye
2022-09-08 23:42       ` Victoria Dye
2022-09-08 23:58       ` Junio C Hamano
2022-10-18 14:03       ` Johannes Schindelin
2022-08-23  8:31   ` [PATCH v2 5/5] cmake: increase time-out for a long-running test Johannes Schindelin via GitGitGadget
2022-09-08  7:34     ` Ævar Arnfjörð Bjarmason
2022-09-08 17:29       ` Victoria Dye
2022-09-08  3:51   ` [PATCH v2 0/5] Some fixes and an improvement for using CTest on Windows Victoria Dye
2022-10-18 10:59   ` [PATCH v3 " Johannes Schindelin via GitGitGadget
2022-10-18 10:59     ` [PATCH v3 1/5] cmake: make it easier to diagnose regressions in CTest runs Johannes Schindelin via GitGitGadget
2022-10-18 13:41       ` Ævar Arnfjörð Bjarmason
2022-10-18 10:59     ` [PATCH v3 2/5] cmake: copy the merge tools for testing Johannes Schindelin via GitGitGadget
2022-10-18 13:49       ` Ævar Arnfjörð Bjarmason
2022-10-18 10:59     ` [PATCH v3 3/5] add -p: avoid ambiguous signed/unsigned comparison Johannes Schindelin via GitGitGadget
2022-10-18 13:53       ` Ævar Arnfjörð Bjarmason
2022-10-18 10:59     ` [PATCH v3 4/5] cmake: avoid editing t/test-lib.sh Johannes Schindelin via GitGitGadget
2022-10-18 13:54       ` Ævar Arnfjörð Bjarmason
2022-10-18 14:21         ` Johannes Schindelin
2022-10-18 10:59     ` [PATCH v3 5/5] cmake: increase time-out for a long-running test Johannes Schindelin via GitGitGadget

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=r1461365-9np6-n988-0288-9ps4o944qqp4@tzk.qr \
    --to=johannes.schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=gitster@pobox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).