{"thread":{"id":"29160","subject":"What's cooking in git.git (Dec 2011, #04; Tue, 13)","startedAt":"2011-12-14T01:19:01Z","lastAt":"2011-12-15T18:19:33Z","messageCount":12,"participants":["Junio C Hamano","Jonathan Nieder","Thomas Rast","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"181109","messageId":"7vobvcrlve.fsf@alter.siamese.dyndns.org","threadId":"29160","inReplyTo":null,"subject":"What's cooking in git.git (Dec 2011, #04; Tue, 13)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-14T01:19:01Z","receivedAt":"2011-12-14T01:19:01Z","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 '-' are\nonly in 'pu' (proposed updates) while commits prefixed with '+' are in\n'next'.\n\nHere are the repositories that have my integration branches:\n\nWith maint, master, next, pu, todo:\n\n        git://git.kernel.org/pub/scm/git/git.git\n        git://repo.or.cz/alt-git.git\n        https://code.google.com/p/git-core/\n        https://github.com/git/git\n\nWith only maint and master:\n\n        git://git.sourceforge.jp/gitroot/git-core/git.git\n        git://git-core.git.sourceforge.net/gitroot/git-core/git-core\n\nWith all the topics and integration branches:\n\n        https://github.com/gitster/git\n\nThe preformatted documentation in HTML and man format are found in:\n\n        git://git.kernel.org/pub/scm/git/git-{htmldocs,manpages}.git/\n        git://repo.or.cz/git-{htmldocs,manpages}.git/\n        https://code.google.com/p/git-{htmldocs,manpages}.git/\n        https://github.com/gitster/git-{htmldocs,manpages}.git/\n\n--------------------------------------------------\n[New Topics]\n\n* jk/maint-fetch-status-table (2011-12-09) 1 commit\n  (merged to 'next' on 2011-12-09 at 159415e)\n + fetch: create status table using strbuf\n\nWill merge to 'master'.\n\n* ci/stripspace-docs (2011-12-12) 1 commit\n  (merged to 'next' on 2011-12-13 at 35b2cdf)\n + Update documentation for stripspace\n\n* jk/maint-mv (2011-12-12) 5 commits\n  (merged to 'next' on 2011-12-13 at 58caedb)\n + mv: be quiet about overwriting\n + mv: improve overwrite warning\n + mv: make non-directory destination error more clear\n + mv: honor --verbose flag\n + docs: mention \"-k\" for both forms of \"git mv\"\n\n* jk/maint-snprintf-va-copy (2011-12-12) 1 commit\n  (merged to 'next' on 2011-12-13 at d37a7e1)\n + compat/snprintf: don't look at va_list twice\n\n* jn/maint-sequencer-fixes (2011-12-12) 7 commits\n  (merged to 'next' on 2011-12-13 at 5b3950c)\n + revert: stop creating and removing sequencer-old directory\n + Revert \"reset: Make reset remove the sequencer state\"\n + revert: do not remove state until sequence is finished\n + revert: allow single-pick in the middle of cherry-pick sequence\n + revert: pass around rev-list args in already-parsed form\n + revert: allow cherry-pick --continue to commit before resuming\n + revert: give --continue handling its own function\n\n* mh/ref-api (2011-12-12) 51 commits\n - repack_without_ref(): call clear_packed_ref_cache()\n - read_packed_refs(): keep track of the directory being worked in\n - is_refname_available(): query only possibly-conflicting references\n - refs: read loose references lazily\n - read_loose_refs(): take a (ref_entry *) as argument\n - struct ref_dir: store a reference to the enclosing ref_cache\n - sort_ref_dir(): take (ref_entry *) instead of (ref_dir *)\n - do_for_each_ref_in_dir*(): take (ref_entry *) instead of (ref_dir *)\n - add_entry(): take (ref_entry *) instead of (ref_dir *)\n - search_ref_dir(): take (ref_entry *) instead of (ref_dir *)\n - find_containing_direntry(): use (ref_entry *) instead of (ref_dir *)\n - add_ref(): take (ref_entry *) instead of (ref_dir *)\n - read_packed_refs(): take (ref_entry *) instead of (ref_dir *)\n - find_ref(): take (ref_entry *) instead of (ref_dir *)\n - is_refname_available(): take (ref_entry *) instead of (ref_dir *)\n - get_loose_refs(): return (ref_entry *) instead of (ref_dir *)\n - get_packed_refs(): return (ref_entry *) instead of (ref_dir *)\n - refs: wrap top-level ref_dirs in ref_entries\n - get_ref_dir(): keep track of the current ref_dir\n - do_for_each_ref(): only iterate over the subtree that was requested\n - refs: sort ref_dirs lazily\n - sort_ref_dir(): do not sort if already sorted\n - refs: store references hierarchically\n - refs.c: rename ref_array -> ref_dir\n - struct ref_entry: nest the value part in a union\n - check_refname_component(): return 0 for zero-length components\n - free_ref_entry(): new function\n - refs.c: reorder definitions more logically\n - is_refname_available(): reimplement using do_for_each_ref_in_array()\n - names_conflict(): simplify implementation\n - names_conflict(): new function, extracted from is_refname_available()\n - repack_without_ref(): reimplement using do_for_each_ref_in_array()\n - do_for_each_ref_in_arrays(): new function\n - do_for_each_ref_in_array(): new function\n - do_for_each_ref(): correctly terminate while processesing extra_refs\n - add_ref(): take a (struct ref_entry *) parameter\n - create_ref_entry(): extract function from add_ref()\n - repack_without_ref(): remove temporary\n - resolve_gitlink_ref_recursive(): change to work with struct ref_cache\n - Pass a (ref_cache *) to the resolve_gitlink_*() helper functions\n - resolve_gitlink_ref(): improve docstring\n - get_ref_dir(): change signature\n - refs: change signatures of get_packed_refs() and get_loose_refs()\n - is_dup_ref(): extract function from sort_ref_array()\n - add_ref(): add docstring\n - parse_ref_line(): add docstring\n - is_refname_available(): remove the \"quiet\" argument\n - clear_ref_array(): rename from free_ref_array()\n - refs: rename parameters result -> sha1\n - refs: rename \"refname\" variables\n - struct ref_entry: document name member\n\n* nd/resolve-ref (2011-12-13) 3 commits\n  (merged to 'next' on 2011-12-13 at c7002e9)\n + Rename resolve_ref() to resolve_ref_unsafe()\n + Convert resolve_ref+xstrdup to new resolve_refdup function\n + revert: convert resolve_ref() to read_ref_full()\n\n* tr/grep-threading (2011-12-12) 3 commits\n - grep: disable threading in non-worktree case\n - grep: enable threading with -p and -W using lazy attribute lookup\n - grep: load funcname patterns for -W\n\nWill merge to 'next' after taking another look.\n\n* tr/pty-all (2011-12-12) 3 commits\n - t/lib-terminal: test test-terminal's sanity\n - test-terminal: set output terminals to raw mode\n - test-terminal: give the child an empty stdin TTY\n\nWill merge to 'next' after taking another look.\n\n* jc/push-ignore-stale (2011-12-13) 2 commits\n - push: --ignore-stale option\n - set_ref_status_for_push(): use transport-flags abstraction\n\n* jk/fetch-no-tail-match-refs (2011-12-13) 4 commits\n  (merged to 'next' on 2011-12-13 at 805c018)\n + connect.c: drop path_match function\n + fetch-pack: match refs exactly\n + t5500: give fully-qualified refs to fetch-pack\n + drop \"match\" parameter from get_remote_heads\n\n* jk/maint-push-over-dav (2011-12-13) 2 commits\n  (merged to 'next' on 2011-12-13 at 45e376c)\n + http-push: enable \"proactive auth\"\n + t5540: test DAV push with authentication\n\n--------------------------------------------------\n[Cooking]\n\n* rr/revert-cherry-pick (2011-12-09) 9 commits\n . revert: simplify communicating command-line arguments\n . revert: report fine-grained error messages from insn parser\n . revert: allow mixed pick and revert instructions\n . t3510 (cherry-pick-sequencer): remove malformed sheet 2\n . t3510 (cherry-pick-sequencer): use exit status\n . revert: simplify getting commit subject in format_todo()\n . revert: tolerate extra spaces, tabs in insn sheet\n . revert: make commit subjects in insn sheet optional\n . revert: free msg in format_todo()\n\nEjected for now, to give higher priority to jn/maint-sequencer-fixes\ntopic.\n\n* ew/keepalive (2011-12-05) 1 commit\n  (merged to 'next' on 2011-12-13 at 1b5d5c4)\n + enable SO_KEEPALIVE for connected TCP sockets\n\n* jc/checkout-m-twoway (2011-12-11) 2 commits\n  (merged to 'next' on 2011-12-11 at b61057f)\n + Test 'checkout -m -- path'\n  (merged to 'next' on 2011-12-09 at c946009)\n + checkout -m: no need to insist on having all 3 stages\n\n* tr/cache-tree (2011-12-06) 5 commits\n  (merged to 'next' on 2011-12-13 at e0da64d)\n + reset: update cache-tree data when appropriate\n + commit: write cache-tree data when writing index anyway\n + Refactor cache_tree_update idiom from commit\n + Test the current state of the cache-tree optimization\n + Add test-scrap-cache-tree\n\n* tr/userdiff-c-returns-pointer (2011-12-06) 1 commit\n  (merged to 'next' on 2011-12-09 at 0b6a092)\n + userdiff: allow * between cpp funcname words\n\nWill merge to 'master'.\n\n* jc/commit-amend-no-edit (2011-12-08) 5 commits\n  (merged to 'next' on 2011-12-09 at b9cfa4e)\n + test: commit --amend should honor --no-edit\n + commit: honour --no-edit\n + t7501 (commit): modernize style\n + test: remove a porcelain test that hard-codes commit names\n + test: add missing \"&&\" after echo command\n\n* jl/submodule-status-failure-report (2011-12-08) 1 commit\n  (merged to 'next' on 2011-12-09 at 53eb3b3)\n + diff/status: print submodule path when looking for changes fails\n\nWill merge to 'master'.\n\n* ks/tag-cleanup (2011-12-09) 1 commit\n  (merged to 'next' on 2011-12-09 at cbea045)\n + git-tag: introduce --cleanup option\n\nWill merge to 'master'.\n\n* rr/test-chaining (2011-12-11) 7 commits\n  (merged to 'next' on 2011-12-13 at b08445e)\n + t3401: use test_commit in setup\n + t3401: modernize style\n + t3040 (subprojects-basic): fix '&&' chaining, modernize style\n + t1510 (worktree): fix '&&' chaining\n + t3030 (merge-recursive): use test_expect_code\n + test: fix '&&' chaining\n + t3200 (branch): fix '&&' chaining\n\n* bc/maint-apply-check-no-patch (2011-12-05) 2 commits\n  (merged to 'next' on 2011-12-09 at fc780cd)\n + builtin/apply.c: report error on failure to recognize input\n + t/t4131-apply-fake-ancestor.sh: fix broken test\n\nWill merge to 'master'.\n\n* aw/rebase-i-stop-on-failure-to-amend (2011-11-30) 1 commit\n  (merged to 'next' on 2011-12-09 at a117e83)\n + rebase -i: interrupt rebase when \"commit --amend\" failed during \"reword\"\n\n* jc/split-blob (2011-12-01) 6 commits\n . WIP (streaming chunked)\n - chunked-object: fallback checkout codepaths\n - bulk-checkin: support chunked-object encoding\n - bulk-checkin: allow the same data to be multiply hashed\n - new representation types in the packstream\n - varint-in-pack: refactor varint encoding/decoding\n (this branch uses jc/stream-to-pack.)\n\nNot ready. At least pack-objects and fsck need to learn the new encoding\nfor the series to be usable locally, and then index-pack/unpack-objects\nneeds to learn it to be used remotely.\n\n* jh/fast-import-notes (2011-11-28) 3 commits\n  (merged to 'next' on 2011-12-09 at 2b01132)\n + fast-import: Fix incorrect fanout level when modifying existing notes refs\n + t9301: Add 2nd testcase exposing bugs in fast-import's notes fanout handling\n + t9301: Fix testcase covering up a bug in fast-import's notes fanout handling\n\nWill merge to 'master'.\n\n* tj/maint-imap-send-remove-unused (2011-11-23) 2 commits\n  (merged to 'next' on 2011-12-09 at 877cc11)\n + Merge branch 'maint' into tj/imap-send-remove-unused\n + imap-send: Remove unused 'use_namespace' variable\n\nWill merge to 'master'.\n\n* cn/maint-lf-to-crlf-filter (2011-11-28) 1 commit\n  (merged to 'next' on 2011-12-09 at c374d14)\n + convert: track state in LF-to-CRLF filter\n\nWill merge to 'master'.\n\n* jn/branch-move-to-self (2011-11-28) 2 commits\n  (merged to 'next' on 2011-12-09 at 7d27260)\n + Allow checkout -B <current-branch> to update the current branch\n + branch: allow a no-op \"branch -M <current-branch> HEAD\"\n\nWill merge to 'master'.\n\n* jk/credentials (2011-12-12) 24 commits\n - contrib: add credential helper for OS X Keychain\n - Makefile: OS X has /dev/tty\n - Makefile: linux has /dev/tty\n - credential: use git_prompt instead of git_getpass\n - prompt: use git_terminal_prompt\n - add generic terminal prompt function\n - refactor git_getpass into generic prompt function\n - move git_getpass to its own source file\n - imap-send: don't check return value of git_getpass\n - imap-send: avoid buffer overflow\n  (merged to 'next' on 2011-12-12 at 7a6d658)\n + t: add test harness for external credential helpers\n + credentials: add \"store\" helper\n + strbuf: add strbuf_add*_urlencode\n + Makefile: unix sockets may not available on some platforms\n + credentials: add \"cache\" helper\n + docs: end-user documentation for the credential subsystem\n + credential: make relevance of http path configurable\n + credential: add credential.*.username\n + credential: apply helper config\n + http: use credential API to get passwords\n + credential: add function for parsing url components\n + introduce credentials API\n + t5550: fix typo\n + test-lib: add test_config_global variant\n\nLooking good. Probably split the later part into its own topic and\nmerge the parts already in 'next' to 'master' soonish.\n\n* nd/ignore-might-be-precious (2011-11-28) 2 commits\n  (merged to 'next' on 2011-12-09 at 1a94553)\n + checkout,merge: disallow overwriting ignored files with --no-overwrite-ignore\n + Merge branch 'nd/maint-ignore-exclude' into nd/ignore-might-be-precious\n\nWill merge to 'master'.\n\n* jk/upload-archive-use-start-command (2011-11-21) 1 commit\n  (merged to 'next' on 2011-12-09 at 88cb83a)\n + upload-archive: use start_command instead of fork\n\n* jk/maint-1.6.2-upload-archive (2011-11-21) 1 commit\n + archive: don't let remote clients get unreachable commits\n (this branch is used by jk/maint-upload-archive.)\n\n* jk/maint-upload-archive (2011-11-21) 1 commit\n  (merged to 'next' on 2011-12-09 at 03deb16)\n + Merge branch 'jk/maint-1.6.2-upload-archive' into jk/maint-upload-archive\n (this branch uses jk/maint-1.6.2-upload-archive.)\n\nWill merge to 'master'.\n\n* ab/enable-i18n (2011-12-05) 1 commit\n  (merged to 'next' on 2011-12-13 at 65af8cd)\n + i18n: add infrastructure for translating Git with gettext\n\n* jc/signed-commit (2011-11-29) 5 commits\n - gpg-interface: allow use of a custom GPG binary\n - pretty: %G[?GS] placeholders\n - test \"commit -S\" and \"log --show-signature\"\n - log: --show-signature\n - commit: teach --gpg-sign option\n\nNot exactly urgent.\n\n* jc/stream-to-pack (2011-12-01) 5 commits\n  (merged to 'next' on 2011-12-09 at d0fd605)\n + bulk-checkin: replace fast-import based implementation\n + csum-file: introduce sha1file_checkpoint\n + finish_tmp_packfile(): a helper function\n + create_tmp_packfile(): a helper function\n + write_pack_header(): a helper function\n (this branch is used by jc/split-blob.)\n\nTeaches \"git add\" to send large-ish blob data straight to a packfile.\nThis is a continuation to the \"large file support\" topic. The codepath to\nmove data from worktree to repository is made aware of streaming, just\nlike the checkout codepath that goes the other way, which was done in the\nprevious \"large file support\" topic in the 1.7.7 cycle.\n\n* jn/gitweb-side-by-side-diff (2011-10-31) 8 commits\n  (merged to 'next' on 2011-12-09 at 7662e58)\n + gitweb: Add navigation to select side-by-side diff\n + gitweb: Use href(-replay=>1,...) for formats links in \"commitdiff\"\n + t9500: Add basic sanity tests for side-by-side diff in gitweb\n + t9500: Add test for handling incomplete lines in diff by gitweb\n + gitweb: Give side-by-side diff extra CSS styling\n + gitweb: Add a feature to show side-by-side diff\n + gitweb: Extract formatting of diff chunk header\n + gitweb: Refactor diff body line classification\n\nReplaces a series from Kato Kazuyoshi on the same topic.\n\nWill merge to 'master'.\n"},{"id":"181114","messageId":"20111214070916.GA14954@elie.hsd1.il.comcast.net","threadId":"29160","inReplyTo":"7vobvcrlve.fsf@alter.siamese.dyndns.org","subject":"tr/pty-all (Re: What's cooking in git.git (Dec 2011, #04; Tue, 13))","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-12-14T07:09:16Z","receivedAt":"2011-12-14T07:09:16Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> * tr/pty-all (2011-12-12) 3 commits\n>  - t/lib-terminal: test test-terminal's sanity\n>  - test-terminal: set output terminals to raw mode\n>  - test-terminal: give the child an empty stdin TTY\n>\n> Will merge to 'next' after taking another look.\n\nThe middle commit looks good.  The bottom commit could be improved as\ndiscussed at [1], but I guess that can happen in-tree.\n\nHowever, the top commit (\"test test-terminal's sanity\") still does not\nseem right to me.\n\nIt makes the same test run three times.  Probably I should send an\nalternate patch to get that sanity-check to run once, but I am also\nnot convinced the sanity-check is needed at all --- wouldn't any test\nthat is relying on output from test_terminal act as a sanity check for\nit already?\n\nAs an aside, I also still believe that running \"git shortlog\" without\nexplicitly passing \"HEAD\" when testing how it reacts to [core] pager\nconfiguration was a bug and a distraction, hence the patch at [2].  Am\nI the only one?  I also find Jeff's patch [3] appealing.\n\nThanks,\nJonathan\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/186923/focus=186944\n[2] http://article.gmane.org/gmane.comp.version-control.git/186932\n[3] http://article.gmane.org/gmane.comp.version-control.git/186936\n"},{"id":"181158","messageId":"201112141717.15021.trast@student.ethz.ch","threadId":"29160","inReplyTo":"20111214070916.GA14954@elie.hsd1.il.comcast.net","subject":"Re: tr/pty-all (Re: What's cooking in git.git (Dec 2011, #04; Tue, 13))","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2011-12-14T16:17:14Z","receivedAt":"2011-12-14T16:17:14Z","isPatch":false,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Jonathan Nieder wrote:\n> Junio C Hamano wrote:\n> > Will merge to 'next' after taking another look.\n> \n> The middle commit looks good.  The bottom commit could be improved as\n> discussed at [1], but I guess that can happen in-tree.\n> \n> However, the top commit (\"test test-terminal's sanity\") still does not\n> seem right to me.\n\nI wasn't under the impression that we were done with this, either :-)\n\n> It makes the same test run three times.  Probably I should send an\n> alternate patch to get that sanity-check to run once, but I am also\n> not convinced the sanity-check is needed at all --- wouldn't any test\n> that is relying on output from test_terminal act as a sanity check for\n> it already?\n\nIt didn't.  Or more precisely, Michael Haggerty ran into the behavior\nof\n\n  git rev-parse ... | while read sha; do git checkout $sha; make test; done\n\ncouldn't make any sense of it, and reported it on IRC.  So in some\nsense, it took infrequent circumstances and two developers' time; next\ntime around I'd prefer it to be detected automatically.\n\n> As an aside, I also still believe that running \"git shortlog\" without\n> explicitly passing \"HEAD\" when testing how it reacts to [core] pager\n> configuration was a bug and a distraction, hence the patch at [2].\n\nWhy not.  Some other test should verify how shortlog reacts to the\ntty-ness of stdin, but that's yet another direction.\n\n> I also find Jeff's patch [3] appealing.\n\nMe too, though wonder whether feeding a file full of garbage wouldn't\nbe better, so as to trip up commands that try to read only from a\nnon-tty stdin.\n\n\n\n> [1] http://thread.gmane.org/gmane.comp.version-control.git/186923/focus=186944\n> [2] http://article.gmane.org/gmane.comp.version-control.git/186932\n> [3] http://article.gmane.org/gmane.comp.version-control.git/186936\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"181173","messageId":"7vpqfrqcch.fsf@alter.siamese.dyndns.org","threadId":"29160","inReplyTo":"201112141717.15021.trast@student.ethz.ch","subject":"Re: tr/pty-all (Re: What's cooking in git.git (Dec 2011, #04; Tue, 13))","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-14T17:42:22Z","receivedAt":"2011-12-14T17:42:22Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast <trast@student.ethz.ch> writes:\n\n> Jonathan Nieder wrote:\n>> Junio C Hamano wrote:\n>> > Will merge to 'next' after taking another look.\n>> \n>> The middle commit looks good.  The bottom commit could be improved as\n>> discussed at [1], but I guess that can happen in-tree.\n>> \n>> However, the top commit (\"test test-terminal's sanity\") still does not\n>> seem right to me.\n>\n> I wasn't under the impression that we were done with this, either :-)\n>\n>> It makes the same test run three times.  Probably I should send an\n>> alternate patch to get that sanity-check to run once, but I am also\n>> not convinced the sanity-check is needed at all --- wouldn't any test\n>> that is relying on output from test_terminal act as a sanity check for\n>> it already?\n>\n> It didn't.  Or more precisely, Michael Haggerty ran into the behavior\n> of\n>\n>   git rev-parse ... | while read sha; do git checkout $sha; make test; done\n>\n> couldn't make any sense of it, and reported it on IRC.  So in some\n> sense, it took infrequent circumstances and two developers' time; next\n> time around I'd prefer it to be detected automatically.\n>\n>> As an aside, I also still believe that running \"git shortlog\" without\n>> explicitly passing \"HEAD\" when testing how it reacts to [core] pager\n>> configuration was a bug and a distraction, hence the patch at [2].\n>\n> Why not.  Some other test should verify how shortlog reacts to the\n> tty-ness of stdin, but that's yet another direction.\n>\n>> I also find Jeff's patch [3] appealing.\n>\n> Me too, though wonder whether feeding a file full of garbage wouldn't\n> be better, so as to trip up commands that try to read only from a\n> non-tty stdin.\n\nWell, I guess I was too quick to pull the trigger after sending the\n\"What's cooking\" out. Sorry about that.\n\nOn the other hand, I think these require relatively low impact changes\nthat can be handled in-tree and the downsides of the series like running\nprerequisite tests more than once are not serious show stoppers, so it\nisn't a disaster ;-)\n\nThanks both for noticing and commenting. Very much appreciated.\n"},{"id":"181192","messageId":"20111214230713.GA13128@sigill.intra.peff.net","threadId":"29160","inReplyTo":"201112141717.15021.trast@student.ethz.ch","subject":"Re: tr/pty-all (Re: What's cooking in git.git (Dec 2011, #04; Tue, 13))","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-12-14T23:07:13Z","receivedAt":"2011-12-14T23:07:13Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 14, 2011 at 05:17:14PM +0100, Thomas Rast wrote:\n\n> > I also find Jeff's patch [3] appealing.\n> \n> Me too, though wonder whether feeding a file full of garbage wouldn't\n> be better, so as to trip up commands that try to read only from a\n> non-tty stdin.\n\nInteresting idea. Instead of getting an immediate EOF from /dev/null,\nthey'll get some junk which they may or may not complain about. I played\naround with this a bit, redirecting test stdin from a file with:\n\n  This is a garbage file that will be connected to the stdin of each\n  test.\n\n(with the hope that when you see that in an error message, it will be\nobvious what has happened).\n\nIt turns out that shortlog (the one bug of this type in the test suite)\ndoesn't care at all. It will happily chew on that garbage and behave\njust the same as if it had been given /dev/null.\n\nThere is also the minor issue that if there are multiple invocations of\nthe input-chewing command, the first one will eat all of the input, and\nsubsequent ones will just get the equivalent of /dev/null anyway. I.e.:\n\n  test_expect_success 'git shortlog $foo' ;# will eat all of our stdin\n  test_expect_success 'git shortlog $bar' ;# sees immediate EOF\n\nThat may not matter, though, as you would hope for the first invocation\nto complain, at which point you fix it, revealing the failures in later\ntests (or if you're clever, noticing in the first place that they all\nhave the same bug).\n\nYou can make this somewhat more robust by redirecting stdin for\n_test_eval (so each test gets a fresh descriptor to the garbage file).\nHowever, that disallows redirecting a specific test's stdin in the test\nscript, like:\n\n  test_expect_success 'some test' '\n    cat >expect &&\n    some_command >actual &&\n    test_cmp expect actual\n  ' <<\\EOF\n  the expected\n  data\n  EOF\n\nThis construct probably seems silly at first, but see how it is used in\nt6006:\n\n  test_format author %an%n%ae%n%ad%n%aD%n%at <<\\EOF\n  commit ...\n  A U Thor\n  author@example.com\n  ... etc ...\n  EOF\n\nwhere test_format is a generic function that just gobbles stdin into the\n\"expect\" file. As it turns out, test_format actually runs the \"cat\"\noutside of the \"test_expect_success\", but quite often we put it inside.\nSo maybe it isn't worth caring about. You could also argue that the\nwhole thing should just be inside a test_expect_success.\n\n-Peff\n"},{"id":"181193","messageId":"20111214232151.GB13128@sigill.intra.peff.net","threadId":"29160","inReplyTo":"20111214230713.GA13128@sigill.intra.peff.net","subject":"Re: tr/pty-all (Re: What's cooking in git.git (Dec 2011, #04; Tue, 13))","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-12-14T23:21:51Z","receivedAt":"2011-12-14T23:21:51Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 14, 2011 at 06:07:13PM -0500, Jeff King wrote:\n\n> On Wed, Dec 14, 2011 at 05:17:14PM +0100, Thomas Rast wrote:\n> \n> > > I also find Jeff's patch [3] appealing.\n> > \n> > Me too, though wonder whether feeding a file full of garbage wouldn't\n> > be better, so as to trip up commands that try to read only from a\n> > non-tty stdin.\n> \n> Interesting idea. Instead of getting an immediate EOF from /dev/null,\n> they'll get some junk which they may or may not complain about. I played\n> around with this a bit, redirecting test stdin from a file with:\n\nSo here is what that patch looked like. As I mentioned, it doesn't\nactually catch the shortlog problem, but it would fix\nMichael's issue with an outer reading loop.\n\n-- >8 --\nSubject: [PATCH] test-lib: redirect stdin of tests\n\nWe want to run tests in a predictable, sterile environment\nso we can get repeatable results.  They should take as\nlittle input as possible from the environment outside the\ntest script. We already sanitize environment variables, but\nleave stdin untouched. This means that scripts can\naccidentally be impacted by content on stdin, or whether\nstdin isatty().\n\nFurthermore, scripts reading from stdin can be annoying to\nouter loops which care about their stdin offset, like:\n\n  while read sha1; do\n      make test\n  done\n\nA test which accidentally reads stdin would soak up all of\nthe rest of the input intended for the outer shell loop.\n\nLet's redirect stdin from a known source to eliminate\nvariation. We could just connect it to /dev/null. However,\ntests which accidentally read stdin would then see immediate\nEOF, which may or may not cause them to notice the errror.\nInstead, let's connect it to a file with random garbage in\nit, in the hope that it will be more likely to trigger an\nerror in the mis-written test.\n\nWe'll also leave file descriptor 6 as a link to the original\nstdin. Tests shouldn't need to look at this, but it can be\nconvenient for inserting interactive commands while\ndebugging tests (e.g., you could insert \"bash <&6 >&3 2>&4\"\nto run interactive commands in the environment of the test\nscript).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/stdin-garbage |    1 +\n t/test-lib.sh   |    4 +++-\n 2 files changed, 4 insertions(+), 1 deletions(-)\n create mode 100644 t/stdin-garbage\n\ndiff --git a/t/stdin-garbage b/t/stdin-garbage\nnew file mode 100644\nindex 0000000..3a2ebc2\n--- /dev/null\n+++ b/t/stdin-garbage\n@@ -0,0 +1 @@\n+This is a garbage file that will be connected to the stdin of each test.\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex bdd9513..67eb078 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -198,6 +198,8 @@ else\n \texec 4>/dev/null 3>/dev/null\n fi\n \n+exec 6<&0\n+\n test_failure=0\n test_count=0\n test_fixed=0\n@@ -469,7 +471,7 @@ test_debug () {\n test_eval_ () {\n \t# This is a separate function because some tests use\n \t# \"return\" to end a test_expect_success block early.\n-\teval >&3 2>&4 \"$*\"\n+\teval <\"$TEST_DIRECTORY/stdin-garbage\" >&3 2>&4 \"$*\"\n }\n \n test_run_ () {\n-- \n1.7.8.rc2.30.g803b1a\n"},{"id":"181194","messageId":"20111214233119.GA2354@elie","threadId":"29160","inReplyTo":"20111214232151.GB13128@sigill.intra.peff.net","subject":"Re: tr/pty-all (Re: What's cooking in git.git (Dec 2011, #04; Tue, 13))","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-12-14T23:31:19Z","receivedAt":"2011-12-14T23:31:19Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n[...]\n> A test which accidentally reads stdin would soak up all of\n> the rest of the input intended for the outer shell loop.\n> \n> Let's redirect stdin from a known source to eliminate\n> variation. We could just connect it to /dev/null. However,\n> tests which accidentally read stdin would then see immediate\n> EOF, which may or may not cause them to notice the errror.\n[...]\n> +++ b/t/test-lib.sh\n[...]\n> @@ -469,7 +471,7 @@ test_debug () {\n>  test_eval_ () {\n>  \t# This is a separate function because some tests use\n>  \t# \"return\" to end a test_expect_success block early.\n> -\teval >&3 2>&4 \"$*\"\n> +\teval <\"$TEST_DIRECTORY/stdin-garbage\" >&3 2>&4 \"$*\"\n\nHow about /dev/urandom on platforms that support it?  It wouldn't be\nas pleasant to debug as \"This is a magic stdin garbage stream\", but it\nwould be more likely to (despite the name :)) predictably trip errors,\nor at least hangs, in problematic tests.\n\nWith or without something along those lines on top, your patch looks\nlike a good change.\n\nThanks for a thoughtful analysis.\nJonathan\n"},{"id":"181196","messageId":"20111215002530.GA2566@sigill.intra.peff.net","threadId":"29160","inReplyTo":"20111214233119.GA2354@elie","subject":"Re: tr/pty-all (Re: What's cooking in git.git (Dec 2011, #04; Tue, 13))","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-12-15T00:25:30Z","receivedAt":"2011-12-15T00:25:30Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 14, 2011 at 05:31:19PM -0600, Jonathan Nieder wrote:\n\n> > @@ -469,7 +471,7 @@ test_debug () {\n> >  test_eval_ () {\n> >  \t# This is a separate function because some tests use\n> >  \t# \"return\" to end a test_expect_success block early.\n> > -\teval >&3 2>&4 \"$*\"\n> > +\teval <\"$TEST_DIRECTORY/stdin-garbage\" >&3 2>&4 \"$*\"\n> \n> How about /dev/urandom on platforms that support it?  It wouldn't be\n> as pleasant to debug as \"This is a magic stdin garbage stream\", but it\n> would be more likely to (despite the name :)) predictably trip errors,\n> or at least hangs, in problematic tests.\n\nI'd rather have something deterministic. If you really want to be mean\nto accidental readers of stdin, then put binary junk into stdin-garbage\n(even the results of a single run of /dev/urandom, if you like). But I\nsuspect arbitrary text is good enough to throw a monkey wrench into\nanything that will care about its input (and those that don't are beyond\nour ability to auto-detect anyway[1]). And it's way easier to debug than\nseeing random binary bits.\n\n-Peff\n\n[1] Actually, you could abandon the idea of feeding garbage altogether,\nand instead open the descriptor outside the test, then check that its\noffset is still 0 after the test. You'd have to use a helper program to\ndo the ftell(), but it should work as the descriptor position will be\nshared.\n"},{"id":"181197","messageId":"20111215005057.GB2566@sigill.intra.peff.net","threadId":"29160","inReplyTo":"20111215002530.GA2566@sigill.intra.peff.net","subject":"Re: tr/pty-all (Re: What's cooking in git.git (Dec 2011, #04; Tue, 13))","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-12-15T00:50:57Z","receivedAt":"2011-12-15T00:50:57Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 14, 2011 at 07:25:30PM -0500, Jeff King wrote:\n\n> [1] Actually, you could abandon the idea of feeding garbage altogether,\n> and instead open the descriptor outside the test, then check that its\n> offset is still 0 after the test. You'd have to use a helper program to\n> do the ftell(), but it should work as the descriptor position will be\n> shared.\n\nAnd here's what that patch would look like. You still want to feed a\ngarbage file, because you want to make sure that there is something for\nit to actually read. And then either it can choke on the garbage and\nfail, or if not, you can detect afterwards that it was read.\n\nThis correctly detects the bug in t7006. I can't decide if it's clever\nor ugly.\n\n---\n t/stdin-garbage |    1 +\n t/test-lib.sh   |   16 ++++++++++++++++\n 2 files changed, 17 insertions(+), 0 deletions(-)\n create mode 100644 t/stdin-garbage\n\ndiff --git a/t/stdin-garbage b/t/stdin-garbage\nnew file mode 100644\nindex 0000000..3a2ebc2\n--- /dev/null\n+++ b/t/stdin-garbage\n@@ -0,0 +1 @@\n+This is a garbage file that will be connected to the stdin of each test.\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex bdd9513..9b4692b 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -466,6 +466,10 @@ test_debug () {\n \ttest \"$debug\" = \"\" || eval \"$1\"\n }\n \n+test_stdin_unread_ () {\n+\ttest \"`perl -e 'print tell(STDIN)'`\" = 0\n+}\n+\n test_eval_ () {\n \t# This is a separate function because some tests use\n \t# \"return\" to end a test_expect_success block early.\n@@ -475,9 +479,21 @@ test_eval_ () {\n test_run_ () {\n \ttest_cleanup=:\n \texpecting_failure=$2\n+\t# feed the test a bogus stdin, but keep a spare descriptor open in case\n+\t# the test redirects stdin, which affects us since it is an eval\n+\texec 6<&0\n+\texec 7<\"$TEST_DIRECTORY/stdin-garbage\"\n+\texec 0<&7\n \ttest_eval_ \"$1\"\n \teval_ret=$?\n \n+\t# check that nobody read from our bogus descriptor\n+\tif test $eval_ret = 0 && ! test_stdin_unread_ <&7; then\n+\t\techo >&4 \"bug in the test script: somebody read from stdin\"\n+\t\teval_ret=1\n+\tfi\n+\texec 0<&6\n+\n \tif test -z \"$immediate\" || test $eval_ret = 0 || test -n \"$expecting_failure\"\n \tthen\n \t\ttest_eval_ \"$test_cleanup\"\n-- \n1.7.8.rc2.30.g803b1a\n"},{"id":"181205","messageId":"7vfwgmplgd.fsf@alter.siamese.dyndns.org","threadId":"29160","inReplyTo":"20111215005057.GB2566@sigill.intra.peff.net","subject":"Re: tr/pty-all (Re: What's cooking in git.git (Dec 2011, #04; Tue, 13))","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-15T03:23:14Z","receivedAt":"2011-12-15T03:23:14Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> This correctly detects the bug in t7006. I can't decide if it's clever\n> or ugly.\n\nI would say it falls on the latter side of the line by small margin. Let's\ndo the /dev/null thing and be done with it.\n"},{"id":"181209","messageId":"20111215065529.GA1327@sigill.intra.peff.net","threadId":"29160","inReplyTo":"7vfwgmplgd.fsf@alter.siamese.dyndns.org","subject":"Re: tr/pty-all (Re: What's cooking in git.git (Dec 2011, #04; Tue, 13))","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-12-15T06:55:29Z","receivedAt":"2011-12-15T06:55:29Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 14, 2011 at 07:23:14PM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > This correctly detects the bug in t7006. I can't decide if it's clever\n> > or ugly.\n> \n> I would say it falls on the latter side of the line by small margin. Let's\n> do the /dev/null thing and be done with it.\n\nDarn, I wanted to post it on my fridge with an \"A+\".\n\nHere's a cleaned-up version of the /dev/null one.\n\n-- >8 --\nSubject: [PATCH] test-lib: redirect stdin of tests\n\nWe want to run tests in a predictable, sterile environment\nso we can get repeatable results.  They should take as\nlittle input as possible from the environment outside the\ntest script. We already sanitize environment variables, but\nleave stdin untouched. This means that scripts can\naccidentally be impacted by content on stdin, or whether\nstdin isatty().\n\nFurthermore, scripts reading from stdin can be annoying to\nouter loops which care about their stdin offset, like:\n\n  while read sha1; do\n      make test\n  done\n\nA test which accidentally reads stdin would soak up all of\nthe rest of the input intended for the outer shell loop.\n\nLet's redirect stdin from /dev/null, which solves both\nof these problems. It won't detect tests accidentally\nreading from stdin, but since doing so now gives a\ndeterministic result, we don't need to consider that an\nerror.\n\nWe'll also leave file descriptor 6 as a link to the original\nstdin. Tests shouldn't need to look at this, but it can be\nconvenient for inserting interactive commands while\ndebugging tests (e.g., you could insert \"bash <&6 >&3 2>&4\"\nto run interactive commands in the environment of the test\nscript).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI went the \"redirect each test individually\" route. In the course of my\nexperimentation, I notice that some tests (e.g., t4013) will do:\n\n  while read x; do\n          test_expect_success \"test something ($x)\" \"\n            ... do some test involving $x ...\n          \"\n  done <<\\EOF\n  ... some values of $x ....\n  EOF\n\nThis protects those loops from accidental stdin-readers inside the test\nscripts, too.\n\n t/test-lib.sh |    3 ++-\n 1 files changed, 2 insertions(+), 1 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex bdd9513..5ea9fe3 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -191,6 +191,7 @@ then\n fi\n \n exec 5>&1\n+exec 6<&0\n if test \"$verbose\" = \"t\"\n then\n \texec 4>&2 3>&1\n@@ -469,7 +470,7 @@ test_debug () {\n test_eval_ () {\n \t# This is a separate function because some tests use\n \t# \"return\" to end a test_expect_success block early.\n-\teval >&3 2>&4 \"$*\"\n+\teval </dev/null >&3 2>&4 \"$*\"\n }\n \n test_run_ () {\n-- \n1.7.8.rc2.30.g803b1a\n"},{"id":"181236","messageId":"7vr505ofyi.fsf@alter.siamese.dyndns.org","threadId":"29160","inReplyTo":"20111215065529.GA1327@sigill.intra.peff.net","subject":"Re: tr/pty-all (Re: What's cooking in git.git (Dec 2011, #04; Tue, 13))","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-15T18:19:33Z","receivedAt":"2011-12-15T18:19:33Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Wed, Dec 14, 2011 at 07:23:14PM -0800, Junio C Hamano wrote:\n>\n>> Jeff King <peff@peff.net> writes:\n>> \n>> > This correctly detects the bug in t7006. I can't decide if it's clever\n>> > or ugly.\n>> \n>> I would say it falls on the latter side of the line by small margin. Let's\n>> do the /dev/null thing and be done with it.\n>\n> Darn, I wanted to post it on my fridge with an \"A+\".\n\nHeh.\n\nIf it were without the perl \"tell\" bits, it would have been \"clever and\nclean\", perhaps like\n\n\tarrange the following to read from t/random-stdin file\n         - run the test script\n         - \"read x\"\n        compare the $x you just read with t/random-stdin file\n\nBut I think it is good enough to be stupid and clean.\n\n> Here's a cleaned-up version of the /dev/null one.\n\nThanks.\n"}]}