Re: [PATCH] git-sh-setup: Restore sourcability from outside scripts

[Junio C Hamano <gitster@pobox.com>]

Anders Kaseorg <andersk@mit.edu> writes:

> v2.10.0-rc0~45^2~2 “i18n: git-sh-setup.sh: mark strings for
> translation” broke outside scripts such as guilt that source
> git-sh-setup as described in the documentation:
>
> $ . "$(git --exec-path)/git-sh-setup"
> sh: 6: .: git-sh-i18n: not found
>
> This also affects contrib/convert-grafts-to-replace-refs.sh and
> contrib/rerere-train.sh in tree.  Fix this by using git --exec-path to
> find git-sh-i18n.
>
> While we’re here, move the sourcing of git-sh-i18n below the shell
> portability fixes.
>
> Signed-off-by: Anders Kaseorg <andersk@mit.edu>
> ---

Looks good.

Our in-tree scripts rely on the fact that $PATH is adjusted to have
$GIT_EXEC_PATH early (either by getting invoked indirectly by "git"
potty, or the requirement to do so for people and scripts that still
run our in-tree scripts with dashed e.g. "git-rebase" form) by the
time they run.  But when sh-setup dot-sources git-sh-i18n for its
own use, it should be explicit to name which one of the many copies
that may appear in directories on user's $PATH (one among which is
the one in $GIT_EXEC_PATH) it wants to use.  And this patch does the
right thing by not relying on the $PATH, but instead naming the
exact path using $(git --exec-path)/ prefix, to the included file.

In other words, I think this patch is a pure bugfix, even if there
is no third-party script that includes it.  We may want to have the
above as the rationale to apply this patch in the proposed log
message, though.

> Is this a supported use of git-sh-setup?  Although the documentation is
> clear that the end user should not invoke it directly, it seems to imply
> that scripts may do this, and in practice it has worked until v2.10.0.

It is correct for the documentation to say that this is not a
"command" end users would want to run; they cannot invoke it as a
standalone command as it is written as a dot-sourced shell library.

Even though it is intended solely for internal use, so far we have
not removed things from there, which would have signalled people
that third-party scripts can also dot-source it.  We may want to
reserve the right to break them in the future, but because this is a
pure bugfix, "can third-party rely on the interface not changing?"
is not a question we need to answer in this thread---there is no
reason to leave this broken.

Thanks.

>  git-sh-setup.sh | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/git-sh-setup.sh b/git-sh-setup.sh
> index a8a4576..240c7eb 100644
> --- a/git-sh-setup.sh
> +++ b/git-sh-setup.sh
> @@ -2,9 +2,6 @@
>  # to set up some variables pointing at the normal git directories and
>  # a few helper shell functions.
>  
> -# Source git-sh-i18n for gettext support.
> -. git-sh-i18n
> -
>  # Having this variable in your environment would break scripts because
>  # you would cause "cd" to be taken to unexpected places.  If you
>  # like CDPATH, define it for your interactive shell sessions without
> @@ -46,6 +43,9 @@ git_broken_path_fix () {
>  
>  # @@BROKEN_PATH_FIX@@
>  
> +# Source git-sh-i18n for gettext support.
> +. "$(git --exec-path)/git-sh-i18n"
> +
>  die () {
>  	die_with_status 1 "$@"
>  }