Re: [PATCH v3 1/1] completion: graduate out of contrib

Ævar Arnfjörð Bjarmason wrote:

On Wed, Jul 14 2021, Felipe Contreras wrote:

quoted

--- a/Makefile
+++ b/Makefile

@@ -532,6 +532,7 @@ sharedir = $(prefix)/share
 gitwebdir = $(sharedir)/gitweb
 perllibdir = $(sharedir)/perl5
 localedir = $(sharedir)/locale
+bashcompdir = $(sharedir)/bash-completion/completions
 template_dir = share/git-core/templates
 htmldir = $(prefix)/share/doc/git-doc
 ETC_GITCONFIG = $(sysconfdir)/gitconfig

@@ -2015,6 +2016,7 @@ bindir_relative_SQ = $(subst ','\'',$(bindir_relative))
 mandir_SQ = $(subst ','\'',$(mandir))
 mandir_relative_SQ = $(subst ','\'',$(mandir_relative))
 infodir_relative_SQ = $(subst ','\'',$(infodir_relative))
+sharedir_SQ = $(subst ','\'',$(sharedir))
 perllibdir_SQ = $(subst ','\'',$(perllibdir))
 localedir_SQ = $(subst ','\'',$(localedir))
 localedir_relative_SQ = $(subst ','\'',$(localedir_relative))

@@ -2025,6 +2027,7 @@ htmldir_relative_SQ = $(subst ','\'',$(htmldir_relative))
 prefix_SQ = $(subst ','\'',$(prefix))
 perllibdir_relative_SQ = $(subst ','\'',$(perllibdir_relative))
 gitwebdir_SQ = $(subst ','\'',$(gitwebdir))
+bashcompdir_SQ = $(subst ','\'',$(bashcompdir))
 
 SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
 TEST_SHELL_PATH_SQ = $(subst ','\'',$(TEST_SHELL_PATH))

@@ -3112,6 +3115,13 @@ quick-install-man:
 quick-install-html:
 	$(MAKE) -C Documentation quick-install-html
 
+install-extra: install-completion
+
+install-completion:
+	$(INSTALL) -D -m 644 extra/completion/git-completion.bash '$(DESTDIR_SQ)$(bashcompdir_SQ)'/git
+	$(INSTALL) -D -m 644 extra/completion/git-prompt.sh '$(DESTDIR_SQ)$(sharedir_SQ)'/git-core/git-prompt.sh
+	$(INSTALL) -D -m 644 extra/completion/git-completion.zsh '$(DESTDIR_SQ)$(sharedir_SQ)'/zsh/site-functions/_git
+

These are missing a .PHONY target (like the other install-* targets).

All right.

The bash-completion target corresponds to what I've got in Debian's git
package, but not the prompt:
    
    $ dpkg -L git|grep -e completion -e prompt
    /etc/bash_completion.d
    /etc/bash_completion.d/git-prompt
    /usr/lib/git-core/git-sh-prompt
    /usr/share/bash-completion
    /usr/share/bash-completion/completions
    /usr/share/bash-completion/completions/git
    /usr/share/bash-completion/completions/gitk

I've got no idea what we should pick by default though, maybe what you
have is more standard.

git-prompt.sh is not really part of the completion stuff, so I don't
think the location Debian chose is correct. At some point in time they
were together, so perhaps that why they chose the same location.

Generally scripts and other shared data belongs in /usr/share.

Also why /git and /_git for bash and zsh (looks good) but /git-prompt
instead of /git-prompt.sh?

Probably because it was split from 'git'.

quoted

 ### Maintainer's dist rules

diff --git a/contrib/completion/git-completion.bash b/extra/completion/git-completion.bash
similarity index 100%
rename from contrib/completion/git-completion.bash
rename to extra/completion/git-completion.bash
diff --git a/contrib/completion/git-completion.zsh b/extra/completion/git-completion.zsh
similarity index 100%
rename from contrib/completion/git-completion.zsh
rename to extra/completion/git-completion.zsh
diff --git a/contrib/completion/git-prompt.sh b/extra/completion/git-prompt.sh
similarity index 100%
rename from contrib/completion/git-prompt.sh
rename to extra/completion/git-prompt.sh
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index cb057ef161..32601b755d 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh

@@ -36,7 +36,7 @@ complete ()
 GIT_TESTING_ALL_COMMAND_LIST='add checkout check-attr rebase ls-files'
 GIT_TESTING_PORCELAIN_COMMAND_LIST='add checkout rebase'
 
-. "$GIT_BUILD_DIR/contrib/completion/git-completion.bash"
+. "$GIT_BUILD_DIR/extra/completion/git-completion.bash"
 
 # We don't need this function to actually join words or do anything special.
 # Also, it's cleaner to avoid touching bash's internal completion variables.

@@ -2383,14 +2383,14 @@ test_expect_success 'git clone --config= - value' '
 test_expect_success 'sourcing the completion script clears cached commands' '
 	__git_compute_all_commands &&
 	verbose test -n "$__git_all_commands" &&
-	. "$GIT_BUILD_DIR/contrib/completion/git-completion.bash" &&
+	. "$GIT_BUILD_DIR/extra/completion/git-completion.bash" &&
 	verbose test -z "$__git_all_commands"
 '
 
 test_expect_success 'sourcing the completion script clears cached merge strategies' '
 	__git_compute_merge_strategies &&
 	verbose test -n "$__git_merge_strategies" &&
-	. "$GIT_BUILD_DIR/contrib/completion/git-completion.bash" &&
+	. "$GIT_BUILD_DIR/extra/completion/git-completion.bash" &&
 	verbose test -z "$__git_merge_strategies"
 '
 

@@ -2399,7 +2399,7 @@ test_expect_success 'sourcing the completion script clears cached --options' '
 	verbose test -n "$__gitcomp_builtin_checkout" &&
 	__gitcomp_builtin notes_edit &&
 	verbose test -n "$__gitcomp_builtin_notes_edit" &&
-	. "$GIT_BUILD_DIR/contrib/completion/git-completion.bash" &&
+	. "$GIT_BUILD_DIR/extra/completion/git-completion.bash" &&
 	verbose test -z "$__gitcomp_builtin_checkout" &&
 	verbose test -z "$__gitcomp_builtin_notes_edit"
 '

diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
index bbd513bab0..784e523fd4 100755
--- a/t/t9903-bash-prompt.sh
+++ b/t/t9903-bash-prompt.sh

@@ -10,7 +10,7 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
 . ./lib-bash.sh
 
-. "$GIT_BUILD_DIR/contrib/completion/git-prompt.sh"
+. "$GIT_BUILD_DIR/extra/completion/git-prompt.sh"
 
 actual="$TRASH_DIRECTORY/actual"
 c_red='\\[\\e[31m\\]'

It's more of a "for bonus points", but a nic way to round-trip this
would be to make this work with GIT_TEST_INSTALLED.

I.e. source these relative to GIT_EXEC_PATH, not $GIT_BUILD_DIR, I think
that just sourcing them as e.g.:

    . git-completion.bash

But the GIT_TEST_INSTALLED case is tricker, maybe we'd need to add a
"git --share-path" :(

Yes, we would need --share-path, and perhaps GIT_SHARE_PATH, and so on.

Might make sense for a future patch.

-- 
Felipe Contreras