threads / discuss / 47996

What's cooking in git.git (Mar 2018, #02; Tue, 6)

Subject: What's cooking in git.git (Mar 2018, #02; Tue, 6)

## tl;dr

53 messages between Mar 6, 2018 and Nov 23, 2018.

replies: 52people: 12as markdown or json

Junio C Hamano· Mar 6, 2018, 23:34 UTC · lore

Here are the topics that have been cooking. Commits prefixed with '-' are only in 'pu' (proposed updates) while commits prefixed with '+' are in 'next'. The ones marked with '.' do not appear in any of the integration branches, but I am still holding onto them.

I haven't seen enough reviews (nor I had chance to sufficiently review them myself) to comfortably decide what to do with them on a handful of new topics that appeared in the past week or two, so the ones in the new topics section may not have any description yet in this issue.

You can find the changes described here in the integration branches of the repositories listed at

    http://git-blame.blogspot.com/p/git-public-repositories.html

-------------------------------------------------- [Graduated to "master"]

* ab/fetch-prune (2018-02-09) 17 commits
  (merged to 'next' on 2018-02-27 at eafb648dd9)
 + fetch: make the --prune-tags work with <url>
 + fetch: add a --prune-tags option and fetch.pruneTags config
 + fetch tests: add scaffolding for the new fetch.pruneTags
 + git-fetch & config doc: link to the new PRUNING section
 + git remote doc: correct dangerous lies about what prune does
 + git fetch doc: add a new section to explain the ins & outs of pruning
 + fetch tests: fetch <url> <spec> as well as fetch [<remote>]
 + fetch tests: expand case/esac for later change
 + fetch tests: double quote a variable for interpolation
 + fetch tests: test --prune and refspec interaction
 + fetch tests: add a tag to be deleted to the pruning tests
 + fetch tests: re-arrange arguments for future readability
 + fetch tests: refactor in preparation for testing tag pruning
 + remote: add a macro for "refs/tags/*:refs/tags/*"
 + fetch: stop accessing "remote" variable indirectly
 + fetch: trivially refactor assignment to ref_nr
 + fetch: don't redundantly NULL something calloc() gave us
 "git fetch --prune-tags" may be used as a handy short-hand for
 getting rid of stale tags that are locally held.
* ab/simplify-perl-makefile (2018-02-15) 1 commit
  (merged to 'next' on 2018-02-27 at b0d68a2013)
 + Makefile: generate Git(3pm) as dependency of the 'doc' and 'man' targets
 Hotfix for a topic already in 'master'.
* bw/c-plus-plus (2018-02-22) 37 commits
  (merged to 'next' on 2018-02-27 at daf85c03de)
 + replace: rename 'new' variables
 + trailer: rename 'template' variables
 + tempfile: rename 'template' variables
 + wrapper: rename 'template' variables
 + environment: rename 'namespace' variables
 + diff: rename 'template' variables
 + environment: rename 'template' variables
 + init-db: rename 'template' variables
 + unpack-trees: rename 'new' variables
 + trailer: rename 'new' variables
 + submodule: rename 'new' variables
 + split-index: rename 'new' variables
 + remote: rename 'new' variables
 + ref-filter: rename 'new' variables
 + read-cache: rename 'new' variables
 + line-log: rename 'new' variables
 + imap-send: rename 'new' variables
 + http: rename 'new' variables
 + entry: rename 'new' variables
 + diffcore-delta: rename 'new' variables
 + diff: rename 'new' variables
 + diff-lib: rename 'new' variable
 + commit: rename 'new' variables
 + combine-diff: rename 'new' variables
 + remote: rename 'new' variables
 + reflog: rename 'new' variables
 + pack-redundant: rename 'new' variables
 + help: rename 'new' variables
 + checkout: rename 'new' variables
 + apply: rename 'new' variables
 + apply: rename 'try' variables
 + diff: rename 'this' variables
 + rev-parse: rename 'this' variable
 + pack-objects: rename 'this' variables
 + blame: rename 'this' variables
 + object: rename function 'typename' to 'type_name'
 + object_info: change member name from 'typename' to 'type_name'
 We now avoid using identifiers that clash with C++ keywords.  Even though
 it is not a goal to compile Git with C++ compilers, changes like
 this help use of code analysis tools that targets C++ on our
 codebase.
* bw/doc-submodule-recurse-config-with-clone (2018-02-21) 1 commit
  (merged to 'next' on 2018-02-27 at 5b12841508)
 + submodule: indicate that 'submodule.recurse' doesn't apply to clone
 Doc update.
* bw/perl-timegm-timelocal-fix (2018-02-23) 1 commit
  (merged to 'next' on 2018-02-27 at 565a3141ce)
 + perl: call timegm and timelocal with 4-digit year
 Y2k20 fix ;-) for our perl scripts.
* jc/allow-ff-merging-kept-tags (2018-02-16) 1 commit
  (merged to 'next' on 2018-02-27 at 8b03610d2b)
 + merge: allow fast-forward when merging a tracked tag
 Since Git 1.7.9, "git merge" defaulted to --no-ff (i.e. even when
 the side branch being merged is a descendant of the current commit,
 create a merge commit instead of fast-forwarding) when merging a
 tag object.  This was appropriate default for integrators who pull
 signed tags from their downstream contributors, but caused an
 unnecessary merges when used by downstream contributors who
 habitually "catch up" their topic branches with tagged releases
 from the upstream.  Update "git merge" to default to --no-ff only
 when merging a tag object that does *not* sit at its usual place in
 refs/tags/ hierarchy, and allow fast-forwarding otherwise, to
 mitigate the problem.
* jk/cached-commit-buffer (2018-02-22) 2 commits
  (merged to 'next' on 2018-02-27 at af791d9a1e)
 + revision: drop --show-all option
 + commit: drop uses of get_cached_commit_buffer()
 Code clean-up.
* jk/strbuf-read-file-close-error (2018-02-23) 1 commit
  (merged to 'next' on 2018-02-27 at c5dfe33335)
 + strbuf_read_file(): preserve errno across close() call
 Code clean-up.
* jk/test-helper-v-output-fix (2018-02-22) 1 commit
  (merged to 'next' on 2018-02-27 at c9109977e8)
 + t: send verbose test-helper output to fd 4
 Test framework update.
* ms/non-ascii-ticks (2018-02-22) 1 commit
  (merged to 'next' on 2018-02-27 at 41159fc4f0)
 + Documentation/gitsubmodules.txt: avoid non-ASCII apostrophes
 Doc markup fix.
* nd/rebase-show-current-patch (2018-02-12) 3 commits
  (merged to 'next' on 2018-02-27 at 5a4e23a77c)
 + rebase: introduce and use pseudo-ref REBASE_HEAD
 + rebase: add --show-current-patch
 + am: add --show-current-patch
 The new "--show-current-patch" option gives an end-user facing way
 to get the diff being applied when "git rebase" (and "git am")
 stops with a conflict.
* nm/tag-edit (2018-02-07) 1 commit
  (merged to 'next' on 2018-02-27 at 3bc8345213)
 + tag: add --edit option
 "git tag" learned an explicit "--edit" option that allows the
 message given via "-m" and "-F" to be further edited.
* pw/add-p-single (2018-02-13) 3 commits
  (merged to 'next' on 2018-02-27 at 0e2bd585e3)
 + add -p: improve error messages
 + add -p: only bind search key if there's more than one hunk
 + add -p: only display help for active keys
 "git add -p" used to offer "/" (look for a matching hunk) as a
 choice, even there was only one hunk, which has been corrected.
 Also the single-key help is now given only for keys that are
 enabled (e.g. help for '/' won't be shown when there is only one
 hunk).
* rs/strbuf-read-file-or-whine (2018-02-22) 1 commit
  (merged to 'next' on 2018-02-27 at 56017cb5e2)
 + sequencer: factor out strbuf_read_file_or_whine()
 Code clean-up.
* sb/color-h-cleanup (2018-02-13) 1 commit
  (merged to 'next' on 2018-02-27 at 617345de77)
 + color.h: document and modernize header
 (this branch is used by sb/blame-color.)
 Devdoc update.
* sg/t6300-modernize (2018-02-13) 1 commit
  (merged to 'next' on 2018-02-27 at b6f13b6915)
 + t6300-for-each-ref: fix "more than one quoting style" tests
 Test update.
* sm/mv-dry-run-update (2018-02-07) 2 commits
  (merged to 'next' on 2018-02-27 at 17eef62ddf)
 + mv: remove unneeded 'if (!show_only)'
 + t7001: add test case for --dry-run
 Code clean-up.
* xz/send-email-batch-size (2018-02-12) 1 commit
  (merged to 'next' on 2018-02-27 at da0247d532)
 + send-email: error out when relogin delay is missing
 "git send-email" learned to complain when the batch-size option is
 not defined when the relogin-delay option is, since these two are
 mutually required.

-------------------------------------------------- [New Topics]

* bb/git-gui-ssh-key-files (2018-03-02) 2 commits
 - Merge branch 'bb/ssh-key-files' of git-gui into bb/git-gui-ssh-key-files
 - git-gui: search for all current SSH key types
* bp/git-gui-bind-kp-enter (2018-03-02) 2 commits
 - Merge branch 'bp/bind-kp-enter' of git-gui into bp/git-gui-bind-kp-enter
 - git-gui: bind CTRL/CMD+numpad ENTER to do_commit
* cb/git-gui-ttk-style (2018-03-05) 2 commits
 - Merge branch 'cb/ttk-style' of git-gui into cb/git-gui-ttk-style
 - git-gui: workaround ttk:style theme use
* dp/git-el-ls-files-excludes (2018-03-05) 1 commit
 - git.el: handle default excludesfile properly
* jk/add-i-diff-filter (2018-03-05) 2 commits
 - add--interactive: detect bogus diffFilter output
 - t3701: add a test for interactive.diffFilter
* jk/smart-http-protocol-doc-fix (2018-03-05) 1 commit
 - smart-http: document flush after "# service" line
* nd/object-allocation-comments (2018-03-06) 2 commits
 - object.h: realign object flag allocation comment
 - object.h: update flag allocation comment
* nd/pack-objects-pack-struct (2018-03-05) 9 commits
 - pack-objects: reorder 'hash' to pack struct object_entry
 - pack-objects: refer to delta objects by index instead of pointer
 - pack-objects: move in_pack out of struct object_entry
 - pack-objects: move in_pack_pos out of struct object_entry
 - pack-objects: note about in_pack_header_size
 - pack-objects: use bitfield for object_entry::depth
 - pack-objects: use bitfield for object_entry::dfs_state
 - pack-objects: turn type and in_pack_type to bitfields
 - pack-objects: document holes in struct object_entry.h
* nd/repack-keep-pack (2018-03-06) 5 commits
 - pack-objects: display progress in get_object_details()
 - pack-objects: show some progress when counting kept objects
 - gc --auto: exclude base pack if not enough mem to "repack -ad"
 - repack: add --keep-pack option
 - t7700: have closing quote of a test at the beginning of line
* nd/worktree-prune (2018-03-06) 3 commits
 - worktree prune: improve prune logic when worktree is moved
 - worktree: delete dead code
 - gc.txt: more details about what gc does
* pw/add-p-select (2018-03-06) 3 commits
 - add -p: optimize line selection for short hunks
 - add -p: allow line selection to be inverted
 - add -p: select individual hunk lines
 (this branch uses pw/add-p-recount.)

-------------------------------------------------- [Stalled]

* sb/blame-color (2018-02-13) 3 commits
 - builtin/blame: highlight recently changed lines
 - builtin/blame: add option to color metadata fields separately
 - builtin/blame: dim uninteresting metadata
 Expecting a reroll.
 cf. https://public-inbox.org/git/20171110011002.10179-1-sbeller@google.com/#t
 error messages are funny, can segfault, ...
* av/fsmonitor-updates (2018-01-04) 6 commits
 - fsmonitor: use fsmonitor data in `git diff`
 - fsmonitor: remove debugging lines from t/t7519-status-fsmonitor.sh
 - fsmonitor: make output of test-dump-fsmonitor more concise
 - fsmonitor: update helper tool, now that flags are filled later
 - fsmonitor: stop inline'ing mark_fsmonitor_valid / _invalid
 - dir.c: update comments to match argument name
 Code clean-up on fsmonitor integration, plus optional utilization
 of the fsmonitor data in diff-files.
 Waiting for an update.
 cf. <alpine.DEB.2.21.1.1801042335130.32@MININT-6BKU6QN.europe.corp.microsoft.com>
* pb/bisect-helper-2 (2017-10-28) 8 commits
 - t6030: make various test to pass GETTEXT_POISON tests
 - bisect--helper: `bisect_start` shell function partially in C
 - bisect--helper: `get_terms` & `bisect_terms` shell function in C
 - bisect--helper: `bisect_next_check` shell function in C
 - bisect--helper: `check_and_set_terms` shell function in C
 - wrapper: move is_empty_file() and rename it as is_empty_or_missing_file()
 - bisect--helper: `bisect_write` shell function in C
 - bisect--helper: `bisect_reset` shell function in C
 Expecting a reroll.
 cf. <0102015f5e5ee171-f30f4868-886f-47a1-a4e4-b4936afc545d-000000@eu-west-1.amazonses.com>
* dj/runtime-prefix (2017-12-05) 4 commits
 . exec_cmd: RUNTIME_PREFIX on some POSIX systems
 . Makefile: add Perl runtime prefix support
 . Makefile: add support for "perllibdir"
 . Makefile: generate Perl header from template file
 A build-time option has been added to allow Git to be told to refer
 to its associated files relative to the main binary, in the same
 way that has been possible on Windows for quite some time, for
 Linux, BSDs and Darwin.
 Perhaps it is about time to reboot the effort?
* mk/http-backend-content-length (2017-11-27) 4 commits
 - SQUASH???
 - t5560-http-backend-noserver.sh: add CONTENT_LENGTH cases
 - SQUASH???
 - http-backend: respect CONTENT_LENGTH as specified by rfc3875
 The http-backend (used for smart-http transport) used to slurp the
 whole input until EOF, without paying attention to CONTENT_LENGTH
 that is supplied in the environment and instead expecting the Web
 server to close the input stream.  This has been fixed.
 Expecting a reroll.
 Suggested fixes to be used when rerolling is queued, but I'd
 prefer _not_ squashing them myself.
 Also, it may be too complex solution for the problem.
 cf. <20171204171308.GA13332@sigill.intra.peff.net>
* cc/require-tcl-tk-for-build (2017-11-29) 2 commits
 - travis-ci: avoid new tcl/tk build requirement
 - Makefile: check that tcl/tk is installed
 A first-time builder of Git may have installed neither tclsh nor
 msgfmt, in which case git-gui and gitk part will fail and break the
 build.  As a workaround, refuse to run a build when tclsh is not
 installed and NO_TCLTK is not set.
 Stalled for too long without any response; will discard.
 I still feel that requring tclsh to be installed, with or without
 "escape hatch" for experts, may be too heavy-handed.
* mg/merge-base-fork-point (2017-09-17) 3 commits
 - merge-base: find fork-point outside partial reflog
 - merge-base: return fork-point outside reflog
 - t6010: test actual test output
 "merge-base --fork-point $branch $commit" is used to guess on which
 commit among the commits that were once at the tip of the $branch the
 $commit was built on top of, and it learns these historical tips from
 the reflog of the $branch.  When the true fork-point is lost due to
 pruning of old reflog entries, the command does not give any output,
 because it has no way to guess correctly and does not want to mislead
 the user with a wrong guess.
 The command has been updated to give the best but not known to be
 correct guess, based on a hope that a merge-base between $commit and a
 virtual merge across all the reflog entries that still are available
 for $branch may still be a closer to the true fork-point than the
 merge-base between $commit and the current tip of the $branch.
 This may have to be offered by an additional option, to allow the
 users that are prepared to see a potentially incorrect guess to opt
 into the feature, without affecting the current callers that may not
 be prepared to accept a guess that is not known to be correct.
 Stalled for too long without any response; will discard.
* jk/drop-ancient-curl (2017-08-09) 5 commits
 - http: #error on too-old curl
 - curl: remove ifdef'd code never used with curl >=7.19.4
 - http: drop support for curl < 7.19.4
 - http: drop support for curl < 7.16.0
 - http: drop support for curl < 7.11.1
 Some code in http.c that has bitrot is being removed.
 Expecting a reroll.
* mk/use-size-t-in-zlib (2017-08-10) 1 commit
 . zlib.c: use size_t for size
 The wrapper to call into zlib followed our long tradition to use
 "unsigned long" for sizes of regions in memory, which have been
 updated to use "size_t".
 Needs resurrecting by making sure the fix is good and still applies
 (or adjusted to today's codebase).
* mg/status-in-progress-info (2017-05-10) 2 commits
 - status --short --inprogress: spell it as --in-progress
 - status: show in-progress info for short status
 "git status" learns an option to report various operations
 (e.g. "merging") that the user is in the middle of.
 Stalled for too long without any response; will discard.
 cf. <xmqqmvakcdqw.fsf@gitster.mtv.corp.google.com>

-------------------------------------------------- [Cooking]

* sg/travis-build-during-script-phase (2018-01-08) 1 commit
  (merged to 'next' on 2018-03-02 at 29e1585ae7)
 + travis-ci: build Git during the 'script' phase
 Build the executable in 'script' phase in Travis CI integration, to
 follow the established practice, rather than during 'before_script'
 phase.  This allows the CI categorize the failures better ('failed'
 is project's fault, 'errored' is build environment's).
 Will merge to 'master'.
* np/send-email-header-parsing (2017-12-15) 1 commit
 - send-email: extract email-parsing code into a subroutine
 (this branch is used by cl/send-email-reply-to.)
 Code refactoring.
* ld/p4-unshelve (2018-02-22) 1 commit
 - git-p4: add unshelve command
 "git p4" learned to "unshelve" shelved commit from P4.
 Will hold, perhaps drop and use format-change that uses a proper "diff".
 cf. <CAE5ih7_ooDMqVtTMoQ70s5XCkncr04HY0JkqSp1UmKQeG81oaA@mail.gmail.com>
* ps/contains-id-error-message (2018-03-06) 1 commit
 - parse-options: squelch usage help on certain errors
 "git tag --contains no-such-commit" gave a full list of options
 after giving an error message.
 Rebooted and fixed the root cause of the issue at a lower level.
* rv/grep-cleanup (2018-02-23) 2 commits
  (merged to 'next' on 2018-03-02 at 4aafca15f9)
 + grep: simplify grep_oid and grep_file
 + grep: move grep_source_init outside critical section
 Threaded "git grep" has been optimized to avoid allocation in code
 section that is covered under a mutex.
 Will merge to 'master'.
* sg/subtree-signed-commits (2018-02-23) 1 commit
  (merged to 'next' on 2018-03-02 at c5f6fd33e6)
 + subtree: fix add and pull for GPG-signed commits
 "git subtree" script (in contrib/) scripted around "git log", whose
 output got affected by end-user configuration like log.showsignature
 Will merge to 'master'.
* ds/find-unique-abbrev-optim (2018-02-27) 1 commit
  (merged to 'next' on 2018-03-02 at 0b6d4f9335)
 + sha1_name: fix uninitialized memory errors
 While finding unique object name abbreviation, the code may
 accidentally have read beyond the end of the array of object names
 in a pack.
 Will merge to 'master'.
* ds/mark-parents-uninteresting-optim (2018-02-27) 1 commit
  (merged to 'next' on 2018-03-02 at 5a42c79806)
 + revision.c: reduce object database queries
 Micro optimization in revision traversal code.
 Will merge to 'master'.
* jc/test-must-be-empty (2018-02-27) 1 commit
  (merged to 'next' on 2018-03-02 at ec129f1b97)
 + test_must_be_empty: make sure the file exists, not just empty
 Test framework tweak to catch developer thinko.
 Will merge to 'master'.
* ma/roll-back-lockfiles (2018-02-28) 5 commits
  (merged to 'next' on 2018-03-06 at be29bf891c)
 + sequencer: do not roll back lockfile unnecessarily
 + merge: always roll back lock in `checkout_fast_forward()`
 + merge-recursive: always roll back lock in `merge_recursive_generic()`
 + sequencer: always roll back lock in `do_recursive_merge()`
 + sequencer: make lockfiles non-static
 (this branch is used by ma/skip-writing-unchanged-index.)
 Some codepaths used to take a lockfile and did not roll it back;
 they are automatically rolled back at program exit, so there is no
 real "breakage", but it still is a good practice to roll back when
 you are done with a lockfile.
 Will merge to 'master'.
* mk/doc-pretty-fill (2018-02-27) 1 commit
  (merged to 'next' on 2018-03-02 at 623461b127)
 + docs/pretty-formats: fix typo '% <(<N>)' -> '%<|(<N>)'
 Docfix.
 Will merge to 'master'.
* nd/remove-ignore-env-field (2018-03-05) 5 commits
 - repository: delete ignore_env member
 - sha1_file.c: move delayed getenv(altdb) back to setup_git_env()
 - repository.c: delete dead functions
 - repository.c: move env-related setup code back to environment.c
 - repository: initialize the_repository in main()
 (this branch is used by sb/object-store and sb/packfiles-in-repository.)
* rj/test-i18ngrep (2018-02-28) 2 commits
  (merged to 'next' on 2018-03-06 at 7ea1a2352c)
 + t5536: simplify checking of messages output to stderr
 + t4151: consolidate multiple calls to test_i18ngrep
 Test updates.
 Will merge to 'master'.
* rs/perf-repeat-thrice-by-default (2018-02-27) 1 commit
  (merged to 'next' on 2018-03-02 at 4898b3c450)
 + perf: use GIT_PERF_REPEAT_COUNT=3 by default even without config file
 Perf test regression fix.
 Will merge to 'master'.
* sb/object-store (2018-03-05) 27 commits
 - sha1_file: allow sha1_loose_object_info to handle arbitrary repositories
 - sha1_file: allow map_sha1_file to handle arbitrary repositories
 - sha1_file: allow map_sha1_file_1 to handle arbitrary repositories
 - sha1_file: allow open_sha1_file to handle arbitrary repositories
 - sha1_file: allow stat_sha1_file to handle arbitrary repositories
 - sha1_file: allow sha1_file_name to handle arbitrary repositories
 - sha1_file: add repository argument to sha1_loose_object_info
 - sha1_file: add repository argument to map_sha1_file
 - sha1_file: add repository argument to map_sha1_file_1
 - sha1_file: add repository argument to open_sha1_file
 - sha1_file: add repository argument to stat_sha1_file
 - sha1_file: add repository argument to sha1_file_name
 - sha1_file: allow prepare_alt_odb to handle arbitrary repositories
 - sha1_file: allow link_alt_odb_entries to handle arbitrary repositories
 - sha1_file: add repository argument to prepare_alt_odb
 - sha1_file: add repository argument to link_alt_odb_entries
 - sha1_file: add repository argument to read_info_alternates
 - sha1_file: add repository argument to link_alt_odb_entry
 - sha1_file: add raw_object_store argument to alt_odb_usable
 - pack: move approximate object count to object store
 - pack: move prepare_packed_git_run_once to object store
 - object-store: close all packs upon clearing the object store
 - object-store: move packed_git and packed_git_mru to object store
 - object-store: free alt_odb_list
 - object-store: move alt_odb_list and alt_odb_tail to object store
 - object-store: migrate alternates struct and functions from cache.h
 - repository: introduce raw object store field
 (this branch is used by sb/packfiles-in-repository; uses nd/remove-ignore-env-field.)
 Refactoring the internal global data structure to make it possible
 to open multiple repositories, work with and then close them.
 Rerolled by Duy on top of a separate preliminary clean-up topic.
 The resulting structure of the topics looked very sensible.
* sb/packfiles-in-repository (2018-03-05) 12 commits
 - packfile: keep prepare_packed_git() private
 - packfile: allow find_pack_entry to handle arbitrary repositories
 - packfile: add repository argument to find_pack_entry
 - packfile: allow reprepare_packed_git to handle arbitrary repositories
 - packfile: allow prepare_packed_git to handle arbitrary repositories
 - packfile: allow prepare_packed_git_one to handle arbitrary repositories
 - packfile: add repository argument to reprepare_packed_git
 - packfile: add repository argument to prepare_packed_git
 - packfile: add repository argument to prepare_packed_git_one
 - packfile: allow install_packed_git to handle arbitrary repositories
 - packfile: allow rearrange_packed_git to handle arbitrary repositories
 - packfile: allow prepare_packed_git_mru to handle arbitrary repositories
 (this branch uses nd/remove-ignore-env-field and sb/object-store.)
 Refactoring of the internal global data structure continues.
* sg/test-x (2018-02-28) 11 commits
  (merged to 'next' on 2018-03-06 at ab0684b27c)
 + travis-ci: run tests with '-x' tracing
 + t/README: add a note about don't saving stderr of compound commands
 + t1510-repo-setup: mark as untraceable with '-x'
 + t9903-bash-prompt: don't check the stderr of __git_ps1()
 + t5570-git-daemon: don't check the stderr of a subshell
 + t5526: use $TRASH_DIRECTORY to specify the path of GIT_TRACE log file
 + t5500-fetch-pack: don't check the stderr of a subshell
 + t3030-merge-recursive: don't check the stderr of a subshell
 + t1507-rev-parse-upstream: don't check the stderr of a shell function
 + t: add means to disable '-x' tracing for individual test scripts
 + t: prevent '-x' tracing from interfering with test helpers' stderr
 Running test scripts under -x option of the shell is often not a
 useful way to debug them, because the error messages from the
 commands tests try to capture and inspect are contaminated by the
 tracing output by the shell.  An earlier work done to make it more
 pleasant to run tests under -x with recent versions of bash is
 extended to cover posix shells that do not support BASH_XTRACEFD.
 Will merge to 'master'.
* ab/gc-auto-in-commit (2018-03-01) 1 commit
  (merged to 'next' on 2018-03-02 at 96a5a4d629)
 + commit: run git gc --auto just before the post-commit hook
 "git commit" used to run "gc --auto" near the end, which was lost
 when the command was reimplemented in C by mistake.
 Will merge to 'master'.
* ab/pre-auto-gc-battery (2018-02-28) 1 commit
  (merged to 'next' on 2018-03-06 at ca9cb273cb)
 + hooks/pre-auto-gc-battery: allow gc to run on non-laptops
 A sample auto-gc hook (in contrib/) to skip auto-gc while on
 battery has been updated to almost always allow running auto-gc
 unless on_ac_power command is absolutely sure that we are on
 battery power (earlier, it skipped unless the command is sure that
 we are on ac power).
 Will merge to 'master'.
* ag/userdiff-go-funcname (2018-03-01) 1 commit
  (merged to 'next' on 2018-03-02 at ea404d1be9)
 + userdiff: add built-in pattern for golang
 "git diff" and friends learned funcname patterns for Go language
 source files.
 Will merge to 'master'.
* ma/skip-writing-unchanged-index (2018-03-01) 1 commit
 - write_locked_index(): add flag to avoid writing unchanged index
 (this branch uses ma/roll-back-lockfiles.)
 Internal API clean-up to allow write_locked_index() optionally skip
 writing the in-core index when it is not modified.
 May want to merge into ma/roll-back-lockfiles topic before merging
 to 'next'.
* jh/status-no-ahead-behind (2018-01-24) 4 commits
  (merged to 'next' on 2018-03-02 at 68bde8d571)
 + status: support --no-ahead-behind in long format
 + status: update short status to respect --no-ahead-behind
 + status: add --[no-]ahead-behind to status and commit for V2 format.
 + stat_tracking_info: return +1 when branches not equal
 "git status" can spend a lot of cycles to compute the relation
 between the current branch and its upstream, which can now be
 disabled with "--no-ahead-behind" option.
 Will merge to 'master'.
* nd/tilde-expand-opt-file-value (2018-02-14) 2 commits
 - init-db: change --template type to OPTION_FILENAME
 - parse-options: expand $HOME on filename options
 "git cmd --opt=~u/path/to/file" did not tilde-expand "~u" part to
 the path to the home directory of user 'u'
 Will discard.
 This may make the resulting whole more confusing, though.
 cf. <87wozffavp.fsf@evledraar.gmail.com>
* ab/perl-fixes (2018-03-05) 13 commits
 - perl Git::LoadCPAN: emit better errors under NO_PERL_CPAN_FALLBACKS
 - Makefile: add NO_PERL_CPAN_FALLBACKS knob
 - perl: move the perl/Git/FromCPAN tree to perl/FromCPAN
 - perl: generalize the Git::LoadCPAN facility
 - perl: move CPAN loader wrappers to another namespace
 - perl: update our copy of Mail::Address
 - perl: update our ancient copy of Error.pm
 - git-send-email: unconditionally use Net::{SMTP,Domain}
 - Git.pm: hard-depend on the File::{Temp,Spec} modules
 - gitweb: hard-depend on the Digest::MD5 5.8 module
 - Git.pm: add the "use warnings" pragma
 - Git.pm: remove redundant "use strict" from sub-package
 - perl: *.pm files should not have the executable bit
 Clean-up to various pieces of Perl code we have.
 WIll merge to 'next'.
* ds/commit-graph (2018-02-20) 13 commits
 - commit-graph: build graph from starting commits
 - commit-graph: read only from specific pack-indexes
 - commit: integrate commit graph with commit parsing
 - commit-graph: close under reachability
 - commit-graph: add core.commitGraph setting
 - commit-graph: implement --delete-expired
 - commit-graph: implement --set-latest
 - commit-graph: implement git commit-graph read
 - commit-graph: implement 'git-commit-graph write'
 - commit-graph: implement write_commit_graph()
 - commit-graph: create git-commit-graph builtin
 - graph: add commit graph design document
 - commit-graph: add format document
 Precompute and store information necessary for ancestry traversal
 in a separate file to optimize graph walking.
 Reroll exists, but it appears that there will be a further reroll.
 cf. <1519698787-190494-1-git-send-email-dstolee@microsoft.com>
* ot/ref-filter-cleanup (2018-02-21) 2 commits
  (merged to 'next' on 2018-03-02 at 3b4c39a4b5)
 + ref-filter: get rid of goto
 + ref-filter: get rid of duplicate code
 Code cleanup.
 Will merge to 'master'.
* ma/config-page-only-in-list-mode (2018-02-21) 3 commits
 - config: change default of `pager.config` to "on"
 - config: respect `pager.config` in list/get-mode only
 - t7006: add tests for how git config paginates
 In a way similar to how "git tag" learned to honor the pager
 setting only in the list mode, "git config" learned to ignore the
 pager setting when it is used for setting values (i.e. when the
 purpose of the operation is not to "show").
 Is this ready for 'next'?
* pw/add-p-recount (2018-03-05) 9 commits
  (merged to 'next' on 2018-03-06 at 68952f9bb0)
 + add -p: don't rely on apply's '--recount' option
 + add -p: fix counting when splitting and coalescing
 + add -p: calculate offset delta for edited patches
 + add -p: adjust offsets of subsequent hunks when one is skipped
 + t3701: add failing test for pathological context lines
 + t3701: don't hard code sha1 hash values
 + t3701: use test_write_lines and write_script
 + t3701: indent here documents
 + add -i: add function to format hunk header
 (this branch is used by pw/add-p-select.)
 "git add -p" has been lazy in coalescing split patches before
 passing the result to underlying "git apply", leading to corner
 case bugs; the logic to prepare the patch to be applied after hunk
 selections has been tightened.
 Will merge to 'master'.
* bp/untracked-cache-noflush (2018-02-28) 2 commits
  (merged to 'next' on 2018-03-02 at 709887971b)
 + untracked cache: use git_env_bool() not getenv() for customization
 + dir.c: don't flag the index as dirty for changes to the untracked cache
 Writing out the index file when the only thing that changed in it
 is the untracked cache information is often wasteful, and this has
 been optimized out.
 Will merge to 'master'.
* nd/diff-stat-with-summary (2018-02-27) 2 commits
  (merged to 'next' on 2018-03-06 at d543f92f5e)
 + diff: add --compact-summary
 + diff.c: refactor pprint_rename() to use strbuf
 "git diff" and friends learned "--compact-summary" that shows the
 information usually given with the "--summary" option on the same
 line as the diffstat output of the "--stat" option (which saves
 vertical space and keeps info on a single path at the same place).
 Will merge to 'master'.
* nd/parseopt-completion (2018-03-06) 44 commits
 - SQUASH???
 - completion: simplify _git_notes
 - completion: don't set PARSE_OPT_NOCOMPLETE on --rerere-autoupdate
  (merged to 'next' on 2018-03-02 at d72a6525fd)
 + completion: use __gitcomp_builtin in _git_worktree
 + completion: use __gitcomp_builtin in _git_tag
 + completion: use __gitcomp_builtin in _git_status
 + completion: use __gitcomp_builtin in _git_show_branch
 + completion: use __gitcomp_builtin in _git_rm
 + completion: use __gitcomp_builtin in _git_revert
 + completion: use __gitcomp_builtin in _git_reset
 + completion: use __gitcomp_builtin in _git_replace
 + remote: force completing --mirror= instead of --mirror
 + completion: use __gitcomp_builtin in _git_remote
 + completion: use __gitcomp_builtin in _git_push
 + completion: use __gitcomp_builtin in _git_pull
 + completion: use __gitcomp_builtin in _git_notes
 + completion: use __gitcomp_builtin in _git_name_rev
 + completion: use __gitcomp_builtin in _git_mv
 + completion: use __gitcomp_builtin in _git_merge_base
 + completion: use __gitcomp_builtin in _git_merge
 + completion: use __gitcomp_builtin in _git_ls_remote
 + completion: use __gitcomp_builtin in _git_ls_files
 + completion: use __gitcomp_builtin in _git_init
 + completion: use __gitcomp_builtin in _git_help
 + completion: use __gitcomp_builtin in _git_grep
 + completion: use __gitcomp_builtin in _git_gc
 + completion: use __gitcomp_builtin in _git_fsck
 + completion: use __gitcomp_builtin in _git_fetch
 + completion: use __gitcomp_builtin in _git_difftool
 + completion: use __gitcomp_builtin in _git_describe
 + completion: use __gitcomp_builtin in _git_config
 + completion: use __gitcomp_builtin in _git_commit
 + completion: use __gitcomp_builtin in _git_clone
 + completion: use __gitcomp_builtin in _git_clean
 + completion: use __gitcomp_builtin in _git_cherry_pick
 + completion: use __gitcomp_builtin in _git_checkout
 + completion: use __gitcomp_builtin in _git_branch
 + completion: use __gitcomp_builtin in _git_apply
 + completion: use __gitcomp_builtin in _git_am
 + completion: use __gitcomp_builtin in _git_add
 + git-completion.bash: introduce __gitcomp_builtin
 + parse-options: let OPT__FORCE take optional flags argument
 + parse-options: add OPT_xxx_F() variants
 + parse-options: support --git-completion-helper
 Teach parse-options API an option to help the completion script,
 and make use of the mechanism in command line completion.
 Will merge to 'master'.
* pc/submodule-helper-foreach (2018-02-02) 5 commits
 - submodule: port submodule subcommand 'foreach' from shell to C
 - submodule foreach: document variable '$displaypath'
 - submodule foreach: clarify the '$toplevel' variable documentation
 - submodule foreach: document '$sm_path' instead of '$path'
 - submodule foreach: correct '$path' in nested submodules from a subdirectory
 Expecting a response to review comments
 e.g. cf. <20180206150044.1bffbb573c088d38c8e44bf5@google.com>
* tg/worktree-add-existing-branch (2018-02-05) 3 commits
 - worktree: teach "add" to check out existing branches
 - worktree: be clearer when "add" dwim-ery kicks in
 - worktree: improve message when creating a new worktree
 "git worktree add" learned to check out an existing branch.
 Expecting a reroll.
 cf. <CAPig+cRLohiqR_Drh7P0q3XbvC22WLjNwH0YLZo3dqFzZZuAPw@mail.gmail.com>
 cf. <CACsJy8BEKYqW+Ne_WY2RBaSbb9OKcjREtrawStj=eJsVsia_Jw@mail.gmail.com>
 The general idea is good, just end-user facing messages are found
 suboptimal.
* nd/worktree-move (2018-03-06) 8 commits
  (merged to 'next' on 2018-03-06 at a26271e7de)
 + t2028: fix minor error and issues in newly-added "worktree move" tests
  (merged to 'next' on 2018-03-02 at 5c514dfc92)
 + worktree remove: allow it when $GIT_WORK_TREE is already gone
 + worktree remove: new command
 + worktree move: refuse to move worktrees with submodules
 + worktree move: accept destination as directory
 + worktree move: new command
 + worktree.c: add update_worktree_location()
 + worktree.c: add validate_worktree()
 "git worktree" learned move and remove subcommands.
 Will merge to 'master'.
* cl/send-email-reply-to (2018-03-06) 2 commits
 - send-email: support separate Reply-To address
 - send-email: rename variable for clarity
 (this branch uses np/send-email-header-parsing.)
 "git send-email" learned "--reply-to=<address>" option.
 Will merge to 'next'.
* js/rebase-recreate-merge (2018-02-23) 12 commits
 - rebase -i: introduce --recreate-merges=[no-]rebase-cousins
 - pull: accept --rebase=recreate to recreate the branch topology
 - sequencer: handle post-rewrite for merge commands
 - sequencer: make refs generated by the `label` command worktree-local
 - rebase: introduce the --recreate-merges option
 - rebase-helper --make-script: introduce a flag to recreate merges
 - sequencer: fast-forward merge commits, if possible
 - sequencer: introduce the `merge` command
 - sequencer: introduce new commands to reset the revision
 - git-rebase--interactive: clarify arguments
 - sequencer: make rearrange_squash() a bit more obvious
 - sequencer: avoid using errno clobbered by rollback_lock_file()
 "git rebase" learned "--recreate-merges" to transplant the whole
 topology of commit graph elsewhere.
 Will merge to 'next'.
* bw/protocol-v2 (2018-03-02) 36 commits
 - SQUASH???
 - remote-curl: don't request v2 when pushing
 - remote-curl: implement stateless-connect command
 - http: eliminate "# service" line when using protocol v2
 - http: don't always add Git-Protocol header
 - http: allow providing extra headers for http requests
 - remote-curl: store the protocol version the server responded with
 - remote-curl: create copy of the service name
 - pkt-line: add packet_buf_write_len function
 - transport-helper: introduce stateless-connect
 - transport-helper: refactor process_connect_service
 - transport-helper: remove name parameter
 - connect: don't request v2 when pushing
 - connect: refactor git_connect to only get the protocol version once
 - fetch-pack: support shallow requests
 - fetch-pack: perform a fetch using v2
 - upload-pack: introduce fetch server command
 - push: pass ref patterns when pushing
 - fetch: pass ref patterns when fetching
 - ls-remote: pass ref patterns when requesting a remote's refs
 - transport: convert transport_get_remote_refs to take a list of ref patterns
 - transport: convert get_refs_list to take a list of ref patterns
 - connect: request remote refs using v2
 - ls-refs: introduce ls-refs server command
 - serve: introduce git-serve
 - test-pkt-line: introduce a packet-line test helper
 - protocol: introduce enum protocol_version value protocol_v2
 - transport: store protocol version
 - connect: discover protocol version outside of get_remote_heads
 - connect: convert get_remote_heads to use struct packet_reader
 - transport: use get_refs_via_connect to get refs
 - upload-pack: factor out processing lines
 - upload-pack: convert to a builtin
 - pkt-line: add delim packet support
 - pkt-line: allow peeking a packet line without consuming it
 - pkt-line: introduce packet_read_with_status
 The beginning of the next-gen transfer protocol.
* ls/checkout-encoding (2018-03-06) 8 commits
 - convert: add round trip check based on 'core.checkRoundtripEncoding'
 - convert: add tracing for 'working-tree-encoding' attribute
 - convert: check for detectable errors in UTF encodings
 - convert: add 'working-tree-encoding' attribute
 - utf8: add function to detect a missing UTF-16/32 BOM
 - utf8: add function to detect prohibited UTF-16/32 BOM
 - strbuf: add xstrdup_toupper()
 - strbuf: remove unnecessary NUL assignment in xstrdup_tolower()
 The new "checkout-encoding" attribute can ask Git to convert the
 contents to the specified encoding when checking out to the working
 tree (and the other way around when checking in).
 Expecting a reroll; it is almost there, though.
 cf. <570D707A-DD9E-4397-8155-E8B3C3D09760@gmail.com>
* en/rename-directory-detection (2018-02-27) 29 commits
  (merged to 'next' on 2018-03-06 at d42470f86e)
 + merge-recursive: ensure we write updates for directory-renamed file
 + merge-recursive: avoid spurious rename/rename conflict from dir renames
 + directory rename detection: new testcases showcasing a pair of bugs
 + merge-recursive: fix remaining directory rename + dirty overwrite cases
 + merge-recursive: fix overwriting dirty files involved in renames
 + merge-recursive: avoid clobbering untracked files with directory renames
 + merge-recursive: apply necessary modifications for directory renames
 + merge-recursive: when comparing files, don't include trees
 + merge-recursive: check for file level conflicts then get new name
 + merge-recursive: add computation of collisions due to dir rename & merging
 + merge-recursive: check for directory level conflicts
 + merge-recursive: add get_directory_renames()
 + merge-recursive: make a helper function for cleanup for handle_renames
 + merge-recursive: split out code for determining diff_filepairs
 + merge-recursive: make !o->detect_rename codepath more obvious
 + merge-recursive: fix leaks of allocated renames and diff_filepairs
 + merge-recursive: introduce new functions to handle rename logic
 + merge-recursive: move the get_renames() function
 + directory rename detection: tests for handling overwriting dirty files
 + directory rename detection: tests for handling overwriting untracked files
 + directory rename detection: miscellaneous testcases to complete coverage
 + directory rename detection: testcases exploring possibly suboptimal merges
 + directory rename detection: more involved edge/corner testcases
 + directory rename detection: testcases checking which side did the rename
 + directory rename detection: files/directories in the way of some renames
 + directory rename detection: partially renamed directory testcase/discussion
 + directory rename detection: testcases to avoid taking detection too far
 + directory rename detection: directory splitting testcases
 + directory rename detection: basic testcases
 Rename detection logic in "diff" family that is used in "merge" has
 learned to guess when all of x/a, x/b and x/c have moved to z/a,
 z/b and z/c, it is likely that x/d added in the meantime would also
 want to move to z/d by taking the hint that the entire directory
 'x' moved to 'z'.  A bug causing dirty files involved in a rename
 to be overwritten during merge has also been fixed as part of this
 work.
 Will merge to 'master'.

-------------------------------------------------- [Discarded]

* ot/cat-batch-format (2018-02-12) 23 commits
 . cat-file: update of docs
 . cat-file: tests for new atoms added
 . for-each-ref: tests for new atoms added
 . ref-filter: unifying formatting of cat-file opts
 . ref-filter: make populate_value() internal again
 . cat-file: reuse printing logic from ref-filter
 . ref-filter: make valid_atom general again
 . ref-filter: make cat_file_info independent
 . cat-file: move skip_object_info into ref-filter
 . ref_filter: add is_atom_used function
 . ref-filter: get rid of mark_atom_in_object_info()
 . cat-file: start reusing populate_value()
 . ref-filter: rename field in ref_array_item stuct
 . ref-filter: make populate_value() global
 . cat-file: start use ref_array_item struct
 . ref-filter: reuse parse_ref_filter_atom()
 . cat-file: start migrating formatting to ref-filter
 . cat-file: split expand_atom() into 2 functions
 . cat-file: move struct expand_data into ref-filter
 . ref-filter: make valid_atom as function parameter
 . cat-file: reuse struct ref_format
 . ref-filter: add return value to some functions
 . ref-filter: get rid of goto
 Teach "cat-file --batch" to reuse the formatting machinery shared
 by for-each-ref, branch --list, and tag --list.
 Discarded, as a rebooted effort is beginning elsewhere.
 Allocates flex-array on stack, etc.
 cf. <58b2bdcd-d621-fd21-ab4d-6a9478319b19@ramsayjones.plus.com>
Johannes Schindelin· Mar 7, 2018, 12:34 UTC · re: Junio C Hamano · lore

Re: What's cooking in git.git (Mar 2018, #02; Tue, 6)

Hi Dan,
On Tue, 6 Mar 2018, Junio C Hamano wrote:
Show 12 quoted lines
> * dj/runtime-prefix (2017-12-05) 4 commits
>  . exec_cmd: RUNTIME_PREFIX on some POSIX systems
>  . Makefile: add Perl runtime prefix support
>  . Makefile: add support for "perllibdir"
>  . Makefile: generate Perl header from template file
> 
>  A build-time option has been added to allow Git to be told to refer
>  to its associated files relative to the main binary, in the same
>  way that has been possible on Windows for quite some time, for
>  Linux, BSDs and Darwin.
> 
>  Perhaps it is about time to reboot the effort?
You probably missed this in the huge "What's cooking" mail. Are you game?

Ciao, Johannes

Ævar Arnfjörð Bjarmason· Mar 8, 2018, 09:22 UTC · re: Johannes Schindelin · lore

Re: What's cooking in git.git (Mar 2018, #02; Tue, 6)

On Wed, Mar 07 2018, Johannes Schindelin jotted:
Show 18 quoted lines
> Hi Dan,
>
> On Tue, 6 Mar 2018, Junio C Hamano wrote:
>
>> * dj/runtime-prefix (2017-12-05) 4 commits
>>  . exec_cmd: RUNTIME_PREFIX on some POSIX systems
>>  . Makefile: add Perl runtime prefix support
>>  . Makefile: add support for "perllibdir"
>>  . Makefile: generate Perl header from template file
>>
>>  A build-time option has been added to allow Git to be told to refer
>>  to its associated files relative to the main binary, in the same
>>  way that has been possible on Windows for quite some time, for
>>  Linux, BSDs and Darwin.
>>
>>  Perhaps it is about time to reboot the effort?
>
> You probably missed this in the huge "What's cooking" mail. Are you game?

It would be great to have this rebooted now that my perl cleanup efforts have un-blocked this. Will be happy to help review & test the next iteration.

Daniel Jacques· Mar 8, 2018, 13:12 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: What's cooking in git.git (Mar 2018, #02; Tue, 6)

> It would be great to have this rebooted now that my perl cleanup efforts
> have un-blocked this. Will be happy to help review & test the next
> iteration.

Yes, I was just thinking the same thing. I wanted to make sure the Perl changes had landed, and I'm pleased to see that they have. I should have time in the next few days to rebase and put up a new version of the patch series. I'll keep you in the loop, and thanks for pinging!

Ævar Arnfjörð Bjarmason· Mar 13, 2018, 12:36 UTC · re: Daniel Jacques · lore

Why don't we symlink libexec/git-core/* to bin/git?

On Thu, Mar 08 2018, Daniel Jacques jotted:
Show 8 quoted lines
>> It would be great to have this rebooted now that my perl cleanup efforts
>> have un-blocked this. Will be happy to help review & test the next
>> iteration.
>
> Yes, I was just thinking the same thing. I wanted to make sure the Perl
> changes had landed, and I'm pleased to see that they have. I should have
> time in the next few days to rebase and put up a new version of the patch
> series. I'll keep you in the loop, and thanks for pinging!

Related to this, I came across this bug report https://gitlab.com/gitlab-org/omnibus-gitlab/issues/3265 which is wondering why we're installing N copies of the git binary, presumably they're building with NO_INSTALL_HARDLINKS.

Just doing this:
    diff --git a/Makefile b/Makefile
    index de4b8f0c02..2222319a4f 100644
    --- a/Makefile
    +++ b/Makefile
    @@ -2596,7 +2596,7 @@ endif
              for p in git$X $(filter $(install_bindir_programs),$(ALL_PROGRAMS)); do \
                    $(RM) "$$execdir/$$p" && \
                    test -z "$(NO_INSTALL_HARDLINKS)$(NO_CROSS_DIRECTORY_HARDLINKS)" && \
    -               ln "$$bindir/$$p" "$$execdir/$$p" 2>/dev/null || \
    +               ln -s "$$bindir/$$p" "$$execdir/$$p" 2>/dev/null || \
                    cp "$$bindir/$$p" "$$execdir/$$p" || exit; \
              done; \
            } && \

Seems to work for me, although obviously this would need to be optional, and it'll get in the way of Daniel's patch since it use the absolute path.

But is there any reason anyone can think of for why we shouldn't be figuring out the relative path and symlinking the two?

Junio C Hamano· Mar 13, 2018, 18:36 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: Why don't we symlink libexec/git-core/* to bin/git?

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 7 quoted lines
> Related to this, I came across this bug report
> https://gitlab.com/gitlab-org/omnibus-gitlab/issues/3265 which is
> wondering why we're installing N copies of the git binary, presumably
> they're building with NO_INSTALL_HARDLINKS.
> ...
> But is there any reason anyone can think of for why we shouldn't be
> figuring out the relative path and symlinking the two?

There is no fundamental reason not to offer such an "install" method as an option; unless you count a more philosophical aversion to use symlinks due to (perceived) additional fragility, that is.

The resulting code may become messier than without, but as long as it is without the reasonable range for usual price we would pay for a new "feature", that would be tolerable, I guess.

Randall S. Becker· Mar 13, 2018, 19:32 UTC · re: Junio C Hamano · lore

RE: Why don't we symlink libexec/git-core/* to bin/git?

On March 13, 2018 2:37 PM, Junio C Hamano wrote:
Show 18 quoted lines
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> 
> > Related to this, I came across this bug report
> > https://gitlab.com/gitlab-org/omnibus-gitlab/issues/3265 which is
> > wondering why we're installing N copies of the git binary, presumably
> > they're building with NO_INSTALL_HARDLINKS.
> > ...
> > But is there any reason anyone can think of for why we shouldn't be
> > figuring out the relative path and symlinking the two?
> 
> 
> There is no fundamental reason not to offer such an "install" method as an
> option; unless you count a more philosophical aversion to use symlinks due
> to (perceived) additional fragility, that is.
> 
> The resulting code may become messier than without, but as long as it is
> without the reasonable range for usual price we would pay for a new
> "feature", that would be tolerable, I guess.
A possible (remote) reason for not doing this is in environments using ACLs that somehow want different access permissions on some functions vs. others AND where the platform does not have the ability to separately secure links vs. objects. I don't know of such an environment, but you never know. I know it's a stretch, but I can see security-types being worried about this. I do know of environments where /usr/local/lib is secured differently from /usr/local/bin to prevent inappropriate .so loads on a selective basis, so there's that. Again, this is a stretch. As long as we continue to have a method of forcing the expensive way for the paranoidly inclined ;)    -- not meaning offence to those, of course.

Cheers, Randall

-- Brief whoami:
 NonStop developer since approximately 211288444200000000
 UNIX developer since approximately 421664400
-- In my real life, I talk too much.
Ævar Arnfjörð Bjarmason· Mar 13, 2018, 20:39 UTC · re: Junio C Hamano · lore

[PATCH 0/3] Makefile: add a INSTALL_SYMLINKS option

On Tue, Mar 13 2018, Junio C. Hamano jotted:
Show 18 quoted lines
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> Related to this, I came across this bug report
>> https://gitlab.com/gitlab-org/omnibus-gitlab/issues/3265 which is
>> wondering why we're installing N copies of the git binary, presumably
>> they're building with NO_INSTALL_HARDLINKS.
>> ...
>> But is there any reason anyone can think of for why we shouldn't be
>> figuring out the relative path and symlinking the two?
>
>
> There is no fundamental reason not to offer such an "install" method
> as an option; unless you count a more philosophical aversion to use
> symlinks due to (perceived) additional fragility, that is.
>
> The resulting code may become messier than without, but as long as
> it is without the reasonable range for usual price we would pay for
> a new "feature", that would be tolerable, I guess.

Cool. I think it makes sense for us to have this. Here's an implementation of it. The 3/3 patch looks a bit scary, but "git show" with --word-diff will show that the change is minimal.

This steals a small piece from Daniel's relocatable series, and doesn't in any way conflict with it. None of this will need to be fixed up to make git relocatable since all the symlinks are relative already.

Ævar Arnfjörð Bjarmason (3):
  Makefile: fix broken bindir_relative variable
  Makefile: add a gitexecdir_relative variable
  Makefile: optionally symlink libexec/git-core binaries to bin/git
 Makefile | 52 +++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 35 insertions(+), 17 deletions(-)
-- 
2.15.1.424.g9478a66081
Ævar Arnfjörð Bjarmason· Mar 13, 2018, 20:39 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH 1/3] Makefile: fix broken bindir_relative variable

Change the bindir_relative variable to work like the other *_relative variables, which are computed as a function of the absolute path. Before this change, supplying e.g. bindir=/tmp/git/binaries to the Makefile would yield a bindir_relative of just "bin", as opposed to "binaries".

This logic was originally added back in 026fa0d5ad ("Move computation of absolute paths from Makefile to runtime (in preparation for RUNTIME_PREFIX)", 2009-01-18), then later in 971f85388f ("Makefile: make mandir, htmldir and infodir absolute", 2013-02-24) when more *_relative variables were added those new variables didn't have this bug, but bindir_relative was never fixed.

There is a small change in behavior here, which is that setting bindir_relative as an argument to the Makefile won't work anymore, I think that's fine, since this was always intended as an internal variable (e.g. INSTALL documents bindir=*, not bindir_relative=*).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/Makefile b/Makefile
index de4b8f0c02..b2f8f2b171 100644
--- a/Makefile
+++ b/Makefile
@@ -468,8 +468,7 @@ ARFLAGS = rcs
 # This can help installing the suite in a relocatable way.
 
 prefix = $(HOME)
-bindir_relative = bin
-bindir = $(prefix)/$(bindir_relative)
+bindir = $(prefix)/bin
 mandir = $(prefix)/share/man
 infodir = $(prefix)/share/info
 gitexecdir = libexec/git-core
@@ -486,6 +485,7 @@ lib = lib
 # DESTDIR =
 pathsep = :
 
+bindir_relative = $(patsubst $(prefix)/%,%,$(bindir))
 mandir_relative = $(patsubst $(prefix)/%,%,$(mandir))
 infodir_relative = $(patsubst $(prefix)/%,%,$(infodir))
 htmldir_relative = $(patsubst $(prefix)/%,%,$(htmldir))
-- 
2.15.1.424.g9478a66081
Ævar Arnfjörð Bjarmason· Mar 13, 2018, 20:39 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH 3/3] Makefile: optionally symlink libexec/git-core binaries to bin/git

Add a INSTALL_SYMLINKS option which if enabled, changes the default hardlink installation method to one where the relevant binaries in libexec/git-core are symlinked back to ../../bin, instead of being hardlinked.

This new option also overrides the behavior of the existing NO_*_HARDLINKS variables which in some cases would produce symlinks within to libexec/, e.g. "git-add" symlinked to "git" which would be copy of the "git" found in bin/, now "git-add" in libexec/ is always going to be symlinked to the "git" found in the bin/ directory.

This option is being added because:
 1) I think it makes what we're doing a lot more obvious. E.g. I'd
    never noticed that the libexec binaries were really just hardlinks
    since e.g. ls(1) won't show that in any obvious way. You need to
    start stat(1)-ing things and look at the inodes to see what's
    going on.
 2) Some tools have very crappy support for hardlinks, e.g. the Git
    shipped with GitLab is much bigger than it should be because
    they're using a chef module that doesn't know about hardlinks, see
    https://github.com/chef/omnibus/issues/827
    I've also ran into other related issues that I think are explained
    by this, e.g. compiling git with debugging and rpm refusing to
    install a ~200MB git package with 2GB left on the FS, I think that
    was because it doesn't consider hardlinks, just the sum of the
    byte size of everything in the package.

As for the implementation, the "../../bin" noted above will vary given some given some values of "../.." and "bin" depending on the depth of the gitexecdir relative to the destdir, and the "bindir" target, e.g. setting "bindir=/tmp/git/binaries gitexecdir=foo/bar/baz" will do the right thing and produce this result:

    $ file /tmp/git/foo/bar/baz/git-add
    /tmp/git/foo/bar/baz/git-add: symbolic link to ../../../binaries/git
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 46 +++++++++++++++++++++++++++++++---------------
 1 file changed, 31 insertions(+), 15 deletions(-)
diff --git a/Makefile b/Makefile
index ee0b6c8940..ac7616422d 100644
--- a/Makefile
+++ b/Makefile
@@ -329,6 +329,13 @@ all::
 # when hardlinking a file to another name and unlinking the original file right
 # away (some NTFS drivers seem to zero the contents in that scenario).
 #
+# Define INSTALL_SYMLINKS if you prefer to have everything that can be
+# symlinked between bin/ and libexec/ to use relative symlinks between
+# the two. This option overrides NO_CROSS_DIRECTORY_HARDLINKS and
+# NO_INSTALL_HARDLINKS which will also use symlinking by indirection
+# within the same directory in some cases, INSTALL_SYMLINKS will
+# always symlink to the final target directly.
+#
 # Define NO_CROSS_DIRECTORY_HARDLINKS if you plan to distribute the installed
 # programs as a tar, where bin/ and libexec/ might be on different file systems.
 #
@@ -2594,35 +2601,44 @@ 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 -z "$(NO_INSTALL_HARDLINKS)$(NO_CROSS_DIRECTORY_HARDLINKS)" && \
-		ln "$$bindir/$$p" "$$execdir/$$p" 2>/dev/null || \
-		cp "$$bindir/$$p" "$$execdir/$$p" || exit; \
+		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 -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; \
+		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 -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; \
+		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 -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; \
+		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 && \
 	./check_bindir "z$$bindir" "z$$execdir" "$$bindir/git-add$X"
 
-- 
2.15.1.424.g9478a66081
Johannes Sixt· Mar 14, 2018, 07:20 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 3/3] Makefile: optionally symlink libexec/git-core binaries to bin/git

Am 13.03.2018 um 21:39 schrieb Ævar Arnfjörð Bjarmason:
Show 10 quoted lines
> Add a INSTALL_SYMLINKS option which if enabled, changes the default
> hardlink installation method to one where the relevant binaries in
> libexec/git-core are symlinked back to ../../bin, instead of being
> hardlinked.
> 
> This new option also overrides the behavior of the existing
> NO_*_HARDLINKS variables which in some cases would produce symlinks
> within to libexec/, e.g. "git-add" symlinked to "git" which would be
> copy of the "git" found in bin/, now "git-add" in libexec/ is always
> going to be symlinked to the "git" found in the bin/ directory.

It is important to leave the default at hard-linking the binaries, because on Windows symbolic links are second class citizens (they require special privileges and there is a distinction between link targets being files or directories). Hard links work well.

Show 15 quoted lines
> 
> This option is being added because:
> 
>   1) I think it makes what we're doing a lot more obvious. E.g. I'd
>      never noticed that the libexec binaries were really just hardlinks
>      since e.g. ls(1) won't show that in any obvious way. You need to
>      start stat(1)-ing things and look at the inodes to see what's
>      going on.
> 
>   2) Some tools have very crappy support for hardlinks, e.g. the Git
>      shipped with GitLab is much bigger than it should be because
>      they're using a chef module that doesn't know about hardlinks, see
>      https://github.com/chef/omnibus/issues/827
> 
>      I've also ran into other related issues that I think are explained
s/ran/run/
Show 7 quoted lines
>      by this, e.g. compiling git with debugging and rpm refusing to
>      install a ~200MB git package with 2GB left on the FS, I think that
>      was because it doesn't consider hardlinks, just the sum of the
>      byte size of everything in the package.
> 
> As for the implementation, the "../../bin" noted above will vary given
> some given some values of "../.." and "bin" depending on the depth of
s/given some//
Show 23 quoted lines
> the gitexecdir relative to the destdir, and the "bindir" target,
> e.g. setting "bindir=/tmp/git/binaries gitexecdir=foo/bar/baz" will do
> the right thing and produce this result:
> 
>      $ file /tmp/git/foo/bar/baz/git-add
>      /tmp/git/foo/bar/baz/git-add: symbolic link to ../../../binaries/git
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>   Makefile | 46 +++++++++++++++++++++++++++++++---------------
>   1 file changed, 31 insertions(+), 15 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index ee0b6c8940..ac7616422d 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -329,6 +329,13 @@ all::
>   # when hardlinking a file to another name and unlinking the original file right
>   # away (some NTFS drivers seem to zero the contents in that scenario).
>   #
> +# Define INSTALL_SYMLINKS if you prefer to have everything that can be
> +# symlinked between bin/ and libexec/ to use relative symlinks between
> +# the two. This option overrides NO_CROSS_DIRECTORY_HARDLINKS and
s/ between the two//
> +# NO_INSTALL_HARDLINKS which will also use symlinking by indirection
> +# within the same directory in some cases, INSTALL_SYMLINKS will
> +# always symlink to the final target directly.

"the final target"? Do you mean "the git executable installed in $bindir" or something like this?

Show 20 quoted lines
> +#
>   # Define NO_CROSS_DIRECTORY_HARDLINKS if you plan to distribute the installed
>   # programs as a tar, where bin/ and libexec/ might be on different file systems.
>   #
> @@ -2594,35 +2601,44 @@ 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 -z "$(NO_INSTALL_HARDLINKS)$(NO_CROSS_DIRECTORY_HARDLINKS)" && \
> -		ln "$$bindir/$$p" "$$execdir/$$p" 2>/dev/null || \
> -		cp "$$bindir/$$p" "$$execdir/$$p" || exit; \
> +		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; } \

I think that it is unnecessary to place the later options in {} brackets because && and || have equal precedence in shell scripts. That is:

	want symlinks? &&
	make symlinks ||
	want hard links? &&
	make hard links ||
	make copies ||
	exit

Of course, it means that when symlinking fails, it falls back to hard links (if permitted) or copies, whichever works. But that also happens with your version.

(Ditto in the rest of the hunk, which I don't repeat here.)
-- Hannes
Ævar Arnfjörð Bjarmason· Mar 14, 2018, 10:14 UTC · re: Johannes Sixt · lore

Re: [PATCH 3/3] Makefile: optionally symlink libexec/git-core binaries to bin/git

On Wed, Mar 14 2018, Johannes Sixt jotted:
Show 16 quoted lines
> Am 13.03.2018 um 21:39 schrieb Ævar Arnfjörð Bjarmason:
>> Add a INSTALL_SYMLINKS option which if enabled, changes the default
>> hardlink installation method to one where the relevant binaries in
>> libexec/git-core are symlinked back to ../../bin, instead of being
>> hardlinked.
>>
>> This new option also overrides the behavior of the existing
>> NO_*_HARDLINKS variables which in some cases would produce symlinks
>> within to libexec/, e.g. "git-add" symlinked to "git" which would be
>> copy of the "git" found in bin/, now "git-add" in libexec/ is always
>> going to be symlinked to the "git" found in the bin/ directory.
>
> It is important to leave the default at hard-linking the binaries,
> because on Windows symbolic links are second class citizens (they
> require special privileges and there is a distinction between link
> targets being files or directories). Hard links work well.

Yeah makes sense. I just want to add this as an option, and think if it's proven to be un-buggy we could probably turn it on by default on the *nix's if people prefer that, but yeah, we'll definitely need the uname detection.

Show 53 quoted lines
>>
>> This option is being added because:
>>
>>   1) I think it makes what we're doing a lot more obvious. E.g. I'd
>>      never noticed that the libexec binaries were really just hardlinks
>>      since e.g. ls(1) won't show that in any obvious way. You need to
>>      start stat(1)-ing things and look at the inodes to see what's
>>      going on.
>>
>>   2) Some tools have very crappy support for hardlinks, e.g. the Git
>>      shipped with GitLab is much bigger than it should be because
>>      they're using a chef module that doesn't know about hardlinks, see
>>      https://github.com/chef/omnibus/issues/827
>>
>>      I've also ran into other related issues that I think are explained
>
> s/ran/run/
>
>>      by this, e.g. compiling git with debugging and rpm refusing to
>>      install a ~200MB git package with 2GB left on the FS, I think that
>>      was because it doesn't consider hardlinks, just the sum of the
>>      byte size of everything in the package.
>>
>> As for the implementation, the "../../bin" noted above will vary given
>> some given some values of "../.." and "bin" depending on the depth of
>
> s/given some//
>
>> the gitexecdir relative to the destdir, and the "bindir" target,
>> e.g. setting "bindir=/tmp/git/binaries gitexecdir=foo/bar/baz" will do
>> the right thing and produce this result:
>>
>>      $ file /tmp/git/foo/bar/baz/git-add
>>      /tmp/git/foo/bar/baz/git-add: symbolic link to ../../../binaries/git
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>   Makefile | 46 +++++++++++++++++++++++++++++++---------------
>>   1 file changed, 31 insertions(+), 15 deletions(-)
>>
>> diff --git a/Makefile b/Makefile
>> index ee0b6c8940..ac7616422d 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -329,6 +329,13 @@ all::
>>   # when hardlinking a file to another name and unlinking the original file right
>>   # away (some NTFS drivers seem to zero the contents in that scenario).
>>   #
>> +# Define INSTALL_SYMLINKS if you prefer to have everything that can be
>> +# symlinked between bin/ and libexec/ to use relative symlinks between
>> +# the two. This option overrides NO_CROSS_DIRECTORY_HARDLINKS and
>
> s/ between the two//
Thanks. Will fix the above in a subsequent submission.
Show 6 quoted lines
>> +# NO_INSTALL_HARDLINKS which will also use symlinking by indirection
>> +# within the same directory in some cases, INSTALL_SYMLINKS will
>> +# always symlink to the final target directly.
>
> "the final target"? Do you mean "the git executable installed in
> $bindir" or something like this?

I'm not explaining this well, but what I mean is that right now if you supply NO_INSTALL_HARDLINKS you end up with this:

    bin/git
    libexec/git
    libexec/git-add -> git

I.e. we make two copies of the "git" binary, and then just symlink within the libexec dir, whereas with this change:

    bin/git
    libexec/git -> ../bin/git
    libexec/git-add -> ../bin/git

I.e. we'll only install one "git" and never copy it, and to the extent that we need symlinking we're always going to symlink directly to the binary, i.e. not:

    bin/git
    libexec/git -> ../bin/git
    libexec/git-add -> git
Even though that would also work, I just don't think it makes sense.
Show 36 quoted lines
>> +#
>>   # Define NO_CROSS_DIRECTORY_HARDLINKS if you plan to distribute the installed
>>   # programs as a tar, where bin/ and libexec/ might be on different file systems.
>>   #
>> @@ -2594,35 +2601,44 @@ 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 -z "$(NO_INSTALL_HARDLINKS)$(NO_CROSS_DIRECTORY_HARDLINKS)" && \
>> -		ln "$$bindir/$$p" "$$execdir/$$p" 2>/dev/null || \
>> -		cp "$$bindir/$$p" "$$execdir/$$p" || exit; \
>> +		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; } \
>
> I think that it is unnecessary to place the later options in {}
> brackets because && and || have equal precedence in shell
> scripts. That is:
>
> 	want symlinks? &&
> 	make symlinks ||
> 	want hard links? &&
> 	make hard links ||
> 	make copies ||
> 	exit
>
> Of course, it means that when symlinking fails, it falls back to hard
> links (if permitted) or copies, whichever works. But that also happens
> with your version.
>
> (Ditto in the rest of the hunk, which I don't repeat here.)

Yes. This is shitty, I'll change it. I'm going to inject a patch series earlier in this series so we'll do:

 	want symlinks? &&
 	make symlinks ||
 	want hard links? &&
 	make hard links ||
 	want copy fallback? &&
 	make copies ||
 	exit

And then turn on the copy fallback by default. Right now we'll silently hide errors during "make install", which isn't nice. I'll leave the fallback on by default (for now), but I'd like to declare that I want to install symlinks, and if that fails I should get an error, not a silent fallback to copy.

Linus Torvalds· Mar 14, 2018, 17:21 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 3/3] Makefile: optionally symlink libexec/git-core binaries to bin/git

On Wed, Mar 14, 2018 at 3:14 AM, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 11 quoted lines
> On Wed, Mar 14 2018, Johannes Sixt jotted:
>>
>> It is important to leave the default at hard-linking the binaries,
>> because on Windows symbolic links are second class citizens (they
>> require special privileges and there is a distinction between link
>> targets being files or directories). Hard links work well.
>
> Yeah makes sense. I just want to add this as an option, and think if
> it's proven to be un-buggy we could probably turn it on by default on
> the *nix's if people prefer that, but yeah, we'll definitely need the
> uname detection.
I definitely would prefer to make symlinks the default on unix.

It's what we used to do (long long ago), and as you pointed out, it's a lot clearer what's going on too when you don't have to look at inode numbers and link counts.

Forcing hardlinking everywhere by default just because Windows filesystems suck donkey ass through a straw is not the right thing either.

                Linus
Johannes Schindelin· Mar 15, 2018, 17:05 UTC · re: Linus Torvalds · lore

Re: [PATCH 3/3] Makefile: optionally symlink libexec/git-core binaries to bin/git

Hi Linus,
On Wed, 14 Mar 2018, Linus Torvalds wrote:
Show 23 quoted lines
> On Wed, Mar 14, 2018 at 3:14 AM, Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
> > On Wed, Mar 14 2018, Johannes Sixt jotted:
> >>
> >> It is important to leave the default at hard-linking the binaries,
> >> because on Windows symbolic links are second class citizens (they
> >> require special privileges and there is a distinction between link
> >> targets being files or directories). Hard links work well.
> >
> > Yeah makes sense. I just want to add this as an option, and think if
> > it's proven to be un-buggy we could probably turn it on by default on
> > the *nix's if people prefer that, but yeah, we'll definitely need the
> > uname detection.
> 
> I definitely would prefer to make symlinks the default on unix.
> 
> It's what we used to do (long long ago), and as you pointed out, it's
> a lot clearer what's going on too when you don't have to look at inode
> numbers and link counts.
> 
> Forcing hardlinking everywhere by default just because Windows
> filesystems suck donkey ass through a straw is not the right thing
> either.

The most sensible thing, of course, would be to *not* link the builtins at all. I mean, we deprecated the dashed form (which was a design mistake, whether you admit it or not) a long time ago.

Ciao, Johannes

Linus Torvalds· Mar 15, 2018, 17:42 UTC · re: Johannes Schindelin · lore

Re: [PATCH 3/3] Makefile: optionally symlink libexec/git-core binaries to bin/git

On Thu, Mar 15, 2018 at 10:05 AM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

> The most sensible thing, of course, would be to *not* link the builtins at
> all. I mean, we deprecated the dashed form (which was a design mistake,
> whether you admit it or not) a long time ago.
That's probably not a bad idea for the builtin commands. At least as an option.

We do end up still using the dashed form for certain things, but they are already special-cased (ie things like "git-receive-pack" and "git-shell" that very much get executed directly, and for fundamental reasons).

As to it being a design mistake? No, not really. It made a lot of sense at the time. The fact is, the problem is Windows, not git. I know you have your hangups, but that's your problem.

               Linus
Johannes Schindelin· Mar 16, 2018, 11:48 UTC · re: Linus Torvalds · lore

Re: [PATCH 3/3] Makefile: optionally symlink libexec/git-core binaries to bin/git

Hi Linus.
On Thu, 15 Mar 2018, Linus Torvalds wrote:
> We do end up still using the dashed form for certain things, but they
> are already special-cased (ie things like "git-receive-pack" and
> "git-shell" that very much get executed directly, and for fundamental
> reasons).
Please do elaborate.

Ciao, Johannes

Ævar Arnfjörð Bjarmason· Mar 16, 2018, 12:43 UTC · re: Johannes Schindelin · lore

Re: [PATCH 3/3] Makefile: optionally symlink libexec/git-core binaries to bin/git

On Fri, Mar 16 2018, Johannes Schindelin jotted:
Show 10 quoted lines
> Hi Linus.
>
> On Thu, 15 Mar 2018, Linus Torvalds wrote:
>
>> We do end up still using the dashed form for certain things, but they
>> are already special-cased (ie things like "git-receive-pack" and
>> "git-shell" that very much get executed directly, and for fundamental
>> reasons).
>
> Please do elaborate.

If you were to set set "/bin/git shell" in /etc/password it would not do the right thing as far as I know. Is that a shell name with a space in it, or the "shell" argument to /bin/git?

There's also the fully dashed forms of stuff like git-receive-pack is part of the over-ssh convention, i.e.:

    ssh <host> git-upload-pack ...

That being said I think Linus is conflating two things here. If we still had just the dashed forms on *nix we'd still have the issue of what it does to shell completion, which is one thing that got brought up in the discussion to create the "git" wrapper at the time. There were also other reasons IIRC.

That's an entirely separate discussion from how we go about either hard- or symlinking some stuff git is using, whether or not that's ever directly exposed to the user.

Having said that I have a WIP re-roll which where I'm aiming to:
 * Add a NO_INSTALL_CP_FALLBACK flag, so we won't implicitly fall back
   to cp silently (unless told so)
 * Remove the 2>/dev/null we're doing on everything. That pre-dates the
   NO_*_*HARDLINKS flags and we shouldn't be doing that anymore.
 * Add an option where we optionally won't install the majority of these
   dashed forms, regardless of whether we choose hardlinks or
   symlinks. We'll still need some linking as some dashed forms we can't
   remove, as noted above.

I didn't expect Junio to merge this down to `next` so fast, so I'll wait until INSTALL_SYMLINKS lands. As far as I know the code as-is in next isn't buggy, I'd just like to improve it a bit more, so I'll need to rebase what I have on top of that (which is fine).

Johannes Schindelin· Mar 19, 2018, 11:34 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 3/3] Makefile: optionally symlink libexec/git-core binaries to bin/git

Hi Ævar,
On Fri, 16 Mar 2018, Ævar Arnfjörð Bjarmason wrote:
Show 15 quoted lines
> 
> On Fri, Mar 16 2018, Johannes Schindelin jotted:
> 
> > On Thu, 15 Mar 2018, Linus Torvalds wrote:
> >
> >> We do end up still using the dashed form for certain things, but they
> >> are already special-cased (ie things like "git-receive-pack" and
> >> "git-shell" that very much get executed directly, and for fundamental
> >> reasons).
> >
> > Please do elaborate.
> 
> If you were to set set "/bin/git shell" in /etc/password it would not do
> the right thing as far as I know. Is that a shell name with a space in
> it, or the "shell" argument to /bin/git?

True. And `git-shell` is not a builtin, so it does not even matter with regards to this discussion.

> There's also the fully dashed forms of stuff like git-receive-pack is
> part of the over-ssh convention, i.e.:
> 
>     ssh <host> git-upload-pack ...

Even if upload-pack is not a builtin (and thus still has to be its own executable), receive-pack *is*, so this does affect our current discussion.

This is a real problem. And it is our own darned fault because we let an implementation detail bleed into a protocol. We could have designed that a lot better.

Of course we should fix this, though. There is literally no good reason that I can think of why we should not change this to `ssh <host> git upload-pack ...` (of course with an insanely long deprecation period).

Ciao, Dscho

Linus Torvalds· Mar 19, 2018, 21:21 UTC · re: Johannes Schindelin · lore

Re: [PATCH 3/3] Makefile: optionally symlink libexec/git-core binaries to bin/git

On Mon, Mar 19, 2018, 04:34 Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

>
> This is a real problem.
No it isn't.

We already handle those special cases specially, and install them in the bin directory (as opposed to libexec). And it all works fine.

Look into the bin directory some day. You'll find things like
  git-cvsserver
  gitk
  git-receive-pack
  git-shell
  git-upload-archive
  git-upload-pack

there, and the fact that a couple of them happen to be built-ins is an IMPLEMENTATION DETAIL, not a "Oh we should have used just 'git' for them".

The design of having separate programs is the *good* conceptual design. And we damn well should keep it for these things that are used for special purposes.

The fact that two of them have become built-ins as part of the git binary is incidental. It shouldn't be visible in the names, because it really is just an internal implementation thing, not anything fundamental.

> And it is our own darned fault because we let an
> implementation detail bleed into a protocol. We could have designed that a
> lot better.

And by "we" you clearly mean "not you", and by "we could have designed that a lot better" you must mean "and it was very well designed by competent people who didn't use bad operating systems".

> Of course we should fix this, though. There is literally no good reason
Go away.

We shouldn't fix it, it's all fine as-is, and there were tons of f*cking good reasons for why git did what it did. The main one being "it's a collection of scripts", which was what git _was_, for chrissake. And using spaces and running some idiotic and hard-to-verify script de-multiplexer is the WRONG THING for things like "git-shell" and "git-receive-pack" and friends.

Right now you can actually verify exactly what "git-shell" does. Or you could replace - or remove - it entirely if you don't like it. And never have to worry about running "git" with some "shell" subcommand.

And you know that it's not an alias, for example. Because "git-xyz" simply does not look up aliases.

So really. Go away, Johannes. Your concerns are complete and utter BS.

The real problem is that Windows is badly designed, but since it's easy to work around (by using hard-linking on Windows), nobody sane cares.

The solution is simple, and was already suggested: use symlinks (like we used to!) on non-windows systems. End of story.

And for the libexec thing, we might want to deprecate those names, if somebody wants to, but it's not like it actually hurts, and it gives backwards compatibility.

Btw, real Windows people know all about backwards compatibility. Ask around competent people inside MS whether it's an important thing.

So stop this idiotic "bad design" crap. Somebody working on Windows simply can't afford your attitude.

Somebody who didn't design it in the first place can't afford your attitude.
                         Linus
Ævar Arnfjörð Bjarmason· Nov 2, 2018, 22:37 UTC · re: Ævar Arnfjörð Bjarmason · lore

[RFC/PATCH 0/5] stop installing old libexec aliases like "git-init"

I think up to patch 4 here should be near a state that's ready for inclusion.

Although I'm on the fence with the approach in 1/5. Should this be a giant getopt switch statement like that in a helper script? An alternative would be to write out a shell file similar to GIT-BUILD-OPTIONS and source that from this thing. I don't know, what do you all think?

The idea with 4/5 was to make this symlink mode the default in config.mak.uname and have a blacklist of systems like Windows that couldn't deal with it.

Since my ad874608d8 ("Makefile: optionally symlink libexec/git-core binaries to bin/git", 2018-03-13) I see that e.g. Debian and GitLab have started shipping with the INSTALL_SYMLINKS flag, so making that unconditional is the next logical step.

The 5th one is more radical. See https://public-inbox.org/git/87woyfdkoi.fsf@evledraar.gmail.com/ from back in March for context.

I'd like to say it's ready, but I've spotted some fallout:
 * Help like "git ninit" suggesting "git init" doesn't work, this is
   because load_command_list() in help.c doesn't look out our
   in-memory idea of builtins, it reads the libexecdir, so if we don't
   have the programs there it doesn't know about it.
 * GIT_TEST_INSTALLED breaks entirely under this, as early as the
   heuristic for "are we built?" being "do we have git-init in
   libexecdir?". I tried a bit to make this work, but there's a lot of
   dependencies there.
 * We still (and this is also true of my ad874608d8) hardlink
   everything in the build dir via a different part of the Makefile,
   ideally we should do exactly the same thing there so also normal
   tests and not just GIT_TEST_INSTALLED (if that worked) would test
   in the same mode.
   I gave making that work a bit of a try and gave up in the Makefile
   jungle.
Ævar Arnfjörð Bjarmason (5):
  Makefile: move long inline shell loops in "install" into helper
  Makefile: conform some of the code to our coding standards
  Makefile: stop hiding failures during "install"
  Makefile: add NO_INSTALL_SYMLINKS_FALLBACK switch
  Makefile: Add a NO_INSTALL_BUILTIN_EXECDIR_ALIASES flag
 Makefile         |  65 +++++++++++--------------
 install_programs | 124 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 151 insertions(+), 38 deletions(-)
 create mode 100755 install_programs
-- 
2.19.1.930.g4563a0d9d0
Junio C Hamano· Nov 3, 2018, 01:17 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [RFC/PATCH 0/5] stop installing old libexec aliases like "git-init"

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
Show 6 quoted lines
> Although I'm on the fence with the approach in 1/5. Should this be a
> giant getopt switch statement like that in a helper script?
>
> An alternative would be to write out a shell file similar to
> GIT-BUILD-OPTIONS and source that from this thing. I don't know,
> what do you all think?

Not really. Why do we iterate over these in a shell loop, rather than having make to figure them out, just like we do when we "loop over the source files and turn them into object files" without using a shell loop? What's so special about enumerating the installation targets and iterating over the enumeration to perform an action on each of them? I think that is the first question we should be asking before patch 1/5, which already assumes that it has been decided that it must be done with a shell loop.

	I think "first install 'git' itself, and then make these
	other things derived from it" should let $(MAKE) install
	things in parallel just like it can naturally do many things
	in parallel, and the dependency rule to do so should not be
	so bad, I suspect.

This is a tangent, but I have long been wishing that somebody would notice that output during install and (dist)clean without V=1 is so different from the normal targets and do something about it, and hoped that that somebody finally turned out to be you doing so in this series X-<.

> I'd like to say it's ready, but I've spotted some fallout:

I still have not recovered from the trauma I suffered post 1.6.0 era, so I would rather *not* engage in a long discussion like this one (it is a long thread; reserve a solid hour to read it through if you are interested),

https://public-inbox.org/git/alpine.LFD.1.10.0808261435470.3363@nehalem.linux-foundation.org/

which would be needed to defend the choice, if we decide to omit installing the git-foo on disk in a released version.

I personally have no objection to offer a knob that can e used to force installation of symlinks without falling back to other methods. I think it would be ideal to do so without special casing symbolic links---rather, it would be ideal if it were a single knob INSTALL_GIT_FOO_METHOD=(symlinks|hardlinks|copies) that says "I want them to be installed as (symlinks|hardlinks|copies), no fallbacks".

Show 11 quoted lines
> Ævar Arnfjörð Bjarmason (5):
>   Makefile: move long inline shell loops in "install" into helper
>   Makefile: conform some of the code to our coding standards
>   Makefile: stop hiding failures during "install"
>   Makefile: add NO_INSTALL_SYMLINKS_FALLBACK switch
>   Makefile: Add a NO_INSTALL_BUILTIN_EXECDIR_ALIASES flag
>
>  Makefile         |  65 +++++++++++--------------
>  install_programs | 124 +++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 151 insertions(+), 38 deletions(-)
>  create mode 100755 install_programs
Ævar Arnfjörð Bjarmason· Nov 5, 2018, 11:36 UTC · re: Junio C Hamano · lore

Re: [RFC/PATCH 0/5] stop installing old libexec aliases like "git-init"

On Sat, Nov 03 2018, Junio C Hamano wrote:
Show 29 quoted lines
> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> Although I'm on the fence with the approach in 1/5. Should this be a
>> giant getopt switch statement like that in a helper script?
>>
>> An alternative would be to write out a shell file similar to
>> GIT-BUILD-OPTIONS and source that from this thing. I don't know,
>> what do you all think?
>
> Not really.  Why do we iterate over these in a shell loop, rather
> than having make to figure them out, just like we do when we "loop
> over the source files and turn them into object files" without using
> a shell loop?  What's so special about enumerating the installation
> targets and iterating over the enumeration to perform an action on
> each of them?  I think that is the first question we should be
> asking before patch 1/5, which already assumes that it has been
> decided that it must be done with a shell loop.
>
> 	I think "first install 'git' itself, and then make these
> 	other things derived from it" should let $(MAKE) install
> 	things in parallel just like it can naturally do many things
> 	in parallel, and the dependency rule to do so should not be
> 	so bad, I suspect.
>
> This is a tangent, but I have long been wishing that somebody would
> notice that output during install and (dist)clean without V=1 is so
> different from the normal targets and do something about it, and
> hoped that that somebody finally turned out to be you doing so in
> this series X-<.

I'm all for this, but don't have enough Make skills to make it happen. Can you or someone else post a WIP patch showing how to do this?

What would the targets look like? Something that's a build target for the target file in the installation directory, so e.g. if you ran "all install" and had modified just one file (and no recursive rebuilds) you'd install just that one file?

Early on in the "install" target we install many of these programs, and then some of these for-loops re-install on top of them. Actually now that I think of this this is one of the reasons for the 2>/dev/null probably, i.e. run "install" twice and you don't want to get errors.

Anyway, regardless of how the for-loop looks like (shell or make-powered) I split this up because it was getting really hard to maintain the *inner* part of those loops. I.e. needing to specially quote everything, end lines with \ etc.

But reading on...
Show 11 quoted lines
>> I'd like to say it's ready, but I've spotted some fallout:
>
> I still have not recovered from the trauma I suffered post 1.6.0
> era, so I would rather *not* engage in a long discussion like this
> one (it is a long thread; reserve a solid hour to read it through if
> you are interested),
>
> https://public-inbox.org/git/alpine.LFD.1.10.0808261435470.3363@nehalem.linux-foundation.org/
>
> which would be needed to defend the choice, if we decide to omit
> installing the git-foo on disk in a released version.
Thanks. I'll read that later.
Show 6 quoted lines
> I personally have no objection to offer a knob that can e used to
> force installation of symlinks without falling back to other
> methods.  I think it would be ideal to do so without special casing
> symbolic links---rather, it would be ideal if it were a single knob
> INSTALL_GIT_FOO_METHOD=(symlinks|hardlinks|copies) that says "I want
> them to be installed as (symlinks|hardlinks|copies), no fallbacks".

... If you're happy to accept a patch that rips out this whole conditional fallback logic and just makes it an if/elsif/elsif for symlink/hardlink/copy this makes things a lot easier.

Show 11 quoted lines
>> Ævar Arnfjörð Bjarmason (5):
>>   Makefile: move long inline shell loops in "install" into helper
>>   Makefile: conform some of the code to our coding standards
>>   Makefile: stop hiding failures during "install"
>>   Makefile: add NO_INSTALL_SYMLINKS_FALLBACK switch
>>   Makefile: Add a NO_INSTALL_BUILTIN_EXECDIR_ALIASES flag
>>
>>  Makefile         |  65 +++++++++++--------------
>>  install_programs | 124 +++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 151 insertions(+), 38 deletions(-)
>>  create mode 100755 install_programs
Johannes Schindelin· Nov 12, 2018, 13:33 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [RFC/PATCH 0/5] stop installing old libexec aliases like "git-init"

Hi Ævar,
On Fri, 2 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
>  * GIT_TEST_INSTALLED breaks entirely under this, as early as the
>    heuristic for "are we built?" being "do we have git-init in
>    libexecdir?". I tried a bit to make this work, but there's a lot of
>    dependencies there.

I have a couple of patches in the pipeline to improve `GIT_TEST_INSTALLED`, as I needed it to work without hardlinked copies of the built-ins. These patches might help this here isue.

Show 8 quoted lines
>  * We still (and this is also true of my ad874608d8) hardlink
>    everything in the build dir via a different part of the Makefile,
>    ideally we should do exactly the same thing there so also normal
>    tests and not just GIT_TEST_INSTALLED (if that worked) would test
>    in the same mode.
> 
>    I gave making that work a bit of a try and gave up in the Makefile
>    jungle.
Yep.

Ciao, Dscho

Ævar Arnfjörð Bjarmason· Nov 16, 2018, 10:38 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [RFC/PATCH 0/5] stop installing old libexec aliases like "git-init"

On Fri, Nov 02 2018, Ævar Arnfjörð Bjarmason wrote:
Show 28 quoted lines
> I think up to patch 4 here should be near a state that's ready for
> inclusion.
>
> Although I'm on the fence with the approach in 1/5. Should this be a
> giant getopt switch statement like that in a helper script? An
> alternative would be to write out a shell file similar to
> GIT-BUILD-OPTIONS and source that from this thing. I don't know, what
> do you all think?
>
> The idea with 4/5 was to make this symlink mode the default in
> config.mak.uname and have a blacklist of systems like Windows that
> couldn't deal with it.
>
> Since my ad874608d8 ("Makefile: optionally symlink libexec/git-core
> binaries to bin/git", 2018-03-13) I see that e.g. Debian and GitLab
> have started shipping with the INSTALL_SYMLINKS flag, so making that
> unconditional is the next logical step.
>
> The 5th one is more radical. See
> https://public-inbox.org/git/87woyfdkoi.fsf@evledraar.gmail.com/ from
> back in March for context.
>
> I'd like to say it's ready, but I've spotted some fallout:
>
>  * Help like "git ninit" suggesting "git init" doesn't work, this is
>    because load_command_list() in help.c doesn't look out our
>    in-memory idea of builtins, it reads the libexecdir, so if we don't
>    have the programs there it doesn't know about it.

A follow-up on this: We should really fix this for other reasons. I.e. compile in some "this is stuff we ourselves think is in git".

There's other manifestations of this, e.g.:
    git-sizer --help # => shows you help
    git sizer --help # => says it doesn't have a manpage

Because we aren't aware that git-sizer is some external tool, and that we should route --help to it.

Non-withstanding the arguable bug that things like git-sizer shouldn't be allowing themselves to be invoked by "git" like that without guaranteeing that it can consume all the options 'git' expects. When I had to deal with a similar problem in an external git-* command I was maintaining I simply made it an error to invoke it as "git mything" instead of "git-mything".

Show 25 quoted lines
>  * GIT_TEST_INSTALLED breaks entirely under this, as early as the
>    heuristic for "are we built?" being "do we have git-init in
>    libexecdir?". I tried a bit to make this work, but there's a lot of
>    dependencies there.
>
>  * We still (and this is also true of my ad874608d8) hardlink
>    everything in the build dir via a different part of the Makefile,
>    ideally we should do exactly the same thing there so also normal
>    tests and not just GIT_TEST_INSTALLED (if that worked) would test
>    in the same mode.
>
>    I gave making that work a bit of a try and gave up in the Makefile
>    jungle.
>
> Ævar Arnfjörð Bjarmason (5):
>   Makefile: move long inline shell loops in "install" into helper
>   Makefile: conform some of the code to our coding standards
>   Makefile: stop hiding failures during "install"
>   Makefile: add NO_INSTALL_SYMLINKS_FALLBACK switch
>   Makefile: Add a NO_INSTALL_BUILTIN_EXECDIR_ALIASES flag
>
>  Makefile         |  65 +++++++++++--------------
>  install_programs | 124 +++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 151 insertions(+), 38 deletions(-)
>  create mode 100755 install_programs
Michael Haggerty· Nov 16, 2018, 16:00 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [RFC/PATCH 0/5] stop installing old libexec aliases like "git-init"

On Fri, Nov 16, 2018 at 11:38 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 12 quoted lines
> [...]
> A follow-up on this: We should really fix this for other
> reasons. I.e. compile in some "this is stuff we ourselves think is in
> git".
>
> There's other manifestations of this, e.g.:
>
>     git-sizer --help # => shows you help
>     git sizer --help # => says it doesn't have a manpage
>
> Because we aren't aware that git-sizer is some external tool, and that
> we should route --help to it.

That would be nice. This has been an annoying for several tools named `git-foo` that I have worked on (e.g., git-sizer, git-imerge, git-when-merged, plus many internal tools).

Show 6 quoted lines
> Non-withstanding the arguable bug that things like git-sizer shouldn't
> be allowing themselves to be invoked by "git" like that without
> guaranteeing that it can consume all the options 'git' expects. When I
> had to deal with a similar problem in an external git-* command I was
> maintaining I simply made it an error to invoke it as "git mything"
> instead of "git-mything".

Hmmm, I always thought that it was intended and supported that an external tool can name itself `git-foo` so that it can be invoked as `git foo`.

Which git options do you think that such a tool should be expected to support? Many useful ones, like `-C <path>`, `--git-dir=<path>`, `--work-tree=<path>`, `-c <name>=<value>`, and `--no-replace-objects`, work pretty much for free if the external tool uses `git` to interact with the repository. I use such options regularly with external tools. IMO it would be a regression for these tools to refuse to run when invoked as, say, `git -C path/to/repo foo`.

Michael
Ævar Arnfjörð Bjarmason· Nov 16, 2018, 19:22 UTC · re: Michael Haggerty · lore

Re: [RFC/PATCH 0/5] stop installing old libexec aliases like "git-init"

On Fri, Nov 16 2018, Michael Haggerty wrote:
Show 37 quoted lines
> On Fri, Nov 16, 2018 at 11:38 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> [...]
>> A follow-up on this: We should really fix this for other
>> reasons. I.e. compile in some "this is stuff we ourselves think is in
>> git".
>>
>> There's other manifestations of this, e.g.:
>>
>>     git-sizer --help # => shows you help
>>     git sizer --help # => says it doesn't have a manpage
>>
>> Because we aren't aware that git-sizer is some external tool, and that
>> we should route --help to it.
>
> That would be nice. This has been an annoying for several tools named
> `git-foo` that I have worked on (e.g., git-sizer, git-imerge,
> git-when-merged, plus many internal tools).
>
>> Non-withstanding the arguable bug that things like git-sizer shouldn't
>> be allowing themselves to be invoked by "git" like that without
>> guaranteeing that it can consume all the options 'git' expects. When I
>> had to deal with a similar problem in an external git-* command I was
>> maintaining I simply made it an error to invoke it as "git mything"
>> instead of "git-mything".
>
> Hmmm, I always thought that it was intended and supported that an
> external tool can name itself `git-foo` so that it can be invoked as
> `git foo`.
>
> Which git options do you think that such a tool should be expected to
> support? Many useful ones, like `-C <path>`, `--git-dir=<path>`,
> `--work-tree=<path>`, `-c <name>=<value>`, and `--no-replace-objects`,
> work pretty much for free if the external tool uses `git` to interact
> with the repository. I use such options regularly with external tools.
> IMO it would be a regression for these tools to refuse to run when
> invoked as, say, `git -C path/to/repo foo`.

I don't mean we should forbid it, just that if you have an external git-foo tool meant to be invoked like "git-foo" that and not "git foo" it might be worth to save yourself the trouble and not support the latter. I thought git-sizer was one such tool, since it docs just mention "git-sizer".

But yeah, "-c" etc. are useful, and we definitely want to support this in the general case. E.g. for "git-annex" and others that are meant to be used like that.

So maybe we should just document this interface better. It seems one implicit dependency is that we expect a manpage for the tool to exist for --help.

Or, keep some list of internal git tools and treat them specially. But I see now that would break "git annex --help" in the other direction, but maybe that would be better. I don't know.

As I recall I stopped supporting "git" invocation for the tool I was fixing a long time ago because of some funny interaction with terminals & signals. I.e. git itself would eat some of them, but the tool wanted to handle it instead.

But I don't remember the details, and can't reproduce it now with:
    $ cat ~/bin/git-sigint 
    #!/usr/bin/env perl
    $SIG{INT} = sub { warn localtime . " " . $$ };
    sleep 1 while 1;
    $ git sigint # or git-sigint
    [... behaves the same either way ...]
So maybe it was something else, or I'm misremembering...
Jeff King· Nov 17, 2018, 06:39 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [RFC/PATCH 0/5] stop installing old libexec aliases like "git-init"

On Fri, Nov 16, 2018 at 08:22:11PM +0100, Ævar Arnfjörð Bjarmason wrote:
> So maybe we should just document this interface better. It seems one
> implicit dependency is that we expect a manpage for the tool to exist
> for --help.

Yeah, I think this really the only problematic assumption. The rest of "-c", "--git-dir", etc, are just manipulating the environment, and that gets passed through to sub-invocations of Git (so if I have a script which calls git-config, it will pick up the "-c" config).

It would be nice if there was a way to ask "is there a manpage?", and fallback to running "git-cmd --help". But:

  1. I don't think there is a portable way to query that via man. And
     anyway, depending platform and config, it may be opening a browser
     to show HTML documentation (or I think even texinfo?).
  2. We can just ask whether "man git-sizer" (or whatever help display
     command) returned a non-zero exit code, and fall back to "git-sizer
     --help". But there's an infinite-loop possibility here: running
     "git-sizer --help" does what we want. But if "man git-log" failed,
     we'd run "git-log --help", which in turn runs "git help log", which
     runs "man git-log", and so on.
     You can break that loop with an environment variable for "I already
     tried to show the manpage", which would I guess convert "--help" to
     "-h".
So it's maybe do-able, but not quite as trivial as one might hope.
Show 10 quoted lines
> But I don't remember the details, and can't reproduce it now with:
> 
>     $ cat ~/bin/git-sigint 
>     #!/usr/bin/env perl
>     $SIG{INT} = sub { warn localtime . " " . $$ };
>     sleep 1 while 1;
>     $ git sigint # or git-sigint
>     [... behaves the same either way ...]
> 
> So maybe it was something else, or I'm misremembering...

I think that generally works because hitting ^C is going to send SIGINT to the whole process group. A more interesting case is:

  git sigint &
  kill -INT $!

There $! is a parent "git" process that is just waiting on git-sigint to die. But that works, too, because the parent relays common signals due to 10c6cddd92 (dashed externals: kill children on exit, 2012-01-08). So you might have been remembering issues prior to that commit (or uncommon signals; these come from sigchain_push_common).

-Peff
Johannes Schindelin· Nov 22, 2018, 12:48 UTC · re: Jeff King · lore

Re: [RFC/PATCH 0/5] stop installing old libexec aliases like "git-init"

Hi Peff,
On Sat, 17 Nov 2018, Jeff King wrote:
Show 30 quoted lines
> On Fri, Nov 16, 2018 at 08:22:11PM +0100, Ævar Arnfjörð Bjarmason wrote:
> 
> > So maybe we should just document this interface better. It seems one
> > implicit dependency is that we expect a manpage for the tool to exist
> > for --help.
> 
> Yeah, I think this really the only problematic assumption. The rest of
> "-c", "--git-dir", etc, are just manipulating the environment, and that
> gets passed through to sub-invocations of Git (so if I have a script
> which calls git-config, it will pick up the "-c" config).
> 
> It would be nice if there was a way to ask "is there a manpage?", and
> fallback to running "git-cmd --help". But:
> 
>   1. I don't think there is a portable way to query that via man. And
>      anyway, depending platform and config, it may be opening a browser
>      to show HTML documentation (or I think even texinfo?).
> 
>   2. We can just ask whether "man git-sizer" (or whatever help display
>      command) returned a non-zero exit code, and fall back to "git-sizer
>      --help". But there's an infinite-loop possibility here: running
>      "git-sizer --help" does what we want. But if "man git-log" failed,
>      we'd run "git-log --help", which in turn runs "git help log", which
>      runs "man git-log", and so on.
> 
>      You can break that loop with an environment variable for "I already
>      tried to show the manpage", which would I guess convert "--help" to
>      "-h".
> 
> So it's maybe do-able, but not quite as trivial as one might hope.

A trivial alternative would be to recommend adding a man page for 3rd-party git-<tool>s.

In other words, as soon as `git-sizer` is accompanied by `git-sizer.1` in one of the appropriate locations, it is set.

(Actually, it is not: on Windows, it would have to add git-sizer.html in the appropriate location, but we can deal with this if needed.)

Show 22 quoted lines
> > But I don't remember the details, and can't reproduce it now with:
> > 
> >     $ cat ~/bin/git-sigint 
> >     #!/usr/bin/env perl
> >     $SIG{INT} = sub { warn localtime . " " . $$ };
> >     sleep 1 while 1;
> >     $ git sigint # or git-sigint
> >     [... behaves the same either way ...]
> > 
> > So maybe it was something else, or I'm misremembering...
> 
> I think that generally works because hitting ^C is going to send SIGINT
> to the whole process group. A more interesting case is:
> 
>   git sigint &
>   kill -INT $!
> 
> There $! is a parent "git" process that is just waiting on git-sigint to
> die. But that works, too, because the parent relays common signals due
> to 10c6cddd92 (dashed externals: kill children on exit, 2012-01-08). So
> you might have been remembering issues prior to that commit (or uncommon
> signals; these come from sigchain_push_common).

FWIW I do have a couple of scripts I use that I install into `$HOME/bin/git-<tool>`. Although, granted, I essentially switched to aliases for most of them, where the aliases still call a script that is checked out in some folder in my home directory. The reason: this works in more circumstances, as I do not have to add `$HOME/bin` to the `PATH`, say, in PowerShell.

So YMMV with git-<tool>s. My rule of thumb is: if I want to use this myself only, I'll make it an alias. If I want to ship it (e.g. with Git for Windows), I'll make it a git-<tool>.

Ciao, Dscho

Jeff King· Nov 22, 2018, 16:06 UTC · re: Johannes Schindelin · lore

Re: [RFC/PATCH 0/5] stop installing old libexec aliases like "git-init"

On Thu, Nov 22, 2018 at 01:48:53PM +0100, Johannes Schindelin wrote:
Show 7 quoted lines
> > So it's maybe do-able, but not quite as trivial as one might hope.
> 
> A trivial alternative would be to recommend adding a man page for
> 3rd-party git-<tool>s.
> 
> In other words, as soon as `git-sizer` is accompanied by `git-sizer.1` in
> one of the appropriate locations, it is set.

Yes, it would be nice if everything did ship with a manpage. Unfortunately that complicates installation, where the installation for many such tools is "save this file to your $PATH".

Tools like git-sizer may be getting big and popular enough to merit the extra infrastructure, but I think there are many smaller scripts which don't.

Show 10 quoted lines
> FWIW I do have a couple of scripts I use that I install into
> `$HOME/bin/git-<tool>`. Although, granted, I essentially switched to
> aliases for most of them, where the aliases still call a script that is
> checked out in some folder in my home directory. The reason: this works in
> more circumstances, as I do not have to add `$HOME/bin` to the `PATH`,
> say, in PowerShell.
> 
> So YMMV with git-<tool>s. My rule of thumb is: if I want to use this
> myself only, I'll make it an alias. If I want to ship it (e.g. with Git
> for Windows), I'll make it a git-<tool>.

I have a handful of personal git-* scripts: mostly ones that are big enough to be unwieldy as an alias. But then, $PATH management is pretty straightforward on my platforms, so it's easier to drop a script there than to point an alias to a non-git-* script.

-Peff
Johannes Schindelin· Nov 23, 2018, 11:19 UTC · re: Jeff King · lore

Re: [RFC/PATCH 0/5] stop installing old libexec aliases like "git-init"

Hi Peff,
On Thu, 22 Nov 2018, Jeff King wrote:
Show 10 quoted lines
> On Thu, Nov 22, 2018 at 01:48:53PM +0100, Johannes Schindelin wrote:
> 
> > So YMMV with git-<tool>s. My rule of thumb is: if I want to use this
> > myself only, I'll make it an alias. If I want to ship it (e.g. with Git
> > for Windows), I'll make it a git-<tool>.
> 
> I have a handful of personal git-* scripts: mostly ones that are big
> enough to be unwieldy as an alias. But then, $PATH management is pretty
> straightforward on my platforms, so it's easier to drop a script there
> than to point an alias to a non-git-* script.

Oh, my Git aliases pretty much *all* point to a single script that takes subcommands.

Ciao, Dscho

Ævar Arnfjörð Bjarmason· Nov 2, 2018, 22:37 UTC · re: Ævar Arnfjörð Bjarmason · lore

[RFC/PATCH 1/5] Makefile: move long inline shell loops in "install" into helper

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 && \
+	./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
-- 
2.19.1.930.g4563a0d9d0
Eric Sunshine· Nov 4, 2018, 01:09 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [RFC/PATCH 1/5] Makefile: move long inline shell loops in "install" into helper

'sb/filenames-with-dashes'On Fri, Nov 2, 2018 at 6:38 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 12 quoted lines
> 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>
> ---
>  install_programs | 89 ++++++++++++++++++++++++++++++++++++++++++++++++

Pure nitpick: Earlier this year, Stefan made an effort[1] to eradicate filenames with underscores and replace them with hyphenated filenames. Perhaps name this "install-programs", instead.

[1]: sb/filenames-with-dashes
Show 14 quoted lines
> diff --git a/install_programs b/install_programs
> @@ -0,0 +1,89 @@
> +while test $# != 0
> +do
> +       case "$1" in
> +       --X=*)
> +               X="${1#--X=}"
> +               ;;
> +       --RM=*)
> +               RM="${1#--RM=}"
> +               ;;
> +       --bindir=*)
> +               bindir="${1#--bindir=}"
> +               ;;

Is the intention that the user might have X, RM, 'bindir', etc. already in the environment, and the switches in this script merely override those values? Or is the intention that X, RM, 'bindir, etc. should all start out unset? If the latter, perhaps start the script with an initialization block which clears all these variables first:

    X=
    RM=
    bindir=
    ...
Johannes Schindelin· Nov 12, 2018, 14:03 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [RFC/PATCH 1/5] Makefile: move long inline shell loops in "install" into helper

Hi,
On Fri, 2 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
Show 62 quoted lines
> 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...
Show 112 quoted lines
> +	./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
> 
> 
Ævar Arnfjörð Bjarmason· Nov 12, 2018, 14:42 UTC · re: Johannes Schindelin · lore

Re: [RFC/PATCH 1/5] Makefile: move long inline shell loops in "install" into helper

On Mon, Nov 12 2018, Johannes Schindelin wrote:
Show 236 quoted lines
> 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.

I'll try for a non-RFC re-submission of this which won't have the 5/5 NO_INSTALL_BUILTIN_EXECDIR_ALIASES (for now) which'll hopefully be ready for inclusion.

I tried to fold up some of these special cases into one thing before, but managing the exceptions gets messier to read in my opinion than just having some duplication.

But more to the point, between your suggestion and Junio's to do all of this with some make construct: Yeah we should make it awesome, but I think a logical first step for a patch series like this is to just as-is use the existing logic we have now with some minor fixes for bugs.

We can refactor this later, I'm mainly interested in moving this over to a *.sh first so that process is less of a mess, fixing some minor bugs, and not getting a first iteration of improvements stuck on a much bigger review for "I rewrote the way the whole install process works" series.

Johannes Schindelin· Nov 12, 2018, 16:32 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [RFC/PATCH 1/5] Makefile: move long inline shell loops in "install" into helper

Hi Ævar,
On Mon, 12 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
Show 254 quoted lines
> On Mon, Nov 12 2018, Johannes Schindelin wrote:
> 
> > 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.
> 
> I'll try for a non-RFC re-submission of this which won't have the 5/5
> NO_INSTALL_BUILTIN_EXECDIR_ALIASES (for now) which'll hopefully be ready
> for inclusion.
> 
> I tried to fold up some of these special cases into one thing before,
> but managing the exceptions gets messier to read in my opinion than just
> having some duplication.
> 
> But more to the point, between your suggestion and Junio's to do all of
> this with some make construct: Yeah we should make it awesome, but I
> think a logical first step for a patch series like this is to just as-is
> use the existing logic we have now with some minor fixes for bugs.
> 
> We can refactor this later, I'm mainly interested in moving this over to
> a *.sh first so that process is less of a mess, fixing some minor bugs,
> and not getting a first iteration of improvements stuck on a much bigger
> review for "I rewrote the way the whole install process works" series.

Did you seen any mistake in my version? Because if you didn't, then it would be unfortunate to leave this technical debt unaddressed. Just moving the mess really does nothing to address it, and my version should be almost there to actually do address the mess.

Ciao, Dscho

Ævar Arnfjörð Bjarmason· Nov 16, 2018, 10:32 UTC · re: Johannes Schindelin · lore

Re: [RFC/PATCH 1/5] Makefile: move long inline shell loops in "install" into helper

On Mon, Nov 12 2018, Johannes Schindelin wrote:
Show 263 quoted lines
> Hi Ævar,
>
> On Mon, 12 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
>
>> On Mon, Nov 12 2018, Johannes Schindelin wrote:
>>
>> > 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.
>>
>> I'll try for a non-RFC re-submission of this which won't have the 5/5
>> NO_INSTALL_BUILTIN_EXECDIR_ALIASES (for now) which'll hopefully be ready
>> for inclusion.
>>
>> I tried to fold up some of these special cases into one thing before,
>> but managing the exceptions gets messier to read in my opinion than just
>> having some duplication.
>>
>> But more to the point, between your suggestion and Junio's to do all of
>> this with some make construct: Yeah we should make it awesome, but I
>> think a logical first step for a patch series like this is to just as-is
>> use the existing logic we have now with some minor fixes for bugs.
>>
>> We can refactor this later, I'm mainly interested in moving this over to
>> a *.sh first so that process is less of a mess, fixing some minor bugs,
>> and not getting a first iteration of improvements stuck on a much bigger
>> review for "I rewrote the way the whole install process works" series.
>
> Did you seen any mistake in my version? Because if you didn't, then it
> would be unfortunate to leave this technical debt unaddressed. Just moving
> the mess really does nothing to address it, and my version should be
> almost there to actually do address the mess.

It's not that I found some bug, and I'm not saying it should be left unaddressed. Just pointing out that saying "let's make this even better while we're at it" can make perfect the enemy of the good, particularly in this case since we have no tests for this build procedure and there's a myriad of option combinations that need to be tested.

Ævar Arnfjörð Bjarmason· Nov 2, 2018, 22:37 UTC · re: Ævar Arnfjörð Bjarmason · lore

[RFC/PATCH 2/5] Makefile: conform some of the code to our coding standards

This code is still very much unlike our usual style since it was lifted from the Makefile, but we can at least make some of it use the usual style and line spacing.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 install_programs | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)
diff --git a/install_programs b/install_programs
index e287108112..d3333cd25f 100755
--- a/install_programs
+++ b/install_programs
@@ -50,17 +50,21 @@ do
 	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
+
+if test "$bindir/" != "$execdir/"
+then
+	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
+fi &&
+
+for p in $list_bindir_git_dashed
+do
 	$RM "$bindir/$p" &&
 	test -n "$INSTALL_SYMLINKS" &&
 	ln -s "git$X" "$bindir/$p" ||
@@ -69,6 +73,7 @@ for p in $list_bindir_git_dashed; do
 	  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" &&
@@ -78,6 +83,7 @@ for p in $list_execdir_git_dashed; do
 	  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" &&
-- 
2.19.1.930.g4563a0d9d0
Ævar Arnfjörð Bjarmason· Nov 2, 2018, 22:37 UTC · re: Ævar Arnfjörð Bjarmason · lore

[RFC/PATCH 3/5] Makefile: stop hiding failures during "install"

Change the fallback mechanism where we try to create hardlinks and ultimately fall back on a plain copy to emit the errors it encounters instead of hiding them away and silently falling back to copying.

Hiding these errors dates back to 3e073dc561 ("Makefile: always provide a fallback when hardlinks fail", 2008-08-25) when the existing "hardlink or copy" logic was amended to hide the errors.

At that time "make install" hadn't yet been taught any of the NO_*_HARDLINK options, that happened later in 3426e34fed ("Add NO_CROSS_DIRECTORY_HARDLINKS support to the Makefile", 2009-05-11) and was finally finished to roughly the current form in 70de5e65e8 ("Makefile: NO_INSTALL_HARDLINKS", 2012-05-02).

If someone is building a git in an environment where hard linking fails, they can now specify some combination of NO_INSTALL_HARDLINKS=YesPlease and NO_INSTALL_HARDLINKS=YesPlease, it doesn't make sense anymore to not only implicitly fall back to copying, but to do so silently.

This change leaves no way to not get errors spewed if we're trying and failing to e.g. make symlinks and having to fall back on "cp". I think that's OK.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 install_programs | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/install_programs b/install_programs
index d3333cd25f..367e9a6cdf 100755
--- a/install_programs
+++ b/install_programs
@@ -58,7 +58,7 @@ then
 		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 ||
+		  ln "$bindir/$p" "$execdir/$p" ||
 		  cp "$bindir/$p" "$execdir/$p" || exit; }
 	done
 fi &&
@@ -69,8 +69,8 @@ do
 	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 ||
+	  ln "$bindir/git$X" "$bindir/$p" ||
+	  ln -s "git$X" "$bindir/$p" ||
 	  cp "$bindir/git$X" "$bindir/$p" || exit; }
 done &&
 
@@ -79,8 +79,8 @@ for p in $list_execdir_git_dashed; do
 	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 ||
+	  ln "$execdir/git$X" "$execdir/$p" ||
+	  ln -s "git$X" "$execdir/$p" ||
 	  cp "$execdir/git$X" "$execdir/$p" || exit; }
 done &&
 
@@ -89,7 +89,7 @@ for p in $list_execdir_curl_aliases; do
 	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 ||
+	  ln "$execdir/git-remote-http$X" "$execdir/$p" ||
+	  ln -s "git-remote-http$X" "$execdir/$p" ||
 	  cp "$execdir/git-remote-http$X" "$execdir/$p" || exit; }
 done
-- 
2.19.1.930.g4563a0d9d0
Ævar Arnfjörð Bjarmason· Nov 2, 2018, 22:37 UTC · re: Ævar Arnfjörð Bjarmason · lore

[RFC/PATCH 4/5] Makefile: add NO_INSTALL_SYMLINKS_FALLBACK switch

Add a switch for use in conjunction with the INSTALL_SYMLINKS flag added in ad874608d8 ("Makefile: optionally symlink libexec/git-core binaries to bin/git", 2018-03-13).

Now it's possible to install git with:
    INSTALL_SYMLINKS=YesPlease NO_INSTALL_SYMLINKS_FALLBACK=YesPlease

And know for sure that there's not going to be any silent fallbacks on hardlinks or copying of files if symlinking fails.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile         |  5 ++++
 install_programs | 69 ++++++++++++++++++++++++++++++++----------------
 2 files changed, 51 insertions(+), 23 deletions(-)
diff --git a/Makefile b/Makefile
index aa6ca1fa68..07c8b74353 100644
--- a/Makefile
+++ b/Makefile
@@ -342,6 +342,10 @@ all::
 # within the same directory in some cases, INSTALL_SYMLINKS will
 # always symlink to the final target directly.
 #
+# Define NO_INSTALL_SYMLINKS_FALLBACK if in conjunction with
+# INSTALL_SYMLINKS if you'd prefer not to have the install procedure
+# fallack on hardlinking or copying if "ln -s" fails.
+#
 # Define NO_CROSS_DIRECTORY_HARDLINKS if you plan to distribute the installed
 # programs as a tar, where bin/ and libexec/ might be on different file systems.
 #
@@ -2818,6 +2822,7 @@ endif
 		--flag-install-symlinks="$(INSTALL_SYMLINKS)" \
 		--flag-no-install-hardlinks="$(NO_INSTALL_HARDLINKS)" \
 		--flag-no-cross-directory-hardlinks="$(NO_CROSS_DIRECTORY_HARDLINKS)" \
+		--flag-no-install-symlinks-fallback="$(NO_INSTALL_SYMLINKS_FALLBACK)" \
 		--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)" \
diff --git a/install_programs b/install_programs
index 367e9a6cdf..51e08019dd 100755
--- a/install_programs
+++ b/install_programs
@@ -30,6 +30,9 @@ do
 	--flag-no-cross-directory-hardlinks=*)
 		NO_CROSS_DIRECTORY_HARDLINKS="${1#--flag-no-cross-directory-hardlinks=}"
 		;;
+	--flag-no-install-symlinks-fallback=*)
+		NO_INSTALL_SYMLINKS_FALLBACK="${1#--flag-no-install-symlinks-fallback=}"
+		;;
 	--list-bindir-standalone=*)
 		list_bindir_standalone="${1#--list-bindir-standalone=}"
 		;;
@@ -55,41 +58,61 @@ if test "$bindir/" != "$execdir/"
 then
 	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" ||
-		  cp "$bindir/$p" "$execdir/$p" || exit; }
+		if test -n "$INSTALL_SYMLINKS" -a -n "$NO_INSTALL_SYMLINKS_FALLBACK"
+		then
+			ln -s "$destdir_from_execdir/$bindir_relative/$p" "$execdir/$p"
+		else
+			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" ||
+			  cp "$bindir/$p" "$execdir/$p" || exit; }
+		fi
 	done
 fi &&
 
 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" ||
-	  ln -s "git$X" "$bindir/$p" ||
-	  cp "$bindir/git$X" "$bindir/$p" || exit; }
+	if test -n "$INSTALL_SYMLINKS" -a -n "$NO_INSTALL_SYMLINKS_FALLBACK"
+	then
+		ln -s "git$X" "$bindir/$p"
+	else
+		test -n "$INSTALL_SYMLINKS" &&
+		ln -s "git$X" "$bindir/$p" ||
+		{ test -z "$NO_INSTALL_HARDLINKS" &&
+		  ln "$bindir/git$X" "$bindir/$p" ||
+		  ln -s "git$X" "$bindir/$p" ||
+		  cp "$bindir/git$X" "$bindir/$p" || exit; }
+	fi
 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" ||
-	  ln -s "git$X" "$execdir/$p" ||
-	  cp "$execdir/git$X" "$execdir/$p" || exit; }
+	if test -n "$INSTALL_SYMLINKS" -a -n "$NO_INSTALL_SYMLINKS_FALLBACK"
+	then
+		ln -s "$destdir_from_execdir/$bindir_relative/git$X" "$execdir/$p"
+	else
+		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" ||
+		  ln -s "git$X" "$execdir/$p" ||
+		  cp "$execdir/git$X" "$execdir/$p" || exit; }
+	fi
 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" ||
-	  ln -s "git-remote-http$X" "$execdir/$p" ||
-	  cp "$execdir/git-remote-http$X" "$execdir/$p" || exit; }
+	if test -n "$INSTALL_SYMLINKS" -a -n "$NO_INSTALL_SYMLINKS_FALLBACK"
+	then
+		ln -s "git-remote-http$X" "$execdir/$p"
+	else
+		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" ||
+		  ln -s "git-remote-http$X" "$execdir/$p" ||
+		  cp "$execdir/git-remote-http$X" "$execdir/$p" || exit; }
+	fi
 done
-- 
2.19.1.930.g4563a0d9d0
Eric Sunshine· Nov 4, 2018, 01:01 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [RFC/PATCH 4/5] Makefile: add NO_INSTALL_SYMLINKS_FALLBACK switch

On Fri, Nov 2, 2018 at 6:38 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
Show 9 quoted lines
> Add a switch for use in conjunction with the INSTALL_SYMLINKS flag
> added in ad874608d8 ("Makefile: optionally symlink libexec/git-core
> binaries to bin/git", 2018-03-13).
> [...]
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> diff --git a/Makefile b/Makefile
> @@ -342,6 +342,10 @@ all::
> +# Define NO_INSTALL_SYMLINKS_FALLBACK if in conjunction with
s/if in/in/
> +# INSTALL_SYMLINKS if you'd prefer not to have the install procedure
> +# fallack on hardlinking or copying if "ln -s" fails.
> +#
Ævar Arnfjörð Bjarmason· Nov 2, 2018, 22:37 UTC · re: Ævar Arnfjörð Bjarmason · lore

[RFC/PATCH 5/5] Makefile: Add a NO_INSTALL_BUILTIN_EXECDIR_ALIASES flag

Back when git was initially written the likes of "git-add", "git-init" etc. were installed in the user's $PATH. A few years later everything, with a few exceptions like git-upload-pack and git-receive-pack, was expected to be invoked as "git $cmd".

Now something like a decade later we're still installing these old commands in gitexecdir. This is so someone with a shellscript that still targets e.g. "git-init" can add $(git --exec-path) to their $PATH and not have to change their script.

Let's add an option to break this backwards compatibility. Now with NO_INSTALL_BUILTIN_EXECDIR_ALIASES=YesPlease there's only 3 programs in the bindir that are hardlinked to "git" (receive-pack, upload-archive & upload-pack), and 3 in the gitexecdir (git-remote-{ftp,ftps,https} linked to git-remote-http).

There's no cross-directory links anymore, so the "NO_CROSS_DIRECTORY_HARDLINKS" flag becomes redundant under this new option.

1. https://public-inbox.org/git/87woyfdkoi.fsf@evledraar.gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile         |  8 ++++++++
 install_programs | 36 +++++++++++++++++++++---------------
 2 files changed, 29 insertions(+), 15 deletions(-)
diff --git a/Makefile b/Makefile
index 07c8b74353..a849a7b6d1 100644
--- a/Makefile
+++ b/Makefile
@@ -346,6 +346,13 @@ all::
 # INSTALL_SYMLINKS if you'd prefer not to have the install procedure
 # fallack on hardlinking or copying if "ln -s" fails.
 #
+# Define NO_INSTALL_BUILTIN_EXECDIR_ALIASES if you'd like to skip
+# installing legacy such as "git-init" and "git-add" in the
+# gitexecdir. Unless you're on a system where "which git-init" is
+# expected to returns something set this. Users have been expected to
+# use the likes of "git init" for ages now, these programs were only
+# provided for legacy compatibility.
+#
 # Define NO_CROSS_DIRECTORY_HARDLINKS if you plan to distribute the installed
 # programs as a tar, where bin/ and libexec/ might be on different file systems.
 #
@@ -2823,6 +2830,7 @@ endif
 		--flag-no-install-hardlinks="$(NO_INSTALL_HARDLINKS)" \
 		--flag-no-cross-directory-hardlinks="$(NO_CROSS_DIRECTORY_HARDLINKS)" \
 		--flag-no-install-symlinks-fallback="$(NO_INSTALL_SYMLINKS_FALLBACK)" \
+		--flag-no-install-builtin-execdir-aliases="$(NO_INSTALL_BUILTIN_EXECDIR_ALIASES)" \
 		--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)" \
diff --git a/install_programs b/install_programs
index 51e08019dd..8d89cd9984 100755
--- a/install_programs
+++ b/install_programs
@@ -33,6 +33,9 @@ do
 	--flag-no-install-symlinks-fallback=*)
 		NO_INSTALL_SYMLINKS_FALLBACK="${1#--flag-no-install-symlinks-fallback=}"
 		;;
+	--flag-no-install-builtin-execdir-aliases=*)
+		NO_INSTALL_BUILTIN_EXECDIR_ALIASES="${1#--flag-no-install-builtin-execdir-aliases=}"
+		;;
 	--list-bindir-standalone=*)
 		list_bindir_standalone="${1#--list-bindir-standalone=}"
 		;;
@@ -54,7 +57,7 @@ do
 	shift
 done &&
 
-if test "$bindir/" != "$execdir/"
+if test "$bindir/" != "$execdir/" -a -z "$NO_INSTALL_BUILTIN_EXECDIR_ALIASES"
 then
 	for p in $list_bindir_standalone; do
 		$RM "$execdir/$p" &&
@@ -87,20 +90,23 @@ do
 	fi
 done &&
 
-for p in $list_execdir_git_dashed; do
-	$RM "$execdir/$p" &&
-	if test -n "$INSTALL_SYMLINKS" -a -n "$NO_INSTALL_SYMLINKS_FALLBACK"
-	then
-		ln -s "$destdir_from_execdir/$bindir_relative/git$X" "$execdir/$p"
-	else
-		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" ||
-		  ln -s "git$X" "$execdir/$p" ||
-		  cp "$execdir/git$X" "$execdir/$p" || exit; }
-	fi
-done &&
+if test -z "$NO_INSTALL_BUILTIN_EXECDIR_ALIASES"
+then
+	for p in $list_execdir_git_dashed; do
+		$RM "$execdir/$p" &&
+		if test -n "$INSTALL_SYMLINKS" -a -n "$NO_INSTALL_SYMLINKS_FALLBACK"
+		then
+			ln -s "$destdir_from_execdir/$bindir_relative/git$X" "$execdir/$p"
+		else
+			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" ||
+			  ln -s "git$X" "$execdir/$p" ||
+			  cp "$execdir/git$X" "$execdir/$p" || exit; }
+		fi
+	done
+fi &&
 
 for p in $list_execdir_curl_aliases; do
 	$RM "$execdir/$p" &&
-- 
2.19.1.930.g4563a0d9d0
Eric Sunshine· Nov 4, 2018, 01:04 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [RFC/PATCH 5/5] Makefile: Add a NO_INSTALL_BUILTIN_EXECDIR_ALIASES flag

On Fri, Nov 2, 2018 at 6:38 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
Show 14 quoted lines
> Let's add an option to break this backwards compatibility. Now with
> NO_INSTALL_BUILTIN_EXECDIR_ALIASES=YesPlease there's only 3 programs
> in the bindir that are hardlinked to "git" (receive-pack,
> upload-archive & upload-pack), and 3 in the
> gitexecdir (git-remote-{ftp,ftps,https} linked to git-remote-http).
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> diff --git a/Makefile b/Makefile
> @@ -346,6 +346,13 @@ all::
> +# Define NO_INSTALL_BUILTIN_EXECDIR_ALIASES if you'd like to skip
> +# installing legacy such as "git-init" and "git-add" in the
> +# gitexecdir. Unless you're on a system where "which git-init" is
> +# expected to returns something set this. Users have been expected to

s/returns/return/ s/something/&,/

Although, it's not clear what "return something" means. Perhaps rephrase it as:
   ...git-init is expected to exist, set this.
> +# use the likes of "git init" for ages now, these programs were only
> +# provided for legacy compatibility.
> +#
Johannes Schindelin· Nov 12, 2018, 14:14 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [RFC/PATCH 5/5] Makefile: Add a NO_INSTALL_BUILTIN_EXECDIR_ALIASES flag

Hi Ævar,
On Fri, 2 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
Show 24 quoted lines
> Back when git was initially written the likes of "git-add", "git-init"
> etc. were installed in the user's $PATH. A few years later everything,
> with a few exceptions like git-upload-pack and git-receive-pack, was
> expected to be invoked as "git $cmd".
> 
> Now something like a decade later we're still installing these old
> commands in gitexecdir. This is so someone with a shellscript that
> still targets e.g. "git-init" can add $(git --exec-path) to their
> $PATH and not have to change their script.
> 
> Let's add an option to break this backwards compatibility. Now with
> NO_INSTALL_BUILTIN_EXECDIR_ALIASES=YesPlease there's only 3 programs
> in the bindir that are hardlinked to "git" (receive-pack,
> upload-archive & upload-pack), and 3 in the
> gitexecdir (git-remote-{ftp,ftps,https} linked to git-remote-http).
> 
> There's no cross-directory links anymore, so the
> "NO_CROSS_DIRECTORY_HARDLINKS" flag becomes redundant under this new
> option.
> 
> 1. https://public-inbox.org/git/87woyfdkoi.fsf@evledraar.gmail.com/
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
I like the idea.

With my suggested refactoring that avoids the non-DRY code, this patch would also become much simpler (as would 2/5 -- 4/5).

However, I would not call these "aliases". That's just confusing. Maybe NO_INSTALL_DASHED_BUILTINS would be better? It certainly would not have confused me.

Ciao, Dscho

Show 95 quoted lines
>  Makefile         |  8 ++++++++
>  install_programs | 36 +++++++++++++++++++++---------------
>  2 files changed, 29 insertions(+), 15 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 07c8b74353..a849a7b6d1 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -346,6 +346,13 @@ all::
>  # INSTALL_SYMLINKS if you'd prefer not to have the install procedure
>  # fallack on hardlinking or copying if "ln -s" fails.
>  #
> +# Define NO_INSTALL_BUILTIN_EXECDIR_ALIASES if you'd like to skip
> +# installing legacy such as "git-init" and "git-add" in the
> +# gitexecdir. Unless you're on a system where "which git-init" is
> +# expected to returns something set this. Users have been expected to
> +# use the likes of "git init" for ages now, these programs were only
> +# provided for legacy compatibility.
> +#
>  # Define NO_CROSS_DIRECTORY_HARDLINKS if you plan to distribute the installed
>  # programs as a tar, where bin/ and libexec/ might be on different file systems.
>  #
> @@ -2823,6 +2830,7 @@ endif
>  		--flag-no-install-hardlinks="$(NO_INSTALL_HARDLINKS)" \
>  		--flag-no-cross-directory-hardlinks="$(NO_CROSS_DIRECTORY_HARDLINKS)" \
>  		--flag-no-install-symlinks-fallback="$(NO_INSTALL_SYMLINKS_FALLBACK)" \
> +		--flag-no-install-builtin-execdir-aliases="$(NO_INSTALL_BUILTIN_EXECDIR_ALIASES)" \
>  		--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)" \
> diff --git a/install_programs b/install_programs
> index 51e08019dd..8d89cd9984 100755
> --- a/install_programs
> +++ b/install_programs
> @@ -33,6 +33,9 @@ do
>  	--flag-no-install-symlinks-fallback=*)
>  		NO_INSTALL_SYMLINKS_FALLBACK="${1#--flag-no-install-symlinks-fallback=}"
>  		;;
> +	--flag-no-install-builtin-execdir-aliases=*)
> +		NO_INSTALL_BUILTIN_EXECDIR_ALIASES="${1#--flag-no-install-builtin-execdir-aliases=}"
> +		;;
>  	--list-bindir-standalone=*)
>  		list_bindir_standalone="${1#--list-bindir-standalone=}"
>  		;;
> @@ -54,7 +57,7 @@ do
>  	shift
>  done &&
>  
> -if test "$bindir/" != "$execdir/"
> +if test "$bindir/" != "$execdir/" -a -z "$NO_INSTALL_BUILTIN_EXECDIR_ALIASES"
>  then
>  	for p in $list_bindir_standalone; do
>  		$RM "$execdir/$p" &&
> @@ -87,20 +90,23 @@ do
>  	fi
>  done &&
>  
> -for p in $list_execdir_git_dashed; do
> -	$RM "$execdir/$p" &&
> -	if test -n "$INSTALL_SYMLINKS" -a -n "$NO_INSTALL_SYMLINKS_FALLBACK"
> -	then
> -		ln -s "$destdir_from_execdir/$bindir_relative/git$X" "$execdir/$p"
> -	else
> -		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" ||
> -		  ln -s "git$X" "$execdir/$p" ||
> -		  cp "$execdir/git$X" "$execdir/$p" || exit; }
> -	fi
> -done &&
> +if test -z "$NO_INSTALL_BUILTIN_EXECDIR_ALIASES"
> +then
> +	for p in $list_execdir_git_dashed; do
> +		$RM "$execdir/$p" &&
> +		if test -n "$INSTALL_SYMLINKS" -a -n "$NO_INSTALL_SYMLINKS_FALLBACK"
> +		then
> +			ln -s "$destdir_from_execdir/$bindir_relative/git$X" "$execdir/$p"
> +		else
> +			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" ||
> +			  ln -s "git$X" "$execdir/$p" ||
> +			  cp "$execdir/git$X" "$execdir/$p" || exit; }
> +		fi
> +	done
> +fi &&
>  
>  for p in $list_execdir_curl_aliases; do
>  	$RM "$execdir/$p" &&
> -- 
> 2.19.1.930.g4563a0d9d0
> 
> 
Johannes Schindelin· Mar 15, 2018, 17:03 UTC · re: Johannes Sixt · lore

Re: [PATCH 3/3] Makefile: optionally symlink libexec/git-core binaries to bin/git

Hi,
On Wed, 14 Mar 2018, Johannes Sixt wrote:
Show 16 quoted lines
> Am 13.03.2018 um 21:39 schrieb Ævar Arnfjörð Bjarmason:
> > Add a INSTALL_SYMLINKS option which if enabled, changes the default
> > hardlink installation method to one where the relevant binaries in
> > libexec/git-core are symlinked back to ../../bin, instead of being
> > hardlinked.
> > 
> > This new option also overrides the behavior of the existing
> > NO_*_HARDLINKS variables which in some cases would produce symlinks
> > within to libexec/, e.g. "git-add" symlinked to "git" which would be
> > copy of the "git" found in bin/, now "git-add" in libexec/ is always
> > going to be symlinked to the "git" found in the bin/ directory.
> 
> It is important to leave the default at hard-linking the binaries, because on
> Windows symbolic links are second class citizens (they require special
> privileges and there is a distinction between link targets being files or
> directories). Hard links work well.

To clarify: symbolic links do not exist in Windows Vista and earlier. (There exists a concept called Junction points, but it has subtly different semantics than symbolic links, different enough that we cannot emulate symbolic links via Junctions).

Windows 7 and later do have symbolic links, but they require elevated privileges to be created, as Hannes pointed out.

Since Windows 10 version 1703 (Creators Update), enabling Developer Mode will disable this restriction and allow creating symlinks without UAC elevation. See https://blogs.windows.com/buildingapps/2016/12/02/symlinks-windows-10/ for details.

In Git for Windows, I originally missed the memo and forgot to add support for the special flag, but since Git for Windows v2.13.1, users can create symbolic links without administrators' privileges on Windows 10 (Creators Update or later) in Developer Mode.

Of course, we still support Windows all the way back to Vista, so the default is still: no symbolic links.

Thanks for your attention, Dscho

Ævar Arnfjörð Bjarmason· Mar 13, 2018, 20:39 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH 2/3] Makefile: add a gitexecdir_relative variable

This variable will be e.g. "libexec/git-core" if gitexecdir=/tmp/git/libexec/git-core is given. It'll be used by a subsequent change.

This is stolen from the yet-to-be integrated (needs resubmission) "Makefile: add Perl runtime prefix support" patch on the mailing list. See <20180108030239.92036-3-dnj@google.com> (https://public-inbox.org/git/20180108030239.92036-3-dnj@google.com/).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 2 ++
 1 file changed, 2 insertions(+)
diff --git a/Makefile b/Makefile
index b2f8f2b171..ee0b6c8940 100644
--- a/Makefile
+++ b/Makefile
@@ -488,6 +488,7 @@ pathsep = :
 bindir_relative = $(patsubst $(prefix)/%,%,$(bindir))
 mandir_relative = $(patsubst $(prefix)/%,%,$(mandir))
 infodir_relative = $(patsubst $(prefix)/%,%,$(infodir))
+gitexecdir_relative = $(patsubst $(prefix)/%,%,$(gitexecdir))
 htmldir_relative = $(patsubst $(prefix)/%,%,$(htmldir))
 
 export prefix bindir sharedir sysconfdir gitwebdir perllibdir localedir
@@ -1735,6 +1736,7 @@ infodir_relative_SQ = $(subst ','\'',$(infodir_relative))
 perllibdir_SQ = $(subst ','\'',$(perllibdir))
 localedir_SQ = $(subst ','\'',$(localedir))
 gitexecdir_SQ = $(subst ','\'',$(gitexecdir))
+gitexecdir_relative_SQ = $(subst ','\'',$(gitexecdir_relative))
 template_dir_SQ = $(subst ','\'',$(template_dir))
 htmldir_relative_SQ = $(subst ','\'',$(htmldir_relative))
 prefix_SQ = $(subst ','\'',$(prefix))
-- 
2.15.1.424.g9478a66081
Ævar Arnfjörð Bjarmason· Mar 14, 2018, 10:18 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: Why don't we symlink libexec/git-core/* to bin/git?

On Tue, Mar 13 2018, Ævar Arnfjörð Bjarmason jotted:
Show 38 quoted lines
> On Thu, Mar 08 2018, Daniel Jacques jotted:
>
>>> It would be great to have this rebooted now that my perl cleanup efforts
>>> have un-blocked this. Will be happy to help review & test the next
>>> iteration.
>>
>> Yes, I was just thinking the same thing. I wanted to make sure the Perl
>> changes had landed, and I'm pleased to see that they have. I should have
>> time in the next few days to rebase and put up a new version of the patch
>> series. I'll keep you in the loop, and thanks for pinging!
>
> Related to this, I came across this bug report
> https://gitlab.com/gitlab-org/omnibus-gitlab/issues/3265 which is
> wondering why we're installing N copies of the git binary, presumably
> they're building with NO_INSTALL_HARDLINKS.
>
> Just doing this:
>
>     diff --git a/Makefile b/Makefile
>     index de4b8f0c02..2222319a4f 100644
>     --- a/Makefile
>     +++ b/Makefile
>     @@ -2596,7 +2596,7 @@ endif
>               for p in git$X $(filter $(install_bindir_programs),$(ALL_PROGRAMS)); do \
>                     $(RM) "$$execdir/$$p" && \
>                     test -z "$(NO_INSTALL_HARDLINKS)$(NO_CROSS_DIRECTORY_HARDLINKS)" && \
>     -               ln "$$bindir/$$p" "$$execdir/$$p" 2>/dev/null || \
>     +               ln -s "$$bindir/$$p" "$$execdir/$$p" 2>/dev/null || \
>                     cp "$$bindir/$$p" "$$execdir/$$p" || exit; \
>               done; \
>             } && \
>
> Seems to work for me, although obviously this would need to be optional,
> and it'll get in the way of Daniel's patch since it use the absolute
> path.
>
> But is there any reason anyone can think of for why we shouldn't be
> figuring out the relative path and symlinking the two?

Also, as another follow-up question. we have stuff like "git-add" in the libexec/ directory, but when you run "git add" the bin/git binary just handles that internally, it's not dispatching to libexec/git-add.

Is the only reason we're still installing these binaries like git-add in libexec for compatibility with some old installation where that was added to the $PATH, shouldn't we (and I can write this patch) also have a toggle for "I want the modern install method" which would not install any of these binaries like git-add at all?

Then the libexec/ dir would only contain things that we really do need the bin/git to dispatch to, like git-svn, git-bisect etc.

Junio C Hamano· Mar 14, 2018, 16:07 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: Why don't we symlink libexec/git-core/* to bin/git?

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 8 quoted lines
> Is the only reason we're still installing these binaries like git-add in
> libexec for compatibility with some old installation where that was
> added to the $PATH, shouldn't we (and I can write this patch) also have
> a toggle for "I want the modern install method" which would not install
> any of these binaries like git-add at all?
>
> Then the libexec/ dir would only contain things that we really do need
> the bin/git to dispatch to, like git-svn, git-bisect etc.

Removing them by default was proposed and failed; see this thread for example:

  https://public-inbox.org/git/7vr68b8q9p.fsf@gitster.siamese.dyndns.org/#t

If a packager ships Git without these copies in libexec, that is not the Git that promised users that prepending the $(git --exec-path) aka GIT_EXEC_PATH to your $PATH is a valid way to preserve their older script.

I do not think anybody actually minds to have an option to omit them as long as the users understand the consequence (i.e. old promises broken) and know they are not affected (i.e. they do not have scripts that rely on the old promise).

Johannes Schindelin· Mar 15, 2018, 17:16 UTC · re: Junio C Hamano · lore

Re: Why don't we symlink libexec/git-core/* to bin/git?

Hi Junio,
On Wed, 14 Mar 2018, Junio C Hamano wrote:
Show 15 quoted lines
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> 
> > Is the only reason we're still installing these binaries like git-add in
> > libexec for compatibility with some old installation where that was
> > added to the $PATH, shouldn't we (and I can write this patch) also have
> > a toggle for "I want the modern install method" which would not install
> > any of these binaries like git-add at all?
> >
> > Then the libexec/ dir would only contain things that we really do need
> > the bin/git to dispatch to, like git-svn, git-bisect etc.
> 
> Removing them by default was proposed and failed; see this thread
> for example:
> 
>   https://public-inbox.org/git/7vr68b8q9p.fsf@gitster.siamese.dyndns.org/#t

Let's add a very, very important piece of information that was missing: this thread is from late August 2008. We had deprecated the dashed form "only for a couple of months" by then (we removed the dashed form from the completions end of April 2008 in 799596a5d06 (completion: remove use of dashed git commands, 2008-04-20) for example).

Show 9 quoted lines
> If a packager ships Git without these copies in libexec, that is not
> the Git that promised users that prepending the $(git --exec-path)
> aka GIT_EXEC_PATH to your $PATH is a valid way to preserve their
> older script.
> 
> I do not think anybody actually minds to have an option to omit them
> as long as the users understand the consequence (i.e. old promises
> broken) and know they are not affected (i.e. they do not have
> scripts that rely on the old promise).

I am glad that you changed your stance from "without dashed builtins, your Git is broken" to this much more tenable position to state that it may break super-old promises whose use we discouraged already a full decade ago.

To add some interesting information to this: in MinGit (the light-weight "Git for applications" we bundle to avoid adding a hefty 230MB to any application that wants to bundle Git for Windows), we simply ignored that old promise. We do support hooks written as Unix shell scripts in MinGit, and we have not had a single report since offering MinGit with v2.9.2 on July 16th, 2016, that it broke anybody's scripts, so it seems that users are more sensible than our promises ;-)

Not requiring Git to install any type of link makes it even possible to bundle it as .zip file (which, let's face it, is the de facto standard for cross-platform archiving).

Ciao, Dscho

Duy Nguyen· Mar 16, 2018, 17:29 UTC · re: Johannes Schindelin · lore

Re: Why don't we symlink libexec/git-core/* to bin/git?

On Thu, Mar 15, 2018 at 6:16 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 7 quoted lines
> To add some interesting information to this: in MinGit (the light-weight
> "Git for applications" we bundle to avoid adding a hefty 230MB to any
> application that wants to bundle Git for Windows), we simply ignored that
> old promise. We do support hooks written as Unix shell scripts in MinGit,
> and we have not had a single report since offering MinGit with v2.9.2 on
> July 16th, 2016, that it broke anybody's scripts, so it seems that users
> are more sensible than our promises ;-)

That's very good to hear. Perhaps we could slowly move away from symlinking (or even hard linking) these builtin commands (with a couple exception like receive-pack and stuff) ? We don't have to do it right now but we can start announcing that we will drop it in maybe 2 or 3 releases. We do provide a new make target to recreate these links so that packagers can make a "compat" package that contains just these links if they want to. But by default a git package will have no links.

-- 
Duy
Johannes Schindelin· Mar 30, 2018, 08:59 UTC · re: Duy Nguyen · lore

Re: Why don't we symlink libexec/git-core/* to bin/git?

Hi Duy,
On Fri, 16 Mar 2018, Duy Nguyen wrote:
Show 14 quoted lines
> On Thu, Mar 15, 2018 at 6:16 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > To add some interesting information to this: in MinGit (the
> > light-weight "Git for applications" we bundle to avoid adding a hefty
> > 230MB to any application that wants to bundle Git for Windows), we
> > simply ignored that old promise. We do support hooks written as Unix
> > shell scripts in MinGit, and we have not had a single report since
> > offering MinGit with v2.9.2 on July 16th, 2016, that it broke
> > anybody's scripts, so it seems that users are more sensible than our
> > promises ;-)
> 
> That's very good to hear. Perhaps we could slowly move away from
> symlinking (or even hard linking) these builtin commands (with a
> couple exception like receive-pack and stuff) ?

I would hope so. As I said before: the fact that Git started out with everything as dashed subcommands is an implementation detail that unfortunately leaked into many parts of Git's UI. We can fix this.

Show 5 quoted lines
> We don't have to do it right now but we can start announcing that we
> will drop it in maybe 2 or 3 releases. We do provide a new make target
> to recreate these links so that packagers can make a "compat" package
> that contains just these links if they want to. But by default a git
> package will have no links.

I think that makes a *ton* of sense. Let's get to work after v2.17.0? (Same for your excellent work on t/helper/test-tool)

Ciao, Dscho

Martin Ågren· Mar 9, 2018, 06:15 UTC · re: Junio C Hamano · lore

Re: What's cooking in git.git (Mar 2018, #02; Tue, 6)

On 7 March 2018 at 00:34, Junio C Hamano <gitster@pobox.com> wrote:
Show 11 quoted lines
> * ma/config-page-only-in-list-mode (2018-02-21) 3 commits
>  - config: change default of `pager.config` to "on"
>  - config: respect `pager.config` in list/get-mode only
>  - t7006: add tests for how git config paginates
>
>  In a way similar to how "git tag" learned to honor the pager
>  setting only in the list mode, "git config" learned to ignore the
>  pager setting when it is used for setting values (i.e. when the
>  purpose of the operation is not to "show").
>
>  Is this ready for 'next'?

I am not aware of any open questions or issues. You thought out loud about how the series was structured, in particular about introducing a successful test, then redefining it, as opposed to introducing it as a failing test, then making it succeed. I hope I managed to motivate my choice better in v2 (which is what you have picked up).

Duy wondered if it was sane to use a pager when we know that we are "--get"-ing at most one config item. In v2, I addressed this by turning on paging for a more careful selection of "--get"-ters.

Martin
Duy Nguyen· Mar 9, 2018, 09:54 UTC · re: Martin Ågren · lore

Re: What's cooking in git.git (Mar 2018, #02; Tue, 6)

On Fri, Mar 9, 2018 at 1:15 PM, Martin Ågren <martin.agren@gmail.com> wrote:
Show 23 quoted lines
> On 7 March 2018 at 00:34, Junio C Hamano <gitster@pobox.com> wrote:
>
>> * ma/config-page-only-in-list-mode (2018-02-21) 3 commits
>>  - config: change default of `pager.config` to "on"
>>  - config: respect `pager.config` in list/get-mode only
>>  - t7006: add tests for how git config paginates
>>
>>  In a way similar to how "git tag" learned to honor the pager
>>  setting only in the list mode, "git config" learned to ignore the
>>  pager setting when it is used for setting values (i.e. when the
>>  purpose of the operation is not to "show").
>>
>>  Is this ready for 'next'?
>
> I am not aware of any open questions or issues. You thought out loud
> about how the series was structured, in particular about introducing a
> successful test, then redefining it, as opposed to introducing it as a
> failing test, then making it succeed. I hope I managed to motivate my
> choice better in v2 (which is what you have picked up).
>
> Duy wondered if it was sane to use a pager when we know that we are
> "--get"-ing at most one config item. In v2, I addressed this by turning
> on paging for a more careful selection of "--get"-ters.

Yeah I got busy with stuff and didn't look at it. I've just checked what's in 'pu'. Looks good to me.

-- 
Duy
Junio C Hamano· Mar 9, 2018, 17:19 UTC · re: Martin Ågren · lore

Re: What's cooking in git.git (Mar 2018, #02; Tue, 6)

Martin Ågren <martin.agren@gmail.com> writes:
Show 11 quoted lines
>>  Is this ready for 'next'?
>
> I am not aware of any open questions or issues. You thought out loud
> about how the series was structured, in particular about introducing a
> successful test, then redefining it, as opposed to introducing it as a
> failing test, then making it succeed. I hope I managed to motivate my
> choice better in v2 (which is what you have picked up).
>
> Duy wondered if it was sane to use a pager when we know that we are
> "--get"-ing at most one config item. In v2, I addressed this by turning
> on paging for a more careful selection of "--get"-ters.

Yeah, I am aware of these exchanges, and they are resolved nicely, I think. I was mostly asking if other people have concerns we haven't thought of yet.

Let's merge this to 'next', then.
Thanks.

← back to recent threads