{"thread":{"id":"6140","subject":"[PATCH 0/11] Misc. pull/merge/am improvements","startedAt":"2006-12-28T07:34:41Z","lastAt":"2006-12-31T01:21:15Z","messageCount":10,"participants":["Shawn Pearce","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":11},"messages":[{"id":"30406","messageId":"20061228073441.GB17304@spearce.org","threadId":"6140","inReplyTo":null,"subject":"[PATCH 0/11] Misc. pull/merge/am improvements","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2006-12-28T07:34:41Z","receivedAt":"2006-12-28T07:34:41Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"This series of patches is a group of minor interface and performance\nimprovements for am/pull/rebase.\n\n 1 Use GIT_REFLOG_ACTION environment variable instead.\n 2 Honor GIT_REFLOG_ACTION in git-rebase.\n\n These two replace my prior 2 patch series in the same area.\n\n 3 Use branch names in 'git-rebase -m' conflict hunks.\n\n This is a UI improvement to show better conflicts out of rebase.\n\n 4 Ensure `git-pull` fails if `git-merge` fails.\n\n Trivial fix to make git-pull more friendly to other scripts.\n\n 5 Honor pull.{twohead,octopus} in git-merge.\n 6 Allow git-merge to select the default strategy.\n\n This moves the default merge strategy selection into git-merge,\n making it part of that tool rather than git-pull. This makes\n it possible to get the same behavior from `git merge foo` as\n you already get from `git pull . foo`.\n\n 7 Avoid git-fetch in `git-pull .` when possible.\n\n This is a performance improvement for pull, and offers some\n other nice benefits (see patch).\n\n 8 Move better_branch_name above get_ref in merge-recursive.\n 9 Allow merging bare trees in merge-recursive.\n 10 Use merge-recursive in git-am -3.\n\n These three switch to merge-recursive in git-am, see patch 10's\n message for the benefits.\n\n 11 Improve merge performance by avoiding in-index merges.\n\n This is a general performance improvement for all two-headed\n merges which might use merge-recursive.\n\n\nI'd like to see these appear in v1.5.0, but we're getting close to\nthe release so I can understand if they get delayed.\n\n-- \nShawn.\n"},{"id":"30422","messageId":"7vmz58whnx.fsf@assigned-by-dhcp.cox.net","threadId":"6140","inReplyTo":"20061228073441.GB17304@spearce.org","subject":"Re: [PATCH 0/11] Misc. pull/merge/am improvements","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-12-28T08:25:54Z","receivedAt":"2006-12-28T08:25:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shawn Pearce writes:\n\n> I'd like to see these appear in v1.5.0, but we're getting close to\n> the release so I can understand if they get delayed.\n\nI think shooting for -rc1 by mid January and stabilizing by\nearly to mid February for the real release, would be a realistic\ntimeline.\n\nActually I am getting more greedy, and would not mind to have\nclean ups and a few more features in the new release, even the\nones that we have talked about but have not implemented.\n\nI am very tempted to have sliding window mmap() if it helps\npeople on cygwin, for example. Also, I've been running with\n\"next\" for my daily pushes and pulls without trouble, and I am\nvery tempted to push out the shallow-clone topic. Not that I\nthink its shallow-clone part is useful -- I do not use the\nfeature myself so I cannot judge -- but at least when shallow is\nenabled on neither ends, it does not seem to break anything.\n\nAlthough I do find the detached HEAD attractive and would want\nto have it eventually, I suspect that even if it materializes\nsoon enough, it would at least need a couple of weeks of testing\nin 'next', so making -rc1 wait for it might push back the\nrelease a bit too much.\n"},{"id":"30423","messageId":"20061228084245.GA18150@spearce.org","threadId":"6140","inReplyTo":"7vmz58whnx.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 0/11] Misc. pull/merge/am improvements","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2006-12-28T08:42:45Z","receivedAt":"2006-12-28T08:42:45Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano wrote:\n> Actually I am getting more greedy, and would not mind to have\n> clean ups and a few more features in the new release, even the\n> ones that we have talked about but have not implemented.\n\nI know Linus wants me to fix the bash completion so it adds a trailing\nspace when something gets completed and is unique and no additional\ntext is expected to follow for that argument. I just haven't put\nthe effort into it, even though I think I have a solution.\n\nBasically I'm saying I probably could have another round of bash\ncompletion in another week which may be worth considering for\ninclusion. ;-)\n \n> I am very tempted to have sliding window mmap() if it helps\n> people on cygwin, for example.\n\nEspecially now that NO_MMAP is the default on that platform.\nAt this point it may be ready to graduate to next to try and get a\nwider audience. Since fixing that segfault in pack-objects I can't\nbreak it. Of course I couldn't break it before you found that error,\nso take my words with a grain of salt... ;-)\n\n> Also, I've been running with\n> \"next\" for my daily pushes and pulls without trouble, and I am\n> very tempted to push out the shallow-clone topic.\n\nHasn't that been in next for a while now? I pretty much always\nrun next, and have also been using it on that non-publishable\nrepository for almost two months now. I've got 20 other users who I\ncollaborate with on cygwin running next... we haven't had any issue\nwith the portions of shallow-clone which had already moved in, and\nI upgrade that environment almost daily to keep current. Of course\nwe also haven't tried to actually use the shallow-clone feature as\nwe haven't needed it (the repository is only 50 MiB packed).\n\n> Although I do find the detached HEAD attractive and would want\n> to have it eventually, I suspect that even if it materializes\n> soon enough, it would at least need a couple of weeks of testing\n> in 'next', so making -rc1 wait for it might push back the\n> release a bit too much.\n\nAgreed. It would be nice to implement, expecially for a major\nrelease like 1.5.0. I don't think its that difficult to do, we've\nall just been distracted by other topics and nobody has put code\nforward for it. If a well-written implementation materialized in\nthe next few days it might get enough cooking time before rc1 to\ninclude it.\n\n-- \nShawn.\n"},{"id":"30435","messageId":"7vtzzguvrp.fsf@assigned-by-dhcp.cox.net","threadId":"6140","inReplyTo":"20061228084245.GA18150@spearce.org","subject":"Re: [PATCH 0/11] Misc. pull/merge/am improvements","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-12-28T11:04:10Z","receivedAt":"2006-12-28T11:04:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shawn Pearce writes:\n\n> Junio C Hamano wrote:\n> ...\n>> I am very tempted to have sliding window mmap() if it helps\n>> people on cygwin, for example.\n>\n> Especially now that NO_MMAP is the default on that platform.\n> At this point it may be ready to graduate to next to try and get a\n> wider audience. Since fixing that segfault in pack-objects I can't\n> break it. Of course I couldn't break it before you found that error,\n> so take my words with a grain of salt... ;-)\n\nWell, I have a bad news for you.\n\n\"ng refs/heads/master n/a (unpacker error)\" is back. I cannot\npush things out.\n\nBut a bad news is that the problem does not decompose so easily;\ngit-push has too many players involved. I _think_ I have the\nlist of positive and negative objects fed to rev-list --objects\n--thin --stdin, whose output is in turn fed to pack-objects, but\nmanually running these steps in isolation seems to produce an Ok\nresult that index-pack --stdin --fix-thin accepts happily.\n\nWhile I was looking at the problem, I noticed something a bit\neasier to reproduce and should be lot easier to diagnose. At\nhttp://userweb.kernel.org/~junio/broken.tar, I have a tarball of\ngit.git repository.\n\nWhen you extract it, you will have a directory \"broken/\" with a\nsingle directory \".git/\" in it. The repository passes\n\"fsck-objects --full\" from master just fine.\n\nHowever, if you try to \"git repack -a -d\" it using \"next\" plus\n\"sp/mmap\" with your latest fix (\"pu\" also has it), you will see:\n\n Generating pack...\n Done counting 32054 objects.\n fatal: internal error: pack revindex corrupt\n\nIn that sample repository, I have pack.window, and\nrepack.deltabaseoffset. These settings do not seem to affect\nthe breakage.\n"},{"id":"30463","messageId":"20061229045327.GD12072@spearce.org","threadId":"6140","inReplyTo":"7vtzzguvrp.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 0/11] Misc. pull/merge/am improvements","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2006-12-29T04:53:27Z","receivedAt":"2006-12-29T04:53:27Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano wrote:\n> While I was looking at the problem, I noticed something a bit\n> easier to reproduce and should be lot easier to diagnose. At\n> http://userweb.kernel.org/~junio/broken.tar, I have a tarball of\n> git.git repository.\n\nThanks. I downloaded that tar but I can't debug it right now.\nI'm feeling under the weather and already had a long day; I'm too\nfried to seriously look at this pack code. I'll do it tomorrow\nevening.\n\n-- \nShawn.\n"},{"id":"30467","messageId":"7vmz57p6tn.fsf@assigned-by-dhcp.cox.net","threadId":"6140","inReplyTo":"20061229045327.GD12072@spearce.org","subject":"Re: [PATCH 0/11] Misc. pull/merge/am improvements","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-12-29T06:14:12Z","receivedAt":"2006-12-29T06:14:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shawn Pearce writes:\n\n> Junio C Hamano wrote:\n>> While I was looking at the problem, I noticed something a bit\n>> easier to reproduce and should be lot easier to diagnose. At\n>> http://userweb.kernel.org/~junio/broken.tar, I have a tarball of\n>> git.git repository.\n>\n> Thanks. I downloaded that tar but I can't debug it right now.\n> I'm feeling under the weather and already had a long day; I'm too\n> fried to seriously look at this pack code. I'll do it tomorrow\n> evening.\n\nI think there is a thinko in the OFS_DELTA arm of that switch\nstatement. You are resetting buf to (in-pack-offset + used), so\nyou should fetch the variably encoded length starting from buf[0].\n\nThis seems to fix that particular repacking for me.\n\n---\n\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex e9a1804..42dd8c8 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -1003,6 +1003,7 @@ static void check_object(struct object_entry *entry)\n \t\tif (!no_reuse_delta) {\n \t\t\tunsigned char c, *base_name;\n \t\t\tunsigned long ofs;\n+\t\t\tunsigned long used_0;\n \t\t\t/* there is at least 20 bytes left in the pack */\n \t\t\tswitch (entry->in_pack_type) {\n \t\t\tcase OBJ_REF_DELTA:\n@@ -1013,14 +1014,15 @@ static void check_object(struct object_entry *entry)\n \t\t\tcase OBJ_OFS_DELTA:\n \t\t\t\tbuf = use_pack(p, &w_curs,\n \t\t\t\t\tentry->in_pack_offset + used, NULL);\n-\t\t\t\tc = buf[used++];\n+\t\t\t\tused_0 = 0;\n+\t\t\t\tc = buf[used_0++];\n \t\t\t\tofs = c & 127;\n \t\t\t\twhile (c & 128) {\n \t\t\t\t\tofs += 1;\n \t\t\t\t\tif (!ofs || ofs & ~(~0UL >> 7))\n \t\t\t\t\t\tdie(\"delta base offset overflow in pack for %s\",\n \t\t\t\t\t\t sha1_to_hex(entry->sha1));\n-\t\t\t\t\tc = buf[used++];\n+\t\t\t\t\tc = buf[used_0++];\n \t\t\t\t\tofs = (ofs << 7) + (c & 127);\n \t\t\t\t}\n \t\t\t\tif (ofs >= entry->in_pack_offset)\n@@ -1028,6 +1030,7 @@ static void check_object(struct object_entry *entry)\n \t\t\t\t\t sha1_to_hex(entry->sha1));\n \t\t\t\tofs = entry->in_pack_offset - ofs;\n \t\t\t\tbase_name = find_packed_object_name(p, ofs);\n+\t\t\t\tused += used_0;\n \t\t\t\tbreak;\n \t\t\tdefault:\n \t\t\t\tbase_name = NULL;\n"},{"id":"30478","messageId":"Pine.LNX.4.63.0612291845070.19693@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"6140","inReplyTo":"7vmz58whnx.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 0/11] Misc. pull/merge/am improvements","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2006-12-29T17:46:02Z","receivedAt":"2006-12-29T17:46:02Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 28 Dec 2006, Junio C Hamano wrote:\n\n> Although I do find the detached HEAD attractive [...]\n\nYou do mean \"git fetch --depth 0\", don't you? (Totally untested, of \ncourse.)\n\nCiao,\nDscho\n"},{"id":"30479","messageId":"7vtzzeoafc.fsf@assigned-by-dhcp.cox.net","threadId":"6140","inReplyTo":"Pine.LNX.4.63.0612291845070.19693@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: [PATCH 0/11] Misc. pull/merge/am improvements","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-12-29T17:53:59Z","receivedAt":"2006-12-29T17:53:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> On Thu, 28 Dec 2006, Junio C Hamano wrote:\n>\n>> Although I do find the detached HEAD attractive [...]\n>\n> You do mean \"git fetch --depth 0\", don't you? (Totally untested, of \n> course.)\n\nNo, what I meant was\n\n\t$ git checkout v1.5.0\n\tChecking out a tag -- you are not on any branch now...\n $ ls -l .git/HEAD\n -rw-rw-r-- 1 junio junio 41 2006-12-29 09:51 .git/HEAD\n\t$ git branch\n master\n $ git commit -m 'fix' -a; echo $?\n You cannot commit without the current branch.\n 0\n $ git checkout -b maint-1.5.0\n $ git commit -m 'fix' -a\n"},{"id":"30511","messageId":"7vhcvdjiap.fsf@assigned-by-dhcp.cox.net","threadId":"6140","inReplyTo":"20061228084245.GA18150@spearce.org","subject":"Re: [PATCH 0/11] Misc. pull/merge/am improvements","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-12-30T19:27:26Z","receivedAt":"2006-12-30T19:27:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shawn Pearce writes:\n\n>> I am very tempted to have sliding window mmap() if it helps\n>> people on cygwin, for example.\n>\n> Especially now that NO_MMAP is the default on that platform.\n> At this point it may be ready to graduate to next to try and get a\n> wider audience. Since fixing that segfault in pack-objects I can't\n> break it. Of course I couldn't break it before you found that error,\n> so take my words with a grain of salt... ;-)\n\nAfter I fixed a few problems in sp/mmap topic (one was a bug\nthat screwed up OBJ_OFS_DELTA codepath in pack-objects, another\nwas an unrelated bug in send-pack that merging this series\nrevealed), I've tried to redo _all_ merges leading to the tip of\n'pu' in git.git history, with small and default mmap window\nsizes on my primary machine.\n\nWith the default setting, the whole experiment to redo 1561\nmerges took this much:\n\n193.54user 131.17system 5:23.48elapsed 100%CPU\n(0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+35700235minor)pagefaults 0swaps\n\nWhile with smaller window size i.e. with\n\n [core]\n packedgitwindowsize = 20\n packedgitlimit = 4194304\n\nthe numbers are almost the same:\n\n195.90user 134.40system 5:29.14elapsed 100%CPU\n(0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+35862520minor)pagefaults 0swaps\n\nA more important thing is that the merge results exactly match\nwhat I get without sp/mmap (i.e. 'next'). Interestingly, with\neither configurations, sp/mmap is slightly faster than 'next'\nbut I haven't done repeated tests so it may not be statistically\nsignificant.\n\nThis is by no means a proof of correctness, but is a good way to\ngain more confidence in the code.\n"},{"id":"30523","messageId":"20061231012115.GA5082@spearce.org","threadId":"6140","inReplyTo":"7vmz57p6tn.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 0/11] Misc. pull/merge/am improvements","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2006-12-31T01:21:15Z","receivedAt":"2006-12-31T01:21:15Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano wrote:\n> > Junio C Hamano wrote:\n> >> While I was looking at the problem, I noticed something a bit\n> >> easier to reproduce and should be lot easier to diagnose. At\n> >> http://userweb.kernel.org/~junio/broken.tar, I have a tarball of\n> >> git.git repository.\n> >\n> \n> I think there is a thinko in the OFS_DELTA arm of that switch\n> statement. You are resetting buf to (in-pack-offset + used), so\n> you should fetch the variably encoded length starting from buf[0].\n\nYes. Your patch looks correct; that's a really bad thinko on\nmy part. Thanks for tracking it down, and fixing it.\n\n-- \nShawn.\n"}]}