git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Ævar Arnfjörð Bjarmason  <avarab@gmail.com>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
	Daniel Jacques <dnj@google.com>,
	Steffen Prohaska <prohaska@zib.de>,
	John Keeping <john@keeping.me.uk>, Stan Hu <stanhu@gmail.com>,
	Richard Clamp <richardc@unixbeard.net>, Jeff King <peff@peff.net>
Subject: Re: [RFC/PATCH 1/5] Makefile: move long inline shell loops in "install" into helper
Date: Mon, 12 Nov 2018 15:03:54 +0100 (STD)
Message-ID: <nycvar.QRO.7.76.6.1811121449080.39@tvgsbejvaqbjf.bet> (raw)
In-Reply-To: <20181102223743.4331-2-avarab@gmail.com>

[-- Attachment #1: Type: text/plain, Size: 8138 bytes --]

Hi,

On Fri, 2 Nov 2018, Ævar Arnfjörð Bjarmason wrote:

> Move a 37 line for-loop mess out of "install" and into a helper
> script. This started out fairly innocent but over the years has grown
> into a hard-to-maintain monster, and my recent ad874608d8 ("Makefile:
> optionally symlink libexec/git-core binaries to bin/git", 2018-03-13)
> certainly didn't help.
> 
> The shell code is ported pretty much as-is (with getopts added), it'll
> be fixed & prettified in subsequent commits.
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  Makefile         | 52 ++++++++--------------------
>  install_programs | 89 ++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 103 insertions(+), 38 deletions(-)
>  create mode 100755 install_programs
> 
> diff --git a/Makefile b/Makefile
> index bbfbb4292d..aa6ca1fa68 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2808,44 +2808,20 @@ endif
>  	bindir=$$(cd '$(DESTDIR_SQ)$(bindir_SQ)' && pwd) && \
>  	execdir=$$(cd '$(DESTDIR_SQ)$(gitexec_instdir_SQ)' && pwd) && \
>  	destdir_from_execdir_SQ=$$(echo '$(gitexecdir_relative_SQ)' | sed -e 's|[^/][^/]*|..|g') && \
> -	{ test "$$bindir/" = "$$execdir/" || \
> -	  for p in git$X $(filter $(install_bindir_programs),$(ALL_PROGRAMS)); do \
> -		$(RM) "$$execdir/$$p" && \
> -		test -n "$(INSTALL_SYMLINKS)" && \
> -		ln -s "$$destdir_from_execdir_SQ/$(bindir_relative_SQ)/$$p" "$$execdir/$$p" || \
> -		{ test -z "$(NO_INSTALL_HARDLINKS)$(NO_CROSS_DIRECTORY_HARDLINKS)" && \
> -		  ln "$$bindir/$$p" "$$execdir/$$p" 2>/dev/null || \
> -		  cp "$$bindir/$$p" "$$execdir/$$p" || exit; } \
> -	  done; \
> -	} && \
> -	for p in $(filter $(install_bindir_programs),$(BUILT_INS)); do \
> -		$(RM) "$$bindir/$$p" && \
> -		test -n "$(INSTALL_SYMLINKS)" && \
> -		ln -s "git$X" "$$bindir/$$p" || \
> -		{ test -z "$(NO_INSTALL_HARDLINKS)" && \
> -		  ln "$$bindir/git$X" "$$bindir/$$p" 2>/dev/null || \
> -		  ln -s "git$X" "$$bindir/$$p" 2>/dev/null || \
> -		  cp "$$bindir/git$X" "$$bindir/$$p" || exit; } \
> -	done && \
> -	for p in $(BUILT_INS); do \
> -		$(RM) "$$execdir/$$p" && \
> -		test -n "$(INSTALL_SYMLINKS)" && \
> -		ln -s "$$destdir_from_execdir_SQ/$(bindir_relative_SQ)/git$X" "$$execdir/$$p" || \
> -		{ test -z "$(NO_INSTALL_HARDLINKS)" && \
> -		  ln "$$execdir/git$X" "$$execdir/$$p" 2>/dev/null || \
> -		  ln -s "git$X" "$$execdir/$$p" 2>/dev/null || \
> -		  cp "$$execdir/git$X" "$$execdir/$$p" || exit; } \
> -	done && \
> -	remote_curl_aliases="$(REMOTE_CURL_ALIASES)" && \
> -	for p in $$remote_curl_aliases; do \
> -		$(RM) "$$execdir/$$p" && \
> -		test -n "$(INSTALL_SYMLINKS)" && \
> -		ln -s "git-remote-http$X" "$$execdir/$$p" || \
> -		{ test -z "$(NO_INSTALL_HARDLINKS)" && \
> -		  ln "$$execdir/git-remote-http$X" "$$execdir/$$p" 2>/dev/null || \
> -		  ln -s "git-remote-http$X" "$$execdir/$$p" 2>/dev/null || \
> -		  cp "$$execdir/git-remote-http$X" "$$execdir/$$p" || exit; } \
> -	done && \

This indeed looks like a mess...

> +	./install_programs \
> +		--X="$$X" \
> +		--RM="$(RM)" \
> +		--bindir="$$bindir" \
> +		--bindir-relative="$(bindir_relative_SQ)" \
> +		--execdir="$$execdir" \
> +		--destdir-from-execdir="$$destdir_from_execdir_SQ" \
> +		--flag-install-symlinks="$(INSTALL_SYMLINKS)" \
> +		--flag-no-install-hardlinks="$(NO_INSTALL_HARDLINKS)" \
> +		--flag-no-cross-directory-hardlinks="$(NO_CROSS_DIRECTORY_HARDLINKS)" \
> +		--list-bindir-standalone="git$X $(filter $(install_bindir_programs),$(ALL_PROGRAMS))" \
> +		--list-bindir-git-dashed="$(filter $(install_bindir_programs),$(BUILT_INS))" \
> +		--list-execdir-git-dashed="$(BUILT_INS)" \
> +		--list-execdir-curl-aliases="$(REMOTE_CURL_ALIASES)" && \
>  	./check_bindir "z$$bindir" "z$$execdir" "$$bindir/git-add$X"
>  
>  .PHONY: install-gitweb install-doc install-man install-man-perl install-html install-info install-pdf
> diff --git a/install_programs b/install_programs
> new file mode 100755
> index 0000000000..e287108112
> --- /dev/null
> +++ b/install_programs
> @@ -0,0 +1,89 @@
> +#!/bin/sh
> +
> +while test $# != 0
> +do
> +	case "$1" in
> +	--X=*)
> +		X="${1#--X=}"
> +		;;
> +	--RM=*)
> +		RM="${1#--RM=}"
> +		;;
> +	--bindir=*)
> +		bindir="${1#--bindir=}"
> +		;;
> +	--bindir-relative=*)
> +		bindir_relative="${1#--bindir-relative=}"
> +		;;
> +	--execdir=*)
> +		execdir="${1#--execdir=}"
> +		;;
> +	--destdir-from-execdir=*)
> +		destdir_from_execdir="${1#--destdir-from-execdir=}"
> +		;;
> +	--flag-install-symlinks=*)
> +		INSTALL_SYMLINKS="${1#--flag-install-symlinks=}"
> +		;;
> +	--flag-no-install-hardlinks=*)
> +		NO_INSTALL_HARDLINKS="${1#--flag-no-install-hardlinks=}"
> +		;;
> +	--flag-no-cross-directory-hardlinks=*)
> +		NO_CROSS_DIRECTORY_HARDLINKS="${1#--flag-no-cross-directory-hardlinks=}"
> +		;;
> +	--list-bindir-standalone=*)
> +		list_bindir_standalone="${1#--list-bindir-standalone=}"
> +		;;
> +	--list-bindir-git-dashed=*)
> +		list_bindir_git_dashed="${1#--list-bindir-git-dashed=}"
> +		;;
> +	--list-execdir-git-dashed=*)
> +		list_execdir_git_dashed="${1#--list-execdir-git-dashed=}"
> +		;;
> +	--list-execdir-curl-aliases=*)
> +		list_execdir_curl_aliases="${1#--list-execdir-curl-aliases=}"
> +		;;
> +
> +	*)
> +		echo "Unknown option $1"
> +		exit 1
> +		;;
> +	esac
> +	shift
> +done &&
> +{ test "$bindir/" = "$execdir/" ||
> +  for p in $list_bindir_standalone; do
> +	$RM "$execdir/$p" &&
> +	test -n "$INSTALL_SYMLINKS" &&
> +	ln -s "$destdir_from_execdir/$bindir_relative/$p" "$execdir/$p" ||
> +	{ test -z "$NO_INSTALL_HARDLINKS$NO_CROSS_DIRECTORY_HARDLINKS" &&
> +	  ln "$bindir/$p" "$execdir/$p" 2>/dev/null ||
> +	  cp "$bindir/$p" "$execdir/$p" || exit; }
> +  done;
> +} &&
> +for p in $list_bindir_git_dashed; do
> +	$RM "$bindir/$p" &&
> +	test -n "$INSTALL_SYMLINKS" &&
> +	ln -s "git$X" "$bindir/$p" ||
> +	{ test -z "$NO_INSTALL_HARDLINKS" &&
> +	  ln "$bindir/git$X" "$bindir/$p" 2>/dev/null ||
> +	  ln -s "git$X" "$bindir/$p" 2>/dev/null ||
> +	  cp "$bindir/git$X" "$bindir/$p" || exit; }
> +done &&
> +for p in $list_execdir_git_dashed; do
> +	$RM "$execdir/$p" &&
> +	test -n "$INSTALL_SYMLINKS" &&
> +	ln -s "$destdir_from_execdir/$bindir_relative/git$X" "$execdir/$p" ||
> +	{ test -z "$NO_INSTALL_HARDLINKS" &&
> +	  ln "$execdir/git$X" "$execdir/$p" 2>/dev/null ||
> +	  ln -s "git$X" "$execdir/$p" 2>/dev/null ||
> +	  cp "$execdir/git$X" "$execdir/$p" || exit; }
> +done &&
> +for p in $list_execdir_curl_aliases; do
> +	$RM "$execdir/$p" &&
> +	test -n "$INSTALL_SYMLINKS" &&
> +	ln -s "git-remote-http$X" "$execdir/$p" ||
> +	{ test -z "$NO_INSTALL_HARDLINKS" &&
> +	  ln "$execdir/git-remote-http$X" "$execdir/$p" 2>/dev/null ||
> +	  ln -s "git-remote-http$X" "$execdir/$p" 2>/dev/null ||
> +	  cp "$execdir/git-remote-http$X" "$execdir/$p" || exit; }
> +done

... but so does this. I would be very surprised if these four very
similar-looking constructs could not be refactored into a single shell
script that is then called four times with different parameters.

Something like

	#!/bin/sh

	from=
	while case "$1" in
	--no-hardlinks)
		NO_INSTALL_HARDLINKS=t
		;;
	--from=*)
		from="${1#*=}"
		;;
	*)
		break
		;;
	esac; do
		shift
	done

	test $# -gt 3 || {
		echo "Usage: $0 [--no-hardlinks] <from-dir> <to-dir> <file>..." >&2
		exit 1
	}

	fromdir="$1"
	todir="$2"
	shift
	shift

	for p in "$@"
	do
		$RM "$todir/$p" &&
		test -n "$INSTALL_SYMLINKS" &&
		ln -s "$fromdir/${from:-$p}" "$todir/$p" ||
		{ test -z "$NO_INSTALL_HARDLINKS" &&
		  ln "$fromdir/${from:-$p}" "$todir/$p" ||
		  ln -s "$fromdir/${from:-$p}" "$todir/$p" ||
		  cp "$fromdir/${from:-$p}" "$todir/$p" || exit; }
	done

and then calling it using

	test "$bindir/" = "$execdir/" ||
	link-or-copy ${NO_CROSS_DIRECTORY_HARDLINKS:+--no-hardlinks} \
		"$bindir" "$execdir" $list_bindir_standalone
	link-or-copy --from=git$X "$bindir" "$bindir" $list_bindir_git_dashed
	link-or-copy --from=git$X "$bindir" "$execdir" $list_bindir_git_dashed
	link-or-copy --from=git-remote-http$X "$bindir" "$execdir" $list_execdir_curl_aliases

That would at least DRY up this mess a bit.

Ciao,
Dscho

> -- 
> 2.19.1.930.g4563a0d9d0
> 
> 

  parent reply index

Thread overview: 53+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-03-06 23:34 What's cooking in git.git (Mar 2018, #02; Tue, 6) Junio C Hamano
2018-03-07 12:34 ` Johannes Schindelin
2018-03-08  9:22   ` Ævar Arnfjörð Bjarmason
2018-03-08 13:12     ` Daniel Jacques
2018-03-13 12:36       ` Why don't we symlink libexec/git-core/* to bin/git? Ævar Arnfjörð Bjarmason
2018-03-13 18:36         ` Junio C Hamano
2018-03-13 19:32           ` Randall S. Becker
2018-03-13 20:39           ` [PATCH 0/3] Makefile: add a INSTALL_SYMLINKS option Ævar Arnfjörð Bjarmason
2018-03-13 20:39           ` [PATCH 1/3] Makefile: fix broken bindir_relative variable Ævar Arnfjörð Bjarmason
2018-03-13 20:39           ` [PATCH 2/3] Makefile: add a gitexecdir_relative variable Ævar Arnfjörð Bjarmason
2018-03-13 20:39           ` [PATCH 3/3] Makefile: optionally symlink libexec/git-core binaries to bin/git Ævar Arnfjörð Bjarmason
2018-03-14  7:20             ` Johannes Sixt
2018-03-14 10:14               ` Ævar Arnfjörð Bjarmason
2018-03-14 17:21                 ` Linus Torvalds
2018-03-15 17:05                   ` Johannes Schindelin
2018-03-15 17:42                     ` Linus Torvalds
2018-03-16 11:48                       ` Johannes Schindelin
2018-03-16 12:43                         ` Ævar Arnfjörð Bjarmason
2018-03-19 11:34                           ` Johannes Schindelin
2018-03-19 21:21                             ` Linus Torvalds
2018-11-02 22:37                           ` [RFC/PATCH 0/5] stop installing old libexec aliases like "git-init" Ævar Arnfjörð Bjarmason
2018-11-03  1:17                             ` Junio C Hamano
2018-11-05 11:36                               ` Ævar Arnfjörð Bjarmason
2018-11-12 13:33                             ` Johannes Schindelin
2018-11-16 10:38                             ` Ævar Arnfjörð Bjarmason
2018-11-16 16:00                               ` Michael Haggerty
2018-11-16 19:22                                 ` Ævar Arnfjörð Bjarmason
2018-11-17  6:39                                   ` Jeff King
2018-11-22 12:48                                     ` Johannes Schindelin
2018-11-22 16:06                                       ` Jeff King
2018-11-23 11:19                                         ` Johannes Schindelin
2018-11-02 22:37                           ` [RFC/PATCH 1/5] Makefile: move long inline shell loops in "install" into helper Ævar Arnfjörð Bjarmason
2018-11-04  1:09                             ` Eric Sunshine
2018-11-12 14:03                             ` Johannes Schindelin [this message]
2018-11-12 14:42                               ` Ævar Arnfjörð Bjarmason
2018-11-12 16:32                                 ` Johannes Schindelin
2018-11-16 10:32                                   ` Ævar Arnfjörð Bjarmason
2018-11-02 22:37                           ` [RFC/PATCH 2/5] Makefile: conform some of the code to our coding standards Ævar Arnfjörð Bjarmason
2018-11-02 22:37                           ` [RFC/PATCH 3/5] Makefile: stop hiding failures during "install" Ævar Arnfjörð Bjarmason
2018-11-02 22:37                           ` [RFC/PATCH 4/5] Makefile: add NO_INSTALL_SYMLINKS_FALLBACK switch Ævar Arnfjörð Bjarmason
2018-11-04  1:01                             ` Eric Sunshine
2018-11-02 22:37                           ` [RFC/PATCH 5/5] Makefile: Add a NO_INSTALL_BUILTIN_EXECDIR_ALIASES flag Ævar Arnfjörð Bjarmason
2018-11-04  1:04                             ` Eric Sunshine
2018-11-12 14:14                             ` Johannes Schindelin
2018-03-15 17:03               ` [PATCH 3/3] Makefile: optionally symlink libexec/git-core binaries to bin/git Johannes Schindelin
2018-03-14 10:18         ` Why don't we symlink libexec/git-core/* to bin/git? Ævar Arnfjörð Bjarmason
2018-03-14 16:07           ` Junio C Hamano
2018-03-15 17:16             ` Johannes Schindelin
2018-03-16 17:29               ` Duy Nguyen
2018-03-30  8:59                 ` Johannes Schindelin
2018-03-09  6:15 ` What's cooking in git.git (Mar 2018, #02; Tue, 6) Martin Ågren
2018-03-09  9:54   ` Duy Nguyen
2018-03-09 17:19   ` Junio C Hamano

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=nycvar.QRO.7.76.6.1811121449080.39@tvgsbejvaqbjf.bet \
    --to=johannes.schindelin@gmx.de \
    --cc=avarab@gmail.com \
    --cc=dnj@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=john@keeping.me.uk \
    --cc=peff@peff.net \
    --cc=prohaska@zib.de \
    --cc=richardc@unixbeard.net \
    --cc=stanhu@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox