All,
I recently discovered that `git config` does not respect read-only files.
This caused unexpected difficulty in managing the global .gitconfig
for a system account shared by a large team. A team member was able to
execute a `git config --global` command without any notice or warning
that the underlying config file had been marked read-only in an
attempt to prevent unintentional changes. If instead git had raised a
warning saying that the "gitconfig is read-only" this would have
prevented that team member from accidentally breaking our git config.
Bug detail:
Due to the implementation strategy of
config::git_config_set_multivar_in_file_gently (
https://github.com/git/git/blob/5b33cb1fd733f581da07ae8afa7e9547eafd248e/config.c#L2074
) the file permissions of the target .gitconfig file are not
respected.
Proposal:
Part 1) Add a .gitconfig variable to respect a read-only gitconfig
file and optional "--force" override option for the `git config`
command
Such a gitconfig variable could be defined as:
config.respectFileMode: [ "never", "allow-override", "always" ]
Where:
* never - read-only file mode of config files are ignored (aka:
existing behavior)
* allow-override - read-only file mode of config files is respected
unless the user provides a "--force" option to `git config`
* always - read-only file mode of config files is respected (and the
"--force" option does not work)
Part 2) Change config::git_config_set_multivar_in_file_gently (
https://github.com/git/git/blob/5b33cb1fd733f581da07ae8afa7e9547eafd248e/config.c#L2077
) to verify write permissions on the destination depending on the
specified config.respectFileMode variable and "--force" option.
I think that this is a reasonably sized change that enables users to
opt-in to a 'strict mode' while preserving current behavior.
Thoughts?
Tested with:
OS: Linux
Version: 2.9.0 (issue exists in current master branch)
Am 08.11.2016 um 16:22 schrieb Jonathan Word:
Proposal:
Part 1) Add a .gitconfig variable to respect a read-only gitconfig
file and optional "--force" override option for the `git config`
command
Such a gitconfig variable could be defined as:
config.respectFileMode: [ "never", "allow-override", "always" ]
[...]
Thoughts?
I'd consider disrespecting file permissions to be a bug. Only very few tools allow to do so ('rm' is the only other one coming to mind right now), for good reason. If they do, only with additional parameters or by additional user interaction. Git should follow this strategy.
Which means: respect file permissions, no additional config variable and only if there's very substantial reason, add a --force. KISS.
That said, disrespecting permissions requires additional code, so it'd be interesting to know why this code was added. The relevant commit in the git.git repo should tell.
Markus
--
- - - - - - - - - - - - - - - - - - -
Dipl. Ing. (FH) Markus Hitter
http://www.jump-ing.de/
I proposed a variant that would be fully backwards-compatible (don't
know who might rely on the functionality http://xkcd.com/1172/ )
however I'd be happy to see the change without additional config +1
... that's a call for this list as maintainers.
The root of the issue is that tempfile::rename_tempfile (
https://github.com/git/git/blob/35f6318d44379452d8d33e880d8df0267b4a0cd0/tempfile.c#L288
) relies on http://man7.org/linux/man-pages/man2/rename.2.html which,
only requires directory write permissions - not file write
permissions. As you point out 'rm' is another example of this paradigm
and it works exactly the same way.
The point of confusion to users ( / my team) is that `git config`
gives the appearance of editing / modifying the .gitconfig file
in-place (where file permissions would be respected) however the
actual implementation performs the equivalent of a rm+mv which only
respects directory permissions.
The `git config` command is only one of many that leverage that
rename_tempfile function, if opting to respect file-level permissions
across the board then the desired change is probably at that level
rather than in config::git_config_set_multivar_in_file_gently which
would only add respect for file-level permissions to the one command.
Cheeers,
On Tue, Nov 8, 2016 at 11:49 AM, Markus Hitter [off-list ref] wrote:
Am 08.11.2016 um 16:22 schrieb Jonathan Word:
quoted
Proposal:
Part 1) Add a .gitconfig variable to respect a read-only gitconfig
file and optional "--force" override option for the `git config`
command
Such a gitconfig variable could be defined as:
config.respectFileMode: [ "never", "allow-override", "always" ]
[...]
Thoughts?
I'd consider disrespecting file permissions to be a bug. Only very few tools allow to do so ('rm' is the only other one coming to mind right now), for good reason. If they do, only with additional parameters or by additional user interaction. Git should follow this strategy.
Which means: respect file permissions, no additional config variable and only if there's very substantial reason, add a --force. KISS.
That said, disrespecting permissions requires additional code, so it'd be interesting to know why this code was added. The relevant commit in the git.git repo should tell.
Markus
--
- - - - - - - - - - - - - - - - - - -
Dipl. Ing. (FH) Markus Hitter
http://www.jump-ing.de/
On Tue, Nov 08, 2016 at 12:18:22PM -0500, Jonathan Word wrote:
The point of confusion to users ( / my team) is that `git config`
gives the appearance of editing / modifying the .gitconfig file
in-place (where file permissions would be respected) however the
actual implementation performs the equivalent of a rm+mv which only
respects directory permissions.
The reason for the tmpfile/rename is that git-config actually takes a
dot-lock on the file while writing it. Simultaneous writers are blocked,
and simultaneous readers see an atomic view of the file (either the
state before or after the write, but never a half-written file). Most
of git's file-writes are done this way.
The `git config` command is only one of many that leverage that
rename_tempfile function, if opting to respect file-level permissions
across the board then the desired change is probably at that level
rather than in config::git_config_set_multivar_in_file_gently which
would only add respect for file-level permissions to the one command.
I am not convinced this is a code problem and not simply a documentation
issue, but if you wanted to add an option to try to respect file
permissions, then yes, I agree it should be done across the board.
Probably converting "rename(from, to)" to first check "access(to,
W_OK)". That's racy, but it's the best we could do.
-Peff