threads / patch / 6140

patch, 11 partsMisc. pull/merge/am improvements

Subject: [PATCH 0/11] Misc. pull/merge/am improvements

## tl;dr

10 messages between Dec 28, 2006 and Dec 31, 2006. Diffs are folded; open one to read it.

replies: 9people: 3as markdown or json

Shawn Pearce· Dec 28, 2006, 07:34 UTC · lore

This series of patches is a group of minor interface and performance improvements for am/pull/rebase.

  1 Use GIT_REFLOG_ACTION environment variable instead.
  2 Honor GIT_REFLOG_ACTION in git-rebase.
    These two replace my prior 2 patch series in the same area.
  3 Use branch names in 'git-rebase -m' conflict hunks.
    This is a UI improvement to show better conflicts out of rebase.
  4 Ensure `git-pull` fails if `git-merge` fails.
    Trivial fix to make git-pull more friendly to other scripts.
  5 Honor pull.{twohead,octopus} in git-merge.
  6 Allow git-merge to select the default strategy.
    This moves the default merge strategy selection into git-merge,
    making it part of that tool rather than git-pull.  This makes
    it possible to get the same behavior from `git merge foo` as
    you already get from `git pull . foo`.
  7 Avoid git-fetch in `git-pull .` when possible.
    This is a performance improvement for pull, and offers some
    other nice benefits (see patch).
  8 Move better_branch_name above get_ref in merge-recursive.
  9 Allow merging bare trees in merge-recursive.
 10 Use merge-recursive in git-am -3.
    These three switch to merge-recursive in git-am, see patch 10's
    message for the benefits.
 11 Improve merge performance by avoiding in-index merges.
    This is a general performance improvement for all two-headed
    merges which might use merge-recursive.

I'd like to see these appear in v1.5.0, but we're getting close to the release so I can understand if they get delayed.

-- 
Shawn.
Junio C Hamano· Dec 28, 2006, 08:25 UTC · re: Shawn Pearce · lore

Re: [PATCH 0/11] Misc. pull/merge/am improvements

Shawn Pearce <spearce@spearce.org> writes:
> I'd like to see these appear in v1.5.0, but we're getting close to
> the release so I can understand if they get delayed.

I think shooting for -rc1 by mid January and stabilizing by early to mid February for the real release, would be a realistic timeline.

Actually I am getting more greedy, and would not mind to have clean ups and a few more features in the new release, even the ones that we have talked about but have not implemented.

I am very tempted to have sliding window mmap() if it helps people on cygwin, for example. Also, I've been running with "next" for my daily pushes and pulls without trouble, and I am very tempted to push out the shallow-clone topic. Not that I think its shallow-clone part is useful -- I do not use the feature myself so I cannot judge -- but at least when shallow is enabled on neither ends, it does not seem to break anything.

Although I do find the detached HEAD attractive and would want to have it eventually, I suspect that even if it materializes soon enough, it would at least need a couple of weeks of testing in 'next', so making -rc1 wait for it might push back the release a bit too much.

Shawn Pearce· Dec 28, 2006, 08:42 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/11] Misc. pull/merge/am improvements

Junio C Hamano <junkio@cox.net> wrote:
> Actually I am getting more greedy, and would not mind to have
> clean ups and a few more features in the new release, even the
> ones that we have talked about but have not implemented.

I know Linus wants me to fix the bash completion so it adds a trailing space when something gets completed and is unique and no additional text is expected to follow for that argument. I just haven't put the effort into it, even though I think I have a solution.

Basically I'm saying I probably could have another round of bash
completion in another week which may be worth considering for
inclusion.  ;-)
 
> I am very tempted to have sliding window mmap() if it helps
> people on cygwin, for example.

Especially now that NO_MMAP is the default on that platform. At this point it may be ready to graduate to next to try and get a wider audience. Since fixing that segfault in pack-objects I can't break it. Of course I couldn't break it before you found that error, so take my words with a grain of salt... ;-)

> Also, I've been running with
> "next" for my daily pushes and pulls without trouble, and I am
> very tempted to push out the shallow-clone topic.

Hasn't that been in next for a while now? I pretty much always run next, and have also been using it on that non-publishable repository for almost two months now. I've got 20 other users who I collaborate with on cygwin running next... we haven't had any issue with the portions of shallow-clone which had already moved in, and I upgrade that environment almost daily to keep current. Of course we also haven't tried to actually use the shallow-clone feature as we haven't needed it (the repository is only 50 MiB packed).

Show 5 quoted lines
> Although I do find the detached HEAD attractive and would want
> to have it eventually, I suspect that even if it materializes
> soon enough, it would at least need a couple of weeks of testing
> in 'next', so making -rc1 wait for it might push back the
> release a bit too much.

Agreed. It would be nice to implement, expecially for a major release like 1.5.0. I don't think its that difficult to do, we've all just been distracted by other topics and nobody has put code forward for it. If a well-written implementation materialized in the next few days it might get enough cooking time before rc1 to include it.

-- 
Shawn.
Junio C Hamano· Dec 28, 2006, 11:04 UTC · re: Shawn Pearce · lore

Re: [PATCH 0/11] Misc. pull/merge/am improvements

Shawn Pearce <spearce@spearce.org> writes:
Show 10 quoted lines
> Junio C Hamano <junkio@cox.net> wrote:
> ...
>> I am very tempted to have sliding window mmap() if it helps
>> people on cygwin, for example.
>
> Especially now that NO_MMAP is the default on that platform.
> At this point it may be ready to graduate to next to try and get a
> wider audience.  Since fixing that segfault in pack-objects I can't
> break it.  Of course I couldn't break it before you found that error,
> so take my words with a grain of salt... ;-)
Well, I have a bad news for you.

"ng refs/heads/master n/a (unpacker error)" is back. I cannot push things out.

But a bad news is that the problem does not decompose so easily; git-push has too many players involved. I _think_ I have the list of positive and negative objects fed to rev-list --objects --thin --stdin, whose output is in turn fed to pack-objects, but manually running these steps in isolation seems to produce an Ok result that index-pack --stdin --fix-thin accepts happily.

While I was looking at the problem, I noticed something a bit easier to reproduce and should be lot easier to diagnose. At http://userweb.kernel.org/~junio/broken.tar, I have a tarball of git.git repository.

When you extract it, you will have a directory "broken/" with a single directory ".git/" in it. The repository passes "fsck-objects --full" from master just fine.

However, if you try to "git repack -a -d" it using "next" plus "sp/mmap" with your latest fix ("pu" also has it), you will see:

        Generating pack...
        Done counting 32054 objects.
        fatal: internal error: pack revindex corrupt

In that sample repository, I have pack.window, and repack.deltabaseoffset. These settings do not seem to affect the breakage.

Shawn Pearce· Dec 29, 2006, 04:53 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/11] Misc. pull/merge/am improvements

Junio C Hamano <junkio@cox.net> wrote:
> While I was looking at the problem, I noticed something a bit
> easier to reproduce and should be lot easier to diagnose.  At
> http://userweb.kernel.org/~junio/broken.tar, I have a tarball of
> git.git repository.

Thanks. I downloaded that tar but I can't debug it right now. I'm feeling under the weather and already had a long day; I'm too fried to seriously look at this pack code. I'll do it tomorrow evening.

-- 
Shawn.
Junio C Hamano· Dec 29, 2006, 06:14 UTC · re: Shawn Pearce · lore

Re: [PATCH 0/11] Misc. pull/merge/am improvements

Shawn Pearce <spearce@spearce.org> writes:
Show 10 quoted lines
> Junio C Hamano <junkio@cox.net> wrote:
>> While I was looking at the problem, I noticed something a bit
>> easier to reproduce and should be lot easier to diagnose.  At
>> http://userweb.kernel.org/~junio/broken.tar, I have a tarball of
>> git.git repository.
>
> Thanks.  I downloaded that tar but I can't debug it right now.
> I'm feeling under the weather and already had a long day; I'm too
> fried to seriously look at this pack code.  I'll do it tomorrow
> evening.

I think there is a thinko in the OFS_DELTA arm of that switch statement. You are resetting buf to (in-pack-offset + used), so you should fetch the variably encoded length starting from buf[0].

This seems to fix that particular repacking for me.
---
Show changes to builtin-pack-objects.c +5 −2
diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index e9a1804..42dd8c8 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -1003,6 +1003,7 @@ static void check_object(struct object_entry *entry)
 		if (!no_reuse_delta) {
 			unsigned char c, *base_name;
 			unsigned long ofs;
+			unsigned long used_0;
 			/* there is at least 20 bytes left in the pack */
 			switch (entry->in_pack_type) {
 			case OBJ_REF_DELTA:
@@ -1013,14 +1014,15 @@ static void check_object(struct object_entry *entry)
 			case OBJ_OFS_DELTA:
 				buf = use_pack(p, &w_curs,
 					entry->in_pack_offset + used, NULL);
-				c = buf[used++];
+				used_0 = 0;
+				c = buf[used_0++];
 				ofs = c & 127;
 				while (c & 128) {
 					ofs += 1;
 					if (!ofs || ofs & ~(~0UL >> 7))
 						die("delta base offset overflow in pack for %s",
 						    sha1_to_hex(entry->sha1));
-					c = buf[used++];
+					c = buf[used_0++];
 					ofs = (ofs << 7) + (c & 127);
 				}
 				if (ofs >= entry->in_pack_offset)
@@ -1028,6 +1030,7 @@ static void check_object(struct object_entry *entry)
 					    sha1_to_hex(entry->sha1));
 				ofs = entry->in_pack_offset - ofs;
 				base_name = find_packed_object_name(p, ofs);
+				used += used_0;
 				break;
 			default:
 				base_name = NULL;
Shawn Pearce· Dec 31, 2006, 01:21 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/11] Misc. pull/merge/am improvements

Junio C Hamano <junkio@cox.net> wrote:
Show 10 quoted lines
> > Junio C Hamano <junkio@cox.net> wrote:
> >> While I was looking at the problem, I noticed something a bit
> >> easier to reproduce and should be lot easier to diagnose.  At
> >> http://userweb.kernel.org/~junio/broken.tar, I have a tarball of
> >> git.git repository.
> >
> 
> I think there is a thinko in the OFS_DELTA arm of that switch
> statement.  You are resetting buf to (in-pack-offset + used), so
> you should fetch the variably encoded length starting from buf[0].

Yes. Your patch looks correct; that's a really bad thinko on my part. Thanks for tracking it down, and fixing it.

-- 
Shawn.
Junio C Hamano· Dec 30, 2006, 19:27 UTC · re: Shawn Pearce · lore

Re: [PATCH 0/11] Misc. pull/merge/am improvements

Shawn Pearce <spearce@spearce.org> writes:
Show 8 quoted lines
>> I am very tempted to have sliding window mmap() if it helps
>> people on cygwin, for example.
>
> Especially now that NO_MMAP is the default on that platform.
> At this point it may be ready to graduate to next to try and get a
> wider audience.  Since fixing that segfault in pack-objects I can't
> break it.  Of course I couldn't break it before you found that error,
> so take my words with a grain of salt... ;-)

After I fixed a few problems in sp/mmap topic (one was a bug that screwed up OBJ_OFS_DELTA codepath in pack-objects, another was an unrelated bug in send-pack that merging this series revealed), I've tried to redo _all_ merges leading to the tip of 'pu' in git.git history, with small and default mmap window sizes on my primary machine.

With the default setting, the whole experiment to redo 1561 merges took this much:

193.54user 131.17system 5:23.48elapsed 100%CPU (0avgtext+0avgdata 0maxresident)k 0inputs+0outputs (0major+35700235minor)pagefaults 0swaps

While with smaller window size i.e. with
        [core]
                packedgitwindowsize = 20
                packedgitlimit = 4194304
the numbers are almost the same:

195.90user 134.40system 5:29.14elapsed 100%CPU (0avgtext+0avgdata 0maxresident)k 0inputs+0outputs (0major+35862520minor)pagefaults 0swaps

A more important thing is that the merge results exactly match what I get without sp/mmap (i.e. 'next'). Interestingly, with either configurations, sp/mmap is slightly faster than 'next' but I haven't done repeated tests so it may not be statistically significant.

This is by no means a proof of correctness, but is a good way to gain more confidence in the code.

Johannes Schindelin· Dec 29, 2006, 17:46 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/11] Misc. pull/merge/am improvements

Hi,
On Thu, 28 Dec 2006, Junio C Hamano wrote:
> Although I do find the detached HEAD attractive [...]

You do mean "git fetch --depth 0", don't you? (Totally untested, of course.)

Ciao, Dscho

Junio C Hamano· Dec 29, 2006, 17:53 UTC · re: Johannes Schindelin · lore

Re: [PATCH 0/11] Misc. pull/merge/am improvements

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 6 quoted lines
> On Thu, 28 Dec 2006, Junio C Hamano wrote:
>
>> Although I do find the detached HEAD attractive [...]
>
> You do mean "git fetch --depth 0", don't you? (Totally untested, of 
> course.)
No, what I meant was
	$ git checkout v1.5.0
	Checking out a tag -- you are not on any branch now...
        $ ls -l .git/HEAD
        -rw-rw-r-- 1 junio junio 41 2006-12-29 09:51 .git/HEAD
	$ git branch
          master
        $ git commit -m 'fix' -a; echo $?
        You cannot commit without the current branch.
        0
        $ git checkout -b maint-1.5.0
        $ git commit -m 'fix' -a

← back to recent threads