> At this stage the files are now indented, so if it failed and you run > `git commit` again it will commit with the indention changes.
No, because at no point a "git add" is happening, so the changes made by pgindent are not staged. As long as you don't run the second "git commit" with the -a flag the commit will be exactly the same as you prepared it before. > Sure, that's your choice. My intended audience here is committers, who > of course do work on master. Yes I understand, I meant it would be nice if the script had a environment variable like PG_COMMIT_HOOK_ALL_BRANCHES (bad name) for this purpose. On Thu, 26 Jan 2023 at 17:54, Andrew Dunstan <and...@dunslane.net> wrote: > > > On 2023-01-26 Th 11:16, Jelte Fennema wrote: > > On Thu, 26 Jan 2023 at 15:40, Andrew Dunstan <and...@dunslane.net> wrote: > >> I didn't really like your hook, as it forces a reindent, and many people > >> won't want that (for reasons given elsewhere in this thread). > > I'm not sure what you mean by "forces a reindent". Like I explained > > you can simply run "git commit" again to ignore the changes and > > commit anyway. As long as the files are indented on your filesystem > > the hook doesn't care if you actually included the indentation changes > > in the changes that you're currently committing. > > > Your hook does this: > > > +git diff --cached --name-only --diff-filter=ACMR | grep '\.[ch]$' |\ > + xargs src/tools/pgindent/pgindent --silent-diff \ > + || { > + echo ERROR: Aborting commit because pgindent was not run > + git diff --cached --name-only --diff-filter=ACMR | grep > '\.[ch]$' | xargs src/tools/pgindent/pgindent > + exit 1 > + } > > > At this stage the files are now indented, so if it failed and you run > `git commit` again it will commit with the indention changes. > > > > > > So to be completely clear you can do the following with my hook: > > git commit # runs pgindent and fails > > git commit # commits changes anyway > > git commit -am 'Run pgindent' # commit indentation changes separately > > > > Or what I usually do: > > git commit # runs pgindent and fails > > git add --patch # choose relevant changes to add to commit > > git commit # commit the changes > > git checkout -- . # undo irrelevant changes on filesystem > > > > Honestly PGAUTOINDENT=no seems stricter, since the only > > way to bypass the failure is now to run manually run pgindent > > or git commit with the --no-verify flag. > > > >> files=$(git diff --cached --name-only --diff-filter=ACMR) > >> src/tools/pgindent/pgindent $files > > That seems like it would fail if there's any files or directories with > > spaces in them. Maybe this isn't something we care about though. > > > We don't have any, and the filenames git produces are relative to the > git root. I don't think this is an issue. > > > > > >> # no need to filter files - pgindent ignores everything that isn't a > >> # .c or .h file > > If the first argument is a non .c or .h file, then pgindent interprets > > it as the typedefs file. So it's definitely important to filter non .c > > and .h files out. Because now if you commit a single > > non .c or .h file this hook messes up the indentation in all of > > your files. You can reproduce by running: > > src/tools/pgindent/pgindent README > > > > I have a patch at [1] to remove this misfeature. > > > > > >> # only do this on master > >> test "$branch" = "master" || return 0 > > I would definitely want a way to disable this check. As a normal > > submitter I never work directly on master. > > > Sure, that's your choice. My intended audience here is committers, who > of course do work on master. > > > cheers > > > andrew > > > [1] https://postgr.es/m/21bb8573-9e56-812b-84cf-1e4f3c4c2...@dunslane.net > > -- > Andrew Dunstan > EDB: https://www.enterprisedb.com >