{"thread":{"id":"46517","subject":"[PATCH] Drop some dashes from built-in invocations in scripts","startedAt":"2017-08-05T06:50:34Z","lastAt":"2017-08-07T21:48:18Z","messageCount":16,"participants":["Michael Forney","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"325637","messageId":"20170805064905.5948-1-mforney@mforney.org","threadId":"46517","inReplyTo":null,"subject":"[PATCH] Drop some dashes from built-in invocations in scripts","fromName":"Michael Forney","fromEmail":"mforney@mforney.org","sentAt":"2017-08-05T06:49:05Z","receivedAt":"2017-08-05T06:50:34Z","isPatch":true,"sender":{"key":"mforney@mforney.org","avatar":"https://avatars.githubusercontent.com/u/52851?v=4"},"body":"This way, they still work even if the built-in symlinks aren't\ninstalled.\n\nSigned-off-by: Michael Forney <mforney@mforney.org>\n---\nIt looks like there was an effort to do this a number of years ago (through\n`make remove-dashes`). These are just a few I noticed were still left in the\n.sh scripts.\n\n git-merge-octopus.sh  | 2 +-\n git-merge-one-file.sh | 8 ++++----\n git-merge-resolve.sh  | 2 +-\n git-stash.sh          | 2 +-\n git-submodule.sh      | 6 +++---\n 5 files changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/git-merge-octopus.sh b/git-merge-octopus.sh\nindex bcf0d92ec..6c390d6c2 100755\n--- a/git-merge-octopus.sh\n+++ b/git-merge-octopus.sh\n@@ -100,7 +100,7 @@ do\n \tif test $? -ne 0\n \tthen\n \t\tgettextln \"Simple merge did not work, trying automatic merge.\"\n-\t\tgit-merge-index -o git-merge-one-file -a ||\n+\t\tgit merge-index -o git-merge-one-file -a ||\n \t\tOCTOPUS_FAILURE=1\n \t\tnext=$(git write-tree 2>/dev/null)\n \tfi\ndiff --git a/git-merge-one-file.sh b/git-merge-one-file.sh\nindex 424b034e3..9879c5939 100755\n--- a/git-merge-one-file.sh\n+++ b/git-merge-one-file.sh\n@@ -115,16 +115,16 @@ case \"${1:-.}${2:-.}${3:-.}\" in\n \t\t;;\n \tesac\n \n-\tsrc1=$(git-unpack-file $2)\n-\tsrc2=$(git-unpack-file $3)\n+\tsrc1=$(git unpack-file $2)\n+\tsrc2=$(git unpack-file $3)\n \tcase \"$1\" in\n \t'')\n \t\techo \"Added $4 in both, but differently.\"\n-\t\torig=$(git-unpack-file e69de29bb2d1d6434b8b29ae775ad8c2e48c5391)\n+\t\torig=$(git unpack-file e69de29bb2d1d6434b8b29ae775ad8c2e48c5391)\n \t\t;;\n \t*)\n \t\techo \"Auto-merging $4\"\n-\t\torig=$(git-unpack-file $1)\n+\t\torig=$(git unpack-file $1)\n \t\t;;\n \tesac\n \ndiff --git a/git-merge-resolve.sh b/git-merge-resolve.sh\nindex c9da747fc..343fe7bcc 100755\n--- a/git-merge-resolve.sh\n+++ b/git-merge-resolve.sh\n@@ -45,7 +45,7 @@ then\n \texit 0\n else\n \techo \"Simple merge failed, trying Automatic merge.\"\n-\tif git-merge-index -o git-merge-one-file -a\n+\tif git merge-index -o git-merge-one-file -a\n \tthen\n \t\texit 0\n \telse\ndiff --git a/git-stash.sh b/git-stash.sh\nindex 9b6c2da7b..9aa09c3a3 100755\n--- a/git-stash.sh\n+++ b/git-stash.sh\n@@ -573,7 +573,7 @@ apply_stash () {\n \n \tif test -n \"$u_tree\"\n \tthen\n-\t\tGIT_INDEX_FILE=\"$TMPindex\" git-read-tree \"$u_tree\" &&\n+\t\tGIT_INDEX_FILE=\"$TMPindex\" git read-tree \"$u_tree\" &&\n \t\tGIT_INDEX_FILE=\"$TMPindex\" git checkout-index --all &&\n \t\trm -f \"$TMPindex\" ||\n \t\tdie \"$(gettext \"Could not restore untracked files from stash entry\")\"\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex e131760ee..ffa2d6648 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -864,7 +864,7 @@ cmd_summary() {\n \t\t\t\ttest $status != A && test $ignore_config = all && continue\n \t\t\tfi\n \t\t\t# Also show added or modified modules which are checked out\n-\t\t\tGIT_DIR=\"$sm_path/.git\" git-rev-parse --git-dir >/dev/null 2>&1 &&\n+\t\t\tGIT_DIR=\"$sm_path/.git\" git rev-parse --git-dir >/dev/null 2>&1 &&\n \t\t\tprintf '%s\\n' \"$sm_path\"\n \t\tdone\n \t)\n@@ -898,11 +898,11 @@ cmd_summary() {\n \t\tmissing_dst=\n \n \t\ttest $mod_src = 160000 &&\n-\t\t! GIT_DIR=\"$name/.git\" git-rev-parse -q --verify $sha1_src^0 >/dev/null &&\n+\t\t! GIT_DIR=\"$name/.git\" git rev-parse -q --verify $sha1_src^0 >/dev/null &&\n \t\tmissing_src=t\n \n \t\ttest $mod_dst = 160000 &&\n-\t\t! GIT_DIR=\"$name/.git\" git-rev-parse -q --verify $sha1_dst^0 >/dev/null &&\n+\t\t! GIT_DIR=\"$name/.git\" git rev-parse -q --verify $sha1_dst^0 >/dev/null &&\n \t\tmissing_dst=t\n \n \t\tdisplay_name=$(git submodule--helper relative-path \"$name\" \"$wt_prefix\")\n-- \n2.13.3\n\n"},{"id":"325644","messageId":"alpine.DEB.2.21.1.1708060012080.4271@virtualbox","threadId":"46517","inReplyTo":"20170805064905.5948-1-mforney@mforney.org","subject":"Re: [PATCH] Drop some dashes from built-in invocations in scripts","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-08-05T22:15:46Z","receivedAt":"2017-08-05T22:15:55Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Michael,\n\nOn Fri, 4 Aug 2017, Michael Forney wrote:\n\n> This way, they still work even if the built-in symlinks aren't\n> installed.\n> \n> Signed-off-by: Michael Forney <mforney@mforney.org>\n> ---\n> It looks like there was an effort to do this a number of years ago (through\n> `make remove-dashes`). These are just a few I noticed were still left in the\n> .sh scripts.\n\nI am very much in favor of this patch. It also helps with things like\nbuilding Git in Visual Studio (where hard-linking builtins is not part of\nthe process).\n\nSee also\nhttps://github.com/git-for-windows/git/compare/2006e3973ce6%5E...2006e3973ce6%5E2\n\nThanks,\nJohannes\n"},{"id":"325646","messageId":"xmqqlgmxskm6.fsf@gitster.mtv.corp.google.com","threadId":"46517","inReplyTo":"20170805064905.5948-1-mforney@mforney.org","subject":"Re: [PATCH] Drop some dashes from built-in invocations in scripts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-05T22:41:37Z","receivedAt":"2017-08-05T22:41:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Forney <mforney@mforney.org> writes:\n\n> This way, they still work even if the built-in symlinks aren't\n> installed.\n>\n> Signed-off-by: Michael Forney <mforney@mforney.org>\n> ---\n> It looks like there was an effort to do this a number of years ago (through\n> `make remove-dashes`). These are just a few I noticed were still left in the\n> .sh scripts.\n\nThanks for working on this.  \n\nHave you made sure that all of these scripts, before calling\n'git-foo' in the current code, update their PATH so that these found\nin the bog standard place (i.e. GIT_EXEC_PATH)?\n\nThe reason I ask is because we can rest assured these changes will\nbe a no-regression improvement if you did so.  I do not offhand\nthink of a reason why these scripts wouldn't be doing so, but it\nnever hurts to make sure.\n\n>\n>  git-merge-octopus.sh  | 2 +-\n>  git-merge-one-file.sh | 8 ++++----\n>  git-merge-resolve.sh  | 2 +-\n>  git-stash.sh          | 2 +-\n>  git-submodule.sh      | 6 +++---\n>  5 files changed, 10 insertions(+), 10 deletions(-)\n>\n> diff --git a/git-merge-octopus.sh b/git-merge-octopus.sh\n> index bcf0d92ec..6c390d6c2 100755\n> --- a/git-merge-octopus.sh\n> +++ b/git-merge-octopus.sh\n> @@ -100,7 +100,7 @@ do\n>  \tif test $? -ne 0\n>  \tthen\n>  \t\tgettextln \"Simple merge did not work, trying automatic merge.\"\n> -\t\tgit-merge-index -o git-merge-one-file -a ||\n> +\t\tgit merge-index -o git-merge-one-file -a ||\n>  \t\tOCTOPUS_FAILURE=1\n>  \t\tnext=$(git write-tree 2>/dev/null)\n>  \tfi\n> diff --git a/git-merge-one-file.sh b/git-merge-one-file.sh\n> index 424b034e3..9879c5939 100755\n> --- a/git-merge-one-file.sh\n> +++ b/git-merge-one-file.sh\n> @@ -115,16 +115,16 @@ case \"${1:-.}${2:-.}${3:-.}\" in\n>  \t\t;;\n>  \tesac\n>  \n> -\tsrc1=$(git-unpack-file $2)\n> -\tsrc2=$(git-unpack-file $3)\n> +\tsrc1=$(git unpack-file $2)\n> +\tsrc2=$(git unpack-file $3)\n>  \tcase \"$1\" in\n>  \t'')\n>  \t\techo \"Added $4 in both, but differently.\"\n> -\t\torig=$(git-unpack-file e69de29bb2d1d6434b8b29ae775ad8c2e48c5391)\n> +\t\torig=$(git unpack-file e69de29bb2d1d6434b8b29ae775ad8c2e48c5391)\n>  \t\t;;\n>  \t*)\n>  \t\techo \"Auto-merging $4\"\n> -\t\torig=$(git-unpack-file $1)\n> +\t\torig=$(git unpack-file $1)\n>  \t\t;;\n>  \tesac\n>  \n> diff --git a/git-merge-resolve.sh b/git-merge-resolve.sh\n> index c9da747fc..343fe7bcc 100755\n> --- a/git-merge-resolve.sh\n> +++ b/git-merge-resolve.sh\n> @@ -45,7 +45,7 @@ then\n>  \texit 0\n>  else\n>  \techo \"Simple merge failed, trying Automatic merge.\"\n> -\tif git-merge-index -o git-merge-one-file -a\n> +\tif git merge-index -o git-merge-one-file -a\n>  \tthen\n>  \t\texit 0\n>  \telse\n> diff --git a/git-stash.sh b/git-stash.sh\n> index 9b6c2da7b..9aa09c3a3 100755\n> --- a/git-stash.sh\n> +++ b/git-stash.sh\n> @@ -573,7 +573,7 @@ apply_stash () {\n>  \n>  \tif test -n \"$u_tree\"\n>  \tthen\n> -\t\tGIT_INDEX_FILE=\"$TMPindex\" git-read-tree \"$u_tree\" &&\n> +\t\tGIT_INDEX_FILE=\"$TMPindex\" git read-tree \"$u_tree\" &&\n>  \t\tGIT_INDEX_FILE=\"$TMPindex\" git checkout-index --all &&\n>  \t\trm -f \"$TMPindex\" ||\n>  \t\tdie \"$(gettext \"Could not restore untracked files from stash entry\")\"\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index e131760ee..ffa2d6648 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -864,7 +864,7 @@ cmd_summary() {\n>  \t\t\t\ttest $status != A && test $ignore_config = all && continue\n>  \t\t\tfi\n>  \t\t\t# Also show added or modified modules which are checked out\n> -\t\t\tGIT_DIR=\"$sm_path/.git\" git-rev-parse --git-dir >/dev/null 2>&1 &&\n> +\t\t\tGIT_DIR=\"$sm_path/.git\" git rev-parse --git-dir >/dev/null 2>&1 &&\n>  \t\t\tprintf '%s\\n' \"$sm_path\"\n>  \t\tdone\n>  \t)\n> @@ -898,11 +898,11 @@ cmd_summary() {\n>  \t\tmissing_dst=\n>  \n>  \t\ttest $mod_src = 160000 &&\n> -\t\t! GIT_DIR=\"$name/.git\" git-rev-parse -q --verify $sha1_src^0 >/dev/null &&\n> +\t\t! GIT_DIR=\"$name/.git\" git rev-parse -q --verify $sha1_src^0 >/dev/null &&\n>  \t\tmissing_src=t\n>  \n>  \t\ttest $mod_dst = 160000 &&\n> -\t\t! GIT_DIR=\"$name/.git\" git-rev-parse -q --verify $sha1_dst^0 >/dev/null &&\n> +\t\t! GIT_DIR=\"$name/.git\" git rev-parse -q --verify $sha1_dst^0 >/dev/null &&\n>  \t\tmissing_dst=t\n>  \n>  \t\tdisplay_name=$(git submodule--helper relative-path \"$name\" \"$wt_prefix\")\n"},{"id":"325647","messageId":"CAGw6cBtKF-Xt4z3m4gBDQvaSnurbtHURe737s8XMX78ca_RTcA@mail.gmail.com","threadId":"46517","inReplyTo":"xmqqlgmxskm6.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] Drop some dashes from built-in invocations in scripts","fromName":"Michael Forney","fromEmail":"mforney@mforney.org","sentAt":"2017-08-05T22:54:32Z","receivedAt":"2017-08-05T22:54:40Z","isPatch":true,"sender":{"key":"mforney@mforney.org","avatar":"https://avatars.githubusercontent.com/u/52851?v=4"},"body":"On 8/5/17, Junio C Hamano <gitster@pobox.com> wrote:\n> Have you made sure that all of these scripts, before calling\n> 'git-foo' in the current code, update their PATH so that these found\n> in the bog standard place (i.e. GIT_EXEC_PATH)?\n>\n> The reason I ask is because we can rest assured these changes will\n> be a no-regression improvement if you did so.  I do not offhand\n> think of a reason why these scripts wouldn't be doing so, but it\n> never hurts to make sure.\n\nI just checked and all the scripts make some other call to a built-in\nwith `git foo`, so I think it should be safe.\n"},{"id":"325649","messageId":"alpine.DEB.2.21.1.1708060127330.4271@virtualbox","threadId":"46517","inReplyTo":"xmqqlgmxskm6.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] Drop some dashes from built-in invocations in scripts","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-08-05T23:30:21Z","receivedAt":"2017-08-05T23:30:37Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Sat, 5 Aug 2017, Junio C Hamano wrote:\n\n> Michael Forney <mforney@mforney.org> writes:\n> \n> > This way, they still work even if the built-in symlinks aren't\n> > installed.\n> >\n> > Signed-off-by: Michael Forney <mforney@mforney.org>\n> > ---\n> > It looks like there was an effort to do this a number of years ago (through\n> > `make remove-dashes`). These are just a few I noticed were still left in the\n> > .sh scripts.\n> \n> Thanks for working on this.  \n> \n> Have you made sure that all of these scripts, before calling\n> 'git-foo' in the current code, update their PATH so that these found\n> in the bog standard place (i.e. GIT_EXEC_PATH)?\n\nThe scripts do not update their PATH. `git` does, before they are called.\n\nFor example, if you call `git stash`, the `git` executable will first\nfigure out that `stash` is not a builtin (unless you run with the\nexperimental patch series that still has not seen much review, more is the\nshame, including on myself), and then call the dashed form. At this point,\n`git` will already have added the exec path to the PATH variable so that\n`git-stash` is found. So then `git-stash` runs, and does not even need to\ntake pains to adjust the PATH variable again! Magic!\n\n;-)\n\nCiao,\nDscho\n"},{"id":"325650","messageId":"xmqqh8xlsiaq.fsf@gitster.mtv.corp.google.com","threadId":"46517","inReplyTo":"CAGw6cBtKF-Xt4z3m4gBDQvaSnurbtHURe737s8XMX78ca_RTcA@mail.gmail.com","subject":"Re: [PATCH] Drop some dashes from built-in invocations in scripts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-05T23:31:41Z","receivedAt":"2017-08-05T23:31:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Forney <mforney@mforney.org> writes:\n\n> On 8/5/17, Junio C Hamano <gitster@pobox.com> wrote:\n>> Have you made sure that all of these scripts, before calling\n>> 'git-foo' in the current code, update their PATH so that these found\n>> in the bog standard place (i.e. GIT_EXEC_PATH)?\n>>\n>> The reason I ask is because we can rest assured these changes will\n>> be a no-regression improvement if you did so.  I do not offhand\n>> think of a reason why these scripts wouldn't be doing so, but it\n>> never hurts to make sure.\n>\n> I just checked and all the scripts make some other call to a built-in\n> with `git foo`, so I think it should be safe.\n\nAs long as they are the same \"foo\"'s, then the check you did is\nperfectly fine.  The (unlikely I would think) case that can lead to\na regression is if these script deliberately used `git-foo` to find\nthem on $PATH, which can be different from 'git foo' that is found\nby 'git' in its own binary (as all of them are built-ins), and that\nis why I asked.\n\nThanks.\n"},{"id":"325652","messageId":"CAGw6cBsYiGH1h8C8qFp-yX3arzkaRi_vghpjbErxjoYHXxpu+Q@mail.gmail.com","threadId":"46517","inReplyTo":"xmqqh8xlsiaq.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] Drop some dashes from built-in invocations in scripts","fromName":"Michael Forney","fromEmail":"mforney@mforney.org","sentAt":"2017-08-06T05:35:47Z","receivedAt":"2017-08-06T05:35:54Z","isPatch":true,"sender":{"key":"mforney@mforney.org","avatar":"https://avatars.githubusercontent.com/u/52851?v=4"},"body":"On 8/5/17, Junio C Hamano <gitster@pobox.com> wrote:\n> Michael Forney <mforney@mforney.org> writes:\n>> On 8/5/17, Junio C Hamano <gitster@pobox.com> wrote:\n>>> Have you made sure that all of these scripts, before calling\n>>> 'git-foo' in the current code, update their PATH so that these found\n>>> in the bog standard place (i.e. GIT_EXEC_PATH)?\n>>>\n>>> The reason I ask is because we can rest assured these changes will\n>>> be a no-regression improvement if you did so.  I do not offhand\n>>> think of a reason why these scripts wouldn't be doing so, but it\n>>> never hurts to make sure.\n>>\n>> I just checked and all the scripts make some other call to a built-in\n>> with `git foo`, so I think it should be safe.\n>\n> As long as they are the same \"foo\"'s, then the check you did is\n> perfectly fine.  The (unlikely I would think) case that can lead to\n> a regression is if these script deliberately used `git-foo` to find\n> them on $PATH, which can be different from 'git foo' that is found\n> by 'git' in its own binary (as all of them are built-ins), and that\n> is why I asked.\n\nAh. Well, it looks like all but git-merge-resolve.sh run `.\ngit-sh-setup`, so we know that GIT_EXEC_PATH must in their PATH (and\nat the front unless the script was invoked directly).\n\ngit-merge-resolve.sh does not do this, so I suppose if the user ran\n$GIT_EXEC_PATH/git-merge-resolve directly, and also had a custom\ngit-merge-index executable in their PATH, that would switch to running\nthe git merge-index built-in instead.\n"},{"id":"325664","messageId":"xmqq8tiwrwo3.fsf@gitster.mtv.corp.google.com","threadId":"46517","inReplyTo":"CAGw6cBsYiGH1h8C8qFp-yX3arzkaRi_vghpjbErxjoYHXxpu+Q@mail.gmail.com","subject":"Re: [PATCH] Drop some dashes from built-in invocations in scripts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-07T01:31:08Z","receivedAt":"2017-08-07T01:31:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Forney <mforney@mforney.org> writes:\n\n> Ah. Well, it looks like all but git-merge-resolve.sh run `.\n> git-sh-setup`, so we know that GIT_EXEC_PATH must in their PATH (and\n> at the front unless the script was invoked directly).\n>\n> git-merge-resolve.sh does not do this, so I suppose if the user ran\n> $GIT_EXEC_PATH/git-merge-resolve directly, and also had a custom\n> git-merge-index executable in their PATH, that would switch to running\n> the git merge-index built-in instead.\n\nWe are good then.  Nobody other than \"git merge\" should be running\nmerge-resolve, so the original probably is OK.\n\nThanks for digging deeper.\n"},{"id":"325694","messageId":"xmqqshh3qqs4.fsf@gitster.mtv.corp.google.com","threadId":"46517","inReplyTo":"20170805064905.5948-1-mforney@mforney.org","subject":"Re: [PATCH] Drop some dashes from built-in invocations in scripts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-07T16:35:55Z","receivedAt":"2017-08-07T16:36:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Forney <mforney@mforney.org> writes:\n\n> This way, they still work even if the built-in symlinks aren't\n> installed.\n>\n> Signed-off-by: Michael Forney <mforney@mforney.org>\n> ---\n> It looks like there was an effort to do this a number of years ago (through\n> `make remove-dashes`). These are just a few I noticed were still left in the\n> .sh scripts.\n\nOur goal was *not* to have *no* \"git-foo\" on the filesystem,\nthough.  It happened in v1.6.0 timeframe and it was about removing\n\"git-foo\" from end-user's $PATH.\n\nEarlier there was a more ambitious proposal to remove all \"git-foo\"\neven from $GIT_EXEC_PATH for built-in commands, but that plan was\nscuttled [*1*].\n\nThe changes in your patch still are good changes to make sure people\nwho copy & paste code would see fewer instances of \"git-foo\", but\n\"will still work even if I break my installation of Git by removing\nthem from the filesystem\" is not the project's goal.  \n\nIIUC, you will need \"$GIT_EXEC_PATH/git-checkout\" on the filesystem\nif you want your \"git co\" alias to work, as we spawn built-in as a\ndashed external.\n\n\n[Reference]\n\n*1* https://public-inbox.org/git/alpine.LFD.1.10.0808261114070.3363@nehalem.linux-foundation.org/\n\n\n"},{"id":"325700","messageId":"xmqqo9rrqp3l.fsf@gitster.mtv.corp.google.com","threadId":"46517","inReplyTo":"xmqqshh3qqs4.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] Drop some dashes from built-in invocations in scripts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-07T17:12:14Z","receivedAt":"2017-08-07T17:12:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Earlier there was a more ambitious proposal to remove all \"git-foo\"\n> even from $GIT_EXEC_PATH for built-in commands, but that plan was\n> scuttled [*1*].\n>\n> The changes in your patch still are good changes to make sure people\n> who copy & paste code would see fewer instances of \"git-foo\", but\n> \"will still work even if I break my installation of Git by removing\n> them from the filesystem\" is not the project's goal.  \n>\n> IIUC, you will need \"$GIT_EXEC_PATH/git-checkout\" on the filesystem\n> if you want your \"git co\" alias to work, as we spawn built-in as a\n> dashed external.\n\nJust to avoid possible confusion, the above is not to say \"once it\nis decided, you are not allowed to bring fresh arguments to the\ndiscussion\".  As Peff said [*2*] in that old discussion thread, the\ncircumstances may have changed over 9 years, and it may benefit to\nrevisit some old decisions.\n\nSo in that sense, I do not mind somebody makes a fresh proposal,\nwhich would probably be similar to what I did back then in [*3*],\nwhich is at the beginning of that old thread.  But I do not think I\nwould be doing so myself, and I suspect that I would not be very\nsupportive for such a proposal, because my gut feeling is that the\nupside is not big enough compared to downsides.\n\nThe obvious upside is that you do not have to have many built-in\ncommands on the filesystem, either as a hardlink, a copy, or a\nsymbolic link.  But we will be breaking people's scripts by breaking\nthe 11-year old promise that we will keep their \"git-foo\" working as\nlong as they prepend $GIT_EXEC_PATH to their $PATH we we did so.\nAnother downside is that we now will expose which subcommands are\nbuilt-in and which are not, which is unnecessarily implementation\ndetail we'd want end-users to rely on.\n\nThe \"'git co' may stop working\" I mentioned in my previous message\nis not counted as a downside---if the upside is large enough, I\nthink that \"we respawn git-foo as dashed built-in when running an\nalias that expands to 'foo'\" can be fixed to respawn \"git foo\"\ninstead of \"git-foo\".  But there may be other downside that we may\nnot be able to fix, and I suspect that \"we'd be breaking the 11-year\nold promise\" is something we would not be able to fix.  That is why\nI doubt that I would be advocating the removal of \"git-foo\" from the\nfilesystem myself.\n\n\n[References]\n\n*1* https://public-inbox.org/git/alpine.LFD.1.10.0808261114070.3363@nehalem.linux-foundation.org/\n\n*2* https://public-inbox.org/git/20080826145719.GB5046@coredump.intra.peff.net/\n\n*3* https://public-inbox.org/git/7vprnzt7d5.fsf@gitster.siamese.dyndns.org/\n\n"},{"id":"325708","messageId":"CAGw6cBvE-OJW0gSRCYOS5VT+Ff+eiCMdJg2A2rBi_HXFObdJRw@mail.gmail.com","threadId":"46517","inReplyTo":"xmqqo9rrqp3l.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] Drop some dashes from built-in invocations in scripts","fromName":"Michael Forney","fromEmail":"mforney@mforney.org","sentAt":"2017-08-07T17:58:34Z","receivedAt":"2017-08-07T17:58:44Z","isPatch":true,"sender":{"key":"mforney@mforney.org","avatar":"https://avatars.githubusercontent.com/u/52851?v=4"},"body":"On 8/7/17, Junio C Hamano <gitster@pobox.com> wrote:\n> Just to avoid possible confusion, the above is not to say \"once it\n> is decided, you are not allowed to bring fresh arguments to the\n> discussion\".  As Peff said [*2*] in that old discussion thread, the\n> circumstances may have changed over 9 years, and it may benefit to\n> revisit some old decisions.\n>\n> So in that sense, I do not mind somebody makes a fresh proposal,\n> which would probably be similar to what I did back then in [*3*],\n> which is at the beginning of that old thread.  But I do not think I\n> would be doing so myself, and I suspect that I would not be very\n> supportive for such a proposal, because my gut feeling is that the\n> upside is not big enough compared to downsides.\n>\n> The obvious upside is that you do not have to have many built-in\n> commands on the filesystem, either as a hardlink, a copy, or a\n> symbolic link.  But we will be breaking people's scripts by breaking\n> the 11-year old promise that we will keep their \"git-foo\" working as\n> long as they prepend $GIT_EXEC_PATH to their $PATH we we did so.\n> Another downside is that we now will expose which subcommands are\n> built-in and which are not, which is unnecessarily implementation\n> detail we'd want end-users to rely on.\n>\n> The \"'git co' may stop working\" I mentioned in my previous message\n> is not counted as a downside---if the upside is large enough, I\n> think that \"we respawn git-foo as dashed built-in when running an\n> alias that expands to 'foo'\" can be fixed to respawn \"git foo\"\n> instead of \"git-foo\".  But there may be other downside that we may\n> not be able to fix, and I suspect that \"we'd be breaking the 11-year\n> old promise\" is something we would not be able to fix.  That is why\n> I doubt that I would be advocating the removal of \"git-foo\" from the\n> filesystem myself.\n\nThanks for the history and explanation, Junio. I agree with your analysis.\n\nI didn't know that git aliases invoke the `git-foo` path for built-ins\n(I don't use them much myself, so didn't notice).\n\nI also didn't know that it was supported to add GIT_EXEC_DIR to your\nPATH to be able to call `git-foo`. I generally think of /libexec as\nimplementation-specific executables that a tool may call internally\n(in that sense, whether or not the commands are built-ins would remain\nan implementation-detail).\n\nHowever, I still think the patch should be applied for\nself-consistency at least (git-submodule.sh currently calls both `git\nrev-parse` and `git-rev-parse`). Also, based on Johannes' reply, it\nmay still be useful for git-for-windows.\n\n>\n> [References]\n>\n> *1*\n> https://public-inbox.org/git/alpine.LFD.1.10.0808261114070.3363@nehalem.linux-foundation.org/\n>\n> *2*\n> https://public-inbox.org/git/20080826145719.GB5046@coredump.intra.peff.net/\n>\n> *3* https://public-inbox.org/git/7vprnzt7d5.fsf@gitster.siamese.dyndns.org/\n"},{"id":"325713","messageId":"xmqq60dzqm0z.fsf@gitster.mtv.corp.google.com","threadId":"46517","inReplyTo":"CAGw6cBvE-OJW0gSRCYOS5VT+Ff+eiCMdJg2A2rBi_HXFObdJRw@mail.gmail.com","subject":"Re: [PATCH] Drop some dashes from built-in invocations in scripts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-07T18:18:36Z","receivedAt":"2017-08-07T18:18:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Forney <mforney@mforney.org> writes:\n\n> However, I still think the patch should be applied for\n> self-consistency at least (git-submodule.sh currently calls both `git\n> rev-parse` and `git-rev-parse`).\n\nOh, there is no question about the changes in the patch being good,\nas I already said.  We want to make sure that people who copy &\npaste code would see fewer instances of \"git-foo\".\n\nI was commenting on the justification in your proposed log message,\nwhich I realized was a bit misleading, after deciding that the patch\ntext itself is good and I need to apply it.\n\nThanks for the clean-up.\n"},{"id":"325728","messageId":"alpine.DEB.2.21.1.1708072113570.4271@virtualbox","threadId":"46517","inReplyTo":"xmqqshh3qqs4.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] Drop some dashes from built-in invocations in scripts","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-08-07T19:22:57Z","receivedAt":"2017-08-07T19:23:21Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 7 Aug 2017, Junio C Hamano wrote:\n\n> Michael Forney <mforney@mforney.org> writes:\n> \n> > This way, they still work even if the built-in symlinks aren't\n> > installed.\n> >\n> > Signed-off-by: Michael Forney <mforney@mforney.org>\n> > ---\n> > It looks like there was an effort to do this a number of years ago (through\n> > `make remove-dashes`). These are just a few I noticed were still left in the\n> > .sh scripts.\n> \n> Our goal was *not* to have *no* \"git-foo\" on the filesystem,\n> though. It happened in v1.6.0 timeframe and it was about removing\n> \"git-foo\" from end-user's $PATH.\n\nThat was in the v1.6.0 timeframe. It is past time to reconsider the goal,\nthough, as there is really very little use in keeping the dashed forms.\n\nAnd it does hurt in some circumstances. Take for example .zip files: they\ndo not support hard-links. So if you need to distribute Git binaries in\n.zip files, you are not only affected negatively by the less-than-stellar\ncompression ratio compared to .bz2 let alone LZMA, Git adds insult to\ninjury by *forcing* an additional inflation by pointlessly adding the\nbuiltins *again*.\n\n> Earlier there was a more ambitious proposal to remove all \"git-foo\"\n> even from $GIT_EXEC_PATH for built-in commands, but that plan was\n> scuttled [*1*].\n\nThat *1* is not a good reference, I am afraid. It says very little in\naddition to paraphrased commands to stop responding (when a more civilized\ncall back to rational arguments might have been a lot more productive).\n\nIn that light, would you kindly explain in your own words what is your\ncurrent thinking on shipping dashed forms of builtins?\n\nI mean, I can understand for git-upload-pack, to help with determining\npermissions on server sides (it is easier to filter out all `git` commands\nthan to painstakingly look whether argv[1] equals `upload-pack`). It's\nsort of a very unfortunate outlier.\n\nBut I cannot understand at all why we insist on installing hardlinked\ncopies (or not so hardlinked copies when hardlinks are unavailable) for\nbuiltins, when these copies really outlived their usefulness a long, long\ntime ago.\n\nSo I would love to hear the arguments for keeping the dashed forms of\nbuiltins, even if the only surviving argument may be \"I dig in my feet\nbecause I always said we'd keep them\".\n\nCiao,\nDscho\n"},{"id":"325734","messageId":"xmqqo9rrp3ab.fsf@gitster.mtv.corp.google.com","threadId":"46517","inReplyTo":"alpine.DEB.2.21.1.1708072113570.4271@virtualbox","subject":"Re: [PATCH] Drop some dashes from built-in invocations in scripts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-07T19:48:44Z","receivedAt":"2017-08-07T19:48:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> So I would love to hear the arguments for keeping the dashed forms of\n> builtins, even if the only surviving argument may be \"I dig in my feet\n> because I always said we'd keep them\".\n\nI think I already did ;-)\n"},{"id":"325743","messageId":"alpine.DEB.2.21.1.1708072300310.4271@virtualbox","threadId":"46517","inReplyTo":"xmqqshh3qqs4.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] Drop some dashes from built-in invocations in scripts","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-08-07T21:07:04Z","receivedAt":"2017-08-07T21:07:24Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nI feel a bit talked to my hand, as the only reply I was graced was a \"I\nthink I already did\". So this will be my last reply on this matter for a\nwhile.\n\nOn Mon, 7 Aug 2017, Junio C Hamano wrote:\n\n> IIUC, you will need \"$GIT_EXEC_PATH/git-checkout\" on the filesystem if\n> you want your \"git co\" alias to work, as we spawn built-in as a dashed\n> external.\n\nAnd of course this is just the status quo, not an argument why it should\nbe so for eternity. Because that would be circular reasoning and prevent\nus from improving things.\n\nIt is still arguably wrong to call the dashed form for builtins when we\nalready have enough information at our hands to tell that it is a builtin:\n\n\thttps://github.com/git-for-windows/git/commit/bad2c6978ec\n\nGranted, this duplicates code a little, as it was developed under time\npressure (and it is necessary to allow the test suite to be run on Git\nbuilt in Visual Studio using an installed Git for Windows for the Unix\nutilities). As above, though, the current state of this patch does not\nprevent any improvement in the future.\n\nCiao,\nDscho\n"},{"id":"325766","messageId":"xmqqshh3nj75.fsf@gitster.mtv.corp.google.com","threadId":"46517","inReplyTo":"alpine.DEB.2.21.1.1708072300310.4271@virtualbox","subject":"Re: [PATCH] Drop some dashes from built-in invocations in scripts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-07T21:47:58Z","receivedAt":"2017-08-07T21:48:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> I feel a bit talked to my hand, as the only reply I was graced was a \"I\n> think I already did\". So this will be my last reply on this matter for a\n> while.\n\nAh, I meant this thing:\n\n  https://public-inbox.org/git/xmqqo9rrqp3l.fsf@gitster.mtv.corp.google.com\n\nI got an impression that you didn't read it before you typed the\nmessage I gave that response to.\n\nThere are a few more exchanged, including Michael's response to that\nmessage on that thread.\n"}]}