Re: [lmi] PATCH: C++20 and clang

[Vadim Zeitlin]

On Tue, 20 Apr 2021 02:19:03 +0000 Greg Chicares <gchicares@sbcglobal.net> 
wrote:

GC> so I think this is what you expected to see:
GC>    8be683cc1..9414edf44  master     -> origin/master

 Yes, indeed, and this explains the mystery, thanks.

GC> But the command was wrong--I forgot the two dots:
GC> 
GC> -  $git cherry-pick xanadu/clang-cxx20
GC> +  $git cherry-pick ..xanadu/clang-cxx20
GC> 
GC> so now I go back and run the correct command:
GC> 
GC>   $git cherry-pick ..xanadu/clang-cxx20

 This is going to try to re-apply all the commits from that branch.
Normally all but one should be skipped, but there was a small change/fixup
to one of them, so you'd get at least a conflict for it. What you really
wanted was to cherry pick just the single missing commit.

 BTW, this is the kind of problems that doesn't exist when using git-merge,
which allows Git to determine which commits are and are not already present
in the current branch and intelligently resolve, or even avoid, the
conflicts. By using git-cherry-pick you basically manually do the work that
Git would have done for you automatically -- and, unsurprisingly, people
make mistakes more often than Git does, so this is more error-prone, in
addition to being more time-consuming.

GC> which looks hopeless, but when I look where I've ended up...
GC> 
GC>   $git status
GC>   On branch master
GC>   Your branch is ahead of 'origin/master' by 1 commit.
GC>     (use "git push" to publish your local commits)
GC> 
GC> ...it looks like exactly where I want to be.
GC> 
GC> There's nothing ugly in git-status, git-log shows only what
GC> should be there, and all the tests pass...so now I push.

 Yes, everything looks good, thank you!

GC> > GC> I did some fixups to make 'make $coefficiency check_concinnity'
GC> > GC> succeed.
GC> > 
GC> >  Oops, really sorry about this, I should have rerun it. But I don't
GC> > understand how did the CI builds pass, they're supposed to check for
GC> > this... oh, now I see: the problem was indeed detected, see
GC> > 
GC> > 
https://github.com/let-me-illustrate/lmi/runs/2382487360?check_suite_focus=true#step:22:25
GC> > 
GC> > but apparently "make check_concinnity" doesn't exit with error even when 
it
GC> > does detect problems. This is really unexpected and counter-intuitive.
GC> > Would you consider accepting patches changing this?
GC> 
GC> I'm loth to touch it,

 FWIW touching it would involve exactly one character change:

---------------------------------- >8 --------------------------------------
diff --git a/GNUmakefile b/GNUmakefile
index f08c70e0a..8bcfbca84 100644
--- a/GNUmakefile
+++ b/GNUmakefile
@@ -501,7 +501,7 @@ check_concinnity: source_clean custom_tools
              || $(ECHO) "... in file $$z"; \
          done;
        @$(ECHO) "  Miscellaneous problems:"
-       @-cd $(prefascicle_dir) && $(PERFORM) $(TEST_CODING_RULES) *
+       @cd $(prefascicle_dir) && $(PERFORM) $(TEST_CODING_RULES) *

 
################################################################################

---------------------------------- >8 --------------------------------------

I have no idea what is this "-" doing here, except that it was always there
ever since this line was added back in 7afe844dc (Quadruple the speed of
'make check_conformity', 2006-01-18).

 Are you still reluctant to remove it?

GC> and AFAICT there's no need to touch it because ./hooks/pre-commit
GC> already does what you want.

 Just FYI, but I've had to disable it, as well as commit-msg hook, locally
because they don't deal with some less usual (although by no means exotic,
e.g. just cancelling editing the commit message is enough to confuse the
latter script) situations. I know that I love to complain, but these
scripts are really quite user-unfriendly.

 I'm also hesitant to run the hook directly -- which is, if I understand
you correctly, what you propose -- because Git hooks are supposed to only
be run by Git and not manually and because outputting "COMMIT ABORTED" in
the build logs would be confusing.

 All in all, making "make check_concinnity" work as expected seems like
such an obviously better idea that it's very difficult for me to understand
why are you so loth to change it, even if it's perfectly clear that you
are.

GC> The hook does have one problem, which I haven't addressed yet
GC> because, while I'm somewhat less loth to touch that one, it
GC> doesn't seem trivial or fun. The problem is that if the
GC> 'test_coding_rules' binary needs to be rebuilt, the hook
GC> reports failure even when it will succeed once rebuilt,
GC> because rebuilding it spews out rebuilding commands that the
GC> hook does not expect. I'd be glad if you want to patch that.

 I don't really want to (in part because the whole rebuilding thing doesn't
work for me when using autotools anyhow), but this seems easy enough to fix
by just invoking "make ... custom_tools" first and ignoring any messages
from it, before the existing make command.

 Also, just in case it's worth stating explicitly, fixing "make
check_concinnity" would also fix this problem, as the hook could be
simplified and not do any output filtering at all and simply rely on the
exit code.


GC> Well...I deliberately try not to read those explanations, because
GC> skimming them sometimes makes me fear that a ponderous change
GC> is being proposed, when applying the changes shows it to be small.

 I'm really sorry, this definitely wasn't the intended effect and I'll stop
writing frightening amounts of explanations for any further changes like
this in the future. However I was still planing to write a cover letter for
the std::filesystem patch, would this be superfluous or, worse,
counterproductive, as well, or do you think that this could be useful for a
bigger change like this one?

 Please let me know and thanks in advance,
VZ

pgpZVbBsjRlui.pgp
Description: PGP signature