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

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

[Junio C Hamano]

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

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

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

[Jonathan Nieder]

Junio C Hamano wrote:

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

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

[Thomas Rast]

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.



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

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

[Junio C Hamano]

Thomas Rast <trast@student.ethz.ch> writes:

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

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

[Jeff King]

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:

  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

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

[Jeff King]

On Wed, Dec 14, 2011 at 06:07:13PM -0500, Jeff King wrote:

> 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

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

[Jonathan Nieder]

Jeff King wrote:

[...]
> 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
[...]
> @@ -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

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

[Jeff King]

On Wed, Dec 14, 2011 at 05:31:19PM -0600, Jonathan Nieder wrote:

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

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

[Jeff King]

On Wed, Dec 14, 2011 at 07:25:30PM -0500, Jeff King wrote:

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

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

[Junio C Hamano]

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.

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

[Jeff King]

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

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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