Revert "[Tiny PR] Add updated README files during precommit"

[ellatrix]

Reverts #18657.

I don't know how to fix the problem. It's super annoying that the recommit hook creates changes though.

• Either we should add them to the commit, but only the changes created by the docgen command.
• Or we should error when docgen changed any files so the changes can be added manually.

@nosolosw, do you have any idea on how to approach this?

In the meantime this PR reverts #18657.

[aduth]

We can work it out separately 👍

[epiqueras]

Thanks for fixing this so quickly 😄

[oandregal]

A few thoughts:

• docgen doesn't compare the before/after state of the files it modifies (so can't output a non-zero exit code). This is a git-land utility.
• It's doable to write a pre-commit script that builds upon the existing bin/update-readmes. The way I'd do it is by making the update-readmes script output the list of files it touches, and then having a bin/pre-commit script that does something with those.
• Automatically adding the modified READMES can be problematic with edge cases and the way people uses git. Example (this would get even trickier when people only stage part of the files):
  • you do some code modifications that would trigger a documentation change in READMEs
  • you add to the git staging area only a subset of those changes
  • when committing, all READMEs get updated and committed despite only a subset of them should have been (the ones affected by the staged changes)
  • you push the changes
  • CI receives the git push and executes the check-local-changes script (which check whether there are differences between the docs generated by the CI job and the docs included in the commit). At this point, CI reports an error because the READMEs received are different from the READMEs that it builds.

Given this, I'd go for writing a pre-commit hook that prevents you from committing if there are uncommitted changes to documentation. Does this sound good? I can prepare something in the coming days if this is the direction we want to go.

[ellatrix]

Given this, I'd go for writing a pre-commit hook that prevents you from committing if there are uncommitted changes to documentation. Does this sound good? I can prepare something in the coming days if this is the direction we want to go.

That sounds great! Thank you!

[epiqueras]

Why not just build the docs for staged changes only?

We can stash the other changes while building.

[oandregal]

Why not just build the docs for staged changes only?

I thought about this as well. If it was only for files that are staged whole that'd be great. Unfortunately, that assumption is not true in a lot of cases: staging only a part of a file is something that I regularly do and I've seen other people doing as well. At that point, building the docs for staged changes gets more complicated and covering that use case would be an order of magnitude more complex (if not more). I believe we can get a bigger return of investment with the pre-commit hook.

[epiqueras]

That makes sense. Thanks for clarifying! 😄

[oandregal]

It looks like husky, the package we use to run git hooks, was updated to the v3 line by #17310, which means it requires git > 2.13.2. My OS bundles a lower git version, so I've just realized that existing hooks don't work for me (lint, auto doc generation, etc).

I guess if we haven't had reports for this it in the past couple of months it means that the issue isn't widespread and people do have a compatible git version. What a lovely way to start this task, though.