From: Jonathan Nieder <jrnieder@gmail.com>
To: Dridi Boukelmoune <dridi.boukelmoune@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH] git-sh: Avoid sourcing scripts with git --exec-path
Date: Thu, 28 Sep 2017 20:48:56 -0700 [thread overview]
Message-ID: <20170929034856.GB28303@aiede.mtv.corp.google.com> (raw)
In-Reply-To: <20170928223134.GA30744@varnish>
Hi,
Dridi Boukelmoune wrote:
> For end users making use of a custom exec path many commands will simply
> fail. Adding git's exec path to the PATH also allows overriding git-sh-*
> scripts, not just adding commands. One can then patch a script without
> tainting their system installation of git for example.
>
> Signed-off-by: Dridi Boukelmoune <dridi.boukelmoune@gmail.com>
This has been broken for a while, but better late than never to
address it.
[...]
> --- a/Documentation/CodingGuidelines
> +++ b/Documentation/CodingGuidelines
> @@ -151,6 +151,28 @@ For shell scripts specifically (not exhaustive):
> interface translatable. See "Marking strings for translation" in
> po/README.
>
> + - When sourcing a "git-sh-*" file using "git --exec-path" make sure
> + to only use its base name.
> +
> + (incorrect)
> + . "$(git --exec-path)/git-sh-setup"
> +
> + (correct)
> + . git-sh-setup
I wonder if we can make this so intuitive that it doesn't need
mentioning in CodingGuidelines. What if the test harness
t/test-lib.sh were to set a GIT_EXEC_PATH with multiple directories in
it? That way, authors of new commands would not have to be careful to
look out for this issue proactively because the testsuite would catch
it.
[...]
> +++ b/contrib/convert-grafts-to-replace-refs.sh
> @@ -5,7 +5,8 @@
>
> GRAFTS_FILE="${GIT_DIR:-.git}/info/grafts"
>
> -. $(git --exec-path)/git-sh-setup
> +PATH="$(git --exec-path):$PATH"
> +. git-sh-setup
I wish there were a simple way to do this that doesn't involve
polluting $PATH with the dashed commands. E.g. we can do something
like
OIFS=$IFS
IFS=:
set -f
for d in $(git --exec-path)
do
if test -e "$d/git-sh-setup"
then
. "$d/git-sh-setup"
break
fi
done
set +f
IFS=$OIFS
but that is not very simple. Something like
old_PATH=$PATH
PATH=$(git --exec-path):$PATH
. git-sh-setup
PATH=$old_PATH
looks like it could work, but it would undo the work git-sh-setup
does to set a sane $PATH on platforms like Solaris.
> --- a/contrib/rerere-train.sh
> +++ b/contrib/rerere-train.sh
> @@ -7,7 +7,8 @@ USAGE="$me rev-list-args"
>
> SUBDIRECTORY_OK=Yes
> OPTIONS_SPEC=
> -. "$(git --exec-path)/git-sh-setup"
> +PATH="$(git --exec-path):$PATH"
> +. git-sh-setup
This makes me similarly unhappy about PATH pollution, but it may be
that there's nothing to be done about that.
[...]
> --- a/contrib/subtree/git-subtree.sh
> +++ b/contrib/subtree/git-subtree.sh
> @@ -32,7 +32,6 @@ squash merge subtree changes as a single commit
> "
> eval "$(echo "$OPTS_SPEC" | git rev-parse --parseopt -- "$@" || echo exit $?)"
>
> -PATH=$PATH:$(git --exec-path)
> . git-sh-setup
This looks like a change that could be separated into a separate
patch, both because it is to contrib/subtree (which is maintained
separately) and because it is not necessary for the goal described in
this patch's commit message. I do like this change, since it improves
consistency with other commands.
> --- a/git-sh-setup.sh
> +++ b/git-sh-setup.sh
> @@ -44,7 +44,7 @@ git_broken_path_fix () {
> # @@BROKEN_PATH_FIX@@
>
> # Source git-sh-i18n for gettext support.
> -. "$(git --exec-path)/git-sh-i18n"
> +. git-sh-i18n
Do git-mergetool--lib.txt, git-parse-remote.txt, git-sh-i18n.txt,
and git-sh-setup.txt in Documentation/ need the same treatment?
Summary: I like the goal of this patch but I am nervous about the
side-effect it introduces of PATH pollution. Is there a way around
that? If there isn't, then this needs a few tweaks and then it should
be ready to go.
Thanks and hope that helps,
Jonathan
next prev parent reply other threads:[~2017-09-29 3:49 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-09-28 22:31 [PATCH] git-sh: Avoid sourcing scripts with git --exec-path Dridi Boukelmoune
2017-09-29 3:48 ` Jonathan Nieder [this message]
2017-09-29 3:58 ` Junio C Hamano
2017-09-29 4:21 ` Jonathan Nieder
2017-09-29 5:00 ` Junio C Hamano
2017-09-29 11:58 ` Dridi Boukelmoune
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=20170929034856.GB28303@aiede.mtv.corp.google.com \
--to=jrnieder@gmail.com \
--cc=dridi.boukelmoune@gmail.com \
--cc=git@vger.kernel.org \
/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.