All of lore.kernel.org
 help / color / mirror / Atom feed
From: Chris Webb <chris@arachsys.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH] Replace hard-coded path with one configurable at make time
Date: Sun, 4 Apr 2010 23:28:01 +0100	[thread overview]
Message-ID: <20100404222801.GB31315@arachsys.com> (raw)
In-Reply-To: <7vk4sm7vao.fsf@alter.siamese.dyndns.org>

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

>  * What's the point of making this configurable, other than "because we
>    can"?

I have a local patch against git to fix these paths, as I run it on slightly
unusual systems with a non-standard directory layout (no /usr, but
/local/bin in some cases) and I don't like to see incorrect paths compiled
into my binaries. It occurred to me that if I want to fix it one way
locally, others may well want to vary it too for different reasons, e.g. to
add /opt/bin or /usr/gnu to the default path.

Ultimately, I guess it feels like it should be configurable rather than
needing to be patched in the source for the same reason prefix or gitexecdir
is, but this is definitely for a minority audience!

Were it just exec_cmd.c, I would just have changed it to use _PATH_DEFPATH
from <paths.h> in preference to a make variable, as that should always give
an appropriate value for a correctly put-together system and is a sensible
place to treat as the central definition of 'default path'. However, in this
case it's needed in the shell script too and I don't think I can easily get
at _PATH_DEFPATH from there.

>  * Use of "$(x_SQ)" is about protecting whitespaces and single quotes in
>    the literal from make and shell, but does not have anything to do with
>    protecting things like $foo in that literal from the location $x is
>    eventually embedded in.  As long as paths on DEFPATH do not have double
>    quote in it (which would be a sane assumption), the patch to exec_cmd.c
>    would work fine, but I don't know if you need an extra quoting when
>    DEFPATH is used in shell scripts.  E.g. DEFPATH=$GIT_EXEC_PATH:/usr/bin
>    would have GIT_EXEC_PATH expanded in mongoose configuration file, but
>    will not be expanded in exec_cmd.c, leading to an inconsistent
>    behaviour.

Oh I see, yes; I didn't worry about quoting it correctly in the generated
shell script, assuming it would be reasonable... but if I'm assuming it's
reasonable there's no point in the _SQ to protect the shell invoking sed in
the first place.

I also notice that the makefile makes the assumption that ' might occur in
pathological paths and so needs quoting, but then uses sed 's|x|y|g' for
(say) @@PERL@@ which will break for other pathological paths containing | or
\1 and so on. Tidying that up fully might be entertaining!

Cheers,

Chris.

  reply	other threads:[~2010-04-04 22:29 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2010-04-04 20:48 [PATCH] Replace hard-coded path with one configurable at make time Chris Webb
2010-04-04 21:38 ` Junio C Hamano
2010-04-04 22:28   ` Chris Webb [this message]
2010-04-06 16:35     ` Chris Webb
2010-04-06 16:36       ` [PATCH] Replace hard-coded path with one from <paths.h> Chris Webb
2010-04-07 10:57         ` Erik Faye-Lund
2010-04-08 10:58           ` Chris Webb
2010-04-08 11:26             ` Erik Faye-Lund
2010-04-08 11:57               ` [PATCH v2] " Chris Webb
2010-04-08 12:08                 ` Erik Faye-Lund
2010-04-09  5:45                   ` Chris Webb
2010-04-13  9:06                     ` Chris Webb
2010-04-13  9:07                       ` [PATCH v3] " Chris Webb
2010-04-13 20:01                         ` Junio C Hamano
2010-04-14  7:22                           ` Chris Webb
2010-04-15 12:27                           ` Jakub Narebski
2010-04-15 19:15                             ` Junio C Hamano
2010-04-15 12:25                         ` Jakub Narebski
2010-04-15 12:40                           ` Chris Webb
2010-04-15 12:57                             ` Jakub Narebski
2010-04-15 13:01                               ` Chris Webb
2010-04-15 13:21                                 ` [PATCH v4] " Chris Webb
2010-04-09 23:54             ` [PATCH] " Tait
2010-04-06 16:57       ` [PATCH] git-instaweb: pass through invoking user's path to gitweb CGI script Chris Webb

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

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20100404222801.GB31315@arachsys.com \
    --to=chris@arachsys.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@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.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.