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

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 <redacted>
---

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.

 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 "$@"
 }

-- 
2.10.1

On Sun, Oct 30, 2016 at 3:10 AM, Anders Kaseorg [off-list ref] wrote:

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 seems like a reasonable fix for this issue. However as far as I
can tell git-sh-setup was never meant to be used by outside scripts
that didn't ship as part of git itself.

If that's the case any change in the API which AFAICT is now
considered internal might break them, so should some part of that be
made public & documented as such?

On Sun, 30 Oct 2016, Ævar Arnfjörð Bjarmason wrote:

This seems like a reasonable fix for this issue. However as far as I
can tell git-sh-setup was never meant to be used by outside scripts
that didn't ship as part of git itself.

If that's the case any change in the API which AFAICT is now
considered internal might break them, so should some part of that be
made public & documented as such?

It is documented (Documentation/git-sh-setup.txt), and this is not the 
internal Documentation/technical section of the documentation, so my 
default assumption would be that everything shown there is intended as 
public.  I only bring this up as a question because it was apparently 
allowed to break.  If I’m wrong and it isn’t public, other patches are 
needed (to the documentation and to its users in contrib).

Anders

From: "Anders Kaseorg" <redacted>

On Sun, 30 Oct 2016, Ævar Arnfjörð Bjarmason wrote:

quoted

This seems like a reasonable fix for this issue. However as far as I
can tell git-sh-setup was never meant to be used by outside scripts
that didn't ship as part of git itself.

If that's the case any change in the API which AFAICT is now
considered internal might break them, so should some part of that be
made public & documented as such?

It is documented (Documentation/git-sh-setup.txt), and this is not the
internal Documentation/technical section of the documentation, so my
default assumption would be that everything shown there is intended as
public.  I only bring this up as a question because it was apparently
allowed to break.  If I’m wrong and it isn’t public, other patches are
needed (to the documentation and to its users in contrib).

But the Documenation does say ::

- This is not a command the end user would want to run. Ever.

- This documentation is meant for people who are studying the Porcelain-ish 
scripts and/or are writing new ones.
--

So there is a cautionary word or two there...

The question would then become: what (if anything) was missing in the 
documentation?...
maybe the inclusion of Ævar's "[Not] to be used by outside scripts that 
didn't ship as part of git itself."?
Or a comment that it may change in newer versions.
Though the code fix may still be reasonable..


Philip 

On Sun, Oct 30, 2016 at 08:09:21PM -0000, Philip Oakley wrote:

quoted

It is documented (Documentation/git-sh-setup.txt), and this is not the
internal Documentation/technical section of the documentation, so my
default assumption would be that everything shown there is intended as
public.  I only bring this up as a question because it was apparently
allowed to break.  If I’m wrong and it isn’t public, other patches are
needed (to the documentation and to its users in contrib).

But the Documenation does say ::

- This is not a command the end user would want to run. Ever.

- This documentation is meant for people who are studying the Porcelain-ish
scripts and/or are writing new ones.
--

Historically speaking, porcelain-ish scripts were carried both in and
out of git.git. These days what we consider porcelain is usually carried
in-tree, but I don't think it's unreasonable for people building their
own scripts to want to make use of git-sh-setup. And we've generally
tried to retain backwards compatibility in the functions it provides,
even to out-of-tree scripts.

So I think it is worth applying the fix at the start of this thread to
keep that working.

As for a documentation change for "do not use this for out-of-tree
scripts", I am mildly negative, as I don't think that matches historical
practice.

-Peff

,On Sun, Oct 30, 2016 at 10:12 PM, Jeff King [off-list ref] wrote:

On Sun, Oct 30, 2016 at 08:09:21PM -0000, Philip Oakley wrote:

quoted

quoted

It is documented (Documentation/git-sh-setup.txt), and this is not the
internal Documentation/technical section of the documentation, so my
default assumption would be that everything shown there is intended as
public.  I only bring this up as a question because it was apparently
allowed to break.  If I’m wrong and it isn’t public, other patches are
needed (to the documentation and to its users in contrib).

But the Documenation does say ::

- This is not a command the end user would want to run. Ever.

- This documentation is meant for people who are studying the Porcelain-ish
scripts and/or are writing new ones.
--

Historically speaking, porcelain-ish scripts were carried both in and
out of git.git. These days what we consider porcelain is usually carried
in-tree, but I don't think it's unreasonable for people building their
own scripts to want to make use of git-sh-setup. And we've generally
tried to retain backwards compatibility in the functions it provides,
even to out-of-tree scripts.

So I think it is worth applying the fix at the start of this thread to
keep that working.

As for a documentation change for "do not use this for out-of-tree
scripts", I am mildly negative, as I don't think that matches historical
practice.

I don't see why we shouldn't have some stable shellscript function API
if that's needed either.

I just wanted to point out that currently git-sh-setup isn't
documented as such. So at least a follow-up patch to the documentation
seems in order.

This did break in v2.10.0, and it's taken a couple of months to notice
this, so clearly it's not very widely used, which says something about
the cost-benefit of maintaining this for external users.

It's probably worthwhile to split off git-sh-setup into git-sh-setup &
git-sh-setup-internal along with a documentation fix. A lot of what
it's doing (e.g. git_broken_path_fix(), and adding a die() function)
is probably only needed internally by git itself. The
git-sh-setup-internal should be the thing sourcing "git-sh-i18n", I
don't see how anyone out-of-tree could make use of that. Surely nobody
needs to re-emit the exact message we shipped with our *.po files.

On Sun, 30 Oct 2016, Ævar Arnfjörð Bjarmason wrote:

This did break in v2.10.0, and it's taken a couple of months to notice
this, so clearly it's not very widely used, which says something about
the cost-benefit of maintaining this for external users.

For the record, in case this affects the calculation, it was noticed that 
guilt was broken a just couple of days after the first git 2.10.x upload 
to Debian, which was last weekend.

https://bugs.debian.org/842477
http://repo.or.cz/guilt.git/blob/v0.36:/guilt#l28

(I have no further opinion; I trust that Junio has all the information 
needed to decide one way or the other.)

Anders