[Commented On] D10538: tests: allow trunk versions of clang-format to be used

Sun Oct 17 19:09:10 UTC 2021

durin42 added a comment.

  In D10538#179050 <https://phab.mercurial-scm.org/D10538#179050>, @marmoute wrote:

  > In D10538#179035 <https://phab.mercurial-scm.org/D10538#179035>, @spectral wrote:
  >
  >> I'm still struggling to identify where my proposed fix makes any of this worse.
  >> Current state: people with a version of clang-format <11, or people with a 'trunk' version of clang-format, don't get their code run through clang-format at all, and send changes that will break CI when they land (unless the issue is caught during review).
  >> Proposed state: that no longer happens for people running a 'trunk' version of clang-format, just those using clang-format <11.
  >
  > I feel like the proposal is going in the wrong direction because it make a more diverse set of version passe the version checks while I feel like we need to more toward a narrower check. Different formater version tends to produce different results and it seems saner to pin the project to specific versions (that we update from time to time).
  > For the record, right now we have :
  >
  > - black: check says ">= 20.8b1", CI uses "20.8b1"
  > - clang: check says ">= 11", CI uses debian's clang-format-11 package (in practice "Debian clang-format version 11.1.0-++20210622113218+1fdec59bffc1-1~exp1~20210622213839.163")
  > - rust: check says "nightly-2020-10-04", CI uses "nightly-2020-10-04"
  >
  > Is my feedback clearer and does it make sense to you? Or do you feel like I missed something?

  Do we know of specific formatting differences between `clang-format` 11/12/13/HEAD? It could be that `clang-format` has stabilized enough for C that we're fussing over nothing. If we're not fussing over nothing, maybe we should have a "swat a fly with a buick" solution and check in a script that abuses docker to always run an exact clang-format that contributors can use when they touch C code. I think that `hg fix` can be taught to only reformat edited lines too, so I would expect that in the common case there aren't any formatting differences in the vast majority of our patches, so we're strictly better off with contributors being bugged by clang-format test failures than not? Maybe we can add a --check mode to `hg fix` that could check only edited lines, thus dramatically reducing the rate of false positives?

  Assuming there is substantial formatting skew: could we add an environment variable here a la "HGHAVE_ACCEPT_ANY_CLANG_FORMAT" so that people can opt-in to seeing HEAD-formatter results? Or maybe there's no substantive skew between clang 12 (or clang 13) and HEAD, and we can update the specific version we require.

  (Basically, I'm wondering if we're arguing about a hypothetical problem, or an actual one. And if it's an actual one, if we can find some way to only format-check draft-phase patches that probably solves the issue anyway...)

  >> Potential differences between those two states (all of these assume that the user authoring the changes are using a 'trunk' version that produces formatting incompatible with the "released version that is >=11" check we currently have, either because their 'trunk' version is too old OR too new; I'm assuming that having a 'trunk' version installed is already rather rare for non-Googlers):
  >>
  >> - [extremely unlikely] the user runs check-code-format, it produces a warning for *only* their changes, they format it with that style of formatting, and send the change. This is *indistinguishable* for reviewers and automation from the user just formatting it like this themselves, not having clang-format installed at all. This is extremely unlikely because any sort of formatting breakages are almost certainly going to affect more than just the changes the user is making.
  >> - [very unlikely] the user runs check-code-format, it produces a warning for their changes and a bunch of other unrelated files, they *don't* ask "what's up with this" and instead send a change with *just* a series of fixes to make the entire codebase compatible with their version of clang-format (and incompatible with the version(s) we officially support. That was really nice of them to do that, but it's easy to reject the change if this happens; it's a shame they spent time on it just to have it rejected, but I'm assuming the likelihood of this is very low, as I stated -- either they'll ask "why is this test broken in files I didn't modify", and we reply "your clang-format is an unsupported version", or they'll just ignore the output of that test, assuming it's "someone else's problem". If they then send a change that is formatted using the incompatible clang-format, we're back where we started: this is indistinguishable from the user not having clang-format installed at all.
  >
  > My own experience is more about running blindly formatting on my whole stack (`hg fix --rev .#stack`) and as this usually happens after I checked  the details of the commits, unrelated formatting change might goes unnoticed. So laxer formatter version check might bite me here.

  `hg fix` would already not check the version of `clang-format` involved, so I'm not sure this is strictly related.

  >> - [likely] I'll break the CI with bad formatting less often :)
  >
  > May I convince you todo CI runs on your changes before submitting them for review ? That would achieve the same purpose, and apply to more than just bad formatting.

  That's an extra step, and one that requires registering an account on a new service, etc. I don't like adding the expectation that people will use a (proprietary?) service as part of the contribution flow, even if that service is owned and operated by friends of the project.

  I suppose, worst case, we could have a bot that can recognize formatting issues and just _does_ `hg fix` with the canonical version of formatters, and then it's not a big deal.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D10538/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D10538

To: spectral, #hg-reviewers, durin42
Cc: marmoute, durin42, martinvonz, joerg.sonnenberger, mercurial-patches
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.mercurial-scm.org/pipermail/mercurial-patches/attachments/20211017/139cb0b5/attachment-0002.html>