What's cooking in git.git (May 2017, #06; Mon, 22)

What's cooking in git.git (May 2017, #06; Mon, 22)

[Junio C Hamano]

What's cooking in git.git (May 2017, #06; Mon, 22)
--------------------------------------------------

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.

The tip of 'next' has been rewound, and the first batch of topics
for the next release have been merged to 'master'.  I tentatively
wrote doen that this cycle will last for 11 weeks, completing at the
end of July.

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]

* ab/conditional-config-with-symlinks (2017-05-17) 1 commit
 - config: match both symlink & realpath versions in IncludeIf.gitdir:*

 The recently introduced "[includeIf "gitdir:$dir"] path=..."
 mechansim has further been taught to take symlinks into account.
 The directory "$dir" specified in "gitdir:$dir" may be a symlink to
 a real location, not something that $(getcwd) may return.  In such
 a case, a realpath of "$dir" is compared with the real path of the
 current repository to determine if the contents from the named path
 should be included.

 Will merge to 'next'.


* bm/interpret-trailers-cut-line-is-eom (2017-05-18) 1 commit
 - interpret-trailers: honor the cut line

 "git interpret-trailers", when used as GIT_EDITOR for "git commit
 -v", looked for and appended to a trailer block at the very end,
 i.e. at the end of the "diff" output.  The command has been
 corrected to pay attention to the cut-mark line "commit -v" adds to
 the buffer---the real trailer block should appear just before it.

 Will merge to 'next'.


* jc/skip-test-in-the-middle (2017-05-18) 2 commits
 - t5545: enhance test coverage when no http server is installed
 - test: allow skipping the remainder

 A recent update to t5545-push-options.sh started skipping all the
 tests in the script when a web server testing is disabled or
 unavailable, not just the ones that require a web server.  Non HTTP
 tests have been salvaged to always run in this script.

 Will merge to 'next'.


* jk/alternate-ref-optim (2017-05-18) 1 commit
 - t5400: avoid concurrent writes into a trace file

 A test allowed both "git push" and "git receive-pack" on the other
 end write their traces into the same file.  This is OK on platforms
 that allows atomically appending to a file opened with O_APPEND,
 but on other platforms led to a mangled output, causing
 intermittent test failures.  This has been fixed by disabling
 traces from "receive-pack" in the test.

 Will merge to 'next'.


* mh/packed-ref-store-prep (2017-05-18) 23 commits
 - cache_ref_iterator_begin(): avoid priming unneeded directories
 - ref-filter: limit traversal to prefix
 - create_ref_entry(): remove `check_name` option
 - refs_ref_iterator_begin(): handle `GIT_REF_PARANOIA`
 - read_packed_refs(): report unexpected fopen() failures
 - read_packed_refs(): do more of the work of reading packed refs
 - get_packed_ref_cache(): assume "packed-refs" won't change while locked
 - should_pack_ref(): new function, extracted from `files_pack_refs()`
 - ref_update_reject_duplicates(): add a sanity check
 - ref_update_reject_duplicates(): use `size_t` rather than `int`
 - ref_update_reject_duplicates(): expose function to whole refs module
 - ref_transaction_commit(): break into multiple functions
 - files_transaction_cleanup(): new helper function
 - files_ref_store: put the packed files lock directly in this struct
 - files-backend: move `lock` member to `files_ref_store`
 - lockfile: add a new method, is_lock_file_locked()
 - ref_store: take `logmsg` parameter when deleting references
 - refs: use `size_t` indexes when iterating over ref transaction updates
 - refs_ref_iterator_begin(): don't check prefixes redundantly
 - prefix_ref_iterator: don't trim too much
 - ref_iterator_begin_fn(): fix docstring
 - refs.h: clarify docstring for the ref_transaction_update()-related fns
 - t3600: clean up permissions test properly

 The implementation of "ref" API around the "packed refs" have been
 cleaned up, in preparation for further changes.

 Expecting a reroll.


* sl/clean-d-ignored-fix (2017-05-22) 7 commits
 - SQUASH????
 - clean: teach clean -d to skip dirs containing ignored files
 - dir: expose cmp_name() and check_contains()
 - dir: hide untracked contents of untracked dirs
 - dir: recurse into untracked dirs for ignored files
 - t7061: status --ignored should search untracked dirs
 - t7300: clean -d should skip dirs with ignored files

 "git clean -d" used to clean directories that has ignored files,
 even though the command should not lose ignored ones without "-x".
 This has been corrected.


* tg/stash-push-fixup (2017-05-17) 1 commit
 - completion: add git stash push

 The shell completion script (in contrib/) learned "git stash" has
 a new "push" subcommand.

 Will merge to 'next'.


* ab/sha1dc (2017-05-22) 3 commits
 - sha1collisiondetection: automatically enable when submodule is populated
 - sha1dc: optionally use sha1collisiondetection as a submodule
 - sha1dc: update from upstream

 The "collission-detecting" implementation of SHA-1 hash we borrowed
 from is replaced by directly binding the upstream project as our
 submodule.

 Will keep in 'pu' for a few CI cycles.
 Impact to the various build and release infrastructure of using
 submodule is not yet fully known, but this lets us dip our toes.


* bp/fsmonitor (2017-05-21) 10 commits
 - SQUASH???
 - SQUASH???
 - SQUASH???
 - fsmonitor: add a sample query-fsmonitor hook script for Watchman
 - fsmonitor: add documentation for the fsmonitor extension.
 - fsmonitor: add test cases for fsmonitor extension
 - SQUASH???
 - fsmonitor: teach git to optionally utilize a file system monitor to speed up detecting new or changed files.
 - dir: make lookup_untracked() available outside of dir.c
 - bswap: add 64 bit endianness helper get_be64

 Expecting reroll.


* dk/send-email-avoid-net-smtp-ssl-when-able (2017-05-20) 1 commit
 - send-email: Net::SMTP::SSL is obsolete, use only when necessary

 "git send-email" now uses Net::SMTP::SSL, which is obsolete, only
 when needed.  Recent versions of Net::SMTP can do TLS natively.

 Will merge to 'next'.


* jc/name-rev-lw-tag (2017-03-29) 2 commits
 - name-rev: favor describing with tags and use committer date to tiebreak
 - name-rev: refactor logic to see if a new candidate is a better name
 (this branch is used by mg/name-rev-debug.)

 "git describe --contains" penalized light-weight tags so much that
 they were almost never considered.  Instead, give them about the
 same chance to be considered as an annotated tag that is the same
 age as the underlying commit would.

 Will merge to 'next'.


* jk/diff-blob (2017-05-20) 15 commits
 - diff: use blob path for blob/file diffs
 - diff: use pending "path" if it is available
 - diff: use the word "path" instead of "name" for blobs
 - diff: pass whole pending entry in blobinfo
 - handle_revision_arg: record paths for pending objects
 - handle_revision_arg: record modes for "a..b" endpoints
 - t4063: add tests of direct blob diffs
 - get_sha1_with_context: dynamically allocate oc->path
 - get_sha1_with_context: always initialize oc->symlink_path
 - sha1_name: consistently refer to object_context as "oc"
 - handle_revision_arg: add handle_dotdot() helper
 - handle_revision_arg: hoist ".." check out of range parsing
 - handle_revision_arg: stop using "dotdot" as a generic pointer
 - handle_revision_arg: simplify commit reference lookups
 - handle_revision_arg: reset "dotdot" consistently

 The result from "git diff" that compares two blobs, e.g. "git diff
 $commit1:$path $commit2:$path", used to be shown with the full
 object name as given on the command line, but it is more natural to
 use the $path in the output and use it to look up .gitattributes.

 Needs review.


* jk/ignore-broken-tags-when-ignoring-missing-links (2017-05-20) 1 commit
 - revision.c: ignore broken tags with ignore_missing_links

 Tag objects, which are not reachable from any ref, that point at
 missing objects were mishandled by "git gc" and friends (they
 should silently be ignored instead)
 
 Will merge to 'next'.


* js/bs-is-a-dir-sep-on-windows (2017-05-20) 2 commits
 - Windows: do not treat a path with backslashes as a remote's nick name
 - mingw.h: permit arguments with side effects for is_dir_sep

 "foo\bar\baz" in "git fetch foo\bar\baz", even though there is no
 slashes in it, cannot be a nickname for a remote on Windows, as
 that is likely to be a pathname on a local filesystem.

 Will merge to 'next'.


* js/larger-timestamps (2017-05-20) 1 commit
 - name-rev: change a "long" variable to timestamp_t

 A follow-up hotfix for a topic already in 'master'.

 Will merge to 'next'.


* km/log-showsignature-doc (2017-05-20) 1 commit
 - config.txt: add an entry for log.showSignature

 Will merge to 'next'.


* kn/ref-filter-branch-list (2017-05-20) 1 commit
 - ref-filter: resolve HEAD when parsing %(HEAD) atom

 "git for-each-ref --format=..." with %(HEAD) in the format used to
 resolve the HEAD symref as many times as it had processed refs,
 which was wasteful, and "git branch" shared the same problem.

 Will merge to 'next'.


* pw/rebase-i-regression-fix (2017-05-20) 3 commits
 - rebase -i: add missing newline to end of message
 - rebase -i: silence stash apply
 - rebase -i: fix reflog message

 Regression fix to topic recently merged to 'master'.

 Will merge to 'next'.


* sb/diff-color-move (2017-05-20) 20 commits
 - diff.c: color moved lines differently
 - diff: buffer all output if asked to
 - diff.c: emit_line includes whitespace highlighting
 - diff.c: convert diff_summary to use emit_line_*
 - diff.c: convert diff_flush to use emit_line_*
 - diff.c: convert word diffing to use emit_line_*
 - diff.c: convert show_stats to use emit_line_*
 - diff.c: convert emit_binary_diff_body to use emit_line_*
 - submodule.c: convert show_submodule_summary to use emit_line_fmt
 - diff.c: convert emit_rewrite_lines to use emit_line_*
 - diff.c: convert emit_rewrite_diff to use emit_line_*
 - diff.c: convert builtin_diff to use emit_line_*
 - diff.c: convert fn_out_consume to use emit_line
 - diff.c: inline emit_line_0 into emit_line
 - diff.c: emit_line_0 takes parameter whether to output line prefix
 - diff.c: emit_line_0 can handle no color setting
 - diff.c: teach emit_line_0 to accept sign parameter
 - diff.c: factor out diff_flush_patch_all_file_pairs
 - diff: move line ending check into emit_hunk_header
 - diff: readability fix

 "git diff" has been taught to optionally paint new lines that are
 the same as deleted lines elsewhere differently from genuinely new
 lines.


* xz/send-email-batch-size (2017-05-22) 2 commits
 - SQUASH???
 - send-email: --batch-size to work around some SMTP server limit

 "git send-email" learned to overcome some SMTP server limitation
 that does not allow many pieces of e-mails to be sent over a single
 session.


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

* mg/status-in-progress-info (2017-05-10) 2 commits
 - status --short --inprogress: spell it as --in-progress
 - status: show in-progress info for short status

 "git status" learns an option to report various operations
 (e.g. "merging") that the user is in the middle of.

 cf. <xmqqmvakcdqw.fsf@gitster.mtv.corp.google.com>


* mg/name-rev-debug (2017-03-31) 2 commits
 - describe: pass --debug down to name-rev
 - name-rev: provide debug output
 (this branch uses jc/name-rev-lw-tag.)

 "git describe --debug --contains" did not add any meaningful
 information, even though without "--contains" it did.


* nd/worktree-move (2017-04-20) 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.

 Expecting a reroll.
 cf. <20170420101024.7593-1-pclouds@gmail.com>
 cf. <20170421145916.mknekgqzhxffu7di@sigill.intra.peff.net>
 cf. <d0e81b1e-5869-299e-f462-4d43dc997bd1@ramsayjones.plus.com>


* df/dir-iter-remove-subtree (2017-04-19) 5 commits
 - remove_subtree(): reimplement using iterators
 - dir_iterator: rewrite state machine model
 - dir_iterator: refactor dir_iterator_advance
 - remove_subtree(): test removing nested directories
 - dir_iterator: add tests for dir_iterator API

 Update the dir-iterator API and use it to reimplement
 remove_subtree().

 A reroll exists that is based on the updated 'master', but I ran
 out of time trying to get it to work with other topics in flight
 in 'pu'.
 GSoC microproject.


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

 Expecting a reroll.


* sk/dash-is-previous (2017-03-01) 5 commits
 - revert.c: delegate handling of "-" shorthand to setup_revisions
 - sha1_name.c: teach get_sha1_1 "-" shorthand for "@{-1}"
 - revision.c: args starting with "-" might be a revision
 - revision.c: swap if/else blocks
 - revision.c: do not update argv with unknown option

 A dash "-" can be written to mean "the branch that was previously
 checked out" in more places.

 Needs review.
 cf. <1488007487-12965-1-git-send-email-kannan.siddharth12@gmail.com>

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

* ah/log-decorate-default-to-auto (2017-05-15) 1 commit
  (merged to 'next' on 2017-05-20 at 209614479c)
 + builtin/log: honor log.decorate

 Setting "log.decorate=false" in the configuration file did not take
 effect in v2.13, which has been corrected.

 Will merge to 'master'.


* jh/memihash-opt (2017-05-16) 5 commits
 - p0004: don't error out if test repo is too small
 - p0004: don't abort if multi-threaded is too slow
 - p0004: use test_perf
 - p0004: avoid using pipes
 - p0004: simplify calls of test-lazy-init-name-hash

 perf-test update.

 Will merge to 'next'.


* jk/bug-to-abort (2017-05-22) 4 commits
  (merged to 'next' on 2017-05-22 at 20371eebb6)
 + usage: add NORETURN to BUG() function definitions
  (merged to 'next' on 2017-05-20 at db65acc882)
 + config: complain about --local outside of a git repo
 + setup_git_env: convert die("BUG") to BUG()
 + usage.c: add BUG() function

 Introduce the BUG() macro to improve die("BUG: ...").

 Will merge to 'master'.


* jk/no-looking-at-dotgit-outside-repo (2017-05-15) 1 commit
 - config: complain about --local outside of a git repo

 Will discard.
 Superseded by jk/bug-to-abort.


* jk/update-links-in-docs (2017-05-15) 1 commit
 - doc: use https links to Wikipedia to avoid http redirects

 A few http:// links that are redirected to https:// in the
 documentation have been updated to https:// links.

 Will merge to 'next'.


* js/blame-lib (2017-05-15) 22 commits
 - blame: create entry prepend function in libgit
 - blame: create scoreboard setup function in libgit
 - blame: create scoreboard init function in libgit
 - blame: move scoreboard-related methods to libgit
 - blame: move fake-commit-related methods to libgit
 - blame: move origin-related methods to libgit
 - blame: rework methods that determine 'final' commit
 - blame: wrap blame_sort and compare_blame_final
 - blame: move progess updates to a scoreboard callback
 - blame: make sanity_check use a callback in scoreboard
 - blame: move no_whole_file_rename flag to scoreboard
 - blame: move xdl_opts flags to scoreboard
 - blame: move show_root flag to scoreboard
 - blame: move reverse flag to scoreboard
 - blame: move contents_from to scoreboard
 - blame: move copy/move thresholds to scoreboard
 - blame: move stat counters to scoreboard
 - blame: move scoreboard structure to header
 - blame: move origin and entry structures to header
 - blame: remove unused parameters
 - blame: move textconv_object with related functions
 - blame: remove unneeded dependency on blob.h

 The internal logic used in "git blame" has been libified to make it
 easier to use by cgit.


* jt/send-email-validate-hook (2017-05-16) 1 commit
 - send-email: support validate hook

 "git send-email" learned to run sendemail-validate hook to inspect
 and reject a message before sending it out.

 Will merge to 'next'.


* bw/pathspec-sans-the-index (2017-05-12) 6 commits
 - pathspec: convert find_pathspecs_matching_against_index to take an index
 - pathspec: remove PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP
 - ls-files: prevent prune_cache from overeagerly pruning submodules
 - pathspec: remove PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE flag
 - submodule: add die_in_unpopulated_submodule function
 - pathspec: provide a more descriptive die message

 Simplify parse_pathspec() codepath and stop it from looking at the
 default in-core index.

 Will merge to 'next'.


* jt/fetch-allow-tip-sha1-implicitly (2017-05-16) 1 commit
 - fetch-pack: always allow fetching of literal SHA1s

 There is no good reason why "git fetch $there $sha1" should fail
 when the $sha1 names an object at the tip of an advertised ref,
 even when the other side hasn't enabled allowTipSHA1InWant.

 Will merge to 'next'.


* jc/read-tree-empty-with-m (2017-05-10) 1 commit
  (merged to 'next' on 2017-05-20 at 7e86815364)
 + read-tree: "read-tree -m --empty" does not make sense

 "git read-tree -m" (no tree-ish) gave a nonsense suggestion "use
 --empty if you want to clear the index".  With "-m", such a request
 will still fail anyway, as you'd need to name at least one tree-ish
 to be merged.

 Will merge to 'master'.


* ab/perf-wildmatch (2017-05-12) 2 commits
 - perf: add test showing exponential growth in path globbing
 - perf: add function to setup a fresh test repo

 Add perf-test for wildmatch.

 Will merge to 'next'.


* jk/doc-config-include (2017-05-12) 4 commits
 - docs/config: consistify include.path examples
 - docs/config: avoid the term "expand" for includes
 - docs/config: give a relative includeIf example
 - docs/config: clarify include/includeIf relationship

 Clarify documentation for include.path and includeIf.<condition>.path
 configuration variables.

 Will merge to 'next'.


* js/retire-old-remote-spec (2017-05-12) 11 commits
 . PREVIEW: remove support for .git/remotes/ and .git/branches/
 . PREVIEW: t0060: stop testing support for .git/remotes/ and .git/branches/
 . PREVIEW: t5515: remove .git/remotes/ and .git/branches/ tests
 . PREVIEW: remote: remove support for migrating ancient remotes
 . PREVIEW: t5516: stop testing .git/branches/ functionality
 . PREVIEW: t5510: convert .git/remotes/ test to use a regular remote
 . Revert "Revert "Don't create the $GIT_DIR/branches directory on init""
 . remote: warn loud and clear when .git/remotes/ is *still* used
 . remote: warn loud and clear when .git/branches/ is *still* used
 . Documentation: really deprecate .git/remotes/ and .git/branches/
 . git-parse-remote: fix highly misleading man page

 Stop reading from .git/remotes/ and .git/branches/, two old ways
 that we have been supporting to configure short-hands for fetching
 from remote repositories.

 Will discard.
 The procedure of removing an old and unused feature is correctly
 followed by this series, but it is far from clear that the feature
 this attempts to remove is unused or useless.


* ab/grep-preparatory-cleanup (2017-05-21) 30 commits
 - grep: assert that threading is enabled when calling grep_{lock,unlock}
 - grep: given --threads with NO_PTHREADS=YesPlease, warn
 - pack-objects: fix buggy warning about threads
 - pack-objects & index-pack: add test for --threads warning
 - test-lib: add a PTHREADS prerequisite
 - grep: move is_fixed() earlier to avoid forward declaration
 - grep: change internal *pcre* variable & function names to be *pcre1*
 - grep: change the internal PCRE macro names to be PCRE1
 - grep: factor test for \0 in grep patterns into a function
 - grep: remove redundant regflags assignments
 - grep: catch a missing enum in switch statement
 - perf: add a comparison test of log --grep regex engines
 - perf: add a comparison test of grep regex engines with -F
 - perf: add a comparison test of grep regex engines
 - perf: emit progress output when unpacking & building
 - perf: add a GIT_PERF_MAKE_COMMAND for when *_MAKE_OPTS won't do
 - grep: add tests to fix blind spots with \0 patterns
 - grep: prepare for testing binary regexes containing rx metacharacters
 - grep: add a test helper function for less verbose -f \0 tests
 - grep: add tests for grep pattern types being passed to submodules
 - grep: amend submodule recursion test for regex engine testing
 - grep: add tests for --threads=N and grep.threads
 - grep: change non-ASCII -i test to stop using --debug
 - grep: add a test for backreferences in PCRE patterns
 - grep: add a test asserting that --perl-regexp dies when !PCRE
 - log: make --regexp-ignore-case work with --perl-regexp
 - log: add exhaustive tests for pattern style options & config
 - test-lib: rename the LIBPCRE prerequisite to PCRE
 - grep & rev-list doc: stop promising libpcre for --perl-regexp
 - Makefile & configure: reword inaccurate comment about PCRE

 The internal implementation of "git grep" has seen some clean-up.


* jt/push-options-doc (2017-05-10) 2 commits
  (merged to 'next' on 2017-05-20 at ca7f344111)
 + receive-pack: verify push options in cert
 + docs: correct receive.advertisePushOptions default

 The receive-pack program now makes sure that the push certificate
 records the same set of push options used for pushing.

 Will merge to 'master'.


* dt/unpack-save-untracked-cache-extension (2017-05-20) 1 commit
 - unpack-trees: preserve index extensions

 When "git checkout", "git merge", etc. manipulates the in-core
 index, various pieces of information in the index extensions are
 discarded from the original state, as it is usually not the case
 that they are kept up-to-date and in-sync with the operation on the
 main index.  The untracked cache extension is copied across these
 operations now, which would speed up "git status" (as long as the
 cache is properly invalidated).

 Will merge to 'next'.


* sg/clone-refspec-from-command-line-config (2017-05-16) 4 commits
 - clone: use free_refspec() to free refspec list
 - remote: drop free_refspecs() function
 - Documentation/clone: document ignored configuration variables
 - clone: respect additional configured fetch refspecs during initial fetch

 "git clone -c var=val" is a way to set configuration variables in
 the resulting repository, but it is more useful to also make these
 variables take effect while the initial clone is happening,
 e.g. these configuration variables could be fetch refspecs.


* sb/checkout-recurse-submodules (2017-05-04) 3 commits
  (merged to 'next' on 2017-05-20 at f972b2069f)
 + submodule: properly recurse for read-tree and checkout
 + submodule: avoid auto-discovery in new working tree manipulator code
 + submodule_move_head: reuse child_process structure for futher commands

 "git checkout --recurse-submodules" did not quite work with a
 submodule that itself has submodules.

 Will merge to 'master'.


* ja/do-not-ask-needless-questions (2017-05-12) 3 commits
 - git-filter-branch: be more direct in an error message
 - read-tree -m: make error message for merging 0 trees less smart aleck
 - usability: don't ask questions if no reply is required

 Git sometimes gives an advice in a rhetorical question that does
 not require an answer, which can confuse new users and non native
 speakers.  Attempt to rephrase them.

 Will merge to 'next'.


* ab/doc-replace-gmane-links (2017-05-09) 2 commits
  (merged to 'next' on 2017-05-20 at 2c4f96560c)
 + doc: replace more gmane links
 + doc: replace a couple of broken gmane links

 The Web interface to gmane news archive is long gone, even though
 the articles are still accessible via NTTP.  Replace the links with
 ones to public-inbox.org.  Because their message identification is
 based on the actual message-id, it is likely that it will be easier
 to migrate away from it if/when necessary.

 Will merge to 'master'.


* ab/fix-poison-tests (2017-05-11) 3 commits
  (merged to 'next' on 2017-05-20 at 5b13ba86bd)
 + travis-ci: add job to run tests with GETTEXT_POISON
 + travis-ci: setup "prove cache" in "script" step
 + tests: fix tests broken under GETTEXT_POISON=YesPlease

 Update tests to pass under GETTEXT_POISON (a mechanism to ensure
 that output strings that should not be translated are not
 translated by mistake), and tell TravisCI to run them.

 Will merge to 'master'.


* bw/dir-c-stops-relying-on-the-index (2017-05-06) 14 commits
  (merged to 'next' on 2017-05-20 at 1f1b764ec8)
 + dir: convert fill_directory to take an index
 + dir: convert read_directory to take an index
 + dir: convert read_directory_recursive to take an index
 + dir: convert open_cached_dir to take an index
 + dir: convert is_excluded to take an index
 + dir: convert prep_exclude to take an index
 + dir: convert add_excludes to take an index
 + dir: convert is_excluded_from_list to take an index
 + dir: convert last_exclude_matching_from_list to take an index
 + dir: convert dir_add* to take an index
 + dir: convert get_dtype to take index
 + dir: convert directory_exists_in_index to take index
 + dir: convert read_skip_worktree_file_from_index to take an index
 + dir: stop using the index compatibility macros

 API update.

 Will merge to 'master'.


* jk/diff-submodule-diff-inline (2017-05-08) 1 commit
  (merged to 'next' on 2017-05-20 at da7eb2626a)
 + diff: recurse into nested submodules for inline diff

 "git diff --submodule=diff" now recurses into nested submodules.

 Will merge to 'master'.


* jk/disable-pack-reuse-when-broken (2017-05-09) 2 commits
  (merged to 'next' on 2017-05-20 at 684e921273)
 + t5310: fix "; do" style
 + pack-objects: disable pack reuse for object-selection options

 "pack-objects" can stream a slice of an existing packfile out when
 the pack bitmap can tell that the reachable objects are all needed
 in the output, without inspecting individual objects.  This
 strategy however would not work well when "--local" and other
 options are in use, and need to be disabled.

 Will merge to 'master'.


* js/eol-on-ourselves (2017-05-10) 6 commits
  (merged to 'next' on 2017-05-20 at 8023c277d2)
 + t4051: mark supporting files as requiring LF-only line endings
 + Fix the remaining tests that failed with core.autocrlf=true
 + t3901: move supporting files into t/t3901/
 + completion: mark bash script as LF-only
 + git-new-workdir: mark script as LF-only
 + Fix build with core.autocrlf=true

 Make sure our tests would pass when the sources are checked out
 with "platform native" line ending convention by default on
 Windows.  Some "text" files out tests use and the test scripts
 themselves that are meant to be run with /bin/sh, ought to be
 checked out with eol=LF even on Windows.

 Will merge to 'master'.


* nd/split-index-unshare (2017-05-08) 2 commits
  (merged to 'next' on 2017-05-20 at 9048cdc2ce)
 + p3400: add perf tests for rebasing many changes
 + split-index: add and use unshare_split_index()

 Plug some leaks and updates internal API used to implement the
 split index feature to make it easier to avoid such a leak in the
 future.

 Will merge to 'master'.


* rs/checkout-am-fix-unborn (2017-05-08) 2 commits
  (merged to 'next' on 2017-05-20 at d4f1ee3ed7)
 + am: check return value of resolve_refdup before using hash
 + checkout: check return value of resolve_refdup before using hash

 A few codepaths in "checkout" and "am" working on an unborn branch
 tried to access an uninitialized piece of memory.

 Will merge to 'master'.


* bw/submodule-with-bs-path (2017-05-01) 1 commit
  (merged to 'next' on 2017-05-20 at b740f784cb)
 + t7400: add !CYGWIN prerequisite to 'add with \\ in path'

 A hotfix to a topic that is already in v2.13.

 Will merge to 'master'.


* js/plug-leaks (2017-05-09) 26 commits
  (merged to 'next' on 2017-05-20 at fb136ea2dc)
 + checkout: fix memory leak
 + submodule_uses_worktrees(): plug memory leak
 + show_worktree(): plug memory leak
 + name-rev: avoid leaking memory in the `deref` case
 + remote: plug memory leak in match_explicit()
 + add_reflog_for_walk: avoid memory leak
 + shallow: avoid memory leak
 + line-log: avoid memory leak
 + receive-pack: plug memory leak in update()
 + fast-export: avoid leaking memory in handle_tag()
 + mktree: plug memory leaks reported by Coverity
 + pack-redundant: plug memory leak
 + setup_discovered_git_dir(): plug memory leak
 + setup_bare_git_dir(): help static analysis
 + split_commit_in_progress(): simplify & fix memory leak
 + checkout: fix memory leak
 + cat-file: fix memory leak
 + mailinfo & mailsplit: check for EOF while parsing
 + status: close file descriptor after reading git-rebase-todo
 + difftool: address a couple of resource/memory leaks
 + get_mail_commit_oid(): avoid resource leak
 + git_config_rename_section_in_file(): avoid resource leak
 + add_commit_patch_id(): avoid allocating memory unnecessarily
 + winansi: avoid buffer overrun
 + winansi: avoid use of uninitialized value
 + mingw: avoid memory leak when splitting PATH

 Fix memory leaks pointed out by Coverity (and people).

 Will merge to 'master'.


* jh/close-index-before-stat (2017-04-28) 1 commit
  (merged to 'next' on 2017-05-16 at 0c0372eb02)
 + read-cache: close index.lock in do_write_index

 Originally merged to 'next' on 2017-04-30

 The timestamp of the index file is now taken after the file is
 closed, to help Windows, on which a stale timestamp is reported by
 fstat() on a file that is opened for writing and data was written
 but not yet closed.

 Will cook in 'next'.


* ls/travis-relays-for-windows-ci (2017-05-04) 2 commits
  (merged to 'next' on 2017-05-20 at 47f34b78e2)
 + travis-ci: retry if Git for Windows CI returns HTTP error 502 or 503
 + travis-ci: handle Git for Windows CI status "failed" explicitly

 Will merge to 'master'.


* mb/diff-default-to-indent-heuristics (2017-05-09) 4 commits
 - add--interactive: drop diff.indentHeuristic handling
 - diff: enable indent heuristic by default
 - diff: have the diff-* builtins configure diff before initializing revisions
 - diff: make the indent heuristic part of diff's basic configuration

 Make the "indent" heuristics the default in "diff" and diff.indentHeuristics
 configuration variable an escape hatch for those who do no want it.

 Kicked out of next; it seems it is still getting review suggestions?


* tb/dedup-crlf-tests (2017-05-10) 1 commit
  (merged to 'next' on 2017-05-20 at 0a7ffff154)
 + t0027: tests are not expensive; remove t0025

 Will merge to 'master'.


* jc/repack-threads (2017-04-27) 1 commit
  (merged to 'next' on 2017-05-20 at c1fd54a2a4)
 + repack: accept --threads=<n> and pass it down to pack-objects

 "git repack" learned to accept the --threads=<n> option and pass it
 to pack-objects.

 Will merge to 'master'.


* nd/fopen-errors (2017-05-09) 23 commits
 - t1308: add a test case on open a config directory
 - config.c: handle error on failing to fopen()
 - xdiff-interface.c: report errno on failure to stat() or fopen()
 - wt-status.c: report error on failure to fopen()
 - server-info: report error on failure to fopen()
 - sequencer.c: report error on failure to fopen()
 - rerere.c: report correct errno
 - rerere.c: report error on failure to fopen()
 - remote.c: report error on failure to fopen()
 - commit.c: report error on failure to fopen() the graft file
 - log: fix memory leak in open_next_file()
 - log: report errno on failure to fopen() a file
 - blame: report error on open if graft_file is a directory
 - bisect: report on fopen() error
 - ident.c: use fopen_or_warn()
 - attr.c: use fopen_or_warn()
 - wrapper.c: add fopen_or_warn()
 - wrapper.c: add warn_on_fopen_errors()
 - config.mak.uname: set FREAD_READS_DIRECTORIES for Darwin, too
 - config.mak.uname: set FREAD_READS_DIRECTORIES for Linux and FreeBSD
 - clone: use xfopen() instead of fopen()
 - Use xfopen() in more places
 - git_fopen: fix a sparse 'not declared' warning

 We often try to open a file for reading whose existence is
 optional, and silently ignore errors from open/fopen; report such
 errors if they are not due to missing files.


* bc/object-id (2017-05-08) 53 commits
  (merged to 'next' on 2017-05-20 at e7372733fb)
 + object: convert parse_object* to take struct object_id
 + tree: convert parse_tree_indirect to struct object_id
 + sequencer: convert do_recursive_merge to struct object_id
 + diff-lib: convert do_diff_cache to struct object_id
 + builtin/ls-tree: convert to struct object_id
 + merge: convert checkout_fast_forward to struct object_id
 + sequencer: convert fast_forward_to to struct object_id
 + builtin/ls-files: convert overlay_tree_on_cache to object_id
 + builtin/read-tree: convert to struct object_id
 + sha1_name: convert internals of peel_onion to object_id
 + upload-pack: convert remaining parse_object callers to object_id
 + revision: convert remaining parse_object callers to object_id
 + revision: rename add_pending_sha1 to add_pending_oid
 + http-push: convert process_ls_object and descendants to object_id
 + refs/files-backend: convert many internals to struct object_id
 + refs: convert struct ref_update to use struct object_id
 + ref-filter: convert some static functions to struct object_id
 + Convert struct ref_array_item to struct object_id
 + Convert the verify_pack callback to struct object_id
 + Convert lookup_tag to struct object_id
 + log-tree: convert to struct object_id
 + Convert lookup_tree to struct object_id
 + builtin/reflog: convert tree_is_complete to take struct object_id
 + tree: convert read_tree_1 to use struct object_id internally
 + Convert lookup_blob to struct object_id
 + Convert remaining callers of lookup_blob to object_id
 + builtin/unpack-objects: convert to struct object_id
 + pack: convert struct pack_idx_entry to struct object_id
 + Convert lookup_commit* to struct object_id
 + Convert remaining callers of lookup_commit_reference* to object_id
 + builtin/tag: convert to struct object_id
 + sequencer: convert some functions to struct object_id
 + shallow: convert shallow registration functions to object_id
 + revision: convert prepare_show_merge to struct object_id
 + notes-utils: convert internals to struct object_id
 + http-push: convert some static functions to struct object_id
 + tag: convert parse_tag_buffer to struct object_id
 + builtin/verify-commit: convert to struct object_id
 + reflog_expire: convert to struct object_id
 + parse-options-cb: convert to struct object_id
 + notes-cache: convert to struct object_id
 + submodule: convert merge_submodule to use struct object_id
 + fast-import: convert to struct object_id
 + fast-import: convert internal structs to struct object_id
 + builtin/rev-parse: convert to struct object_id
 + builtin/blame: convert static function to struct object_id
 + branch: convert to struct object_id
 + bundle: convert to struct object_id
 + builtin/prune: convert to struct object_id
 + builtin/name-rev: convert to struct object_id
 + Convert struct cache_tree to use struct object_id
 + Clean up outstanding object_id transforms.
 + fetch-pack: convert to struct object_id

 Conversion from uchar[20] to struct object_id continues.

 Will merge to 'master'.


* jc/checkout-working-tree-only (2017-04-27) 1 commit
 - checkout: add --working-tree-only option

 "git checkout <tree-ish> <pathspec>" learned a variant that does
 not update the index when doing its thing.

 This was more of "the world could have been like this" illustration
 rather than a "let's make this change" proposal.  Unless people
 really want it, I am inclined to discard this topic.  Opinions?


* js/rebase-i-final (2017-05-02) 10 commits
 - rebase -i: rearrange fixup/squash lines using the rebase--helper
 - t3415: test fixup with wrapped oneline
 - rebase -i: skip unnecessary picks using the rebase--helper
 - rebase -i: check for missing commits in the rebase--helper
 - t3404: relax rebase.missingCommitsCheck tests
 - rebase -i: also expand/collapse the SHA-1s via the rebase--helper
 - rebase -i: do not invent onelines when expanding/collapsing SHA-1s
 - rebase -i: remove useless indentation
 - rebase -i: generate the script via rebase--helper
 - t3415: verify that an empty instructionFormat is handled as before

 The final batch to "git rebase -i" updates to move more code from
 the shell script to C.

 Needs review.


* bw/forking-and-threading (2017-05-15) 14 commits
 - usage.c: drop set_error_handle()
 - run-command: restrict PATH search to executable files
 - run-command: expose is_executable function
 - run-command: block signals between fork and execve
 - run-command: add note about forking and threading
 - run-command: handle dup2 and close errors in child
 - run-command: eliminate calls to error handling functions in child
 - run-command: don't die in child when duping /dev/null
 - run-command: prepare child environment before forking
 - string-list: add string_list_remove function
 - run-command: use the async-signal-safe execv instead of execvp
 - run-command: prepare command before forking
 - t0061: run_command executes scripts without a #! line
 - t5550: use write_script to generate post-update hook

 The "run-command" API implementation has been made more robust
 against dead-locking in a threaded environment.

 Will merge to 'next'.


* sb/reset-recurse-submodules (2017-04-23) 4 commits
  (merged to 'next' on 2017-05-20 at 8adafdf1be)
 + builtin/reset: add --recurse-submodules switch
 + submodule.c: submodule_move_head works with broken submodules
 + submodule.c: uninitialized submodules are ignored in recursive commands
 + entry.c: submodule recursing: respect force flag correctly

 "git reset" learned "--recurse-submodules" option.

 Will merge to 'master'.


* bp/sub-process-convert-filter (2017-05-15) 11 commits
 - convert: update subprocess_read_status() to not die on EOF
 - sub-process: move sub-process functions into separate files
 - convert: rename reusable sub-process functions
 - convert: update generic functions to only use generic data structures
 - convert: separate generic structures and variables from the filter specific ones
 - convert: split start_multi_file_filter() into two separate functions
 - pkt-line: annotate packet_writel with LAST_ARG_MUST_BE_NULL
 - convert: move packet_write_line() into pkt-line as packet_writel()
 - pkt-line: add packet_read_line_gently()
 - pkt-line: fix packet_read_line() to handle len < 0 errors
 - convert: remove erroneous tests for errno == EPIPE

 Code from "conversion using external process" codepath has been
 extracted to a separate sub-process.[ch] module.

 Will merge to 'next'.


* nd/prune-in-worktree (2017-04-24) 12 commits
 - rev-list: expose and document --single-worktree
 - revision.c: --reflog add HEAD reflog from all worktrees
 - files-backend: make reflog iterator go through per-worktree reflog
 - revision.c: --all adds HEAD from all worktrees
 - refs: remove dead for_each_*_submodule()
 - revision.c: use refs_for_each*() instead of for_each_*_submodule()
 - refs: add refs_head_ref()
 - refs: move submodule slash stripping code to get_submodule_ref_store
 - refs.c: refactor get_submodule_ref_store(), share common free block
 - revision.c: --indexed-objects add objects from all worktrees
 - revision.c: refactor add_index_objects_to_pending()
 - revision.h: new flag in struct rev_info wrt. worktree-related refs

 "git gc" and friends when multiple worktrees are used off of a
 single repository did not consider the index and per-worktree refs
 of other worktrees as the root for reachability traversal, making
 objects that are in use only in other worktrees to be subject to
 garbage collection.

 Expecting a reroll.
 Has been waiting for nd/worktree-kill-parse-ref to settle.
 cf. <CACsJy8ADCVBiLoPg_Tz0L6CMdh_eFmK4RYzfQ-PmUgBK7w9e=A@mail.gmail.com>


--------------------------------------------------
[Discard]

* ab/compat-regex-update (2017-05-12) 3 commits
 . DONTMERGE compat/regex: make it compile with -Werror=int-to-pointer-cast
 . compat/regex: update the gawk regex engine from upstream
 . compat/regex: add a README with a maintenance guide

 Update compat/regex we borrowed from gawk.  It seems that some
 customizations we made to the older one were dropped by mistake.

 Will discard.
 cf. <CACBZZX4UUwzRQmyH8joYaqHnuVTjVtGBHp+iZKcnAnwoM_ZJhg@mail.gmail.com>

Re: What's cooking in git.git (May 2017, #06; Mon, 22)

[Jonathan Nieder]

Hi,

Junio C Hamano wrote:

> * bw/forking-and-threading (2017-05-15) 14 commits
>  - usage.c: drop set_error_handle()
>  - run-command: restrict PATH search to executable files
>  - run-command: expose is_executable function
>  - run-command: block signals between fork and execve
>  - run-command: add note about forking and threading
>  - run-command: handle dup2 and close errors in child
>  - run-command: eliminate calls to error handling functions in child
>  - run-command: don't die in child when duping /dev/null
>  - run-command: prepare child environment before forking
>  - string-list: add string_list_remove function
>  - run-command: use the async-signal-safe execv instead of execvp
>  - run-command: prepare command before forking
>  - t0061: run_command executes scripts without a #! line
>  - t5550: use write_script to generate post-update hook
>
>  The "run-command" API implementation has been made more robust
>  against dead-locking in a threaded environment.
>
>  Will merge to 'next'.

What's holding this up?  The deadlock it fixed was a real,
non-theoretical issue.

Thanks,
Jonathan

Re: What's cooking in git.git (May 2017, #06; Mon, 22)

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Junio C Hamano wrote:
>
>> * bw/forking-and-threading (2017-05-15) 14 commits
>>  - usage.c: drop set_error_handle()
>>  - run-command: restrict PATH search to executable files
>>  - run-command: expose is_executable function
>>  - run-command: block signals between fork and execve
>>  - run-command: add note about forking and threading
>>  - run-command: handle dup2 and close errors in child
>>  - run-command: eliminate calls to error handling functions in child
>>  - run-command: don't die in child when duping /dev/null
>>  - run-command: prepare child environment before forking
>>  - string-list: add string_list_remove function
>>  - run-command: use the async-signal-safe execv instead of execvp
>>  - run-command: prepare command before forking
>>  - t0061: run_command executes scripts without a #! line
>>  - t5550: use write_script to generate post-update hook
>>
>>  The "run-command" API implementation has been made more robust
>>  against dead-locking in a threaded environment.
>>
>>  Will merge to 'next'.
>
> What's holding this up?  The deadlock it fixed was a real,
> non-theoretical issue.

Does "Being down-sick over the weekend" count?

Re: What's cooking in git.git (May 2017, #06; Mon, 22)

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
>> Junio C Hamano wrote:

>>> * bw/forking-and-threading (2017-05-15) 14 commits
[...]
>>>  The "run-command" API implementation has been made more robust
>>>  against dead-locking in a threaded environment.
>>>
>>>  Will merge to 'next'.
>>
>> What's holding this up?  The deadlock it fixed was a real,
>> non-theoretical issue.
>
> Does "Being down-sick over the weekend" count?

Yes.  Sorry, I should have asked in a more productive way.

I applied the patches locally to ensure tests continue to pass
reproducibly but was worried because the patches had been ejected from
"next" and had been in "Will merge" state for the last couple of
pushes.

Your health is more important.  I hope you get well soon.  Sorry
again, and thanks for looking them over.  

Sincerely,
Jonathan

Re: What's cooking in git.git (May 2017, #06; Mon, 22)

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Junio C Hamano wrote:
>> Jonathan Nieder <jrnieder@gmail.com> writes:
>>> Junio C Hamano wrote:
>
>>>> * bw/forking-and-threading (2017-05-15) 14 commits
> [...]
>>>>  The "run-command" API implementation has been made more robust
>>>>  against dead-locking in a threaded environment.
>>>>
>>>>  Will merge to 'next'.
>>>
>>> What's holding this up?  The deadlock it fixed was a real,
>>> non-theoretical issue.
>>
>> Does "Being down-sick over the weekend" count?
>
> Yes.  Sorry, I should have asked in a more productive way.
>
> I applied the patches locally to ensure tests continue to pass
> reproducibly but was worried because the patches had been ejected from
> "next" and had been in "Will merge" state for the last couple of
> pushes.

There also is "which ones should go first to 'master'".  Regression
fixes want to go earlier than fixes to age-old issues, for example.

In any case, today was my "move them to 'next'" day (I aim to have
approximately 2 such days a week), so quite a few things will now be
in 'next' when today's integration cycle finishes.

Re: What's cooking in git.git (May 2017, #06; Mon, 22)

[Duy Nguyen]

On Mon, May 22, 2017 at 1:11 PM, Junio C Hamano <gitster@pobox.com> wrote:
> * nd/fopen-errors (2017-05-09) 23 commits
>  - t1308: add a test case on open a config directory
>  - config.c: handle error on failing to fopen()
>  - xdiff-interface.c: report errno on failure to stat() or fopen()
>  - wt-status.c: report error on failure to fopen()
>  - server-info: report error on failure to fopen()
>  - sequencer.c: report error on failure to fopen()
>  - rerere.c: report correct errno
>  - rerere.c: report error on failure to fopen()
>  - remote.c: report error on failure to fopen()
>  - commit.c: report error on failure to fopen() the graft file
>  - log: fix memory leak in open_next_file()
>  - log: report errno on failure to fopen() a file
>  - blame: report error on open if graft_file is a directory
>  - bisect: report on fopen() error
>  - ident.c: use fopen_or_warn()
>  - attr.c: use fopen_or_warn()
>  - wrapper.c: add fopen_or_warn()
>  - wrapper.c: add warn_on_fopen_errors()
>  - config.mak.uname: set FREAD_READS_DIRECTORIES for Darwin, too
>  - config.mak.uname: set FREAD_READS_DIRECTORIES for Linux and FreeBSD
>  - clone: use xfopen() instead of fopen()
>  - Use xfopen() in more places
>  - git_fopen: fix a sparse 'not declared' warning
>
>  We often try to open a file for reading whose existence is
>  optional, and silently ignore errors from open/fopen; report such
>  errors if they are not due to missing files.

If anyone wants to continue this, I've cleaned up the series based on
Johannes comments here [1]. It does not have the Darwin change though.
There was the last question about the '*' test change in ref path and
what exact code change causes that, which I can't answer because I
don't have Windows, or the time to simulate and pinpoint the fault on
Linux.

[1] https://github.com/pclouds/git/commits/fopen-or-warn
-- 
Duy

Re: What's cooking in git.git (May 2017, #06; Mon, 22)

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Mon, May 22, 2017 at 1:11 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> * nd/fopen-errors (2017-05-09) 23 commits
>>  - t1308: add a test case on open a config directory
>>  - config.c: handle error on failing to fopen()
>>  - xdiff-interface.c: report errno on failure to stat() or fopen()
>>  - wt-status.c: report error on failure to fopen()
>>  - server-info: report error on failure to fopen()
>>  - sequencer.c: report error on failure to fopen()
>>  - rerere.c: report correct errno
>>  - rerere.c: report error on failure to fopen()
>>  - remote.c: report error on failure to fopen()
>>  - commit.c: report error on failure to fopen() the graft file
>>  - log: fix memory leak in open_next_file()
>>  - log: report errno on failure to fopen() a file
>>  - blame: report error on open if graft_file is a directory
>>  - bisect: report on fopen() error
>>  - ident.c: use fopen_or_warn()
>>  - attr.c: use fopen_or_warn()
>>  - wrapper.c: add fopen_or_warn()
>>  - wrapper.c: add warn_on_fopen_errors()
>>  - config.mak.uname: set FREAD_READS_DIRECTORIES for Darwin, too
>>  - config.mak.uname: set FREAD_READS_DIRECTORIES for Linux and FreeBSD
>>  - clone: use xfopen() instead of fopen()
>>  - Use xfopen() in more places
>>  - git_fopen: fix a sparse 'not declared' warning
>>
>>  We often try to open a file for reading whose existence is
>>  optional, and silently ignore errors from open/fopen; report such
>>  errors if they are not due to missing files.
>
> If anyone wants to continue this, I've cleaned up the series based on
> Johannes comments here [1]. It does not have the Darwin change though.

Also it seems to be missing the SUPPRESS_FOPEN_REDEF thing by
Ramsay.  I'll mix these two in, post to the list for review and
requeue.

Thanks.


> There was the last question about the '*' test change in ref path and
> what exact code change causes that, which I can't answer because I
> don't have Windows, or the time to simulate and pinpoint the fault on
> Linux.
>
> [1] https://github.com/pclouds/git/commits/fopen-or-warn

[PATCH v3 00/13] reporting unexpected errors after (f)open

[Junio C Hamano]

These are taken from https://github.com/pclouds/git/commits/fopen-or-warn
cf. <CACsJy8CzgHc=qe5w=FGJJ=ZU0a+JiqBrjWHV7SH3rAPKmOOKoA@mail.gmail.com>
with a few bits salvaged from its v2 version.


Ramsay Jones (1):
  git_fopen: fix a sparse 'not declared' warning

Nguyễn Thái Ngọc Duy (9):
  use xfopen() in more places
  clone: use xfopen() instead of fopen()
  config.mak.uname: set FREAD_READS_DIRECTORIES for Linux and FreeBSD
  wrapper.c: add and use warn_on_fopen_errors()
  wrapper.c: add and use fopen_or_warn()
  wrapper.c: make warn_on_inaccessible() static
  print errno when reporting a system call error
  rerere.c: move error_errno() closer to the source system call
  log: fix memory leak in open_next_file()

Junio C Hamano (3):
  config.mak.uname: set FREAD_READS_DIRECTORIES for Darwin, too
  wrapper: factor out is_missing_file_error()
  is_missing_file_error(): work around EINVAL on Windows

Patches 12 & 13 are my attempt to answer the issue the series had on
Windows.
cf. <xmqq37bseddy.fsf@gitster.mtv.corp.google.com>


 attr.c                |  7 ++-----
 bisect.c              |  7 ++-----
 builtin/am.c          |  8 ++------
 builtin/blame.c       |  2 +-
 builtin/clone.c       |  2 +-
 builtin/commit.c      |  5 +----
 builtin/fast-export.c |  4 +---
 builtin/fsck.c        |  3 +--
 builtin/log.c         | 11 ++++++++---
 builtin/merge.c       |  4 +---
 builtin/pull.c        |  3 +--
 commit.c              |  2 +-
 compat/fopen.c        |  4 ++--
 config.c              |  5 ++++-
 config.mak.uname      |  4 ++++
 diff.c                |  8 ++------
 dir.c                 |  6 +++---
 fast-import.c         |  4 +---
 git-compat-util.h     | 15 +++++++++------
 ident.c               |  8 +++-----
 remote-testsvn.c      |  8 ++------
 remote.c              |  4 ++--
 rerere.c              |  7 ++++---
 sequencer.c           |  8 ++++----
 server-info.c         |  2 +-
 t/t1308-config-set.sh | 13 ++++++++++++-
 t/t5512-ls-remote.sh  | 13 ++++++++++---
 wrapper.c             | 49 ++++++++++++++++++++++++++++++++++++++++++++-----
 wt-status.c           |  3 ++-
 xdiff-interface.c     |  4 ++--
 30 files changed, 133 insertions(+), 90 deletions(-)

-- 
2.13.0-491-g71cfeddc25

[PATCH v3 01/13] git_fopen: fix a sparse 'not declared' warning

[Junio C Hamano]

From: Ramsay Jones <ramsay@ramsayjones.plus.com>

If git is built with the FREAD_READS_DIRECTORIES build variable set, this
would cause sparse to issue a 'not declared, should it be static?' warning
on Linux. This is a result of the method employed by 'compat/fopen.c' to
suppress the (possible) redefinition of the (system) fopen macro, which
also removes the extern declaration of the git_fopen function.

In order to suppress the warning, introduce a new macro to suppress the
definition (or possibly the re-definition) of the fopen symbol as a macro
override. This new macro (SUPPRESS_FOPEN_REDEFINITION) is only defined in
'compat/fopen.c', just prior to the inclusion of the 'git-compat-util.h'
header file.

Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 compat/fopen.c    |  4 ++--
 git-compat-util.h | 10 ++++++----
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/compat/fopen.c b/compat/fopen.c
index b5ca142fed..107b3e8182 100644
--- a/compat/fopen.c
+++ b/compat/fopen.c
@@ -1,14 +1,14 @@
 /*
  *  The order of the following two lines is important.
  *
- *  FREAD_READS_DIRECTORIES is undefined before including git-compat-util.h
+ *  SUPPRESS_FOPEN_REDEFINITION is defined before including git-compat-util.h
  *  to avoid the redefinition of fopen within git-compat-util.h. This is
  *  necessary since fopen is a macro on some platforms which may be set
  *  based on compiler options. For example, on AIX fopen is set to fopen64
  *  when _LARGE_FILES is defined. The previous technique of merely undefining
  *  fopen after including git-compat-util.h is inadequate in this case.
  */
-#undef FREAD_READS_DIRECTORIES
+#define SUPPRESS_FOPEN_REDEFINITION
 #include "../git-compat-util.h"
 
 FILE *git_fopen(const char *path, const char *mode)
diff --git a/git-compat-util.h b/git-compat-util.h
index bd04564a69..6be55cf8b3 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -689,10 +689,12 @@ char *gitstrdup(const char *s);
 #endif
 
 #ifdef FREAD_READS_DIRECTORIES
-#ifdef fopen
-#undef fopen
-#endif
-#define fopen(a,b) git_fopen(a,b)
+# if !defined(SUPPRESS_FOPEN_REDEFINITION)
+#  ifdef fopen
+#   undef fopen
+#  endif
+#  define fopen(a,b) git_fopen(a,b)
+# endif
 extern FILE *git_fopen(const char*, const char*);
 #endif
 
-- 
2.13.0-491-g71cfeddc25

[PATCH v3 02/13] use xfopen() in more places

[Junio C Hamano]

From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

xfopen()

 - provides error details
 - explains error on reading, or writing, or whatever operation
 - has l10n support
 - prints file name in the error

Some of these are missing in the places that are replaced with xfopen(),
which is a clear win. In some other places, it's just less code (not as
clearly a win as the previous case but still is).

The only slight regresssion is in remote-testsvn, where we don't report
the file class (marks files) in the error messages anymore. But since
this is a _test_ svn remote transport, I'm not too concerned.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 bisect.c              | 5 +----
 builtin/am.c          | 8 ++------
 builtin/commit.c      | 5 +----
 builtin/fast-export.c | 4 +---
 builtin/fsck.c        | 3 +--
 builtin/merge.c       | 4 +---
 builtin/pull.c        | 3 +--
 diff.c                | 8 ++------
 fast-import.c         | 4 +---
 remote-testsvn.c      | 8 ++------
 10 files changed, 13 insertions(+), 39 deletions(-)

diff --git a/bisect.c b/bisect.c
index 08c9fb7266..469a3e9061 100644
--- a/bisect.c
+++ b/bisect.c
@@ -438,10 +438,7 @@ static void read_bisect_paths(struct argv_array *array)
 {
 	struct strbuf str = STRBUF_INIT;
 	const char *filename = git_path_bisect_names();
-	FILE *fp = fopen(filename, "r");
-
-	if (!fp)
-		die_errno(_("Could not open file '%s'"), filename);
+	FILE *fp = xfopen(filename, "r");
 
 	while (strbuf_getline_lf(&str, fp) != EOF) {
 		strbuf_trim(&str);
diff --git a/builtin/am.c b/builtin/am.c
index a95dd8b4e6..f5dac7783e 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1275,12 +1275,8 @@ static int parse_mail(struct am_state *state, const char *mail)
 		die("BUG: invalid value for state->scissors");
 	}
 
-	mi.input = fopen(mail, "r");
-	if (!mi.input)
-		die("could not open input");
-	mi.output = fopen(am_path(state, "info"), "w");
-	if (!mi.output)
-		die("could not open output 'info'");
+	mi.input = xfopen(mail, "r");
+	mi.output = xfopen(am_path(state, "info"), "w");
 	if (mailinfo(&mi, am_path(state, "msg"), am_path(state, "patch")))
 		die("could not parse patch");
 
diff --git a/builtin/commit.c b/builtin/commit.c
index 1d805f5da8..eda0d32311 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1695,10 +1695,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		if (!reflog_msg)
 			reflog_msg = "commit (merge)";
 		pptr = commit_list_append(current_head, pptr);
-		fp = fopen(git_path_merge_head(), "r");
-		if (fp == NULL)
-			die_errno(_("could not open '%s' for reading"),
-				  git_path_merge_head());
+		fp = xfopen(git_path_merge_head(), "r");
 		while (strbuf_getline_lf(&m, fp) != EOF) {
 			struct commit *parent;
 
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index e0220630d0..128b99e6da 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -905,9 +905,7 @@ static void export_marks(char *file)
 static void import_marks(char *input_file)
 {
 	char line[512];
-	FILE *f = fopen(input_file, "r");
-	if (!f)
-		die_errno("cannot read '%s'", input_file);
+	FILE *f = xfopen(input_file, "r");
 
 	while (fgets(line, sizeof(line), f)) {
 		uint32_t mark;
diff --git a/builtin/fsck.c b/builtin/fsck.c
index b5e13a4556..00beaaa4e6 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -280,8 +280,7 @@ static void check_unreachable_object(struct object *obj)
 				free(filename);
 				return;
 			}
-			if (!(f = fopen(filename, "w")))
-				die_errno("Could not open '%s'", filename);
+			f = xfopen(filename, "w");
 			if (obj->type == OBJ_BLOB) {
 				if (stream_blob_to_fd(fileno(f), &obj->oid, NULL, 1))
 					die_errno("Could not write '%s'", filename);
diff --git a/builtin/merge.c b/builtin/merge.c
index 703827f006..65a1501858 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -839,9 +839,7 @@ static int suggest_conflicts(void)
 	struct strbuf msgbuf = STRBUF_INIT;
 
 	filename = git_path_merge_msg();
-	fp = fopen(filename, "a");
-	if (!fp)
-		die_errno(_("Could not open '%s' for writing"), filename);
+	fp = xfopen(filename, "a");
 
 	append_conflicts_hint(&msgbuf);
 	fputs(msgbuf.buf, fp);
diff --git a/builtin/pull.c b/builtin/pull.c
index dd1a4a94e4..589c25becf 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -337,8 +337,7 @@ static void get_merge_heads(struct oid_array *merge_heads)
 	struct strbuf sb = STRBUF_INIT;
 	struct object_id oid;
 
-	if (!(fp = fopen(filename, "r")))
-		die_errno(_("could not open '%s' for reading"), filename);
+	fp = xfopen(filename, "r");
 	while (strbuf_getline_lf(&sb, fp) != EOF) {
 		if (get_oid_hex(sb.buf, &oid))
 			continue;  /* invalid line: does not start with SHA1 */
diff --git a/diff.c b/diff.c
index 11eef1c85d..b6597ce568 100644
--- a/diff.c
+++ b/diff.c
@@ -4071,9 +4071,7 @@ int diff_opt_parse(struct diff_options *options,
 		DIFF_OPT_CLR(options, FUNCCONTEXT);
 	else if ((argcount = parse_long_opt("output", av, &optarg))) {
 		char *path = prefix_filename(prefix, optarg);
-		options->file = fopen(path, "w");
-		if (!options->file)
-			die_errno("Could not open '%s'", path);
+		options->file = xfopen(path, "w");
 		options->close_file = 1;
 		if (options->use_color != GIT_COLOR_ALWAYS)
 			options->use_color = GIT_COLOR_NEVER;
@@ -4807,9 +4805,7 @@ void diff_flush(struct diff_options *options)
 		 */
 		if (options->close_file)
 			fclose(options->file);
-		options->file = fopen("/dev/null", "w");
-		if (!options->file)
-			die_errno("Could not open /dev/null");
+		options->file = xfopen("/dev/null", "w");
 		options->close_file = 1;
 		for (i = 0; i < q->nr; i++) {
 			struct diff_filepair *p = q->queue[i];
diff --git a/fast-import.c b/fast-import.c
index cf58f875b8..420b3a00d3 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -3274,9 +3274,7 @@ static void option_export_pack_edges(const char *edges)
 {
 	if (pack_edges)
 		fclose(pack_edges);
-	pack_edges = fopen(edges, "a");
-	if (!pack_edges)
-		die_errno("Cannot open '%s'", edges);
+	pack_edges = xfopen(edges, "a");
 }
 
 static int parse_one_option(const char *option)
diff --git a/remote-testsvn.c b/remote-testsvn.c
index f87bf851ba..50404ef343 100644
--- a/remote-testsvn.c
+++ b/remote-testsvn.c
@@ -124,10 +124,8 @@ static int note2mark_cb(const unsigned char *object_sha1,
 static void regenerate_marks(void)
 {
 	int ret;
-	FILE *marksfile = fopen(marksfilename, "w+");
+	FILE *marksfile = xfopen(marksfilename, "w+");
 
-	if (!marksfile)
-		die_errno("Couldn't create mark file %s.", marksfilename);
 	ret = for_each_note(NULL, 0, note2mark_cb, marksfile);
 	if (ret)
 		die("Regeneration of marks failed, returned %d.", ret);
@@ -148,9 +146,7 @@ static void check_or_regenerate_marks(int latestrev)
 	marksfile = fopen(marksfilename, "r");
 	if (!marksfile) {
 		regenerate_marks();
-		marksfile = fopen(marksfilename, "r");
-		if (!marksfile)
-			die_errno("cannot read marks file %s!", marksfilename);
+		marksfile = xfopen(marksfilename, "r");
 		fclose(marksfile);
 	} else {
 		strbuf_addf(&sb, ":%d ", latestrev);
-- 
2.13.0-491-g71cfeddc25

[PATCH v3 03/13] clone: use xfopen() instead of fopen()

[Junio C Hamano]

From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

copy_alternates() called fopen() without handling errors. By switching
to xfopen(), this bug is fixed.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/clone.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index de85b85254..dde4fe73af 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -357,7 +357,7 @@ static void copy_alternates(struct strbuf *src, struct strbuf *dst,
 	 * to turn entries with paths relative to the original
 	 * absolute, so that they can be used in the new repository.
 	 */
-	FILE *in = fopen(src->buf, "r");
+	FILE *in = xfopen(src->buf, "r");
 	struct strbuf line = STRBUF_INIT;
 
 	while (strbuf_getline(&line, in) != EOF) {
-- 
2.13.0-491-g71cfeddc25

[PATCH v3 04/13] config.mak.uname: set FREAD_READS_DIRECTORIES for Linux and FreeBSD

[Junio C Hamano]

From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

This variable is added [1] with the assumption that on a sane system,
fopen(<dir>, "r") should return NULL. Linux and FreeBSD do not meet this
expectation while at least Windows and AIX do. Let's make sure they
behave the same way.

I only tested one version on Linux (4.7.0 with glibc 2.22) and
FreeBSD (11.0) but since GNU/kFreeBSD is fbsd kernel with gnu userspace,
I'm pretty sure it shares the same problem.

[1] cba22528fa (Add compat/fopen.c which returns NULL on attempt to open
    directory - 2008-02-08)

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 config.mak.uname      | 3 +++
 t/t1308-config-set.sh | 8 ++++++++
 2 files changed, 11 insertions(+)

diff --git a/config.mak.uname b/config.mak.uname
index 399fe19271..a25ffddb3e 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -36,6 +36,7 @@ ifeq ($(uname_S),Linux)
 	NEEDS_LIBRT = YesPlease
 	HAVE_GETDELIM = YesPlease
 	SANE_TEXT_GREP=-a
+	FREAD_READS_DIRECTORIES = UnfortunatelyYes
 endif
 ifeq ($(uname_S),GNU/kFreeBSD)
 	HAVE_ALLOCA_H = YesPlease
@@ -43,6 +44,7 @@ ifeq ($(uname_S),GNU/kFreeBSD)
 	HAVE_PATHS_H = YesPlease
 	DIR_HAS_BSD_GROUP_SEMANTICS = YesPlease
 	LIBC_CONTAINS_LIBINTL = YesPlease
+	FREAD_READS_DIRECTORIES = UnfortunatelyYes
 endif
 ifeq ($(uname_S),UnixWare)
 	CC = cc
@@ -201,6 +203,7 @@ ifeq ($(uname_S),FreeBSD)
 	GMTIME_UNRELIABLE_ERRORS = UnfortunatelyYes
 	HAVE_BSD_SYSCTL = YesPlease
 	PAGER_ENV = LESS=FRX LV=-c MORE=FRX
+	FREAD_READS_DIRECTORIES = UnfortunatelyYes
 endif
 ifeq ($(uname_S),OpenBSD)
 	NO_STRCASESTR = YesPlease
diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
index ff50960cca..72d5f1f931 100755
--- a/t/t1308-config-set.sh
+++ b/t/t1308-config-set.sh
@@ -183,6 +183,14 @@ test_expect_success 'proper error on non-existent files' '
 	test_cmp expect actual
 '
 
+test_expect_success 'proper error on directory "files"' '
+	echo "Error (-1) reading configuration file a-directory." >expect &&
+	mkdir a-directory &&
+	test_expect_code 2 test-config configset_get_value foo.bar a-directory 2>output &&
+	grep "^Error" output >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success POSIXPERM,SANITY 'proper error on non-accessible files' '
 	chmod -r .git/config &&
 	test_when_finished "chmod +r .git/config" &&
-- 
2.13.0-491-g71cfeddc25

[PATCH v3 05/13] config.mak.uname: set FREAD_READS_DIRECTORIES for Darwin, too

[Junio C Hamano]

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 config.mak.uname | 1 +
 1 file changed, 1 insertion(+)

diff --git a/config.mak.uname b/config.mak.uname
index a25ffddb3e..1743890164 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -110,6 +110,7 @@ ifeq ($(uname_S),Darwin)
 	BASIC_CFLAGS += -DPRECOMPOSE_UNICODE
 	BASIC_CFLAGS += -DPROTECT_HFS_DEFAULT=1
 	HAVE_BSD_SYSCTL = YesPlease
+	FREAD_READS_DIRECTORIES = UnfortunatelyYes
 endif
 ifeq ($(uname_S),SunOS)
 	NEEDS_SOCKET = YesPlease
-- 
2.13.0-491-g71cfeddc25

[PATCH v3 06/13] wrapper.c: add and use warn_on_fopen_errors()

[Junio C Hamano]

From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

In many places, Git warns about an inaccessible file after a fopen()
failed. To discern these cases from other cases where we want to warn
about inaccessible files, introduce a new helper specifically to test
whether fopen() failed because the current user lacks the permission to
open file in question.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 config.c              |  3 +++
 dir.c                 |  6 +++---
 git-compat-util.h     |  2 ++
 t/t1308-config-set.sh |  3 ++-
 wrapper.c             | 10 ++++++++++
 5 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/config.c b/config.c
index b4a3205da3..2894fbb6d0 100644
--- a/config.c
+++ b/config.c
@@ -2640,6 +2640,9 @@ int git_config_rename_section_in_file(const char *config_filename,
 	}
 
 	if (!(config_file = fopen(config_filename, "rb"))) {
+		ret = warn_on_fopen_errors(config_filename);
+		if (ret)
+			goto out;
 		/* no config file means nothing to rename, no error */
 		goto commit_and_out;
 	}
diff --git a/dir.c b/dir.c
index f451bfa48c..be616e884e 100644
--- a/dir.c
+++ b/dir.c
@@ -745,9 +745,9 @@ static int add_excludes(const char *fname, const char *base, int baselen,
 
 	fd = open(fname, O_RDONLY);
 	if (fd < 0 || fstat(fd, &st) < 0) {
-		if (errno != ENOENT)
-			warn_on_inaccessible(fname);
-		if (0 <= fd)
+		if (fd < 0)
+			warn_on_fopen_errors(fname);
+		else
 			close(fd);
 		if (!check_index ||
 		    (buf = read_skip_worktree_file_from_index(fname, &size, sha1_stat)) == NULL)
diff --git a/git-compat-util.h b/git-compat-util.h
index 6be55cf8b3..eb5c18c7fd 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1101,6 +1101,8 @@ int access_or_die(const char *path, int mode, unsigned flag);
 
 /* Warn on an inaccessible file that ought to be accessible */
 void warn_on_inaccessible(const char *path);
+/* Warn on an inaccessible file if errno indicates this is an error */
+int warn_on_fopen_errors(const char *path);
 
 #ifdef GMTIME_UNRELIABLE_ERRORS
 struct tm *git_gmtime(const time_t *);
diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
index 72d5f1f931..df92fdcd40 100755
--- a/t/t1308-config-set.sh
+++ b/t/t1308-config-set.sh
@@ -195,7 +195,8 @@ test_expect_success POSIXPERM,SANITY 'proper error on non-accessible files' '
 	chmod -r .git/config &&
 	test_when_finished "chmod +r .git/config" &&
 	echo "Error (-1) reading configuration file .git/config." >expect &&
-	test_expect_code 2 test-config configset_get_value foo.bar .git/config 2>actual &&
+	test_expect_code 2 test-config configset_get_value foo.bar .git/config 2>output &&
+	grep "^Error" output >actual &&
 	test_cmp expect actual
 '
 
diff --git a/wrapper.c b/wrapper.c
index d837417709..20c25e7e65 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -418,6 +418,16 @@ FILE *fopen_for_writing(const char *path)
 	return ret;
 }
 
+int warn_on_fopen_errors(const char *path)
+{
+	if (errno != ENOENT && errno != ENOTDIR) {
+		warn_on_inaccessible(path);
+		return -1;
+	}
+
+	return 0;
+}
+
 int xmkstemp(char *template)
 {
 	int fd;
-- 
2.13.0-491-g71cfeddc25

[PATCH v3 07/13] wrapper.c: add and use fopen_or_warn()

[Junio C Hamano]

From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

When fopen() returns NULL, it could be because the given path does not
exist, but it could also be some other errors and the caller has to
check. Add a wrapper so we don't have to repeat the same error check
everywhere.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 attr.c                |  7 ++-----
 bisect.c              |  2 +-
 builtin/blame.c       |  2 +-
 commit.c              |  2 +-
 config.c              |  2 +-
 git-compat-util.h     |  1 +
 ident.c               |  8 +++-----
 remote.c              |  4 ++--
 rerere.c              |  2 +-
 sequencer.c           |  8 ++++----
 server-info.c         |  2 +-
 t/t1308-config-set.sh |  2 ++
 t/t5512-ls-remote.sh  | 13 ++++++++++---
 wrapper.c             | 11 +++++++++++
 wt-status.c           |  3 ++-
 15 files changed, 43 insertions(+), 26 deletions(-)

diff --git a/attr.c b/attr.c
index 7e2134471c..821203e2a9 100644
--- a/attr.c
+++ b/attr.c
@@ -720,16 +720,13 @@ void git_attr_set_direction(enum git_attr_direction new_direction,
 
 static struct attr_stack *read_attr_from_file(const char *path, int macro_ok)
 {
-	FILE *fp = fopen(path, "r");
+	FILE *fp = fopen_or_warn(path, "r");
 	struct attr_stack *res;
 	char buf[2048];
 	int lineno = 0;
 
-	if (!fp) {
-		if (errno != ENOENT && errno != ENOTDIR)
-			warn_on_inaccessible(path);
+	if (!fp)
 		return NULL;
-	}
 	res = xcalloc(1, sizeof(*res));
 	while (fgets(buf, sizeof(buf), fp)) {
 		char *bufp = buf;
diff --git a/bisect.c b/bisect.c
index 469a3e9061..bb28bf63b2 100644
--- a/bisect.c
+++ b/bisect.c
@@ -666,7 +666,7 @@ static int is_expected_rev(const struct object_id *oid)
 	if (stat(filename, &st) || !S_ISREG(st.st_mode))
 		return 0;
 
-	fp = fopen(filename, "r");
+	fp = fopen_or_warn(filename, "r");
 	if (!fp)
 		return 0;
 
diff --git a/builtin/blame.c b/builtin/blame.c
index 07506a3e45..34445d7894 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2071,7 +2071,7 @@ static int prepare_lines(struct scoreboard *sb)
  */
 static int read_ancestry(const char *graft_file)
 {
-	FILE *fp = fopen(graft_file, "r");
+	FILE *fp = fopen_or_warn(graft_file, "r");
 	struct strbuf buf = STRBUF_INIT;
 	if (!fp)
 		return -1;
diff --git a/commit.c b/commit.c
index 73c78c2b80..3eeda081f9 100644
--- a/commit.c
+++ b/commit.c
@@ -167,7 +167,7 @@ struct commit_graft *read_graft_line(char *buf, int len)
 
 static int read_graft_file(const char *graft_file)
 {
-	FILE *fp = fopen(graft_file, "r");
+	FILE *fp = fopen_or_warn(graft_file, "r");
 	struct strbuf buf = STRBUF_INIT;
 	if (!fp)
 		return -1;
diff --git a/config.c b/config.c
index 2894fbb6d0..e54d99d519 100644
--- a/config.c
+++ b/config.c
@@ -1422,7 +1422,7 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)
 	int ret = -1;
 	FILE *f;
 
-	f = fopen(filename, "r");
+	f = fopen_or_warn(filename, "r");
 	if (f) {
 		flockfile(f);
 		ret = do_config_from_file(fn, CONFIG_ORIGIN_FILE, filename, filename, f, data);
diff --git a/git-compat-util.h b/git-compat-util.h
index eb5c18c7fd..f74b401810 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -802,6 +802,7 @@ extern int xmkstemp(char *template);
 extern int xmkstemp_mode(char *template, int mode);
 extern char *xgetcwd(void);
 extern FILE *fopen_for_writing(const char *path);
+extern FILE *fopen_or_warn(const char *path, const char *mode);
 
 #define ALLOC_ARRAY(x, alloc) (x) = xmalloc(st_mult(sizeof(*(x)), (alloc)))
 #define REALLOC_ARRAY(x, alloc) (x) = xrealloc((x), st_mult(sizeof(*(x)), (alloc)))
diff --git a/ident.c b/ident.c
index bea871c8e0..91c7609055 100644
--- a/ident.c
+++ b/ident.c
@@ -72,12 +72,10 @@ static int add_mailname_host(struct strbuf *buf)
 	FILE *mailname;
 	struct strbuf mailnamebuf = STRBUF_INIT;
 
-	mailname = fopen("/etc/mailname", "r");
-	if (!mailname) {
-		if (errno != ENOENT)
-			warning_errno("cannot open /etc/mailname");
+	mailname = fopen_or_warn("/etc/mailname", "r");
+	if (!mailname)
 		return -1;
-	}
+
 	if (strbuf_getline(&mailnamebuf, mailname) == EOF) {
 		if (ferror(mailname))
 			warning_errno("cannot read /etc/mailname");
diff --git a/remote.c b/remote.c
index 801137c72e..1f2453d0f6 100644
--- a/remote.c
+++ b/remote.c
@@ -251,7 +251,7 @@ static const char *skip_spaces(const char *s)
 static void read_remotes_file(struct remote *remote)
 {
 	struct strbuf buf = STRBUF_INIT;
-	FILE *f = fopen(git_path("remotes/%s", remote->name), "r");
+	FILE *f = fopen_or_warn(git_path("remotes/%s", remote->name), "r");
 
 	if (!f)
 		return;
@@ -277,7 +277,7 @@ static void read_branches_file(struct remote *remote)
 {
 	char *frag;
 	struct strbuf buf = STRBUF_INIT;
-	FILE *f = fopen(git_path("branches/%s", remote->name), "r");
+	FILE *f = fopen_or_warn(git_path("branches/%s", remote->name), "r");
 
 	if (!f)
 		return;
diff --git a/rerere.c b/rerere.c
index 3bd55caf3b..971bfedfb2 100644
--- a/rerere.c
+++ b/rerere.c
@@ -200,7 +200,7 @@ static struct rerere_id *new_rerere_id(unsigned char *sha1)
 static void read_rr(struct string_list *rr)
 {
 	struct strbuf buf = STRBUF_INIT;
-	FILE *in = fopen(git_path_merge_rr(), "r");
+	FILE *in = fopen_or_warn(git_path_merge_rr(), "r");
 
 	if (!in)
 		return;
diff --git a/sequencer.c b/sequencer.c
index 10c3b4ff81..11b5c7c114 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -897,8 +897,8 @@ static void flush_rewritten_pending(void) {
 	FILE *out;
 
 	if (strbuf_read_file(&buf, rebase_path_rewritten_pending(), 82) > 0 &&
-			!get_sha1("HEAD", newsha1) &&
-			(out = fopen(rebase_path_rewritten_list(), "a"))) {
+	    !get_sha1("HEAD", newsha1) &&
+	    (out = fopen_or_warn(rebase_path_rewritten_list(), "a"))) {
 		char *bol = buf.buf, *eol;
 
 		while (*bol) {
@@ -917,7 +917,7 @@ static void flush_rewritten_pending(void) {
 
 static void record_in_rewritten(struct object_id *oid,
 		enum todo_command next_command) {
-	FILE *out = fopen(rebase_path_rewritten_pending(), "a");
+	FILE *out = fopen_or_warn(rebase_path_rewritten_pending(), "a");
 
 	if (!out)
 		return;
@@ -1378,7 +1378,7 @@ static int read_populate_todo(struct todo_list *todo_list,
 
 	if (is_rebase_i(opts)) {
 		struct todo_list done = TODO_LIST_INIT;
-		FILE *f = fopen(rebase_path_msgtotal(), "w");
+		FILE *f = fopen_or_warn(rebase_path_msgtotal(), "w");
 
 		if (strbuf_read_file(&done.buf, rebase_path_done(), 0) > 0 &&
 				!parse_insn_buffer(done.buf.buf, &done))
diff --git a/server-info.c b/server-info.c
index f6c1a3dfb0..e01ac154a8 100644
--- a/server-info.c
+++ b/server-info.c
@@ -133,7 +133,7 @@ static int read_pack_info_file(const char *infofile)
 	char line[1000];
 	int old_cnt = 0;
 
-	fp = fopen(infofile, "r");
+	fp = fopen_or_warn(infofile, "r");
 	if (!fp)
 		return 1; /* nonexistent is not an error. */
 
diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
index df92fdcd40..e495a61616 100755
--- a/t/t1308-config-set.sh
+++ b/t/t1308-config-set.sh
@@ -187,6 +187,7 @@ test_expect_success 'proper error on directory "files"' '
 	echo "Error (-1) reading configuration file a-directory." >expect &&
 	mkdir a-directory &&
 	test_expect_code 2 test-config configset_get_value foo.bar a-directory 2>output &&
+	grep "^warning:" output &&
 	grep "^Error" output >actual &&
 	test_cmp expect actual
 '
@@ -196,6 +197,7 @@ test_expect_success POSIXPERM,SANITY 'proper error on non-accessible files' '
 	test_when_finished "chmod +r .git/config" &&
 	echo "Error (-1) reading configuration file .git/config." >expect &&
 	test_expect_code 2 test-config configset_get_value foo.bar .git/config 2>output &&
+	grep "^warning:" output &&
 	grep "^Error" output >actual &&
 	test_cmp expect actual
 '
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 94fc9be9ce..02106c9226 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -85,8 +85,15 @@ test_expect_success 'use branch.<name>.remote if possible' '
 '
 
 test_expect_success 'confuses pattern as remote when no remote specified' '
-	cat >exp <<-\EOF &&
-	fatal: '\''refs*master'\'' does not appear to be a git repository
+	if test_have_prereq MINGW
+	then
+		# Windows does not like asterisks in pathname
+		does_not_exist=master
+	else
+		does_not_exist="refs*master"
+	fi &&
+	cat >exp <<-EOF &&
+	fatal: '\''$does_not_exist'\'' does not appear to be a git repository
 	fatal: Could not read from remote repository.
 
 	Please make sure you have the correct access rights
@@ -98,7 +105,7 @@ test_expect_success 'confuses pattern as remote when no remote specified' '
 	# fetch <branch>.
 	# We could just as easily have used "master"; the "*" emphasizes its
 	# role as a pattern.
-	test_must_fail git ls-remote refs*master >actual 2>&1 &&
+	test_must_fail git ls-remote "$does_not_exist" >actual 2>&1 &&
 	test_i18ncmp exp actual
 '
 
diff --git a/wrapper.c b/wrapper.c
index 20c25e7e65..6e513c904a 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -428,6 +428,17 @@ int warn_on_fopen_errors(const char *path)
 	return 0;
 }
 
+FILE *fopen_or_warn(const char *path, const char *mode)
+{
+	FILE *fp = fopen(path, mode);
+
+	if (fp)
+		return fp;
+
+	warn_on_fopen_errors(path);
+	return NULL;
+}
+
 int xmkstemp(char *template)
 {
 	int fd;
diff --git a/wt-status.c b/wt-status.c
index 0375484962..cdf9f5eed2 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1065,7 +1065,8 @@ static void show_am_in_progress(struct wt_status *s,
 static char *read_line_from_git_path(const char *filename)
 {
 	struct strbuf buf = STRBUF_INIT;
-	FILE *fp = fopen(git_path("%s", filename), "r");
+	FILE *fp = fopen_or_warn(git_path("%s", filename), "r");
+
 	if (!fp) {
 		strbuf_release(&buf);
 		return NULL;
-- 
2.13.0-491-g71cfeddc25

[PATCH v3 08/13] wrapper.c: make warn_on_inaccessible() static

[Junio C Hamano]

From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

After the last patch, this function is not used outside anymore. Keep it
static.

Noticed-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-compat-util.h |  2 --
 wrapper.c         | 10 +++++-----
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index f74b401810..87f47828a5 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1100,8 +1100,6 @@ int remove_or_warn(unsigned int mode, const char *path);
 int access_or_warn(const char *path, int mode, unsigned flag);
 int access_or_die(const char *path, int mode, unsigned flag);
 
-/* Warn on an inaccessible file that ought to be accessible */
-void warn_on_inaccessible(const char *path);
 /* Warn on an inaccessible file if errno indicates this is an error */
 int warn_on_fopen_errors(const char *path);
 
diff --git a/wrapper.c b/wrapper.c
index 6e513c904a..b117eb14a4 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -418,6 +418,11 @@ FILE *fopen_for_writing(const char *path)
 	return ret;
 }
 
+static void warn_on_inaccessible(const char *path)
+{
+	warning_errno(_("unable to access '%s'"), path);
+}
+
 int warn_on_fopen_errors(const char *path)
 {
 	if (errno != ENOENT && errno != ENOTDIR) {
@@ -597,11 +602,6 @@ int remove_or_warn(unsigned int mode, const char *file)
 	return S_ISGITLINK(mode) ? rmdir_or_warn(file) : unlink_or_warn(file);
 }
 
-void warn_on_inaccessible(const char *path)
-{
-	warning_errno(_("unable to access '%s'"), path);
-}
-
 static int access_error_is_ok(int err, unsigned flag)
 {
 	return err == ENOENT || err == ENOTDIR ||
-- 
2.13.0-491-g71cfeddc25

[PATCH v3 09/13] print errno when reporting a system call error

[Junio C Hamano]

From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/log.c     | 3 ++-
 rerere.c          | 4 ++--
 xdiff-interface.c | 4 ++--
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index b3b10cc1ed..26d6a3cf14 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -858,7 +858,8 @@ static int open_next_file(struct commit *commit, const char *subject,
 		printf("%s\n", filename.buf + outdir_offset);
 
 	if ((rev->diffopt.file = fopen(filename.buf, "w")) == NULL)
-		return error(_("Cannot open patch file %s"), filename.buf);
+		return error_errno(_("Cannot open patch file %s"),
+				   filename.buf);
 
 	strbuf_release(&filename);
 	return 0;
diff --git a/rerere.c b/rerere.c
index 971bfedfb2..1351b0c3fb 100644
--- a/rerere.c
+++ b/rerere.c
@@ -484,13 +484,13 @@ static int handle_file(const char *path, unsigned char *sha1, const char *output
 	io.input = fopen(path, "r");
 	io.io.wrerror = 0;
 	if (!io.input)
-		return error("Could not open %s", path);
+		return error_errno("Could not open %s", path);
 
 	if (output) {
 		io.io.output = fopen(output, "w");
 		if (!io.io.output) {
 			fclose(io.input);
-			return error("Could not write %s", output);
+			return error_errno("Could not write %s", output);
 		}
 	}
 
diff --git a/xdiff-interface.c b/xdiff-interface.c
index 060038c2d6..d3f78ca2a7 100644
--- a/xdiff-interface.c
+++ b/xdiff-interface.c
@@ -164,9 +164,9 @@ int read_mmfile(mmfile_t *ptr, const char *filename)
 	size_t sz;
 
 	if (stat(filename, &st))
-		return error("Could not stat %s", filename);
+		return error_errno("Could not stat %s", filename);
 	if ((f = fopen(filename, "rb")) == NULL)
-		return error("Could not open %s", filename);
+		return error_errno("Could not open %s", filename);
 	sz = xsize_t(st.st_size);
 	ptr->ptr = xmalloc(sz ? sz : 1);
 	if (sz && fread(ptr->ptr, sz, 1, f) != 1) {
-- 
2.13.0-491-g71cfeddc25

[PATCH v3 10/13] rerere.c: move error_errno() closer to the source system call

[Junio C Hamano]

From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

We are supposed to report errno from fopen(). fclose() between fopen()
and the report function could either change errno or reset it.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 rerere.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/rerere.c b/rerere.c
index 1351b0c3fb..c26c29f87a 100644
--- a/rerere.c
+++ b/rerere.c
@@ -489,8 +489,9 @@ static int handle_file(const char *path, unsigned char *sha1, const char *output
 	if (output) {
 		io.io.output = fopen(output, "w");
 		if (!io.io.output) {
+			error_errno("Could not write %s", output);
 			fclose(io.input);
-			return error_errno("Could not write %s", output);
+			return -1;
 		}
 	}
 
-- 
2.13.0-491-g71cfeddc25

[PATCH v3 11/13] log: fix memory leak in open_next_file()

[Junio C Hamano]

From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

Noticed-by: Jeff King <peff@peff.net>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/log.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 26d6a3cf14..f075838df9 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -842,8 +842,10 @@ static int open_next_file(struct commit *commit, const char *subject,
 	if (output_directory) {
 		strbuf_addstr(&filename, output_directory);
 		if (filename.len >=
-		    PATH_MAX - FORMAT_PATCH_NAME_MAX - suffix_len)
+		    PATH_MAX - FORMAT_PATCH_NAME_MAX - suffix_len) {
+			strbuf_release(&filename);
 			return error(_("name of output directory is too long"));
+		}
 		strbuf_complete(&filename, '/');
 	}
 
@@ -857,9 +859,11 @@ static int open_next_file(struct commit *commit, const char *subject,
 	if (!quiet)
 		printf("%s\n", filename.buf + outdir_offset);
 
-	if ((rev->diffopt.file = fopen(filename.buf, "w")) == NULL)
-		return error_errno(_("Cannot open patch file %s"),
-				   filename.buf);
+	if ((rev->diffopt.file = fopen(filename.buf, "w")) == NULL) {
+		error_errno(_("Cannot open patch file %s"), filename.buf);
+		strbuf_release(&filename);
+		return -1;
+	}
 
 	strbuf_release(&filename);
 	return 0;
-- 
2.13.0-491-g71cfeddc25

[PATCH v3 12/13] wrapper: factor out is_missing_file_error()

[Junio C Hamano]

Our code often opens a path to an optional file, to work on its
contents when we can successfully open it.  We can ignore a failure
to open if such an optional file does not exist, but we do want to
report a failure in opening for other reasons (e.g. we got an I/O
error, or the file is there, but we lack the permission to open).

There is a logic to determine if an errno left by open/fopen
indicates such an ignorable error.  Split it out into a helper
function.  We may want to make it an extern in later steps, as many
hits from "git grep ENOENT" would instead want to be using the same
logic.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 wrapper.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/wrapper.c b/wrapper.c
index b117eb14a4..f1c87ec7ea 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -423,9 +423,23 @@ static void warn_on_inaccessible(const char *path)
 	warning_errno(_("unable to access '%s'"), path);
 }
 
+/*
+ * Our code often opens a path to an optional file, to work on its
+ * contents when we can successfully open it.  We can ignore a failure
+ * to open if such an optional file does not exist, but we do want to
+ * report a failure in opening for other reasons (e.g. we got an I/O
+ * error, or the file is there, but we lack the permission to open).
+ *
+ * Call this function after seeing an error from open() or fopen() to
+ * see if the errno indicates a missing file that we can safely ignore.
+ */
+static int is_missing_file_error(int errno_) {
+	return (errno_ == ENOENT || errno_ == ENOTDIR);
+}
+
 int warn_on_fopen_errors(const char *path)
 {
-	if (errno != ENOENT && errno != ENOTDIR) {
+	if (!is_missing_file_error(errno)) {
 		warn_on_inaccessible(path);
 		return -1;
 	}
-- 
2.13.0-491-g71cfeddc25

[PATCH v3 13/13] is_missing_file_error(): work around EINVAL on Windows

[Junio C Hamano]

When asked to open/fopen a path, e.g. "a/b:/c", which does not exist
on the filesystem, Windows (correctly) fails to open it but sets
EINVAL to errno because the pathname has characters that cannot be
stored in its filesystem.

As this is an expected failure, teach is_missing_file_error() helper
about this case.

This is RFC, as there may be a case where we get EINVAL from
open/fopen for reasons other than "the filesystem does not like this
pathname" that may be worth reporting to the user, and this change
is sweeping such an error under the rug.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 wrapper.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/wrapper.c b/wrapper.c
index f1c87ec7ea..74aa3b7803 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -434,6 +434,10 @@ static void warn_on_inaccessible(const char *path)
  * see if the errno indicates a missing file that we can safely ignore.
  */
 static int is_missing_file_error(int errno_) {
+#ifdef GIT_WINDOWS_NATIVE
+	if (errno_ == EINVAL)
+		return 1;
+#endif
 	return (errno_ == ENOENT || errno_ == ENOTDIR);
 }
 
-- 
2.13.0-491-g71cfeddc25

[PATCH 1/2] mingw: verify that paths are not mistaken for remote nicknames

[Johannes Sixt]

Am 26.05.2017 um 05:35 schrieb Junio C Hamano:
> When asked to open/fopen a path, e.g. "a/b:/c", which does not exist
> on the filesystem, Windows (correctly) fails to open it but sets
> EINVAL to errno because the pathname has characters that cannot be
> stored in its filesystem.
> 
> As this is an expected failure, teach is_missing_file_error() helper
> about this case.
> 
> This is RFC, as there may be a case where we get EINVAL from
> open/fopen for reasons other than "the filesystem does not like this
> pathname" that may be worth reporting to the user, and this change
> is sweeping such an error under the rug.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>   wrapper.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/wrapper.c b/wrapper.c
> index f1c87ec7ea..74aa3b7803 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -434,6 +434,10 @@ static void warn_on_inaccessible(const char *path)
>    * see if the errno indicates a missing file that we can safely ignore.
>    */
>   static int is_missing_file_error(int errno_) {
> +#ifdef GIT_WINDOWS_NATIVE
> +	if (errno_ == EINVAL)
> +		return 1;
> +#endif
>   	return (errno_ == ENOENT || errno_ == ENOTDIR);
>   }

I would prefer to catch the case in the compatibility layer. Here is
a two patch series that would replace your 12/13 and 13/13.

---- 8< ----
From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Subject: mingw: verify that paths are not mistaken for remote nicknames

This added test case simply verifies that users will not be bothered
with bogus complaints à la

	warning: unable to access '.git/remotes/D:\repo': Invalid argument

when fetching from a Windows path (in this case, D:\repo).

[j6t: mark the new test as test_expect_failure]

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 t/t5580-clone-push-unc.sh | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/t/t5580-clone-push-unc.sh b/t/t5580-clone-push-unc.sh
index b195f71ea9..fd719a209e 100755
--- a/t/t5580-clone-push-unc.sh
+++ b/t/t5580-clone-push-unc.sh
@@ -1,13 +1,19 @@
 #!/bin/sh
 
-test_description='various UNC path tests (Windows-only)'
+test_description='various Windows-only path tests'
 . ./test-lib.sh
 
 if ! test_have_prereq MINGW; then
-	skip_all='skipping UNC path tests, requires Windows'
+	skip_all='skipping Windows-only path tests'
 	test_done
 fi
 
+test_expect_failure 'remote nick cannot contain backslashes' '
+	BACKSLASHED="$(pwd | tr / \\\\)" &&
+	git ls-remote "$BACKSLASHED" >out 2>err &&
+	! grep "unable to access" err
+'
+
 UNCPATH="$(pwd)"
 case "$UNCPATH" in
 [A-Z]:*)
-- 
2.13.0.55.g17b7d13330

[PATCH 2/2] mingw_fopen: report ENOENT for invalid file names

[Johannes Sixt]

On Windows, certain characters are prohibited in file names, most
prominently the colon. When fopen() is called with such an invalid file
name, the underlying Windows API actually reports a particular error,
but since there is no suitable errno value, this error is translated
to EINVAL. Detect the case and report ENOENT instead.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
  compat/mingw.c            | 2 ++
  t/t5580-clone-push-unc.sh | 2 +-
  2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 62109cc4e6..ce6fe8f46b 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -423,6 +423,8 @@ FILE *mingw_fopen (const char *filename, const char 
*otype)
  		return NULL;
  	}
  	file = _wfopen(wfilename, wotype);
+	if (!file && GetLastError() == ERROR_INVALID_NAME)
+		errno = ENOENT;
  	if (file && hide && set_hidden_flag(wfilename, 1))
  		warning("could not mark '%s' as hidden.", filename);
  	return file;
diff --git a/t/t5580-clone-push-unc.sh b/t/t5580-clone-push-unc.sh
index fd719a209e..93ce99ba3c 100755
--- a/t/t5580-clone-push-unc.sh
+++ b/t/t5580-clone-push-unc.sh
@@ -8,7 +8,7 @@ if ! test_have_prereq MINGW; then
  	test_done
  fi

-test_expect_failure 'remote nick cannot contain backslashes' '
+test_expect_success 'remote nick cannot contain backslashes' '
  	BACKSLASHED="$(pwd | tr / \\\\)" &&
  	git ls-remote "$BACKSLASHED" >out 2>err &&
  	! grep "unable to access" err
-- 
2.13.0.55.g17b7d13330

Re: [PATCH 1/2] mingw: verify that paths are not mistaken for remote nicknames

[Ævar Arnfjörð Bjarmason]

On Mon, May 29, 2017 at 10:25 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> Am 26.05.2017 um 05:35 schrieb Junio C Hamano:
>> When asked to open/fopen a path, e.g. "a/b:/c", which does not exist
>> on the filesystem, Windows (correctly) fails to open it but sets
>> EINVAL to errno because the pathname has characters that cannot be
>> stored in its filesystem.
>>
>> As this is an expected failure, teach is_missing_file_error() helper
>> about this case.
>>
>> This is RFC, as there may be a case where we get EINVAL from
>> open/fopen for reasons other than "the filesystem does not like this
>> pathname" that may be worth reporting to the user, and this change
>> is sweeping such an error under the rug.
>>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---
>>   wrapper.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/wrapper.c b/wrapper.c
>> index f1c87ec7ea..74aa3b7803 100644
>> --- a/wrapper.c
>> +++ b/wrapper.c
>> @@ -434,6 +434,10 @@ static void warn_on_inaccessible(const char *path)
>>    * see if the errno indicates a missing file that we can safely ignore.
>>    */
>>   static int is_missing_file_error(int errno_) {
>> +#ifdef GIT_WINDOWS_NATIVE
>> +     if (errno_ == EINVAL)
>> +             return 1;
>> +#endif
>>       return (errno_ == ENOENT || errno_ == ENOTDIR);
>>   }
>
> I would prefer to catch the case in the compatibility layer. Here is
> a two patch series that would replace your 12/13 and 13/13.
>
> ---- 8< ----
> From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> Subject: mingw: verify that paths are not mistaken for remote nicknames
>
> This added test case simply verifies that users will not be bothered
> with bogus complaints à la
>
>         warning: unable to access '.git/remotes/D:\repo': Invalid argument
>
> when fetching from a Windows path (in this case, D:\repo).
>
> [j6t: mark the new test as test_expect_failure]
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
>  t/t5580-clone-push-unc.sh | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/t/t5580-clone-push-unc.sh b/t/t5580-clone-push-unc.sh
> index b195f71ea9..fd719a209e 100755
> --- a/t/t5580-clone-push-unc.sh
> +++ b/t/t5580-clone-push-unc.sh
> @@ -1,13 +1,19 @@
>  #!/bin/sh
>
> -test_description='various UNC path tests (Windows-only)'
> +test_description='various Windows-only path tests'
>  . ./test-lib.sh
>
>  if ! test_have_prereq MINGW; then
> -       skip_all='skipping UNC path tests, requires Windows'
> +       skip_all='skipping Windows-only path tests'
>         test_done
>  fi
>
> +test_expect_failure 'remote nick cannot contain backslashes' '
> +       BACKSLASHED="$(pwd | tr / \\\\)" &&
> +       git ls-remote "$BACKSLASHED" >out 2>err &&
> +       ! grep "unable to access" err
> +'

Doesn't this need test_i18ngrep?:

    wrapper.c:581:  warning_errno(_("unable to access '%s'"), path);
    wrapper.c:602:          die_errno(_("unable to access '%s'"), path);

Or is it one of these, which isn't translated:

    http-push.c:1531:               error("unable to access '%s': %s",
url, curl_errorstr);
    remote-curl.c:319:              die("unable to access '%s': %s",
url.buf, curl_errorstr);


> +
>  UNCPATH="$(pwd)"
>  case "$UNCPATH" in
>  [A-Z]:*)
> --
> 2.13.0.55.g17b7d13330

Re: [PATCH 1/2] mingw: verify that paths are not mistaken for remote nicknames

[Johannes Sixt]

Am 29.05.2017 um 22:40 schrieb Ævar Arnfjörð Bjarmason:
> On Mon, May 29, 2017 at 10:25 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>> diff --git a/t/t5580-clone-push-unc.sh b/t/t5580-clone-push-unc.sh
>> index b195f71ea9..fd719a209e 100755
>> --- a/t/t5580-clone-push-unc.sh
>> +++ b/t/t5580-clone-push-unc.sh
>> @@ -1,13 +1,19 @@
>>   #!/bin/sh
>>
>> -test_description='various UNC path tests (Windows-only)'
>> +test_description='various Windows-only path tests'
>>   . ./test-lib.sh
>>
>>   if ! test_have_prereq MINGW; then
>> -       skip_all='skipping UNC path tests, requires Windows'
>> +       skip_all='skipping Windows-only path tests'
>>          test_done
>>   fi
>>
>> +test_expect_failure 'remote nick cannot contain backslashes' '
>> +       BACKSLASHED="$(pwd | tr / \\\\)" &&
>> +       git ls-remote "$BACKSLASHED" >out 2>err &&
>> +       ! grep "unable to access" err
>> +'
> 
> Doesn't this need test_i18ngrep?:

Good catch! It would be this one in warn_on_inaccessible:

>      wrapper.c:581:  warning_errno(_("unable to access '%s'"), path);

But actually, I'm more worried about the unholy mix of 
one-test-first-then-skip_all-later that occurs in this test script (I do 
not mean the skip_all that is visible in the context, there are others 
later). I think there was some buzz recently that prove only understands 
a summary line that reads "1..0", but here we would see "1..1". What to 
do? Reorganize the test script? Dscho, any ideas?

-- Hannes

Re: [PATCH 1/2] mingw: verify that paths are not mistaken for remote nicknames

[Ramsay Jones]

On 29/05/17 22:02, Johannes Sixt wrote:
> Am 29.05.2017 um 22:40 schrieb Ævar Arnfjörð Bjarmason:
>> On Mon, May 29, 2017 at 10:25 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>>> diff --git a/t/t5580-clone-push-unc.sh b/t/t5580-clone-push-unc.sh
>>> index b195f71ea9..fd719a209e 100755
>>> --- a/t/t5580-clone-push-unc.sh
>>> +++ b/t/t5580-clone-push-unc.sh
>>> @@ -1,13 +1,19 @@
>>>   #!/bin/sh
>>>
>>> -test_description='various UNC path tests (Windows-only)'
>>> +test_description='various Windows-only path tests'
>>>   . ./test-lib.sh
>>>
>>>   if ! test_have_prereq MINGW; then
>>> -       skip_all='skipping UNC path tests, requires Windows'
>>> +       skip_all='skipping Windows-only path tests'
>>>          test_done
>>>   fi
>>>
>>> +test_expect_failure 'remote nick cannot contain backslashes' '
>>> +       BACKSLASHED="$(pwd | tr / \\\\)" &&
>>> +       git ls-remote "$BACKSLASHED" >out 2>err &&
>>> +       ! grep "unable to access" err
>>> +'
>>
>> Doesn't this need test_i18ngrep?:
> 
> Good catch! It would be this one in warn_on_inaccessible:
> 
>>      wrapper.c:581:  warning_errno(_("unable to access '%s'"), path);
> 
> But actually, I'm more worried about the unholy mix of one-test-first-then-skip_all-later that occurs in this test script (I do not mean the skip_all that is visible in the context, there are others later). I think there was some buzz recently that prove only understands a summary line that reads "1..0", but here we would see "1..1". What to do? Reorganize the test script? Dscho, any ideas?

See commit c7018be509 ("test: allow skipping the remainder", 18-05-2017)
which is currently merged to the 'next' branch (merge 03b8a61e47 of the
'jc/skip-test-in-the-middle' branch).

(see also http://testanything.org)

If you look at http://testanything.org//tap-specification.html, it shows
that you are allowed to annotate a plan of '1..0' with a SKIP directive
to explain why no tests in a file were run. However, a plan with '1..n'
(for any n > 0) must not have any annotation. (Back in 2012, when I wrote
commit bf4b721932, I found much better documentation than the above!)

So, after commit c7018be509, you can now use the 'skip_all' facility
after having run some tests; it now converts that into an 'SKIP comment'
just before the test plan, effectively skipping the remainder of the
tests in the file. (since we are using a 'trailing plan', and have thus
not declared how many tests we had intended to run, we can output an
accurate plan).

ATB,
Ramsay Jones

Re: [PATCH 1/2] mingw: verify that paths are not mistaken for remote nicknames

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> Am 29.05.2017 um 22:40 schrieb Ævar Arnfjörð Bjarmason:
>> On Mon, May 29, 2017 at 10:25 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>>> diff --git a/t/t5580-clone-push-unc.sh b/t/t5580-clone-push-unc.sh
>>> index b195f71ea9..fd719a209e 100755
>>> --- a/t/t5580-clone-push-unc.sh
>>> +++ b/t/t5580-clone-push-unc.sh
>>> @@ -1,13 +1,19 @@
>>>   #!/bin/sh
>>>
>>> -test_description='various UNC path tests (Windows-only)'
>>> +test_description='various Windows-only path tests'
>>>   . ./test-lib.sh
>>>
>>>   if ! test_have_prereq MINGW; then
>>> -       skip_all='skipping UNC path tests, requires Windows'
>>> +       skip_all='skipping Windows-only path tests'
>>>          test_done
>>>   fi
>>>
>>> +test_expect_failure 'remote nick cannot contain backslashes' '
>>> +       BACKSLASHED="$(pwd | tr / \\\\)" &&
>>> +       git ls-remote "$BACKSLASHED" >out 2>err &&
>>> +       ! grep "unable to access" err
>>> +'
>>
>> Doesn't this need test_i18ngrep?:
>
> Good catch! It would be this one in warn_on_inaccessible:
>
>>      wrapper.c:581:  warning_errno(_("unable to access '%s'"), path);
>
> But actually, I'm more worried about the unholy mix of
> one-test-first-then-skip_all-later that occurs in this test script (I
> do not mean the skip_all that is visible in the context, there are
> others later). I think there was some buzz recently that prove only
> understands a summary line that reads "1..0", but here we would see
> "1..1". What to do? Reorganize the test script? Dscho, any ideas?

Put this new test after the other skip_all/test_done and you'd be
fine, I think.  It should come after the "setup" test anyway, no?

Re: [PATCH 1/2] mingw: verify that paths are not mistaken for remote nicknames

[Junio C Hamano]

Ramsay Jones <ramsay@ramsayjones.plus.com> writes:

> See commit c7018be509 ("test: allow skipping the remainder", 18-05-2017)
> which is currently merged to the 'next' branch (merge 03b8a61e47 of the
> 'jc/skip-test-in-the-middle' branch).
>
> (see also http://testanything.org)
>
> If you look at http://testanything.org//tap-specification.html, it shows
> that you are allowed to annotate a plan of '1..0' with a SKIP directive
> to explain why no tests in a file were run. However, a plan with '1..n'
> (for any n > 0) must not have any annotation. (Back in 2012, when I wrote
> commit bf4b721932, I found much better documentation than the above!)
>
> So, after commit c7018be509, you can now use the 'skip_all' facility
> after having run some tests; it now converts that into an 'SKIP comment'
> just before the test plan, effectively skipping the remainder of the
> tests in the file. (since we are using a 'trailing plan', and have thus
> not declared how many tests we had intended to run, we can output an
> accurate plan).

Yes, but I consider that c7018be509 is an ugly workaround, not a
part of a properly designed framework.  Unless it is absolutely
necessary to run some tests before we may conditionally want to say
"skip_all/test_done", we should strive to add tests _after_ these
conditional skil_all/test_done is done.

In this case, I do not see there is a strong reason why the new test
must come before the "setup" test.  Sure, it does not use UNCPATH so
the new test may be able to run even when the current path cannot be
spelled as UNC, but that is a natural fallout of (ab)using the test
script that was meant for UNC testing for something else, so I think
a proper way out would be either (1) to use a separate script, if
the new test wants to run whether UNC path can be determined, or (2)
just accept the fact that the new test will only be run when UNC
paths are tested.  Relying on the hack c7018be509 did is much less
appealing.

Re: [PATCH 1/2] mingw: verify that paths are not mistaken for remote nicknames

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> I would prefer to catch the case in the compatibility layer. Here is
> a two patch series that would replace your 12/13 and 13/13.

Thanks.  It is good that I can drop that last one.  Will replace
(with a SQUASH??? for 1/2).

[PATCH 1/2] compat-util: is_missing_file_error()

[Junio C Hamano]

Our code often opens a path to an optional file, to work on its
contents when we can successfully open it.  We can ignore a failure
to open if such an optional file does not exist, but we do want to
report a failure in opening for other reasons (e.g. we got an I/O
error, or the file is there, but we lack the permission to open).

The exact errors we need to ignore are ENOENT (obviously) and
ENOTDIR (less obvious).  Instead of repeating comparison of errno
with these two constants, introduce a helper function to do so.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * Now this is independent of nd/fopen-errors topic.  We could tweak
   nd/fopen-errors topic to use this later, after both of these two
   topics have graduated.

 git-compat-util.h | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/git-compat-util.h b/git-compat-util.h
index 8a4a3f85e7..8a3c680626 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1115,6 +1115,21 @@ struct tm *git_gmtime_r(const time_t *, struct tm *);
 #define getc_unlocked(fh) getc(fh)
 #endif
 
+/*
+ * Our code often opens a path to an optional file, to work on its
+ * contents when we can successfully open it.  We can ignore a failure
+ * to open if such an optional file does not exist, but we do want to
+ * report a failure in opening for other reasons (e.g. we got an I/O
+ * error, or the file is there, but we lack the permission to open).
+ *
+ * Call this function after seeing an error from open() or fopen() to
+ * see if the errno indicates a missing file that we can safely ignore.
+ */
+static inline int is_missing_file_error(int errno_)
+{
+	return (errno_ == ENOENT || errno_ == ENOTDIR);
+}
+
 extern int cmd_main(int, const char **);
 
 #endif
-- 
2.13.0-496-ga689fffbe2

[PATCH 2/2] treewide: use is_missing_file_error() where ENOENT and ENOTDIR are checked

[Junio C Hamano]

Using the is_missing_file_error() helper introduced in the previous
step, update all hits from

  $ git grep -e ENOENT --and -e ENOTDIR

There are codepaths that only check ENOENT, and it is possible that
some of them should be checking both.  Updating them is kept out of
this step deliberately, as we do not want to change behaviour in this
step.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 apply.c                | 2 +-
 builtin/rm.c           | 2 +-
 builtin/update-index.c | 2 +-
 diff-lib.c             | 2 +-
 dir.c                  | 2 +-
 setup.c                | 2 +-
 sha1_name.c            | 4 ++--
 wrapper.c              | 4 ++--
 8 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/apply.c b/apply.c
index 0e2caeab9c..59bb3497de 100644
--- a/apply.c
+++ b/apply.c
@@ -3741,7 +3741,7 @@ static int check_to_create(struct apply_state *state,
 			return 0;
 
 		return EXISTS_IN_WORKTREE;
-	} else if ((errno != ENOENT) && (errno != ENOTDIR)) {
+	} else if (!is_missing_file_error(errno)) {
 		return error_errno("%s", new_name);
 	}
 	return 0;
diff --git a/builtin/rm.c b/builtin/rm.c
index fb79dcab18..30c4332c68 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -129,7 +129,7 @@ static int check_local_mod(struct object_id *head, int index_only)
 		ce = active_cache[pos];
 
 		if (lstat(ce->name, &st) < 0) {
-			if (errno != ENOENT && errno != ENOTDIR)
+			if (!is_missing_file_error(errno))
 				warning_errno(_("failed to stat '%s'"), ce->name);
 			/* It already vanished from the working tree */
 			continue;
diff --git a/builtin/update-index.c b/builtin/update-index.c
index d530e89368..4e9402984a 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -253,7 +253,7 @@ static int remove_one_path(const char *path)
  */
 static int process_lstat_error(const char *path, int err)
 {
-	if (err == ENOENT || err == ENOTDIR)
+	if (is_missing_file_error(err))
 		return remove_one_path(path);
 	return error("lstat(\"%s\"): %s", path, strerror(err));
 }
diff --git a/diff-lib.c b/diff-lib.c
index 52447466b5..88fc71e89e 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -29,7 +29,7 @@
 static int check_removed(const struct cache_entry *ce, struct stat *st)
 {
 	if (lstat(ce->name, st) < 0) {
-		if (errno != ENOENT && errno != ENOTDIR)
+		if (!is_missing_file_error(errno))
 			return -1;
 		return 1;
 	}
diff --git a/dir.c b/dir.c
index aeeb5ce104..98efe9d1c5 100644
--- a/dir.c
+++ b/dir.c
@@ -2235,7 +2235,7 @@ int remove_path(const char *name)
 {
 	char *slash;
 
-	if (unlink(name) && errno != ENOENT && errno != ENOTDIR)
+	if (unlink(name) && !is_missing_file_error(errno))
 		return -1;
 
 	slash = strrchr(name, '/');
diff --git a/setup.c b/setup.c
index 8f64fbdfb2..bb6a2c1beb 100644
--- a/setup.c
+++ b/setup.c
@@ -147,7 +147,7 @@ int check_filename(const char *prefix, const char *arg)
 		name = arg;
 	if (!lstat(name, &st))
 		return 1; /* file exists */
-	if (errno == ENOENT || errno == ENOTDIR)
+	if (is_missing_file_error(errno))
 		return 0; /* file does not exist */
 	die_errno("failed to stat '%s'", arg);
 }
diff --git a/sha1_name.c b/sha1_name.c
index 26ceec1d79..af7500037d 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1406,7 +1406,7 @@ static void diagnose_invalid_sha1_path(const char *prefix,
 	if (file_exists(filename))
 		die("Path '%s' exists on disk, but not in '%.*s'.",
 		    filename, object_name_len, object_name);
-	if (errno == ENOENT || errno == ENOTDIR) {
+	if (is_missing_file_error(errno)) {
 		char *fullname = xstrfmt("%s%s", prefix, filename);
 
 		if (!get_tree_entry(tree_sha1, fullname,
@@ -1471,7 +1471,7 @@ static void diagnose_invalid_index_path(int stage,
 
 	if (file_exists(filename))
 		die("Path '%s' exists on disk, but not in the index.", filename);
-	if (errno == ENOENT || errno == ENOTDIR)
+	if (is_missing_file_error(errno))
 		die("Path '%s' does not exist (neither on disk nor in the index).",
 		    filename);
 
diff --git a/wrapper.c b/wrapper.c
index 0542fc7582..2fbbd81359 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -583,8 +583,8 @@ void warn_on_inaccessible(const char *path)
 
 static int access_error_is_ok(int err, unsigned flag)
 {
-	return err == ENOENT || err == ENOTDIR ||
-		((flag & ACCESS_EACCES_OK) && err == EACCES);
+	return (is_missing_file_error(err) ||
+		((flag & ACCESS_EACCES_OK) && err == EACCES));
 }
 
 int access_or_warn(const char *path, int mode, unsigned flag)
-- 
2.13.0-496-ga689fffbe2

Re: [PATCH 1/2] mingw: verify that paths are not mistaken for remote nicknames

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

>> Doesn't this need test_i18ngrep?:
>
> Good catch! It would be this one in warn_on_inaccessible:
>
>>      wrapper.c:581:  warning_errno(_("unable to access '%s'"), path);
>
> But actually, I'm more worried about the unholy mix of
> one-test-first-then-skip_all-later that occurs in this test script (I
> do not mean the skip_all that is visible in the context, there are
> others later). I think there was some buzz recently that prove only
> understands a summary line that reads "1..0", but here we would see
> "1..1". What to do? Reorganize the test script? Dscho, any ideas?

For now I've queued this on top of 1/2, so that suggestions are not
lost, and then tweaked 2/2 (as context for the patch to the test
changes).  

Either an ack or a reroll is appreciated (I do not think we'd
terribly mind if this test were added to another script, or if this
test were skipped when UNC path cannot be determined even though it
does not need that prereq.  Also UNC_PATH can become prereq that is
tested by individual test in this script and the new test can be
added without requiring that prereq).

Thanks.

 t/t5580-clone-push-unc.sh | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/t/t5580-clone-push-unc.sh b/t/t5580-clone-push-unc.sh
index fd719a209e..944730cddc 100755
--- a/t/t5580-clone-push-unc.sh
+++ b/t/t5580-clone-push-unc.sh
@@ -8,12 +8,6 @@ if ! test_have_prereq MINGW; then
 	test_done
 fi
 
-test_expect_failure 'remote nick cannot contain backslashes' '
-	BACKSLASHED="$(pwd | tr / \\\\)" &&
-	git ls-remote "$BACKSLASHED" >out 2>err &&
-	! grep "unable to access" err
-'
-
 UNCPATH="$(pwd)"
 case "$UNCPATH" in
 [A-Z]:*)
@@ -51,4 +45,10 @@ test_expect_success push '
 	test "$rev" = "$(git rev-parse --verify refs/heads/to-push)"
 '
 
+test_expect_failure 'remote nick cannot contain backslashes' '
+	BACKSLASHED="$(pwd | tr / \\\\)" &&
+	git ls-remote "$BACKSLASHED" >out 2>err &&
+	test_i18ngrep ! "unable to access" err
+'
+
 test_done
-- 
2.13.0-493-g9105ebc082

Re: [PATCH 1/2] mingw: verify that paths are not mistaken for remote nicknames

[Ramsay Jones]

On 30/05/17 01:03, Junio C Hamano wrote:
> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
> 
>> See commit c7018be509 ("test: allow skipping the remainder", 18-05-2017)
>> which is currently merged to the 'next' branch (merge 03b8a61e47 of the
>> 'jc/skip-test-in-the-middle' branch).
>>
>> (see also http://testanything.org)
>>
>> If you look at http://testanything.org//tap-specification.html, it shows
>> that you are allowed to annotate a plan of '1..0' with a SKIP directive
>> to explain why no tests in a file were run. However, a plan with '1..n'
>> (for any n > 0) must not have any annotation. (Back in 2012, when I wrote
>> commit bf4b721932, I found much better documentation than the above!)
>>
>> So, after commit c7018be509, you can now use the 'skip_all' facility
>> after having run some tests; it now converts that into an 'SKIP comment'
>> just before the test plan, effectively skipping the remainder of the
>> tests in the file. (since we are using a 'trailing plan', and have thus
>> not declared how many tests we had intended to run, we can output an
>> accurate plan).
> 
> Yes, but I consider that c7018be509 is an ugly workaround, not a
> part of a properly designed framework.  Unless it is absolutely
> necessary to run some tests before we may conditionally want to say
> "skip_all/test_done", we should strive to add tests _after_ these
> conditional skil_all/test_done is done.

yes, I don't disagree with that (which is why I said that I would
have split t5545 into two files). ;-)

> In this case, I do not see there is a strong reason why the new test
> must come before the "setup" test.  Sure, it does not use UNCPATH so
> the new test may be able to run even when the current path cannot be
> spelled as UNC, but that is a natural fallout of (ab)using the test
> script that was meant for UNC testing for something else, so I think
> a proper way out would be either (1) to use a separate script, if
> the new test wants to run whether UNC path can be determined,

Yes, I had intended to suggest this (or an existing script, protected
by the MINGW prerequisite), but forgot!

>                                                              or (2)
> just accept the fact that the new test will only be run when UNC
> paths are tested.

I prefer (1).

>                   Relying on the hack c7018be509 did is much less
> appealing.

ATB,
Ramsay Jones

Re: [PATCH v3 05/13] config.mak.uname: set FREAD_READS_DIRECTORIES for Darwin, too

[Stefan Beller]

On Thu, May 25, 2017 at 8:35 PM, Junio C Hamano <gitster@pobox.com> wrote:

Do we have any reasons for that, or pointers on the mailing list, that this
is a good idea or needed? Does it fix a bug or enable a new feature on Darwin?

> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  config.mak.uname | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/config.mak.uname b/config.mak.uname
> index a25ffddb3e..1743890164 100644
> --- a/config.mak.uname
> +++ b/config.mak.uname
> @@ -110,6 +110,7 @@ ifeq ($(uname_S),Darwin)
>         BASIC_CFLAGS += -DPRECOMPOSE_UNICODE
>         BASIC_CFLAGS += -DPROTECT_HFS_DEFAULT=1
>         HAVE_BSD_SYSCTL = YesPlease
> +       FREAD_READS_DIRECTORIES = UnfortunatelyYes
>  endif
>  ifeq ($(uname_S),SunOS)
>         NEEDS_SOCKET = YesPlease
> --
> 2.13.0-491-g71cfeddc25
>

Re: [PATCH v3 10/13] rerere.c: move error_errno() closer to the source system call

[Stefan Beller]

On Thu, May 25, 2017 at 8:35 PM, Junio C Hamano <gitster@pobox.com> wrote:
> From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>
> We are supposed to report errno from fopen(). fclose() between fopen()
> and the report function could either change errno or reset it.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  rerere.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/rerere.c b/rerere.c
> index 1351b0c3fb..c26c29f87a 100644
> --- a/rerere.c
> +++ b/rerere.c
> @@ -489,8 +489,9 @@ static int handle_file(const char *path, unsigned char *sha1, const char *output
>         if (output) {
>                 io.io.output = fopen(output, "w");
>                 if (!io.io.output) {
> +                       error_errno("Could not write %s", output);
>                         fclose(io.input);
> -                       return error_errno("Could not write %s", output);
> +                       return -1;

and here we assume fclose doesn't error out.

This is ok as we are sure it cannot be EBADF as we just opened that
fd and as we did not read from io.input yet, any errno from close(2), fflush(3)
are not expected?

close(2) may yield an EINTR, in which case we may want to

  while (fclose(io.input) && errno != EINTR)
    ; /*do nothing*/

On the other hand this is already in the error path, so maybe we're ok
leaking an fd here in case of a signal?

Re: [PATCH v3 13/13] is_missing_file_error(): work around EINVAL on Windows

[Stefan Beller]

On Thu, May 25, 2017 at 8:35 PM, Junio C Hamano <gitster@pobox.com> wrote:
> When asked to open/fopen a path, e.g. "a/b:/c", which does not exist
> on the filesystem, Windows (correctly) fails to open it but sets
> EINVAL to errno

errno to EINVAL (as of now it sounds as if it is a EINVAL = errno,
which makes no sense to me)

> because the pathname has characters that cannot be
> stored in its filesystem.
>
> As this is an expected failure, teach is_missing_file_error() helper
> about this case.
>
> This is RFC,

cc'd people knowledgeable of Windows.

> as there may be a case where we get EINVAL from
> open/fopen for reasons other than "the filesystem does not like this
> pathname" that may be worth reporting to the user, and this change
> is sweeping such an error under the rug.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  wrapper.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/wrapper.c b/wrapper.c
> index f1c87ec7ea..74aa3b7803 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -434,6 +434,10 @@ static void warn_on_inaccessible(const char *path)
>   * see if the errno indicates a missing file that we can safely ignore.
>   */
>  static int is_missing_file_error(int errno_) {
> +#ifdef GIT_WINDOWS_NATIVE
> +       if (errno_ == EINVAL)
> +               return 1;
> +#endif
>         return (errno_ == ENOENT || errno_ == ENOTDIR);
>  }
>
> --
> 2.13.0-491-g71cfeddc25
>

Re: [PATCH 1/2] mingw: verify that paths are not mistaken for remote nicknames

[Johannes Sixt]

Am 30.05.2017 um 06:46 schrieb Junio C Hamano:
> Johannes Sixt <j6t@kdbg.org> writes:
> 
>>> Doesn't this need test_i18ngrep?:
>>
>> Good catch! It would be this one in warn_on_inaccessible:
>>
>>>       wrapper.c:581:  warning_errno(_("unable to access '%s'"), path);
>>
>> But actually, I'm more worried about the unholy mix of
>> one-test-first-then-skip_all-later that occurs in this test script (I
>> do not mean the skip_all that is visible in the context, there are
>> others later). I think there was some buzz recently that prove only
>> understands a summary line that reads "1..0", but here we would see
>> "1..1". What to do? Reorganize the test script? Dscho, any ideas?
> 
> For now I've queued this on top of 1/2, so that suggestions are not
> lost, and then tweaked 2/2 (as context for the patch to the test
> changes).
> 
> Either an ack or a reroll is appreciated (I do not think we'd
> terribly mind if this test were added to another script, or if this
> test were skipped when UNC path cannot be determined even though it
> does not need that prereq.  Also UNC_PATH can become prereq that is
> tested by individual test in this script and the new test can be
> added without requiring that prereq).
> 
> Thanks.
> 
>   t/t5580-clone-push-unc.sh | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/t/t5580-clone-push-unc.sh b/t/t5580-clone-push-unc.sh
> index fd719a209e..944730cddc 100755
> --- a/t/t5580-clone-push-unc.sh
> +++ b/t/t5580-clone-push-unc.sh
> @@ -8,12 +8,6 @@ if ! test_have_prereq MINGW; then
>   	test_done
>   fi
>   
> -test_expect_failure 'remote nick cannot contain backslashes' '
> -	BACKSLASHED="$(pwd | tr / \\\\)" &&
> -	git ls-remote "$BACKSLASHED" >out 2>err &&
> -	! grep "unable to access" err
> -'
> -
>   UNCPATH="$(pwd)"
>   case "$UNCPATH" in
>   [A-Z]:*)
> @@ -51,4 +45,10 @@ test_expect_success push '
>   	test "$rev" = "$(git rev-parse --verify refs/heads/to-push)"
>   '
>   
> +test_expect_failure 'remote nick cannot contain backslashes' '
> +	BACKSLASHED="$(pwd | tr / \\\\)" &&
> +	git ls-remote "$BACKSLASHED" >out 2>err &&
> +	test_i18ngrep ! "unable to access" err
> +'
> +
>   test_done
> 

ACK!

-- Hannes

Re: [PATCH v3 05/13] config.mak.uname: set FREAD_READS_DIRECTORIES for Darwin, too

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> On Thu, May 25, 2017 at 8:35 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Do we have any reasons for that, or pointers on the mailing list, that this
> is a good idea or needed? Does it fix a bug or enable a new feature on Darwin?

BSD lets you open() a directory for reading and hence we require
this workaround on it; I think somebody in an earlier round
suspected that that Darwin would have inherited the same from its
BSD lineage, and I don't see a reason to contradict with that
suspicion, either.

>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---
>>  config.mak.uname | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/config.mak.uname b/config.mak.uname
>> index a25ffddb3e..1743890164 100644
>> --- a/config.mak.uname
>> +++ b/config.mak.uname
>> @@ -110,6 +110,7 @@ ifeq ($(uname_S),Darwin)
>>         BASIC_CFLAGS += -DPRECOMPOSE_UNICODE
>>         BASIC_CFLAGS += -DPROTECT_HFS_DEFAULT=1
>>         HAVE_BSD_SYSCTL = YesPlease
>> +       FREAD_READS_DIRECTORIES = UnfortunatelyYes
>>  endif
>>  ifeq ($(uname_S),SunOS)
>>         NEEDS_SOCKET = YesPlease
>> --
>> 2.13.0-491-g71cfeddc25
>>

Re: [PATCH v3 13/13] is_missing_file_error(): work around EINVAL on Windows

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> On Thu, May 25, 2017 at 8:35 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> When asked to open/fopen a path, e.g. "a/b:/c", which does not exist
>> on the filesystem, Windows (correctly) fails to open it but sets
>> EINVAL to errno
>
> errno to EINVAL (as of now it sounds as if it is a EINVAL = errno,
> which makes no sense to me)
>
>> because the pathname has characters that cannot be
>> stored in its filesystem.
>>
>> As this is an expected failure, teach is_missing_file_error() helper
>> about this case.
>>
>> This is RFC,
>
> cc'd people knowledgeable of Windows.

This has been resolved I think with J6t/Dscho's patch yesterday.

Thanks.

Re: [PATCH v3 05/13] config.mak.uname: set FREAD_READS_DIRECTORIES for Darwin, too

[Stefan Beller]

On Tue, May 30, 2017 at 4:14 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> On Thu, May 25, 2017 at 8:35 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Do we have any reasons for that, or pointers on the mailing list, that this
>> is a good idea or needed? Does it fix a bug or enable a new feature on Darwin?
>
> BSD lets you open() a directory for reading and hence we require
> this workaround on it; I think somebody in an earlier round
> suspected that that Darwin would have inherited the same from its
> BSD lineage, and I don't see a reason to contradict with that
> suspicion, either.
>

Sorry about not being clear:
I - just like some future reader - did not have the context of the previous
round, so please put this information into the commit message.

Thanks,
Stefan

Re: [PATCH v3 13/13] is_missing_file_error(): work around EINVAL on Windows

[Stefan Beller]

On Tue, May 30, 2017 at 4:17 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:

>> cc'd people knowledgeable of Windows.
>
> This has been resolved I think with J6t/Dscho's patch yesterday.
>
> Thanks.

Yeah I saw those patches after reviewing this series.

Sorry for the noise,
Stefan