{"thread":{"id":"44560","subject":"What's cooking in git.git (Nov 2016, #06; Mon, 28)","startedAt":"2016-11-29T00:15:19Z","lastAt":"2016-12-01T19:53:26Z","messageCount":34,"participants":["Junio C Hamano","Brandon Williams","Jeff King","Stefan Beller","Johannes Sixt"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"306527","messageId":"xmqqk2bngn03.fsf@gitster.mtv.corp.google.com","threadId":"44560","inReplyTo":null,"subject":"What's cooking in git.git (Nov 2016, #06; Mon, 28)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-29T00:15:08Z","receivedAt":"2016-11-29T00:15:19Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Here are the topics that have been cooking. Commits prefixed with\n'-' are only in 'pu' (proposed updates) while commits prefixed with\n'+' are in 'next'. The ones marked with '.' do not appear in any of\nthe integration branches, but I am still holding onto them.\n\nYou can find the changes described here in the integration branches\nof the repositories listed at\n\n http://git-blame.blogspot.com/p/git-public-repositories.html\n\n--------------------------------------------------\n[New Topics]\n\n* jc/renormalize-merge-kill-safer-crlf (2016-11-28) 2 commits\n - merge-recursive: handle NULL in add_cacheinfo() correctly\n - cherry-pick: demonstrate a segmentation fault\n\n Fix a corner case in merge-recursive regression that crept in\n during 2.10 development cycle.\n\n Will merge to 'next'.\n\n\n* js/difftool-builtin (2016-11-28) 2 commits\n - difftool: implement the functionality in the builtin\n - difftool: add a skeleton for the upcoming builtin\n\n Rewrite a scripted porcelain \"git difftool\" in C.\n\n Under discussion.\n\n\n* nd/qsort-in-merge-recursive (2016-11-28) 1 commit\n - merge-recursive.c: use string_list_sort instead of qsort\n\n Code simplification.\n\n Will merge to 'next'.\n\n--------------------------------------------------\n[Stalled]\n\n* jc/retire-compaction-heuristics (2016-11-02) 3 commits\n - SQUASH???\n - SQUASH???\n - diff: retire the original experimental \"compaction\" heuristics\n\n Waiting for a reroll.\n\n\n* jc/abbrev-autoscale-config (2016-11-01) 1 commit\n - config.abbrev: document the new default that auto-scales\n\n Waiting for a reroll.\n\n\n* jk/nofollow-attr-ignore (2016-11-02) 5 commits\n - exclude: do not respect symlinks for in-tree .gitignore\n - attr: do not respect symlinks for in-tree .gitattributes\n - exclude: convert \"check_index\" into a flags field\n - attr: convert \"macro_ok\" into a flags field\n - add open_nofollow() helper\n\n As we do not follow symbolic links when reading control files like\n .gitignore and .gitattributes from the index, match the behaviour\n and not follow symbolic links when reading them from the working\n tree. This also tightens security a bit by not leaking contents of\n an unrelated file in the error messages when it is pointed at by\n one of these files that is a symbolic link.\n\n Perhaps we want to cover .gitmodules too with the same mechanism?\n\n\n* nd/worktree-move (2016-11-28) 11 commits\n . worktree remove: new command\n . worktree move: refuse to move worktrees with submodules\n . worktree move: accept destination as directory\n . worktree move: new command\n . worktree.c: add update_worktree_location()\n . worktree.c: add validate_worktree()\n . copy.c: convert copy_file() to copy_dir_recursively()\n . copy.c: style fix\n . copy.c: convert bb_(p)error_msg to error(_errno)\n . copy.c: delete unused code in copy_file()\n . copy.c: import copy_file() from busybox\n (this branch uses nd/worktree-list-fixup.)\n\n \"git worktree\" learned move and remove subcommands.\n\n Reported to break builds on Windows.\n\n\n* jc/bundle (2016-03-03) 6 commits\n - index-pack: --clone-bundle option\n - Merge branch 'jc/index-pack' into jc/bundle\n - bundle v3: the beginning\n - bundle: keep a copy of bundle file name in the in-core bundle header\n - bundle: plug resource leak\n - bundle doc: 'verify' is not about verifying the bundle\n\n The beginning of \"split bundle\", which could be one of the\n ingredients to allow \"git clone\" traffic off of the core server\n network to CDN.\n\n While I think it would make it easier for people to experiment and\n build on if the topic is merged to 'next', I am at the same time a\n bit reluctant to merge an unproven new topic that introduces a new\n file format, which we may end up having to support til the end of\n time. It is likely that to support a \"prime clone from CDN\", it\n would need a lot more than just \"these are the heads and the pack\n data is over there\", so this may not be sufficient.\n\n Will discard.\n\n\n* mh/connect (2016-06-06) 10 commits\n - connect: [host:port] is legacy for ssh\n - connect: move ssh command line preparation to a separate function\n - connect: actively reject git:// urls with a user part\n - connect: change the --diag-url output to separate user and host\n - connect: make parse_connect_url() return the user part of the url as a separate value\n - connect: group CONNECT_DIAG_URL handling code\n - connect: make parse_connect_url() return separated host and port\n - connect: re-derive a host:port string from the separate host and port variables\n - connect: call get_host_and_port() earlier\n - connect: document why we sometimes call get_port after get_host_and_port\n\n Rewrite Git-URL parsing routine (hopefully) without changing any\n behaviour.\n\n It has been two months without any support. We may want to discard\n this.\n\n\n* ec/annotate-deleted (2015-11-20) 1 commit\n - annotate: skip checking working tree if a revision is provided\n\n Usability fix for annotate-specific \" \" syntax with deleted\n files.\n\n Has been waiting for a review for too long without seeing anything.\n\n Will discard.\n\n\n* dk/gc-more-wo-pack (2016-01-13) 4 commits\n - gc: clean garbage .bitmap files from pack dir\n - t5304: ensure non-garbage files are not deleted\n - t5304: test .bitmap garbage files\n - prepare_packed_git(): find more garbage\n\n Follow-on to dk/gc-idx-wo-pack topic, to clean up stale\n .bitmap and .keep files.\n\n Has been waiting for a reroll for too long.\n cf. \n\n Will discard.\n\n\n* jc/diff-b-m (2015-02-23) 5 commits\n . WIPWIP\n . WIP: diff-b-m\n - diffcore-rename: allow easier debugging\n - diffcore-rename.c: add locate_rename_src()\n - diffcore-break: allow debugging\n\n \"git diff -B -M\" produced incorrect patch when the postimage of a\n completely rewritten file is similar to the preimage of a removed\n file; such a resulting file must not be expressed as a rename from\n other place.\n\n The fix in this patch is broken, unfortunately.\n\n Will discard.\n\n--------------------------------------------------\n[Cooking]\n\n* bw/push-dry-run (2016-11-23) 2 commits\n - push: fix --dry-run to not push submodules\n - push: --dry-run updates submodules when --recurse-submodules=on-demand\n (this branch uses hv/submodule-not-yet-pushed-fix.)\n\n \"git push --dry-run --recurse-submodule=on-demand\" wasn't\n \"--dry-run\" in the submodules.\n\n Will merge to 'next'.\n\n\n* sb/push-make-submodule-check-the-default (2016-10-10) 2 commits\n - push: change submodule default to check when submodules exist\n - submodule add: extend force flag to add existing repos\n\n Turn the default of \"push.recurseSubmodules\" to \"check\" when\n submodules seem to be in use.\n\n Need to rebase on hv/submodule-not-yet-pushed-fix and then consider\n merging to 'next'.\n\n\n* jk/rev-parse-symbolic-parents-fix (2016-11-16) 1 commit\n - rev-parse: fix parent shorthands with --symbolic\n\n \"git rev-parse --symbolic\" failed with a more recent notation like\n \"HEAD^-1\" and \"HEAD^!\".\n\n Will merge to 'next'.\n\n\n* nd/worktree-list-fixup (2016-11-28) 5 commits\n - worktree list: keep the list sorted\n - worktree.c: get_worktrees() takes a new flag argument\n - get_worktrees() must return main worktree as first item even on error\n - worktree: reorder an if statement\n - worktree.c: zero new 'struct worktree' on allocation\n (this branch is used by nd/worktree-move.)\n\n The output from \"git worktree list\" was made in readdir() order,\n and was unstable.\n\n Will merge to 'next'.\n\n\n* jk/trailers-placeholder-in-pretty (2016-11-21) 2 commits\n - ref-filter: add support to display trailers as part of contents\n - pretty: add %(trailers) format for displaying trailers of a commit message\n\n In addition to %(subject), %(body), \"log --pretty=format:...\"\n learned a new placeholder %(trailers).\n\n Will merge to 'next'.\n\n\n* sb/submodule-intern-gitdir (2016-11-22) 5 commits\n - SQUASH\n - submodule: add embed-git-dir function\n - test-lib-functions.sh: teach test_commit -C \n - submodule helper: support super prefix\n - submodule: use absolute path for computing relative path connecting\n\n A new submodule helper \"git submodule embedgitdirs\" to make it\n easier to move embedded .git/ directory for submodules in a\n superproject to .git/modules/ (and point the latter with the former\n that is turned into a \"gitdir:\" file) has been added.\n\n Need to read it over again, deal with SQUASH, and may ask for a\n reroll.\n\n\n* dt/empty-submodule-in-merge (2016-11-17) 1 commit\n - submodules: allow empty working-tree dirs in merge/cherry-pick\n\n An empty directory in a working tree that can simply be nuked used\n to interfere while merging or cherry-picking a change to create a\n submodule directory there, which has been fixed..\n\n Waiting for review.\n\n\n* bw/grep-recurse-submodules (2016-11-22) 6 commits\n - grep: search history of moved submodules\n - grep: enable recurse-submodules to work on objects\n - grep: optionally recurse into submodules\n - grep: add submodules as a grep source type\n - submodules: load gitmodules file from commit sha1\n - submodules: add helper functions to determine presence of submodules\n\n \"git grep\" learns to optionally recurse into submodules\n\n Has anybody else seen t7814 being flakey with this series?\n\n\n* dt/smart-http-detect-server-going-away (2016-11-18) 2 commits\n (merged to 'next' on 2016-11-21 at d502523347)\n + upload-pack: optionally allow fetching any sha1\n + remote-curl: don't hang when a server dies before any output\n\n When the http server gives an incomplete response to a smart-http\n rpc call, it could lead to client waiting for a full response that\n will never come. Teach the client side to notice this condition\n and abort the transfer.\n\n An improvement counterproposal has failed.\n cf. <20161114194049.mktpsvgdhex2f4zv@sigill.intra.peff.net>\n\n Will cook in 'next'.\n\n\n* mm/push-social-engineering-attack-doc (2016-11-14) 1 commit\n (merged to 'next' on 2016-11-16 at b7c1b27563)\n + doc: mention transfer data leaks in more places\n\n Doc update on fetching and pushing.\n\n Will cook in 'next'.\n\n\n* jc/compression-config (2016-11-15) 1 commit\n (merged to 'next' on 2016-11-23 at b3c9254897)\n + compression: unify pack.compression configuration parsing\n\n Compression setting for producing packfiles were spread across\n three codepaths, one of which did not honor any configuration.\n Unify these so that all of them honor core.compression and\n pack.compression variables the same way.\n\n Will cook in 'next'.\n\n\n* mm/gc-safety-doc (2016-11-16) 1 commit\n (merged to 'next' on 2016-11-17 at fc0d225b6b)\n + git-gc.txt: expand discussion of races with other processes\n\n Doc update.\n\n Will cook in 'next'.\n\n\n* hv/submodule-not-yet-pushed-fix (2016-11-16) 4 commits\n (merged to 'next' on 2016-11-21 at 1a599af962)\n + submodule_needs_pushing(): explain the behaviour when we cannot answer\n + batch check whether submodule needs pushing into one call\n + serialize collection of refs that contain submodule changes\n + serialize collection of changed submodules\n (this branch is used by bw/push-dry-run.)\n\n The code in \"git push\" to compute if any commit being pushed in the\n superproject binds a commit in a submodule that hasn't been pushed\n out was overly inefficient, making it unusable even for a small\n project that does not have any submodule but have a reasonable\n number of refs.\n\n Will cook in 'next'.\n\n\n* kn/ref-filter-branch-list (2016-11-15) 18 commits\n - for-each-ref: do not segv with %(HEAD) on an unborn branch\n - branch: implement '--format' option\n - branch: use ref-filter printing APIs\n - branch, tag: use porcelain output\n - ref-filter: allow porcelain to translate messages in the output\n - ref-filter: add `:dir` and `:base` options for ref printing atoms\n - ref-filter: make remote_ref_atom_parser() use refname_atom_parser_internal()\n - ref-filter: introduce symref_atom_parser() and refname_atom_parser()\n - ref-filter: introduce refname_atom_parser_internal()\n - ref-filter: make \"%(symref)\" atom work with the ':short' modifier\n - ref-filter: add support for %(upstream:track,nobracket)\n - ref-filter: make %(upstream:track) prints \"[gone]\" for invalid upstreams\n - ref-filter: introduce format_ref_array_item()\n - ref-filter: move get_head_description() from branch.c\n - ref-filter: modify \"%(objectname:short)\" to take length\n - ref-filter: implement %(if:equals=) and %(if:notequals=)\n - ref-filter: include reference to 'used_atom' within 'atom_value'\n - ref-filter: implement %(if), %(then), and %(else) atoms\n\n The code to list branches in \"git branch\" has been consolidated\n with the more generic ref-filter API.\n\n Rerolled, reviewed, looking good.\n Expecting a reroll.\n cf. <20161108201211.25213-1-Karthik.188@gmail.com>\n cf. \n cf. \n\n\n* bw/transport-protocol-policy (2016-11-09) 2 commits\n (merged to 'next' on 2016-11-16 at 1391d3eeed)\n + transport: add protocol policy config option\n + lib-proto-disable: variable name fix\n\n Finer-grained control of what protocols are allowed for transports\n during clone/fetch/push have been enabled via a new configuration\n mechanism.\n\n Will cook in 'next'.\n\n\n* jt/fetch-no-redundant-tag-fetch-map (2016-11-11) 1 commit\n (merged to 'next' on 2016-11-16 at 5846c27cc5)\n + fetch: do not redundantly calculate tag refmap\n\n Code cleanup to avoid using redundant refspecs while fetching with\n the --tags option.\n\n Will cook in 'next'.\n\n\n* sb/submodule-config-cleanup (2016-11-22) 3 commits\n (merged to 'next' on 2016-11-23 at c02e578b49)\n + submodule-config: clarify parsing of null_sha1 element\n + submodule-config: rename commit_sha1 to treeish_name\n + submodule config: inline config_from_{name, path}\n\n Minor code clean-up.\n\n Will cook in 'next'.\n\n\n* jc/push-default-explicit (2016-10-31) 2 commits\n (merged to 'next' on 2016-11-01 at 8dc3a6cf25)\n + push: test pushing ambiguously named branches\n + push: do not use potentially ambiguous default refspec\n\n A lazy \"git push\" without refspec did not internally use a fully\n specified refspec to perform 'current', 'simple', or 'upstream'\n push, causing unnecessary \"ambiguous ref\" errors.\n\n Will cook in 'next'.\n\n\n* jt/use-trailer-api-in-commands (2016-11-02) 6 commits\n - sequencer: use trailer's trailer layout\n - trailer: have function to describe trailer layout\n - trailer: avoid unnecessary splitting on lines\n - commit: make ignore_non_trailer take buf/len\n - SQUASH???\n - trailer: be stricter in parsing separators\n\n Commands that operate on a log message and add lines to the trailer\n blocks, such as \"format-patch -s\", \"cherry-pick (-x|-s)\", and\n \"commit -s\", have been taught to use the logic of and share the\n code with \"git interpret-trailer\".\n\n Will merge to 'next' after dealing with the SQUASH???\n\n\n* nd/rebase-forget (2016-11-28) 1 commit\n - rebase: add --forget to cleanup rebase, leave everything else untouched\n\n \"git rebase\" learned \"--forget\" option, which allows a user to\n remove the metadata left by an earlier \"git rebase\" that was\n manually aborted without using \"git rebase --abort\".\n\n Will merge to 'next'.\n\n\n* jc/git-open-cloexec (2016-11-02) 3 commits\n - sha1_file: stop opening files with O_NOATIME\n - git_open_cloexec(): use fcntl(2) w/ FD_CLOEXEC fallback\n - git_open(): untangle possible NOATIME and CLOEXEC interactions\n\n The codeflow of setting NOATIME and CLOEXEC on file descriptors Git\n opens has been simplified.\n\n We may want to drop the tip one.\n\n\n* jk/no-looking-at-dotgit-outside-repo-final (2016-10-26) 1 commit\n (merged to 'next' on 2016-10-26 at 220e160451)\n + setup_git_env: avoid blind fall-back to \".git\"\n\n This is the endgame of the topic to avoid blindly falling back to\n \".git\" when the setup sequence said we are _not_ in Git repository.\n A corner case that happens to work right now may be broken by a\n call to die(\"BUG\").\n\n Will cook in 'next'.\n\n\n* jc/reset-unmerge (2016-10-24) 1 commit\n - reset: --unmerge\n\n After \"git add\" is run prematurely during a conflict resolution,\n \"git diff\" can no longer be used as a way to sanity check by\n looking at the combined diff. \"git reset\" learned a new\n \"--unmerge\" option to recover from this situation.\n\n Will discard.\n This may not be needed, given that update-index has a similar\n option.\n\n\n* jc/merge-base-fp-only (2016-10-19) 8 commits\n . merge-base: fp experiment\n - merge: allow to use only the fp-only merge bases\n - merge-base: limit the output to bases that are on first-parent chain\n - merge-base: mark bases that are on first-parent chain\n - merge-base: expose get_merge_bases_many_0() a bit more\n - merge-base: stop moving commits around in remove_redundant()\n - sha1_name: remove ONELINE_SEEN bit\n - commit: simplify fastpath of merge-base\n\n An experiment of merge-base that ignores common ancestors that are\n not on the first parent chain.\n\n Will discard.\n The whole premise feels wrong.\n\n\n* tb/convert-stream-check (2016-10-27) 2 commits\n - convert.c: stream and fast search for binary\n - read-cache: factor out get_sha1_from_index() helper\n\n End-of-line conversion sometimes needs to see if the current blob\n in the index has NULs and CRs to base its decision. We used to\n always get a full statistics over the blob, but in many cases we\n can return early when we have seen \"enough\" (e.g. if we see a\n single NUL, the blob will be handled as binary). The codepaths\n have been optimized by using streaming interface.\n\n Will discard.\n Retracted.\n cf. <20161102071646.GA5094@tb-raspi>\n\n\n* pb/bisect (2016-10-18) 27 commits\n - bisect--helper: remove the dequote in bisect_start()\n - bisect--helper: retire `--bisect-auto-next` subcommand\n - bisect--helper: retire `--bisect-autostart` subcommand\n - bisect--helper: retire `--bisect-write` subcommand\n - bisect--helper: `bisect_replay` shell function in C\n - bisect--helper: `bisect_log` shell function in C\n - bisect--helper: retire `--write-terms` subcommand\n - bisect--helper: retire `--check-expected-revs` subcommand\n - bisect--helper: `bisect_state` & `bisect_head` shell function in C\n - bisect--helper: `bisect_autostart` shell function in C\n - bisect--helper: retire `--next-all` subcommand\n - bisect--helper: retire `--bisect-clean-state` subcommand\n - bisect--helper: `bisect_next` and `bisect_auto_next` shell function in C\n - t6030: no cleanup with bad merge base\n - bisect--helper: `bisect_start` shell function partially in C\n - bisect--helper: `get_terms` & `bisect_terms` shell function in C\n - bisect--helper: `bisect_next_check` & bisect_voc shell function in C\n - bisect--helper: `check_and_set_terms` shell function in C\n - bisect--helper: `bisect_write` shell function in C\n - bisect--helper: `is_expected_rev` & `check_expected_revs` shell function in C\n - bisect--helper: `bisect_reset` shell function in C\n - wrapper: move is_empty_file() and rename it as is_empty_or_missing_file()\n - t6030: explicitly test for bisection cleanup\n - bisect--helper: `bisect_clean_state` shell function in C\n - bisect--helper: `write_terms` shell function in C\n - bisect: rewrite `check_term_format` shell function in C\n - bisect--helper: use OPT_CMDMODE instead of OPT_BOOL\n\n Move more parts of \"git bisect\" to C.\n\n Waiting for review.\n\n\n* st/verify-tag (2016-10-10) 7 commits\n - t/t7004-tag: Add --format specifier tests\n - t/t7030-verify-tag: Add --format specifier tests\n - builtin/tag: add --format argument for tag -v\n - builtin/verify-tag: add --format to verify-tag\n - tag: add format specifier to gpg_verify_tag\n - ref-filter: add function to print single ref_array_item\n - gpg-interface, tag: add GPG_VERIFY_QUIET flag\n\n \"git tag\" and \"git verify-tag\" learned to put GPG verification\n status in their \"--format=\" output format.\n\n Waiting for a reroll.\n cf. <20161007210721.20437-1-santiago@nyu.edu>\n\n\n* sb/attr (2016-11-11) 35 commits\n - completion: clone can initialize specific submodules\n - clone: add --init-submodule= switch\n - submodule update: add `--init-default-path` switch\n - pathspec: allow escaped query values\n - pathspec: allow querying for attributes\n - pathspec: move prefix check out of the inner loop\n - pathspec: move long magic parsing out of prefix_pathspec\n - Documentation: fix a typo\n - attr: keep attr stack for each check\n - attr: convert to new threadsafe API\n - attr: make git_check_attr_counted static\n - attr.c: outline the future plans by heavily commenting\n - attr.c: always pass check[] to collect_some_attrs()\n - attr.c: introduce empty_attr_check_elems()\n - attr.c: correct ugly hack for git_all_attrs()\n - attr.c: rename a local variable check\n - attr.c: pass struct git_attr_check down the callchain\n - attr.c: add push_stack() helper\n - attr: support quoting pathname patterns in C style\n - attr: expose validity check for attribute names\n - attr: add counted string version of git_check_attr()\n - attr: retire git_check_attrs() API\n - attr: convert git_check_attrs() callers to use the new API\n - attr: convert git_all_attrs() to use \"struct git_attr_check\"\n - attr: (re)introduce git_check_attr() and struct git_attr_check\n - attr: rename function and struct related to checking attributes\n - attr.c: plug small leak in parse_attr_line()\n - attr.c: tighten constness around \"git_attr\" structure\n - attr.c: simplify macroexpand_one()\n - attr.c: mark where #if DEBUG ends more clearly\n - attr.c: complete a sentence in a comment\n - attr.c: explain the lack of attr-name syntax check in parse_attr()\n - attr.c: update a stale comment on \"struct match_attr\"\n - attr.c: use strchrnul() to scan for one line\n - commit.c: use strchrnul() to scan for one line\n\n The attributes API has been updated so that it can later be\n optimized using the knowledge of which attributes are queried.\n Building on top of the updated API, the pathspec machinery learned\n to select only paths with given attributes set.\n\n Waiting for review.\n\n\n* va/i18n-perl-scripts (2016-11-11) 16 commits\n - i18n: difftool: mark warnings for translation\n - i18n: send-email: mark composing message for translation\n - i18n: send-email: mark string with interpolation for translation\n - i18n: send-email: mark warnings and errors for translation\n - i18n: send-email: mark strings for translation\n - i18n: add--interactive: mark status words for translation\n - i18n: add--interactive: remove %patch_modes entries\n - i18n: add--interactive: mark edit_hunk_manually message for translation\n - i18n: add--interactive: i18n of help_patch_cmd\n - i18n: add--interactive: mark patch prompt for translation\n - i18n: add--interactive: mark plural strings\n - i18n: clean.c: match string with git-add--interactive.perl\n - i18n: add--interactive: mark strings with interpolation for translation\n - i18n: add--interactive: mark simple here-documents for translation\n - i18n: add--interactive: mark strings for translation\n - Git.pm: add subroutines for commenting lines\n\n Porcelain scripts written in Perl are getting internationalized.\n\n Waiting for review.\n\n\n* jc/latin-1 (2016-09-26) 2 commits\n (merged to 'next' on 2016-09-28 at c8673e03c2)\n + utf8: accept \"latin-1\" as ISO-8859-1\n + utf8: refactor code to decide fallback encoding\n\n Some platforms no longer understand \"latin-1\" that is still seen in\n the wild in e-mail headers; replace them with \"iso-8859-1\" that is\n more widely known when conversion fails from/to it.\n\n Will cook in 'next'.\n\n\n* sg/fix-versioncmp-with-common-suffix (2016-09-08) 5 commits\n - versioncmp: cope with common leading parts in versionsort.prereleaseSuffix\n - versioncmp: pass full tagnames to swap_prereleases()\n - t7004-tag: add version sort tests to show prerelease reordering issues\n - t7004-tag: use test_config helper\n - t7004-tag: delete unnecessary tags with test_when_finished\n\n The prereleaseSuffix feature of version comparison that is used in\n \"git tag -l\" did not correctly when two or more prereleases for the\n same release were present (e.g. when 2.0, 2.0-beta1, and 2.0-beta2\n are there and the code needs to compare 2.0-beta1 and 2.0-beta2).\n\n Waiting for a reroll.\n cf. <20160908223727.Horde.jVOOJ278ssZ3qkyjkmyqZD-@webmail.informatik.kit.edu>\n\n\n* jc/pull-rebase-ff (2016-07-28) 1 commit\n - pull: fast-forward \"pull --rebase=true\"\n\n \"git pull --rebase\", when there is no new commits on our side since\n we forked from the upstream, should be able to fast-forward without\n invoking \"git rebase\", but it didn't.\n\n Needs a real log message and a few tests.\n\n\n* jc/merge-drop-old-syntax (2015-04-29) 1 commit\n (merged to 'next' on 2016-10-11 at 8928c8b9b3)\n + merge: drop 'git merge HEAD ' syntax\n\n Stop supporting \"git merge HEAD \" syntax that has\n been deprecated since October 2007, and issues a deprecation\n warning message since v2.5.0.\n\n Will cook in 'next'.\n"},{"id":"306530","messageId":"20161129010538.GA121643@google.com","threadId":"44560","inReplyTo":"xmqqk2bngn03.fsf@gitster.mtv.corp.google.com","subject":"Re: What's cooking in git.git (Nov 2016, #06; Mon, 28)","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-11-29T01:05:38Z","receivedAt":"2016-11-29T01:05:46Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 11/28, Junio C Hamano wrote:\n> * bw/grep-recurse-submodules (2016-11-22) 6 commits\n> - grep: search history of moved submodules\n> - grep: enable recurse-submodules to work on objects\n> - grep: optionally recurse into submodules\n> - grep: add submodules as a grep source type\n> - submodules: load gitmodules file from commit sha1\n> - submodules: add helper functions to determine presence of submodules\n> \n> \"git grep\" learns to optionally recurse into submodules\n> \n> Has anybody else seen t7814 being flakey with this series?\n\nWhich tests in particular are you seeing issues with? I can't see any\nissues running it locally.\n\n-- \nBrandon Williams\n"},{"id":"306532","messageId":"20161129063759.6mgmpqx3kbyuqjwi@sigill.intra.peff.net","threadId":"44560","inReplyTo":"20161129010538.GA121643@google.com","subject":"Re: What's cooking in git.git (Nov 2016, #06; Mon, 28)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-29T06:37:59Z","receivedAt":"2016-11-29T06:39:44Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 28, 2016 at 05:05:38PM -0800, Brandon Williams wrote:\n\n> On 11/28, Junio C Hamano wrote:\n> > * bw/grep-recurse-submodules (2016-11-22) 6 commits\n> > - grep: search history of moved submodules\n> > - grep: enable recurse-submodules to work on objects\n> > - grep: optionally recurse into submodules\n> > - grep: add submodules as a grep source type\n> > - submodules: load gitmodules file from commit sha1\n> > - submodules: add helper functions to determine presence of submodules\n> > \n> > \"git grep\" learns to optionally recurse into submodules\n> > \n> > Has anybody else seen t7814 being flakey with this series?\n> \n> Which tests in particular are you seeing issues with? I can't see any\n> issues running it locally.\n\nIt looks like tests 14 and 15 are racy. The whole script usually passes,\nbut if I run it under my stress script[1], it fails within 5-10 seconds\non one of those two.\n\nThe failures always look like (this one is from test 15, but the one in\ntest 14 is similar):\n\n --- expect 2016-11-29 06:26:37.874664486 +0000\n +++ actual 2016-11-29 06:26:37.878664486 +0000\n @@ -1,2 +1 @@\n -file:foobar\n sub-moved/file:foobar\n\nI haven't dug into it, but I don't see anything obviously racy-looking\nin the test, so presumably it's in the code. Without looking, but\nknowing the nature of the code, I'd guess the probable avenues are:\n\n 1. A read() or write() that gets split under load (just because\n there's processes piping to other processes here).\n\n 2. Grep threads doing more complicated stuff that needs to take a\n lock. You might try building with -fsanitize=thread to see if it\n turns up anything.\n\n-Peff\n\n[1] https://github.com/peff/git/blob/meta/stress\n"},{"id":"306533","messageId":"20161129065125.cwlbkctniy7oshj2@sigill.intra.peff.net","threadId":"44560","inReplyTo":"20161129063759.6mgmpqx3kbyuqjwi@sigill.intra.peff.net","subject":"Re: What's cooking in git.git (Nov 2016, #06; Mon, 28)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-29T06:51:25Z","receivedAt":"2016-11-29T06:51:40Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 29, 2016 at 01:37:59AM -0500, Jeff King wrote:\n\n> 2. Grep threads doing more complicated stuff that needs to take a\n> lock. You might try building with -fsanitize=thread to see if it\n> turns up anything.\n\nI tried this and it didn't find anything useful. It complains about\nmultiple threads calling want_color() at the same time, which you can\nsilence with something like:\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 2c727ef49..d48846f40 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -207,6 +207,12 @@ static void start_threads(struct grep_opt *opt)\n {\n \tint i;\n \n+\t/*\n+\t * trigger want_color() for its side effect of caching the result;\n+\t * otherwise the threads will fight over setting the cache\n+\t */\n+\twant_color(GIT_COLOR_AUTO);\n+\n \tpthread_mutex_init(&grep_mutex, NULL);\n \tpthread_mutex_init(&grep_read_mutex, NULL);\n \tpthread_mutex_init(&grep_attr_mutex, NULL);\n\nBut the problem persists even with that patch, so it is something else.\nIt may still be a threading problem; -fsanitize=thread isn't perfect. I\nalso couldn't get the stress-test to fail when compiled with it. But\nthat may simply mean that the timing of the resulting binary is changed\nenough not to trigger the issue.\n\n-Peff\n"},{"id":"306534","messageId":"20161129065912.xa7itc3os425mr3r@sigill.intra.peff.net","threadId":"44560","inReplyTo":"xmqqk2bngn03.fsf@gitster.mtv.corp.google.com","subject":"Re: What's cooking in git.git (Nov 2016, #06; Mon, 28)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-29T06:59:12Z","receivedAt":"2016-11-29T06:59:31Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 28, 2016 at 04:15:08PM -0800, Junio C Hamano wrote:\n\n> * jk/nofollow-attr-ignore (2016-11-02) 5 commits\n> - exclude: do not respect symlinks for in-tree .gitignore\n> - attr: do not respect symlinks for in-tree .gitattributes\n> - exclude: convert \"check_index\" into a flags field\n> - attr: convert \"macro_ok\" into a flags field\n> - add open_nofollow() helper\n> \n> As we do not follow symbolic links when reading control files like\n> .gitignore and .gitattributes from the index, match the behaviour\n> and not follow symbolic links when reading them from the working\n> tree. This also tightens security a bit by not leaking contents of\n> an unrelated file in the error messages when it is pointed at by\n> one of these files that is a symbolic link.\n> \n> Perhaps we want to cover .gitmodules too with the same mechanism?\n\nYes, sorry I haven't pushed that forward. I started on covering\n.gitmodules, too, but it's much more complicated than the other two,\nbecause we sometimes read them via \"git config -f\". So we have to\nsomehow teach git-config to start using O_NOFOLLOW in those cases.\n\nI'm actually considering scrapping the approach you've queued above, and\njust teaching verify_path() to reject any index entry starting with\n\".git\" that is a symlink.\n\n-Peff\n"},{"id":"306550","messageId":"xmqqfumagmso.fsf@gitster.mtv.corp.google.com","threadId":"44560","inReplyTo":"20161129065912.xa7itc3os425mr3r@sigill.intra.peff.net","subject":"Re: What's cooking in git.git (Nov 2016, #06; Mon, 28)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-29T18:31:51Z","receivedAt":"2016-11-29T18:32:13Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I'm actually considering scrapping the approach you've queued above, and\n> just teaching verify_path() to reject any index entry starting with\n> \".git\" that is a symlink.\n\nHmph, that's a thought.\n"},{"id":"306552","messageId":"20161129183758.wicmteo5vdvygu2q@sigill.intra.peff.net","threadId":"44560","inReplyTo":"xmqqfumagmso.fsf@gitster.mtv.corp.google.com","subject":"Re: What's cooking in git.git (Nov 2016, #06; Mon, 28)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-29T18:37:58Z","receivedAt":"2016-11-29T18:38:07Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 29, 2016 at 10:31:51AM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > I'm actually considering scrapping the approach you've queued above, and\n> > just teaching verify_path() to reject any index entry starting with\n> > \".git\" that is a symlink.\n> \n> Hmph, that's a thought.\n\nI was resistant to it at first because we'll have to deal with all of\nthe headaches of matching case-folding, but if we just match \".git*\" and\nnot \".gitmodules\", \".gitattributes\", etc, it actually gets easier. I\nthink we can basically build off of the existing is_hfs_dotgit() and\nis_ntfs_dotgit() functions.\n\nI haven't written the code yet, though, so there may be complications.\n\n-Peff\n"},{"id":"306559","messageId":"CAGZ79kZbBufaKoJyTQa_DseE5GofMAwb=ujtEYn35r9VEwdJ2g@mail.gmail.com","threadId":"44560","inReplyTo":"xmqqk2bngn03.fsf@gitster.mtv.corp.google.com","subject":"Re: What's cooking in git.git (Nov 2016, #06; Mon, 28)","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-11-29T19:21:39Z","receivedAt":"2016-11-29T19:21:55Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Nov 28, 2016 at 4:15 PM, Junio C Hamano wrote:\n>\n> * sb/push-make-submodule-check-the-default (2016-10-10) 2 commits\n> - push: change submodule default to check when submodules exist\n> - submodule add: extend force flag to add existing repos\n>\n> Turn the default of \"push.recurseSubmodules\" to \"check\" when\n> submodules seem to be in use.\n>\n> Need to rebase on hv/submodule-not-yet-pushed-fix and then consider\n> merging to 'next'.\n\nThe rebase is without merge conflicts, so I assume there is no\nwork needed by me here.\n\n> * sb/submodule-intern-gitdir (2016-11-22) 5 commits\n> - SQUASH\n> - submodule: add embed-git-dir function\n> - test-lib-functions.sh: teach test_commit -C \n> - submodule helper: support super prefix\n> - submodule: use absolute path for computing relative path connecting\n>\n> A new submodule helper \"git submodule embedgitdirs\" to make it\n> easier to move embedded .git/ directory for submodules in a\n> superproject to .git/modules/ (and point the latter with the former\n> that is turned into a \"gitdir:\" file) has been added.\n>\n> Need to read it over again, deal with SQUASH, and may ask for a\n> reroll.\n\nOk, I looked over it again and I may see some issues:\n* it is applicable to all submodules recursive by default, i.e. really\n \"all submodules reachable from this superproject\". I anticipate\n this to be the most relevant use case (i.e. as a preparatory step\n for having e.g. git-checkout work), but there are no other commands\n yet that are recursing into submodules recursively by default. So\n a discussion/disagreement on the default may come up.\n (We also may want to see a --[no-]recursive flag)\n\n* The output is okay-ish, but could be better as it is a mix of\n relative and absolute path:\n\n Migrating git directory of plugins/cookbook from\n '/absolute/path/here/gerrit/plugins/cookbook/.git' to\n '/absolute/path/here/gerrit/.git/modules/plugins/cookbook/'\n\n On the other hand this seems like what the user may need,\n as it is the maximum for trouble shooting\n\n* As this is a subcommand we do not need to add it to command-list.txt\n However we may want to discuss if some submodule commands are\n porcelain (all except for the new embedgitdirs?) and if this new command\n is plumbing. We could also argue the submodule--helper (which needs\n listing in command-list.txt as a plumbing command?) is the plumbing\n equivalent and the \"submodule embedgitdirs\" is the porcelain.\n\n* any other part where we need to add documentation for a new command?\n\nFYI: I have a series cooking internally that adds a new page in\nDocumentation/submodules that introduces the concept of submodules,\nwhich then allows Documentation/git-submodule.txt to be focused on the\nactual command and its options.\n\n>\n> * dt/empty-submodule-in-merge (2016-11-17) 1 commit\n> - submodules: allow empty working-tree dirs in merge/cherry-pick\n>\n> Waiting for review\n\nThat slipped by me. Will review.\n\n> * sb/attr (2016-11-11) 35 commits\n> - completion: clone can initialize specific submodules\n> - clone: add --init-submodule= switch\n> - submodule update: add `--init-default-path` switch\n\nI may end up rerolling these top three patches as its own series\nagain without the underlying pathspec support.\n\nI will investigate if we need the mutex at all for the attribute\ncode or if we can initialize all attrs (in the pathspecs) before the\nthreaded preload_index takes place. That sounds cleaner to me,\nbut I do not prioritize it as high.\n\n> Waiting for review.\n\nThere was some review by Duy and Brandon, I may reroll with just their\nissues addressed.\n\nThanks,\nStefan\n"},{"id":"306560","messageId":"xmqqk2bmf5pv.fsf@gitster.mtv.corp.google.com","threadId":"44560","inReplyTo":"CAGZ79kZbBufaKoJyTQa_DseE5GofMAwb=ujtEYn35r9VEwdJ2g@mail.gmail.com","subject":"Re: What's cooking in git.git (Nov 2016, #06; Mon, 28)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-29T19:26:04Z","receivedAt":"2016-11-29T19:26:21Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> On Mon, Nov 28, 2016 at 4:15 PM, Junio C Hamano wrote:\n>>\n>> * sb/push-make-submodule-check-the-default (2016-10-10) 2 commits\n>> - push: change submodule default to check when submodules exist\n>> - submodule add: extend force flag to add existing repos\n>>\n>> Turn the default of \"push.recurseSubmodules\" to \"check\" when\n>> submodules seem to be in use.\n>>\n>> Need to rebase on hv/submodule-not-yet-pushed-fix and then consider\n>> merging to 'next'.\n>\n> The rebase is without merge conflicts, so I assume there is no\n> work needed by me here.\n\nCorrect.\n\n>> * sb/submodule-intern-gitdir (2016-11-22) 5 commits\n>> - SQUASH\n>> - submodule: add embed-git-dir function\n>> - test-lib-functions.sh: teach test_commit -C \n>> - submodule helper: support super prefix\n>> - submodule: use absolute path for computing relative path connecting\n>>\n>> A new submodule helper \"git submodule embedgitdirs\" to make it\n>> easier to move embedded .git/ directory for submodules in a\n>> superproject to .git/modules/ (and point the latter with the former\n>> that is turned into a \"gitdir:\" file) has been added.\n>>\n>> Need to read it over again, deal with SQUASH, and may ask for a\n>> reroll.\n>\n> Ok, I looked over it again and I may see some issues:\n\nOK then I'll procrastinate on this.\n\n> That slipped by me. Will review.\n>\n>> * sb/attr (2016-11-11) 35 commits\n>> - completion: clone can initialize specific submodules\n>> - clone: add --init-submodule= switch\n>> - submodule update: add `--init-default-path` switch\n>\n> I may end up rerolling these top three patches as its own series\n> again without the underlying pathspec support.\n>\n> I will investigate if we need the mutex at all for the attribute\n> code or if we can initialize all attrs (in the pathspecs) before the\n> threaded preload_index takes place. That sounds cleaner to me,\n> but I do not prioritize it as high.\n\nOK.\n\nThanks.\n"},{"id":"306562","messageId":"CAGZ79kbzLhJDqgv5WxRU-gLEwSSsspEythszAu8pZeEjoWUozg@mail.gmail.com","threadId":"44560","inReplyTo":"xmqqk2bmf5pv.fsf@gitster.mtv.corp.google.com","subject":"Re: What's cooking in git.git (Nov 2016, #06; Mon, 28)","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-11-29T19:29:56Z","receivedAt":"2016-11-29T19:30:02Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Nov 29, 2016 at 11:26 AM, Junio C Hamano wrote:\n\n>>> Need to read it over again, deal with SQUASH, and may ask for a\n>>> reroll.\n>>\n>> Ok, I looked over it again and I may see some issues:\n>\n> OK then I'll procrastinate on this.\n\nThis is not what I had in mind when typing out the issues.\nI'd rather wanted to hear \"these issues are all non-issues,\nyour design is actually correct as a first solution\". ;)\n"},{"id":"306583","messageId":"CAGZ79kb36bOuwhqHZYY5zmOOcqerwSh=HRV91K1FiAQLEzZAGQ@mail.gmail.com","threadId":"44560","inReplyTo":"CAGZ79kZbBufaKoJyTQa_DseE5GofMAwb=ujtEYn35r9VEwdJ2g@mail.gmail.com","subject":"Re: What's cooking in git.git (Nov 2016, #06; Mon, 28)","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-11-30T00:25:30Z","receivedAt":"2016-11-30T00:25:35Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Nov 29, 2016 at 11:21 AM, Stefan Beller wrote:\n>\n>>\n>> * dt/empty-submodule-in-merge (2016-11-17) 1 commit\n>> - submodules: allow empty working-tree dirs in merge/cherry-pick\n>>\n>> Waiting for review\n>\n> That slipped by me. Will review.\n>\n\nI reviewed what you have queued and it still looks good to me.\n"},{"id":"306598","messageId":"20161130195427.GA166433@google.com","threadId":"44560","inReplyTo":"20161129065125.cwlbkctniy7oshj2@sigill.intra.peff.net","subject":"Re: What's cooking in git.git (Nov 2016, #06; Mon, 28)","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-11-30T19:54:27Z","receivedAt":"2016-11-30T19:54:39Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 11/29, Jeff King wrote:\n> On Tue, Nov 29, 2016 at 01:37:59AM -0500, Jeff King wrote:\n> \n> > 2. Grep threads doing more complicated stuff that needs to take a\n> > lock. You might try building with -fsanitize=thread to see if it\n> > turns up anything.\n> \n> I tried this and it didn't find anything useful. It complains about\n> multiple threads calling want_color() at the same time, which you can\n> silence with something like:\n> \n> diff --git a/builtin/grep.c b/builtin/grep.c\n> index 2c727ef49..d48846f40 100644\n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n> @@ -207,6 +207,12 @@ static void start_threads(struct grep_opt *opt)\n> {\n> \tint i;\n> \n> +\t/*\n> +\t * trigger want_color() for its side effect of caching the result;\n> +\t * otherwise the threads will fight over setting the cache\n> +\t */\n> +\twant_color(GIT_COLOR_AUTO);\n> +\n> \tpthread_mutex_init(&grep_mutex, NULL);\n> \tpthread_mutex_init(&grep_read_mutex, NULL);\n> \tpthread_mutex_init(&grep_attr_mutex, NULL);\n> \n> But the problem persists even with that patch, so it is something else.\n> It may still be a threading problem; -fsanitize=thread isn't perfect. I\n> also couldn't get the stress-test to fail when compiled with it. But\n> that may simply mean that the timing of the resulting binary is changed\n> enough not to trigger the issue.\n> \n> -Peff\n\nWith you're stress script I'm able to see the failures. The interesting\nthing is that the entry missing is always from the non-submodule file.\n\n-- \nBrandon Williams\n"},{"id":"306635","messageId":"20161130232823.GA192901@google.com","threadId":"44560","inReplyTo":"20161130195427.GA166433@google.com","subject":"Re: What's cooking in git.git (Nov 2016, #06; Mon, 28)","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-11-30T23:28:23Z","receivedAt":"2016-11-30T23:28:43Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 11/30, Brandon Williams wrote:\n> On 11/29, Jeff King wrote:\n> > On Tue, Nov 29, 2016 at 01:37:59AM -0500, Jeff King wrote:\n> > \n> > > 2. Grep threads doing more complicated stuff that needs to take a\n> > > lock. You might try building with -fsanitize=thread to see if it\n> > > turns up anything.\n> > \n> > I tried this and it didn't find anything useful. It complains about\n> > multiple threads calling want_color() at the same time, which you can\n> > silence with something like:\n> > \n> > diff --git a/builtin/grep.c b/builtin/grep.c\n> > index 2c727ef49..d48846f40 100644\n> > --- a/builtin/grep.c\n> > +++ b/builtin/grep.c\n> > @@ -207,6 +207,12 @@ static void start_threads(struct grep_opt *opt)\n> > {\n> > \tint i;\n> > \n> > +\t/*\n> > +\t * trigger want_color() for its side effect of caching the result;\n> > +\t * otherwise the threads will fight over setting the cache\n> > +\t */\n> > +\twant_color(GIT_COLOR_AUTO);\n> > +\n> > \tpthread_mutex_init(&grep_mutex, NULL);\n> > \tpthread_mutex_init(&grep_read_mutex, NULL);\n> > \tpthread_mutex_init(&grep_attr_mutex, NULL);\n> > \n> > But the problem persists even with that patch, so it is something else.\n> > It may still be a threading problem; -fsanitize=thread isn't perfect. I\n> > also couldn't get the stress-test to fail when compiled with it. But\n> > that may simply mean that the timing of the resulting binary is changed\n> > enough not to trigger the issue.\n> > \n> > -Peff\n> \n> With you're stress script I'm able to see the failures. The interesting\n> thing is that the entry missing is always from the non-submodule file.\n\nSo I couldn't find a race condition in the code. I tracked the problem\nto grep_source_load_file which attempts to run lstat on the file so that\nit can read it into a buffer. The lstat call fails with ENOENT (which\nconveniently is skipped by the if statement which calls error_errno). So\nfor some reason the file cannot be found and read into memory resulting\nin nothing being grep'ed for that particular file (since the buffer is\nNULL).\n\nMaybe there is something wrong with the way I wrote the tests\nthemselves? So I could try rewriting them. Any other ideas?\n\n-- \nBrandon Williams\n"},{"id":"306637","messageId":"20161130233204.ihbrjwwu3yiv4ugq@sigill.intra.peff.net","threadId":"44560","inReplyTo":"20161130232823.GA192901@google.com","subject":"Re: What's cooking in git.git (Nov 2016, #06; Mon, 28)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-30T23:32:04Z","receivedAt":"2016-11-30T23:32:11Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 30, 2016 at 03:28:23PM -0800, Brandon Williams wrote:\n\n> So I couldn't find a race condition in the code. I tracked the problem\n> to grep_source_load_file which attempts to run lstat on the file so that\n> it can read it into a buffer. The lstat call fails with ENOENT (which\n> conveniently is skipped by the if statement which calls error_errno). So\n> for some reason the file cannot be found and read into memory resulting\n> in nothing being grep'ed for that particular file (since the buffer is\n> NULL).\n\nThat's definitely weird. Is it possible that any of the underlying calls\nfrom another thread are using chdir()? I think realpath() make do that\nbehind the scenes, and there may be others.\n\nA full strace from a failing case would be interesting reading. In\ntheory we should be able to get that by running the stress script for\nlong enough. :)\n\n-Peff\n"},{"id":"306639","messageId":"20161130234056.iltitkszvccbjivp@sigill.intra.peff.net","threadId":"44560","inReplyTo":"20161130233204.ihbrjwwu3yiv4ugq@sigill.intra.peff.net","subject":"Re: What's cooking in git.git (Nov 2016, #06; Mon, 28)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-30T23:40:56Z","receivedAt":"2016-11-30T23:41:03Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 30, 2016 at 06:32:04PM -0500, Jeff King wrote:\n\n> On Wed, Nov 30, 2016 at 03:28:23PM -0800, Brandon Williams wrote:\n> \n> > So I couldn't find a race condition in the code. I tracked the problem\n> > to grep_source_load_file which attempts to run lstat on the file so that\n> > it can read it into a buffer. The lstat call fails with ENOENT (which\n> > conveniently is skipped by the if statement which calls error_errno). So\n> > for some reason the file cannot be found and read into memory resulting\n> > in nothing being grep'ed for that particular file (since the buffer is\n> > NULL).\n> \n> That's definitely weird. Is it possible that any of the underlying calls\n> from another thread are using chdir()? I think realpath() make do that\n> behind the scenes, and there may be others.\n> \n> A full strace from a failing case would be interesting reading. In\n> theory we should be able to get that by running the stress script for\n> long enough. :)\n\nActually, it failed pretty much immediately. I guess the extra stracing\nchanges the timing to make the problem _more_ likely.\n\nAnd indeed, I see:\n\n20867 lstat(\"fi:le\", \n20813 <... read resumed> \"\", 232) = 0\n20871 futex(0x558cdec8b164, FUTEX_WAIT_PRIVATE, 7, NULL \n20813 close(7 \n20870 <... futex resumed> ) = 0\n20869 lstat(\".gitmodules\", \n20813 <... close resumed> ) = 0\n20865 set_robust_list(0x7f1df92579e0, 24 \n20813 lstat(\"su:b/../.git/modules/su:b/commondir\", 0x7ffecc8b3ac0) = -1 ENOENT (No such file or directory)\n20865 <... set_robust_list resumed> ) = 0\n20868 set_robust_list(0x7f1df7a549e0, 24 \n20813 access(\"su:b/../.git/modules/su:b/objects\", X_OK) = 0\n20813 access(\"su:b/../.git/modules/su:b/refs\", X_OK) = 0\n20813 stat(\"su:b/../.git/modules/su:b\", {st_mode=S_IFDIR|0755, st_size=280, ...}) = 0\n20813 getcwd(\"/var/ram/git-stress/root-4/trash directory.t7814-grep-recurse-submodules/parent\", 129) = 80\n20869 <... lstat resumed> {st_mode=S_IFREG|0644, st_size=47, ...}) = 0\n20813 chdir(\"su:b/../.git/modules/su:b\") = 0\n20869 open(\".gitmodules\", O_RDONLY \n20813 getcwd( \n20867 <... lstat resumed> 0x7f1df8254cf0) = -1 ENOENT (No such file or directory)\n\nwhere 20813 and 20867 are two threads of the main process. One is doing\nthe lstat and the other calls chdir at the same moment.\n\n-Peff\n"},{"id":"306640","messageId":"20161130234248.GB192901@google.com","threadId":"44560","inReplyTo":"20161130234056.iltitkszvccbjivp@sigill.intra.peff.net","subject":"Re: What's cooking in git.git (Nov 2016, #06; Mon, 28)","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-11-30T23:42:48Z","receivedAt":"2016-11-30T23:42:57Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 11/30, Jeff King wrote:\n> On Wed, Nov 30, 2016 at 06:32:04PM -0500, Jeff King wrote:\n> \n> > On Wed, Nov 30, 2016 at 03:28:23PM -0800, Brandon Williams wrote:\n> > \n> > > So I couldn't find a race condition in the code. I tracked the problem\n> > > to grep_source_load_file which attempts to run lstat on the file so that\n> > > it can read it into a buffer. The lstat call fails with ENOENT (which\n> > > conveniently is skipped by the if statement which calls error_errno). So\n> > > for some reason the file cannot be found and read into memory resulting\n> > > in nothing being grep'ed for that particular file (since the buffer is\n> > > NULL).\n> > \n> > That's definitely weird. Is it possible that any of the underlying calls\n> > from another thread are using chdir()? I think realpath() make do that\n> > behind the scenes, and there may be others.\n> > \n> > A full strace from a failing case would be interesting reading. In\n> > theory we should be able to get that by running the stress script for\n> > long enough. :)\n> \n> Actually, it failed pretty much immediately. I guess the extra stracing\n> changes the timing to make the problem _more_ likely.\n> \n> And indeed, I see:\n> \n> 20867 lstat(\"fi:le\", \n> 20813 <... read resumed> \"\", 232) = 0\n> 20871 futex(0x558cdec8b164, FUTEX_WAIT_PRIVATE, 7, NULL \n> 20813 close(7 \n> 20870 <... futex resumed> ) = 0\n> 20869 lstat(\".gitmodules\", \n> 20813 <... close resumed> ) = 0\n> 20865 set_robust_list(0x7f1df92579e0, 24 \n> 20813 lstat(\"su:b/../.git/modules/su:b/commondir\", 0x7ffecc8b3ac0) = -1 ENOENT (No such file or directory)\n> 20865 <... set_robust_list resumed> ) = 0\n> 20868 set_robust_list(0x7f1df7a549e0, 24 \n> 20813 access(\"su:b/../.git/modules/su:b/objects\", X_OK) = 0\n> 20813 access(\"su:b/../.git/modules/su:b/refs\", X_OK) = 0\n> 20813 stat(\"su:b/../.git/modules/su:b\", {st_mode=S_IFDIR|0755, st_size=280, ...}) = 0\n> 20813 getcwd(\"/var/ram/git-stress/root-4/trash directory.t7814-grep-recurse-submodules/parent\", 129) = 80\n> 20869 <... lstat resumed> {st_mode=S_IFREG|0644, st_size=47, ...}) = 0\n> 20813 chdir(\"su:b/../.git/modules/su:b\") = 0\n> 20869 open(\".gitmodules\", O_RDONLY \n> 20813 getcwd( \n> 20867 <... lstat resumed> 0x7f1df8254cf0) = -1 ENOENT (No such file or directory)\n> \n> where 20813 and 20867 are two threads of the main process. One is doing\n> the lstat and the other calls chdir at the same moment.\n> \n> -Peff\n\nYeah so it looks like the start_command function calls chdir. Which\nmeans any uses of the run-command interface are not thread safe....\n\nFor now the work around could be to just pass \"-C \" to the child\nprocess instead of relying on run-command to chdir.\n\n-- \nBrandon Williams\n"},{"id":"306641","messageId":"CAGZ79kadJ+8CyyDTM=ZLLCirJYhFwPQ0jxZRPmGY4jCh9UB4sA@mail.gmail.com","threadId":"44560","inReplyTo":"20161130234056.iltitkszvccbjivp@sigill.intra.peff.net","subject":"Re: What's cooking in git.git (Nov 2016, #06; Mon, 28)","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-11-30T23:43:15Z","receivedAt":"2016-11-30T23:43:21Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Nov 30, 2016 at 3:40 PM, Jeff King wrote:\n> On Wed, Nov 30, 2016 at 06:32:04PM -0500, Jeff King wrote:\n>\n>> On Wed, Nov 30, 2016 at 03:28:23PM -0800, Brandon Williams wrote:\n>>\n>> > So I couldn't find a race condition in the code. I tracked the problem\n>> > to grep_source_load_file which attempts to run lstat on the file so that\n>> > it can read it into a buffer. The lstat call fails with ENOENT (which\n>> > conveniently is skipped by the if statement which calls error_errno). So\n>> > for some reason the file cannot be found and read into memory resulting\n>> > in nothing being grep'ed for that particular file (since the buffer is\n>> > NULL).\n>>\n>> That's definitely weird. Is it possible that any of the underlying calls\n>> from another thread are using chdir()? I think realpath() make do that\n>> behind the scenes, and there may be others.\n>>\n>> A full strace from a failing case would be interesting reading. In\n>> theory we should be able to get that by running the stress script for\n>> long enough. :)\n>\n> Actually, it failed pretty much immediately. I guess the extra stracing\n> changes the timing to make the problem _more_ likely.\n>\n> And indeed, I see:\n>\n> 20867 lstat(\"fi:le\", \n> 20813 <... read resumed> \"\", 232) = 0\n> 20871 futex(0x558cdec8b164, FUTEX_WAIT_PRIVATE, 7, NULL \n> 20813 close(7 \n> 20870 <... futex resumed> ) = 0\n> 20869 lstat(\".gitmodules\", \n> 20813 <... close resumed> ) = 0\n> 20865 set_robust_list(0x7f1df92579e0, 24 \n> 20813 lstat(\"su:b/../.git/modules/su:b/commondir\", 0x7ffecc8b3ac0) = -1 ENOENT (No such file or directory)\n> 20865 <... set_robust_list resumed> ) = 0\n> 20868 set_robust_list(0x7f1df7a549e0, 24 \n> 20813 access(\"su:b/../.git/modules/su:b/objects\", X_OK) = 0\n> 20813 access(\"su:b/../.git/modules/su:b/refs\", X_OK) = 0\n> 20813 stat(\"su:b/../.git/modules/su:b\", {st_mode=S_IFDIR|0755, st_size=280, ...}) = 0\n> 20813 getcwd(\"/var/ram/git-stress/root-4/trash directory.t7814-grep-recurse-submodules/parent\", 129) = 80\n> 20869 <... lstat resumed> {st_mode=S_IFREG|0644, st_size=47, ...}) = 0\n> 20813 chdir(\"su:b/../.git/modules/su:b\") = 0\n> 20869 open(\".gitmodules\", O_RDONLY \n> 20813 getcwd( \n> 20867 <... lstat resumed> 0x7f1df8254cf0) = -1 ENOENT (No such file or directory)\n>\n> where 20813 and 20867 are two threads of the main process. One is doing\n> the lstat and the other calls chdir at the same moment.\n>\n\nLessons learned here:\nThe run-command API is not thread safe when used with setting the directory.\nIf you need to run a thing in a threaded environment run\ngit -C ... such that the child chdirs.\n\nAre there any other threaded environments that run things with .dir set?\n"},{"id":"306642","messageId":"20161130234636.6az7xfywzprpn6ly@sigill.intra.peff.net","threadId":"44560","inReplyTo":"20161130234248.GB192901@google.com","subject":"Re: What's cooking in git.git (Nov 2016, #06; Mon, 28)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-30T23:46:36Z","receivedAt":"2016-11-30T23:47:01Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 30, 2016 at 03:42:48PM -0800, Brandon Williams wrote:\n\n> > where 20813 and 20867 are two threads of the main process. One is doing\n> > the lstat and the other calls chdir at the same moment.\n>\n> Yeah so it looks like the start_command function calls chdir. Which\n> means any uses of the run-command interface are not thread safe....\n\nThat seems crazy. The chdir should be happening on the child side of the\nfork (and looking at the code, it seems to be the case). And on the\nWindows side, without fork, it's an option to the spawn call, which\nmakes sense.\n\n> For now the work around could be to just pass \"-C \" to the child\n> process instead of relying on run-command to chdir.\n\nYeah, that would push it after the exec. I just don't understand why\nthat would be necessary.\n\n-Peff\n"},{"id":"306644","messageId":"20161130235756.GC192901@google.com","threadId":"44560","inReplyTo":"20161130234636.6az7xfywzprpn6ly@sigill.intra.peff.net","subject":"Re: What's cooking in git.git (Nov 2016, #06; Mon, 28)","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-11-30T23:57:56Z","receivedAt":"2016-11-30T23:58:04Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 11/30, Jeff King wrote:\n> On Wed, Nov 30, 2016 at 03:42:48PM -0800, Brandon Williams wrote:\n> \n> > > where 20813 and 20867 are two threads of the main process. One is doing\n> > > the lstat and the other calls chdir at the same moment.\n> >\n> > Yeah so it looks like the start_command function calls chdir. Which\n> > means any uses of the run-command interface are not thread safe....\n> \n> That seems crazy. The chdir should be happening on the child side of the\n> fork (and looking at the code, it seems to be the case). And on the\n> Windows side, without fork, it's an option to the spawn call, which\n> makes sense.\n> \n> > For now the work around could be to just pass \"-C \" to the child\n> > process instead of relying on run-command to chdir.\n> \n> Yeah, that would push it after the exec. I just don't understand why\n> that would be necessary.\n> \n> -Peff\n\nYou're right, I jumped the gun. That doesn't seem to fix the problem\n(as I'm still seeing the same failure).\n\n\n-- \nBrandon Williams\n"},{"id":"306645","messageId":"20161130235952.4j63gkdlbobjitdb@sigill.intra.peff.net","threadId":"44560","inReplyTo":"20161130234636.6az7xfywzprpn6ly@sigill.intra.peff.net","subject":"Re: What's cooking in git.git (Nov 2016, #06; Mon, 28)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-30T23:59:52Z","receivedAt":"2016-11-30T23:59:59Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 30, 2016 at 06:46:36PM -0500, Jeff King wrote:\n\n> > For now the work around could be to just pass \"-C \" to the child\n> > process instead of relying on run-command to chdir.\n> \n> Yeah, that would push it after the exec. I just don't understand why\n> that would be necessary.\n\nHmm. It still seems to fail, even with the workaround (the patch in\nrun-command is there to make sure there's not some other call that we're\nnot catching):\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 2c727ef49..3323a3e7f 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -553,6 +553,7 @@ static int grep_submodule_launch(struct grep_opt *opt,\n \targv_array_pushf(&cp.args, \"--super-prefix=%s%s/\",\n \t\t\t super_prefix ? super_prefix : \"\",\n \t\t\t name);\n+\targv_array_pushl(&cp.args, \"-C\", gs->path, NULL);\n \targv_array_push(&cp.args, \"grep\");\n \n \t/*\n@@ -586,7 +587,6 @@ static int grep_submodule_launch(struct grep_opt *opt,\n \t}\n \n \tcp.git_cmd = 1;\n-\tcp.dir = gs->path;\n \n \t/*\n \t * Capture output to output buffer and check the return code from the\ndiff --git a/run-command.c b/run-command.c\nindex 5a4dbb66d..d040f4f77 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -393,6 +393,8 @@ int start_command(struct child_process *cmd)\n \t\t\tclose(cmd->out);\n \t\t}\n \n+\t\tif (cmd->dir && git_env_bool(\"GIT_NO_CHDIR\", 0))\n+\t\t\tdie(\"temporarily disallowing chdir\");\n \t\tif (cmd->dir && chdir(cmd->dir))\n \t\t\tdie_errno(\"exec '%s': cd to '%s' failed\", cmd->argv[0],\n \t\t\t cmd->dir);\ndiff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh\nindex 050777186..591ff74ed 100755\n--- a/t/t7814-grep-recurse-submodules.sh\n+++ b/t/t7814-grep-recurse-submodules.sh\n@@ -175,7 +175,7 @@ test_expect_success 'grep recurse submodule colon in name' '\n \tfi:le:foobar\n \tsu:b/fi:le:foobar\n \tEOF\n-\tgit -C parent grep -e \"foobar\" --recurse-submodules >actual &&\n+\tGIT_NO_CHDIR=1 strace -o foo.out -f git -C parent grep -e \"foobar\" --recurse-submodules >actual &&\n \ttest_cmp expect actual &&\n \n \tcat >expect <<-\\EOF &&\n\n\nSo I think there is some other chdir(). I'm not sure if there is an easy\nway to get a backtrace on every call to chdir() in every thread. I'm\nsure somebody more clever than me could figure out how to make gdb do it\nautomatically, but it might be workable manually. I think the chdir was\nin the main thread.\n\n-Peff\n"},{"id":"306646","messageId":"20161201000437.altjlcsn4u7rwe5c@sigill.intra.peff.net","threadId":"44560","inReplyTo":"20161130235952.4j63gkdlbobjitdb@sigill.intra.peff.net","subject":"Re: What's cooking in git.git (Nov 2016, #06; Mon, 28)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-01T00:04:38Z","receivedAt":"2016-12-01T00:04:44Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 30, 2016 at 06:59:52PM -0500, Jeff King wrote:\n\n> So I think there is some other chdir(). I'm not sure if there is an easy\n> way to get a backtrace on every call to chdir() in every thread. I'm\n> sure somebody more clever than me could figure out how to make gdb do it\n> automatically, but it might be workable manually. I think the chdir was\n> in the main thread.\n\nAh, that turned out to be quite easy. The culprit is probably:\n\n(gdb) bt\n#0 chdir () at ../sysdeps/unix/syscall-template.S:84\n#1 0x00005555555fe259 in real_path_internal (path=0x5555559f6b30 \"su:b/../.git/modules/su:b\", die_on_error=1)\n at abspath.c:84\n#2 0x00005555555fe48a in real_path (path=0x5555559f6b30 \"su:b/../.git/modules/su:b\") at abspath.c:135\n#3 0x00005555556d09e6 in read_gitfile_gently (path=0x5555559f6ac0 \"su:b/.git\", return_error_code=0x0)\n at setup.c:555\n#4 0x00005555556d19cf in resolve_gitdir (suspect=0x5555559f6ac0 \"su:b/.git\") at setup.c:1021\n#5 0x00005555556e7e34 in is_submodule_populated (path=0x5555559f5ec8 \"su:b\") at submodule.c:244\n#6 0x00005555555a0f05 in grep_submodule (opt=0x7fffffffd8b0, sha1=0x0, filename=0x5555559f5ec8 \"su:b\", \n path=0x5555559f5ec8 \"su:b\") at builtin/grep.c:619\n#7 0x00005555555a12ac in grep_cache (opt=0x7fffffffd8b0, pathspec=0x7fffffffd880, cached=0) at builtin/grep.c:700\n#8 0x00005555555a36cb in cmd_grep (argc=0, argv=0x7fffffffdf40, prefix=0x0) at builtin/grep.c:1257\n#9 0x000055555556603b in run_builtin (p=0x5555559b3ad8 , argc=4, argv=0x7fffffffdf40) at git.c:373\n#10 0x00005555555662bc in handle_builtin (argc=4, argv=0x7fffffffdf40) at git.c:572\n#11 0x000055555556641a in run_argv (argcp=0x7fffffffddfc, argv=0x7fffffffddf0) at git.c:630\n#12 0x00005555555665a8 in cmd_main (argc=4, argv=0x7fffffffdf40) at git.c:702\n#13 0x00005555555fde47 in main (argc=7, argv=0x7fffffffdf28) at common-main.c:40\n\nSo is_submodule_populated() needs to take a lock. But what's really\ngross is that the _other_ threads need to lock just to call lstat().\nPresumably it could be done as a reader/writer type of lock where many\n\"reader\" threads can take the \"I need to lstat()\" lock simultaneously,\nbut block when an \"I'm going to chdir()\" writer holds it.\n\n-Peff\n"},{"id":"306647","messageId":"20161201000605.GD192901@google.com","threadId":"44560","inReplyTo":"20161130235952.4j63gkdlbobjitdb@sigill.intra.peff.net","subject":"Re: What's cooking in git.git (Nov 2016, #06; Mon, 28)","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-12-01T00:06:05Z","receivedAt":"2016-12-01T00:07:05Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 11/30, Jeff King wrote:\n> So I think there is some other chdir(). I'm not sure if there is an easy\n> way to get a backtrace on every call to chdir() in every thread. I'm\n> sure somebody more clever than me could figure out how to make gdb do it\n> automatically, but it might be workable manually. I think the chdir was\n> in the main thread.\n> \n> -Peff\n\nYeah maybe we're missing something else...\n\nHow did you run strace with your stress script?\n\n-- \nBrandon Williams\n"},{"id":"306648","messageId":"20161201000824.GE192901@google.com","threadId":"44560","inReplyTo":"20161201000437.altjlcsn4u7rwe5c@sigill.intra.peff.net","subject":"Re: What's cooking in git.git (Nov 2016, #06; Mon, 28)","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-12-01T00:08:24Z","receivedAt":"2016-12-01T00:08:44Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 11/30, Jeff King wrote:\n> On Wed, Nov 30, 2016 at 06:59:52PM -0500, Jeff King wrote:\n> \n> > So I think there is some other chdir(). I'm not sure if there is an easy\n> > way to get a backtrace on every call to chdir() in every thread. I'm\n> > sure somebody more clever than me could figure out how to make gdb do it\n> > automatically, but it might be workable manually. I think the chdir was\n> > in the main thread.\n> \n> Ah, that turned out to be quite easy. The culprit is probably:\n> \n> (gdb) bt\n> #0 chdir () at ../sysdeps/unix/syscall-template.S:84\n> #1 0x00005555555fe259 in real_path_internal (path=0x5555559f6b30 \"su:b/../.git/modules/su:b\", die_on_error=1)\n> at abspath.c:84\n> #2 0x00005555555fe48a in real_path (path=0x5555559f6b30 \"su:b/../.git/modules/su:b\") at abspath.c:135\n> #3 0x00005555556d09e6 in read_gitfile_gently (path=0x5555559f6ac0 \"su:b/.git\", return_error_code=0x0)\n> at setup.c:555\n> #4 0x00005555556d19cf in resolve_gitdir (suspect=0x5555559f6ac0 \"su:b/.git\") at setup.c:1021\n> #5 0x00005555556e7e34 in is_submodule_populated (path=0x5555559f5ec8 \"su:b\") at submodule.c:244\n> #6 0x00005555555a0f05 in grep_submodule (opt=0x7fffffffd8b0, sha1=0x0, filename=0x5555559f5ec8 \"su:b\", \n> path=0x5555559f5ec8 \"su:b\") at builtin/grep.c:619\n> #7 0x00005555555a12ac in grep_cache (opt=0x7fffffffd8b0, pathspec=0x7fffffffd880, cached=0) at builtin/grep.c:700\n> #8 0x00005555555a36cb in cmd_grep (argc=0, argv=0x7fffffffdf40, prefix=0x0) at builtin/grep.c:1257\n> #9 0x000055555556603b in run_builtin (p=0x5555559b3ad8 , argc=4, argv=0x7fffffffdf40) at git.c:373\n> #10 0x00005555555662bc in handle_builtin (argc=4, argv=0x7fffffffdf40) at git.c:572\n> #11 0x000055555556641a in run_argv (argcp=0x7fffffffddfc, argv=0x7fffffffddf0) at git.c:630\n> #12 0x00005555555665a8 in cmd_main (argc=4, argv=0x7fffffffdf40) at git.c:702\n> #13 0x00005555555fde47 in main (argc=7, argv=0x7fffffffdf28) at common-main.c:40\n> \n> So is_submodule_populated() needs to take a lock. But what's really\n> gross is that the _other_ threads need to lock just to call lstat().\n> Presumably it could be done as a reader/writer type of lock where many\n> \"reader\" threads can take the \"I need to lstat()\" lock simultaneously,\n> but block when an \"I'm going to chdir()\" writer holds it.\n> \n> -Peff\n\nOh interesting, I wonder if there is a way to not have to perform a\nchdir since taking a lock to lstat wouldn't be ideal.\n\nThanks for helping out with this!\n\n-- \nBrandon Williams\n"},{"id":"306649","messageId":"CAGZ79kaaJgsa8SsvWiGkmKQU7Wv_PD6O50ybm=rxPySk=z9N7Q@mail.gmail.com","threadId":"44560","inReplyTo":"20161201000824.GE192901@google.com","subject":"Re: What's cooking in git.git (Nov 2016, #06; Mon, 28)","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-01T00:14:41Z","receivedAt":"2016-12-01T00:14:47Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> Oh interesting, I wonder if there is a way to not have to perform a\n> chdir since taking a lock to lstat wouldn't be ideal.\n\nI think we could rewrite is_submodule_populated to be\n\nint is_submodule_populated_cheap_with_no_chdir(char *path)\n{\n return stat(path + \".git\")\n}\n\ni.e. just take the presence of the .git file/dir as a hint to run\nthe child process?\n"},{"id":"306651","messageId":"20161201001901.mjof24j2qsklnf6f@sigill.intra.peff.net","threadId":"44560","inReplyTo":"20161201000824.GE192901@google.com","subject":"Re: What's cooking in git.git (Nov 2016, #06; Mon, 28)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-01T00:19:02Z","receivedAt":"2016-12-01T00:19:09Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 30, 2016 at 04:06:05PM -0800, Brandon Williams wrote:\n\n> On 11/30, Jeff King wrote:\n> > So I think there is some other chdir(). I'm not sure if there is an easy\n> > way to get a backtrace on every call to chdir() in every thread. I'm\n> > sure somebody more clever than me could figure out how to make gdb do it\n> > automatically, but it might be workable manually. I think the chdir was\n> > in the main thread.\n> > \n> > -Peff\n> \n> Yeah maybe we're missing something else...\n> \n> How did you run strace with your stress script?\n\nIt's hidden in the patch I sent a moment ago, but basically just \"strace\n-o foo.out\" will dump the trace in the trash directory. After the stress\nscript runs, you can \"cat fail/trash*/foo.out\".\n\n> > (gdb) bt\n> > #0 chdir () at ../sysdeps/unix/syscall-template.S:84\n> > #1 0x00005555555fe259 in real_path_internal (path=0x5555559f6b30 \"su:b/../.git/modules/su:b\", die_on_error=1)\n> > at abspath.c:84\n> > #2 0x00005555555fe48a in real_path (path=0x5555559f6b30 \"su:b/../.git/modules/su:b\") at abspath.c:135\n> > #3 0x00005555556d09e6 in read_gitfile_gently (path=0x5555559f6ac0 \"su:b/.git\", return_error_code=0x0)\n> > at setup.c:555\n> > #4 0x00005555556d19cf in resolve_gitdir (suspect=0x5555559f6ac0 \"su:b/.git\") at setup.c:1021\n> > #5 0x00005555556e7e34 in is_submodule_populated (path=0x5555559f5ec8 \"su:b\") at submodule.c:244\n> > #6 0x00005555555a0f05 in grep_submodule (opt=0x7fffffffd8b0, sha1=0x0, filename=0x5555559f5ec8 \"su:b\", \n> > path=0x5555559f5ec8 \"su:b\") at builtin/grep.c:619\n> > #7 0x00005555555a12ac in grep_cache (opt=0x7fffffffd8b0, pathspec=0x7fffffffd880, cached=0) at builtin/grep.c:700\n> > #8 0x00005555555a36cb in cmd_grep (argc=0, argv=0x7fffffffdf40, prefix=0x0) at builtin/grep.c:1257\n> > #9 0x000055555556603b in run_builtin (p=0x5555559b3ad8 , argc=4, argv=0x7fffffffdf40) at git.c:373\n> > #10 0x00005555555662bc in handle_builtin (argc=4, argv=0x7fffffffdf40) at git.c:572\n> > #11 0x000055555556641a in run_argv (argcp=0x7fffffffddfc, argv=0x7fffffffddf0) at git.c:630\n> > #12 0x00005555555665a8 in cmd_main (argc=4, argv=0x7fffffffdf40) at git.c:702\n> > #13 0x00005555555fde47 in main (argc=7, argv=0x7fffffffdf28) at common-main.c:40\n> > \n> > So is_submodule_populated() needs to take a lock. But what's really\n> > gross is that the _other_ threads need to lock just to call lstat().\n> > Presumably it could be done as a reader/writer type of lock where many\n> > \"reader\" threads can take the \"I need to lstat()\" lock simultaneously,\n> > but block when an \"I'm going to chdir()\" writer holds it.\n> \n> Oh interesting, I wonder if there is a way to not have to perform a\n> chdir since taking a lock to lstat wouldn't be ideal.\n\nI don't think so. It comes from real_path(), which needs to either\nchdir(), or start interpreting symbolic links itself (and madness that\nway lies).\n\nI think with a reader/writer lock as I described it wouldn't be too bad.\nThe common case would pay only the locking cost and not ever block,\nsince submodules are rare (and they're super-heavyweight to descend into\nanyway).\n\nI think putting it at the individual lstat() would be way too low, but\nprobably you could do it right before calling grep_source(). It may even\nbe possible to do some of the submodule work ahead of time while holding\ngrep_lock().\n\n> Thanks for helping out with this!\n\nI wasn't planning on it, but this turned into an intriguing puzzle. ;)\n\n-Peff\n"},{"id":"306656","messageId":"20161201011439.GF192901@google.com","threadId":"44560","inReplyTo":"CAGZ79kaaJgsa8SsvWiGkmKQU7Wv_PD6O50ybm=rxPySk=z9N7Q@mail.gmail.com","subject":"Re: What's cooking in git.git (Nov 2016, #06; Mon, 28)","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-12-01T01:14:39Z","receivedAt":"2016-12-01T01:21:55Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 11/30, Stefan Beller wrote:\n> > Oh interesting, I wonder if there is a way to not have to perform a\n> > chdir since taking a lock to lstat wouldn't be ideal.\n> \n> I think we could rewrite is_submodule_populated to be\n> \n> int is_submodule_populated_cheap_with_no_chdir(char *path)\n> {\n> return stat(path + \".git\")\n> }\n> \n> i.e. just take the presence of the .git file/dir as a hint to run\n> the child process?\n\nI like this approach, its a quick (thread-safe) check to see if the\nsubmodule is interesting. If there happens to be an error with the\nsubmodule's .git file/directory then the child process will fail out.\n\n-- \nBrandon Williams\n"},{"id":"306675","messageId":"fa54bb08-4206-ebd5-6808-f7de6cf4b9a2@kdbg.org","threadId":"44560","inReplyTo":"20161130234056.iltitkszvccbjivp@sigill.intra.peff.net","subject":"Re: What's cooking in git.git (Nov 2016, #06; Mon, 28)","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-12-01T07:09:16Z","receivedAt":"2016-12-01T07:09:27Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 01.12.2016 um 00:40 schrieb Jeff King:\n> 20813 access(\"su:b/../.git/modules/su:b/refs\", X_OK) = 0\n\nSide note: where does this weirdness \"su:b\" come from? (I haven't looked \nat any of the patches, yet, let alone tested.) Colons in file or \ndirectory names won't work on Windows (in the way one would expect).\n\n-- Hannes\n\n"},{"id":"306676","messageId":"20161201071906.unrerembedeja3wa@sigill.intra.peff.net","threadId":"44560","inReplyTo":"fa54bb08-4206-ebd5-6808-f7de6cf4b9a2@kdbg.org","subject":"Re: What's cooking in git.git (Nov 2016, #06; Mon, 28)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-01T07:19:06Z","receivedAt":"2016-12-01T07:19:13Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 01, 2016 at 08:09:16AM +0100, Johannes Sixt wrote:\n\n> Am 01.12.2016 um 00:40 schrieb Jeff King:\n> > 20813 access(\"su:b/../.git/modules/su:b/refs\", X_OK) = 0\n> \n> Side note: where does this weirdness \"su:b\" come from? (I haven't looked at\n> any of the patches, yet, let alone tested.) Colons in file or directory\n> names won't work on Windows (in the way one would expect).\n\nIt's explicitly used in the test, I assume to check that the recursive\ngrep is not confused into treating the name as a tree-ish.\n\nI think it would have to either be marked with a prereq on Windows, or\nmodified to do the whole thing in-index (and use \"grep --cached\").\n\n-Peff\n"},{"id":"306678","messageId":"20161201083005.dui572o4jxsqacas@sigill.intra.peff.net","threadId":"44560","inReplyTo":"xmqqk2bngn03.fsf@gitster.mtv.corp.google.com","subject":"bw/transport-protocol-policy","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-01T08:30:05Z","receivedAt":"2016-12-01T08:36:53Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 28, 2016 at 04:15:08PM -0800, Junio C Hamano wrote:\n\n> * bw/transport-protocol-policy (2016-11-09) 2 commits\n> (merged to 'next' on 2016-11-16 at 1391d3eeed)\n> + transport: add protocol policy config option\n> + lib-proto-disable: variable name fix\n> \n> Finer-grained control of what protocols are allowed for transports\n> during clone/fetch/push have been enabled via a new configuration\n> mechanism.\n> \n> Will cook in 'next'.\n\nI was looking at the way the http code feeds protocol restrictions to\nCURLOPT_REDIR_PROTOCOLS, and I think this topic is missing two elements:\n\n 1. The new policy config lets you say \"only allow this protocol when\n the user specifies it\". But when http.c calls is_transport_allowed(),\n the latter has no idea that we are asking it about potential\n redirects (which obviously do _not_ come from the user), and would\n erroneously allow them.\n\n I think this needs fixed before the topic is merged. It's not a\n regression, as it only comes into play if you use the new policy\n config. But it is a minor security hole in the new feature.\n\n 2. If your curl is too old to support CURLOPT_REDIR_PROTOCOLS, we will\n warn if there is a protocol whitelist in effect. But that check\n only covers the environment whitelist, and we do not warn if you\n restrict other protocols.\n\n I actually think this should probably just warn indiscriminately.\n Even without a Git protocol whitelist specified, the code serves to\n prevent curl from redirecting to bizarre protocols like smtp. The\n affected curl versions are from 2009 and prior, so I kind of doubt\n it matters much either way (I'm actually tempted to suggest we bump\n the minimum curl version there; there's a ton of #ifdef cruft going\n back to 2002-era versions of libcurl).\n\n-Peff\n"},{"id":"306700","messageId":"20161201181415.GA54082@google.com","threadId":"44560","inReplyTo":"20161201083005.dui572o4jxsqacas@sigill.intra.peff.net","subject":"Re: bw/transport-protocol-policy","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-12-01T18:14:15Z","receivedAt":"2016-12-01T18:14:28Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 12/01, Jeff King wrote:\n> On Mon, Nov 28, 2016 at 04:15:08PM -0800, Junio C Hamano wrote:\n> \n> > * bw/transport-protocol-policy (2016-11-09) 2 commits\n> > (merged to 'next' on 2016-11-16 at 1391d3eeed)\n> > + transport: add protocol policy config option\n> > + lib-proto-disable: variable name fix\n> > \n> > Finer-grained control of what protocols are allowed for transports\n> > during clone/fetch/push have been enabled via a new configuration\n> > mechanism.\n> > \n> > Will cook in 'next'.\n> \n> I was looking at the way the http code feeds protocol restrictions to\n> CURLOPT_REDIR_PROTOCOLS, and I think this topic is missing two elements:\n> \n> 1. The new policy config lets you say \"only allow this protocol when\n> the user specifies it\". But when http.c calls is_transport_allowed(),\n> the latter has no idea that we are asking it about potential\n> redirects (which obviously do _not_ come from the user), and would\n> erroneously allow them.\n> \n> I think this needs fixed before the topic is merged. It's not a\n> regression, as it only comes into play if you use the new policy\n> config. But it is a minor security hole in the new feature.\n\nI agree and it should be an easy fix. We can just add a parameter like\nso:\n\ndiff --git a/transport.c b/transport.c\nindex 2c0ec76..d38d50f 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -723,7 +723,7 @@ static enum protocol_allow_config get_protocol_config(const char *type)\n \treturn PROTOCOL_ALLOW_USER_ONLY;\n }\n \n-int is_transport_allowed(const char *type)\n+int is_transport_allowed(const char *type, int redirect)\n {\n \tconst struct string_list *whitelist = protocol_whitelist();\n \tif (whitelist)\n@@ -735,7 +735,7 @@ int is_transport_allowed(const char *type)\n \tcase PROTOCOL_ALLOW_NEVER:\n \t\treturn 0;\n \tcase PROTOCOL_ALLOW_USER_ONLY:\n-\t\treturn git_env_bool(\"GIT_PROTOCOL_FROM_USER\", 1);\n+\t\treturn git_env_bool(\"GIT_PROTOCOL_FROM_USER\", !redirect);\n \t}\n \n \tdie(\"BUG: invalid protocol_allow_config type\");\n\nThat way the libcurl code can say it is asking if it is ok to redirect\nto that protocol.\n\n> \n> 2. If your curl is too old to support CURLOPT_REDIR_PROTOCOLS, we will\n> warn if there is a protocol whitelist in effect. But that check\n> only covers the environment whitelist, and we do not warn if you\n> restrict other protocols.\n> \n> I actually think this should probably just warn indiscriminately.\n> Even without a Git protocol whitelist specified, the code serves to\n> prevent curl from redirecting to bizarre protocols like smtp. The\n> affected curl versions are from 2009 and prior, so I kind of doubt\n> it matters much either way (I'm actually tempted to suggest we bump\n> the minimum curl version there; there's a ton of #ifdef cruft going\n> back to 2002-era versions of libcurl).\n\nWe should switch to warning all the time since this series adds in\ndefault whitelisted/blacklisted protocols anyways.\n\n-- \nBrandon Williams\n"},{"id":"306714","messageId":"20161201192055.44mtvtacyhpbqbqu@sigill.intra.peff.net","threadId":"44560","inReplyTo":"20161201181415.GA54082@google.com","subject":"Re: bw/transport-protocol-policy","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-01T19:20:55Z","receivedAt":"2016-12-01T19:21:26Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 01, 2016 at 10:14:15AM -0800, Brandon Williams wrote:\n\n> > 1. The new policy config lets you say \"only allow this protocol when\n> > the user specifies it\". But when http.c calls is_transport_allowed(),\n> > the latter has no idea that we are asking it about potential\n> > redirects (which obviously do _not_ come from the user), and would\n> > erroneously allow them.\n> > \n> > I think this needs fixed before the topic is merged. It's not a\n> > regression, as it only comes into play if you use the new policy\n> > config. But it is a minor security hole in the new feature.\n> \n> I agree and it should be an easy fix. We can just add a parameter like\n> so:\n> \n> diff --git a/transport.c b/transport.c\n> index 2c0ec76..d38d50f 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -723,7 +723,7 @@ static enum protocol_allow_config get_protocol_config(const char *type)\n> \treturn PROTOCOL_ALLOW_USER_ONLY;\n> }\n> \n> -int is_transport_allowed(const char *type)\n> +int is_transport_allowed(const char *type, int redirect)\n> {\n> \tconst struct string_list *whitelist = protocol_whitelist();\n> \tif (whitelist)\n> @@ -735,7 +735,7 @@ int is_transport_allowed(const char *type)\n> \tcase PROTOCOL_ALLOW_NEVER:\n> \t\treturn 0;\n> \tcase PROTOCOL_ALLOW_USER_ONLY:\n> -\t\treturn git_env_bool(\"GIT_PROTOCOL_FROM_USER\", 1);\n> +\t\treturn git_env_bool(\"GIT_PROTOCOL_FROM_USER\", !redirect);\n> \t}\n> \n> \tdie(\"BUG: invalid protocol_allow_config type\");\n> \n> That way the libcurl code can say it is asking if it is ok to redirect\n> to that protocol.\n\nI wouldn't expect anyone to ever set GIT_PROTOCOL_FROM_USER=1, but it\ndoes behave in a funny way here, overriding the \"redirect\" flag. I think\nwe'd want something more like:\n\n if (redirect < 0)\n\tredirect = git_env_bool(\"GIT_PROTOCOL_FROM_USER\", 1);\n\nand then pass in \"-1\" from transport_check_allowed().\n\nI think that's sufficient to fix the topic as-is. However, the http\nredirect series adds an extra complication, because with http-alternates\nwe resolve some of the redirects ourselves. So in those cases we'd want\nto restrict CURLOPT_PROTOCOLS as if they were redirects. We may need to\nset up two CURLOPT values: ones from the user and ones from redirects,\nand then feed them to CURLOPT_PROTOCOLS and CURLOPT_REDIR_PROTOCOLS as\nappropriate depending on the request context.\n\n> We should switch to warning all the time since this series adds in\n> default whitelisted/blacklisted protocols anyways.\n\nYeah, good point. As a bonus it makes the code simpler.\n\n-Peff\n"},{"id":"306715","messageId":"20161201193524.GC54082@google.com","threadId":"44560","inReplyTo":"20161201192055.44mtvtacyhpbqbqu@sigill.intra.peff.net","subject":"Re: bw/transport-protocol-policy","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-12-01T19:35:24Z","receivedAt":"2016-12-01T19:35:46Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 12/01, Jeff King wrote:\n> On Thu, Dec 01, 2016 at 10:14:15AM -0800, Brandon Williams wrote:\n> \n> > > 1. The new policy config lets you say \"only allow this protocol when\n> > > the user specifies it\". But when http.c calls is_transport_allowed(),\n> > > the latter has no idea that we are asking it about potential\n> > > redirects (which obviously do _not_ come from the user), and would\n> > > erroneously allow them.\n> > > \n> > > I think this needs fixed before the topic is merged. It's not a\n> > > regression, as it only comes into play if you use the new policy\n> > > config. But it is a minor security hole in the new feature.\n> > \n> > I agree and it should be an easy fix. We can just add a parameter like\n> > so:\n> > \n> > diff --git a/transport.c b/transport.c\n> > index 2c0ec76..d38d50f 100644\n> > --- a/transport.c\n> > +++ b/transport.c\n> > @@ -723,7 +723,7 @@ static enum protocol_allow_config get_protocol_config(const char *type)\n> > \treturn PROTOCOL_ALLOW_USER_ONLY;\n> > }\n> > \n> > -int is_transport_allowed(const char *type)\n> > +int is_transport_allowed(const char *type, int redirect)\n> > {\n> > \tconst struct string_list *whitelist = protocol_whitelist();\n> > \tif (whitelist)\n> > @@ -735,7 +735,7 @@ int is_transport_allowed(const char *type)\n> > \tcase PROTOCOL_ALLOW_NEVER:\n> > \t\treturn 0;\n> > \tcase PROTOCOL_ALLOW_USER_ONLY:\n> > -\t\treturn git_env_bool(\"GIT_PROTOCOL_FROM_USER\", 1);\n> > +\t\treturn git_env_bool(\"GIT_PROTOCOL_FROM_USER\", !redirect);\n> > \t}\n> > \n> > \tdie(\"BUG: invalid protocol_allow_config type\");\n> > \n> > That way the libcurl code can say it is asking if it is ok to redirect\n> > to that protocol.\n> \n> I wouldn't expect anyone to ever set GIT_PROTOCOL_FROM_USER=1, but it\n> does behave in a funny way here, overriding the \"redirect\" flag. I think\n> we'd want something more like:\n> \n> if (redirect < 0)\n> \tredirect = git_env_bool(\"GIT_PROTOCOL_FROM_USER\", 1);\n> \n> and then pass in \"-1\" from transport_check_allowed().\n\nI don't think I quite follow your solution but I came up with this:\n\n case PROTOCOL_ALLOW_USER_ONLY:\n return redirect ? 0 : git_env_bool(\"GIT_PROTOCOL_FROM_USER\", 1);\n\nWhich should address the same issue.\n\n-- \nBrandon Williams\n"},{"id":"306722","messageId":"20161201194621.dd3dbjv25ul6qgu5@sigill.intra.peff.net","threadId":"44560","inReplyTo":"20161201193524.GC54082@google.com","subject":"Re: bw/transport-protocol-policy","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-01T19:46:21Z","receivedAt":"2016-12-01T19:46:27Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 01, 2016 at 11:35:24AM -0800, Brandon Williams wrote:\n\n> > I wouldn't expect anyone to ever set GIT_PROTOCOL_FROM_USER=1, but it\n> > does behave in a funny way here, overriding the \"redirect\" flag. I think\n> > we'd want something more like:\n> > \n> > if (redirect < 0)\n> > \tredirect = git_env_bool(\"GIT_PROTOCOL_FROM_USER\", 1);\n> > \n> > and then pass in \"-1\" from transport_check_allowed().\n> \n> I don't think I quite follow your solution but I came up with this:\n> \n> case PROTOCOL_ALLOW_USER_ONLY:\n> return redirect ? 0 : git_env_bool(\"GIT_PROTOCOL_FROM_USER\", 1);\n> \n> Which should address the same issue.\n\nI think mine was confused a bit by using the word \"redirect\". It was\nreally meant to be \"from_user\", and could take three values: definitely\nyes, definitely no, and unknown (-1). And in the unknown case, we pull\nthe value from the environment.\n\nYours combines \"definitely no\" and \"unknown\" into a single value (\"1\" in\nyour case, but that is because \"redirect\" and \"from_user\" have inverted\nlogic from each other).\n\nI think that is OK, as there isn't any case where a caller would want to\nsay \"definitely no\". The most they would say is \"_I_ am not doing\nanything to make you think this value is not from the user\", but we\nwould still want to check the environment to see that nobody _else_ had\nput in such a restriction.\n\n-Peff\n"},{"id":"306726","messageId":"20161201195318.GE54082@google.com","threadId":"44560","inReplyTo":"20161201194621.dd3dbjv25ul6qgu5@sigill.intra.peff.net","subject":"Re: bw/transport-protocol-policy","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-12-01T19:53:18Z","receivedAt":"2016-12-01T19:53:26Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 12/01, Jeff King wrote:\n> On Thu, Dec 01, 2016 at 11:35:24AM -0800, Brandon Williams wrote:\n> \n> > > I wouldn't expect anyone to ever set GIT_PROTOCOL_FROM_USER=1, but it\n> > > does behave in a funny way here, overriding the \"redirect\" flag. I think\n> > > we'd want something more like:\n> > > \n> > > if (redirect < 0)\n> > > \tredirect = git_env_bool(\"GIT_PROTOCOL_FROM_USER\", 1);\n> > > \n> > > and then pass in \"-1\" from transport_check_allowed().\n> > \n> > I don't think I quite follow your solution but I came up with this:\n> > \n> > case PROTOCOL_ALLOW_USER_ONLY:\n> > return redirect ? 0 : git_env_bool(\"GIT_PROTOCOL_FROM_USER\", 1);\n> > \n> > Which should address the same issue.\n> \n> I think mine was confused a bit by using the word \"redirect\". It was\n> really meant to be \"from_user\", and could take three values: definitely\n> yes, definitely no, and unknown (-1). And in the unknown case, we pull\n> the value from the environment.\n> \n> Yours combines \"definitely no\" and \"unknown\" into a single value (\"1\" in\n> your case, but that is because \"redirect\" and \"from_user\" have inverted\n> logic from each other).\n> \n> I think that is OK, as there isn't any case where a caller would want to\n> say \"definitely no\". The most they would say is \"_I_ am not doing\n> anything to make you think this value is not from the user\", but we\n> would still want to check the environment to see that nobody _else_ had\n> put in such a restriction.\n> \n> -Peff\n\nOh ok, I see what you were going for now. That may be a better\n(clearer) solution then what I just sent out. Mostly because we can\nmaintain the same logic polarity and use the same vocabulary.\n\n-- \nBrandon Williams\n"}]}