What's cooking in git.git (Dec 2011, #02; Mon, 5)

What's cooking in git.git (Dec 2011, #02; Mon, 5)

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

The tip of 'next' will be rewound soonish, kicking a handful of topics
back to 'pu' that are important enough to get them right rather than
piling "oops, that was a mistake" fix-ups in 'next'.

All of the topics in Stalled section will be purged without prejudice;
owners of respective topics are free to re-submit after addressing the
issues raised during the previous reviews and re-ignite the discussion.

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]

* bc/maint-apply-check-no-patch (2011-12-05) 2 commits
 - builtin/apply.c: report error on failure to recognize input
 - t/t4131-apply-fake-ancestor.sh: fix broken test

Will merge to 'next'.

--------------------------------------------------
[Graduated to "master"]

* ab/clang-lints (2011-11-06) 2 commits
  (merged to 'next' on 2011-11-13 at a573aec)
 + cast variable in call to free() in builtin/diff.c and submodule.c
 + apply: get rid of useless x < 0 comparison on a size_t type

* gh/userdiff-matlab (2011-11-15) 1 commit
  (merged to 'next' on 2011-11-18 at 10cd275)
 + Add built-in diff patterns for MATLAB code

* jc/maint-pack-object-cycle (2011-11-16) 1 commit
  (merged to 'next' on 2011-11-18 at 3715a81)
 + pack-object: tolerate broken packs that have duplicated objects

Make the client side more robust against bogus pack stream; the problem
was discovered by accident while repacking a clone obtained from somewhat
buggy test server.

* jc/index-pack-reject-dups (2011-11-16) 1 commit
  (merged to 'next' on 2011-11-18 at 2090221)
 + receive-pack, fetch-pack: reject bogus pack that records objects twice

And this is the prevention to reject such pack stream in the first place.

* jk/refresh-porcelain-output (2011-11-18) 3 commits
  (merged to 'next' on 2011-11-18 at 872f25e)
 + refresh_index: make porcelain output more specific
 + refresh_index: rename format variables
 + read-cache: let refresh_cache_ent pass up changed flags

* mf/curl-select-fdset (2011-11-04) 4 commits
  (merged to 'next' on 2011-11-06 at a49516f)
 + http: drop "local" member from request struct
 + http.c: Rely on select instead of tracking whether data was received
 + http.c: Use timeout suggested by curl instead of fixed 50ms timeout
 + http.c: Use curl_multi_fdset to select on curl fds instead of just sleeping

Reduces unnecessary waits.

* na/strtoimax (2011-11-05) 3 commits
  (merged to 'next' on 2011-11-14 at b64e1cb)
 + Support sizes >=2G in various config options accepting 'g' sizes.
 + Compatibility: declare strtoimax() under NO_STRTOUMAX
 + Add strtoimax() compatibility function.

* nd/fsck-progress (2011-11-06) 4 commits
  (merged to 'next' on 2011-11-13 at 8831811)
 + fsck: print progress
 + fsck: avoid reading every object twice
 + verify_packfile(): check as many object as possible in a pack
 + fsck: return error code when verify_pack() goes wrong

* nd/maint-ignore-exclude (2011-11-28) 1 commit
 + checkout,merge: loosen overwriting untracked file check based on info/exclude
 (this branch is used by nd/ignore-might-be-precious.)

* nd/misc-cleanups (2011-10-27) 6 commits
  (merged to 'next' on 2011-10-28 at 2527a49)
 + unpack_object_header_buffer(): clear the size field upon error
 + tree_entry_interesting: make use of local pointer "item"
 + tree_entry_interesting(): give meaningful names to return values
 + read_directory_recursive: reduce one indentation level
 + get_tree_entry(): do not call find_tree_entry() on an empty tree
 + tree-walk.c: do not leak internal structure in tree_entry_len()

* nd/prune-progress (2011-11-07) 3 commits
  (merged to 'next' on 2011-11-13 at c5722ac)
 + reachable: per-object progress
 + prune: handle --progress/no-progress
 + prune: show progress while marking reachable objects

* vr/git-merge-doc (2011-11-21) 1 commit
 + Show error for 'git merge' with unset merge.defaultToUpstream

* vr/msvc (2011-10-31) 3 commits
  (merged to 'next' on 2011-11-14 at f46d021)
 + MSVC: Remove unneeded header stubs
 + Compile fix for MSVC: Include <io.h>
 + Compile fix for MSVC: Do not include sys/resources.h

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

All of the topics in Stalled section will be discarded soonish (see the
cover).

* hv/submodule-merge-search (2011-10-13) 4 commits
 - submodule.c: make two functions static
 - allow multiple calls to submodule merge search for the same path
 - push: Don't push a repository with unpushed submodules
 - push: teach --recurse-submodules the on-demand option

What the topic aims to achieve may make sense, but the implementation
looked somewhat suboptimal.

* sr/transport-helper-fix-rfc (2011-07-19) 2 commits
 - t5800: point out that deleting branches does not work
 - t5800: document inability to push new branch with old content

See comments on sr/fix-fast-export-tips topic.

* sr/fix-fast-export-tips (2011-11-05) 3 commits
 - fast-export: output reset command for commandline revs
 - fast-export: do not refer to non-existing marks
 - t9350: point out that refs are not updated correctly

The bottom commit from the stalled sr/transport-helper-fix-rfc topic is
fixed with this. It may make sense to drop the other topic and include
that commit in this series.

The command line parser is still too lax and accepts malformed input, but
this is a step in the right direction and tightening the command line now
should be doable without a low level surgery that touches codepaths that
are unrelated to the command line processing like the previous attempt
used to do.

* jc/lookup-object-hash (2011-08-11) 6 commits
 - object hash: replace linear probing with 4-way cuckoo hashing
 - object hash: we know the table size is a power of two
 - object hash: next_size() helper for readability
 - pack-objects --count-only
 - object.c: remove duplicated code for object hashing
 - object.c: code movement for readability

I do not think there is anything fundamentally wrong with this series, but
the risk of breakage far outweighs observed performance gain in one
particular workload.

* jc/verbose-checkout (2011-10-16) 2 commits
 - checkout -v: give full status output after switching branches
 - checkout: move the local changes report to the end

This is just to leave a record that the reason why we do not do this not
because we are incapable of coding this, but because it is not a good idea
to do this. I suspect people who are new to git that might think they need
it would soon realize the don't.

Will drop.

* eh/grep-scale-to-cpunum (2011-11-05) 1 commit
 - grep: detect number of CPUs for thread spawning

Kills I/O parallelism and needs to be improved or discarded.

* jc/commit-tree-extra (2011-11-12) 2 commits
 - commit-tree: teach -C <extra-commit>
 - commit-tree: teach -x <extra>
 (this branch uses jc/pull-signed-tag; is tangled with jc/signed-commit.)

Not absolutely needed; parked in 'pu' but may drop.

* cb/daemon-permission-errors (2011-10-17) 2 commits
 - daemon: report permission denied error to clients
 - daemon: add tests

The tip commit might be loosening things a bit too much.

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

* sg/complete-refs (2011-10-21) 9 commits
  (merged to 'next' on 2011-10-26 at d65e2b4)
 + completion: remove broken dead code from __git_heads() and __git_tags()
 + completion: fast initial completion for config 'remote.*.fetch' value
 + completion: improve ls-remote output filtering in __git_refs_remotes()
 + completion: query only refs/heads/ in __git_refs_remotes()
 + completion: support full refs from remote repositories
 + completion: improve ls-remote output filtering in __git_refs()
 + completion: make refs completion consistent for local and remote repos
 + completion: optimize refs completion
 + completion: document __gitcomp()

Will merge to 'master.

* aw/rebase-i-stop-on-failure-to-amend (2011-11-30) 1 commit
 - rebase -i: interrupt rebase when "commit --amend" failed during "reword"

Will merge to 'next'.

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

* jh/fast-import-notes (2011-11-28) 3 commits
 - 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 'next'.

* ld/p4-labels-branches (2011-11-30) 4 commits
 - git-p4: importing labels should cope with missing owner
 - git-p4: add test for p4 labels
 - git-p4: cope with labels with empty descriptions
 - git-p4: handle p4 branches and labels containing shell chars

I understand this has been retracted---please correct me otherwise.
Will discard, expecting a reroll.

* tj/maint-imap-send-remove-unused (2011-11-23) 2 commits
 - Merge branch 'maint' into tj/imap-send-remove-unused
 - imap-send: Remove unused 'use_namespace' variable

Will merge to 'next'.

* cn/maint-lf-to-crlf-filter (2011-11-28) 1 commit
 - convert: track state in LF-to-CRLF filter

Will merge to 'next' after taking another look.

* jn/branch-move-to-self (2011-11-28) 2 commits
 - Allow checkout -B <current-branch> to update the current branch
 - branch: allow a no-op "branch -M <current-branch> HEAD"

Will merge to 'next' after taking another look.

* jk/credentials (2011-11-28) 20 commits
 - fixup! 034c066e
 - compat/getpass: add a /dev/tty implementation
 - credential: use git_prompt instead of git_getpass
 - prompt: add PROMPT_ECHO flag
 - stub out getpass_echo function
 - refactor git_getpass into generic prompt function
 - move git_getpass to its own source file
 - t: add test harness for external credential helpers
 - credentials: add "store" helper
 - strbuf: add strbuf_add*_urlencode
 - 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

Expecting a reroll?

* nd/ignore-might-be-precious (2011-11-28) 2 commits
 - 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 'next' after taking another look.

* jk/upload-archive-use-start-command (2011-11-21) 1 commit
 - upload-archive: use start_command instead of fork

What's the status of this one?

* 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
 - 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 'next' after taking another look.

* ab/enable-i18n (2011-12-05) 2 commits
 - Makefile: Add missing "sparse object" to gettext target-specific rule
 - i18n: add infrastructure for translating Git with gettext

Will merge to 'next' after squashing the two and re-sending to the list
for review comments.

* nd/resolve-ref (2011-12-05) 2 commits
  (merged to 'next' on 2011-12-05 at cc79e86)
 + Copy resolve_ref() return value for longer use
 + Convert many resolve_ref() calls to read_ref*() and ref_exists()

Will merge to 'master'.

* 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
 (this branch uses jc/pull-signed-tag; is tangled with jc/commit-tree-extra.)

Not exactly urgent.
Will concentrate on getting jc/pull-signed-tag in first.

* jc/pull-signed-tag (2011-11-12) 15 commits
  (merged to 'next' on 2011-11-14 at 25e8838)
 + commit-tree: teach -m/-F options to read logs from elsewhere
 + commit-tree: update the command line parsing
 + commit: teach --amend to carry forward extra headers
 + merge: force edit and no-ff mode when merging a tag object
 + commit: copy merged signed tags to headers of merge commit
 + merge: record tag objects without peeling in MERGE_HEAD
 + merge: make usage of commit->util more extensible
 + fmt-merge-msg: Add contents of merged tag in the merge message
 + fmt-merge-msg: package options into a structure
 + fmt-merge-msg: avoid early returns
 + refs DWIMmery: use the same rule for both "git fetch" and others
 + fetch: allow "git fetch $there v1.0" to fetch a tag
 + merge: notice local merging of tags and keep it unwrapped
 + fetch: do not store peeled tag object names in FETCH_HEAD
 + Split GPG interface into its own helper library
 (this branch is used by jc/commit-tree-extra and jc/signed-commit.)

Allow pulling/merging a signed tag instead of a branch tip, and record
the GPG signature in the merge commit for later audit.

Will merge to 'master'.

* jc/request-pull-show-head-4 (2011-11-09) 12 commits
  (merged to 'next' on 2011-11-13 at e473fd2)
 + request-pull: use the annotated tag contents
  (merged to 'next' on 2011-10-15 at 7e340ff)
 + fmt-merge-msg.c: Fix an "dubious one-bit signed bitfield" sparse error
  (merged to 'next' on 2011-10-10 at 092175e)
 + environment.c: Fix an sparse "symbol not declared" warning
 + builtin/log.c: Fix an "Using plain integer as NULL pointer" warning
  (merged to 'next' on 2011-10-07 at fcaeca0)
 + fmt-merge-msg: use branch.$name.description
  (merged to 'next' on 2011-10-06 at fa5e0fe)
 + request-pull: use the branch description
 + request-pull: state what commit to expect
 + request-pull: modernize style
 + branch: teach --edit-description option
 + format-patch: use branch description in cover letter
 + branch: add read_branch_desc() helper function
 + Merge branch 'bk/ancestry-path' into jc/branch-desc

Allow setting "description" for branches and use it to help communications
between humans in various workflow elements. It also allows requesting for
a signed tag to be pulled and shows the tag message in the generated message.

Will merge to 'master'.

* ab/pull-rebase-config (2011-11-07) 1 commit
  (merged to 'next' on 2011-11-13 at 72bb2d5)
 + pull: introduce a pull.rebase option to enable --rebase

Will merge to 'master'.

* jc/stream-to-pack (2011-12-01) 5 commits
 - 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.

Will merge to 'next'.

* jn/gitweb-side-by-side-diff (2011-10-31) 8 commits
 - 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.
Is this ready for 'next'?

* rs/allocate-cache-entry-individually (2011-10-26) 2 commits
  (merged to 'next' on 2011-10-27 at 2e4acd6)
 + cache.h: put single NUL at end of struct cache_entry
 + read-cache.c: allocate index entries individually

Will merge to 'master'.

* rr/revert-cherry-pick (2011-10-23) 5 commits
  (merged to 'next' on 2011-10-26 at 27b7496)
 + revert: simplify communicating command-line arguments
 + revert: allow mixed pick and revert instructions
 + revert: make commit subjects in insn sheet optional
 + revert: simplify getting commit subject in format_todo()
 + revert: free msg in format_todo()

The internals of "git revert/cherry-pick" has been further refactored to
serve as the basis for the sequencer.

Is this ready for 'master', or should we eject it to 'pu', expecing a
re-roll?

* mh/ref-api-2 (2011-11-16) 15 commits
  (merged to 'next' on 2011-11-16 at 511457f)
 + refs: loosen over-strict "format" check
  (merged to 'next' on 2011-10-19 at cc89f0e)
 + 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
 + cache.h: add comments for git_path() and git_path_submodule()
 (this branch is tangled with mh/ref-api-3.)

Expecting a re-roll; will eject from 'pu' so that the topic can lose the
hot-fix at the tip.

* mh/ref-api-3 (2011-11-16) 26 commits
  (merged to 'next' on 2011-11-16 at cc76151)
 + refs: loosen over-strict "format" check
  (merged to 'next' on 2011-10-23 at 92e2d35)
 + 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_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()
 + parse_ref_line(): add a check that the refname is properly formatted
 + repack_without_ref(): remove temporary
 + Rename another local variable name -> refname
  (merged to 'next' on 2011-10-19 at cc89f0e)
 + 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
 + cache.h: add comments for git_path() and git_path_submodule()
 (this branch is tangled with mh/ref-api-2.)

Expecting a re-roll; will eject from 'pu' so that the topioc can lose the
hot-fix at the tip.

Re: What's cooking in git.git (Dec 2011, #02; Mon, 5)

[Ramkumar Ramachandra]

Hi Junio,

Junio C Hamano wrote:
> * rr/revert-cherry-pick (2011-10-23) 5 commits
>  (merged to 'next' on 2011-10-26 at 27b7496)
> [...]
>
> The internals of "git revert/cherry-pick" has been further refactored to
> serve as the basis for the sequencer.
>
> Is this ready for 'master', or should we eject it to 'pu', expecing a
> re-roll?

[Notation: `rr/sequencer` is a new series I'm working on based on
`rr/revert-cherry-pick`]

I'm confused and I need your advice.  I've stopped working on
`rr/sequencer` because this branch is out-of-date now.  We can do one
of the following the improve the situation:
0. You can merge this into `master` resolving the conflicts: it's a
fairly straightforward resolution.  As soon as you publish the new
`master`, I can continue working on `rr/sequencer`.
1. I can post the `rr/revert-cherry-pick` to the list, hoping that it
will make it to `master` without more disruptions.  I can rebase
`rr/sequencer` on this new series and continue working.  For your
reference, this [1] is what I'll be posting to the list if we pick
this option.
2. I can squash `rr/sequencer` into `rr/revert-cherry-pick` to post an
entirely new series.  I think this is a really bad option.

Thanks.

-- Ram

[1]: https://github.com/artagnon/git/commits/rr/revert-cherry-pick

Re: What's cooking in git.git (Dec 2011, #02; Mon, 5)

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> 0. You can merge this into `master` resolving the conflicts: it's a
> fairly straightforward resolution.  As soon as you publish the new
> `master`, I can continue working on `rr/sequencer`.
> 1. I can post the `rr/revert-cherry-pick` to the list, hoping that it
> will make it to `master` without more disruptions.  I can rebase
> `rr/sequencer` on this new series and continue working.  For your
> reference, this [1] is what I'll be posting to the list if we pick
> this option.

Merging

    https://github.com/artagnon/git.git rr/revert-cherry-pick ;# 1b7c2632

and merging rr/revert-cherry-pick currently in 'pu' (6a156fd) to 'master'
results in identical trees, so I think these amount to the same thing.

A few comments and thoughts, all minor.

 * On "revert: simplify getting commit subject in format_todo()"

   The old code used to use get_message() on cur->item, checked its return
   value (if cur->item is not parsed, or has already been used and its buffer
   cleared, cur->item->buffer would be NULL and get_message() returns -1) and
   issued an error. The new code uses find_commit_subject without first
   checking if cur->item->buffer is NULL.

   Does this mean the old code was overly defensive, or is the new code too
   lax?

   I understand that parse_insn_line() uses lookup_commit_reference() which
   calls parse_object() on the object name (and if it is a tag, deref_tag()
   will parse the tagged object until we see something that is not a tag), so
   we know cur->item is parsed before we see it, so I suspect you merely were
   being overly defensive, but I may be missing something.

 * On "revert: make commit subjects in insn sheet optional"

   After finding the verb and advancing the pointer "bol" at the beginning of
   the object name, end_of_object_name variable points at the first SP or LF
   using strcspn(bol, " \n"), but I wonder why we are not grabbing a run of
   hexdigits instead, i.e. strspn(bol, "0123456789abcdef"). Is this so that
   we can allow something like "pick rr/revert-cherry-pick~3"?

   I also wonder if this should be (sorry for pcre) "(pick|revert)\s+(\S+)\s"
   instead, i.e. allow users with fat fingers to use one or more SP or even HT
   to separate the verb and the operand.

   The last test you added to 3510 in this patch runs test_line_count
   unconditionally, by the way.

 * On "revert: allow mixed pick and revert instructions"

   Reporting what we did not understand from parse_insn_line() is a good
   idea, but I think the line number should be reported at the beginning
   of the same line.

I'd say that I'd prefer queuing re-rolled patches posted on the list; I'll
discard the rr/revert-cherry-pick (6a156fd) from my tree.

Thanks.

Re: What's cooking in git.git (Dec 2011, #02; Mon, 5)

[Ramkumar Ramachandra]

Hi Junio,

Junio C Hamano wrote:
> A few comments and thoughts, all minor.
>
>  * On "revert: simplify getting commit subject in format_todo()"
>
>   The old code used to use get_message() on cur->item, checked its return
>   value (if cur->item is not parsed, or has already been used and its buffer
>   cleared, cur->item->buffer would be NULL and get_message() returns -1) and
>   issued an error. The new code uses find_commit_subject without first
>   checking if cur->item->buffer is NULL.
>
>   Does this mean the old code was overly defensive, or is the new code too
>   lax?
>
>   I understand that parse_insn_line() uses lookup_commit_reference() which
>   calls parse_object() on the object name (and if it is a tag, deref_tag()
>   will parse the tagged object until we see something that is not a tag), so
>   we know cur->item is parsed before we see it, so I suspect you merely were
>   being overly defensive, but I may be missing something.

Right.  Actually, I was being overly defensive because I was being
lazy about having to deal the an empty-commit-subject case in the
parser.  With "revert: make commit subjects in insn sheet optional",
that's no more a concern- I'll reorder these two patches, and explain
this detail in the commit message in the re-roll.

>  * On "revert: make commit subjects in insn sheet optional"
>
>   After finding the verb and advancing the pointer "bol" at the beginning of
>   the object name, end_of_object_name variable points at the first SP or LF
>   using strcspn(bol, " \n"), but I wonder why we are not grabbing a run of
>   hexdigits instead, i.e. strspn(bol, "0123456789abcdef"). Is this so that
>   we can allow something like "pick rr/revert-cherry-pick~3"?

Yes, it is exactly for that reason :)

>   I also wonder if this should be (sorry for pcre) "(pick|revert)\s+(\S+)\s"
>   instead, i.e. allow users with fat fingers to use one or more SP or even HT
>   to separate the verb and the operand.

Hm, I'm not too enthusiastic about this change, because we don't
advertise hand-editing the instruction sheet yet: it's just some extra
parsing cruft along with guardian buffer-overflow code that buys us no
immediate benefits.

>   The last test you added to 3510 in this patch runs test_line_count
>   unconditionally, by the way.

Good catch.  Missing the chaining '&&' seems to be quite a common
error: I vaguely remember seeing a patch to detect this sometime ago.
Jonathan?

>  * On "revert: allow mixed pick and revert instructions"
>
>   Reporting what we did not understand from parse_insn_line() is a good
>   idea, but I think the line number should be reported at the beginning
>   of the same line.

Makes sense.  Do you like this?

diff --git a/builtin/revert.c b/builtin/revert.c
index e447449..cfa770e 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -715,7 +715,8 @@ static int format_todo(struct strbuf *buf, struct
replay_insn_list *todo_list)
 	return 0;
 }

-static int parse_insn_line(char *bol, char *eol, struct replay_insn_list *item)
+static int parse_insn_line(char *bol, char *eol,
+			struct replay_insn_list *item, int lineno)
 {
 	unsigned char commit_sha1[20];
 	char *end_of_object_name;
@@ -731,7 +732,8 @@ static int parse_insn_line(char *bol, char *eol,
struct replay_insn_list *item)
 		size_t len = strchrnul(bol, '\n') - bol;
 		if (len > 255)
 			len = 255;
-		return error(_("Unrecognized action: %.*s"), (int)len, bol);
+		return error(_("Line %d: Unrecognized action: %.*s"),
+			lineno, (int)len, bol);
 	}

 	end_of_object_name = bol + strcspn(bol, " \n");
@@ -741,11 +743,11 @@ static int parse_insn_line(char *bol, char *eol,
struct replay_insn_list *item)
 	*end_of_object_name = saved;

 	if (status < 0)
-		return error(_("Malformed object name: %s"), bol);
+		return error(_("Line %d: Malformed object name: %s"), lineno, bol);

 	item->operand = lookup_commit_reference(commit_sha1);
 	if (!item->operand)
-		return error(_("Not a valid commit: %s"), bol);
+		return error(_("Line %d: Not a valid commit: %s"), lineno, bol);

 	item->next = NULL;
 	return 0;
@@ -760,8 +762,8 @@ static int parse_insn_buffer(char *buf, struct
replay_insn_list **todo_list)

 	for (i = 1; *p; i++) {
 		char *eol = strchrnul(p, '\n');
-		if (parse_insn_line(p, eol, &item) < 0)
-			return error(_("on line %d."), i);
+		if (parse_insn_line(p, eol, &item, lineno) < 0)
+			return -1;
 		next = replay_insn_list_append(item.action, item.operand, next);
 		p = *eol ? eol + 1 : eol;
 	}
--

> I'd say that I'd prefer queuing re-rolled patches posted on the list; I'll
> discard the rr/revert-cherry-pick (6a156fd) from my tree.

Thanks for letting me know -- will post shortly.

-- Ram

&&-chaining tester

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> Missing the chaining '&&' seems to be quite a common
> error: I vaguely remember seeing a patch to detect this sometime ago.
> Jonathan?

I guess you mean [1].

If you want to deploy it, you have three choices:

 - maintain a list of tests known to have broken &&-chaining, and
   set GIT_SKIP_TESTS to avoid them

 - fix all cases of broken &&-chaining.  For extra bonus points,
   send out those patches, respond to reviews, and gently usher
   them into Junio's tree

 - modify the &&-chaining tester to output a list of tests encountered
   with broken &&-chaining instead of halting on the first one.

Some day, I'd like to (help) carry out option (b), but please don't
hold your breath.

[1] http://thread.gmane.org/gmane.comp.version-control.git/157903/focus=158265

Re: &&-chaining tester

[Ramkumar Ramachandra]

Hi,

Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
>
>> Missing the chaining '&&' seems to be quite a common
>> error: I vaguely remember seeing a patch to detect this sometime ago.
>> Jonathan?
>
> [...]
>  - fix all cases of broken &&-chaining.  For extra bonus points,
>   send out those patches, respond to reviews, and gently usher
>   them into Junio's tree


Hm, involves a huge amount of janitorial work.  Anyway, thanks for
pointing me in the right direction- here's a small start.

[What I used; for reference]
--
 t/test-lib.sh |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index bdd9513..3bf75ac 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -472,7 +472,17 @@ test_eval_ () {
 	eval >&3 2>&4 "$*"
 }
 
+check_command_chaining_ () {
+	eval >&3 2>&4 "(exit 189) && $1"
+	eval_chain_ret=$?
+	if test "$eval_chain_ret" != 189
+	then
+		echo '[CHAIN_BUG] in the test script: missing "&&" in test commands'
+	fi
+}
+
 test_run_ () {
+	check_command_chaining_ "$1"
 	test_cleanup=:
 	expecting_failure=$2
 	test_eval_ "$1"
-- 

Thanks.

-- Ram

Ramkumar Ramachandra (15):
  t1013 (loose-object-format): fix && chaining
  t1300 (repo-config): fix && chaining
  t1412 (reflog-loop): fix && chaining
  t1007 (hash-object): fix && chaining
  t1510 (repo-setup): fix && chaining
  t1511 (rev-parse-caret): fix && chaining
  t1510 (worktree): fix && chaining
  t3200 (branch): fix && chaining
  t3418 (rebase-continue): fix && chaining
  t3400 (rebase): fix && chaining
  t3310 (notes-merge-manual-resolve): fix && chaining
  t3419 (rebase-patch-id): fix && chaining
  t3030 (merge-recursive): use test_expect_code
  t1006 (cat-file): use test_cmp
  t3040 (subprojects-basic): modernize style

 t/t1006-cat-file.sh                   |   53 +++---------
 t/t1007-hash-object.sh                |   16 ++--
 t/t1013-loose-object-format.sh        |    2 +-
 t/t1300-repo-config.sh                |    2 +-
 t/t1412-reflog-loop.sh                |    2 +-
 t/t1501-worktree.sh                   |    6 +-
 t/t1510-repo-setup.sh                 |    4 +-
 t/t1511-rev-parse-caret.sh            |    2 +-
 t/t3030-merge-recursive.sh            |   72 ++---------------
 t/t3040-subprojects-basic.sh          |  144 ++++++++++++++++----------------
 t/t3200-branch.sh                     |    4 +-
 t/t3310-notes-merge-manual-resolve.sh |   10 +-
 t/t3400-rebase.sh                     |    4 +-
 t/t3418-rebase-continue.sh            |    4 +-
 t/t3419-rebase-patch-id.sh            |    2 +-
 15 files changed, 119 insertions(+), 208 deletions(-)

-- 
1.7.7.3

[PATCH 01/15] t1013 (loose-object-format): fix && chaining

[Ramkumar Ramachandra]

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t1013-loose-object-format.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/t/t1013-loose-object-format.sh b/t/t1013-loose-object-format.sh
index 0a9cedd..fbf5f2f 100755
--- a/t/t1013-loose-object-format.sh
+++ b/t/t1013-loose-object-format.sh
@@ -34,7 +34,7 @@ assert_blob_equals() {
 }
 
 test_expect_success setup '
-	cp -R "$TEST_DIRECTORY/t1013/objects" .git/
+	cp -R "$TEST_DIRECTORY/t1013/objects" .git/ &&
 	git --version
 '
 
-- 
1.7.7.3

[PATCH 02/15] t1300 (repo-config): fix && chaining

[Ramkumar Ramachandra]

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t1300-repo-config.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 51caff0..0690e0e 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -38,7 +38,7 @@ cat > expect << EOF
 	WhatEver = Second
 EOF
 test_expect_success 'similar section' '
-	git config Cores.WhatEver Second
+	git config Cores.WhatEver Second &&
 	test_cmp expect .git/config
 '
 
-- 
1.7.7.3

[PATCH 03/15] t1412 (reflog-loop): fix && chaining

[Ramkumar Ramachandra]

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t1412-reflog-loop.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/t/t1412-reflog-loop.sh b/t/t1412-reflog-loop.sh
index 647d888..3acd895 100755
--- a/t/t1412-reflog-loop.sh
+++ b/t/t1412-reflog-loop.sh
@@ -20,7 +20,7 @@ test_expect_success 'setup reflog with alternating commits' '
 '
 
 test_expect_success 'reflog shows all entries' '
-	cat >expect <<-\EOF
+	cat >expect <<-\EOF &&
 		topic@{0} reset: moving to two
 		topic@{1} reset: moving to one
 		topic@{2} reset: moving to two
-- 
1.7.7.3

[PATCH 04/15] t1007 (hash-object): fix && chaining

[Ramkumar Ramachandra]

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t1007-hash-object.sh |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/t/t1007-hash-object.sh b/t/t1007-hash-object.sh
index 6d52b82..316c60a 100755
--- a/t/t1007-hash-object.sh
+++ b/t/t1007-hash-object.sh
@@ -154,13 +154,13 @@ test_expect_success 'check that --no-filters option works with --stdin-paths' '
 pop_repo
 
 for args in "-w --stdin" "--stdin -w"; do
-	push_repo
+	push_repo &&
 
 	test_expect_success "hash from stdin and write to database ($args)" '
 		test $example_sha1 = $(git hash-object $args < example)
-	'
+	' &&
 
-	test_blob_exists $example_sha1
+	test_blob_exists $example_sha1 &&
 
 	pop_repo
 done
@@ -176,20 +176,20 @@ test_expect_success "hash two files with names on stdin" '
 '
 
 for args in "-w --stdin-paths" "--stdin-paths -w"; do
-	push_repo
+	push_repo &&
 
 	test_expect_success "hash two files with names on stdin and write to database ($args)" '
 		test "$sha1s" = "$(echo_without_newline "$filenames" | git hash-object $args)"
-	'
+	' &&
 
-	test_blob_exists $hello_sha1
-	test_blob_exists $example_sha1
+	test_blob_exists $hello_sha1 &&
+	test_blob_exists $example_sha1 &&
 
 	pop_repo
 done
 
 test_expect_success 'corrupt tree' '
-	echo abc >malformed-tree
+	echo abc >malformed-tree &&
 	test_must_fail git hash-object -t tree malformed-tree
 '
 
-- 
1.7.7.3

Re: [PATCH 04/15] t1007 (hash-object): fix && chaining

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>

I think there's room for at least one line of description about why
one would want to do this.

IMHO if the patches are only being sent to me, Junio, and the
mailing list (and not cc-ed to different people), then there's no
reason to split them up when they have the same topic.

Aside from that, patches 1-3 look good.  Now for this one:

[...]
> --- a/t/t1007-hash-object.sh
> +++ b/t/t1007-hash-object.sh
> @@ -154,13 +154,13 @@ test_expect_success 'check that --no-filters option works with --stdin-paths' '
>  pop_repo
>  
>  for args in "-w --stdin" "--stdin -w"; do
> -	push_repo
> +	push_repo &&
>  
>  	test_expect_success "hash from stdin and write to database ($args)" '
>  		test $example_sha1 = $(git hash-object $args < example)
> -	'
> +	' &&

I don't see how this would have any effect.  Is it intended?

Re: [PATCH 04/15] t1007 (hash-object): fix && chaining

[Ramkumar Ramachandra]

Hi Jonathan,

Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
> [...]
>
> IMHO if the patches are only being sent to me, Junio, and the
> mailing list (and not cc-ed to different people), then there's no
> reason to split them up when they have the same topic.

Right.  I'll re-roll with similar patches squashed together -- it's
easier to review and fixup in this form.

>> --- a/t/t1007-hash-object.sh
>> +++ b/t/t1007-hash-object.sh
>> @@ -154,13 +154,13 @@ test_expect_success 'check that --no-filters option works with --stdin-paths' '
>>  pop_repo
>>
>>  for args in "-w --stdin" "--stdin -w"; do
>> -     push_repo
>> +     push_repo &&
>>
>>       test_expect_success "hash from stdin and write to database ($args)" '
>>               test $example_sha1 = $(git hash-object $args < example)
>> -     '
>> +     ' &&
>
> I don't see how this would have any effect.  Is it intended?

Oops, looks like I was in a bit of a rush.  Good catch.

-- Ram

[PATCH 05/15] t1510 (repo-setup): fix && chaining

[Ramkumar Ramachandra]

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t1510-repo-setup.sh |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t1510-repo-setup.sh b/t/t1510-repo-setup.sh
index ec50a9a..80aedfc 100755
--- a/t/t1510-repo-setup.sh
+++ b/t/t1510-repo-setup.sh
@@ -603,7 +603,7 @@ test_expect_success '#22a: core.worktree = GIT_DIR = .git dir' '
 	# like case #6.
 
 	setup_repo 22a "$here/22a/.git" "" unset &&
-	setup_repo 22ab . "" unset
+	setup_repo 22ab . "" unset &&
 	mkdir -p 22a/.git/sub 22a/sub &&
 	mkdir -p 22ab/.git/sub 22ab/sub &&
 	try_case 22a/.git unset . \
@@ -742,7 +742,7 @@ test_expect_success '#28: core.worktree and core.bare conflict (gitfile case)' '
 # Case #29: GIT_WORK_TREE(+core.worktree) overrides core.bare (gitfile case).
 test_expect_success '#29: setup' '
 	setup_repo 29 non-existent gitfile true &&
-	mkdir -p 29/sub/sub 29/wt/sub
+	mkdir -p 29/sub/sub 29/wt/sub &&
 	(
 		cd 29 &&
 		GIT_WORK_TREE="$here/29" &&
-- 
1.7.7.3

[PATCH 06/15] t1511 (rev-parse-caret): fix && chaining

[Ramkumar Ramachandra]

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t1511-rev-parse-caret.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/t/t1511-rev-parse-caret.sh b/t/t1511-rev-parse-caret.sh
index e043cb7..eaefc77 100755
--- a/t/t1511-rev-parse-caret.sh
+++ b/t/t1511-rev-parse-caret.sh
@@ -6,7 +6,7 @@ test_description='tests for ref^{stuff}'
 
 test_expect_success 'setup' '
 	echo blob >a-blob &&
-	git tag -a -m blob blob-tag `git hash-object -w a-blob`
+	git tag -a -m blob blob-tag `git hash-object -w a-blob` &&
 	mkdir a-tree &&
 	echo moreblobs >a-tree/another-blob &&
 	git add . &&
-- 
1.7.7.3

[PATCH 07/15] t1510 (worktree): fix && chaining

[Ramkumar Ramachandra]

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t1501-worktree.sh |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t1501-worktree.sh b/t/t1501-worktree.sh
index 6384983..d9761bd 100755
--- a/t/t1501-worktree.sh
+++ b/t/t1501-worktree.sh
@@ -48,7 +48,7 @@ test_expect_success 'setup: helper for testing rev-parse' '
 '
 
 test_expect_success 'setup: core.worktree = relative path' '
-	unset GIT_WORK_TREE;
+	unset GIT_WORK_TREE &&
 	GIT_DIR=repo.git &&
 	GIT_CONFIG="$(pwd)"/$GIT_DIR/config &&
 	export GIT_DIR GIT_CONFIG &&
@@ -89,7 +89,7 @@ test_expect_success 'subdir of work tree' '
 '
 
 test_expect_success 'setup: core.worktree = absolute path' '
-	unset GIT_WORK_TREE;
+	unset GIT_WORK_TREE &&
 	GIT_DIR=$(pwd)/repo.git &&
 	GIT_CONFIG=$GIT_DIR/config &&
 	export GIT_DIR GIT_CONFIG &&
@@ -334,7 +334,7 @@ test_expect_success 'absolute pathspec should fail gracefully' '
 '
 
 test_expect_success 'make_relative_path handles double slashes in GIT_DIR' '
-	>dummy_file
+	>dummy_file &&
 	echo git --git-dir="$(pwd)//repo.git" --work-tree="$(pwd)" add dummy_file &&
 	git --git-dir="$(pwd)//repo.git" --work-tree="$(pwd)" add dummy_file
 '
-- 
1.7.7.3

Re: [PATCH 07/15] t1510 (worktree): fix && chaining

[Jonathan Nieder]

Patches 5 and 6 look safe, though I haven't tested them.

Ramkumar Ramachandra wrote:

> --- a/t/t1501-worktree.sh
> +++ b/t/t1501-worktree.sh
> @@ -48,7 +48,7 @@ test_expect_success 'setup: helper for testing rev-parse' '
>  '
>  
>  test_expect_success 'setup: core.worktree = relative path' '
> -	unset GIT_WORK_TREE;
> +	unset GIT_WORK_TREE &&

On some shells, like /usr/xpg4/bin/sh on Solaris, unset returns nonzero
status when the variable passed was already unset.  Will this work on
such platforms, or does it need to be changed to use sane_unset?

Re: [PATCH 07/15] t1510 (worktree): fix && chaining

[Ramkumar Ramachandra]

Hi Jonathan,

Jonathan Nieder wrote:
> [...]
> On some shells, like /usr/xpg4/bin/sh on Solaris, unset returns nonzero
> status when the variable passed was already unset.  Will this work on
> such platforms, or does it need to be changed to use sane_unset?

Interesting.  Thanks for catching this.

-- Ram

[PATCH 08/15] t3200 (branch): fix && chaining

[Ramkumar Ramachandra]

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t3200-branch.sh |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index bc73c20..7877290 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -22,7 +22,7 @@ test_expect_success \
 
 test_expect_success \
     'git branch --help should not have created a bogus branch' '
-     git branch --help </dev/null >/dev/null 2>/dev/null;
+     git branch --help </dev/null >/dev/null 2>/dev/null &&
      test_path_is_missing .git/refs/heads/--help
 '
 
@@ -88,7 +88,7 @@ test_expect_success \
 test_expect_success \
     'git branch -m n/n n should work' \
        'git branch -l n/n &&
-        git branch -m n/n n
+        git branch -m n/n n &&
 	test_path_is_file .git/logs/refs/heads/n'
 
 test_expect_success 'git branch -m o/o o should fail when o/p exists' '
-- 
1.7.7.3

Re: [PATCH 08/15] t3200 (branch): fix && chaining

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> --- a/t/t3200-branch.sh
> +++ b/t/t3200-branch.sh
> @@ -22,7 +22,7 @@ test_expect_success \
>  
>  test_expect_success \
>      'git branch --help should not have created a bogus branch' '
> -     git branch --help </dev/null >/dev/null 2>/dev/null;
> +     git branch --help </dev/null >/dev/null 2>/dev/null &&
>       test_path_is_missing .git/refs/heads/--help

Won't this break when running tests for the first time, before the git
manpages are installed?

>  '
>  
> @@ -88,7 +88,7 @@ test_expect_success \
>  test_expect_success \
>      'git branch -m n/n n should work' \
>         'git branch -l n/n &&
> -        git branch -m n/n n
> +        git branch -m n/n n &&
>  	test_path_is_file .git/logs/refs/heads/n'

Good catch.

Re: [PATCH 08/15] t3200 (branch): fix && chaining

[Ramkumar Ramachandra]

Hi Jonathan,

Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
>
>> --- a/t/t3200-branch.sh
>> +++ b/t/t3200-branch.sh
>> @@ -22,7 +22,7 @@ test_expect_success \
>>
>>  test_expect_success \
>>      'git branch --help should not have created a bogus branch' '
>> -     git branch --help </dev/null >/dev/null 2>/dev/null;
>> +     git branch --help </dev/null >/dev/null 2>/dev/null &&
>>       test_path_is_missing .git/refs/heads/--help
>
> Won't this break when running tests for the first time, before the git
> manpages are installed?

Used test_might_fail to guard it this time.  Thanks.

-- Ram

[PATCH 09/15] t3418 (rebase-continue): fix && chaining

[Ramkumar Ramachandra]

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t3418-rebase-continue.sh |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
index 1e855cd..2680375 100755
--- a/t/t3418-rebase-continue.sh
+++ b/t/t3418-rebase-continue.sh
@@ -51,7 +51,7 @@ test_expect_success 'rebase --continue remembers merge strategy and options' '
 	test_commit "commit-new-file-F3-on-topic-branch" F3 32 &&
 	test_when_finished "rm -fr test-bin funny.was.run" &&
 	mkdir test-bin &&
-	cat >test-bin/git-merge-funny <<-EOF
+	cat >test-bin/git-merge-funny <<-EOF &&
 	#!$SHELL_PATH
 	case "\$1" in --opt) ;; *) exit 2 ;; esac
 	shift &&
@@ -77,7 +77,7 @@ test_expect_success 'rebase --continue remembers merge strategy and options' '
 test_expect_success 'rebase --continue remembers --rerere-autoupdate' '
 	rm -fr .git/rebase-* &&
 	git reset --hard commit-new-file-F3-on-topic-branch &&
-	git checkout master
+	git checkout master &&
 	test_commit "commit-new-file-F3" F3 3 &&
 	git config rerere.enabled true &&
 	test_must_fail git rebase -m master topic &&
-- 
1.7.7.3

[PATCH 10/15] t3400 (rebase): fix && chaining

[Ramkumar Ramachandra]

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t3400-rebase.sh |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 6eaecec..c355533 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -172,8 +172,8 @@ test_expect_success 'fail when upstream arg is missing and not configured' '
 
 test_expect_success 'default to @{upstream} when upstream arg is missing' '
 	git checkout -b default topic &&
-	git config branch.default.remote .
-	git config branch.default.merge refs/heads/master
+	git config branch.default.remote . &&
+	git config branch.default.merge refs/heads/master &&
 	git rebase &&
 	test "$(git rev-parse default~1)" = "$(git rev-parse master)"
 '
-- 
1.7.7.3

[PATCH 11/15] t3310 (notes-merge-manual-resolve): fix && chaining

[Ramkumar Ramachandra]

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t3310-notes-merge-manual-resolve.sh |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh
index 4ec4d11..4367197 100755
--- a/t/t3310-notes-merge-manual-resolve.sh
+++ b/t/t3310-notes-merge-manual-resolve.sh
@@ -389,7 +389,7 @@ test_expect_success 'abort notes merge' '
 	test_must_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&
 	test_cmp /dev/null output &&
 	# m has not moved (still == y)
-	test "$(git rev-parse refs/notes/m)" = "$(cat pre_merge_y)"
+	test "$(git rev-parse refs/notes/m)" = "$(cat pre_merge_y)" &&
 	# Verify that other notes refs has not changed (w, x, y and z)
 	verify_notes w &&
 	verify_notes x &&
@@ -525,9 +525,9 @@ EOF
 	test -f .git/NOTES_MERGE_WORKTREE/$commit_sha3 &&
 	test -f .git/NOTES_MERGE_WORKTREE/$commit_sha4 &&
 	# Refs are unchanged
-	test "$(git rev-parse refs/notes/m)" = "$(git rev-parse refs/notes/w)"
-	test "$(git rev-parse refs/notes/y)" = "$(git rev-parse NOTES_MERGE_PARTIAL^1)"
-	test "$(git rev-parse refs/notes/m)" != "$(git rev-parse NOTES_MERGE_PARTIAL^1)"
+	test "$(git rev-parse refs/notes/m)" = "$(git rev-parse refs/notes/w)" &&
+	test "$(git rev-parse refs/notes/y)" = "$(git rev-parse NOTES_MERGE_PARTIAL^1)" &&
+	test "$(git rev-parse refs/notes/m)" != "$(git rev-parse NOTES_MERGE_PARTIAL^1)" &&
 	# Mention refs/notes/m, and its current and expected value in output
 	grep -q "refs/notes/m" output &&
 	grep -q "$(git rev-parse refs/notes/m)" output &&
@@ -545,7 +545,7 @@ test_expect_success 'resolve situation by aborting the notes merge' '
 	test_must_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&
 	test_cmp /dev/null output &&
 	# m has not moved (still == w)
-	test "$(git rev-parse refs/notes/m)" = "$(git rev-parse refs/notes/w)"
+	test "$(git rev-parse refs/notes/m)" = "$(git rev-parse refs/notes/w)" &&
 	# Verify that other notes refs has not changed (w, x, y and z)
 	verify_notes w &&
 	verify_notes x &&
-- 
1.7.7.3

[PATCH 12/15] t3419 (rebase-patch-id): fix && chaining

[Ramkumar Ramachandra]

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t3419-rebase-patch-id.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/t/t3419-rebase-patch-id.sh b/t/t3419-rebase-patch-id.sh
index bd8efaf..e70ac10 100755
--- a/t/t3419-rebase-patch-id.sh
+++ b/t/t3419-rebase-patch-id.sh
@@ -39,7 +39,7 @@ run()
 }
 
 test_expect_success 'setup' '
-	git commit --allow-empty -m initial
+	git commit --allow-empty -m initial &&
 	git tag root
 '
 
-- 
1.7.7.3

[PATCH 13/15] t3030 (merge-recursive): use test_expect_code

[Ramkumar Ramachandra]

Use test_expect_code in preference to repeatedly checking exit codes
by hand.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t3030-merge-recursive.sh |   72 ++++----------------------------------------
 1 files changed, 6 insertions(+), 66 deletions(-)

diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh
index 55ef189..a5e3da7 100755
--- a/t/t3030-merge-recursive.sh
+++ b/t/t3030-merge-recursive.sh
@@ -285,17 +285,7 @@ test_expect_success 'merge-recursive simple' '
 	rm -fr [abcd] &&
 	git checkout -f "$c2" &&
 
-	git merge-recursive "$c0" -- "$c2" "$c1"
-	status=$?
-	case "$status" in
-	1)
-		: happy
-		;;
-	*)
-		echo >&2 "why status $status!!!"
-		false
-		;;
-	esac
+	test_expect_code 1 git merge-recursive "$c0" -- "$c2" "$c1"
 '
 
 test_expect_success 'merge-recursive result' '
@@ -334,17 +324,7 @@ test_expect_success 'merge-recursive remove conflict' '
 	rm -fr [abcd] &&
 	git checkout -f "$c1" &&
 
-	git merge-recursive "$c0" -- "$c1" "$c5"
-	status=$?
-	case "$status" in
-	1)
-		: happy
-		;;
-	*)
-		echo >&2 "why status $status!!!"
-		false
-		;;
-	esac
+	test_expect_code 1 git merge-recursive "$c0" -- "$c1" "$c5"
 '
 
 test_expect_success 'merge-recursive remove conflict' '
@@ -388,17 +368,7 @@ test_expect_success 'merge-recursive d/f conflict' '
 	git reset --hard &&
 	git checkout -f "$c1" &&
 
-	git merge-recursive "$c0" -- "$c1" "$c4"
-	status=$?
-	case "$status" in
-	1)
-		: happy
-		;;
-	*)
-		echo >&2 "why status $status!!!"
-		false
-		;;
-	esac
+	test_expect_code 1 git merge-recursive "$c0" -- "$c1" "$c4"
 '
 
 test_expect_success 'merge-recursive d/f conflict result' '
@@ -422,17 +392,7 @@ test_expect_success 'merge-recursive d/f conflict the other way' '
 	git reset --hard &&
 	git checkout -f "$c4" &&
 
-	git merge-recursive "$c0" -- "$c4" "$c1"
-	status=$?
-	case "$status" in
-	1)
-		: happy
-		;;
-	*)
-		echo >&2 "why status $status!!!"
-		false
-		;;
-	esac
+	test_expect_code 1 git merge-recursive "$c0" -- "$c4" "$c1"
 '
 
 test_expect_success 'merge-recursive d/f conflict result the other way' '
@@ -456,17 +416,7 @@ test_expect_success 'merge-recursive d/f conflict' '
 	git reset --hard &&
 	git checkout -f "$c1" &&
 
-	git merge-recursive "$c0" -- "$c1" "$c6"
-	status=$?
-	case "$status" in
-	1)
-		: happy
-		;;
-	*)
-		echo >&2 "why status $status!!!"
-		false
-		;;
-	esac
+	test_expect_code 1 git merge-recursive "$c0" -- "$c1" "$c6"
 '
 
 test_expect_success 'merge-recursive d/f conflict result' '
@@ -490,17 +440,7 @@ test_expect_success 'merge-recursive d/f conflict' '
 	git reset --hard &&
 	git checkout -f "$c6" &&
 
-	git merge-recursive "$c0" -- "$c6" "$c1"
-	status=$?
-	case "$status" in
-	1)
-		: happy
-		;;
-	*)
-		echo >&2 "why status $status!!!"
-		false
-		;;
-	esac
+	test_expect_code 1 git merge-recursive "$c0" -- "$c6" "$c1"
 '
 
 test_expect_success 'merge-recursive d/f conflict result' '
-- 
1.7.7.3

Re: [PATCH 13/15] t3030 (merge-recursive): use test_expect_code

[Jonathan Nieder]

Patches 9-12 look good.

Ramkumar Ramachandra wrote:

> Use test_expect_code in preference to repeatedly checking exit codes
> by hand.
[...]
> +++ b/t/t3030-merge-recursive.sh
> @@ -285,17 +285,7 @@ test_expect_success 'merge-recursive simple' '
>  	rm -fr [abcd] &&
>  	git checkout -f "$c2" &&
>  
> -	git merge-recursive "$c0" -- "$c2" "$c1"
> -	status=$?
> -	case "$status" in
> -	1)
> -		: happy
> -		;;
> -	*)
> -		echo >&2 "why status $status!!!"
> -		false
> -		;;
> -	esac
> +	test_expect_code 1 git merge-recursive "$c0" -- "$c2" "$c1"

Yeah, this style is kind of repetitive.  Worse, if "git checkout"
fails with status 1, it would yield a false success.  Thanks for
fixing it.

[PATCH 14/15] t1006 (cat-file): use test_cmp

[Ramkumar Ramachandra]

Use test_cmp in preference to repeatedly comparing command outputs by
hand.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t1006-cat-file.sh |   53 +++++++++++---------------------------------------
 1 files changed, 12 insertions(+), 41 deletions(-)

diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
index d8b7f2f..af6cacc 100755
--- a/t/t1006-cat-file.sh
+++ b/t/t1006-cat-file.sh
@@ -45,57 +45,28 @@ $content"
 
     test -z "$content" ||
     test_expect_success "Content of $type is correct" '
-	expect="$(maybe_remove_timestamp "$content" $no_ts)"
-	actual="$(maybe_remove_timestamp "$(git cat-file $type $sha1)" $no_ts)"
-
-        if test "z$expect" = "z$actual"
-	then
-		: happy
-	else
-		echo "Oops: expected $expect"
-		echo "but got $actual"
-		false
-        fi
+	maybe_remove_timestamp "$content" $no_ts >expect &&
+	maybe_remove_timestamp "$(git cat-file $type $sha1)" $no_ts >actual &&
+	test_cmp expect actual
     '
 
     test_expect_success "Pretty content of $type is correct" '
-	expect="$(maybe_remove_timestamp "$pretty_content" $no_ts)"
-	actual="$(maybe_remove_timestamp "$(git cat-file -p $sha1)" $no_ts)"
-        if test "z$expect" = "z$actual"
-	then
-		: happy
-	else
-		echo "Oops: expected $expect"
-		echo "but got $actual"
-		false
-        fi
+	maybe_remove_timestamp "$pretty_content" $no_ts >expect &&
+	maybe_remove_timestamp "$(git cat-file -p $sha1)" $no_ts >actual &&
+	test_cmp expect actual
     '
 
     test -z "$content" ||
     test_expect_success "--batch output of $type is correct" '
-	expect="$(maybe_remove_timestamp "$batch_output" $no_ts)"
-	actual="$(maybe_remove_timestamp "$(echo $sha1 | git cat-file --batch)" $no_ts)"
-        if test "z$expect" = "z$actual"
-	then
-		: happy
-	else
-		echo "Oops: expected $expect"
-		echo "but got $actual"
-		false
-        fi
+	maybe_remove_timestamp "$batch_output" $no_ts >expect &&
+	maybe_remove_timestamp "$(echo $sha1 | git cat-file --batch)" $no_ts >actual &&
+	test_cmp expect actual
     '
 
     test_expect_success "--batch-check output of $type is correct" '
-	expect="$sha1 $type $size"
-	actual="$(echo_without_newline $sha1 | git cat-file --batch-check)"
-        if test "z$expect" = "z$actual"
-	then
-		: happy
-	else
-		echo "Oops: expected $expect"
-		echo "but got $actual"
-		false
-        fi
+	echo "$sha1 $type $size" >expect &&
+	echo_without_newline $sha1 | git cat-file --batch-check >actual &&
+	test_cmp expect actual
     '
 }
 
-- 
1.7.7.3

Re: [PATCH 14/15] t1006 (cat-file): use test_cmp

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> Use test_cmp in preference to repeatedly comparing command outputs by
> hand.
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  t/t1006-cat-file.sh |   53 +++++++++++---------------------------------------
>  1 files changed, 12 insertions(+), 41 deletions(-)

I guess test_cmp didn't exist yet when this code was written, though
that doesn't explain why "diff" or "cmp" was not used.

Doesn't this miss some other instances of the

	test $expect = $actual

idiom in the same test script?  What distinguishes the cases that you
fixed from the ones skipped, and is it worth an inconsistent style to
convert some without the others?

[PATCH 15/15] t3040 (subprojects-basic): modernize style

[Ramkumar Ramachandra]

Put the opening quote starting each test on the same line as the
test_expect_* invocation.  While at it:

- Indent the file with tabs, not spaces.

- Guard commands that prepare test input for individual tests in the
  same test_expect_success, so that their scope is clearer and errors
  at that stage can be caught.

- Use <<\-EOF in preference to <<EOF to save readers the trouble of
  looking for variable interpolations.

- Include "setup" in the titles of test assertions that prepare for
  later ones to make it more obvious which tests can be skipped.

- Chain commands with &&.  Breaks in a test assertion's && chain can
  potentially hide failures from earlier commands in the chain.

- Use test_expect_code() in preference to checking the exit status of
  various statements by hand.

Inspired-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t3040-subprojects-basic.sh |  144 +++++++++++++++++++++---------------------
 1 files changed, 72 insertions(+), 72 deletions(-)

diff --git a/t/t3040-subprojects-basic.sh b/t/t3040-subprojects-basic.sh
index f6973e9..0a4ff6d 100755
--- a/t/t3040-subprojects-basic.sh
+++ b/t/t3040-subprojects-basic.sh
@@ -3,81 +3,81 @@
 test_description='Basic subproject functionality'
 . ./test-lib.sh
 
-test_expect_success 'Super project creation' \
-    ': >Makefile &&
-    git add Makefile &&
-    git commit -m "Superproject created"'
-
-
-cat >expected <<EOF
-:000000 160000 00000... A	sub1
-:000000 160000 00000... A	sub2
-EOF
-test_expect_success 'create subprojects' \
-    'mkdir sub1 &&
-    ( cd sub1 && git init && : >Makefile && git add * &&
-    git commit -q -m "subproject 1" ) &&
-    mkdir sub2 &&
-    ( cd sub2 && git init && : >Makefile && git add * &&
-    git commit -q -m "subproject 2" ) &&
-    git update-index --add sub1 &&
-    git add sub2 &&
-    git commit -q -m "subprojects added" &&
-    git diff-tree --abbrev=5 HEAD^ HEAD |cut -d" " -f-3,5- >current &&
-    test_cmp expected current'
-
-git branch save HEAD
-
-test_expect_success 'check if fsck ignores the subprojects' \
-    'git fsck --full'
-
-test_expect_success 'check if commit in a subproject detected' \
-    '( cd sub1 &&
-    echo "all:" >>Makefile &&
-    echo "	true" >>Makefile &&
-    git commit -q -a -m "make all" ) && {
-        git diff-files --exit-code
-	test $? = 1
-    }'
-
-test_expect_success 'check if a changed subproject HEAD can be committed' \
-    'git commit -q -a -m "sub1 changed" && {
-	git diff-tree --exit-code HEAD^ HEAD
-	test $? = 1
-    }'
-
-test_expect_success 'check if diff-index works for subproject elements' \
-    'git diff-index --exit-code --cached save -- sub1
-    test $? = 1'
-
-test_expect_success 'check if diff-tree works for subproject elements' \
-    'git diff-tree --exit-code HEAD^ HEAD -- sub1
-    test $? = 1'
-
-test_expect_success 'check if git diff works for subproject elements' \
-    'git diff --exit-code HEAD^ HEAD
-    test $? = 1'
-
-test_expect_success 'check if clone works' \
-    'git ls-files -s >expected &&
-    git clone -l -s . cloned &&
-    ( cd cloned && git ls-files -s ) >current &&
-    test_cmp expected current'
-
-test_expect_success 'removing and adding subproject' \
-    'git update-index --force-remove -- sub2 &&
-    mv sub2 sub3 &&
-    git add sub3 &&
-    git commit -q -m "renaming a subproject" && {
-	git diff -M --name-status --exit-code HEAD^ HEAD
-	test $? = 1
-    }'
+test_expect_success 'setup: create superproject' '
+	: >Makefile &&
+	git add Makefile &&
+	git commit -m "Superproject created"
+'
+
+test_expect_success 'setup: create subprojects' '
+	mkdir sub1 &&
+	( cd sub1 && git init && : >Makefile && git add * &&
+	git commit -q -m "subproject 1" ) &&
+	mkdir sub2 &&
+	( cd sub2 && git init && : >Makefile && git add * &&
+	git commit -q -m "subproject 2" ) &&
+	git update-index --add sub1 &&
+	git add sub2 &&
+	git commit -q -m "subprojects added" &&
+	git diff-tree --abbrev=5 HEAD^ HEAD |cut -d" " -f-3,5- >current &&
+	git branch save HEAD &&
+	cat >expected <<-\EOF &&
+	:000000 160000 00000... A	sub1
+	:000000 160000 00000... A	sub2
+	EOF
+	test_cmp expected current
+'
+
+test_expect_success 'check if fsck ignores the subprojects' '
+	git fsck --full
+'
+
+test_expect_success 'check if commit in a subproject detected' '
+	( cd sub1 &&
+	echo "all:" >>Makefile &&
+	echo "	true" >>Makefile &&
+	git commit -q -a -m "make all" ) &&
+	test_expect_code 1 git diff-files --exit-code
+'
+
+test_expect_success 'check if a changed subproject HEAD can be committed' '
+	git commit -q -a -m "sub1 changed" &&
+	test_expect_code 1 git diff-tree --exit-code HEAD^ HEAD
+'
+
+test_expect_success 'check if diff-index works for subproject elements' '
+	test_expect_code 1 git diff-index --exit-code --cached save -- sub1
+'
+
+test_expect_success 'check if diff-tree works for subproject elements' '
+	test_expect_code 1 git diff-tree --exit-code HEAD^ HEAD -- sub1
+'
+
+test_expect_success 'check if git diff works for subproject elements' '
+	test_expect_code 1 git diff --exit-code HEAD^ HEAD
+'
+
+test_expect_success 'check if clone works' '
+	git ls-files -s >expected &&
+	git clone -l -s . cloned &&
+	( cd cloned && git ls-files -s ) >current &&
+	test_cmp expected current
+'
+
+test_expect_success 'removing and adding subproject' '
+	git update-index --force-remove -- sub2 &&
+	mv sub2 sub3 &&
+	git add sub3 &&
+	git commit -q -m "renaming a subproject" &&
+	test_expect_code 1 git diff -M --name-status --exit-code HEAD^ HEAD
+'
 
 # the index must contain the object name the HEAD of the
 # subproject sub1 was at the point "save"
-test_expect_success 'checkout in superproject' \
-    'git checkout save &&
-    git diff-index --exit-code --raw --cached save -- sub1'
+test_expect_success 'checkout in superproject' '
+	git checkout save &&
+	git diff-index --exit-code --raw --cached save -- sub1
+'
 
 # just interesting what happened...
 # git diff --name-status -M save master
-- 
1.7.7.3

Re: [PATCH 15/15] t3040 (subprojects-basic): modernize style

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> Put the opening quote starting each test on the same line as the
> test_expect_* invocation.  While at it:
[...]
> - Use <<\-EOF in preference to <<EOF to save readers the trouble of
>   looking for variable interpolations.

I think you mean <<-\EOF. :)

[...]
> - Chain commands with &&.  Breaks in a test assertion's && chain can
>   potentially hide failures from earlier commands in the chain.
>
> - Use test_expect_code() in preference to checking the exit status of
>   various statements by hand.

I guess these two are the motivation?

> Inspired-by: Jonathan Nieder <jrnieder@gmail.com>

Oh, dear.

> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
[...]
> --- a/t/t3040-subprojects-basic.sh
> +++ b/t/t3040-subprojects-basic.sh
> @@ -3,81 +3,81 @@
>  test_description='Basic subproject functionality'
>  . ./test-lib.sh
>  
> -test_expect_success 'Super project creation' \
> -    ': >Makefile &&
> -    git add Makefile &&
> -    git commit -m "Superproject created"'
> -
> -
> -cat >expected <<EOF
[...]

It would be easier to read if each preimage test assertion were next to
the corresponding postimage test assertion.  Does "git diff --patience"
do better?

> -test_expect_success 'Super project creation' \
> -    ': >Makefile &&
> -    git add Makefile &&
> -    git commit -m "Superproject created"'
> -
> -
> +test_expect_success 'setup: create superproject' '
> +	: >Makefile &&
> +	git add Makefile &&
> +	git commit -m "Superproject created"
> +'
> +

Ok, makes sense.

> -cat >expected <<EOF
> -:000000 160000 00000... A	sub1
> -:000000 160000 00000... A	sub2
> -EOF
> -test_expect_success 'create subprojects' \
> -    'mkdir sub1 &&
> -    ( cd sub1 && git init && : >Makefile && git add * &&
> -    git commit -q -m "subproject 1" ) &&
> +test_expect_success 'setup: create subprojects' '
> +	mkdir sub1 &&
> +	( cd sub1 && git init && : >Makefile && git add * &&
> +	git commit -q -m "subproject 1" ) &&

If cleaning up the style anyway, I would write this as

	mkdir sub1 &&
	(
		cd sub1 &&
		git init &&
		>Makefile &&
		git add Makefile &&
		git commit -m "subproject 1"
	)

Or

	mkdir sub1 &&
	(
		cd sub1 &&
		git init &&
		test_commit subproject-1 Makefile
	)

But leaving it alone like you did is probably better. ;-)

[...]
> -    git diff-tree --abbrev=5 HEAD^ HEAD |cut -d" " -f-3,5- >current &&
> -    test_cmp expected current'
> -
> -git branch save HEAD
> -
> +	git diff-tree --abbrev=5 HEAD^ HEAD |cut -d" " -f-3,5- >current &&
> +	git branch save HEAD &&
> +	cat >expected <<-\EOF &&
> +	:000000 160000 00000... A	sub1
> +	:000000 160000 00000... A	sub2
> +	EOF
> +	test_cmp expected current
> +'
> +

At first I wondered if "git branch save HEAD" is logically part of the
same test.  After all, it's just meant as a baseline for use by later
tests.

After thinking about it for a few seconds, though, that's exactly what
this test is about.  Maybe it would have been clearer if the two setup
tests were combined into one (but please don't take this advice too
seriously; I'm just musing).

> -test_expect_success 'check if fsck ignores the subprojects' \
> -    'git fsck --full'
> +test_expect_success 'check if fsck ignores the subprojects' '
> +	git fsck --full
> +'

Does this test imply that one of the subprojects is broken somehow?

> -
> -test_expect_success 'check if commit in a subproject detected' \
> -    '( cd sub1 &&
> -    echo "all:" >>Makefile &&
> -    echo "	true" >>Makefile &&
> -    git commit -q -a -m "make all" ) && {
> -        git diff-files --exit-code
> -	test $? = 1
> -    }'
> +
> +test_expect_success 'check if commit in a subproject detected' '
> +	( cd sub1 &&
> +	echo "all:" >>Makefile &&
> +	echo "	true" >>Makefile &&
> +	git commit -q -a -m "make all" ) &&
> +	test_expect_code 1 git diff-files --exit-code
> +'

Nice.  Style again: I'd be tempted to reformat as

	(
		cd sub1 &&
		echo "all:" >>Makefile &&
		...
	) &&
	test_expect_code 1 git diff-files --exit-code

to make the subshell scope a little clearer, but exercising restraint
like you did may be better.

[...]
>  
>  # the index must contain the object name the HEAD of the
>  # subproject sub1 was at the point "save"
> -test_expect_success 'checkout in superproject' \
> -    'git checkout save &&
> -    git diff-index --exit-code --raw --cached save -- sub1'
> +test_expect_success 'checkout in superproject' '
> +	git checkout save &&
> +	git diff-index --exit-code --raw --cached save -- sub1
> +'

Thanks much.  The result really is a little easier on the eyes, and
the changes look safe.

Re: [PATCH 15/15] t3040 (subprojects-basic): modernize style

[Ramkumar Ramachandra]

Hi,

Jonathan Nieder wrote:
> It would be easier to read if each preimage test assertion were next to
> the corresponding postimage test assertion.  Does "git diff --patience"
> do better?

No, unfortunately.

> If cleaning up the style anyway, I would write this as
> [...]
> But leaving it alone like you did is probably better. ;-)

Exercising restraint.  Otherwise, I'm tempted to fix every little nit
in every test I'm touching in the series :P

>> -test_expect_success 'check if fsck ignores the subprojects' \
>> -    'git fsck --full'
>> +test_expect_success 'check if fsck ignores the subprojects' '
>> +     git fsck --full
>> +'
>
> Does this test imply that one of the subprojects is broken somehow?

Hm, you're right- the test doesn't seem to make any sense.  I'll deal
with it in another patch (unrelated to this series).

Thanks.

-- Ram

Re: &&-chaining tester

[Matthieu Moy]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Hm, involves a huge amount of janitorial work.  Anyway, thanks for
> pointing me in the right direction- here's a small start.

Patches 1-14 look good to me (with Jonathan's remarks). I didn't review
patch 15.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: What's cooking in git.git (Dec 2011, #02; Mon, 5)

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

>>  * On "revert: make commit subjects in insn sheet optional"
>>
>>   After finding the verb and advancing the pointer "bol" at the beginning of
>>   the object name, end_of_object_name variable points at the first SP or LF
>>   using strcspn(bol, " \n"), but I wonder why we are not grabbing a run of
>>   hexdigits instead, i.e. strspn(bol, "0123456789abcdef"). Is this so that
>>   we can allow something like "pick rr/revert-cherry-pick~3"?
>
> Yes, it is exactly for that reason :)

That is not explained in the commit message, which in itself is a problem
that needs to be addressed, but ...

>>   I also wonder if this should be (sorry for pcre) "(pick|revert)\s+(\S+)\s"
>>   instead, i.e. allow users with fat fingers to use one or more SP or even HT
>>   to separate the verb and the operand.
>
> Hm, I'm not too enthusiastic about this change, because we don't
> advertise hand-editing the instruction sheet yet:...

... is inconsistent with the above.

If you plan to later allow editing and if you know what kind of editing
you are going to allow, it would be better to prepare the parser to accept
input with a reasonable slack, especially because you are already touching
it in this series anyway.

On the other hand, if you do not want to think about (or do not want us to
get distracted by thinking about) allowing editing in this series, which
is also a sensible stance to take, the parser should be made to accept
only what the mechanism in the series would produce and error out
otherwise, leaving the entire "the user now can edit and here are the rules"
part for a separate series that comes after this series is perfected.

>>  * On "revert: allow mixed pick and revert instructions"
>>
>>   Reporting what we did not understand from parse_insn_line() is a good
>>   idea, but I think the line number should be reported at the beginning
>>   of the same line.
>
> Makes sense.  Do you like this?

Not particularly.

> -		return error(_("Unrecognized action: %.*s"), (int)len, bol);
> +		return error(_("Line %d: Unrecognized action: %.*s"),

It may be just me, but personally I prefer to see which file of what line
had the problem, i.e. "%s:%d: Unrecognized action:%s".

But if there is not much point in reporting the filename because it always
is the same, "Unrecognized action (line %d): %s" would also be fine.

Re: What's cooking in git.git (Dec 2011, #02; Mon, 5)

[Jeff King]

On Mon, Dec 05, 2011 at 09:01:52PM -0800, Junio C Hamano wrote:

> * jk/credentials (2011-11-28) 20 commits
>  - fixup! 034c066e
>  - compat/getpass: add a /dev/tty implementation
>  - credential: use git_prompt instead of git_getpass
>  - prompt: add PROMPT_ECHO flag
>  - stub out getpass_echo function
>  - refactor git_getpass into generic prompt function
>  - move git_getpass to its own source file
>  - t: add test harness for external credential helpers
>  - credentials: add "store" helper
>  - strbuf: add strbuf_add*_urlencode
>  - 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
> 
> Expecting a reroll?

Yes, I have a re-roll ready. I was holding back to see if some of the
helper authors might comment, but got nothing. Perhaps the new version
will spur some interest.

Also, let's drop the top git_getpass bits from the topic for now (they
will not be part of my rebase). They are a separate topic that can go on
top, but I think there was some question from Erik of whether we should
simply roll our own getpass().

> * jk/upload-archive-use-start-command (2011-11-21) 1 commit
>  - upload-archive: use start_command instead of fork
> 
> What's the status of this one?

I think what you have in pu is good, but of course I didn't actually
test it on Windows. Erik?

> * 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
>  - 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 'next' after taking another look.

Thanks. I also have some followup patches to re-loosen to at least
trees reachable from refs. Do you want to leave the tightening to the
maint track, and then consider the re-loosening for master?

-Peff

Re: What's cooking in git.git (Dec 2011, #02; Mon, 5)

[Erik Faye-Lund]

On Tue, Dec 6, 2011 at 6:52 AM, Jeff King <peff@peff.net> wrote:
> On Mon, Dec 05, 2011 at 09:01:52PM -0800, Junio C Hamano wrote:
>
>> * jk/credentials (2011-11-28) 20 commits
>>  - fixup! 034c066e
>>  - compat/getpass: add a /dev/tty implementation
>>  - credential: use git_prompt instead of git_getpass
>>  - prompt: add PROMPT_ECHO flag
>>  - stub out getpass_echo function
>>  - refactor git_getpass into generic prompt function
>>  - move git_getpass to its own source file
>>  - t: add test harness for external credential helpers
>>  - credentials: add "store" helper
>>  - strbuf: add strbuf_add*_urlencode
>>  - 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
>>
>> Expecting a reroll?
>
> Yes, I have a re-roll ready. I was holding back to see if some of the
> helper authors might comment, but got nothing. Perhaps the new version
> will spur some interest.
>
> Also, let's drop the top git_getpass bits from the topic for now (they
> will not be part of my rebase). They are a separate topic that can go on
> top, but I think there was some question from Erik of whether we should
> simply roll our own getpass().
>
>> * jk/upload-archive-use-start-command (2011-11-21) 1 commit
>>  - upload-archive: use start_command instead of fork
>>
>> What's the status of this one?
>
> I think what you have in pu is good, but of course I didn't actually
> test it on Windows. Erik?
>

I tried to check out ee27ca4 and build, and got hit by a wall of warnings:

compat/mingw.h: In function 'waitpid':
compat/mingw.h:117: warning: pointer targets in passing argument 1 of
'_cwait' differ in signedness
d:\msysgit\mingw\bin\../lib/gcc/mingw32/4.4.0/../../../../include/process.h:60:
note: expected 'int *' but argument is of type 'unsigned int *'

This seems to be an issue that has been around since the birth of the
windows port, and can be fixed by making our waitpid signature
identical to what POSIX expects
(http://pubs.opengroup.org/onlinepubs/7908799/xsh/waitpid.html):

---8<---
diff --git a/compat/mingw.h b/compat/mingw.h
index a255898..2036013 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -111,7 +111,7 @@ static inline int mingw_unlink(const char *pathname)
 }
 #define unlink mingw_unlink

-static inline int waitpid(pid_t pid, unsigned *status, unsigned options)
+static inline int waitpid(pid_t pid, int *status, int options)
 {
 	if (options == 0)
 		return _cwait(status, pid, 0);

---8<---

But then, there's a couple of much more scary warnings:

decorate.c: In function 'hash_obj':
decorate.c:11: warning: dereferencing type-punned pointer will break
strict-aliasing rules
<snip>
object.c: In function 'hash_obj':
object.c:48: warning: dereferencing type-punned pointer will break
strict-aliasing rules
<snip>
builtin-merge-recursive.c: In function 'cmd_merge_recursive':
builtin-merge-recursive.c:49: warning: unknown conversion type character 'z' in
format
builtin-merge-recursive.c:49: warning: format '%s' expects type 'char *', but ar
gument 2 has type 'unsigned int'
builtin-merge-recursive.c:49: warning: too many arguments for format

Looking at the code, the first two looks like the real thing, and not
just false positives.

IIRC, the strict aliasing rule is not a part of C89, but is in C99.
And GCC starts to generate code that depends on strict-aliasing rules
with -O3. We don't use -O3, so this might be theoretical, at least on
my machine at this time.

The last one is a usage if "%zu", which our printf doesn't support.
This comes from commit 73118f89 ("merge-recursive.c: Add more generic
merge_recursive_generic()"), and should probably be fixed by doing:
---8<---
diff --git a/builtin-merge-recursive.c b/builtin-merge-recursive.c
index 703045b..02242cc 100644
--- a/builtin-merge-recursive.c
+++ b/builtin-merge-recursive.c
@@ -45,8 +45,9 @@ int cmd_merge_recursive(int argc, const char **argv,
const char *prefix)
 			bases[bases_count++] = sha;
 		}
 		else
-			warning("Cannot handle more than %zu bases. "
-				"Ignoring %s.", ARRAY_SIZE(bases)-1, argv[i]);
+			warning("Cannot handle more than %"PRIuMAX" bases. "
+				"Ignoring %s.",
+				(uintmax_t)ARRAY_SIZE(bases)-1, argv[i]);
 	}
 	if (argc - i != 3) /* "--" "<head>" "<remote>" */
 		die("Not handling anything other than two heads merge.");
---8<---

But no matter what, something has clearly happened to our warning
level, and that should probably be investigated.

Back to topic: after fixing these (apart from the strict aliasing
issues), t5000-tar-tree pass fine here. So I guess that's at least
something. I haven't tested from git-daemon yet, but I don't have time
for that right now...

I'll submit the first fix later tonight, as it's clearly an
improvement IMO... but perhaps it's better if Stephan just squash in
the latter in the next round of the series, since it seems to be in a
pu-only series so far?

Re: What's cooking in git.git (Dec 2011, #02; Mon, 5)

[Jeff King]

On Tue, Dec 06, 2011 at 12:22:06PM +0100, Erik Faye-Lund wrote:

> >> * jk/upload-archive-use-start-command (2011-11-21) 1 commit
> >>  - upload-archive: use start_command instead of fork
> >>
> >> What's the status of this one?
> >
> > I think what you have in pu is good, but of course I didn't actually
> > test it on Windows. Erik?
> >
> 
> I tried to check out ee27ca4 and build, and got hit by a wall of warnings:

I think you are on the wrong topic. ee27ca4 is the maint topic for
"don't let remote clients get unreachable commits", and is based on the
ancient 1.6.2. Which is why you are getting all of those warnings.

The topic in question is jk/upload-archive-use-start-command, which is
at 1bc01ef, and should be based on recent-ish master.

-Peff

Re: What's cooking in git.git (Dec 2011, #02; Mon, 5)

[Erik Faye-Lund]

On Tue, Dec 6, 2011 at 7:52 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Dec 06, 2011 at 12:22:06PM +0100, Erik Faye-Lund wrote:
>
>> >> * jk/upload-archive-use-start-command (2011-11-21) 1 commit
>> >>  - upload-archive: use start_command instead of fork
>> >>
>> >> What's the status of this one?
>> >
>> > I think what you have in pu is good, but of course I didn't actually
>> > test it on Windows. Erik?
>> >
>>
>> I tried to check out ee27ca4 and build, and got hit by a wall of warnings:
>
> I think you are on the wrong topic. ee27ca4 is the maint topic for
> "don't let remote clients get unreachable commits", and is based on the
> ancient 1.6.2. Which is why you are getting all of those warnings.
>

You are indeed right. Thanks for spotting :)

> The topic in question is jk/upload-archive-use-start-command, which is
> at 1bc01ef, and should be based on recent-ish master.

t5000-tar-tree pass for that one as well. No warnings this time :P

Re: What's cooking in git.git (Dec 2011, #02; Mon, 5)

[Junio C Hamano]

Erik Faye-Lund <kusmabite@gmail.com> writes:

>> The topic in question is jk/upload-archive-use-start-command, which is
>> at 1bc01ef, and should be based on recent-ish master.
>
> t5000-tar-tree pass for that one as well. No warnings this time :P

Thanks; let's move it forward, then.

Re: What's cooking in git.git (Dec 2011, #02; Mon, 5)

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Also, let's drop the top git_getpass bits from the topic for now (they
> will not be part of my rebase). They are a separate topic that can go on
> top, but I think there was some question from Erik of whether we should
> simply roll our own getpass().

Sounds sensible.

I suspect that there may be a codepath where we could ask both username
and password; instead of making two consecutive calls to getpass() or
git_prompt(), the series may want to give a higher level abstraction, so
that GUI can show a dialog with two input fields (single-line input and
password input) and interact only once with the user. Such an input widget
could _show_ the username, and optionally even let it edited (there may be
ramifications depending on how the codepath uses the username), while
asking for the corresponding password.

>> * 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
>>  - 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 'next' after taking another look.
>
> Thanks. I also have some followup patches to re-loosen to at least
> trees reachable from refs. Do you want to leave the tightening to the
> maint track, and then consider the re-loosening for master?

I was planning to first have the really tight version graduate to 'master'
and ship it in 1.7.9, while possibly merging that to 1.7.8.X series. If we
hear complaints from real users in the meantime before or after such
releases, we could apply loosening patch on top of these topics and call
them "regression fix", but I have been assuming that nobody would have
been using this backdoor for anything that really matters.

Re: What's cooking in git.git (Dec 2011, #02; Mon, 5)

[Jeff King]

On Tue, Dec 06, 2011 at 10:35:25AM -0800, Junio C Hamano wrote:

> > Also, let's drop the top git_getpass bits from the topic for now (they
> > will not be part of my rebase). They are a separate topic that can go on
> > top, but I think there was some question from Erik of whether we should
> > simply roll our own getpass().
> 
> Sounds sensible.
> 
> I suspect that there may be a codepath where we could ask both username
> and password; instead of making two consecutive calls to getpass() or
> git_prompt(), the series may want to give a higher level abstraction, so
> that GUI can show a dialog with two input fields (single-line input and
> password input) and interact only once with the user. Such an input widget
> could _show_ the username, and optionally even let it edited (there may be
> ramifications depending on how the codepath uses the username), while
> asking for the corresponding password.

Yes, I've considered that, too. But I think the idea of a combined
username/password is part of the credential code, and the right
call chain is something like:

  credential_fill
    -> call helpers with "get"; return if it works
    -> credential_getpass
       -> call helpers with "ask" for combined GUI prompt
       -> otherwise, use git_prompt
          -> git_prompt("username")
          -> git_prompt("password")

So the "switch getpass to a generic prompt" idea is separate from
providing that higher-level abstraction.

> >> * 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.)
> [...]
> I was planning to first have the really tight version graduate to 'master'
> and ship it in 1.7.9, while possibly merging that to 1.7.8.X series. If we
> hear complaints from real users in the meantime before or after such
> releases, we could apply loosening patch on top of these topics and call
> them "regression fix", but I have been assuming that nobody would have
> been using this backdoor for anything that really matters.

OK. I'll hold back on the loosening then.

-Peff

Re: What's cooking in git.git (Dec 2011, #02; Mon, 5)

[Nguyen Thai Ngoc Duy]

On Tue, Dec 6, 2011 at 12:01 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Here are the topics that have been cooking.  Commits prefixed with '-' are
> only in 'pu' (proposed updates) while commits prefixed with '+' are in
> 'next'.

What can I do to get "build in repack" [1] series in or moved forward?

[1] http://thread.gmane.org/gmane.comp.version-control.git/185190
-- 
Duy

Re: What's cooking in git.git (Dec 2011, #02; Mon, 5)

[Junio C Hamano]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

> What can I do to get "build in repack" [1] series in or moved forward?

Resubmit to re-ignite interest in reviews, perhaps?

As long as it is done right at the implementation level, I do not think
there is anything controversial in the desired end result, iow it is not
like we need three people who want to have repack rewritten in C for it to
happen.

It may be tricky to get the flushing of old and new packfiles right,
though.

Use of reprepare_packed_git() is prerequisite if you want to do it in a
single process, but once you start using that API, it may not gain much
performance benefit to link the whole pack-objects logic in to the process
over a much simpler and less error prone approach of just rewriting the
shell script straight to a small C program that spawns pack-objects
binary.

Re: What's cooking in git.git (Dec 2011, #02; Mon, 5)

[Nguyen Thai Ngoc Duy]

On Tue, Dec 6, 2011 at 12:01 PM, Junio C Hamano <gitster@pobox.com> wrote:
> * nd/resolve-ref (2011-12-05) 2 commits
>  (merged to 'next' on 2011-12-05 at cc79e86)
>  + Copy resolve_ref() return value for longer use
>  + Convert many resolve_ref() calls to read_ref*() and ref_exists()
>
> Will merge to 'master'.

I thought this would be replaced by a new version [1] I posted a while
ago (and forgot to address comments). Do you want me to rebase that on
top of this or replace it?

[1] http://article.gmane.org/gmane.comp.version-control.git/185580
-- 
Duy

Re: What's cooking in git.git (Dec 2011, #02; Mon, 5)

[Junio C Hamano]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

> I thought this would be replaced by a new version [1] I posted a while
> ago (and forgot to address comments). Do you want me to rebase that on
> top of this or replace it?

I think what was merged to 'master' is already sane. If you have updates,
treat them just like other new topics.

Thanks.

Re: What's cooking in git.git (Dec 2011, #02; Mon, 5)

[Luke Diamand]

On 06/12/11 05:01, Junio C Hamano wrote:
> Here are the topics that have been cooking.  Commits prefixed with '-' are
> only in 'pu' (proposed updates) while commits prefixed with '+' are in
> 'next'.
>


>
> Will merge to 'next'.
>
> * ld/p4-labels-branches (2011-11-30) 4 commits
>   - git-p4: importing labels should cope with missing owner
>   - git-p4: add test for p4 labels
>   - git-p4: cope with labels with empty descriptions
>   - git-p4: handle p4 branches and labels containing shell chars
>
> I understand this has been retracted---please correct me otherwise.
> Will discard, expecting a reroll.

Yes, discard this one and I'll re-roll it with fixes for the other label 
issues.

Thanks
Luke