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

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

[Junio C Hamano]

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. <f3b891c3-381f-de42-51d8-24fdfbca91d2@gmail.com>
 cf. <xmqq603yn50l.fsf@gitster-ct.c.googlers.com>
 cf. <xmqqzi1alodz.fsf@gitster-ct.c.googlers.com>
 cf. <xmqqvabylnbi.fsf@gitster-ct.c.googlers.com>


* 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. <9895c7b7-eac4-28c1-90c6-443acd1131b7@talktalk.net>


* 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. <20171204171308.GA13332@sigill.intra.peff.net>


* 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. <nycvar.QRO.7.76.6.1805052351560.77@tvgsbejvaqbjf.bet>


* 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. <20180507090109.GA367@sigill.intra.peff.net>


* 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. <20180507115950.3887-1-szeder.dev@gmail.com>


* 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. <0de30972-b0a2-67e8-7cff-c19daf9ece8b@gmail.com>

jk/branch-l-0-deprecation (was Re: What's cooking in git.git (May 2018, #02; Thu, 17))

[Kaartic Sivaraam]

[-- Attachment #1.1: Type: text/plain, Size: 2261 bytes --]

On Thursday 17 May 2018 11:31 AM, Junio C Hamano wrote:
> * jk/branch-l-0-deprecation (2018-03-26) 3 commits
> 
> ...
>
>  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?

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!


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: jk/branch-l-0-deprecation (was Re: What's cooking in git.git (May 2018, #02; Thu, 17))

[Ævar Arnfjörð Bjarmason]

On Thu, May 17 2018, Kaartic Sivaraam wrote:

> On Thursday 17 May 2018 11:31 AM, Junio C Hamano wrote:
>> * jk/branch-l-0-deprecation (2018-03-26) 3 commits
>>
>> ...
>>
>>  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?
>
> 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.

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.

Re: jk/branch-l-0-deprecation (was Re: What's cooking in git.git (May 2018, #02; Thu, 17))

[Kaartic Sivaraam]

[-- Attachment #1.1: Type: text/plain, Size: 2207 bytes --]

Hi Ævar,

On Thursday 17 May 2018 03:18 PM, Ævar Arnfjörð Bjarmason wrote:
> 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 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.
>
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!


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: jk/branch-l-0-deprecation (was Re: What's cooking in git.git (May 2018, #02; Thu, 17))

[Ævar Arnfjörð Bjarmason]

On Thu, May 17 2018, Kaartic Sivaraam wrote:

> Hi Ævar,
>
> On Thursday 17 May 2018 03:18 PM, Ævar Arnfjörð Bjarmason wrote:
>> 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 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.
>>
> 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.

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

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

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.

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

[Derrick Stolee]

On 5/17/2018 2:01 AM, Junio C Hamano wrote:
> * 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.

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

Re: jk/branch-l-0-deprecation (was Re: What's cooking in git.git (May 2018, #02; Thu, 17))

[Jeff King]

On Thu, May 17, 2018 at 11:48:33AM +0200, Ævar Arnfjörð Bjarmason wrote:

> > 	    If there are ample branches, the warning message might be
> > 	    hidden out of screen but we shouldn't rely on that, I
> > 	    suppose.
> 
> Also if we have lots of branches, depending on your pager settings you
> won't see this at all, I have:

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:

diff --git a/builtin/branch.c b/builtin/branch.c
index 452742fecf..f51b89e962 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -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

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

[Stefan Beller]

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

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/20180510195849.28023-4-sbeller@google.com/
I'll resend shortly.

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

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.

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

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

[PATCH 0/2] Reroll 2 last commits of sb/object-store-replace

[Stefan Beller]

>> * 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'.
>
>Please do not.

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
diff --git c/object.c w/object.c
index cdf520084df..97245fdea25 100644
--- c/object.c
+++ w/object.c
@@ -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

[PATCH 1/2] object.c: free replace map in raw_object_store_clear

[Stefan Beller]

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.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 object.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/object.c b/object.c
index 66cffaf6e51..97245fdea25 100644
--- a/object.c
+++ b/object.c
@@ -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

[PATCH 2/2] replace-object.c: remove the_repository from prepare_replace_object

[Stefan Beller]

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.

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 replace_object.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/replace_object.c b/replace_object.c
index 246b98cd4f1..801b5c16789 100644
--- a/replace_object.c
+++ b/replace_object.c
@@ -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

[PATCH] merge-recursive: give notice when submodule commit gets fast-forwarded

[Stefan Beller]

From: Leif Middelschulte <Leif.Middelschulte@gmail.com>

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

Signed-off-by: Leif Middelschulte <Leif.Middelschulte@gmail.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 merge-recursive.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

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

This is the resend, with an interdiff as follows.

Junio wrote in http://public-inbox.org/git/xmqqk1s474vx.fsf@gitster-ct.c.googlers.com:

> Perhaps Leif can elaborate why this change is a good idea in the
> first place?

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));


diff --git a/merge-recursive.c b/merge-recursive.c
index 0571919ee0a..a9aecccb8c3 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -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

[PATCH 0/8] Reroll of sb/diff-color-move-more

[Stefan Beller]

>> * sb/diff-color-move-more (2018-04-25) 7 commits
>...
>>
>>  Will merge to 'next'.
>
>I did not get around to fix it up, there are still review
>comments outstanding. (The test is broken in the last commit.)

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 <sbeller@google.com>
    -    Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
     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

[PATCH 1/8] xdiff/xdiff.h: remove unused flags

[Stefan Beller]

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

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 xdiff/xdiff.h | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h
index c1937a29112..2356da5f784 100644
--- a/xdiff/xdiff.h
+++ b/xdiff/xdiff.h
@@ -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

[PATCH 2/8] xdiff/xdiffi.c: remove unneeded function declarations

[Stefan Beller]

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

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 xdiff/xdiffi.c | 17 -----------------
 1 file changed, 17 deletions(-)

diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c
index 0de1ef463bf..3e8aff92bc4 100644
--- a/xdiff/xdiffi.c
+++ b/xdiff/xdiffi.c
@@ -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

[PATCH 3/8] diff.c: do not pass diff options as keydata to hashmap

[Stefan Beller]

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.

Explanation-by: Jeff King <peff@peff.net>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 diff.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/diff.c b/diff.c
index 1289df4b1f9..ce7bedc1b92 100644
--- a/diff.c
+++ b/diff.c
@@ -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

[PATCH 4/8] diff.c: adjust hash function signature to match hashmap expectation

[Stefan Beller]

This makes the follow up patch easier.

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 diff.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/diff.c b/diff.c
index ce7bedc1b92..d1bae900cdc 100644
--- a/diff.c
+++ b/diff.c
@@ -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

[PATCH 5/8] diff.c: add a blocks mode for moved code detection

[Stefan Beller]

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.

Suggested-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
 (https://public-inbox.org/git/87o9j0uljo.fsf@evledraar.gmail.com/)
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/diff-options.txt |  8 ++++--
 diff.c                         |  6 +++--
 diff.h                         |  5 ++--
 t/t4015-diff-whitespace.sh     | 48 +++++++++++++++++++++++++++++++++-
 4 files changed, 60 insertions(+), 7 deletions(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index e3a44f03cdc..bb9f1b7cd82 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -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::
diff --git a/diff.c b/diff.c
index d1bae900cdc..95c51c0b7df 100644
--- a/diff.c
+++ b/diff.c
@@ -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);
diff --git a/diff.h b/diff.h
index d29560f822c..7bd4f182c33 100644
--- a/diff.h
+++ b/diff.h
@@ -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
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 17df491a3ab..45091abb192 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -1223,7 +1223,7 @@ test_expect_success 'plain moved code, inside file' '
 	test_cmp expected actual
 '
 
-test_expect_success 'detect permutations inside moved code -- dimmed_zebra' '
+test_expect_success 'detect blocks of moved code' '
 	git reset --hard &&
 	cat <<-\EOF >lines.txt &&
 		long line 1
@@ -1271,6 +1271,52 @@ test_expect_success 'detect permutations inside moved code -- dimmed_zebra' '
 	test_config color.diff.newMovedDimmed "normal cyan" &&
 	test_config color.diff.oldMovedAlternativeDimmed "normal blue" &&
 	test_config color.diff.newMovedAlternativeDimmed "normal yellow" &&
+
+	git diff HEAD --no-renames --color-moved=blocks --color |
+		grep -v "index" |
+		test_decode_color >actual &&
+	cat <<-\EOF >expected &&
+	<BOLD>diff --git a/lines.txt b/lines.txt<RESET>
+	<BOLD>--- a/lines.txt<RESET>
+	<BOLD>+++ b/lines.txt<RESET>
+	<CYAN>@@ -1,16 +1,16 @@<RESET>
+	<MAGENTA>-long line 1<RESET>
+	<MAGENTA>-long line 2<RESET>
+	<MAGENTA>-long line 3<RESET>
+	 line 4<RESET>
+	 line 5<RESET>
+	 line 6<RESET>
+	 line 7<RESET>
+	 line 8<RESET>
+	 line 9<RESET>
+	<CYAN>+<RESET><CYAN>long line 1<RESET>
+	<CYAN>+<RESET><CYAN>long line 2<RESET>
+	<CYAN>+<RESET><CYAN>long line 3<RESET>
+	<CYAN>+<RESET><CYAN>long line 14<RESET>
+	<CYAN>+<RESET><CYAN>long line 15<RESET>
+	<CYAN>+<RESET><CYAN>long line 16<RESET>
+	 line 10<RESET>
+	 line 11<RESET>
+	 line 12<RESET>
+	 line 13<RESET>
+	<MAGENTA>-long line 14<RESET>
+	<MAGENTA>-long line 15<RESET>
+	<MAGENTA>-long line 16<RESET>
+	EOF
+	test_cmp expected actual
+
+'
+
+test_expect_success 'detect permutations inside moved code -- dimmed_zebra' '
+	# reuse setup from test before!
+	test_config color.diff.oldMoved "magenta" &&
+	test_config color.diff.newMoved "cyan" &&
+	test_config color.diff.oldMovedAlternative "blue" &&
+	test_config color.diff.newMovedAlternative "yellow" &&
+	test_config color.diff.oldMovedDimmed "normal magenta" &&
+	test_config color.diff.newMovedDimmed "normal cyan" &&
+	test_config color.diff.oldMovedAlternativeDimmed "normal blue" &&
+	test_config color.diff.newMovedAlternativeDimmed "normal yellow" &&
 	git diff HEAD --no-renames --color-moved=dimmed_zebra --color |
 		grep -v "index" |
 		test_decode_color >actual &&
-- 
2.17.0.582.gccdcbd54c44.dirty

[PATCH 6/8] diff.c: decouple white space treatment from move detection algorithm

[Stefan Beller]

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.

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/diff-options.txt | 13 +++++
 diff.c                         | 19 ++++++-
 diff.h                         |  1 +
 t/t4015-diff-whitespace.sh     | 90 +++++++++++++++++++++++++++++++---
 4 files changed, 114 insertions(+), 9 deletions(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index bb9f1b7cd82..7b2527b9a19 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -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
diff --git a/diff.c b/diff.c
index 95c51c0b7df..b5819dd538f 100644
--- a/diff.c
+++ b/diff.c
@@ -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"))
diff --git a/diff.h b/diff.h
index 7bd4f182c33..de5dc680051 100644
--- a/diff.h
+++ b/diff.h
@@ -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);
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 45091abb192..751fc478dde 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -1441,7 +1441,10 @@ test_expect_success 'move detection ignoring whitespace ' '
 	line 4
 	line 5
 	EOF
-	git diff HEAD --no-renames --color-moved --color |
+	git diff HEAD --no-renames --color-moved --color \
+		--color-moved-no-ignore-all-space \
+		--color-moved-no-ignore-space-change \
+		--color-moved-no-ignore-space-at-eol |
 		grep -v "index" |
 		test_decode_color >actual &&
 	cat <<-\EOF >expected &&
@@ -1465,7 +1468,10 @@ test_expect_success 'move detection ignoring whitespace ' '
 	EOF
 	test_cmp expected actual &&
 
-	git diff HEAD --no-renames -w --color-moved --color |
+	git diff HEAD --no-renames --color-moved --color \
+		--color-moved-ignore-all-space \
+		--color-moved-no-ignore-space-change \
+		--color-moved-no-ignore-space-at-eol |
 		grep -v "index" |
 		test_decode_color >actual &&
 	cat <<-\EOF >expected &&
@@ -1505,7 +1511,10 @@ test_expect_success 'move detection ignoring whitespace changes' '
 	line 5
 	EOF
 
-	git diff HEAD --no-renames --color-moved --color |
+	git diff HEAD --no-renames --color-moved --color \
+		--color-moved-no-ignore-all-space \
+		--color-moved-no-ignore-space-change \
+		--color-moved-no-ignore-space-at-eol |
 		grep -v "index" |
 		test_decode_color >actual &&
 	cat <<-\EOF >expected &&
@@ -1529,7 +1538,10 @@ test_expect_success 'move detection ignoring whitespace changes' '
 	EOF
 	test_cmp expected actual &&
 
-	git diff HEAD --no-renames -b --color-moved --color |
+	git diff HEAD --no-renames --color-moved --color \
+		--color-moved-no-ignore-all-space \
+		--color-moved-no-ignore-space-at-eol \
+		--color-moved-ignore-space-change |
 		grep -v "index" |
 		test_decode_color >actual &&
 	cat <<-\EOF >expected &&
@@ -1572,7 +1584,10 @@ test_expect_success 'move detection ignoring whitespace at eol' '
 	# avoid cluttering the output with complaints about our eol whitespace
 	test_config core.whitespace -blank-at-eol &&
 
-	git diff HEAD --no-renames --color-moved --color |
+	git diff HEAD --no-renames --color-moved --color \
+		--color-moved-no-ignore-all-space \
+		--color-moved-no-ignore-space-change \
+		--color-moved-no-ignore-space-at-eol |
 		grep -v "index" |
 		test_decode_color >actual &&
 	cat <<-\EOF >expected &&
@@ -1596,7 +1611,10 @@ test_expect_success 'move detection ignoring whitespace at eol' '
 	EOF
 	test_cmp expected actual &&
 
-	git diff HEAD --no-renames --ignore-space-at-eol --color-moved --color |
+	git diff HEAD --no-renames --color-moved --color \
+		--color-moved-no-ignore-all-space \
+		--color-moved-no-ignore-space-change \
+		--color-moved-ignore-space-at-eol |
 		grep -v "index" |
 		test_decode_color >actual &&
 	cat <<-\EOF >expected &&
@@ -1768,7 +1786,65 @@ test_expect_success 'move detection with submodules' '
 
 	# nor did we mess with it another way
 	git diff --submodule=diff --color | test_decode_color >expect &&
-	test_cmp expect decoded_actual
+	test_cmp expect decoded_actual &&
+	rm -rf bananas &&
+	git submodule deinit bananas
+'
+
+test_expect_success 'only move detection ignores white spaces' '
+	git reset --hard &&
+	q_to_tab <<-\EOF >text.txt &&
+		a long line to exceed per-line minimum
+		another long line to exceed per-line minimum
+		original file
+	EOF
+	git add text.txt &&
+	git commit -m "add text" &&
+	q_to_tab <<-\EOF >text.txt &&
+		Qa long line to exceed per-line minimum
+		Qanother long line to exceed per-line minimum
+		new file
+	EOF
+
+	# Make sure we get a different diff using -w
+	git diff --color --color-moved -w \
+		--color-moved-no-ignore-all-space \
+		--color-moved-no-ignore-space-change \
+		--color-moved-no-ignore-space-at-eol |
+		grep -v "index" |
+		test_decode_color >actual &&
+	q_to_tab <<-\EOF >expected &&
+	<BOLD>diff --git a/text.txt b/text.txt<RESET>
+	<BOLD>--- a/text.txt<RESET>
+	<BOLD>+++ b/text.txt<RESET>
+	<CYAN>@@ -1,3 +1,3 @@<RESET>
+	 Qa long line to exceed per-line minimum<RESET>
+	 Qanother long line to exceed per-line minimum<RESET>
+	<RED>-original file<RESET>
+	<GREEN>+<RESET><GREEN>new file<RESET>
+	EOF
+	test_cmp expected actual &&
+
+	# And now ignoring white space only in the move detection
+	git diff --color --color-moved \
+		--color-moved-ignore-all-space \
+		--color-moved-ignore-space-change \
+		--color-moved-ignore-space-at-eol |
+		grep -v "index" |
+		test_decode_color >actual &&
+	q_to_tab <<-\EOF >expected &&
+	<BOLD>diff --git a/text.txt b/text.txt<RESET>
+	<BOLD>--- a/text.txt<RESET>
+	<BOLD>+++ b/text.txt<RESET>
+	<CYAN>@@ -1,3 +1,3 @@<RESET>
+	<BOLD;MAGENTA>-a long line to exceed per-line minimum<RESET>
+	<BOLD;MAGENTA>-another long line to exceed per-line minimum<RESET>
+	<RED>-original file<RESET>
+	<BOLD;YELLOW>+<RESET>Q<BOLD;YELLOW>a long line to exceed per-line minimum<RESET>
+	<BOLD;YELLOW>+<RESET>Q<BOLD;YELLOW>another long line to exceed per-line minimum<RESET>
+	<GREEN>+<RESET><GREEN>new file<RESET>
+	EOF
+	test_cmp expected actual
 '
 
 test_done
-- 
2.17.0.582.gccdcbd54c44.dirty

[PATCH 7/8] diff.c: add --color-moved-ignore-space-delta option

[Stefan Beller]

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

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 Documentation/diff-options.txt |  4 ++
 diff.c                         | 83 +++++++++++++++++++++++++++++++---
 diff.h                         |  2 +
 t/t4015-diff-whitespace.sh     | 54 ++++++++++++++++++++++
 4 files changed, 137 insertions(+), 6 deletions(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 7b2527b9a19..facdbc8f95f 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -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.
diff --git a/diff.c b/diff.c
index b5819dd538f..1227a4d2a83 100644
--- a/diff.c
+++ b/diff.c
@@ -709,6 +709,31 @@ struct moved_entry {
 	struct moved_entry *next_line;
 };
 
+struct ws_delta {
+	char *string; /* The prefix delta, which is the same in the block */
+	int direction; /* adding or removing the line? */
+	int missmatch; /* in the remainder */
+};
+#define WS_DELTA_INIT { NULL, 0, 0 }
+
+static void compute_ws_delta(const struct emitted_diff_symbol *a,
+			     const struct emitted_diff_symbol *b,
+			     struct ws_delta *out)
+{
+	const struct emitted_diff_symbol *longer =  a->len > b->len ? a : b;
+	const struct emitted_diff_symbol *shorter = a->len > b->len ? b : a;
+	int d = longer->len - shorter->len;
+
+	out->missmatch = !memcmp(longer->line + d, shorter->line, shorter->len);
+	out->string = xmemdupz(longer->line, d);
+	out->direction = (a == longer);
+}
+
+static int compare_ws_delta(const struct ws_delta *a, const struct ws_delta *b)
+{
+	return a->direction == b->direction && !strcmp(a->string, b->string);
+}
+
 static int moved_entry_cmp(const void *hashmap_cmp_fn_data,
 			   const void *entry,
 			   const void *entry_or_key,
@@ -720,6 +745,15 @@ static int moved_entry_cmp(const void *hashmap_cmp_fn_data,
 	unsigned flags = diffopt->color_moved_ws_handling
 			 & XDF_WHITESPACE_FLAGS;
 
+	if (diffopt->color_moved_ws_handling & COLOR_MOVED_DELTA_WHITESPACES)
+		/*
+		 * As there is not specific white space config given,
+		 * we'd need to check for a new block, so ignore all
+		 * white space. The setup of the white space
+		 * configuration for the next block is done else where
+		 */
+		flags |= XDF_IGNORE_WHITESPACE;
+
 	return !xdiff_compare_lines(a->es->line, a->es->len,
 				    b->es->line, b->es->len,
 				    flags);
@@ -772,7 +806,8 @@ static void add_lines_to_move_detection(struct diff_options *o,
 }
 
 static int shrink_potential_moved_blocks(struct moved_entry **pmb,
-					 int pmb_nr)
+					 int pmb_nr,
+					 struct ws_delta **wsd)
 {
 	int lp, rp;
 
@@ -788,6 +823,10 @@ static int shrink_potential_moved_blocks(struct moved_entry **pmb,
 
 		if (lp < pmb_nr && rp > -1 && lp < rp) {
 			pmb[lp] = pmb[rp];
+			if (*wsd) {
+				free((*wsd)[lp].string);
+				(*wsd)[lp] = (*wsd)[rp];
+			}
 			pmb[rp] = NULL;
 			rp--;
 			lp++;
@@ -837,8 +876,11 @@ static void mark_color_as_moved(struct diff_options *o,
 {
 	struct moved_entry **pmb = NULL; /* potentially moved blocks */
 	int pmb_nr = 0, pmb_alloc = 0;
-	int n, flipped_block = 1, block_length = 0;
 
+	struct ws_delta *wsd = NULL; /* white space deltas between pmb */
+	int wsd_alloc = 0;
+
+	int n, flipped_block = 1, block_length = 0;
 
 	for (n = 0; n < o->emitted_symbols->nr; n++) {
 		struct hashmap *hm = NULL;
@@ -881,14 +923,31 @@ static void mark_color_as_moved(struct diff_options *o,
 			struct moved_entry *p = pmb[i];
 			struct moved_entry *pnext = (p && p->next_line) ?
 					p->next_line : NULL;
-			if (pnext && !hm->cmpfn(o, pnext, match, NULL)) {
-				pmb[i] = p->next_line;
+
+			if (o->color_moved_ws_handling & COLOR_MOVED_DELTA_WHITESPACES) {
+				struct ws_delta out = WS_DELTA_INIT;
+
+				if (pnext)
+					compute_ws_delta(l, pnext->es, &out);
+				if (pnext &&
+				    !hm->cmpfn(o, pnext, match, NULL) &&
+				    compare_ws_delta(&out, &wsd[i])) {
+					pmb[i] = p->next_line;
+					/* wsd[i] is the same */
+				} else {
+					pmb[i] = NULL;
+				}
+				free(out.string);
 			} else {
-				pmb[i] = NULL;
+				if (pnext && !hm->cmpfn(o, pnext, match, NULL)) {
+					pmb[i] = p->next_line;
+				} else {
+					pmb[i] = NULL;
+				}
 			}
 		}
 
-		pmb_nr = shrink_potential_moved_blocks(pmb, pmb_nr);
+		pmb_nr = shrink_potential_moved_blocks(pmb, pmb_nr, &wsd);
 
 		if (pmb_nr == 0) {
 			/*
@@ -897,6 +956,10 @@ static void mark_color_as_moved(struct diff_options *o,
 			 */
 			for (; match; match = hashmap_get_next(hm, match)) {
 				ALLOC_GROW(pmb, pmb_nr + 1, pmb_alloc);
+				if (o->color_moved_ws_handling & COLOR_MOVED_DELTA_WHITESPACES) {
+					ALLOC_GROW(wsd, pmb_nr + 1, wsd_alloc);
+					compute_ws_delta(l, match->es, &wsd[pmb_nr]);
+				}
 				pmb[pmb_nr++] = match;
 			}
 
@@ -914,6 +977,7 @@ static void mark_color_as_moved(struct diff_options *o,
 	adjust_last_block(o, n, block_length);
 
 	free(pmb);
+	free(wsd);
 }
 
 #define DIFF_SYMBOL_MOVED_LINE_ZEBRA_MASK \
@@ -4647,12 +4711,16 @@ int diff_opt_parse(struct diff_options *options,
 		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-no-ignore-space-prefix-delta"))
+		options->color_moved_ws_handling &= ~COLOR_MOVED_DELTA_WHITESPACES;
 	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, "--color-moved-ignore-space-prefix-delta"))
+		options->color_moved_ws_handling |= COLOR_MOVED_DELTA_WHITESPACES;
 	else if (!strcmp(arg, "--indent-heuristic"))
 		DIFF_XDL_SET(options, INDENT_HEURISTIC);
 	else if (!strcmp(arg, "--no-indent-heuristic"))
@@ -5558,6 +5626,9 @@ static void diff_flush_patch_all_file_pairs(struct diff_options *o)
 			hashmap_init(&del_lines, moved_entry_cmp, o, 0);
 			hashmap_init(&add_lines, moved_entry_cmp, o, 0);
 
+			if (o->color_moved_ws_handling & COLOR_MOVED_DELTA_WHITESPACES)
+				o->color_moved_ws_handling |= XDF_IGNORE_WHITESPACE;
+
 			add_lines_to_move_detection(o, &add_lines, &del_lines);
 			mark_color_as_moved(o, &add_lines, &del_lines);
 			if (o->color_moved == COLOR_MOVED_ZEBRA_DIM)
diff --git a/diff.h b/diff.h
index de5dc680051..b00ea76c083 100644
--- a/diff.h
+++ b/diff.h
@@ -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;
 };
 
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 751fc478dde..37ff528822f 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -1847,4 +1847,58 @@ test_expect_success 'only move detection ignores white spaces' '
 	test_cmp expected actual
 '
 
+test_expect_success 'compare whitespace delta across moved blocks' '
+
+	git reset --hard &&
+	q_to_tab <<-\EOF >text.txt &&
+	QIndented
+	QText across
+	Qthree lines
+	QBut! <- this stands out
+	Qthis one
+	QQline did
+	Qnot adjust
+	EOF
+
+	git add text.txt &&
+	git commit -m "add text.txt" &&
+
+	q_to_tab <<-\EOF >text.txt &&
+	QQIndented
+	QQText across
+	QQthree lines
+	QQQBut! <- this stands out
+	this one
+	Qline did
+	not adjust
+	EOF
+
+	git diff --color --color-moved --color-moved-ignore-space-prefix-delta |
+		grep -v "index" |
+		test_decode_color >actual &&
+
+	q_to_tab <<-\EOF >expected &&
+		<BOLD>diff --git a/text.txt b/text.txt<RESET>
+		<BOLD>--- a/text.txt<RESET>
+		<BOLD>+++ b/text.txt<RESET>
+		<CYAN>@@ -1,7 +1,7 @@<RESET>
+		<BOLD;MAGENTA>-QIndented<RESET>
+		<BOLD;MAGENTA>-QText across<RESET>
+		<BOLD;MAGENTA>-Qthree lines<RESET>
+		<RED>-QBut! <- this stands out<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>
+		<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
+'
+
 test_done
-- 
2.17.0.582.gccdcbd54c44.dirty

[PATCH 8/8] diff: color-moved white space handling options imply color-moved

[Stefan Beller]

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.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 diff.c                     | 18 +++++++++++++-----
 t/t4015-diff-whitespace.sh |  9 +++++++--
 2 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/diff.c b/diff.c
index 1227a4d2a83..a10184e576e 100644
--- a/diff.c
+++ b/diff.c
@@ -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);
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 37ff528822f..e1ab08d7bb4 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -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

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

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

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

Please do not replace what already hit 'next'.

> https://public-inbox.org/git/20180510195849.28023-4-sbeller@google.com/
> I'll resend shortly.
>
>>
>> * 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'.
>
> 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.
>
>> * 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'.
>
> 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

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

[Stefan Beller]

On Thu, May 17, 2018 at 3:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>>> * 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'.
>>
>> 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
>
> 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.

Re: [PATCH 0/8] Reroll of sb/diff-color-move-more

[Jonathan Tan]

On Thu, 17 May 2018 12:46:45 -0700
Stefan Beller <sbeller@google.com> wrote:

> 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

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/20180424153513.dc2404cd111c44ac78bd8ed8@google.com/
[2] https://public-inbox.org/git/20180424171123.7092788b94498908c25eccf0@google.com/

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

[Junio C Hamano]

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

> Stefan Beller <sbeller@google.com> writes:
>
>>> * 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'.
>>
>> 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
>
> Please do not replace what already hit 'next'.

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

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

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 object.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/object.c b/object.c
index cdf520084d..97245fdea2 100644
--- a/object.c
+++ b/object.c
@@ -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

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

[Stefan Beller]

>> Please do not replace what already hit 'next'.
>
> Here is what I made these two into a relative fix on top.

Thanks, I was about to prepare the same.

Re: [PATCH 6/8] diff.c: decouple white space treatment from move detection algorithm

[Simon Ruderich]

On Thu, May 17, 2018 at 12:46:51PM -0700, Stefan Beller wrote:
> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
> index bb9f1b7cd82..7b2527b9a19 100644
> --- a/Documentation/diff-options.txt
> +++ b/Documentation/diff-options.txt
> @@ -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

Hello,

I think it would be better to specify the options unabbreviated.
Not being able to search the man page for
"--color-moved-ignore-space-at-eol" or
"--color-moved-no-ignore-space-at-eol" can be a major pain when
looking for documentation. So maybe something like this instead:

> +--color-moved-ignore-space-at-eol::
> +--color-moved-no-ignore-space-at-eol::
> +	Ignore changes in whitespace at EOL when performing the move
> +	detection for --color-moved.

Regards
Simon
-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9

Re: [PATCH 6/8] diff.c: decouple white space treatment from move detection algorithm

[Stefan Beller]

On Thu, May 17, 2018 at 9:00 PM, Simon Ruderich <simon@ruderich.org> wrote:
> On Thu, May 17, 2018 at 12:46:51PM -0700, Stefan Beller wrote:
>> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
>> index bb9f1b7cd82..7b2527b9a19 100644
>> --- a/Documentation/diff-options.txt
>> +++ b/Documentation/diff-options.txt
>> @@ -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
>
> Hello,
>
> I think it would be better to specify the options unabbreviated.
> Not being able to search the man page for
> "--color-moved-ignore-space-at-eol" or
> "--color-moved-no-ignore-space-at-eol" can be a major pain when
> looking for documentation. So maybe something like this instead:
>
>> +--color-moved-ignore-space-at-eol::
>> +--color-moved-no-ignore-space-at-eol::
>> +     Ignore changes in whitespace at EOL when performing the move
>> +     detection for --color-moved.

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

[PATCH v2 0/1] rebased: inform about auto submodule ff

[Leif Middelschulte]

From: Leif Middelschulte <Leif.Middelschulte@gmail.com>

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/xmqqk1s474vx.fsf@gitster-ct.c.googlers.com/
[1] https://public-inbox.org/git/20180517184008.25445-1-sbeller@google.com/

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)

[PATCH v3 0/1] rebased: inform about auto submodule ff

[Leif Middelschulte]

From: Leif Middelschulte <Leif.Middelschulte@gmail.com>

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/xmqqk1s474vx.fsf@gitster-ct.c.googlers.com/
[1] https://public-inbox.org/git/20180517184008.25445-1-sbeller@google.com/

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)

[PATCH 1/1] Inform about fast-forwarding of submodules during merge

[Leif Middelschulte]

From: Leif Middelschulte <Leif.Middelschulte@gmail.com>

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/

Signed-off-by: Leif Middelschulte <Leif.Middelschulte@gmail.com>
---
 merge-recursive.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/merge-recursive.c b/merge-recursive.c
index a4b91d17f..e2c99924d 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -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)

Re: [PATCH 1/1] Inform about fast-forwarding of submodules during merge

[Elijah Newren]

Hi Leif,

On Fri, May 18, 2018 at 12:48 PM, Leif Middelschulte
<leif.middelschulte@gmail.com> wrote:
> From: Leif Middelschulte <Leif.Middelschulte@gmail.com>
>
> 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.

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/xmqqo9hg7554.fsf@gitster-ct.c.googlers.com/
[2] https://public-inbox.org/git/CABPp-BGaibCPWuCnaX5Af=sv-2zvyhNcupT+-PkxHDfJBg_Vbw@mail.gmail.com/


> +               } else if (show(o, 2))
> +                       output(o, 2, _("Fast-forwarding submodule %s to %s"), path, oid_to_hex(b));
...
> +               } else if (show(o, 2))
> +                       output(o, 2, _("Fast-forwarding submodule %s to %s"), path, oid_to_hex(a));

Also, by analogy to the "Auto-merging $FILE" comparison, the "to %s"
on these two lines feels out of place.  Users can just look at the
submodule to see what it was updated to.  In a sea of output from
merging, this extra detail feels like noise for the standard use-case,
unless I'm misunderstanding how submodules are special.  Junio also
commented on this in the same email referenced above (at [1]).  Is
there a reason this is an important piece of the message for you to be
shown at the standard merge verbosity?

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

[brian m. carlson]

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

On Thu, May 17, 2018 at 03:01:40PM +0900, Junio C Hamano wrote:
> * 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'.

I think there was one minor change Stefan wanted out of this series.
I'll send a reroll (and tbdiff) with just that change.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 867 bytes --]

Re: [PATCH 1/1] Inform about fast-forwarding of submodules during merge

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

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

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

> ...
> Also, by analogy to the "Auto-merging $FILE" comparison, the "to %s"
> on these two lines feels out of place.  Users can just look at the
> submodule to see what it was updated to.  In a sea of output from
> merging, this extra detail feels like noise for the standard use-case,
> unless I'm misunderstanding how submodules are special.

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.

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

[Stefan Beller]

>>  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'.
>
> I think there was one minor change Stefan wanted out of this series.
> I'll send a reroll (and tbdiff) with just that change.

I was just unsure how we treat [e]grep in our test suite, so I asked
questions out of sheer curiosity. I have no objections to the change
made, though.

Thanks!

jk/branch-l-0-deprecation (was Re: What's cooking in git.git (May 2018, #02; Thu, 17))

[Kaartic Sivaraam]

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

On Thursday 17 May 2018 07:06 PM, Jeff King wrote:
> 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.
> 

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


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

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.


diff --git a/builtin/branch.c b/builtin/branch.c
index 452742fec..f3c5181bb 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 870 bytes --]

Re: jk/branch-l-0-deprecation (was Re: What's cooking in git.git (May 2018, #02; Thu, 17))

[Jeff King]

On Thu, May 24, 2018 at 08:40:18PM +0530, Kaartic Sivaraam wrote:

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

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

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

[Jeff King]

On Thu, May 24, 2018 at 03:22:14PM -0400, Jeff King wrote:

> On Thu, May 24, 2018 at 08:40:18PM +0530, Kaartic Sivaraam wrote:
> 
> > > 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. 
> > 
> > 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.
> 
> 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.

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.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/branch.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index e50a5a1680..55bfacd843 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -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

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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

> @@ -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");
> +	}

So from that point of view, we may need a separate message to warn
users who _do_ want listing with '-l' before jk/branch-l-1-removal
removes it?  

The jk/branch-l-2-resurrection topic later repurposes '-l' for
'--list' but until that happens 'git branch -l' will error not, no?

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

[Jeff King]

On Fri, May 25, 2018 at 10:55:45AM +0900, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > 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.
> 
> "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.

> > @@ -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");
> > +	}
> 
> So from that point of view, we may need a separate message to warn
> users who _do_ want listing with '-l' before jk/branch-l-1-removal
> removes it?  
> 
> The jk/branch-l-2-resurrection topic later repurposes '-l' for
> '--list' but until that happens 'git branch -l' will error not, no?

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.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/branch.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 55bfacd843..b0b33dab94 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -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

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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

> 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.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/branch.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 55bfacd843..b0b33dab94 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -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) {

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

[Junio C Hamano]

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

>> -		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");
>> +		}

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.

sb/submodule-move-nested breaks t7411 under GIT_FSMONITOR_TEST

[Ævar Arnfjörð Bjarmason]

On Thu, May 17 2018, Junio C Hamano wrote:

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

I didn't spot this earlier because I don't test this a lot, but I've
bisected the following breakage down to da62f786d2 ("submodule: fixup
nested submodules after moving the submodule", 2018-03-28) (and manually
confirmed by reverting). On Linux both Debian & CentOS I get tests 3 and
4 failing with:

     GIT_FSMONITOR_TEST=$PWD/t7519/fsmonitor-all ./t7411-submodule-config.sh

-v -x output follows:

expecting success: 
	mkdir submodule &&
	(cd submodule &&
		git init &&
		echo a >a &&
		git add . &&
		git commit -ma
	) &&
	mkdir super &&
	(cd super &&
		git init &&
		git submodule add ../submodule &&
		git submodule add ../submodule a &&
		git commit -m "add as submodule and as a" &&
		git mv a b &&
		git commit -m "move a to b"
	)

+ mkdir submodule
+ cd submodule
+ git init
Initialized empty Git repository in /home/avar/g/git/t/trash directory.t7411-submodule-config/submodule/.git/
+ echo a
+ git add .
+ git commit -ma
[master (root-commit) 27e9f0e] a
 Author: A U Thor <author@example.com>
 1 file changed, 1 insertion(+)
 create mode 100644 a
+ mkdir super
+ cd super
+ git init
Initialized empty Git repository in /home/avar/g/git/t/trash directory.t7411-submodule-config/super/.git/
+ git submodule add ../submodule
Cloning into '/home/avar/g/git/t/trash directory.t7411-submodule-config/super/submodule'...
done.
+ git submodule add ../submodule a
Cloning into '/home/avar/g/git/t/trash directory.t7411-submodule-config/super/a'...
done.
+ git commit -m add as submodule and as a
[master (root-commit) 5a1dac1] add as submodule and as a
 Author: A U Thor <author@example.com>
 3 files changed, 8 insertions(+)
 create mode 100644 .gitmodules
 create mode 160000 a
 create mode 160000 submodule
+ git mv a b
+ git commit -m move a to b
[master ab1e9c7] move a to b
 Author: A U Thor <author@example.com>
 1 file changed, 0 insertions(+), 0 deletions(-)
 rename a => b (100%)
ok 1 - submodule config cache setup

expecting success: 
	test_when_finished "rm -rf repo" &&
	test_create_repo repo &&
	cat >repo/.gitmodules <<-\EOF &&
	[submodule "s"]
		path
		ignore
	EOF
	(
		cd repo &&
		test_must_fail test-submodule-config "" s 2>actual &&
		test_i18ngrep "bad config" actual
	)

+ test_when_finished rm -rf repo
+ test 0 = 0
+ test_cleanup={ rm -rf repo
		} && (exit "$eval_ret"); eval_ret=$?; :
+ test_create_repo repo
+ test 1 = 1
+ repo=repo
+ mkdir -p repo
+ cd repo
+ /home/avar/g/git/t/../git-init --template=/home/avar/g/git/t/../templates/blt/
Initialized empty Git repository in /home/avar/g/git/t/trash directory.t7411-submodule-config/repo/.git/
+ mv .git/hooks .git/hooks-disabled
+ cat
+ cd repo
+ test_must_fail test-submodule-config  s
+ test_i18ngrep bad config actual
+ test -n 
+ test x! = xbad config
+ grep bad config actual
fatal: bad config line 2 in file /home/avar/g/git/t/trash directory.t7411-submodule-config/repo/.gitmodules
+ rm -rf repo
+ exit 0
+ eval_ret=0
+ :
ok 2 - configuration parsing with error

expecting success: 
	(cd super &&
		test-submodule-config \
			HEAD^ a \
			HEAD b \
			HEAD^ submodule \
			HEAD submodule \
				>actual &&
		test_cmp expect actual
	)

+ cd super
+ test-submodule-config HEAD^ a HEAD b HEAD^ submodule HEAD submodule
warning: ab1e9c76c1e6a15df51b20e75552ec5ad00708ac:.gitmodules, multiple configurations found for 'submodule.submodule.path'. Skipping second one!
warning: ab1e9c76c1e6a15df51b20e75552ec5ad00708ac:.gitmodules, multiple configurations found for 'submodule.submodule.url'. Skipping second one!
warning: ab1e9c76c1e6a15df51b20e75552ec5ad00708ac:.gitmodules, multiple configurations found for 'submodule.a.path'. Skipping second one!
warning: ab1e9c76c1e6a15df51b20e75552ec5ad00708ac:.gitmodules, multiple configurations found for 'submodule.a.url'. Skipping second one!
Submodule not found.
Usage: /home/avar/g/git/t/helper/test-submodule-config [<commit> <submodulepath>] ...
error: last command exited with $?=1
not ok 3 - test parsing and lookup of submodule config by path
#	
#		(cd super &&
#			test-submodule-config \
#				HEAD^ a \
#				HEAD b \
#				HEAD^ submodule \
#				HEAD submodule \
#					>actual &&
#			test_cmp expect actual
#		)
#	

expecting success: 
	(cd super &&
		test-submodule-config --name \
			HEAD^ a \
			HEAD a \
			HEAD^ submodule \
			HEAD submodule \
				>actual &&
		test_cmp expect actual
	)

+ cd super
+ test-submodule-config --name HEAD^ a HEAD a HEAD^ submodule HEAD submodule
+ test_cmp expect actual
+ diff -u expect actual
--- expect	2018-05-25 12:28:30.000000000 +0000
+++ actual	2018-05-25 12:28:30.000000000 +0000
@@ -1,4 +1,4 @@
 Submodule name: 'a' for path 'a'
-Submodule name: 'a' for path 'b'
+Submodule name: 'a' for path 'a'
 Submodule name: 'submodule' for path 'submodule'
 Submodule name: 'submodule' for path 'submodule'
error: last command exited with $?=1
not ok 4 - test parsing and lookup of submodule config by name
#	
#		(cd super &&
#			test-submodule-config --name \
#				HEAD^ a \
#				HEAD a \
#				HEAD^ submodule \
#				HEAD submodule \
#					>actual &&
#			test_cmp expect actual
#		)
#	

expecting success: 
	(cd super &&
		cp .gitmodules .gitmodules.bak &&
		echo "	value = \"" >>.gitmodules &&
		git add .gitmodules &&
		mv .gitmodules.bak .gitmodules &&
		git commit -m "add error" &&
		test-submodule-config \
			HEAD b \
			HEAD submodule \
				>actual &&
		test_cmp expect_error actual
	)

+ cd super
+ cp .gitmodules .gitmodules.bak
+ echo 	value = "
+ git add .gitmodules
+ mv .gitmodules.bak .gitmodules
+ git commit -m add error
[master 7cb6f0e] add error
 Author: A U Thor <author@example.com>
 1 file changed, 2 insertions(+), 1 deletion(-)
+ test-submodule-config HEAD b HEAD submodule
error: bad config line 7 in submodule-blob 7cb6f0e38e67568f41da9828cc7d5eb1c753b7b4:.gitmodules
+ test_cmp expect_error actual
+ diff -u expect_error actual
ok 5 - error in one submodule config lets continue

expecting success: 
	(cd super &&
		sha1=$(git rev-parse HEAD) &&
		test-submodule-config \
			HEAD b \
			HEAD submodule \
				2>actual_err &&
		test_i18ngrep "submodule-blob $sha1:.gitmodules" actual_err >/dev/null
	)

+ cd super
+ git rev-parse HEAD
+ sha1=7cb6f0e38e67568f41da9828cc7d5eb1c753b7b4
+ test-submodule-config HEAD b HEAD submodule
Submodule name: 'a' for path 'b'
Submodule name: 'submodule' for path 'submodule'
+ test_i18ngrep submodule-blob 7cb6f0e38e67568f41da9828cc7d5eb1c753b7b4:.gitmodules actual_err
+ test -n 
+ test x! = xsubmodule-blob 7cb6f0e38e67568f41da9828cc7d5eb1c753b7b4:.gitmodules
+ grep submodule-blob 7cb6f0e38e67568f41da9828cc7d5eb1c753b7b4:.gitmodules actual_err
ok 6 - error message contains blob reference

expecting success: 
	(
		cd super &&
		git tag new_tag &&
		tree=$(git rev-parse HEAD^{tree}) &&
		commit=$(git rev-parse HEAD^{commit}) &&
		test-submodule-config $commit b >expect &&
		test-submodule-config $tree b >actual.1 &&
		test-submodule-config new_tag b >actual.2 &&
		test_cmp expect actual.1 &&
		test_cmp expect actual.2
	)

+ cd super
+ git tag new_tag
+ git rev-parse HEAD^{tree}
+ tree=934687743cd5ca685cfa8d3c77aaebb75ee12e1f
+ git rev-parse HEAD^{commit}
+ commit=7cb6f0e38e67568f41da9828cc7d5eb1c753b7b4
+ test-submodule-config 7cb6f0e38e67568f41da9828cc7d5eb1c753b7b4 b
error: bad config line 7 in submodule-blob 7cb6f0e38e67568f41da9828cc7d5eb1c753b7b4:.gitmodules
+ test-submodule-config 934687743cd5ca685cfa8d3c77aaebb75ee12e1f b
error: bad config line 7 in submodule-blob 934687743cd5ca685cfa8d3c77aaebb75ee12e1f:.gitmodules
+ test-submodule-config new_tag b
error: bad config line 7 in submodule-blob 7cb6f0e38e67568f41da9828cc7d5eb1c753b7b4:.gitmodules
+ test_cmp expect actual.1
+ diff -u expect actual.1
+ test_cmp expect actual.2
+ diff -u expect actual.2
ok 7 - using different treeishs works

expecting success: 
	(cd super &&
		git config -f .gitmodules \
			submodule.submodule.fetchrecursesubmodules blabla &&
		git add .gitmodules &&
		git config --unset -f .gitmodules \
			submodule.submodule.fetchrecursesubmodules &&
		git commit -m "add error in fetchrecursesubmodules" &&
		test-submodule-config \
			HEAD b \
			HEAD submodule \
				>actual &&
		test_cmp expect_error actual  &&
		git reset --hard HEAD^
	)

+ cd super
+ git config -f .gitmodules submodule.submodule.fetchrecursesubmodules blabla
+ git add .gitmodules
+ git config --unset -f .gitmodules submodule.submodule.fetchrecursesubmodules
+ git commit -m add error in fetchrecursesubmodules
[master 1fa1547] add error in fetchrecursesubmodules
 Author: A U Thor <author@example.com>
 1 file changed, 1 insertion(+), 1 deletion(-)
+ test-submodule-config HEAD b HEAD submodule
+ test_cmp expect_error actual
+ diff -u expect_error actual
+ git reset --hard HEAD^
HEAD is now at 7cb6f0e add error
ok 8 - error in history in fetchrecursesubmodule lets continue

# failed 2 among 8 test(s)
1..8

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

[Jeff King]

On Fri, May 25, 2018 at 06:14:16PM +0900, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> >> -		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");
> >> +		}
> 
> 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

Re: sb/submodule-move-nested breaks t7411 under GIT_FSMONITOR_TEST

[Stefan Beller]

Hi Ævar,

On Fri, May 25, 2018 at 5:28 AM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
> On Thu, May 17 2018, Junio C Hamano wrote:
>
>> * 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.
>
> I didn't spot this earlier because I don't test this a lot, but I've
> bisected the following breakage down to da62f786d2 ("submodule: fixup
> nested submodules after moving the submodule", 2018-03-28) (and manually
> confirmed by reverting). On Linux both Debian & CentOS I get tests 3 and
> 4 failing with:
>
>      GIT_FSMONITOR_TEST=$PWD/t7519/fsmonitor-all ./t7411-submodule-config.sh

I can reproduce this. I'll look into it.

Re: sb/submodule-move-nested breaks t7411 under GIT_FSMONITOR_TEST

[Stefan Beller]

On Fri, May 25, 2018 at 5:28 AM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
> On Thu, May 17 2018, Junio C Hamano wrote:
>
>> * 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.
>
> I didn't spot this earlier because I don't test this a lot, but I've
> bisected the following breakage down to da62f786d2 ("submodule: fixup
> nested submodules after moving the submodule", 2018-03-28) (and manually
> confirmed by reverting). On Linux both Debian & CentOS I get tests 3 and
> 4 failing with:
>
>      GIT_FSMONITOR_TEST=$PWD/t7519/fsmonitor-all ./t7411-submodule-config.sh
>
> -v -x output follows:
>
> expecting success:
>         mkdir submodule &&
>         (cd submodule &&
>                 git init &&
>                 echo a >a &&
>                 git add . &&
>                 git commit -ma
>         ) &&
>         mkdir super &&
>         (cd super &&
>                 git init &&
>                 git submodule add ../submodule &&
>                 git submodule add ../submodule a &&
>                 git commit -m "add as submodule and as a" &&
>                 git mv a b &&
>                 git commit -m "move a to b"
>         )

when you add a test_pause here and dump the
state of the setup, then it can be observed that when the fsmonitor is active
the last commit is different; without fsmonitor the moved gitlink and the change
to the .gitmodules file is part of the commit, i.e.

$ git -C super show
        commit d3d90b70a01bd17d026f75a803c8b65f5903a7c0 (HEAD -> master)
        Author: A U Thor <author@example.com>
        Date:   Fri May 25 19:21:58 2018 +0000

            move a to b

        diff --git a/.gitmodules b/.gitmodules
        index 3f4d474..6149210 100644
        --- a/.gitmodules
        +++ b/.gitmodules
        @@ -2,5 +2,5 @@
          path = submodule
          url = ../submodule
         [submodule "a"]
        - path = a
        + path = b
          url = ../submodule
        diff --git a/a b/b
        similarity index 100%
        rename from a
        rename to b
When running with the fsmonitor:

$ git -C super show
        commit 57022a92acf46f303498c045440ec099cbc35a2d (HEAD -> master)
        Author: A U Thor <author@example.com>
        Date:   Fri May 25 19:22:52 2018 +0000

            move a to b

        diff --git a/a b/b
        similarity index 100%
        rename from a
        rename to b
$ git -C super diff
        diff --git a/.gitmodules b/.gitmodules
        index 3f4d474..6149210 100644
        --- a/.gitmodules
        +++ b/.gitmodules
        @@ -2,5 +2,5 @@
          path = submodule
          url = ../submodule
         [submodule "a"]
        - path = a
        + path = b
          url = ../submodule

This hints at a problem with git commit;

I tried adding test_tick, to unconfuse the fsmonitor, but that doesn't help,
digging further, the problem is in the git mv command, which fails to
add the change in
.gitmodules to the index.

Adding the verbose flag to stage_updated_gitmodules() that is called by
git-mv very late in the game, such that

void stage_updated_gitmodules(struct index_state *istate)
{
    trace_printf("staging .gitmodules files");
    if (add_file_to_index(istate, GITMODULES_FILE, ADD_CACHE_VERBOSE))
        die(_("staging updated .gitmodules failed"));
}

We would get a message if the .gitmodules file is staged correctly, as
add_file_to_index() that calls add_to_index that would print

    if (verbose && !was_same)
        printf("add '%s'\n", path);

I could not see that message, so I suspect, that there is something
racy.

Will debug further.

[RFC PATCH 0/3] usage: prefix all lines in `vreportf()`, not just the first

[Martin Ågren]

On 25 May 2018 at 11:14, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>>> +                    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).");
>
> 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.

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

[RFC PATCH 1/3] usage: extract `prefix_suffix_lines()` from `advise()`

[Martin Ågren]

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.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
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(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index f9e4c5f9bc..23445f7ab9 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -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)));
diff --git a/advice.c b/advice.c
index 370a56d054..ffb29e7ef4 100644
--- a/advice.c
+++ b/advice.c
@@ -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)
diff --git a/usage.c b/usage.c
index cdd534c9df..80f9c1d14b 100644
--- a/usage.c
+++ b/usage.c
@@ -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

[RFC PATCH 2/3] usage: prefix all lines in `vreportf()`, not just the first

[Martin Ågren]

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.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
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(-)

diff --git a/t/t1011-read-tree-sparse-checkout.sh b/t/t1011-read-tree-sparse-checkout.sh
index 0c6f48f302..31b0702e6c 100755
--- a/t/t1011-read-tree-sparse-checkout.sh
+++ b/t/t1011-read-tree-sparse-checkout.sh
@@ -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
diff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh
index 4ee009da66..80d35087b7 100755
--- a/t/t1506-rev-parse-diagnosis.sh
+++ b/t/t1506-rev-parse-diagnosis.sh
@@ -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
 }
diff --git a/t/t1600-index.sh b/t/t1600-index.sh
index c4422312f4..39a707c94a 100755
--- a/t/t1600-index.sh
+++ b/t/t1600-index.sh
@@ -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
 	)
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index b8fbdefcdc..cd4a10df2d 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -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 &&
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 6a949484d0..7e505163f6 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -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
diff --git a/t/t7607-merge-overwrite.sh b/t/t7607-merge-overwrite.sh
index dd8ab7ede1..985fbab0b0 100755
--- a/t/t7607-merge-overwrite.sh
+++ b/t/t7607-merge-overwrite.sh
@@ -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
 
diff --git a/t/t7609-merge-co-error-msgs.sh b/t/t7609-merge-co-error-msgs.sh
index e90413204e..54bc3d2c9c 100755
--- a/t/t7609-merge-co-error-msgs.sh
+++ b/t/t7609-merge-co-error-msgs.sh
@@ -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
 
diff --git a/usage.c b/usage.c
index 80f9c1d14b..6a5669922f 100644
--- a/usage.c
+++ b/usage.c
@@ -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

[RFC PATCH 3/3] usage: translate the "error: "-prefix and others

[Martin Ågren]

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.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
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(-)

diff --git a/usage.c b/usage.c
index 6a5669922f..7709cb22e7 100644
--- a/usage.c
+++ b/usage.c
@@ -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

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

[Junio C Hamano]

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

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

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 <peff@peff.net>
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.

Signed-off-by: Jeff King <peff@peff.net>
Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>
Reviewed-by: Jacob Keller <jacob.keller@gmail.com>
[jc: added a few tests]
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/branch.c  | 14 --------------
 t/t3200-branch.sh | 12 ++++++++++++
 2 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index b0b33dab94..01b35b3c3d 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -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),
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index da97b8a62b..eca75d3ca1 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -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

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

[Junio C Hamano]

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

> 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 <peff@peff.net>
> Date: Mon, 26 Mar 2018 03:29:22 -0400
> Subject: [PATCH] branch: drop deprecated "-l" option

And this is the final "reincarnation" step.

-- >8 --
From: Jeff King <peff@peff.net>
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.

Signed-off-by: Jeff King <peff@peff.net>
Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>
Reviewed-by: Jacob Keller <jacob.keller@gmail.com>
[jc: adjusted the new tests added earlier]
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/branch.c  | 2 +-
 t/t3200-branch.sh | 8 +-------
 2 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 01b35b3c3d..1d06f5c547 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -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")),
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index eca75d3ca1..022d6a41c8 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -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

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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.

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

[Kaartic Sivaraam]

[-- Attachment #1.1: Type: text/plain, Size: 3202 bytes --]

On Friday 25 May 2018 01:01 AM, Jeff King wrote:
> On Thu, May 24, 2018 at 03:22:14PM -0400, Jeff King wrote:
> 
> 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.
>

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/1527174618.10589.4.camel@gmail.com/


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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

[Kaartic Sivaraam]

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

On Friday 25 May 2018 08:10 AM, Jeff King wrote:
> 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.
> 

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 ;-)


> +			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).");

I suppose s/alias/deprecated alias/ makes the point that '-l' will be
removed more stronger.


-- 
Sivaraam

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 870 bytes --]

Re: [RFC PATCH 1/3] usage: extract `prefix_suffix_lines()` from `advise()`

[Junio C Hamano]

Martin Ågren <martin.agren@gmail.com> writes:

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

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?

> +
> +/*
> + * 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);
> +
> ...
> diff --git a/usage.c b/usage.c
> index cdd534c9df..80f9c1d14b 100644
> --- a/usage.c
> +++ b/usage.c
> @@ -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];

I guess we can directly use this even in the codepath that
implements die() without having to worry about the helper making any
extra allocation, which is good.

Re: [RFC PATCH 2/3] usage: prefix all lines in `vreportf()`, not just the first

[Junio C Hamano]

Martin Ågren <martin.agren@gmail.com> writes:

> diff --git a/t/t1011-read-tree-sparse-checkout.sh b/t/t1011-read-tree-sparse-checkout.sh
> index 0c6f48f302..31b0702e6c 100755
> --- a/t/t1011-read-tree-sparse-checkout.sh
> +++ b/t/t1011-read-tree-sparse-checkout.sh
> @@ -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

This shows the typical effect of this series, which (I subjectively
think) gives us a more pleasant end-user experience.

> diff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh
> index 4ee009da66..80d35087b7 100755
> --- a/t/t1506-rev-parse-diagnosis.sh
> +++ b/t/t1506-rev-parse-diagnosis.sh
> @@ -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

And this, too.

> diff --git a/usage.c b/usage.c
> index 80f9c1d14b..6a5669922f 100644
> --- a/usage.c
> +++ b/usage.c
> @@ -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, "");
>  }

Re: [RFC PATCH 1/3] usage: extract `prefix_suffix_lines()` from `advise()`

[Duy Nguyen]

On Fri, May 25, 2018 at 11:00 PM, Martin Ågren <martin.agren@gmail.com> wrote:
> 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.
>
> Signed-off-by: Martin Ågren <martin.agren@gmail.com>
> ---
> I'm open for suggestions on the naming of `prefix_suffix_lines()`...

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

Re: [RFC PATCH 2/3] usage: prefix all lines in `vreportf()`, not just the first

[Duy Nguyen]

On Mon, May 28, 2018 at 11:25 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Martin Ågren <martin.agren@gmail.com> writes:
>
>> diff --git a/t/t1011-read-tree-sparse-checkout.sh b/t/t1011-read-tree-sparse-checkout.sh
>> index 0c6f48f302..31b0702e6c 100755
>> --- a/t/t1011-read-tree-sparse-checkout.sh
>> +++ b/t/t1011-read-tree-sparse-checkout.sh
>> @@ -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
>
> This shows the typical effect of this series, which (I subjectively
> think) gives us a more pleasant end-user experience.

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

Re: [RFC PATCH 2/3] usage: prefix all lines in `vreportf()`, not just the first

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

>>> +error:       sub/added
>>> +error:       sub/addedtoo
>>> +error: Please move or remove them before you switch branches.
>>>  Aborting
>>>  EOF
>>
>> This shows the typical effect of this series, which (I subjectively
>> think) gives us a more pleasant end-user experience.
>
> 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.

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.

Re: [RFC PATCH 2/3] usage: prefix all lines in `vreportf()`, not just the first

[Martin Ågren]

On 28 May 2018 at 23:45, Junio C Hamano <gitster@pobox.com> wrote:
> Duy Nguyen <pclouds@gmail.com> writes:
>
>>>> +error:       sub/added
>>>> +error:       sub/addedtoo
>>>> +error: Please move or remove them before you switch branches.
>>>>  Aborting
>>>>  EOF
>>>
>>> This shows the typical effect of this series, which (I subjectively
>>> think) gives us a more pleasant end-user experience.
>>
>> 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.

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

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.

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

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

Martin

Re: [RFC PATCH 2/3] usage: prefix all lines in `vreportf()`, not just the first

[Junio C Hamano]

Martin Ågren <martin.agren@gmail.com> writes:

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

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.

Re: [RFC PATCH 2/3] usage: prefix all lines in `vreportf()`, not just the first

[Martin Ågren]

On 29 May 2018 at 07:50, Junio C Hamano <gitster@pobox.com> wrote:
> Martin Ågren <martin.agren@gmail.com> writes:
>
>>>  - 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.
>>
>> 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.
>
> 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

Re: [RFC PATCH 2/3] usage: prefix all lines in `vreportf()`, not just the first

[Junio C Hamano]

Martin Ågren <martin.agren@gmail.com> writes:

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

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

Re: [RFC PATCH 2/3] usage: prefix all lines in `vreportf()`, not just the first

[Duy Nguyen]

On Tue, May 29, 2018 at 6:49 AM, Martin Ågren <martin.agren@gmail.com> wrote:
> On 28 May 2018 at 23:45, Junio C Hamano <gitster@pobox.com> wrote:
>> Duy Nguyen <pclouds@gmail.com> writes:
>>
>>>>> +error:       sub/added
>>>>> +error:       sub/addedtoo
>>>>> +error: Please move or remove them before you switch branches.
>>>>>  Aborting
>>>>>  EOF
>>>>
>>>> This shows the typical effect of this series, which (I subjectively
>>>> think) gives us a more pleasant end-user experience.
>>>
>>> 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.
>
> 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.

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

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

[Jeff King]

On Sun, May 27, 2018 at 12:15:40AM +0530, Kaartic Sivaraam wrote:

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

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

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

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

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

[Jeff King]

On Sat, May 26, 2018 at 11:32:35AM +0900, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > 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).
> 
> 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 <peff@peff.net>
> 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.

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

diff --git a/builtin/branch.c b/builtin/branch.c
index 01b35b3c3d..f7cd333587 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -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"));

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

[Jeff King]

On Tue, May 29, 2018 at 05:20:29PM -0400, Jeff King wrote:

> 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).
> [...]
> -	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");
> -		}
> -	}
> -

Oh, and did we want to mark these for translation on the step 0 branch?
Obviously that would impact this hunk.

-Peff

Re: [RFC PATCH 2/3] usage: prefix all lines in `vreportf()`, not just the first

[Jeff King]

On Mon, May 28, 2018 at 06:25:18PM +0900, Junio C Hamano wrote:

> Martin Ågren <martin.agren@gmail.com> writes:
> 
> > diff --git a/t/t1011-read-tree-sparse-checkout.sh b/t/t1011-read-tree-sparse-checkout.sh
> > index 0c6f48f302..31b0702e6c 100755
> > --- a/t/t1011-read-tree-sparse-checkout.sh
> > +++ b/t/t1011-read-tree-sparse-checkout.sh
> > @@ -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
> 
> This shows the typical effect of this series, which (I subjectively
> think) gives us a more pleasant end-user experience.

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/20170111140758.yyfsc3r3spqpi6es@sigill.intra.peff.net/

and down-thread.

-Peff

Re: [RFC PATCH 1/3] usage: extract `prefix_suffix_lines()` from `advise()`

[Jeff King]

On Mon, May 28, 2018 at 08:40:16PM +0200, Duy Nguyen wrote:

> On Fri, May 25, 2018 at 11:00 PM, Martin Ågren <martin.agren@gmail.com> wrote:
> > 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.
> >
> > Signed-off-by: Martin Ågren <martin.agren@gmail.com>
> > ---
> > I'm open for suggestions on the naming of `prefix_suffix_lines()`...
> 
> I think the important verb, print (to FILE*), is somehow missing. This
> current name would be great if it produces another str(buf).

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

-Peff

Re: [RFC PATCH 1/3] usage: extract `prefix_suffix_lines()` from `advise()`

[Jeff King]

On Fri, May 25, 2018 at 11:00:53PM +0200, Martin Ågren wrote:

> +/*
> + * 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);

Should this perhaps learn about colors, too?

That would save us from having to allocate here:

> diff --git a/advice.c b/advice.c
> index 370a56d054..ffb29e7ef4 100644
> --- a/advice.c
> +++ b/advice.c
> @@ -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));

But most importantly, it means we could eventually colorize errors, too,
where we are not allowed to allocate.

So perhaps:

  void report_lines(FILE *out,
                    const char *color, const char *color_reset,
		    const char *prefix, const char *msg);

or something?

-Peff

Re: [RFC PATCH 1/3] usage: extract `prefix_suffix_lines()` from `advise()`

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> But most importantly, it means we could eventually colorize errors, too,
> where we are not allowed to allocate.
>
> So perhaps:
>
>   void report_lines(FILE *out,
>                     const char *color, const char *color_reset,
> 		    const char *prefix, const char *msg);
>
> or something?

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.

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> -		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");
>> -		}
>> -	}
>> -
>
> Oh, and did we want to mark these for translation on the step 0 branch?
> Obviously that would impact this hunk.

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 ;-)

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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. ;)
> ...
>> Anyways, if you think it mustn't turn into an error now and only in the
>> next stage, a suggestion follows in another thread.
>
> 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).

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.

Re: [RFC PATCH 1/3] usage: extract `prefix_suffix_lines()` from `advise()`

[Junio C Hamano]

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

> Jeff King <peff@peff.net> writes:
>
>> But most importantly, it means we could eventually colorize errors, too,
>> where we are not allowed to allocate.
>>
>> So perhaps:
>>
>>   void report_lines(FILE *out,
>>                     const char *color, const char *color_reset,
>> 		    const char *prefix, const char *msg);
>>
>> or something?
>
> 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.

Re: [RFC PATCH 2/3] usage: prefix all lines in `vreportf()`, not just the first

[Martin Ågren]

On 29 May 2018 at 17:50, Duy Nguyen <pclouds@gmail.com> wrote:
> On Tue, May 29, 2018 at 6:49 AM, Martin Ågren <martin.agren@gmail.com> wrote:
>> On 28 May 2018 at 23:45, Junio C Hamano <gitster@pobox.com> wrote:
>>> Duy Nguyen <pclouds@gmail.com> writes:
>>>
>>>>>> +error:       sub/added
>>>>>> +error:       sub/addedtoo
>>>>>> +error: Please move or remove them before you switch branches.
>>>>>>  Aborting
>>>>>>  EOF
>>>>>
>>>>> This shows the typical effect of this series, which (I subjectively
>>>>> think) gives us a more pleasant end-user experience.
>>>>
>>>> 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.
>>
>> 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.
>
> 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.

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

Re: [RFC PATCH 2/3] usage: prefix all lines in `vreportf()`, not just the first

[Martin Ågren]

On 29 May 2018 at 23:32, Jeff King <peff@peff.net> wrote:
> On Mon, May 28, 2018 at 06:25:18PM +0900, Junio C Hamano wrote:
>
>> This shows the typical effect of this series, which (I subjectively
>> think) gives us a more pleasant end-user experience.
>
> 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/20170111140758.yyfsc3r3spqpi6es@sigill.intra.peff.net/
>
> and down-thread.

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

Martin

Re: [RFC PATCH 1/3] usage: extract `prefix_suffix_lines()` from `advise()`

[Martin Ågren]

On 30 May 2018 at 08:00, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Jeff King <peff@peff.net> writes:
>>
>>> But most importantly, it means we could eventually colorize errors, too,
>>> where we are not allowed to allocate.
>>>
>>> So perhaps:
>>>
>>>   void report_lines(FILE *out,
>>>                     const char *color, const char *color_reset,
>>>                  const char *prefix, const char *msg);
>>>
>>> or something?
>>
>> 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.

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

> 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."));
>
[...]
> 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).

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

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

[Jeff King]

On Wed, May 30, 2018 at 11:48:32AM +0900, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> >> -		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");
> >> -		}
> >> -	}
> >> -
> >
> > Oh, and did we want to mark these for translation on the step 0 branch?
> > Obviously that would impact this hunk.
> 
> 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

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

[Jeff King]

On Wed, May 30, 2018 at 11:52:19AM +0900, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > 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. ;)
> > ...
> >> Anyways, if you think it mustn't turn into an error now and only in the
> >> next stage, a suggestion follows in another thread.
> >
> > 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).
> 
> 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

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

[Kaartic.Sivaraam]

On Wednesday 30 May 2018 08:22 AM, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
>> 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. ;)
>> ...
>>> Anyways, if you think it mustn't turn into an error now and only in the
>>> next stage, a suggestion follows in another thread.
>>
>> 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).
> 
> 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.
> 

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

Re: [RFC PATCH 1/3] usage: extract `prefix_suffix_lines()` from `advise()`

[Jeff King]

On Wed, May 30, 2018 at 12:26:40PM +0200, Martin Ågren wrote:

> >>> So perhaps:
> >>>
> >>>   void report_lines(FILE *out,
> >>>                     const char *color, const char *color_reset,
> >>>                  const char *prefix, const char *msg);
> >>>
> >>> or something?
> >>
> >> 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.
> 
> (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.)

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

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

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

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

Thanks for a concise and readably summary ;-)

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

[Duy Nguyen]

On Fri, May 25, 2018 at 4:40 AM, Jeff King <peff@peff.net> wrote:
> -- >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.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/branch.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 55bfacd843..b0b33dab94 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -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");
> +               }

This is already in 'next', but could you do a follow up patch to mark
these strings for translation? While at there, concatenating them into
full sentences would also help translators.

>         }
>
>         if (delete) {
> --
> 2.17.0.1391.g6fdbf40724
>



-- 
Duy

Re: [PATCH] branch: issue "-l" deprecation warning after pager starts

[Jeff King]

On Sat, Jun 02, 2018 at 06:46:31AM +0200, Duy Nguyen wrote:

> >         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");
> > +               }
> 
> This is already in 'next', but could you do a follow up patch to mark
> these strings for translation? While at there, concatenating them into
> full sentences would also help translators.

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

Re: [PATCH 0/8] Reroll of sb/diff-color-move-more

[Jacob Keller]

On Thu, May 17, 2018 at 12:46 PM, Stefan Beller <sbeller@google.com> wrote:
>>> * sb/diff-color-move-more (2018-04-25) 7 commits
>>...
>>>
>>>  Will merge to 'next'.
>>
>>I did not get around to fix it up, there are still review
>>comments outstanding. (The test is broken in the last commit.)
>
> 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
>

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

Re: sb/submodule-move-nested breaks t7411 under GIT_FSMONITOR_TEST

[Ævar Arnfjörð Bjarmason]

On Fri, May 25 2018, Stefan Beller wrote:

> On Fri, May 25, 2018 at 5:28 AM, Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>>
>> On Thu, May 17 2018, Junio C Hamano wrote:
>>
>>> * 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.
>>
>> I didn't spot this earlier because I don't test this a lot, but I've
>> bisected the following breakage down to da62f786d2 ("submodule: fixup
>> nested submodules after moving the submodule", 2018-03-28) (and manually
>> confirmed by reverting). On Linux both Debian & CentOS I get tests 3 and
>> 4 failing with:
>>
>>      GIT_FSMONITOR_TEST=$PWD/t7519/fsmonitor-all ./t7411-submodule-config.sh
>>
>> -v -x output follows:
>>
>> expecting success:
>>         mkdir submodule &&
>>         (cd submodule &&
>>                 git init &&
>>                 echo a >a &&
>>                 git add . &&
>>                 git commit -ma
>>         ) &&
>>         mkdir super &&
>>         (cd super &&
>>                 git init &&
>>                 git submodule add ../submodule &&
>>                 git submodule add ../submodule a &&
>>                 git commit -m "add as submodule and as a" &&
>>                 git mv a b &&
>>                 git commit -m "move a to b"
>>         )
>
> when you add a test_pause here and dump the
> state of the setup, then it can be observed that when the fsmonitor is active
> the last commit is different; without fsmonitor the moved gitlink and the change
> to the .gitmodules file is part of the commit, i.e.
>
> $ git -C super show
>         commit d3d90b70a01bd17d026f75a803c8b65f5903a7c0 (HEAD -> master)
>         Author: A U Thor <author@example.com>
>         Date:   Fri May 25 19:21:58 2018 +0000
>
>             move a to b
>
>         diff --git a/.gitmodules b/.gitmodules
>         index 3f4d474..6149210 100644
>         --- a/.gitmodules
>         +++ b/.gitmodules
>         @@ -2,5 +2,5 @@
>           path = submodule
>           url = ../submodule
>          [submodule "a"]
>         - path = a
>         + path = b
>           url = ../submodule
>         diff --git a/a b/b
>         similarity index 100%
>         rename from a
>         rename to b
> When running with the fsmonitor:
>
> $ git -C super show
>         commit 57022a92acf46f303498c045440ec099cbc35a2d (HEAD -> master)
>         Author: A U Thor <author@example.com>
>         Date:   Fri May 25 19:22:52 2018 +0000
>
>             move a to b
>
>         diff --git a/a b/b
>         similarity index 100%
>         rename from a
>         rename to b
> $ git -C super diff
>         diff --git a/.gitmodules b/.gitmodules
>         index 3f4d474..6149210 100644
>         --- a/.gitmodules
>         +++ b/.gitmodules
>         @@ -2,5 +2,5 @@
>           path = submodule
>           url = ../submodule
>          [submodule "a"]
>         - path = a
>         + path = b
>           url = ../submodule
>
> This hints at a problem with git commit;
>
> I tried adding test_tick, to unconfuse the fsmonitor, but that doesn't help,
> digging further, the problem is in the git mv command, which fails to
> add the change in
> .gitmodules to the index.
>
> Adding the verbose flag to stage_updated_gitmodules() that is called by
> git-mv very late in the game, such that
>
> void stage_updated_gitmodules(struct index_state *istate)
> {
>     trace_printf("staging .gitmodules files");
>     if (add_file_to_index(istate, GITMODULES_FILE, ADD_CACHE_VERBOSE))
>         die(_("staging updated .gitmodules failed"));
> }
>
> We would get a message if the .gitmodules file is staged correctly, as
> add_file_to_index() that calls add_to_index that would print
>
>     if (verbose && !was_same)
>         printf("add '%s'\n", path);
>
> I could not see that message, so I suspect, that there is something
> racy.
>
> Will debug further.

I spotted this again after testing the split index (see
https://public-inbox.org/git/87va7ireuu.fsf@evledraar.gmail.com/) and
was testing the fsmonitor test mode as well.

So gentle *poke*: Did you get anywhere with debugging this? It's still
failing on "master" now.

Re: sb/submodule-move-nested breaks t7411 under GIT_FSMONITOR_TEST

[Stefan Beller]

> > Will debug further.
>
> I spotted this again after testing the split index (see
> https://public-inbox.org/git/87va7ireuu.fsf@evledraar.gmail.com/) and
> was testing the fsmonitor test mode as well.
>
> So gentle *poke*: Did you get anywhere with debugging this? It's still
> failing on "master" now.

I started looking into this again, help would be appreciated, as I do not
quite understand the fsmonitor part.

The error is in the setup, where we have "git mv a b"

GIT_TRACE_FSMONITOR=1 GIT_TRACE=1
GIT_FSMONITOR_TEST=$PWD/t7519/fsmonitor-all
./t7411-submodule-config.sh -d -i -v -x
++ git mv a b
trace: built-in: git mv a b
read fsmonitor extension successful
add fsmonitor
refresh fsmonitor
trace: run_command: cd '/u/git/t/trash
directory.t7411-submodule-config/super'; /u/git/t/t7519/fsmonitor-all
1 1536252819824793728
fsmonitor process '/u/git/t/t7519/fsmonitor-all' returned success
mark_fsmonitor_clean '.gitmodules'
write fsmonitor extension successful

or with more trace_printfs littered through the code
(https://github.com/stefanbeller/git/tree/submodule_fsmoitor_debug)

trace: built-in: git mv a b
read fsmonitor extension successful
add fsmonitor
refresh fsmonitor
trace: run_command: cd '/u/git/t/trash
directory.t7411-submodule-config/super'; /u/git/t/t7519/fsmonitor-all
1 1536252497951329341
fsmonitor process '/u/git/t/t7519/fsmonitor-all' returned success
need to stage .gitmodules
calling add_file_to_index


I suspect that the FSMONITOR API is handled wrongly by the part of git-mv
that writes out the .gitmodules file (if needed) and the .git/index (as needed).

Ben, do you have an idea?

Thanks,
Stefan

RE: sb/submodule-move-nested breaks t7411 under GIT_FSMONITOR_TEST

[Ben Peart]

> -----Original Message-----
> From: Stefan Beller <sbeller@google.com>
> Sent: Thursday, September 6, 2018 12:57 PM
> To: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Cc: Junio C Hamano <gitster@pobox.com>; git <git@vger.kernel.org>; Ben
> Peart <Ben.Peart@microsoft.com>
> Subject: Re: sb/submodule-move-nested breaks t7411 under
> GIT_FSMONITOR_TEST
> 
> > > Will debug further.
> >
> > I spotted this again after testing the split index (see
> >
> https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpublic-
> inbox.org%2Fgit%2F87va7ireuu.fsf%40evledraar.gmail.com%2F&amp;data=0
> 2%7C01%7CBen.Peart%40microsoft.com%7C27c901f198a24e5a045a08d6141
> 9ce42%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636718498401
> 788697&amp;sdata=JfavHtKafiJRAVnvMXU3nw1RI27G4OFxs3Ymt7STbvM%3
> D&amp;reserved=0) and
> > was testing the fsmonitor test mode as well.
> >
> > So gentle *poke*: Did you get anywhere with debugging this? It's still
> > failing on "master" now.
> 
> I started looking into this again, help would be appreciated, as I do not
> quite understand the fsmonitor part.
> 
> The error is in the setup, where we have "git mv a b"
> 
> GIT_TRACE_FSMONITOR=1 GIT_TRACE=1
> GIT_FSMONITOR_TEST=$PWD/t7519/fsmonitor-all
> ./t7411-submodule-config.sh -d -i -v -x
> ++ git mv a b
> trace: built-in: git mv a b
> read fsmonitor extension successful
> add fsmonitor
> refresh fsmonitor
> trace: run_command: cd '/u/git/t/trash
> directory.t7411-submodule-config/super'; /u/git/t/t7519/fsmonitor-all
> 1 1536252819824793728
> fsmonitor process '/u/git/t/t7519/fsmonitor-all' returned success
> mark_fsmonitor_clean '.gitmodules'
> write fsmonitor extension successful
> 
> or with more trace_printfs littered through the code
> (https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub
> .com%2Fstefanbeller%2Fgit%2Ftree%2Fsubmodule_fsmoitor_debug&amp;d
> ata=02%7C01%7CBen.Peart%40microsoft.com%7C27c901f198a24e5a045a08
> d61419ce42%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C6367184
> 98401788697&amp;sdata=poAUFlVDkoNP56t1hmdxHHyChrH05cut1iSVt70yJ
> eo%3D&amp;reserved=0)
> 
> trace: built-in: git mv a b
> read fsmonitor extension successful
> add fsmonitor
> refresh fsmonitor
> trace: run_command: cd '/u/git/t/trash
> directory.t7411-submodule-config/super'; /u/git/t/t7519/fsmonitor-all
> 1 1536252497951329341
> fsmonitor process '/u/git/t/t7519/fsmonitor-all' returned success
> need to stage .gitmodules
> calling add_file_to_index
> 
> 
> I suspect that the FSMONITOR API is handled wrongly by the part of git-mv
> that writes out the .gitmodules file (if needed) and the .git/index (as needed).
> 
> Ben, do you have an idea?
> 

I'll take a look as soon as I can (and at the other fsmonitor test issue Ævar sent email about) but it may be a few days before I get a chance.

I haven't had a chance to look into this yet but here are a couple of SWAG's I'd start with:

I wonder if there is a missing call to mark_fsmonitor_invalid() in the "git mv" codepath somewhere.

refresh_fsmonitor() only runs once per git command - is it possible that "git mv" is triggering it early, making a change, and then updating the index with stale fsmonitor data?

Sorry, I'll look as soon as I can.

Ben

> Thanks,
> Stefan

Re: sb/submodule-move-nested breaks t7411 under GIT_FSMONITOR_TEST

[Stefan Beller]

> > Ben, do you have an idea?
> >
>
> I'll take a look as soon as I can (and at the other fsmonitor test issue Ævar sent email about) but it may be a few days before I get a chance.
>
> I haven't had a chance to look into this yet but here are a couple of SWAG's I'd start with:
>

> is it possible that "git mv" is triggering it early, making a change, and then updating the index with stale fsmonitor data?

This is exactly what is happening. Thanks for describing the situation
precisely.

> I wonder if there is a missing call to mark_fsmonitor_invalid() in the "git mv" codepath somewhere.
>
> refresh_fsmonitor() only runs once per git command

Just naively adding mark_fsmonitor_invalid doesn't work, as then
we have a sequence of

fsmonitor process '/u/git/t/t7519/fsmonitor-all' returned success
mark_fsmonitor_clean '.gitmodules'
mark_fsmonitor_invalid '.gitmodules'
write fsmonitor extension successful

and the marking invalid doesn't seem to override the first
mark as valid ?

>
> Sorry, I'll look as soon as I can.
>
> Ben

Thanks!
Stefan

[PATCH] git-mv: allow submodules and fsmonitor to work together

[Stefan Beller]

It was reported that

  GIT_FSMONITOR_TEST=$PWD/t7519/fsmonitor-all ./t7411-submodule-config.sh

breaks as the .gitmodules file is modified and staged after the fsmonitor
considers it clean. Mark the .gitmodules file to be not clean before
staging.

Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Inspired-by: Ben Peart <benpeart@microsoft.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---

I am not quite sure if this is the correct approach and handling of the
fsmonitor API, but it unbreaks the test.

> Just naively adding mark_fsmonitor_invalid doesn't work, as then ...

Adding it before the staging, works.

Please double check!

Thanks,
Stefan

 submodule.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/submodule.c b/submodule.c
index 50cbf5f13ed..56b0d5fe24e 100644
--- a/submodule.c
+++ b/submodule.c
@@ -22,6 +22,7 @@
 #include "worktree.h"
 #include "parse-options.h"
 #include "object-store.h"
+#include "fsmonitor.h"
 
 static int config_update_recurse_submodules = RECURSE_SUBMODULES_OFF;
 static struct string_list changed_submodule_names = STRING_LIST_INIT_DUP;
@@ -149,6 +150,15 @@ int remove_path_from_gitmodules(const char *path)
 
 void stage_updated_gitmodules(struct index_state *istate)
 {
+	struct cache_entry *ce;
+	int pos;
+
+	pos = index_name_pos(istate, GITMODULES_FILE, strlen(GITMODULES_FILE));
+	ce = (0 <= pos) ? istate->cache[pos] : NULL;
+
+	if (ce)
+		mark_fsmonitor_invalid(istate, ce);
+
 	if (add_file_to_index(istate, GITMODULES_FILE, 0))
 		die(_("staging updated .gitmodules failed"));
 }
-- 
2.19.0.rc2.392.g5ba43deb5a-goog

Re: [PATCH] git-mv: allow submodules and fsmonitor to work together

[Ben Peart]

On 9/6/2018 4:34 PM, Stefan Beller wrote:
> It was reported that
> 
>    GIT_FSMONITOR_TEST=$PWD/t7519/fsmonitor-all ./t7411-submodule-config.sh
> 
> breaks as the .gitmodules file is modified and staged after the fsmonitor
> considers it clean. Mark the .gitmodules file to be not clean before
> staging.
> 
> Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Inspired-by: Ben Peart <benpeart@microsoft.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> 
> I am not quite sure if this is the correct approach and handling of the
> fsmonitor API, but it unbreaks the test.
> 
>> Just naively adding mark_fsmonitor_invalid doesn't work, as then ...
> 
> Adding it before the staging, works.
> 
> Please double check!

I took a look at this bug/patch and wondered why add_file_to_index() 
wasn't properly handling the .gitmodules file.  On investigation, I 
chased it down to what looks like a faulty test in 
is_staging_gitmodules_ok().

I believe the following is a better patch for this bug:

diff --git a/submodule.c b/submodule.c
index 50cbf5f13e..1e7194af28 100644
--- a/submodule.c
+++ b/submodule.c
@@ -65,8 +65,7 @@ int is_staging_gitmodules_ok(struct index_state *istate)
         if ((pos >= 0) && (pos < istate->cache_nr)) {
                 struct stat st;
                 if (lstat(GITMODULES_FILE, &st) == 0 &&
-                   ie_match_stat(istate, istate->cache[pos], &st,
-                                 CE_MATCH_IGNORE_FSMONITOR) & DATA_CHANGED)
+                   ie_match_stat(istate, istate->cache[pos], &st, 0) & 
DATA_CHANGED)
                         return 0;
         }

Please double check but I just don't understand why the .gitmodules file 
should force the fsmonitor data to be ignored.  This flag was added to 
enable proper behavior in the preload_thread() logic and I don't believe 
it is appropriate here.

Ben

> 
> Thanks,
> Stefan
> 
>   submodule.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/submodule.c b/submodule.c
> index 50cbf5f13ed..56b0d5fe24e 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -22,6 +22,7 @@
>   #include "worktree.h"
>   #include "parse-options.h"
>   #include "object-store.h"
> +#include "fsmonitor.h"
>   
>   static int config_update_recurse_submodules = RECURSE_SUBMODULES_OFF;
>   static struct string_list changed_submodule_names = STRING_LIST_INIT_DUP;
> @@ -149,6 +150,15 @@ int remove_path_from_gitmodules(const char *path)
>   
>   void stage_updated_gitmodules(struct index_state *istate)
>   {
> +	struct cache_entry *ce;
> +	int pos;
> +
> +	pos = index_name_pos(istate, GITMODULES_FILE, strlen(GITMODULES_FILE));
> +	ce = (0 <= pos) ? istate->cache[pos] : NULL;
> +
> +	if (ce)
> +		mark_fsmonitor_invalid(istate, ce);
> +
>   	if (add_file_to_index(istate, GITMODULES_FILE, 0))
>   		die(_("staging updated .gitmodules failed"));
>   }
> 

[PATCH v1] git-mv: allow submodules and fsmonitor to work together

[Ben Peart]

It was reported that

   GIT_FSMONITOR_TEST=$PWD/t7519/fsmonitor-all ./t7411-submodule-config.sh

breaks as the fsmonitor data is out of sync with the state of the .gitmodules
file. Update is_staging_gitmodules_ok() so that it no longer tells
ie_match_stat() to ignore refreshing the fsmonitor data.

Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Helped-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Ben Peart <benpeart@microsoft.com>
---

Notes:
    Base Ref: v2.19.0-rc2
    Web-Diff: https://github.com/benpeart/git/commit/ed30e1a885
    Checkout: git fetch https://github.com/benpeart/git fsmonitor-t7411-v1 && git checkout ed30e1a885

 submodule.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/submodule.c b/submodule.c
index 50cbf5f13e..1e7194af28 100644
--- a/submodule.c
+++ b/submodule.c
@@ -65,8 +65,7 @@ int is_staging_gitmodules_ok(struct index_state *istate)
 	if ((pos >= 0) && (pos < istate->cache_nr)) {
 		struct stat st;
 		if (lstat(GITMODULES_FILE, &st) == 0 &&
-		    ie_match_stat(istate, istate->cache[pos], &st,
-				  CE_MATCH_IGNORE_FSMONITOR) & DATA_CHANGED)
+		    ie_match_stat(istate, istate->cache[pos], &st, 0) & DATA_CHANGED)
 			return 0;
 	}
 

base-commit: c05048d43925ab8edcb36663752c2b4541911231
-- 
2.18.0.windows.1

Re: [PATCH v1] git-mv: allow submodules and fsmonitor to work together

[Stefan Beller]

On Mon, Sep 10, 2018 at 9:29 AM Ben Peart <benpeart@microsoft.com> wrote:
>
> It was reported that
>
>    GIT_FSMONITOR_TEST=$PWD/t7519/fsmonitor-all ./t7411-submodule-config.sh
>
> breaks as the fsmonitor data is out of sync with the state of the .gitmodules
> file. Update is_staging_gitmodules_ok() so that it no longer tells
> ie_match_stat() to ignore refreshing the fsmonitor data.

Wondering how this came to be,
7da9aba4178 (submodule: used correct index in is_staging_gitmodules_ok,
2017-12-12) last touched this line, but is unrelated as the fsmonitor
behavior was
there before.

Before that, we have 883e248b8a0 (fsmonitor: teach git to optionally utilize a
file system monitor to speed up detecting new or changed files., 2017-09-22)
that was written by you, who knows the fsmonitor better than I do (or Brandon
who wrote the commit referenced above).

Looking through the archive, it seems that we might have more such hidden
gems?

https://public-inbox.org/git/f50825a4-fa15-9f28-a079-853e78ee8e2e@gmail.com/

Anyway, I think this is a better fix than what I proposed for sure.

Thanks for looking into this!

Stefan

>
> Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Helped-by: Stefan Beller <sbeller@google.com>
> Signed-off-by: Ben Peart <benpeart@microsoft.com>
> ---
>
> Notes:
>     Base Ref: v2.19.0-rc2
>     Web-Diff: https://github.com/benpeart/git/commit/ed30e1a885
>     Checkout: git fetch https://github.com/benpeart/git fsmonitor-t7411-v1 && git checkout ed30e1a885
>
>  submodule.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/submodule.c b/submodule.c
> index 50cbf5f13e..1e7194af28 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -65,8 +65,7 @@ int is_staging_gitmodules_ok(struct index_state *istate)
>         if ((pos >= 0) && (pos < istate->cache_nr)) {
>                 struct stat st;
>                 if (lstat(GITMODULES_FILE, &st) == 0 &&
> -                   ie_match_stat(istate, istate->cache[pos], &st,
> -                                 CE_MATCH_IGNORE_FSMONITOR) & DATA_CHANGED)
> +                   ie_match_stat(istate, istate->cache[pos], &st, 0) & DATA_CHANGED)
>                         return 0;
>         }
>
>
> base-commit: c05048d43925ab8edcb36663752c2b4541911231
> --
> 2.18.0.windows.1
>

Re: [PATCH v1] git-mv: allow submodules and fsmonitor to work together

[Ben Peart]

On 9/10/2018 1:07 PM, Stefan Beller wrote:
> On Mon, Sep 10, 2018 at 9:29 AM Ben Peart <benpeart@microsoft.com> wrote:
>>
>> It was reported that
>>
>>     GIT_FSMONITOR_TEST=$PWD/t7519/fsmonitor-all ./t7411-submodule-config.sh
>>
>> breaks as the fsmonitor data is out of sync with the state of the .gitmodules
>> file. Update is_staging_gitmodules_ok() so that it no longer tells
>> ie_match_stat() to ignore refreshing the fsmonitor data.
> 
> Wondering how this came to be,
> 7da9aba4178 (submodule: used correct index in is_staging_gitmodules_ok,
> 2017-12-12) last touched this line, but is unrelated as the fsmonitor
> behavior was
> there before.
> 
> Before that, we have 883e248b8a0 (fsmonitor: teach git to optionally utilize a
> file system monitor to speed up detecting new or changed files., 2017-09-22)
> that was written by you, who knows the fsmonitor better than I do (or Brandon
> who wrote the commit referenced above).
> 
> Looking through the archive, it seems that we might have more such hidden
> gems?

Fortunately, the only one left is the one in preload_index() which is 
what the flag was created to handle so I think we're ok.

> 
> https://public-inbox.org/git/f50825a4-fa15-9f28-a079-853e78ee8e2e@gmail.com/
> 
> Anyway, I think this is a better fix than what I proposed for sure.
> 
> Thanks for looking into this!
> 
> Stefan
> 
>>
>> Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> Helped-by: Stefan Beller <sbeller@google.com>
>> Signed-off-by: Ben Peart <benpeart@microsoft.com>
>> ---
>>
>> Notes:
>>      Base Ref: v2.19.0-rc2
>>      Web-Diff: https://github.com/benpeart/git/commit/ed30e1a885
>>      Checkout: git fetch https://github.com/benpeart/git fsmonitor-t7411-v1 && git checkout ed30e1a885
>>
>>   submodule.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/submodule.c b/submodule.c
>> index 50cbf5f13e..1e7194af28 100644
>> --- a/submodule.c
>> +++ b/submodule.c
>> @@ -65,8 +65,7 @@ int is_staging_gitmodules_ok(struct index_state *istate)
>>          if ((pos >= 0) && (pos < istate->cache_nr)) {
>>                  struct stat st;
>>                  if (lstat(GITMODULES_FILE, &st) == 0 &&
>> -                   ie_match_stat(istate, istate->cache[pos], &st,
>> -                                 CE_MATCH_IGNORE_FSMONITOR) & DATA_CHANGED)
>> +                   ie_match_stat(istate, istate->cache[pos], &st, 0) & DATA_CHANGED)
>>                          return 0;
>>          }
>>
>>
>> base-commit: c05048d43925ab8edcb36663752c2b4541911231
>> --
>> 2.18.0.windows.1
>>