git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* [PATCH] Drop some dashes from built-in invocations in scripts
@ 2017-08-05  6:49 Michael Forney
  2017-08-05 22:15 ` Johannes Schindelin
                   ` (2 more replies)
  0 siblings, 3 replies; 16+ messages in thread
From: Michael Forney @ 2017-08-05  6:49 UTC (permalink / raw)
  To: git

This way, they still work even if the built-in symlinks aren't
installed.

Signed-off-by: Michael Forney <mforney@mforney.org>
---
It looks like there was an effort to do this a number of years ago (through
`make remove-dashes`). These are just a few I noticed were still left in the
.sh scripts.

 git-merge-octopus.sh  | 2 +-
 git-merge-one-file.sh | 8 ++++----
 git-merge-resolve.sh  | 2 +-
 git-stash.sh          | 2 +-
 git-submodule.sh      | 6 +++---
 5 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/git-merge-octopus.sh b/git-merge-octopus.sh
index bcf0d92ec..6c390d6c2 100755
--- a/git-merge-octopus.sh
+++ b/git-merge-octopus.sh
@@ -100,7 +100,7 @@ do
 	if test $? -ne 0
 	then
 		gettextln "Simple merge did not work, trying automatic merge."
-		git-merge-index -o git-merge-one-file -a ||
+		git merge-index -o git-merge-one-file -a ||
 		OCTOPUS_FAILURE=1
 		next=$(git write-tree 2>/dev/null)
 	fi
diff --git a/git-merge-one-file.sh b/git-merge-one-file.sh
index 424b034e3..9879c5939 100755
--- a/git-merge-one-file.sh
+++ b/git-merge-one-file.sh
@@ -115,16 +115,16 @@ case "${1:-.}${2:-.}${3:-.}" in
 		;;
 	esac
 
-	src1=$(git-unpack-file $2)
-	src2=$(git-unpack-file $3)
+	src1=$(git unpack-file $2)
+	src2=$(git unpack-file $3)
 	case "$1" in
 	'')
 		echo "Added $4 in both, but differently."
-		orig=$(git-unpack-file e69de29bb2d1d6434b8b29ae775ad8c2e48c5391)
+		orig=$(git unpack-file e69de29bb2d1d6434b8b29ae775ad8c2e48c5391)
 		;;
 	*)
 		echo "Auto-merging $4"
-		orig=$(git-unpack-file $1)
+		orig=$(git unpack-file $1)
 		;;
 	esac
 
diff --git a/git-merge-resolve.sh b/git-merge-resolve.sh
index c9da747fc..343fe7bcc 100755
--- a/git-merge-resolve.sh
+++ b/git-merge-resolve.sh
@@ -45,7 +45,7 @@ then
 	exit 0
 else
 	echo "Simple merge failed, trying Automatic merge."
-	if git-merge-index -o git-merge-one-file -a
+	if git merge-index -o git-merge-one-file -a
 	then
 		exit 0
 	else
diff --git a/git-stash.sh b/git-stash.sh
index 9b6c2da7b..9aa09c3a3 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -573,7 +573,7 @@ apply_stash () {
 
 	if test -n "$u_tree"
 	then
-		GIT_INDEX_FILE="$TMPindex" git-read-tree "$u_tree" &&
+		GIT_INDEX_FILE="$TMPindex" git read-tree "$u_tree" &&
 		GIT_INDEX_FILE="$TMPindex" git checkout-index --all &&
 		rm -f "$TMPindex" ||
 		die "$(gettext "Could not restore untracked files from stash entry")"
diff --git a/git-submodule.sh b/git-submodule.sh
index e131760ee..ffa2d6648 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -864,7 +864,7 @@ cmd_summary() {
 				test $status != A && test $ignore_config = all && continue
 			fi
 			# Also show added or modified modules which are checked out
-			GIT_DIR="$sm_path/.git" git-rev-parse --git-dir >/dev/null 2>&1 &&
+			GIT_DIR="$sm_path/.git" git rev-parse --git-dir >/dev/null 2>&1 &&
 			printf '%s\n' "$sm_path"
 		done
 	)
@@ -898,11 +898,11 @@ cmd_summary() {
 		missing_dst=
 
 		test $mod_src = 160000 &&
-		! GIT_DIR="$name/.git" git-rev-parse -q --verify $sha1_src^0 >/dev/null &&
+		! GIT_DIR="$name/.git" git rev-parse -q --verify $sha1_src^0 >/dev/null &&
 		missing_src=t
 
 		test $mod_dst = 160000 &&
-		! GIT_DIR="$name/.git" git-rev-parse -q --verify $sha1_dst^0 >/dev/null &&
+		! GIT_DIR="$name/.git" git rev-parse -q --verify $sha1_dst^0 >/dev/null &&
 		missing_dst=t
 
 		display_name=$(git submodule--helper relative-path "$name" "$wt_prefix")
-- 
2.13.3


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH] Drop some dashes from built-in invocations in scripts
  2017-08-05  6:49 [PATCH] Drop some dashes from built-in invocations in scripts Michael Forney
@ 2017-08-05 22:15 ` Johannes Schindelin
  2017-08-05 22:41 ` Junio C Hamano
  2017-08-07 16:35 ` Junio C Hamano
  2 siblings, 0 replies; 16+ messages in thread
From: Johannes Schindelin @ 2017-08-05 22:15 UTC (permalink / raw)
  To: Michael Forney; +Cc: git

Hi Michael,

On Fri, 4 Aug 2017, Michael Forney wrote:

> This way, they still work even if the built-in symlinks aren't
> installed.
> 
> Signed-off-by: Michael Forney <mforney@mforney.org>
> ---
> It looks like there was an effort to do this a number of years ago (through
> `make remove-dashes`). These are just a few I noticed were still left in the
> .sh scripts.

I am very much in favor of this patch. It also helps with things like
building Git in Visual Studio (where hard-linking builtins is not part of
the process).

See also
https://github.com/git-for-windows/git/compare/2006e3973ce6%5E...2006e3973ce6%5E2

Thanks,
Johannes

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] Drop some dashes from built-in invocations in scripts
  2017-08-05  6:49 [PATCH] Drop some dashes from built-in invocations in scripts Michael Forney
  2017-08-05 22:15 ` Johannes Schindelin
@ 2017-08-05 22:41 ` Junio C Hamano
  2017-08-05 22:54   ` Michael Forney
  2017-08-05 23:30   ` Johannes Schindelin
  2017-08-07 16:35 ` Junio C Hamano
  2 siblings, 2 replies; 16+ messages in thread
From: Junio C Hamano @ 2017-08-05 22:41 UTC (permalink / raw)
  To: Michael Forney; +Cc: git

Michael Forney <mforney@mforney.org> writes:

> This way, they still work even if the built-in symlinks aren't
> installed.
>
> Signed-off-by: Michael Forney <mforney@mforney.org>
> ---
> It looks like there was an effort to do this a number of years ago (through
> `make remove-dashes`). These are just a few I noticed were still left in the
> .sh scripts.

Thanks for working on this.  

Have you made sure that all of these scripts, before calling
'git-foo' in the current code, update their PATH so that these found
in the bog standard place (i.e. GIT_EXEC_PATH)?

The reason I ask is because we can rest assured these changes will
be a no-regression improvement if you did so.  I do not offhand
think of a reason why these scripts wouldn't be doing so, but it
never hurts to make sure.

>
>  git-merge-octopus.sh  | 2 +-
>  git-merge-one-file.sh | 8 ++++----
>  git-merge-resolve.sh  | 2 +-
>  git-stash.sh          | 2 +-
>  git-submodule.sh      | 6 +++---
>  5 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/git-merge-octopus.sh b/git-merge-octopus.sh
> index bcf0d92ec..6c390d6c2 100755
> --- a/git-merge-octopus.sh
> +++ b/git-merge-octopus.sh
> @@ -100,7 +100,7 @@ do
>  	if test $? -ne 0
>  	then
>  		gettextln "Simple merge did not work, trying automatic merge."
> -		git-merge-index -o git-merge-one-file -a ||
> +		git merge-index -o git-merge-one-file -a ||
>  		OCTOPUS_FAILURE=1
>  		next=$(git write-tree 2>/dev/null)
>  	fi
> diff --git a/git-merge-one-file.sh b/git-merge-one-file.sh
> index 424b034e3..9879c5939 100755
> --- a/git-merge-one-file.sh
> +++ b/git-merge-one-file.sh
> @@ -115,16 +115,16 @@ case "${1:-.}${2:-.}${3:-.}" in
>  		;;
>  	esac
>  
> -	src1=$(git-unpack-file $2)
> -	src2=$(git-unpack-file $3)
> +	src1=$(git unpack-file $2)
> +	src2=$(git unpack-file $3)
>  	case "$1" in
>  	'')
>  		echo "Added $4 in both, but differently."
> -		orig=$(git-unpack-file e69de29bb2d1d6434b8b29ae775ad8c2e48c5391)
> +		orig=$(git unpack-file e69de29bb2d1d6434b8b29ae775ad8c2e48c5391)
>  		;;
>  	*)
>  		echo "Auto-merging $4"
> -		orig=$(git-unpack-file $1)
> +		orig=$(git unpack-file $1)
>  		;;
>  	esac
>  
> diff --git a/git-merge-resolve.sh b/git-merge-resolve.sh
> index c9da747fc..343fe7bcc 100755
> --- a/git-merge-resolve.sh
> +++ b/git-merge-resolve.sh
> @@ -45,7 +45,7 @@ then
>  	exit 0
>  else
>  	echo "Simple merge failed, trying Automatic merge."
> -	if git-merge-index -o git-merge-one-file -a
> +	if git merge-index -o git-merge-one-file -a
>  	then
>  		exit 0
>  	else
> diff --git a/git-stash.sh b/git-stash.sh
> index 9b6c2da7b..9aa09c3a3 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -573,7 +573,7 @@ apply_stash () {
>  
>  	if test -n "$u_tree"
>  	then
> -		GIT_INDEX_FILE="$TMPindex" git-read-tree "$u_tree" &&
> +		GIT_INDEX_FILE="$TMPindex" git read-tree "$u_tree" &&
>  		GIT_INDEX_FILE="$TMPindex" git checkout-index --all &&
>  		rm -f "$TMPindex" ||
>  		die "$(gettext "Could not restore untracked files from stash entry")"
> diff --git a/git-submodule.sh b/git-submodule.sh
> index e131760ee..ffa2d6648 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -864,7 +864,7 @@ cmd_summary() {
>  				test $status != A && test $ignore_config = all && continue
>  			fi
>  			# Also show added or modified modules which are checked out
> -			GIT_DIR="$sm_path/.git" git-rev-parse --git-dir >/dev/null 2>&1 &&
> +			GIT_DIR="$sm_path/.git" git rev-parse --git-dir >/dev/null 2>&1 &&
>  			printf '%s\n' "$sm_path"
>  		done
>  	)
> @@ -898,11 +898,11 @@ cmd_summary() {
>  		missing_dst=
>  
>  		test $mod_src = 160000 &&
> -		! GIT_DIR="$name/.git" git-rev-parse -q --verify $sha1_src^0 >/dev/null &&
> +		! GIT_DIR="$name/.git" git rev-parse -q --verify $sha1_src^0 >/dev/null &&
>  		missing_src=t
>  
>  		test $mod_dst = 160000 &&
> -		! GIT_DIR="$name/.git" git-rev-parse -q --verify $sha1_dst^0 >/dev/null &&
> +		! GIT_DIR="$name/.git" git rev-parse -q --verify $sha1_dst^0 >/dev/null &&
>  		missing_dst=t
>  
>  		display_name=$(git submodule--helper relative-path "$name" "$wt_prefix")

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] Drop some dashes from built-in invocations in scripts
  2017-08-05 22:41 ` Junio C Hamano
@ 2017-08-05 22:54   ` Michael Forney
  2017-08-05 23:31     ` Junio C Hamano
  2017-08-05 23:30   ` Johannes Schindelin
  1 sibling, 1 reply; 16+ messages in thread
From: Michael Forney @ 2017-08-05 22:54 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

On 8/5/17, Junio C Hamano <gitster@pobox.com> wrote:
> Have you made sure that all of these scripts, before calling
> 'git-foo' in the current code, update their PATH so that these found
> in the bog standard place (i.e. GIT_EXEC_PATH)?
>
> The reason I ask is because we can rest assured these changes will
> be a no-regression improvement if you did so.  I do not offhand
> think of a reason why these scripts wouldn't be doing so, but it
> never hurts to make sure.

I just checked and all the scripts make some other call to a built-in
with `git foo`, so I think it should be safe.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] Drop some dashes from built-in invocations in scripts
  2017-08-05 22:41 ` Junio C Hamano
  2017-08-05 22:54   ` Michael Forney
@ 2017-08-05 23:30   ` Johannes Schindelin
  1 sibling, 0 replies; 16+ messages in thread
From: Johannes Schindelin @ 2017-08-05 23:30 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Michael Forney, git

Hi Junio,

On Sat, 5 Aug 2017, Junio C Hamano wrote:

> Michael Forney <mforney@mforney.org> writes:
> 
> > This way, they still work even if the built-in symlinks aren't
> > installed.
> >
> > Signed-off-by: Michael Forney <mforney@mforney.org>
> > ---
> > It looks like there was an effort to do this a number of years ago (through
> > `make remove-dashes`). These are just a few I noticed were still left in the
> > .sh scripts.
> 
> Thanks for working on this.  
> 
> Have you made sure that all of these scripts, before calling
> 'git-foo' in the current code, update their PATH so that these found
> in the bog standard place (i.e. GIT_EXEC_PATH)?

The scripts do not update their PATH. `git` does, before they are called.

For example, if you call `git stash`, the `git` executable will first
figure out that `stash` is not a builtin (unless you run with the
experimental patch series that still has not seen much review, more is the
shame, including on myself), and then call the dashed form. At this point,
`git` will already have added the exec path to the PATH variable so that
`git-stash` is found. So then `git-stash` runs, and does not even need to
take pains to adjust the PATH variable again! Magic!

;-)

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] Drop some dashes from built-in invocations in scripts
  2017-08-05 22:54   ` Michael Forney
@ 2017-08-05 23:31     ` Junio C Hamano
  2017-08-06  5:35       ` Michael Forney
  0 siblings, 1 reply; 16+ messages in thread
From: Junio C Hamano @ 2017-08-05 23:31 UTC (permalink / raw)
  To: Michael Forney; +Cc: git

Michael Forney <mforney@mforney.org> writes:

> On 8/5/17, Junio C Hamano <gitster@pobox.com> wrote:
>> Have you made sure that all of these scripts, before calling
>> 'git-foo' in the current code, update their PATH so that these found
>> in the bog standard place (i.e. GIT_EXEC_PATH)?
>>
>> The reason I ask is because we can rest assured these changes will
>> be a no-regression improvement if you did so.  I do not offhand
>> think of a reason why these scripts wouldn't be doing so, but it
>> never hurts to make sure.
>
> I just checked and all the scripts make some other call to a built-in
> with `git foo`, so I think it should be safe.

As long as they are the same "foo"'s, then the check you did is
perfectly fine.  The (unlikely I would think) case that can lead to
a regression is if these script deliberately used `git-foo` to find
them on $PATH, which can be different from 'git foo' that is found
by 'git' in its own binary (as all of them are built-ins), and that
is why I asked.

Thanks.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] Drop some dashes from built-in invocations in scripts
  2017-08-05 23:31     ` Junio C Hamano
@ 2017-08-06  5:35       ` Michael Forney
  2017-08-07  1:31         ` Junio C Hamano
  0 siblings, 1 reply; 16+ messages in thread
From: Michael Forney @ 2017-08-06  5:35 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

On 8/5/17, Junio C Hamano <gitster@pobox.com> wrote:
> Michael Forney <mforney@mforney.org> writes:
>> On 8/5/17, Junio C Hamano <gitster@pobox.com> wrote:
>>> Have you made sure that all of these scripts, before calling
>>> 'git-foo' in the current code, update their PATH so that these found
>>> in the bog standard place (i.e. GIT_EXEC_PATH)?
>>>
>>> The reason I ask is because we can rest assured these changes will
>>> be a no-regression improvement if you did so.  I do not offhand
>>> think of a reason why these scripts wouldn't be doing so, but it
>>> never hurts to make sure.
>>
>> I just checked and all the scripts make some other call to a built-in
>> with `git foo`, so I think it should be safe.
>
> As long as they are the same "foo"'s, then the check you did is
> perfectly fine.  The (unlikely I would think) case that can lead to
> a regression is if these script deliberately used `git-foo` to find
> them on $PATH, which can be different from 'git foo' that is found
> by 'git' in its own binary (as all of them are built-ins), and that
> is why I asked.

Ah. Well, it looks like all but git-merge-resolve.sh run `.
git-sh-setup`, so we know that GIT_EXEC_PATH must in their PATH (and
at the front unless the script was invoked directly).

git-merge-resolve.sh does not do this, so I suppose if the user ran
$GIT_EXEC_PATH/git-merge-resolve directly, and also had a custom
git-merge-index executable in their PATH, that would switch to running
the git merge-index built-in instead.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] Drop some dashes from built-in invocations in scripts
  2017-08-06  5:35       ` Michael Forney
@ 2017-08-07  1:31         ` Junio C Hamano
  0 siblings, 0 replies; 16+ messages in thread
From: Junio C Hamano @ 2017-08-07  1:31 UTC (permalink / raw)
  To: Michael Forney; +Cc: git

Michael Forney <mforney@mforney.org> writes:

> Ah. Well, it looks like all but git-merge-resolve.sh run `.
> git-sh-setup`, so we know that GIT_EXEC_PATH must in their PATH (and
> at the front unless the script was invoked directly).
>
> git-merge-resolve.sh does not do this, so I suppose if the user ran
> $GIT_EXEC_PATH/git-merge-resolve directly, and also had a custom
> git-merge-index executable in their PATH, that would switch to running
> the git merge-index built-in instead.

We are good then.  Nobody other than "git merge" should be running
merge-resolve, so the original probably is OK.

Thanks for digging deeper.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] Drop some dashes from built-in invocations in scripts
  2017-08-05  6:49 [PATCH] Drop some dashes from built-in invocations in scripts Michael Forney
  2017-08-05 22:15 ` Johannes Schindelin
  2017-08-05 22:41 ` Junio C Hamano
@ 2017-08-07 16:35 ` Junio C Hamano
  2017-08-07 17:12   ` Junio C Hamano
                     ` (2 more replies)
  2 siblings, 3 replies; 16+ messages in thread
From: Junio C Hamano @ 2017-08-07 16:35 UTC (permalink / raw)
  To: Michael Forney; +Cc: git

Michael Forney <mforney@mforney.org> writes:

> This way, they still work even if the built-in symlinks aren't
> installed.
>
> Signed-off-by: Michael Forney <mforney@mforney.org>
> ---
> It looks like there was an effort to do this a number of years ago (through
> `make remove-dashes`). These are just a few I noticed were still left in the
> .sh scripts.

Our goal was *not* to have *no* "git-foo" on the filesystem,
though.  It happened in v1.6.0 timeframe and it was about removing
"git-foo" from end-user's $PATH.

Earlier there was a more ambitious proposal to remove all "git-foo"
even from $GIT_EXEC_PATH for built-in commands, but that plan was
scuttled [*1*].

The changes in your patch still are good changes to make sure people
who copy & paste code would see fewer instances of "git-foo", but
"will still work even if I break my installation of Git by removing
them from the filesystem" is not the project's goal.  

IIUC, you will need "$GIT_EXEC_PATH/git-checkout" on the filesystem
if you want your "git co" alias to work, as we spawn built-in as a
dashed external.


[Reference]

*1* https://public-inbox.org/git/alpine.LFD.1.10.0808261114070.3363@nehalem.linux-foundation.org/



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] Drop some dashes from built-in invocations in scripts
  2017-08-07 16:35 ` Junio C Hamano
@ 2017-08-07 17:12   ` Junio C Hamano
  2017-08-07 17:58     ` Michael Forney
  2017-08-07 19:22   ` Johannes Schindelin
  2017-08-07 21:07   ` Johannes Schindelin
  2 siblings, 1 reply; 16+ messages in thread
From: Junio C Hamano @ 2017-08-07 17:12 UTC (permalink / raw)
  To: Michael Forney; +Cc: git

Junio C Hamano <gitster@pobox.com> writes:

> Earlier there was a more ambitious proposal to remove all "git-foo"
> even from $GIT_EXEC_PATH for built-in commands, but that plan was
> scuttled [*1*].
>
> The changes in your patch still are good changes to make sure people
> who copy & paste code would see fewer instances of "git-foo", but
> "will still work even if I break my installation of Git by removing
> them from the filesystem" is not the project's goal.  
>
> IIUC, you will need "$GIT_EXEC_PATH/git-checkout" on the filesystem
> if you want your "git co" alias to work, as we spawn built-in as a
> dashed external.

Just to avoid possible confusion, the above is not to say "once it
is decided, you are not allowed to bring fresh arguments to the
discussion".  As Peff said [*2*] in that old discussion thread, the
circumstances may have changed over 9 years, and it may benefit to
revisit some old decisions.

So in that sense, I do not mind somebody makes a fresh proposal,
which would probably be similar to what I did back then in [*3*],
which is at the beginning of that old thread.  But I do not think I
would be doing so myself, and I suspect that I would not be very
supportive for such a proposal, because my gut feeling is that the
upside is not big enough compared to downsides.

The obvious upside is that you do not have to have many built-in
commands on the filesystem, either as a hardlink, a copy, or a
symbolic link.  But we will be breaking people's scripts by breaking
the 11-year old promise that we will keep their "git-foo" working as
long as they prepend $GIT_EXEC_PATH to their $PATH we we did so.
Another downside is that we now will expose which subcommands are
built-in and which are not, which is unnecessarily implementation
detail we'd want end-users to rely on.

The "'git co' may stop working" I mentioned in my previous message
is not counted as a downside---if the upside is large enough, I
think that "we respawn git-foo as dashed built-in when running an
alias that expands to 'foo'" can be fixed to respawn "git foo"
instead of "git-foo".  But there may be other downside that we may
not be able to fix, and I suspect that "we'd be breaking the 11-year
old promise" is something we would not be able to fix.  That is why
I doubt that I would be advocating the removal of "git-foo" from the
filesystem myself.


[References]

*1* https://public-inbox.org/git/alpine.LFD.1.10.0808261114070.3363@nehalem.linux-foundation.org/

*2* https://public-inbox.org/git/20080826145719.GB5046@coredump.intra.peff.net/

*3* https://public-inbox.org/git/7vprnzt7d5.fsf@gitster.siamese.dyndns.org/


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] Drop some dashes from built-in invocations in scripts
  2017-08-07 17:12   ` Junio C Hamano
@ 2017-08-07 17:58     ` Michael Forney
  2017-08-07 18:18       ` Junio C Hamano
  0 siblings, 1 reply; 16+ messages in thread
From: Michael Forney @ 2017-08-07 17:58 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

On 8/7/17, Junio C Hamano <gitster@pobox.com> wrote:
> Just to avoid possible confusion, the above is not to say "once it
> is decided, you are not allowed to bring fresh arguments to the
> discussion".  As Peff said [*2*] in that old discussion thread, the
> circumstances may have changed over 9 years, and it may benefit to
> revisit some old decisions.
>
> So in that sense, I do not mind somebody makes a fresh proposal,
> which would probably be similar to what I did back then in [*3*],
> which is at the beginning of that old thread.  But I do not think I
> would be doing so myself, and I suspect that I would not be very
> supportive for such a proposal, because my gut feeling is that the
> upside is not big enough compared to downsides.
>
> The obvious upside is that you do not have to have many built-in
> commands on the filesystem, either as a hardlink, a copy, or a
> symbolic link.  But we will be breaking people's scripts by breaking
> the 11-year old promise that we will keep their "git-foo" working as
> long as they prepend $GIT_EXEC_PATH to their $PATH we we did so.
> Another downside is that we now will expose which subcommands are
> built-in and which are not, which is unnecessarily implementation
> detail we'd want end-users to rely on.
>
> The "'git co' may stop working" I mentioned in my previous message
> is not counted as a downside---if the upside is large enough, I
> think that "we respawn git-foo as dashed built-in when running an
> alias that expands to 'foo'" can be fixed to respawn "git foo"
> instead of "git-foo".  But there may be other downside that we may
> not be able to fix, and I suspect that "we'd be breaking the 11-year
> old promise" is something we would not be able to fix.  That is why
> I doubt that I would be advocating the removal of "git-foo" from the
> filesystem myself.

Thanks for the history and explanation, Junio. I agree with your analysis.

I didn't know that git aliases invoke the `git-foo` path for built-ins
(I don't use them much myself, so didn't notice).

I also didn't know that it was supported to add GIT_EXEC_DIR to your
PATH to be able to call `git-foo`. I generally think of /libexec as
implementation-specific executables that a tool may call internally
(in that sense, whether or not the commands are built-ins would remain
an implementation-detail).

However, I still think the patch should be applied for
self-consistency at least (git-submodule.sh currently calls both `git
rev-parse` and `git-rev-parse`). Also, based on Johannes' reply, it
may still be useful for git-for-windows.

>
> [References]
>
> *1*
> https://public-inbox.org/git/alpine.LFD.1.10.0808261114070.3363@nehalem.linux-foundation.org/
>
> *2*
> https://public-inbox.org/git/20080826145719.GB5046@coredump.intra.peff.net/
>
> *3* https://public-inbox.org/git/7vprnzt7d5.fsf@gitster.siamese.dyndns.org/

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] Drop some dashes from built-in invocations in scripts
  2017-08-07 17:58     ` Michael Forney
@ 2017-08-07 18:18       ` Junio C Hamano
  0 siblings, 0 replies; 16+ messages in thread
From: Junio C Hamano @ 2017-08-07 18:18 UTC (permalink / raw)
  To: Michael Forney; +Cc: git

Michael Forney <mforney@mforney.org> writes:

> However, I still think the patch should be applied for
> self-consistency at least (git-submodule.sh currently calls both `git
> rev-parse` and `git-rev-parse`).

Oh, there is no question about the changes in the patch being good,
as I already said.  We want to make sure that people who copy &
paste code would see fewer instances of "git-foo".

I was commenting on the justification in your proposed log message,
which I realized was a bit misleading, after deciding that the patch
text itself is good and I need to apply it.

Thanks for the clean-up.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] Drop some dashes from built-in invocations in scripts
  2017-08-07 16:35 ` Junio C Hamano
  2017-08-07 17:12   ` Junio C Hamano
@ 2017-08-07 19:22   ` Johannes Schindelin
  2017-08-07 19:48     ` Junio C Hamano
  2017-08-07 21:07   ` Johannes Schindelin
  2 siblings, 1 reply; 16+ messages in thread
From: Johannes Schindelin @ 2017-08-07 19:22 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Michael Forney, git

Hi Junio,

On Mon, 7 Aug 2017, Junio C Hamano wrote:

> Michael Forney <mforney@mforney.org> writes:
> 
> > This way, they still work even if the built-in symlinks aren't
> > installed.
> >
> > Signed-off-by: Michael Forney <mforney@mforney.org>
> > ---
> > It looks like there was an effort to do this a number of years ago (through
> > `make remove-dashes`). These are just a few I noticed were still left in the
> > .sh scripts.
> 
> Our goal was *not* to have *no* "git-foo" on the filesystem,
> though. It happened in v1.6.0 timeframe and it was about removing
> "git-foo" from end-user's $PATH.

That was in the v1.6.0 timeframe. It is past time to reconsider the goal,
though, as there is really very little use in keeping the dashed forms.

And it does hurt in some circumstances. Take for example .zip files: they
do not support hard-links. So if you need to distribute Git binaries in
.zip files, you are not only affected negatively by the less-than-stellar
compression ratio compared to .bz2 let alone LZMA, Git adds insult to
injury by *forcing* an additional inflation by pointlessly adding the
builtins *again*.

> Earlier there was a more ambitious proposal to remove all "git-foo"
> even from $GIT_EXEC_PATH for built-in commands, but that plan was
> scuttled [*1*].

That *1* is not a good reference, I am afraid. It says very little in
addition to paraphrased commands to stop responding (when a more civilized
call back to rational arguments might have been a lot more productive).

In that light, would you kindly explain in your own words what is your
current thinking on shipping dashed forms of builtins?

I mean, I can understand for git-upload-pack, to help with determining
permissions on server sides (it is easier to filter out all `git` commands
than to painstakingly look whether argv[1] equals `upload-pack`). It's
sort of a very unfortunate outlier.

But I cannot understand at all why we insist on installing hardlinked
copies (or not so hardlinked copies when hardlinks are unavailable) for
builtins, when these copies really outlived their usefulness a long, long
time ago.

So I would love to hear the arguments for keeping the dashed forms of
builtins, even if the only surviving argument may be "I dig in my feet
because I always said we'd keep them".

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] Drop some dashes from built-in invocations in scripts
  2017-08-07 19:22   ` Johannes Schindelin
@ 2017-08-07 19:48     ` Junio C Hamano
  0 siblings, 0 replies; 16+ messages in thread
From: Junio C Hamano @ 2017-08-07 19:48 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Michael Forney, git

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> So I would love to hear the arguments for keeping the dashed forms of
> builtins, even if the only surviving argument may be "I dig in my feet
> because I always said we'd keep them".

I think I already did ;-)

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] Drop some dashes from built-in invocations in scripts
  2017-08-07 16:35 ` Junio C Hamano
  2017-08-07 17:12   ` Junio C Hamano
  2017-08-07 19:22   ` Johannes Schindelin
@ 2017-08-07 21:07   ` Johannes Schindelin
  2017-08-07 21:47     ` Junio C Hamano
  2 siblings, 1 reply; 16+ messages in thread
From: Johannes Schindelin @ 2017-08-07 21:07 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Michael Forney, git

Hi Junio,

I feel a bit talked to my hand, as the only reply I was graced was a "I
think I already did". So this will be my last reply on this matter for a
while.

On Mon, 7 Aug 2017, Junio C Hamano wrote:

> IIUC, you will need "$GIT_EXEC_PATH/git-checkout" on the filesystem if
> you want your "git co" alias to work, as we spawn built-in as a dashed
> external.

And of course this is just the status quo, not an argument why it should
be so for eternity. Because that would be circular reasoning and prevent
us from improving things.

It is still arguably wrong to call the dashed form for builtins when we
already have enough information at our hands to tell that it is a builtin:

	https://github.com/git-for-windows/git/commit/bad2c6978ec

Granted, this duplicates code a little, as it was developed under time
pressure (and it is necessary to allow the test suite to be run on Git
built in Visual Studio using an installed Git for Windows for the Unix
utilities). As above, though, the current state of this patch does not
prevent any improvement in the future.

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] Drop some dashes from built-in invocations in scripts
  2017-08-07 21:07   ` Johannes Schindelin
@ 2017-08-07 21:47     ` Junio C Hamano
  0 siblings, 0 replies; 16+ messages in thread
From: Junio C Hamano @ 2017-08-07 21:47 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Michael Forney, git

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> I feel a bit talked to my hand, as the only reply I was graced was a "I
> think I already did". So this will be my last reply on this matter for a
> while.

Ah, I meant this thing:

  https://public-inbox.org/git/xmqqo9rrqp3l.fsf@gitster.mtv.corp.google.com

I got an impression that you didn't read it before you typed the
message I gave that response to.

There are a few more exchanged, including Michael's response to that
message on that thread.

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2017-08-07 21:48 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-05  6:49 [PATCH] Drop some dashes from built-in invocations in scripts Michael Forney
2017-08-05 22:15 ` Johannes Schindelin
2017-08-05 22:41 ` Junio C Hamano
2017-08-05 22:54   ` Michael Forney
2017-08-05 23:31     ` Junio C Hamano
2017-08-06  5:35       ` Michael Forney
2017-08-07  1:31         ` Junio C Hamano
2017-08-05 23:30   ` Johannes Schindelin
2017-08-07 16:35 ` Junio C Hamano
2017-08-07 17:12   ` Junio C Hamano
2017-08-07 17:58     ` Michael Forney
2017-08-07 18:18       ` Junio C Hamano
2017-08-07 19:22   ` Johannes Schindelin
2017-08-07 19:48     ` Junio C Hamano
2017-08-07 21:07   ` Johannes Schindelin
2017-08-07 21:47     ` Junio C Hamano

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).