{"thread":{"id":"40907","subject":"Git 'pre-receive' hook and 'git-clang-format' script to reliably reject pushes that violate code style conventions","startedAt":"2015-11-28T22:18:15Z","lastAt":"2015-12-01T15:46:10Z","messageCount":2,"participants":["Alexander Shukaev"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"273818","messageId":"565A2827.3000609@Alexander.Shukaev.name","threadId":"40907","inReplyTo":null,"subject":"Git 'pre-receive' hook and 'git-clang-format' script to reliably reject pushes that violate code style conventions","fromName":"Alexander Shukaev","fromEmail":"git@alexander.shukaev.name","sentAt":"2015-11-28T22:18:15Z","receivedAt":"2015-11-28T22:18:15Z","isPatch":false,"sender":{"key":"git@alexander.shukaev.name","avatar":null},"body":"Hello,\n\nI have posted a question on Stack Overflow [1].  I would like to post it \nhere as well since nobody has answered it yet.\n\nLet's immediately start with a scrap of the `pre-receive` hook that I've \nalready written:\n\n     #!/bin/sh\n     ##\n       format_bold='\\e[1m'\n        format_red='\\e[31m'\n     format_normal='\\e[0m'\n     ##\n     log_error=\"${format_bold}${format_red}error${format_normal}\"\n     ##\n     stdout() {\n       printf \"${1}\" \"${@:2}\"\n     }\n     ##\n     stderr() {\n       stdout \"${@}\" 1>&2\n     }\n     ##\n     output() {\n       stdout \"${1}\\n\" \"${@:2}\"\n     }\n     ##\n     error() {\n       stderr \"${log_error}: ${1}\\n\" \"${@:2}\"\n     }\n     ##\n     die() {\n       error \"${@}\"\n       exit 1\n     }\n     ##\n     list() {\n       git rev-list \"${@}\"\n     }\n     ##\n     clang_format() {\n       git --no-pager clang-format --style='file' \"${@}\"\n     }\n     ##\n     while read sha1_old sha1_new ref; do\n       case \"${ref}\" in\n       refs/heads/*)\n         branch=\"$(expr \"${ref}\" : 'refs/heads/\\(.*\\)')\"\n         if [ \"$(expr \"${sha1_new}\" : '0*$')\" -ne 0 ]; then # delete\n           unset sha1_new\n           # ...\n         else # update\n           if [ \"$(expr \"${sha1_old}\" : '0*$')\" -ne 0 ]; then # create\n             unset sha1_old\n             sha1_range=\"${sha1_new}\"\n           else\n             sha1_range=\"${sha1_old}..${sha1_new}\"\n             # ...\n             fi\n           fi\n           # ...\n                  GIT_WORK_TREE=\"$(mktemp --tmpdir -d 'gitXXXXXX')\"\n           export GIT_WORK_TREE\n                  GIT_DIR=\"${GIT_WORK_TREE}/.git\"\n           export GIT_DIR\n           mkdir -p \"${GIT_DIR}\"\n           cp -a * \"${GIT_DIR}/\"\n           ln -s \"${PWD}/../.clang-format\" \"${GIT_WORK_TREE}/\"\n           error=\n           for sha1 in $(list \"${sha1_range}\"); do\n             git checkout --force \"${sha1}\" > '/dev/null' 2>&1\n             if [ \"$(list --count \"${sha1}\")\" -eq 1 ]; then\n               # What should I put here?\n             else\n               git reset --soft 'HEAD~1' > '/dev/null' 2>&1\n             fi\n             diff=\"$(clang_format --diff)\"\n             if [ \"${diff%% *}\" = 'diff' ]; then\n               error=1\n               error '%s: %s\\n%s'                                       \\\n                     'Code style issues detected'                       \\\n                     \"${sha1}\"                                          \\\n                     \"${diff}\"                                          \\\n                     1>&2\n             fi\n           done\n           if [ -n \"${error}\" ]; then\n             die '%s' 'Code style issues detected'\n           fi\n         fi\n         ;;\n       refs/tags/*)\n         tag=\"$(expr \"${ref}\" : 'refs/tags/\\(.*\\)')\"\n         # ...\n         ;;\n       *)\n         # ...\n         ;;\n       esac\n     done\n     exit 0\n\n**NOTE:**\nPlaces with irrelevant code are stubbed with `# ...`.\n\n**NOTE:**\nIf you are not familiar with `git-clang-format`, take a look [here][2].\n\nThat hook works as expected, and so far, I didn't notice any bugs, but \nif you spot any problem or have an improvement suggestion, I'd \nappreciate any report.  Probably, I should give a comment on what's the \nintention behind this hook.  Well, it does check every pushed revision \nfor compliance with code style conventions using `git-clang-format`, and \nif any of them does not comply, it will output the relevant diff (the \none telling developers what should be fixed) for each of them. \nBasically, I have two in-depth questions regarding this hook.\n\nFirst, notice that I perform copy of the remote's (server) bare \nrepository to some temporary directory and check out the code for \nanalysis there.  Let me explain the intention of this.  Note that I do \nseveral `git checkout`s and `git reset`s (due to `for` loop) in order to \nanalyze all of the pushed revisions individually with \n`git-clang-format`.  What I am trying to avoid here, is the (possible) \nconcurrency issue on push access to the remote's (server) bare \nrepository.  That is, I'm under impression that if multiple developers \nwill try to push at the same time to a remote with this `pre-receive` \nhook installed, that might cause problems if each of these push \n\"sessions\" does not do `git checkout`s and `git reset`s with its private \ncopy of the repository.  So, to put it simple, does `git-daemon` have \nbuilt-in lock management for concurrent push \"sessions\"?  Will it \nexecute the corresponding `pre-receive` hook instances strictly \nsequentially or there is a possibility of interleaving (which can \npotentially cause undefined behavior)?  Something tells me that there \nshould be a built-in solution for this problem with concrete guarantees, \notherwise how would remotes work in general (even without complex hooks) \nbeing subjected to concurrent pushes?  If there is such a built-in \nsolution, then the copy is redundant and simply reusing the bare \nrepository would actually speed up the processing.  By the way, any \nreference to official documentation regarding this question is very welcome.\n\nSecond, `git-clang-format` processes only *staged* (but not committed) \nchanges vs. specific commit (`HEAD` by default).  Thus, you can easily \nsee where a corner case lies.  Yes, it's with the *root* commits \n(revisions).  In fact, `git reset --soft 'HEAD~1'` cannot be applied to \nroot commits as they have no parents to reset to.  Hence, the following \ncheck with my second question is there:\n\n             if [ \"$(list --count \"${sha1}\")\" -eq 1 ]; then\n               # What should I put here?\n             else\n               git reset --soft 'HEAD~1' > '/dev/null' 2>&1\n             fi\n\nI've tried `git update-ref -d 'HEAD'` but this breaks the repository in \nsuch a way that `git-clang-format` is not able to process it anymore.  I \nbelieve this is related to the fact that all of these pushed revisions \nthat are being analyzed (including this root one) do not really belong \nto any branch yet.  That is, they are in *detached* `HEAD` state.  It \nwould be perfect to find a solution to this corner case as well, so that \n*initial* commits can also undergo the same check by `git-clang-format` \nfor compliance with code style conventions.\n\nPeace.\n\n[1] http://stackoverflow.com/q/33924519/1743860\n[2] \nhttp://llvm.org/svn/llvm-project/cfe/trunk/tools/clang-format/git-clang-format\n"},{"id":"273860","messageId":"565DC0C2.6040309@Alexander.Shukaev.name","threadId":"40907","inReplyTo":"565A2827.3000609@Alexander.Shukaev.name","subject":"Re: Git 'pre-receive' hook and 'git-clang-format' script to reliably reject pushes that violate code style conventions","fromName":"Alexander Shukaev","fromEmail":"git@alexander.shukaev.name","sentAt":"2015-12-01T15:46:10Z","receivedAt":"2015-12-01T15:46:10Z","isPatch":false,"sender":{"key":"git@alexander.shukaev.name","avatar":null},"body":"I don't want to smell like a nasty bumper here, but assuming that my \nquestions were posted not in appropriate time (Saturday evening), I \nwould like to call for help one last time.  Thank you.\n\nKind regards,\nAlexander\n"}]}