What's cooking in git.git (May 2018, #02; Thu, 17)

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.

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"]

* hn/sort-ls-remote (2018-04-09) 1 commit
(merged to 'next' on 2018-04-30 at 244ca5d30a)
+ ls-remote: create '--sort' option

"git ls-remote" learned an option to allow sorting its output based on the refnames being shown.

* sb/object-store-replace (2018-04-12) 15 commits
(merged to 'next' on 2018-04-25 at 9a213fb505)
+ replace-object: allow lookup_replace_object to handle arbitrary repositories
+ replace-object: allow do_lookup_replace_object to handle arbitrary repositories
+ replace-object: allow prepare_replace_object to handle arbitrary repositories
+ refs: allow for_each_replace_ref to handle arbitrary repositories
+ refs: store the main ref store inside the repository struct
+ replace-object: add repository argument to lookup_replace_object
+ replace-object: add repository argument to do_lookup_replace_object
+ replace-object: add repository argument to prepare_replace_object
+ refs: add repository argument to for_each_replace_ref
+ refs: add repository argument to get_main_ref_store
+ replace-object: check_replace_refs is safe in multi repo environment
+ replace-object: eliminate replace objects prepared flag
+ object-store: move lookup_replace_object to replace-object.h
+ replace-object: move replace_map to object store
+ replace_object: use oidmap
(this branch is used by sb/object-store-alloc and sb/oid-object-info.)

The effort to pass the repository in-core structure throughout the API continues. This round deals with the code that implements the refs/replace/ mechanism.

* ab/git-svn-get-record-typofix (2018-04-09) 1 commit (merged to 'next' on 2018-04-30 at 23f875f6b9)
+ git-svn: avoid warning on undef readline()

"git svn" had a minor thinko/typo which has been fixed.

* ab/nuke-emacs-contrib (2018-04-16) 1 commit (merged to 'next' on 2018-04-25 at 9b133d8a65)
+ git{,-blame}.el: remove old bitrotting Emacs code

The scripts in contrib/emacs/ have outlived their usefulness and have been replaced with a stub that errors out and tells the user there are replacements.

* ab/simplify-perl-makefile (2018-04-19) 2 commits (merged to 'next' on 2018-04-25 at 906cf21682)
+ Makefile: mark perllibdir as a .PHONY target (merged to 'next' on 2018-04-17 at 4448756934)
+ perl: fix installing modules from contrib

Recent simplification of build procedure forgot a bit of tweak to the build procedure of contrib/mw-to-git/

* bt/gpg-interface (2018-04-16) 7 commits (merged to 'next' on 2018-04-30 at 50c507b7d8)
+ gpg-interface: find the last gpg signature line
+ gpg-interface: extract gpg line matching helper
+ gpg-interface: fix const-correctness of "eol" pointer
+ gpg-interface: use size_t for signature buffer size
+ gpg-interface: modernize function declarations
+ gpg-interface: handle bool user.signingkey
+ t7004: fix mistaken tag name

What is queued here is only the obviously correct and uncontroversial code clean-up part, which is an earlier 7 patches, of a larger series.

The remainder that is not queued introduces a few configuration variables to deal with e-signature backends with different signature format.

* bw/protocol-v2 (2018-03-15) 35 commits
(merged to 'next' on 2018-04-11 at 23ee234a2c)
+ 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 prefixes when pushing
+ fetch: pass ref prefixes when fetching
+ ls-remote: pass ref prefixes when requesting a remote's refs
+ transport: convert transport_get_remote_refs to take a list of ref prefixes
+ transport: convert get_refs_list to take a list of ref prefixes
+ 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
(this branch is used by bw/server-options and jt/partial-clone-proto-v2.)

The beginning of the next-gen transfer protocol.

* dj/runtime-prefix (2018-04-24) 7 commits
(merged to 'next' on 2018-04-25 at e7e635a70e)
+ Makefile: quote $INSTLIBDIR when passing it to sed
+ Makefile: remove unused @@PERLLIBDIR@@ substitution variable (merged to 'next' on 2018-04-17 at a69aaa7a22)
+ mingw/msvc: use the new-style RUNTIME_PREFIX helper
+ exec_cmd: provide a new-style RUNTIME_PREFIX helper for Windows
+ exec_cmd: RUNTIME_PREFIX on some POSIX systems
+ Makefile: add Perl runtime prefix support
+ Makefile: generate Perl header from template file (this branch is tangled with js/runtime-prefix.)

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.

* ds/commit-graph (2018-04-11) 16 commits
(merged to 'next' on 2018-04-25 at 18af3d28d9)
+ commit-graph: implement "--append" option
+ 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 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
+ csum-file: refactor finalize_hashfile() method
+ csum-file: rename hashclose() to finalize_hashfile()
+ Merge branch 'jk/cached-commit-buffer' into HEAD
+ Merge branch 'jt/binsearch-with-fanout' into HEAD
(this branch is used by ds/generation-numbers and ds/lazy-load-trees.)

Precompute and store information necessary for ancestry traversal in a separate file to optimize graph walking.

Will merge to 'master'.

* jc/parseopt-expiry-errors (2018-04-23) 2 commits (merged to 'next' on 2018-04-30 at 637085f3d8)
+ parseopt: handle malformed --expire arguments more nicely
+ gc: do not upcase error message shown with die()

"git gc --prune=nonsense" spent long time repacking and then silently failed when underlying "git prune --expire=nonsense" failed to parse its command line. This has been corrected.

* js/colored-push-errors (2018-04-24) 4 commits (merged to 'next' on 2018-04-30 at 31076c52a2)
+ config: document the settings to colorize push errors/hints
+ push: test to verify that push errors are colored
+ push: colorize errors
+ color: introduce support for colorizing stderr

Error messages from "git push" can be painted for more visibility.

* js/empty-config-section-fix (2018-04-09) 15 commits (merged to 'next' on 2018-04-25 at 1690df3e5f)
+ git_config_set: reuse empty sections
+ git config --unset: remove empty sections (in the common case)
+ git_config_set: make use of the config parser's event stream
+ git_config_set: do not use a state machine
+ config_set_store: rename some fields for consistency
+ config: avoid using the global variable store
+ config: introduce an optional event stream while parsing
+ t1300: --unset-all can leave an empty section behind (bug)
+ t1300: add a few more hairy examples of sections becoming empty
+ t1300: remove unreasonable expectation from TODO
+ t1300: avoid relying on a bug
+ config --replace-all: avoid extra line breaks
+ t1300: demonstrate that --replace-all can "invent" newlines
+ t1300: rename it to reflect that repo-config was deprecated
+ git_config_set: fix off-by-two

"git config --unset a.b", when "a.b" is the last variable in an otherwise empty section "a", left an empty section "a" behind, and worse yet, a subsequent "git config a.c value" did not reuse that empty shell and instead created a new one. These have been (partially) corrected.

* js/ident-date-fix (2018-04-19) 1 commit (merged to 'next' on 2018-04-30 at d50ec2f4c1)
+ sequencer: reset the committer date before commits

During a "rebase -i" session, the code could give older timestamp to commits created by later "pick" than an earlier "reword", which has been corrected.

* js/runtime-prefix (2018-04-24) 8 commits
(merged to 'next' on 2018-04-30 at c6cfccf40e)
+ Avoid multiple PREFIX definitions
+ git_setup_gettext: plug memory leak
+ gettext: avoid initialization if the locale dir is not present (merged to 'next' on 2018-04-17 at a69aaa7a22)
+ mingw/msvc: use the new-style RUNTIME_PREFIX helper
+ exec_cmd: provide a new-style RUNTIME_PREFIX helper for Windows
+ exec_cmd: RUNTIME_PREFIX on some POSIX systems
+ Makefile: add Perl runtime prefix support
+ Makefile: generate Perl header from template file (this branch is tangled with dj/runtime-prefix.)

* ls/checkout-encoding (2018-04-16) 10 commits
(merged to 'next' on 2018-04-25 at e0f8554b2a)
+ 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
+ utf8: teach same_encoding() alternative UTF encoding names
+ strbuf: add a case insensitive starts_with()
+ 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).

* ma/double-dashes-in-docs (2018-04-18) 4 commits (merged to 'next' on 2018-04-25 at aaac2dc63c)
+ git-submodule.txt: quote usage in monospace, drop backslash
+ git-[short]log.txt: unify quoted standalone --
+ doc: convert [\--] to [--]
+ doc: convert \--option to --option

Doc formatting updates.

* ma/fast-export-skip-merge-fix (2018-04-21) 1 commit (merged to 'next' on 2018-04-30 at f7fca02ab1)
+ fast-export: fix regression skipping some merge-commits

"git fast-export" had a regression in v2.15.0 era where it skipped some merge commits in certain cases, which has been corrected.

* ma/http-walker-no-partial (2018-04-24) 2 commits (merged to 'next' on 2018-04-30 at 4582c99ba8)
+ walker: drop fields of struct walker which are always 1
+ http-fetch: make -a standard behavior

"git http-fetch" (deprecated) had an optional and experimental "feature" to fetch only commits and/or trees, which nobody used. This has been removed.

* nd/submodule-status-fix (2018-04-19) 1 commit (merged to 'next' on 2018-04-30 at 34d1f9ca83)
+ submodule--helper: don't print null in 'submodule status'

"git submodule status" did not check the symbolic revision name it computed for the submodule HEAD is not the NULL, and threw it at printf routines, which has been corrected.

* nd/warn-more-for-devs (2018-04-16) 4 commits
(merged to 'next' on 2018-04-25 at 2978e61414)
+ Makefile: add a DEVOPTS to get all of -Wextra
+ Makefile: add a DEVOPTS to suppress -Werror under DEVELOPER
+ Makefile: detect compiler and enable more warnings in DEVELOPER=1
+ connect.c: mark die_initial_contact() NORETURN

The build procedure "make DEVELOPER=YesPlease" learned to enable a bit more warning options depending on the compiler used to help developers more. There also is "make DEVOPTS=tokens" knob available now, for those who want to help fixing warnings we usually ignore, for example.

* ot/libify-get-ref-atom-value (2018-03-29) 6 commits (merged to 'next' on 2018-04-25 at 056bcaa69c)
+ ref-filter: libify get_ref_atom_value()
+ ref-filter: add return value to parsers
+ ref-filter: change parsing function error handling
+ ref-filter: add return value && strbuf to handlers
+ ref-filter: start adding strbufs with errors
+ ref-filter: add shortcut to work with strbufs

Code restructuring, in preparation for further work.

* sa/send-email-dedup-some-headers (2018-04-19) 1 commit (merged to 'next' on 2018-04-30 at 2a1fd8217e)
+ send-email: avoid duplicate In-Reply-To/References

When fed input that already has In-Reply-To: and/or References:
headers and told to add the same information, "git send-email" added these headers separately, instead of appending to an existing one, which is a violation of the RFC. This has been corrected.

* sb/submodule-move-nested (2018-03-29) 6 commits
(merged to 'next' on 2018-04-25 at 86b177433a)
+ submodule: fixup nested submodules after moving the submodule
+ submodule-config: remove submodule_from_cache
+ submodule-config: add repository argument to submodule_from_{name, path}
+ submodule-config: allow submodule_free to handle arbitrary repositories
+ grep: remove "repo" arg from non-supporting funcs
+ submodule.h: drop declaration of connect_work_tree_and_git_dir

Moving a submodule that itself has submodule in it with "git mv" forgot to make necessary adjustment to the nested sub-submodules; now the codepath learned to recurse into the submodules.

* sb/worktree-remove-opt-force (2018-04-18) 1 commit (merged to 'next' on 2018-04-25 at 0367d52a4b)
+ worktree: accept -f as short for --force for removal

"git worktree remove" learned that "-f" is a shorthand for "--force" option, just like for "git worktree add".

* sg/completion-clear-cached (2018-04-18) 1 commit (merged to 'next' on 2018-04-25 at 9178da6c3d)
+ completion: reduce overhead of clearing cached --options

The completion script (in contrib/) learned to clear cached list of command line options upon dot-sourcing it again in a more efficient way.

* sg/doc-gc-quote-mismatch-fix (2018-04-18) 1 commit (merged to 'next' on 2018-04-25 at bc3d1abf45)
+ docs/git-gc: fix minor rendering issue

Doc formatting fix.

* so/glossary-ancestor (2018-04-17) 1 commit
(merged to 'next' on 2018-04-25 at 0a849fee00)
+ glossary: substitute "ancestor" for "direct ancestor" in 'push' description.

Docfix.

* tb/config-default (2018-04-23) 3 commits
(merged to 'next' on 2018-04-25 at 59bb6beb2a)
+ builtin/config: introduce color type specifier
+ config.c: introduce 'git_config_color' to parse ANSI colors
+ builtin/config: introduce --default
(this branch uses tb/config-type.)

"git config --get" learned the "--default" option, to help the calling script. Building on top of the tb/config-type topic, the "git config" learns "--type=color" type. Taken together, you can do things like "git config --get foo.color --default blue" and get the ANSI color sequence for the color given to foo.color variable, or "blue" if the variable does not exist.

* tb/config-type (2018-04-19) 2 commits
(merged to 'next' on 2018-04-25 at fe69e93c82)
+ builtin/config.c: support --type=<type> as preferred alias for --<type>
+ builtin/config.c: treat type specifiers singularly
(this branch is used by tb/config-default.)

The "git config" command uses separate options e.g. "--int", "--bool", etc. to specify what type the caller wants the value to be interpreted as. A new "--type=<typename>" option has been introduced, which would make it cleaner to define new types.

* tg/demote-stash-save-in-completion (2018-04-20) 2 commits (merged to 'next' on 2018-04-30 at 93d0af5375)
+ completion: make stash -p and alias for stash push -p
+ completion: stop showing 'save' for stash by default

The command line completion (in contrib/) has been taught that "git stash save" has been deprecated ("git stash push" is the preferred spelling in the new world) and does not offer it as a possible completion candidate when "git stash push" can be.

* tq/t1510 (2018-04-17) 1 commit
(merged to 'next' on 2018-04-25 at 5710c81979)
+ t1510-repo-setup.sh: remove useless mkdir

Test cleanup.

* tz/doc-git-urls-reference (2018-04-20) 1 commit (merged to 'next' on 2018-04-30 at 39926c99fd)
+ doc/clone: update caption for GIT URLS cross-reference

Doc fix.

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

* hn/sort-ls-remote (2018-05-14) 1 commit
(merged to 'next' on 2018-05-16 at 64336850f2)
+ t5512: run git fetch inside test

Hotfix.

Will merge to 'master'.

* sb/object-store-replace (2018-05-10) 2 commits
(merged to 'next' on 2018-05-16 at 41bbedcc81)
+ replace-object.c: remove the_repository from prepare_replace_object
+ object.c: free replace map in raw_object_store_clear

Hotfix.

Will merge to 'master'.

* sg/t6500-no-redirect-of-stdin (2018-05-09) 1 commit
- t6050-replace: don't disable stdin for the whole test script

Test cleanup.

Will merge to 'next'.

* ao/config-api-doc (2018-05-11) 1 commit
- doc: fix config API documentation about config_with_options

Doc update.

Will merge to 'next'.

* bc/hash-independent-tests (2018-05-16) 28 commits
- t5300: abstract away SHA-1-specific constants
- t4208: abstract away SHA-1-specific constants
- t4045: abstract away SHA-1-specific constants
- t4042: abstract away SHA-1-specific constants
- t4205: sort log output in a hash-independent way
- t/lib-diff-alternative: abstract away SHA-1-specific constants
- t4030: abstract away SHA-1-specific constants
- t4029: abstract away SHA-1-specific constants
- t4029: fix test indentation
- t4022: abstract away SHA-1-specific constants
- t4020: abstract away SHA-1-specific constants
- t4014: abstract away SHA-1-specific constants
- t4008: abstract away SHA-1-specific constants
- t4007: abstract away SHA-1-specific constants
- t3905: abstract away SHA-1-specific constants
- t3702: abstract away SHA-1-specific constants
- t3103: abstract away SHA-1-specific constants
- t2203: abstract away SHA-1-specific constants
- t: skip pack tests if not using SHA-1
- t4044: skip test if not using SHA-1
- t1512: skip test if not using SHA-1
- t1007: annotate with SHA1 prerequisite
- t0000: annotate with SHA1 prerequisite
- t: switch $_x40 to $OID_REGEX
- t/test-lib: introduce OID_REGEX
- t: switch $_z40 to $ZERO_OID
- t/test-lib: introduce ZERO_OID
- t/test-lib: add an SHA1 prerequisite

Many tests hardcode the raw object names, which would change once we migrate away from SHA-1. While some of them must test against exact object names, most of them do not have to use hardcoded constants in the test. The latter kind of tests have been updated to test the moral equivalent of the original without hardcoding the actual object names.

Will merge to 'next'.

* bp/status-rename-config (2018-05-13) 1 commit
- add status config and command line options for rename detection (this branch uses em/status-rename-config.)

"git status" learned to honor a new status.renames configuration to skip rename detection, which could be useful for those who want to do so without disabling the default rename detection done by the "git diff" command.

Will merge to 'next'.

* bw/refspec-api (2018-05-16) 35 commits
- submodule: convert push_unpushed_submodules to take a struct refspec
- remote: convert check_push_refs to take a struct refspec
- remote: convert match_push_refs to take a struct refspec
- http-push: store refspecs in a struct refspec
- transport: remove transport_verify_remote_names
- send-pack: store refspecs in a struct refspec
- transport: convert transport_push to take a struct refspec
- push: convert to use struct refspec
- push: check for errors earlier
- remote: convert match_explicit_refs to take a struct refspec
- remote: convert get_ref_match to take a struct refspec
- remote: convert query_refspecs to take a struct refspec
- remote: convert apply_refspecs to take a struct refspec
- remote: convert get_stale_heads to take a struct refspec
- fetch: convert prune_refs to take a struct refspec
- fetch: convert get_ref_map to take a struct refspec
- fetch: convert do_fetch to take a struct refspec
- refspec: remove the deprecated functions
- fetch: convert refmap to use struct refspec
- fetch: convert fetch_one to use struct refspec
- transport-helper: convert to use struct refspec
- remote: convert fetch refspecs to struct refspec
- remote: convert push refspecs to struct refspec
- fast-export: convert to use struct refspec
- clone: convert cmd_clone to use refspec_item_init
- remote: convert match_push_refs to use struct refspec
- remote: convert check_push_refs to use struct refspec
- transport: convert transport_push to use struct refspec
- pull: convert get_tracking_branch to use refspec_item_init
- submodule--helper: convert push_check to use struct refspec
- refspec: convert valid_fetch_refspec to use parse_refspec
- refspec: introduce struct refspec
- refspec: rename struct refspec to struct refspec_item
- refspec: factor out parsing a single refspec
- refspec: move refspec parsing logic into its own file

An API update for handling of refspecs used by fetch & push codepath.

Will merge to 'next'.

* ds/commit-graph-lockfile-fix (2018-05-11) 1 commit
- commit-graph: fix UX issue when .lock file exists (this branch uses ds/generation-numbers and ds/lazy-load-trees.)

Update to ds/generation-numbers topic.

* jk/apply-p-doc (2018-05-11) 1 commit
- apply: clarify "-p" documentation

Doc update.

Will merge to 'next'.

* jk/unavailable-can-be-missing (2018-05-13) 4 commits
- mark_parents_uninteresting(): avoid most allocation
- mark_parents_uninteresting(): replace list with stack
- mark_parents_uninteresting(): drop missing object check
- mark_tree_contents_uninteresting(): drop missing object check

Code clean-up to turn history traversal more robust in a semi-corrupt repository.

Will merge to 'next'.

* lm/credential-netrc (2018-05-14) 2 commits
- git-credential-netrc: accept gpg option
- git-credential-netrc: adapt to test framework for git

Update credential-netrc helper (in contrib/) to allow customizing the GPG used to decrypt the encrypted .netrc file.

Will merge to 'next'.

* ma/create-pseudoref-with-null-old-oid (2018-05-13) 3 commits
- refs: handle zero oid for pseudorefs
- t1400: add tests around adding/deleting pseudorefs
- refs.c: refer to "object ID", not "sha1", in error messages

"git update-ref A B" is supposed to ensure that ref A does not yet exist when B is a NULL OID, but this check was not done correctly for pseudo-refs outside refs/ hierarchy, e.g. MERGE_HEAD.

Will merge to 'next'.

* ma/lockfile-cleanup (2018-05-10) 5 commits
- lock_file: move static locks into functions
- lock_file: make function-local locks non-static
- refs.c: do not die if locking fails in delete_pseudoref()
- refs.c: do not die if locking fails in write_pseudoref()
- t/helper/test-write-cache: clean up lock-handling

Code clean-up to adjust to a more recent lockfile API convention that allows lockfile instances kept on the stack.

Will merge to 'next'.

* nd/commit-util-to-slab (2018-05-14) 14 commits
- commit.h: delete 'util' field in struct commit
- merge: use commit-slab in merge remote desc instead of commit->util
- log: use commit-slab in prepare_bases() instead of commit->util
- show-branch: use commit-slab for commit-name instead of commit->util
- name-rev: use commit-slab for rev-name instead of commit->util
- bisect.c: use commit-slab for commit weight instead of commit->util
- revision.c: use commit-slab for show_source
- sequencer.c: use commit-slab to associate todo items to commits
- sequencer.c: use commit-slab to mark seen commits
- shallow.c: use commit-slab for commit depth instead of commit->util
- describe: use commit-slab for commit names instead of commit->util
- blame: use commit-slab for blame suspects instead of commit->util
- commit-slab: support shared commit-slab
- commit-slab.h: code split

The in-core "commit" object had an all-purpose "void *util" field, which was tricky to use especially in library-ish part of the code. All of the existing uses of the field has been migrated to a more dedicated "commit-slab" mechanism and the field is eliminated.

Will merge to 'next'.

* nd/diff-apply-ita (2018-05-14) 2 commits
- apply: add --intent-to-add
- diff: turn --ita-invisible-in-index on by default

"git diff" compares the index and the working tree. For paths added with intent-to-add bit, the command shows the full contents of them as added, but the paths themselves were not marked as new files. They are now shown as new by default.

"git apply" learned the "--intent-to-add" option so that an otherwise working-tree-only application of a patch will add new paths to the index marked with the "intent-to-add" bit.

Will merge to and cook in 'next'.

* nd/pack-format-doc (2018-05-13) 1 commit
- pack-format.txt: more details on pack file format

Doc update.

Will merge to 'next'.

* nd/pack-struct-commit (2018-05-13) 1 commit
- commit.h: rearrange 'index' to shrink struct commit

Memory optimization.

Will merge to 'next'.

* nd/repo-clear-keep-the-index (2018-05-10) 1 commit
- repository: fix free problem with repo_clear(the_repository)

the_repository->index is not a allocated piece of memory but repo_clear() indiscriminately attempted to free(3) it, which has been corrected.

Will merge to 'next'.

* nd/term-columns (2018-05-13) 2 commits
- column: fix off-by-one default width
- pager: set COLUMNS to term_columns()

The code did not propagate the terminal width to subprocesses via COLUMNS environment variable, which it now does. This caused trouble to "git column" helper subprocess when "git tag --column=row" tried to list the existing tags on a display with non-default width.

Will merge to 'next'.

* nd/travis-gcc-7 (2018-05-14) 1 commit
- travis-ci: run gcc-7 on linux-gcc jobs

Developer support. Use newer GCC on one of the builds done at TravisCI.org to get more warnings and errors diagnosed.

Will merge to 'next'.

* rs/no-null-ptr-arith-in-fast-export (2018-05-10) 1 commit
- fast-export: avoid NULL pointer arithmetic

Code clean-up to avoid non-standard-conformant pointer arithmetic.

Will merge to 'next'.

* sb/grep-die-on-unreadable-index (2018-05-16) 1 commit
- grep: handle corrupt index files early

* sb/object-store-grafts (2018-05-16) 19 commits
- commit: allow lookup_commit_graft to handle arbitrary repositories
- commit: allow prepare_commit_graft to handle arbitrary repositories
- shallow: migrate shallow information into the object parser
- path.c: migrate git_path_ to take a repository argument
- cache: convert get_graft_file to handle arbitrary repositories
- commit: convert read_graft_file to handle arbitrary repositories
- commit: convert register_commit_graft to handle arbitrary repositories
- commit: convert commit_graft_pos() to handle arbitrary repositories
- shallow: add repository argument to is_repository_shallow
- shallow: add repository argument to check_shallow_file_for_update
- shallow: add repository argument to register_shallow
- shallow: add repository argument to set_alternate_shallow_file
- commit: add repository argument to lookup_commit_graft
- commit: add repository argument to prepare_commit_graft
- commit: add repository argument to read_graft_file
- commit: add repository argument to register_commit_graft
- commit: add repository argument to commit_graft_pos
- object: move grafts to object parser
- object-store: move object access functions to object-store.h (this branch uses sb/object-store-alloc and sb/oid-object-info.)

* sb/submodule-merge-in-merge-recursive (2018-05-16) 3 commits
- merge-recursive: give notice when submodule commit gets fast-forwarded
- merge-recursive: i18n submodule merge output and respect verbosity
- submodule.c: move submodule merging to merge-recursive.c

By code restructuring of submodule merge in merge-recursive, informational messages from the codepath are now given using the same mechanism as other output, and honor the merge.verbosity configuration. The code also learned to give a few new messages when a submodule three-way merge resolves cleanly when one side records a descendant of the commit chosen by the other side.

Will merge to 'next'.

* sb/submodule-update-try-harder (2018-05-16) 1 commit
- git-submodule.sh: try harder to fetch a submodule

"git submodule update" attempts two different kinds of "git fetch" against the upstream repository to grab a commit bound at the submodule's path, but it incorrectly gave up if the first kind (i.e. a normal fetch) failed, making the second "last resort" one (i.e. fetching an exact commit object by object name) ineffective. This has been corrected.

Will merge to 'next'.

* sg/t5310-jgit-bitmap-test (2018-05-11) 1 commit
- t5310-pack-bitmaps: make JGit tests work with GIT_TEST_SPLIT_INDEX

Test update.

Will merge to 'next'.

* sg/t5516-fixes (2018-05-11) 2 commits
- t5516-fetch-push: fix broken &&-chain
- t5516-fetch-push: fix 'push with dry-run' test

Test fixes.

Will merge to 'next'.

* sg/t7005-spaces-in-filenames-cleanup (2018-05-15) 1 commit
- t7005-editor: get rid of the SPACES_IN_FILENAMES prereq

Test update.

Will merge to 'next'.

* tb/grep-only-matching (2018-05-14) 2 commits
- builtin/grep.c: teach '-o', '--only-matching' to 'git-grep'
- grep.c: extract show_line_header()
(this branch uses tb/grep-column.)

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

* jm/cache-entry-from-mem-pool (2018-05-02) 5 commits
- block alloc: add validations around cache_entry lifecyle
- block alloc: allocate cache entries from mem_pool
- mem-pool: fill out functionality
- block alloc: add lifecycle APIs for cache_entry structs
- read-cache: teach refresh_cache_entry() to take istate

For a large tree, the index needs to hold many cache entries allocated on heap. These cache entries are now allocated out of a dedicated memory pool to amortize malloc(3) overhead.

Needs review.
Is the "caller always knows which pool an entry came from and calls the right kind of free" a feasible approach?
Oh, another thing, there is "lifecyle" typo in the title.

* ab/fetch-tags-noclobber (2018-05-16) 9 commits
- fixup! push tests: assert re-pushing annotated tags
- fetch: stop clobbering existing tags without --force
- fetch tests: add a test clobbering tag behavior
- fetch tests: correct a comment "remove it" -> "remove them"
- push doc: correct lies about how push refspecs work
- push tests: assert re-pushing annotated tags
- push tests: add more testing for forced tag pushing
- push tests: fix logic error in "push" test assertion
- push tests: remove redundant 'git push' invocation

Expecting a reboot of the discussion to take it to some conclusion and then a reroll.
cf. <[email protected]> cf. <[email protected]>
cf. <[email protected]>
cf. <[email protected]>

* pw/add-p-select (2018-03-16) 3 commits
- add -p: optimize line selection for short hunks
- add -p: allow line selection to be inverted
- add -p: select individual hunk lines

"git add -p" interactive interface learned to let users choose individual added/removed lines to be used in the operation, instead of accepting or rejecting a whole hunk.

Expecting a reroll to reignite the discussion. cf. <[email protected]>

* jh/json-writer (2018-03-28) 1 commit
- json_writer: new routines to create data in JSON format

Preparatory code to later add json output for unspecified telemetry data.

We do not add random code that does not have real users to our codebase, so let's have it wait until such a real code materializes before too long.

* hn/bisect-first-parent (2018-04-21) 1 commit
- bisect: create 'bisect_flags' parameter in find_bisection()

Preliminary code update to allow passing more flags down the bisection codepath in the future.

We do not add random code that does not have real users to our codebase, so let's have it wait until such a real code materializes before too long.

* is/parsing-line-range (2018-04-27) 2 commits
. log: prevent error if line range ends past end of file . blame: prevent error if range ends past end of file

Parsing of -L[<N>][,[<M>]] parameters "git blame" and "git log" take has been tweaked.

Seems to break a few tests.

* 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>

* 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. <[email protected]>

* 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).

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

* ld/p4-unshelve (2018-05-16) 1 commit
- git-p4: add unshelve command

"git p4" learned to "unshelve" shelved commit from P4.

Will merge to 'next'.

* pc/submodule-helper-foreach (2018-05-11) 4 commits
- submodule: port submodule subcommand 'foreach' from shell to C
- submodule foreach: document variable '$displaypath'
- submodule foreach: document '$sm_path' instead of '$path'
- submodule foreach: correct '$path' in nested submodules from a subdirectory

The bulk of "git submodule foreach" has been rewritten in C.

Will merge to and cook in 'next'.

* tb/test-apfs-utf8-normalization (2018-05-02) 1 commit (merged to 'next' on 2018-05-16 at feabe72b42)
+ test: correct detection of UTF8_NFD_TO_NFC for APFS

A test to see if the filesystem normalizes UTF-8 filename has been updated to check what we need to know in a more direct way, i.e. a path created in NFC form can be accessed with NFD form (or vice versa) to cope with APFS as well as HFS.

Will merge to 'master'.

* ab/get-short-oid (2018-05-11) 5 commits
- get_short_oid: sort ambiguous objects by type, then SHA-1
- sha1-name.c: move around the collect_ambiguous() function
- git-p4: change "commitish" typo to "committish"
- sha1-array.h: align function arguments
- sha1-name.c: remove stray newline

When a short hexadecimal string is used to name an object but there are multiple objects that share the string as the prefix of their names, the code lists these ambiguous candidates in a help message. These object names are now sorted according to their types for easier eyeballing.

Will merge to 'next'.

* ah/misc-doc-updates (2018-05-06) 7 commits (merged to 'next' on 2018-05-16 at e2e3b68a66)
+ doc: normalize [--options] to [options] in git-diff
+ doc: add note about shell quoting to revision.txt
+ git-svn: remove ''--add-author-from' for 'commit-diff'
+ doc: add '-d' and '-o' for 'git push'
+ doc: clarify ignore rules for git ls-files
+ doc: align 'diff --no-index' in text and synopsis
+ doc: improve formatting in githooks.txt

Misc doc fixes.

Will merge to 'master'.

* bc/format-patch-cover-no-attach (2018-05-02) 1 commit (merged to 'next' on 2018-05-16 at fa1ffeb3fe)
+ format-patch: make cover letters always text/plain

"git format-patch --cover --attach" created a broken MIME multipart message for the cover letter, which has been fixed by keeping the cover letter as plain text file.

Will merge to 'master'.

* bp/test-drop-caches (2018-05-04) 1 commit
(merged to 'next' on 2018-05-16 at 0e40ab2408)
+ test-drop-caches: simplify delay loading of NtSetSystemInformation

Code simplification.

Will merge to 'master'.

* cc/perf-bisect (2018-05-06) 1 commit
(merged to 'next' on 2018-05-16 at 5a078a2fdf)
+ perf/bisect_run_script: disable codespeed

Performance test updates.

Will merge to 'master'.

* cf/submodule-progress-dissociate (2018-05-04) 3 commits
- submodule: add --dissociate option to add/update commands
- submodule: add --progress option to add command
- submodule: clean up subsititions in script

"git submodule update" and "git submodule add" supported the "--reference" option to borrow objects from a neighbouring local repository like "git clone" does, but lacked the more recent invention "--dissociate". Also "git submodule add" has been taught to take the "--progress" option.

Will merge to 'next'..
"subsititions" needs to be typofixed ;-)

* dd/send-email-reedit (2018-05-06) 1 commit
- git-send-email: allow re-editing of message

"git send-email" can sometimes offer confirmation dialog "Send this email?" with choices 'Yes', 'No', 'Quit', and 'All'. A new action 'Edit' has been added to this dialog's choice.

Will merge to 'next'.

* em/status-rename-config (2018-05-06) 1 commit (merged to 'next' on 2018-05-16 at 33c1cc093c)
+ wt-status: use settings from git_diff_ui_config (this branch is used by bp/status-rename-config.)

"git status" learned to pay attention to UI related diff configuration variables such as diff.renames.

Will merge to 'master'.

* js/branch-diff (2018-05-16) 19 commits
- fixup! Add a function to solve least-cost assignment problems
- completion: support branch-diff
- branch-diff: add a man page
- branch-diff --dual-color: work around bogus white-space warning
- branch-diff: offer to dual-color the diffs
- diff: add an internal option to dual-color diffs of diffs
- color: provide inverted colors, too
- branch-diff: use color for the commit pairs
- branch-diff: add tests
- branch-diff: do not show "function names" in hunk headers
- branch-diff: adjust the output of the commit pairs
- branch-diff: suppress the diff headers
- branch-diff: indent the diffs just like tbdiff
- branch-diff: right-trim commit messages
- branch-diff: also show the diff between patches
- branch-diff: improve the order of the shown commits
- branch-diff: first rudimentary implementation
- Add a new builtin: branch-diff
- Add a function to solve least-cost assignment problems

"git tbdiff" that lets us compare individual patches in two iterations of a topic has been rewritten and made into a built-in command.

Expecting a reroll.
cf. <[email protected]>

* js/sequencer-and-root-commits (2018-05-06) 6 commits
- rebase --rebase-merges: root commits can be cousins, too
- rebase --rebase-merges: a "merge" into a new root is a fast-forward
- sequencer: allow introducing new root commits
- rebase -i --root: let the sequencer handle even the initial part
- sequencer: learn about the special "fake root commit" handling
- sequencer: extract helper to update active_cache_tree (this branch uses js/rebase-recreate-merge.)

The implementation of "git rebase -i --root" has been updated to use the sequencer machinery more.

Will merge to 'next'.

* js/use-bug-macro (2018-05-10) 5 commits
- BUG_exit_code: fix sparse "symbol not declared" warning
- Convert remaining die*(BUG) messages
- Replace all die("BUG: ...") calls by BUG() ones
- run-command: use BUG() to report bugs, not die()
- test-tool: help verifying BUG() code paths

Developer support update, by using BUG() macro instead of die() to mark codepaths that should not happen more clearly.

Will merge to 'next'.
Further updates can come on top later.
cf. <[email protected]>

* jt/partial-clone-proto-v2 (2018-05-06) 4 commits
- {fetch,upload}-pack: support filter in protocol v2
- upload-pack: read config when serving protocol v2
- upload-pack: fix error message typo
- Merge branch 'bw/protocol-v2' into jt/partial-clone-proto-v2

Transfer protocol v2 learned to support the partial clone.

Will merge to 'next'.

* ma/doc-expand-tabs (2018-05-02) 1 commit
- revisions.txt: expand tabs to spaces in diagram

Fix one instance of asciidoctor's misformatting by expanding a tab into spaces in a literal block.

Will discard. This approach is less maintainable than the approach taken by bc/asciidoctor-tab-width topic.

* nd/completion-aliasfiletype-typofix (2018-05-06) 1 commit (merged to 'next' on 2018-05-16 at 045e4ac190)
+ completion: fix misspelled config key aliasesfiletype

Typofix.

Will merge to 'master'.

* nd/doc-header (2018-05-02) 1 commit
(merged to 'next' on 2018-05-16 at 0599eb3ec9)
+ doc: keep first level section header in upper case

Doc formatting fix.

Will merge to 'master'.

* nd/pack-unreachable-objects-doc (2018-05-06) 1 commit (merged to 'next' on 2018-05-16 at c4bf977564)
+ pack-objects: validation and documentation about unreachable options

Doc update.

Will merge to 'master'.

* sb/object-store-alloc (2018-05-16) 13 commits
- alloc: allow arbitrary repositories for alloc functions
- object: allow create_object to handle arbitrary repositories
- object: allow grow_object_hash to handle arbitrary repositories
- alloc: add repository argument to alloc_commit_index
- alloc: add repository argument to alloc_report
- alloc: add repository argument to alloc_object_node
- alloc: add repository argument to alloc_tag_node
- alloc: add repository argument to alloc_commit_node
- alloc: add repository argument to alloc_tree_node
- alloc: add repository argument to alloc_blob_node
- object: add repository argument to grow_object_hash
- object: add repository argument to create_object
- repository: introduce parsed objects field
(this branch is used by sb/object-store-grafts; uses sb/oid-object-info.)

The conversion to pass "the_repository" and then "a_repository" throughout the object access API continues.

* tb/grep-column (2018-05-14) 7 commits
- contrib/git-jump/git-jump: jump to match column in addition to line
- grep.c: add configuration variables to show matched option
- builtin/grep.c: add '--column' option to 'git-grep(1)'
- grep.c: display column number of first match
- grep.[ch]: extend grep_opt to allow showing matched column
- grep.c: expose matched column in match_line()
- Documentation/config.txt: camel-case lineNumber for consistency (this branch is used by tb/grep-only-matching.)

"git grep" learned the "--column" option that gives not just the line number but the column number of the hit.

* bc/asciidoctor-tab-width (2018-05-07) 2 commits (merged to 'next' on 2018-05-16 at be2a42c473)
+ Documentation: render revisions correctly under Asciidoctor
+ Documentation: use 8-space tabs with Asciidoctor

Asciidoctor gives a reasonable imitation for AsciiDoc, but does not render illustration in a literal block correctly when indented with HT by default. The problem is fixed by forcing 8-space tabs.

Will merge to 'master'.

* bc/mailmap-self (2018-05-08) 1 commit (merged to 'next' on 2018-05-16 at a009c64bd2)
+ mailmap: update brian m. carlson's email address

Will merge to 'master'.

* cc/perf-aggregate-unknown-option (2018-04-26) 1 commit (merged to 'next' on 2018-05-08 at db7d2870f8)
+ perf/aggregate: use Getopt::Long for option parsing

Perf-test helper updates.

Will merge to 'master'.

* ab/perl-python-attrs (2018-04-27) 3 commits
(merged to 'next' on 2018-05-08 at b440e9bbb9)
+ .gitattributes: add a diff driver for Python
+ .gitattributes: use the "perl" differ for Perl
+ .gitattributes: add *.pl extension for Perl

We learned that our source files with ".pl" and ".py" extensions are Perl and Python files respectively and changes to them are better viewed as such with appropriate diff drivers.

Will merge to 'master'.

* js/test-unset-prereq (2018-04-30) 1 commit (merged to 'next' on 2018-05-08 at 3aecbf25a3)
+ tests: introduce test_unset_prereq, for debugging

Test debugging aid.

Will merge to 'master'.
cf. <[email protected]>

* fg/completion-external (2018-05-07) 1 commit (merged to 'next' on 2018-05-16 at 5d83f92caf)
+ completion: load completion file for external subcommand

The command line completion mechanism (in contrib/) learned to load custom completion file for "git $command" where $command is a custom "git-$command" that the end user has on the $PATH when using newer version of bash.

Will merge to 'master'.
cf. <CAM0VKjkTu+OkLM3gvX73mWugxArCVmqRBmWGHiKuLiLRNkkNow@mail.gmail.com>

* js/deprecate-grafts (2018-04-30) 12 commits
(merged to 'next' on 2018-05-08 at 1d7b31d179)
+ Remove obsolete script to convert grafts to replace refs
+ technical/shallow: describe why shallow cannot use replace refs
+ technical/shallow: stop referring to grafts
+ filter-branch: stop suggesting to use grafts
+ Deprecate support for .git/info/grafts
+ Add a test for git replace --convert-graft-file
+ replace: introduce --convert-graft-file
+ replace: prepare create_graft() for converting graft files wholesale
+ replace: "libify" create_graft() and callees
+ replace: avoid using die() to indicate a bug
+ commit: Let the callback of for_each_mergetag return on error
+ argv_array: offer to split a string by whitespace

The functionality of "$GIT_DIR/info/grafts" has been superseded by the "refs/replace/" mechanism for some time now, but the internal code had support for it in many places, which has been cleaned up in order to drop support of the "grafts" mechanism.

Will merge to 'master'.

* js/rebase-i-clean-msg-after-fixup-continue (2018-05-02) 4 commits (merged to 'next' on 2018-05-08 at 7e684c153d)
+ rebase --skip: clean up commit message after a failed fixup/squash
+ sequencer: always commit without editing when asked for
+ rebase -i: Handle "combination of <n> commits" with GETTEXT_POISON
+ rebase -i: demonstrate bugs with fixup!/squash! commit messages

"git rebase -i" sometimes left intermediate "# This is a combination of N commits" message meant for the human consumption inside an editor in the final result in certain corner cases, which has been fixed.

Will merge to 'master'.

* bw/server-options (2018-04-24) 4 commits (merged to 'next' on 2018-05-08 at a18ce56f3c)
+ fetch: send server options when using protocol v2
+ ls-remote: send server options when using protocol v2
+ serve: introduce the server-option capability
+ Merge branch 'bw/protocol-v2' into HEAD

The transport protocol v2 is getting updated further.

Will merge to 'master'.

* bc/object-id (2018-05-02) 42 commits
- merge-one-file: compute empty blob object ID
- add--interactive: compute the empty tree value
- Update shell scripts to compute empty tree object ID
- sha1_file: only expose empty object constants through git_hash_algo
- dir: use the_hash_algo for empty blob object ID
- sequencer: use the_hash_algo for empty tree object ID
- cache-tree: use is_empty_tree_oid
- sha1_file: convert cached object code to struct object_id
- builtin/reset: convert use of EMPTY_TREE_SHA1_BIN
- builtin/receive-pack: convert one use of EMPTY_TREE_SHA1_HEX
- wt-status: convert two uses of EMPTY_TREE_SHA1_HEX
- submodule: convert several uses of EMPTY_TREE_SHA1_HEX
- sequencer: convert one use of EMPTY_TREE_SHA1_HEX
- merge: convert empty tree constant to the_hash_algo
- builtin/merge: switch tree functions to use object_id
- builtin/am: convert uses of EMPTY_TREE_SHA1_BIN to the_hash_algo
- sha1-file: add functions for hex empty tree and blob OIDs
- builtin/receive-pack: avoid hard-coded constants for push certs
- diff: specify abbreviation size in terms of the_hash_algo
- upload-pack: replace use of several hard-coded constants
- revision: replace use of hard-coded constants
- http: eliminate hard-coded constants
- dir: convert struct untracked_cache_dir to object_id
- commit: convert uses of get_sha1_hex to get_oid_hex
- index-pack: abstract away hash function constant
- pack-redundant: convert linked lists to use struct object_id
- Update struct index_state to use struct object_id
- split-index: convert struct split_index to object_id
- submodule-config: convert structures to object_id
- fsck: convert static functions to struct object_id
- tree-walk: convert get_tree_entry_follow_symlinks to object_id
- tree-walk: avoid hard-coded 20 constant
- pack-redundant: abstract away hash algorithm
- pack-objects: abstract away hash algorithm
- packfile: abstract away hash constant values
- packfile: convert find_pack_entry to object_id
- sha1-file: convert freshen functions to object_id
- packfile: convert has_sha1_pack to object_id
- packfile: remove unused member from struct pack_entry
- Remove unused member in struct object_context
- server-info: remove unused members from struct pack_info
- cache: add a function to read an object ID from a buffer

Conversion from uchar[20] to struct object_id continues.

Will merge to 'next'.

* sb/oid-object-info (2018-04-26) 9 commits
(merged to 'next' on 2018-05-08 at f3c08f298e)
+ cache.h: allow oid_object_info to handle arbitrary repositories
+ packfile: add repository argument to cache_or_unpack_entry
+ packfile: add repository argument to unpack_entry
+ packfile: add repository argument to read_object
+ packfile: add repository argument to packed_object_info
+ packfile: add repository argument to packed_to_object_type
+ packfile: add repository argument to retry_bad_packed_offset
+ cache.h: add repository argument to oid_object_info
+ cache.h: add repository argument to oid_object_info_extended (this branch is used by sb/object-store-alloc and sb/object-store-grafts.)

The codepath around object-info API has been taught to take the repository object (which in turn tells the API which object store the objects are to be located).

Will merge to 'master'.

* en/unpack-trees-split-index-fix (2018-05-02) 1 commit (merged to 'next' on 2018-05-16 at 1adff065b2)
+ unpack_trees: fix breakage when o->src_index != o->dst_index

The split-index feature had a long-standing and dormant bug in certain use of the in-core merge machinery, which has been fixed.

Will merge to 'master'.
cf. <CACsJy8CeDhrT9GXe9q5gqsAeq_sSQ8jyF2nMOFxzjwKtE31oPQ@mail.gmail.com>

* bp/merge-rename-config (2018-05-08) 3 commits
- merge: pass aggressive when rename detection is turned off
- merge: add merge.renames config setting
- merge: update documentation for {merge,diff}.renameLimit (this branch uses en/rename-directory-detection-reboot.)

With merge.renames configuration set to false, the recursive merge strategy can be told not to spend cycles trying to find renamed paths and merge them accordingly.

Will merge to 'next'.

* en/git-debugger (2018-04-25) 1 commit
(merged to 'next' on 2018-05-08 at 73369cd1e5)
+ Make running git under other debugger-like programs easy

Dev support.

Will merge to 'master'.

* js/no-pager-shorthand (2018-05-04) 1 commit
(merged to 'next' on 2018-05-08 at 10e6031dd1)
+ git: add -P as a short option for --no-pager

"git --no-pager cmd" did not have short-and-sweet single letter option. Now it does.

Will merge to 'master'.

* sb/diff-color-move-more (2018-04-25) 7 commits
- diff.c: add --color-moved-ignore-space-delta option
- diff.c: decouple white space treatment from move detection algorithm
- diff.c: add a blocks mode for moved code detection
- diff.c: adjust hash function signature to match hashmap expectation
- diff.c: do not pass diff options as keydata to hashmap
- xdiff/xdiffi.c: remove unneeded function declarations
- xdiff/xdiff.h: remove unused flags

"git diff --color-moved" feature has further been tweaked.

Will merge to 'next'.

* ds/generation-numbers (2018-05-02) 11 commits
- commit-graph.txt: update design document
- merge: check config before loading commits
- commit: use generation number in remove_redundant()
- commit: add short-circuit to paint_down_to_common()
- commit: use generation numbers for in_merge_bases()
- ref-filter: use generation number for --contains
- commit-graph: always load commit-graph information
- commit: use generations in paint_down_to_common()
- commit-graph: compute generation numbers
- commit: add generation number to struct commmit
- ref-filter: fix outdated comment on in_commit_list
(this branch is used by ds/commit-graph-lockfile-fix; uses ds/lazy-load-trees.)

A recently added "commit-graph" datafile has learned to store pre-computed generation numbers to speed up the decisions to stop history traversal.

Is this ready for 'next' with ds/commit-graph-lockfile-fix? A commit with triple 'm' needs its title amended, though.

* en/rename-directory-detection-reboot (2018-05-08) 36 commits (merged to 'next' on 2018-05-08 at be350ebc17)
+ merge-recursive: fix check for skipability of working tree updates
+ merge-recursive: make "Auto-merging" comment show for other merges
+ merge-recursive: fix remainder of was_dirty() to use original index
+ merge-recursive: fix was_tracked() to quit lying with some renamed paths
+ t6046: testcases checking whether updates can be skipped in a merge
+ merge-recursive: avoid triggering add_cacheinfo error with dirty mod
+ merge-recursive: move more is_dirty handling to merge_content
+ merge-recursive: improve add_cacheinfo error handling
+ 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
(this branch is used by bp/merge-rename-config.)

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. Incidentally, this also avoids updating a file in the working tree after a (non-trivial) merge whose result matches what our side originally had.

Will merge to 'master'.

* nd/command-list (2018-05-10) 13 commits
- completion: allow to customize the completable command list
- completion: let git provide the completable command list
- command-list.txt: documentation and guide line
- help: use command-list.txt for the source of guides
- help: add "-a --verbose" to list all commands with synopsis
- git: support --list-cmds=list-<category>
- completion: implement and use --list-cmds=main,others
- git --list-cmds: collect command list in a string_list
- git.c: convert --list-* to --list-cmds=*
- Remove common-cmds.h
- help: use command-list.h for common command list
- generate-cmds.sh: export all commands to command-list.h
- generate-cmds.sh: factor out synopsis extract code

The list of commands with their various attributes were spread across a few places in the build procedure, but it now is getting a bit more consolidated to allow more automation.

Will merge to 'next'.

* sg/complete-paths (2018-04-17) 11 commits
(merged to 'next' on 2018-05-08 at 2a11444f90)
+ completion: fill COMPREPLY directly when completing paths
+ completion: improve handling quoted paths in 'git ls-files's output
+ completion: remove repeated dirnames with 'awk' during path completion
+ t9902-completion: ignore COMPREPLY element order in some tests
+ completion: use 'awk' to strip trailing path components
+ completion: let 'ls-files' and 'diff-index' filter matching paths
+ completion: improve handling quoted paths on the command line
+ completion: support completing non-ASCII pathnames
+ completion: simplify prefix path component handling during path completion
+ completion: move __git_complete_index_file() next to its helpers
+ t9902-completion: add tests demonstrating issues with quoted pathnames

Command line completion (in contrib/) learned to complete pathnames for various commands better.

Will merge to 'master'.

* sb/blame-color (2018-04-24) 3 commits
- builtin/blame: add new coloring scheme config
- builtin/blame: highlight recently changed lines
- builtin/blame: dim uninteresting metadata lines

"git blame" learns to unhighlight uninteresting metadata from the originating commit on lines that are the same as the previous one, and also paint lines in different colors depending on the age of the commit.

Will merge to 'next'.

* ds/lazy-load-trees (2018-05-02) 6 commits
(merged to 'next' on 2018-05-02 at d54016d9e3)
+ coccinelle: avoid wrong transformation suggestions from commit.cocci (merged to 'next' on 2018-04-25 at b90813f421)
+ commit-graph: lazy-load trees for commits
+ treewide: replace maybe_tree with accessor methods
+ commit: create get_commit_tree() method
+ treewide: rename tree to maybe_tree
+ Merge branch 'bw/c-plus-plus' into ds/lazy-load-trees
(this branch is used by ds/commit-graph-lockfile-fix and ds/generation-numbers.)

The code has been taught to use the duplicated information stored in the commit-graph file to learn the tree object name for a commit to avoid opening and parsing the commit object when it makes sense to do so.

Will merge to 'master'.

* jk/branch-l-0-deprecation (2018-03-26) 3 commits
(merged to 'next' on 2018-04-11 at 9b2b0305dd)
+ branch: deprecate "-l" option
+ t: switch "branch -l" to "branch --create-reflog"
+ t3200: unset core.logallrefupdates when testing reflog creation (this branch is used by jk/branch-l-1-removal and jk/branch-l-2-reincarnation.)

The "-l" option in "git branch -l" is an unfortunate short-hand for "--create-reflog", but many users, both old and new, somehow expect it to be something else, perhaps "--list". This step deprecates the short-hand and warns about the future removal of the it when it is used.

Will cook in 'next'.
Perhaps merge to 'master' immediately after 2.18 release?

* jk/branch-l-1-removal (2018-03-26) 1 commit
- branch: drop deprecated "-l" option
(this branch is used by jk/branch-l-2-reincarnation; uses jk/branch-l-0-deprecation.)

Following the "git branch -l" deprecation, the short-hand is removed.

Will keep in 'pu'.

* jk/branch-l-2-reincarnation (2018-03-26) 1 commit
- branch: make "-l" a synonym for "--list"
(this branch uses jk/branch-l-0-deprecation and jk/branch-l-1-removal.)

Following the "git branch -l" removal, "-l" is resurrected as a short-hand for "--list".

Will keep in 'pu'.

* nd/pack-objects-pack-struct (2018-04-16) 15 commits (merged to 'next' on 2018-05-16 at 171459475d)
+ ci: exercise the whole test suite with uncommon code in pack-objects
+ pack-objects: reorder members to shrink struct object_entry
+ pack-objects: shrink delta_size field in struct object_entry
+ pack-objects: shrink size field in struct object_entry
+ pack-objects: clarify the use of object_entry::size
+ pack-objects: don't check size when the object is bad
+ pack-objects: shrink z_delta_size field in 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: 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: a bit of document about struct object_entry
+ read-cache.c: make $GIT_TEST_SPLIT_INDEX boolean

"git pack-objects" needs to allocate tons of "struct object_entry" while doing its work, and shrinking its size helps the performance quite a bit.

Will merge to 'master'.
cf. <CACsJy8CuVRy4UPEwXJJYAjePEz5zjKMLhRjh9UFw0DPYTzobkw@mail.gmail.com>

* nd/repack-keep-pack (2018-04-16) 7 commits
(merged to 'next' on 2018-05-08 at ab906be358)
+ pack-objects: show some progress when counting kept objects
+ gc --auto: exclude base pack if not enough mem to "repack -ad"
+ gc: handle a corner case in gc.bigPackThreshold
+ gc: add gc.bigPackThreshold config
+ gc: add --keep-largest-pack option
+ repack: add --keep-pack option
+ t7700: have closing quote of a test at the beginning of line

"git gc" in a large repository takes a lot of time as it considers to repack all objects into one pack by default. The command has been taught to pretend as if the largest existing packfile is marked with ".keep" so that it is left untouched while objects in other packs and loose ones are repacked.

Will merge to 'master'.
cf. <CACsJy8CuVRy4UPEwXJJYAjePEz5zjKMLhRjh9UFw0DPYTzobkw@mail.gmail.com>

* tg/worktree-add-existing-branch (2018-04-30) 4 commits (merged to 'next' on 2018-05-08 at 8b76505192)
+ worktree: teach "add" to check out existing branches
+ worktree: factor out dwim_branch function
+ worktree: improve message when creating a new worktree
+ worktree: remove extra members from struct add_opts

"git worktree add" learned to check out an existing branch.

Will merge to 'master'.

* js/rebase-recreate-merge (2018-04-26) 17 commits
(merged to 'next' on 2018-05-16 at f1aeea2879)
+ rebase -i --rebase-merges: add a section to the man page
+ rebase -i: introduce --rebase-merges=[no-]rebase-cousins
+ pull: accept --rebase=merges to recreate the branch topology
+ rebase --rebase-merges: avoid "empty merges"
+ sequencer: handle post-rewrite for merge commands
+ sequencer: make refs generated by the label command worktree-local
+ rebase --rebase-merges: add test for --keep-empty
+ rebase: introduce the --rebase-merges option
+ rebase-helper --make-script: introduce a flag to rebase merges
+ sequencer: fast-forward merge commands, if possible
+ sequencer: introduce the merge command
+ sequencer: introduce new commands to reset the revision
+ git-rebase--interactive: clarify arguments
+ sequencer: offer helpful advice when a command was rescheduled
+ sequencer: refactor how original todo list lines are accessed
+ sequencer: make rearrange_squash() a bit more obvious
+ sequencer: avoid using errno clobbered by rollback_lock_file() (this branch is used by js/sequencer-and-root-commits.)

"git rebase" learned "--rebase-merges" to transplant the whole topology of commit graph elsewhere.

Will merge to 'master'.

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

* js/runtime-prefix-windows (2018-03-27) 5 commits
. mingw/msvc: use the new-style RUNTIME_PREFIX helper . exec_cmd: provide a new-style RUNTIME_PREFIX helper for Windows . exec_cmd: RUNTIME_PREFIX on some POSIX systems
. Makefile: add Perl runtime prefix support
. Makefile: generate Perl header from template file

The Windows port was the first that allowed Git to be installed anywhere by having its components refer to each other with relative pathnames. The recent dj/runtime-prefix topic extends the idea to other platforms, and its approach has been adopted back in the Windows port.

Ejected, as the parent topic dj/runtime-prefix covers Windows now.

* bp/fsexcludes (2018-04-16) 2 commits
. fsmonitor: switch to use new fsexcludes logic and remove unused untracked cache based logic . fsexcludes: add a programmatic way to exclude files from git's working directory traversal logic

Can we have a few lines summary here, just like we have for other topic ;-) I personally take the overlong title of these commits as a sign that they can further be simplified and cleaned up by splitting, focusing the scope, etc.

Retracted.
cf. <[email protected]>

I still have a slight feeling that we shouldn't list the branches for "git branch -l" during the deprecation period. If feel this because

i) It would avoid confusions for the users during the deprecation period

ii) The warning message seems to add to the confusion:

$ git branch -l
warning: the '-l' alias for '--create-reflog' is deprecated;
warning: it will be removed in a future version of Git
* master
...

If there are ample branches, the warning message might be hidden out of screen but we shouldn't rely on that, I suppose.

Sivaraam

QUOTE:

“The most valuable person on any team is the person who makes everyone
else on the team more valuable, not the person who knows the most.”

      - John Sonmez


Sivaraam?

You possibly might have noticed that my signature recently changed from
'Kaartic' to 'Sivaraam' both of which are parts of my name. I find the
new signature to be better for several reasons one of which is that the
former signature has a lot of ambiguities in the place I live as it is a
common name (NOTE: it's not a common spelling, just a common name). So,
I switched signatures before it's too late.

That said, I won't mind you calling me 'Kaartic' if you like it [of
course ;-)]. You can always call me using either of the names.


KIND NOTE TO THE NATIVE ENGLISH SPEAKER:

As I'm not a native English speaker myself, there might be mistaeks in
my usage of English. I apologise for any mistakes that I make.

It would be "helpful" if you take the time to point out the mistakes.

It would be "super helpful" if you could provide suggestions about how
to correct those mistakes.

Thanks in advance!

Also if we have lots of branches, depending on your pager settings you won't see this at all, I have:

    PAGER=less LESS="--IGNORE-CASE --LONG-PROMPT --QUIET --chop-long-lines --RAW-CONTROL-CHARS --no-init --quit-if-one-screen"

I've ended up with that $LESS setting via hackery over the years, so maybe I'm doing something retarded, minimal test case:

    PAGER=less  LESS="--no-init --quit-if-one-screen" git branch -l

So with those two options I don't get the stderr output at all, but if I remove either one of those options once I quit the pager I get the output at the end.

So I think this is probably OK for most users, if the have very few branches they'll see it, and then if they use default pager settings they'll see the stderr output once they quit the pager. I don't know how common my (mis)configuration is.

Hi Ævar,

I'm not sure this is ok, because I still see the stderr output with your minimal test case even when I have enough branches to not fit in one screen. The stderr output is of course above the pager output (after I quit the pager) and gets hidden out-of display as I stated before. I also get weird 'ESC[m' characters with you minimal test case. I'm not sure what I'm missing.

What version of 'less' do you use? Is any other configuration that you didn't mention affecting what your observation?

Sivaraam

QUOTE:

“The most valuable person on any team is the person who makes everyone
else on the team more valuable, not the person who knows the most.”

      - John Sonmez


Sivaraam?

You possibly might have noticed that my signature recently changed from
'Kaartic' to 'Sivaraam' both of which are parts of my name. I find the
new signature to be better for several reasons one of which is that the
former signature has a lot of ambiguities in the place I live as it is a
common name (NOTE: it's not a common spelling, just a common name). So,
I switched signatures before it's too late.

That said, I won't mind you calling me 'Kaartic' if you like it [of
course ;-)]. You can always call me using either of the names.


KIND NOTE TO THE NATIVE ENGLISH SPEAKER:

As I'm not a native English speaker myself, there might be mistaeks in
my usage of English. I apologise for any mistakes that I make.

It would be "helpful" if you take the time to point out the mistakes.

It would be "super helpful" if you could provide suggestions about how
to correct those mistakes.

Thanks in advance!

I don't get it anywhere, above or below.

Both "less 481 (GNU regular expressions)" and version 487. I upgraded and tried both. This is on Debian stable (and newer version from testing).

It may be something in my system config, but I tried sudo-ing to a user that only has stock Debian config & none of my custom .bashrc etc, and I get the same thing.

We redirect stderr to the pager, as well, exactly to catch this sort of case. But because git-branch does not kick in the pager until later (because it only wants to do it for list-mode), that happens _after_ we've emitted the message.

So one fix would be to teach deprecated_reflog_option_cb() to just set a flag rather than printing the warning, and then issue the warning. Something like this:

index 452742fecf..f51b89e962 100644
@@ -34,6 +34,7 @@ static const char * const builtin_branch_usage[] = {
 	NULL
 };
 
+static int used_deprecated_reflog_option;
 static const char *head;
 static struct object_id head_oid;
 

@@ -576,8 +577,7 @@ static int edit_branch_description(const char *branch_name)
 static int deprecated_reflog_option_cb(const struct option *opt,
 				       const char *arg, int unset)
 {
-	warning("the '-l' alias for '--create-reflog' is deprecated;");
-	warning("it will be removed in a future version of Git");
+	used_deprecated_reflog_option = 1;
 	*(int *)opt->value = !unset;
 	return 0;
 }

@@ -703,6 +703,11 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	if (list)
 		setup_auto_pager("branch", 1);
 
+	if (used_deprecated_reflog_option) {
+		warning("the '-l' alias for '--create-reflog' is deprecated;");
+		warning("it will be removed in a future version of Git");
+	}
+
 	if (delete) {
 		if (!argc)
 			die(_("branch name required"));

On the other hand, I'm not sure this is that big a deal. The point of the deprecation warning is to catch people who are actually trying to use "-l" as "--create-reflog", and that case does not page. The people doing "git branch -l" are actually getting what they want eventually, which is to turn it into "--list". In the interim step where it becomes an unknown option, they'll get a hard error.

-Peff

I observe exactly the consequence of this behaviour. First, the error is emitted and then the pager kicks in to list the branches.

I just thought we wouldn't want to surprise/confuse users who try to use "git branch -l" with the warning message about "create reflog" along-side the list of branches. That would just add to the confusion. So, I thought we should error out when users do "git branch -l" instead. Something like the following should help us prevent "git branch -l" from listing branch names and might also prevent the confusion.

index 452742fec..f3c5181bb 100644
@@ -672,7 +672,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
        argc = parse_options(argc, argv, prefix, options, builtin_branch_usage,
                             0);
 
-       if (!delete && !rename && !copy && !edit_description && !new_upstream && !unset_upstream && argc == 0)
+       if (!delete && !rename && !copy && !edit_description && !new_upstream && !unset_upstream && !reflog && argc == 0)
                list = 1;
 
        if (filter.with_commit || filter.merge != REF_FILTER_MERGED_NONE || filter.points_at.nr ||

Kaartic

Yeah, I think that's just a more extreme version of the current plan (it turns it immediately into a hard error instead of warning for a while). If we just make the warning easier to see in the paged case, I think that makes the current plan fine.

I'll wrap up the patch I sent earlier.

-Peff

Hmm, actually, I suppose the true value of the warning is to help people doing "git branch -l foo", and it would still work there. The "more extreme" from your suggested patch would only affect "branch -l".

Still, I think I prefer the gentler version that we get by keeping it as a warning even in the latter case.

Here's the patch. It goes on top of jk/branch-l-0-deprecation (and will naturally conflict with the removal branch, but the resolution should be obvious).

-- >8 --
Subject: [PATCH] branch: issue "-l" deprecation warning after pager starts

If you run "git branch -l", we issue a deprecation warning that "-l" is probably not doing what you expect. But we do so while parsing the options, which is _before_ we start the pager. Depending on your pager settings, this means that the warning may get totally overwritten by the pager.

Instead, let's delay the message until after we would have started the pager. If we do page, then it will end up inside the pager (since we redirect stderr). And if not (including the case when you really did mean for "-l" to work as "--create-reflog"), then it will still get shown on stderr.

---
 builtin/branch.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

index e50a5a1680..55bfacd843 100644
@@ -34,6 +34,7 @@ static const char * const builtin_branch_usage[] = {
 	NULL
 };
 
+static int used_deprecated_reflog_option;
 static const char *head;
 static struct object_id head_oid;
 

@@ -573,8 +574,7 @@ static int edit_branch_description(const char *branch_name)
 static int deprecated_reflog_option_cb(const struct option *opt,
 				       const char *arg, int unset)
 {
-	warning("the '-l' alias for '--create-reflog' is deprecated;");
-	warning("it will be removed in a future version of Git");
+	used_deprecated_reflog_option = 1;
 	*(int *)opt->value = !unset;
 	return 0;
 }

@@ -700,6 +700,11 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	if (list)
 		setup_auto_pager("branch", 1);
 
+	if (used_deprecated_reflog_option) {
+		warning("the '-l' alias for '--create-reflog' is deprecated;");
+		warning("it will be removed in a future version of Git");
+	}
+
 	if (delete) {
 		if (!argc)
 			die(_("branch name required"));

2.17.0.1391.g6fdbf40724

"git branch -l newbranch [forkpoint]" that warns "We won't be doing reflog creation with -l" is good, but "git branch -l" that warns "We won't be doing reflog creation with -l" sounds like a pure noise, as the user would say "Irrelevant, I am not doing that anyway--I am listing".

The warning to prepare users for the next step jk/branch-l-1-removal should say "we won't be accepting '-l' as a silent and unadvertised synonym soon. Spell it as --list" when "git branch -l" is given, I would think.

I hoped that reminding them that "-l is a synonym for --create-reflog" would serve as a gentle reminder that they're Doing It Wrong. I guess we could be more explicit, though.

It is not "we won't be accepting -l as a synonym" though. It was never a synonym, it's just that it didn't happen to do anything in list mode.

Yes, after step 1 it will error out. Again, I hoped the existing message would prepare people. But maybe we should do this on top of what I posted earlier?

-- >8 --
Subject: [PATCH] branch: customize "-l" warning in list mode

People mistakenly use "git branch -l", thinking that it triggers list mode. It doesn't, but the lack of non-option arguments in that command does (and the "-l" becomes a silent noop).

Since afc968e579 (branch: deprecate "-l" option, 2018-03-26) we've warned that "-l" is going away. But the warning text is primarily aimed at people who _meant_ to use "-l", as in "git branch -l foo". People who mistakenly said "git branch -l" may be left puzzled.

Let's make it clear that:

1. No, "-l" didn't do what they thought here.

2. It's going away, and what they should do instead.

---
 builtin/branch.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

index 55bfacd843..b0b33dab94 100644
@@ -701,8 +701,14 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		setup_auto_pager("branch", 1);
 
 	if (used_deprecated_reflog_option) {
-		warning("the '-l' alias for '--create-reflog' is deprecated;");
-		warning("it will be removed in a future version of Git");
+		if (list) {
+			warning("the '-l' option is an alias for '--create-reflog' and");
+			warning("has no effect in list mode. This option will soon be");
+			warning("removed and you should omit it (or use '--list' instead).");
+		} else {
+			warning("the '-l' alias for '--create-reflog' is deprecated;");
+			warning("it will be removed in a future version of Git");
+		}
 	}
 
 	if (delete) {

2.17.0.1391.g6fdbf40724

Yup, thanks for being extra explicit. I do imagine there are quite a few of us who would be puzzled without this update (but with the previous one to unhide it from behind the pager).

By the way, this is one of these times when I feel that we should have a better multi-line message support in die/error/warning/info functions. Ideally, I should be able to write

warning(_("the '-l' option is an alias for '--create-reflog' and\n"
"has no effect in list mode, This option will soon be\n" "removed and you should omit it (or use '--list' instead)."));

and warning() would:

- do the sprintf formatting thing as necessary to prepare a long multi-line message;

- chomp that into lines at '\n' boundary; and

- give each of these lines with _("warning: ") prefixed.

That way, translators can choose to make the resulting message to different number of lines from the original easily.

Yep, I totally agree. In past discussions I was thinking mostly of the pain of writing these multi-line messages. But I imagine it is absolute hell for translators, and we should fix it for that reason.

(Also, I guess this message probably ought to be marked for translation).

-Peff

Needless to tell you, I worked backwards when noticing that these three warning() lines are not marked for translation ;-).

But of course, fixing this in a naïve way will involve memory allocation during execution of die(), which may well be due to OOM, which is why we knew the need but haven't found a good solution to it.

[...]

[...]

How about something like this? The first two patches implement the above three points, except for the translation of "warning: ".

The third patch is the main reason this is marked RFC. It translates "warning: " and similar, and breaks quite a few tests under GETTEXT_POISON since we grep for, e.g., "warning" on stderr. I could annotate those tests, but since I'm running out of time anyway, I thought I'd post this as a heads-up of "I'm looking into this".

I do wonder: If our tests rely on grepping for "warning" (for pretty good reasons), how many scripts out there do something similar (for maybe-not-so-good reasons, but still)? Do we want to avoid breaking them?

Also left to do is to convert any existing lego-ing users to the single-string form that Junio wished for above.

Martin

Martin Ågren (3):
usage: extract prefix_suffix_lines() from advise()
usage: prefix all lines in vreportf(), not just the first
usage: translate the "error: "-prefix and others

 t/t1011-read-tree-sparse-checkout.sh |  6 ++---
 t/t1506-rev-parse-diagnosis.sh       |  2 +-
 t/t1600-index.sh                     |  6 ++---
 t/t3600-rm.sh                        | 36 ++++++++++++-------------
 t/t5512-ls-remote.sh                 |  6 ++---
 t/t7607-merge-overwrite.sh           |  6 ++---
 t/t7609-merge-co-error-msgs.sh       | 39 ++++++++++++++--------------
 git-compat-util.h                    |  8 ++++++
 advice.c                             | 18 ++++++-------
 usage.c                              | 28 ++++++++++++++++----
 10 files changed, 89 insertions(+), 66 deletions(-)

2.17.0.1181.g093e983b05

advice.c contains a useful code snippet which takes a multi-line string and prints the lines, prefixing and suffixing each line with two constant strings. This was originally added in 23cb5bf3b3 (i18n of multi-line advice messages, 2011-12-22) to produce such output:

hint: some multi-line advice
hint: prefixed with "hint: "

The prefix is actually colored after 960786e761 (push: colorize errors, 2018-04-21) and each line has a suffix for resetting the color.

The next commit will teach the same "prefix all the lines"-trick to the code that produces, e.g., "warning: "-messages. In preparation for that, extract the code for printing the individual lines and expose it through git-compat-util.h.

---
I'm open for suggestions on the naming of prefix_suffix_lines()...

 git-compat-util.h |  8 ++++++++
 advice.c          | 18 ++++++++----------
 usage.c           | 18 ++++++++++++++++++
 3 files changed, 34 insertions(+), 10 deletions(-)

index f9e4c5f9bc..23445f7ab9 100644
@@ -415,6 +415,14 @@ static inline char *git_find_last_dir_sep(const char *path)
 struct strbuf;
 
 /* General helper functions */
+
+/*
+ * Write the message to the file, prefixing and suffixing
+ * each line with `prefix` resp. `suffix`.
+ */
+void prefix_suffix_lines(FILE *f, const char *prefix,
+			 const char *message, const char *suffix);
+
 extern void vreportf(const char *prefix, const char *err, va_list params);
 extern NORETURN void usage(const char *err);
 extern NORETURN void usagef(const char *err, ...) __attribute__((format (printf, 1, 2)));

index 370a56d054..ffb29e7ef4 100644
@@ -79,24 +79,22 @@ static struct {
 
 void advise(const char *advice, ...)
 {
+	struct strbuf prefix = STRBUF_INIT;
 	struct strbuf buf = STRBUF_INIT;
 	va_list params;
-	const char *cp, *np;
+
+	strbuf_addf(&prefix, _("%shint: "),
+		    advise_get_color(ADVICE_COLOR_HINT));
 
 	va_start(params, advice);
 	strbuf_vaddf(&buf, advice, params);
 	va_end(params);
 
-	for (cp = buf.buf; *cp; cp = np) {
-		np = strchrnul(cp, '\n');
-		fprintf(stderr,	_("%shint: %.*s%s\n"),
-			advise_get_color(ADVICE_COLOR_HINT),
-			(int)(np - cp), cp,
-			advise_get_color(ADVICE_COLOR_RESET));
-		if (*np)
-			np++;
-	}
+	prefix_suffix_lines(stderr, prefix.buf, buf.buf,
+			    advise_get_color(ADVICE_COLOR_RESET));
+
 	strbuf_release(&buf);
+	strbuf_release(&prefix);
 }
 
 int git_default_advice_config(const char *var, const char *value)

index cdd534c9df..80f9c1d14b 100644
@@ -6,6 +6,24 @@
 #include "git-compat-util.h"
 #include "cache.h"
 
+void prefix_suffix_lines(FILE *f,
+			 const char *prefix,
+			 const char *message,
+			 const char *suffix)
+{
+	const char *cp, *np;
+
+	for (cp = message; *cp; cp = np) {
+		np = strchrnul(cp, '\n');
+		fprintf(f, "%s%.*s%s\n",
+			prefix,
+			(int)(np - cp), cp,
+			suffix);
+		if (*np)
+			np++;
+	}
+}
+
 void vreportf(const char *prefix, const char *err, va_list params)
 {
 	char msg[4096];

2.17.0.1181.g093e983b05

Is there a verb that means "have/place the thing in between two other things" or "Bring two things and place them on each side of the third thing" in a more concise way? Wrap? Sandwich? Enclose?

I think the important verb, print (to FILE*), is somehow missing. This current name would be great if it produces another str(buf).

Duy

It can't use a strbuf, since we want vreportf() to avoid calling any allocation functions.

-Peff

Sounds good to me. And if you hate the repeated "error:" prefix that makes the prefix on the second and subsequent lines included in cutting and pasting, we could use the two-prefix idea elsewhere in the thread, too.

If we do not want duplicate prefix, another approach is to leave everything including alignment up to the translators. For example,

error(_("the first line of error.\n"
" ... and the second."));

could be a valid way to produce

error: the first line of error.
... and the second.

on the display, and get it translated to

ERRORX: THE FIRST LINE OF ERROR.
... AND THE SECOND.

with these entries for that hypothetical "shout in caps" language.

msgid "error: "
msgstr "ERRORX: "
msgid "the first line of error.\n ... and the second."
msgstr"THE FIRST LINE OF ERROR.\n ... AND THE SECOND."

So I do not think the two-prefix idea is necessary (and if people prefer to have repeated prefix, that can also be done perfectly well within this scheme---the second and subsequent message needs to duplicate "error:" at the beginning, which is sort of ugly, but the leading spaces for alignment we see above already knows how wide "error:" and its translation is, so it is not that much worse anyway).

But still, your suggestion to allow color/reset so that we can do without extra allocation is a good idea.

(That also gets rid of the minor strangeness of my series to introduce an overly broad suffix and use it only for resetting color, not actually for giving any textual suffix.)

[...]

Thanks for lots of good input. Doing the indenting in the error does mean that to indent properly, the translator needs to know that it is an error, and not a warning. Or one could say that they would need to know the amount of indentation anyway so that they can know where to wrap optimally.

Another consequence is that if we can emit a certain string as either, e.g., a warning or an error depending on the context, we need to address that. Using contexts, of course. Thank you for the hint about that. (I have not checked if we actually have such "this is a warning or an error"-strings currently.)

Somehow it feels slightly cleaner to me, at least on first thought, to try to decouple the indenting from the translating and line-wrapping. But as noted above, the indenting does affect how the line-wrapping should/may be done.

I won't have as much time as I'd like for tinkering with this the next week, or even weeks. Hopefully when I do get around to it, I will have processed all the very good input I have received and turn that into something good.

Thanks,
Martin

Yeah, that "suffix" argument left me scratching my head at first. :)

Yeah, agreed. Once report_lines() (or whatever we call it) knows what the prefix is, it is capable of handling indentation on its own either by repeating the prefix or by computing its visual width and outputting appropriate spaces (which we can probably never get perfect without knowing about the terminal, but we could at least make an approximation, or even use some ANSI magic -- we could even make it run-time configurable, though I'm not sure anybody would care that much).

-Peff

Teach vreportf() to prefix all lines with the given prefix, not only the first line. This matches how "hint: " is being shown, and affects "error: ", "fatal: ", "usage: ", "warning: " and "BUG: " (as well as any out-of-tree and future users).

Note that we need to adjust quite a few tests as a result of this change. All of these changes are because we obviously need to prefix more lines in various "expect"-files -- except for one instance of a trailing empty line that disappears with this commit (see t7609). This is a very minor change, and arguably a good one.

---
Looking back at this, I wonder if the opposite approach would be better, that is, making advise() use vreportf() after teaching the latter the multi-line trick.

 t/t1011-read-tree-sparse-checkout.sh |  6 ++---
 t/t1506-rev-parse-diagnosis.sh       |  2 +-
 t/t1600-index.sh                     |  6 ++---
 t/t3600-rm.sh                        | 36 ++++++++++++-------------
 t/t5512-ls-remote.sh                 |  6 ++---
 t/t7607-merge-overwrite.sh           |  6 ++---
 t/t7609-merge-co-error-msgs.sh       | 39 ++++++++++++++--------------
 usage.c                              |  2 +-
 8 files changed, 51 insertions(+), 52 deletions(-)

index 0c6f48f302..31b0702e6c 100755
@@ -243,9 +243,9 @@ test_expect_success 'print errors when failed to update worktree' '
 	test_must_fail git checkout top 2>actual &&
 	cat >expected <<\EOF &&
 error: The following untracked working tree files would be overwritten by checkout:
-	sub/added
-	sub/addedtoo
-Please move or remove them before you switch branches.
+error: 	sub/added
+error: 	sub/addedtoo
+error: Please move or remove them before you switch branches.
 Aborting
 EOF
 	test_i18ncmp expected actual

index 4ee009da66..80d35087b7 100755
@@ -11,7 +11,7 @@ test_did_you_mean ()
 	sq="'" &&
 	cat >expected <<-EOF &&
 	fatal: Path '$2$3' $4, but not ${5:-$sq$3$sq}.
-	Did you mean '$1:$2$3'${2:+ aka $sq$1:./$3$sq}?
+	fatal: Did you mean '$1:$2$3'${2:+ aka $sq$1:./$3$sq}?
 	EOF
 	test_cmp expected error
 }

index c4422312f4..39a707c94a 100755
@@ -16,7 +16,7 @@ test_expect_success 'bogus GIT_INDEX_VERSION issues warning' '
 		git add a 2>&1 | sed "s/[0-9]//" >actual.err &&
 		sed -e "s/ Z$/ /" <<-\EOF >expect.err &&
 			warning: GIT_INDEX_VERSION set, but the value is invalid.
-			Using version Z
+			warning: Using version Z
 		EOF
 		test_i18ncmp expect.err actual.err
 	)

@@ -30,7 +30,7 @@ test_expect_success 'out of bounds GIT_INDEX_VERSION issues warning' '
 		git add a 2>&1 | sed "s/[0-9]//" >actual.err &&
 		sed -e "s/ Z$/ /" <<-\EOF >expect.err &&
 			warning: GIT_INDEX_VERSION set, but the value is invalid.
-			Using version Z
+			warning: Using version Z
 		EOF
 		test_i18ncmp expect.err actual.err
 	)

@@ -54,7 +54,7 @@ test_expect_success 'out of bounds index.version issues warning' '
 		git add a 2>&1 | sed "s/[0-9]//" >actual.err &&
 		sed -e "s/ Z$/ /" <<-\EOF >expect.err &&
 			warning: index.version set, but the value is invalid.
-			Using version Z
+			warning: Using version Z
 		EOF
 		test_i18ncmp expect.err actual.err
 	)

index b8fbdefcdc..cd4a10df2d 100755
@@ -771,10 +771,10 @@ test_expect_success 'setup for testing rm messages' '
 test_expect_success 'rm files with different staged content' '
 	cat >expect <<-\EOF &&
 	error: the following files have staged content different from both the
-	file and the HEAD:
-	    bar.txt
-	    foo.txt
-	(use -f to force removal)
+	error: file and the HEAD:
+	error:     bar.txt
+	error:     foo.txt
+	error: (use -f to force removal)
 	EOF
 	echo content1 >foo.txt &&
 	echo content1 >bar.txt &&

@@ -785,9 +785,9 @@ test_expect_success 'rm files with different staged content' '
 test_expect_success 'rm files with different staged content without hints' '
 	cat >expect <<-\EOF &&
 	error: the following files have staged content different from both the
-	file and the HEAD:
-	    bar.txt
-	    foo.txt
+	error: file and the HEAD:
+	error:     bar.txt
+	error:     foo.txt
 	EOF
 	echo content2 >foo.txt &&
 	echo content2 >bar.txt &&

@@ -798,8 +798,8 @@ test_expect_success 'rm files with different staged content without hints' '
 test_expect_success 'rm file with local modification' '
 	cat >expect <<-\EOF &&
 	error: the following file has local modifications:
-	    foo.txt
-	(use --cached to keep the file, or -f to force removal)
+	error:     foo.txt
+	error: (use --cached to keep the file, or -f to force removal)
 	EOF
 	git commit -m "testing rm 3" &&
 	echo content3 >foo.txt &&

@@ -810,7 +810,7 @@ test_expect_success 'rm file with local modification' '
 test_expect_success 'rm file with local modification without hints' '
 	cat >expect <<-\EOF &&
 	error: the following file has local modifications:
-	    bar.txt
+	error:     bar.txt
 	EOF
 	echo content4 >bar.txt &&
 	test_must_fail git -c advice.rmhints=false rm bar.txt 2>actual &&

@@ -820,8 +820,8 @@ test_expect_success 'rm file with local modification without hints' '
 test_expect_success 'rm file with changes in the index' '
 	cat >expect <<-\EOF &&
 	error: the following file has changes staged in the index:
-	    foo.txt
-	(use --cached to keep the file, or -f to force removal)
+	error:     foo.txt
+	error: (use --cached to keep the file, or -f to force removal)
 	EOF
 	git reset --hard &&
 	echo content5 >foo.txt &&

@@ -833,7 +833,7 @@ test_expect_success 'rm file with changes in the index' '
 test_expect_success 'rm file with changes in the index without hints' '
 	cat >expect <<-\EOF &&
 	error: the following file has changes staged in the index:
-	    foo.txt
+	error:     foo.txt
 	EOF
 	test_must_fail git -c advice.rmhints=false rm foo.txt 2>actual &&
 	test_i18ncmp expect actual

@@ -842,12 +842,12 @@ test_expect_success 'rm file with changes in the index without hints' '
 test_expect_success 'rm files with two different errors' '
 	cat >expect <<-\EOF &&
 	error: the following file has staged content different from both the
-	file and the HEAD:
-	    foo1.txt
-	(use -f to force removal)
+	error: file and the HEAD:
+	error:     foo1.txt
+	error: (use -f to force removal)
 	error: the following file has changes staged in the index:
-	    bar1.txt
-	(use --cached to keep the file, or -f to force removal)
+	error:     bar1.txt
+	error: (use --cached to keep the file, or -f to force removal)
 	EOF
 	echo content >foo1.txt &&
 	git add foo1.txt &&

index 6a949484d0..7e505163f6 100755
@@ -131,9 +131,9 @@ test_expect_success 'confuses pattern as remote when no remote specified' '
 	cat >exp <<-EOF &&
 	fatal: '\''$does_not_exist'\'' does not appear to be a git repository
 	fatal: Could not read from remote repository.
-
-	Please make sure you have the correct access rights
-	and the repository exists.
+	fatal: 
+	fatal: Please make sure you have the correct access rights
+	fatal: and the repository exists.
 	EOF
 	#
 	# Do not expect "git ls-remote <pattern>" to work; ls-remote needs

index dd8ab7ede1..985fbab0b0 100755
@@ -116,9 +116,9 @@ test_expect_success 'will not overwrite untracked subtree' '
 
 cat >expect <<\EOF
 error: The following untracked working tree files would be overwritten by merge:
-	sub
-	sub2
-Please move or remove them before you merge.
+error: 	sub
+error: 	sub2
+error: Please move or remove them before you merge.
 Aborting
 EOF
 

index e90413204e..54bc3d2c9c 100755
@@ -27,11 +27,11 @@ test_expect_success 'setup' '
 
 cat >expect <<\EOF
 error: The following untracked working tree files would be overwritten by merge:
-	five
-	four
-	three
-	two
-Please move or remove them before you merge.
+error: 	five
+error: 	four
+error: 	three
+error: 	two
+error: Please move or remove them before you merge.
 Aborting
 EOF
 

@@ -50,13 +50,13 @@ test_expect_success 'untracked files overwritten by merge (fast and non-fast for
 
 cat >expect <<\EOF
 error: Your local changes to the following files would be overwritten by merge:
-	four
-	three
-	two
-Please commit your changes or stash them before you merge.
+error: 	four
+error: 	three
+error: 	two
+error: Please commit your changes or stash them before you merge.
 error: The following untracked working tree files would be overwritten by merge:
-	five
-Please move or remove them before you merge.
+error: 	five
+error: Please move or remove them before you merge.
 Aborting
 EOF
 

@@ -70,9 +70,9 @@ test_expect_success 'untracked files or local changes ovewritten by merge' '
 
 cat >expect <<\EOF
 error: Your local changes to the following files would be overwritten by checkout:
-	rep/one
-	rep/two
-Please commit your changes or stash them before you switch branches.
+error: 	rep/one
+error: 	rep/two
+error: Please commit your changes or stash them before you switch branches.
 Aborting
 EOF
 

@@ -92,9 +92,9 @@ test_expect_success 'cannot switch branches because of local changes' '
 
 cat >expect <<\EOF
 error: Your local changes to the following files would be overwritten by checkout:
-	rep/one
-	rep/two
-Please commit your changes or stash them before you switch branches.
+error: 	rep/one
+error: 	rep/two
+error: Please commit your changes or stash them before you switch branches.
 Aborting
 EOF
 

@@ -106,9 +106,8 @@ test_expect_success 'not uptodate file porcelain checkout error' '
 
 cat >expect <<\EOF
 error: Updating the following directories would lose untracked files in them:
-	rep
-	rep2
-
+error: 	rep
+error: 	rep2
 Aborting
 EOF
 

index 80f9c1d14b..6a5669922f 100644
@@ -34,7 +34,7 @@ void vreportf(const char *prefix, const char *err, va_list params)
 		if (iscntrl(*p) && *p != '\t' && *p != '\n')
 			*p = '?';
 	}
-	fprintf(stderr, "%s%s\n", prefix, msg);
+	prefix_suffix_lines(stderr, prefix, msg, "");
 }
 
 static NORETURN void usage_builtin(const char *err, va_list params)

2.17.0.1181.g093e983b05

Also, very subjectively, I'm torn about this. To me, just one "error/warning/fatal" at the start of the first paragraph feels much better. If we have to somehow mark the second paragraph that "this is also part of the error message" then it's probably better to rephrase.

Duy

I personally can go either way. If you prefer less noisy route, we could change the function signature of vreportf() to take a prefix for the first line and another prefix for the remaining lines and pass that through down to the "split and print with prefix" helper.

That way, we can

- allow callers to align 1st prefix (e.g. "error: ") with the leading indent for the second and subsequent lines by passing the second prefix with appropriate display width.

- allow translators to grow or shrink number of lines a given message takes, and to decide where in the translated string to wrap lines.

Even though step 3/3 may become a bit awkward (the second prefix would most likely be only whitespace, and you'd need to write something silly like _("\t")), we can still keep the alignment if we wanted to.

Would you feel the same about "hint: "? We already do prefix all the lines there. It seems to we we should probably do the same for "hint: " as for "warning: ", whatever we decide is right.

I suspect this second prefix would always consist of strlen(first_prefix) spaces? We should be able to construct it on the fly, without any need for manual counting and human mistakes.

Thanks both for your comments. I'll see what I can come up with.

Martin

I wouldn't be so bold to claim that, given especially that we are talking about i18n/l10n where byte count, character count and display width are all different even on a terminal with fixed-width font.

You are of course correct. I should have my morning tea before typing.

About the _("\t")-approach that you mentioned up-thread. It would allow a translator to adjust all the indentations for a particular language.
To be clear, what you mean is _(" " /* 9 spaces */) to align
nicely with "warning: ", which is the longest English string. Then the
translator would translate the nine spaces and all of "fatal: " and
others to padded strings, all of the same length (not necessarily nine). Correct?

That approach seems a bit shaky, if nothing else because we may one day similarly want to use nine "translated" spaces in some other context. Or maybe this is actually i18n-best-practices.

It also means that shorter prefixes are somewhat arbitrarily padded, just because there exists a longer prefix that some other code path may
want to use. OTOH, if a "warning: " is followed by an "error: ", both
lines/blocks would have the same indentation, which might perhaps be (slightly) preferable.

Martin

I was envisioning that these

error: the first line of an error message
and the second line indented by 7 places (strlen("error:")+1)
info: the first line of an info message
and the second line indented by 6 places (strlen("info:")+1)

are produced by

	vreportf("error: ", "       " /* 7 spaces */,
		 "the first line of an error message\nand the second ...");
	vreportf("info: ", "      " /* 6 spaces */,
		 "the first line of an info message\nand the second ...");

And if all of these string literals were inside _(), then depending on how many display columns translated version of "error" and "info" takes in the target language, these 7-space and 6-space secondary prefixes would be "translated" differently.

Of course, since your language may translate "error" and "fatal" to different display columns, the 7-space secondary prefix in this one

	vreportf("fatal: ", "       " /* 7 spaces */,
		 "the first line of a fatal error message\nand the second ...");

needs to be mapped to a string that is differnt from the 7-space for "error: ". I think you would use "contexts" to map the same source 7-space to different translated string when it becomes necessary.

https://www.gnu.org/software/gettext/manual/html_node/Contexts.html

It may depend on context. Let's look at the commit that introduces this "hint:" prefix, 38ef61cfde (advice: Introduce error_resolve_conflict - 2011-08-04). The example in the commit message shows the hint paragraph sandwiched by an error and a fatal one:

error: 'commit' is not possible because you have unmerged files.
hint: Fix them up in the work tree ...
hint: ...
fatal: Exiting because of an unresolved conflict.

I think in this case (dense paragraphs of different message types) yes it might make sense to prefix lines with "hint:". But when there's only one type of message like the "error" part quoted at the top, it feels too verbose to have error: prefix everywhere.

Duy

Hmm, that's interesting. Deciding based on what has already been output seems feasible, although it sounds like the potential target of infinite tweaking of some central logic for deciding which way to go. Or, lots of various places to try and make consistent. I am tempted towards indenting with spaces in v2, and to leave "hint: " alone as an outlier. (It always was one. :-/ ) I'll keep your feedback in mind.

Thanks,
Martin

Heh, that is one of the cases that I found most ugly when I looked into this earlier (and in particular, because I think it makes cut-and-paste a little harder).

More discussion in:

https://public-inbox.org/git/[email protected]/

and down-thread.

-Peff

Thanks for the pointer. I had missed that thread entirely.

Martin

Translate the "error: ", "fatal: ", "usage: " and "warning: " prefixes that we use for reporting that kind of information.

Do not translate "BUG: ". We tend to prefer the messages themselves to be non-translated (and they're not supposed to ever appear anyway) so it makes sense to let the prefix be nontranslated, too.

---
This change breaks several tests under GETTEXT_POISON, e.g. t6030 which does this:

	git bisect skip > my_bisect_log.txt 2>&1 &&
	grep "warning" my_bisect_log.txt

 usage.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

index 6a5669922f..7709cb22e7 100644
@@ -39,24 +39,24 @@ void vreportf(const char *prefix, const char *err, va_list params)
 
 static NORETURN void usage_builtin(const char *err, va_list params)
 {
-	vreportf("usage: ", err, params);
+	vreportf(_("usage: "), err, params);
 	exit(129);
 }
 
 static NORETURN void die_builtin(const char *err, va_list params)
 {
-	vreportf("fatal: ", err, params);
+	vreportf(_("fatal: "), err, params);
 	exit(128);
 }
 
 static void error_builtin(const char *err, va_list params)
 {
-	vreportf("error: ", err, params);
+	vreportf(_("error: "), err, params);
 }
 
 static void warn_builtin(const char *warn, va_list params)
 {
-	vreportf("warning: ", warn, params);
+	vreportf(_("warning: "), warn, params);
 }
 
 static int die_is_recursing_builtin(void)

2.17.0.1181.g093e983b05

With these two patches queued on top of jk/branch-l-0-deprecation, the follow-up patches jk/branch-l-1-removal that makes 'branch -l' to fail and then jk/branch-l-2-reincarnation that makes 'branch -l' a synonym to 'branch --list' needs rebasing. Both are trivial and hopefully I did them correctly.

-- >8 --
From: Jeff King <[email protected]>
Date: Mon, 26 Mar 2018 03:29:22 -0400
Subject: [PATCH] branch: drop deprecated "-l" option

We marked the "-l" option as deprecated back in <insert sha1 here>. Now that sufficient time has passed, let's follow through and get rid of it.

---
 builtin/branch.c  | 14 --------------
 t/t3200-branch.sh | 12 ++++++++++++
 2 files changed, 12 insertions(+), 14 deletions(-)

index b0b33dab94..01b35b3c3d 100644
@@ -571,14 +571,6 @@ static int edit_branch_description(const char *branch_name)
 	return 0;
 }
 
-static int deprecated_reflog_option_cb(const struct option *opt,
-				       const char *arg, int unset)
-{
-	used_deprecated_reflog_option = 1;
-	*(int *)opt->value = !unset;
-	return 0;
-}
-
 int cmd_branch(int argc, const char **argv, const char *prefix)
 {
 	int delete = 0, rename = 0, copy = 0, force = 0, list = 0;

@@ -622,12 +614,6 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		OPT_BIT('C', NULL, &copy, N_("copy a branch, even if target exists"), 2),
 		OPT_BOOL(0, "list", &list, N_("list branch names")),
 		OPT_BOOL(0, "create-reflog", &reflog, N_("create the branch's reflog")),
-		{
-			OPTION_CALLBACK, 'l', NULL, &reflog, NULL,
-			N_("deprecated synonym for --create-reflog"),
-			PARSE_OPT_NOARG | PARSE_OPT_HIDDEN,
-			deprecated_reflog_option_cb
-		},
 		OPT_BOOL(0, "edit-description", &edit_description,
 			 N_("edit the description for the branch")),
 		OPT__FORCE(&force, N_("force creation, move/rename, deletion"), PARSE_OPT_NOCOMPLETE),

index da97b8a62b..eca75d3ca1 100755
@@ -45,6 +45,12 @@ test_expect_success 'git branch HEAD should fail' '
 	test_must_fail git branch HEAD
 '
 
+test_expect_success 'git branch -l no longer is --create-reflog' '
+	test_when_finished "git branch -D new-branch-with-reflog || :" &&
+	test_must_fail git branch -l new-branch-with-reflog &&
+	test_must_fail git rev-parse --verify refs/heads/new-branch-with-reflog
+'
+
 cat >expect <<EOF
 $_z40 $HEAD $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150200 +0000	branch: Created from master
 EOF

@@ -288,6 +294,12 @@ test_expect_success 'git branch --list -v with --abbrev' '
 
 '
 
+test_expect_failure 'git branch -l eventually is --list' '
+	git branch --list >expect &&
+	git branch -l >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'git branch --column' '
 	COLUMNS=81 git branch --column=column >actual &&
 	cat >expected <<\EOF &&

2.17.0-775-ge144d126d7

And this is the final "reincarnation" step.

-- >8 --
From: Jeff King <[email protected]>
Date: Mon, 26 Mar 2018 03:29:48 -0400
Subject: [PATCH] branch: make "-l" a synonym for "--list"

The other "mode" options of git-branch have short-option aliases that are easy to type (e.g., "-d" and "-m"). Let's give "--list" the same treatment.

This also makes it consistent with the similar "git tag -l" option.

We didn't do this originally because "--create-reflog" was squatting on the "-l" option. Now that sufficient time has passed with that alias removed, we can finally repurpose it.

---
 builtin/branch.c  | 2 +-
 t/t3200-branch.sh | 8 +-------
 2 files changed, 2 insertions(+), 8 deletions(-)

index 01b35b3c3d..1d06f5c547 100644
@@ -612,7 +612,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		OPT_BIT('M', NULL, &rename, N_("move/rename a branch, even if target exists"), 2),
 		OPT_BIT('c', "copy", &copy, N_("copy a branch and its reflog"), 1),
 		OPT_BIT('C', NULL, &copy, N_("copy a branch, even if target exists"), 2),
-		OPT_BOOL(0, "list", &list, N_("list branch names")),
+		OPT_BOOL('l', "list", &list, N_("list branch names")),
 		OPT_BOOL(0, "create-reflog", &reflog, N_("create the branch's reflog")),
 		OPT_BOOL(0, "edit-description", &edit_description,
 			 N_("edit the description for the branch")),

index eca75d3ca1..022d6a41c8 100755
@@ -45,12 +45,6 @@ test_expect_success 'git branch HEAD should fail' '
 	test_must_fail git branch HEAD
 '
 
-test_expect_success 'git branch -l no longer is --create-reflog' '
-	test_when_finished "git branch -D new-branch-with-reflog || :" &&
-	test_must_fail git branch -l new-branch-with-reflog &&
-	test_must_fail git rev-parse --verify refs/heads/new-branch-with-reflog
-'
-
 cat >expect <<EOF
 $_z40 $HEAD $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150200 +0000	branch: Created from master
 EOF

@@ -294,7 +288,7 @@ test_expect_success 'git branch --list -v with --abbrev' '
 
 '
 
-test_expect_failure 'git branch -l eventually is --list' '
+test_expect_success 'git branch -l is --list' '
 	git branch --list >expect &&
 	git branch -l >actual &&
 	test_cmp expect actual

2.17.0-775-ge144d126d7

Thanks. There's one bit missing here, because it did not cause a textual conflict during the rebase (but it's now dead code). Patch below (to be squashed to the tip of jk/branch-l-1-removal).

We may also want to fill in <insert sha1 here>. I think it's afc968e579 (branch: deprecate "-l" option, 2018-03-26), which should be stable at this point (it's already merged to 'next').

index 01b35b3c3d..f7cd333587 100644
@@ -34,7 +34,6 @@ static const char * const builtin_branch_usage[] = {
 	NULL
 };
 
-static int used_deprecated_reflog_option;
 static const char *head;
 static struct object_id head_oid;
 

@@ -686,17 +685,6 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	if (list)
 		setup_auto_pager("branch", 1);
 
-	if (used_deprecated_reflog_option) {
-		if (list) {
-			warning("the '-l' option is an alias for '--create-reflog' and");
-			warning("has no effect in list mode. This option will soon be");
-			warning("removed and you should omit it (or use '--list' instead).");
-		} else {
-			warning("the '-l' alias for '--create-reflog' is deprecated;");
-			warning("it will be removed in a future version of Git");
-		}
-	}
-
 	if (delete) {
 		if (!argc)
 			die(_("branch name required"));

I was hoping that we can settle the "multi-line message translation that can potentially result in different number of lines" issue before we have to mark the above for translation ;-)

Yeah, right after saying that I realized it would create horrible translation-lego. I agree we should punt for now.

-Peff

So, this patch is to improve the user experience of people who use "git branch -l" for listing and not for the people who forget to give a new branch name argument for "-l". In that case, this makes sense.

BTW, I hope people don't start wondering why "git branch -d" doesn't trigger list mode ;-)

Sivaraam

Already being discussed elsewhere on the thread; we're hoping for a warning() that will auto-prefix each one.

That said, I think I'm going to re-roll this in the direction discussed elsewhere in the thread (skipping the warning for list-mode).

-Peff

I never wanted to suppress the warning message in the latter case. I just wanted to avoid listing the branches. Actually the patch I sent in one the previous threads[1] that avoids listing the branches has the following behaviour,

$ git branch -l
warning: the '-l' alias for '--create-reflog' is deprecated;
warning: it will be removed in a future version of Git
usage: git branch [<options>] [-r | -a] [--merged | --no-merged]
   or: git branch [<options>] [-l] [-f] <branch-name> [<start-point>]
   or: git branch [<options>] [-r] (-d | -D) <branch-name>...
   or: git branch [<options>] (-m | -M) [<old-branch>] <new-branch>
   or: git branch [<options>] (-c | -C) [<old-branch>] <new-branch>
   or: git branch [<options>] [-r | -a] [--points-at]
   or: git branch [<options>] [-r | -a] [--format]

So, the warning message isn't lost. It just prevents the listing of branches.

Wait, maybe I'm misunderstanding what you mean by "warning". You're probably meaning something related to the way Git exits in both cases. With your patch "git branch -l" prints a warning, lists the branches and has an exit status of 0. With my patch it prints the warning, the usage specifications with exit status 128. In that case, I still don't think it's bad to turn "git branch -l" into an error now as it's been incorrect for a long time now and it's not wrong if we correct it now.

Anyways, if you think it mustn't turn into an error now and only in the next stage, a suggestion follows in another thread.

[1]: https://public-inbox.org/git/[email protected]/

Sivaraam

QUOTE:

“The most valuable person on any team is the person who makes everyone
else on the team more valuable, not the person who knows the most.”

      - John Sonmez


Sivaraam?

You possibly might have noticed that my signature recently changed from
'Kaartic' to 'Sivaraam' both of which are parts of my name. I find the
new signature to be better for several reasons one of which is that the
former signature has a lot of ambiguities in the place I live as it is a
common name (NOTE: it's not a common spelling, just a common name). So,
I switched signatures before it's too late.

That said, I won't mind you calling me 'Kaartic' if you like it [of
course ;-)]. You can always call me using either of the names.


KIND NOTE TO THE NATIVE ENGLISH SPEAKER:

As I'm not a native English speaker myself, there might be mistaeks in
my usage of English. I apologise for any mistakes that I make.

It would be "helpful" if you take the time to point out the mistakes.

It would be "super helpful" if you could provide suggestions about how
to correct those mistakes.

Thanks in advance!

Right, what I meant by "gentler" is that we continue to perform the same behavior as the old version, alongside the warning. It's arguable here because running "git branch -l" has _always_ been wrong. It's just wrong in a way that happens to do what the user wants. ;)

I don't think "mustn't", but I have a slight preference for what I posted, as I think it is a little friendlier during the transition (at the risk of somebody missing the warning, but then step 2 turns it into a hard error anyway, so they'll certainly find out then).

-Peff

Well, we could keep treating '-l' given in contexts where we have silently ignored the option and did "list" instead as before during the transition, until the very end where it becomes an explicit "list" command, no? Then there is no need to even warn against '-l' that is ignored because we are listing in the earliest step. The only usage that requires a warning then becomes '-l' used for its original meaning to create a reflog, right? That sounds gentler to me.

So I think you're proposing:

- step 0: warn about "-l" when it actually gets used, and otherwise continue ignoring

- step 1: turn "-l" into "--list"

- step 2: there is no step 2

I can't think of any reason that would work, and it lets us reclaim it for "--list" sooner. I guess "when it gets used" is maybe not the right criterion. We'd warn on:

git branch -l foo

But we wouldn't on:

git branch -d -l foo

That's clearly nonsense, but we're probably better off complaining. So I guess the right rule is to warn when we are not in list-mode, and otherwise quietly accept it.

That does mean that anybody who misses the deprecation warning may be surprised when "branch -l foo" starts listing instead of creating a branch with a reflog (whereas in the current 3-step plan, we have a period in the middle where that's a hard error). That may be OK, though, and is a natural consequence of getting to the end step sooner (even with a 3-step plan, anybody who skips the versions in the middle _could_ be surprised).

-Peff

Thanks for a concise and readably summary ;-)

That sounds interesting. I guess that would avoid the confusion I was speaking of from the beginning of this thread as the warning message would not be shown at all for "git branch -l". Of course, it would then confuse people who discover that "git branch -l" lists and "git branch -l $prefix" creates a branch with name "$prefix" (if it's valid) instead of listing branch names with prefix "$prefix". So, it might be worth considering.

BTW, I suspect this would make the deprecation of "-l" a little unsual as the no. of people who see the deprecation warning would be less as the warning is shown only for "git branch -l $branch" and I also suspect the no. of users of that command would be very less as previously pointed by someone elsewhere.

--
Sivaraam

With the lockfile fix, it should be ready. I've been giving this significant testing on my machine and a few other developers here. The next version of GVFS is shipping with this code and with GVFS controlling the maintenance of the commit-graph file. That code has been cooking with our CI builds for a while, with full functional tests against the Windows repository. The only bugs we've found are the fix in "merge: check config before loading commits" and in ds/commit-graph-lockfile-fix.

Sorry for the triple-m.

Thanks,
-Stolee

Please do not.
(Or do, but then be prepared for another hotfix.)

The commit sb/object-store-replace^ needs more free'ing and shall be replaced with
https://public-inbox.org/git/[email protected]/
I'll resend shortly.

Merging would be ok, but I would rather not.
A resend will be only for cosmetic effect, as I messed up the last commit

So, please hold in pu.

I did not get around to fix it up, there are still review comments outstanding. (The test is broken in the last commit.)

Please hold in pu;

Thanks,
Stefan

Here comes a reroll with another fix, diff below, as the branch-diff did not give a good output[1].

Thanks,
Stefan

$ git diff origin/sb/object-store-replace

index cdf520084df..97245fdea25 100644
@@ -480,6 +480,8 @@ void raw_object_store_clear(struct raw_object_store *o)
 {
        FREE_AND_NULL(o->objectdir);
        FREE_AND_NULL(o->alternate_db);
+
+       oidmap_free(o->replace_map, 1);
        FREE_AND_NULL(o->replace_map);
 
        free_alt_odbs(o);

[1]

$ ./git-branch-diff origin/sb/object-store-replace^^..origin/sb/object-store-replace sb/object-store-replace^^..sb/object-store-replace
1:  d5873072054 < -:  ----------- object.c: free replace map in raw_object_store_clear
-:  ----------- > 1:  88d456c5d24 object.c: free replace map in raw_object_store_clear
2:  74fd0705bb3 = 2:  6da07fe2f9c replace-object.c: remove the_repository from prepare_replace_object

Stefan Beller (2):
object.c: free replace map in raw_object_store_clear replace-object.c: remove the_repository from prepare_replace_object

 object.c         | 3 +++
 replace_object.c | 2 +-
 2 files changed, 4 insertions(+), 1 deletion(-)

2.17.0.582.gccdcbd54c44.dirty

The replace map for objects was missed to free in the object store in the conversion of c1274495 ("replace-object: eliminate replace objects prepared flag", 2018-04-11). We also missed to free the replaced objects that are put into the replace map in that whole series.

---
 object.c | 3 +++
 1 file changed, 3 insertions(+)

index 66cffaf6e51..97245fdea25 100644
@@ -481,6 +481,9 @@ void raw_object_store_clear(struct raw_object_store *o)
 	FREE_AND_NULL(o->objectdir);
 	FREE_AND_NULL(o->alternate_db);
 
+	oidmap_free(o->replace_map, 1);
+	FREE_AND_NULL(o->replace_map);
+
 	free_alt_odbs(o);
 	o->alt_odb_tail = NULL;
 

2.17.0.582.gccdcbd54c44.dirty

This was missed in 5982da9d2ce (replace-object: allow prepare_replace_object to handle arbitrary repositories, 2018-04-11)

Technically the code works correctly as the replace_map is the same size in different repositories, however it is hard to read. So convert the code to the familiar pattern of dereferencing the pointer that we assign in the sizeof itself.

---
 replace_object.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 246b98cd4f1..801b5c16789 100644
@@ -37,7 +37,7 @@ static void prepare_replace_object(struct repository *r)
 		return;
 
 	r->objects->replace_map =
-		xmalloc(sizeof(*the_repository->objects->replace_map));
+		xmalloc(sizeof(*r->objects->replace_map));
 	oidmap_init(r->objects->replace_map, 0);
 
 	for_each_replace_ref(r, register_replace_ref, NULL);

2.17.0.582.gccdcbd54c44.dirty

From: Leif Middelschulte <[email protected]>

Inform the user about an automatically fast-forwarded submodule. The silent merge behavior was introduced by commit 68d03e4a6e44 ("Implement automatic fast-forward merge for submodules", 2010-07-07)).

---
 merge-recursive.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

This is the resend, with an interdiff as follows.

Junio wrote in http://public-inbox.org/git/[email protected]:

which is also outstanding.

Leif can you pick this patch and resend it with a proper commit message?

    # diff --git c/merge-recursive.c w/merge-recursive.c
    # index 29a430c418a..a9aecccb8c3 100644
    # --- c/merge-recursive.c
    # +++ w/merge-recursive.c
    # @@ -1094,7 +1094,7 @@ static int merge_submodule(struct merge_options *o,
    #  	if (in_merge_bases(commit_a, commit_b)) {
    #  		oidcpy(result, b);
    #  		if (show(o, 3)) {
    # -			output(o, 1, _("Fast-forwarding submodule %s to the following commit:"), path);
    # +			output(o, 3, _("Fast-forwarding submodule %s to the following commit:"), path);
    #  			output_commit_title(o, commit_b);
    #  		} else if (show(o, 2))
    #  			output(o, 2, _("Fast-forwarding submodule %s to %s"), path, oid_to_hex(b));
    # @@ -1106,7 +1106,7 @@ static int merge_submodule(struct merge_options *o,
    #  	if (in_merge_bases(commit_b, commit_a)) {
    #  		oidcpy(result, a);
    #  		if (show(o, 3)) {
    # -			output(o, 1, _("Fast-forwarding submodule %s to the following commit:"), path);
    # +			output(o, 3, _("Fast-forwarding submodule %s to the following commit:"), path);
    #  			output_commit_title(o, commit_a);
    #  		} else if (show(o, 2))
    #  			output(o, 2, _("Fast-forwarding submodule %s to %s"), path, oid_to_hex(a));

index 0571919ee0a..a9aecccb8c3 100644
@@ -1093,10 +1093,26 @@ static int merge_submodule(struct merge_options *o,
 	/* Case #1: a is contained in b or vice versa */
 	if (in_merge_bases(commit_a, commit_b)) {
 		oidcpy(result, b);
+		if (show(o, 3)) {
+			output(o, 3, _("Fast-forwarding submodule %s to the following commit:"), path);
+			output_commit_title(o, commit_b);
+		} else if (show(o, 2))
+			output(o, 2, _("Fast-forwarding submodule %s to %s"), path, oid_to_hex(b));
+		else
+			; /* no output */
+
 		return 1;
 	}
 	if (in_merge_bases(commit_b, commit_a)) {
 		oidcpy(result, a);
+		if (show(o, 3)) {
+			output(o, 3, _("Fast-forwarding submodule %s to the following commit:"), path);
+			output_commit_title(o, commit_a);
+		} else if (show(o, 2))
+			output(o, 2, _("Fast-forwarding submodule %s to %s"), path, oid_to_hex(a));
+		else
+			; /* no output */
+
 		return 1;
 	}
 

2.17.0.582.gccdcbd54c44.dirty

From: Leif Middelschulte <[email protected]>

This is a follow-up on Junio C Hamano's comments [0] and Stefan Beller's request
[1] for a more explainatory/elaborate commit message.

[0] https://public-inbox.org/git/[email protected]/
[1] https://public-inbox.org/git/[email protected]/

Leif Middelschulte (1):
Inform about fast-forwarding of submodules during merge

 merge-recursive.c | 4 ++++
 1 file changed, 4 insertions(+)

2.15.1 (Apple Git-101)

From: Leif Middelschulte <[email protected]>

This is a follow-up on Junio C Hamano's comments [0] and Stefan Beller's request
[1] for a more explainatory/elaborate commit message.

[0] https://public-inbox.org/git/[email protected]/
[1] https://public-inbox.org/git/[email protected]/

Leif Middelschulte (1):
Inform about fast-forwarding of submodules during merge

 merge-recursive.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

2.15.1 (Apple Git-101)

From: Leif Middelschulte <[email protected]>

Silent fast-forwarding might lead to inconveniences in cases where submodules are expected to have a certain revision, because 'more recent' (therefore fast-forwardable) versions might break behavior/contain regressions.

A use-case is the integration (merge) phase as part of the feature-centric 'git-flow' workflow [0]. I.e. a feature might be well-tested with a certain submodule revision, but break because of regressions (or changes in general) within an updated version of the sourced submodule.

This change tries to support the integrator by telling her about another possible source of unexpected behavior (differing submodule versions) she might see during integration tests.

[0] http://nvie.com/posts/a-successful-git-branching-model/

---
 merge-recursive.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

index a4b91d17f..e2c99924d 100644
@@ -1093,10 +1093,26 @@ static int merge_submodule(struct merge_options *o,
 	/* Case #1: a is contained in b or vice versa */
 	if (in_merge_bases(commit_a, commit_b)) {
 		oidcpy(result, b);
+		if (show(o, 3)) {
+			output(o, 3, _("Fast-forwarding submodule %s to the following commit:"), path);
+			output_commit_title(o, commit_b);
+		} else if (show(o, 2))
+			output(o, 2, _("Fast-forwarding submodule %s to %s"), path, oid_to_hex(b));
+		else
+			; /* no output */
+
 		return 1;
 	}
 	if (in_merge_bases(commit_b, commit_a)) {
 		oidcpy(result, a);
+		if (show(o, 3)) {
+			output(o, 3, _("Fast-forwarding submodule %s to the following commit:"), path);
+			output_commit_title(o, commit_a);
+		} else if (show(o, 2))
+			output(o, 2, _("Fast-forwarding submodule %s to %s"), path, oid_to_hex(a));
+		else
+			; /* no output */
+
 		return 1;
 	}
 

2.15.1 (Apple Git-101)

Hi Leif,

Thanks for continuing to push on this. This looks good so far (to me), but I was also hoping to see the analogy between these messages and "Auto-merging $FILE" for regular files mentioned. Both Junio[1] and I[2] pointed out this similarity, and I think this similarity/analogy is useful additional motivation for making this change.

[1] https://public-inbox.org/git/[email protected]/
[2] https://public-inbox.org/git/CABPp-BGaibCPWuCnaX5Af=sv-2zvyhNcupT+-PkxHDfJBg_Vbw@mail.gmail.com/

...

... meaning that it should be discussed and named as the primary reason why this change is a good idea?

Re-reading what Leif wrote in the first paragraph, I tend to think that "the more recent version may break us" Leif gives is not a particularly convincing one. After all, if we did not change the commit bound at a submodule since we forked, while they changed it to something else (either old or new), even though our changes may have been fully tested with the version of the submodule we have been testing with, it may break with the version the merged branch has been using. Such an update is cleanly and silently resolved at the tree-level three-way merge, but the risk of breakage is no different to the case this patch adds new notices to.

More importantly, the same "the changes we made may get broken by changes in areas that are textually unrelated they made" will happen without submodules. Content-level three-way merges that resolves cleanly at the textual level may need to get semantic adjustment. Do we treat clean 3-way content merges as suspicious and give a similar warning? That smells like madness.

But as you said, we give "Auto-merging $FILE" notice to clean 3-way merge at the content-level for normal files, and there is no good reason why we should not do the same for submodules when one fast-forwards to the other, which is an analogue to the content-level 3-way merge where one branch's version is a superset of the other ones. And that is quite a convincing reason why a new "Auto-merging $SUBMODULE" notice is a good idea.

Now you meantion it, that part of the message does look more like a debugging aid than a feature that helps actual end-users. After all, if our side did not change the commit recorded for the submodule while their side changed, we do not report the result of such a tree-level three-way merge that takes what commit they had at their tip.

This is a reroll of sb/diff-color-move-more, with the test fixed as well as another extra patch, that would have caught the bad test.

The range diff is below.

Thanks,
Stefan

Stefan Beller (8):
xdiff/xdiff.h: remove unused flags
xdiff/xdiffi.c: remove unneeded function declarations diff.c: do not pass diff options as keydata to hashmap diff.c: adjust hash function signature to match hashmap expectation diff.c: add a blocks mode for moved code detection diff.c: decouple white space treatment from move detection algorithm diff.c: add --color-moved-ignore-space-delta option
diff: color-moved white space handling options imply color-moved

 Documentation/diff-options.txt |  25 ++++-
 diff.c                         | 138 +++++++++++++++++++----
 diff.h                         |   8 +-
 t/t4015-diff-whitespace.sh     | 197 +++++++++++++++++++++++++++++++--
 xdiff/xdiff.h                  |   8 --
 xdiff/xdiffi.c                 |  17 ---
 6 files changed, 336 insertions(+), 57 deletions(-)

2.17.0.582.gccdcbd54c44.dirty

1:  a7a7af6b76b = 1:  a7a7af6b76b xdiff/xdiff.h: remove unused flags
2:  a7b6aaf7bc0 = 2:  a7b6aaf7bc0 xdiff/xdiffi.c: remove unneeded function declarations
3:  d9e57cc6b05 = 3:  d9e57cc6b05 diff.c: do not pass diff options as keydata to hashmap
4:  87111ba726d = 4:  87111ba726d diff.c: adjust hash function signature to match hashmap expectation
5:  9559b8cb456 = 5:  9559b8cb456 diff.c: add a blocks mode for moved code detection
6:  41a70464209 = 6:  41a70464209 diff.c: decouple white space treatment from move detection algorithm
7:  c0114b2ce56 ! 7:  72bb8213cab diff.c: add --color-moved-ignore-space-delta option
    @@ -6,7 +6,6 @@
         changes uniformly.
     
         Signed-off-by: Stefan Beller <[email protected]>
    -    Signed-off-by: Junio C Hamano <[email protected]>
     
     diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
     --- a/Documentation/diff-options.txt
    @@ -237,7 +236,7 @@
     +	not adjust
     +	EOF
     +
    -+	git diff --color --color-moved-ignore-space-prefix-delta |
    ++	git diff --color --color-moved --color-moved-ignore-space-prefix-delta |
     +		grep -v "index" |
     +		test_decode_color >actual &&
     +
    @@ -246,20 +245,20 @@
     +		<BOLD>--- a/text.txt<RESET>
     +		<BOLD>+++ b/text.txt<RESET>
     +		<CYAN>@@ -1,7 +1,7 @@<RESET>
    -+		<RED>-QIndented<RESET>
    -+		<RED>-QText across<RESET>
    -+		<RED>-Qthree lines<RESET>
    ++		<BOLD;MAGENTA>-QIndented<RESET>
    ++		<BOLD;MAGENTA>-QText across<RESET>
    ++		<BOLD;MAGENTA>-Qthree lines<RESET>
     +		<RED>-QBut! <- this stands out<RESET>
    -+		<RED>-Qthis one<RESET>
    -+		<RED>-QQline did<RESET>
    -+		<RED>-Qnot adjust<RESET>
    -+		<GREEN>+<RESET>QQ<GREEN>Indented<RESET>
    -+		<GREEN>+<RESET>QQ<GREEN>Text across<RESET>
    -+		<GREEN>+<RESET>QQ<GREEN>three lines<RESET>
    ++		<BOLD;MAGENTA>-Qthis one<RESET>
    ++		<BOLD;MAGENTA>-QQline did<RESET>
    ++		<BOLD;MAGENTA>-Qnot adjust<RESET>
    ++		<BOLD;YELLOW>+<RESET>QQ<BOLD;YELLOW>Indented<RESET>
    ++		<BOLD;YELLOW>+<RESET>QQ<BOLD;YELLOW>Text across<RESET>
    ++		<BOLD;YELLOW>+<RESET>QQ<BOLD;YELLOW>three lines<RESET>
     +		<GREEN>+<RESET>QQQ<GREEN>But! <- this stands out<RESET>
    -+		<GREEN>+<RESET><GREEN>this one<RESET>
    -+		<GREEN>+<RESET>Q<GREEN>line did<RESET>
    -+		<GREEN>+<RESET><GREEN>not adjust<RESET>
    ++		<BOLD;YELLOW>+<RESET><BOLD;YELLOW>this one<RESET>
    ++		<BOLD;YELLOW>+<RESET>Q<BOLD;YELLOW>line did<RESET>
    ++		<BOLD;YELLOW>+<RESET><BOLD;YELLOW>not adjust<RESET>
     +	EOF
     +
     +	test_cmp expected actual
-:  ----------- > 8:  9e0508c8381 diff: color-moved white space handling options imply color-moved

These flags were there since the beginning (3443546f6e (Use a *real* built-in diff generator, 2006-03-24), but were never used. Remove them.

---
 xdiff/xdiff.h | 8 --------
 1 file changed, 8 deletions(-)

index c1937a29112..2356da5f784 100644
@@ -52,14 +52,6 @@ extern "C" {
 #define XDL_EMIT_FUNCNAMES (1 << 0)
 #define XDL_EMIT_FUNCCONTEXT (1 << 2)
 
-#define XDL_MMB_READONLY (1 << 0)
-
-#define XDL_MMF_ATOMIC (1 << 0)
-
-#define XDL_BDOP_INS 1
-#define XDL_BDOP_CPY 2
-#define XDL_BDOP_INSB 3
-
 /* merge simplification levels */
 #define XDL_MERGE_MINIMAL 0
 #define XDL_MERGE_EAGER 1

2.17.0.582.gccdcbd54c44.dirty

There is no need to forward-declare these functions, as they are used after their implementation only.

---
 xdiff/xdiffi.c | 17 -----------------
 1 file changed, 17 deletions(-)

index 0de1ef463bf..3e8aff92bc4 100644
@@ -22,34 +22,17 @@
 
 #include "xinclude.h"
 
-
-
 #define XDL_MAX_COST_MIN 256
 #define XDL_HEUR_MIN_COST 256
 #define XDL_LINE_MAX (long)((1UL << (CHAR_BIT * sizeof(long) - 1)) - 1)
 #define XDL_SNAKE_CNT 20
 #define XDL_K_HEUR 4
 
-
-
 typedef struct s_xdpsplit {
 	long i1, i2;
 	int min_lo, min_hi;
 } xdpsplit_t;
 
-
-
-
-static long xdl_split(unsigned long const *ha1, long off1, long lim1,
-		      unsigned long const *ha2, long off2, long lim2,
-		      long *kvdf, long *kvdb, int need_min, xdpsplit_t *spl,
-		      xdalgoenv_t *xenv);
-static xdchange_t *xdl_add_change(xdchange_t *xscr, long i1, long i2, long chg1, long chg2);
-
-
-
-
-
 /*
  * See "An O(ND) Difference Algorithm and its Variations", by Eugene Myers.
  * Basically considers a "box" (off1, off2, lim1, lim2) and scan from both

2.17.0.582.gccdcbd54c44.dirty

When we initialize the hashmap, we give it a pointer to the diff_options, which it then passes along to each call of the hashmap_cmp_fn function. There's no need to pass it a second time as the "keydata" parameter, and our comparison functions never look at keydata.

This was a mistake left over from an earlier round of 2e2d5ac184 (diff.c: color moved lines differently, 2017-06-30), before hashmap learned to pass the data pointer for us.

---
 diff.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 1289df4b1f9..ce7bedc1b92 100644
@@ -842,13 +842,13 @@ static void mark_color_as_moved(struct diff_options *o,
 		case DIFF_SYMBOL_PLUS:
 			hm = del_lines;
 			key = prepare_entry(o, n);
-			match = hashmap_get(hm, key, o);
+			match = hashmap_get(hm, key, NULL);
 			free(key);
 			break;
 		case DIFF_SYMBOL_MINUS:
 			hm = add_lines;
 			key = prepare_entry(o, n);
-			match = hashmap_get(hm, key, o);
+			match = hashmap_get(hm, key, NULL);
 			free(key);
 			break;
 		default:

2.17.0.582.gccdcbd54c44.dirty

This makes the follow up patch easier.

---
 diff.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

index ce7bedc1b92..d1bae900cdc 100644
@@ -707,11 +707,15 @@ struct moved_entry {
 	struct moved_entry *next_line;
 };
 
-static int moved_entry_cmp(const struct diff_options *diffopt,
-			   const struct moved_entry *a,
-			   const struct moved_entry *b,
+static int moved_entry_cmp(const void *hashmap_cmp_fn_data,
+			   const void *entry,
+			   const void *entry_or_key,
 			   const void *keydata)
 {
+	const struct diff_options *diffopt = hashmap_cmp_fn_data;
+	const struct moved_entry *a = entry;
+	const struct moved_entry *b = entry_or_key;
+
 	return !xdiff_compare_lines(a->es->line, a->es->len,
 				    b->es->line, b->es->len,
 				    diffopt->xdl_opts);

@@ -5534,10 +5538,8 @@ static void diff_flush_patch_all_file_pairs(struct diff_options *o)
 		if (o->color_moved) {
 			struct hashmap add_lines, del_lines;
 
-			hashmap_init(&del_lines,
-				     (hashmap_cmp_fn)moved_entry_cmp, o, 0);
-			hashmap_init(&add_lines,
-				     (hashmap_cmp_fn)moved_entry_cmp, o, 0);
+			hashmap_init(&del_lines, moved_entry_cmp, o, 0);
+			hashmap_init(&add_lines, moved_entry_cmp, o, 0);
 
 			add_lines_to_move_detection(o, &add_lines, &del_lines);
 			mark_color_as_moved(o, &add_lines, &del_lines);

2.17.0.582.gccdcbd54c44.dirty

The new "blocks" mode provides a middle ground between plain and zebra. It is as intuitive (few colors) as plain, but still has the requirement for a minimum of lines/characters to count a block as moved.

(https://public-inbox.org/git/[email protected]/)

---
 Documentation/diff-options.txt |  8 ++++--
 diff.c                         |  6 +++--
 diff.h                         |  5 ++--
 t/t4015-diff-whitespace.sh     | 48 +++++++++++++++++++++++++++++++++-
 4 files changed, 60 insertions(+), 7 deletions(-)

index e3a44f03cdc..bb9f1b7cd82 100644
@@ -276,10 +276,14 @@ plain::
 	that are added somewhere else in the diff. This mode picks up any
 	moved line, but it is not very useful in a review to determine
 	if a block of code was moved without permutation.
-zebra::
+blocks:
 	Blocks of moved text of at least 20 alphanumeric characters
 	are detected greedily. The detected blocks are
-	painted using either the 'color.diff.{old,new}Moved' color or
+	painted using either the 'color.diff.{old,new}Moved' color.
+	Adjacent blocks cannot be told apart.
+zebra::
+	Blocks of moved text are detected as in 'blocks' mode. The blocks
+	are painted using either the 'color.diff.{old,new}Moved' color or
 	'color.diff.{old,new}MovedAlternative'. The change between
 	the two colors indicates that a new block was detected.
 dimmed_zebra::

index d1bae900cdc..95c51c0b7df 100644
@@ -271,6 +271,8 @@ static int parse_color_moved(const char *arg)
 		return COLOR_MOVED_NO;
 	else if (!strcmp(arg, "plain"))
 		return COLOR_MOVED_PLAIN;
+	else if (!strcmp(arg, "blocks"))
+		return COLOR_MOVED_BLOCKS;
 	else if (!strcmp(arg, "zebra"))
 		return COLOR_MOVED_ZEBRA;
 	else if (!strcmp(arg, "default"))

@@ -278,7 +280,7 @@ static int parse_color_moved(const char *arg)
 	else if (!strcmp(arg, "dimmed_zebra"))
 		return COLOR_MOVED_ZEBRA_DIM;
 	else
-		return error(_("color moved setting must be one of 'no', 'default', 'zebra', 'dimmed_zebra', 'plain'"));
+		return error(_("color moved setting must be one of 'no', 'default', 'blocks', 'zebra', 'dimmed_zebra', 'plain'"));
 }
 
 int git_diff_ui_config(const char *var, const char *value, void *cb)

@@ -903,7 +905,7 @@ static void mark_color_as_moved(struct diff_options *o,
 
 		block_length++;
 
-		if (flipped_block)
+		if (flipped_block && o->color_moved != COLOR_MOVED_BLOCKS)
 			l->flags |= DIFF_SYMBOL_MOVED_LINE_ALT;
 	}
 	adjust_last_block(o, n, block_length);

index d29560f822c..7bd4f182c33 100644
@@ -208,8 +208,9 @@ struct diff_options {
 	enum {
 		COLOR_MOVED_NO = 0,
 		COLOR_MOVED_PLAIN = 1,
-		COLOR_MOVED_ZEBRA = 2,
-		COLOR_MOVED_ZEBRA_DIM = 3,
+		COLOR_MOVED_BLOCKS = 2,
+		COLOR_MOVED_ZEBRA = 3,
+		COLOR_MOVED_ZEBRA_DIM = 4,
 	} color_moved;
 	#define COLOR_MOVED_DEFAULT COLOR_MOVED_ZEBRA
 	#define COLOR_MOVED_MIN_ALNUM_COUNT 20

2.17.0.582.gccdcbd54c44.dirty

In the original implementation of the move detection logic the choice for ignoring white space changes is the same for the move detection as it is for the regular diff. Some cases came up where different treatment would have been nice.

Allow the user to specify that whitespace should be ignored differently during detection of moved lines than during generation of added and removed lines. This is done by providing analogs to the --ignore-space-at-eol, -b, and -w options (namely,
--color-moved-[no-]ignore-space-at-eol --color-moved-[no-]ignore-space-change --color-moved-[no-]ignore-all-space) that affect only the color of the
output, and making the existing --ignore-space-at-eol, -b, and -w options no longer affect the color of the output.

As we change the default, we'll adjust the tests.

For now we do not infer any options to treat whitespaces in the move detection from the generic white space options given to diff. This can be tuned later to reasonable default.

---
 Documentation/diff-options.txt | 13 +++++
 diff.c                         | 19 ++++++-
 diff.h                         |  1 +
 t/t4015-diff-whitespace.sh     | 90 +++++++++++++++++++++++++++++++---
 4 files changed, 114 insertions(+), 9 deletions(-)

index bb9f1b7cd82..7b2527b9a19 100644
@@ -292,6 +292,19 @@ dimmed_zebra::
 	blocks are considered interesting, the rest is uninteresting.
 --
 
+--color-moved-[no-]ignore-space-at-eol::
+	Ignore changes in whitespace at EOL when performing the move
+	detection for --color-moved.
+--color-moved-[no-]ignore-space-change::
+	Ignore changes in amount of whitespace when performing the move
+	detection for --color-moved.  This ignores whitespace
+	at line end, and considers all other sequences of one or
+	more whitespace characters to be equivalent.
+--color-moved-[no-]ignore-all-space::
+	Ignore whitespace when comparing lines when performing the move
+	detection for --color-moved.  This ignores differences even if
+	one line has whitespace where the other line has none.
+
 --word-diff[=<mode>]::
 	Show a word diff, using the <mode> to delimit changed words.
 	By default, words are delimited by whitespace; see

index 95c51c0b7df..b5819dd538f 100644
@@ -717,10 +717,12 @@ static int moved_entry_cmp(const void *hashmap_cmp_fn_data,
 	const struct diff_options *diffopt = hashmap_cmp_fn_data;
 	const struct moved_entry *a = entry;
 	const struct moved_entry *b = entry_or_key;
+	unsigned flags = diffopt->color_moved_ws_handling
+			 & XDF_WHITESPACE_FLAGS;
 
 	return !xdiff_compare_lines(a->es->line, a->es->len,
 				    b->es->line, b->es->len,
-				    diffopt->xdl_opts);
+				    flags);
 }
 
 static struct moved_entry *prepare_entry(struct diff_options *o,

@@ -728,8 +730,9 @@ static struct moved_entry *prepare_entry(struct diff_options *o,
 {
 	struct moved_entry *ret = xmalloc(sizeof(*ret));
 	struct emitted_diff_symbol *l = &o->emitted_symbols->buf[line_no];
+	unsigned flags = o->color_moved_ws_handling & XDF_WHITESPACE_FLAGS;
 
-	ret->ent.hash = xdiff_hash_string(l->line, l->len, o->xdl_opts);
+	ret->ent.hash = xdiff_hash_string(l->line, l->len, flags);
 	ret->es = l;
 	ret->next_line = NULL;
 

@@ -4638,6 +4641,18 @@ int diff_opt_parse(struct diff_options *options,
 		DIFF_XDL_SET(options, IGNORE_CR_AT_EOL);
 	else if (!strcmp(arg, "--ignore-blank-lines"))
 		DIFF_XDL_SET(options, IGNORE_BLANK_LINES);
+	else if (!strcmp(arg, "--color-moved-no-ignore-all-space"))
+		options->color_moved_ws_handling &= ~XDF_IGNORE_WHITESPACE;
+	else if (!strcmp(arg, "--color-moved-no-ignore-space-change"))
+		options->color_moved_ws_handling &= ~XDF_IGNORE_WHITESPACE_CHANGE;
+	else if (!strcmp(arg, "--color-moved-no-ignore-space-at-eol"))
+		options->color_moved_ws_handling &= ~XDF_IGNORE_WHITESPACE_AT_EOL;
+	else if (!strcmp(arg, "--color-moved-ignore-all-space"))
+		options->color_moved_ws_handling |= XDF_IGNORE_WHITESPACE;
+	else if (!strcmp(arg, "--color-moved-ignore-space-change"))
+		options->color_moved_ws_handling |= XDF_IGNORE_WHITESPACE_CHANGE;
+	else if (!strcmp(arg, "--color-moved-ignore-space-at-eol"))
+		options->color_moved_ws_handling |= XDF_IGNORE_WHITESPACE_AT_EOL;
 	else if (!strcmp(arg, "--indent-heuristic"))
 		DIFF_XDL_SET(options, INDENT_HEURISTIC);
 	else if (!strcmp(arg, "--no-indent-heuristic"))

index 7bd4f182c33..de5dc680051 100644
@@ -214,6 +214,7 @@ struct diff_options {
 	} color_moved;
 	#define COLOR_MOVED_DEFAULT COLOR_MOVED_ZEBRA
 	#define COLOR_MOVED_MIN_ALNUM_COUNT 20
+	int color_moved_ws_handling;
 };
 
 void diff_emit_submodule_del(struct diff_options *o, const char *line);

2.17.0.582.gccdcbd54c44.dirty

That makes sense.

Stepping back a bit, looking for similar precedents, we have lots of "[no-]" strings in our documentation. But that is ok, as we the prefix is at the beginning of the option ("--[no-]foo-bar"), such that searching for the whole option without the leading dashes ("foo-bar") will still find the option.

So maybe another option would be rename the negative options to "--no-color-moved-ignore-space-at-eol", such that the documentation could fall back to the old pattern of "--[no-]long-name".

Initially I was tempted on not choosing such names, as I viewed all this white space options specific to the color-move feature, such that a prefix of "--color-moved" might be desirable. Turning off one sub-feature in that feature would naturally be --feature-no-subfeature instead of negating the whole feature.

I also cannot find a good existing example for subfeatures in features, they would usually come as --feature=<option1, option2>

Undecided,
Stefan

This marks moved code still as blocks when their indentation level changes uniformly.

---
 Documentation/diff-options.txt |  4 ++
 diff.c                         | 83 +++++++++++++++++++++++++++++++---
 diff.h                         |  2 +
 t/t4015-diff-whitespace.sh     | 54 ++++++++++++++++++++++
 4 files changed, 137 insertions(+), 6 deletions(-)

index 7b2527b9a19..facdbc8f95f 100644
@@ -304,6 +304,10 @@ dimmed_zebra::
 	Ignore whitespace when comparing lines when performing the move
 	detection for --color-moved.  This ignores differences even if
 	one line has whitespace where the other line has none.
+--color-moved-[no-]ignore-space-prefix-delta::
+	Ignores whitespace when comparing lines when performing the move
+	detection for --color-moved. This ignores uniform differences
+	of white space at the beginning lines in moved blocks.
 
 --word-diff[=<mode>]::
 	Show a word diff, using the <mode> to delimit changed words.

index de5dc680051..b00ea76c083 100644
@@ -214,6 +214,8 @@ struct diff_options {
 	} color_moved;
 	#define COLOR_MOVED_DEFAULT COLOR_MOVED_ZEBRA
 	#define COLOR_MOVED_MIN_ALNUM_COUNT 20
+	/* XDF_WHITESPACE_FLAGS regarding block detection are set at 2, 3, 4 */
+	#define COLOR_MOVED_DELTA_WHITESPACES	(1 << 22)
 	int color_moved_ws_handling;
 };
 

2.17.0.582.gccdcbd54c44.dirty

When giving options how move coloring should treat white spaces, the user expects that move coloring is actually active. If it is not active, we can just take the default mode.

---
 diff.c                     | 18 +++++++++++++-----
 t/t4015-diff-whitespace.sh |  9 +++++++--
 2 files changed, 20 insertions(+), 7 deletions(-)

index 1227a4d2a83..a10184e576e 100644
@@ -4713,15 +4713,23 @@ int diff_opt_parse(struct diff_options *options,
 		options->color_moved_ws_handling &= ~XDF_IGNORE_WHITESPACE_AT_EOL;
 	else if (!strcmp(arg, "--color-moved-no-ignore-space-prefix-delta"))
 		options->color_moved_ws_handling &= ~COLOR_MOVED_DELTA_WHITESPACES;
-	else if (!strcmp(arg, "--color-moved-ignore-all-space"))
+	else if (!strcmp(arg, "--color-moved-ignore-all-space")) {
+		if (options->color_moved == COLOR_MOVED_NO)
+			options->color_moved = COLOR_MOVED_DEFAULT;
 		options->color_moved_ws_handling |= XDF_IGNORE_WHITESPACE;
-	else if (!strcmp(arg, "--color-moved-ignore-space-change"))
+	} else if (!strcmp(arg, "--color-moved-ignore-space-change")) {
 		options->color_moved_ws_handling |= XDF_IGNORE_WHITESPACE_CHANGE;
-	else if (!strcmp(arg, "--color-moved-ignore-space-at-eol"))
+		if (options->color_moved == COLOR_MOVED_NO)
+			options->color_moved = COLOR_MOVED_DEFAULT;
+	} else if (!strcmp(arg, "--color-moved-ignore-space-at-eol")) {
 		options->color_moved_ws_handling |= XDF_IGNORE_WHITESPACE_AT_EOL;
-	else if (!strcmp(arg, "--color-moved-ignore-space-prefix-delta"))
+		if (options->color_moved == COLOR_MOVED_NO)
+			options->color_moved = COLOR_MOVED_DEFAULT;
+	} else if (!strcmp(arg, "--color-moved-ignore-space-prefix-delta")) {
 		options->color_moved_ws_handling |= COLOR_MOVED_DELTA_WHITESPACES;
-	else if (!strcmp(arg, "--indent-heuristic"))
+		if (options->color_moved == COLOR_MOVED_NO)
+			options->color_moved = COLOR_MOVED_DEFAULT;
+	} else if (!strcmp(arg, "--indent-heuristic"))
 		DIFF_XDL_SET(options, INDENT_HEURISTIC);
 	else if (!strcmp(arg, "--no-indent-heuristic"))
 		DIFF_XDL_CLR(options, INDENT_HEURISTIC);

index 37ff528822f..e1ab08d7bb4 100755
@@ -1875,7 +1875,11 @@ test_expect_success 'compare whitespace delta across moved blocks' '
 
 	git diff --color --color-moved --color-moved-ignore-space-prefix-delta |
 		grep -v "index" |
-		test_decode_color >actual &&
+		test_decode_color >actual1 &&
+
+	git diff --color --color-moved-ignore-space-prefix-delta |
+		grep -v "index" |
+		test_decode_color >actual2 &&
 
 	q_to_tab <<-\EOF >expected &&
 		<BOLD>diff --git a/text.txt b/text.txt<RESET>

@@ -1898,7 +1902,8 @@ test_expect_success 'compare whitespace delta across moved blocks' '
 		<BOLD;YELLOW>+<RESET><BOLD;YELLOW>not adjust<RESET>
 	EOF
 
-	test_cmp expected actual
+	test_cmp expected actual1 &&
+	test_cmp expected actual2
 '
 
 test_done

2.17.0.582.gccdcbd54c44.dirty

The test in patch 7 is indeed fixed, and patch 8 looks good to me.

There are still some review comments of mine outstanding [1] [2] but if we decide that this series is good enough for now, that's OK.

[1] https://public-inbox.org/git/[email protected]/
[2] https://public-inbox.org/git/[email protected]/

I've been using this locally, and it's really nice. One question I had, are there plans to make the whitespace options configurable? I really like the option for enabling lines to count as moved when the whitespace changes uniformly, (it helps make changes more obvious when doing indentation changes such as wrapping code within a block). However, it's rather a long option name to type out. I didn't see any obvious config options to enable it by default though.

Thanks,
Jake

Please do not replace what already hit 'next'.

I missed that it hit next. Sorry about that. I'll roll a patch on top then.

Here is what I made these two into a relative fix on top.

-- >8 --
From: Stefan Beller <[email protected]>
Date: Thu, 17 May 2018 11:29:57 -0700
Subject: [PATCH] object.c: clear replace map before freeing it

---
 object.c | 2 ++
 1 file changed, 2 insertions(+)

index cdf520084d..97245fdea2 100644
@@ -480,6 +480,8 @@ void raw_object_store_clear(struct raw_object_store *o)
 {
 	FREE_AND_NULL(o->objectdir);
 	FREE_AND_NULL(o->alternate_db);
+
+	oidmap_free(o->replace_map, 1);
 	FREE_AND_NULL(o->replace_map);
 
 	free_alt_odbs(o);

2.17.0-582-gccdcbd54c4

Thanks, I was about to prepare the same.