git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Cc: Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org
Subject: Re: [PATCH 5/5] tests: explicitly use `git.exe` on Windows
Date: Wed, 14 Nov 2018 23:47:33 +0900	[thread overview]
Message-ID: <xmqqefbnn2be.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <nycvar.QRO.7.76.6.1811141422270.39@tvgsbejvaqbjf.bet> (Johannes Schindelin's message of "Wed, 14 Nov 2018 14:24:45 +0100 (STD)")

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> The latter half of this change is a good one.  Given what the
>> proposed log message of this patch says
>> 
>>     Note also: the many, many calls to `git this` and `git that` are
>>     unaffected, as the regular PATH search will find the `.exe` files on
>>     Windows (and not be confused by a directory of the name `git` that is
>>     in one of the directories listed in the `PATH` variable), while
>>     `/path/to/git` would not, per se, know that it is looking for an
>>     executable and happily prefer such a directory.
>> 
>> which I read as "path-prefixed invocation, i.e. some/path/to/git, is
>> bad, it must be spelled some/path/to/git.exe", I am surprised that
>> tests ever worked on Windows without these five patches, as this
>> line used to read like this:
>> 
>> 	"$GIT_BUILD_DIR/git" >/dev/null
>> 	if test $? != 1
>> 	then
>> 		...
>> 
>> Wouldn't "$GIT_BUILD_DIR/git" have failed (and "executable not
>> found" hopefully won't produce exit code 1) and stopped the test
>> suite from running even after you built git and not under the
>> test-installed-git mode?
>
> "$GIT_BUILD_DIR/git" did not fail until I regularly built with Visual
> Studio (and my Visual Studio project generator generates a directory named
> "git" to live alongside "git.exe").
>
> And when it failed, I patched Git for Windows. Fast-forward, years later I
> managed to contribute the patch we are discussing right now ;-)

OK, I still cannot read it out of what you wrote in the proposed log
message without aid, but in essense the crux of the problem is that
invoking "some/path/to/git" finds "some/path/to/git.exe" only when
there is no "some/path/to/git" directory at the same time, and if
that directory exists, tries and fails to execute that directory,
and the change in this patch protects us from that problem.

Did I get it right?  I'd really prefer to see it more clearly
written in the log message so the next person who reads "git log"
do not have to ask the same question to you.

Assuming that I read it correctly, I think this patch as a whole
makes tons of sense as a change to make the invocation more robust.

Thanks.

  reply	other threads:[~2018-11-14 14:47 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-11-12 13:48 [PATCH 0/5] tests: various improvements to the GIT_TEST_INSTALLED feature Johannes Schindelin via GitGitGadget
2018-11-12 13:48 ` [PATCH 1/5] tests: fix GIT_TEST_INSTALLED's PATH to include t/helper/ Johannes Schindelin via GitGitGadget
2018-11-14  4:53   ` Junio C Hamano
2018-11-12 13:48 ` [PATCH 2/5] tests: respect GIT_TEST_INSTALLED when initializing repositories Johannes Schindelin via GitGitGadget
2018-11-14  4:55   ` Junio C Hamano
2018-11-14 13:16     ` Johannes Schindelin
2018-11-14 14:59       ` Junio C Hamano
2018-11-14 21:38       ` Jeff King
2018-11-15 12:29         ` Johannes Schindelin
2018-11-15 12:41           ` Jeff King
2018-11-12 13:48 ` [PATCH 3/5] t/lib-gettext: test installed git-sh-i18n if GIT_TEST_INSTALLED is set Johannes Schindelin via GitGitGadget
2018-11-14  4:56   ` Junio C Hamano
2018-11-12 13:48 ` [PATCH 4/5] tests: do not require Git to be built when testing an installed Git Johannes Schindelin via GitGitGadget
2018-11-14  5:01   ` Junio C Hamano
2018-11-14 13:20     ` Johannes Schindelin
2018-11-14 12:52   ` Jeff King
2018-11-14 13:41     ` Johannes Schindelin
2018-11-12 13:48 ` [PATCH 5/5] tests: explicitly use `git.exe` on Windows Johannes Schindelin via GitGitGadget
2018-11-14  5:14   ` Junio C Hamano
2018-11-14 13:24     ` Johannes Schindelin
2018-11-14 14:47       ` Junio C Hamano [this message]
2018-11-14 16:32 ` [PATCH v2 0/5] tests: various improvements to the GIT_TEST_INSTALLED feature Johannes Schindelin via GitGitGadget
2018-11-14 16:32   ` [PATCH v2 1/5] tests: fix GIT_TEST_INSTALLED's PATH to include t/helper/ Johannes Schindelin via GitGitGadget
2018-11-14 16:32   ` [PATCH v2 2/5] tests: respect GIT_TEST_INSTALLED when initializing repositories Johannes Schindelin via GitGitGadget
2018-11-14 16:32   ` [PATCH v2 3/5] t/lib-gettext: test installed git-sh-i18n if GIT_TEST_INSTALLED is set Johannes Schindelin via GitGitGadget
2018-11-14 16:32   ` [PATCH v2 4/5] tests: do not require Git to be built when testing an installed Git Johannes Schindelin via GitGitGadget
2018-11-14 16:32   ` [PATCH v2 5/5] tests: explicitly use `git.exe` on Windows 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

  List information: http://vger.kernel.org/majordomo-info.html

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

  git send-email \
    --in-reply-to=xmqqefbnn2be.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.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.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

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 read-only IMAP folder(s) and NNTP newsgroup(s).