On Sat, Jan 11, 2025 at 10:07:57AM -0500, rsbecker@xxxxxxxxxxxxx wrote:
> Question from my community. The standard git install does not
> automatically create the /usr/local/etc directory if it does not
> exist, so the git config --system fails. If etc/gitconfig is missing,
> the command also fails. Similarly, if ~/.gitconfig is missing, git
> config -global fails.
It's not the missing file that is the problem, but the containing
directory, right? I.e.:
$ make ETC_GITCONFIG=/tmp/foo/bar
[reading returns an error, but that is natural because we did not
find the config key in question]
$ ./git config --system foo.bar; echo $?
1
[but writing will fail to create the lock file]
$ ./git config --system foo.bar baz
error: could not lock config file /tmp/foo/bar: No such file or directory
> Is this intended? I have been telling my people that they should
> pre-create those files with appropriate security. I have mused that
> having git auto-create this file may introduce potential CVEs,
> explaining the situation. Is this assumption correct?
This is what I'd expect. We do often automatically create leading
directories as necessary within the repository, but it gets weird
outside of that (and you probably wouldn't have permissions anyway).
It's less weird perhaps as part of "make install", but it still feels
like it is outside of Git's scope, and is more of a packaging issue. If
you're feeding /usr/local/etc to Git's build knobs, then you probably
should be the one making sure it exists.
All of this applies doubly so for ~/.gitconfig. We cannot do it at "make
install" time, since there may be many users. We could auto-create it at
run-time, but if the user's home directory does not exist that is likely
either a sign that something is terrible misconfigured, or the user in
question is not meant to have a home directory (daemon user ids, etc).
So creating it would probably be surprising.
I do think the error message could be a little more friendly; the
problem is not that /tmp/foo/bar does not exist, but that some leading
directory does not. But that is the nature of ENOENT.
We don't even get to see the actual lock filename fed to open(), since
it is done behind the scenes in hold_lock_file_for_update(). Possibly we
should be using unable_to_lock_message(). Or hmm, looks like we can feed
a flag to do it for us, like:
diff --git a/config.c b/config.c
index 50f2d17b39..5f8e0c667a 100644
--- a/config.c
+++ b/config.c
@@ -3206,9 +3206,9 @@ int repo_config_set_multivar_in_file_gently(struct repository *r,
* The lock serves a purpose in addition to locking: the new
* contents of .git/config will be written into it.
*/
- fd = hold_lock_file_for_update(&lock, config_filename, 0);
+ fd = hold_lock_file_for_update(&lock, config_filename,
+ LOCK_REPORT_ON_ERROR);
if (fd < 0) {
- error_errno(_("could not lock config file %s"), config_filename);
ret = CONFIG_NO_LOCK;
goto out_free;
}
That is only marginally better, though:
$ ./git config --system foo.bar baz
error: Unable to create '/tmp/foo/bar.lock': No such file or directory
It does diagnose other problems like EEXIST, and IMHO it would not be
unreasonable for it to recognize ENOENT and see if we can even stat()
the surrounding directory.
-Peff
