Code review comment for ~sahnaseredini/ubuntu-cve-tracker:amir-dev

I am also onboard with the intent of this change as I have likely
been tripped up by the issue it's trying to address at least once,
and think about while I'm waiting for check-syntax to finish.
Thanks Amir for pushing on it.

On Tue, Dec 12, 2023 at 12:50:33AM -0000, Alex Murray wrote:
> Whilst I like the purpose of this change, I wonder if it may be too
> invasive as it breaks the ability to easily break up one large change
> into multiple smaller commits (without then having to 'ignore' each
> until the last one is reached).

I have similar concerns, because I do try to break up commits in such
a way when I can. One mitigating factor for it as proposed is that it
only is testing the active/ subdirectory, which at least makes code
changes (i.e. the scripts/ directory tree) not subject to it.

I had a couple of thoughts on how to target the check more closely:

- move the check for unstaged changes inside the
  $UCT_IGNORE_CHECK_SYNTAX check, though honestly this is not much
  different than git commit -n.

- Only warn about unstaged changes in the files that are modified
  as part of the current commit. This would likely catch the common
  "commit, check-syntax whinges about something, fix it, forget to git
  add the fix, commit" cycle, while reducing the chances of tripping
  on multiple small changes (though at least for code, I do use git
  add -p sometimes, less so I think for CVE entries).

In an *ideal* world, git wold give us a snapshot of the staged changes
and our pre-commit hooks would be directed to operate on that.
Unfortunately, it does not, and hacking something up to do that
in the pre-commit hook would probably make an already non-trivial
operation take even longer.

> I wonder if we should instead move our hooks to `pre-push` rathar than
> `pre-commit` since then we can try and ensure the entire set of changes
> is consistent?

Hrm, maybe. I do like having the individual commits be generally
consistent, too.

--
Steve Beattie
<email address hidden>