{"thread":{"id":"44401","subject":"[PATCH] git-sh-setup: Restore sourcability from outside scripts","startedAt":"2016-10-30T02:10:15Z","lastAt":"2016-10-31T00:30:53Z","messageCount":9,"participants":["Anders Kaseorg","Ævar Arnfjörð Bjarmason","Philip Oakley","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"305176","messageId":"alpine.DEB.2.10.1610292153300.60842@buzzword-bingo.mit.edu","threadId":"44401","inReplyTo":null,"subject":"[PATCH] git-sh-setup: Restore sourcability from outside scripts","fromName":"Anders Kaseorg","fromEmail":"andersk@mit.edu","sentAt":"2016-10-30T02:10:02Z","receivedAt":"2016-10-30T02:10:15Z","isPatch":true,"sender":{"key":"andersk@mit.edu","avatar":"https://avatars.githubusercontent.com/u/26471?v=4"},"body":"v2.10.0-rc0~45^2~2 “i18n: git-sh-setup.sh: mark strings for\ntranslation” broke outside scripts such as guilt that source\ngit-sh-setup as described in the documentation:\n\n$ . \"$(git --exec-path)/git-sh-setup\"\nsh: 6: .: git-sh-i18n: not found\n\nThis also affects contrib/convert-grafts-to-replace-refs.sh and\ncontrib/rerere-train.sh in tree.  Fix this by using git --exec-path to\nfind git-sh-i18n.\n\nWhile we’re here, move the sourcing of git-sh-i18n below the shell\nportability fixes.\n\nSigned-off-by: Anders Kaseorg <andersk@mit.edu>\n---\n\nIs this a supported use of git-sh-setup?  Although the documentation is\nclear that the end user should not invoke it directly, it seems to imply\nthat scripts may do this, and in practice it has worked until v2.10.0.\n\n git-sh-setup.sh | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/git-sh-setup.sh b/git-sh-setup.sh\nindex a8a4576..240c7eb 100644\n--- a/git-sh-setup.sh\n+++ b/git-sh-setup.sh\n@@ -2,9 +2,6 @@\n # to set up some variables pointing at the normal git directories and\n # a few helper shell functions.\n \n-# Source git-sh-i18n for gettext support.\n-. git-sh-i18n\n-\n # Having this variable in your environment would break scripts because\n # you would cause \"cd\" to be taken to unexpected places.  If you\n # like CDPATH, define it for your interactive shell sessions without\n@@ -46,6 +43,9 @@ git_broken_path_fix () {\n \n # @@BROKEN_PATH_FIX@@\n \n+# Source git-sh-i18n for gettext support.\n+. \"$(git --exec-path)/git-sh-i18n\"\n+\n die () {\n \tdie_with_status 1 \"$@\"\n }\n-- \n2.10.1\n\n"},{"id":"305185","messageId":"CACBZZX4SnJj6ZYK-Ha3EtiWUf_n=+LZ=UeS=7vxgsj8s=bi3Sg@mail.gmail.com","threadId":"44401","inReplyTo":"alpine.DEB.2.10.1610292153300.60842@buzzword-bingo.mit.edu","subject":"Re: [PATCH] git-sh-setup: Restore sourcability from outside scripts","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2016-10-30T17:55:57Z","receivedAt":"2016-10-30T17:56:26Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Sun, Oct 30, 2016 at 3:10 AM, Anders Kaseorg <andersk@mit.edu> wrote:\n> v2.10.0-rc0~45^2~2 “i18n: git-sh-setup.sh: mark strings for\n> translation” broke outside scripts such as guilt that source\n> git-sh-setup as described in the documentation:\n>\n> $ . \"$(git --exec-path)/git-sh-setup\"\n> sh: 6: .: git-sh-i18n: not found\n\nThis seems like a reasonable fix for this issue. However as far as I\ncan tell git-sh-setup was never meant to be used by outside scripts\nthat didn't ship as part of git itself.\n\nIf that's the case any change in the API which AFAICT is now\nconsidered internal might break them, so should some part of that be\nmade public & documented as such?\n"},{"id":"305187","messageId":"alpine.DEB.2.10.1610301503280.60842@buzzword-bingo.mit.edu","threadId":"44401","inReplyTo":"CACBZZX4SnJj6ZYK-Ha3EtiWUf_n=+LZ=UeS=7vxgsj8s=bi3Sg@mail.gmail.com","subject":"Re: [PATCH] git-sh-setup: Restore sourcability from outside scripts","fromName":"Anders Kaseorg","fromEmail":"andersk@mit.edu","sentAt":"2016-10-30T19:21:21Z","receivedAt":"2016-10-30T19:21:35Z","isPatch":true,"sender":{"key":"andersk@mit.edu","avatar":"https://avatars.githubusercontent.com/u/26471?v=4"},"body":"On Sun, 30 Oct 2016, Ævar Arnfjörð Bjarmason wrote:\n> This seems like a reasonable fix for this issue. However as far as I\n> can tell git-sh-setup was never meant to be used by outside scripts\n> that didn't ship as part of git itself.\n> \n> If that's the case any change in the API which AFAICT is now\n> considered internal might break them, so should some part of that be\n> made public & documented as such?\n\nIt is documented (Documentation/git-sh-setup.txt), and this is not the \ninternal Documentation/technical section of the documentation, so my \ndefault assumption would be that everything shown there is intended as \npublic.  I only bring this up as a question because it was apparently \nallowed to break.  If I’m wrong and it isn’t public, other patches are \nneeded (to the documentation and to its users in contrib).\n\nAnders\n"},{"id":"305188","messageId":"223121D101D844DEBF086AC40A5AF4CB@PhilipOakley","threadId":"44401","inReplyTo":"alpine.DEB.2.10.1610301503280.60842@buzzword-bingo.mit.edu","subject":"Re: [PATCH] git-sh-setup: Restore sourcability from outside scripts","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":null,"receivedAt":"2016-10-30T20:09:43Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Anders Kaseorg\" <andersk@mit.edu>\n> On Sun, 30 Oct 2016, Ævar Arnfjörð Bjarmason wrote:\n>> This seems like a reasonable fix for this issue. However as far as I\n>> can tell git-sh-setup was never meant to be used by outside scripts\n>> that didn't ship as part of git itself.\n>>\n>> If that's the case any change in the API which AFAICT is now\n>> considered internal might break them, so should some part of that be\n>> made public & documented as such?\n>\n> It is documented (Documentation/git-sh-setup.txt), and this is not the\n> internal Documentation/technical section of the documentation, so my\n> default assumption would be that everything shown there is intended as\n> public.  I only bring this up as a question because it was apparently\n> allowed to break.  If I’m wrong and it isn’t public, other patches are\n> needed (to the documentation and to its users in contrib).\n>\nBut the Documenation does say ::\n\n- This is not a command the end user would want to run. Ever.\n\n- This documentation is meant for people who are studying the Porcelain-ish \nscripts and/or are writing new ones.\n--\n\nSo there is a cautionary word or two there...\n\nThe question would then become: what (if anything) was missing in the \ndocumentation?...\nmaybe the inclusion of Ævar's \"[Not] to be used by outside scripts that \ndidn't ship as part of git itself.\"?\nOr a comment that it may change in newer versions.\nThough the code fix may still be reasonable..\n\n\nPhilip \n\n"},{"id":"305189","messageId":"20161030211227.4gqovv7mt7mtnpy7@sigill.intra.peff.net","threadId":"44401","inReplyTo":"223121D101D844DEBF086AC40A5AF4CB@PhilipOakley","subject":"Re: [PATCH] git-sh-setup: Restore sourcability from outside scripts","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-30T21:12:27Z","receivedAt":"2016-10-30T21:12:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 30, 2016 at 08:09:21PM -0000, Philip Oakley wrote:\n\n> > It is documented (Documentation/git-sh-setup.txt), and this is not the\n> > internal Documentation/technical section of the documentation, so my\n> > default assumption would be that everything shown there is intended as\n> > public.  I only bring this up as a question because it was apparently\n> > allowed to break.  If I’m wrong and it isn’t public, other patches are\n> > needed (to the documentation and to its users in contrib).\n> > \n> But the Documenation does say ::\n> \n> - This is not a command the end user would want to run. Ever.\n> \n> - This documentation is meant for people who are studying the Porcelain-ish\n> scripts and/or are writing new ones.\n> --\n\nHistorically speaking, porcelain-ish scripts were carried both in and\nout of git.git. These days what we consider porcelain is usually carried\nin-tree, but I don't think it's unreasonable for people building their\nown scripts to want to make use of git-sh-setup. And we've generally\ntried to retain backwards compatibility in the functions it provides,\neven to out-of-tree scripts.\n\nSo I think it is worth applying the fix at the start of this thread to\nkeep that working.\n\nAs for a documentation change for \"do not use this for out-of-tree\nscripts\", I am mildly negative, as I don't think that matches historical\npractice.\n\n-Peff\n"},{"id":"305190","messageId":"CACBZZX6ArQdG202n-SouwDhoTE1LF=69mKjWQv8HPKJ+K_0fJQ@mail.gmail.com","threadId":"44401","inReplyTo":"20161030211227.4gqovv7mt7mtnpy7@sigill.intra.peff.net","subject":"Re: [PATCH] git-sh-setup: Restore sourcability from outside scripts","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2016-10-30T22:11:10Z","receivedAt":"2016-10-30T22:11:37Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":",On Sun, Oct 30, 2016 at 10:12 PM, Jeff King <peff@peff.net> wrote:\n> On Sun, Oct 30, 2016 at 08:09:21PM -0000, Philip Oakley wrote:\n>\n>> > It is documented (Documentation/git-sh-setup.txt), and this is not the\n>> > internal Documentation/technical section of the documentation, so my\n>> > default assumption would be that everything shown there is intended as\n>> > public.  I only bring this up as a question because it was apparently\n>> > allowed to break.  If I’m wrong and it isn’t public, other patches are\n>> > needed (to the documentation and to its users in contrib).\n>> >\n>> But the Documenation does say ::\n>>\n>> - This is not a command the end user would want to run. Ever.\n>>\n>> - This documentation is meant for people who are studying the Porcelain-ish\n>> scripts and/or are writing new ones.\n>> --\n>\n> Historically speaking, porcelain-ish scripts were carried both in and\n> out of git.git. These days what we consider porcelain is usually carried\n> in-tree, but I don't think it's unreasonable for people building their\n> own scripts to want to make use of git-sh-setup. And we've generally\n> tried to retain backwards compatibility in the functions it provides,\n> even to out-of-tree scripts.\n>\n> So I think it is worth applying the fix at the start of this thread to\n> keep that working.\n>\n> As for a documentation change for \"do not use this for out-of-tree\n> scripts\", I am mildly negative, as I don't think that matches historical\n> practice.\n\nI don't see why we shouldn't have some stable shellscript function API\nif that's needed either.\n\nI just wanted to point out that currently git-sh-setup isn't\ndocumented as such. So at least a follow-up patch to the documentation\nseems in order.\n\nThis did break in v2.10.0, and it's taken a couple of months to notice\nthis, so clearly it's not very widely used, which says something about\nthe cost-benefit of maintaining this for external users.\n\nIt's probably worthwhile to split off git-sh-setup into git-sh-setup &\ngit-sh-setup-internal along with a documentation fix. A lot of what\nit's doing (e.g. git_broken_path_fix(), and adding a die() function)\nis probably only needed internally by git itself. The\ngit-sh-setup-internal should be the thing sourcing \"git-sh-i18n\", I\ndon't see how anyone out-of-tree could make use of that. Surely nobody\nneeds to re-emit the exact message we shipped with our *.po files.\n"},{"id":"305191","messageId":"xmqqlgx5v5iq.fsf@gitster.mtv.corp.google.com","threadId":"44401","inReplyTo":"alpine.DEB.2.10.1610292153300.60842@buzzword-bingo.mit.edu","subject":"Re: [PATCH] git-sh-setup: Restore sourcability from outside scripts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-30T22:25:17Z","receivedAt":"2016-10-30T22:25:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Anders Kaseorg <andersk@mit.edu> writes:\n\n> v2.10.0-rc0~45^2~2 “i18n: git-sh-setup.sh: mark strings for\n> translation” broke outside scripts such as guilt that source\n> git-sh-setup as described in the documentation:\n>\n> $ . \"$(git --exec-path)/git-sh-setup\"\n> sh: 6: .: git-sh-i18n: not found\n>\n> This also affects contrib/convert-grafts-to-replace-refs.sh and\n> contrib/rerere-train.sh in tree.  Fix this by using git --exec-path to\n> find git-sh-i18n.\n>\n> While we’re here, move the sourcing of git-sh-i18n below the shell\n> portability fixes.\n>\n> Signed-off-by: Anders Kaseorg <andersk@mit.edu>\n> ---\n\nLooks good.\n\nOur in-tree scripts rely on the fact that $PATH is adjusted to have\n$GIT_EXEC_PATH early (either by getting invoked indirectly by \"git\"\npotty, or the requirement to do so for people and scripts that still\nrun our in-tree scripts with dashed e.g. \"git-rebase\" form) by the\ntime they run.  But when sh-setup dot-sources git-sh-i18n for its\nown use, it should be explicit to name which one of the many copies\nthat may appear in directories on user's $PATH (one among which is\nthe one in $GIT_EXEC_PATH) it wants to use.  And this patch does the\nright thing by not relying on the $PATH, but instead naming the\nexact path using $(git --exec-path)/ prefix, to the included file.\n\nIn other words, I think this patch is a pure bugfix, even if there\nis no third-party script that includes it.  We may want to have the\nabove as the rationale to apply this patch in the proposed log\nmessage, though.\n\n> Is this a supported use of git-sh-setup?  Although the documentation is\n> clear that the end user should not invoke it directly, it seems to imply\n> that scripts may do this, and in practice it has worked until v2.10.0.\n\nIt is correct for the documentation to say that this is not a\n\"command\" end users would want to run; they cannot invoke it as a\nstandalone command as it is written as a dot-sourced shell library.\n\nEven though it is intended solely for internal use, so far we have\nnot removed things from there, which would have signalled people\nthat third-party scripts can also dot-source it.  We may want to\nreserve the right to break them in the future, but because this is a\npure bugfix, \"can third-party rely on the interface not changing?\"\nis not a question we need to answer in this thread---there is no\nreason to leave this broken.\n\nThanks.\n\n>  git-sh-setup.sh | 6 +++---\n>  1 file changed, 3 insertions(+), 3 deletions(-)\n>\n> diff --git a/git-sh-setup.sh b/git-sh-setup.sh\n> index a8a4576..240c7eb 100644\n> --- a/git-sh-setup.sh\n> +++ b/git-sh-setup.sh\n> @@ -2,9 +2,6 @@\n>  # to set up some variables pointing at the normal git directories and\n>  # a few helper shell functions.\n>  \n> -# Source git-sh-i18n for gettext support.\n> -. git-sh-i18n\n> -\n>  # Having this variable in your environment would break scripts because\n>  # you would cause \"cd\" to be taken to unexpected places.  If you\n>  # like CDPATH, define it for your interactive shell sessions without\n> @@ -46,6 +43,9 @@ git_broken_path_fix () {\n>  \n>  # @@BROKEN_PATH_FIX@@\n>  \n> +# Source git-sh-i18n for gettext support.\n> +. \"$(git --exec-path)/git-sh-i18n\"\n> +\n>  die () {\n>  \tdie_with_status 1 \"$@\"\n>  }\n\n"},{"id":"305192","messageId":"xmqqh97tv470.fsf@gitster.mtv.corp.google.com","threadId":"44401","inReplyTo":"CACBZZX6ArQdG202n-SouwDhoTE1LF=69mKjWQv8HPKJ+K_0fJQ@mail.gmail.com","subject":"Re: [PATCH] git-sh-setup: Restore sourcability from outside scripts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-30T22:53:55Z","receivedAt":"2016-10-30T22:54:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n(commenting out of order)\n\n> It's probably worthwhile to split off git-sh-setup into git-sh-setup &\n> git-sh-setup-internal along with a documentation fix. A lot of what\n> it's doing (e.g. git_broken_path_fix(), and adding a die() function)\n> is probably only needed internally by git itself. The\n> git-sh-setup-internal should be the thing sourcing \"git-sh-i18n\", I\n> don't see how anyone out-of-tree could make use of that. Surely nobody\n> needs to re-emit the exact message we shipped with our *.po files.\n\nMy reading of d323c6b641 (\"i18n: git-sh-setup.sh: mark strings for\ntranslation\", 2016-06-17) is abit different.  It needs to dot-source\nthe i18n stuff because the shell library functions it contains need\nthe localization support in the messages they emit.  IOW, I do not\nthink i18n belongs to -internal at all.\n\n> I don't see why we shouldn't have some stable shellscript function API\n> if that's needed either.\n>\n> I just wanted to point out that currently git-sh-setup isn't\n> documented as such. So at least a follow-up patch to the documentation\n> seems in order.\n>\n> This did break in v2.10.0, and it's taken a couple of months to notice\n> this, so clearly it's not very widely used, which says something about\n> the cost-benefit of maintaining this for external users.\n\nI am not sure if \"stable API\" in sh-setup is a good thing for the\necosystem in the longer term.\n\nAs more and more in-tree scripted Porcelain commands migrate to C,\nmany helper functions in sh-setup will lose their in-tree users.\nFor example, get_author_ident_from_commit used to have three in-tree\ncustomers (git-commit.sh, git-am.sh and git-rebase--interactive.sh),\nbut the first two is long gone and the third one may soon lose its\nneed to call it.  Once a helper function in setup-sh loses all\nin-tree users, we may no longer _break_ that helper, but that is\nsimply because we feel no need to touch it.  The in-tree Porcelain\ncommands that migrated to C however will enhance the counterpart\nthey use in C to be more featureful or fix longstanding bugs in the\nC version they use, while sh-setup version bitrot and making old\npractice obsolete for \"modern\" use of Git of the day.  \n\nKeeping such a stale version that we do not use, or even we attempt\nto update it without having a good vehicle to test the change\nourselves (because we no longer have any in-tree users) will be\ndisservice to third-party scripts---the only thing they are getting\nby using the stale one, instead of reinventing their own that they\nmay be responsible to keep up to date, is that they share the same\nstaleness as everybody else that use the sh-setup version as a\nthird-party.\n\nI am not arguing that we should remove what loses all in-tree users\nimmediately.  At least not yet.  But I wanted to point out that it\nmay not be a good use of our brain cycles to keep the API \"stable\"\nby keeping what in-tree users do not use anymore, especially if it\ndoes not help third-party users in the long run.\n\n\n"},{"id":"305194","messageId":"alpine.DEB.2.10.1610302021060.20998@buzzword-bingo.mit.edu","threadId":"44401","inReplyTo":"CACBZZX6ArQdG202n-SouwDhoTE1LF=69mKjWQv8HPKJ+K_0fJQ@mail.gmail.com","subject":"Re: [PATCH] git-sh-setup: Restore sourcability from outside scripts","fromName":"Anders Kaseorg","fromEmail":"andersk@mit.edu","sentAt":"2016-10-31T00:30:39Z","receivedAt":"2016-10-31T00:30:53Z","isPatch":true,"sender":{"key":"andersk@mit.edu","avatar":"https://avatars.githubusercontent.com/u/26471?v=4"},"body":"On Sun, 30 Oct 2016, Ævar Arnfjörð Bjarmason wrote:\n> This did break in v2.10.0, and it's taken a couple of months to notice\n> this, so clearly it's not very widely used, which says something about\n> the cost-benefit of maintaining this for external users.\n\nFor the record, in case this affects the calculation, it was noticed that \nguilt was broken a just couple of days after the first git 2.10.x upload \nto Debian, which was last weekend.\n\nhttps://bugs.debian.org/842477\nhttp://repo.or.cz/guilt.git/blob/v0.36:/guilt#l28\n\n(I have no further opinion; I trust that Junio has all the information \nneeded to decide one way or the other.)\n\nAnders\n"}]}