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

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

[Shawn Pearce]

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.

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

[Junio C Hamano]

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.

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

[Shawn Pearce]

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

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

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

[Junio C Hamano]

Shawn Pearce <spearce@spearce.org> writes:

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

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

[Shawn Pearce]

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.

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

[Junio C Hamano]

Shawn Pearce <spearce@spearce.org> writes:

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

---

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;

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

[Johannes Schindelin]

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

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

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> 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

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

[Junio C Hamano]

Shawn Pearce <spearce@spearce.org> writes:

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

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

[Shawn Pearce]

Junio C Hamano <junkio@cox.net> wrote:
> > 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.