receive.denyNonNonFastForwards not denying force update

receive.denyNonNonFastForwards not denying force update

[John Arthorne]

At eclipse.org we wanted all git repositories to disallow non-fastforward
commits by default. So, we set receive.denyNonFastForwards=true as a system
configuration setting. However, this does not prevent a non-fastforward
force push. If we set the same configuration setting in the local repository
configuration then it does prevent non-fastforward pushes.

For all the details see this bugzilla, particularly comment #59 where we
finally narrowed this down:

https://bugs.eclipse.org/bugs/show_bug.cgi?id=343150

This is on git version 1.7.4.1.

The Git book recommends setting this property at the system level:

http://git-scm.com/book/ch7-1.html (near the bottom)

Can someone confirm if this is intended behaviour or not. We ended up
using a script to set a local config property in each repository, but
with several hundred git repositories it would be much easier if the
system setting was honoured.

Thanks,
John Arthorne

Re: receive.denyNonNonFastForwards not denying force update

[Junio C Hamano]

John Arthorne <arthorne.eclipse@gmail.com> writes:

> For all the details see this bugzilla, particularly comment #59 where we
> finally narrowed this down:
>
> https://bugs.eclipse.org/bugs/show_bug.cgi?id=343150

What does "at the system level" in your "does *not* work at the
system level." exactly mean?

A configuration variable in the repository configuration take
precedence over user preference $HOME/.gitconfig which in turn take
precedence over system wide default /etc/gitconfig (or whereever you
or your distro decided to place it).  In other words, if you have
"[receive] denyNonFastForwards" in the system wide default, you can
say "[receive] denyNonFastForwards = false" in one particular
repository to allow it for that repository.

Re: receive.denyNonNonFastForwards not denying force update

[Sitaram Chamarty]

On Mon, Aug 20, 2012 at 10:35 PM, Junio C Hamano <gitster@pobox.com> wrote:
> John Arthorne <arthorne.eclipse@gmail.com> writes:
>
>> For all the details see this bugzilla, particularly comment #59 where we
>> finally narrowed this down:
>>
>> https://bugs.eclipse.org/bugs/show_bug.cgi?id=343150
>
> What does "at the system level" in your "does *not* work at the
> system level." exactly mean?

"git config --system receive.denynonfastforwards true" is not honored.
 At all.  (And I checked there was nothing overriding it).

"--global" does work (is honored).

Tested on 1.7.11

Re: receive.denyNonNonFastForwards not denying force update

[Junio C Hamano]

Sitaram Chamarty <sitaramc@gmail.com> writes:

> On Mon, Aug 20, 2012 at 10:35 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> John Arthorne <arthorne.eclipse@gmail.com> writes:
>>
>>> For all the details see this bugzilla, particularly comment #59 where we
>>> finally narrowed this down:
>>>
>>> https://bugs.eclipse.org/bugs/show_bug.cgi?id=343150
>>
>> What does "at the system level" in your "does *not* work at the
>> system level." exactly mean?
>
> "git config --system receive.denynonfastforwards true" is not honored.
>  At all.  (And I checked there was nothing overriding it).
>
> "--global" does work (is honored).
>
> Tested on 1.7.11

Thanks, and interesting.

Does anybody recall if this is something we did on purpose?  After
eyeballing the callchain starting from cmd_receive_pack() down to
receive_pack_config(), nothing obvious jumps at me.

Could this be caused by a chrooted environment not having
/etc/gitconfig (now I am just speculating)?

A quick "strace -f -o /tmp/tr git push ../neigh" seems to indicate
that at least access() is called on "/etc/gitconfig" as I expect,
which makes me think that near the beginning of git_config_early(),
we would read from /etc/gitconfig if the file existed (I do not
install any distro "git", so there is no /etc/gitconfig on my box).

Puzzled.

Re: receive.denyNonNonFastForwards not denying force update

[Brandon Casey]

On Mon, Aug 20, 2012 at 6:22 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Sitaram Chamarty <sitaramc@gmail.com> writes:
>
>> On Mon, Aug 20, 2012 at 10:35 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> John Arthorne <arthorne.eclipse@gmail.com> writes:
>>>
>>>> For all the details see this bugzilla, particularly comment #59 where we
>>>> finally narrowed this down:
>>>>
>>>> https://bugs.eclipse.org/bugs/show_bug.cgi?id=343150
>>>
>>> What does "at the system level" in your "does *not* work at the
>>> system level." exactly mean?
>>
>> "git config --system receive.denynonfastforwards true" is not honored.
>>  At all.  (And I checked there was nothing overriding it).
>>
>> "--global" does work (is honored).
>>
>> Tested on 1.7.11
>
> Thanks, and interesting.
>
> Does anybody recall if this is something we did on purpose?  After
> eyeballing the callchain starting from cmd_receive_pack() down to
> receive_pack_config(), nothing obvious jumps at me.
>
> Could this be caused by a chrooted environment not having
> /etc/gitconfig (now I am just speculating)?
>
> A quick "strace -f -o /tmp/tr git push ../neigh" seems to indicate
> that at least access() is called on "/etc/gitconfig" as I expect,
> which makes me think that near the beginning of git_config_early(),
> we would read from /etc/gitconfig if the file existed (I do not
> install any distro "git", so there is no /etc/gitconfig on my box).
>
> Puzzled.

Seems to work for me.  Force push was denied when
receive.denyNonFastForwards was set to true in system-level gitconfig.
 Tested with git installed in my home directory, so my system-level
gitconfig was at $HOME/etc/gitconfig.

Sitaram and John, are you sure you modified the correct file?  Also be
sure you're using the git-receive-pack that expects the system
gitconfig at the place that you think it is.

The system-level gitconfig is hard-coded in the git binary and may not
always be at /etc/gitconfig.  It is usually set to be relative to the
installation directory "$prefix" in the Makefile.  I don't think we
expose the path to the system-level gitconfig file anywhere in the ui.
 One way to figure out where it should be is to use 'git config' to
edit it like this:

   git config --system -e

Hopefully your editor exposes the path that it is editing even if you
don't have permission to modify it.

I'm thinking that the git-receive-pack binary that you guys used
expects the system gitconfig to be in a different location than the
one you modified.

-Brandon

Re: receive.denyNonNonFastForwards not denying force update

[Jeff King]

On Mon, Aug 20, 2012 at 06:22:26PM -0700, Junio C Hamano wrote:

> > "git config --system receive.denynonfastforwards true" is not honored.
> >  At all.  (And I checked there was nothing overriding it).
> >
> > "--global" does work (is honored).
> >
> > Tested on 1.7.11
> 
> Thanks, and interesting.
> 
> Does anybody recall if this is something we did on purpose?  After
> eyeballing the callchain starting from cmd_receive_pack() down to
> receive_pack_config(), nothing obvious jumps at me.

No, I do not think it was on purpose. And it would be very hard to do
so, anyway; config callbacks are not given any information about the
source of the config variable, and cannot distinguish between repo,
global, and system-level config variables.

> Could this be caused by a chrooted environment not having
> /etc/gitconfig (now I am just speculating)?

That seems far more likely to me. Another possibility is that the file
is not readable by the user running receive-pack.

> A quick "strace -f -o /tmp/tr git push ../neigh" seems to indicate
> that at least access() is called on "/etc/gitconfig" as I expect,
> which makes me think that near the beginning of git_config_early(),
> we would read from /etc/gitconfig if the file existed (I do not
> install any distro "git", so there is no /etc/gitconfig on my box).

I just did a few quick tests both across local repos and across an ssh
session. receive.denynonfastforwards worked just fine in my
/etc/gitconfig in both cases. So the likely cause would be that git
cannot access that file for some reason (chroot or permissions).

-Peff

Re: receive.denyNonNonFastForwards not denying force update

[Sitaram Chamarty]

On Tue, Aug 21, 2012 at 6:52 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Sitaram Chamarty <sitaramc@gmail.com> writes:
>
>> On Mon, Aug 20, 2012 at 10:35 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> John Arthorne <arthorne.eclipse@gmail.com> writes:
>>>
>>>> For all the details see this bugzilla, particularly comment #59 where we
>>>> finally narrowed this down:
>>>>
>>>> https://bugs.eclipse.org/bugs/show_bug.cgi?id=343150
>>>
>>> What does "at the system level" in your "does *not* work at the
>>> system level." exactly mean?
>>
>> "git config --system receive.denynonfastforwards true" is not honored.
>>  At all.  (And I checked there was nothing overriding it).
>>
>> "--global" does work (is honored).
>>
>> Tested on 1.7.11
>
> Thanks, and interesting.

Uggh.  My fault this one.

I had a very tight umask on root, and running 'git config --system'
created an /etc/gitconfig that was not readable by a normal user.

Running strace clued me in...

John: maybe it's as simple as that in your case too.

Junio/Brandon/Jeff: sorry for the false corroboration of John's report!

Re: receive.denyNonNonFastForwards not denying force update

[Jay Soffian]

On Mon, Aug 20, 2012 at 9:53 PM, Brandon Casey <drafnel@gmail.com> wrote:
>    git config --system -e
>
> Hopefully your editor exposes the path that it is editing even if you
> don't have permission to modify it.

  GIT_EDITOR=echo git config --system -e

works for me.

j.

Re: receive.denyNonNonFastForwards not denying force update

[Junio C Hamano]

Jay Soffian <jaysoffian@gmail.com> writes:

> On Mon, Aug 20, 2012 at 9:53 PM, Brandon Casey <drafnel@gmail.com> wrote:
>>    git config --system -e
>>
>> Hopefully your editor exposes the path that it is editing even if you
>> don't have permission to modify it.
>
>   GIT_EDITOR=echo git config --system -e
>
> works for me.

Clever ;-)

Re: receive.denyNonNonFastForwards not denying force update

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Aug 20, 2012 at 06:22:26PM -0700, Junio C Hamano wrote:
>
>> Does anybody recall if this is something we did on purpose?  After
>> eyeballing the callchain starting from cmd_receive_pack() down to
>> receive_pack_config(), nothing obvious jumps at me.
>
> No, I do not think it was on purpose. And it would be very hard to do
> so, anyway; config callbacks are not given any information about the
> source of the config variable, and cannot distinguish between repo,
> global, and system-level config variables.

I was looking for setenv() to refuse system wide defaults; that
actually is fairly simple.

>> Could this be caused by a chrooted environment not having
>> /etc/gitconfig (now I am just speculating)?
>
> That seems far more likely to me. Another possibility is that the file
> is not readable by the user running receive-pack.

Good point. We explicitly use access(R_OK) and pretend as if a path
that is known to exist but not readable is missing; perhaps we may
want to diagnose this as a misconfiguration and issue a warning?

Re: receive.denyNonNonFastForwards not denying force update

[Jeff King]

On Mon, Aug 20, 2012 at 08:49:42PM -0700, Junio C Hamano wrote:

> > No, I do not think it was on purpose. And it would be very hard to do
> > so, anyway; config callbacks are not given any information about the
> > source of the config variable, and cannot distinguish between repo,
> > global, and system-level config variables.
> 
> I was looking for setenv() to refuse system wide defaults; that
> actually is fairly simple.

Ah. I was thinking we had ripped those out (since they were primarily
about the test suite, and we found other ways of working around them),
but we do indeed still have GIT_CONFIG_NOSYSTEM.  So yet another
possibility is that the OP has that environment variable set for some
odd reason.

> > That seems far more likely to me. Another possibility is that the
> > file is not readable by the user running receive-pack.
> 
> Good point. We explicitly use access(R_OK) and pretend as if a path
> that is known to exist but not readable is missing; perhaps we may
> want to diagnose this as a misconfiguration and issue a warning?

I think that makes sense. Like this patch?

-- >8 --
Subject: [PATCH] config: warn on inaccessible files

Before reading a config file, we check "!access(path, R_OK)"
to make sure that the file exists and is readable. If it's
not, then we silently ignore it.

For the case of ENOENT, this is fine, as the presence of the
file is optional. For other cases, though, it may indicate a
configuration error (e.g., not having permissions to read
the file). Let's print a warning in these cases to let the
user know.

Signed-off-by: Jeff King <peff@peff.net>
---
This catches the common code path of git itself trying to read the
config file.  The "git config foo.bar" lookup path does not warn, as it
just tries to fopen each file (and silently bails if a file cannot be
opened).  However, since before doing its actual lookup, it would run
git_config() anyway, you will already have seen the warning.

You can get multiple warnings from this, as some programs read the
config multiple times. I don't think it's really worth caring about, as
you would want to fix such a misconfiguration quickly anyway.

A bigger question is whether people are stuck living with such a
misconfiguration (e.g., inaccessible directories made by a clueless
admin), and would be annoyed at having no way to turn this feature off.

 builtin/config.c  |  4 ++--
 config.c          | 10 +++++-----
 git-compat-util.h |  3 +++
 wrapper.c         |  8 ++++++++
 4 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index 8cd08da..b0394ef 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -396,8 +396,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 			 */
 			die("$HOME not set");
 
-		if (access(user_config, R_OK) &&
-		    xdg_config && !access(xdg_config, R_OK))
+		if (access_or_warn(user_config, R_OK) &&
+		    xdg_config && !access_or_warn(xdg_config, R_OK))
 			given_config_file = xdg_config;
 		else
 			given_config_file = user_config;
diff --git a/config.c b/config.c
index 2b706ea..08e47e2 100644
--- a/config.c
+++ b/config.c
@@ -60,7 +60,7 @@ static int handle_path_include(const char *path, struct config_include_data *inc
 		path = buf.buf;
 	}
 
-	if (!access(path, R_OK)) {
+	if (!access_or_warn(path, R_OK)) {
 		if (++inc->depth > MAX_INCLUDE_DEPTH)
 			die(include_depth_advice, MAX_INCLUDE_DEPTH, path,
 			    cf && cf->name ? cf->name : "the command line");
@@ -939,23 +939,23 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)
 
 	home_config_paths(&user_config, &xdg_config, "config");
 
-	if (git_config_system() && !access(git_etc_gitconfig(), R_OK)) {
+	if (git_config_system() && !access_or_warn(git_etc_gitconfig(), R_OK)) {
 		ret += git_config_from_file(fn, git_etc_gitconfig(),
 					    data);
 		found += 1;
 	}
 
-	if (xdg_config && !access(xdg_config, R_OK)) {
+	if (xdg_config && !access_or_warn(xdg_config, R_OK)) {
 		ret += git_config_from_file(fn, xdg_config, data);
 		found += 1;
 	}
 
-	if (user_config && !access(user_config, R_OK)) {
+	if (user_config && !access_or_warn(user_config, R_OK)) {
 		ret += git_config_from_file(fn, user_config, data);
 		found += 1;
 	}
 
-	if (repo_config && !access(repo_config, R_OK)) {
+	if (repo_config && !access_or_warn(repo_config, R_OK)) {
 		ret += git_config_from_file(fn, repo_config, data);
 		found += 1;
 	}
diff --git a/git-compat-util.h b/git-compat-util.h
index 35b095e..5a520e2 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -604,6 +604,9 @@ int rmdir_or_warn(const char *path);
  */
 int remove_or_warn(unsigned int mode, const char *path);
 
+/* Call access(2), but warn for any error besides ENOENT. */
+int access_or_warn(const char *path, int mode);
+
 /* Get the passwd entry for the UID of the current process. */
 struct passwd *xgetpwuid_self(void);
 
diff --git a/wrapper.c b/wrapper.c
index b5e33e4..b40c7e7 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -403,6 +403,14 @@ int remove_or_warn(unsigned int mode, const char *file)
 	return S_ISGITLINK(mode) ? rmdir_or_warn(file) : unlink_or_warn(file);
 }
 
+int access_or_warn(const char *path, int mode)
+{
+	int ret = access(path, mode);
+	if (ret && errno != ENOENT)
+		warning(_("unable to access '%s': %s"), path, strerror(errno));
+	return ret;
+}
+
 struct passwd *xgetpwuid_self(void)
 {
 	struct passwd *pw;
-- 
1.7.12.4.g4e9f38f

Re: receive.denyNonNonFastForwards not denying force update

[Jeff King]

On Tue, Aug 21, 2012 at 02:10:59AM -0400, Jeff King wrote:

> I think that makes sense. Like this patch?
> 
> -- >8 --
> Subject: [PATCH] config: warn on inaccessible files
> 
> Before reading a config file, we check "!access(path, R_OK)"
> to make sure that the file exists and is readable. If it's
> not, then we silently ignore it.
> 
> For the case of ENOENT, this is fine, as the presence of the
> file is optional. For other cases, though, it may indicate a
> configuration error (e.g., not having permissions to read
> the file). Let's print a warning in these cases to let the
> user know.

And this might be a good follow-on:

-- >8 --
Subject: [PATCH] gitignore: report access errors of exclude files

When we try to access gitignore files, we check for their
existence with a call to "access". We silently ignore
missing files. However, if a file is not readable, this may
be a configuration error; let's warn the user.

For $GIT_DIR/info/excludes or core.excludesfile, we can just
use access_or_warn. However, for per-directory files we
actually try to open them, so we must add a custom warning.

Signed-off-by: Jeff King <peff@peff.net>
---
 dir.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/dir.c b/dir.c
index 240bf0c..4ee16b5 100644
--- a/dir.c
+++ b/dir.c
@@ -397,6 +397,8 @@ int add_excludes_from_file_to_list(const char *fname,
 
 	fd = open(fname, O_RDONLY);
 	if (fd < 0 || fstat(fd, &st) < 0) {
+		if (errno != ENOENT)
+			warn(_("unable to access '%s': %s"), fname, strerror(errno));
 		if (0 <= fd)
 			close(fd);
 		if (!check_index ||
@@ -1311,9 +1313,9 @@ void setup_standard_excludes(struct dir_struct *dir)
 		home_config_paths(NULL, &xdg_path, "ignore");
 		excludes_file = xdg_path;
 	}
-	if (!access(path, R_OK))
+	if (!access_or_warn(path, R_OK))
 		add_excludes_from_file(dir, path);
-	if (excludes_file && !access(excludes_file, R_OK))
+	if (excludes_file && !access_or_warn(excludes_file, R_OK))
 		add_excludes_from_file(dir, excludes_file);
 }
 
-- 
1.7.12.4.g4e9f38f

Re: receive.denyNonNonFastForwards not denying force update

[Jeff King]

On Tue, Aug 21, 2012 at 02:22:19AM -0400, Jeff King wrote:

> And this might be a good follow-on:
> 
> -- >8 --
> Subject: [PATCH] gitignore: report access errors of exclude files

...and it would probably help if I gave you the version that actually
compiled.

-- >8 --
Subject: [PATCH] gitignore: report access errors of exclude files

When we try to access gitignore files, we check for their
existence with a call to "access". We silently ignore
missing files. However, if a file is not readable, this may
be a configuration error; let's warn the user.

For $GIT_DIR/info/excludes or core.excludesfile, we can just
use access_or_warn. However, for per-directory files we
actually try to open them, so we must add a custom warning.

Signed-off-by: Jeff King <peff@peff.net>
---
 dir.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/dir.c b/dir.c
index 240bf0c..ea74048 100644
--- a/dir.c
+++ b/dir.c
@@ -397,6 +397,8 @@ int add_excludes_from_file_to_list(const char *fname,
 
 	fd = open(fname, O_RDONLY);
 	if (fd < 0 || fstat(fd, &st) < 0) {
+		if (errno != ENOENT)
+			warning(_("unable to access '%s': %s"), fname, strerror(errno));
 		if (0 <= fd)
 			close(fd);
 		if (!check_index ||
@@ -1311,9 +1313,9 @@ void setup_standard_excludes(struct dir_struct *dir)
 		home_config_paths(NULL, &xdg_path, "ignore");
 		excludes_file = xdg_path;
 	}
-	if (!access(path, R_OK))
+	if (!access_or_warn(path, R_OK))
 		add_excludes_from_file(dir, path);
-	if (excludes_file && !access(excludes_file, R_OK))
+	if (excludes_file && !access_or_warn(excludes_file, R_OK))
 		add_excludes_from_file(dir, excludes_file);
 }
 
-- 
1.7.12.4.g4e9f38f

Re: receive.denyNonNonFastForwards not denying force update

[Jeff King]

On Tue, Aug 21, 2012 at 02:22:19AM -0400, Jeff King wrote:

> And this might be a good follow-on:
> 
> -- >8 --
> Subject: [PATCH] gitignore: report access errors of exclude files

And if we are going to do that, then we almost certainly want to do
this.

-- >8 --
Subject: [PATCH] attr: warn on inaccessible attribute files

Just like config and gitignore files, we silently ignore
missing or inaccessible attribute files. An existent but
inaccessible file is probably a configuration error, so
let's warn the user.

Signed-off-by: Jeff King <peff@peff.net>
---
 attr.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/attr.c b/attr.c
index b52efb5..cab01b8 100644
--- a/attr.c
+++ b/attr.c
@@ -352,8 +352,11 @@ static struct attr_stack *read_attr_from_file(const char *path, int macro_ok)
 	char buf[2048];
 	int lineno = 0;
 
-	if (!fp)
+	if (!fp) {
+		if (errno != ENOENT)
+			warning(_("unable to access '%s': %s"), path, strerror(errno));
 		return NULL;
+	}
 	res = xcalloc(1, sizeof(*res));
 	while (fgets(buf, sizeof(buf), fp))
 		handle_attr_line(res, buf, path, ++lineno, macro_ok);
-- 
1.7.12.4.g4e9f38f

Re: receive.denyNonNonFastForwards not denying force update

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> You can get multiple warnings from this, as some programs read the
> config multiple times. I don't think it's really worth caring about, as
> you would want to fix such a misconfiguration quickly anyway.

I agree that we wouldn't care too much about the multiple warnings,
and even if we did, it would be easy to correct. Instead of having a
call to warning(_("unable to access..."), path, strerror(errno))
directly in acceess_or_warn(), make that a helper function that is
called from there and other places you warn in your other patches,
and maintain a small table of already-warned-for paths in the helper,
and we are done.

> A bigger question is whether people are stuck living with such a
> misconfiguration (e.g., inaccessible directories made by a clueless
> admin), and would be annoyed at having no way to turn this feature off.

Yes, /etc/gitconfig would certainly have that issue; exclude and
attr you deal with your other patches are safe, though.

Modulo the above "you might want to turn the call to warn() to
another helper that can be used from elsewhere", this patch looks
perfect to me.

Thanks.

Re: receive.denyNonNonFastForwards not denying force update

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Aug 21, 2012 at 02:10:59AM -0400, Jeff King wrote:
>
>> I think that makes sense. Like this patch?
>> 
>> -- >8 --
>> Subject: [PATCH] config: warn on inaccessible files
>> 
>> Before reading a config file, we check "!access(path, R_OK)"
>> to make sure that the file exists and is readable. If it's
>> not, then we silently ignore it.
>> 
>> For the case of ENOENT, this is fine, as the presence of the
>> file is optional. For other cases, though, it may indicate a
>> configuration error (e.g., not having permissions to read
>> the file). Let's print a warning in these cases to let the
>> user know.
>
> And this might be a good follow-on:
>
> -- >8 --
> Subject: [PATCH] gitignore: report access errors of exclude files
>
> When we try to access gitignore files, we check for their
> existence with a call to "access". We silently ignore
> missing files. However, if a file is not readable, this may
> be a configuration error; let's warn the user.
>
> For $GIT_DIR/info/excludes or core.excludesfile, we can just
> use access_or_warn. However, for per-directory files we
> actually try to open them, so we must add a custom warning.

There are a couple of users of add_excludes_from_file() that is
outside the per-directory walking in ls-files and unpack-trees; I
think both are OK with this change, but the one in ls-files may want
to issue a warning or even an error upon ENOENT.

Not a regression with this patch; just something we may want to do
while we are in the vicinity.

> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  dir.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/dir.c b/dir.c
> index 240bf0c..4ee16b5 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -397,6 +397,8 @@ int add_excludes_from_file_to_list(const char *fname,
>  
>  	fd = open(fname, O_RDONLY);
>  	if (fd < 0 || fstat(fd, &st) < 0) {
> +		if (errno != ENOENT)
> +			warn(_("unable to access '%s': %s"), fname, strerror(errno));
>  		if (0 <= fd)
>  			close(fd);
>  		if (!check_index ||
> @@ -1311,9 +1313,9 @@ void setup_standard_excludes(struct dir_struct *dir)
>  		home_config_paths(NULL, &xdg_path, "ignore");
>  		excludes_file = xdg_path;
>  	}
> -	if (!access(path, R_OK))
> +	if (!access_or_warn(path, R_OK))
>  		add_excludes_from_file(dir, path);
> -	if (excludes_file && !access(excludes_file, R_OK))
> +	if (excludes_file && !access_or_warn(excludes_file, R_OK))
>  		add_excludes_from_file(dir, excludes_file);
>  }

Re: receive.denyNonNonFastForwards not denying force update

[Jeff King]

On Tue, Aug 21, 2012 at 09:50:27AM -0700, Junio C Hamano wrote:

> > Subject: [PATCH] gitignore: report access errors of exclude files
> >
> > When we try to access gitignore files, we check for their
> > existence with a call to "access". We silently ignore
> > missing files. However, if a file is not readable, this may
> > be a configuration error; let's warn the user.
> >
> > For $GIT_DIR/info/excludes or core.excludesfile, we can just
> > use access_or_warn. However, for per-directory files we
> > actually try to open them, so we must add a custom warning.
> 
> There are a couple of users of add_excludes_from_file() that is
> outside the per-directory walking in ls-files and unpack-trees; I
> think both are OK with this change, but the one in ls-files may want
> to issue a warning or even an error upon ENOENT.
> 
> Not a regression with this patch; just something we may want to do
> while we are in the vicinity.

The two I see are:

  1. unpack-trees:verify_absent

     This looks like it is reading info/sparse-checkout. But I think it
     is OK for that file to be missing, no?

  2. ls-files:option_parse_exclude_from

      This handles --exclude-from. I would expect most callers to be
      converted to --exclude-standard these days, but originally callers
      did something like:

        git ls-files \
          --exclude-from=$GIT_DIR/info/exclude \
          --exclude-per-directory=.gitignore \
          ...

       While it would be friendlier to a user calling ls-files to warn
       about a missing entry in the first case (since they explicitly
       typed it, they presumably expect it to work). But for a script
       calling the ls-files plumbing, that --exclude-from has always
       meant "if it's there, use it, but otherwise, don't worry".

       Probably no such callers exist anymore, but complaining would be
       a regression for them.

-Peff

Re: receive.denyNonNonFastForwards not denying force update

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Modulo the above "you might want to turn the call to warn() to
> another helper that can be used from elsewhere", this patch looks
> perfect to me.

And that "modulo" is fairly simple if we wanted to go that route.

 attr.c            | 2 +-
 dir.c             | 2 +-
 git-compat-util.h | 3 +++
 wrapper.c         | 7 ++++++-
 4 files changed, 11 insertions(+), 3 deletions(-)

diff --git c/attr.c w/attr.c
index cab01b8..f12c83f 100644
--- c/attr.c
+++ w/attr.c
@@ -354,7 +354,7 @@ static struct attr_stack *read_attr_from_file(const char *path, int macro_ok)
 
 	if (!fp) {
 		if (errno != ENOENT)
-			warning(_("unable to access '%s': %s"), path, strerror(errno));
+			warn_on_inaccessible(path);
 		return NULL;
 	}
 	res = xcalloc(1, sizeof(*res));
diff --git c/dir.c w/dir.c
index ea74048..4868339 100644
--- c/dir.c
+++ w/dir.c
@@ -398,7 +398,7 @@ int add_excludes_from_file_to_list(const char *fname,
 	fd = open(fname, O_RDONLY);
 	if (fd < 0 || fstat(fd, &st) < 0) {
 		if (errno != ENOENT)
-			warning(_("unable to access '%s': %s"), fname, strerror(errno));
+			warn_on_inaccessible(fname);
 		if (0 <= fd)
 			close(fd);
 		if (!check_index ||
diff --git c/git-compat-util.h w/git-compat-util.h
index 5a520e2..000042d 100644
--- c/git-compat-util.h
+++ w/git-compat-util.h
@@ -607,6 +607,9 @@ int remove_or_warn(unsigned int mode, const char *path);
 /* Call access(2), but warn for any error besides ENOENT. */
 int access_or_warn(const char *path, int mode);
 
+/* Warn on an inaccessible file that ought to be accessible */
+void warn_on_inaccessible(const char *path);
+
 /* Get the passwd entry for the UID of the current process. */
 struct passwd *xgetpwuid_self(void);
 
diff --git c/wrapper.c w/wrapper.c
index b40c7e7..68739aa 100644
--- c/wrapper.c
+++ w/wrapper.c
@@ -403,11 +403,16 @@ int remove_or_warn(unsigned int mode, const char *file)
 	return S_ISGITLINK(mode) ? rmdir_or_warn(file) : unlink_or_warn(file);
 }
 
+void warn_on_inaccessible(const char *path)
+{
+	warning(_("unable to access '%s': %s"), path, strerror(errno));
+}
+
 int access_or_warn(const char *path, int mode)
 {
 	int ret = access(path, mode);
 	if (ret && errno != ENOENT)
-		warning(_("unable to access '%s': %s"), path, strerror(errno));
+		warn_on_inaccessible(path);
 	return ret;
 }
 

Re: receive.denyNonNonFastForwards not denying force update

[Jeff King]

On Tue, Aug 21, 2012 at 02:52:02PM -0700, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Modulo the above "you might want to turn the call to warn() to
> > another helper that can be used from elsewhere", this patch looks
> > perfect to me.
> 
> And that "modulo" is fairly simple if we wanted to go that route.
> 
>  attr.c            | 2 +-
>  dir.c             | 2 +-
>  git-compat-util.h | 3 +++
>  wrapper.c         | 7 ++++++-
>  4 files changed, 11 insertions(+), 3 deletions(-)

Yeah, that looks fine to me if you want to squash it in.

-Peff

Re: receive.denyNonNonFastForwards not denying force update

[John Arthorne]

Just to close the loop on this thread, it did turn out to be a
permission problem in our case. It was difficult to track down because
it was only a problem on one server in the cluster. Each server had a
system git config file at /usr/local/etc/gitconfig. This was a symlink
pointing to a single common config file at /etc/gitconfig. This real
file had correct content and permissions, and all the machines where
eclipse.org allows shell access had correct symlinks. So any tests on
the command line always showed that the system config looked fine.
However on git.eclipse.org, which is the machine with the central
repositories we are pushing to, the symlink was missing o+rx. For
security reasons this machine doesn't allow shell access, but our
pushes to this machine were failing to honour the system
configuration. I gather the patch prepared earlier in this thread will
cause an error to be reported when the system config could not be
read, which sounds like a good fix to help others track down problems
like this.

John Arthorne


On Fri, Aug 17, 2012 at 12:26 PM, John Arthorne
<arthorne.eclipse@gmail.com> wrote:
> At eclipse.org we wanted all git repositories to disallow non-fastforward
> commits by default. So, we set receive.denyNonFastForwards=true as a system
> configuration setting. However, this does not prevent a non-fastforward
> force push. If we set the same configuration setting in the local repository
> configuration then it does prevent non-fastforward pushes.
>
> For all the details see this bugzilla, particularly comment #59 where we
> finally narrowed this down:
>
> https://bugs.eclipse.org/bugs/show_bug.cgi?id=343150
>
> This is on git version 1.7.4.1.
>
> The Git book recommends setting this property at the system level:
>
> http://git-scm.com/book/ch7-1.html (near the bottom)
>
> Can someone confirm if this is intended behaviour or not.
>
> Thanks,
> John Arthorne