threads / discuss / 35155

What's cooking in git.git (Oct 2013, #03; Wed, 16)

Subject: What's cooking in git.git (Oct 2013, #03; Wed, 16)

## tl;dr

10 messages between Oct 16, 2013 and Oct 22, 2013.

replies: 9people: 3as markdown or json

Junio C Hamano· Oct 16, 2013, 21:43 UTC · lore

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

I think I correctly inherited all the topics Jonathan kept track of during my absence (big thanks to Jonathan); if a topic that has been in his tree is missing, please holler.

I am chewing through the list backlog but still have a long way to go. Please be patient for the rest of the week.

You can find the changes described here in the integration branches of the repositories listed at

    http://git-blame.blogspot.com/p/git-public-repositories.html

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

* fc/styles (2013-10-16) 7 commits
 - block-sha1/sha1.c: have SP around arithmetic operators
 - base85.c: have SP around arithmetic operators
 - archive.c: have SP around arithmetic operators
 - alloc.c: have SP around arithmetic operators
 - abspath.c: have SP around arithmetic operators
 - alias: have SP around arithmetic operators
 - C: have space around && and || operators
 C coding style fixes.  The ones near the tip have not been sent to
 the list yet (they cover the same kind of style violation as the
 second one) and I may either send them or drop some of them if they
 turn out to conflict with other work in flight---I still haven't
 caught up with the backlog and do not know.
* jk/remote-literal-string-leakfix (2013-10-15) 1 commit
 - remote: do not copy "origin" string literal
 Will merge to 'next'.
* jk/split-broken-ident (2013-10-15) 2 commits
 - SQUASH??? remove reverse scan to simplify the logic
 - split_ident: parse timestamp from end of line
 Make the fall-back parsing of commit objects with broken author or
 committer lines more robust to pick up the timestamps.
 Will merge to 'next', perhaps after dropping the top one.
* sg/prompt-svn-remote-fix (2013-10-15) 1 commit
 - bash prompt: don't use '+=' operator in show upstream code path
 Bash portability fix.
 Will merge to 'next'.
* sc/doc-howto-dumb-http (2013-10-16) 1 commit
 - doc/howto: warn about (dumb)http server document being too old
 Will merge to 'next'.
* sg/t3600-nul-sha1-fix (2013-10-16) 1 commit
 - t3600: fix broken "choking git rm" test
 Will merge to 'next'.

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

* tr/merge-recursive-index-only (2013-07-07) 3 commits
 - merge-recursive: -Xindex-only to leave worktree unchanged
 - merge-recursive: untangle double meaning of o->call_depth
 - merge-recursive: remove dead conditional in update_stages()
 Holding until there is a caller to learn from.
* jc/ref-excludes (2013-09-03) 2 commits
 - document --exclude option
 - revision: introduce --exclude=<glob> to tame wildcards
 People often wished a way to tell "git log --branches" (and "git
 log --remotes --not --branches") to exclude some local branches
 from the expansion of "--branches" (similarly for "--tags", "--all"
 and "--glob=<pattern>").  Now they have one.
 Needs a matching change to rev-parse.
* rv/send-email-cache-generated-mid (2013-08-21) 2 commits
 - git-send-email: Cache generated message-ids, use them when prompting
 - git-send-email: add optional 'choices' parameter to the ask sub
* rj/read-default-config-in-show-ref-pack-refs (2013-06-17) 3 commits
 - ### DONTMERGE: needs better explanation on what config they need
 - pack-refs.c: Add missing call to git_config()
 - show-ref.c: Add missing call to git_config()
 The changes themselves are probably good, but it is unclear what
 basic setting needs to be read for which exact operation.
 Waiting for clarification.
 $gmane/228294
* jh/shorten-refname (2013-05-07) 4 commits
 - t1514: refname shortening is done after dereferencing symbolic refs
 - shorten_unambiguous_ref(): Fix shortening refs/remotes/origin/HEAD to origin
 - t1514: Demonstrate failure to correctly shorten "refs/remotes/origin/HEAD"
 - t1514: Add tests of shortening refnames in strict/loose mode
 When remotes/origin/HEAD is not a symbolic ref, "rev-parse
 --abbrev-ref remotes/origin/HEAD" ought to show "origin", not
 "origin/HEAD", which is fixed with this series (if it is a symbolic
 ref that points at remotes/origin/something, then it should show
 "origin/something" and it already does).
 Expecting a reroll, as an early part of a larger series.
 $gmane/225137
* jc/format-patch (2013-04-22) 2 commits
 - format-patch: --inline-single
 - format-patch: rename "no_inline" field
 A new option to send a single patch to the standard output to be
 appended at the bottom of a message.  I personally have no need for
 this, but it was easy enough to cobble together.  Tests, docs and
 stripping out more MIMEy stuff are left as exercises to interested
 parties.
* jk/gitweb-utf8 (2013-04-08) 4 commits
 - gitweb: Fix broken blob action parameters on blob/commitdiff pages
 - gitweb: Don't append ';js=(0|1)' to external links
 - gitweb: Make feed title valid utf8
 - gitweb: Fix utf8 encoding for blob_plain, blobdiff_plain, commitdiff_plain, and patch
 Various fixes to gitweb.
 Drew Northup volunteered to take a look into this.
 $gmane/226216
* jc/show-branch (2013-06-07) 5 commits
 - show-branch: use commit slab to represent bitflags of arbitrary width
 - show-branch.c: remove "all_mask"
 - show-branch.c: abstract out "flags" operation
 - show-branch.c: lift all_mask/all_revs to a global static
 - show-branch.c: update comment style
 Waiting for the final step to lift the hard-limit before sending it out.

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

* mg/more-textconv (2013-05-10) 7 commits
  (merged to 'next' on 2013-10-14 at 8a12490)
 + grep: honor --textconv for the case rev:path
 + grep: allow to use textconv filters
 + t7008: demonstrate behavior of grep with textconv
 + cat-file: do not die on --textconv without textconv filters
 + show: honor --textconv for blobs
 + diff_opt: track whether flags have been set explicitly
 + t4030: demonstrate behavior of show with textconv
 Make "git grep" and "git show" pay attention to --textconv when
 dealing with blob objects.
* ak/submodule-foreach-quoting (2013-09-27) 1 commit
  (merged to 'next' on 2013-10-14 at d77c5f1)
 + submodule foreach: skip eval for more than one argument
 A behavior change, but a worthwhile one: "git submodule foreach"
 was treating its arguments as part of a single command to be
 concatenated and passed to a shell, making writing buggy
 scripts too easy.
 This patch preserves the old "just pass it to the shell" behavior
 when a single argument is passed to 'git submodule foreach' and
 moves to a new "skip the shell and use the arguments passed
 unmolested" behavior when more than one argument is passed.
 The old behavior (always concatenating and passing to the shell)
 was similar to the 'ssh' command, while the new behavior (switching
 on the number of arguments) is what 'xterm -e' does.
 May need more thought to make sure this change is advertised well
 so that scripts that used multiple arguments but added their own
 extra layer of quoting are not broken.
* ew/keepalive (2013-10-16) 2 commits
  (merged to 'next' on 2013-10-16 at 56fd9f3)
 + http: use curl's tcp keepalive if available
  (merged to 'next' on 2013-10-14 at 24d786f)
 + http: enable keepalive on TCP sockets
* hu/cherry-pick-previous-branch (2013-10-10) 1 commit
  (merged to 'next' on 2013-10-14 at 090934f)
 + cherry-pick: handle "-" after parsing options
 "git cherry-pick" without further options would segfault.
 Could use a follow-up to handle '-' after argv[1] better.
* jc/pack-objects (2013-02-04) 1 commit
  (merged to 'next' on 2013-10-14 at 8e8feb6)
 + pack-objects: shrink struct object_entry
* jc/prompt-upstream (2013-10-14) 1 commit
  (merged to 'next' on 2013-10-14 at 270ef7b)
 + git-prompt.sh: optionally show upstream branch name
 An enhancement to the GIT_PS1_SHOWUPSTREAM facility.
* jk/http-auth-redirects (2013-10-14) 9 commits
 - remote-curl: rewrite base url from info/refs redirects
 - remote-curl: store url as a strbuf
 - remote-curl: make refs_url a strbuf
 - http: update base URLs when we see redirects
 - http: provide effective url to callers
 - http: hoist credential request out of handle_curl_result
  (merged to 'next' on 2013-10-14 at a0642be)
 + http: refactor options to http_get_*
 + http_request: factor out curlinfo_strbuf
 + http_get_file: style fixes
 Handle the case where http transport gets redirected during the
 authorization request better.
 Will merge to 'next'.
* jl/submodule-mv (2013-10-13) 1 commit
 - mv: Fix spurious warning when moving a file in presence of submodules
 Moving a regular file in a repository with a .gitmodules file was
 producing a warning 'Could not find section in .gitmodules where
 path=<filename>'.
 jrneider: "The test can use a little cleanup.  Otherwise looks good".
* yt/shortened-rename (2013-10-13) 1 commit
 - diff.c: keep arrow(=>) on show_stats()'s shortened filename part to make rename visible.
 Reroll v6 hasn't been picked up yet.
* bc/gnome-keyring (2013-10-16) 16 commits
 - contrib/git-credential-gnome-keyring.c: support really ancient gnome-keyring
 - contrib/git-credential-gnome-keyring.c: support ancient gnome-keyring
 - contrib/git-credential-gnome-keyring.c: report failure to store password
 - contrib/git-credential-gnome-keyring.c: use glib messaging functions
 - contrib/git-credential-gnome-keyring.c: use glib memory allocation functions
 - contrib/git-credential-gnome-keyring.c: use secure memory for reading passwords
 - contrib/git-credential-gnome-keyring.c: use secure memory functions for passwds
 - contrib/git-credential-gnome-keyring.c: use gnome helpers in keyring_object()
 - contrib/git-credential-gnome-keyring.c: set Gnome application name
 - contrib/git-credential-gnome-keyring.c: ensure buffer is non-empty before accessing
 - contrib/git-credential-gnome-keyring.c: strlen() returns size_t, not ssize_t
 - contrib/git-credential-gnome-keyring.c: exit non-zero when called incorrectly
 - contrib/git-credential-gnome-keyring.c: add static where applicable
 - contrib/git-credential-gnome-keyring.c: *style* use "if ()" not "if()" etc.
 - contrib/git-credential-gnome-keyring.c: remove unused die() function
 - contrib/git-credential-gnome-keyring.c: remove unnecessary pre-declarations
 Cleanups and tweaks for credential handling to work with ancient versions
 of the gnome-keyring library that are still in use.
 Will merge to 'next'.
* kb/fast-hashmap (2013-09-25) 6 commits
 - fixup! diffcore-rename.c: simplify finding exact renames
 - diffcore-rename.c: use new hash map implementation
 - diffcore-rename.c: simplify finding exact renames
 - diffcore-rename.c: move code around to prepare for the next patch
 - buitin/describe.c: use new hash map implementation
 - add a hashtable implementation that supports O(1) removal
 Improvements to our hash table to get it to meet the needs of the
 msysgit fscache project, with some nice performance improvements.
 Looks promising.  Needs style review and a sanity-check on the
 design before including in 'next'.  Expecting a reroll once review
 settles down.
* jc/revision-range-unpeel (2013-10-15) 1 commit
  (merged to 'next' on 2013-10-16 at d04ddfe)
 + revision: do not peel tags used in range notation
 "git rev-list --objects ^v1.0^ v1.0" gave v1.0 tag itself in the
 output, but "git rev-list --objects v1.0^..v1.0" did not.
* tg/perf-lib-test-perf-cleanup (2013-09-19) 2 commits
 - perf-lib: add test_perf_cleanup target
 - perf-lib: split starting the test from the execution
 Add test_perf_cleanup shell function to the perf suite, that allows
 the script writers to define a test with a clean-up action.
 Holding until needed.
* jk/format-patch-from (2013-09-20) 1 commit
  (merged to 'next' on 2013-09-20 at 0506530)
 + format-patch: print in-body "From" only when needed
 "format-patch --from=<whom>" forgot to omit unnecessary in-body
 from line, i.e. when <whom> is the same as the real author.
 Will merge to 'master'.
* es/name-hash-no-trailing-slash-in-dirs (2013-09-17) 4 commits
  (merged to 'next' on 2013-09-20 at 9633d9a)
 + dir: revert work-around for retired dangerous behavior
 + name-hash: stop storing trailing '/' on paths in index_state.dir_hash
 + employ new explicit "exists in index?" API
 + name-hash: refactor polymorphic index_name_exists()
 Clean up the internal of the name-hash mechanism used to work
 around case insensitivity on some filesystems to cleanly fix a
 long-standing API glitch where the caller of cache_name_exists()
 that ask about a directory with a counted string was required to
 have '/' at one location past the end of the string.
 Will merge to 'master'.
* po/dot-url (2013-10-15) 3 commits
  (merged to 'next' on 2013-10-15 at 312d0af)
 + doc/cli: make "dot repository" an independent bullet point
  (merged to 'next' on 2013-09-20 at 6a12786)
 + config doc: update dot-repository notes
 + doc: command line interface (cli) dot-repository dwimmery
 Explain how '.' can be used to refer to the "current repository"
 in the documentation.
 Will merge to 'master'.
* jc/upload-pack-send-symref (2013-09-17) 7 commits
  (merged to 'next' on 2013-10-16 at eb1ae25)
 + clone: test the new HEAD detection logic
 + connect: annotate refs with their symref information in get_remote_head()
 + connect.c: make parse_feature_value() static
 + upload-pack: send non-HEAD symbolic refs
 + upload-pack: send symbolic ref information as capability
 + upload-pack.c: do not pass confusing cb_data to mark_our_ref()
 + t5505: fix "set-head --auto with ambiguous HEAD" test
 One long-standing flaw in the pack transfer protocol used by "git
 clone" was that there was no way to tell the other end which branch
 "HEAD" points at, and the receiving end needed to guess.  A new
 capability has been defined in the pack protocol to convey this
 information so that cloning from a repository with more than one
 branches pointing at the same commit where the HEAD is at now
 reliably sets the initial branch in the resulting repository.
 Will merge to 'master'.
* jk/clone-progress-to-stderr (2013-09-18) 3 commits
  (merged to 'next' on 2013-09-25 at 137af9e)
 + clone: always set transport options
 + clone: treat "checking connectivity" like other progress
 + clone: send diagnostic messages to stderr
 Some progress and diagnostic messages from "git clone" were
 incorrectly sent to the standard output stream, not to the standard
 error stream.
 Will merge to 'master'.
* jx/relative-path-regression-fix (2013-10-14) 3 commits
 - Use simpler relative_path when set_git_dir
  (merged to 'next' on 2013-10-14 at 704b9ee)
 + relative_path should honor dos-drive-prefix
 + test: use unambigous leading path (/foo) for MSYS
 Waiting for the review to settle.
* jc/checkout-detach-doc (2013-09-11) 1 commit
  (merged to 'next' on 2013-09-17 at 438cf13)
 + checkout: update synopsys and documentation on detaching HEAD
 "git checkout [--detach] <commit>" was listed poorly in the
 synopsis section of its documentation.
 Will merge to 'master'.
* jk/trailing-slash-in-pathspec (2013-09-13) 2 commits
  (merged to 'next' on 2013-09-17 at 18fe277)
 + reset: handle submodule with trailing slash
 + rm: re-use parse_pathspec's trailing-slash removal
 Code refactoring.
 Will merge to 'master'.
* lc/filter-branch-too-many-refs (2013-09-12) 1 commit
  (merged to 'next' on 2013-09-17 at 31cd01a)
 + Allow git-filter-branch to process large repositories with lots of branches.
 "git filter-branch" in a repository with many refs blew limit of
 command line length.
 Will merge to 'master'.
* np/pack-v4 (2013-09-18) 90 commits
 - packv4-parse.c: add tree offset caching
 - t1050: replace one instance of show-index with verify-pack
 - index-pack, pack-objects: allow creating .idx v2 with .pack v4
 - unpack-objects: decode v4 trees
 - unpack-objects: allow to save processed bytes to a buffer
 - ...
 Nico and Duy advancing the eternal vaporware pack-v4.  This is here
 primarily for wider distribution of the preview edition.
* sb/repack-in-c (2013-09-17) 3 commits
  (merged to 'next' on 2013-09-25 at 7c47036)
 + repack: improve warnings about failure of renaming and removing files
 + repack: retain the return value of pack-objects
 + repack: rewrite the shell script in C
 Rerolled, and I think it is in a reasonably good shape.
 Will merge to 'master'.
* jc/reflog-doc (2013-06-19) 1 commit
  (merged to 'next' on 2013-09-25 at 4eb0c14)
 + setup_reflog_action: document the rules for using GIT_REFLOG_ACTION
 Document rules to use GIT_REFLOG_ACTION variable in the scripted
 Porcelain.  git-rebase--interactive locally violates them, but it
 is a leaf user that does not call out to or dot-source other
 scripts, so it does not urgently need to be fixed.
 Will merge to 'master'.
* jn/add-2.0-u-A-sans-pathspec (2013-04-26) 1 commit
 - git add: -u/-A now affects the entire working tree
 Will cook in 'next' until Git 2.0.
* jc/core-checkstat-2.0 (2013-05-06) 1 commit
 - core.statinfo: remove as promised in Git 2.0
 Will cook in 'next' until Git 2.0.
* jc/push-2.0-default-to-simple (2013-06-18) 1 commit
 - push: switch default from "matching" to "simple"
 Will cook in 'next' until Git 2.0.
* jc/add-2.0-ignore-removal (2013-04-22) 1 commit
 - git add <pathspec>... defaults to "-A"
 Updated endgame for "git add <pathspec>" that defaults to "--all"
 aka "--no-ignore-removal".
 Will cook in 'next' until Git 2.0.
* jc/hold-diff-remove-q-synonym-for-no-deletion (2013-07-19) 1 commit
 - diff: remove "diff-files -q" in a version of Git in a distant future
 Will cook in 'next' until a distant future.

-------------------------------------------------- [Discarded]

* jc/pull-training-wheel (2013-07-19) 1 commit
  (merged to 'next' on 2013-08-28 at c39bd15)
 + pull: require choice between rebase/merge on non-fast-forward pull
 Make "git pull" (without arguments that say what branch to
 integrate from where) refuse with "it does not fast forward; choose
 between 'pull --merge' and 'pull --rebase'".
 This topic has been reverted from 'next'.  Will wait for the
 conclusion of the discussion to seek a more user-friendly
 alternative; it is likely that it will be based on the simpler
 approach Felipe posted earlier.
Karsten Blees· Oct 17, 2013, 09:48 UTC · re: Junio C Hamano · lore

Re: What's cooking in git.git (Oct 2013, #03; Wed, 16)

Am 16.10.2013 23:43, schrieb Junio C Hamano:
Show 8 quoted lines
> * kb/fast-hashmap (2013-09-25) 6 commits
>  - fixup! diffcore-rename.c: simplify finding exact renames
>  - diffcore-rename.c: use new hash map implementation
>  - diffcore-rename.c: simplify finding exact renames
>  - diffcore-rename.c: move code around to prepare for the next patch
>  - buitin/describe.c: use new hash map implementation
>  - add a hashtable implementation that supports O(1) removal
> 
I posted a much more complete v3 [1], but somehow missed Jonathan's fixup! commit.
Btw., 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?
[1] http://thread.gmane.org/gmane.comp.version-control.git/235644
Junio C Hamano· Oct 17, 2013, 20:40 UTC · re: Karsten Blees · lore

Re: What's cooking in git.git (Oct 2013, #03; Wed, 16)

Karsten Blees <karsten.blees@gmail.com> writes:
Show 11 quoted lines
> Am 16.10.2013 23:43, schrieb Junio C Hamano:
>> * kb/fast-hashmap (2013-09-25) 6 commits
>>  - fixup! diffcore-rename.c: simplify finding exact renames
>>  - diffcore-rename.c: use new hash map implementation
>>  - diffcore-rename.c: simplify finding exact renames
>>  - diffcore-rename.c: move code around to prepare for the next patch
>>  - buitin/describe.c: use new hash map implementation
>>  - add a hashtable implementation that supports O(1) removal
>> 
>
> I posted a much more complete v3 [1], but somehow missed Jonathan's fixup! commit.
Thanks; I'll replace the above with v3 and squash the fix-up in.
Junio C Hamano· Oct 17, 2013, 21:07 UTC · re: Junio C Hamano · lore

Re: What's cooking in git.git (Oct 2013, #03; Wed, 16)

Junio C Hamano <gitster@pobox.com> writes:
Show 15 quoted lines
> Karsten Blees <karsten.blees@gmail.com> writes:
>
>> Am 16.10.2013 23:43, schrieb Junio C Hamano:
>>> * kb/fast-hashmap (2013-09-25) 6 commits
>>>  - fixup! diffcore-rename.c: simplify finding exact renames
>>>  - diffcore-rename.c: use new hash map implementation
>>>  - diffcore-rename.c: simplify finding exact renames
>>>  - diffcore-rename.c: move code around to prepare for the next patch
>>>  - buitin/describe.c: use new hash map implementation
>>>  - add a hashtable implementation that supports O(1) removal
>>> 
>>
>> I posted a much more complete v3 [1], but somehow missed Jonathan's fixup! commit.
>
> Thanks; I'll replace the above with v3 and squash the fix-up in.

Interestingly, v3 applied on 'maint' and then merged to 'master' seems to break t3600 and t7001 with a coredump.

It would conflict with es/name-hash-no-trailing-slash-in-dirs that has been cooking in 'next', too; the resolution might be trivial but I didn't look too deeply into it.

Karsten Blees· Oct 18, 2013, 00:42 UTC · re: Junio C Hamano · lore

Re: What's cooking in git.git (Oct 2013, #03; Wed, 16)

Am 17.10.2013 23:07, schrieb Junio C Hamano:
Show 25 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> Karsten Blees <karsten.blees@gmail.com> writes:
>>
>>> Am 16.10.2013 23:43, schrieb Junio C Hamano:
>>>> * kb/fast-hashmap (2013-09-25) 6 commits
>>>>  - fixup! diffcore-rename.c: simplify finding exact renames
>>>>  - diffcore-rename.c: use new hash map implementation
>>>>  - diffcore-rename.c: simplify finding exact renames
>>>>  - diffcore-rename.c: move code around to prepare for the next patch
>>>>  - buitin/describe.c: use new hash map implementation
>>>>  - add a hashtable implementation that supports O(1) removal
>>>>
>>>
>>> I posted a much more complete v3 [1], but somehow missed Jonathan's fixup! commit.
>>
>> Thanks; I'll replace the above with v3 and squash the fix-up in.
> 
> Interestingly, v3 applied on 'maint' and then merged to 'master'
> seems to break t3600 and t7001 with a coredump.
> 
> It would conflict with es/name-hash-no-trailing-slash-in-dirs that
> has been cooking in 'next', too; the resolution might be trivial but
> I didn't look too deeply into it.
> 

I've pushed a rebased version to https://github.com/kblees/git/commits/kb/hashmap-v3-next (no changes yet except for Jonathan's fixup in #04 and merge resolution).

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.
Can't we just use add_file_to_cache here (which replaces cache_entries by creating a copy)?
diff --git a/submodule.c b/submodule.c
index 1905d75..e388487 100644
--- a/submodule.c
+++ b/submodule.c
@@ -116,30 +116,7 @@ int remove_path_from_gitmodules(const char *path)
 
 void stage_updated_gitmodules(void)
 {
-       struct strbuf buf = STRBUF_INIT;
-       struct stat st;
-       int pos;
-       struct cache_entry *ce;
-       int namelen = strlen(".gitmodules");
-
-       pos = cache_name_pos(".gitmodules", namelen);
-       if (pos < 0) {
-               warning(_("could not find .gitmodules in index"));
-               return;
-       }
-       ce = active_cache[pos];
-       ce->ce_flags = namelen;
-       if (strbuf_read_file(&buf, ".gitmodules", 0) < 0)
-               die(_("reading updated .gitmodules failed"));
-       if (lstat(".gitmodules", &st) < 0)
-               die_errno(_("unable to stat updated .gitmodules"));
-       fill_stat_cache_info(ce, &st);
-       ce->ce_mode = ce_mode_from_stat(ce, st.st_mode);
-       if (remove_cache_entry_at(pos) < 0)
-               die(_("unable to remove .gitmodules from index"));
-       if (write_sha1_file(buf.buf, buf.len, blob_type, ce->sha1))
-               die(_("adding updated .gitmodules failed"));
-       if (add_cache_entry(ce, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE))
+       if (add_file_to_cache(".gitmodules", 0))
                die(_("staging updated .gitmodules failed"));
 }
Junio C Hamano· Oct 18, 2013, 19:09 UTC · re: Karsten Blees · lore

Re: What's cooking in git.git (Oct 2013, #03; Wed, 16)

Karsten Blees <karsten.blees@gmail.com> writes:
> The coredumps are caused by my patch #10, which free()s
> cache_entries when they are removed, in combination with ...

Looking at that patch, it makes me wonder if remove_index_entry_at() and replace_index_entry() should be the ones that frees the old entry in the first place. A caller may already have a ce pointing at an old entry and use the information from old_ce to update a new one after it installed it, e.g.

	old_ce = ...
        new_ce = make_cache_entry(... old_ce->name, ...);
        replace_index_entry(... new_ce);
	new_ce->ce_mode = old_ce->cd_mode;
	free(old_ce);
The same goes for the functions that remove the entry.

But I am probably biased saying this, because in the old days, cache entries could never be freed (they were carved out of a contiguous region of memory, mmapped from the index file). These days, we parse and run ntoh*() on the on-disk cache entries to create in-core form, and the "cache entries should never be freed" is no longer true, but I would not be surprised if there are still some code leftover that relies on "use after free" being safe, leaking unused cache entries.

Going forward, I do agree with your patch #10 that removal or replacing that may make an existing entry unreferenced should free entries that are no longer used, and "use after free" should be forbidden.

Show 22 quoted lines
> Can't we just use add_file_to_cache here (which replaces
> cache_entries by creating a copy)?
>
> diff --git a/submodule.c b/submodule.c
> index 1905d75..e388487 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -116,30 +116,7 @@ int remove_path_from_gitmodules(const char *path)
>  
>  void stage_updated_gitmodules(void)
>  {
> -       struct strbuf buf = STRBUF_INIT;
> -       struct stat st;
> -       int pos;
> -       struct cache_entry *ce;
> -       int namelen = strlen(".gitmodules");
> -
> -       pos = cache_name_pos(".gitmodules", namelen);
> -       if (pos < 0) {
> -               warning(_("could not find .gitmodules in index"));
> -               return;
> -       }

I think the remainder is (morally) equivalent between the original and a single "add-file-to-cache" call, and the version after your "how about this" patch in the message I am responding to looks more correct (e.g. why does the original lstat after it has read the file?).

But this warning may want to stay, no?
Show 15 quoted lines
> -       ce = active_cache[pos];
> -       ce->ce_flags = namelen;
> -       if (strbuf_read_file(&buf, ".gitmodules", 0) < 0)
> -               die(_("reading updated .gitmodules failed"));
> -       if (lstat(".gitmodules", &st) < 0)
> -               die_errno(_("unable to stat updated .gitmodules"));
> -       fill_stat_cache_info(ce, &st);
> -       ce->ce_mode = ce_mode_from_stat(ce, st.st_mode);
> -       if (remove_cache_entry_at(pos) < 0)
> -               die(_("unable to remove .gitmodules from index"));
> -       if (write_sha1_file(buf.buf, buf.len, blob_type, ce->sha1))
> -               die(_("adding updated .gitmodules failed"));
> -       if (add_cache_entry(ce, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE))
> +       if (add_file_to_cache(".gitmodules", 0))
>                 die(_("staging updated .gitmodules failed"));
>  }
Jens Lehmann· Oct 18, 2013, 19:52 UTC · re: Junio C Hamano · lore

Re: What's cooking in git.git (Oct 2013, #03; Wed, 16)

Am 18.10.2013 21:09, schrieb Junio C Hamano:
Show 29 quoted lines
> Karsten Blees <karsten.blees@gmail.com> writes:
>> Can't we just use add_file_to_cache here (which replaces
>> cache_entries by creating a copy)?
>>
>> diff --git a/submodule.c b/submodule.c
>> index 1905d75..e388487 100644
>> --- a/submodule.c
>> +++ b/submodule.c
>> @@ -116,30 +116,7 @@ int remove_path_from_gitmodules(const char *path)
>>  
>>  void stage_updated_gitmodules(void)
>>  {
>> -       struct strbuf buf = STRBUF_INIT;
>> -       struct stat st;
>> -       int pos;
>> -       struct cache_entry *ce;
>> -       int namelen = strlen(".gitmodules");
>> -
>> -       pos = cache_name_pos(".gitmodules", namelen);
>> -       if (pos < 0) {
>> -               warning(_("could not find .gitmodules in index"));
>> -               return;
>> -       }
> 
> I think the remainder is (morally) equivalent between the original
> and a single "add-file-to-cache" call, and the version after your
> "how about this" patch in the message I am responding to looks more
> correct (e.g. why does the original lstat after it has read the
> file?).

Cargo cult programming. I was looking at other code manipulating the index (as Documentation/technical/api-in-core-index.txt is rather terse ;-) and concluded I would need to read the possibly updated st.st_mode, in case updating the config file would have changed that.

> But this warning may want to stay, no?

Of course you are right on this one. All test ran successfully with this patch, so I think adding one for that warning makes sense too. And as that is submodule related stuff I volunteer for fixing all this ;-)

Show 15 quoted lines
>> -       ce = active_cache[pos];
>> -       ce->ce_flags = namelen;
>> -       if (strbuf_read_file(&buf, ".gitmodules", 0) < 0)
>> -               die(_("reading updated .gitmodules failed"));
>> -       if (lstat(".gitmodules", &st) < 0)
>> -               die_errno(_("unable to stat updated .gitmodules"));
>> -       fill_stat_cache_info(ce, &st);
>> -       ce->ce_mode = ce_mode_from_stat(ce, st.st_mode);
>> -       if (remove_cache_entry_at(pos) < 0)
>> -               die(_("unable to remove .gitmodules from index"));
>> -       if (write_sha1_file(buf.buf, buf.len, blob_type, ce->sha1))
>> -               die(_("adding updated .gitmodules failed"));
>> -       if (add_cache_entry(ce, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE))
>> +       if (add_file_to_cache(".gitmodules", 0))
>>                 die(_("staging updated .gitmodules failed"));
Jens Lehmann· Oct 18, 2013, 20:31 UTC · re: Jens Lehmann · lore

[PATCH] submodule: don't access the .gitmodules cache entry after removing it

Commit 5fee995244e introduced the stage_updated_gitmodules() function to add submodule configuration updates to the index. It assumed that even after calling remove_cache_entry_at() the same cache entry would still be valid. This was true in the old days, as cache entries could never be freed, but that is not so sure in the present as there is ongoing work to free removed cache entries, which makes this code segfault.

Fix that by calling add_file_to_cache() instead of open coding it. Also remove the "could not find .gitmodules in index" warning, as that won't happen in regular use cases (and by then just silently adding it to the index we do the right thing).

Thanks-to: Karsten Blees <karsten.blees@gmail.com>
Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
---
Am 18.10.2013 21:52, schrieb Jens Lehmann:
Show 31 quoted lines
> Am 18.10.2013 21:09, schrieb Junio C Hamano:
>> Karsten Blees <karsten.blees@gmail.com> writes:
>>> Can't we just use add_file_to_cache here (which replaces
>>> cache_entries by creating a copy)?
>>>
>>> diff --git a/submodule.c b/submodule.c
>>> index 1905d75..e388487 100644
>>> --- a/submodule.c
>>> +++ b/submodule.c
>>> @@ -116,30 +116,7 @@ int remove_path_from_gitmodules(const char *path)
>>>  
>>>  void stage_updated_gitmodules(void)
>>>  {
>>> -       struct strbuf buf = STRBUF_INIT;
>>> -       struct stat st;
>>> -       int pos;
>>> -       struct cache_entry *ce;
>>> -       int namelen = strlen(".gitmodules");
>>> -
>>> -       pos = cache_name_pos(".gitmodules", namelen);
>>> -       if (pos < 0) {
>>> -               warning(_("could not find .gitmodules in index"));
>>> -               return;
>>> -       }
>>
>> But this warning may want to stay, no?
> 
> Of course you are right on this one. All test ran successfully
> with this patch, so I think adding one for that warning makes
> sense too. And as that is submodule related stuff I volunteer
> for fixing all this ;-)

And after digging deeper and trying to come up with a test case for that I think we can safely remove this warning too. When not having a .gitmodules file update_path_in_gitmodules() and remove_path_from_gitmodules() won't do anything and signal that there is no need for stage_updated_gitmodules(). And if we get a user later that wants to create .gitmodules file (e.g. to register a new submodule) a warning that that file had not been known before isn't helpful. So the only cases triggering that warning would be when the .gitmodules is on the filesystem but not in the index, and I can't think of a regular use case where this happens. So let's drop it too.

 submodule.c | 25 +------------------------
 1 file changed, 1 insertion(+), 24 deletions(-)
diff --git a/submodule.c b/submodule.c
index 1905d75..e388487 100644
--- a/submodule.c
+++ b/submodule.c
@@ -116,30 +116,7 @@ int remove_path_from_gitmodules(const char *path)

 void stage_updated_gitmodules(void)
 {
-	struct strbuf buf = STRBUF_INIT;
-	struct stat st;
-	int pos;
-	struct cache_entry *ce;
-	int namelen = strlen(".gitmodules");
-
-	pos = cache_name_pos(".gitmodules", namelen);
-	if (pos < 0) {
-		warning(_("could not find .gitmodules in index"));
-		return;
-	}
-	ce = active_cache[pos];
-	ce->ce_flags = namelen;
-	if (strbuf_read_file(&buf, ".gitmodules", 0) < 0)
-		die(_("reading updated .gitmodules failed"));
-	if (lstat(".gitmodules", &st) < 0)
-		die_errno(_("unable to stat updated .gitmodules"));
-	fill_stat_cache_info(ce, &st);
-	ce->ce_mode = ce_mode_from_stat(ce, st.st_mode);
-	if (remove_cache_entry_at(pos) < 0)
-		die(_("unable to remove .gitmodules from index"));
-	if (write_sha1_file(buf.buf, buf.len, blob_type, ce->sha1))
-		die(_("adding updated .gitmodules failed"));
-	if (add_cache_entry(ce, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE))
+	if (add_file_to_cache(".gitmodules", 0))
 		die(_("staging updated .gitmodules failed"));
 }
-- 
1.8.4.1.545.gc07df9e
Karsten Blees· Oct 22, 2013, 13:13 UTC · re: Junio C Hamano · lore

Re: What's cooking in git.git (Oct 2013, #03; Wed, 16)

Am 18.10.2013 21:09, schrieb Junio C Hamano:
Show 19 quoted lines
> Karsten Blees <karsten.blees@gmail.com> writes:
> 
>> The coredumps are caused by my patch #10, which free()s
>> cache_entries when they are removed, in combination with ...
> 
> Looking at that patch, it makes me wonder if remove_index_entry_at()
> and replace_index_entry() should be the ones that frees the old
> entry in the first place.  A caller may already have a ce pointing
> at an old entry and use the information from old_ce to update a new
> one after it installed it, e.g.
> 
> 	old_ce = ...
>         new_ce = make_cache_entry(... old_ce->name, ...);
>         replace_index_entry(... new_ce);
> 	new_ce->ce_mode = old_ce->cd_mode;
> 	free(old_ce);
> 
> The same goes for the functions that remove the entry.
> 
Moving 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.
Show 6 quoted lines
> 
> Going forward, I do agree with your patch #10 that removal or
> replacing that may make an existing entry unreferenced should free
> entries that are no longer used, and "use after free" should be
> forbidden.
> 
OK, 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.
Karsten
Jens Lehmann· Oct 18, 2013, 19:37 UTC · re: Karsten Blees · lore

Re: What's cooking in git.git (Oct 2013, #03; Wed, 16)

Am 18.10.2013 02:42, schrieb Karsten Blees:
Show 33 quoted lines
> Am 17.10.2013 23:07, schrieb Junio C Hamano:
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> Karsten Blees <karsten.blees@gmail.com> writes:
>>>
>>>> Am 16.10.2013 23:43, schrieb Junio C Hamano:
>>>>> * kb/fast-hashmap (2013-09-25) 6 commits
>>>>>  - fixup! diffcore-rename.c: simplify finding exact renames
>>>>>  - diffcore-rename.c: use new hash map implementation
>>>>>  - diffcore-rename.c: simplify finding exact renames
>>>>>  - diffcore-rename.c: move code around to prepare for the next patch
>>>>>  - buitin/describe.c: use new hash map implementation
>>>>>  - add a hashtable implementation that supports O(1) removal
>>>>>
>>>>
>>>> I posted a much more complete v3 [1], but somehow missed Jonathan's fixup! commit.
>>>
>>> Thanks; I'll replace the above with v3 and squash the fix-up in.
>>
>> Interestingly, v3 applied on 'maint' and then merged to 'master'
>> seems to break t3600 and t7001 with a coredump.
>>
>> It would conflict with es/name-hash-no-trailing-slash-in-dirs that
>> has been cooking in 'next', too; the resolution might be trivial but
>> I didn't look too deeply into it.
>>
> 
> I've pushed a rebased version to https://github.com/kblees/git/commits/kb/hashmap-v3-next
> (no changes yet except for Jonathan's fixup in #04 and merge resolution).
> 
> 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.
> 
> Can't we just use add_file_to_cache here (which replaces cache_entries by creating a copy)?

No objections from my side. Looks like we could also copy the cache entry just before remove_cache_entry_at() and use that copy afterwards, but your solution is so much shorter that I would really like to use it (unless someone more cache-savvy than me has any objections).

And by the way: this is the last use of remove_cache_entry_at(), would it make sense to remove that define while at it? Only the remove_index_entry_at() function it is defined to is currently used.

Show 41 quoted lines
> diff --git a/submodule.c b/submodule.c
> index 1905d75..e388487 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -116,30 +116,7 @@ int remove_path_from_gitmodules(const char *path)
>  
>  void stage_updated_gitmodules(void)
>  {
> -       struct strbuf buf = STRBUF_INIT;
> -       struct stat st;
> -       int pos;
> -       struct cache_entry *ce;
> -       int namelen = strlen(".gitmodules");
> -
> -       pos = cache_name_pos(".gitmodules", namelen);
> -       if (pos < 0) {
> -               warning(_("could not find .gitmodules in index"));
> -               return;
> -       }
> -       ce = active_cache[pos];
> -       ce->ce_flags = namelen;
> -       if (strbuf_read_file(&buf, ".gitmodules", 0) < 0)
> -               die(_("reading updated .gitmodules failed"));
> -       if (lstat(".gitmodules", &st) < 0)
> -               die_errno(_("unable to stat updated .gitmodules"));
> -       fill_stat_cache_info(ce, &st);
> -       ce->ce_mode = ce_mode_from_stat(ce, st.st_mode);
> -       if (remove_cache_entry_at(pos) < 0)
> -               die(_("unable to remove .gitmodules from index"));
> -       if (write_sha1_file(buf.buf, buf.len, blob_type, ce->sha1))
> -               die(_("adding updated .gitmodules failed"));
> -       if (add_cache_entry(ce, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE))
> +       if (add_file_to_cache(".gitmodules", 0))
>                 die(_("staging updated .gitmodules failed"));
>  }
> 
> --
> 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
> 

← back to recent threads