Git development
 help / color / mirror / Atom feed
* Re: git svn clone terminating prematurely (I think)
From: Steven Line @ 2012-01-12 18:25 UTC (permalink / raw)
  To: git
In-Reply-To: <20120111224833.GA29654@burratino>

Thank you Jonathan.

I had a breakthrough yesterday on this problem.  To make a long story
short my ssh connection to the server where I was running 'nohup git
svn clone' was timing out.  Additionally the nohup I was using wasn't
really protecting the git svn clone process so an hour or two after
the ssh disconnected, the git would terminate leaving me with an
imcomplete repository.  I don't understand why the nohup wasn't
working yet.  So my problem wasn't due to git itself.

I started my most recent git svn clone and it's now been running for
18 hours.  I'm optimistic that I've solved the problem.  If my git
does terminate early then I'll try a 'git svn fetch' to complete the
clone, based on what you said in your post.

-- 
Steven Line
303-910-1212
sline00@gmail.com

^ permalink raw reply

* Re: [BUG] multi-commit cherry-pick messes up the order of commits
From: Junio C Hamano @ 2012-01-12 18:25 UTC (permalink / raw)
  To: Ramkumar Ramachandra
  Cc: Jeff King, SZEDER Gábor, Christian Couder, Christian Couder,
	git, Jonathan Nieder
In-Reply-To: <CALkWK0=Mv_tzNw-hN_9fAr+vABappndEK5iSWQHDk8Yk6Z-stw@mail.gmail.com>

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> What are your thoughts on making it a flag in the revision API to be
> activated with "cherry-pick --literal-order commit1 commit3 commit2"
> or similar?

That is an insane UI for the sake of flexibility.

You should be able to look at revs->cmdline and tell if you need to let
cherry-pick walk (i.e. "cherry-pick master..next"), or if the user wants
individual commits (i.e. "cherry-pick A B C").

And you do prepare_revision_walk() only when you need to walk; otherwise
you use the contents of revs->pending in order.

^ permalink raw reply

* Re: [PATCH] diff --no-index: support more than one file pair
From: Matthieu Moy @ 2012-01-12 18:26 UTC (permalink / raw)
  To: Nguyen Thai Ngoc Duy; +Cc: git
In-Reply-To: <CACsJy8B460CwtACV+o0wnwqi1On_oEavLfDAL8f=w6kyfktKcQ@mail.gmail.com>

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

>>>  - hell, i might even benefit from git copy/modify detection
>>
>> I don't see how, if you specify explicitely the pairs (old, new). You
>> may have such benefit if you let the command-line express "here's a
>> bunch of old files, and a bunch of new ones", but not with your proposed
>> syntax.
>
> That's what git gives to diff machinery: a set of file pairs, and the
> diff machinery has to figure out copy/modify pairs, shuffling them up
> if necessary. I simply cut of tree traversal part out and feed file
> pairs directly to diff machinery.

If you want to benefit from copy detection, you cannot hardcode the fact
that you have as many source and destination files. And even to benefit
from rename detection, I find the user interface really weird. If I
provide files in pairs, I really don't expect Git to shuffle them like

git diff --no-index A1 B1 A2 B2
--- A1
+++ B2
..
--- A2
+++ B1
..

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

^ permalink raw reply

* Re: [PATCH] cherry-pick: add failing test for out-of-order pick
From: Jonathan Nieder @ 2012-01-12 18:32 UTC (permalink / raw)
  To: Ramkumar Ramachandra
  Cc: Jeff King, SZEDER Gábor, Christian Couder, Christian Couder,
	git
In-Reply-To: <1326390647-21446-1-git-send-email-artagnon@gmail.com>

Ramkumar Ramachandra wrote:

>   $ git cherry-pick master..topic
>   $ git cherry-pick topic ^master
>   $ git cherry-pick ^master topic
>
> So the order of the arguments specified on the command-line is
> irrelevant in these cases.  However, there are cases where it is worth
> paying attention to the order.  For instance:
>

This segue feels a bit unnatural.  I think the relevant point was that
early output from revision traversal (and perhaps some other things
--- I haven't checked) relies on commits having been inserted in a
topologically sorted order.

Anyway, I don't think the background is necessary --- the
one-paragraph description below stands well enough alone.

>   $ git cherry-pick commit3 commit1 commit2
> 
> picks commits after sorting by date order, which is counter-intuitive.
> Add a failing test to t3508 (cherry-pick-many-commits) documenting
> this behavior.
> 
> Reported-by: SZEDER Gábor <szeder@ira.uka.de>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
[...]
> --- a/t/t3508-cherry-pick-many-commits.sh
> +++ b/t/t3508-cherry-pick-many-commits.sh
> @@ -59,6 +59,31 @@ test_expect_success 'cherry-pick first..fourth works' '
>  	check_head_differs_from fourth
>  '
>  
> +test_expect_failure 'cherry-pick picks commits in the right order' '

I would say "in the order requested" instead of the right order, since
it is not completely obvious to me what the right order is.

> +	cat <<-\EOF >expected &&
> +	[master OBJID] fourth
> +	 Author: A U Thor <author@example.com>
> +	 1 files changed, 1 insertions(+), 0 deletions(-)
> +	[master OBJID] second
> +	 Author: A U Thor <author@example.com>
> +	 1 files changed, 1 insertions(+), 0 deletions(-)
> +	[master OBJID] third
> +	 Author: A U Thor <author@example.com>
> +	 1 files changed, 1 insertions(+), 0 deletions(-)
> +	EOF

Why check all these details of formatting, instead of e.g. using "git
rev-list | git diff-tree -s --format=%s"?

[...]
> +	test_cmp expected actual.fuzzy &&
> +	check_head_differs_from second

Why make the same check twice?

Hope that helps,
Jonathan

^ permalink raw reply

* Re: [BUG] multi-commit cherry-pick messes up the order of commits
From: Junio C Hamano @ 2012-01-12 18:41 UTC (permalink / raw)
  To: Jonathan Nieder
  Cc: Jeff King, SZEDER Gábor, Christian Couder, Christian Couder,
	git, Ramkumar Ramachandra
In-Reply-To: <20120112174731.GA6038@burratino>

Jonathan Nieder <jrnieder@gmail.com> writes:

> Yes, exactly.  Another question: what should
>
> 	git cherry-pick master..next maint..master
>
> do?

Revision ranges are not defined as a union of sets, but a single set as a
range between zero or more bottom (UNINTERESTING) commits and zero or more
top commits, the ones reachable from the top but not from the bottom, so
the above will work as if you said "^master ^maint master next", which is
the same as "master..next" (if you assume all of "maint" is contained in
"master" all of which is contained in "next", of course).

And it is not likely to change soon.

In the longer term (or in an alternate universe where we were inventing
Git from scratch today without any existing users), we may want to revamp
the revision machinery, taking advantage of the recent addition of the
"cmdline" facility to it, so that we would walk ranges "master..next" and
"maint..master" independently, clearing the object flags as needed between
the separate traversals as needed, and then take a union of these ranges,
before returning results from get_revision() calls.

^ permalink raw reply

* Bug? Git checkout fails with a wrong error message
From: Yves Goergen @ 2012-01-12 18:44 UTC (permalink / raw)
  To: git

Hi,

I am using Git alone for my local software project in Visual Studio 2010. I've
been on the master branch most of the time. Recently I created a new branch to
do a larger refactoring of one of the dialogue windows. I did the following
modifications:

* Rename Form1 to Form1a (including all depending files)
* Add new Form1

I checked this change into the branch, say form-refactoring. Interestingly, Git
didn't notice that I renamed the file Form1.cs into Form1a.cs and created a
brand new, totally different Form1.cs, but instead it noticed a new Form1a.cs
file and found a whole lot of differences between the previous and new Form1.cs
files. This will of course lead to totally garbaged diffs, but I don't care in
this case as long as all files are handled correctly in the end.

Then I switched back to master to do some other small changes. Nothing
conflicting. Until now, everything worked fine.

Today, I wanted to switch back to my branch form-refactoring to continue that
work. But all I get is the following message:

-----
git.exe checkout    form-refactoring

Aborting
error: The following untracked working tree files would be overwritten by
checkout:
Form1.Designer.cs
Please move or remove them before you can switch branches.
-----

What is that supposed to be? The mentioned file is not untracked. Neither in the
master branch, nor in the form-refactoring branch. It is part of both branches,
but one is not a descendent of the other (because it was recreated on the
form-refactoring branch, if that matters). What would happen if I delete it, is
it gone for good then? I don't trust Git to bring back the correct file if I
delete something now. I did not play with any file at all outside of my
mentioned Git operations, so why should I play around with any file to continue
using Git operations now? Git broke it, Git's supposed to handle it now!

Here's some other input:

There are no uncommitted changes in my working directory. 'git status' doesn't
list anything.

The file in question is not untracked. Right now on the master branch, it has a
green checkmark in Explorer (provided by TortoiseGit) and it has a history as
well. There are more Form....Designer.cs files that don't cause any trouble.

'git clean -f -d', 'git reset --hard HEAD', 'git stash' do nothing and don't
help resolving the issue.

Right now, I cannot continue with my work because I cannot switch branches. Is
there an easy solution to this? Is my Git repository broken, all by standard
operations?

^ permalink raw reply

* [PATCH v2] cherry-pick: add failing test for out-of-order pick
From: Ramkumar Ramachandra @ 2012-01-12 19:05 UTC (permalink / raw)
  To: Jonathan Nieder
  Cc: SZEDER Gábor, Christian Couder, Christian Couder, git,
	Jeff King
In-Reply-To: <20120112183246.GB6038@burratino>

The invocation

  $ git cherry-pick commit3 commit1 commit2

picks commits after sorting by date order, which is counter-intuitive.
Add a failing test to t3508 (cherry-pick-many-commits) documenting
this behavior.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Had some weird compulsion to conform to the style of the other tests
 in the previous iteration.

 t/t3508-cherry-pick-many-commits.sh |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/t/t3508-cherry-pick-many-commits.sh b/t/t3508-cherry-pick-many-commits.sh
index 8e09fd0..d9d632d 100755
--- a/t/t3508-cherry-pick-many-commits.sh
+++ b/t/t3508-cherry-pick-many-commits.sh
@@ -59,6 +59,23 @@ test_expect_success 'cherry-pick first..fourth works' '
 	check_head_differs_from fourth
 '
 
+test_expect_failure 'cherry-pick picks commits in the order requested' '
+	git checkout -f master &&
+	git reset --hard first &&
+	test_tick &&
+	git cherry-pick fourth second third &&
+	{
+		git rev-list --reverse HEAD |
+		git diff-tree --stdin -s --format=%s
+	} >actual &&
+	cat >expect <<-\EOF &&
+	fourth
+	second
+	third
+	EOF
+	test_cmp expect actual
+'
+
 test_expect_success 'cherry-pick --strategy resolve first..fourth works' '
 	cat <<-\EOF >expected &&
 	Trying simple merge.
-- 
1.7.8.2

^ permalink raw reply related

* Re: [PATCH] stash show: use default pretty format
From: Junio C Hamano @ 2012-01-12 19:06 UTC (permalink / raw)
  To: Tay Ray Chuan; +Cc: Git Mailing List
In-Reply-To: <1326351953-3724-1-git-send-email-rctay89@gmail.com>

Tay Ray Chuan <rctay89@gmail.com> writes:

> By default (ie. when stash show is invoked without any arguments), the
> diff stat of the stashed changes is displayed. Let git-diff decide the
> default pretty format to use.
>
> This gives git more consistency, as users who have set their
> pretty.format config would naturally expect `git-stash show` to display
> the diff in the same pretty format as the other diff-producing procelain
> like git-log and git-show.

A handful of issues:

 - The stash entries, unlike the usual commits you store on branches and
   inspect with "show", are designed to be quick escapes for emergency
   interruption, and "--stat" is a good default to remind the user what
   she was working on before she was interrupted _without_ scrolling the
   top of the screen away by showing the full diff.  Careful design
   decisions far outweigh mechanical application of "consistency for the
   sake of consistency".

 - What does "pretty.format" has anything to do with "stash"?

 - If it does, why doesn't the script read from it?

 - How does this justify the UI regression for people who are used to the
   good default "--stat" they have been seeing?

^ permalink raw reply

* Re: [BUG] multi-commit cherry-pick messes up the order of commits
From: Johannes Sixt @ 2012-01-12 19:21 UTC (permalink / raw)
  To: Ramkumar Ramachandra
  Cc: Jeff King, SZEDER Gábor, Christian Couder, Christian Couder,
	git, Jonathan Nieder
In-Reply-To: <CALkWK0=Mv_tzNw-hN_9fAr+vABappndEK5iSWQHDk8Yk6Z-stw@mail.gmail.com>

Am 12.01.2012 18:09, schrieb Ramkumar Ramachandra:
> @@ -2054,7 +2054,10 @@ int prepare_revision_walk(struct rev_info *revs)
>                 if (commit) {
>                         if (!(commit->object.flags & SEEN)) {
>                                 commit->object.flags |= SEEN;
> -                               commit_list_insert_by_date(commit,
> &revs->commits
> +                               if (revs->literal_order)
> +                                       commit_list_insert(commit,
> &revs->commits
> +                               else
> +
> commit_list_insert_by_date(commit, &revs-

Why do we need a new flag?

  git show origin/master origin/maint
  git show origin/maint origin/master

show the revisions in different order, in particular, in the order
requested on the command line. Shoudn't cherry-pick be able to do the
same without new hacks?

-- Hannes

^ permalink raw reply

* Re: [PATCH] word-diff: ignore '\ No newline at eof' marker
From: Junio C Hamano @ 2012-01-12 19:23 UTC (permalink / raw)
  To: Thomas Rast; +Cc: Ivan Shirokoff, git
In-Reply-To: <902665ee053876c2684f5b935ee4f81e77122802.1326366909.git.trast@student.ethz.ch>

Thomas Rast <trast@student.ethz.ch> writes:

> A proper fix would defer such markers until the end of the hunk.
> However, word-diff is inherently whitespace-ignoring, so as a cheap
> fix simply ignore the marker (and hide it from the output).

Sounds like a very sensible simplification of the issue.

> We use a prefix match for '\ ' to parallel the logic in
> apply.c:parse_fragment().  We currently do not localize this string
> (just accept other variants of it in git-apply), but this should be
> future-proof.
>
> Noticed-by: Ivan Shirokoff <shirokoff@yandex-team.ru>
> Signed-off-by: Thomas Rast <trast@student.ethz.ch>
> ---
>
>  diff.c                |    8 ++++++++
>  t/t4034-diff-words.sh |   14 ++++++++++++++
>  2 files changed, 22 insertions(+), 0 deletions(-)
>
> diff --git a/diff.c b/diff.c
> index a65223a..996cc60 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -1113,6 +1113,14 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
>  			diff_words_append(line, len,
>  					  &ecbdata->diff_words->plus);
>  			return;
> +		} else if (!prefixcmp(line, "\\ ")) {
> +			/*
> +			 * Silently eat the "no newline at eof" marker
> +			 * (we are diffing without regard to
> +			 * whitespace anyway), and defer processing:
> +			 * more '+' lines could be after it.
> +			 */
> +			return;
>  		}
>  		diff_words_flush(ecbdata);

It took me a while to realize "defer processing" in the comment was meant
to justify the placement of the new block _before_ this flush. Perhaps
rephrasing it to "return without calling diff_words_flush()" would make it
more readable?

Otherwise the patch looks good.

Thanks.

>  		if (ecbdata->diff_words->type == DIFF_WORDS_PORCELAIN) {
> diff --git a/t/t4034-diff-words.sh b/t/t4034-diff-words.sh
> index 6f1e5a2..5c20121 100755
> --- a/t/t4034-diff-words.sh
> +++ b/t/t4034-diff-words.sh
> @@ -334,4 +334,18 @@ test_expect_success 'word-diff with diff.sbe' '
>  	word_diff --word-diff=plain
>  '
>  
> +test_expect_success 'word-diff with no newline at EOF' '
> +	cat >expect <<-\EOF &&
> +	diff --git a/pre b/post
> +	index 7bf316e..3dd0303 100644
> +	--- a/pre
> +	+++ b/post
> +	@@ -1 +1 @@
> +	a a [-a-]{+ab+} a a
> +	EOF
> +	printf "%s" "a a a a a" >pre &&
> +	printf "%s" "a a ab a a" >post &&
> +	word_diff --word-diff=plain
> +'
> +
>  test_done

^ permalink raw reply

* Re: [BUG] multi-commit cherry-pick messes up the order of commits
From: Ramkumar Ramachandra @ 2012-01-12 19:25 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Jeff King, SZEDER Gábor, Christian Couder, Christian Couder,
	git, Jonathan Nieder
In-Reply-To: <7vaa5s3hiq.fsf@alter.siamese.dyndns.org>

Junio C Hamano wrote:
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>
>> What are your thoughts on making it a flag in the revision API to be
>> activated with "cherry-pick --literal-order commit1 commit3 commit2"
>> or similar?
>
> That is an insane UI for the sake of flexibility.
>
> You should be able to look at revs->cmdline and tell if you need to let
> cherry-pick walk (i.e. "cherry-pick master..next"), or if the user wants
> individual commits (i.e. "cherry-pick A B C").
>
> And you do prepare_revision_walk() only when you need to walk; otherwise
> you use the contents of revs->pending in order.

Okay, just to make sure I understand this correctly: if more than one
argument is literally specified, I should not set up the revision
walker and pick the commits listed in revs->pending, correct?  Then,
when I encounter the following command,

  $ git cherry-pick maint ^master

I should just pick two commits: maint, and ^master.  But won't this
introduce some kind of regression for those who expect me to pick the
master..maint range instead?  Has this double-interpretation issue
come up in other commands?  Have we documented this somewhere?

Thanks.

-- Ram

^ permalink raw reply

* Re: [BUG] multi-commit cherry-pick messes up the order of commits
From: Ramkumar Ramachandra @ 2012-01-12 19:29 UTC (permalink / raw)
  To: Johannes Sixt
  Cc: Jeff King, SZEDER Gábor, Christian Couder, Christian Couder,
	git, Jonathan Nieder
In-Reply-To: <4F0F32CC.8040404@kdbg.org>

Hi Johannes,

Johannes Sixt wrote:
> Why do we need a new flag?
>
>  git show origin/master origin/maint
>  git show origin/maint origin/master
>
> show the revisions in different order, in particular, in the order
> requested on the command line. Shoudn't cherry-pick be able to do the
> same without new hacks?

That was my first reaction too -- then I saw builtin/push.c (the
builtin show is quite similar), and found out that it doesn't use the
revision walker at all.  It operates on refs, which has different
semantics altogether (called "refspec" in some places I think).

-- Ram

^ permalink raw reply

* Re: [PATCH v2] cherry-pick: add failing test for out-of-order pick
From: Jonathan Nieder @ 2012-01-12 19:33 UTC (permalink / raw)
  To: Ramkumar Ramachandra
  Cc: SZEDER Gábor, Christian Couder, Christian Couder, git,
	Jeff King
In-Reply-To: <1326395132-18947-1-git-send-email-artagnon@gmail.com>

Ramkumar Ramachandra wrote:

>  Had some weird compulsion to conform to the style of the other tests
>  in the previous iteration.

The tests you're talking about were introduced in commit 7b53b92f to
check for a buglet that made --strategy suppress the progress
reporting ("Finished one cherry-pick.") output cherry-pick normally
would emit.  

So no inconsistency here --- those tests are _intending_ to check the
output format and that cherry-pick, unlike cherry-pick --ff, produces
new commits (though it would probably be clearer to put checks for
these behaviors in separate test assertions), while the new failing
test you are introducing is not about those things.

Striving for a consistent style is certainly not weird.

> --- a/t/t3508-cherry-pick-many-commits.sh
> +++ b/t/t3508-cherry-pick-many-commits.sh
> @@ -59,6 +59,23 @@ test_expect_success 'cherry-pick first..fourth works' '
[...]
> +	git cherry-pick fourth second third &&
> +	{
> +		git rev-list --reverse HEAD |
> +		git diff-tree --stdin -s --format=%s
> +	} >actual &&
> +	cat >expect <<-\EOF &&
> +	fourth
> +	second
> +	third
> +	EOF
> +	test_cmp expect actual

This still feels more convoluted than expected (e.g., why --reverse?).
Something like

	printf "%s\n" third second fourth >expect &&
	...
	git log --format=%s >actual &&
	test_cmp expect actual

should be plenty.

^ permalink raw reply

* Re: [BUG] multi-commit cherry-pick messes up the order of commits
From: Jonathan Nieder @ 2012-01-12 19:34 UTC (permalink / raw)
  To: Ramkumar Ramachandra
  Cc: Johannes Sixt, Jeff King, SZEDER Gábor, Christian Couder,
	Christian Couder, git
In-Reply-To: <CALkWK0=NVUd629FgkPfgi8ZgTuO+a10t+iwbSrAvONCSmeq2rQ@mail.gmail.com>

Ramkumar Ramachandra wrote:

> That was my first reaction too -- then I saw builtin/push.c (the
> builtin show is quite similar), and found out that it doesn't use the
> revision walker at all.

"git push", unlike "git show", does not accept arguments like
maint..master.

^ permalink raw reply

* Re: [BUG] multi-commit cherry-pick messes up the order of commits
From: Jeff King @ 2012-01-12 19:47 UTC (permalink / raw)
  To: Ramkumar Ramachandra
  Cc: Junio C Hamano, SZEDER Gábor, Christian Couder,
	Christian Couder, git, Jonathan Nieder
In-Reply-To: <CALkWK0kk0mVNaetr=triuVYva7inyx2aZvam81qTVA9=Q=UzGw@mail.gmail.com>

On Fri, Jan 13, 2012 at 12:55:58AM +0530, Ramkumar Ramachandra wrote:

> Junio C Hamano wrote:
> > You should be able to look at revs->cmdline and tell if you need to let
> > cherry-pick walk (i.e. "cherry-pick master..next"), or if the user wants
> > individual commits (i.e. "cherry-pick A B C").
> >
> > And you do prepare_revision_walk() only when you need to walk; otherwise
> > you use the contents of revs->pending in order.

I am tempted to suggest that cherry-pick should not feed its arguments
to the revision machinery in the first place, but instead accept a set
of arguments, each argument of which is either a single commit
(interpreted by get_sha1) or a range specifier (which can be fed to
setup-revisions). And then get a linearized set of commits for _each_
argument independently and concatenate them (possibly eliminating
duplicates). That would make all of these work as most people would
expect:

  git cherry-pick A B C
  git cherry-pick A..B
  git cherry-pick A..B B..C

but would be a regression for:

  git cherry-pick B ^A

versus the current code. I suspect that the latter form is not all that
commonly used, though, and certainly I would accept it as a casualty of
making the "A B C" form work. My only hesitation is that it is in fact a
regression.

> Okay, just to make sure I understand this correctly: if more than one
> argument is literally specified, I should not set up the revision
> walker and pick the commits listed in revs->pending, correct?  Then,
> when I encounter the following command,
> 
>   $ git cherry-pick maint ^master
> 
> I should just pick two commits: maint, and ^master.

But ^master is not a commit, it is a negation. So it is nonsensical if
the arguments are considered independent of each other (you _could_
use a heuristic to guess that they are not independent, but I'd rather
not go there). So you'd probably end up just rejecting the arguments.

-Peff

^ permalink raw reply

* Re: [BUG] multi-commit cherry-pick messes up the order of commits
From: Jonathan Nieder @ 2012-01-12 20:11 UTC (permalink / raw)
  To: Jeff King
  Cc: Ramkumar Ramachandra, Junio C Hamano, SZEDER Gábor,
	Christian Couder, Christian Couder, git
In-Reply-To: <20120112194710.GA28148@sigill.intra.peff.net>

Jeff King wrote:

> I am tempted to suggest
[...]
>              That would make all of these work as most people would
> expect:
>
>   git cherry-pick A B C
>   git cherry-pick A..B
>   git cherry-pick A..B B..C
>
> but would be a regression for:
>
>   git cherry-pick B ^A
>
> versus the current code. I suspect that the latter form is not all that
> commonly used, though, and certainly I would accept it as a casualty of
> making the "A B C" form work. My only hesitation is that it is in fact a
> regression.

I find myself using such complicated expressions as

	list-revs-to-skip |
	xargs git cherry-pick --cherry-pick --right-only HEAD...topic --not

so yeah, that would be a pretty serious loss in functionality.

However, moving to something like the far future semantics that Junio
hinted at, for cherry-pick/revert and other --no-walk style commands
only, would not be a regression for me.  The multi-pick feature is
still young, and I _suspect_ changing the meaning of A..B B..C for it
would not inconvenience anybody.

I would even welcome a change in the meaning of B ^A: the most
intuitive thing for it to do would be to cherry-pick the single commit B
when and only when it is not an ancestor of A.

Jonathan

^ permalink raw reply

* Re: [BUG] multi-commit cherry-pick messes up the order of commits
From: Junio C Hamano @ 2012-01-12 20:11 UTC (permalink / raw)
  To: Ramkumar Ramachandra
  Cc: Jeff King, SZEDER Gábor, Christian Couder, Christian Couder,
	git, Jonathan Nieder
In-Reply-To: <CALkWK0kk0mVNaetr=triuVYva7inyx2aZvam81qTVA9=Q=UzGw@mail.gmail.com>

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Okay, just to make sure I understand this correctly: if more than one
> argument is literally specified, I should not set up the revision
> walker and pick the commits listed in revs->pending, correct?

Not really.

A rough approximation would be that if you see any negative ones (either
coming from A..B or ^A), you would always want to walk, giving everything
to prepare_revision_walk()-and-then-get_revision() machiery.

Otherwise you have only zero [*1*] or more positive ones, and you pick
them in the order you find in the pending list, without bothering the
revision traversal machinery at all [*2*].

> when I encounter the following command,
>
>   $ git cherry-pick maint ^master
>
> I should just pick two commits: maint, and ^master.

So the answer is aboslutely no. "maint ^master" is the same as saying
"master..maint".

[Footnote]

*1* You would probably want to error out if you got zero positive ones in
this case (i.e. absolutely nothing was given, neither positive nor
negative).

*2* The reason this is "rough" approximation is because we would probably
want to do Jonathan's "maint..master master..next" someday, and when that
happens, we would need to do much more than "do we have any negative? then
send it through to prepare_revision_walk()". But we are not there yet, so
I think the above is actually more or less the complete implementation.

^ permalink raw reply

* Re: [BUG] multi-commit cherry-pick messes up the order of commits
From: Jeff King @ 2012-01-12 20:17 UTC (permalink / raw)
  To: Jonathan Nieder
  Cc: Ramkumar Ramachandra, Junio C Hamano, SZEDER Gábor,
	Christian Couder, Christian Couder, git
In-Reply-To: <20120112201122.GE6038@burratino>

On Thu, Jan 12, 2012 at 02:11:22PM -0600, Jonathan Nieder wrote:

> > I am tempted to suggest
> [...]
> >              That would make all of these work as most people would
> > expect:
> >
> >   git cherry-pick A B C
> >   git cherry-pick A..B
> >   git cherry-pick A..B B..C
> >
> > but would be a regression for:
> >
> >   git cherry-pick B ^A
> >
> > versus the current code. I suspect that the latter form is not all that
> > commonly used, though, and certainly I would accept it as a casualty of
> > making the "A B C" form work. My only hesitation is that it is in fact a
> > regression.
> 
> I find myself using such complicated expressions as
> 
> 	list-revs-to-skip |
> 	xargs git cherry-pick --cherry-pick --right-only HEAD...topic --not
> 
> so yeah, that would be a pretty serious loss in functionality.

That's gross. :)

But thank you for providing a real-world example. I had a vague notion
that the full power of the revision parser was not actually useful to
people, but clearly not.

OTOH, if cherry-pick were more simplistic, you could perhaps get by
with:

  list-revs-to-skip |
  xargs git rev-list --cherry-pick --right-only HEAD...topic --not |
  git cherry-pick --stdin

-Peff

^ permalink raw reply

* Re: [PATCH v2 2/3] index-pack: eliminate recursion in find_unresolved_deltas
From: Junio C Hamano @ 2012-01-12 20:32 UTC (permalink / raw)
  To: Nguyen Thai Ngoc Duy; +Cc: git, Shawn O. Pearce
In-Reply-To: <CACsJy8Cz-qWs2wrOYTjDMPjJH0wRQCFy9u6OFVPzn6YV0a6WaQ@mail.gmail.com>

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

> 2012/1/10 Junio C Hamano <gitster@pobox.com>:
>> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>>
>>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>>
>> I find both the original and the updated code rather dense to read without
>> annotation, but from a cursory look all changes look good.
>
> Maybe I stared at it for too long it seems obvious to me (hence no
> further description in commit message). Let me describe it (and put in
> commit message later if it makes sense)
>
> Current code already links all bases together in a form of tree, using
> struct base_data, with prev_base pointer to point to parent node. The
> only problem is that struct base_data is all allocated on stack. So we
> need to put all on heap (parse_pack_objects and
> fix_unresolved_deltas). After that, it's simple depth-first traversal
> where each node also maintains its own state (ofs and ref indices to
> iterate over all children nodes).
>
> So we process one node:
>
>  - if it returns a new (child) node (a parent base), we link it to our
> tree, then process the new node.
>  - if it returns nothing, the node is done, free it. We go back to
> parent node and resume whatever it's doing.
>
> and do it until we have no nodes to process.

If you have the current path (base to another that is recorded as a delta
to it to yet another that is recorded as a delta to that delitified
object) on the stack, it is obvious that as you have done with the objects
on the deeper end of the delta chain, the data that becomes unnecessary
will be gone by simply returning from the recursion, but if you "put all
on heap", you would have to do the same freeing as part of the hand-rolled
recursion. It is unclear if, where and how the patch takes care of that
in the above.

Other than that, I find the description very readable.

Thanks.

^ permalink raw reply

* Re: [PATCH/RFC] Restore line limit option in post-receive-email
From: Cheng Leong @ 2012-01-12 21:01 UTC (permalink / raw)
  To: git
In-Reply-To: <CACor6wGkDPSkA_G117YtnsQA7H_cROKKW69m-jW_rb2SrVWo1w@mail.gmail.com>

> Would you like me to reroll a patch with both or is this a trivial fixup?

Bump. What do I need to do to contribute this patch?

Thanks,
Cheng

^ permalink raw reply

* Re: SVN -> Git *but* with special changes
From: Abscissa @ 2012-01-12 21:52 UTC (permalink / raw)
  To: git
In-Reply-To: <1326065910362-7166084.post@n2.nabble.com>

I'm trying to do it manually with git-svn, but I'm getting the same problem.
I have two different SVN repos I want to do (turns out neither have
branches, I had thought they did, but one of them does have tags). Doing
this:

git svn clone {the url to the SVN repo} --prefix=svn/ --preserve-empty-dirs
--authors-file={already prepared authors file} --trunk=trunk --tags=tags

It gets partway through, and then gives me:

Failed to strip path '{some path}/.gitignore' ((?-xism:^trunk(/|$)))

Where {some path} is an empty dir in the trunk of one repo, and a completely
non-existent path in the other. (In both cases I'm looking at the revision
git-svn had gotten to when bailing, plus the revision before and the
revision after).

I don't have a clue what that error message is trying to tell me or what is
going wrong. :(

If it helps, these are the repos I'm trying to convert:

http://svn.dsource.org/projects/semitwist   (git svn clone fails at r46 (out
of 242) on 'src/nonFatalAssertTest/.gitignore')
http://svn.dsource.org/projects/goldie   (git svn clone fails at r85 (out of
557) on 'bin/lang/.gitignore')

(Yes, they do have a non-standard top-level "downloads" directory, but
that's just how that host does file downloads, and I *don't* need to
preserve it)


--
View this message in context: http://git.661346.n2.nabble.com/SVN-Git-but-with-special-changes-tp6840904p7181897.html
Sent from the git mailing list archive at Nabble.com.

^ permalink raw reply

* thin packs ending up fat
From: Jeff King @ 2012-01-12 22:15 UTC (permalink / raw)
  To: git; +Cc: Nicolas Pitre

[This ended up long; the gist of it is that we often fail to find thin
 deltas, making small fetches much bigger than they could be. Read on
 for details].

Recently I was doing some work related to bundles containing thin packs,
and I wanted to generate some thin packs. Much to my surprise, all of my
packs ended up having no thin objects!

It turns out that when packing a subset of a fully packed repo (as we do
for a bundle or for a fetch), we tend not to make thin packs at all.
The culprit is this logic in try_delta:

        /*
         * We do not bother to try a delta that we discarded
         * on an earlier try, but only when reusing delta data.
         */
        if (reuse_delta && trg_entry->in_pack &&
            trg_entry->in_pack == src_entry->in_pack &&
            trg_entry->in_pack_type != OBJ_REF_DELTA &&
            trg_entry->in_pack_type != OBJ_OFS_DELTA)
                return 0;

This does not try to delta objects which are found in the same pack,
under the assumption that we would have tried them last time and found
that they didn't work. For a complete repack, it works well. We don't
miss useful deltas, and it saves a lot of CPU time.

For a thin pack, though, it means we'll typically fail to actually use
the boundary objects. I think what happens is this:

  1. You start at commit C which contains a file with content A. You
     build commit C', which contains content A'.

  2. You repack the repository, putting A and A' in the pack. Our delta
     rules tend to create deltas in reverse-recency order, so A
     typically is stored as a delta based on A', and A' is stored
     whole.

  3. You try to create a thin pack (e.g., with "git bundle create
     foo.bundle C..C'"). We know that we want to send A', and that we
     can assume the other side has A to use as a base in our thin pack.

     But the logic above says "A and A' are both in the same pack, and
     we didn't find a delta for A'. They must not be a good match". But
     in this case it is because our deltas go in the reverse direction,
     and we never tried them.

So I did a little experimenting (scripts and inputs are at the end of
the email). I generated a thin pack for a subset of objects from my
fully packed repo, measuring the size of the resulting pack and the CPU
time used. I did this both with stock git, and a version of git with the
above optimization removed.

I used two datasets to generate the input to pack-objects. In the first,
I used the reflog entries from my refs/remotes/origin/master ref. That
simulates the workload of upload-packs performed for me by kernel.org
over the last month or so. In the second dataset, I took packed the
objects between git v1.0.0 and v1.1.0, between v1.1.0 and v1.2.0, and so
on. This was an attempt to simulate somebody who fetches much less
frequently, or perhaps somebody who generates bundles to sneaker-net
individual versions.

For each dataset, I calculated the average size (in bytes) over all of
the generated packs in the dataset and the average CPU time used per
pack (in seconds).

                  dataset
            | fetches | tags
---------------------------------
     before | 53358   | 2750977
size  after | 32315   | 2652939
     change |   -39%  |      -4%
---------------------------------
     before |  0.18   | 1.12
CPU   after |  0.19   | 1.50
     change |    +5%  |     +33%

For the case of small, frequent packs, we saved a lot of space for a
little bit of extra CPU. But for larger packs, our savings were much
more limited, and we spent a lot more CPU time.  That makes sense; this
is really beneficial for finding extra matches at the boundaries, and
for cases where you can avoid sending an entire undeltified object at
all for a given path.

So I think this is worth pursuing as an optimization to save bandwidth
on transfers, but it would require figuring out the right times to turn
it on. My initial idea was that we would want it off whenever we were
doing a thin pack. But the "tags" dataset shows that it's probably not
worth the trouble for large packs due to the extra CPU time.

I have a feeling that the case it is generally catching is this "reverse
delta" case. I.e., A is based off of B, we are including B, but A is
available as a thin base. But that can probably be extended into a delta
chain (A is based from B which is based on C; we want to include B and
C, but can build off of A as a base. We'll keep the delta from B to C,
but we would want to delta C off of A). So detecting that they are part
of the same chain might require walking the chains.

Maybe it is enough to simply turn off this optimization if the potential
delta source is not being included in the pack (i.e., we are using
--thin and it is a boundary object). Because if both objects are being
sent, we will just end up reusing the delta that goes in the reverse
direction anyway.

Or maybe there is some other clever way of detecting this situation. I'm
open to suggestions.

-Peff

---
This is the test script I used (feed the dataset files on stdin, get
lines of sizes and CPU times on stdout):

-- >8 --
#!/bin/sh

while read revs; do
	for i in $revs; do
		echo $i
	done >input
	echo >&2 "==> $revs"
	time -o time.out -f "%e" \
		git pack-objects --thin --revs --stdout <input >pack.out
	echo "`stat --format=%s pack.out` `cat time.out`"
done
-- 8< --

and the "fetch" dataset:

-- >8 --
8b0e15fa95e11965f18c8d2585dc8ffd9bfc9356 ^7f41b6bbe3181dc4d1687db036bf22316997a1bf
34c4461ae3b353e8c931565d5527b98ed12e3735 ^8b0e15fa95e11965f18c8d2585dc8ffd9bfc9356
463b0ea22b5b9a882e8140d0308433d8cbd0d1fe ^34c4461ae3b353e8c931565d5527b98ed12e3735
288396994f077eec7e7db0017838a5afbfbf81e3 ^463b0ea22b5b9a882e8140d0308433d8cbd0d1fe
05f6edcd2a418a88eeb953d51408a6aeef312f5c ^288396994f077eec7e7db0017838a5afbfbf81e3
08cfdbb88cd6225b4fc4b8a3cecd0e01758c835d ^05f6edcd2a418a88eeb953d51408a6aeef312f5c
87009edcbd0b4987ccb7ba050a1efe368a315753 ^08cfdbb88cd6225b4fc4b8a3cecd0e01758c835d
8963314c77af9a4eda5dcbdbab3d4001af83ad81 ^87009edcbd0b4987ccb7ba050a1efe368a315753
10b2a48113b8ab6b8f48229eb40fc3637ce025ae ^8963314c77af9a4eda5dcbdbab3d4001af83ad81
997a1946a55cafb992c4ba8e5e0795aa73f5a4a9 ^10b2a48113b8ab6b8f48229eb40fc3637ce025ae
e8e1c29021da446d0c50573ef9619bf74f515c20 ^997a1946a55cafb992c4ba8e5e0795aa73f5a4a9
87bf9a7048c623b3567f612ca3e67a0d412fc83d ^e8e1c29021da446d0c50573ef9619bf74f515c20
ee6dfb2d83ba1b057943e705f707fa27e34e47f9 ^87bf9a7048c623b3567f612ca3e67a0d412fc83d
4cb6764227173a6483edbdad09121651bc0b01c3 ^ee6dfb2d83ba1b057943e705f707fa27e34e47f9
8a042478967679b0dab3137f5f18367a0ffdc48a ^4cb6764227173a6483edbdad09121651bc0b01c3
248dbbe83256202f0edd6e1468d01cfbe27fd733 ^8a042478967679b0dab3137f5f18367a0ffdc48a
bc1bbe0c19a6ff39522b4fa3259f34150e308e1f ^248dbbe83256202f0edd6e1468d01cfbe27fd733
4d2440fe0daa9ad1556dfd220af8b3a883cf849d ^bc1bbe0c19a6ff39522b4fa3259f34150e308e1f
f56ef114eeefee755f422e6cbef2d83974cb90f1 ^4d2440fe0daa9ad1556dfd220af8b3a883cf849d
e14d63198867c545d0662afc00bf7be048bf2231 ^f56ef114eeefee755f422e6cbef2d83974cb90f1
017d1e134545db0d162908f3538077eaa1f34fb6 ^e14d63198867c545d0662afc00bf7be048bf2231
fc14b89a7e6899b8ac3b5751ec2d8c98203ea4c2 ^017d1e134545db0d162908f3538077eaa1f34fb6
406da7803217998ff6bf5dc69c55b1613556c2f4 ^fc14b89a7e6899b8ac3b5751ec2d8c98203ea4c2
7e02a6c63a183270b726bb21640059ae16fa48ae ^406da7803217998ff6bf5dc69c55b1613556c2f4
4cb5d10b14dcbe0155bed9c45ccb94e83bd4c599 ^7e02a6c63a183270b726bb21640059ae16fa48ae
9859a023fef30ffebdd22ad9639c587ac720b8b6 ^4cb5d10b14dcbe0155bed9c45ccb94e83bd4c599
57526fde5df201a99afa6d122c3266b3a1c5673a ^9859a023fef30ffebdd22ad9639c587ac720b8b6
73c6b3575bc638b7096ec913bd91193707e2265d ^57526fde5df201a99afa6d122c3266b3a1c5673a
10f4eb652ee4e592f91f638e579d1afcb96c0408 ^73c6b3575bc638b7096ec913bd91193707e2265d
ee228024933069b93ce23a1bd5eeb7ae12c792f2 ^10f4eb652ee4e592f91f638e579d1afcb96c0408
d16520499d2652b5b59dfb25f9cf2d56a4c6913a ^ee228024933069b93ce23a1bd5eeb7ae12c792f2
876a6f4991abdd72ea707b193b4f2b831096ad3c ^d16520499d2652b5b59dfb25f9cf2d56a4c6913a
8d68493f20db71abeb77adc251b2a116fe45cdaa ^876a6f4991abdd72ea707b193b4f2b831096ad3c
3daff7c31998faedbe0dd7e2b8651e351be40d64 ^8d68493f20db71abeb77adc251b2a116fe45cdaa
e443bdfe1e8e1ef8b3665cfd1c1295bd73e13773 ^3daff7c31998faedbe0dd7e2b8651e351be40d64
5d6dfc7cb140a6eb90138334fab2245b69bc8bc4 ^e443bdfe1e8e1ef8b3665cfd1c1295bd73e13773
ec330158ec04849fe5ff2cb8749797cd63ae592b ^5d6dfc7cb140a6eb90138334fab2245b69bc8bc4
17b4e93d5b849293e6a3659bbc4075ed8a6e97e2 ^ec330158ec04849fe5ff2cb8749797cd63ae592b
4570aeb0d85f3b5ff274b6d5a651c2ee06d25d76 ^17b4e93d5b849293e6a3659bbc4075ed8a6e97e2
247f9d23da8cfd255533433ad2aa07d172afac0b ^4570aeb0d85f3b5ff274b6d5a651c2ee06d25d76
eac2d83247ea0a265d923518c26873bb12c33778 ^247f9d23da8cfd255533433ad2aa07d172afac0b
beecc7ab65b31c5471331e64acaa3f722125ea67 ^eac2d83247ea0a265d923518c26873bb12c33778
7e521640c80b4bb871bca7a9259621a7abb303e7 ^beecc7ab65b31c5471331e64acaa3f722125ea67
0e1cfc52de002e2d9b0e6562e8672fee3bf45a67 ^7e521640c80b4bb871bca7a9259621a7abb303e7
-- 8< --

and the "tags" dataset:

-- >8 --
v1.1.0 ^v1.0.0
v1.2.0 ^v1.1.0
v1.3.0 ^v1.2.0
v1.4.0 ^v1.3.0
v1.5.0 ^v1.4.0
v1.6.0 ^v1.5.0
v1.7.0 ^v1.6.0
-- 8< --

^ permalink raw reply

* Re: thin packs ending up fat
From: Jeff King @ 2012-01-12 22:32 UTC (permalink / raw)
  To: git; +Cc: Nicolas Pitre
In-Reply-To: <20120112221523.GA3663@sigill.intra.peff.net>

On Thu, Jan 12, 2012 at 05:15:23PM -0500, Jeff King wrote:

> It turns out that when packing a subset of a fully packed repo (as we do
> for a bundle or for a fetch), we tend not to make thin packs at all.
> The culprit is this logic in try_delta:
> 
>         /*
>          * We do not bother to try a delta that we discarded
>          * on an earlier try, but only when reusing delta data.
>          */
>         if (reuse_delta && trg_entry->in_pack &&
>             trg_entry->in_pack == src_entry->in_pack &&
>             trg_entry->in_pack_type != OBJ_REF_DELTA &&
>             trg_entry->in_pack_type != OBJ_OFS_DELTA)
>                 return 0;
> [...]
> Maybe it is enough to simply turn off this optimization if the potential
> delta source is not being included in the pack (i.e., we are using
> --thin and it is a boundary object). Because if both objects are being
> sent, we will just end up reusing the delta that goes in the reverse
> direction anyway.

Hmm. It turns out this is really easy, because we have already marked
such objects as preferred bases.

So with this patch:

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 96c1680..d05e228 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1439,6 +1439,7 @@ static int try_delta(struct unpacked *trg, struct unpacked *src,
 	 */
 	if (reuse_delta && trg_entry->in_pack &&
 	    trg_entry->in_pack == src_entry->in_pack &&
+	    !src_entry->preferred_base &&
 	    trg_entry->in_pack_type != OBJ_REF_DELTA &&
 	    trg_entry->in_pack_type != OBJ_OFS_DELTA)
 		return 0;

here are the numbers I get:

                  dataset
            | fetches | tags
---------------------------------
     before | 53358   | 2750977
size  after | 32398   | 2668479
     change |   -39%  |      -3%
---------------------------------
     before |  0.18   | 1.12
CPU   after |  0.18   | 1.15
     change |    +0%  |      +3%

So nearly all of the size benefit, but very little CPU change (even the
3% on the larger-pack case is close to the levels of run-to-run noise).
Obviously the size benefit in the larger-pack case isn't impressive, but
I think the "fetches" case is much more indicative of a real server
load.

-Peff

^ permalink raw reply related

* Re: thin packs ending up fat
From: Nicolas Pitre @ 2012-01-12 23:54 UTC (permalink / raw)
  To: Jeff King; +Cc: git
In-Reply-To: <20120112223234.GA4949@sigill.intra.peff.net>

On Thu, 12 Jan 2012, Jeff King wrote:

> On Thu, Jan 12, 2012 at 05:15:23PM -0500, Jeff King wrote:
> 
> > It turns out that when packing a subset of a fully packed repo (as we do
> > for a bundle or for a fetch), we tend not to make thin packs at all.
> > The culprit is this logic in try_delta:
> > 
> >         /*
> >          * We do not bother to try a delta that we discarded
> >          * on an earlier try, but only when reusing delta data.
> >          */
> >         if (reuse_delta && trg_entry->in_pack &&
> >             trg_entry->in_pack == src_entry->in_pack &&
> >             trg_entry->in_pack_type != OBJ_REF_DELTA &&
> >             trg_entry->in_pack_type != OBJ_OFS_DELTA)
> >                 return 0;
> > [...]
> > Maybe it is enough to simply turn off this optimization if the potential
> > delta source is not being included in the pack (i.e., we are using
> > --thin and it is a boundary object). Because if both objects are being
> > sent, we will just end up reusing the delta that goes in the reverse
> > direction anyway.
> 
> Hmm. It turns out this is really easy, because we have already marked
> such objects as preferred bases.

That's exactly what I was about to suggest after reading your first 
email.

> So with this patch:
> 
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 96c1680..d05e228 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -1439,6 +1439,7 @@ static int try_delta(struct unpacked *trg, struct unpacked *src,
>  	 */
>  	if (reuse_delta && trg_entry->in_pack &&
>  	    trg_entry->in_pack == src_entry->in_pack &&
> +	    !src_entry->preferred_base &&
>  	    trg_entry->in_pack_type != OBJ_REF_DELTA &&
>  	    trg_entry->in_pack_type != OBJ_OFS_DELTA)
>  		return 0;

Acked-by: Nicolas Pitre <nico@fluxnic.net>

> here are the numbers I get:
> 
>                   dataset
>             | fetches | tags
> ---------------------------------
>      before | 53358   | 2750977
> size  after | 32398   | 2668479
>      change |   -39%  |      -3%
> ---------------------------------
>      before |  0.18   | 1.12
> CPU   after |  0.18   | 1.15
>      change |    +0%  |      +3%
> 
> So nearly all of the size benefit, but very little CPU change (even the
> 3% on the larger-pack case is close to the levels of run-to-run noise).
> Obviously the size benefit in the larger-pack case isn't impressive, but
> I think the "fetches" case is much more indicative of a real server
> load.

Indeed.  Please make sure to capture those numbers in the commit log.


Nicolas

^ permalink raw reply

* Re: thin packs ending up fat
From: Junio C Hamano @ 2012-01-13  0:14 UTC (permalink / raw)
  To: Jeff King; +Cc: git, Nicolas Pitre
In-Reply-To: <20120112223234.GA4949@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

> Hmm. It turns out this is really easy, because we have already marked
> such objects as preferred bases.
>
> So with this patch:
>
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 96c1680..d05e228 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -1439,6 +1439,7 @@ static int try_delta(struct unpacked *trg, struct unpacked *src,
>  	 */
>  	if (reuse_delta && trg_entry->in_pack &&
>  	    trg_entry->in_pack == src_entry->in_pack &&
> +	    !src_entry->preferred_base &&
>  	    trg_entry->in_pack_type != OBJ_REF_DELTA &&
>  	    trg_entry->in_pack_type != OBJ_OFS_DELTA)
>  		return 0;

Yeah, I was wondering why this one-liner was not in the original message
;-)

> here are the numbers I get:
>
>                   dataset
>             | fetches | tags
> ---------------------------------
>      before | 53358   | 2750977
> size  after | 32398   | 2668479
>      change |   -39%  |      -3%
> ---------------------------------
>      before |  0.18   | 1.12
> CPU   after |  0.18   | 1.15
>      change |    +0%  |      +3%

Looks good.

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox