src/usermod.c: Some cleanups, improvements, fixes, and abstractions about open/close/lock/unlock on database files

[alejandro-colomar]

This is a cleanup before starting to work on moving this to lib/ and using it everywhere.

Cc: @jcpunk, @stoeckmann

---

Revisions:

v2

• Don't compact conditionals.

$ git rd 
 1:  f3527e58 =  1:  f3527e58 src/usermod.c: wsfix
 2:  7569e1eb =  2:  7569e1eb src/usermod.c: Remove dead code
 3:  dd7ff117 =  3:  dd7ff117 src/usermod.c: Set 'sgr_locked = true' only if we really locked
 4:  cebb97ac =  4:  cebb97ac src/usermod.c: Only close files if they were locked
 5:  924c8fae =  5:  924c8fae src/usermod.c: Remove unnecessary nesting
 6:  bf95899d =  6:  bf95899d src/usermod.c: Merge variables into structure
 7:  3063065e =  7:  3063065e src/usermod.c: fail_exit(): Unset lk fields after unlocking
 8:  37fa83a1 =  8:  37fa83a1 src/usermod.c: Split unlock_db() helper from fail_exit()
 9:  c096177f =  9:  c096177f src/usermod.c: Split close_db() helper from close_files()
10:  f824c16d = 10:  f824c16d src/usermod.c: close_db(): First close all files, then unlock all files
11:  431f9888 <  -:  -------- src/usermod.c: close_db(): Compact conditionals
12:  32eb81b6 = 11:  ffa3e457 src/usermod.c: open_files(): Refactor conditionals
13:  36487877 = 12:  720ee763 src/usermod.c: Split open_db() from open_files()

[alejandro-colomar]

@jcpunk , @stoeckmann , since you've recently looked at this code, it'd be good if you could review this PR.

The functions open_db(), close_db(), and unlock_db() are the ones I pretend to move to lib/, along with the type struct lk_db_files (BTW, feel free to suggest better names).

[jcpunk]

The bits I understand all look good to me.

[jcpunk]

Should there be a top level .clang-format so a single massive reformat commit could obsolete the incremental whitespace changes and get put into the github ci?

[alejandro-colomar]

Should there be a top level .clang-format so a single massive reformat commit could obsolete the incremental whitespace changes and get put into the github ci?

I'm not yet convinced about it. But I'll experiment with it.

[alejandro-colomar]

Should there be a top level .clang-format so a single massive reformat commit could obsolete the incremental whitespace changes and get put into the github ci?

I've been experimenting with it, and found it to be terrible. (Maybe I just don't know how to use it correctly?)

I'd like to enforce a few style things, but clang-format(1) seems too strict. Is there a way to tell clang-format(1) "I want to fix spaces between a function and its ( but ignore everything else"?

---

It seems clang-format(1) is indeed as terrible as it seems:
https://stackoverflow.com/questions/30763869/how-can-i-apply-only-one-clang-format-action.

[jcpunk]

For just space removal, I whipped up a few quick sed(1) runs and ended up with : #1593

[stoeckmann]

@jcpunk , @stoeckmann , since you've recently looked at this code, it'd be good if you could review this PR.

The functions open_db(), close_db(), and unlock_db() are the ones I pretend to move to lib/, along with the type struct lk_db_files (BTW, feel free to suggest better names).

Looks good to me.

With these error handlings in open_db() and close_db(), the unlock function could become private to the library, i.e. not callable by applications. Is that your plan as well?

[stoeckmann]

It seems clang-format(1) is indeed as terrible as it seems: https://stackoverflow.com/questions/30763869/how-can-i-apply-only-one-clang-format-action.

I wouldn't call it terrible. It's just not designed for performing incremental steps. One project under clang-format governance is https://github.com/kmod-project/kmod (and yes, I really mean governance if it's in place ...)

You can specify exceptions in files if you disagree with the format, but otherwise it comes in handy as a guard for incoming pull requests and is a one-liner to call.

[stoeckmann]

On the other hand, I also know glib (and thus GNOME as a whole) and their rule is to only properly format a line which is already modified due to a PR. This in turn means that it'll never have any finished code style in it ... ;)

But blaming code lines is way more meaningful this way, after all you are not annoyed with "I have fixed the code style" annotations.

[jcpunk]

My reading of https://git-scm.com/docs/git-blame suggests a file could be used to ignore specific commits for git blame.

[alejandro-colomar]

@jcpunk , @stoeckmann , since you've recently looked at this code, it'd be good if you could review this PR.
The functions open_db(), close_db(), and unlock_db() are the ones I pretend to move to lib/, along with the type struct lk_db_files (BTW, feel free to suggest better names).

Looks good to me.

Thanks!

With these error handlings in open_db() and close_db(), the unlock function could become private to the library, i.e. not callable by applications. Is that your plan as well?

I hadn't thought about it, but we could do it. But then fail_exit() should either be part of the library, or it should close_db() instead of unlock_db(). Any opinion on that?

[stoeckmann]

I hadn't thought about it, but we could do it. But then fail_exit() should either be part of the library, or it should close_db() instead of unlock_db(). Any opinion on that?

Oh, right. We need it to avoid actually writing out data in case of error. Then it's part of the API.

[alejandro-colomar]

It seems clang-format(1) is indeed as terrible as it seems: https://stackoverflow.com/questions/30763869/how-can-i-apply-only-one-clang-format-action.

I wouldn't call it terrible. It's just not designed for performing incremental steps.

The problem is a bit more complex than that.

Most rules should have exceptions, which humans with good taste could choose to apply. Such a strict automation isn't able to do that.

And some rules should be more fine grained: for example, I like indenting function calls after the (, but I prefer indenting function declarations with 4 spaces. Since this distinction isn't implemented in clang-format(1), I must choose one or the other.

Instead, a good design of a tool would recognize that it has limitations, and it should allow specifying that one doesn't want to apply a rule in any currently implemented ways.

One project under clang-format governance is https://github.com/kmod-project/kmod (and yes, I really mean governance if it's in place ...)

You can specify exceptions in files if you disagree with the format, but otherwise it comes in handy as a guard for incoming pull requests and is a one-liner to call.

The thing is I don't want exceptions by file, but by rule.

[alejandro-colomar]

My reading of https://git-scm.com/docs/git-blame suggests a file could be used to ignore specific commits for git blame.

I think that's a bad idea. Such commits could introduce bugs accidentally, so git-blame(1) should show everything. I prefer ignoring them in my head. Tools should not ignore them.