threads / discuss / 29160

What's cooking in git.git (Dec 2011, #04; Tue, 13)

Subject: What's cooking in git.git (Dec 2011, #04; Tue, 13)

## tl;dr

12 messages between Dec 14, 2011 and Dec 15, 2011.

replies: 11people: 4as markdown or json

Junio C Hamano· Dec 14, 2011, 01:19 UTC · lore

Here are the topics that have been cooking. Commits prefixed with '-' are only in 'pu' (proposed updates) while commits prefixed with '+' are in 'next'.

Here are the repositories that have my integration branches:
With maint, master, next, pu, todo:
        git://git.kernel.org/pub/scm/git/git.git
        git://repo.or.cz/alt-git.git
        https://code.google.com/p/git-core/
        https://github.com/git/git
With only maint and master:
        git://git.sourceforge.jp/gitroot/git-core/git.git
        git://git-core.git.sourceforge.net/gitroot/git-core/git-core
With all the topics and integration branches:
        https://github.com/gitster/git
The preformatted documentation in HTML and man format are found in:
        git://git.kernel.org/pub/scm/git/git-{htmldocs,manpages}.git/
        git://repo.or.cz/git-{htmldocs,manpages}.git/
        https://code.google.com/p/git-{htmldocs,manpages}.git/
        https://github.com/gitster/git-{htmldocs,manpages}.git/

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

* jk/maint-fetch-status-table (2011-12-09) 1 commit
  (merged to 'next' on 2011-12-09 at 159415e)
 + fetch: create status table using strbuf
Will merge to 'master'.
* ci/stripspace-docs (2011-12-12) 1 commit
  (merged to 'next' on 2011-12-13 at 35b2cdf)
 + Update documentation for stripspace
* jk/maint-mv (2011-12-12) 5 commits
  (merged to 'next' on 2011-12-13 at 58caedb)
 + mv: be quiet about overwriting
 + mv: improve overwrite warning
 + mv: make non-directory destination error more clear
 + mv: honor --verbose flag
 + docs: mention "-k" for both forms of "git mv"
* jk/maint-snprintf-va-copy (2011-12-12) 1 commit
  (merged to 'next' on 2011-12-13 at d37a7e1)
 + compat/snprintf: don't look at va_list twice
* jn/maint-sequencer-fixes (2011-12-12) 7 commits
  (merged to 'next' on 2011-12-13 at 5b3950c)
 + revert: stop creating and removing sequencer-old directory
 + Revert "reset: Make reset remove the sequencer state"
 + revert: do not remove state until sequence is finished
 + revert: allow single-pick in the middle of cherry-pick sequence
 + revert: pass around rev-list args in already-parsed form
 + revert: allow cherry-pick --continue to commit before resuming
 + revert: give --continue handling its own function
* mh/ref-api (2011-12-12) 51 commits
 - repack_without_ref(): call clear_packed_ref_cache()
 - read_packed_refs(): keep track of the directory being worked in
 - is_refname_available(): query only possibly-conflicting references
 - refs: read loose references lazily
 - read_loose_refs(): take a (ref_entry *) as argument
 - struct ref_dir: store a reference to the enclosing ref_cache
 - sort_ref_dir(): take (ref_entry *) instead of (ref_dir *)
 - do_for_each_ref_in_dir*(): take (ref_entry *) instead of (ref_dir *)
 - add_entry(): take (ref_entry *) instead of (ref_dir *)
 - search_ref_dir(): take (ref_entry *) instead of (ref_dir *)
 - find_containing_direntry(): use (ref_entry *) instead of (ref_dir *)
 - add_ref(): take (ref_entry *) instead of (ref_dir *)
 - read_packed_refs(): take (ref_entry *) instead of (ref_dir *)
 - find_ref(): take (ref_entry *) instead of (ref_dir *)
 - is_refname_available(): take (ref_entry *) instead of (ref_dir *)
 - get_loose_refs(): return (ref_entry *) instead of (ref_dir *)
 - get_packed_refs(): return (ref_entry *) instead of (ref_dir *)
 - refs: wrap top-level ref_dirs in ref_entries
 - get_ref_dir(): keep track of the current ref_dir
 - do_for_each_ref(): only iterate over the subtree that was requested
 - refs: sort ref_dirs lazily
 - sort_ref_dir(): do not sort if already sorted
 - refs: store references hierarchically
 - refs.c: rename ref_array -> ref_dir
 - struct ref_entry: nest the value part in a union
 - check_refname_component(): return 0 for zero-length components
 - free_ref_entry(): new function
 - refs.c: reorder definitions more logically
 - is_refname_available(): reimplement using do_for_each_ref_in_array()
 - names_conflict(): simplify implementation
 - names_conflict(): new function, extracted from is_refname_available()
 - repack_without_ref(): reimplement using do_for_each_ref_in_array()
 - do_for_each_ref_in_arrays(): new function
 - do_for_each_ref_in_array(): new function
 - do_for_each_ref(): correctly terminate while processesing extra_refs
 - add_ref(): take a (struct ref_entry *) parameter
 - create_ref_entry(): extract function from add_ref()
 - repack_without_ref(): remove temporary
 - resolve_gitlink_ref_recursive(): change to work with struct ref_cache
 - Pass a (ref_cache *) to the resolve_gitlink_*() helper functions
 - resolve_gitlink_ref(): improve docstring
 - get_ref_dir(): change signature
 - refs: change signatures of get_packed_refs() and get_loose_refs()
 - is_dup_ref(): extract function from sort_ref_array()
 - add_ref(): add docstring
 - parse_ref_line(): add docstring
 - is_refname_available(): remove the "quiet" argument
 - clear_ref_array(): rename from free_ref_array()
 - refs: rename parameters result -> sha1
 - refs: rename "refname" variables
 - struct ref_entry: document name member
* nd/resolve-ref (2011-12-13) 3 commits
  (merged to 'next' on 2011-12-13 at c7002e9)
 + Rename resolve_ref() to resolve_ref_unsafe()
 + Convert resolve_ref+xstrdup to new resolve_refdup function
 + revert: convert resolve_ref() to read_ref_full()
* tr/grep-threading (2011-12-12) 3 commits
 - grep: disable threading in non-worktree case
 - grep: enable threading with -p and -W using lazy attribute lookup
 - grep: load funcname patterns for -W
Will merge to 'next' after taking another look.
* tr/pty-all (2011-12-12) 3 commits
 - t/lib-terminal: test test-terminal's sanity
 - test-terminal: set output terminals to raw mode
 - test-terminal: give the child an empty stdin TTY
Will merge to 'next' after taking another look.
* jc/push-ignore-stale (2011-12-13) 2 commits
 - push: --ignore-stale option
 - set_ref_status_for_push(): use transport-flags abstraction
* jk/fetch-no-tail-match-refs (2011-12-13) 4 commits
  (merged to 'next' on 2011-12-13 at 805c018)
 + connect.c: drop path_match function
 + fetch-pack: match refs exactly
 + t5500: give fully-qualified refs to fetch-pack
 + drop "match" parameter from get_remote_heads
* jk/maint-push-over-dav (2011-12-13) 2 commits
  (merged to 'next' on 2011-12-13 at 45e376c)
 + http-push: enable "proactive auth"
 + t5540: test DAV push with authentication

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

* rr/revert-cherry-pick (2011-12-09) 9 commits
 . revert: simplify communicating command-line arguments
 . revert: report fine-grained error messages from insn parser
 . revert: allow mixed pick and revert instructions
 . t3510 (cherry-pick-sequencer): remove malformed sheet 2
 . t3510 (cherry-pick-sequencer): use exit status
 . revert: simplify getting commit subject in format_todo()
 . revert: tolerate extra spaces, tabs in insn sheet
 . revert: make commit subjects in insn sheet optional
 . revert: free msg in format_todo()

Ejected for now, to give higher priority to jn/maint-sequencer-fixes topic.

* ew/keepalive (2011-12-05) 1 commit
  (merged to 'next' on 2011-12-13 at 1b5d5c4)
 + enable SO_KEEPALIVE for connected TCP sockets
* jc/checkout-m-twoway (2011-12-11) 2 commits
  (merged to 'next' on 2011-12-11 at b61057f)
 + Test 'checkout -m -- path'
  (merged to 'next' on 2011-12-09 at c946009)
 + checkout -m: no need to insist on having all 3 stages
* tr/cache-tree (2011-12-06) 5 commits
  (merged to 'next' on 2011-12-13 at e0da64d)
 + reset: update cache-tree data when appropriate
 + commit: write cache-tree data when writing index anyway
 + Refactor cache_tree_update idiom from commit
 + Test the current state of the cache-tree optimization
 + Add test-scrap-cache-tree
* tr/userdiff-c-returns-pointer (2011-12-06) 1 commit
  (merged to 'next' on 2011-12-09 at 0b6a092)
 + userdiff: allow * between cpp funcname words
Will merge to 'master'.
* jc/commit-amend-no-edit (2011-12-08) 5 commits
  (merged to 'next' on 2011-12-09 at b9cfa4e)
 + test: commit --amend should honor --no-edit
 + commit: honour --no-edit
 + t7501 (commit): modernize style
 + test: remove a porcelain test that hard-codes commit names
 + test: add missing "&&" after echo command
* jl/submodule-status-failure-report (2011-12-08) 1 commit
  (merged to 'next' on 2011-12-09 at 53eb3b3)
 + diff/status: print submodule path when looking for changes fails
Will merge to 'master'.
* ks/tag-cleanup (2011-12-09) 1 commit
  (merged to 'next' on 2011-12-09 at cbea045)
 + git-tag: introduce --cleanup option
Will merge to 'master'.
* rr/test-chaining (2011-12-11) 7 commits
  (merged to 'next' on 2011-12-13 at b08445e)
 + t3401: use test_commit in setup
 + t3401: modernize style
 + t3040 (subprojects-basic): fix '&&' chaining, modernize style
 + t1510 (worktree): fix '&&' chaining
 + t3030 (merge-recursive): use test_expect_code
 + test: fix '&&' chaining
 + t3200 (branch): fix '&&' chaining
* bc/maint-apply-check-no-patch (2011-12-05) 2 commits
  (merged to 'next' on 2011-12-09 at fc780cd)
 + builtin/apply.c: report error on failure to recognize input
 + t/t4131-apply-fake-ancestor.sh: fix broken test
Will merge to 'master'.
* aw/rebase-i-stop-on-failure-to-amend (2011-11-30) 1 commit
  (merged to 'next' on 2011-12-09 at a117e83)
 + rebase -i: interrupt rebase when "commit --amend" failed during "reword"
* jc/split-blob (2011-12-01) 6 commits
 . WIP (streaming chunked)
 - chunked-object: fallback checkout codepaths
 - bulk-checkin: support chunked-object encoding
 - bulk-checkin: allow the same data to be multiply hashed
 - new representation types in the packstream
 - varint-in-pack: refactor varint encoding/decoding
 (this branch uses jc/stream-to-pack.)

Not ready. At least pack-objects and fsck need to learn the new encoding for the series to be usable locally, and then index-pack/unpack-objects needs to learn it to be used remotely.

* jh/fast-import-notes (2011-11-28) 3 commits
  (merged to 'next' on 2011-12-09 at 2b01132)
 + fast-import: Fix incorrect fanout level when modifying existing notes refs
 + t9301: Add 2nd testcase exposing bugs in fast-import's notes fanout handling
 + t9301: Fix testcase covering up a bug in fast-import's notes fanout handling
Will merge to 'master'.
* tj/maint-imap-send-remove-unused (2011-11-23) 2 commits
  (merged to 'next' on 2011-12-09 at 877cc11)
 + Merge branch 'maint' into tj/imap-send-remove-unused
 + imap-send: Remove unused 'use_namespace' variable
Will merge to 'master'.
* cn/maint-lf-to-crlf-filter (2011-11-28) 1 commit
  (merged to 'next' on 2011-12-09 at c374d14)
 + convert: track state in LF-to-CRLF filter
Will merge to 'master'.
* jn/branch-move-to-self (2011-11-28) 2 commits
  (merged to 'next' on 2011-12-09 at 7d27260)
 + Allow checkout -B <current-branch> to update the current branch
 + branch: allow a no-op "branch -M <current-branch> HEAD"
Will merge to 'master'.
* jk/credentials (2011-12-12) 24 commits
 - contrib: add credential helper for OS X Keychain
 - Makefile: OS X has /dev/tty
 - Makefile: linux has /dev/tty
 - credential: use git_prompt instead of git_getpass
 - prompt: use git_terminal_prompt
 - add generic terminal prompt function
 - refactor git_getpass into generic prompt function
 - move git_getpass to its own source file
 - imap-send: don't check return value of git_getpass
 - imap-send: avoid buffer overflow
  (merged to 'next' on 2011-12-12 at 7a6d658)
 + t: add test harness for external credential helpers
 + credentials: add "store" helper
 + strbuf: add strbuf_add*_urlencode
 + Makefile: unix sockets may not available on some platforms
 + credentials: add "cache" helper
 + docs: end-user documentation for the credential subsystem
 + credential: make relevance of http path configurable
 + credential: add credential.*.username
 + credential: apply helper config
 + http: use credential API to get passwords
 + credential: add function for parsing url components
 + introduce credentials API
 + t5550: fix typo
 + test-lib: add test_config_global variant

Looking good. Probably split the later part into its own topic and merge the parts already in 'next' to 'master' soonish.

* nd/ignore-might-be-precious (2011-11-28) 2 commits
  (merged to 'next' on 2011-12-09 at 1a94553)
 + checkout,merge: disallow overwriting ignored files with --no-overwrite-ignore
 + Merge branch 'nd/maint-ignore-exclude' into nd/ignore-might-be-precious
Will merge to 'master'.
* jk/upload-archive-use-start-command (2011-11-21) 1 commit
  (merged to 'next' on 2011-12-09 at 88cb83a)
 + upload-archive: use start_command instead of fork
* jk/maint-1.6.2-upload-archive (2011-11-21) 1 commit
 + archive: don't let remote clients get unreachable commits
 (this branch is used by jk/maint-upload-archive.)
* jk/maint-upload-archive (2011-11-21) 1 commit
  (merged to 'next' on 2011-12-09 at 03deb16)
 + Merge branch 'jk/maint-1.6.2-upload-archive' into jk/maint-upload-archive
 (this branch uses jk/maint-1.6.2-upload-archive.)
Will merge to 'master'.
* ab/enable-i18n (2011-12-05) 1 commit
  (merged to 'next' on 2011-12-13 at 65af8cd)
 + i18n: add infrastructure for translating Git with gettext
* jc/signed-commit (2011-11-29) 5 commits
 - gpg-interface: allow use of a custom GPG binary
 - pretty: %G[?GS] placeholders
 - test "commit -S" and "log --show-signature"
 - log: --show-signature
 - commit: teach --gpg-sign option
Not exactly urgent.
* jc/stream-to-pack (2011-12-01) 5 commits
  (merged to 'next' on 2011-12-09 at d0fd605)
 + bulk-checkin: replace fast-import based implementation
 + csum-file: introduce sha1file_checkpoint
 + finish_tmp_packfile(): a helper function
 + create_tmp_packfile(): a helper function
 + write_pack_header(): a helper function
 (this branch is used by jc/split-blob.)

Teaches "git add" to send large-ish blob data straight to a packfile. This is a continuation to the "large file support" topic. The codepath to move data from worktree to repository is made aware of streaming, just like the checkout codepath that goes the other way, which was done in the previous "large file support" topic in the 1.7.7 cycle.

* jn/gitweb-side-by-side-diff (2011-10-31) 8 commits
  (merged to 'next' on 2011-12-09 at 7662e58)
 + gitweb: Add navigation to select side-by-side diff
 + gitweb: Use href(-replay=>1,...) for formats links in "commitdiff"
 + t9500: Add basic sanity tests for side-by-side diff in gitweb
 + t9500: Add test for handling incomplete lines in diff by gitweb
 + gitweb: Give side-by-side diff extra CSS styling
 + gitweb: Add a feature to show side-by-side diff
 + gitweb: Extract formatting of diff chunk header
 + gitweb: Refactor diff body line classification
Replaces a series from Kato Kazuyoshi on the same topic.
Will merge to 'master'.
Jonathan Nieder· Dec 14, 2011, 07:09 UTC · re: Junio C Hamano · lore

tr/pty-all (Re: What's cooking in git.git (Dec 2011, #04; Tue, 13))

Junio C Hamano wrote:
Show 6 quoted lines
> * tr/pty-all (2011-12-12) 3 commits
>  - t/lib-terminal: test test-terminal's sanity
>  - test-terminal: set output terminals to raw mode
>  - test-terminal: give the child an empty stdin TTY
>
> Will merge to 'next' after taking another look.

The middle commit looks good. The bottom commit could be improved as discussed at [1], but I guess that can happen in-tree.

However, the top commit ("test test-terminal's sanity") still does not seem right to me.

It makes the same test run three times. Probably I should send an alternate patch to get that sanity-check to run once, but I am also not convinced the sanity-check is needed at all --- wouldn't any test that is relying on output from test_terminal act as a sanity check for it already?

As an aside, I also still believe that running "git shortlog" without explicitly passing "HEAD" when testing how it reacts to [core] pager configuration was a bug and a distraction, hence the patch at [2]. Am I the only one? I also find Jeff's patch [3] appealing.

Thanks, Jonathan

[1] http://thread.gmane.org/gmane.comp.version-control.git/186923/focus=186944 [2] http://article.gmane.org/gmane.comp.version-control.git/186932 [3] http://article.gmane.org/gmane.comp.version-control.git/186936

Thomas Rast· Dec 14, 2011, 16:17 UTC · re: Jonathan Nieder · lore

Re: tr/pty-all (Re: What's cooking in git.git (Dec 2011, #04; Tue, 13))

Jonathan Nieder wrote:
Show 8 quoted lines
> Junio C Hamano wrote:
> > Will merge to 'next' after taking another look.
> 
> The middle commit looks good.  The bottom commit could be improved as
> discussed at [1], but I guess that can happen in-tree.
> 
> However, the top commit ("test test-terminal's sanity") still does not
> seem right to me.
I wasn't under the impression that we were done with this, either :-)
Show 5 quoted lines
> It makes the same test run three times.  Probably I should send an
> alternate patch to get that sanity-check to run once, but I am also
> not convinced the sanity-check is needed at all --- wouldn't any test
> that is relying on output from test_terminal act as a sanity check for
> it already?

It didn't. Or more precisely, Michael Haggerty ran into the behavior of

  git rev-parse ... | while read sha; do git checkout $sha; make test; done

couldn't make any sense of it, and reported it on IRC. So in some sense, it took infrequent circumstances and two developers' time; next time around I'd prefer it to be detected automatically.

> As an aside, I also still believe that running "git shortlog" without
> explicitly passing "HEAD" when testing how it reacts to [core] pager
> configuration was a bug and a distraction, hence the patch at [2].

Why not. Some other test should verify how shortlog reacts to the tty-ness of stdin, but that's yet another direction.

> I also find Jeff's patch [3] appealing.

Me too, though wonder whether feeding a file full of garbage wouldn't be better, so as to trip up commands that try to read only from a non-tty stdin.

> [1] http://thread.gmane.org/gmane.comp.version-control.git/186923/focus=186944
> [2] http://article.gmane.org/gmane.comp.version-control.git/186932
> [3] http://article.gmane.org/gmane.comp.version-control.git/186936
-- 
Thomas Rast
trast@{inf,student}.ethz.ch
Junio C Hamano· Dec 14, 2011, 17:42 UTC · re: Thomas Rast · lore

Re: tr/pty-all (Re: What's cooking in git.git (Dec 2011, #04; Tue, 13))

Thomas Rast <trast@student.ethz.ch> writes:
Show 39 quoted lines
> Jonathan Nieder wrote:
>> Junio C Hamano wrote:
>> > Will merge to 'next' after taking another look.
>> 
>> The middle commit looks good.  The bottom commit could be improved as
>> discussed at [1], but I guess that can happen in-tree.
>> 
>> However, the top commit ("test test-terminal's sanity") still does not
>> seem right to me.
>
> I wasn't under the impression that we were done with this, either :-)
>
>> It makes the same test run three times.  Probably I should send an
>> alternate patch to get that sanity-check to run once, but I am also
>> not convinced the sanity-check is needed at all --- wouldn't any test
>> that is relying on output from test_terminal act as a sanity check for
>> it already?
>
> It didn't.  Or more precisely, Michael Haggerty ran into the behavior
> of
>
>   git rev-parse ... | while read sha; do git checkout $sha; make test; done
>
> couldn't make any sense of it, and reported it on IRC.  So in some
> sense, it took infrequent circumstances and two developers' time; next
> time around I'd prefer it to be detected automatically.
>
>> As an aside, I also still believe that running "git shortlog" without
>> explicitly passing "HEAD" when testing how it reacts to [core] pager
>> configuration was a bug and a distraction, hence the patch at [2].
>
> Why not.  Some other test should verify how shortlog reacts to the
> tty-ness of stdin, but that's yet another direction.
>
>> I also find Jeff's patch [3] appealing.
>
> Me too, though wonder whether feeding a file full of garbage wouldn't
> be better, so as to trip up commands that try to read only from a
> non-tty stdin.

Well, I guess I was too quick to pull the trigger after sending the "What's cooking" out. Sorry about that.

On the other hand, I think these require relatively low impact changes that can be handled in-tree and the downsides of the series like running prerequisite tests more than once are not serious show stoppers, so it isn't a disaster ;-)

Thanks both for noticing and commenting. Very much appreciated.
Jeff King· Dec 14, 2011, 23:07 UTC · re: Thomas Rast · lore

Re: tr/pty-all (Re: What's cooking in git.git (Dec 2011, #04; Tue, 13))

On Wed, Dec 14, 2011 at 05:17:14PM +0100, Thomas Rast wrote:
Show 5 quoted lines
> > I also find Jeff's patch [3] appealing.
> 
> Me too, though wonder whether feeding a file full of garbage wouldn't
> be better, so as to trip up commands that try to read only from a
> non-tty stdin.

Interesting idea. Instead of getting an immediate EOF from /dev/null, they'll get some junk which they may or may not complain about. I played around with this a bit, redirecting test stdin from a file with:

  This is a garbage file that will be connected to the stdin of each
  test.

(with the hope that when you see that in an error message, it will be obvious what has happened).

It turns out that shortlog (the one bug of this type in the test suite) doesn't care at all. It will happily chew on that garbage and behave just the same as if it had been given /dev/null.

There is also the minor issue that if there are multiple invocations of the input-chewing command, the first one will eat all of the input, and subsequent ones will just get the equivalent of /dev/null anyway. I.e.:

  test_expect_success 'git shortlog $foo' ;# will eat all of our stdin
  test_expect_success 'git shortlog $bar' ;# sees immediate EOF

That may not matter, though, as you would hope for the first invocation to complain, at which point you fix it, revealing the failures in later tests (or if you're clever, noticing in the first place that they all have the same bug).

You can make this somewhat more robust by redirecting stdin for _test_eval (so each test gets a fresh descriptor to the garbage file). However, that disallows redirecting a specific test's stdin in the test script, like:

  test_expect_success 'some test' '
    cat >expect &&
    some_command >actual &&
    test_cmp expect actual
  ' <<\EOF
  the expected
  data
  EOF

This construct probably seems silly at first, but see how it is used in t6006:

  test_format author %an%n%ae%n%ad%n%aD%n%at <<\EOF
  commit ...
  A U Thor
  author@example.com
  ... etc ...
  EOF

where test_format is a generic function that just gobbles stdin into the "expect" file. As it turns out, test_format actually runs the "cat" outside of the "test_expect_success", but quite often we put it inside. So maybe it isn't worth caring about. You could also argue that the whole thing should just be inside a test_expect_success.

-Peff
Jeff King· Dec 14, 2011, 23:21 UTC · re: Jeff King · lore

Re: tr/pty-all (Re: What's cooking in git.git (Dec 2011, #04; Tue, 13))

On Wed, Dec 14, 2011 at 06:07:13PM -0500, Jeff King wrote:
Show 11 quoted lines
> On Wed, Dec 14, 2011 at 05:17:14PM +0100, Thomas Rast wrote:
> 
> > > I also find Jeff's patch [3] appealing.
> > 
> > Me too, though wonder whether feeding a file full of garbage wouldn't
> > be better, so as to trip up commands that try to read only from a
> > non-tty stdin.
> 
> Interesting idea. Instead of getting an immediate EOF from /dev/null,
> they'll get some junk which they may or may not complain about. I played
> around with this a bit, redirecting test stdin from a file with:

So here is what that patch looked like. As I mentioned, it doesn't actually catch the shortlog problem, but it would fix Michael's issue with an outer reading loop.

-- >8 --
Subject: [PATCH] test-lib: redirect stdin of tests

We want to run tests in a predictable, sterile environment so we can get repeatable results. They should take as little input as possible from the environment outside the test script. We already sanitize environment variables, but leave stdin untouched. This means that scripts can accidentally be impacted by content on stdin, or whether stdin isatty().

Furthermore, scripts reading from stdin can be annoying to outer loops which care about their stdin offset, like:

  while read sha1; do
      make test
  done

A test which accidentally reads stdin would soak up all of the rest of the input intended for the outer shell loop.

Let's redirect stdin from a known source to eliminate variation. We could just connect it to /dev/null. However, tests which accidentally read stdin would then see immediate EOF, which may or may not cause them to notice the errror. Instead, let's connect it to a file with random garbage in it, in the hope that it will be more likely to trigger an error in the mis-written test.

We'll also leave file descriptor 6 as a link to the original stdin. Tests shouldn't need to look at this, but it can be convenient for inserting interactive commands while debugging tests (e.g., you could insert "bash <&6 >&3 2>&4" to run interactive commands in the environment of the test script).

Signed-off-by: Jeff King <peff@peff.net>
---
 t/stdin-garbage |    1 +
 t/test-lib.sh   |    4 +++-
 2 files changed, 4 insertions(+), 1 deletions(-)
 create mode 100644 t/stdin-garbage
diff --git a/t/stdin-garbage b/t/stdin-garbage
new file mode 100644
index 0000000..3a2ebc2
--- /dev/null
+++ b/t/stdin-garbage
@@ -0,0 +1 @@
+This is a garbage file that will be connected to the stdin of each test.
diff --git a/t/test-lib.sh b/t/test-lib.sh
index bdd9513..67eb078 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -198,6 +198,8 @@ else
 	exec 4>/dev/null 3>/dev/null
 fi
 
+exec 6<&0
+
 test_failure=0
 test_count=0
 test_fixed=0
@@ -469,7 +471,7 @@ test_debug () {
 test_eval_ () {
 	# This is a separate function because some tests use
 	# "return" to end a test_expect_success block early.
-	eval >&3 2>&4 "$*"
+	eval <"$TEST_DIRECTORY/stdin-garbage" >&3 2>&4 "$*"
 }
 
 test_run_ () {
-- 
1.7.8.rc2.30.g803b1a
Jonathan Nieder· Dec 14, 2011, 23:31 UTC · re: Jeff King · lore

Re: tr/pty-all (Re: What's cooking in git.git (Dec 2011, #04; Tue, 13))

Jeff King wrote:
[...]
Show 7 quoted lines
> A test which accidentally reads stdin would soak up all of
> the rest of the input intended for the outer shell loop.
> 
> Let's redirect stdin from a known source to eliminate
> variation. We could just connect it to /dev/null. However,
> tests which accidentally read stdin would then see immediate
> EOF, which may or may not cause them to notice the errror.
[...]
> +++ b/t/test-lib.sh
[...]
Show 6 quoted lines
> @@ -469,7 +471,7 @@ test_debug () {
>  test_eval_ () {
>  	# This is a separate function because some tests use
>  	# "return" to end a test_expect_success block early.
> -	eval >&3 2>&4 "$*"
> +	eval <"$TEST_DIRECTORY/stdin-garbage" >&3 2>&4 "$*"

How about /dev/urandom on platforms that support it? It wouldn't be as pleasant to debug as "This is a magic stdin garbage stream", but it would be more likely to (despite the name :)) predictably trip errors, or at least hangs, in problematic tests.

With or without something along those lines on top, your patch looks like a good change.

Thanks for a thoughtful analysis. Jonathan

Jeff King· Dec 15, 2011, 00:25 UTC · re: Jonathan Nieder · lore

Re: tr/pty-all (Re: What's cooking in git.git (Dec 2011, #04; Tue, 13))

On Wed, Dec 14, 2011 at 05:31:19PM -0600, Jonathan Nieder wrote:
Show 11 quoted lines
> > @@ -469,7 +471,7 @@ test_debug () {
> >  test_eval_ () {
> >  	# This is a separate function because some tests use
> >  	# "return" to end a test_expect_success block early.
> > -	eval >&3 2>&4 "$*"
> > +	eval <"$TEST_DIRECTORY/stdin-garbage" >&3 2>&4 "$*"
> 
> How about /dev/urandom on platforms that support it?  It wouldn't be
> as pleasant to debug as "This is a magic stdin garbage stream", but it
> would be more likely to (despite the name :)) predictably trip errors,
> or at least hangs, in problematic tests.

I'd rather have something deterministic. If you really want to be mean to accidental readers of stdin, then put binary junk into stdin-garbage (even the results of a single run of /dev/urandom, if you like). But I suspect arbitrary text is good enough to throw a monkey wrench into anything that will care about its input (and those that don't are beyond our ability to auto-detect anyway[1]). And it's way easier to debug than seeing random binary bits.

-Peff

[1] Actually, you could abandon the idea of feeding garbage altogether, and instead open the descriptor outside the test, then check that its offset is still 0 after the test. You'd have to use a helper program to do the ftell(), but it should work as the descriptor position will be shared.

Jeff King· Dec 15, 2011, 00:50 UTC · re: Jeff King · lore

Re: tr/pty-all (Re: What's cooking in git.git (Dec 2011, #04; Tue, 13))

On Wed, Dec 14, 2011 at 07:25:30PM -0500, Jeff King wrote:
Show 5 quoted lines
> [1] Actually, you could abandon the idea of feeding garbage altogether,
> and instead open the descriptor outside the test, then check that its
> offset is still 0 after the test. You'd have to use a helper program to
> do the ftell(), but it should work as the descriptor position will be
> shared.

And here's what that patch would look like. You still want to feed a garbage file, because you want to make sure that there is something for it to actually read. And then either it can choke on the garbage and fail, or if not, you can detect afterwards that it was read.

This correctly detects the bug in t7006. I can't decide if it's clever or ugly.

---
 t/stdin-garbage |    1 +
 t/test-lib.sh   |   16 ++++++++++++++++
 2 files changed, 17 insertions(+), 0 deletions(-)
 create mode 100644 t/stdin-garbage
diff --git a/t/stdin-garbage b/t/stdin-garbage
new file mode 100644
index 0000000..3a2ebc2
--- /dev/null
+++ b/t/stdin-garbage
@@ -0,0 +1 @@
+This is a garbage file that will be connected to the stdin of each test.
diff --git a/t/test-lib.sh b/t/test-lib.sh
index bdd9513..9b4692b 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -466,6 +466,10 @@ test_debug () {
 	test "$debug" = "" || eval "$1"
 }
 
+test_stdin_unread_ () {
+	test "`perl -e 'print tell(STDIN)'`" = 0
+}
+
 test_eval_ () {
 	# This is a separate function because some tests use
 	# "return" to end a test_expect_success block early.
@@ -475,9 +479,21 @@ test_eval_ () {
 test_run_ () {
 	test_cleanup=:
 	expecting_failure=$2
+	# feed the test a bogus stdin, but keep a spare descriptor open in case
+	# the test redirects stdin, which affects us since it is an eval
+	exec 6<&0
+	exec 7<"$TEST_DIRECTORY/stdin-garbage"
+	exec 0<&7
 	test_eval_ "$1"
 	eval_ret=$?
 
+	# check that nobody read from our bogus descriptor
+	if test $eval_ret = 0 && ! test_stdin_unread_ <&7; then
+		echo >&4 "bug in the test script: somebody read from stdin"
+		eval_ret=1
+	fi
+	exec 0<&6
+
 	if test -z "$immediate" || test $eval_ret = 0 || test -n "$expecting_failure"
 	then
 		test_eval_ "$test_cleanup"
-- 
1.7.8.rc2.30.g803b1a
Junio C Hamano· Dec 15, 2011, 03:23 UTC · re: Jeff King · lore

Re: tr/pty-all (Re: What's cooking in git.git (Dec 2011, #04; Tue, 13))

Jeff King <peff@peff.net> writes:
> This correctly detects the bug in t7006. I can't decide if it's clever
> or ugly.

I would say it falls on the latter side of the line by small margin. Let's do the /dev/null thing and be done with it.

Jeff King· Dec 15, 2011, 06:55 UTC · re: Junio C Hamano · lore

Re: tr/pty-all (Re: What's cooking in git.git (Dec 2011, #04; Tue, 13))

On Wed, Dec 14, 2011 at 07:23:14PM -0800, Junio C Hamano wrote:
Show 7 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> > This correctly detects the bug in t7006. I can't decide if it's clever
> > or ugly.
> 
> I would say it falls on the latter side of the line by small margin. Let's
> do the /dev/null thing and be done with it.
Darn, I wanted to post it on my fridge with an "A+".
Here's a cleaned-up version of the /dev/null one.
-- >8 --
Subject: [PATCH] test-lib: redirect stdin of tests

We want to run tests in a predictable, sterile environment so we can get repeatable results. They should take as little input as possible from the environment outside the test script. We already sanitize environment variables, but leave stdin untouched. This means that scripts can accidentally be impacted by content on stdin, or whether stdin isatty().

Furthermore, scripts reading from stdin can be annoying to outer loops which care about their stdin offset, like:

  while read sha1; do
      make test
  done

A test which accidentally reads stdin would soak up all of the rest of the input intended for the outer shell loop.

Let's redirect stdin from /dev/null, which solves both of these problems. It won't detect tests accidentally reading from stdin, but since doing so now gives a deterministic result, we don't need to consider that an error.

We'll also leave file descriptor 6 as a link to the original stdin. Tests shouldn't need to look at this, but it can be convenient for inserting interactive commands while debugging tests (e.g., you could insert "bash <&6 >&3 2>&4" to run interactive commands in the environment of the test script).

Signed-off-by: Jeff King <peff@peff.net>
---
I went the "redirect each test individually" route. In the course of my
experimentation, I notice that some tests (e.g., t4013) will do:
  while read x; do
          test_expect_success "test something ($x)" "
            ... do some test involving $x ...
          "
  done <<\EOF
  ... some values of $x ....
  EOF

This protects those loops from accidental stdin-readers inside the test scripts, too.

 t/test-lib.sh |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index bdd9513..5ea9fe3 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -191,6 +191,7 @@ then
 fi
 
 exec 5>&1
+exec 6<&0
 if test "$verbose" = "t"
 then
 	exec 4>&2 3>&1
@@ -469,7 +470,7 @@ test_debug () {
 test_eval_ () {
 	# This is a separate function because some tests use
 	# "return" to end a test_expect_success block early.
-	eval >&3 2>&4 "$*"
+	eval </dev/null >&3 2>&4 "$*"
 }
 
 test_run_ () {
-- 
1.7.8.rc2.30.g803b1a
Junio C Hamano· Dec 15, 2011, 18:19 UTC · re: Jeff King · lore

Re: tr/pty-all (Re: What's cooking in git.git (Dec 2011, #04; Tue, 13))

Jeff King <peff@peff.net> writes:
Show 11 quoted lines
> On Wed, Dec 14, 2011 at 07:23:14PM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > This correctly detects the bug in t7006. I can't decide if it's clever
>> > or ugly.
>> 
>> I would say it falls on the latter side of the line by small margin. Let's
>> do the /dev/null thing and be done with it.
>
> Darn, I wanted to post it on my fridge with an "A+".
Heh.

If it were without the perl "tell" bits, it would have been "clever and clean", perhaps like

	arrange the following to read from t/random-stdin file
         - run the test script
         - "read x"
        compare the $x you just read with t/random-stdin file
But I think it is good enough to be stupid and clean.
> Here's a cleaned-up version of the /dev/null one.
Thanks.

← back to recent threads