{"thread":{"id":"35155","subject":"What's cooking in git.git (Oct 2013, #03; Wed, 16)","startedAt":"2013-10-16T21:43:47Z","lastAt":"2013-10-22T13:13:22Z","messageCount":10,"participants":["Junio C Hamano","Karsten Blees","Jens Lehmann"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"229098","messageId":"xmqqa9i851t8.fsf@gitster.dls.corp.google.com","threadId":"35155","inReplyTo":null,"subject":"What's cooking in git.git (Oct 2013, #03; Wed, 16)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-10-16T21:43:47Z","receivedAt":"2013-10-16T21:43:47Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Here are the topics that have been cooking.  Commits prefixed with\n'-' are only in 'pu' (proposed updates) while commits prefixed with\n'+' are in 'next'.\n\nI think I correctly inherited all the topics Jonathan kept track of\nduring my absence (big thanks to Jonathan); if a topic that has been\nin his tree is missing, please holler.\n\nI am chewing through the list backlog but still have a long way to\ngo. Please be patient for the rest of the week.\n\nYou can find the changes described here in the integration branches\nof the repositories listed at\n\n    http://git-blame.blogspot.com/p/git-public-repositories.html\n\n--------------------------------------------------\n[New Topics]\n\n* fc/styles (2013-10-16) 7 commits\n - block-sha1/sha1.c: have SP around arithmetic operators\n - base85.c: have SP around arithmetic operators\n - archive.c: have SP around arithmetic operators\n - alloc.c: have SP around arithmetic operators\n - abspath.c: have SP around arithmetic operators\n - alias: have SP around arithmetic operators\n - C: have space around && and || operators\n\n C coding style fixes.  The ones near the tip have not been sent to\n the list yet (they cover the same kind of style violation as the\n second one) and I may either send them or drop some of them if they\n turn out to conflict with other work in flight---I still haven't\n caught up with the backlog and do not know.\n\n\n* jk/remote-literal-string-leakfix (2013-10-15) 1 commit\n - remote: do not copy \"origin\" string literal\n\n Will merge to 'next'.\n\n\n* jk/split-broken-ident (2013-10-15) 2 commits\n - SQUASH??? remove reverse scan to simplify the logic\n - split_ident: parse timestamp from end of line\n\n Make the fall-back parsing of commit objects with broken author or\n committer lines more robust to pick up the timestamps.\n\n Will merge to 'next', perhaps after dropping the top one.\n\n\n* sg/prompt-svn-remote-fix (2013-10-15) 1 commit\n - bash prompt: don't use '+=' operator in show upstream code path\n\n Bash portability fix.\n\n Will merge to 'next'.\n\n\n* sc/doc-howto-dumb-http (2013-10-16) 1 commit\n - doc/howto: warn about (dumb)http server document being too old\n\n Will merge to 'next'.\n\n\n* sg/t3600-nul-sha1-fix (2013-10-16) 1 commit\n - t3600: fix broken \"choking git rm\" test\n\n Will merge to 'next'.\n\n--------------------------------------------------\n[Stalled]\n\n* tr/merge-recursive-index-only (2013-07-07) 3 commits\n - merge-recursive: -Xindex-only to leave worktree unchanged\n - merge-recursive: untangle double meaning of o->call_depth\n - merge-recursive: remove dead conditional in update_stages()\n\n Holding until there is a caller to learn from.\n\n\n* jc/ref-excludes (2013-09-03) 2 commits\n - document --exclude option\n - revision: introduce --exclude=<glob> to tame wildcards\n\n People often wished a way to tell \"git log --branches\" (and \"git\n log --remotes --not --branches\") to exclude some local branches\n from the expansion of \"--branches\" (similarly for \"--tags\", \"--all\"\n and \"--glob=<pattern>\").  Now they have one.\n\n Needs a matching change to rev-parse.\n\n\n* rv/send-email-cache-generated-mid (2013-08-21) 2 commits\n - git-send-email: Cache generated message-ids, use them when prompting\n - git-send-email: add optional 'choices' parameter to the ask sub\n\n\n* rj/read-default-config-in-show-ref-pack-refs (2013-06-17) 3 commits\n - ### DONTMERGE: needs better explanation on what config they need\n - pack-refs.c: Add missing call to git_config()\n - show-ref.c: Add missing call to git_config()\n\n The changes themselves are probably good, but it is unclear what\n basic setting needs to be read for which exact operation.\n\n Waiting for clarification.\n $gmane/228294\n\n\n* jh/shorten-refname (2013-05-07) 4 commits\n - t1514: refname shortening is done after dereferencing symbolic refs\n - shorten_unambiguous_ref(): Fix shortening refs/remotes/origin/HEAD to origin\n - t1514: Demonstrate failure to correctly shorten \"refs/remotes/origin/HEAD\"\n - t1514: Add tests of shortening refnames in strict/loose mode\n\n When remotes/origin/HEAD is not a symbolic ref, \"rev-parse\n --abbrev-ref remotes/origin/HEAD\" ought to show \"origin\", not\n \"origin/HEAD\", which is fixed with this series (if it is a symbolic\n ref that points at remotes/origin/something, then it should show\n \"origin/something\" and it already does).\n\n Expecting a reroll, as an early part of a larger series.\n $gmane/225137\n\n\n* jc/format-patch (2013-04-22) 2 commits\n - format-patch: --inline-single\n - format-patch: rename \"no_inline\" field\n\n A new option to send a single patch to the standard output to be\n appended at the bottom of a message.  I personally have no need for\n this, but it was easy enough to cobble together.  Tests, docs and\n stripping out more MIMEy stuff are left as exercises to interested\n parties.\n\n\n* jk/gitweb-utf8 (2013-04-08) 4 commits\n - gitweb: Fix broken blob action parameters on blob/commitdiff pages\n - gitweb: Don't append ';js=(0|1)' to external links\n - gitweb: Make feed title valid utf8\n - gitweb: Fix utf8 encoding for blob_plain, blobdiff_plain, commitdiff_plain, and patch\n\n Various fixes to gitweb.\n\n Drew Northup volunteered to take a look into this.\n $gmane/226216\n\n\n* jc/show-branch (2013-06-07) 5 commits\n - show-branch: use commit slab to represent bitflags of arbitrary width\n - show-branch.c: remove \"all_mask\"\n - show-branch.c: abstract out \"flags\" operation\n - show-branch.c: lift all_mask/all_revs to a global static\n - show-branch.c: update comment style\n\n Waiting for the final step to lift the hard-limit before sending it out.\n\n--------------------------------------------------\n[Cooking]\n\n* mg/more-textconv (2013-05-10) 7 commits\n  (merged to 'next' on 2013-10-14 at 8a12490)\n + grep: honor --textconv for the case rev:path\n + grep: allow to use textconv filters\n + t7008: demonstrate behavior of grep with textconv\n + cat-file: do not die on --textconv without textconv filters\n + show: honor --textconv for blobs\n + diff_opt: track whether flags have been set explicitly\n + t4030: demonstrate behavior of show with textconv\n\n Make \"git grep\" and \"git show\" pay attention to --textconv when\n dealing with blob objects.\n\n\n* ak/submodule-foreach-quoting (2013-09-27) 1 commit\n  (merged to 'next' on 2013-10-14 at d77c5f1)\n + submodule foreach: skip eval for more than one argument\n\n A behavior change, but a worthwhile one: \"git submodule foreach\"\n was treating its arguments as part of a single command to be\n concatenated and passed to a shell, making writing buggy\n scripts too easy.\n\n This patch preserves the old \"just pass it to the shell\" behavior\n when a single argument is passed to 'git submodule foreach' and\n moves to a new \"skip the shell and use the arguments passed\n unmolested\" behavior when more than one argument is passed.\n\n The old behavior (always concatenating and passing to the shell)\n was similar to the 'ssh' command, while the new behavior (switching\n on the number of arguments) is what 'xterm -e' does.\n\n May need more thought to make sure this change is advertised well\n so that scripts that used multiple arguments but added their own\n extra layer of quoting are not broken.\n\n\n* ew/keepalive (2013-10-16) 2 commits\n  (merged to 'next' on 2013-10-16 at 56fd9f3)\n + http: use curl's tcp keepalive if available\n  (merged to 'next' on 2013-10-14 at 24d786f)\n + http: enable keepalive on TCP sockets\n\n\n* hu/cherry-pick-previous-branch (2013-10-10) 1 commit\n  (merged to 'next' on 2013-10-14 at 090934f)\n + cherry-pick: handle \"-\" after parsing options\n\n \"git cherry-pick\" without further options would segfault.\n\n Could use a follow-up to handle '-' after argv[1] better.\n\n\n* jc/pack-objects (2013-02-04) 1 commit\n  (merged to 'next' on 2013-10-14 at 8e8feb6)\n + pack-objects: shrink struct object_entry\n\n\n* jc/prompt-upstream (2013-10-14) 1 commit\n  (merged to 'next' on 2013-10-14 at 270ef7b)\n + git-prompt.sh: optionally show upstream branch name\n\n An enhancement to the GIT_PS1_SHOWUPSTREAM facility.\n\n\n* jk/http-auth-redirects (2013-10-14) 9 commits\n - remote-curl: rewrite base url from info/refs redirects\n - remote-curl: store url as a strbuf\n - remote-curl: make refs_url a strbuf\n - http: update base URLs when we see redirects\n - http: provide effective url to callers\n - http: hoist credential request out of handle_curl_result\n  (merged to 'next' on 2013-10-14 at a0642be)\n + http: refactor options to http_get_*\n + http_request: factor out curlinfo_strbuf\n + http_get_file: style fixes\n\n Handle the case where http transport gets redirected during the\n authorization request better.\n\n Will merge to 'next'.\n\n\n* jl/submodule-mv (2013-10-13) 1 commit\n - mv: Fix spurious warning when moving a file in presence of submodules\n\n Moving a regular file in a repository with a .gitmodules file was\n producing a warning 'Could not find section in .gitmodules where\n path=<filename>'.\n\n jrneider: \"The test can use a little cleanup.  Otherwise looks good\".\n\n\n* yt/shortened-rename (2013-10-13) 1 commit\n - diff.c: keep arrow(=>) on show_stats()'s shortened filename part to make rename visible.\n\n Reroll v6 hasn't been picked up yet.\n\n\n* bc/gnome-keyring (2013-10-16) 16 commits\n - contrib/git-credential-gnome-keyring.c: support really ancient gnome-keyring\n - contrib/git-credential-gnome-keyring.c: support ancient gnome-keyring\n - contrib/git-credential-gnome-keyring.c: report failure to store password\n - contrib/git-credential-gnome-keyring.c: use glib messaging functions\n - contrib/git-credential-gnome-keyring.c: use glib memory allocation functions\n - contrib/git-credential-gnome-keyring.c: use secure memory for reading passwords\n - contrib/git-credential-gnome-keyring.c: use secure memory functions for passwds\n - contrib/git-credential-gnome-keyring.c: use gnome helpers in keyring_object()\n - contrib/git-credential-gnome-keyring.c: set Gnome application name\n - contrib/git-credential-gnome-keyring.c: ensure buffer is non-empty before accessing\n - contrib/git-credential-gnome-keyring.c: strlen() returns size_t, not ssize_t\n - contrib/git-credential-gnome-keyring.c: exit non-zero when called incorrectly\n - contrib/git-credential-gnome-keyring.c: add static where applicable\n - contrib/git-credential-gnome-keyring.c: *style* use \"if ()\" not \"if()\" etc.\n - contrib/git-credential-gnome-keyring.c: remove unused die() function\n - contrib/git-credential-gnome-keyring.c: remove unnecessary pre-declarations\n\n Cleanups and tweaks for credential handling to work with ancient versions\n of the gnome-keyring library that are still in use.\n\n Will merge to 'next'.\n\n\n* kb/fast-hashmap (2013-09-25) 6 commits\n - fixup! diffcore-rename.c: simplify finding exact renames\n - diffcore-rename.c: use new hash map implementation\n - diffcore-rename.c: simplify finding exact renames\n - diffcore-rename.c: move code around to prepare for the next patch\n - buitin/describe.c: use new hash map implementation\n - add a hashtable implementation that supports O(1) removal\n\n Improvements to our hash table to get it to meet the needs of the\n msysgit fscache project, with some nice performance improvements.\n\n Looks promising.  Needs style review and a sanity-check on the\n design before including in 'next'.  Expecting a reroll once review\n settles down.\n\n\n* jc/revision-range-unpeel (2013-10-15) 1 commit\n  (merged to 'next' on 2013-10-16 at d04ddfe)\n + revision: do not peel tags used in range notation\n\n \"git rev-list --objects ^v1.0^ v1.0\" gave v1.0 tag itself in the\n output, but \"git rev-list --objects v1.0^..v1.0\" did not.\n\n\n* tg/perf-lib-test-perf-cleanup (2013-09-19) 2 commits\n - perf-lib: add test_perf_cleanup target\n - perf-lib: split starting the test from the execution\n\n Add test_perf_cleanup shell function to the perf suite, that allows\n the script writers to define a test with a clean-up action.\n\n Holding until needed.\n\n\n* jk/format-patch-from (2013-09-20) 1 commit\n  (merged to 'next' on 2013-09-20 at 0506530)\n + format-patch: print in-body \"From\" only when needed\n\n \"format-patch --from=<whom>\" forgot to omit unnecessary in-body\n from line, i.e. when <whom> is the same as the real author.\n\n Will merge to 'master'.\n\n\n* es/name-hash-no-trailing-slash-in-dirs (2013-09-17) 4 commits\n  (merged to 'next' on 2013-09-20 at 9633d9a)\n + dir: revert work-around for retired dangerous behavior\n + name-hash: stop storing trailing '/' on paths in index_state.dir_hash\n + employ new explicit \"exists in index?\" API\n + name-hash: refactor polymorphic index_name_exists()\n\n Clean up the internal of the name-hash mechanism used to work\n around case insensitivity on some filesystems to cleanly fix a\n long-standing API glitch where the caller of cache_name_exists()\n that ask about a directory with a counted string was required to\n have '/' at one location past the end of the string.\n\n Will merge to 'master'.\n\n\n* po/dot-url (2013-10-15) 3 commits\n  (merged to 'next' on 2013-10-15 at 312d0af)\n + doc/cli: make \"dot repository\" an independent bullet point\n  (merged to 'next' on 2013-09-20 at 6a12786)\n + config doc: update dot-repository notes\n + doc: command line interface (cli) dot-repository dwimmery\n\n Explain how '.' can be used to refer to the \"current repository\"\n in the documentation.\n\n Will merge to 'master'.\n\n\n* jc/upload-pack-send-symref (2013-09-17) 7 commits\n  (merged to 'next' on 2013-10-16 at eb1ae25)\n + clone: test the new HEAD detection logic\n + connect: annotate refs with their symref information in get_remote_head()\n + connect.c: make parse_feature_value() static\n + upload-pack: send non-HEAD symbolic refs\n + upload-pack: send symbolic ref information as capability\n + upload-pack.c: do not pass confusing cb_data to mark_our_ref()\n + t5505: fix \"set-head --auto with ambiguous HEAD\" test\n\n One long-standing flaw in the pack transfer protocol used by \"git\n clone\" was that there was no way to tell the other end which branch\n \"HEAD\" points at, and the receiving end needed to guess.  A new\n capability has been defined in the pack protocol to convey this\n information so that cloning from a repository with more than one\n branches pointing at the same commit where the HEAD is at now\n reliably sets the initial branch in the resulting repository.\n\n Will merge to 'master'.\n\n\n* jk/clone-progress-to-stderr (2013-09-18) 3 commits\n  (merged to 'next' on 2013-09-25 at 137af9e)\n + clone: always set transport options\n + clone: treat \"checking connectivity\" like other progress\n + clone: send diagnostic messages to stderr\n\n Some progress and diagnostic messages from \"git clone\" were\n incorrectly sent to the standard output stream, not to the standard\n error stream.\n\n Will merge to 'master'.\n\n\n* jx/relative-path-regression-fix (2013-10-14) 3 commits\n - Use simpler relative_path when set_git_dir\n  (merged to 'next' on 2013-10-14 at 704b9ee)\n + relative_path should honor dos-drive-prefix\n + test: use unambigous leading path (/foo) for MSYS\n\n Waiting for the review to settle.\n\n\n* jc/checkout-detach-doc (2013-09-11) 1 commit\n  (merged to 'next' on 2013-09-17 at 438cf13)\n + checkout: update synopsys and documentation on detaching HEAD\n\n \"git checkout [--detach] <commit>\" was listed poorly in the\n synopsis section of its documentation.\n\n Will merge to 'master'.\n\n\n* jk/trailing-slash-in-pathspec (2013-09-13) 2 commits\n  (merged to 'next' on 2013-09-17 at 18fe277)\n + reset: handle submodule with trailing slash\n + rm: re-use parse_pathspec's trailing-slash removal\n\n Code refactoring.\n\n Will merge to 'master'.\n\n\n* lc/filter-branch-too-many-refs (2013-09-12) 1 commit\n  (merged to 'next' on 2013-09-17 at 31cd01a)\n + Allow git-filter-branch to process large repositories with lots of branches.\n\n \"git filter-branch\" in a repository with many refs blew limit of\n command line length.\n\n Will merge to 'master'.\n\n\n* np/pack-v4 (2013-09-18) 90 commits\n - packv4-parse.c: add tree offset caching\n - t1050: replace one instance of show-index with verify-pack\n - index-pack, pack-objects: allow creating .idx v2 with .pack v4\n - unpack-objects: decode v4 trees\n - unpack-objects: allow to save processed bytes to a buffer\n - ...\n\n Nico and Duy advancing the eternal vaporware pack-v4.  This is here\n primarily for wider distribution of the preview edition.\n\n\n* sb/repack-in-c (2013-09-17) 3 commits\n  (merged to 'next' on 2013-09-25 at 7c47036)\n + repack: improve warnings about failure of renaming and removing files\n + repack: retain the return value of pack-objects\n + repack: rewrite the shell script in C\n\n Rerolled, and I think it is in a reasonably good shape.\n\n Will merge to 'master'.\n\n\n* jc/reflog-doc (2013-06-19) 1 commit\n  (merged to 'next' on 2013-09-25 at 4eb0c14)\n + setup_reflog_action: document the rules for using GIT_REFLOG_ACTION\n\n Document rules to use GIT_REFLOG_ACTION variable in the scripted\n Porcelain.  git-rebase--interactive locally violates them, but it\n is a leaf user that does not call out to or dot-source other\n scripts, so it does not urgently need to be fixed.\n\n Will merge to 'master'.\n\n\n* jn/add-2.0-u-A-sans-pathspec (2013-04-26) 1 commit\n - git add: -u/-A now affects the entire working tree\n\n Will cook in 'next' until Git 2.0.\n\n\n* jc/core-checkstat-2.0 (2013-05-06) 1 commit\n - core.statinfo: remove as promised in Git 2.0\n\n Will cook in 'next' until Git 2.0.\n\n\n* jc/push-2.0-default-to-simple (2013-06-18) 1 commit\n - push: switch default from \"matching\" to \"simple\"\n\n Will cook in 'next' until Git 2.0.\n\n\n* jc/add-2.0-ignore-removal (2013-04-22) 1 commit\n - git add <pathspec>... defaults to \"-A\"\n\n Updated endgame for \"git add <pathspec>\" that defaults to \"--all\"\n aka \"--no-ignore-removal\".\n\n Will cook in 'next' until Git 2.0.\n\n\n* jc/hold-diff-remove-q-synonym-for-no-deletion (2013-07-19) 1 commit\n - diff: remove \"diff-files -q\" in a version of Git in a distant future\n\n Will cook in 'next' until a distant future.\n\n--------------------------------------------------\n[Discarded]\n\n* jc/pull-training-wheel (2013-07-19) 1 commit\n  (merged to 'next' on 2013-08-28 at c39bd15)\n + pull: require choice between rebase/merge on non-fast-forward pull\n\n Make \"git pull\" (without arguments that say what branch to\n integrate from where) refuse with \"it does not fast forward; choose\n between 'pull --merge' and 'pull --rebase'\".\n\n This topic has been reverted from 'next'.  Will wait for the\n conclusion of the discussion to seek a more user-friendly\n alternative; it is likely that it will be based on the simpler\n approach Felipe posted earlier.\n"},{"id":"229116","messageId":"525FB287.7020204@gmail.com","threadId":"35155","inReplyTo":"xmqqa9i851t8.fsf@gitster.dls.corp.google.com","subject":"Re: What's cooking in git.git (Oct 2013, #03; Wed, 16)","fromName":"Karsten Blees","fromEmail":"karsten.blees@gmail.com","sentAt":"2013-10-17T09:48:55Z","receivedAt":"2013-10-17T09:48:55Z","isPatch":false,"sender":{"key":"karsten.blees@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1111200?v=4"},"body":"Am 16.10.2013 23:43, schrieb Junio C Hamano:\n> * kb/fast-hashmap (2013-09-25) 6 commits\n>  - fixup! diffcore-rename.c: simplify finding exact renames\n>  - diffcore-rename.c: use new hash map implementation\n>  - diffcore-rename.c: simplify finding exact renames\n>  - diffcore-rename.c: move code around to prepare for the next patch\n>  - buitin/describe.c: use new hash map implementation\n>  - add a hashtable implementation that supports O(1) removal\n> \n\nI posted a much more complete v3 [1], but somehow missed Jonathan's fixup! commit.\n\nBtw., the test suite didn't catch the uninitialized variable, neither on mingw nor linux nor with valgrind. Is there a way to run tests with STACK_POISON or something?\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/235644\n"},{"id":"229138","messageId":"xmqqzjq7y6k7.fsf@gitster.dls.corp.google.com","threadId":"35155","inReplyTo":"525FB287.7020204@gmail.com","subject":"Re: What's cooking in git.git (Oct 2013, #03; Wed, 16)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-10-17T20:40:40Z","receivedAt":"2013-10-17T20:40:40Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karsten Blees <karsten.blees@gmail.com> writes:\n\n> Am 16.10.2013 23:43, schrieb Junio C Hamano:\n>> * kb/fast-hashmap (2013-09-25) 6 commits\n>>  - fixup! diffcore-rename.c: simplify finding exact renames\n>>  - diffcore-rename.c: use new hash map implementation\n>>  - diffcore-rename.c: simplify finding exact renames\n>>  - diffcore-rename.c: move code around to prepare for the next patch\n>>  - buitin/describe.c: use new hash map implementation\n>>  - add a hashtable implementation that supports O(1) removal\n>> \n>\n> I posted a much more complete v3 [1], but somehow missed Jonathan's fixup! commit.\n\nThanks; I'll replace the above with v3 and squash the fix-up in.\n"},{"id":"229141","messageId":"xmqqmwm7y5bf.fsf@gitster.dls.corp.google.com","threadId":"35155","inReplyTo":"xmqqzjq7y6k7.fsf@gitster.dls.corp.google.com","subject":"Re: What's cooking in git.git (Oct 2013, #03; Wed, 16)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-10-17T21:07:32Z","receivedAt":"2013-10-17T21:07:32Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Karsten Blees <karsten.blees@gmail.com> writes:\n>\n>> Am 16.10.2013 23:43, schrieb Junio C Hamano:\n>>> * kb/fast-hashmap (2013-09-25) 6 commits\n>>>  - fixup! diffcore-rename.c: simplify finding exact renames\n>>>  - diffcore-rename.c: use new hash map implementation\n>>>  - diffcore-rename.c: simplify finding exact renames\n>>>  - diffcore-rename.c: move code around to prepare for the next patch\n>>>  - buitin/describe.c: use new hash map implementation\n>>>  - add a hashtable implementation that supports O(1) removal\n>>> \n>>\n>> I posted a much more complete v3 [1], but somehow missed Jonathan's fixup! commit.\n>\n> Thanks; I'll replace the above with v3 and squash the fix-up in.\n\nInterestingly, v3 applied on 'maint' and then merged to 'master'\nseems to break t3600 and t7001 with a coredump.\n\nIt would conflict with es/name-hash-no-trailing-slash-in-dirs that\nhas been cooking in 'next', too; the resolution might be trivial but\nI didn't look too deeply into it.\n"},{"id":"229155","messageId":"526083E7.6080701@gmail.com","threadId":"35155","inReplyTo":"xmqqmwm7y5bf.fsf@gitster.dls.corp.google.com","subject":"Re: What's cooking in git.git (Oct 2013, #03; Wed, 16)","fromName":"Karsten Blees","fromEmail":"karsten.blees@gmail.com","sentAt":"2013-10-18T00:42:15Z","receivedAt":"2013-10-18T00:42:15Z","isPatch":false,"sender":{"key":"karsten.blees@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1111200?v=4"},"body":"Am 17.10.2013 23:07, schrieb Junio C Hamano:\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n>> Karsten Blees <karsten.blees@gmail.com> writes:\n>>\n>>> Am 16.10.2013 23:43, schrieb Junio C Hamano:\n>>>> * kb/fast-hashmap (2013-09-25) 6 commits\n>>>>  - fixup! diffcore-rename.c: simplify finding exact renames\n>>>>  - diffcore-rename.c: use new hash map implementation\n>>>>  - diffcore-rename.c: simplify finding exact renames\n>>>>  - diffcore-rename.c: move code around to prepare for the next patch\n>>>>  - buitin/describe.c: use new hash map implementation\n>>>>  - add a hashtable implementation that supports O(1) removal\n>>>>\n>>>\n>>> I posted a much more complete v3 [1], but somehow missed Jonathan's fixup! commit.\n>>\n>> Thanks; I'll replace the above with v3 and squash the fix-up in.\n> \n> Interestingly, v3 applied on 'maint' and then merged to 'master'\n> seems to break t3600 and t7001 with a coredump.\n> \n> It would conflict with es/name-hash-no-trailing-slash-in-dirs that\n> has been cooking in 'next', too; the resolution might be trivial but\n> I didn't look too deeply into it.\n> \n\nI've pushed a rebased version to https://github.com/kblees/git/commits/kb/hashmap-v3-next\n(no changes yet except for Jonathan's fixup in #04 and merge resolution).\n\nThe coredumps are caused by my patch #10, which free()s cache_entries when they are removed, in combination with submodule.c::stage_updated_gitmodules (5fee9952 \"submodule.c: add .gitmodules staging helper functions\"), which removes a cache_entry, then modifies and re-adds the (now) free()d memory.\n\nCan't we just use add_file_to_cache here (which replaces cache_entries by creating a copy)?\n\n\ndiff --git a/submodule.c b/submodule.c\nindex 1905d75..e388487 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -116,30 +116,7 @@ int remove_path_from_gitmodules(const char *path)\n \n void stage_updated_gitmodules(void)\n {\n-       struct strbuf buf = STRBUF_INIT;\n-       struct stat st;\n-       int pos;\n-       struct cache_entry *ce;\n-       int namelen = strlen(\".gitmodules\");\n-\n-       pos = cache_name_pos(\".gitmodules\", namelen);\n-       if (pos < 0) {\n-               warning(_(\"could not find .gitmodules in index\"));\n-               return;\n-       }\n-       ce = active_cache[pos];\n-       ce->ce_flags = namelen;\n-       if (strbuf_read_file(&buf, \".gitmodules\", 0) < 0)\n-               die(_(\"reading updated .gitmodules failed\"));\n-       if (lstat(\".gitmodules\", &st) < 0)\n-               die_errno(_(\"unable to stat updated .gitmodules\"));\n-       fill_stat_cache_info(ce, &st);\n-       ce->ce_mode = ce_mode_from_stat(ce, st.st_mode);\n-       if (remove_cache_entry_at(pos) < 0)\n-               die(_(\"unable to remove .gitmodules from index\"));\n-       if (write_sha1_file(buf.buf, buf.len, blob_type, ce->sha1))\n-               die(_(\"adding updated .gitmodules failed\"));\n-       if (add_cache_entry(ce, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE))\n+       if (add_file_to_cache(\".gitmodules\", 0))\n                die(_(\"staging updated .gitmodules failed\"));\n }\n"},{"id":"229184","messageId":"xmqqfvrywg4y.fsf@gitster.dls.corp.google.com","threadId":"35155","inReplyTo":"526083E7.6080701@gmail.com","subject":"Re: What's cooking in git.git (Oct 2013, #03; Wed, 16)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-10-18T19:09:01Z","receivedAt":"2013-10-18T19:09:01Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karsten Blees <karsten.blees@gmail.com> writes:\n\n> The coredumps are caused by my patch #10, which free()s\n> cache_entries when they are removed, in combination with ...\n\nLooking at that patch, it makes me wonder if remove_index_entry_at()\nand replace_index_entry() should be the ones that frees the old\nentry in the first place.  A caller may already have a ce pointing\nat an old entry and use the information from old_ce to update a new\none after it installed it, e.g.\n\n\told_ce = ...\n        new_ce = make_cache_entry(... old_ce->name, ...);\n        replace_index_entry(... new_ce);\n\tnew_ce->ce_mode = old_ce->cd_mode;\n\tfree(old_ce);\n\nThe same goes for the functions that remove the entry.\n\nBut I am probably biased saying this, because in the old days, cache\nentries could never be freed (they were carved out of a contiguous\nregion of memory, mmapped from the index file).  These days, we\nparse and run ntoh*() on the on-disk cache entries to create in-core\nform, and the \"cache entries should never be freed\" is no longer\ntrue, but I would not be surprised if there are still some code\nleftover that relies on \"use after free\" being safe, leaking unused\ncache entries.\n\nGoing forward, I do agree with your patch #10 that removal or\nreplacing that may make an existing entry unreferenced should free\nentries that are no longer used, and \"use after free\" should be\nforbidden.\n\n> Can't we just use add_file_to_cache here (which replaces\n> cache_entries by creating a copy)?\n>\n> diff --git a/submodule.c b/submodule.c\n> index 1905d75..e388487 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -116,30 +116,7 @@ int remove_path_from_gitmodules(const char *path)\n>  \n>  void stage_updated_gitmodules(void)\n>  {\n> -       struct strbuf buf = STRBUF_INIT;\n> -       struct stat st;\n> -       int pos;\n> -       struct cache_entry *ce;\n> -       int namelen = strlen(\".gitmodules\");\n> -\n> -       pos = cache_name_pos(\".gitmodules\", namelen);\n> -       if (pos < 0) {\n> -               warning(_(\"could not find .gitmodules in index\"));\n> -               return;\n> -       }\n\nI think the remainder is (morally) equivalent between the original\nand a single \"add-file-to-cache\" call, and the version after your\n\"how about this\" patch in the message I am responding to looks more\ncorrect (e.g. why does the original lstat after it has read the\nfile?).\n\nBut this warning may want to stay, no?\n\n> -       ce = active_cache[pos];\n> -       ce->ce_flags = namelen;\n> -       if (strbuf_read_file(&buf, \".gitmodules\", 0) < 0)\n> -               die(_(\"reading updated .gitmodules failed\"));\n> -       if (lstat(\".gitmodules\", &st) < 0)\n> -               die_errno(_(\"unable to stat updated .gitmodules\"));\n> -       fill_stat_cache_info(ce, &st);\n> -       ce->ce_mode = ce_mode_from_stat(ce, st.st_mode);\n> -       if (remove_cache_entry_at(pos) < 0)\n> -               die(_(\"unable to remove .gitmodules from index\"));\n> -       if (write_sha1_file(buf.buf, buf.len, blob_type, ce->sha1))\n> -               die(_(\"adding updated .gitmodules failed\"));\n> -       if (add_cache_entry(ce, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE))\n> +       if (add_file_to_cache(\".gitmodules\", 0))\n>                 die(_(\"staging updated .gitmodules failed\"));\n\n\n\n>  }\n"},{"id":"229187","messageId":"52618DEE.6010000@web.de","threadId":"35155","inReplyTo":"526083E7.6080701@gmail.com","subject":"Re: What's cooking in git.git (Oct 2013, #03; Wed, 16)","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2013-10-18T19:37:18Z","receivedAt":"2013-10-18T19:37:18Z","isPatch":false,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 18.10.2013 02:42, schrieb Karsten Blees:\n> Am 17.10.2013 23:07, schrieb Junio C Hamano:\n>> Junio C Hamano <gitster@pobox.com> writes:\n>>\n>>> Karsten Blees <karsten.blees@gmail.com> writes:\n>>>\n>>>> Am 16.10.2013 23:43, schrieb Junio C Hamano:\n>>>>> * kb/fast-hashmap (2013-09-25) 6 commits\n>>>>>  - fixup! diffcore-rename.c: simplify finding exact renames\n>>>>>  - diffcore-rename.c: use new hash map implementation\n>>>>>  - diffcore-rename.c: simplify finding exact renames\n>>>>>  - diffcore-rename.c: move code around to prepare for the next patch\n>>>>>  - buitin/describe.c: use new hash map implementation\n>>>>>  - add a hashtable implementation that supports O(1) removal\n>>>>>\n>>>>\n>>>> I posted a much more complete v3 [1], but somehow missed Jonathan's fixup! commit.\n>>>\n>>> Thanks; I'll replace the above with v3 and squash the fix-up in.\n>>\n>> Interestingly, v3 applied on 'maint' and then merged to 'master'\n>> seems to break t3600 and t7001 with a coredump.\n>>\n>> It would conflict with es/name-hash-no-trailing-slash-in-dirs that\n>> has been cooking in 'next', too; the resolution might be trivial but\n>> I didn't look too deeply into it.\n>>\n> \n> I've pushed a rebased version to https://github.com/kblees/git/commits/kb/hashmap-v3-next\n> (no changes yet except for Jonathan's fixup in #04 and merge resolution).\n> \n> The coredumps are caused by my patch #10, which free()s cache_entries when they are removed, in combination with submodule.c::stage_updated_gitmodules (5fee9952 \"submodule.c: add .gitmodules staging helper functions\"), which removes a cache_entry, then modifies and re-adds the (now) free()d memory.\n> \n> Can't we just use add_file_to_cache here (which replaces cache_entries by creating a copy)?\n\nNo objections from my side. Looks like we could also copy the\ncache entry just before remove_cache_entry_at() and use that\ncopy afterwards, but your solution is so much shorter that I\nwould really like to use it (unless someone more cache-savvy\nthan me has any objections).\n\nAnd by the way: this is the last use of remove_cache_entry_at(),\nwould it make sense to remove that define while at it? Only the\nremove_index_entry_at() function it is defined to is currently\nused.\n\n> diff --git a/submodule.c b/submodule.c\n> index 1905d75..e388487 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -116,30 +116,7 @@ int remove_path_from_gitmodules(const char *path)\n>  \n>  void stage_updated_gitmodules(void)\n>  {\n> -       struct strbuf buf = STRBUF_INIT;\n> -       struct stat st;\n> -       int pos;\n> -       struct cache_entry *ce;\n> -       int namelen = strlen(\".gitmodules\");\n> -\n> -       pos = cache_name_pos(\".gitmodules\", namelen);\n> -       if (pos < 0) {\n> -               warning(_(\"could not find .gitmodules in index\"));\n> -               return;\n> -       }\n> -       ce = active_cache[pos];\n> -       ce->ce_flags = namelen;\n> -       if (strbuf_read_file(&buf, \".gitmodules\", 0) < 0)\n> -               die(_(\"reading updated .gitmodules failed\"));\n> -       if (lstat(\".gitmodules\", &st) < 0)\n> -               die_errno(_(\"unable to stat updated .gitmodules\"));\n> -       fill_stat_cache_info(ce, &st);\n> -       ce->ce_mode = ce_mode_from_stat(ce, st.st_mode);\n> -       if (remove_cache_entry_at(pos) < 0)\n> -               die(_(\"unable to remove .gitmodules from index\"));\n> -       if (write_sha1_file(buf.buf, buf.len, blob_type, ce->sha1))\n> -               die(_(\"adding updated .gitmodules failed\"));\n> -       if (add_cache_entry(ce, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE))\n> +       if (add_file_to_cache(\".gitmodules\", 0))\n>                 die(_(\"staging updated .gitmodules failed\"));\n>  }\n> \n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n> \n"},{"id":"229189","messageId":"52619183.5010608@web.de","threadId":"35155","inReplyTo":"xmqqfvrywg4y.fsf@gitster.dls.corp.google.com","subject":"Re: What's cooking in git.git (Oct 2013, #03; Wed, 16)","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2013-10-18T19:52:35Z","receivedAt":"2013-10-18T19:52:35Z","isPatch":false,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 18.10.2013 21:09, schrieb Junio C Hamano:\n> Karsten Blees <karsten.blees@gmail.com> writes:\n>> Can't we just use add_file_to_cache here (which replaces\n>> cache_entries by creating a copy)?\n>>\n>> diff --git a/submodule.c b/submodule.c\n>> index 1905d75..e388487 100644\n>> --- a/submodule.c\n>> +++ b/submodule.c\n>> @@ -116,30 +116,7 @@ int remove_path_from_gitmodules(const char *path)\n>>  \n>>  void stage_updated_gitmodules(void)\n>>  {\n>> -       struct strbuf buf = STRBUF_INIT;\n>> -       struct stat st;\n>> -       int pos;\n>> -       struct cache_entry *ce;\n>> -       int namelen = strlen(\".gitmodules\");\n>> -\n>> -       pos = cache_name_pos(\".gitmodules\", namelen);\n>> -       if (pos < 0) {\n>> -               warning(_(\"could not find .gitmodules in index\"));\n>> -               return;\n>> -       }\n> \n> I think the remainder is (morally) equivalent between the original\n> and a single \"add-file-to-cache\" call, and the version after your\n> \"how about this\" patch in the message I am responding to looks more\n> correct (e.g. why does the original lstat after it has read the\n> file?).\n\nCargo cult programming. I was looking at other code manipulating\nthe index (as Documentation/technical/api-in-core-index.txt is\nrather terse ;-) and concluded I would need to read the possibly\nupdated st.st_mode, in case updating the config file would have\nchanged that.\n\n> But this warning may want to stay, no?\n\nOf course you are right on this one. All test ran successfully\nwith this patch, so I think adding one for that warning makes\nsense too. And as that is submodule related stuff I volunteer\nfor fixing all this ;-)\n\n>> -       ce = active_cache[pos];\n>> -       ce->ce_flags = namelen;\n>> -       if (strbuf_read_file(&buf, \".gitmodules\", 0) < 0)\n>> -               die(_(\"reading updated .gitmodules failed\"));\n>> -       if (lstat(\".gitmodules\", &st) < 0)\n>> -               die_errno(_(\"unable to stat updated .gitmodules\"));\n>> -       fill_stat_cache_info(ce, &st);\n>> -       ce->ce_mode = ce_mode_from_stat(ce, st.st_mode);\n>> -       if (remove_cache_entry_at(pos) < 0)\n>> -               die(_(\"unable to remove .gitmodules from index\"));\n>> -       if (write_sha1_file(buf.buf, buf.len, blob_type, ce->sha1))\n>> -               die(_(\"adding updated .gitmodules failed\"));\n>> -       if (add_cache_entry(ce, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE))\n>> +       if (add_file_to_cache(\".gitmodules\", 0))\n>>                 die(_(\"staging updated .gitmodules failed\"));\n"},{"id":"229190","messageId":"52619A91.7090602@web.de","threadId":"35155","inReplyTo":"52619183.5010608@web.de","subject":"[PATCH] submodule: don't access the .gitmodules cache entry after removing it","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2013-10-18T20:31:13Z","receivedAt":"2013-10-18T20:31:13Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Commit 5fee995244e introduced the stage_updated_gitmodules() function to\nadd submodule configuration updates to the index. It assumed that even\nafter calling remove_cache_entry_at() the same cache entry would still be\nvalid. This was true in the old days, as cache entries could never be\nfreed, but that is not so sure in the present as there is ongoing work to\nfree removed cache entries, which makes this code segfault.\n\nFix that by calling add_file_to_cache() instead of open coding it. Also\nremove the \"could not find .gitmodules in index\" warning, as that won't\nhappen in regular use cases (and by then just silently adding it to the\nindex we do the right thing).\n\nThanks-to: Karsten Blees <karsten.blees@gmail.com>\nSigned-off-by: Jens Lehmann <Jens.Lehmann@web.de>\n---\n\nAm 18.10.2013 21:52, schrieb Jens Lehmann:\n> Am 18.10.2013 21:09, schrieb Junio C Hamano:\n>> Karsten Blees <karsten.blees@gmail.com> writes:\n>>> Can't we just use add_file_to_cache here (which replaces\n>>> cache_entries by creating a copy)?\n>>>\n>>> diff --git a/submodule.c b/submodule.c\n>>> index 1905d75..e388487 100644\n>>> --- a/submodule.c\n>>> +++ b/submodule.c\n>>> @@ -116,30 +116,7 @@ int remove_path_from_gitmodules(const char *path)\n>>>  \n>>>  void stage_updated_gitmodules(void)\n>>>  {\n>>> -       struct strbuf buf = STRBUF_INIT;\n>>> -       struct stat st;\n>>> -       int pos;\n>>> -       struct cache_entry *ce;\n>>> -       int namelen = strlen(\".gitmodules\");\n>>> -\n>>> -       pos = cache_name_pos(\".gitmodules\", namelen);\n>>> -       if (pos < 0) {\n>>> -               warning(_(\"could not find .gitmodules in index\"));\n>>> -               return;\n>>> -       }\n>>\n>> But this warning may want to stay, no?\n> \n> Of course you are right on this one. All test ran successfully\n> with this patch, so I think adding one for that warning makes\n> sense too. And as that is submodule related stuff I volunteer\n> for fixing all this ;-)\n\nAnd after digging deeper and trying to come up with a test\ncase for that I think we can safely remove this warning too.\nWhen not having a .gitmodules file update_path_in_gitmodules()\nand remove_path_from_gitmodules() won't do anything and signal\nthat there is no need for stage_updated_gitmodules(). And if\nwe get a user later that wants to create .gitmodules file (e.g.\nto register a new submodule) a warning that that file had not\nbeen known before isn't helpful. So the only cases triggering\nthat warning would be when the .gitmodules is on the filesystem\nbut not in the index, and I can't think of a regular use case\nwhere this happens. So let's drop it too.\n\n\n submodule.c | 25 +------------------------\n 1 file changed, 1 insertion(+), 24 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 1905d75..e388487 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -116,30 +116,7 @@ int remove_path_from_gitmodules(const char *path)\n\n void stage_updated_gitmodules(void)\n {\n-\tstruct strbuf buf = STRBUF_INIT;\n-\tstruct stat st;\n-\tint pos;\n-\tstruct cache_entry *ce;\n-\tint namelen = strlen(\".gitmodules\");\n-\n-\tpos = cache_name_pos(\".gitmodules\", namelen);\n-\tif (pos < 0) {\n-\t\twarning(_(\"could not find .gitmodules in index\"));\n-\t\treturn;\n-\t}\n-\tce = active_cache[pos];\n-\tce->ce_flags = namelen;\n-\tif (strbuf_read_file(&buf, \".gitmodules\", 0) < 0)\n-\t\tdie(_(\"reading updated .gitmodules failed\"));\n-\tif (lstat(\".gitmodules\", &st) < 0)\n-\t\tdie_errno(_(\"unable to stat updated .gitmodules\"));\n-\tfill_stat_cache_info(ce, &st);\n-\tce->ce_mode = ce_mode_from_stat(ce, st.st_mode);\n-\tif (remove_cache_entry_at(pos) < 0)\n-\t\tdie(_(\"unable to remove .gitmodules from index\"));\n-\tif (write_sha1_file(buf.buf, buf.len, blob_type, ce->sha1))\n-\t\tdie(_(\"adding updated .gitmodules failed\"));\n-\tif (add_cache_entry(ce, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE))\n+\tif (add_file_to_cache(\".gitmodules\", 0))\n \t\tdie(_(\"staging updated .gitmodules failed\"));\n }\n\n-- \n1.8.4.1.545.gc07df9e\n"},{"id":"229283","messageId":"526679F2.4060701@gmail.com","threadId":"35155","inReplyTo":"xmqqfvrywg4y.fsf@gitster.dls.corp.google.com","subject":"Re: What's cooking in git.git (Oct 2013, #03; Wed, 16)","fromName":"Karsten Blees","fromEmail":"karsten.blees@gmail.com","sentAt":"2013-10-22T13:13:22Z","receivedAt":"2013-10-22T13:13:22Z","isPatch":false,"sender":{"key":"karsten.blees@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1111200?v=4"},"body":"Am 18.10.2013 21:09, schrieb Junio C Hamano:\n> Karsten Blees <karsten.blees@gmail.com> writes:\n> \n>> The coredumps are caused by my patch #10, which free()s\n>> cache_entries when they are removed, in combination with ...\n> \n> Looking at that patch, it makes me wonder if remove_index_entry_at()\n> and replace_index_entry() should be the ones that frees the old\n> entry in the first place.  A caller may already have a ce pointing\n> at an old entry and use the information from old_ce to update a new\n> one after it installed it, e.g.\n> \n> \told_ce = ...\n>         new_ce = make_cache_entry(... old_ce->name, ...);\n>         replace_index_entry(... new_ce);\n> \tnew_ce->ce_mode = old_ce->cd_mode;\n> \tfree(old_ce);\n> \n> The same goes for the functions that remove the entry.\n> \n\nMoving free() to the callers or caller's callers would make it much more complicated (more places to change). Besides, most callers don't even have a reference to old_ce and simply remove by position. Of course, this doesn't prevent caller's caller's callers to keep a reference to a removed / replaced entry, as found by Thomas.\n\n> \n> Going forward, I do agree with your patch #10 that removal or\n> replacing that may make an existing entry unreferenced should free\n> entries that are no longer used, and \"use after free\" should be\n> forbidden.\n> \n\nOK, I'll spend some more time analyzing the call hierarchies to see if there are more uses of removed cache_entries. I'll try to post an updated v4 by the end of the week.\n\nKarsten\n"}]}