From: Junio C Hamano <gitster@pobox.com>
To: Sebastian Kisela <skisela@redhat.com>
Cc: git@vger.kernel.org, Todd Zullinger <tmz@pobox.com>
Subject: Re: [PATCH 1/2] git-instaweb: support Fedora/Red Hat apache module path
Date: Tue, 07 Aug 2018 14:25:57 -0700 [thread overview]
Message-ID: <xmqqk1p1kgwq.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <20180807072548.12764-1-skisela@redhat.com> (Sebastian Kisela's message of "Tue, 7 Aug 2018 09:25:47 +0200")
Sebastian Kisela <skisela@redhat.com> writes:
> On Fedora-derived systems, the apache httpd package installs modules
> under /usr/lib{,64}/httpd/modules, depending on whether the system is
> 32- or 64-bit. A symlink from /etc/httpd/modules is created which
> points to the proper module path. Use it to support apache on Fedora,
> CentOS, and Red Hat systems.
>
> Written with assistance of Todd Zullinger <tmz@pobox.com>
>
> Signed-off-by: Sebastian Kisela <skisela@redhat.com>
> ---
> git-instaweb.sh | 2 ++
> 1 file changed, 2 insertions(+)
Thanks for a patch, and welcome to the Git development community.
> diff --git a/git-instaweb.sh b/git-instaweb.sh
> index 47e38f34c..e42e58528 100755
> --- a/git-instaweb.sh
> +++ b/git-instaweb.sh
> @@ -332,6 +332,8 @@ apache2_conf () {
> module_path="/usr/lib/httpd/modules"
> test -d "/usr/lib/apache2/modules" &&
> module_path="/usr/lib/apache2/modules"
> + test -d "/etc/httpd/modules" &&
> + module_path="/etc/httpd/modules"
The original already has the same issue with two entries but this
patch makes it even worse by adding yet another one. The longer the
code follows a bad pattern, the more it encourages future developers
to follow the same bad pattern, and usually the best time to do a
clean up like the following
if test -z "$module_path"
then
for candidate in \
/etc/httpd \
/usr/lib/apache2 \
/usr/lib/httpd \
do
if test -d "$candidate/modules"
then
module_path="$candidate/modules"
break
fi
done
fi
is when you go from 2 to 3. Two points to note are:
- It would be easier to add the fourth one this way
- The explicit "break" makes it clear that the paths are listed in
decreasing order of precedence (i.e. /etc/httpd if exists makes
/usr/lib/httpd ignored even if the latter exists); the original
"test -d X && M=X ; test -d Y && M=Y" gives higher precedence to
the later items but readers need to wonder if it is intended or
the code is simply being sloppy.
Hope this helps.
next prev parent reply other threads:[~2018-08-07 21:26 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-08-07 7:25 [PATCH 1/2] git-instaweb: support Fedora/Red Hat apache module path Sebastian Kisela
2018-08-07 7:25 ` [PATCH 2/2] git-instaweb: fix apache2 config with apache >= 2.4 Sebastian Kisela
2018-08-07 21:25 ` Junio C Hamano [this message]
2018-08-07 22:11 ` [PATCH 1/2] git-instaweb: support Fedora/Red Hat apache module path Junio C Hamano
2018-08-08 8:49 ` Sebastian Kisela
2018-08-08 8:49 ` Sebastian Kisela
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqk1p1kgwq.fsf@gitster-ct.c.googlers.com \
--to=gitster@pobox.com \
--cc=git@vger.kernel.org \
--cc=skisela@redhat.com \
--cc=tmz@pobox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).