| Message ID | 20200710024728.3100527-1-sandals@crustytoothpaste.net (mailing list archive) |
|---|---|
| Headers | show |
| Series | SHA-256, part 3/3 | expand |
On 7/9/2020 10:46 PM, brian m. carlson wrote: > This is the final part required for the stage 4 implementation of > SHA-256. WOOHOO! What a milestone! As usual, your commits are excellently organized and clear. I could not find any fault in any of them. The proof is really in the pudding: does this pass the test suite when GIT_TEST_DEFAULT_HASH=sha256? You add that as a mode to the CI scripts, so we will know. I made a recommendation for a different model with how to do the CI, but it's super minor and can be done later. Basically, if we create a new job for SHA-256 mode, then we can more quickly identify when a test failure is due to that toggle and not other optional GIT_TEST_* variables. I hope to play around with SHA256-enabled repos a bit later, to see if I can find any issues poking around on my own. I doubt I will, with how thoroughly you modified the test suite. Thanks for this incredible achievement! -Stolee
On 2020-07-10 at 15:14:37, Derrick Stolee wrote: > On 7/9/2020 10:46 PM, brian m. carlson wrote: > > This is the final part required for the stage 4 implementation of > > SHA-256. > > WOOHOO! What a milestone! I'm also excited about this. It's been a lot of work, but we're finally here. > As usual, your commits are excellently organized and clear. I could > not find any fault in any of them. Thanks. > The proof is really in the pudding: does this pass the test suite > when GIT_TEST_DEFAULT_HASH=sha256? You add that as a mode to the > CI scripts, so we will know. I've seen several cases where we've accidentally regressed things with SHA-256, so it seemed only prudent to set up CI. I've run it locally on my system and it works for me, but we'll see how it fares on the CI system. > I made a recommendation for a different model with how to do the CI, > but it's super minor and can be done later. Basically, if we create > a new job for SHA-256 mode, then we can more quickly identify when > a test failure is due to that toggle and not other optional GIT_TEST_* > variables. I think that's a good suggestion, and I'm familiar enough with GitHub Actions that I think I can set up an additional job. If I reroll, I'll try to squash such a change in. > I hope to play around with SHA256-enabled repos a bit later, to see > if I can find any issues poking around on my own. I doubt I will, > with how thoroughly you modified the test suite. For folks who are looking for a more convenient way to get patches, you're welcome to grab them from the transition-stage-4 branch on https://github.com/bk2204/git.git or https://git.crustytoothpaste.net/git/bmc/git.git. The GitHub URL has slightly more bandwidth and a generally better uptime, since I don't live in a datacenter.
"brian m. carlson" <sandals@crustytoothpaste.net> writes: > On 2020-07-10 at 15:14:37, Derrick Stolee wrote: >> On 7/9/2020 10:46 PM, brian m. carlson wrote: >> > This is the final part required for the stage 4 implementation of >> > SHA-256. >> >> WOOHOO! What a milestone! > > I'm also excited about this. It's been a lot of work, but we're finally > here. Thanks.
"brian m. carlson" <sandals@crustytoothpaste.net> writes: > On 2020-07-10 at 15:14:37, Derrick Stolee wrote: >> On 7/9/2020 10:46 PM, brian m. carlson wrote: >> > This is the final part required for the stage 4 implementation of >> > SHA-256. >> >> WOOHOO! What a milestone! > > I'm also excited about this. It's been a lot of work, but we're finally > here. This topic sits at the tip of 'seen' (formerly known as 'pu'), and https://travis-ci.org/github/git/git/jobs/707050671 shows that t7063 is broken at the tip of 'seen'. - At the tip of this topic, t7063 passes. - There is no other topic that touches t7063 in flight. - seen^1, i.e. everything other than this topic merged, passes t7063. Ahh, this is an easy one. It is an interaction between this one and the dl/test-must-fail-fixes-6 topic. There are a few hunks like this in this topic. - test_cmp ../expect ../actual + test_might_fail test_cmp ../expect ../actual and the other series tightens test_must/might_fail so that these test helpers can only be used on "git" (other users should just use "! cmd" or "cmd || :" instead). I do not think it was an explicit objective for Denton's series to catch the use of test_might_fail with test_cmp specifically, but I offhand do not think of a good use case for saying "expect and actual may sometimes be the same, but they may be different", so in that sense, it contributed to find a nonsensical code. I haven't read thru all the 38 patches of this series, so there may be an obvious reason why we may want to have such a thing expressed that I am missing, though... Thanks.
On 2020-07-11 at 00:37:14, Junio C Hamano wrote: > I do not think it was an explicit objective for Denton's series to > catch the use of test_might_fail with test_cmp specifically, but I > offhand do not think of a good use case for saying "expect and > actual may sometimes be the same, but they may be different", so in > that sense, it contributed to find a nonsensical code. I haven't > read thru all the 38 patches of this series, so there may be an > obvious reason why we may want to have such a thing expressed that I > am missing, though... As mentioned upthread, my patch is definitely not correct. I've squashed in a fix to remove the test_might_fail and will send out a reroll later this weekend. I want to wait a little bit in case anyone has immediate comments on things so as not to send out patches too frequently. If the breakage is bothersome for you, please feel free to just remove those test_might_fail entries in the meantime, and the series should function correctly.