Re: [PATCH v2] gitweb: Fixes error handling when reading configuration
- From
Marcelo Roberto Jimenez <marcelo.jimenez@gmail.com>
- Date
- Jan 11, 2024, 02:14 UTC
- Message-ID
- <CACjc_5rgO5Zcxndawm=3eRUX1Tuqh5LT05LhK+4V45RmUWmQHw@mail.gmail.com>
- In-Reply-To
- <xmqqzfxc7mfh.fsf@gitster.g>
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@mail.gmail.com/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@pobox.com> wrote:
Show 53 quoted lines
>
> Marcelo Roberto Jimenez <marcelo.jimenez@gmail.com> 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@gmail.com>
> > ---
> > 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;
> > }