Re: What's cooking in git.git (Jan 2017, #04; Mon, 23)
Lars Schneider writes: > The difference between Travis and my machine is that I changed the > default shell to ZSH with a few plugins [1]. If I run the test with > plain BASH on my Mac then I can reproduce the test failure. Therefore, > we might want to adjust the commit message if anyone else can reproduce > the problem on a Mac. With "Reportedly macOS does this, at least in the Travis builds.", even with the result from you in your follow-up message to the message I am responding to, I think what Peff wrote in the log message is good enough. Thanks for digging, and thanks Peff for coming up the workaround.
Re: What's cooking in git.git (Jan 2017, #04; Mon, 23)
On Thu, Jan 26, 2017 at 10:48:30AM +0100, Lars Schneider wrote: > Oh. I must have made a mistake on my very first test run. I can reproduce > the failure with ZSH and my plugins... looks like it's a Mac OS problem > and no TravisCI only problem after all. Thanks for digging into it. If it's really /bin/mv that causes the problem, then I doubly think the "mv -f" patch is the right fix. -Peff
Re: What's cooking in git.git (Jan 2017, #04; Mon, 23)
> On 26 Jan 2017, at 10:14, Lars Schneider wrote: > > >> On 25 Jan 2017, at 23:51, Junio C Hamano wrote: >> >> Jeff King writes: >> >>> I guess the way to dig would be to add a test that looks at the output >>> of "type mv" or something, push it to a Travis-hooked branch, and then >>> wait for the output >> >> Sounds tempting ;-) > > Well, I tried that: > > mv is /bin/mv > > ... and "/bin/mv" is exactly the version that I have on my machine. > > The difference between Travis and my machine is that I changed the > default shell to ZSH with a few plugins [1]. If I run the test with > plain BASH on my Mac then I can reproduce the test failure. Therefore, > we might want to adjust the commit message if anyone else can reproduce > the problem on a Mac. > > I can even reproduce the failure if I run the test with plain ZSH. > However, I can't find a plugin that defines an alias for "mv". Puzzled... > > - Lars > > [1] https://github.com/robbyrussell/oh-my-zsh Oh. I must have made a mistake on my very first test run. I can reproduce the failure with ZSH and my plugins... looks like it's a Mac OS problem and no TravisCI only problem after all. Sorry for the noise/confusion, Lars
Re: What's cooking in git.git (Jan 2017, #04; Mon, 23)
> On 25 Jan 2017, at 23:51, Junio C Hamano wrote: > > Jeff King writes: > >> I guess the way to dig would be to add a test that looks at the output >> of "type mv" or something, push it to a Travis-hooked branch, and then >> wait for the output > > Sounds tempting ;-) Well, I tried that: mv is /bin/mv ... and "/bin/mv" is exactly the version that I have on my machine. The difference between Travis and my machine is that I changed the default shell to ZSH with a few plugins [1]. If I run the test with plain BASH on my Mac then I can reproduce the test failure. Therefore, we might want to adjust the commit message if anyone else can reproduce the problem on a Mac. I can even reproduce the failure if I run the test with plain ZSH. However, I can't find a plugin that defines an alias for "mv". Puzzled... - Lars [1] https://github.com/robbyrussell/oh-my-zsh
Re: What's cooking in git.git (Jan 2017, #04; Mon, 23)
Jeff King writes: > I guess the way to dig would be to add a test that looks at the output > of "type mv" or something, push it to a Travis-hooked branch, and then > wait for the output Sounds tempting ;-)
Re: What's cooking in git.git (Jan 2017, #04; Mon, 23)
Stefan Beller writes: > On Mon, Jan 23, 2017 at 4:18 PM, Junio C Hamano wrote: >> >> * sb/unpack-trees-super-prefix (2017-01-12) 5 commits >> - SQUASH >> - unpack-trees: support super-prefix option >> - t1001: modernize style >> - t1000: modernize style >> - read-tree: use OPT_BOOL instead of OPT_SET_INT >> >> "git read-tree" and its underlying unpack_trees() machinery learned >> to report problematic paths prefixed with the --super-prefix option. >> >> Expecting a reroll. >> The first three are in good shape. The last one needs a better >> explanation and possibly an update to its test. >> cf. >> > > Please see > https://public-inbox.org/git/20170118010520.8804-1-sbel...@google.com/ Thanks, applied. Let's move this forward.
Re: What's cooking in git.git (Jan 2017, #04; Mon, 23)
On Wed, Jan 25, 2017 at 10:16:40AM -0800, Junio C Hamano wrote: > > But whatever the cause, I think the workaround I posted is > > easy enough to do. > > Or spelling it explicitly as "/bin/mv" (forgetting systems that does > not have it in /bin but as /usr/bin/mv) would also defeat alias if > that were the cause. Yes, but I think it's less tricky and unportable to write "mv -f" than "/bin/mv". So even if it _is_ a funny alias thing, I think my patch is the right fix. > One downside of working it around like your patch does, or spelling > it out as "/bin/mv", is that we'd need to worry about all the uses > of "mv" in our scripts. If this were _only_ happening in the Travis > environment, I'd prefer to see why it happens only there and fix that > instead. I would be curious to know whether it is a funny thing in the Travis environment, or if some version of macOS "mv" really is that braindead (and it is just the case that Travis has that version and Lars's computer doesn't). I just didn't want to waste anybody's time digging into it if it won't affect our patch. I guess the way to dig would be to add a test that looks at the output of "type mv" or something, push it to a Travis-hooked branch, and then wait for the output -Peff
Re: What's cooking in git.git (Jan 2017, #04; Mon, 23)
On Mon, Jan 23, 2017 at 4:18 PM, Junio C Hamano wrote: > > * sb/unpack-trees-super-prefix (2017-01-12) 5 commits > - SQUASH > - unpack-trees: support super-prefix option > - t1001: modernize style > - t1000: modernize style > - read-tree: use OPT_BOOL instead of OPT_SET_INT > > "git read-tree" and its underlying unpack_trees() machinery learned > to report problematic paths prefixed with the --super-prefix option. > > Expecting a reroll. > The first three are in good shape. The last one needs a better > explanation and possibly an update to its test. > cf. > Please see https://public-inbox.org/git/20170118010520.8804-1-sbel...@google.com/ > > * sb/submodule-doc (2017-01-12) 3 commits > - submodules: add a background story > - submodule update documentation: don't repeat ourselves > - submodule documentation: add options to the subcommand > > Needs review. The first 2 commit are asking for the standard review, whereas the last patch (submodules: add a background story), needs more of a design review/opinion if such a patch is a good idea actually. Thanks, Stefan
Re: What's cooking in git.git (Jan 2017, #04; Mon, 23)
Jeff King writes: > On Wed, Jan 25, 2017 at 06:01:11PM +0100, Johannes Schindelin wrote: > >> > Looks like "mv" prompts and then fails to move the file (so we get the >> > dangling blob for the source blob, and fsck doesn't report failure >> > because we didn't actually corrupt the destination blob). >> >> IIRC I had similar problems years ago, on a machine where the >> administrator defined mandatory aliases, including mv="mv -i". > > Yeah, that was my first thought, too. But this should be a > non-interactive shell, which would generally avoid loading rc files. I > think there are some exceptions, though (e.g., setting ENV or BASH_ENV). > Loading aliases like "mv -i" for non-interactive shells seems somewhat > insane to me. It does to me, too. > But whatever the cause, I think the workaround I posted is > easy enough to do. Or spelling it explicitly as "/bin/mv" (forgetting systems that does not have it in /bin but as /usr/bin/mv) would also defeat alias if that were the cause. One downside of working it around like your patch does, or spelling it out as "/bin/mv", is that we'd need to worry about all the uses of "mv" in our scripts. If this were _only_ happening in the Travis environment, I'd prefer to see why it happens only there and fix that instead.
Re: What's cooking in git.git (Jan 2017, #04; Mon, 23)
On Wed, Jan 25, 2017 at 06:01:11PM +0100, Johannes Schindelin wrote: > > Looks like "mv" prompts and then fails to move the file (so we get the > > dangling blob for the source blob, and fsck doesn't report failure > > because we didn't actually corrupt the destination blob). > > IIRC I had similar problems years ago, on a machine where the > administrator defined mandatory aliases, including mv="mv -i". Yeah, that was my first thought, too. But this should be a non-interactive shell, which would generally avoid loading rc files. I think there are some exceptions, though (e.g., setting ENV or BASH_ENV). Loading aliases like "mv -i" for non-interactive shells seems somewhat insane to me. But whatever the cause, I think the workaround I posted is easy enough to do. -Peff
Re: What's cooking in git.git (Jan 2017, #04; Mon, 23)
Hi Peff, On Tue, 24 Jan 2017, Jeff King wrote: > On Tue, Jan 24, 2017 at 11:04:21AM +0100, Lars Schneider wrote: > > > "fsck: prepare dummy objects for --connectivity-check" seems to > > make t1450-fsck.sh fail on macOS with TravisCI: > > > > Initialized empty Git repository in /Users/travis/build/git/git/t/trash > > directory.t1450-fsck/connectivity-only/.git/ > > [master (root-commit) 86520b7] empty > > Author: A U Thor > > 2 files changed, 1 insertion(+) > > create mode 100644 empty > > create mode 100644 empty.t > > override r--r--r-- travis/staff for > > .git/objects/e6/9de29bb2d1d6434b8b29ae775ad8c2e48c5391? (y/n [n]) not > > overwritten > > dangling blob c21c9352f7526e9576892a6631e0e8cf1fccd34d > > Looks like "mv" prompts and then fails to move the file (so we get the > dangling blob for the source blob, and fsck doesn't report failure > because we didn't actually corrupt the destination blob). IIRC I had similar problems years ago, on a machine where the administrator defined mandatory aliases, including mv="mv -i". Ciao, Johannes
Re: What's cooking in git.git (Jan 2017, #04; Mon, 23)
> On 24 Jan 2017, at 14:27, Jeff King wrote: > > On Tue, Jan 24, 2017 at 11:04:21AM +0100, Lars Schneider wrote: > >> "fsck: prepare dummy objects for --connectivity-check" seems to >> make t1450-fsck.sh fail on macOS with TravisCI: >> >> Initialized empty Git repository in /Users/travis/build/git/git/t/trash >> directory.t1450-fsck/connectivity-only/.git/ >> [master (root-commit) 86520b7] empty >> Author: A U Thor >> 2 files changed, 1 insertion(+) >> create mode 100644 empty >> create mode 100644 empty.t >> override r--r--r-- travis/staff for >> .git/objects/e6/9de29bb2d1d6434b8b29ae775ad8c2e48c5391? (y/n [n]) not >> overwritten >> dangling blob c21c9352f7526e9576892a6631e0e8cf1fccd34d > > Looks like "mv" prompts and then fails to move the file (so we get the > dangling blob for the source blob, and fsck doesn't report failure because > we didn't actually corrupt the destination blob). > > If I'm understanding the behavior correctly, this violates POSIX, which > says: > > 1. If the destination path exists, the −f option is not specified, and > either of the following conditions is true: > > a. The permissions of the destination path do not permit writing > and the standard input is a terminal. > > b. The −i option is specified. > > the mv utility shall write a prompt to standard error and read a > line from standard input. If the response is not affirmative, > mv shall do nothing more with the current source_file and go on > to any remaining source_files. > > Our stdin isn't a terminal, and we do not specify "-i", so I think it > shouldn't prompt. It also exits with code 0 after failing to move, > which is another surprise. > > Here's a patch (tested by me that it works on Linux, but I don't know > for sure that it fixes the Travis problem). > > -- >8 -- > Subject: [PATCH] t1450: use "mv -f" within loose object directory > > The loose objects are created with mode 0444. That doesn't > prevent them being overwritten by rename(), but some > versions of "mv" will be extra careful and prompt the user, > even without "-i". > > Reportedly macOS does this, at least in the Travis builds. > The prompt reads from /dev/null, defaulting to "no", and the > object isn't moved. Then to make matters even more > interesting, it still returns "0" and the rest of the test > proceeds, but with a broken setup. > > We can work around it by using "mv -f" to override the > prompt. This should work as it's already used in t5504 for > the same purpose. > > Signed-off-by: Jeff King > --- > t/t1450-fsck.sh | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh > index efaf41b68..33a51c9a6 100755 > --- a/t/t1450-fsck.sh > +++ b/t/t1450-fsck.sh > @@ -536,7 +536,7 @@ test_expect_success 'fsck --connectivity-only' ' > # free to examine the type if it chooses. > empty=.git/objects/e6/9de29bb2d1d6434b8b29ae775ad8c2e48c5391 && > blob=$(echo unrelated | git hash-object -w --stdin) && > - mv $(sha1_file $blob) $empty && > + mv -f $(sha1_file $blob) $empty && > > test_must_fail git fsck --strict && > git fsck --strict --connectivity-only && > -- > 2.11.0.840.gd37c5973a Ack. This patch fixes the issue: https://travis-ci.org/larsxschneider/git/builds/194819605 Thanks, Lars
Re: What's cooking in git.git (Jan 2017, #04; Mon, 23)
On Tue, Jan 24, 2017 at 11:04:21AM +0100, Lars Schneider wrote: > "fsck: prepare dummy objects for --connectivity-check" seems to > make t1450-fsck.sh fail on macOS with TravisCI: > > Initialized empty Git repository in /Users/travis/build/git/git/t/trash > directory.t1450-fsck/connectivity-only/.git/ > [master (root-commit) 86520b7] empty > Author: A U Thor > 2 files changed, 1 insertion(+) > create mode 100644 empty > create mode 100644 empty.t > override r--r--r-- travis/staff for > .git/objects/e6/9de29bb2d1d6434b8b29ae775ad8c2e48c5391? (y/n [n]) not > overwritten > dangling blob c21c9352f7526e9576892a6631e0e8cf1fccd34d Looks like "mv" prompts and then fails to move the file (so we get the dangling blob for the source blob, and fsck doesn't report failure because we didn't actually corrupt the destination blob). If I'm understanding the behavior correctly, this violates POSIX, which says: 1. If the destination path exists, the −f option is not specified, and either of the following conditions is true: a. The permissions of the destination path do not permit writing and the standard input is a terminal. b. The −i option is specified. the mv utility shall write a prompt to standard error and read a line from standard input. If the response is not affirmative, mv shall do nothing more with the current source_file and go on to any remaining source_files. Our stdin isn't a terminal, and we do not specify "-i", so I think it shouldn't prompt. It also exits with code 0 after failing to move, which is another surprise. Here's a patch (tested by me that it works on Linux, but I don't know for sure that it fixes the Travis problem). -- >8 -- Subject: [PATCH] t1450: use "mv -f" within loose object directory The loose objects are created with mode 0444. That doesn't prevent them being overwritten by rename(), but some versions of "mv" will be extra careful and prompt the user, even without "-i". Reportedly macOS does this, at least in the Travis builds. The prompt reads from /dev/null, defaulting to "no", and the object isn't moved. Then to make matters even more interesting, it still returns "0" and the rest of the test proceeds, but with a broken setup. We can work around it by using "mv -f" to override the prompt. This should work as it's already used in t5504 for the same purpose. Signed-off-by: Jeff King --- t/t1450-fsck.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh index efaf41b68..33a51c9a6 100755 --- a/t/t1450-fsck.sh +++ b/t/t1450-fsck.sh @@ -536,7 +536,7 @@ test_expect_success 'fsck --connectivity-only' ' # free to examine the type if it chooses. empty=.git/objects/e6/9de29bb2d1d6434b8b29ae775ad8c2e48c5391 && blob=$(echo unrelated | git hash-object -w --stdin) && - mv $(sha1_file $blob) $empty && + mv -f $(sha1_file $blob) $empty && test_must_fail git fsck --strict && git fsck --strict --connectivity-only && -- 2.11.0.840.gd37c5973a
Re: What's cooking in git.git (Jan 2017, #04; Mon, 23)
> On 24 Jan 2017, at 01:18, Junio C Hamano wrote: > > * jk/fsck-connectivity-check-fix (2017-01-17) 6 commits > (merged to 'next' on 2017-01-23 at e8e9b76b84) > + fsck: check HAS_OBJ more consistently > + fsck: do not fallback "git fsck " to "git fsck" > + fsck: tighten error-checks of "git fsck " > + fsck: prepare dummy objects for --connectivity-check > + fsck: report trees as dangling > + t1450: clean up sub-objects in duplicate-entry test > > "git fsck --connectivity-check" was not working at all. > > Will merge to 'master'. "fsck: prepare dummy objects for --connectivity-check" seems to make t1450-fsck.sh fail on macOS with TravisCI: Initialized empty Git repository in /Users/travis/build/git/git/t/trash directory.t1450-fsck/connectivity-only/.git/ [master (root-commit) 86520b7] empty Author: A U Thor 2 files changed, 1 insertion(+) create mode 100644 empty create mode 100644 empty.t override r--r--r-- travis/staff for .git/objects/e6/9de29bb2d1d6434b8b29ae775ad8c2e48c5391? (y/n [n]) not overwritten dangling blob c21c9352f7526e9576892a6631e0e8cf1fccd34d test_must_fail: command succeeded: git fsck --strict not ok 58 - fsck --connectivity-only More test output: https://travis-ci.org/git/git/jobs/194663620 For some reason I am not able to replicate that behavior on my local macOS machine. I found the commit using bisect on TravisCI: https://api.travis-ci.org/jobs/194746454/log.txt?deansi=true Any idea what might be wrong? - Lars
