Git Mailing List Archive on lore.kernel.org
 help / color / Atom feed
From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Junio C Hamano <gitster@pobox.com>
Cc: Johannes Sixt <j6t@kdbg.org>,
	Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org
Subject: Re: [PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles
Date: Sun, 1 Dec 2019 10:53:25 +0100 (CET)
Message-ID: <nycvar.QRO.7.76.6.1912011042460.31080@tvgsbejvaqbjf.bet> (raw)
In-Reply-To: <xmqqfti41rz1.fsf@gitster-ct.c.googlers.com>

Hi Junio,

On Sat, 30 Nov 2019, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
>
> > Johannes Sixt <j6t@kdbg.org> writes:
> >
> >> Just like on POSIX the value of errno is indeterminate after a
> >> successful system call, the value of GetLastError() indeterminate after
> >> a successful Windows API call. Therefore, the err_win_to_posix() would
> >> not be able to point at a bogus caller reliably. For this reason, let's
> >> consider the function as a simple error code translator, and then
> >> translating ERROR_SUCCESS to 0 (or is there ESUCCESS?) makes total sense.
> >
> > OK, that makes sense.
>
> Actually, I do not think it makes that much sense, especially in the
> context of this patch that claims to map success to 0 (assuming that
> no E_ANYTHING has the value of zero---I am not sure POSIX gives such
> a guarantee) "for good measure".

But the intention is not to help correct callers! The purpose of mapping
`ERROR_SUCCESS` to 0 will help _only_ callers that try to report an error
when none happened. On Windows, which is the only platform where the
discussed function is compiled and used, `strerror(0)` has the
well-established behavior of returning "No error".

Now imagine that a user encounters a bug where a code path does indeed
incorrectly report an error where no error occurred (or where Git is
looking for the error too late or something like that). You probably agree
with me that we should really avoid reporting "Function not implemented"
in such a case and rather say "No error", which will help figure out the
bug much quicker.

And that is what this patch is all about.

Ciao,
Dscho

> Even if Windows API makes the GetLastError() unusable after a
> success call (unlike POSIX, where errno is left alone), the API
> calls themselves would be signaling their own success or failure,
> right?  So I would have imagined that any kosher caller would be
> doing this:
>
> 	if (SomeWinAPI() != SUCCESS) {
> 		errno = err_win_to_posix(GetLastError());
> 		... possibly other reactions to the error ...
> 	}
>
> If using a value grabbed from GetLastError() after a successfull
> Windows API call is a wrong thing to do as you taught us in your
> message, then an unconditional
>
> 	SomeWinAPI();
> 	errno = err_win_to_posix(GetLastError());
>
> would be wrong anyway, and touching errno unconditionally, when the
> previous call may have succeeded without checking, makes the pattern
> doubly wrong, no?
>
> Having said that, I do not expect myself to be looking into and
> fixing anything in compat/*win*.[ch], so I do not care too deeply
> either way, but I thought that it would help keep the sanity of
> developers involved if we touched errno only upon a failure from an
> underlying system.
>
> Thanks.
>

  reply index

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-29 21:44 [PATCH 0/1] " Johannes Schindelin via GitGitGadget
2019-11-29 21:44 ` [PATCH 1/1] mingw: do set `errno` correctly when trying to restrict handle inheritance Johannes Schindelin via GitGitGadget
2019-11-29 23:02   ` Johannes Sixt
2019-11-30 22:06     ` Johannes Schindelin
2019-11-30 22:16       ` Johannes Sixt
2019-11-30 10:36 ` [PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles Johannes Schindelin via GitGitGadget
2019-11-30 10:36   ` [PATCH v2 1/2] mingw: do set `errno` correctly when trying to restrict handle inheritance Johannes Schindelin via GitGitGadget
2019-11-30 10:36   ` [PATCH v2 2/2] mingw: translate ERROR_SUCCESS to errno = 0 Johannes Schindelin via GitGitGadget
2019-11-30 18:04   ` [PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles Junio C Hamano
2019-11-30 19:13     ` Johannes Sixt
2019-11-30 20:11       ` Andreas Schwab
2019-11-30 20:23         ` Junio C Hamano
2019-11-30 20:43           ` Andreas Schwab
2019-11-30 21:22             ` Johannes Schindelin
2019-11-30 20:21       ` Junio C Hamano
2019-12-01  6:26         ` Junio C Hamano
2019-12-01  9:53           ` Johannes Schindelin [this message]
2019-12-02  6:07             ` Junio C Hamano
2019-12-02  9:05               ` Johannes Schindelin
2019-11-30 19:20   ` Johannes Sixt
2019-11-30 22:09     ` Junio C Hamano
2019-12-02 11:33   ` [PATCH v3 " Johannes Schindelin via GitGitGadget
2019-12-02 11:33     ` [PATCH v3 1/2] mingw: do set `errno` correctly when trying to restrict handle inheritance Johannes Schindelin via GitGitGadget
2019-12-02 11:33     ` [PATCH v3 2/2] mingw: translate ERROR_SUCCESS to errno = 0 Johannes Schindelin via GitGitGadget
2019-12-02 17:35     ` [PATCH v3 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles Johannes Sixt
2019-12-02 19:04       ` Junio C Hamano
2019-12-03 12:04       ` Johannes Schindelin

Reply instructions:

You may reply publically 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=nycvar.QRO.7.76.6.1912011042460.31080@tvgsbejvaqbjf.bet \
    --to=johannes.schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=gitster@pobox.com \
    --cc=j6t@kdbg.org \
    /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

Git Mailing List Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/git/0 git/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 git git/ https://lore.kernel.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.git


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git