What's cooking in git.git (Nov 2012, #02; Fri, 9)

What's cooking in git.git (Nov 2012, #02; Fri, 9)

[Jeff King]

What's cooking in git.git (Nov 2012, #02; Fri, 9)
--------------------------------------------------

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 fourth batch of topics has graduated to master. This should be my
last integration cycle, as Junio will be back to take over before the
next one.

You can find the changes described here in the integration branches of
my repository at:

  git://github.com/peff/git.git

Until Junio returns, kernel.org and the other "usual" places will not be
updated.

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

* cr/push-force-tag-update (2012-11-09) 5 commits
 - push: update remote tags only with force
 - push: flag updates that require force
 - push: flag updates
 - push: add advice for rejected tag reference
 - push: return reject reasons via a mask

 Require "-f" for push to update a tag, even if it is a fast-forward.

 Needs review.

 I'm undecided yet on whether the goal is the right thing to do, but it
 does prevent some potential mistakes. I haven't looked closely at the
 implementation itself; review from interested parties would be helpful.


* fc/fast-export-fixes (2012-11-08) 14 commits
 - fast-export: don't handle uninteresting refs
 - fast-export: make sure updated refs get updated
 - fast-export: fix comparison in tests
 - fast-export: trivial cleanup
 - remote-testgit: make clear the 'done' feature
 - remote-testgit: report success after an import
 - remote-testgit: exercise more features
 - remote-testgit: cleanup tests
 - remote-testgit: remove irrelevant test
 - remote-testgit: get rid of non-local functionality
 - Add new simplified git-remote-testgit
 - Rename git-remote-testgit to git-remote-testpy
 - remote-testgit: fix direction of marks
 - fast-export: avoid importing blob marks

 Improvements to fix fast-export bugs, including how refs pointing to
 already-seen commits are handled. An earlier 4-commit version of this
 series looked good to me, but this much-expanded version has not seen
 any comments.

 Needs review.


* mg/maint-pull-suggest-upstream-to (2012-11-08) 1 commit
 - push/pull: adjust missing upstream help text to changed interface

 Follow-on to the new "--set-upstream-to" topic from v1.8.0 to avoid
 suggesting the deprecated "--set-upstream".

 Will merge to 'next'.


* mh/alt-odb-string-list-cleanup (2012-11-08) 2 commits
 - link_alt_odb_entries(): take (char *, len) rather than two pointers
 - link_alt_odb_entries(): use string_list_split_in_place()

 Cleanups in the alternates code. Fixes a potential bug and makes the
 code much cleaner.

 Will merge to 'next'.


* pf/editor-ignore-sigint (2012-11-08) 1 commit
 - launch_editor: ignore SIGINT while the editor has control

 Avoid confusing cases where the user hits Ctrl-C while in the editor
 session, not realizing git will receive the signal. Since most editors
 will take over the terminal and will block SIGINT, this is not likely
 to confuse anyone.

 Comments welcome from people using unusual editors (e.g., a script that
 starts an editor in another window then blocks, waiting for the user to
 finish).


* pp/gitweb-config-underscore (2012-11-08) 1 commit
 - gitweb: make remote_heads config setting work

 The key "gitweb.remote_heads" is not legal git config; this maps it to
 "gitweb.remoteheads".

 Junio raised a good point about the implementation for three-level
 variables.

 Expecting a re-roll.


* pw/maint-p4-rcs-expansion-newline (2012-11-08) 1 commit
 - git p4: RCS expansion should not span newlines

 I do not have p4 to play with, but looks obviously correct to me.

 Will merge to 'next'.


* rh/maint-gitweb-highlight-ext (2012-11-08) 1 commit
 - gitweb.perl: fix %highlight_ext mappings

 Fixes a clever misuse of perl's list interpretation.

 Will merge to 'next'.


* rr/submodule-diff-config (2012-11-08) 3 commits
 - submodule: display summary header in bold
 - diff: introduce diff.submodule configuration variable
 - Documentation: move diff.wordRegex from config.txt to diff-config.txt

 Lets "git diff --submodule=log" become the default via configuration.

 Seems like a good direction, though I had a few comments.

 Expecting a re-roll.


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

* fc/completion-send-email-with-format-patch (2012-10-16) 1 commit
  (merged to 'next' on 2012-11-04 at 0a6366e)
 + completion: add format-patch options to send-email

 Will merge to 'master' in the fourth batch.


* js/format-2047 (2012-10-18) 7 commits
  (merged to 'next' on 2012-10-25 at 76d91fe)
 + format-patch tests: check quoting/encoding in To: and Cc: headers
 + format-patch: fix rfc2047 address encoding with respect to rfc822 specials
 + format-patch: make rfc2047 encoding more strict
 + format-patch: introduce helper function last_line_length()
 + format-patch: do not wrap rfc2047 encoded headers too late
 + format-patch: do not wrap non-rfc2047 headers too early
 + utf8: fix off-by-one wrapping of text

 Fixes many rfc2047 quoting issues in the output from format-patch.

 Will merge to 'master' in the fourth batch.


* km/send-email-compose-encoding (2012-10-25) 5 commits
  (merged to 'next' on 2012-10-29 at d7d2bb4)
 + git-send-email: add rfc2047 quoting for "=?"
 + git-send-email: introduce quote_subject()
 + git-send-email: skip RFC2047 quoting for ASCII subjects
 + git-send-email: use compose-encoding for Subject
  (merged to 'next' on 2012-10-25 at 5447367)
 + git-send-email: introduce compose-encoding

 "git send-email --compose" can let the user create a non-ascii
 cover letter message, but there was not a way to mark it with
 appropriate content type before sending it out.

 Further updates fix subject quoting.

 Will merge to 'master' in the fourth batch.


* mh/maint-parse-dirstat-fix (2012-10-29) 1 commit
  (merged to 'next' on 2012-11-04 at 852d609)
 + parse_dirstat_params(): use string_list to split comma-separated string

 Cleans up some code and avoids a potential bug.

 Will merge to 'master' in the fourth batch.


* mo/cvs-server-cleanup (2012-10-26) 11 commits
  (merged to 'next' on 2012-10-29 at 4e70622)
 + Use character class for sed expression instead of \s
  (merged to 'next' on 2012-10-25 at c70881d)
 + cvsserver status: provide real sticky info
 + cvsserver: cvs add: do not expand directory arguments
 + cvsserver: use whole CVS rev number in-process; don't strip "1." prefix
 + cvsserver: split up long lines in req_{status,diff,log}
 + cvsserver: clean up client request handler map comments
 + cvsserver: remove unused functions _headrev and gethistory
 + cvsserver update: comment about how we shouldn't remove a user-modified file
 + cvsserver: add comments about database schema/usage
 + cvsserver: removed unused sha1Or-k mode from kopts_from_path
 + cvsserver t9400: add basic 'cvs log' test
 (this branch is tangled with mo/cvs-server-updates.)

 Cleanups to prepare for mo/cvs-server-updates.

 Will merge to 'master' in the fourth batch.


* nd/attr-match-optim-more (2012-10-15) 7 commits
  (merged to 'next' on 2012-10-25 at 09f70ce)
 + attr: more matching optimizations from .gitignore
 + gitignore: make pattern parsing code a separate function
 + exclude: split pathname matching code into a separate function
 + exclude: fix a bug in prefix compare optimization
 + exclude: split basename matching code into a separate function
 + exclude: stricten a length check in EXC_FLAG_ENDSWITH case
 + Merge commit 'f9f6e2c' into nd/attr-match-optim-more
 (this branch is used by as/check-ignore and nd/wildmatch.)

 Start laying the foundation to build the "wildmatch" after we can
 agree on its desired semantics.

 Will merge to 'master' in the fourth batch.


* nd/builtin-to-libgit (2012-10-29) 7 commits
  (merged to 'next' on 2012-11-04 at 06cbf9b)
 + fetch-pack: move core code to libgit.a
 + fetch-pack: remove global (static) configuration variable "args"
 + send-pack: move core code to libgit.a
 + Move setup_diff_pager to libgit.a
 + Move print_commit_list to libgit.a
 + Move estimate_bisect_steps to libgit.a
 + Move try_merge_command and checkout_fast_forward to libgit.a

 Code cleanups so that libgit.a does not depend on anything in the
 builtin/ directory.

 Some of the code movement is pretty big, but there doesn't seem to be
 any conflicts with topics in flight.

 Will merge to 'master' in the fourth batch.


* nd/tree-walk-enum-cleanup (2012-10-19) 1 commit
  (merged to 'next' on 2012-11-04 at 8ccdf98)
 + tree-walk: use enum interesting instead of integer

 Will merge to 'master' in the fourth batch.


* ph/maint-submodule-status-fix (2012-10-29) 2 commits
  (merged to 'next' on 2012-11-04 at d700e02)
 + submodule status: remove unused orig_* variables
 + t7407: Fix recursive submodule test

 Cleans up some leftover bits from an earlier submodule change.

 Will merge to 'master' in the fourth batch.


* rs/lock-correct-ref-during-delete (2012-10-16) 1 commit
  (merged to 'next' on 2012-10-25 at 9341eea)
 + refs: lock symref that is to be deleted, not its target

 When "update-ref -d --no-deref SYM" tried to delete a symbolic ref
 SYM, it incorrectly locked the underlying reference pointed by SYM,
 not the symbolic ref itself.

 Will merge to 'master' in the fourth batch.


* sz/maint-curl-multi-timeout (2012-10-19) 1 commit
  (merged to 'next' on 2012-11-04 at f696dd8)
 + Fix potential hang in https handshake

 Sometimes curl_multi_timeout() function suggested a wrong timeout
 value when there is no file descriptors to wait on and the http
 transport ended up sleeping for minutes in select(2) system call.
 Detect this and reduce the wait timeout in such a case.

 Will merge to 'master' in the fourth batch.

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

* rc/maint-complete-git-p4 (2012-09-24) 1 commit
  (merged to 'next' on 2012-10-29 at af52cef)
 + Teach git-completion about git p4

 Comment from Pete will need to be addressed in a follow-up patch.


* as/test-tweaks (2012-09-20) 7 commits
 - tests: paint unexpectedly fixed known breakages in bold red
 - tests: test the test framework more thoroughly
 - [SQUASH] t/t0000-basic.sh: quoting of TEST_DIRECTORY is screwed up
 - tests: refactor mechanics of testing in a sub test-lib
 - tests: paint skipped tests in bold blue
 - tests: test number comes first in 'not ok $count - $message'
 - tests: paint known breakages in bold yellow

 Various minor tweaks to the test framework to paint its output
 lines in colors that match what they mean better.

 Has the "is this really blue?" issue Peff raised resolved???


* jc/maint-name-rev (2012-09-17) 7 commits
 - describe --contains: use "name-rev --algorithm=weight"
 - name-rev --algorithm=weight: tests and documentation
 - name-rev --algorithm=weight: cache the computed weight in notes
 - name-rev --algorithm=weight: trivial optimization
 - name-rev: --algorithm option
 - name_rev: clarify the logic to assign a new tip-name to a commit
 - name-rev: lose unnecessary typedef

 "git name-rev" names the given revision based on a ref that can be
 reached in the smallest number of steps from the rev, but that is
 not useful when the caller wants to know which tag is the oldest one
 that contains the rev.  This teaches a new mode to the command that
 uses the oldest ref among those which contain the rev.

 I am not sure if this is worth it; for one thing, even with the help
 from notes-cache, it seems to make the "describe --contains" even
 slower. Also the command will be unusably slow for a user who does
 not have a write access (hence unable to create or update the
 notes-cache).

 Stalled mostly due to lack of responses.


* jc/xprm-generation (2012-09-14) 1 commit
 - test-generation: compute generation numbers and clock skews

 A toy to analyze how bad the clock skews are in histories of real
 world projects.

 Stalled mostly due to lack of responses.


* jc/blame-no-follow (2012-09-21) 2 commits
 - blame: pay attention to --no-follow
 - diff: accept --no-follow option

 Teaches "--no-follow" option to "git blame" to disable its
 whole-file rename detection.

 Stalled mostly due to lack of responses.


* jc/doc-default-format (2012-10-07) 2 commits
 - [SQAUSH] allow "cd Doc* && make DEFAULT_DOC_TARGET=..."
 - Allow generating a non-default set of documentation

 Need to address the installation half if this is to be any useful.


* mk/maint-graph-infinity-loop (2012-09-25) 1 commit
 - graph.c: infinite loop in git whatchanged --graph -m

 The --graph code fell into infinite loop when asked to do what the
 code did not expect ;-)

 Anybody who worked on "--graph" wants to comment?
 Stalled mostly due to lack of responses.


* jc/add-delete-default (2012-08-13) 1 commit
 - git add: notice removal of tracked paths by default

 "git add dir/" updated modified files and added new files, but does
 not notice removed files, which may be "Huh?" to some users.  They
 can of course use "git add -A dir/", but why should they?

 Resurrected from graveyard, as I thought it was a worthwhile thing
 to do in the longer term.

 Waiting for comments.


* mb/remote-default-nn-origin (2012-07-11) 6 commits
 - Teach get_default_remote to respect remote.default.
 - Test that plain "git fetch" uses remote.default when on a detached HEAD.
 - Teach clone to set remote.default.
 - Teach "git remote" about remote.default.
 - Teach remote.c about the remote.default configuration setting.
 - Rename remote.c's default_remote_name static variables.

 When the user does not specify what remote to interact with, we
 often attempt to use 'origin'.  This can now be customized via a
 configuration variable.

 Expecting a re-roll.

 "The first remote becomes the default" bit is better done as a
 separate step.


* mh/ceiling (2012-10-29) 8 commits
 - string_list_longest_prefix(): remove function
 - setup_git_directory_gently_1(): resolve symlinks in ceiling paths
 - longest_ancestor_length(): require prefix list entries to be normalized
 - longest_ancestor_length(): take a string_list argument for prefixes
 - longest_ancestor_length(): use string_list_split()
 - Introduce new function real_path_if_valid()
 - real_path_internal(): add comment explaining use of cwd
 - Introduce new static function real_path_internal()

 Elements of GIT_CEILING_DIRECTORIES list may not match the real
 pathname we obtain from getcwd(), leading the GIT_DIR discovery
 logic to escape the ceilings the user thought to have specified.


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

* mo/cvs-server-updates (2012-10-16) 10 commits
 - cvsserver Documentation: new cvs ... -r support
 - cvsserver: add t9402 to test branch and tag refs
 - cvsserver: support -r and sticky tags for most operations
 - cvsserver: Add version awareness to argsfromdir
 - cvsserver: generalize getmeta() to recognize commit refs
 - cvsserver: implement req_Sticky and related utilities
 - cvsserver: add misc commit lookup, file meta data, and file listing functions
 - cvsserver: define a tag name character escape mechanism
 - cvsserver: cleanup extra slashes in filename arguments
 - cvsserver: factor out git-log parsing logic

 Needs review by folks interested in cvsserver.


* ta/doc-cleanup (2012-10-25) 6 commits
 - Documentation: build html for all files in technical and howto
 - Documentation/howto: convert plain text files to asciidoc
 - Documentation/technical: convert plain text files to asciidoc
 - Change headline of technical/send-pack-pipeline.txt to not confuse its content with content from git-send-pack.txt
 - Shorten two over-long lines in git-bisect-lk2009.txt by abbreviating some sha1
 - Split over-long synopsis in git-fetch-pack.txt into several lines

 Will merge to 'next'.


* lt/diff-stat-show-0-lines (2012-10-17) 1 commit
 - Fix "git diff --stat" for interesting - but empty - file changes

 We failed to mention a file without any content change but whose
 permission bit was modified, or (worse yet) a new file without any
 content in the "git diff --stat" output.

 Needs some test updates.


* jc/prettier-pretty-note (2012-10-26) 11 commits
  (merged to 'next' on 2012-11-04 at 40e3e48)
 + Doc User-Manual: Patch cover letter, three dashes, and --notes
 + Doc format-patch: clarify --notes use case
 + Doc notes: Include the format-patch --notes option
 + Doc SubmittingPatches: Mention --notes option after "cover letter"
 + Documentation: decribe format-patch --notes
 + format-patch --notes: show notes after three-dashes
 + format-patch: append --signature after notes
 + pretty_print_commit(): do not append notes message
 + pretty: prepare notes message at a centralized place
 + format_note(): simplify API
 + pretty: remove reencode_commit_message()

 Now that Philip has submitted some documentation updates, this is
 looking more ready.

 Will merge to 'master' in the fifth batch.


* jc/same-encoding (2012-11-04) 1 commit
  (merged to 'next' on 2012-11-04 at 54991f2)
 + reencode_string(): introduce and use same_encoding()

 Various codepaths checked if two encoding names are the same using
 ad-hoc code and some of them ended up asking iconv() to convert
 between "utf8" and "UTF-8".  The former is not a valid way to spell
 the encoding name, but often people use it by mistake, and we
 equated them in some but not all codepaths. Introduce a new helper
 function to make these codepaths consistent.

 Will merge to 'master' in the fifth batch.


* cr/cvsimport-local-zone (2012-11-04) 2 commits
  (merged to 'next' on 2012-11-04 at 292f0b4)
 + cvsimport: work around perl tzset issue
 + git-cvsimport: allow author-specific timezones

 Allows "cvsimport" to read per-author timezone from the author info
 file.

 Will merge to 'master' in the fifth batch.


* fc/zsh-completion (2012-10-29) 3 commits
 - completion: add new zsh completion
 - completion: add new __gitcompadd helper
 - completion: get rid of empty COMPREPLY assignments

 There were some comments on this, but I wasn't clear on the outcome.

 Need to take a closer look.


* jc/apply-trailing-blank-removal (2012-10-12) 1 commit
 - apply.c:update_pre_post_images(): the preimage can be truncated

 Fix to update_pre_post_images() that did not take into account the
 possibility that whitespace fix could shrink the preimage and
 change the number of lines in it.

 Extra set of eyeballs appreciated.


* jn/warn-on-inaccessible-loosen (2012-10-14) 4 commits
 - config: exit on error accessing any config file
 - doc: advertise GIT_CONFIG_NOSYSTEM
 - config: treat user and xdg config permission problems as errors
 - config, gitignore: failure to access with ENOTDIR is ok

 An RFC to deal with a situation where .config/git is a file and we
 notice .config/git/config is not readable due to ENOTDIR, not
 ENOENT; I think a bit more refactored approach to consistently
 address permission errors across config, exclude and attrs is
 desirable.  Don't we also need a check for an opposite situation
 where we open .config/git/config or .gitattributes for reading but
 they turn out to be directories?


* as/check-ignore (2012-11-08) 14 commits
 - t0007: fix tests on Windows
 - Documentation/check-ignore: we show the deciding match, not the first
 - Add git-check-ignore sub-command
 - dir.c: provide free_directory() for reclaiming dir_struct memory
 - pathspec.c: move reusable code from builtin/add.c
 - dir.c: refactor treat_gitlinks()
 - dir.c: keep track of where patterns came from
 - dir.c: refactor is_path_excluded()
 - dir.c: refactor is_excluded()
 - dir.c: refactor is_excluded_from_list()
 - dir.c: rename excluded() to is_excluded()
 - dir.c: rename excluded_from_list() to is_excluded_from_list()
 - dir.c: rename path_excluded() to is_path_excluded()
 - dir.c: rename cryptic 'which' variable to more consistent name

 Duy helped to reroll this.

 Expecting a re-roll.


* so/prompt-command (2012-10-17) 4 commits
  (merged to 'next' on 2012-10-25 at 79565a1)
 + coloured git-prompt: paint detached HEAD marker in red
 + Fix up colored git-prompt
 + show color hints based on state of the git tree
 + Allow __git_ps1 to be used in PROMPT_COMMAND

 Updates __git_ps1 so that it can be used as $PROMPT_COMMAND,
 instead of being used for command substitution in $PS1, to embed
 color escape sequences in its output.

 Will cook in 'next'.


* aw/rebase-am-failure-detection (2012-10-11) 1 commit
 - rebase: Handle cases where format-patch fails

 I am unhappy a bit about the possible performance implications of
 having to store the output in a temporary file only for a rare case
 of format-patch aborting.


* nd/wildmatch (2012-10-15) 13 commits
  (merged to 'next' on 2012-10-25 at 510e8df)
 + Support "**" wildcard in .gitignore and .gitattributes
 + wildmatch: make /**/ match zero or more directories
 + wildmatch: adjust "**" behavior
 + wildmatch: fix case-insensitive matching
 + wildmatch: remove static variable force_lower_case
 + wildmatch: make wildmatch's return value compatible with fnmatch
 + t3070: disable unreliable fnmatch tests
 + Integrate wildmatch to git
 + wildmatch: follow Git's coding convention
 + wildmatch: remove unnecessary functions
 + Import wildmatch from rsync
 + ctype: support iscntrl, ispunct, isxdigit and isprint
 + ctype: make sane_ctype[] const array

 Allows pathname patterns in .gitignore and .gitattributes files
 with double-asterisks "foo/**/bar" to match any number of directory
 hierarchies.

 I suspect that this needs to be plugged to pathspec matching code;
 otherwise "git log -- 'Docum*/**/*.txt'" would not show the log for
 commits that touch Documentation/git.txt, which would be confusing
 to the users.

 Will cook in 'next'.


* jk/lua-hackery (2012-10-07) 6 commits
 - pretty: fix up one-off format_commit_message calls
 - Minimum compilation fixup
 - Makefile: make "lua" a bit more configurable
 - add a "lua" pretty format
 - add basic lua infrastructure
 - pretty: make some commit-parsing helpers more public

 Interesting exercise. When we do this for real, we probably would want
 to wrap a commit to make it more like an "object" with methods like
 "parents", etc.


* nd/pretty-placeholder-with-color-option (2012-09-30) 9 commits
 . pretty: support %>> that steal trailing spaces
 . pretty: support truncating in %>, %< and %><
 . pretty: support padding placeholders, %< %> and %><
 . pretty: two phase conversion for non utf-8 commits
 . utf8.c: add utf8_strnwidth() with the ability to skip ansi sequences
 . utf8.c: move display_mode_esc_sequence_len() for use by other functions
 . pretty: support %C(auto[,N]) to turn on coloring on next placeholder(s)
 . pretty: split parsing %C into a separate function
 . pretty: share code between format_decoration and show_decorations

 This causes warnings with -Wuninitialized, so I've ejected it from pu
 for the time being.


* jc/maint-fetch-tighten-refname-check (2012-10-19) 1 commit
  (merged to 'next' on 2012-11-04 at eda85ef)
 + get_fetch_map(): tighten checks on dest refs

 This was split out from discarded jc/maint-push-refs-all topic.

 Will merge to 'master' in the fifth batch.


* jh/symbolic-ref-d (2012-10-21) 1 commit
  (merged to 'next' on 2012-11-04 at b0762f5)
 + git symbolic-ref --delete $symref

 Add "symbolic-ref -d SYM" to delete a symbolic ref SYM.

 It is already possible to remove a symbolic ref with "update-ref -d
 --no-deref", but it may be a good addition for completeness.

 Will merge to 'master' in the fifth batch.


* jh/update-ref-d-through-symref (2012-10-21) 2 commits
 - Fix failure to delete a packed ref through a symref
 - t1400-update-ref: Add test verifying bug with symrefs in delete_ref()

 "update-ref -d --deref SYM" to delete a ref through a symbolic ref
 that points to it did not remove it correctly.


* jk/config-ignore-duplicates (2012-10-29) 9 commits
  (merged to 'next' on 2012-10-29 at 67fa0a2)
 + builtin/config.c: Fix a sparse warning
  (merged to 'next' on 2012-10-25 at 233df08)
 + git-config: use git_config_with_options
 + git-config: do not complain about duplicate entries
 + git-config: collect values instead of immediately printing
 + git-config: fix regexp memory leaks on error conditions
 + git-config: remove memory leak of key regexp
 + t1300: test "git config --get-all" more thoroughly
 + t1300: remove redundant test
 + t1300: style updates

 Drop duplicate detection from git-config; this lets it
 better match the internal config callbacks, which clears up
 some corner cases with includes.

 Will cook in 'next'.


* ph/submodule-sync-recursive (2012-10-29) 2 commits
  (merged to 'next' on 2012-11-04 at a000f78)
 + Add tests for submodule sync --recursive
 + Teach --recursive to submodule sync

 Adds "--recursive" option to submodule sync.

 Will merge to 'master' in the fifth batch.


* fc/completion-test-simplification (2012-10-29) 2 commits
 - completion: simplify __gitcomp test helper
 - completion: refactor __gitcomp related tests

 Clean up completion tests.

 There were some comments on the list.

 Expecting a re-roll.


* fc/remote-testgit-feature-done (2012-10-29) 1 commit
 - remote-testgit: properly check for errors

 Needs review.


* jk/maint-diff-grep-textconv (2012-10-28) 1 commit
  (merged to 'next' on 2012-11-04 at 790337b)
 + diff_grep: use textconv buffers for add/deleted files
 (this branch is used by jk/pickaxe-textconv.)

 Fixes inconsistent use of textconv with "git log -G".

 Will merge to 'master' in the fifth batch.


* jk/pickaxe-textconv (2012-10-28) 2 commits
 - pickaxe: use textconv for -S counting
 - pickaxe: hoist empty needle check
 (this branch uses jk/maint-diff-grep-textconv.)

 Use textconv filters when searching with "log -S".

 Waiting for a sanity check and review from Junio.


* as/maint-doc-fix-no-post-rewrite (2012-11-02) 1 commit
  (merged to 'next' on 2012-11-09 at 117a91e)
 + commit: fixup misplacement of --no-post-rewrite description

 Will merge to 'master' in the fifth batch.


* fc/remote-bzr (2012-11-08) 5 commits
 - remote-bzr: update working tree
 - remote-bzr: add support for remote repositories
 - remote-bzr: add support for pushing
 - remote-bzr: add simple tests
 - Add new remote-bzr transport helper

 New remote helper for bzr.

 Will merge to 'next'.


* fc/remote-hg (2012-11-04) 16 commits
 - remote-hg: the author email can be null
 - remote-hg: add option to not track branches
 - remote-hg: add extra author test
 - remote-hg: add tests to compare with hg-git
 - remote-hg: add bidirectional tests
 - test-lib: avoid full path to store test results
 - remote-hg: add basic tests
 - remote-hg: fake bookmark when there's none
 - remote-hg: add compat for hg-git author fixes
 - remote-hg: add support for hg-git compat mode
 - remote-hg: match hg merge behavior
 - remote-hg: make sure the encoding is correct
 - remote-hg: add support to push URLs
 - remote-hg: add support for remote pushing
 - remote-hg: add support for pushing
 - Add new remote-hg transport helper

 New remote helper for hg.

 Will merge to 'next'.


* jk/maint-http-half-auth-fetch (2012-10-31) 2 commits
  (merged to 'next' on 2012-11-09 at af69926)
 + remote-curl: retry failed requests for auth even with gzip
 + remote-curl: hoist gzip buffer size to top of post_rpc

 Fixes fetch from servers that ask for auth only during the actual
 packing phase. This is not really a recommended configuration, but it
 cleans up the code at the same time.

 Will merge to 'master' in the sixth batch.


* js/hp-nonstop (2012-10-30) 1 commit
  (merged to 'next' on 2012-11-09 at fe58205)
 + fix 'make test' for HP NonStop

 Will merge to 'master' in the fifth batch.


* kb/preload-index-more (2012-11-02) 1 commit
  (merged to 'next' on 2012-11-09 at a750ebd)
 + update-index/diff-index: use core.preloadindex to improve performance

 Use preloadindex in more places, which has a nice speedup on systems
 with slow stat calls (and even on Linux).

 Will merge to 'master' in the sixth batch.


* mh/notes-string-list (2012-11-08) 5 commits
  (merged to 'next' on 2012-11-09 at 7a4c58c)
 + string_list_add_refs_from_colon_sep(): use string_list_split()
 + notes: fix handling of colon-separated values
 + combine_notes_cat_sort_uniq(): sort and dedup lines all at once
 + Initialize sort_uniq_list using named constant
 + string_list: add a function string_list_remove_empty_items()

 Improve the asymptotic performance of the cat_sort_uniq notes merge
 strategy.

 Will merge to 'master' in the fifth batch.


* mh/strbuf-split (2012-11-04) 4 commits
  (merged to 'next' on 2012-11-09 at fa984b1)
 + strbuf_split*(): document functions
 + strbuf_split*(): rename "delim" parameter to "terminator"
 + strbuf_split_buf(): simplify iteration
 + strbuf_split_buf(): use ALLOC_GROW()

 Cleanups and documentation for strbuf_split.

 Will merge to 'master' in the fifth batch.


* mm/maint-doc-commit-edit (2012-11-02) 1 commit
  (merged to 'next' on 2012-11-09 at 8dab7f5)
 + Document 'git commit --no-edit' explicitly

 Will merge to 'master' in the fifth batch.

Re: What's cooking in git.git (Nov 2012, #02; Fri, 9)

[Ralf Thielow]

On Fri, Nov 9, 2012 at 8:23 PM, Jeff King <peff@peff.net> wrote:
>
> You can find the changes described here in the integration branches of
> my repository at:
>
>   git://github.com/peff/git.git

It seems that the repo doesn't contain the integration branches?!?

$ git remote add peff git://github.com/peff/git.git
$ git fetch -v peff
From git://github.com/peff/git
 * [new branch]      maint      -> peff/maint
 * [new branch]      master     -> peff/master
 * [new branch]      next       -> peff/next
 * [new branch]      pu         -> peff/pu
 * [new branch]      todo       -> peff/todo
$

Re: What's cooking in git.git (Nov 2012, #02; Fri, 9)

[Ralf Thielow]

On Fri, Nov 9, 2012 at 9:01 PM, Ralf Thielow <ralf.thielow@gmail.com> wrote:
> On Fri, Nov 9, 2012 at 8:23 PM, Jeff King <peff@peff.net> wrote:
>>
>> You can find the changes described here in the integration branches of
>> my repository at:
>>
>>   git://github.com/peff/git.git
>
> It seems that the repo doesn't contain the integration branches?!?
>
> $ git remote add peff git://github.com/peff/git.git
> $ git fetch -v peff
> From git://github.com/peff/git
>  * [new branch]      maint      -> peff/maint
>  * [new branch]      master     -> peff/master
>  * [new branch]      next       -> peff/next
>  * [new branch]      pu         -> peff/pu
>  * [new branch]      todo       -> peff/todo
> $

But "integration branches" means "master", "next" and "pu" than I haven't
said anything. ;) Sorry for the noise.

Re: What's cooking in git.git (Nov 2012, #02; Fri, 9)

[Jeff King]

On Fri, Nov 09, 2012 at 09:06:47PM +0100, Ralf Thielow wrote:

> > It seems that the repo doesn't contain the integration branches?!?
> >
> > $ git remote add peff git://github.com/peff/git.git
> > $ git fetch -v peff
> > From git://github.com/peff/git
> >  * [new branch]      maint      -> peff/maint
> >  * [new branch]      master     -> peff/master
> >  * [new branch]      next       -> peff/next
> >  * [new branch]      pu         -> peff/pu
> >  * [new branch]      todo       -> peff/todo
> > $
> 
> But "integration branches" means "master", "next" and "pu" than I haven't
> said anything. ;) Sorry for the noise.

Right. :)

I have not been pushing the individual topic branches to make life
easier for people who usually just track Junio's kernel.org repository,
and would not welcome suddenly getting a hundred extra remote branches.
I can make them public if it makes life easier for people, but it may
not be worth it at this point, with Junio returning soon.

-Peff

Re: What's cooking in git.git (Nov 2012, #02; Fri, 9)

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I have not been pushing the individual topic branches to make life
> easier for people who usually just track Junio's kernel.org repository,
> and would not welcome suddenly getting a hundred extra remote branches.
> I can make them public if it makes life easier for people, but it may
> not be worth it at this point, with Junio returning soon.

What we should have arranged was to have https://github.com/git/git
(which is not even owned by me, but I asked somebody at GitHub to
assign me a write privilege) writable by the interim maintainer, so
that normal people would keep pulling from there, while the interim
maintainer can choose to publish broken-out branches to his
repository.

And it is not too late to do so; from the look of your "What's
cooking", you are doing pretty well ;-).

Re: What's cooking in git.git (Nov 2012, #02; Fri, 9)

[Kalle Olavi Niemitalo]

Jeff King <peff@peff.net> writes:

>  Comments welcome from people using unusual editors (e.g., a script that
>  starts an editor in another window then blocks, waiting for the user to
>  finish).

I often run a shell in Emacs in X, then start git commit in that
shell.  $EDITOR is emacsclient --current-frame, which asks the
existing Emacs instance to load the file and waits until I press
C-x # in Emacs to mark the file done.  If I want to abort the
commit, it is most intuitive to return to the *Shell* buffer in
Emacs and press C-c C-c (comint-interrupt-subjob) to send SIGINT
to git from there.  (I see that "an empty message aborts the
commit", and indeed it does, but well, I prefer not to trust such
a feature if I can instead just interrupt the thing.)

With pf/editor-ignore-sigint, C-c C-c in the *Shell* buffer kills
neither git nor the emacsclient started by git.  This is not good.
SIGQUIT from C-c C-\ (comint-quit-subjob) still works though.

Re: What's cooking in git.git (Nov 2012, #02; Fri, 9)

[Felipe Contreras]

Hi,

On Fri, Nov 9, 2012 at 8:23 PM, Jeff King <peff@peff.net> wrote:

> * fc/fast-export-fixes (2012-11-08) 14 commits
>  - fast-export: don't handle uninteresting refs
>  - fast-export: make sure updated refs get updated
>  - fast-export: fix comparison in tests
>  - fast-export: trivial cleanup
>  - remote-testgit: make clear the 'done' feature
>  - remote-testgit: report success after an import
>  - remote-testgit: exercise more features
>  - remote-testgit: cleanup tests
>  - remote-testgit: remove irrelevant test
>  - remote-testgit: get rid of non-local functionality
>  - Add new simplified git-remote-testgit
>  - Rename git-remote-testgit to git-remote-testpy
>  - remote-testgit: fix direction of marks
>  - fast-export: avoid importing blob marks
>
>  Improvements to fix fast-export bugs, including how refs pointing to
>  already-seen commits are handled. An earlier 4-commit version of this
>  series looked good to me, but this much-expanded version has not seen
>  any comments.
>
>  Needs review.

I can send the previous 4-commit version if needed, the only thing
that changed is the commit messages.

I think it's unfortunate that 4-commit version would not be mentioning
that it fixes the above tests, but hey; I did what I could.

> * fc/zsh-completion (2012-10-29) 3 commits
>  - completion: add new zsh completion
>  - completion: add new __gitcompadd helper
>  - completion: get rid of empty COMPREPLY assignments
>
>  There were some comments on this, but I wasn't clear on the outcome.
>
>  Need to take a closer look.

SZEDER should probably take a look. I think it should be better than
the previous series.

> * fc/completion-test-simplification (2012-10-29) 2 commits
>  - completion: simplify __gitcomp test helper
>  - completion: refactor __gitcomp related tests
>
>  Clean up completion tests.
>
>  There were some comments on the list.
>
>  Expecting a re-roll.

The second patch I can re-roll, but the first patch needs some
external input. My preference is that tests should also be simple and
maintainable, SZEDER's preference is that tests are better being
explicit and verbose (even if harder to maintain) to minimize possible
issues in the tests.

> * fc/remote-testgit-feature-done (2012-10-29) 1 commit
>  - remote-testgit: properly check for errors
>
>  Needs review.

Sverre probably should reply. I think I already addressed his comments
and the patch should be OK to push.

But probably it's not that important considering the testgit
refactoring, and also I'm thinking that we need to actually check the
status of the process[1] because the situation is still not OK with
pushing, and I'm learning it the hard way with a buggy remote helper.

> * fc/remote-bzr (2012-11-08) 5 commits
>  - remote-bzr: update working tree
>  - remote-bzr: add support for remote repositories
>  - remote-bzr: add support for pushing
>  - remote-bzr: add simple tests
>  - Add new remote-bzr transport helper
>
>  New remote helper for bzr.
>
>  Will merge to 'next'.

I already have a newer version of this with support for special modes:
executable files, symlinks, etc. I think a reroll would make sense.

> * fc/remote-hg (2012-11-04) 16 commits
>  - remote-hg: the author email can be null
>  - remote-hg: add option to not track branches
>  - remote-hg: add extra author test
>  - remote-hg: add tests to compare with hg-git
>  - remote-hg: add bidirectional tests
>  - test-lib: avoid full path to store test results
>  - remote-hg: add basic tests
>  - remote-hg: fake bookmark when there's none
>  - remote-hg: add compat for hg-git author fixes
>  - remote-hg: add support for hg-git compat mode
>  - remote-hg: match hg merge behavior
>  - remote-hg: make sure the encoding is correct
>  - remote-hg: add support to push URLs
>  - remote-hg: add support for remote pushing
>  - remote-hg: add support for pushing
>  - Add new remote-hg transport helper
>
>  New remote helper for hg.
>
>  Will merge to 'next'.

:)

I have a few patches on top of this, but they can probably wait.

Cheers.

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

-- 
Felipe Contreras

Re: What's cooking in git.git (Nov 2012, #02; Fri, 9)

[Jeff King]

On Sat, Nov 10, 2012 at 12:21:48AM +0100, Felipe Contreras wrote:

> > * fc/fast-export-fixes (2012-11-08) 14 commits
> >  - fast-export: don't handle uninteresting refs
> >  - fast-export: make sure updated refs get updated
> >  - fast-export: fix comparison in tests
> >  - fast-export: trivial cleanup
> >  - remote-testgit: make clear the 'done' feature
> >  - remote-testgit: report success after an import
> >  - remote-testgit: exercise more features
> >  - remote-testgit: cleanup tests
> >  - remote-testgit: remove irrelevant test
> >  - remote-testgit: get rid of non-local functionality
> >  - Add new simplified git-remote-testgit
> >  - Rename git-remote-testgit to git-remote-testpy
> >  - remote-testgit: fix direction of marks
> >  - fast-export: avoid importing blob marks
> >
> >  Improvements to fix fast-export bugs, including how refs pointing to
> >  already-seen commits are handled. An earlier 4-commit version of this
> >  series looked good to me, but this much-expanded version has not seen
> >  any comments.
> >
> >  Needs review.
> 
> I can send the previous 4-commit version if needed, the only thing
> that changed is the commit messages.

In the actual code, perhaps, but aren't there significant changes to the
git-remote-testgit infrastructure that were not originally present? That
could use some review.

I also seem to recall that the tests in this version rely on the presence of bash;
don't we still need to mark the tests with a prerequisite?

> > * fc/completion-test-simplification (2012-10-29) 2 commits
> >  - completion: simplify __gitcomp test helper
> >  - completion: refactor __gitcomp related tests
> >
> >  Clean up completion tests.
> >
> >  There were some comments on the list.
> >
> >  Expecting a re-roll.
> 
> The second patch I can re-roll, but the first patch needs some
> external input. My preference is that tests should also be simple and
> maintainable, SZEDER's preference is that tests are better being
> explicit and verbose (even if harder to maintain) to minimize possible
> issues in the tests.

I think it is better to keep the tests simple and maintainable. If there
are multiple ways to do things and they all need testing, then that
should be clear from the tests, not done haphazardly because some tests
happen to use a different way of doing things.

I seem to recall there was a one-liner fix that needed to be rolled in,
which is why I held it out of next.

> > * fc/remote-bzr (2012-11-08) 5 commits
> >  - remote-bzr: update working tree
> >  - remote-bzr: add support for remote repositories
> >  - remote-bzr: add support for pushing
> >  - remote-bzr: add simple tests
> >  - Add new remote-bzr transport helper
> >
> >  New remote helper for bzr.
> >
> >  Will merge to 'next'.
> 
> I already have a newer version of this with support for special modes:
> executable files, symlinks, etc. I think a reroll would make sense.

Thanks for letting me know.

-Peff

Re: What's cooking in git.git (Nov 2012, #02; Fri, 9)

[Jeff King]

On Fri, Nov 09, 2012 at 12:27:35PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > I have not been pushing the individual topic branches to make life
> > easier for people who usually just track Junio's kernel.org repository,
> > and would not welcome suddenly getting a hundred extra remote branches.
> > I can make them public if it makes life easier for people, but it may
> > not be worth it at this point, with Junio returning soon.
> 
> What we should have arranged was to have https://github.com/git/git
> (which is not even owned by me, but I asked somebody at GitHub to
> assign me a write privilege) writable by the interim maintainer, so
> that normal people would keep pulling from there, while the interim
> maintainer can choose to publish broken-out branches to his
> repository.

Yes, I have write access to that repository, too, but I intentionally
held off from updating it out of a sense of nervousness. I figured if I
screwed up anything too badly, people who were clued-in enough to switch
to pulling from my repository would be clued-in enough to rebase across
any too-horrible mistake I made. ;)

I think if we do this again, I will make the same split you do (git/git
for integration branches, peff/git as a mirror of my private repo).

> And it is not too late to do so; from the look of your "What's
> cooking", you are doing pretty well ;-).

Any fool can merge topics to master. The real test will be how many
regressions people report in the next two weeks. :)

By the way, I did not touch 'maint' at all while you were gone. I don't
know what your usual method is for keeping track of maint-worthy topics
after they have gone to master. The usual "what's cooking" workflow
keeps track of things going to master, but no more; I'd guess you
probably just merge to maint when you delete them from last cycle's
"graduated to master" list.

I just let them stew in master for a bit longer, and we can easily find
and merge them with "git branch --no-merged maint | grep maint".

-Peff

Re: What's cooking in git.git (Nov 2012, #02; Fri, 9)

[Felipe Contreras]

On Sat, Nov 10, 2012 at 1:33 AM, Jeff King <peff@peff.net> wrote:
> On Sat, Nov 10, 2012 at 12:21:48AM +0100, Felipe Contreras wrote:
>
>> > * fc/fast-export-fixes (2012-11-08) 14 commits
>> >  - fast-export: don't handle uninteresting refs
>> >  - fast-export: make sure updated refs get updated
>> >  - fast-export: fix comparison in tests
>> >  - fast-export: trivial cleanup
>> >  - remote-testgit: make clear the 'done' feature
>> >  - remote-testgit: report success after an import
>> >  - remote-testgit: exercise more features
>> >  - remote-testgit: cleanup tests
>> >  - remote-testgit: remove irrelevant test
>> >  - remote-testgit: get rid of non-local functionality
>> >  - Add new simplified git-remote-testgit
>> >  - Rename git-remote-testgit to git-remote-testpy
>> >  - remote-testgit: fix direction of marks
>> >  - fast-export: avoid importing blob marks
>> >
>> >  Improvements to fix fast-export bugs, including how refs pointing to
>> >  already-seen commits are handled. An earlier 4-commit version of this
>> >  series looked good to me, but this much-expanded version has not seen
>> >  any comments.
>> >
>> >  Needs review.
>>
>> I can send the previous 4-commit version if needed, the only thing
>> that changed is the commit messages.
>
> In the actual code, perhaps, but aren't there significant changes to the
> git-remote-testgit infrastructure that were not originally present? That
> could use some review.
>
> I also seem to recall that the tests in this version rely on the presence of bash;
> don't we still need to mark the tests with a prerequisite?

I meant in the 4-commits.

>> > * fc/completion-test-simplification (2012-10-29) 2 commits
>> >  - completion: simplify __gitcomp test helper
>> >  - completion: refactor __gitcomp related tests
>> >
>> >  Clean up completion tests.
>> >
>> >  There were some comments on the list.
>> >
>> >  Expecting a re-roll.
>>
>> The second patch I can re-roll, but the first patch needs some
>> external input. My preference is that tests should also be simple and
>> maintainable, SZEDER's preference is that tests are better being
>> explicit and verbose (even if harder to maintain) to minimize possible
>> issues in the tests.
>
> I think it is better to keep the tests simple and maintainable. If there
> are multiple ways to do things and they all need testing, then that
> should be clear from the tests, not done haphazardly because some tests
> happen to use a different way of doing things.

Good, that's what my first patch does; no functional changes, just
refactor code into a single function.

> I seem to recall there was a one-liner fix that needed to be rolled in,
> which is why I held it out of next.

Yes, that I can reroll.

Cheers.

-- 
Felipe Contreras

Re: What's cooking in git.git (Nov 2012, #02; Fri, 9)

[SZEDER Gábor]

Hi,

On Fri, Nov 09, 2012 at 07:33:31PM -0500, Jeff King wrote:
> On Sat, Nov 10, 2012 at 12:21:48AM +0100, Felipe Contreras wrote:
> > > * fc/completion-test-simplification (2012-10-29) 2 commits
> > >  - completion: simplify __gitcomp test helper
> > >  - completion: refactor __gitcomp related tests
> > >
> > >  Clean up completion tests.
> > >
> > >  There were some comments on the list.
> > >
> > >  Expecting a re-roll.
> > 
> > The second patch I can re-roll, but the first patch needs some
> > external input. My preference is that tests should also be simple and
> > maintainable, SZEDER's preference is that tests are better being
> > explicit and verbose (even if harder to maintain) to minimize possible
> > issues in the tests.
> 
> I think it is better to keep the tests simple and maintainable.

Maintainable?  There is nothing to maintain here.  Users' completion
scripts depend on __gitcomp(), so its behavior shouldn't be changed.
It can only be extended by a fifth parameter or by quoting words when
necessary, but these future changes must not alter the current
behavior checked by these tests, therefore even then these tests must
be left intact.

Simple?  Currently you only need to look at __gitcomp() and the test
itself to understand what's going on.  With this series you'll also
need to look at test_gitcomp(), figure out what its parameters are
supposed to mean, and possibly get puzzled on the way why __gitcomp()
is now seemingly called with only one parameter.

So, I don't see much benefit in this series (except the part to use
print_comp instead of "change IFS && echo", but that's already done in
this patch:
http://article.gmane.org/gmane.comp.version-control.git/207927).

OTOH, this series has some serious drawbacks.

It makes debugging more difficult.  While working on the quoting
issues I managed to break completion tests many-many times lately.  In
normal tests I could add a few debugging instructions to the failed
test to find out where the breakage lies, without affecting other
tests.  However, if the failed test uses the test_completion() helper,
then I have to add debugging instructions to test_completion() itself,
too.  This is bad, because many tests use this helper function and are
therefore affected by the debugging instructions, producing truckloads
of output making it difficult to dig out the relevant parts, or, worse
yet, causing breakages in other tests.  With this series the same
difficulties will come to __gitcomp() tests, too.

It can also encourage writing bad tests, similar to those that managed
to cram many test_completion() lines into a single tests, giving me
headaches to figure out what went wrong this time.


Best,
Gábor

Re: What's cooking in git.git (Nov 2012, #02; Fri, 9)

[Paul Fox]

kalle olavi niemitalo wrote:
 > Jeff King <peff@peff.net> writes:
 > 
 > >  Comments welcome from people using unusual editors (e.g., a script that
 > >  starts an editor in another window then blocks, waiting for the user to
 > >  finish).
 > 
 > I often run a shell in Emacs in X, then start git commit in that
 > shell.  $EDITOR is emacsclient --current-frame, which asks the
 > existing Emacs instance to load the file and waits until I press
 > C-x # in Emacs to mark the file done.  If I want to abort the
 > commit, it is most intuitive to return to the *Shell* buffer in
 > Emacs and press C-c C-c (comint-interrupt-subjob) to send SIGINT
 > to git from there.  (I see that "an empty message aborts the
 > commit", and indeed it does, but well, I prefer not to trust such
 > a feature if I can instead just interrupt the thing.)
 > 
 > With pf/editor-ignore-sigint, C-c C-c in the *Shell* buffer kills
 > neither git nor the emacsclient started by git.  This is not good.
 > SIGQUIT from C-c C-\ (comint-quit-subjob) still works though.

when i implemented the change, i wondered if some twisted emacs
workflow would be an issue. ;-)  and i almost blocked SIGQUIT as
well -- the two programs i looked at for precedent (CVS and MH) both
block both SIGQUIT and SIGINT when spawning an editor.

but since emacs users must have dealt with CVS for a long time before
dealing with git, how might they have done so?

the existing git behavior is bad for non-emacs users, and git itself
provides an abort-the-operation mechanism (i.e., writing an empty
message), so i'm not convinced your use case invalidates the new
behavior.  (though it might spotlight a need for this being prominent
in release notes.)

paul
=---------------------
 paul fox, pgf@foxharp.boston.ma.us (arlington, ma, where it's 40.6 degrees)

Re: What's cooking in git.git (Nov 2012, #02; Fri, 9)

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Nov 09, 2012 at 12:27:35PM -0800, Junio C Hamano wrote:
>
>> What we should have arranged was to have https://github.com/git/git
>> (which is not even owned by me, but I asked somebody at GitHub to
>> assign me a write privilege) writable by the interim maintainer, so
>> that normal people would keep pulling from there, while the interim
>> maintainer can choose to publish broken-out branches to his
>> repository.
>
> Yes, I have write access to that repository, too, but I intentionally
> held off from updating it out of a sense of nervousness. I figured if I
> screwed up anything too badly, people who were clued-in enough to switch
> to pulling from my repository would be clued-in enough to rebase across
> any too-horrible mistake I made. ;)

That "nervousness" reminds me of myself when I took over.  Before I
could ask for a few weeks of practice period, Linus arranged to have
folks at k.org to chown the authoritative location to me, declaring
"no practice period; it's already done and it's all yours".

And I made at least one mistake pushing 'master' with one commit
rewound too much (corrected by pushing an extra merge).  Luckily,
the world did not end ;-).

> I think if we do this again, I will make the same split you do (git/git
> for integration branches, peff/git as a mirror of my private repo).

I am fairly sure I'll have to ask you (or somebody else) again next
year around late September.

>> And it is not too late to do so; from the look of your "What's
>> cooking", you are doing pretty well ;-).
>
> Any fool can merge topics to master. The real test will be how many
> regressions people report in the next two weeks. :)

I agree that the actual merging to 'master' is mechanical with the
procedure built around Meta/Reintegrate.  Important decisions are
made before you merge a topic to 'next' and mark topics as "Will
merge to 'master'."  My comment was about that, and your responses
to the list messages.

> By the way, I did not touch 'maint' at all while you were gone. I don't
> know what your usual method is for keeping track of maint-worthy topics
> after they have gone to master. The usual "what's cooking" workflow
> keeps track of things going to master, but no more; I'd guess you
> probably just merge to maint when you delete them from last cycle's
> "graduated to master" list.

That is done by eyeballing output from Meta/GRADUATED (which spits
out something that could be fed to shell, but I do not fully trust
its logic, and always eyeball them before I prepare the temporary
file to feed Meta/Reintegrate to update 'maint').

Re: What's cooking in git.git (Nov 2012, #02; Fri, 9)

[Felipe Contreras]

On Sat, Nov 10, 2012 at 1:32 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
> Hi,
>
> On Fri, Nov 09, 2012 at 07:33:31PM -0500, Jeff King wrote:
>> On Sat, Nov 10, 2012 at 12:21:48AM +0100, Felipe Contreras wrote:
>> > > * fc/completion-test-simplification (2012-10-29) 2 commits
>> > >  - completion: simplify __gitcomp test helper
>> > >  - completion: refactor __gitcomp related tests
>> > >
>> > >  Clean up completion tests.
>> > >
>> > >  There were some comments on the list.
>> > >
>> > >  Expecting a re-roll.
>> >
>> > The second patch I can re-roll, but the first patch needs some
>> > external input. My preference is that tests should also be simple and
>> > maintainable, SZEDER's preference is that tests are better being
>> > explicit and verbose (even if harder to maintain) to minimize possible
>> > issues in the tests.
>>
>> I think it is better to keep the tests simple and maintainable.
>
> Maintainable?  There is nothing to maintain here.  Users' completion
> scripts depend on __gitcomp(), so its behavior shouldn't be changed.
> It can only be extended by a fifth parameter or by quoting words when
> necessary, but these future changes must not alter the current
> behavior checked by these tests, therefore even then these tests must
> be left intact.

I disagree. If we add a new parameter to __gitcomp(), and we need to
add a new parameter to test_gitcomp(), so be it. Yes, we might change
the behavior of the other tests, but that's what reviews are for: to
make sure we don't alter other behavior by mistake. That's what we do
for code, and that what we should do for tests.

But in this particular case nothing would need to change because
test_gitcomp() would pass whatever arguments it receives, being four,
or five, or twenty. So this is not a concern, maybe some other kind of
change, but not this.

Compare this:
http://article.gmane.org/gmane.comp.version-control.git/208168

To this:
http://article.gmane.org/gmane.comp.version-control.git/207927

If we ever need to make changes to the __gitcomp tests, a small change
is better than a big change; IOW: the test code would be more
maintainable.

Even more, the end result is much less code: less code = more maintainability.

> Simple?  Currently you only need to look at __gitcomp() and the test
> itself to understand what's going on.  With this series you'll also
> need to look at test_gitcomp(), figure out what its parameters are
> supposed to mean, and possibly get puzzled on the way why __gitcomp()
> is now seemingly called with only one parameter.

Maybe it's easier for you to understand, but certainly not for other
people: 'declare -a COMPREPLY'? cur? print_comp? What does that even
mean? Chances are most people don't even know what __gitcomp is.
Fortunately they don't have to dig too deep to find all those things
out, but they can do the same for test_gitcomp().

It might make sense to add some comment on top of test_gitcomp to
explain the arguments and the input to make things easier, but that
would only make it more readable, not less, than the current
situation, because right now you would have to add a similar comment
to each and every block of code that calls __gitcomp.

Functions make our life easier.

> So, I don't see much benefit in this series (except the part to use
> print_comp instead of "change IFS && echo", but that's already done in
> this patch:
> http://article.gmane.org/gmane.comp.version-control.git/207927).

Yes, and that version is much bigger than this:
http://article.gmane.org/gmane.comp.version-control.git/208168

Code that allows the later patch is more maintainable.

> OTOH, this series has some serious drawbacks.
>
> It makes debugging more difficult.  While working on the quoting
> issues I managed to break completion tests many-many times lately.  In
> normal tests I could add a few debugging instructions to the failed
> test to find out where the breakage lies, without affecting other
> tests.  However, if the failed test uses the test_completion() helper,
> then I have to add debugging instructions to test_completion() itself,
> too.  This is bad, because many tests use this helper function and are
> therefore affected by the debugging instructions, producing truckloads
> of output making it difficult to dig out the relevant parts, or, worse
> yet, causing breakages in other tests.  With this series the same
> difficulties will come to __gitcomp() tests, too.

What I do is copy the function to test_gitcomp2() and add the
debugging there, and only call it from the places where I want the
debugging. I don't think this is an issue.

In fact, I think it's an advantage; sometimes that's exactly what you
want, to add the debugging for everything.

> It can also encourage writing bad tests, similar to those that managed
> to cram many test_completion() lines into a single tests, giving me
> headaches to figure out what went wrong this time.

That's policy. We can decide what each test-case contains right here,
on the mailing list. There's no need to have verbose code to slightly
hint a policy.

Cheers.

-- 
Felipe Contreras

Re: What's cooking in git.git (Nov 2012, #02; Fri, 9)

[Kalle Olavi Niemitalo]

Paul Fox <pgf@foxharp.boston.ma.us> writes:

> when i implemented the change, i wondered if some twisted emacs
> workflow would be an issue. ;-)  and i almost blocked SIGQUIT as
> well -- the two programs i looked at for precedent (CVS and MH) both
> block both SIGQUIT and SIGINT when spawning an editor.
>
> but since emacs users must have dealt with CVS for a long time before
> dealing with git, how might they have done so?

I think I usually ran CVS via vc.el, which prompts for a commit
message in Emacs before it runs cvs commit.  So CVS did not need
to run $EDITOR.

I just tried emacsclient with CVS 1.12.13-MirDebian-9, and it
behaves somewhat differently from Git with pf/editor-ignore-sigint.
When I tell Emacs to send SIGINT to the *Shell* buffer, CVS prompts:

cvs commit: warning: editor session failed

Log message unchanged or not specified
a)bort, c)ontinue, e)dit, !)reuse this message unchanged for remaining dirs
Action: (continue) 

and then I can choose to abort.

With strace, it looks like CVS sets SIG_IGN as the handler of
SIGINT and SIGQUIT only in the parent process after forking, not
in the child process that executes the editor.

CVS also temporarily blocks signals by calling sigprocmask, but
it undoes that before it forks or waits for the child process.

Re: What's cooking in git.git (Nov 2012, #02; Fri, 9)

[Junio C Hamano]

SZEDER Gábor <szeder@ira.uka.de> writes:

>> I think it is better to keep the tests simple and maintainable.
>
> Maintainable?  There is nothing to maintain here....
> ...
> OTOH, this series has some serious drawbacks.
>
> It makes debugging more difficult....

Are these referring to the same aspect of the series?  The concern
you described about debuggability matches my impression IIRC back
when I took a look at the series, which I would count as a large
part of keeping tests maintainable.

But you may be referring to something different (sorry, not on my
primary machine yet).

Re: What's cooking in git.git (Nov 2012, #02; Fri, 9)

[Andreas Schwab]

Kalle Olavi Niemitalo <kon@iki.fi> writes:

> With strace, it looks like CVS sets SIG_IGN as the handler of
> SIGINT and SIGQUIT only in the parent process after forking, not
> in the child process that executes the editor.
>
> CVS also temporarily blocks signals by calling sigprocmask, but
> it undoes that before it forks or waits for the child process.

This emulates what system(3) does.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: What's cooking in git.git (Nov 2012, #02; Fri, 9)

[Paul Fox]

kalle olavi niemitalo wrote:
 > Paul Fox <pgf@foxharp.boston.ma.us> writes:
 > 
 > > when i implemented the change, i wondered if some twisted emacs
 > > workflow would be an issue. ;-)  and i almost blocked SIGQUIT as
 > > well -- the two programs i looked at for precedent (CVS and MH) both
 > > block both SIGQUIT and SIGINT when spawning an editor.
 > >
 > > but since emacs users must have dealt with CVS for a long time before
 > > dealing with git, how might they have done so?
 > 
 > I think I usually ran CVS via vc.el, which prompts for a commit
 > message in Emacs before it runs cvs commit.  So CVS did not need
 > to run $EDITOR.
 > 
 > I just tried emacsclient with CVS 1.12.13-MirDebian-9, and it
 > behaves somewhat differently from Git with pf/editor-ignore-sigint.
 > When I tell Emacs to send SIGINT to the *Shell* buffer, CVS prompts:
 > 
 > cvs commit: warning: editor session failed
 > 
 > Log message unchanged or not specified
 > a)bort, c)ontinue, e)dit, !)reuse this message unchanged for remaining dirs
 > Action: (continue) 

you're sending SIGINT to the cvs commit command, and that causes the
editor to die right away?  that's surprising.  i can replicate your
described behavior by setting $VISUAL to a script that just sleeps, and
sending SIGTERM to the cvs commit process.  but not by sending SIGINT.

well, i'm not sure what to say.  there's a real problem when using the
current code and traditional editors.  i thought that the patch in
pf/editor-ignore-sigint reflected standard practice, and indeed it
accomplishes exactly the right thing with those editors.  you've shown
a particular work flow involving emacsclient that won't work anymore
with the change made, though there are workarounds.  perhaps there's
something the other editors themselves should be doing differently,
but i don't know what that might be.

paul

 > 
 > and then I can choose to abort.
 > 
 > With strace, it looks like CVS sets SIG_IGN as the handler of
 > SIGINT and SIGQUIT only in the parent process after forking, not
 > in the child process that executes the editor.
 > 
 > CVS also temporarily blocks signals by calling sigprocmask, but
 > it undoes that before it forks or waits for the child process.

=---------------------
 paul fox, pgf@foxharp.boston.ma.us (arlington, ma, where it's 45.5 degrees)

Re: What's cooking in git.git (Nov 2012, #02; Fri, 9)

[Kalle Olavi Niemitalo]

Paul Fox <pgf@foxharp.boston.ma.us> writes:

> you're sending SIGINT to the cvs commit command, and that causes the
> editor to die right away?

That's right.  It is not a quirk of shell-mode in Emacs, because
I get the same result with ^C in xterm too.

% EDITOR="$HOME/prefix/x86_64-unknown-linux-gnu/bin/emacsclient --current-frame"
% export EDITOR
% cvs commit BUGIT
Waiting for Emacs...^Ccvs commit: warning: editor session failed

Log message unchanged or not specified
a)bort, c)ontinue, e)dit, !)reuse this message unchanged for remaining dirs
Action: (continue) a
cvs [commit aborted]: aborted by user
% 

While cvs was waiting from emacsclient:

% cat /proc/2030/stat
2030 (cvs) S 1849 2030 1849 34816 2030 4202496 598 0 0 0 0 0 0 0 20 0 1 0 94752537 34254848 410 18446744073709551615 140168182550528 140168183348316 140737407935424 140737407931680 140168163193950 0 0 6 20513 0 0 0 17 2 0 0 0 0 0
% grep 'Name\|Pid\|Sig' /proc/2030/status
Name:	cvs
Pid:	2030
PPid:	1849
TracerPid:	0
SigQ:	0/28998
SigPnd:	0000000000000000
SigBlk:	0000000000000000
SigIgn:	0000000000000006
SigCgt:	0000000180005021
% cat /proc/2031/stat
2031 (emacsclient) S 2030 2030 1849 34816 2030 4202496 155 0 0 0 0 0 0 0 20 0 1 0 94752538 4169728 81 18446744073709551615 4194304 4210620 140735996104016 140735996095456 140664960886018 0 0 0 0 0 0 0 17 1 0 0 0 0 0
% grep 'Name\|Pid\|Sig' /proc/2031/status
Name:	emacsclient
Pid:	2031
PPid:	2030
TracerPid:	0
SigQ:	0/28998
SigPnd:	0000000000000000
SigBlk:	0000000000000000
SigIgn:	0000000000000000
SigCgt:	0000000000000000
%

which I interpret to mean both processes were in process group
2030, the cvs process ignored SIGINT and SIGQUIT, the emacsclient
process neither ignored nor handled any signals, and neither
process blocked any signals (not even SIGCHLD as system(3) would).
When ^C in the terminal sent SIGINT to the process group, it
terminated the emacsclient process only.

If git did the same thing as cvs here, i.e. ignore the signals in
the parent process only and check the exit status of the editor,
I think that would be OK.

Re: What's cooking in git.git (Nov 2012, #02; Fri, 9)

[Andreas Schwab]

Kalle Olavi Niemitalo <kon@iki.fi> writes:

> and neither process blocked any signals (not even SIGCHLD as system(3)
> would).

If you don't have a SIGCHLD handler it won't matter anyway.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: What's cooking in git.git (Nov 2012, #02; Fri, 9)

[Jeff King]

On Sun, Nov 11, 2012 at 09:02:48AM +0200, Kalle Olavi Niemitalo wrote:

> If git did the same thing as cvs here, i.e. ignore the signals in
> the parent process only and check the exit status of the editor,
> I think that would be OK.

Silly me. When I thought through the impact of Paul's patch, I knew that
we would notice signal death of the editor. But I totally forgot to
consider that the blocked signal is inherited by the child process. I
think we just need to move the signal() call to after we've forked. Like
this (on top of Paul's patch):

diff --git a/editor.c b/editor.c
index 3ca361b..0ed23ce 100644
--- a/editor.c
+++ b/editor.c
@@ -38,11 +38,20 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en
 
 	if (strcmp(editor, ":")) {
 		const char *args[] = { editor, path, NULL };
+		struct child_process p;
 		int ret;
 
+		memset(&p, 0, sizeof(p));
+		p.argv = args;
+		p.env = env;
+		p.use_shell = 1;
+		if (start_command(&p) < 0)
+			return error("unable to start editor '%s'", editor);
+
 		sigchain_push(SIGINT, SIG_IGN);
-		ret = run_command_v_opt_cd_env(args, RUN_USING_SHELL, NULL, env);
+		ret = finish_command(&p);
 		sigchain_pop(SIGINT);
+
 		if (ret)
 			return error("There was a problem with the editor '%s'.",
 					editor);

Note that this will give you a slightly verbose message from git.
Potentially we could notice editor death due to SIGINT and suppress the
message, under the assumption that the user hit ^C and does not need to
be told.

-Peff

[PATCH 0/5] ignore SIGINT while editor runs

[Jeff King]

On Sun, Nov 11, 2012 at 10:48:46AM -0500, Jeff King wrote:

> Silly me. When I thought through the impact of Paul's patch, I knew that
> we would notice signal death of the editor. But I totally forgot to
> consider that the blocked signal is inherited by the child process. I
> think we just need to move the signal() call to after we've forked. Like
> this (on top of Paul's patch):
> [...]
> Note that this will give you a slightly verbose message from git.
> Potentially we could notice editor death due to SIGINT and suppress the
> message, under the assumption that the user hit ^C and does not need to
> be told.

Here's a series that I think should resolve the situation for everybody.

  [1/5]: launch_editor: refactor to use start/finish_command

The cleanup I sent out a few minutes ago.

  [2/5]: launch_editor: ignore SIGINT while the editor has control

Paul's patch rebased on my 1/5.

  [3/5]: run-command: drop silent_exec_failure arg from wait_or_whine
  [4/5]: run-command: do not warn about child death by SIGINT
  [5/5]: launch_editor: propagate SIGINT from editor to git

Act more like current git when the editor dies from SIGINT.

-Peff

[PATCH 1/5] launch_editor: refactor to use start/finish_command

[Jeff King]

The launch_editor function uses the convenient run_command_*
interface. Let's use the more flexible start_command and
finish_command functions, which will let us manipulate the
parent state while we're waiting for the child to finish.

Signed-off-by: Jeff King <peff@peff.net>
---
 editor.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/editor.c b/editor.c
index d834003..842f782 100644
--- a/editor.c
+++ b/editor.c
@@ -37,8 +37,16 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en
 
 	if (strcmp(editor, ":")) {
 		const char *args[] = { editor, path, NULL };
+		struct child_process p;
 
-		if (run_command_v_opt_cd_env(args, RUN_USING_SHELL, NULL, env))
+		memset(&p, 0, sizeof(p));
+		p.argv = args;
+		p.env = env;
+		p.use_shell = 1;
+		if (start_command(&p) < 0)
+			return error("unable to start editor '%s'", editor);
+
+		if (finish_command(&p))
 			return error("There was a problem with the editor '%s'.",
 					editor);
 	}
-- 
1.8.0.207.gdf2154c

[PATCH 2/5] launch_editor: ignore SIGINT while the editor has control

[Paul Fox]

The user's editor likely catches SIGINT (ctrl-C).  but if
the user spawns a command from the editor and uses ctrl-C to
kill that command, the SIGINT will likely also kill git
itself (depending on the editor, this can leave the terminal
in an unusable state).

Signed-off-by: Paul Fox <pgf@foxharp.boston.ma.us>
Signed-off-by: Jeff King <peff@peff.net>
---
 editor.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/editor.c b/editor.c
index 842f782..28aae85 100644
--- a/editor.c
+++ b/editor.c
@@ -1,6 +1,7 @@
 #include "cache.h"
 #include "strbuf.h"
 #include "run-command.h"
+#include "sigchain.h"
 
 #ifndef DEFAULT_EDITOR
 #define DEFAULT_EDITOR "vi"
@@ -38,6 +39,7 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en
 	if (strcmp(editor, ":")) {
 		const char *args[] = { editor, path, NULL };
 		struct child_process p;
+		int ret;
 
 		memset(&p, 0, sizeof(p));
 		p.argv = args;
@@ -46,7 +48,10 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en
 		if (start_command(&p) < 0)
 			return error("unable to start editor '%s'", editor);
 
-		if (finish_command(&p))
+		sigchain_push(SIGINT, SIG_IGN);
+		ret = finish_command(&p);
+		sigchain_pop(SIGINT);
+		if (ret)
 			return error("There was a problem with the editor '%s'.",
 					editor);
 	}
-- 
1.8.0.207.gdf2154c

[PATCH 3/5] run-command: drop silent_exec_failure arg from wait_or_whine

[Jeff King]

We do not actually use this parameter; instead we complain
from the child itself (for fork/exec) or from start_command
(if we are using spawn on Windows).

Signed-off-by: Jeff King <peff@peff.net>
---
Just a cleanup I noticed while in the area.

 run-command.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/run-command.c b/run-command.c
index 3b982e4..3aae270 100644
--- a/run-command.c
+++ b/run-command.c
@@ -226,7 +226,7 @@ static inline void set_cloexec(int fd)
 		fcntl(fd, F_SETFD, flags | FD_CLOEXEC);
 }
 
-static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure)
+static int wait_or_whine(pid_t pid, const char *argv0)
 {
 	int status, code = -1;
 	pid_t waiting;
@@ -432,8 +432,7 @@ fail_pipe:
 		 * At this point we know that fork() succeeded, but execvp()
 		 * failed. Errors have been reported to our stderr.
 		 */
-		wait_or_whine(cmd->pid, cmd->argv[0],
-			      cmd->silent_exec_failure);
+		wait_or_whine(cmd->pid, cmd->argv[0]);
 		failed_errno = errno;
 		cmd->pid = -1;
 	}
@@ -538,7 +537,7 @@ fail_pipe:
 
 int finish_command(struct child_process *cmd)
 {
-	return wait_or_whine(cmd->pid, cmd->argv[0], cmd->silent_exec_failure);
+	return wait_or_whine(cmd->pid, cmd->argv[0]);
 }
 
 int run_command(struct child_process *cmd)
-- 
1.8.0.207.gdf2154c

[PATCH 4/5] run-command: do not warn about child death by SIGINT

[Jeff King]

SIGINT is not generally an interesting signal to the user,
since it is typically caused by them hitting "^C" or
otherwise telling their terminal to send the signal.

Signed-off-by: Jeff King <peff@peff.net>
---
I thought about making this an optional parameter for run-command, but
it seems like everybody would want this (and most callsites will
complain about a failed command separately, anyway, so it is not like
errors would go unnoticed).

 run-command.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/run-command.c b/run-command.c
index 3aae270..0527c61 100644
--- a/run-command.c
+++ b/run-command.c
@@ -242,7 +242,8 @@ static int wait_or_whine(pid_t pid, const char *argv0)
 		error("waitpid is confused (%s)", argv0);
 	} else if (WIFSIGNALED(status)) {
 		code = WTERMSIG(status);
-		error("%s died of signal %d", argv0, code);
+		if (code != SIGINT)
+			error("%s died of signal %d", argv0, code);
 		/*
 		 * This return value is chosen so that code & 0xff
 		 * mimics the exit code that a POSIX shell would report for
-- 
1.8.0.207.gdf2154c

[PATCH 5/5] launch_editor: propagate SIGINT from editor to git

[Jeff King]

We block SIGINT while the editor runs so that git is not
killed accidentally by a stray "^C" meant for the editor or
its subprocesses. This works because most editors ignore
SIGINT.

However, some editor wrappers, like emacsclient, expect to
die due to ^C. We detect the signal death in the editor and
properly exit, but not before writing a useless error
message to stderr. Instead, let's notice when the editor was
killed by SIGINT and just raise the signal on ourselves.
This skips the message and looks to our parent like we
received SIGINT ourselves.

The end effect is that if the user's editor ignores SIGINT,
we will, too. And if it does not, then we will behave as if
we did not ignore it. That should make all users happy.

Note that in the off chance that another part of git has
ignored SIGINT while calling launch_editor, we will still
properly detect and propagate the failed return code from
the editor (i.e., the worst case is that we generate the
useless error, not fail to notice the editor's death).

Signed-off-by: Jeff King <peff@peff.net>
---
 editor.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/editor.c b/editor.c
index 28aae85..1275527 100644
--- a/editor.c
+++ b/editor.c
@@ -51,6 +51,8 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en
 		sigchain_push(SIGINT, SIG_IGN);
 		ret = finish_command(&p);
 		sigchain_pop(SIGINT);
+		if (WIFSIGNALED(ret) && WTERMSIG(ret) == SIGINT)
+			raise(SIGINT);
 		if (ret)
 			return error("There was a problem with the editor '%s'.",
 					editor);
-- 
1.8.0.207.gdf2154c

[PATCH 2/5] launch_editor: ignore SIGINT while the editor has control

[Jeff King]

From: Paul Fox <pgf@foxharp.boston.ma.us>

The user's editor likely catches SIGINT (ctrl-C).  but if
the user spawns a command from the editor and uses ctrl-C to
kill that command, the SIGINT will likely also kill git
itself (depending on the editor, this can leave the terminal
in an unusable state).

Signed-off-by: Paul Fox <pgf@foxharp.boston.ma.us>
Signed-off-by: Jeff King <peff@peff.net>
---
Whoops, my original sending of this actually had Paul in the email's
From field, not in the pseudo-header of the commit. Apologies if you
receive an extra forged copy.

 editor.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/editor.c b/editor.c
index 842f782..28aae85 100644
--- a/editor.c
+++ b/editor.c
@@ -1,6 +1,7 @@
 #include "cache.h"
 #include "strbuf.h"
 #include "run-command.h"
+#include "sigchain.h"
 
 #ifndef DEFAULT_EDITOR
 #define DEFAULT_EDITOR "vi"
@@ -38,6 +39,7 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en
 	if (strcmp(editor, ":")) {
 		const char *args[] = { editor, path, NULL };
 		struct child_process p;
+		int ret;
 
 		memset(&p, 0, sizeof(p));
 		p.argv = args;
@@ -46,7 +48,10 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en
 		if (start_command(&p) < 0)
 			return error("unable to start editor '%s'", editor);
 
-		if (finish_command(&p))
+		sigchain_push(SIGINT, SIG_IGN);
+		ret = finish_command(&p);
+		sigchain_pop(SIGINT);
+		if (ret)
 			return error("There was a problem with the editor '%s'.",
 					editor);
 	}
-- 
1.8.0.207.gdf2154c

Re: [PATCH 3/5] run-command: drop silent_exec_failure arg from wait_or_whine

[Felipe Contreras]

On Sun, Nov 11, 2012 at 5:55 PM, Jeff King <peff@peff.net> wrote:
> We do not actually use this parameter; instead we complain
> from the child itself (for fork/exec) or from start_command
> (if we are using spawn on Windows).

FWIW I noticed the same while looking at that code. Looks good to me.

-- 
Felipe Contreras

Re: [PATCH 0/5] ignore SIGINT while editor runs

[Paul Fox]

jeff wrote:
 > On Sun, Nov 11, 2012 at 10:48:46AM -0500, Jeff King wrote:
 > 
 > > Silly me. When I thought through the impact of Paul's patch, I knew that
 > > we would notice signal death of the editor. But I totally forgot to
 > > consider that the blocked signal is inherited by the child process. I
 > > think we just need to move the signal() call to after we've forked. Like
 > > this (on top of Paul's patch):
 > > [...]
 > > Note that this will give you a slightly verbose message from git.
 > > Potentially we could notice editor death due to SIGINT and suppress the
 > > message, under the assumption that the user hit ^C and does not need to
 > > be told.
 > 
 > Here's a series that I think should resolve the situation for everybody.

thanks!  i've tested -- this certainly scratches my initial itch.

ack,
paul

 > 
 >   [1/5]: launch_editor: refactor to use start/finish_command
 > 
 > The cleanup I sent out a few minutes ago.
 > 
 >   [2/5]: launch_editor: ignore SIGINT while the editor has control
 > 
 > Paul's patch rebased on my 1/5.
 > 
 >   [3/5]: run-command: drop silent_exec_failure arg from wait_or_whine
 >   [4/5]: run-command: do not warn about child death by SIGINT
 >   [5/5]: launch_editor: propagate SIGINT from editor to git
 > 
 > Act more like current git when the editor dies from SIGINT.
 > 
 > -Peff
 > --
 > To unsubscribe from this list: send the line "unsubscribe git" in
 > the body of a message to majordomo@vger.kernel.org
 > More majordomo info at  http://vger.kernel.org/majordomo-info.html

=---------------------
 paul fox, pgf@foxharp.boston.ma.us (arlington, ma, where it's 56.3 degrees)

Re: [PATCH 0/5] ignore SIGINT while editor runs

[Krzysztof Mazur]

On Sun, Nov 11, 2012 at 11:31:00AM -0500, Jeff King wrote:
> 
> Here's a series that I think should resolve the situation for everybody.
> 
>   [1/5]: launch_editor: refactor to use start/finish_command
> 
> The cleanup I sent out a few minutes ago.
> 
>   [2/5]: launch_editor: ignore SIGINT while the editor has control
> 
> Paul's patch rebased on my 1/5.
> 
>   [3/5]: run-command: drop silent_exec_failure arg from wait_or_whine
>   [4/5]: run-command: do not warn about child death by SIGINT
>   [5/5]: launch_editor: propagate SIGINT from editor to git
> 
> Act more like current git when the editor dies from SIGINT.
> 

Looks ok, but what about SIGQUIT? Some editors like GNU ed (0.4 and 1.6)
ignore SIGQUIT, and after SIGQUIT git dies, but editor is still running.
After pressing any key ed receives -EIO and prints "stdin: Input/output
error". GNU ed 1.6 then exits, but ed 0.4 prints this error forever.
Maybe git should kill the editor in such case?

Krzysiek

Re: [PATCH 5/5] launch_editor: propagate SIGINT from editor to git

[Johannes Sixt]

Am 11.11.2012 17:57, schrieb Jeff King:
> @@ -51,6 +51,8 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en
>  		sigchain_push(SIGINT, SIG_IGN);
>  		ret = finish_command(&p);
>  		sigchain_pop(SIGINT);
> +		if (WIFSIGNALED(ret) && WTERMSIG(ret) == SIGINT)
> +			raise(SIGINT);

The return value of finish_command() is already a digested version of
waitpid's status value. According to
Documentation/technical/api-run-command.txt:

. If the program terminated due to a signal, then the return value is
the signal number - 128, ...

the correct condition would be

		if (ret == SIGINT - 128)

-- Hannes

Re: [PATCH 0/5] ignore SIGINT while editor runs

[Paul Fox]

krzysztof wrote:
 > On Sun, Nov 11, 2012 at 11:31:00AM -0500, Jeff King wrote:
 > > 
 > > Here's a series that I think should resolve the situation for everybody.
 > > 
 > >   [1/5]: launch_editor: refactor to use start/finish_command
 > > 
 > > The cleanup I sent out a few minutes ago.
 > > 
 > >   [2/5]: launch_editor: ignore SIGINT while the editor has control
 > > 
 > > Paul's patch rebased on my 1/5.
 > > 
 > >   [3/5]: run-command: drop silent_exec_failure arg from wait_or_whine
 > >   [4/5]: run-command: do not warn about child death by SIGINT
 > >   [5/5]: launch_editor: propagate SIGINT from editor to git
 > > 
 > > Act more like current git when the editor dies from SIGINT.
 > > 
 > 
 > Looks ok, but what about SIGQUIT? Some editors like GNU ed (0.4 and 1.6)
 > ignore SIGQUIT, and after SIGQUIT git dies, but editor is still running.
 > After pressing any key ed receives -EIO and prints "stdin: Input/output
 > error". GNU ed 1.6 then exits, but ed 0.4 prints this error forever.
 > Maybe git should kill the editor in such case?

there's certainly lots of precedent for treating SIGINT and SIGQUIT
the same.  but there's also some merit to saying that if the user
knows to send SIGQUIT instead of SIGINT, they may well have a reason. 
(after all, if we always treat them the same, there's no point in
having both.)

the em editor (linus' microemacs) behaves as you describe ed 0.4 does,
except without the error message -- it just spins silently getting EIO
from reading stdin.  i think em needs to be fixed, and it sounds like
GNU ed already has been.  (unless i misunderstand the relationship of
0.4 and 1.6.)

paul

 > 
 > Krzysiek
 > --
 > To unsubscribe from this list: send the line "unsubscribe git" in
 > the body of a message to majordomo@vger.kernel.org
 > More majordomo info at  http://vger.kernel.org/majordomo-info.html

=---------------------
 paul fox, pgf@foxharp.boston.ma.us (arlington, ma, where it's 57.2 degrees)

Re: [PATCH 0/5] ignore SIGINT while editor runs

[Krzysztof Mazur]

On Sun, Nov 11, 2012 at 03:24:19PM -0500, Paul Fox wrote:
> krzysztof wrote:
>  > Looks ok, but what about SIGQUIT? Some editors like GNU ed (0.4 and 1.6)
>  > ignore SIGQUIT, and after SIGQUIT git dies, but editor is still running.
>  > After pressing any key ed receives -EIO and prints "stdin: Input/output
>  > error". GNU ed 1.6 then exits, but ed 0.4 prints this error forever.
>  > Maybe git should kill the editor in such case?
> 
> there's certainly lots of precedent for treating SIGINT and SIGQUIT
> the same.  but there's also some merit to saying that if the user
> knows to send SIGQUIT instead of SIGINT, they may well have a reason. 
> (after all, if we always treat them the same, there's no point in
> having both.)

That's why I'm proposing in case of SIGQUIT just killing the editor
(SIGTERM is sufficient for ed).

So git will ignore SIGINT, but die on SIGQUIT (and kill editor
that ignores SIGQUIT).

> 
> the em editor (linus' microemacs) behaves as you describe ed 0.4 does,
> except without the error message -- it just spins silently getting EIO
> from reading stdin.  i think em needs to be fixed, and it sounds like
> GNU ed already has been.  (unless i misunderstand the relationship of
> 0.4 and 1.6.)

Yes, the version 1.6 is fixed, it just prints an error once and exits.

Krzysiek

Re: [PATCH 0/5] ignore SIGINT while editor runs

[Andreas Schwab]

Krzysztof Mazur <krzysiek@podlesie.net> writes:

> That's why I'm proposing in case of SIGQUIT just killing the editor
> (SIGTERM is sufficient for ed).
>
> So git will ignore SIGINT, but die on SIGQUIT (and kill editor
> that ignores SIGQUIT).

system(3) also ignores SIGQUIT.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH 2/5] launch_editor: ignore SIGINT while the editor has control

[Junio C Hamano]

How did this message happen?

    Subject: [PATCH 2/5] launch_editor: ignore SIGINT while the editor has control
    To: Kalle Olavi Niemitalo <kon@iki.fi>
    Cc: Paul Fox <pgf@foxharp.boston.ma.us>, git@vger.kernel.org
    Date: Sun, 11 Nov 2012 11:55:11 -0500
    Message-ID: <20121111165510.GB19850@sigill.intra.peff.net>
    References: <20121111163100.GB13188@sigill.intra.peff.net>

    The user's editor likely catches SIGINT (ctrl-C).  but if
    the user spawns a command from the editor and uses ctrl-C to
    kill that command, the SIGINT will likely also kill git
    itself (depending on the editor, this can leave the terminal
    in an unusable state).

    Signed-off-by: Paul Fox <pgf@foxharp.boston.ma.us>
    Signed-off-by: Jeff King <peff@peff.net>
    ---

Judging from S-o-b, message-id and EHLO, I think this was sent by
Peff, but came without Sender: or anything.

Just being curious.

Re: [PATCH 2/5] launch_editor: ignore SIGINT while the editor has control

[Jeff King]

On Mon, Nov 12, 2012 at 09:44:49AM -0800, Junio C Hamano wrote:

> How did this message happen?
> 
>     Subject: [PATCH 2/5] launch_editor: ignore SIGINT while the editor has control
>     To: Kalle Olavi Niemitalo <kon@iki.fi>
>     Cc: Paul Fox <pgf@foxharp.boston.ma.us>, git@vger.kernel.org
>     Date: Sun, 11 Nov 2012 11:55:11 -0500
>     Message-ID: <20121111165510.GB19850@sigill.intra.peff.net>
>     References: <20121111163100.GB13188@sigill.intra.peff.net>
> 
>     The user's editor likely catches SIGINT (ctrl-C).  but if
>     the user spawns a command from the editor and uses ctrl-C to
>     kill that command, the SIGINT will likely also kill git
>     itself (depending on the editor, this can leave the terminal
>     in an unusable state).
> 
>     Signed-off-by: Paul Fox <pgf@foxharp.boston.ma.us>
>     Signed-off-by: Jeff King <peff@peff.net>
>     ---
> 
> Judging from S-o-b, message-id and EHLO, I think this was sent by
> Peff, but came without Sender: or anything.
> 
> Just being curious.

I screwed up when sending out the series and did not properly move
Paul's "From" address from the email header down to the body. It is not
a git or send-email screw-up; I load format-patch output directly into
mutt as a template. Since I do not often send out other people's
patches, I never bothered to write a script to migrate the "from" into
the body automatically.

-Peff

Re: [PATCH 5/5] launch_editor: propagate SIGINT from editor to git

[Jeff King]

On Sun, Nov 11, 2012 at 08:48:38PM +0100, Johannes Sixt wrote:

> Am 11.11.2012 17:57, schrieb Jeff King:
> > @@ -51,6 +51,8 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en
> >  		sigchain_push(SIGINT, SIG_IGN);
> >  		ret = finish_command(&p);
> >  		sigchain_pop(SIGINT);
> > +		if (WIFSIGNALED(ret) && WTERMSIG(ret) == SIGINT)
> > +			raise(SIGINT);
> 
> The return value of finish_command() is already a digested version of
> waitpid's status value. According to
> Documentation/technical/api-run-command.txt:
> 
> . If the program terminated due to a signal, then the return value is
> the signal number - 128, ...
> 
> the correct condition would be
> 
> 		if (ret == SIGINT - 128)

Yeah, that is the same thing as WTERMSIG (which uses "ret & 0x7f") for
the range of -127..-1. I do not mind changing it to match run-command's
stated output, but I am curious whether there are systems where WTERMSIG
is not defined in the same way, and the code would break.

-Peff