Test failure

I'm on a macbook running a beta of Mac OS Yosemite 10.10.1. I've never been able to get GETTEXT to work so I have

NO_GETTEXT=1

in my makefile, but other than that I'm using the master branch of the official github mirror.

When I build and run tests I get

[11:17][michael.blume@tcc-michael-4:~/workspace/git/t(master)]$ ./t1410-reflog.sh
ok 1 - setup
ok 2 - rewind
ok 3 - corrupt and check
ok 4 - reflog expire --dry-run should not touch reflog ok 5 - reflog expire
ok 6 - prune and fsck
ok 7 - recover and check
ok 8 - delete
ok 9 - rewind2
ok 10 - --expire=never
ok 11 - gc.reflogexpire=never
ok 12 - gc.reflogexpire=false
ok 13 - checkout should not delete log for packed ref not ok 14 - stale dirs do not cause d/f conflicts (reflogs on) #

# test_when_finished "git branch -d a || git branch -d a/b" &&
#
# git branch a/b master &&
# echo "a/b@{0} branch: Created from master" >expect &&
# git log -g --format="%gd %gs" a/b >actual &&
# test_cmp expect actual &&
# git branch -d a/b &&
#
# # now logs/refs/heads/a is a stale directory, but
# # we should move it out of the way to create "a" reflog
# git branch a master &&
# echo "a@{0} branch: Created from master" >expect &&
# git log -g --format="%gd %gs" a >actual &&
# test_cmp expect actual
#
not ok 15 - stale dirs do not cause d/f conflicts (reflogs off)
#
# test_when_finished "git branch -d a || git branch -d a/b" &&
#
# git branch a/b master &&
# echo "a/b@{0} branch: Created from master" >expect &&
# git log -g --format="%gd %gs" a/b >actual &&
# test_cmp expect actual &&
# git branch -d a/b &&
#
# # same as before, but we only create a reflog for "a" if
# # it already exists, which it does not
# git -c core.logallrefupdates=false branch a master &&
# : >expect &&
# git log -g --format="%gd %gs" a >actual &&
# test_cmp expect actual
#
# failed 2 among 15 test(s)

(I get the same thing with 'make test' but this is me just running the problematic test)

A quick search seems to indicate the test is pretty new?
http://www.mail-archive.com/[email protected]/msg60495.html

Test passes in my ubuntu vm.

What does "./t1410-reflog.sh -v -i" report?

Yes, it is new. In these cases there's often some silly little platform incompatibility in the test script, but I don't see one. So maybe the incompatibility is in the code itself; I'm wondering if OS X returns something besides EISDIR when trying to open a directory.

Unfortunately I don't have an OS X install handy to test on.

-Peff

I lied; it turns out I still had access to an old VM. The problem did turn out to be rather silly. Here's a patch.

-- >8 --
Two tests recently added to t1410 create branches "a" and "a/b" to test d/f conflicts on reflogs. Earlier, unrelated tests in that script create the path "A/B" in the working tree. There's no conflict on a case-sensitive filesystem, but on a case-insensitive one, "git log" will complain that "a/b" is both a revision and a working tree path.

We could fix this by using a "--" to disambiguate, but we are probably better off using names that are less confusing to make it more clear that they are unrelated to the working tree files. This patch turns "a/b" into "one/two".

---
The line-diff is hard to read, but if anyone was looking for a chance to test-drive contrib/diff-highlight, it does a good job of making the change easy to see.

 t/t1410-reflog.sh | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

index 976c1d4..8cf4461 100755
@@ -254,36 +254,36 @@ test_expect_success 'checkout should not delete log for packed ref' '
 '
 
 test_expect_success 'stale dirs do not cause d/f conflicts (reflogs on)' '
-	test_when_finished "git branch -d a || git branch -d a/b" &&
+	test_when_finished "git branch -d one || git branch -d one/two" &&
 
-	git branch a/b master &&
-	echo "a/b@{0} branch: Created from master" >expect &&
-	git log -g --format="%gd %gs" a/b >actual &&
+	git branch one/two master &&
+	echo "one/two@{0} branch: Created from master" >expect &&
+	git log -g --format="%gd %gs" one/two >actual &&
 	test_cmp expect actual &&
-	git branch -d a/b &&
+	git branch -d one/two &&
 
-	# now logs/refs/heads/a is a stale directory, but
-	# we should move it out of the way to create "a" reflog
-	git branch a master &&
-	echo "a@{0} branch: Created from master" >expect &&
-	git log -g --format="%gd %gs" a >actual &&
+	# now logs/refs/heads/one is a stale directory, but
+	# we should move it out of the way to create "one" reflog
+	git branch one master &&
+	echo "one@{0} branch: Created from master" >expect &&
+	git log -g --format="%gd %gs" one >actual &&
 	test_cmp expect actual
 '
 
 test_expect_success 'stale dirs do not cause d/f conflicts (reflogs off)' '
-	test_when_finished "git branch -d a || git branch -d a/b" &&
+	test_when_finished "git branch -d one || git branch -d one/two" &&
 
-	git branch a/b master &&
-	echo "a/b@{0} branch: Created from master" >expect &&
-	git log -g --format="%gd %gs" a/b >actual &&
+	git branch one/two master &&
+	echo "one/two@{0} branch: Created from master" >expect &&
+	git log -g --format="%gd %gs" one/two >actual &&
 	test_cmp expect actual &&
-	git branch -d a/b &&
+	git branch -d one/two &&
 
-	# same as before, but we only create a reflog for "a" if
+	# same as before, but we only create a reflog for "one" if
 	# it already exists, which it does not
-	git -c core.logallrefupdates=false branch a master &&
+	git -c core.logallrefupdates=false branch one master &&
 	: >expect &&
-	git log -g --format="%gd %gs" a >actual &&
+	git log -g --format="%gd %gs" one >actual &&
 	test_cmp expect actual
 '
 

2.1.2.596.g7379948

Works for me, thanks =)

I'm curious now, is there an automated build of git running on a mac anywhere? There's a mac mini running jenkins in my office and it's possible I could convince someone to let me set up a git build that'll e-mail me if there's a test failure.

I am not aware of a Macintosh person who regularly runs tests, but if there were, we hopefully will hear from them soonish ;-).

Thanks for the offer, we would definitely want to take you up on that, if there is nobody doing that already.

I think there are several people who run the tests on OS X fairly regularly (I note that another fix for the t1410 problem has already materialized :) ).

However, I think it is nice when test failures are caught early, before you have merged topics (to master or elsewhere). That helps isolate the failures to their particular topics.

I know you "make test" before pushing out the results of any integration you do. And I recall that for a while (and maybe still?) you even did so on VMs of a few common platforms. OS X is notoriously irritating to run in a VM, but would you be interested in a network-accessible install that you could push to and "make test" on as part of your routine?

If what Michael is offering cannot do that, I am sure I can get GitHub to set something up.

-Peff

Even if it were network-accessible, the latency for doing an integration cycle and pushing there and waiting for the tests to come back may make it impractical to use it as "part of pre-pushout test".

But I would think that the project certainly would benefit if a post-receive hook at Github did an automated test on the tip of 'pu' and bisected a breakage, if found, to pinpoint the patch that breaks. It could even send a notice to the author of the non-portable patch ;-).

That would be "post-pushout test", but as long as it catches new breakages before they hit 'next', the resulting benefit to the project would be very similar (the difference is only one integration cycle).

I had the impression you were already doing "for i in my_vms; do git push $i && ssh $i make test; done", so in theory this could be done in parallel. But yeah, I definitely wouldn't want to add latency to your workflow.

Yeah, you're right. Testing on "pu" is probably enough. Coupled with automated bisection, the "pinpointing" part is not that important, and pu is often "early enough" to catch problems before people try to build on top of them. So I think if Michael is still willing to set up his build box, just pulling and building your "pu" automatically would be a good start.

GitHub supports trigger hooks on pushes, but I do not think that is even necessary here. Just polling once or twice a day would probably give us good enough turn-around time.

I think somebody just needs to write the auto-bisect script. I suspect the simplest form is something like:

  # fetch a new version; exit early if we already checked this one
  git fetch
  test "$(git rev-parse pu-ok)" = "$(git rev-parse origin/pu)" &&exit 0
  git reset --hard origin/pu

  # quick path: all tests succeed
  if make test; then
    git tag -f pu-ok HEAD
    exit 0
  fi

  # slow path: bisect the actual commit. We do not need worry about
  # cutting each bisection step down to the minimal size (e.g., finding
  # the actual failing test), because this should run only infrequently.
  git bisect start HEAD pu-ok
  git bisect run make test

The real work would be in wrapping it up in a cron-job or similar so that it produces no output on success, and sends an email (or whatever) with the bisect results when it fails.

-Peff

The basic idea is to run
make clean &&
git fetch git.git &&
git checkout git.git/pu &&
make -k test

(and pipe stdlog+stdout into a file)

I have a script here doing that, if you want, I can send it to you

Actually I have a build break in pu on my mac right now. Seems to build fine in ubuntu, but:

progress.c:66:15: error: use of undeclared identifier 'CLOCK_MONOTONIC'

        timer_create(CLOCK_MONOTONIC, &sev, &progress_timer);
                     ^
./git-compat-util.h:233:54: note: expanded from macro 'timer_create'
#define timer_create(clockid, sevp, timerp) ((void) (clockid), (void) (s...
                                                     ^
progress.c:71:2: warning: incompatible pointer to integer conversion assigning
      to '__darwin_time_t' (aka 'long') from '__darwin_time_t *' (aka 'long *');
      remove & [-Wint-conversion]
        timer_settime(progress_timer, 0, &value, NULL);
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./git-compat-util.h:242:29: note: expanded from macro 'timer_settime'
        _ivalue.it_interval.tv_sec = value.it_interval.tv_sec;
         \
                                   ^ ~~~~~~~~~~~~~~~~~~~~~~~~
progress.c:71:2: error: invalid operands to binary expression
      ('long *' and 'long')
        timer_settime(progress_timer, 0, &value, NULL);
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./git-compat-util.h:243:58: note: expanded from macro 'timer_settime'
        _ivalue.it_interval.tv_usec = value.it_interval.tv_nsec /
1000L;        \
                                      ~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~
progress.c:71:2: error: use of undeclared identifier '_ivalue_it_value'
./git-compat-util.h:245:2: note: expanded from macro 'timer_settime'
        _ivalue_it_value.tv_usec = value.it_value.tv_nsec / 1000L...
        ^
progress.c:71:2: error: invalid operands to binary expression
      ('long *' and 'long')
        timer_settime(progress_timer, 0, &value, NULL);
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./git-compat-util.h:245:52: note: expanded from macro 'timer_settime'
        _ivalue_it_value.tv_usec = value.it_value.tv_nsec / 1000L;
         \
                                   ~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~
progress.c:71:35: warning: expression result unused [-Wunused-value]
        timer_settime(progress_timer, 0, &value, NULL);
        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~
./git-compat-util.h:244:27: note: expanded from macro 'timer_settime'
        _ivalue.it_value.tv_sec  value.it_value.tv_sec...
                                 ^

progress.c:74:1: error: expected 'while' in do/while loop static void clear_progress_signal(void)
^
progress.c:71:2: note: to match this 'do'

        timer_settime(progress_timer, 0, &value, NULL);
        ^
./git-compat-util.h:240:52: note: expanded from macro 'timer_settime'
  ...flags, value, ovalue) do {                         \
                           ^

progress.c:264:2: error: expected '}'
}
^
progress.c:50:1: note: to match this '{'
{
^
2 warnings and 6 errors generated.
make: *** [progress.o] Error 1

That is not an unkown issue.
You need to revert one commit (this is my copy of pu from today)

Revert "use timer_settime() for new platforms"

This reverts commit 0950f08806208b4e3d0aff5f4f7d497637ba4cab.

Ok, with that I have a different test failure on the pu branch -- please tell me if I'm spamming the list or if there's some other protocol I should be using to report issues on pu.

$ ./t4213-log-remerge-diff.sh -v
Initialized empty Git repository in
/Users/michael.blume/workspace/git/t/trash
directory.t4213-log-remerge-diff/.git/
expecting success:
test_commit A file original &&
test_commit B file change &&
git checkout -b side A &&
test_commit C file side &&
git checkout -b delete A &&
git rm file &&
test_commit D &&
git checkout -b benign master &&
test_must_fail git merge C &&
test_commit M1 file merged &&
git checkout -b evil B &&
test_must_fail git merge C &&
test_commit M2 file change &&
git checkout -b dm C &&
test_must_fail git merge D &&
test_commit M3 file resolved &&
git checkout -b unrelated A &&
test_commit unrelated_file &&
git merge C &&
test_tick &&
git tag M4 &&
git checkout -b dir A &&
mkdir sub &&
test_commit dir sub/file &&
git checkout -b file A &&
test_commit file sub &&
git checkout -b dirfile tags/dir &&
test_must_fail git merge tags/file &&
git rm --cached sub &&
test_commit M5 sub/file resolved &&
git checkout -b filedir tags/file &&
test_must_fail git merge tags/dir &&
git rm --cached sub/file &&
rm -rf sub &&
test_commit M6 sub resolved &&
git branch -D master side delete dir file

[master (root-commit) aca07f6] A
Author: A U Thor <[email protected]>

 1 file changed, 1 insertion(+)
 create mode 100644 file
[master a3cd1bc] B
 Author: A U Thor <[email protected]>
 1 file changed, 1 insertion(+), 1 deletion(-)
Switched to a new branch 'side'
[side 61d2ae1] C
 Author: A U Thor <[email protected]>
 1 file changed, 1 insertion(+), 1 deletion(-)

Switched to a new branch 'delete'
rm 'file'
[delete 63d1471] D
Author: A U Thor <[email protected]>

 2 files changed, 1 insertion(+), 1 deletion(-)
 create mode 100644 D.t
 delete mode 100644 file

Switched to a new branch 'benign'
Merging:
a3cd1bc B
virtual C
found 1 common ancestor:
aca07f6 A
Auto-merging file
CONFLICT (content): Merge conflict in file
Automatic merge failed; fix conflicts and then commit the result. [benign 19968bc] M1
Author: A U Thor <[email protected]>
Switched to a new branch 'evil'
Merging:
a3cd1bc B
virtual C
found 1 common ancestor:
aca07f6 A
Auto-merging file
CONFLICT (content): Merge conflict in file
Automatic merge failed; fix conflicts and then commit the result. [evil 93e9c1d] M2
Author: A U Thor <[email protected]>
Switched to a new branch 'dm'
Merging:
61d2ae1 C
virtual D
found 1 common ancestor:
aca07f6 A
CONFLICT (modify/delete): file deleted in D and modified in HEAD. Version HEAD of file left in tree.
Automatic merge failed; fix conflicts and then commit the result. [dm 35c618f] M3
Author: A U Thor <[email protected]>
Switched to a new branch 'unrelated'
[unrelated c19dbae] unrelated_file
Author: A U Thor <[email protected]>

 1 file changed, 1 insertion(+)
 create mode 100644 unrelated_file.t

Merging:
c19dbae unrelated_file
virtual C
found 1 common ancestor:
aca07f6 A
Merge made by the 'recursive' strategy.

 file | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Switched to a new branch 'dir'
[dir 8ac24b8] dir
 Author: A U Thor <[email protected]>
 1 file changed, 1 insertion(+)
 create mode 100644 sub/file
Switched to a new branch 'file'
[file 36416b6] file
 Author: A U Thor <[email protected]>
 1 file changed, 1 insertion(+)
 create mode 100644 sub

Switched to a new branch 'dirfile'
Merging:
8ac24b8 dir
virtual tags/file
found 1 common ancestor:
aca07f6 A
Adding sub/file
CONFLICT (directory/file): There is a directory with name sub in HEAD. Adding sub as sub~tags_file
Automatic merge failed; fix conflicts and then commit the result.
sub: needs merge
rm 'sub'
[dirfile f0a64c3] M5
Author: A U Thor <[email protected]>
Switched to a new branch 'filedir'
Merging:
36416b6 file
virtual tags/dir
found 1 common ancestor:
aca07f6 A
Adding sub/file
Removing sub to make room for subdirectory

CONFLICT (file/directory): There is a directory with name sub in tags/dir. Adding sub as sub~HEAD
Automatic merge failed; fix conflicts and then commit the result. rm 'sub/file'
[filedir 7182a02] M6
Author: A U Thor <[email protected]>
Deleted branch master (was a3cd1bc).
Deleted branch side (was 61d2ae1).
Deleted branch delete (was 63d1471).
Deleted branch dir (was 8ac24b8).
Deleted branch file (was 36416b6).
ok 1 - setup

expecting success:
git log -p --cc unrelated >expected &&
git log -p --remerge-diff unrelated >actual &&
test_cmp expected actual

ok 2 - unrelated merge: without conflicts

expecting success:
git log -1 -p --remerge-diff benign >output &&
clean_output <output >actual &&
test_cmp expected actual

@@ -1,12 +1,12 @@
 commit benign
-Merge:
+Merge: a3cd1bc 61d2ae1
 Author: A U Thor <[email protected]>
-Date:
+Date:   Thu Apr 7 15:17:13 2005 -0700
 
     M1
 
 diff --git a/file b/file
-index
+index cc073fb..20b117f 100644
 --- a/file
 +++ b/file
 @@ -1,5 +1 @@

not ok 3 - benign merge: conflicts resolved
#

# git log -1 -p --remerge-diff benign >output &&
# clean_output <output >actual &&
# test_cmp expected actual
#

expecting success:
git log -1 --remerge-diff -p evil >output &&
clean_output <output >actual &&
test_cmp expected actual

@@ -1,12 +1,12 @@
 commit evil
-Merge:
+Merge: a3cd1bc 61d2ae1
 Author: A U Thor <[email protected]>
-Date:
+Date:   Thu Apr 7 15:18:13 2005 -0700
 
     M2
 
 diff --git a/file b/file
-index
+index cc073fb..0835e4f 100644
 --- a/file
 +++ b/file
 @@ -1,5 +1 @@

not ok 4 - evil merge: changes ignored
#

# git log -1 --remerge-diff -p evil >output &&
# clean_output <output >actual &&
# test_cmp expected actual
#

expecting success:
git log -1 --remerge-diff -p dm >output &&
clean_output <output >actual &&
test_cmp expected actual

@@ -1,12 +1,12 @@
 commit dm
-Merge:
+Merge: 61d2ae1 63d1471
 Author: A U Thor <[email protected]>
-Date:
+Date:   Thu Apr 7 15:19:13 2005 -0700
 
     M3
 
 diff --git a/file b/file
-index
+index dd79d22..2ab19ae 100644
 --- a/file
 +++ b/file
 @@ -1,4 +1 @@

not ok 5 - delete/modify conflict
#

# git log -1 --remerge-diff -p dm >output &&
# clean_output <output >actual &&
# test_cmp expected actual
#

expecting success:
git log -1 --remerge-diff -p dirfile >output &&
clean_output <output >actual &&
test_cmp expected actual

@@ -1,12 +1,12 @@
 commit dirfile
-Merge:
+Merge: 8ac24b8 36416b6
 Author: A U Thor <[email protected]>
-Date:
+Date:   Thu Apr 7 15:24:13 2005 -0700
 
     M5
 
 diff --git a/sub/file b/sub/file
-index
+index 0d2ecd7..2ab19ae 100644
 --- a/sub/file
 +++ b/sub/file
 @@ -1 +1 @@

@@ -14,7 +14,7 @@
 +resolved
 diff --git a/sub~tags/file b/sub~tags/file
 deleted file mode 100644
-index
+index f73f309..0000000
 --- a/sub~tags/file
 +++ /dev/null
 @@ -1 +0,0 @@

not ok 6 - file/directory conflict resulting in directory #

# git log -1 --remerge-diff -p dirfile >output &&
# clean_output <output >actual &&
# test_cmp expected actual
#

checking known breakage:
git log -1 --remerge-diff -p filedir >output &&
clean_output <output >actual &&
test_cmp expected actual

@@ -1,21 +1,28 @@
 commit filedir
-Merge:
+Merge: 36416b6 8ac24b8
 Author: A U Thor <[email protected]>
-Date:
+Date:   Thu Apr 7 15:25:13 2005 -0700
 
     M6
 
 diff --git a/sub b/sub
-index
---- a/sub
+new file mode 100644
+index 0000000..2ab19ae
+--- /dev/null
 +++ b/sub
-@@ -1 +1 @@
--file
+@@ -0,0 +1 @@
 +resolved
 diff --git a/sub/file b/sub/file
 deleted file mode 100644
-index
+index 0d2ecd7..0000000
 --- a/sub/file
 +++ /dev/null
 @@ -1 +0,0 @@
 -dir
+diff --git a/sub~tags/file b/sub~tags/file
+deleted file mode 100644
+index f73f309..0000000
+--- a/sub~tags/file
++++ /dev/null
+@@ -1 +0,0 @@
+-file

not ok 7 - file/directory conflict resulting in file # TODO known breakage

# still have 1 known breakage(s)
# failed 4 among remaining 6 test(s)
1..7

Thanks for a clear problem description.

Hmph, but the branch a and the file A _do_ have names that are unrelated to each other, and it is only the case insensitive fs that is confused ;-). Renaming is not so bad and certainly is not wrong per-se as a workaround, but I have this suspicion that it sends a wrong message to people on such a filesystem, namely, "you cannot use Git to manage a file called 'master'", or something silly like that. Disambiguation with double-dashes does not have such a problem, and instead shows a way how scripts that are meant to be portable is written. More importantly, that is more in line with the problem description (i.e. we complain pointing out the ambiguity, implying that everything is fine as long as you disambiguate).

So I would rather see the workaround done that way.

But that is only if this were before you actually wrote the patch. The above is not a preference strong enough to make me ask you to reroll ;-)

Thanks.

My main concern is that it leaves t1410 in an "accident waiting to happen" state, where some hapless developer will add a new test using a/b and not realizing they need to be careful to disambiguate. The test will pass for them on Linux, but some luckless OS X user will end up wasting time tracking down the error.

Or another way of looking at it: it is perfectly possible to have git manage a file called "master" or even "HEAD". But that does not mean it is a _good idea_, or is without annoyances. :)

The alternate form is quite trivial. I think I still prefer the "one/two" version, but here is the "--" patch for reference. You can decide which to pick up.

-- >8 --
Subject: t1410: fix breakage on case-insensitive filesystems

Two tests recently added to t1410 create branches "a" and "a/b" to test d/f conflicts on reflogs. Earlier tests in that script create the path "A/B" in the working tree. There's no conflict on a case-sensitive filesystem, but on a case-insensitive one, "git log" will complain that "a/b" is both a revision and a working tree path.

We can fix this by using "--" to disambiguate.

---
 t/t1410-reflog.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

index 976c1d4..48bcd59 100755
@@ -258,7 +258,7 @@ test_expect_success 'stale dirs do not cause d/f conflicts (reflogs on)' '
 
 	git branch a/b master &&
 	echo "a/b@{0} branch: Created from master" >expect &&
-	git log -g --format="%gd %gs" a/b >actual &&
+	git log -g --format="%gd %gs" a/b -- >actual &&
 	test_cmp expect actual &&
 	git branch -d a/b &&
 

@@ -266,7 +266,7 @@ test_expect_success 'stale dirs do not cause d/f conflicts (reflogs on)' '
 	# we should move it out of the way to create "a" reflog
 	git branch a master &&
 	echo "a@{0} branch: Created from master" >expect &&
-	git log -g --format="%gd %gs" a >actual &&
+	git log -g --format="%gd %gs" a -- >actual &&
 	test_cmp expect actual
 '
 

@@ -275,7 +275,7 @@ test_expect_success 'stale dirs do not cause d/f conflicts (reflogs off)' '
 
 	git branch a/b master &&
 	echo "a/b@{0} branch: Created from master" >expect &&
-	git log -g --format="%gd %gs" a/b >actual &&
+	git log -g --format="%gd %gs" a/b -- >actual &&
 	test_cmp expect actual &&
 	git branch -d a/b &&
 

@@ -283,7 +283,7 @@ test_expect_success 'stale dirs do not cause d/f conflicts (reflogs off)' '
 	# it already exists, which it does not
 	git -c core.logallrefupdates=false branch a master &&
 	: >expect &&
-	git log -g --format="%gd %gs" a >actual &&
+	git log -g --format="%gd %gs" a -- >actual &&
 	test_cmp expect actual
 '
 

2.1.2.596.g7379948

Am 09.11.2014 um 02:59 schrieb Jeff King:

Yes, this is expected. The warning actually comes from the "git branch -d" run during cleanup. Branch "one" exists but has no reflog. Instead there is a crufty dir call "logs/refs/heads/one". The deletion process blindly calls "unlink" on it and complains when it fails for reasons other than ENOENT.

We could suppress that warning, but I didn't think it was really worth the bother. This is such a funny setup (reflogs on, then off, then on, _and_ a d/f conflict in the branches) that I doubt it will come up much.

I seem to recall that ancient versions of SunOS used to do bad things when you called "unlink" on a directory, but I do not know if that is still the case (I also would doubt this is the only place in git where we blindly do an "unlink if you can" without actually checking the file.

That looks more like it is failing the actual test (i.e., the creation of branch "one" when there is cruft in the reflog). My guess is that calling open() on a directory is giving us EACCES instead of EISDIR. Can you verify that?

If that is the case, then this isn't a new breakage, I think, but just code we weren't previously exercising. It would be interesting to know whether:

git config core.logallrefupdates true
git branch one/two
git branch -d one/two
git branch one

works (even without my patch). If so, then there's probably something else going on.

The relevant bits are in log_ref_setup. We try to open() once, and accept EISDIR as a hint that we may need to remove an empty directory and try again. If Windows gives us EACCES, then we may need to have that trigger the same code.

-Peff

Am 12.11.2014 22:59, schrieb Jeff King:

Don't know what you mean with "my patch" (the one I was responding to touches only t1410). But the sequence works as expected with a version built in September:

C:\Temp\gittest>git init
Initialized empty Git repository in C:/Temp/gittest/.git/

C:\Temp\gittest>git commit --allow-empty -m init
[master (root-commit) 2e78994] init

C:\Temp\gittest>git branch one/two

C:\Temp\gittest>git branch -d one/two
Deleted branch one/two (was 2e78994).

C:\Temp\gittest>git branch one

C:\Temp\gittest>git version
git version 2.1.0.rc2.1268.g12ef091

Thanks, that is a starting point.

-- Hannes

The patch you are responding to is a fix-up for 9233887, which tweaked the code and added those tests in the first place (I doubt it would work for you, though, as it has a problem on case-insensitive filesystems).

Hmph. So that would mean my theory is not right. Or maybe I am not accounting for something else in my analysis.

I guess it is odd that the test right before the failing one passes (it is basically that same sequence, with reflogs turned on for both operations), which implies that we are properly getting EISDIR. The only difference in the failing test is that reflogs are turned off for the "git branch one" operation. But I cannot see why that would be broken if the other one passes.

I wish it were easy for me to ssh into a Windows VM and run gdb. ;)

-Peff

(+cc msysgit)

Am 13.11.2014 um 10:08 schrieb Jeff King:

That is indeed the case. It's an ancient bug in our wrapper mingw_open().

Not a comment, on this paragraph of yours, but while I was walking through the code with gdb, I was wondering why the reflog directory is being touched at all when core.logallrefupdates is off (in log_ref_setup via log_ref_write). With the patch below I now get the same unlink warning as on Linux.

--- 8< ---
Subject: [PATCH] Windows: correct detection of EISDIR in mingw_open()

According to the Linux open(2) man page, open() returns EISDIR if a directory was attempted to be opened for writing. Our emulation in mingw_open() does not get this right: it checks only for O_CREAT. Fix it to check for one of the write flags.

This fixes a failure in reflog handling, which opens files with O_APPEND|O_WRONLY, but without O_CREAT, and expects EISDIR when the named file happens to be a directory.

---
 compat/mingw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 2ee3fe3..fc64b73 100644
@@ -312,7 +312,7 @@ int mingw_open (const char *filename, int oflags, ...)
 		return -1;
 	fd = _wopen(wfilename, oflags, mode);
 
-	if (fd < 0 && (oflags & O_CREAT) && errno == EACCES) {
+	if (fd < 0 && (oflags & (O_WRONLY|O_RDWR)) && errno == EACCES) {
 		DWORD attrs = GetFileAttributesW(wfilename);
 		if (attrs != INVALID_FILE_ATTRIBUTES && (attrs & FILE_ATTRIBUTE_DIRECTORY))
 			errno = EISDIR;

2.0.0.12.gbcf935e

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to [email protected]
To unsubscribe from this group, send email to
[email protected]
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "Git for Windows" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
For more options, visit https://groups.google.com/d/optout.

It's because of the weird reflog rules: we will use a reflog file that exists (no matter what core.logallrefupdates says), but only _create_ a new one if the config tells us to. I think this dates back to the origin of reflogs, where we weren't sure at first that you would always want them on.

Ah, so we do get EISDIR with O_CREAT. Which explains why the first test passed and the second failed (we only pass O_CREAT when logallrefupdates is on). The behavior you saw (and this fix) make perfect sense to me then.

I am not sure if I get to Acked-by things in compat/mingw, but I am pretty sure this is the right fix. :)

-Peff

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to [email protected]
To unsubscribe from this group, send email to
[email protected]
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "Git for Windows" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
For more options, visit https://groups.google.com/d/optout.

Am 14.11.2014 um 22:04 schrieb Andreas Schwab:

Hrm... I wondered why O_RDONLY can be zero. If I find O_ACCMODE on Windows, I'll use it, otherwise, the patch as is will have to do - we don't have to be particularly portable in this corner of the code.

-- Hannes

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to [email protected]
To unsubscribe from this group, send email to
[email protected]
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "Git for Windows" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
For more options, visit https://groups.google.com/d/optout.

According to the Linux open(2) man page, open() returns EISDIR if a directory was attempted to be opened for writing. Our emulation in mingw_open() does not get this right: it checks only for O_CREAT. Fix it to check for a write request.

This fixes a failure in reflog handling, which opens files with O_APPEND|O_WRONLY, but without O_CREAT, and expects EISDIR when the named file happens to be a directory.

---
This version of the patch uses O_ACCMODE.

 compat/mingw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 2ee3fe3..c702731 100644
@@ -312,7 +312,7 @@ int mingw_open (const char *filename, int oflags, ...)
 		return -1;
 	fd = _wopen(wfilename, oflags, mode);
 
-	if (fd < 0 && (oflags & O_CREAT) && errno == EACCES) {
+	if (fd < 0 && (oflags & O_ACCMODE) != O_RDONLY && errno == EACCES) {
 		DWORD attrs = GetFileAttributesW(wfilename);
 		if (attrs != INVALID_FILE_ATTRIBUTES && (attrs & FILE_ATTRIBUTE_DIRECTORY))
 			errno = EISDIR;

2.0.0.12.gbcf935e

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to [email protected]
To unsubscribe from this group, send email to
[email protected]
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "Git for Windows" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
For more options, visit https://groups.google.com/d/optout.

I do not do Windows, but your analysis feels really sound and explains the symptom well, and the change looks like the right fix to the issue.

Thanks.

Hmph, or perhaps "branch -d one/two" fails to remove the reflog and does not notice the failure? But creation of "one" with reflog disabled shouldn't be affected in such a case, either. Puzzled...

likewise.

$ ./t1410-reflog.sh -v -i
Initialized empty Git repository in
/Users/michael.blume/workspace/git/t/trash
directory.t1410-reflog/.git/
expecting success:
mkdir -p A/B &&
echo rat >C &&
echo ox >A/D &&
echo tiger >A/B/E &&
git add . &&

test_tick && git commit -m rabbit &&
H=git rev-parse --verify HEAD &&
A=git rev-parse --verify HEAD:A &&
B=git rev-parse --verify HEAD:A/B &&
C=git rev-parse --verify HEAD:C &&
D=git rev-parse --verify HEAD:A/D &&
E=git rev-parse --verify HEAD:A/B/E &&
check_fsck &&

test_chmod +x C &&
git add C &&
test_tick && git commit -m dragon &&
L=git rev-parse --verify HEAD &&
check_fsck &&

rm -f C A/B/E &&
echo snake >F &&
echo horse >A/G &&
git add F A/G &&
test_tick && git commit -a -m sheep &&
F=git rev-parse --verify HEAD:F &&
G=git rev-parse --verify HEAD:A/G &&
I=git rev-parse --verify HEAD:A &&
J=git rev-parse --verify HEAD &&
check_fsck &&

rm -f A/G &&
test_tick && git commit -a -m monkey &&
K=git rev-parse --verify HEAD &&
check_fsck &&

check_have A B C D E F G H I J K L &&

git prune &&

check_have A B C D E F G H I J K L &&

check_fsck &&

test_line_count = 4 .git/logs/refs/heads/master

[master (root-commit) 07fcc4f] rabbit
Author: A U Thor <[email protected]>

 3 files changed, 3 insertions(+)
 create mode 100644 A/B/E
 create mode 100644 A/D
 create mode 100644 C
Checking object directories: 100% (256/256), done.
[master 7d1a0b8] dragon
 Author: A U Thor <[email protected]>
 1 file changed, 0 insertions(+), 0 deletions(-)
 mode change 100644 => 100755 C
Checking object directories: 100% (256/256), done.
[master 15ab5f0] sheep
 Author: A U Thor <[email protected]>
 4 files changed, 2 insertions(+), 2 deletions(-)
 delete mode 100644 A/B/E
 create mode 100644 A/G
 delete mode 100755 C
 create mode 100644 F
Checking object directories: 100% (256/256), done.
[master 3ac530f] monkey
 Author: A U Thor <[email protected]>
 1 file changed, 1 deletion(-)
 delete mode 100644 A/G

Checking object directories: 100% (256/256), done. tree
tree
blob
blob
blob
blob
blob
commit
tree
commit
commit
commit
tree
tree
blob
blob
blob
blob
blob
commit
tree
commit
commit
commit
Checking object directories: 100% (256/256), done. ok 1 - setup

expecting success:
test_tick && git reset --hard HEAD~2 &&
test -f C &&
test -f A/B/E &&
! test -f F &&
! test -f A/G &&

check_have A B C D E F G H I J K L &&

git prune &&

check_have A B C D E F G H I J K L &&

test_line_count = 5 .git/logs/refs/heads/master

HEAD is now at 7d1a0b8 dragon
tree
tree
blob
blob
blob
blob
blob
commit
tree
commit
commit
commit
tree
tree
blob
blob
blob
blob
blob
commit
tree
commit
commit
commit
ok 2 - rewind

expecting success:

corrupt $F &&
check_fsck "missing blob $F"

Checking object directories: 100% (256/256), done. missing blob 9ae84adb2704cbd49549e52169b4043871e13432 ok 3 - corrupt and check

expecting success:

git reflog expire --dry-run \
--expire=$(($test_tick - 10000)) \
--expire-unreachable=$(($test_tick - 10000)) \
--stale-fix \
--all &&

test_line_count = 5 .git/logs/refs/heads/master &&

check_fsck "missing blob $F"

would prune commit: sheep
would prune commit: monkey
would prune reset: moving to HEAD~2
would prune commit: sheep
would prune commit: monkey
would prune reset: moving to HEAD~2
Checking object directories: 100% (256/256), done. missing blob 9ae84adb2704cbd49549e52169b4043871e13432 ok 4 - reflog expire --dry-run should not touch reflog

expecting success:

git reflog expire --verbose \
--expire=$(($test_tick - 10000)) \
--expire-unreachable=$(($test_tick - 10000)) \
--stale-fix \
--all &&

test_line_count = 2 .git/logs/refs/heads/master &&

check_fsck "dangling commit $K"

Marking reachable objects...
keep commit (initial): rabbit
keep commit: dragon
prune commit: sheep
prune commit: monkey
prune reset: moving to HEAD~2
keep commit (initial): rabbit
keep commit: dragon
prune commit: sheep
prune commit: monkey
prune reset: moving to HEAD~2
Checking object directories: 100% (256/256), done. dangling commit 3ac530f2f6c3a45206a6b2d455baf89d1c8f672d ok 5 - reflog expire

expecting success:

git prune &&
check_fsck &&

check_have A B C D E H L &&
check_dont_have F G I J K

Checking object directories: 100% (256/256), done.
tree
tree
blob
blob
blob
commit
commit
fatal: git cat-file 9ae84adb2704cbd49549e52169b4043871e13432: bad file
fatal: git cat-file 41ca11d3340423c1922fe4110d2327bda0891b45: bad file
fatal: git cat-file 1fe0f573dce6e694ce16e98a936c65db295d1948: bad file
fatal: git cat-file 15ab5f0ad4d9117a7cdcbea9673ec0fff6ca8e55: bad file
fatal: git cat-file 3ac530f2f6c3a45206a6b2d455baf89d1c8f672d: bad file ok 6 - prune and fsck

expecting success:

recover $F &&
check_fsck "dangling blob $F"

Checking object directories: 100% (256/256), done. dangling blob 9ae84adb2704cbd49549e52169b4043871e13432 ok 7 - recover and check

expecting success:
echo 1 > C &&
test_tick &&
git commit -m rat C &&

echo 2 > C &&
test_tick &&
git commit -m ox C &&

echo 3 > C &&
test_tick &&
git commit -m tiger C &&

HEAD_entry_count=$(git reflog | wc -l) && master_entry_count=$(git reflog show master | wc -l) &&

test $HEAD_entry_count = 5 &&
test $master_entry_count = 5 &&

git reflog delete master@{1} &&
git reflog show master > output &&
test $(($master_entry_count - 1)) = $(wc -l < output) && test $HEAD_entry_count = $(git reflog | wc -l) && ! grep ox < output &&

master_entry_count=$(wc -l < output) &&

git reflog delete HEAD@{1} &&
test $(($HEAD_entry_count -1)) = $(git reflog | wc -l) && test $master_entry_count = $(git reflog show master | wc -l) &&

HEAD_entry_count=$(git reflog | wc -l) &&

git reflog delete master@{07.04.2005.15:15:00.-0700} && git reflog show master > output &&
test $(($master_entry_count - 1)) = $(wc -l < output) && ! grep dragon < output

[master b60a214] rat
Author: A U Thor <[email protected]>

 1 file changed, 1 insertion(+), 1 deletion(-)
[master 9908ef9] ox
 Author: A U Thor <[email protected]>
 1 file changed, 1 insertion(+), 1 deletion(-)
[master b93561f] tiger
 Author: A U Thor <[email protected]>
 1 file changed, 1 insertion(+), 1 deletion(-)

ok 8 - delete

expecting success:

test_tick && git reset --hard HEAD~2 &&
test_line_count = 4 .git/logs/refs/heads/master

HEAD is now at b60a214 rat
ok 9 - rewind2

expecting success:

git reflog expire --verbose \
--expire=never \
--expire-unreachable=never \
--all &&
test_line_count = 4 .git/logs/refs/heads/master

keep commit (initial): rabbit
keep commit: dragon
keep commit: rat
keep commit: tiger
keep reset: moving to HEAD~2
keep commit (initial): rabbit
keep commit: rat
keep commit: tiger
keep reset: moving to HEAD~2
ok 10 - --expire=never

expecting success:

git config gc.reflogexpire never &&
git config gc.reflogexpireunreachable never &&
git reflog expire --verbose --all &&
test_line_count = 4 .git/logs/refs/heads/master

keep commit (initial): rabbit
keep commit: dragon
keep commit: rat
keep commit: tiger
keep reset: moving to HEAD~2
keep commit (initial): rabbit
keep commit: rat
keep commit: tiger
keep reset: moving to HEAD~2
ok 11 - gc.reflogexpire=never

expecting success:

git config gc.reflogexpire false &&
git config gc.reflogexpireunreachable false && git reflog expire --verbose --all && test_line_count = 4 .git/logs/refs/heads/master &&

git config --unset gc.reflogexpire &&
git config --unset gc.reflogexpireunreachable

keep commit (initial): rabbit
keep commit: dragon
keep commit: rat
keep commit: tiger
keep reset: moving to HEAD~2
keep commit (initial): rabbit
keep commit: rat
keep commit: tiger
keep reset: moving to HEAD~2
ok 12 - gc.reflogexpire=false

expecting success:
test $(git reflog master | wc -l) = 4 &&
git branch foo &&
git pack-refs --all &&
git checkout foo &&
test $(git reflog master | wc -l) = 4

Switched to branch 'foo'
ok 13 - checkout should not delete log for packed ref

expecting success:
test_when_finished "git branch -d a || git branch -d a/b" &&

git branch a/b master &&
echo "a/b@{0} branch: Created from master" >expect && git log -g --format="%gd %gs" a/b >actual && test_cmp expect actual &&
git branch -d a/b &&

# now logs/refs/heads/a is a stale directory, but
# we should move it out of the way to create "a" reflog
git branch a master &&
echo "a@{0} branch: Created from master" >expect &&
git log -g --format="%gd %gs" a >actual &&
test_cmp expect actual

fatal: ambiguous argument 'a/b': both revision and filename Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'
not ok 14 - stale dirs do not cause d/f conflicts (reflogs on) #

# test_when_finished "git branch -d a || git branch -d a/b" &&
#
# git branch a/b master &&
# echo "a/b@{0} branch: Created from master" >expect &&
# git log -g --format="%gd %gs" a/b >actual &&
# test_cmp expect actual &&
# git branch -d a/b &&
#
# # now logs/refs/heads/a is a stale directory, but
# # we should move it out of the way to create "a" reflog
# git branch a master &&
# echo "a@{0} branch: Created from master" >expect &&
# git log -g --format="%gd %gs" a >actual &&
# test_cmp expect actual
#