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

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

[Junio C Hamano]

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

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.

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

[Karsten Blees]

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.

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

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

[Junio C Hamano]

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.

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

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

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

[Karsten Blees]

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)?


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"));
 }

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

[Junio C Hamano]

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.

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

> -       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"));



>  }

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

[Jens Lehmann]

Am 18.10.2013 02:42, schrieb Karsten Blees:
> 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.

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

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

[Jens Lehmann]

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;
>> -       }
> 
> 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 ;-)

>> -       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"));

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

[Jens Lehmann]

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

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

[Karsten Blees]

Am 18.10.2013 21:09, schrieb Junio C Hamano:
> 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.

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