New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix custom pattern expansion on Windows + comments #1025

Merged
merged 2 commits into from Feb 6, 2017

Conversation

Projects
None yet
5 participants
@msprotz

This comment has been minimized.

Show comment
Hide comment
@msprotz

msprotz Feb 1, 2017

Contributor

@dra27 thoughts?

Contributor

msprotz commented Feb 1, 2017

@dra27 thoughts?

@dra27

Looks fine - could your test.ml case from the PR form a very quick testsuite test?

Show outdated Hide outdated byterun/win32.c
for (i = strlen(prefix); i > 0; i--) {
char c = prefix[i - 1];
if (c == '\\' || c == '/' || c == ':') { prefix[i] = 0; break; }
}
/* No separator was found, it's a pattern of the form Foo* */

This comment has been minimized.

@dra27

dra27 Feb 1, 2017

Contributor

Possibly No separator was found, it's a filename pattern? For some reason, to me "pattern of the form Foo*" excludes "Foo*Bar", but maybe that's just me!

@dra27

dra27 Feb 1, 2017

Contributor

Possibly No separator was found, it's a filename pattern? For some reason, to me "pattern of the form Foo*" excludes "Foo*Bar", but maybe that's just me!

@xavierleroy

This comment has been minimized.

Show comment
Hide comment
@xavierleroy

xavierleroy Feb 3, 2017

Contributor

This PR is an example of what is wrong in our process: the code submitted fixes an old-standing bug in a rarely-exercised part of the system, and is obviously correct. Yet CI blocks because a Changes entry is missing, reviewer asks for a legitimate but cosmetic change, submitter doesn't react, and the bug remains there. Who will break the deadlock?

Contributor

xavierleroy commented Feb 3, 2017

This PR is an example of what is wrong in our process: the code submitted fixes an old-standing bug in a rarely-exercised part of the system, and is obviously correct. Yet CI blocks because a Changes entry is missing, reviewer asks for a legitimate but cosmetic change, submitter doesn't react, and the bug remains there. Who will break the deadlock?

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Feb 3, 2017

Contributor

@xavierleroy - well, that's part of the Git process in general, surely? However, what will happen here is that if Jonathan doesn't get to it in the next few days, I'll either push a commit here with the Changes file and the test or I'll redo the GPR (former much more likely).

If I'd wanted to unblock the CI, I'd have added the label and restarted test 3 in Travis (takes seconds, even on my iPhone!) but as this one will want a Changes entry, I decided to leave it.

In the background, @avsm and I quietly restarted the failing AppVeyor test for this PR, once we fixed the problem there!

Contributor

dra27 commented Feb 3, 2017

@xavierleroy - well, that's part of the Git process in general, surely? However, what will happen here is that if Jonathan doesn't get to it in the next few days, I'll either push a commit here with the Changes file and the test or I'll redo the GPR (former much more likely).

If I'd wanted to unblock the CI, I'd have added the label and restarted test 3 in Travis (takes seconds, even on my iPhone!) but as this one will want a Changes entry, I decided to leave it.

In the background, @avsm and I quietly restarted the failing AppVeyor test for this PR, once we fixed the problem there!

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Feb 3, 2017

Member

Having the Continuous Integration complain about a missing Changes entry is a good thing in the current case, as it really is a mistake to not have a Changes entry here -- it is about fixing a user-observable bug, people in the future may want to know which OCaml versions are affected, eg. to make version choices in Windows-based teaching environments. That seems to be a part of the process working correctly, on the contrary.

(I think Jonathan will break the deadlock on his next context switch. Distributed work incurs latencies, sure, but those at hand here seem reasonable, and well within the bounds of the delay for the next release -- Jonathan did the right thing of submitting a properly-targeted Mantis ticket so there is low risk of forgetting about it.)

Member

gasche commented Feb 3, 2017

Having the Continuous Integration complain about a missing Changes entry is a good thing in the current case, as it really is a mistake to not have a Changes entry here -- it is about fixing a user-observable bug, people in the future may want to know which OCaml versions are affected, eg. to make version choices in Windows-based teaching environments. That seems to be a part of the process working correctly, on the contrary.

(I think Jonathan will break the deadlock on his next context switch. Distributed work incurs latencies, sure, but those at hand here seem reasonable, and well within the bounds of the delay for the next release -- Jonathan did the right thing of submitting a properly-targeted Mantis ticket so there is low risk of forgetting about it.)

@protz

This comment has been minimized.

Show comment
Hide comment
@protz

protz Feb 3, 2017

protz commented Feb 3, 2017

@msprotz

This comment has been minimized.

Show comment
Hide comment
@msprotz

msprotz Feb 6, 2017

Contributor

For me, this is good to merge.

Contributor

msprotz commented Feb 6, 2017

For me, this is good to merge.

@xavierleroy xavierleroy merged commit 561b846 into ocaml:trunk Feb 6, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017

Merge pull request #1025 from msprotz/trunk
Fix expansion of file patterns on command-line under Windows

Fixes: MPR#7473.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment