Merge ~sahnaseredini/ubuntu-cve-tracker:amir-dev into ubuntu-cve-tracker:master

[Amir Naseredini]

Adding changes in order to catch unwanted unstaged errors!

There's a situation where the pre-commit-syntax-check hook might be bypassed, and this change is proposed in order to avoid that.

This happens if one makes changes to CVE files in active, makes them staged and tries to commit them, but the pre-commit-syntax-check hook complains that syntax in the one or more CVE files is not quite correct. In case you change the broken file (and you're like me!) and try to commit without staging the new changes, pre-commit-syntax-check hook does not complain and lets you commit, then push. This change is meant to stop this using a warning similar to the existing ones!

[Alex Murray (alexmurray)]

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 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?

[Steve Beattie (sbeattie)]

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>

[Amir Naseredini (sahnaseredini)]

Alex:
Thanks for the review. I agree that the check is a bit invasive, so I applied and pushed one of the Steve's suggestions. Now it's trying to get the staged files and checks if:
1- they're in UTC/active
2- they've got any unstaged changes

Also regarding moving the checks to pre-push hooks, when possible I generally like to see commits stable on themselves as opposed to in groups e.g. just in case someone needed to go back and checkout one of them. that's why I think having pre-commit hooks can still be a good idea. So I agree with Steve regarding having them.

Please let me know what you think.

Steve:
Thanks for the review. The new commit should mitigate the situation a lot more based on one of your suggestions and as explained above.

Please let me know what you think.

[Alex Murray (alexmurray)]

LGTM - thanks Amir!

review: Approve

diff --git a/scripts/git-hooks/pre-commit b/scripts/git-hooks/pre-commit
index aad92cb..dfd8618 100755
--- a/scripts/git-hooks/pre-commit
+++ b/scripts/git-hooks/pre-commit
@@ -35,6 +35,15 @@ set -e
 
 trap cleanup EXIT INT HUP
 
+if git diff --staged --name-only | grep --quiet "active"; then
+    if ! git diff --quiet $(git diff --staged --name-only | grep "active"); then
+        read -p 'Found unstaged changes in staged files from $UTC/active. Enter "ignore" to continue or anything else to exit: ' answer < /dev/tty
+        if ! [ "${answer}" = "ignore" ] ; then
+            exit 1
+        fi
+    fi
+fi
+
 if [ -z "${UCT_IGNORE_CHECK_SYNTAX}" ] ; then
     modified_files="$(mktemp -t ucthook-XXXXXXXXXX)"
     modified_json_files="$(mktemp -t ucthook-XXXXXXXXXX)"