From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com>,
git@vger.kernel.org
Subject: Re: [PATCH 3/5] tests: explicitly skip `chmod` calls on Windows
Date: Mon, 22 Aug 2022 12:19:24 +0200 (CEST) [thread overview]
Message-ID: <p79ppo86-022r-7sss-41q1-39n499n3266s@tzk.qr> (raw)
In-Reply-To: <220811.86wnbfowbg.gmgdl@evledraar.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 4159 bytes --]
Hi Ævar,
On Thu, 11 Aug 2022, Ævar Arnfjörð Bjarmason wrote:
> On Wed, Aug 10 2022, Johannes Schindelin via GitGitGadget wrote:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> > [...]
> > However, this quirk is only in effect as long as `chmod` is run inside
> > the pseudo Unix root directory structure or within the home directory.
> > When run outside, such invocations fail like this:
> >
> > chmod: changing permissions of '<file>': Invalid argument
>
> ..ok, but...
>
> > diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> > index 6da7273f1d5..7c63b22acab 100644
> > --- a/t/test-lib-functions.sh
> > +++ b/t/test-lib-functions.sh
> > @@ -492,7 +492,10 @@ test_commit_bulk () {
> > # of a file in the working directory and add it to the index.
> >
> > test_chmod () {
> > - chmod "$@" &&
> > + if test_have_prereq !MINGW
> > + then
> > + chmod "$@"
> > + fi &&
> > git update-index --add "--chmod=$@"
> > }
> >
> > @@ -548,7 +551,10 @@ write_script () {
> > echo "#!${2-"$SHELL_PATH"}" &&
> > cat
> > } >"$1" &&
> > - chmod +x "$1"
> > + if test_have_prereq !MINGW
> > + then
> > + chmod +x "$1"
> > + fi
>
> ... you get +x semantics by default, so we didn't need that "chmod +x"
> in the first place?
No. We do not get that `chmod +x` semantics by default. Those `chmod +x`
statements are treated as (expensive) no-ops by default. This is what I
meant when I said this in the commit message (that is missing from the
quoted text above):
[...] it pretends that it succeeded, when in reality it did not
do a thing [...]
I do not know how to say this more clearly.
> The rest of "test_chmod" seems to *happen to* pass +x or -x, but we
> don't care about that, regardless of the "pseudo Unix root directory"?
The rest of "test_chmod" is even quoted above, so we do not need to leave
anybody guessing as to what it does:
git update-index --add "--chmod=$@"
This asks Git to update the index with the executable bit explicitly
turned on or off, regardless of the information that is available on disk.
And yes, Git does what we expect it to do here.
> What if we get a "test_chmod -o <file>", won't this silently do the
> wrong thing?
But we don't?
The code under discussion is Git's test suite, after all, not something
that every Git user is expected to use in more ways than the core Git
developers can imagine.
So even a cursory `git grep test_chmod upstream/seen` could have shown
that there is no such user, and if this was supposed to be a "But what if
we do that at some stage in the future?" feedback, said feedback could be
construed as to intentionally use up valuable contributor time.
> If so isn't something in this direction (untested) a more targeted &
> obvious fix?:
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 10258def7be..1c3b6692388 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -1690,6 +1690,16 @@ case $uname_s in
> find () {
> /usr/bin/find "$@"
> }
> + chmod () {
> + case "$1" in
> + +x|-x)
> + return;
> + ;;
> + *)
> + ;;
> + esac &&
> + /usr/bin/chmod "$@"
> + }
> # git sees Windows-style pwd
> pwd () {
> builtin pwd -W
>
>
In that form, I will reject this suggestion as over-engineered and
convoluted. Why is the `*)` clause empty? Why is there an early return
guarding a single statement? Why is the `/usr/bin/chmod` call not in the
`*)` clause to begin with? Why does this code take pains to handle cases
other than `-x` and `+x` when we do not have any callers, and even in the
experimental patches, there are no such users in sight? I would like to
encourage you to address such issues during review before even sending the
mail in the future.
Having said that, there is a nugget in this feedback that I find valuable.
Instead of wasting the run time (even on non-Windows platforms!) to
determine whether the `MINGW` prereq is set in order to skip the `chmod`
call or not, we can make `chmod` a no-op explicitly in that `case
$uname_s` block.
I will make it so.
Ciao,
Johannes
next prev parent reply other threads:[~2022-08-22 10:19 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 [this message]
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
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=p79ppo86-022r-7sss-41q1-39n499n3266s@tzk.qr \
--to=johannes.schindelin@gmx.de \
--cc=avarab@gmail.com \
--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.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.