Hi Junio, Ok, I'll put on my devil's prosecutor's hat then :) I am not sure you read the first message in this thread (I know, I messed up, sorry), but there I describe the real case that happened to me, and the current behavior made it much harder to find out what was happening. That is the one: https://lore.kernel.org/git/CACjc_5pdijCZrrXQWHswsxYrGUzZ7pZq+nj3SzY1z+Xxop64Ww@xxxxxxxxxxxxxx/T/#u If the configuration file is there, but it behaves as it is not, one will quickly point its fingers to the software that reads it. In the end, if I had seen the message "Permission denied." that was stored in "$!", I would have had a better clue that the problem was with AppArmor, and not Gitweb. There were a few unanswered questions in stack overflow regarding the issue of enabling features on Gitweb when using "git instaweb" in a persistent way, some of them might have been linked to that. - gitweb refusing to blame: https://serverfault.com/questions/551762/gitweb-refusing-to-blame - How do I enable "blame" when using git instaweb?: https://stackoverflow.com/questions/66688084/how-do-i-enable-blame-when-using-git-instaweb/77793405 - My own question: Problem with `git instaweb` on OpenSUSE Tumbleweed: /etc/gitweb-common.conf is not being read: https://stackoverflow.com/questions/77789216/problem-with-git-instaweb-on-opensuse-tumbleweed-etc-gitweb-common-conf-is-n Let's see if someone comes up with a good reason to find the devil not guilty :) Regards, Marcelo. On Wed, Jan 10, 2024 at 9:17 PM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > Marcelo Roberto Jimenez <marcelo.jimenez@xxxxxxxxx> writes: > > > This patch fixes a possibility of a permission to access error go > > unnoticed. > > > > Perl uses two different variables to manage errors from a do. One > > is $@, which is set in this case when do is unable to compile the > > file. The other is $!, which is set in case do cannot read the file. > > By printing the value of $! I found out that it was set to Permission > > denied. Since the script does not currently test for $!, the error > > goes unnoticed. > > Well explained how the current code behaves and why. > > With my devil's advocate hat on, I suspect that the current > behaviour comes from the wish to see "file exists but unreadable" > and "the named file does not exist" behave the same way. If you > pass the name of a configuration file that does not exist, however, > the codeblock to die does not trigger at all. If a file does exist > but unreadable, to gitweb, it is just as good as having no file to > read configuration data from---either way, it cannot use anything > useful from the named file. So returning silently, which is the > "bug" you are fixing, does not sound too bad. > > I dunno. Let's queue and see what others would say. > > Thanks. > > > Perl do block documentation: https://perldoc.perl.org/functions/do > > > > Signed-off-by: Marcelo Roberto Jimenez <marcelo.jimenez@xxxxxxxxx> > > --- > > gitweb/gitweb.perl | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl > > index e66eb3d9ba..5d0122020f 100755 > > --- a/gitweb/gitweb.perl > > +++ b/gitweb/gitweb.perl > > @@ -728,9 +728,11 @@ sub filter_and_validate_refs { > > sub read_config_file { > > my $filename = shift; > > return unless defined $filename; > > - # die if there are errors parsing config file > > if (-e $filename) { > > do $filename; > > + # die if there is a problem accessing the file > > + die $! if $!; > > + # die if there are errors parsing config file > > die $@ if $@; > > return 1; > > }
