What's cooking in git.git (Jan 2017, #02; Sun, 15)

What's cooking in git.git (Jan 2017, #02; Sun, 15)

[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

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

* bw/read-blob-data-does-not-modify-index-state (2017-01-11) 1 commit
 - index: improve constness for reading blob data

 Code clean-up.

 Will merge to 'next'.


* ep/commit-static-buf-cleanup (2017-01-13) 2 commits
 - builtin/commit.c: switch to xstrfmt(), instead of snprintf()
 - builtin/commit.c: remove the PATH_MAX limitation via dynamic allocation

 Code clean-up.

 Expecting a reroll.
 The tip one would instead be done with strbuf.
 cf. <CA+EOSB=4-TKpi6mr-yVbwRsFrVzE=vo4Y9Qm3VMm7pn=UB1_hQ@mail.gmail.com>


* jk/grep-e-could-be-extended-beyond-posix (2017-01-11) 1 commit
 - t7810: avoid assumption about invalid regex syntax

 Tighten a test to avoid mistaking an extended ERE regexp engine as
 a PRE regexp engine.

 Will merge to 'next'.


* jk/vreport-sanitize (2017-01-11) 2 commits
 - vreport: sanitize ASCII control chars
 - Revert "vreportf: avoid intermediate buffer"

 An error message with an ASCII control character like '\r' in it
 can alter the message to hide its early part, which is problematic
 when a remote side gives such an error message that the local side
 will relay with a "remote: " prefix.

 Will merge to 'next'.


* sb/unpack-trees-super-prefix (2017-01-12) 5 commits
 - SQUASH
 - unpack-trees: support super-prefix option
 - t1001: modernize style
 - t1000: modernize style
 - read-tree: use OPT_BOOL instead of OPT_SET_INT

 "git read-tree" and its underlying unpack_trees() machinery learned
 to report problematic paths prefixed with the --super-prefix option.

 Expecting a reroll.
 The first three are in good shape.  The last one needs a better
 explanation and possibly an update to its test.
 cf. <CAGZ79kaHDnVLQr8WLoaD5KKs7EqeW=KbkptF=iHpU5t054Xcdw@mail.gmail.com>


* bw/grep-recurse-submodules (2016-12-22) 12 commits
  (merged to 'next' on 2016-12-22 at 1ede815b8d)
 + grep: search history of moved submodules
 + grep: enable recurse-submodules to work on <tree> objects
 + grep: optionally recurse into submodules
 + grep: add submodules as a grep source type
 + submodules: load gitmodules file from commit sha1
 + submodules: add helper to determine if a submodule is initialized
 + submodules: add helper to determine if a submodule is populated
  (merged to 'next' on 2016-12-22 at fea8fa870f)
 + real_path: canonicalize directory separators in root parts
 + real_path: have callers use real_pathdup and strbuf_realpath
 + real_path: create real_pathdup
 + real_path: convert real_path_internal to strbuf_realpath
 + real_path: resolve symlinks by hand
 (this branch is tangled with bw/realpath-wo-chdir.)

 "git grep" has been taught to optionally recurse into submodules.

 Will merge to 'master'.


* sb/submodule-config-tests (2017-01-12) 2 commits
 - t7411: test lookup of uninitialized submodules
 - t7411: quote URLs

 Will merge to 'next'.


* sb/submodule-doc (2017-01-12) 3 commits
 - submodules: add a background story
 - submodule update documentation: don't repeat ourselves
 - submodule documentation: add options to the subcommand

 Needs review.


* sb/submodule-embed-gitdir (2017-01-12) 1 commit
 - submodule absorbgitdirs: mention in docstring help

 Help-text fix.

 Will merge to 'next'.


* sb/submodule-init (2017-01-12) 1 commit
 - submodule update --init: display correct path from submodule

 Error message fix.

 Will merge to 'next'.


* vn/diff-ihc-config (2017-01-12) 1 commit
 - diff: add interhunk context config option

 "git diff" learned diff.interHunkContext configuration variable
 that gives the default value for its --inter-hunk-context option.

 Will merge to 'next'.


* ad/bisect-terms (2017-01-13) 1 commit
 - Documentation/bisect: improve on (bad|new) and (good|bad)

 Documentation fix.

 Will merge to 'next'.


* bw/attr (2017-01-15) 27 commits
 - attr: reformat git_attr_set_direction() function
 - attr: push the bare repo check into read_attr()
 - attr: store attribute stacks in hashmap
 - attr: tighten const correctness with git_attr and match_attr
 - attr: remove maybe-real, maybe-macro from git_attr
 - attr: eliminate global check_all_attr array
 - attr: use hashmap for attribute dictionary
 - attr: change validity check for attribute names to use positive logic
 - attr: pass struct attr_check to collect_some_attrs
 - attr: retire git_check_attrs() API
 - attr: convert git_check_attrs() callers to use the new API
 - attr: convert git_all_attrs() to use "struct attr_check"
 - attr: (re)introduce git_check_attr() and struct attr_check
 - attr: rename function and struct related to checking attributes
 - attr.c: outline the future plans by heavily commenting
 - Documentation/gitattributes.txt: fix a typo
 - attr.c: add push_stack() helper
 - attr: support quoting pathname patterns in C style
 - attr.c: plug small leak in parse_attr_line()
 - attr.c: tighten constness around "git_attr" structure
 - attr.c: simplify macroexpand_one()
 - attr.c: mark where #if DEBUG ends more clearly
 - attr.c: complete a sentence in a comment
 - attr.c: explain the lack of attr-name syntax check in parse_attr()
 - attr.c: update a stale comment on "struct match_attr"
 - attr.c: use strchrnul() to scan for one line
 - commit.c: use strchrnul() to scan for one line

 The gitattributes machinery is being taught to work better in a
 multi-threaded environment.

 Needs review.


* jk/loose-object-fsck (2017-01-15) 6 commits
 - fsck: detect trailing garbage in all object types
 - fsck: parse loose object paths directly
 - sha1_file: add read_loose_object() function
 - t1450: test fsck of packed objects
 - sha1_file: fix error message for alternate objects
 - t1450: refactor loose-object removal

 "git fsck" inspects loose objects more carefully now.

 Needs review.


* rh/diff-orderfile-doc (2017-01-15) 2 commits
 - diff: document the format of the -O (diff.orderFile) file
 - diff: document behavior of relative diff.orderFile

 Documentation fix.

 Will merge to 'next'.


* sb/cd-then-git-can-be-written-as-git-c (2017-01-13) 1 commit
 - lib-submodule-update.sh: reduce use of subshell by using "git -C"

 Test clean-up.

 Will merge to 'next'.


* vn/xdiff-func-context (2017-01-15) 1 commit
 - xdiff -W: relax end-of-file function detection

 "git diff -W" has been taught to handle the case where a new
 function is added at the end of the file better.

 Will hold.
 An follow-up change to go back from the line that matches the
 funcline to show comments before the function definition is still
 being discussed.


* ws/request-pull-code-cleanup (2017-01-15) 1 commit
 - request-pull: drop old USAGE stuff

 Code clean-up.

 Will merge to 'next'.

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

* jk/nofollow-attr-ignore (2016-11-02) 5 commits
 . exclude: do not respect symlinks for in-tree .gitignore
 . attr: do not respect symlinks for in-tree .gitattributes
 . exclude: convert "check_index" into a flags field
 . attr: convert "macro_ok" into a flags field
 . add open_nofollow() helper

 As we do not follow symbolic links when reading control files like
 .gitignore and .gitattributes from the index, match the behaviour
 and not follow symbolic links when reading them from the working
 tree.  This also tightens security a bit by not leaking contents of
 an unrelated file in the error messages when it is pointed at by
 one of these files that is a symbolic link.

 Perhaps we want to cover .gitmodules too with the same mechanism?

 Ejected for now.


* jc/bundle (2016-03-03) 6 commits
 - index-pack: --clone-bundle option
 - Merge branch 'jc/index-pack' into jc/bundle
 - bundle v3: the beginning
 - bundle: keep a copy of bundle file name in the in-core bundle header
 - bundle: plug resource leak
 - bundle doc: 'verify' is not about verifying the bundle

 The beginning of "split bundle", which could be one of the
 ingredients to allow "git clone" traffic off of the core server
 network to CDN.

 While I think it would make it easier for people to experiment and
 build on if the topic is merged to 'next', I am at the same time a
 bit reluctant to merge an unproven new topic that introduces a new
 file format, which we may end up having to support til the end of
 time.  It is likely that to support a "prime clone from CDN", it
 would need a lot more than just "these are the heads and the pack
 data is over there", so this may not be sufficient.

 Will discard.


* jc/diff-b-m (2015-02-23) 5 commits
 . WIPWIP
 . WIP: diff-b-m
 - diffcore-rename: allow easier debugging
 - diffcore-rename.c: add locate_rename_src()
 - diffcore-break: allow debugging

 "git diff -B -M" produced incorrect patch when the postimage of a
 completely rewritten file is similar to the preimage of a removed
 file; such a resulting file must not be expressed as a rename from
 other place.

 The fix in this patch is broken, unfortunately.

 Will discard.

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

* ls/p4-retry-thrice (2016-12-29) 1 commit
  (merged to 'next' on 2017-01-10 at c733e27410)
 + git-p4: do not pass '-r 0' to p4 commands

 A recent updates to "git p4" was not usable for older p4 but it
 could be made to work with minimum changes.  Do so.

 Will merge to 'master'.


* mh/ref-remove-empty-directory (2017-01-07) 23 commits
 - files_transaction_commit(): clean up empty directories
 - try_remove_empty_parents(): teach to remove parents of reflogs, too
 - try_remove_empty_parents(): don't trash argument contents
 - try_remove_empty_parents(): rename parameter "name" -> "refname"
 - delete_ref_loose(): inline function
 - delete_ref_loose(): derive loose reference path from lock
 - log_ref_write_1(): inline function
 - log_ref_setup(): manage the name of the reflog file internally
 - log_ref_write_1(): don't depend on logfile argument
 - log_ref_setup(): pass the open file descriptor back to the caller
 - log_ref_setup(): improve robustness against races
 - log_ref_setup(): separate code for create vs non-create
 - log_ref_write(): inline function
 - rename_tmp_log(): improve error reporting
 - rename_tmp_log(): use raceproof_create_file()
 - lock_ref_sha1_basic(): use raceproof_create_file()
 - lock_ref_sha1_basic(): inline constant
 - raceproof_create_file(): new function
 - safe_create_leading_directories(): set errno on SCLD_EXISTS
 - safe_create_leading_directories_const(): preserve errno
 - t5505: use "for-each-ref" to test for the non-existence of references
 - refname_is_safe(): correct docstring
 - files_rename_ref(): tidy up whitespace

 Deletion of a branch "foo/bar" could remove .git/refs/heads/foo
 once there no longer is any other branch whose name begins with
 "foo/", but we didn't do so so far.  Now we do.

 Expecting a reroll.
 cf. <5051c78e-51f9-becd-e1a6-9c0b781d6912@alum.mit.edu>


* pb/test-must-fail-is-for-git (2017-01-09) 2 commits
  (merged to 'next' on 2017-01-10 at 5f24a98779)
 + t9813: avoid using pipes
 + don't use test_must_fail with grep

 Test cleanup.

 Will merge to 'master'.


* jk/archive-zip-userdiff-config (2017-01-07) 1 commit
  (merged to 'next' on 2017-01-10 at ac42e4958c)
 + archive-zip: load userdiff config

 "git archive" did not read the standard configuration files, and
 failed to notice a file that is marked as binary via the userdiff
 driver configuration.

 Will merge to 'master'.


* jk/blame-fixes (2017-01-07) 3 commits
  (merged to 'next' on 2017-01-10 at 18f909da61)
 + blame: output porcelain "previous" header for each file
 + blame: handle --no-abbrev
 + blame: fix alignment with --abbrev=40

 "git blame --porcelain" misidentified the "previous" <commit, path>
 pair (aka "source") when contents came from two or more files.

 Will merge to 'master'.


* jk/rebase-i-squash-count-fix (2017-01-07) 1 commit
  (merged to 'next' on 2017-01-10 at d6cfc6ace2)
 + rebase--interactive: count squash commits above 10 correctly

 "git rebase -i" with a recent update started showing an incorrect
 count when squashing more than 10 commits.

 Will merge to 'master'.


* js/asciidoctor-tweaks (2017-01-13) 2 commits
  (merged to 'next' on 2017-01-15 at 8553c6e513)
 + asciidoctor: fix user-manual to be built by `asciidoctor`
  (merged to 'next' on 2017-01-10 at 087da7b7c1)
 + giteveryday: unbreak rendering with AsciiDoctor

 Adjust documentation to help AsciiDoctor render better while not
 breaking the rendering done by AsciiDoc.

 Will merge to 'master'.


* km/branch-get-push-while-detached (2017-01-07) 1 commit
  (merged to 'next' on 2017-01-10 at a7f8af8c55)
 + branch_get_push: do not segfault when HEAD is detached

 "git <cmd> @{push}" on a detached HEAD used to segfault; it has
 been corrected to error out with a message.

 Will merge to 'master'.


* sb/remove-gitview (2017-01-13) 4 commits
  (merged to 'next' on 2017-01-15 at 7c9eae479e)
 + doc: git-gui browser does not default to HEAD
 + doc: gitk: add the upstream repo location
 + doc: gitk: remove gitview reference
  (merged to 'next' on 2017-01-10 at dcb3abd146)
 + contrib: remove gitview

 Will merge to 'master'.


* sb/submodule-cleanup-export-git-dir-env (2017-01-07) 1 commit
  (merged to 'next' on 2017-01-10 at 2d5db6821e)
 + submodule.c: use GIT_DIR_ENVIRONMENT consistently

 Code cleanup.

 Will merge to 'master'.


* sb/pathspec-errors (2017-01-09) 1 commit
  (merged to 'next' on 2017-01-10 at 432375cb62)
 + pathspec: give better message for submodule related pathspec error
 (this branch uses bw/pathspec-cleanup.)

 Running "git add a/b" when "a" is a submodule correctly errored
 out, but without a meaningful error message.

 Will merge to 'master'.


* ls/filter-process-delayed (2017-01-08) 1 commit
 . convert: add "status=delayed" to filter process protocol

 Ejected, as does not build when merged to 'pu'.


* sp/cygwin-build-fixes (2017-01-09) 2 commits
  (merged to 'next' on 2017-01-10 at 2010fb6c03)
 + Makefile: put LIBS after LDFLAGS for imap-send
 + Makefile: POSIX windres

 Build updates for Cygwin.

 Will merge to 'master'.


* jk/execv-dashed-external (2017-01-09) 3 commits
  (merged to 'next' on 2017-01-10 at 117b506cb0)
 + execv_dashed_external: wait for child on signal death
 + execv_dashed_external: stop exiting with negative code
 + execv_dashed_external: use child_process struct

 Typing ^C to pager, which usually does not kill it, killed Git and
 took the pager down as a collateral damage in certain process-tree
 structure.  This has been fixed.

 Will merge to 'master'.


* rh/mergetool-regression-fix (2017-01-10) 14 commits
  (merged to 'next' on 2017-01-10 at e8e00c798b)
 + mergetool: fix running in subdir when rerere enabled
 + mergetool: take the "-O" out of $orderfile
 + t7610: add test case for rerere+mergetool+subdir bug
 + t7610: spell 'git reset --hard' consistently
 + t7610: don't assume the checked-out commit
 + t7610: always work on a test-specific branch
 + t7610: delete some now-unnecessary 'git reset --hard' lines
 + t7610: run 'git reset --hard' after each test to clean up
 + t7610: don't rely on state from previous test
 + t7610: use test_when_finished for cleanup tasks
 + t7610: move setup code to the 'setup' test case
 + t7610: update branch names to match test number
 + rev-parse doc: pass "--" to rev-parse in the --prefix example
 + .mailmap: record canonical email for Richard Hansen

 "git mergetool" without any pathspec on the command line that is
 run from a subdirectory became no-op in Git v2.11 by mistake, which
 has been fixed.

 Will merge to 'master'.


* sb/unpack-trees-cleanup (2017-01-10) 3 commits
  (merged to 'next' on 2017-01-10 at 95a5f3127c)
 + unpack-trees: factor progress setup out of check_updates
 + unpack-trees: remove unneeded continue
 + unpack-trees: move checkout state into check_updates

 Code cleanup.

 Will merge to 'master'.


* nd/worktree-move (2017-01-09) 6 commits
 - worktree remove: new command
 - worktree move: refuse to move worktrees with submodules
 - worktree move: accept destination as directory
 - worktree move: new command
 - worktree.c: add update_worktree_location()
 - worktree.c: add validate_worktree()

 "git worktree" learned move and remove subcommands.


* dt/disable-bitmap-in-auto-gc (2016-12-29) 2 commits
  (merged to 'next' on 2017-01-10 at 9f4e89e15d)
 + repack: die on incremental + write-bitmap-index
 + auto gc: don't write bitmaps for incremental repacks

 It is natural that "git gc --auto" may not attempt to pack
 everything into a single pack, and there is no point in warning
 when the user has configured the system to use the pack bitmap,
 leading to disabling further "gc".

 Will merge to 'master'.


* js/mingw-test-push-unc-path (2017-01-07) 1 commit
  (merged to 'next' on 2017-01-10 at 249d9f26f3)
 + mingw: add a regression test for pushing to UNC paths

 "git push \\server\share\dir" has recently regressed and then
 fixed.  A test has retroactively been added for this breakage.

 Will merge to 'master'.


* nd/log-graph-configurable-colors (2017-01-08) 1 commit
 - log --graph: customize the graph lines with config log.graphColors

 Some people feel the default set of colors used by "git log --graph"
 rather limiting.  A mechanism to customize the set of colors has
 been introduced.

 Waiting for review comments to be addressed.
 cf. <20170109103258.25341-1-pclouds@gmail.com>


* sb/submodule-rm-absorb (2016-12-27) 4 commits
  (merged to 'next' on 2017-01-10 at 1fc2000a92)
 + rm: absorb a submodules git dir before deletion
 + submodule: rename and add flags to ok_to_remove_submodule
 + submodule: modernize ok_to_remove_submodule to use argv_array
 + submodule.h: add extern keyword to functions

 "git rm" used to refuse to remove a submodule when it has its own
 git repository embedded in its working tree.  It learned to move
 the repository away to $GIT_DIR/modules/ of the superproject
 instead, and allow the submodule to be deleted (as long as there
 will be no loss of local modifications, that is).

 Will merge to 'master'.


* cc/split-index-config (2016-12-26) 21 commits
 - Documentation/git-update-index: explain splitIndex.*
 - Documentation/config: add splitIndex.sharedIndexExpire
 - read-cache: use freshen_shared_index() in read_index_from()
 - read-cache: refactor read_index_from()
 - t1700: test shared index file expiration
 - read-cache: unlink old sharedindex files
 - config: add git_config_get_expiry() from gc.c
 - read-cache: touch shared index files when used
 - sha1_file: make check_and_freshen_file() non static
 - Documentation/config: add splitIndex.maxPercentChange
 - t1700: add tests for splitIndex.maxPercentChange
 - read-cache: regenerate shared index if necessary
 - config: add git_config_get_max_percent_split_change()
 - Documentation/git-update-index: talk about core.splitIndex config var
 - Documentation/config: add information for core.splitIndex
 - t1700: add tests for core.splitIndex
 - update-index: warn in case of split-index incoherency
 - read-cache: add and then use tweak_split_index()
 - split-index: add {add,remove}_split_index() functions
 - config: add git_config_get_split_index()
 - config: mark an error message up for translation

 The experimental "split index" feature has gained a few
 configuration variables to make it easier to use.

 Waiting for review comments to be addressed.
 cf. <20161226102222.17150-1-chriscool@tuxfamily.org>
 cf. <a1a44640-ff6c-2294-72ac-46322eff8505@ramsayjones.plus.com>


* bw/push-submodule-only (2016-12-20) 3 commits
 - push: add option to push only submodules
 - submodules: add RECURSE_SUBMODULES_ONLY value
 - transport: reformat flag #defines to be more readable

 "git submodule push" learned "--recurse-submodules=only option to
 push submodules out without pushing the top-level superproject.


* ls/p4-path-encoding (2016-12-18) 1 commit
 - git-p4: fix git-p4.pathEncoding for removed files

 When "git p4" imports changelist that removes paths, it failed to
 convert pathnames when the p4 used encoding different from the one
 used on the Git side.  This has been corrected.

 Will be rerolled.
 cf. <7E1C7387-4F37-423F-803D-3B5690B49D40@gmail.com>


* bw/pathspec-cleanup (2017-01-08) 16 commits
  (merged to 'next' on 2017-01-10 at 79291ff506)
 + pathspec: rename prefix_pathspec to init_pathspec_item
 + pathspec: small readability changes
 + pathspec: create strip submodule slash helpers
 + pathspec: create parse_element_magic helper
 + pathspec: create parse_long_magic function
 + pathspec: create parse_short_magic function
 + pathspec: factor global magic into its own function
 + pathspec: simpler logic to prefix original pathspec elements
 + pathspec: always show mnemonic and name in unsupported_magic
 + pathspec: remove unused variable from unsupported_magic
 + pathspec: copy and free owned memory
 + pathspec: remove the deprecated get_pathspec function
 + ls-tree: convert show_recursive to use the pathspec struct interface
 + dir: convert fill_directory to use the pathspec struct interface
 + dir: remove struct path_simplify
 + mv: remove use of deprecated 'get_pathspec()'
 (this branch is used by sb/pathspec-errors.)

 Code clean-up in the pathspec API.

 Will merge to 'master'.


* js/prepare-sequencer-more (2017-01-09) 38 commits
 - sequencer (rebase -i): write out the final message
 - sequencer (rebase -i): write the progress into files
 - sequencer (rebase -i): show the progress
 - sequencer (rebase -i): suggest --edit-todo upon unknown command
 - sequencer (rebase -i): show only failed cherry-picks' output
 - sequencer (rebase -i): show only failed `git commit`'s output
 - sequencer: use run_command() directly
 - sequencer: make reading author-script more elegant
 - sequencer (rebase -i): differentiate between comments and 'noop'
 - sequencer (rebase -i): implement the 'drop' command
 - sequencer (rebase -i): allow rescheduling commands
 - sequencer (rebase -i): respect strategy/strategy_opts settings
 - sequencer (rebase -i): respect the rebase.autostash setting
 - sequencer (rebase -i): run the post-rewrite hook, if needed
 - sequencer (rebase -i): record interrupted commits in rewritten, too
 - sequencer (rebase -i): copy commit notes at end
 - sequencer (rebase -i): set the reflog message consistently
 - sequencer (rebase -i): refactor setting the reflog message
 - sequencer (rebase -i): allow fast-forwarding for edit/reword
 - sequencer (rebase -i): implement the 'reword' command
 - sequencer (rebase -i): leave a patch upon error
 - sequencer (rebase -i): update refs after a successful rebase
 - sequencer (rebase -i): the todo can be empty when continuing
 - sequencer (rebase -i): skip some revert/cherry-pick specific code path
 - sequencer (rebase -i): remove CHERRY_PICK_HEAD when no longer needed
 - sequencer (rebase -i): allow continuing with staged changes
 - sequencer (rebase -i): write an author-script file
 - sequencer (rebase -i): implement the short commands
 - sequencer (rebase -i): add support for the 'fixup' and 'squash' commands
 - sequencer (rebase -i): write the 'done' file
 - sequencer (rebase -i): learn about the 'verbose' mode
 - sequencer (rebase -i): implement the 'exec' command
 - sequencer (rebase -i): implement the 'edit' command
 - sequencer (rebase -i): implement the 'noop' command
 - sequencer: support a new action: 'interactive rebase'
 - sequencer: use a helper to find the commit message
 - sequencer: move "else" keyword onto the same line as preceding brace
 - sequencer: avoid unnecessary curly braces

 The sequencer has further been extended in preparation to act as a
 back-end for "rebase -i".

 Waiting for review comments to be addressed.


* bw/realpath-wo-chdir (2017-01-09) 7 commits
  (merged to 'next' on 2017-01-10 at ed315a40c8)
 + real_path: set errno when max number of symlinks is exceeded
 + real_path: prevent redefinition of MAXSYMLINKS
  (merged to 'next' on 2016-12-22 at fea8fa870f)
 + real_path: canonicalize directory separators in root parts
 + real_path: have callers use real_pathdup and strbuf_realpath
 + real_path: create real_pathdup
 + real_path: convert real_path_internal to strbuf_realpath
 + real_path: resolve symlinks by hand
 (this branch is tangled with bw/grep-recurse-submodules.)

 The implementation of "real_path()" was to go there with chdir(2)
 and call getcwd(3), but this obviously wouldn't be usable in a
 threaded environment.  Rewrite it to manually resolve relative
 paths including symbolic links in path components.

 Will merge to 'master'.


* js/difftool-builtin (2017-01-09) 5 commits
 - t7800: run both builtin and scripted difftool, for now
 - difftool: implement the functionality in the builtin
 - difftool: add a skeleton for the upcoming builtin
 - git_exec_path: do not return the result of getenv()
 - git_exec_path: avoid Coverity warning about unfree()d result

 Rewrite a scripted porcelain "git difftool" in C.

 Expecting a reroll.
 cf. <alpine.DEB.2.20.1701091228460.3469@virtualbox>


* sb/push-make-submodule-check-the-default (2016-11-29) 2 commits
  (merged to 'next' on 2016-12-12 at 1863e05af5)
 + push: change submodule default to check when submodules exist
 + submodule add: extend force flag to add existing repos

 Turn the default of "push.recurseSubmodules" to "check" when
 submodules seem to be in use.

 Will cook in 'next'.


* kn/ref-filter-branch-list (2017-01-10) 21 commits
 - SQUASH???
 - branch: implement '--format' option
 - branch: use ref-filter printing APIs
 - branch, tag: use porcelain output
 - ref-filter: allow porcelain to translate messages in the output
 - ref-filter: add an 'rstrip=<N>' option to atoms which deal with refnames
 - ref-filter: modify the 'lstrip=<N>' option to work with negative '<N>'
 - ref-filter: Do not abruptly die when using the 'lstrip=<N>' option
 - ref-filter: rename the 'strip' option to 'lstrip'
 - ref-filter: make remote_ref_atom_parser() use refname_atom_parser_internal()
 - ref-filter: introduce refname_atom_parser()
 - ref-filter: introduce refname_atom_parser_internal()
 - ref-filter: make "%(symref)" atom work with the ':short' modifier
 - ref-filter: add support for %(upstream:track,nobracket)
 - ref-filter: make %(upstream:track) prints "[gone]" for invalid upstreams
 - ref-filter: introduce format_ref_array_item()
 - ref-filter: move get_head_description() from branch.c
 - ref-filter: modify "%(objectname:short)" to take length
 - ref-filter: implement %(if:equals=<string>) and %(if:notequals=<string>)
 - ref-filter: include reference to 'used_atom' within 'atom_value'
 - ref-filter: implement %(if), %(then), and %(else) atoms

 The code to list branches in "git branch" has been consolidated
 with the more generic ref-filter API.

 I think this is almost ready.  Will wait for a few days, squash
 fixes in if needed and merge to 'next'.


* jk/no-looking-at-dotgit-outside-repo-final (2016-10-26) 1 commit
  (merged to 'next' on 2016-12-05 at 0c77e39cd5)
 + setup_git_env: avoid blind fall-back to ".git"

 Originally merged to 'next' on 2016-10-26

 This is the endgame of the topic to avoid blindly falling back to
 ".git" when the setup sequence said we are _not_ in Git repository.
 A corner case that happens to work right now may be broken by a
 call to die("BUG").

 Will cook in 'next'.


* pb/bisect (2016-10-18) 27 commits
 - bisect--helper: remove the dequote in bisect_start()
 - bisect--helper: retire `--bisect-auto-next` subcommand
 - bisect--helper: retire `--bisect-autostart` subcommand
 - bisect--helper: retire `--bisect-write` subcommand
 - bisect--helper: `bisect_replay` shell function in C
 - bisect--helper: `bisect_log` shell function in C
 - bisect--helper: retire `--write-terms` subcommand
 - bisect--helper: retire `--check-expected-revs` subcommand
 - bisect--helper: `bisect_state` & `bisect_head` shell function in C
 - bisect--helper: `bisect_autostart` shell function in C
 - bisect--helper: retire `--next-all` subcommand
 - bisect--helper: retire `--bisect-clean-state` subcommand
 - bisect--helper: `bisect_next` and `bisect_auto_next` shell function in C
 - t6030: no cleanup with bad merge base
 - bisect--helper: `bisect_start` shell function partially in C
 - bisect--helper: `get_terms` & `bisect_terms` shell function in C
 - bisect--helper: `bisect_next_check` & bisect_voc shell function in C
 - bisect--helper: `check_and_set_terms` shell function in C
 - bisect--helper: `bisect_write` shell function in C
 - bisect--helper: `is_expected_rev` & `check_expected_revs` shell function in C
 - bisect--helper: `bisect_reset` shell function in C
 - wrapper: move is_empty_file() and rename it as is_empty_or_missing_file()
 - t6030: explicitly test for bisection cleanup
 - bisect--helper: `bisect_clean_state` shell function in C
 - bisect--helper: `write_terms` shell function in C
 - bisect: rewrite `check_term_format` shell function in C
 - bisect--helper: use OPT_CMDMODE instead of OPT_BOOL

 Move more parts of "git bisect" to C.

 Expecting a reroll.
 cf. <CAFZEwPPXPPHi8KiEGS9ggzNHDCGhuqMgH9Z8-Pf9GLshg8+LPA@mail.gmail.com>
 cf. <CAFZEwPM9RSTGN54dzaw9gO9iZmsYjJ_d1SjUD4EzSDDbmh-XuA@mail.gmail.com>


* st/verify-tag (2016-10-10) 7 commits
 - t/t7004-tag: Add --format specifier tests
 - t/t7030-verify-tag: Add --format specifier tests
 - builtin/tag: add --format argument for tag -v
 - builtin/verify-tag: add --format to verify-tag
 - tag: add format specifier to gpg_verify_tag
 - ref-filter: add function to print single ref_array_item
 - gpg-interface, tag: add GPG_VERIFY_QUIET flag

 "git tag" and "git verify-tag" learned to put GPG verification
 status in their "--format=<placeholders>" output format.

 Waiting for a reroll.
 cf. <20161007210721.20437-1-santiago@nyu.edu>


* sg/fix-versioncmp-with-common-suffix (2017-01-12) 8 commits
 - versioncmp: generalize version sort suffix reordering
 - versioncmp: factor out helper for suffix matching
 - versioncmp: use earliest-longest contained suffix to determine sorting order
 - versioncmp: cope with common part overlapping with prerelease suffix
 - versioncmp: pass full tagnames to swap_prereleases()
 - t7004-tag: add version sort tests to show prerelease reordering issues
 - t7004-tag: use test_config helper
 - t7004-tag: delete unnecessary tags with test_when_finished

 The prereleaseSuffix feature of version comparison that is used in
 "git tag -l" did not correctly when two or more prereleases for the
 same release were present (e.g. when 2.0, 2.0-beta1, and 2.0-beta2
 are there and the code needs to compare 2.0-beta1 and 2.0-beta2).

 Will merge to 'next'.


* jc/merge-drop-old-syntax (2015-04-29) 1 commit
  (merged to 'next' on 2016-12-05 at 041946dae0)
 + merge: drop 'git merge <message> HEAD <commit>' syntax

 Originally merged to 'next' on 2016-10-11

 Stop supporting "git merge <message> HEAD <commit>" syntax that has
 been deprecated since October 2007, and issues a deprecation
 warning message since v2.5.0.

 Will cook in 'next'.

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

* sb/attr (2016-11-11) 35 commits
 . completion: clone can initialize specific submodules
 . clone: add --init-submodule=<pathspec> switch
 . submodule update: add `--init-default-path` switch
 . pathspec: allow escaped query values
 . pathspec: allow querying for attributes
 . pathspec: move prefix check out of the inner loop
 . pathspec: move long magic parsing out of prefix_pathspec
 . Documentation: fix a typo
 . attr: keep attr stack for each check
 . attr: convert to new threadsafe API
 . attr: make git_check_attr_counted static
 . attr.c: outline the future plans by heavily commenting
 . attr.c: always pass check[] to collect_some_attrs()
 . attr.c: introduce empty_attr_check_elems()
 . attr.c: correct ugly hack for git_all_attrs()
 . attr.c: rename a local variable check
 . attr.c: pass struct git_attr_check down the callchain
 . attr.c: add push_stack() helper
 . attr: support quoting pathname patterns in C style
 . attr: expose validity check for attribute names
 . attr: add counted string version of git_check_attr()
 . attr: retire git_check_attrs() API
 . attr: convert git_check_attrs() callers to use the new API
 . attr: convert git_all_attrs() to use "struct git_attr_check"
 . attr: (re)introduce git_check_attr() and struct git_attr_check
 . attr: rename function and struct related to checking attributes
 . attr.c: plug small leak in parse_attr_line()
 . attr.c: tighten constness around "git_attr" structure
 . attr.c: simplify macroexpand_one()
 . attr.c: mark where #if DEBUG ends more clearly
 . attr.c: complete a sentence in a comment
 . attr.c: explain the lack of attr-name syntax check in parse_attr()
 . attr.c: update a stale comment on "struct match_attr"
 . attr.c: use strchrnul() to scan for one line
 . commit.c: use strchrnul() to scan for one line

 The attributes API has been updated so that it can later be
 optimized using the knowledge of which attributes are queried.
 Building on top of the updated API, the pathspec machinery learned
 to select only paths with given attributes set.

 Superseded by bw/attr topic.

Re: What's cooking in git.git (Jan 2017, #02; Sun, 15)

[Johannes Sixt]

Am 16.01.2017 um 02:51 schrieb Junio C Hamano:
> * jk/vreport-sanitize (2017-01-11) 2 commits
>  - vreport: sanitize ASCII control chars
>  - Revert "vreportf: avoid intermediate buffer"
>
>  An error message with an ASCII control character like '\r' in it
>  can alter the message to hide its early part, which is problematic
>  when a remote side gives such an error message that the local side
>  will relay with a "remote: " prefix.
>
>  Will merge to 'next'.

Please be not too hasty with advancing this topic to master. I could 
imagine that there is some unwanted fallout on systems where the 
end-of-line marker CRLF is common. Though, I haven't tested the topic 
myself, yet, nor do I expect regressions in *my* typical workflow.

-- Hannes

Re: What's cooking in git.git (Jan 2017, #02; Sun, 15)

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> Am 16.01.2017 um 02:51 schrieb Junio C Hamano:
>> * jk/vreport-sanitize (2017-01-11) 2 commits
>>  - vreport: sanitize ASCII control chars
>>  - Revert "vreportf: avoid intermediate buffer"
>>
>>  An error message with an ASCII control character like '\r' in it
>>  can alter the message to hide its early part, which is problematic
>>  when a remote side gives such an error message that the local side
>>  will relay with a "remote: " prefix.
>>
>>  Will merge to 'next'.
>
> Please be not too hasty with advancing this topic to master. I could
> imagine that there is some unwanted fallout on systems where the
> end-of-line marker CRLF is common. Though, I haven't tested the topic
> myself, yet, nor do I expect regressions in *my* typical workflow.

Thanks; will wait for a further discussion on the topic's thread
then.

Re: What's cooking in git.git (Jan 2017, #02; Sun, 15)

[Johannes Schindelin]

Hi Hannes,

On Mon, 16 Jan 2017, Johannes Sixt wrote:

> Am 16.01.2017 um 02:51 schrieb Junio C Hamano:
> > * jk/vreport-sanitize (2017-01-11) 2 commits
> >  - vreport: sanitize ASCII control chars
> >  - Revert "vreportf: avoid intermediate buffer"
> >
> >  An error message with an ASCII control character like '\r' in it
> >  can alter the message to hide its early part, which is problematic
> >  when a remote side gives such an error message that the local side
> >  will relay with a "remote: " prefix.
> >
> >  Will merge to 'next'.
> 
> Please be not too hasty with advancing this topic to master. I could imagine
> that there is some unwanted fallout on systems where the end-of-line marker
> CRLF is common. Though, I haven't tested the topic myself, yet, nor do I
> expect regressions in *my* typical workflow.

Thank you for being so attentive.

This topic branch would indeed have caused problems. Worse: it would have
caused problems that are not covered by our test suite, as Git for
Windows' own utilities do not generate CR/LF line endings. So this
regression would have bit our users. Nasty.

Something like this is necessary, at least:

-- snipsnap --
Subject: [PATCH] fixup! vreport: sanitize ASCII control chars

The original patch is incorrect, as it turns carriage returns into
question marks.

However, carriage returns should be left alone when preceding a line feed,
and simply turned into line feeds otherwise.

To make the end result more readable, the logic is inverted so that the
common case (no substitution) is handled first.

While at it, let's lose the unnecessary curly braces.

We also add a regression test that verifies that carriage returns are
handled properly. And as we expect CR/LF to be handled properly also on
platforms other than Windows, this test case is not guarded by the MINGW
prerequisite.

Spotted by Hannes Sixt.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t0000-basic.sh | 8 ++++++++
 usage.c          | 9 ++++++---
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index 60811a3a7c..d494cb7c05 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -1063,4 +1063,12 @@ test_expect_success 'very long name in the index handled sanely' '
 	test $len = 4098
 '
 
+# This test verifies that the error reporting functions handle CR correctly.
+# --abbrev-ref is simply used out of convenience, as it reports an error including
+# a user-specified argument.
+test_expect_success 'error messages treat CR/LF correctly' '
+	test_must_fail git rev-parse --abbrev-ref "$(printf "CR/LF\\r\\n")" 2>err &&
+	grep "^fatal: .*CR/LF$" err
+'
+
 test_done
diff --git a/usage.c b/usage.c
index 50a6ccee44..71bc7c0329 100644
--- a/usage.c
+++ b/usage.c
@@ -15,10 +15,13 @@ void vreportf(const char *prefix, const char *err, va_list params)
 	char *p;
 
 	vsnprintf(msg, sizeof(msg), err, params);
-	for (p = msg; *p; p++) {
-		if (iscntrl(*p) && *p != '\t' && *p != '\n')
+	for (p = msg; *p; p++)
+		if (!iscntrl(*p) || *p == '\t' || *p == '\n')
+			continue;
+		else if (*p != '\r')
 			*p = '?';
-	}
+		else if (p[1] != '\n')
+			*p = '\n';
 	fprintf(fh, "%s%s\n", prefix, msg);
 }
 
-- 
2.11.0.windows.3

Re: What's cooking in git.git (Jan 2017, #02; Sun, 15)

[Jeff King]

On Mon, Jan 16, 2017 at 01:52:39PM +0100, Johannes Schindelin wrote:

> > >  An error message with an ASCII control character like '\r' in it
> > >  can alter the message to hide its early part, which is problematic
> > >  when a remote side gives such an error message that the local side
> > >  will relay with a "remote: " prefix.
> > >
> > >  Will merge to 'next'.
> > 
> > Please be not too hasty with advancing this topic to master. I could imagine
> > that there is some unwanted fallout on systems where the end-of-line marker
> > CRLF is common. Though, I haven't tested the topic myself, yet, nor do I
> > expect regressions in *my* typical workflow.
> 
> Thank you for being so attentive.
> 
> This topic branch would indeed have caused problems. Worse: it would have
> caused problems that are not covered by our test suite, as Git for
> Windows' own utilities do not generate CR/LF line endings. So this
> regression would have bit our users. Nasty.

Hmm.  I am not sure to what degree CRLFs are actually a problem here.
Keep in mind these are error messages generated via error(), and so not
processing arbitrary data. I can imagine that CRs might come from:

  1. A parameter like a filename or revision name. If I ask for a
     rev-parse of "foo\r\n", then it's probably useful to mention the
     "\r" in the error message, rather than ignoring it (or converting
     it to a line-feed).

     And I think that would apply to any input parameter we show via
     error(), etc, if it is connected to a newline (ideally we would
     show newlines as "?", too, but we cannot tell the difference
     between ones from parameters, and ones that are part of the error
     message).

  2. The printf-fmt strings themselves. But these come from C code,
     which just uses "\n". My impression is that it is fprintf() which
     is responsible for converting that to "\r\n". And we are doing our
     sanitizing here between an snprintf(), and an fprintf() of the
     result. So it should see only the raw "\n" fields.

I am certainly open to loosening the sanitizing for CR to make things
work seamlessly on Windows. But I am having trouble imagining a case
that is actually negatively impacted.

> -- snipsnap --
> Subject: [PATCH] fixup! vreport: sanitize ASCII control chars

Given the subtlety here, I'd much rather have a patch on top.

> The original patch is incorrect, as it turns carriage returns into
> question marks.
> 
> However, carriage returns should be left alone when preceding a line feed,
> and simply turned into line feeds otherwise.

The question of whether to leave CRLFs alone is addressed above. But I
do not understand why you'd want a lone CR to be converted to a line
feed. Running:

  git rev-parse --abbrev-ref "$(printf "foo\r")"

with my patch gets you:

  fatal: ambiguous argument 'foo?': unknown revision or path not in the working tree.

But with yours:

  fatal: ambiguous argument 'foo
  ': unknown revision or path not in the working tree.

Obviously the "?" thing is a lossy transformation, but I do not see how
a newline is any less confusing (but see below for some thoughts).

> To make the end result more readable, the logic is inverted so that the
> common case (no substitution) is handled first.
> 
> While at it, let's lose the unnecessary curly braces.

Please don't. Obviously C treats the "if/else" as a single unit, but
IMHO it's less error-prone to include the braces any time there are
multiple visual lines. E.g., something like:

  while (foo)
	if (bar)
		one();
	else
		two();
	three();

is much easier to spot as wrong when you would require braces either
way (and not relevant here, but I'd say that even an inner block with a
comment deserves braces for the same reason).

> We also add a regression test that verifies that carriage returns are
> handled properly. And as we expect CR/LF to be handled properly also on
> platforms other than Windows, this test case is not guarded by the MINGW
> prerequisite.

I am not sure what "properly" means here. In your test:

> +# This test verifies that the error reporting functions handle CR correctly.
> +# --abbrev-ref is simply used out of convenience, as it reports an error including
> +# a user-specified argument.
> +test_expect_success 'error messages treat CR/LF correctly' '
> +	test_must_fail git rev-parse --abbrev-ref "$(printf "CR/LF\\r\\n")" 2>err &&
> +	grep "^fatal: .*CR/LF$" err
> +'

The "\n" is eaten by the shell, and git sees only "CR/LF\r". So we are
not testing the CRLF case in vreportf() at all.

We do end up with "CR/LF\n" in vreportf(), which is presumably converted
by fprintf() to "CR/LF\r\n" on Windows. And so perhaps that is why you
are doing the "convert \r to \n" thing above.

But I still think it's not doing the right thing. Git _didn't_ see CRLF,
it saw CR.

-Peff

Re: What's cooking in git.git (Jan 2017, #02; Sun, 15)

[Johannes Schindelin]

Hi Peff,

On Mon, 16 Jan 2017, Jeff King wrote:

> On Mon, Jan 16, 2017 at 01:52:39PM +0100, Johannes Schindelin wrote:
> 
> > > >  An error message with an ASCII control character like '\r' in it
> > > >  can alter the message to hide its early part, which is problematic
> > > >  when a remote side gives such an error message that the local side
> > > >  will relay with a "remote: " prefix.
> > > >
> > > >  Will merge to 'next'.
> > > 
> > > Please be not too hasty with advancing this topic to master. I could imagine
> > > that there is some unwanted fallout on systems where the end-of-line marker
> > > CRLF is common. Though, I haven't tested the topic myself, yet, nor do I
> > > expect regressions in *my* typical workflow.
> > 
> > Thank you for being so attentive.
> > 
> > This topic branch would indeed have caused problems. Worse: it would have
> > caused problems that are not covered by our test suite, as Git for
> > Windows' own utilities do not generate CR/LF line endings. So this
> > regression would have bit our users. Nasty.
> 
> Hmm.  I am not sure to what degree CRLFs are actually a problem here.
> Keep in mind these are error messages generated via error(), and so not
> processing arbitrary data. I can imagine that CRs might come from:

Please note the regression test I added. It uses rev-parse's --abbrev-ref
option which quotes the argument when erroring out. This argument then
gets munged.

So error() (or in this case, die()) *very much* processes arbitrary data.

I *know* that rev-parse --abbrev-ref is an artificial example, it is
highly unlikely that anybody will use

	git rev-parse --abbrev-ref "$(<call an external program here that
		generates CR/LF line endings>)"

However, there are plenty other cases in regular Git usage where arguments
are generated by external programs to which we have no business dictating a
specific line ending style.

If you absolutely insist, I will spend time to find a plausible example
and use that in the regression test. However, that would not really
improve anything, as the purpose of the regression test is simply to
demonstrate that a user-provided argument's CR/LF does not get munged
incorrectly. And the test I provided serves that purpose perfectly.

>   1. A parameter like a filename or revision name. If I ask for a
>      rev-parse of "foo\r\n", then it's probably useful to mention the
>      "\r" in the error message, rather than ignoring it (or converting
>      it to a line-feed).
> 
>      And I think that would apply to any input parameter we show via
>      error(), etc, if it is connected to a newline (ideally we would
>      show newlines as "?", too, but we cannot tell the difference
>      between ones from parameters, and ones that are part of the error
>      message).

I think it is doing users a really great disservice to munge up CR or LF
into question marks. I *guarantee* you that it confuses users. And not
because they are dumb, but because the code violates the Law of Least
Surprise.

> I am certainly open to loosening the sanitizing for CR to make things
> work seamlessly on Windows. But I am having trouble imagining a case
> that is actually negatively impacted.

Git accepts all kinds of arguments, not just file names. It is totally
legitimate, and you probably can show use cases of that yourself, to
generate those arguments by other programs.

These programs can generate CR/LF line endings.

While well-intentioned, your changes could make things even unreadable.
Certainly confusing.

> > -- snipsnap --
> > Subject: [PATCH] fixup! vreport: sanitize ASCII control chars
> 
> Given the subtlety here, I'd much rather have a patch on top.

Fine.

> > The original patch is incorrect, as it turns carriage returns into
> > question marks.
> > 
> > However, carriage returns should be left alone when preceding a line feed,
> > and simply turned into line feeds otherwise.
> 
> The question of whether to leave CRLFs alone is addressed above. But I
> do not understand why you'd want a lone CR to be converted to a line
> feed. Running:
> 
>   git rev-parse --abbrev-ref "$(printf "foo\r")"
> 
> with my patch gets you:
> 
>   fatal: ambiguous argument 'foo?': unknown revision or path not in the working tree.
> 
> But with yours:
> 
>   fatal: ambiguous argument 'foo
>   ': unknown revision or path not in the working tree.
> 
> Obviously the "?" thing is a lossy transformation,

s/lossy/confusing/

Probably not to you, because you came up with the transformation, but to
literally everybody else.

> but I do not see how a newline is any less confusing (but see below for
> some thoughts).

The more common bug report to be expected would show that multi-line
arguments are converted to ending in question marks. That is not the user
experience for which I am aiming.

> > To make the end result more readable, the logic is inverted so that the
> > common case (no substitution) is handled first.
> > 
> > While at it, let's lose the unnecessary curly braces.
> 
> Please don't. Obviously C treats the "if/else" as a single unit, but
> IMHO it's less error-prone to include the braces any time there are
> multiple visual lines. E.g., something like:
> 
>   while (foo)
> 	if (bar)
> 		one();
> 	else
> 		two();
> 	three();
> 
> is much easier to spot as wrong when you would require braces either
> way (and not relevant here, but I'd say that even an inner block with a
> comment deserves braces for the same reason).

There is no documentation about the preferred coding style.

For years, I saw patches, and provided patches myself, that *avoided*
curly braces when not necessary (in addition to aiming for shorter arms to
come before longer arms in conditionals).

Now all of a sudden Junio *and* you suggest unnecessary curly braces to be
added.

That is inconsistent at best, and confusing. Maybe you two gentle people
can make up your mind and document the final verdict, and for extra
brownie points automate the formatting so that patch reviews are not
dominated by coding style comments that would be better addressed by
machines than by humans.

> > We also add a regression test that verifies that carriage returns are
> > handled properly. And as we expect CR/LF to be handled properly also on
> > platforms other than Windows, this test case is not guarded by the MINGW
> > prerequisite.
> 
> I am not sure what "properly" means here. In your test:
> 
> > +# This test verifies that the error reporting functions handle CR correctly.
> > +# --abbrev-ref is simply used out of convenience, as it reports an error including
> > +# a user-specified argument.
> > +test_expect_success 'error messages treat CR/LF correctly' '
> > +	test_must_fail git rev-parse --abbrev-ref "$(printf "CR/LF\\r\\n")" 2>err &&
> > +	grep "^fatal: .*CR/LF$" err
> > +'
> 
> The "\n" is eaten by the shell, and git sees only "CR/LF\r". So we are
> not testing the CRLF case in vreportf() at all.

True. Should be easy to fix, starting from my patch.

> We do end up with "CR/LF\n" in vreportf(), which is presumably converted
> by fprintf() to "CR/LF\r\n" on Windows. And so perhaps that is why you
> are doing the "convert \r to \n" thing above.
> 
> But I still think it's not doing the right thing. Git _didn't_ see CRLF,
> it saw CR.

You know, I don't care. As long as carriage returns are either left alone
(which conflicts specifically with your stated goal) or at least are
handled gracefully when coming before line feeds.

Converting stray carriage returns to question marks is confusing, and of
course I would take the brunt of the bug reports, so I do not look
favorably on that change.

My patch was just a starter, to help with fixing the patch series before
it hits `next`.

Ciao,
Johannes

Re: What's cooking in git.git (Jan 2017, #02; Sun, 15)

[Johannes Sixt]

Am 16.01.2017 um 18:06 schrieb Johannes Schindelin:
> On Mon, 16 Jan 2017, Jeff King wrote:
>> Hmm.  I am not sure to what degree CRLFs are actually a problem here.
>> Keep in mind these are error messages generated via error(), and so not
>> processing arbitrary data. I can imagine that CRs might come from:
>
> Please note the regression test I added. It uses rev-parse's --abbrev-ref
> option which quotes the argument when erroring out. This argument then
> gets munged.
>
> So error() (or in this case, die()) *very much* processes arbitrary data.
>
> I *know* that rev-parse --abbrev-ref is an artificial example, it is
> highly unlikely that anybody will use
>
> 	git rev-parse --abbrev-ref "$(<call an external program here that
> 		generates CR/LF line endings>)"
>
> However, there are plenty other cases in regular Git usage where arguments
> are generated by external programs to which we have no business dictating a
> specific line ending style.

However, Jeff's patch is intended to catch exactly these cases (not for 
the cases where this happens accidentally, but when they happen with 
malicious intent).

We are talking about user-provided data that is reproduced by die() or 
error(). I daresay that we do not have a single case where it is 
intended that this data is intentionally multi-lined, like a commit 
message. It can only be an accident or malicious when it spans across lines.

I know we allow CR and LF in file names, but in all cases where such a 
name appears in an error message, it is *not important* that the data is 
reproduced exactly. On the contrary, it is usually more helpful to know 
that something strange is going on. The question marks are a strong 
indication to the user for this.

> If you absolutely insist, I will spend time to find a plausible example
> and use that in the regression test.

I don't want to see you on an endeavor with dubious results. I'd prefer 
to wait until the first case of "incorrectly munged data" is reported 
because, as I said, I have a gut feeling that there is none.

>> I am certainly open to loosening the sanitizing for CR to make things
>> work seamlessly on Windows. But I am having trouble imagining a case
>> that is actually negatively impacted.

I came to the same conclusion. I regret having sent out a warning 
message in, well, such a haste(*), without thinking the case through 
first. IMHO, Jeff's patch should be fine as is.

(*) literally; I had to catch a train.

-- Hannes

Re: What's cooking in git.git (Jan 2017, #02; Sun, 15)

[Jeff King]

On Mon, Jan 16, 2017 at 09:33:07PM +0100, Johannes Sixt wrote:

> However, Jeff's patch is intended to catch exactly these cases (not for the
> cases where this happens accidentally, but when they happen with malicious
> intent).
> 
> We are talking about user-provided data that is reproduced by die() or
> error(). I daresay that we do not have a single case where it is intended
> that this data is intentionally multi-lined, like a commit message. It can
> only be an accident or malicious when it spans across lines.
> 
> I know we allow CR and LF in file names, but in all cases where such a name
> appears in an error message, it is *not important* that the data is
> reproduced exactly. On the contrary, it is usually more helpful to know that
> something strange is going on. The question marks are a strong indication to
> the user for this.

Yes, exactly. Thanks for explaining this better than I obviously was
doing. :)

> > If you absolutely insist, I will spend time to find a plausible example
> > and use that in the regression test.
> 
> I don't want to see you on an endeavor with dubious results. I'd prefer to
> wait until the first case of "incorrectly munged data" is reported because,
> as I said, I have a gut feeling that there is none.

Agreed.

-Peff

Re: What's cooking in git.git (Jan 2017, #02; Sun, 15)

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Jan 16, 2017 at 09:33:07PM +0100, Johannes Sixt wrote:
>
>> However, Jeff's patch is intended to catch exactly these cases (not for the
>> cases where this happens accidentally, but when they happen with malicious
>> intent).
>> 
>> We are talking about user-provided data that is reproduced by die() or
>> error(). I daresay that we do not have a single case where it is intended
>> that this data is intentionally multi-lined, like a commit message. It can
>> only be an accident or malicious when it spans across lines.
>> 
>> I know we allow CR and LF in file names, but in all cases where such a name
>> appears in an error message, it is *not important* that the data is
>> reproduced exactly. On the contrary, it is usually more helpful to know that
>> something strange is going on. The question marks are a strong indication to
>> the user for this.
>
> Yes, exactly. Thanks for explaining this better than I obviously was
> doing. :)

Yup.  In the "funny filename" case, the updated code indeed is doing
the right thing.  So one remaining possible issue is if we ourselves
deliberately feed a raw CR (or any non-filtered control code) to
error() and friends because their native effect (like returning the
carriage to overwrite what we have already said with the remainder
of the message by using CR) to functions that go through vreportf()
interface.  I personally do not think there is any---the progress
meter code does use CR for that but they don't do vreportf().

I do not think we write "message\r\n" even for Windows; we rely on
vfprintf() and other stdio layer to turn "message\n" into
"message\r\n" before it hits write(2) or its equivalent?  So I
understand that this should affect LF and CRLF systems the same way
(meaning, no negative effect).

So... can we move this forward?

Re: What's cooking in git.git (Jan 2017, #02; Sun, 15)

[Johannes Sixt]

Am 17.01.2017 um 20:17 schrieb Junio C Hamano:
> So... can we move this forward?

I have no objects anymore.

-- Hannes

Re: What's cooking in git.git (Jan 2017, #02; Sun, 15)

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> Am 17.01.2017 um 20:17 schrieb Junio C Hamano:
>> So... can we move this forward?
>
> I have no objects anymore.
>
> -- Hannes

Alright, thanks.  

Dscho what's your assessment?  My "I do not think" before the quoted
"can we move" above was more about giving a statement for people to
argue against, by saying "no your understanding is wrong", in the
hope that it would highlight why we need more work in this area if
needed and in what way.

Thanks.

Re: What's cooking in git.git (Jan 2017, #02; Sun, 15)

[Johannes Schindelin]

Hi Junio,

On Wed, 18 Jan 2017, Junio C Hamano wrote:

> Johannes Sixt <j6t@kdbg.org> writes:
> 
> > Am 17.01.2017 um 20:17 schrieb Junio C Hamano:
> >> So... can we move this forward?
> >
> > I have no objects anymore.
> 
> Alright, thanks.  
> 
> Dscho what's your assessment?

I still think it will be a problem. I'll address that problem when I get
bug reports, to unblock you.

Ciao,
Johannes

Re: What's cooking in git.git (Jan 2017, #02; Sun, 15)

[Jeff King]

On Mon, Jan 16, 2017 at 06:06:35PM +0100, Johannes Schindelin wrote:

> >      And I think that would apply to any input parameter we show via
> >      error(), etc, if it is connected to a newline (ideally we would
> >      show newlines as "?", too, but we cannot tell the difference
> >      between ones from parameters, and ones that are part of the error
> >      message).
> 
> I think it is doing users a really great disservice to munge up CR or LF
> into question marks. I *guarantee* you that it confuses users. And not
> because they are dumb, but because the code violates the Law of Least
> Surprise.

I'm not sure if you realize that with stock git, the example from your
test looks like this (at least in my terminal):

  $ git.v2.11.0 rev-parse --abbrev-ref "$(printf 'CR/LF\r\n')" >/dev/null
  ': unknown revision or path not in the working tree.
  Use '--' to separate paths from revisions, like this:
  'git <command> [<revision>...] -- [<file>...]'

The "\r" causes us to overwrite the rest of the message, including the
actual filename. With my patch it's:

  $ git rev-parse --abbrev-ref "$(printf 'CR/LF\r\n')" >/dev/null
  fatal: ambiguous argument 'CR/LF?': unknown revision or path not in the working tree.
  Use '--' to separate paths from revisions, like this:
  'git <command> [<revision>...] -- [<file>...]'

I am certainly sympathetic to the idea that the "?" is ugly and
potentially confusing. But I think it's at least a step forward for this
particular example.

I'll snip liberally from the rest of your response, because I think what
JSixt wrote covers it.

> > > While at it, let's lose the unnecessary curly braces.
> > 
> > Please don't. Obviously C treats the "if/else" as a single unit, but
> > IMHO it's less error-prone to include the braces any time there are
> > multiple visual lines. E.g., something like:
> > 
> >   while (foo)
> > 	if (bar)
> > 		one();
> > 	else
> > 		two();
> > 	three();
> > 
> > is much easier to spot as wrong when you would require braces either
> > way (and not relevant here, but I'd say that even an inner block with a
> > comment deserves braces for the same reason).
> 
> There is no documentation about the preferred coding style.

Documentation/CodingGuidelines says:

 - We avoid using braces unnecessarily.  I.e.

        if (bla) {
                x = 1;
        }

   is frowned upon.  A gray area is when the statement extends
   over a few lines, and/or you have a lengthy comment atop of
   it.  Also, like in the Linux kernel, if there is a long list
   of "else if" statements, it can make sense to add braces to
   single line blocks.

I think this is pretty clearly the "gray area" mentioned there. Which
yes, does not say "definitely do it this way", but I hope makes it clear
that you're supposed to use judgement about readability.

-Peff

[PATCH] CodingGuidelines: clarify multi-line brace style

[Jeff King]

On Mon, Jan 16, 2017 at 05:00:14PM -0500, Jeff King wrote:

> > > Please don't. Obviously C treats the "if/else" as a single unit, but
> > > IMHO it's less error-prone to include the braces any time there are
> > > multiple visual lines. E.g., something like:
> > > 
> > >   while (foo)
> > > 	if (bar)
> > > 		one();
> > > 	else
> > > 		two();
> > > 	three();
> > > 
> > > is much easier to spot as wrong when you would require braces either
> > > way (and not relevant here, but I'd say that even an inner block with a
> > > comment deserves braces for the same reason).
> > 
> > There is no documentation about the preferred coding style.
> 
> Documentation/CodingGuidelines says:
> 
>  - We avoid using braces unnecessarily.  I.e.
> 
>         if (bla) {
>                 x = 1;
>         }
> 
>    is frowned upon.  A gray area is when the statement extends
>    over a few lines, and/or you have a lengthy comment atop of
>    it.  Also, like in the Linux kernel, if there is a long list
>    of "else if" statements, it can make sense to add braces to
>    single line blocks.
> 
> I think this is pretty clearly the "gray area" mentioned there. Which
> yes, does not say "definitely do it this way", but I hope makes it clear
> that you're supposed to use judgement about readability.

So here's a patch.

I know we've usually tried to keep this file to guidelines and not
rules, but clearly it has not been clear-cut enough in this instance.

-- >8 --
Subject: [PATCH] CodingGuidelines: clarify multi-line brace style

There are some "gray areas" around when to omit braces from
a conditional or loop body. Since that seems to have
resulted in some arguments, let's be a little more clear
about our preferred style.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/CodingGuidelines | 37 ++++++++++++++++++++++++++++++++-----
 1 file changed, 32 insertions(+), 5 deletions(-)

diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index 4cd95da6b..0e336e99d 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -206,11 +206,38 @@ For C programs:
 		x = 1;
 	}
 
-   is frowned upon.  A gray area is when the statement extends
-   over a few lines, and/or you have a lengthy comment atop of
-   it.  Also, like in the Linux kernel, if there is a long list
-   of "else if" statements, it can make sense to add braces to
-   single line blocks.
+   is frowned upon. But there are a few exceptions:
+
+	- When the statement extends over a few lines (e.g., a while loop
+	  with an embedded conditional, or a comment). E.g.:
+
+		while (foo) {
+			if (x)
+				one();
+			else
+				two();
+		}
+
+		if (foo) {
+			/*
+			 * This one requires some explanation,
+			 * so we're better off with braces to make
+			 * it obvious that the indentation is correct.
+			 */
+			doit();
+		}
+
+	- When there are multiple arms to a conditional, it can make
+	  sense to add braces to single line blocks for consistency.
+	  E.g.:
+
+		if (foo) {
+			doit();
+		} else {
+			one();
+			two();
+			three();
+		}
 
  - We try to avoid assignments in the condition of an "if" statement.
 
-- 
2.11.0.642.gd6f8cda6c

Re: [PATCH] CodingGuidelines: clarify multi-line brace style

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> I think this is pretty clearly the "gray area" mentioned there. Which
>> yes, does not say "definitely do it this way", but I hope makes it clear
>> that you're supposed to use judgement about readability.
>
> So here's a patch.
>
> I know we've usually tried to keep this file to guidelines and not
> rules, but clearly it has not been clear-cut enough in this instance.

I have two "Huh?" with this patch.

 1. Two exceptions are not treated the same way.  The first one is
    "... extends over a few lines, it is excempt from the rule,
    period".  The second one, is ambivalent by saying "it can make
    sense", implying that "it may not make sense", so I am not sure
    if this is clarifying that much.

    If we want to clarify, perhaps drop "it can make sense to" and
    say

	When there are multiple arms to a conditional, and some of
	them require braces, enclose even a single line block in
	braces for consistency.

    perhaps?

 2. I actually think it is OK to leave some things "gray", but the
    confusion comes when people do not know what to do to things
    that are "gray", and they need a rule for that to be spelled
    out.

	When the project says it does not have strong preference
	among multiple choices, you are welcome to write your new
	code using any of them, as long as you are consistent with
	surrounding code.  Do not change style of existing code only
	to flip among these styles, though.

    That obviously is not limited to the rule/guideline for braces.

> -- >8 --
> Subject: [PATCH] CodingGuidelines: clarify multi-line brace style
>
> There are some "gray areas" around when to omit braces from
> a conditional or loop body. Since that seems to have
> resulted in some arguments, let's be a little more clear
> about our preferred style.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  Documentation/CodingGuidelines | 37 ++++++++++++++++++++++++++++++++-----
>  1 file changed, 32 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
> index 4cd95da6b..0e336e99d 100644
> --- a/Documentation/CodingGuidelines
> +++ b/Documentation/CodingGuidelines
> @@ -206,11 +206,38 @@ For C programs:
>  		x = 1;
>  	}
>  
> -   is frowned upon.  A gray area is when the statement extends
> -   over a few lines, and/or you have a lengthy comment atop of
> -   it.  Also, like in the Linux kernel, if there is a long list
> -   of "else if" statements, it can make sense to add braces to
> -   single line blocks.
> +   is frowned upon. But there are a few exceptions:
> +
> +	- When the statement extends over a few lines (e.g., a while loop
> +	  with an embedded conditional, or a comment). E.g.:
> +
> +		while (foo) {
> +			if (x)
> +				one();
> +			else
> +				two();
> +		}
> +
> +		if (foo) {
> +			/*
> +			 * This one requires some explanation,
> +			 * so we're better off with braces to make
> +			 * it obvious that the indentation is correct.
> +			 */
> +			doit();
> +		}
> +
> +	- When there are multiple arms to a conditional, it can make
> +	  sense to add braces to single line blocks for consistency.
> +	  E.g.:
> +
> +		if (foo) {
> +			doit();
> +		} else {
> +			one();
> +			two();
> +			three();
> +		}
>  
>   - We try to avoid assignments in the condition of an "if" statement.

Re: [PATCH] CodingGuidelines: clarify multi-line brace style

[Jeff King]

On Tue, Jan 17, 2017 at 11:39:31AM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> >> I think this is pretty clearly the "gray area" mentioned there. Which
> >> yes, does not say "definitely do it this way", but I hope makes it clear
> >> that you're supposed to use judgement about readability.
> >
> > So here's a patch.
> >
> > I know we've usually tried to keep this file to guidelines and not
> > rules, but clearly it has not been clear-cut enough in this instance.
> 
> I have two "Huh?" with this patch.
> 
>  1. Two exceptions are not treated the same way.  The first one is
>     "... extends over a few lines, it is excempt from the rule,
>     period".  The second one, is ambivalent by saying "it can make
>     sense", implying that "it may not make sense", so I am not sure
>     if this is clarifying that much.
> 
>     If we want to clarify, perhaps drop "it can make sense to" and
>     say
> 
> 	When there are multiple arms to a conditional, and some of
> 	them require braces, enclose even a single line block in
> 	braces for consistency.
> 
>     perhaps?

Yeah. I obviously was adapting the original text, and I think I left too
much of the wishy-washy-ness in. As the point of the patch is to avoid
that, let's take your suggestion. A re-rolled patch is below.

Now the patch is at least self-consistent. The bigger question remains
of: do we want to dictate these rules, or did we prefer the vague
version?

I _thought_ the vague rules were doing fine, but this whole discussion
has made me think otherwise. And I'd just as soon not ever have to
repeat it. ;-/

OTOH, I really do not want to review a bunch of patches that do nothing
but change brace style (and I am sure there is a mix of styles already
in the code base).

>  2. I actually think it is OK to leave some things "gray", but the
>     confusion comes when people do not know what to do to things
>     that are "gray", and they need a rule for that to be spelled
>     out.
> 
> 	When the project says it does not have strong preference
> 	among multiple choices, you are welcome to write your new
> 	code using any of them, as long as you are consistent with
> 	surrounding code.  Do not change style of existing code only
> 	to flip among these styles, though.
> 
>     That obviously is not limited to the rule/guideline for braces.

The existing document says:

    Make your code readable and sensible, and don't try to be clever.
    
    As for more concrete guidelines, just imitate the existing code
    (this is a good guideline, no matter which project you are
    contributing to). It is always preferable to match the _local_
    convention. New code added to Git suite is expected to match
    the overall style of existing code. Modifications to existing
    code is expected to match the style the surrounding code already
    uses (even if it doesn't match the overall style of existing code).
    
    But if you must have a list of rules, here they are.

I guess that is the place to expound on how to interpret the rules. I
dunno. Some of the individual rules that go into "gray areas" already
spell out the "surrounding code" rule.

-Peff

-- >8 --
Subject: [PATCH] CodingGuidelines: clarify multi-line brace style

There are some "gray areas" around when to omit braces from
a conditional or loop body. Since that seems to have
resulted in some arguments, let's be a little more clear
about our preferred style.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/CodingGuidelines | 37 ++++++++++++++++++++++++++++++++-----
 1 file changed, 32 insertions(+), 5 deletions(-)

diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index 4cd95da6b..a4191aa38 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -206,11 +206,38 @@ For C programs:
 		x = 1;
 	}
 
-   is frowned upon.  A gray area is when the statement extends
-   over a few lines, and/or you have a lengthy comment atop of
-   it.  Also, like in the Linux kernel, if there is a long list
-   of "else if" statements, it can make sense to add braces to
-   single line blocks.
+   is frowned upon. But there are a few exceptions:
+
+	- When the statement extends over a few lines (e.g., a while loop
+	  with an embedded conditional, or a comment). E.g.:
+
+		while (foo) {
+			if (x)
+				one();
+			else
+				two();
+		}
+
+		if (foo) {
+			/*
+			 * This one requires some explanation,
+			 * so we're better off with braces to make
+			 * it obvious that the indentation is correct.
+			 */
+			doit();
+		}
+
+	- When there are multiple arms to a conditional and some of them
+	  require braces, enclose even a single line block in braces for
+	  consistency. E.g.:
+
+		if (foo) {
+			doit();
+		} else {
+			one();
+			two();
+			three();
+		}
 
  - We try to avoid assignments in the condition of an "if" statement.
 
-- 
2.11.0.651.g41f4a5c4e

Re: [PATCH] CodingGuidelines: clarify multi-line brace style

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Yeah. I obviously was adapting the original text, and I think I left too
> much of the wishy-washy-ness in. As the point of the patch is to avoid
> that, let's take your suggestion. A re-rolled patch is below.
>
> Now the patch is at least self-consistent. The bigger question remains
> of: do we want to dictate these rules, or did we prefer the vague
> version?

I would say no to make all of them "rules to be dictated".  It is OK
to leave some things "gray".  

But obviously others can vote differently ;-)

> I _thought_ the vague rules were doing fine, but this whole discussion
> has made me think otherwise. And I'd just as soon not ever have to
> repeat it. ;-/
>
> OTOH, I really do not want to review a bunch of patches that do nothing
> but change brace style (and I am sure there is a mix of styles already
> in the code base).

Yes, exactly, that is why the last sentence is in there below.

>> 	When the project says it does not have strong preference
>> 	among multiple choices, you are welcome to write your new
>> 	code using any of them, as long as you are consistent with
>> 	surrounding code.  Do not change style of existing code only
>> 	to flip among these styles, though.

> The existing document says:
>
>     Make your code readable and sensible, and don't try to be clever.
>     
>     As for more concrete guidelines, just imitate the existing code
>     (this is a good guideline, no matter which project you are
>     contributing to). It is always preferable to match the _local_
>     convention. New code added to Git suite is expected to match
>     the overall style of existing code. Modifications to existing
>     code is expected to match the style the surrounding code already
>     uses (even if it doesn't match the overall style of existing code).
>     
>     But if you must have a list of rules, here they are.
>
> I guess that is the place to expound on how to interpret the rules. I
> dunno. Some of the individual rules that go into "gray areas" already
> spell out the "surrounding code" rule.


We are not saying one important thing and that was what invited
useless arguing this time: For "gray" things, the choice is yours in
your new code, but do not churn existing code for "gray" guidelines.

If we made _that_ clear as a rule we dictate, hopefully we'd have
less chance to see "a bunch of patches that do nothing but change
brace style", I would think.

> -- >8 --
> Subject: [PATCH] CodingGuidelines: clarify multi-line brace style
>
> There are some "gray areas" around when to omit braces from
> a conditional or loop body. Since that seems to have
> resulted in some arguments, let's be a little more clear
> about our preferred style.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  Documentation/CodingGuidelines | 37 ++++++++++++++++++++++++++++++++-----
>  1 file changed, 32 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
> index 4cd95da6b..a4191aa38 100644
> --- a/Documentation/CodingGuidelines
> +++ b/Documentation/CodingGuidelines
> @@ -206,11 +206,38 @@ For C programs:
>  		x = 1;
>  	}
>  
> -   is frowned upon.  A gray area is when the statement extends
> -   over a few lines, and/or you have a lengthy comment atop of
> -   it.  Also, like in the Linux kernel, if there is a long list
> -   of "else if" statements, it can make sense to add braces to
> -   single line blocks.
> +   is frowned upon. But there are a few exceptions:
> +
> +	- When the statement extends over a few lines (e.g., a while loop
> +	  with an embedded conditional, or a comment). E.g.:
> +
> +		while (foo) {
> +			if (x)
> +				one();
> +			else
> +				two();
> +		}
> +
> +		if (foo) {
> +			/*
> +			 * This one requires some explanation,
> +			 * so we're better off with braces to make
> +			 * it obvious that the indentation is correct.
> +			 */
> +			doit();
> +		}
> +
> +	- When there are multiple arms to a conditional and some of them
> +	  require braces, enclose even a single line block in braces for
> +	  consistency. E.g.:
> +
> +		if (foo) {
> +			doit();
> +		} else {
> +			one();
> +			two();
> +			three();
> +		}
>  
>   - We try to avoid assignments in the condition of an "if" statement.

Re: What's cooking in git.git (Jan 2017, #02; Sun, 15)

[Jacob Keller]

On Mon, Jan 16, 2017 at 2:00 PM, Jeff King <peff@peff.net> wrote:
> On Mon, Jan 16, 2017 at 06:06:35PM +0100, Johannes Schindelin wrote:
>
>> >      And I think that would apply to any input parameter we show via
>> >      error(), etc, if it is connected to a newline (ideally we would
>> >      show newlines as "?", too, but we cannot tell the difference
>> >      between ones from parameters, and ones that are part of the error
>> >      message).
>>
>> I think it is doing users a really great disservice to munge up CR or LF
>> into question marks. I *guarantee* you that it confuses users. And not
>> because they are dumb, but because the code violates the Law of Least
>> Surprise.
>
> I'm not sure if you realize that with stock git, the example from your
> test looks like this (at least in my terminal):
>
>   $ git.v2.11.0 rev-parse --abbrev-ref "$(printf 'CR/LF\r\n')" >/dev/null
>   ': unknown revision or path not in the working tree.
>   Use '--' to separate paths from revisions, like this:
>   'git <command> [<revision>...] -- [<file>...]'
>
> The "\r" causes us to overwrite the rest of the message, including the
> actual filename. With my patch it's:
>
>   $ git rev-parse --abbrev-ref "$(printf 'CR/LF\r\n')" >/dev/null
>   fatal: ambiguous argument 'CR/LF?': unknown revision or path not in the working tree.
>   Use '--' to separate paths from revisions, like this:
>   'git <command> [<revision>...] -- [<file>...]'
>
> I am certainly sympathetic to the idea that the "?" is ugly and
> potentially confusing. But I think it's at least a step forward for this
> particular example.
>

Would it be possible to convert the CR to a literal \r printing? Since
it's pretty common to show these characters as slash-escaped? (Or is
that too much of a Unix world thing?) I know I'd find \r less
confusing than '?'

Thanks,
Jake

Re: What's cooking in git.git (Jan 2017, #02; Sun, 15)

[Jeff King]

On Mon, Jan 16, 2017 at 05:33:44PM -0800, Jacob Keller wrote:

> > I am certainly sympathetic to the idea that the "?" is ugly and
> > potentially confusing. But I think it's at least a step forward for this
> > particular example.
> 
> Would it be possible to convert the CR to a literal \r printing? Since
> it's pretty common to show these characters as slash-escaped? (Or is
> that too much of a Unix world thing?) I know I'd find \r less
> confusing than '?'

I discussed this in the original commit message.

Yes, it's possible, but it's a little tricky. We want to fprintf() the
whole thing as a unit (to increase the chances of it being done in an
atomic write()). We don't want to use a dynamic buffer, since we might
be called from a signal handler, or when malloc has failed, etc.

So I tried to stick to something that could reliably done in place. We
could probably get by with 2 static buffers and copy from one to the
other (if stack space is an issue, the current one is 4K, which is
probably excessively large anyway).

Mostly I just assumed that it was highly unlikely anybody would see this
escaping in the first place. So I tried to go with the simplest
solution.

-Peff

Re: What's cooking in git.git (Jan 2017, #02; Sun, 15)

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Documentation/CodingGuidelines says:
>
>  - We avoid using braces unnecessarily.  I.e.
>
>         if (bla) {
>                 x = 1;
>         }
>
>    is frowned upon.  A gray area is when the statement extends
>    over a few lines, and/or you have a lengthy comment atop of
>    it.  Also, like in the Linux kernel, if there is a long list
>    of "else if" statements, it can make sense to add braces to
>    single line blocks.
>
> I think this is pretty clearly the "gray area" mentioned there. Which
> yes, does not say "definitely do it this way", but I hope makes it clear
> that you're supposed to use judgement about readability.

I always took "gray area" to mean "we do not have strong preference
either way, i.e.

 * It is OK for you to write your new code in either style (the
   usual "match existing style in surrounding code" applies,
   obviously);

 * It is not OK for you to churn the codebase with a patch that only
   changes existing code to flip between the two styles.

Re: What's cooking in git.git (Jan 2017, #02; Sun, 15)

[Jeff King]

On Tue, Jan 17, 2017 at 11:20:58AM -0800, Junio C Hamano wrote:

> > Documentation/CodingGuidelines says:
> >
> >  - We avoid using braces unnecessarily.  I.e.
> >
> >         if (bla) {
> >                 x = 1;
> >         }
> >
> >    is frowned upon.  A gray area is when the statement extends
> >    over a few lines, and/or you have a lengthy comment atop of
> >    it.  Also, like in the Linux kernel, if there is a long list
> >    of "else if" statements, it can make sense to add braces to
> >    single line blocks.
> >
> > I think this is pretty clearly the "gray area" mentioned there. Which
> > yes, does not say "definitely do it this way", but I hope makes it clear
> > that you're supposed to use judgement about readability.
> 
> I always took "gray area" to mean "we do not have strong preference
> either way, i.e.
> 
>  * It is OK for you to write your new code in either style (the
>    usual "match existing style in surrounding code" applies,
>    obviously);
> 
>  * It is not OK for you to churn the codebase with a patch that only
>    changes existing code to flip between the two styles.

That was my general impression, too. But I seem to recall it was you in
a nearby thread saying that:

  if (foo)
	bar();
  else {
        one();
	two();
  }

was wrong. Maybe I misunderstood.

-Peff

Re: What's cooking in git.git (Jan 2017, #02; Sun, 15)

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> That was my general impression, too. But I seem to recall it was you in
> a nearby thread saying that:
>
>   if (foo)
> 	bar();
>   else {
>         one();
> 	two();
>   }
>
> was wrong. Maybe I misunderstood.

If it were a new code written like the above, that would have been
fine.  If a new code written with both sides inside {}, that would
have been fine, too.

IIRC, it was that the original had {} on both, and a patch tried to
turn that into the above, triggering "why are we churning between
two acceptable forms?"

Re: What's cooking in git.git (Jan 2017, #02; Sun, 15)

[Jeff King]

On Tue, Jan 17, 2017 at 12:32:26PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > That was my general impression, too. But I seem to recall it was you in
> > a nearby thread saying that:
> >
> >   if (foo)
> > 	bar();
> >   else {
> >         one();
> > 	two();
> >   }
> >
> > was wrong. Maybe I misunderstood.
> 
> If it were a new code written like the above, that would have been
> fine.  If a new code written with both sides inside {}, that would
> have been fine, too.
> 
> IIRC, it was that the original had {} on both, and a patch tried to
> turn that into the above, triggering "why are we churning between
> two acceptable forms?"

Ah, OK. I didn't follow that discussion closely enough to realize that.

-Peff

Re: What's cooking in git.git (Jan 2017, #02; Sun, 15)

[Johannes Schindelin]

Hi Junio,

On Sun, 15 Jan 2017, Junio C Hamano wrote:

> * js/prepare-sequencer-more (2017-01-09) 38 commits

I think that it adds confusion rather than value to specifically use a
different branch name than I indicated in my submission, unless there is a
really good reason to do so (which I fail to see here).

>  - sequencer (rebase -i): write out the final message
>  - sequencer (rebase -i): write the progress into files
>  - sequencer (rebase -i): show the progress
>  - sequencer (rebase -i): suggest --edit-todo upon unknown command
>  - sequencer (rebase -i): show only failed cherry-picks' output
>  - sequencer (rebase -i): show only failed `git commit`'s output
>  - sequencer: use run_command() directly
>  - sequencer: make reading author-script more elegant
>  - sequencer (rebase -i): differentiate between comments and 'noop'
>  - sequencer (rebase -i): implement the 'drop' command
>  - sequencer (rebase -i): allow rescheduling commands
>  - sequencer (rebase -i): respect strategy/strategy_opts settings
>  - sequencer (rebase -i): respect the rebase.autostash setting
>  - sequencer (rebase -i): run the post-rewrite hook, if needed
>  - sequencer (rebase -i): record interrupted commits in rewritten, too
>  - sequencer (rebase -i): copy commit notes at end
>  - sequencer (rebase -i): set the reflog message consistently
>  - sequencer (rebase -i): refactor setting the reflog message
>  - sequencer (rebase -i): allow fast-forwarding for edit/reword
>  - sequencer (rebase -i): implement the 'reword' command
>  - sequencer (rebase -i): leave a patch upon error
>  - sequencer (rebase -i): update refs after a successful rebase
>  - sequencer (rebase -i): the todo can be empty when continuing
>  - sequencer (rebase -i): skip some revert/cherry-pick specific code path
>  - sequencer (rebase -i): remove CHERRY_PICK_HEAD when no longer needed
>  - sequencer (rebase -i): allow continuing with staged changes
>  - sequencer (rebase -i): write an author-script file
>  - sequencer (rebase -i): implement the short commands
>  - sequencer (rebase -i): add support for the 'fixup' and 'squash' commands
>  - sequencer (rebase -i): write the 'done' file
>  - sequencer (rebase -i): learn about the 'verbose' mode
>  - sequencer (rebase -i): implement the 'exec' command
>  - sequencer (rebase -i): implement the 'edit' command
>  - sequencer (rebase -i): implement the 'noop' command
>  - sequencer: support a new action: 'interactive rebase'
>  - sequencer: use a helper to find the commit message
>  - sequencer: move "else" keyword onto the same line as preceding brace
>  - sequencer: avoid unnecessary curly braces
> 
>  The sequencer has further been extended in preparation to act as a
>  back-end for "rebase -i".
> 
>  Waiting for review comments to be addressed.

The only outstanding review comments I know about are your objection to
the name of the read_env_script() function (which I shot down as bogus),
and the rather real bug fix I sent out as a fixup! which you may want to
squash in (in the alternative, I can mailbomb v4 of the entire sequencer-i
patch series, that is your choice).

Ciao,
Johannes

Re: What's cooking in git.git (Jan 2017, #02; Sun, 15)

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Hi Junio,
>
> On Sun, 15 Jan 2017, Junio C Hamano wrote:
>
>> * js/prepare-sequencer-more (2017-01-09) 38 commits
>
> I think that it adds confusion rather than value to specifically use a
> different branch name than I indicated in my submission, unless there is a
> really good reason to do so (which I fail to see here).

I've been meaning to rename it to match yours, for the exact reason.
The only reason was I needed my time spent to deal with other
topics, and reusing the same topic name as I used very first time
was less work.  I'll find time to update it (if you are curious, it
is not just "git branch -m").

Thanks.

> The only outstanding review comments I know about are your objection to
> the name of the read_env_script() function (which I shot down as bogus),

Not the "name", but the implementation not sharing the same code
with "am" codepath even though they originate from the same shell
function and serve the same purpose.

> and the rather real bug fix I sent out as a fixup! which you may want to
> squash in (in the alternative, I can mailbomb v4 of the entire sequencer-i
> patch series, that is your choice).

I'd rather see the "make elengant" thing redone in a more sensible
way, but I feel that it is waste of my time to help you see the
distinction between maintainable code and code that happens to work
with today's requirement, so I give up, hoping that other people
will later refactor the code that should be shared after the series
lands.  I'll squash the fixup! thing in, and as I already said a few
days ago, I do not think we'd want v4 (rather we'd want to go
incremental).

Re: What's cooking in git.git (Jan 2017, #02; Sun, 15)

[Johannes Schindelin]

Hi Junio,

On Tue, 17 Jan 2017, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > On Sun, 15 Jan 2017, Junio C Hamano wrote:
> >
> >> * js/prepare-sequencer-more (2017-01-09) 38 commits
> 
> > The only outstanding review comments I know about are your objection
> > to the name of the read_env_script() function (which I shot down as
> > bogus),
> 
> Not the "name", but the implementation not sharing the same code
> with "am" codepath even though they originate from the same shell
> function and serve the same purpose.

They do not originate from the same shell function at all. The scripted
git-am used a function called get_author_ident_from_commit to read the
author-script file. It also used "eval $(cat .../author-script)" to
evaluate the script later on.

And while git-rebase--interactive.sh's ". .../author-script" is very
similar to that latter invocation, I grant you that, the claim that those
codepaths originate from the same shell function is false.

Worse.

git-am.sh not only used a different method to read out the author-script,
and not only left no way for the user to modify that author-script, there
are many more differences in the exact code paths when comparing git-am.sh
to git-rebase--interactive.sh.

git-am.sh goes out of its way to not only apply the patch manually but
also to use the low-level `write-tree` and `commit-tree` commands. The
builtin am reproduces this as faithfully as possible (the author-script is
not actually evaluated, of course, but the builtin am knows that it has
to validate the author-script's contents for the first code path reading
the author-script anyway, so it reuses that function for the second code
path, knowing fully well the exact format of that file because it knows it
wrote it without any chance of the user messing it up).

The builtin am also avoids spawning the low-level commands, opting instead
to call the functions in libgit.a directly. This is all done properly, and
as a consequence, the environment variables GIT_AUTHOR_* never become,
ahem, environment variables in the builtin am, but an "author" parameter
(which is a const char * pointing to a ready-to-use ident line) that gets
passed to the commit_tree() function.

Now, let's have a brief look at git-rebase--interactive.sh.

It was rightfully nicknamed "cherry-pick on drugs" in its early days,
because that is what it does: it calls `git cherry-pick` *most* of the
time.

So what does the (sequencer via being called from the) rebase--helper do?
Yep, exactly: it calls the internal do_pick() function that is the working
horse of the cherry-pick. And does that working horse call write_tree()
and commit_tree()? No, it does not. It *spawns a git-commit process*! And
the way to specify the author there is to pass the GIT_AUTHOR_*
environment variables.

Let's recapitulate.

Not only did the scripted am use a totally different code path to *read*
the author-script than rebase -i, the code path after that also differed
substantially from rebase -i to the point that very, very different Git
commands were called.

It also so happens that the proper way to convert those code paths into
builtin code uses very, very different functions from libgit.a that
require very, very different handling.

The builtin am never sets the environment variables GIT_AUTHOR_* but
instead constructs a complete ident line from it, and therefore *must*
validate the contents of author-script.

The sequencer *has to* set the GIT_AUTHOR_* environment variables because
it calls `git commit` in a different process *that already validates the
GIT_AUTHOR_* variables*.

Even worse (and this is not the first, or second, or third time I pointed
this out): if you mistook the identical file name, and identical content,
to mean that the different code paths *must* use a *single* function to
read the author-script (nevermind that the am code path reads it into a
structure that is specifically designed to support the "am" operation, and
nothing else), you would not only force the sequencer call to provide a
data structure it does not want, not only to validate the contents
unnecessarily, but it would have to lego the parsed contents *back into
almost the original shape as the original contents of the author-script*.
So it would have to add tons of code relative to the current version, for
no benefit at all, *increasing your maintenance burden for no good
reason*.

This repeated suggestion to reuse the code path to read the author-script
repeatedly ignores the fact that we have to do very, very different things
with the contents in those two code paths.

And if that was not enough: we are talking about code that is rock solid,
has been tested for almost a *year*, half of that year with a painful
cross-validation by running old and new rebase -i and comparing their
output. And this rock solid code is what you want to force me to change to
code that neither makes sense nor is tested well.

And I refuse to be forced to do that: we just proved collectively how good
a job we do when reviewing patches: we just flimsically introduced a
regression at the core of the operation of rebase -i. Not despite patch
review. Because of it.

In short: the objections against keeping the read_env_script() function
as-is (now fixed, of course) make no sense from several points of view:
logically, time-wise, and robustness.

> > and the rather real bug fix I sent out as a fixup! which you may want
> > to squash in (in the alternative, I can mailbomb v4 of the entire
> > sequencer-i patch series, that is your choice).
> 
> I'd rather see the "make elengant" thing redone in a more sensible
> way,

No, no, no, NO!

This is starting to become really, really annoying. The reasons you keep
repeating for rewriting this to use the same functions as builtin/am.c *do
not make sense*. Even when you repeat them a hundred times (I know, in the
US you have this really public example where somebody is really successful
repeating incorrect things over and over until some people start to
believe them, but this is not an example you should try to follow).

Just because you keep repeating that "am" and "rebase -i" both read
"author-script" files with the same syntax does not mean that it is
sensible to force them to use the same code path when the data needs to be
processed very differently afterwards!

And you can repeat another thousand times that it would make maintaining
the code easier, and it *still* would not make that statement less false.

The "am" command needs to validate the contents and process them into an
ident line.

The sequencer needs to pass the contents as environment variables to the
"git commit" command that validates the contents itself.

> I'll squash the fixup! thing in, and as I already said a few days ago, I
> do not think we'd want v4 (rather we'd want to go incremental).

Now, those are three statements that all make sense.

Thank you,
Johannes

Re: What's cooking in git.git (Jan 2017, #02; Sun, 15)

[Johannes Schindelin]

Hi Junio,

On Sun, 15 Jan 2017, Junio C Hamano wrote:

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

A suggestion: since it is very, very tedious to find the latest
iteration's thread, as well as the corresponding commit in `pu` (or
whatever other branch you use), and since you auto-generate these lists
anyway, it would make things less cumbersome if the commit names of the
tips, as well as the Message-IDs of the cover-letter (or first patch) were
displayed with the topic branches.

It still would not address e.g. the problem that the original authors are
not notified about the current state of their submission by these What's
Cooking mails.

But it would improve the situation.

> * js/difftool-builtin (2017-01-09) 5 commits
>  - t7800: run both builtin and scripted difftool, for now
>  - difftool: implement the functionality in the builtin
>  - difftool: add a skeleton for the upcoming builtin
>  - git_exec_path: do not return the result of getenv()

This patch was not in my patch submission. Sneaking it into this topic
branch is not incorrect.

It does not matter, though, as I will drop the Coverity patches from this
patch series: the conflation of Coverity fixes with the builtin difftool
was a mistake, and I will thereby address that mistake.

Ciao,
Johannes

[PATCHv3 4/4] unpack-trees: support super-prefix option

[Stefan Beller]

In the future we want to support working tree operations within submodules,
e.g. "git checkout --recurse-submodules", which will update the submodule
to the commit as recorded in its superproject. In the submodule the
unpack-tree operation is carried out as usual, but the reporting to the
user needs to prefix any path with the superproject. The mechanism for
this is the super-prefix. (see 74866d757, git: make super-prefix option)

Add support for the super-prefix option for commands that unpack trees
by wrapping any path output in unpacking trees in the newly introduced
super_prefixed function. This new function prefixes any path with the
super-prefix if there is one.  Assuming the submodule case doesn't happen
in the majority of the cases, we'd want to have a fast behavior for no
super prefix, i.e. no reallocation/copying, but just returning path.

Another aspect of introducing the `super_prefixed` function is to consider
who owns the memory and if this is the right place where the path gets
modified. As the super prefix ought to change the output behavior only and
not the actual unpack tree part, it is fine to be that late in the line.
As we get passed in 'const char *path', we cannot change the path itself,
which means in case of a super prefix we have to copy over the path.
We need two static buffers in that function as the error messages
contain at most two paths.

For testing purposes enable it in read-tree, which has no output
of paths other than an unpack-trees.c. These are all converted in
this patch.

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

This replaces the last two commits (squash and unpack-trees:
support super-prefix option) of sb/unpack-trees-super-prefix.

> * sb/unpack-trees-super-prefix (2017-01-12) 5 commits
>  - SQUASH
>  - unpack-trees: support super-prefix option
>  - t1001: modernize style
>  - t1000: modernize style
>  - read-tree: use OPT_BOOL instead of OPT_SET_INT
>
>  "git read-tree" and its underlying unpack_trees() machinery learned
>  to report problematic paths prefixed with the --super-prefix option.
>
>  Expecting a reroll.
>  The first three are in good shape.  The last one needs a better
>  explanation and possibly an update to its test.
>  cf. <CAGZ79kaHDnVLQr8WLoaD5KKs7EqeW=KbkptF=iHpU5t054Xcdw@mail.gmail.com>
>

This paragraph is for the first version of the last patch?
I reviewed what you queued and both the test is in modern
style as well as the commit message is adequate now.

I agree the SQUASH commit is a good idea, so this is a reroll
with the SQUASH squashed.  No further changes applied.

Thanks,
Stefan

 git.c                       |  2 +-
 t/t1001-read-tree-m-2way.sh |  8 ++++++++
 unpack-trees.c              | 43 ++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 49 insertions(+), 4 deletions(-)

diff --git a/git.c b/git.c
index bbaa949e9c..50e559c2a8 100644
--- a/git.c
+++ b/git.c
@@ -471,7 +471,7 @@ static struct cmd_struct commands[] = {
 	{ "prune-packed", cmd_prune_packed, RUN_SETUP },
 	{ "pull", cmd_pull, RUN_SETUP | NEED_WORK_TREE },
 	{ "push", cmd_push, RUN_SETUP },
-	{ "read-tree", cmd_read_tree, RUN_SETUP },
+	{ "read-tree", cmd_read_tree, RUN_SETUP | SUPPORT_SUPER_PREFIX},
 	{ "receive-pack", cmd_receive_pack },
 	{ "reflog", cmd_reflog, RUN_SETUP },
 	{ "remote", cmd_remote, RUN_SETUP },
diff --git a/t/t1001-read-tree-m-2way.sh b/t/t1001-read-tree-m-2way.sh
index 7b70089705..5ededd8e40 100755
--- a/t/t1001-read-tree-m-2way.sh
+++ b/t/t1001-read-tree-m-2way.sh
@@ -363,6 +363,14 @@ test_expect_success 'a/b (untracked) vs a, plus c/d case test.' '
 	test -f a/b
 '
 
+test_expect_success 'read-tree supports the super-prefix' '
+	cat <<-EOF >expect &&
+		error: Updating '\''fictional/a'\'' would lose untracked files in it
+	EOF
+	test_must_fail git --super-prefix fictional/ read-tree -u -m "$treeH" "$treeM" 2>actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'a/b vs a, plus c/d case setup.' '
 	rm -f .git/index &&
 	rm -fr a &&
diff --git a/unpack-trees.c b/unpack-trees.c
index 7a6df99d10..eee4865804 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -52,6 +52,41 @@ static const char *unpack_plumbing_errors[NB_UNPACK_TREES_ERROR_TYPES] = {
 	  ? ((o)->msgs[(type)])      \
 	  : (unpack_plumbing_errors[(type)]) )
 
+static const char *super_prefixed(const char *path)
+{
+	/*
+	 * It is necessary and sufficient to have two static buffers
+	 * here, as the return value of this function is fed to
+	 * error() using the unpack_*_errors[] templates we see above.
+	 */
+	static struct strbuf buf[2] = {STRBUF_INIT, STRBUF_INIT};
+	static int super_prefix_len = -1;
+	static unsigned idx = ARRAY_SIZE(buf) - 1;
+
+	if (super_prefix_len < 0) {
+		const char *super_prefix = get_super_prefix();
+		if (!super_prefix) {
+			super_prefix_len = 0;
+		} else {
+			int i;
+			for (i = 0; i < ARRAY_SIZE(buf); i++)
+				strbuf_addstr(&buf[i], super_prefix);
+			super_prefix_len = buf[0].len;
+		}
+	}
+
+	if (!super_prefix_len)
+		return path;
+
+	if (++idx >= ARRAY_SIZE(buf))
+		idx = 0;
+
+	strbuf_setlen(&buf[idx], super_prefix_len);
+	strbuf_addstr(&buf[idx], path);
+
+	return buf[idx].buf;
+}
+
 void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
 				  const char *cmd)
 {
@@ -172,7 +207,7 @@ static int add_rejected_path(struct unpack_trees_options *o,
 			     const char *path)
 {
 	if (!o->show_all_errors)
-		return error(ERRORMSG(o, e), path);
+		return error(ERRORMSG(o, e), super_prefixed(path));
 
 	/*
 	 * Otherwise, insert in a list for future display by
@@ -196,7 +231,7 @@ static void display_error_msgs(struct unpack_trees_options *o)
 			something_displayed = 1;
 			for (i = 0; i < rejects->nr; i++)
 				strbuf_addf(&path, "\t%s\n", rejects->items[i].string);
-			error(ERRORMSG(o, e), path.buf);
+			error(ERRORMSG(o, e), super_prefixed(path.buf));
 			strbuf_release(&path);
 		}
 		string_list_clear(rejects, 0);
@@ -1918,7 +1953,9 @@ int bind_merge(const struct cache_entry * const *src,
 			     o->merge_size);
 	if (a && old)
 		return o->gently ? -1 :
-			error(ERRORMSG(o, ERROR_BIND_OVERLAP), a->name, old->name);
+			error(ERRORMSG(o, ERROR_BIND_OVERLAP),
+			      super_prefixed(a->name),
+			      super_prefixed(old->name));
 	if (!a)
 		return keep_entry(old, o);
 	else
-- 
2.11.0.299.g762782ba8a