{"thread":{"id":"15186","subject":"What's cooking in git.git (Aug 2008, #07; Sat, 23)","startedAt":"2008-08-24T03:38:03Z","lastAt":"2008-08-25T21:27:23Z","messageCount":11,"participants":["Junio C Hamano","Johannes Schindelin","Stephen R. van den Berg"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"88345","messageId":"7v1w0ft750.fsf@gitster.siamese.dyndns.org","threadId":"15186","inReplyTo":null,"subject":"What's cooking in git.git (Aug 2008, #07; Sat, 23)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-24T03:38:03Z","receivedAt":"2008-08-24T03:38:03Z","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\nwith '-' are only in 'pu' while commits prefixed with '+' are\nin 'next'.\n\nThe topics list the commits in reverse chronological order.  The topics\nmeant to be merged to the maintenance series have \"maint-\" in their names.\n\nHere is a list of issues/topics we saw on the mailing list but haven't\nresulted in anything queuable in 'pu' yet.  They need further work by\ninterested parties:\n\n * Windows relocatable install\n\n   Steffen Prohaska ($gmane/92605), Johannes Sixt.\n\n * Haiku port\n\n   Andreas Färber ($gmane/92582)\n\n * ksh \"trap foo EXIT\" triggers on function return, loses exit status\n\n   Brandon Casey ($gmane/92873)\n\n * document webdav debugging tip with davfs2\n\n   Giovanni Funchal ($gmane/92745)\n\n * update \"rebase -i\" documentation with examples\n\n   Eric Hanchrow ($gmane/92669)\n\n * pre-push hook\n\n   Scott Chacon ($gmane/92900, $gmane/92936)\n\n * \"git commit --author=nickname\" expanding nickname from somewhere\n\n   Michael J Gruber ($gmane/93274)\n\n * Handling (possibly $HOME-) relative paths in config files\n\n   Karl Chen ($gmane/93250)\n\n * \"submodule sync\"\n\n   David Aguilar ($gmane/93265)\n\n * \"rev-list --bisect --first-parent\"\n\n   Avery Pennarun, me ($gmane/93420)\n\n * \"apply --include\"\n\n   Joe Perches ($gmane/93505)\n\n----------------------------------------------------------------\n[New Topics]\n\n* bd/blame (Thu Aug 21 18:22:01 2008 -0500) 5 commits\n . Use xdiff caching to improve git blame performance\n . Allow xdiff machinery to cache hash results for a file\n . Always initialize xpparam_t to 0\n . Bypass textual patch generation and parsing in git blame\n . Allow alternate \"low-level\" emit function from xdl_diff\n\nRéne had a good comments on how the callback is structured.\n\n* jc/maint-name-hash-clear (Sat Aug 23 13:05:10 2008 -0700) 1 commit\n - discard_cache: reset lazy name_hash bit\n\nI spotted this by accident while working on something unrelated.\n\nWhen a program calls discard_cache() to read the index again, we do not\nproperly re-initialize the name_hash structure that is used by the case\ninsensitivitly logic.  This _might_ improve issues people may be having on\ncase insensitive filesystems.  I dunno.\n\n* mv/maint-merge-fix (Sat Aug 23 12:56:57 2008 -0700) 1 commit\n + merge: fix numerus bugs around \"trivial merge\" area\n\n* ml/submodule (Thu Aug 21 19:54:01 2008 -0400) 2 commits\n + git-submodule.sh - Remove trailing / from URL if found\n + git-submodule.sh - Remove trailing / from URL if found\n\nSoon to be in 'master', I guess.\n\n* np/verify-pack (Fri Aug 22 15:45:53 2008 -0400) 1 commit\n + discard revindex data when pack list changes\n\n* jc/add-ita (Thu Aug 21 01:44:53 2008 -0700) 3 commits\n - git-add --intent-to-add (-N)\n - cached_object: learn empty blob\n - sha1_object_info(): pay attention to cached objects\n\nTeaches \"git add\" to record only the intent to add a path later.\nI think this is better done without the hardcoded empty blob object.\n\n----------------------------------------------------------------\n[Stalled -- Needs Action to Proceed (or to be dropped)]\n\n* sb/daemon (Thu Aug 14 20:02:20 2008 +0200) 4 commits\n - git-daemon: rewrite kindergarden, new option --max-connections\n - git-daemon: Simplify dead-children reaping logic\n - git-daemon: use LOG_PID, simplify logging code\n - git-daemon: call logerror() instead of error()\n\nCan somebody who actually runs the daemon standalone comment on this one?\n\n* jc/cc-ld-dynpath (Sat Aug 16 15:01:23 2008 +0200) 2 commits\n - configure: auto detect dynamic library path switches\n - Makefile: Allow CC_LD_DYNPATH to be overriden\n\nNeeds success reports from people who do use user-defined dynamic library\npath when they build their \"git\" before this series can go anywhere.\n\n* lt/time-reject-fractional-seconds (Sat Aug 16 21:25:40 2008 -0700) 1 commit\n - date/time: do not get confused by fractional seconds\n\nLinus hints further enhancements as \"the right way\", so let's see if\nsomebody else steps up and tries it before merging this to 'next'.\n\n* sp/smart-http (Sun Aug 3 00:25:17 2008 -0700) 2 commits\n - [do not merge -- original version] Add Git-aware CGI for Git-aware\n   smart HTTP transport\n - Add backdoor options to receive-pack for use in Git-aware CGI\n\nThe \"magic\" detection protocol was revised to use POST to info/refs; the\ntop one queued is from before that discussion.\n\n----------------------------------------------------------------\n[Actively Cooking]\n\n* cc/bisect (Fri Aug 22 05:52:29 2008 +0200) 2 commits\n - bisect: only check merge bases when needed\n - bisect: test merge base if good rev is not an ancestor of bad rev\n\n* mv/merge-recursive (Tue Aug 12 22:14:00 2008 +0200) 3 commits\n . Make builtin-revert.c use merge_recursive_generic()\n . merge-recursive.c: Add more generic merge_recursive_generic()\n . Split out merge_recursive() to merge-recursive.c\n\nMiklos will be working on updates based on comments.\n\n* lw/gitweb (Mon Aug 18 21:39:49 2008 +0200) 3 commits\n . gitweb: use new Git::Repo API, and add optional caching\n . add new Perl API: Git::Repo, Git::Commit, Git::Tag, and\n   Git::RepoRoot\n . gitweb: add test suite with Test::WWW::Mechanize::CGI\n\nTentatively dropped as its tests do not seem to pass and I have no time to\nlook at them.\n\n* jc/diff-prefix (Mon Aug 18 20:08:09 2008 -0700) 1 commit\n - diff: vary default prefix depending on what are compared\n\nAs some people may have noticed, I've been running with this one when\nsending out \"How about this\" patches to the discussion threads.\n\n* sp/missing-thin-base (Tue Aug 12 11:31:06 2008 -0700) 1 commit\n + pack-objects: Allow missing base objects when creating thin packs\n\n* tr/filter-branch (Tue Aug 12 10:45:59 2008 +0200) 3 commits\n + filter-branch: use --simplify-merges\n + filter-branch: fix ref rewriting with --subdirectory-filter\n + filter-branch: Extend test to show rewriting bug\n\nFixes a longstanding filter branch bug.  Success stories?\n\n* jc/post-simplify (Fri Aug 15 01:34:51 2008 -0700) 8 commits\n - revision --simplify-merges: incremental simplification\n - revision --simplify-merges: prepare for incremental simplification\n - revision --simplify-merges: make it a no-op without pathspec\n + revision --simplify-merges: do not leave commits unprocessed\n + revision --simplify-merges: use decoration instead of commit->util\n   field\n + Topo-sort before --simplify-merges\n + revision traversal: show full history with merge simplification\n + revision.c: whitespace fix\n\n\"log --full-history\" is with too much clutter, \"log\" itself is too cleverer\nthan some people, and here is the middle level of merge simplification.\n\nI started making this incremental but the progress is not so great.\n\n* tr/rev-list-docs (Tue Aug 12 01:55:37 2008 +0200) 1 commit\n + Documentation: rev-list-options: move --simplify-merges\n   documentation\n\n----------------------------------------------------------------\n[Will merge to master soon]\n\n* jc/no-slim-shell (Tue Aug 19 18:05:43 2008 -0700) 2 commits\n + Build-in \"git-shell\"\n + shell: do not play duplicated definition games to shrink the\n   executable\n\n* mv/merge-custom (Sat Aug 23 19:23:22 2008 -0700) 9 commits\n + t7606: fix custom merge test\n + Fix \"git-merge -s bogo\" help text\n + Update .gitignore to ignore git-help\n + Builtin git-help.\n + builtin-help: always load_command_list() in cmd_help()\n + Add a second testcase for handling invalid strategies in git-merge\n + Add a new test for using a custom merge strategy\n + builtin-merge: allow using a custom strategy\n + builtin-help: make some internal functions available to other\n   builtins\n\nThe one at the tip fixes a test that assumed git-merge has a broken\n\"trivial merge\" implementation.\n\n* jc/add-addremove (Tue Jul 22 22:30:40 2008 -0700) 2 commits\n + builtin-add.c: optimize -A option and \"git add .\"\n + builtin-add.c: restructure the code for maintainability\n\n* am/cherry-pick-rerere (Sun Aug 10 17:18:55 2008 +0530) 1 commit\n + Make cherry-pick use rerere for conflict resolution.\n\n----------------------------------------------------------------\n[On Hold]\n\n* jc/stripspace (Sun Mar 9 00:30:35 2008 -0800) 6 commits\n - git-am --forge: add Signed-off-by: line for the author\n - git-am: clean-up Signed-off-by: lines\n - stripspace: add --log-clean option to clean up signed-off-by:\n   lines\n - stripspace: use parse_options()\n - Add \"git am -s\" test\n - git-am: refactor code to add signed-off-by line for the committer\n\n* jc/send-pack-tell-me-more (Thu Mar 20 00:44:11 2008 -0700) 1 commit\n - \"git push\": tellme-more protocol extension\n\n* jc/merge-whitespace (Sun Feb 24 23:29:36 2008 -0800) 1 commit\n - WIP: start teaching the --whitespace=fix to merge machinery\n\n* jc/blame (Wed Jun 4 22:58:40 2008 -0700) 2 commits\n - blame: show \"previous\" information in --porcelain/--incremental\n   format\n - git-blame: refactor code to emit \"porcelain format\" output\n\n* sg/merge-options (Sun Apr 6 03:23:47 2008 +0200) 1 commit\n + merge: remove deprecated summary and diffstat options and config\n   variables\n\nThis was previously in \"will be in master soon\" category, but it turns out\nthat the synonyms to the ones this one deletes are fairly new invention\nthat happend in 1.5.6 timeframe, and we cannot do this just yet.  Perhaps\nin 1.7.0.\n\n* jc/dashless (Wed Jun 25 15:55:11 2008 -0700) 1 commit\n - Make clients ask for \"git program\" over ssh and local transport\n\nThis is the \"botched\" one.  Will be resurrected during 1.7.0 or 1.8.0\ntimeframe.\n\n* jk/renamelimit (Sat May 3 13:58:42 2008 -0700) 1 commit\n - diff: enable \"too large a rename\" warning when -M/-C is explicitly\n   asked for\n\nThis would be the right thing to do for command line use, but gitk will be\nhit due to tcl/tk's limitation, so I am holding this back for now.\n\n----------------------------------------------------------------\n[Graduated to \"master\"]\n\n* ml/submodule-foreach (Sun Aug 10 19:10:04 2008 -0400) 1 commit\n + git-submodule - Add 'foreach' subcommand\n\n* pm/log-exit-code (Mon Aug 11 08:46:25 2008 +0200) 2 commits\n + Teach git log --exit-code to return an appropriate exit code\n + Teach git log --check to return an appropriate exit code\n\n* sb/commit-tree-minileak (Tue Aug 12 00:35:11 2008 +0200) 1 commit\n + Fix commit_tree() buffer leak\n\n* pb/reflog-dwim (Sun Aug 10 22:22:21 2008 +0200) 1 commit\n + builtin-reflog: Allow reflog expire to name partial ref\n\n* jc/add-stop-at-symlink (Mon Aug 4 00:52:37 2008 -0700) 2 commits\n + add: refuse to add working tree items beyond symlinks\n + update-index: refuse to add working tree items beyond symlinks\n\nFix for a longstanding bug that allows \"git add\" and \"git update-index\" to\nadd a path \"a/b\" to the index when \"a\" is a symbolic link.  We would need\na similar fix for the case where \"a\" is a submodule.\n\n* kh/diff-tree (Sun Aug 10 18:13:04 2008 +0200) 4 commits\n + Add test for diff-tree --stdin with two trees\n + Teach git diff-tree --stdin to diff trees\n + diff-tree: Note that the commit ID is printed with --stdin\n + Refactoring: Split up diff_tree_stdin\n\n* mg/count-objects (Fri Aug 15 00:20:20 2008 -0400) 1 commit\n + count-objects: Add total pack size to verbose output\n\nThis one is without the human readable bits.\n\n* mz/push-verbose (Sat Aug 16 19:58:32 2008 +0200) 1 commit\n + Make push more verbose about illegal combination of options\n\n* jc/index-extended-flags (Sat Aug 16 23:02:08 2008 -0700) 1 commit\n + index: future proof for \"extended\" index entries\n\n* cc/merge-base-many (Sun Jul 27 13:47:22 2008 -0700) 4 commits\n + git-merge-octopus: use (merge-base A (merge B C D E...)) for\n   stepwise merge\n + merge-base-many: add trivial tests based on the documentation\n + documentation: merge-base: explain \"git merge-base\" with more than\n   2 args\n + merge-base: teach \"git merge-base\" to drive underlying\n   merge_bases_many()\n\n* js/parallel-test (Mon Aug 18 12:25:40 2008 -0400) 4 commits\n + Update t/.gitignore to ignore all trash directories\n + Enable parallel tests\n + tests: Clarify dependencies between tests, 'aggregate-results' and\n   'clean'\n + t9700: remove useless check\n\n* jc/test-deeper (Fri Aug 8 02:26:28 2008 -0700) 1 commit\n + tests: use $TEST_DIRECTORY to refer to the t/ directory\n\nThis does not actually move \"t/test directory\" any deeper, but fixes test\nscripts that assume they run immediately below \"t/\" to use TEST_DIRECTORY\nvariable.\n\n* js/mingw-stat (Mon Aug 18 22:01:06 2008 +0200) 2 commits\n + Revert \"Windows: Use a customized struct stat that also has the\n   st_blocks member.\"\n + compat: introduce on_disk_bytes()\n\nThis gets rid of use of st_blocks member (which is XSI but not POSIX\nproper), which was originally prompted by recent Haiku port but it turns\nout MinGW has the same issue as well.  Queued on 'pu' just to have a\nchance to make sure I munged the version j6t sent me correctly before\nmerging it upwards.\n\n* js/checkout-dwim-local (Sat Aug 9 16:00:12 2008 +0200) 1 commit\n + checkout --track: make up a sensible branch name if '-b' was\n   omitted\n\nAlex has update to dwim \"checkout --track remotes/origin/hack\" as well.\n\n* bd/diff-strbuf (Wed Aug 13 23:18:22 2008 -0700) 3 commits\n + xdiff-interface: hide the whole \"xdiff_emit_state\" business from\n   the caller\n + Use strbuf for struct xdiff_emit_state's remainder\n + Make xdi_diff_outf interface for running xdiff_outf diffs\n\nGives measurable performance improvement to textual diff generation.  For\nimproving \"blame\" performance, it might be more effective to hook directly\nto lower level of xdiff machinery so that we do not even have to generate\npatch only to discard after reading \"@@ -l,k +m,n @@\" lines, but that\nwould be a separate topic.\n\n* dp/hash-literally (Sun Aug 3 18:36:22 2008 +0400) 6 commits\n + add --no-filters option to git hash-object\n + add --path option to git hash-object\n + use parse_options() in git hash-object\n + correct usage help string for git-hash-object\n + correct argument checking test for git hash-object\n + teach index_fd to work with pipes\n\nGives a bit more flexibility to hash-objects by allowing us to lie about\nthe path the contents comes from.\n\n* rs/imap (Wed Jul 9 22:29:02 2008 +0100) 5 commits\n + Documentation: Improve documentation for git-imap-send(1)\n + imap-send.c: more style fixes\n + imap-send.c: style fixes\n + git-imap-send: Support SSL\n + git-imap-send: Allow the program to be run from subdirectories of\n   a git tree\n\nSome people seem to prefer having this feature available also with gnutls.\nSuch an enhancement can be done in-tree on top of this series if they are\nso inclined.\n\n* jk/pager-swap (Tue Jul 22 03:14:12 2008 -0400) 2 commits\n + spawn pager via run_command interface\n + run-command: add pre-exec callback\n\nThis changes the parent-child relationship between the pager and the git\nprocess.  We used to make pager the parent which meant that the exit\nstatus from git is lost from the caller.\n\n* ph/enable-threaded (Mon Jul 21 11:23:43 2008 +0200) 1 commit\n + Enable threaded delta search on *BSD and Linux.\n"},{"id":"88346","messageId":"7vej4frrio.fsf@gitster.siamese.dyndns.org","threadId":"15186","inReplyTo":"7v1w0ft750.fsf@gitster.siamese.dyndns.org","subject":"Maintaining \"needswork\" section of \"What's (not) cooking\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-24T04:00:47Z","receivedAt":"2008-08-24T04:00:47Z","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> Here is a list of issues/topics we saw on the mailing list but haven't\n> resulted in anything queuable in 'pu' yet.  They need further work by\n> interested parties:\n> ...\n> [Stalled -- Needs Action to Proceed (or to be dropped)]\n> ...\n\nI am still experimenting with my own workflow with these parts.\n\nWith more people on the list sending patches recently, I still have enough\nmental bandwidth to reject the initial rounds with comments/suggestions\nfor improvements, but I do not think it is realistic to expect me to keep\ntrack of all of their progress on re-submission of improvements anymore (I\nused to prod people privately asking how much they are making progress\nafter not hearing from people who received review comments).\n\nIt is a large loss for all of us when people do not come back with updated\npatches after receiving review comments.  Lost are their good idea based\non the real world needs.  Time other people have volunteered to review\ntheir initial submissions are also wasted when that happens.\n\nI however do not intend to lower the patch acceptance standards.  It is\nnot a workable approach to accept many new features implemented in a\nsubstandard way or designed incoherently, expecting they will eventually\nbe cleaned up in-tree.  Nobody will clean anything up after it is merged,\nand when we give something to the end users, it becomes harder to change\nits behaviour.\n\nIf a few people can volunteer to maintain a list that looks like the above\n(not-quite) quoted ones, we may be able to give people more incentive to\nkeep polishing their patches in response to the reviews they received.\nOther people can also offer help in topics they are interested in, and we\nmay see more topics through their completion as the result.\n\nThoughts?\n.\n"},{"id":"88373","messageId":"alpine.DEB.1.00.0808241955550.24820@pacific.mpi-cbg.de.mpi-cbg.de","threadId":"15186","inReplyTo":"7v1w0ft750.fsf@gitster.siamese.dyndns.org","subject":"Re: What's cooking in git.git (Aug 2008, #07; Sat, 23)","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-08-24T18:12:43Z","receivedAt":"2008-08-24T18:12:43Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 23 Aug 2008, Junio C Hamano wrote:\n\n> ----------------------------------------------------------------\n> [Stalled -- Needs Action to Proceed (or to be dropped)]\n> \n> * sb/daemon (Thu Aug 14 20:02:20 2008 +0200) 4 commits\n>  - git-daemon: rewrite kindergarden, new option --max-connections\n>  - git-daemon: Simplify dead-children reaping logic\n>  - git-daemon: use LOG_PID, simplify logging code\n>  - git-daemon: call logerror() instead of error()\n> \n> Can somebody who actually runs the daemon standalone comment on this \n> one?\n\nI am somewhat uneasy about running my production machine with these \nchanges, since the last commit (the one introducing a kindergarden with \ncradles) is too unobvious for me.\n\nCiao,\nDscho\n"},{"id":"88383","messageId":"7vd4jymdfn.fsf@gitster.siamese.dyndns.org","threadId":"15186","inReplyTo":"alpine.DEB.1.00.0808241955550.24820@pacific.mpi-cbg.de.mpi-cbg.de","subject":"Re: What's cooking in git.git (Aug 2008, #07; Sat, 23)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-24T19:16:12Z","receivedAt":"2008-08-24T19:16:12Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n>> * sb/daemon (Thu Aug 14 20:02:20 2008 +0200) 4 commits\n>>  - git-daemon: rewrite kindergarden, new option --max-connections\n>>  - git-daemon: Simplify dead-children reaping logic\n>>  - git-daemon: use LOG_PID, simplify logging code\n>>  - git-daemon: call logerror() instead of error()\n>> \n>> Can somebody who actually runs the daemon standalone comment on this \n>> one?\n>\n> I am somewhat uneasy about running my production machine with these \n> changes, since the last commit (the one introducing a kindergarden with \n> cradles) is too unobvious for me.\n\nWell, I didn't ask anybody to _run_ it.\n\nI asked people who care about their daemons to comment on the change, so\nthat if there are any issues in the code that I didn't see, the breakage\ngets caught before it propagates to their daemons they build from 'next'\nor 'master'.\n\nHaving said that, I do agree that the kindergarden change is very involved\n(it removes more code than it adds --- the reduction of lines is somewhat\ninflated because quite a lot of comments that have become stale gets\nremoved by the patch).\n"},{"id":"88390","messageId":"7vwsi6kvow.fsf@gitster.siamese.dyndns.org","threadId":"15186","inReplyTo":"7vd4jymdfn.fsf@gitster.siamese.dyndns.org","subject":"Re: What's cooking in git.git (Aug 2008, #07; Sat, 23)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-24T20:24:47Z","receivedAt":"2008-08-24T20:24:47Z","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> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n>\n>>> * sb/daemon (Thu Aug 14 20:02:20 2008 +0200) 4 commits\n>>>  - git-daemon: rewrite kindergarden, new option --max-connections\n>>>  - git-daemon: Simplify dead-children reaping logic\n>>>  - git-daemon: use LOG_PID, simplify logging code\n>>>  - git-daemon: call logerror() instead of error()\n>>> \n>>> Can somebody who actually runs the daemon standalone comment on this \n>>> one?\n>>\n>> I am somewhat uneasy about running my production machine with these \n>> changes, since the last commit (the one introducing a kindergarden with \n>> cradles) is too unobvious for me.\n>\n> Well, I didn't ask anybody to _run_ it.\n>\n> I asked people who care about their daemons to comment on the change, so\n> that if there are any issues in the code that I didn't see, the breakage\n> gets caught before it propagates to their daemons they build from 'next'\n> or 'master'.\n>\n> Having said that, I do agree that the kindergarden change is very involved\n> (it removes more code than it adds --- the reduction of lines is somewhat\n> inflated because quite a lot of comments that have become stale gets\n> removed by the patch).\n\nOk, so I looked at the patch again (sorry, Stephen, for keeping this in a\nlimbo for very long).\n\nFirst of all, I have to say that I think this patch is reasonably well\ndone.  I do see an unnecessary check of xcalloc() return value and some\nstyle differences from what git uses, but that is only an indication of\nthe author not very familiar with git code, and every other aspect of the\nchange shows that this is quite competently done.  The list traversal with\ncradle/blanket/newborn, except using perhaps too cute names for some\npeople's tastes, makes sense.\n\nI however do have one issue with the logic, though.\n\nIt seems that kill_some_child() will not kill anything if nobody else is\ncoming from the same address, while the old code did kill some.  Is this\nintended?\n\nBy the way, add_child() compares the whole \"struct sockaddr_storage\" in\norder to queue the newborn in front of an existing connection from the\nsame address, and kill_some_child() takes advanrage of this to kill the\nnewest connection (\"We kill the newest\" comment should probably be moved\nto add_child() to describe why the queuing is done this way).  If you\nsimplify add_child() to queue the newborn always at the front of the list,\nyour kill_some_child() will continue to do so, so I do not see the point\nof the loop in add_child().  Am I mistaken?\n"},{"id":"88391","messageId":"7vsksukvkx.fsf_-_@gitster.siamese.dyndns.org","threadId":"15186","inReplyTo":"7vwsi6kvow.fsf@gitster.siamese.dyndns.org","subject":"[PATCH 1/3] daemon.c: minor style fixup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-24T20:27:10Z","receivedAt":"2008-08-24T20:27:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":" * \"else\" on the same line as \"}\" that closes corresponding \"if (...) {\";\n\n * multi-line comments begin with \"/*\\n\";\n\n * sizeof, even it is not a function, is written as \"sizeof(...)\";\n\n * no need to check x?alloc() return value -- it would have died;\n\n * \"if (...) { ... }\" that covers the whole function body can be dedented\n   by returning from the function early with \"if (!...) return;\";\n\n * SP on each side of an operator, i.e. \"a > 0\", not \"a>0\";\n\nAlso removes stale comment describing how remove_child() used to do its\nthing.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>:\n---\n\nJunio C Hamano <gitster@pobox.com> writes:\n\n> I however do have one issue with the logic, though.\n>\n> It seems that kill_some_child() will not kill anything if nobody else is\n> coming from the same address, while the old code did kill some.  Is this\n> intended?\n>\n> By the way, add_child() compares the whole \"struct sockaddr_storage\" in\n> order to queue the newborn in front of an existing connection from the\n> same address, and kill_some_child() takes advanrage of this to kill the\n> newest connection (\"We kill the newest\" comment should probably be moved\n> to add_child() to describe why the queuing is done this way).  If you\n> simplify add_child() to queue the newborn always at the front of the list,\n> your kill_some_child() will continue to do so, so I do not see the point\n> of the loop in add_child().  Am I mistaken?\n\nSo here is a three-patch series on top.  I am not sure about the last one.\n\n daemon.c |   72 +++++++++++++++++++++++++------------------------------------\n 1 files changed, 30 insertions(+), 42 deletions(-)\n\ndiff --git a/daemon.c b/daemon.c\nindex 79f0388..35bd34a 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -81,9 +81,9 @@ static void logreport(int priority, const char *err, va_list params)\n \t\tchar buf[1024];\n \t\tvsnprintf(buf, sizeof(buf), err, params);\n \t\tsyslog(priority, \"%s\", buf);\n-\t}\n-\telse {\n-\t\t/* Since stderr is set to linebuffered mode, the\n+\t} else {\n+\t\t/*\n+\t\t * Since stderr is set to linebuffered mode, the\n \t\t * logging of different processes will not overlap\n \t\t */\n \t\tfprintf(stderr, \"[%d] \", (int)getpid());\n@@ -596,31 +596,20 @@ static struct child {\n \n static void add_child(pid_t pid, struct sockaddr *addr, int addrlen)\n {\n-\tstruct child *newborn;\n-\tnewborn = xcalloc(1, sizeof *newborn);\n-\tif (newborn) {\n-\t\tstruct child **cradle;\n-\n-\t\tlive_children++;\n-\t\tnewborn->pid = pid;\n-\t\tmemcpy(&newborn->address, addr, addrlen);\n-\t\tfor (cradle = &firstborn; *cradle; cradle = &(*cradle)->next)\n-\t\t\tif (!memcmp(&(*cradle)->address, &newborn->address,\n-\t\t\t\t   sizeof newborn->address))\n-\t\t\t\tbreak;\n-\t\tnewborn->next = *cradle;\n-\t\t*cradle = newborn;\n-\t}\n-\telse\n-\t\tlogerror(\"Out of memory spawning new child\");\n+\tstruct child *newborn, **cradle;\n+\tnewborn = xcalloc(1, sizeof(*newborn));\n+\n+\tlive_children++;\n+\tnewborn->pid = pid;\n+\tmemcpy(&newborn->address, addr, addrlen);\n+\tfor (cradle = &firstborn; *cradle; cradle = &(*cradle)->next)\n+\t\tif (!memcmp(&(*cradle)->address, &newborn->address,\n+\t\t\t    sizeof(newborn->address)))\n+\t\t\tbreak;\n+\tnewborn->next = *cradle;\n+\t*cradle = newborn;\n }\n \n-/*\n- * Walk from \"deleted\" to \"spawned\", and remove child \"pid\".\n- *\n- * We move everything up by one, since the new \"deleted\" will\n- * be one higher.\n- */\n static void remove_child(pid_t pid)\n {\n \tstruct child **cradle, *blanket;\n@@ -642,18 +631,17 @@ static void remove_child(pid_t pid)\n  */\n static void kill_some_child(void)\n {\n-\tconst struct child *blanket;\n+\tconst struct child *blanket, *next;\n \n-\tif ((blanket = firstborn)) {\n-\t\tconst struct child *next;\n+\tif (!(blanket = firstborn))\n+\t\treturn;\n \n-\t\tfor (; (next = blanket->next); blanket = next)\n-\t\t\tif (!memcmp(&blanket->address, &next->address,\n-\t\t\t\t   sizeof next->address)) {\n-\t\t\t\tkill(blanket->pid, SIGTERM);\n-\t\t\t\tbreak;\n-\t\t\t}\n-\t}\n+\tfor (; (next = blanket->next); blanket = next)\n+\t\tif (!memcmp(&blanket->address, &next->address,\n+\t\t\t    sizeof(next->address))) {\n+\t\t\tkill(blanket->pid, SIGTERM);\n+\t\t\tbreak;\n+\t\t}\n }\n \n static void check_dead_children(void)\n@@ -661,10 +649,10 @@ static void check_dead_children(void)\n \tint status;\n \tpid_t pid;\n \n-\twhile ((pid = waitpid(-1, &status, WNOHANG))>0) {\n+\twhile ((pid = waitpid(-1, &status, WNOHANG)) > 0) {\n \t\tconst char *dead = \"\";\n \t\tremove_child(pid);\n-\t\tif (!WIFEXITED(status) || WEXITSTATUS(status) > 0)\n+\t\tif (!WIFEXITED(status) || (WEXITSTATUS(status) > 0))\n \t\t\tdead = \" (with error)\";\n \t\tloginfo(\"[%d] Disconnected%s\", (int)pid, dead);\n \t}\n@@ -676,7 +664,7 @@ static void handle(int incoming, struct sockaddr *addr, int addrlen)\n \n \tif (max_connections && live_children >= max_connections) {\n \t\tkill_some_child();\n-\t\tsleep(1);\t\t\t /* give it some time to die */\n+\t\tsleep(1);  /* give it some time to die */\n \t\tcheck_dead_children();\n \t\tif (live_children >= max_connections) {\n \t\t\tclose(incoming);\n@@ -705,7 +693,8 @@ static void handle(int incoming, struct sockaddr *addr, int addrlen)\n \n static void child_handler(int signo)\n {\n-\t/* Otherwise empty handler because systemcalls will get interrupted\n+\t/*\n+\t * Otherwise empty handler because systemcalls will get interrupted\n \t * upon signal receipt\n \t * SysV needs the handler to be rearmed\n \t */\n@@ -1089,8 +1078,7 @@ int main(int argc, char **argv)\n \tif (log_syslog) {\n \t\topenlog(\"git-daemon\", LOG_PID, LOG_DAEMON);\n \t\tset_die_routine(daemon_die);\n-\t}\n-\telse\n+\t} else\n \t\tsetlinebuf(stderr); /* avoid splitting a message in the middle */\n \n \tif (inetd_mode && (group_name || user_name))\n-- \n1.6.0.129.ge10d2\n"},{"id":"88392","messageId":"7vod3ikvk5.fsf_-_@gitster.siamese.dyndns.org","threadId":"15186","inReplyTo":"7vwsi6kvow.fsf@gitster.siamese.dyndns.org","subject":"[PATCH 2/3] daemon.c: simplify add_child() and kill_some_child() logic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-24T20:27:38Z","receivedAt":"2008-08-24T20:27:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"kill_some_child() function walks the list of connections and kills the\nfirst connection it finds from the same address.  There is not much point\nto find the existing connection from the same address in add_child() to\ninsert the new one in front of it.  At the front of the whole queue is\njust as good.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n daemon.c |   19 ++++++++++---------\n 1 files changed, 10 insertions(+), 9 deletions(-)\n\ndiff --git a/daemon.c b/daemon.c\nindex 35bd34a..8d2755a 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -596,18 +596,18 @@ static struct child {\n \n static void add_child(pid_t pid, struct sockaddr *addr, int addrlen)\n {\n-\tstruct child *newborn, **cradle;\n-\tnewborn = xcalloc(1, sizeof(*newborn));\n+\tstruct child *newborn;\n \n+\t/*\n+\t * This must be xcalloc() -- we'll compare the whole sockaddr_storage\n+\t * later in kill_some_child().\n+\t */\n+\tnewborn = xcalloc(1, sizeof(*newborn));\n \tlive_children++;\n \tnewborn->pid = pid;\n \tmemcpy(&newborn->address, addr, addrlen);\n-\tfor (cradle = &firstborn; *cradle; cradle = &(*cradle)->next)\n-\t\tif (!memcmp(&(*cradle)->address, &newborn->address,\n-\t\t\t    sizeof(newborn->address)))\n-\t\t\tbreak;\n-\tnewborn->next = *cradle;\n-\t*cradle = newborn;\n+\tnewborn->next = firstborn;\n+\tfirstborn = newborn;\n }\n \n static void remove_child(pid_t pid)\n@@ -627,7 +627,8 @@ static void remove_child(pid_t pid)\n  * This gets called if the number of connections grows\n  * past \"max_connections\".\n  *\n- * We kill the newest connection from a duplicate IP.\n+ * We kill the newest connection from the same address (add_child() queues\n+ * new ones at the front).\n  */\n static void kill_some_child(void)\n {\n-- \n1.6.0.129.ge10d2\n"},{"id":"88393","messageId":"7vk5e6kvaw.fsf_-_@gitster.siamese.dyndns.org","threadId":"15186","inReplyTo":"7vwsi6kvow.fsf@gitster.siamese.dyndns.org","subject":"[PATCH 3/3] daemon.c: make sure kill_some_child() really kills somebody","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-24T20:33:11Z","receivedAt":"2008-08-24T20:33:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"We used to kill nobody if there is no existing connection from the same\naddress the new connection we are trying to handle, and dropped the new\nconnection.  Make sure we at least kill one.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n * I am not sure about this one, but it may be more in spirit with the old\n   behaviour that made sure at max connection limit we killed some to\n   handle new ones.\n\n   Actually I do think this is probably a bad idea.  What we really want\n   to do is to detect an old one that is not making any progress instead.\n   \"old\" we can detect by looking at its position in the queue (or we\n   could even add an explicit timestamp to the child structure), but it is\n   harder to measure \"not making any progress\", especially without going\n   too platform specific, e.g. monitoring rusage or somesuch, which we\n   would want to avoid.\n\n daemon.c |    5 ++++-\n 1 files changed, 4 insertions(+), 1 deletions(-)\n\ndiff --git a/daemon.c b/daemon.c\nindex 8d2755a..a0d8f65 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -641,8 +641,11 @@ static void kill_some_child(void)\n \t\tif (!memcmp(&blanket->address, &next->address,\n \t\t\t    sizeof(next->address))) {\n \t\t\tkill(blanket->pid, SIGTERM);\n-\t\t\tbreak;\n+\t\t\treturn;\n \t\t}\n+\n+\t/* Nobody from the same address?  Kill the youngest one, then. */\n+\tkill(firstborn->pid, SIGTERM);\n }\n \n static void check_dead_children(void)\n-- \n1.6.0.129.ge10d2\n"},{"id":"88471","messageId":"20080825163203.GD22184@cuci.nl","threadId":"15186","inReplyTo":"7vwsi6kvow.fsf@gitster.siamese.dyndns.org","subject":"Re: What's cooking in git.git (Aug 2008, #07; Sat, 23)","fromName":"Stephen R. van den Berg","fromEmail":"srb@cuci.nl","sentAt":"2008-08-25T16:32:03Z","receivedAt":"2008-08-25T16:32:03Z","isPatch":false,"sender":{"key":"srb@cuci.nl","avatar":"https://gravatar.com/avatar/f75389059e827634d38e9df2a9b6ecbd50028b5a454442efa1c7205b7ff29c6a?d=mp&s=160"},"body":"Junio C Hamano wrote:\n>Junio C Hamano <gitster@pobox.com> writes:\n>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n>>>> * sb/daemon (Thu Aug 14 20:02:20 2008 +0200) 4 commits\n>>>>  - git-daemon: rewrite kindergarden, new option --max-connections\n>>>>  - git-daemon: Simplify dead-children reaping logic\n>>>>  - git-daemon: use LOG_PID, simplify logging code\n>>>>  - git-daemon: call logerror() instead of error()\n\n>>>> Can somebody who actually runs the daemon standalone comment on this \n>>>> one?\n\n>> Well, I didn't ask anybody to _run_ it.\n\nActually I *am* running it in production (obviously).\n\n>It seems that kill_some_child() will not kill anything if nobody else is\n>coming from the same address, while the old code did kill some.  Is this\n>intended?\n\nThis is intended.\nThe reasoning goes a bit like this:\n- We already allow a --timeout to be specified, that is taken care of\n  by the child process.\n- The child process is well-behaved, so it should honour its own timeout\n  setting.\n- If the daemon would need to cater for child processes failing to\n  honour their own timeout setting, we have some serious bugs in the\n  child-code, and *that* should be fixed ASAP instead of letting that\n  kind of problems linger by not getting noticed because some daemon\n  tries to clean up after badly-behaving children.\n- Therefore, the only two problems that are left are:\n  + Many connections in total.  That would mean that we can't handle the\n    load.  Well, the best way to respond to that is to diligently\n    process the ones that *did* manage to connect, and *not* to randomly\n    disconnect sessions which are already halfway there.\n  + Many connections in total, but also multiple connections from the\n    same IP-address.  In this case, we could try and evenly distribute\n    the resource and kill off the *newest* connections from the same IP\n    addresses, to allow everyone an equal share to the daemon.\n\nThat means:\n- Random killing is not necessary.\n- Since we know exactly how many clients are running, we can keep tabs\n  on when to fork and when not (further reducing the load problem on\n  a busy site).  Which means that at most we need to\n  kill one client for every new connection.\n- Since we don't want to force old connections to die (we trust our\n  children), we simply refuse further connections as soon as we've\n  reached our limit and don't have resource-eaters (duplicate sessions\n  from the same IP address).\n\n>By the way, add_child() compares the whole \"struct sockaddr_storage\" in\n>order to queue the newborn in front of an existing connection from the\n>same address, and kill_some_child() takes advanrage of this to kill the\n>newest connection (\"We kill the newest\" comment should probably be moved\n>to add_child() to describe why the queuing is done this way).  If you\n>simplify add_child() to queue the newborn always at the front of the list,\n>your kill_some_child() will continue to do so, so I do not see the point\n>of the loop in add_child().  Am I mistaken?\n\nYou are mistaken.\nThe point is that we need to find out *duplicate* IP-adresses.\nI.e. when looking for the child to be killed, the IP-address we're looking\nfor is not the IP-address of the new connection, but we're looking for\ntwo consecutive duplicate addresses.  In order for this to work, we need\nto cluster the connections of the same IP-address, and the newest\nconnection needs to be inserted in front, in order for kill_child to\nkill the most recent connection.\n\nShall I incorporate your suggested changes (as far as I consider them ok)\ninto my patches and resubmit them?\n-- \nSincerely,\n           Stephen R. van den Berg.\nAuto repair rates: basic labor $40/hour; if you wait, $60; if you watch, $80;\nif you ask questions, $100; if you help, $120; if you laugh, $140.\n"},{"id":"88498","messageId":"7vtzd8g851.fsf@gitster.siamese.dyndns.org","threadId":"15186","inReplyTo":"20080825163203.GD22184@cuci.nl","subject":"Re: What's cooking in git.git (Aug 2008, #07; Sat, 23)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-25T20:19:22Z","receivedAt":"2008-08-25T20:19:22Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Stephen R. van den Berg\" <srb@cuci.nl> writes:\n\n> Junio C Hamano wrote:\n> ...\n>>It seems that kill_some_child() will not kill anything if nobody else is\n>>coming from the same address, while the old code did kill some.  Is this\n>>intended?\n>\n> This is intended.\n> ...\n>>...  If you\n>>simplify add_child() to queue the newborn always at the front of the list,\n>>your kill_some_child() will continue to do so, so I do not see the point\n>>of the loop in add_child().  Am I mistaken?\n> ...\n> You are mistaken.\n> The point is that we need to find out *duplicate* IP-adresses.\n\nLightbulb goes on on both counts.  Thanks.\n\n> Shall I incorporate your suggested changes (as far as I consider them ok)\n> into my patches and resubmit them?\n\nI think your response makes the only remaining is the style fixup, which\nwas my [1/3] from yesterday, so I think we are Ok with what already we\nhave.\n\nThanks.\n"},{"id":"88514","messageId":"20080825212723.GA25792@cuci.nl","threadId":"15186","inReplyTo":"7vtzd8g851.fsf@gitster.siamese.dyndns.org","subject":"Re: What's cooking in git.git (Aug 2008, #07; Sat, 23)","fromName":"Stephen R. van den Berg","fromEmail":"srb@cuci.nl","sentAt":"2008-08-25T21:27:23Z","receivedAt":"2008-08-25T21:27:23Z","isPatch":false,"sender":{"key":"srb@cuci.nl","avatar":"https://gravatar.com/avatar/f75389059e827634d38e9df2a9b6ecbd50028b5a454442efa1c7205b7ff29c6a?d=mp&s=160"},"body":"Junio C Hamano wrote:\n>\"Stephen R. van den Berg\" <srb@cuci.nl> writes:\n>I think your response makes the only remaining is the style fixup, which\n>was my [1/3] from yesterday, so I think we are Ok with what already we\n>have.\n\nChecked that one, looked fine.  Thanks.\n-- \nSincerely,\n           Stephen R. van den Berg.\nAuto repair rates: basic labor $40/hour; if you wait, $60; if you watch, $80;\nif you ask questions, $100; if you help, $120; if you laugh, $140.\n"}]}