Hi Peter, On 27 ก.ย. 2012, at 5:03, Peter Rosin <p...@lysator.liu.se> wrote: > On 2012-09-26 14:57, Peter Rosin wrote: >> On 2012-09-22 05:31, Gary V. Vaughan wrote: > > [Heavily snipped] > >> Why not commit the sc_prohibit_test_const_follows_var improvement >> last, after fixing all violations? > > That doesn't make sense, sorry. But the idea would have worked > initially, before the check first existed. I.e., write the check, > fixup violations and commit in smaller digestible chunks, then > finally commit the actual check preventing any new violations from > creeping in.
Even then, I'm on the fence, since that doesn't leave a revision to demonstrate that the syntax-check is finding the violations... on the other hand, we also don't want to push "broken" commits since that obviously screws up git bisection. >>> If not, I will branch before 962aa91, run the script, and then apply >>> the rest of master to that branch one patch at a time until I arrive >>> at a diff that I can apply to master itself, rather than using revert >>> as I did on the current temporary branch. >> >> I have to say, given all these difficulties, is it really worth it? >> Besides, I think > > Hmmm, I said that in the wrong context. Your plan above seems like the > best path forward. I guess I was too "excited" about the bugs and didn't > read properly, sorry again about my poor communication skills. No apologies necessary. I'm working on the revised redo, which will result in a much smaller patch for master later today or tomorrow. > Some of the noise between master and redo-test-operand-order are a bunch > of hunks with this pattern: > - if test bar $op $foo; then > + if test bar $op $foo ; then Nice catch. I already tweaked the script regexs to fix that once, but must have missed one of the ways for that to happen :( > You should perhaps add a commit to redo-test-operand-order which silences > that and makes other more substantial changes stand out more before you > proceed with the above plan? There are perhaps other harmless changes > that can be excluded from the "light" revert? Because who needs the > oscillation? Agreed. It was all these edge cases that made me abandon the scripted solution last year. I'm working on improving the script first to catch this and the other glitches you found. > BTW, here is another strange-looking hunk from > "git diff master redo-test-operand-order" > > diff --git a/m4/libtool.m4 b/m4/libtool.m4 > index 4413a6d..8ec9beb 100644 > --- a/m4/libtool.m4 > +++ b/m4/libtool.m4 > @@ -5563,7 +5563,7 @@ _LT_EOF > ;; > esac > > - if test sni = "$host_vendor"; then > + if test x$host_vendor = xsni; then > case $host in > sysv4 | sysv4.2uw2* | sysv4.3* | sysv5*) > _LT_TAGVAR(export_dynamic_flag_spec, $1)='$wl-Blargedynsym' > > What has caused the difference in this hunk? Why hasn't the script > caught this instance? And why isn't the syntax-check triggered? > Are the "missing" quotes the key here? Not sure... investigating now. > The check is named sc_prohibit_test_const_follows_var, so one > would guess that it should prohibit "test x$foo = xbar", no? Or are > you supposed to be able to dodge the check by needlessly adding x > in front of LHS $variables? Not at all. But the long list of substitutions must be interacting unexpectedly. The idea is for the syntax-check rule to flag any of: test x"$foo" = "xbar" test x"$foo" = xbar test "x$foo" = "xbar" test "x$foo" = xbar test x$foo = xbar where 'x' is one of '.', 'x', or 'X'. I think we're getting much closer to a working solution though. Many thanks for taking the time to review. Cheers, -- Gary V. Vaughan (gary AT gnu DOT org)
