* Re: [PATCH 0/6] archive: refactor and cleanup
From: Lea Wiemann @ 2008-07-15 0:27 UTC (permalink / raw)
To: Lea Wiemann; +Cc: René Scharfe, Git Mailing List
In-Reply-To: <487BE440.9010006@gmail.com>
Lea Wiemann wrote:
> * Unless you have a specific reason, I suggest that you don't Cc Junio
> on patches; he reads all messages on this list (more or less) and will
> usually simply apply your patches once they're reviewed. (Watch for his
> periodical "What's cooking in git.git" messages to see if he got them.)
Uh, I just noticed that you have enough commits in git.git that you
probably didn't need this lecture. ;-) Sorry, Rene. (Thanks Dscho for
the pointer.)
^ permalink raw reply
* Re: [PATCH] t9600: allow testing with cvsps 2.2, including beta versions
From: Junio C Hamano @ 2008-07-15 0:31 UTC (permalink / raw)
To: Pavel Roskin; +Cc: git
In-Reply-To: <20080715002019.5337.81984.stgit@dv.roinet.com>
Pavel Roskin <proski@gnu.org> writes:
> Don't assume that unsupported versions are too old, they may be too new.
Excuse me, but we never assumed such.
Neither too old nor too new ones are proven to work with us yet, so we
cannot be call them supported.
Let's apply your patch and see how well version 2.2 fares.
Thanks.
> diff --git a/t/t9600-cvsimport.sh b/t/t9600-cvsimport.sh
> index 655f882..f92b47a 100755
> --- a/t/t9600-cvsimport.sh
> +++ b/t/t9600-cvsimport.sh
> @@ -20,13 +20,15 @@ cvsps_version=`cvsps -h 2>&1 | sed -ne 's/cvsps version //p'`
> case "$cvsps_version" in
> 2.1)
> ;;
> +2.2*)
> + ;;
Hmm, I would have written:
case "$..." in
2.1 | 2.2 )
Is the reason why you have the asterisk there because the beta one claims
"2.2beta" or something (I am not suggesting to tighten the match, just
asking for information)?
^ permalink raw reply
* Re: [PATCH] t9600: allow testing with cvsps 2.2, including beta versions
From: Pavel Roskin @ 2008-07-15 0:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v1w1wdm37.fsf@gitster.siamese.dyndns.org>
Quoting Junio C Hamano <gitster@pobox.com>:
> Pavel Roskin <proski@gnu.org> writes:
>
>> Don't assume that unsupported versions are too old, they may be too new.
>
> Excuse me, but we never assumed such.
>
> Neither too old nor too new ones are proven to work with us yet, so we
> cannot be call them supported.
The message was "skipping cvsimport tests, cvsps too old" so the
assumption was there.
> Let's apply your patch and see how well version 2.2 fares.
>
> Thanks.
>
>> diff --git a/t/t9600-cvsimport.sh b/t/t9600-cvsimport.sh
>> index 655f882..f92b47a 100755
>> --- a/t/t9600-cvsimport.sh
>> +++ b/t/t9600-cvsimport.sh
>> @@ -20,13 +20,15 @@ cvsps_version=`cvsps -h 2>&1 | sed -ne 's/cvsps
>> version //p'`
>> case "$cvsps_version" in
>> 2.1)
>> ;;
>> +2.2*)
>> + ;;
>
> Hmm, I would have written:
>
> case "$..." in
> 2.1 | 2.2 )
Fine with me, but please allow for extra specifiers.
> Is the reason why you have the asterisk there because the beta one claims
> "2.2beta" or something (I am not suggesting to tighten the match, just
> asking for information)?
It's cvsps 2.2b1 from Fedora Development (cvsps-2.2-0.1.b1.fc10.x86_64)
$ cvsps -h 2>&1 | grep version
cvsps version 2.2b1
--
Regards,
Pavel Roskin
^ permalink raw reply
* Re: [PATCH 3/3] gitweb: use new Git::Repo API, and add optional caching
From: Jakub Narebski @ 2008-07-15 0:52 UTC (permalink / raw)
To: Lea Wiemann; +Cc: git, John Hawley, Petr Baudis
In-Reply-To: <487BE7C4.2050207@gmail.com>
On Tue, 15 July 2008, Lea Wiemann wrote:
> Jakub Narebski wrote:
> >
> > Could you explain then how gitweb cache is invalidated?
>
> Sure; from gitweb.perl:
I'll try to read and comment on patch itself soon.
> # Transient cache entries (like get_sha1('HEAD')) are automatically
> # invalidated when an mtime of either the repository's root directory
> # or of the refs directory or any subdirectory changes. This
> # mechanism *should* detect changes to the repository reliably if you
> # only use git or rsync to write to it,
Nice idea... for project pages. I'm not so sure about projects_list
page, if wouldn't be better to have expire time for *this* page. You
would have/have to stat a lot of files/directories to detect changes.
BTW. some summary of the above should be IMHO in the commit message.
In short, you should write, I think, that caching mechanism uses
any Cache::Cache compatible cache for caching data, that large objects
can be cached on filesystem (or perhaps not, at it is a detail), that
cache validity is checked by stat-ing refs area.
> IOW, gitweb will do a small number of (inexpensive) stat calls on those
> directories each time it's called, and use the most recent mtime as part
> of the cache key for transient entries. Hence those transient entries
> will automatically become invalid once the most recent mtime changes.
>
> (If any of the relevant directories has been modified since the last
> time gitweb checked, gitweb will re-scan the whole tree to check for new
> directories, and record their mtimes as well. See get_last_modification
> if you're interested in more gory details.)
>
> The punchline is, the cache never returns outdated data.
In this case you don't have any basis to set expires for transient views;
on the other hand situation doesn't differ much from serving static
files (wrt. cache validation) so perhaps no expires but no "no-cache"
would be a good solution.
P.S. is there any cache evision mechanism (to limit cache size)
in gitweb cache, or just those implemented by caching backend?
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: [PATCH 3/3] gitweb: use new Git::Repo API, and add optional caching
From: Lea Wiemann @ 2008-07-15 1:16 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git, John Hawley, Petr Baudis
In-Reply-To: <200807150252.52604.jnareb@gmail.com>
Jakub Narebski wrote:
> On Tue, 15 July 2008, Lea Wiemann wrote:
>> # Transient cache entries (like get_sha1('HEAD')) are automatically
>> # invalidated when an mtime [...] changes.
>
> Nice idea... for project pages. I'm not so sure about projects_list
> page, if wouldn't be better to have expire time for *this* page. You
> would have/have to stat a lot of files/directories to detect changes.
It doesn't seem to be too much of a performance issue (it takes ~500ms
to generate the project list on kernel.org), and project lists aren't
requested often enough to be a good optimization target.
I'll see how it performs when the OS's page cache is cold though; if
it's too slow then I might revisit the issue.
> BTW. some summary of the above should be IMHO in the commit message.
*nods*
> In this case you don't have any basis to set expires for transient views;
> on the other hand situation doesn't differ much from serving static
> files (wrt. cache validation) so perhaps no expires but no "no-cache"
> would be a good solution.
Gitweb provides a view on a live repository, so I don't think caching is
usually what you want (e.g. static pages really tend to change much less
frequently). (And yes, my Opera *does* seem to cache those pages unless
you add no-cache.)
Really, I don't think the performance penalty of revalidation will be
significant enough to justify risking stale data. IOW, it's pretty
fast. Check out repo.or.cz's performance (and repo.or.cz doesn't even
use Last-Modified).
> P.S. is there any cache evision mechanism (to limit cache size)
> in gitweb cache, or just those implemented by caching backend?
For the $cache, it's in the caching backend, for the $large_cache, there
is no mechanism, so you need to clean up yourself. See the patch. ;-)
^ permalink raw reply
* Re: [PATCH 3/3] gitweb: use new Git::Repo API, and add optional caching
From: Johannes Schindelin @ 2008-07-15 1:28 UTC (permalink / raw)
To: Lea Wiemann; +Cc: Jakub Narebski, git, John Hawley, Petr Baudis
In-Reply-To: <487BFA67.3020304@gmail.com>
Hi,
On Tue, 15 Jul 2008, Lea Wiemann wrote:
> Jakub Narebski wrote:
> > On Tue, 15 July 2008, Lea Wiemann wrote:
> >> # Transient cache entries (like get_sha1('HEAD')) are automatically
> >> # invalidated when an mtime [...] changes.
> >
> > Nice idea... for project pages. I'm not so sure about projects_list
> > page, if wouldn't be better to have expire time for *this* page. You
> > would have/have to stat a lot of files/directories to detect changes.
>
> It doesn't seem to be too much of a performance issue (it takes ~500ms
> to generate the project list on kernel.org), and project lists aren't
> requested often enough to be a good optimization target.
Wasn't the main page (i.e. the projects list) the reason why kernel.org
has its own little caching mechanism in the first place?
And did Pasky not report recently that repo.or.cz got substantially less
loaded with some caching of its own?
warthog? Pasky?
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH 3/3] gitweb: use new Git::Repo API, and add optional caching
From: J.H. @ 2008-07-15 1:44 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Lea Wiemann, Jakub Narebski, git, Petr Baudis
In-Reply-To: <alpine.LSU.1.00.0807150326050.3486@wbgn129.biozentrum.uni-wuerzburg.de>
On Tue, 2008-07-15 at 03:28 +0200, Johannes Schindelin wrote:
> Hi,
>
> On Tue, 15 Jul 2008, Lea Wiemann wrote:
>
> > Jakub Narebski wrote:
> > > On Tue, 15 July 2008, Lea Wiemann wrote:
> > >> # Transient cache entries (like get_sha1('HEAD')) are automatically
> > >> # invalidated when an mtime [...] changes.
> > >
> > > Nice idea... for project pages. I'm not so sure about projects_list
> > > page, if wouldn't be better to have expire time for *this* page. You
> > > would have/have to stat a lot of files/directories to detect changes.
> >
> > It doesn't seem to be too much of a performance issue (it takes ~500ms
> > to generate the project list on kernel.org), and project lists aren't
> > requested often enough to be a good optimization target.
>
> Wasn't the main page (i.e. the projects list) the reason why kernel.org
> has its own little caching mechanism in the first place?
>
Partially - it was by *far* the biggest problem we were facing, and it
likely has gotten a lot better since I went on a rampage and did
gitweb-caching.
That said there's a relatively small sample-set of files that gets hit
more than others it seems (from stuff I was looking at a while ago) and
limiting the general disk i/o on the kernel.org machines is *always* a
good thing (the less we go to disk the better) so while it started that
way it has been very beneficial, if it does chew up something like
15-20G of disk on those machines.)
- John 'Warthog9' Hawley
> And did Pasky not report recently that repo.or.cz got substantially less
> loaded with some caching of its own?
>
> warthog? Pasky?
>
> Ciao,
> Dscho
^ permalink raw reply
* [PATCH 1/2] restore legacy behavior for read_sha1_file()
From: Nicolas Pitre @ 2008-07-15 1:46 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <alpine.LFD.1.10.0807141937180.12484@xanadu.home>
Since commit 8eca0b47ff1598a6d163df9358c0e0c9bd92d4c8, it is possible
for read_sha1_file() to return NULL even with existing objects when they
are corrupted. Previously a corrupted object would have terminated the
program immediately, effectively making read_sha1_file() return NULL
only when specified object is not found.
Let's restore this behavior for all users of read_sha1_file() and
provide a separate function with the ability to not terminate when
bad objects are encountered.
Signed-off-by: Nicolas Pitre <nico@cam.org>
---
On Mon, 14 Jul 2008, Nicolas Pitre wrote:
> Well, I have a different solution which should restore the original
> "behavior" in the presence of existing but non-readable objects. Patch
> will follow later.
So here it is.
diff --git a/sha1_file.c b/sha1_file.c
index 2df78b5..e281c14 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1006,6 +1006,18 @@ static void mark_bad_packed_object(struct packed_git *p,
p->num_bad_objects++;
}
+static int has_packed_and_bad(const unsigned char *sha1)
+{
+ struct packed_git *p;
+ unsigned i;
+
+ for (p = packed_git; p; p = p->next)
+ for (i = 0; i < p->num_bad_objects; i++)
+ if (!hashcmp(sha1, p->bad_object_sha1 + 20 * i))
+ return 1;
+ return 0;
+}
+
int check_sha1_signature(const unsigned char *sha1, void *map, unsigned long size, const char *type)
{
unsigned char real_sha1[20];
@@ -1647,7 +1659,7 @@ static void *unpack_delta_entry(struct packed_git *p,
sha1_to_hex(base_sha1), (uintmax_t)base_offset,
p->pack_name);
mark_bad_packed_object(p, base_sha1);
- base = read_sha1_file(base_sha1, type, &base_size);
+ base = read_object(base_sha1, type, &base_size);
if (!base)
return NULL;
}
@@ -1945,7 +1957,7 @@ static void *read_packed_sha1(const unsigned char *sha1,
error("failed to read object %s at offset %"PRIuMAX" from %s",
sha1_to_hex(sha1), (uintmax_t)e.offset, e.p->pack_name);
mark_bad_packed_object(e.p, sha1);
- data = read_sha1_file(sha1, type, size);
+ data = read_object(sha1, type, size);
}
return data;
}
@@ -2010,8 +2022,8 @@ int pretend_sha1_file(void *buf, unsigned long len, enum object_type type,
return 0;
}
-void *read_sha1_file(const unsigned char *sha1, enum object_type *type,
- unsigned long *size)
+void *read_object(const unsigned char *sha1, enum object_type *type,
+ unsigned long *size)
{
unsigned long mapsize;
void *map, *buf;
@@ -2037,6 +2049,16 @@ void *read_sha1_file(const unsigned char *sha1, enum object_type *type,
return read_packed_sha1(sha1, type, size);
}
+void *read_sha1_file(const unsigned char *sha1, enum object_type *type,
+ unsigned long *size)
+{
+ void *data = read_object(sha1, type, size);
+ /* legacy behavior is to die on corrupted objects */
+ if (!data && (has_loose_object(sha1) || has_packed_and_bad(sha1)))
+ die("object %s is corrupted", sha1_to_hex(sha1));
+ return data;
+}
+
void *read_object_with_reference(const unsigned char *sha1,
const char *required_type_name,
unsigned long *size,
^ permalink raw reply related
* Re: [PATCH 3/3] gitweb: use new Git::Repo API, and add optional caching
From: Lea Wiemann @ 2008-07-15 1:50 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Jakub Narebski, git, John Hawley, Petr Baudis
In-Reply-To: <alpine.LSU.1.00.0807150326050.3486@wbgn129.biozentrum.uni-wuerzburg.de>
Johannes Schindelin wrote:
> Wasn't the main page (i.e. the projects list) the reason why kernel.org
> has its own little caching mechanism in the first place?
>
> And did Pasky not report recently that repo.or.cz got substantially less
> loaded with some caching of its own?
Yes, you need *some* caching mechanism. Once you have that, it's fine,
at least if the page cache is hot. (The x-thousand stat calls that my
caching implementation issues don't actually take that much time; I
suspect the ~1000 calls to memcached are the more expensive [and
optimizable] part, though I'd have to benchmark that.)
Mainline vs. my caching implementation (both with hot page cache) on
odin3.kernel.org:
$ time wget -qO/dev/null http://localhost/git-lewiemann/vanilla/
real 0m3.070s
$ time wget -qO/dev/null http://localhost/git-lewiemann/
real 0m0.719s
^ permalink raw reply
* [PATCH 2/2] test case for previous commit
From: Nicolas Pitre @ 2008-07-15 1:50 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <alpine.LFD.1.10.0807142134450.12484@xanadu.home>
Here's a test demonstrating a case of what was broken before
(and fixed by) the previous patch.
Signed-off-by: Nicolas Pitre <nico@cam.org>
---
diff --git a/t/t6011-rev-list-with-bad-commit.sh b/t/t6011-rev-list-with-bad-commit.sh
new file mode 100755
index 0000000..e51eb41
--- /dev/null
+++ b/t/t6011-rev-list-with-bad-commit.sh
@@ -0,0 +1,60 @@
+#!/bin/sh
+
+test_description='git rev-list should notice bad commits'
+
+. ./test-lib.sh
+
+# Note:
+# - compression level is set to zero to make "corruptions" easier to perform
+# - reflog is disabled to avoid extra references which would twart the test
+
+test_expect_success 'setup' \
+ '
+ git init &&
+ git config core.compression 0 &&
+ git config core.logallrefupdates false &&
+ echo "foo" > foo &&
+ git add foo &&
+ git commit -m "first commit" &&
+ echo "bar" > bar &&
+ git add bar &&
+ git commit -m "second commit" &&
+ echo "baz" > baz &&
+ git add baz &&
+ git commit -m "third commit" &&
+ echo "foo again" >> foo &&
+ git add foo &&
+ git commit -m "fourth commit" &&
+ git repack -a -f -d
+ '
+
+test_expect_success 'verify number of revisions' \
+ '
+ revs=$(git rev-list --all | wc -l) &&
+ test $revs -eq 4 &&
+ first_commit=$(git rev-parse HEAD~3)
+ '
+
+test_expect_success 'corrupt second commit object' \
+ '
+ perl -i.bak -pe "s/second commit/socond commit/" .git/objects/pack/*.pack &&
+ test_must_fail git fsck --full
+ '
+
+test_expect_success 'rev-list should fail' \
+ '
+ test_must_fail git rev-list --all > /dev/null
+ '
+
+test_expect_success 'git repack _MUST_ fail' \
+ '
+ test_must_fail git repack -a -f -d
+ '
+
+test_expect_success 'first commit is still available' \
+ '
+ git log $first_commit
+ '
+
+test_done
+
^ permalink raw reply related
* Re: [PATCH 3/3] gitweb: use new Git::Repo API, and add optional caching
From: J.H. @ 2008-07-15 2:03 UTC (permalink / raw)
To: Lea Wiemann; +Cc: Johannes Schindelin, Jakub Narebski, git, Petr Baudis
In-Reply-To: <487C0252.4030804@gmail.com>
To continue with that benchmarking it seems if gitweb-caching isn't hot
it takes about 3 seconds, if it is I'm about 7x faster than Lea's ;-)
$ time wget -qO/dev/null http://localhost/git/
real 0m2.952s
user 0m0.001s
sys 0m0.004s
$ time wget -qO/dev/null http://localhost/git/
real 0m0.108s
user 0m0.001s
sys 0m0.001s
But regardless of who's faster - I would still argue there are two
reasons to at least have some caching, even if it's crappy:
1) Prevent the thundering heard problem - many requests for the same
thing all generating the same data is bad, and kills I/O
2) Relatively static data can be generated once and stick around for a
bit and serve more requests more efficiently. Now I agree that
invalidating the cache on a new mtime is better than my current
algorithm (which is purely time based on the cache data vs. the original
data, with some allowances for back-off due to load).
- John 'Warthog9' Hawley
On Tue, 2008-07-15 at 03:50 +0200, Lea Wiemann wrote:
> Johannes Schindelin wrote:
> > Wasn't the main page (i.e. the projects list) the reason why kernel.org
> > has its own little caching mechanism in the first place?
> >
> > And did Pasky not report recently that repo.or.cz got substantially less
> > loaded with some caching of its own?
>
> Yes, you need *some* caching mechanism. Once you have that, it's fine,
> at least if the page cache is hot. (The x-thousand stat calls that my
> caching implementation issues don't actually take that much time; I
> suspect the ~1000 calls to memcached are the more expensive [and
> optimizable] part, though I'd have to benchmark that.)
>
> Mainline vs. my caching implementation (both with hot page cache) on
> odin3.kernel.org:
>
> $ time wget -qO/dev/null http://localhost/git-lewiemann/vanilla/
> real 0m3.070s
> $ time wget -qO/dev/null http://localhost/git-lewiemann/
> real 0m0.719s
^ permalink raw reply
* Re: [PATCH 0/4] Honor core.deltaBaseCacheLimit during index-pack
From: Nicolas Pitre @ 2008-07-15 2:21 UTC (permalink / raw)
To: Andreas Ericsson
Cc: Johannes Schindelin, Jakub Narebski, Shawn O. Pearce,
Junio C Hamano, git, Stephan Hennig
In-Reply-To: <487B4BD8.5030208@op5.se>
On Mon, 14 Jul 2008, Andreas Ericsson wrote:
> Johannes Schindelin wrote:
> > Hi,
> >
> > On Mon, 14 Jul 2008, Andreas Ericsson wrote:
> >
> > > Sorry for being clueless here, but why does the older versions need
> > > to be kept in-memory anyway? Aren't we applying the delta each time
> > > we find one, repeatedly creating a new base-object in-memory for
> > > each delta? If we aren't doing that, why do we need more than just
> > > a small amount of memory just for keeping the delta?
> >
> > Think of a delta chain of 49 delta objects, 1 base object. Now reconstruct
> > all of the objects.
> >
> > If you do it one by one, not storing the intermediate results, you end up
> > applying 1 delta for the first, 2 for the second (first the first, then the
> > second), and so on, in total 1 + 2 + 3 + ... + 49 = 49 * 50 / 2 = 1225
> > times.
> >
> > Compare that to the 49 times when reusing the intermediate results.
> >
>
> That's only true if you discard the result of applying D1 to DB though.
> What I'm wondering is; Why isn't it done like this (pseudo-code):
>
> while (delta = find_earlier_delta(sha1)) {
> if (is_base_object(delta)) {
> base_object = delta;
> break;
> }
> push_delta(delta, patch_stack);
> }
>
> while (pop_delta(patch_stack))
> apply_delta(base_object, delta);
>
>
>
> where "apply_delta" replaces the base_object->blob with the delta
> applied, releasing the previously used memory?
>
> That way, you'll never use more memory than the largest ever size
> of the object + 1 delta at a time and still not apply the delta
> more than delta_chain_length-1 times.
Those delta chains aren't simple chained lists. They are trees of
deltas where one object might be the base for an unlimited number of
deltas of depth 1, and in turn each of those deltas might constitute the
base for an unlimited number of deltas of depth 2, and so on.
So what the code does is to find out which objects are not deltas but
are the base for a delta. Then, for each of them, all deltas having
given object for base are found and they are recursively resolved so
each resolved delta is then considered a possible base for more deltas,
etc. In other words, those deltas are resolved by walking the delta
tree in a "depth first" fashion.
If we discard previous delta bases, we will have to recreate them each
time a delta sibling is processed. And if those delta bases are
themselves deltas then you have an explosion of delta results to
re-compute.
Nicolas
^ permalink raw reply
* Re: [PATCH 1/4] index-pack: Refactor base arguments of resolve_delta into a struct
From: Nicolas Pitre @ 2008-07-15 2:40 UTC (permalink / raw)
To: Shawn O. Pearce
Cc: Junio C Hamano, Junio C Hamano, git, Stephan Hennig,
Andreas Ericsson
In-Reply-To: <1216001267-33235-2-git-send-email-spearce@spearce.org>
On Sun, 13 Jul 2008, Shawn O. Pearce wrote:
> We need to discard base objects which are not recently used if our
> memory gets low, such as when we are unpacking a long delta chain
> of a very large object.
>
> To support tracking the available base objects we combine the
> pointer and size into a struct. Future changes would allow the
> data pointer to be free'd and marked NULL if memory gets low.
>
> Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
ACK
> ---
> index-pack.c | 60 +++++++++++++++++++++++++++++++--------------------------
> 1 files changed, 33 insertions(+), 27 deletions(-)
>
> diff --git a/index-pack.c b/index-pack.c
> index 25db5db..db03478 100644
> --- a/index-pack.c
> +++ b/index-pack.c
> @@ -26,6 +26,11 @@ union delta_base {
> off_t offset;
> };
>
> +struct base_data {
> + void *data;
> + unsigned long size;
> +};
> +
> /*
> * Even if sizeof(union delta_base) == 24 on 64-bit archs, we really want
> * to memcmp() only the first 20 bytes.
> @@ -426,25 +431,25 @@ static void sha1_object(const void *data, unsigned long size,
> }
> }
>
> -static void resolve_delta(struct object_entry *delta_obj, void *base_data,
> - unsigned long base_size, enum object_type type)
> +static void resolve_delta(struct object_entry *delta_obj,
> + struct base_data *base_obj, enum object_type type)
> {
> void *delta_data;
> unsigned long delta_size;
> - void *result;
> - unsigned long result_size;
> union delta_base delta_base;
> int j, first, last;
> + struct base_data result;
>
> delta_obj->real_type = type;
> delta_data = get_data_from_pack(delta_obj);
> delta_size = delta_obj->size;
> - result = patch_delta(base_data, base_size, delta_data, delta_size,
> - &result_size);
> + result.data = patch_delta(base_obj->data, base_obj->size,
> + delta_data, delta_size,
> + &result.size);
> free(delta_data);
> - if (!result)
> + if (!result.data)
> bad_object(delta_obj->idx.offset, "failed to apply delta");
> - sha1_object(result, result_size, type, delta_obj->idx.sha1);
> + sha1_object(result.data, result.size, type, delta_obj->idx.sha1);
> nr_resolved_deltas++;
>
> hashcpy(delta_base.sha1, delta_obj->idx.sha1);
> @@ -452,7 +457,7 @@ static void resolve_delta(struct object_entry *delta_obj, void *base_data,
> for (j = first; j <= last; j++) {
> struct object_entry *child = objects + deltas[j].obj_no;
> if (child->real_type == OBJ_REF_DELTA)
> - resolve_delta(child, result, result_size, type);
> + resolve_delta(child, &result, type);
> }
> }
>
> @@ -462,11 +467,11 @@ static void resolve_delta(struct object_entry *delta_obj, void *base_data,
> for (j = first; j <= last; j++) {
> struct object_entry *child = objects + deltas[j].obj_no;
> if (child->real_type == OBJ_OFS_DELTA)
> - resolve_delta(child, result, result_size, type);
> + resolve_delta(child, &result, type);
> }
> }
>
> - free(result);
> + free(result.data);
> }
>
> static int compare_delta_entry(const void *a, const void *b)
> @@ -481,7 +486,6 @@ static void parse_pack_objects(unsigned char *sha1)
> {
> int i;
> struct delta_entry *delta = deltas;
> - void *data;
> struct stat st;
>
> /*
> @@ -496,7 +500,7 @@ static void parse_pack_objects(unsigned char *sha1)
> nr_objects);
> for (i = 0; i < nr_objects; i++) {
> struct object_entry *obj = &objects[i];
> - data = unpack_raw_entry(obj, &delta->base);
> + void *data = unpack_raw_entry(obj, &delta->base);
> obj->real_type = obj->type;
> if (obj->type == OBJ_REF_DELTA || obj->type == OBJ_OFS_DELTA) {
> nr_deltas++;
> @@ -545,6 +549,7 @@ static void parse_pack_objects(unsigned char *sha1)
> struct object_entry *obj = &objects[i];
> union delta_base base;
> int j, ref, ref_first, ref_last, ofs, ofs_first, ofs_last;
> + struct base_data base_obj;
>
> if (obj->type == OBJ_REF_DELTA || obj->type == OBJ_OFS_DELTA)
> continue;
> @@ -555,22 +560,22 @@ static void parse_pack_objects(unsigned char *sha1)
> ofs = !find_delta_children(&base, &ofs_first, &ofs_last);
> if (!ref && !ofs)
> continue;
> - data = get_data_from_pack(obj);
> + base_obj.data = get_data_from_pack(obj);
> + base_obj.size = obj->size;
> +
> if (ref)
> for (j = ref_first; j <= ref_last; j++) {
> struct object_entry *child = objects + deltas[j].obj_no;
> if (child->real_type == OBJ_REF_DELTA)
> - resolve_delta(child, data,
> - obj->size, obj->type);
> + resolve_delta(child, &base_obj, obj->type);
> }
> if (ofs)
> for (j = ofs_first; j <= ofs_last; j++) {
> struct object_entry *child = objects + deltas[j].obj_no;
> if (child->real_type == OBJ_OFS_DELTA)
> - resolve_delta(child, data,
> - obj->size, obj->type);
> + resolve_delta(child, &base_obj, obj->type);
> }
> - free(data);
> + free(base_obj.data);
> display_progress(progress, nr_resolved_deltas);
> }
> }
> @@ -656,28 +661,29 @@ static void fix_unresolved_deltas(int nr_unresolved)
>
> for (i = 0; i < n; i++) {
> struct delta_entry *d = sorted_by_pos[i];
> - void *data;
> - unsigned long size;
> enum object_type type;
> int j, first, last;
> + struct base_data base_obj;
>
> if (objects[d->obj_no].real_type != OBJ_REF_DELTA)
> continue;
> - data = read_sha1_file(d->base.sha1, &type, &size);
> - if (!data)
> + base_obj.data = read_sha1_file(d->base.sha1, &type, &base_obj.size);
> + if (!base_obj.data)
> continue;
>
> find_delta_children(&d->base, &first, &last);
> for (j = first; j <= last; j++) {
> struct object_entry *child = objects + deltas[j].obj_no;
> if (child->real_type == OBJ_REF_DELTA)
> - resolve_delta(child, data, size, type);
> + resolve_delta(child, &base_obj, type);
> }
>
> - if (check_sha1_signature(d->base.sha1, data, size, typename(type)))
> + if (check_sha1_signature(d->base.sha1, base_obj.data,
> + base_obj.size, typename(type)))
> die("local object %s is corrupt", sha1_to_hex(d->base.sha1));
> - append_obj_to_pack(d->base.sha1, data, size, type);
> - free(data);
> + append_obj_to_pack(d->base.sha1, base_obj.data,
> + base_obj.size, type);
> + free(base_obj.data);
> display_progress(progress, nr_resolved_deltas);
> }
> free(sorted_by_pos);
> --
> 1.5.6.2.393.g45096
>
Nicolas
^ permalink raw reply
* Re: [PATCH 0/4] Honor core.deltaBaseCacheLimit during index-pack
From: Shawn O. Pearce @ 2008-07-15 2:47 UTC (permalink / raw)
To: Nicolas Pitre
Cc: Andreas Ericsson, Johannes Schindelin, Jakub Narebski,
Junio C Hamano, git, Stephan Hennig
In-Reply-To: <alpine.LFD.1.10.0807142203530.12484@xanadu.home>
Nicolas Pitre <nico@cam.org> wrote:
>
> Those delta chains aren't simple chained lists. They are trees of
> deltas where one object might be the base for an unlimited number of
> deltas of depth 1, and in turn each of those deltas might constitute the
> base for an unlimited number of deltas of depth 2, and so on.
>
> So what the code does is to find out which objects are not deltas but
> are the base for a delta. Then, for each of them, all deltas having
> given object for base are found and they are recursively resolved so
> each resolved delta is then considered a possible base for more deltas,
> etc. In other words, those deltas are resolved by walking the delta
> tree in a "depth first" fashion.
>
> If we discard previous delta bases, we will have to recreate them each
> time a delta sibling is processed. And if those delta bases are
> themselves deltas then you have an explosion of delta results to
> re-compute.
Yes, it would be horrible if we had to recompute 10 deltas in order
to recover a previously discarded delta base in order to visit new
siblings.
But its even more horrible that we use 512M of memory in our working
set size on a 256M machine to process a pack that is only 300M in
size, due to long delta chains on large objects. In such a case
the system will swap and perform fairly poorly due to the huge disk
IO necessary to keep moving the working set around.
We're better off keeping our memory usage low and recomputing
the delta base when we need to return to it to process a sibling.
Please. Remember that index-pack, unlike unpack-objects, does not
hold the unresolved deltas in memory while processing the input.
It assumes the total size of the unresolved deltas may exceed
the available memory for our working set and writes them to disk,
to be read back in later during the resolving phase.
At some point it is possible for the completely inflated delta
chain to exceed the physical memory of the system. As soon as you
do that you are committed to some form of swapping. We can probably
do that better from the packfile by reinflating the super compressed
deltas than letting the OS page in huge tracts of the virtual address
space off the swap device. Plus the OS does not need to expend disk
IO to swap out the pages, we have already spent that cost when we
wrote the pack file down to disk as part of our normal operation.
I don't like adding code either. But I think I'm right. We really
need to not allow index-pack to create these massive working sets
and assume the operating system is going to be able to handle
it magically. Memory is not infinite, even if the Turing machine
theory claims it is.
--
Shawn.
^ permalink raw reply
* Re: [PATCH 2/4] index-pack: Chain the struct base_data on the stack for traversal
From: Nicolas Pitre @ 2008-07-15 2:48 UTC (permalink / raw)
To: Shawn O. Pearce
Cc: Junio C Hamano, Junio C Hamano, git, Stephan Hennig,
Andreas Ericsson
In-Reply-To: <1216001267-33235-3-git-send-email-spearce@spearce.org>
On Sun, 13 Jul 2008, Shawn O. Pearce wrote:
> We need to release earlier inflated base objects when memory gets
> low, which means we need to be able to walk up or down the stack
> to locate the objects we want to release, and free their data.
>
> The new link/unlink routines allow inserting and removing the struct
> base_data during recursion inside resolve_delta, and the global
> base_cache gives us the head of the chain (bottom of the stack)
> so we can traverse it.
>
> Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
You don't really need that 'base' pointer, do you? When linking a new
child, then the 'child' pointer simply has to be overwritten. there is
a window where that 'child' pointer would be invalid after the child
structure has been discarded, but no walking of the list should occur at
that point anyway.
> ---
> index-pack.c | 34 +++++++++++++++++++++++++++++++---
> 1 files changed, 31 insertions(+), 3 deletions(-)
>
> diff --git a/index-pack.c b/index-pack.c
> index db03478..6c59fd3 100644
> --- a/index-pack.c
> +++ b/index-pack.c
> @@ -27,6 +27,8 @@ union delta_base {
> };
>
> struct base_data {
> + struct base_data *base;
> + struct base_data *child;
> void *data;
> unsigned long size;
> };
> @@ -48,6 +50,7 @@ struct delta_entry
>
> static struct object_entry *objects;
> static struct delta_entry *deltas;
> +static struct base_data *base_cache;
> static int nr_objects;
> static int nr_deltas;
> static int nr_resolved_deltas;
> @@ -216,6 +219,27 @@ static void bad_object(unsigned long offset, const char *format, ...)
> die("pack has bad object at offset %lu: %s", offset, buf);
> }
>
> +static void link_base_data(struct base_data *base, struct base_data *c)
> +{
> + if (base)
> + base->child = c;
> + else
> + base_cache = c;
> +
> + c->base = base;
> + c->child = NULL;
> +}
> +
> +static void unlink_base_data(struct base_data *c)
> +{
> + struct base_data *base = c->base;
> + if (base)
> + base->child = NULL;
> + else
> + base_cache = NULL;
> + free(c->data);
> +}
> +
> static void *unpack_entry_data(unsigned long offset, unsigned long size)
> {
> z_stream stream;
> @@ -452,6 +476,8 @@ static void resolve_delta(struct object_entry *delta_obj,
> sha1_object(result.data, result.size, type, delta_obj->idx.sha1);
> nr_resolved_deltas++;
>
> + link_base_data(base_obj, &result);
> +
> hashcpy(delta_base.sha1, delta_obj->idx.sha1);
> if (!find_delta_children(&delta_base, &first, &last)) {
> for (j = first; j <= last; j++) {
> @@ -471,7 +497,7 @@ static void resolve_delta(struct object_entry *delta_obj,
> }
> }
>
> - free(result.data);
> + unlink_base_data(&result);
> }
>
> static int compare_delta_entry(const void *a, const void *b)
> @@ -562,6 +588,7 @@ static void parse_pack_objects(unsigned char *sha1)
> continue;
> base_obj.data = get_data_from_pack(obj);
> base_obj.size = obj->size;
> + link_base_data(NULL, &base_obj);
>
> if (ref)
> for (j = ref_first; j <= ref_last; j++) {
> @@ -575,7 +602,7 @@ static void parse_pack_objects(unsigned char *sha1)
> if (child->real_type == OBJ_OFS_DELTA)
> resolve_delta(child, &base_obj, obj->type);
> }
> - free(base_obj.data);
> + unlink_base_data(&base_obj);
> display_progress(progress, nr_resolved_deltas);
> }
> }
> @@ -670,6 +697,7 @@ static void fix_unresolved_deltas(int nr_unresolved)
> base_obj.data = read_sha1_file(d->base.sha1, &type, &base_obj.size);
> if (!base_obj.data)
> continue;
> + link_base_data(NULL, &base_obj);
>
> find_delta_children(&d->base, &first, &last);
> for (j = first; j <= last; j++) {
> @@ -683,7 +711,7 @@ static void fix_unresolved_deltas(int nr_unresolved)
> die("local object %s is corrupt", sha1_to_hex(d->base.sha1));
> append_obj_to_pack(d->base.sha1, base_obj.data,
> base_obj.size, type);
> - free(base_obj.data);
> + unlink_base_data(&base_obj);
> display_progress(progress, nr_resolved_deltas);
> }
> free(sorted_by_pos);
> --
> 1.5.6.2.393.g45096
>
Nicolas
^ permalink raw reply
* Re: [PATCH 3/4] index-pack: Track the object_entry that creates each base_data
From: Nicolas Pitre @ 2008-07-15 2:50 UTC (permalink / raw)
To: Shawn O. Pearce
Cc: Junio C Hamano, Junio C Hamano, git, Stephan Hennig,
Andreas Ericsson
In-Reply-To: <1216001267-33235-4-git-send-email-spearce@spearce.org>
On Sun, 13 Jul 2008, Shawn O. Pearce wrote:
> If we free the data stored within a base_data we need the struct
> object_entry to get the data back again for use with another
> dependent delta. Storing the object_entry* makes it simple to call
> get_data_from_pack() to recover the compressed information.
>
> This however means we must add the missing baes object to the end
Typo?
> of our packfile prior to calling resolve_delta() on each of the
> dependent deltas. Adding the base first ensures we can read the
> base back from the pack we indexing, as if it had been included by
> the remote side.
>
> Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
ACK
> ---
> index-pack.c | 18 ++++++++++++------
> 1 files changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/index-pack.c b/index-pack.c
> index 6c59fd3..7239e89 100644
> --- a/index-pack.c
> +++ b/index-pack.c
> @@ -29,6 +29,7 @@ union delta_base {
> struct base_data {
> struct base_data *base;
> struct base_data *child;
> + struct object_entry *obj;
> void *data;
> unsigned long size;
> };
> @@ -476,6 +477,7 @@ static void resolve_delta(struct object_entry *delta_obj,
> sha1_object(result.data, result.size, type, delta_obj->idx.sha1);
> nr_resolved_deltas++;
>
> + result.obj = delta_obj;
> link_base_data(base_obj, &result);
>
> hashcpy(delta_base.sha1, delta_obj->idx.sha1);
> @@ -588,6 +590,7 @@ static void parse_pack_objects(unsigned char *sha1)
> continue;
> base_obj.data = get_data_from_pack(obj);
> base_obj.size = obj->size;
> + base_obj.obj = obj;
> link_base_data(NULL, &base_obj);
>
> if (ref)
> @@ -633,7 +636,8 @@ static int write_compressed(int fd, void *in, unsigned int size, uint32_t *obj_c
> return size;
> }
>
> -static void append_obj_to_pack(const unsigned char *sha1, void *buf,
> +static struct object_entry *append_obj_to_pack(
> + const unsigned char *sha1, void *buf,
> unsigned long size, enum object_type type)
> {
> struct object_entry *obj = &objects[nr_objects++];
> @@ -654,6 +658,7 @@ static void append_obj_to_pack(const unsigned char *sha1, void *buf,
> obj[1].idx.offset = obj[0].idx.offset + n;
> obj[1].idx.offset += write_compressed(output_fd, buf, size, &obj[0].idx.crc32);
> hashcpy(obj->idx.sha1, sha1);
> + return obj;
> }
>
> static int delta_pos_compare(const void *_a, const void *_b)
> @@ -697,6 +702,12 @@ static void fix_unresolved_deltas(int nr_unresolved)
> base_obj.data = read_sha1_file(d->base.sha1, &type, &base_obj.size);
> if (!base_obj.data)
> continue;
> +
> + if (check_sha1_signature(d->base.sha1, base_obj.data,
> + base_obj.size, typename(type)))
> + die("local object %s is corrupt", sha1_to_hex(d->base.sha1));
> + base_obj.obj = append_obj_to_pack(d->base.sha1, base_obj.data,
> + base_obj.size, type);
> link_base_data(NULL, &base_obj);
>
> find_delta_children(&d->base, &first, &last);
> @@ -706,11 +717,6 @@ static void fix_unresolved_deltas(int nr_unresolved)
> resolve_delta(child, &base_obj, type);
> }
>
> - if (check_sha1_signature(d->base.sha1, base_obj.data,
> - base_obj.size, typename(type)))
> - die("local object %s is corrupt", sha1_to_hex(d->base.sha1));
> - append_obj_to_pack(d->base.sha1, base_obj.data,
> - base_obj.size, type);
> unlink_base_data(&base_obj);
> display_progress(progress, nr_resolved_deltas);
> }
> --
> 1.5.6.2.393.g45096
>
Nicolas
^ permalink raw reply
* Re: Closing the merge window for 1.6.0
From: Shawn O. Pearce @ 2008-07-15 2:51 UTC (permalink / raw)
To: Nicolas Pitre
Cc: Gerrit Pape, Johannes Schindelin, Petr Baudis, Junio C Hamano,
git
In-Reply-To: <alpine.LFD.1.10.0807141351540.12484@xanadu.home>
Nicolas Pitre <nico@cam.org> wrote:
> On Mon, 14 Jul 2008, Gerrit Pape wrote:
>
> > On Mon, Jul 14, 2008 at 12:57:56PM +0100, Johannes Schindelin wrote:
> > > On Mon, 14 Jul 2008, Petr Baudis wrote:
> > > > I'm saying this because I believe the best conservative upper bound for
> > > > backwards compatibility is Git version in Debian stable. It gets
> > > > probably the most stale from all the widely used software distributions
> > > > using Git, and it *is* quite widely used. Etch carries v1.4.4.4, which
> > > > fails miserably on the new packs.
>
> Maybe we can release 1.4.5 with the ability to read index v2? That
> wouldn't be hard to backport the reading part of it.
If we consider that supporting 1.4.4.4 clients is still a priority,
due to the widespread distribution of that version in a popular
version of Debian, we shouldn't be rushing the index v2 or OFS_DELTA
functionality on by default in 1.6.0. Instead we would wait until
Debian stable (and most other widely popular distributions) are on
a modern enough version of Git to understand this format.
Really. As much as I'd love to see the switch to v2 made by default
I don't think we can/should do it unless the majority of the user
base will be able to grok it. And Debian etch sounds like it won't.
--
Shawn.
^ permalink raw reply
* git-mergetool + WinMerge
From: chongyc @ 2008-07-15 2:55 UTC (permalink / raw)
To: git
Hi everybody
I am going to merge conflict by using git-mergetool of msysgit(for
windows) and Winmerge.
what shall i do?
Help me
^ permalink raw reply
* Re: [PATCH 4/4] index-pack: Honor core.deltaBaseCacheLimit when resolving deltas
From: Nicolas Pitre @ 2008-07-15 3:05 UTC (permalink / raw)
To: Shawn O. Pearce
Cc: Junio C Hamano, Junio C Hamano, git, Stephan Hennig,
Andreas Ericsson
In-Reply-To: <1216001267-33235-5-git-send-email-spearce@spearce.org>
On Sun, 13 Jul 2008, Shawn O. Pearce wrote:
> If we are trying to resolve deltas for a long delta chain composed
> of multi-megabyte objects we can easily run into requiring 500M+
> of memory to hold each object in the chain on the call stack while
> we recurse into the dependent objects and resolve them.
>
> We now use a simple delta cache that discards objects near the
> bottom of the call stack first, as they are the most least recently
> used objects in this current delta chain. If we recurse out of a
> chain we may find the base object is no longer available, as it was
> free'd to keep memory under the deltaBaseCacheLimit. In such cases
> we must unpack the base object again, which will require recursing
> back to the root of the top of the delta chain as we released that
> root first.
>
> Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
OK now I see what the 'base' pointer I previously dismissed is really
needed for.
But this patch is suboptimal as it actually recreate the same memory
pressure, to a lesser degree, this series is meant to solve. If you do:
> + struct object_entry *obj = c->obj;
> + void *raw = get_data_from_pack(obj);
> + if (obj->type == OBJ_REF_DELTA || obj->type == OBJ_OFS_DELTA) {
> + c->data = patch_delta(
> + get_base_data(c->base), c->base->size,
> + raw, obj->size,
> + &c->size);
> + free(raw);
What you actually do is to read the delta data in memory, then recurse
down to read more delta data, then recurse down to read the base which
might be yet more delta data in memory, etc. etc. Only when you reach
the bottom of the stack will you start resolving all those deltas in
memory. Instead, the check for a delta object should be done first, and
if so then recursion for the base object be performed _before_ reading
the currently wanted object data. This way you won't have more than one
delta buffer at any time in memory.
Nicolas
^ permalink raw reply
* Re: [PATCH 0/4] Honor core.deltaBaseCacheLimit during index-pack
From: Nicolas Pitre @ 2008-07-15 3:06 UTC (permalink / raw)
To: Shawn O. Pearce
Cc: Andreas Ericsson, Johannes Schindelin, Jakub Narebski,
Junio C Hamano, git, Stephan Hennig
In-Reply-To: <20080715024741.GB1700@spearce.org>
On Tue, 15 Jul 2008, Shawn O. Pearce wrote:
> Nicolas Pitre <nico@cam.org> wrote:
> >
> > Those delta chains aren't simple chained lists. They are trees of
> > deltas where one object might be the base for an unlimited number of
> > deltas of depth 1, and in turn each of those deltas might constitute the
> > base for an unlimited number of deltas of depth 2, and so on.
> >
> > So what the code does is to find out which objects are not deltas but
> > are the base for a delta. Then, for each of them, all deltas having
> > given object for base are found and they are recursively resolved so
> > each resolved delta is then considered a possible base for more deltas,
> > etc. In other words, those deltas are resolved by walking the delta
> > tree in a "depth first" fashion.
> >
> > If we discard previous delta bases, we will have to recreate them each
> > time a delta sibling is processed. And if those delta bases are
> > themselves deltas then you have an explosion of delta results to
> > re-compute.
>
> Yes, it would be horrible if we had to recompute 10 deltas in order
> to recover a previously discarded delta base in order to visit new
> siblings.
>
> But its even more horrible that we use 512M of memory in our working
> set size on a 256M machine to process a pack that is only 300M in
> size, due to long delta chains on large objects. In such a case
> the system will swap and perform fairly poorly due to the huge disk
> IO necessary to keep moving the working set around.
>
> We're better off keeping our memory usage low and recomputing
> the delta base when we need to return to it to process a sibling.
Please relax! ;-)
And have a look in your mbox.
Nicolas
^ permalink raw reply
* Re: Closing the merge window for 1.6.0
From: Martin Langhoff @ 2008-07-15 3:14 UTC (permalink / raw)
To: Teemu Likonen
Cc: Junio C Hamano, Nicolas Pitre, Gerrit Pape, Johannes Schindelin,
Petr Baudis, git
In-Reply-To: <20080714191903.GB5788@mithlond.arda.local>
On Tue, Jul 15, 2008 at 7:19 AM, Teemu Likonen <tlikonen@iki.fi> wrote:
>> But as the upstream, we have our own deprecation schedule.
>
> As Debian stable (4.0 "Etch") and its git 1.4.4.4 was mentioned I'd like
> to point out that git 1.5.6 is available for Etch users from
> kind-of-semi-official <www.backports.org>. So I guess Debian stable
> users aren't left completely behind. Git's web page already advertises
> backports.org version for Etch.
I concur. Users of git on Etch are using backports AFAIK.
We still have the case of the "casual" user, who does not know much
about git, but installs it to clone & review a project's code. There,
if v1.4.4 complains with a useful message, the casual user will swear
a bit and grab a backport. If it dies a horrible uninformative death,
then we will get bogus bug reports, flamage, the works.
cheers,
m
--
martin.langhoff@gmail.com
martin@laptop.org -- School Server Architect
- ask interesting questions
- don't get distracted with shiny stuff - working code first
- http://wiki.laptop.org/go/User:Martinlanghoff
^ permalink raw reply
* Re: [PATCH] git-mailinfo: Fix getting the subject from the body
From: Don Zickus @ 2008-07-15 3:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Lukas Sandström, Git Mailing List
In-Reply-To: <7v3amfxx3a.fsf@gitster.siamese.dyndns.org>
On Sat, Jul 12, 2008 at 02:36:57AM -0700, Junio C Hamano wrote:
> Another thing I noticed and found puzzling is the handling of ">From "
> line that is shown in the context below. check_header() is supposed to
> return true when it handled header (i.e. not part of the commit message)
> and return false when line is not part of the header. As ">From " is part
> of the commit log message, shouldn't it return zero?
>
> Don, this part was what you introduced. Has this codepath ever been
> exercised in the real life?
Heh. Most emails I deal with usually wind up causing the code to stop
looking for header info (still_looking=0). So I never ran into that
scenario. And I never really tried to rely on inbody stuff.
I thought I was mimicing the original code, guess not.
Now that I think about it, I did run into a situation last year where
git-mailinfo parsed the '>From' as an inbody header instead of a commit
msg. I just put a stupid hack in my scripts to work around, thinking it
was my scripts.
Anyway if it returns zero, wouldn't it be better to just remove the check
to begin with? I kinda forgot why it is there in the first place (my
changes just copied it from somewhere else).
Cheers,
Don
^ permalink raw reply
* Re: [PATCH 4/4] index-pack: Honor core.deltaBaseCacheLimit when resolving deltas
From: Shawn O. Pearce @ 2008-07-15 3:18 UTC (permalink / raw)
To: Nicolas Pitre; +Cc: Junio C Hamano, git, Stephan Hennig, Andreas Ericsson
In-Reply-To: <alpine.LFD.1.10.0807142255240.12484@xanadu.home>
Nicolas Pitre <nico@cam.org> wrote:
> On Sun, 13 Jul 2008, Shawn O. Pearce wrote:
>
> But this patch is suboptimal as it actually recreate the same memory
> pressure, to a lesser degree, this series is meant to solve. If you do:
Arrgh. Good catch on review. I'll post a replacement patch shortly
that doesn't suffer from this problem. Thanks!
> > + struct object_entry *obj = c->obj;
> > + void *raw = get_data_from_pack(obj);
> > + if (obj->type == OBJ_REF_DELTA || obj->type == OBJ_OFS_DELTA) {
> > + c->data = patch_delta(
> > + get_base_data(c->base), c->base->size,
> > + raw, obj->size,
> > + &c->size);
> > + free(raw);
--
Shawn.
^ permalink raw reply
* Re: [PATCH 3/4] index-pack: Track the object_entry that creates each base_data
From: Shawn O. Pearce @ 2008-07-15 3:20 UTC (permalink / raw)
To: Nicolas Pitre; +Cc: Junio C Hamano, git, Stephan Hennig, Andreas Ericsson
In-Reply-To: <alpine.LFD.1.10.0807142249100.12484@xanadu.home>
Nicolas Pitre <nico@cam.org> wrote:
> On Sun, 13 Jul 2008, Shawn O. Pearce wrote:
>
> > If we free the data stored within a base_data we need the struct
> > object_entry to get the data back again for use with another
> > dependent delta. Storing the object_entry* makes it simple to call
> > get_data_from_pack() to recover the compressed information.
> >
> > This however means we must add the missing baes object to the end
>
> Typo?
Yea, Dscho also pointed it out.
Junio, if you can, please fix this typo in the commit message.
Its not really a big deal. I have no plans on posting a replacement
patch just for this one small typo. Plenty of my current commits
in git.git have typos too. I'm not that good of a typist. And gcc
doesn't check my commit messages. ;-)
--
Shawn.
^ permalink raw reply
* Re: Closing the merge window for 1.6.0
From: Nicolas Pitre @ 2008-07-15 3:30 UTC (permalink / raw)
To: Shawn O. Pearce
Cc: Gerrit Pape, Johannes Schindelin, Petr Baudis, Junio C Hamano,
git
In-Reply-To: <20080715025127.GC1700@spearce.org>
On Tue, 15 Jul 2008, Shawn O. Pearce wrote:
> Nicolas Pitre <nico@cam.org> wrote:
> > On Mon, 14 Jul 2008, Gerrit Pape wrote:
> >
> > > On Mon, Jul 14, 2008 at 12:57:56PM +0100, Johannes Schindelin wrote:
> > > > On Mon, 14 Jul 2008, Petr Baudis wrote:
> > > > > I'm saying this because I believe the best conservative upper bound for
> > > > > backwards compatibility is Git version in Debian stable. It gets
> > > > > probably the most stale from all the widely used software distributions
> > > > > using Git, and it *is* quite widely used. Etch carries v1.4.4.4, which
> > > > > fails miserably on the new packs.
> >
> > Maybe we can release 1.4.5 with the ability to read index v2? That
> > wouldn't be hard to backport the reading part of it.
>
> If we consider that supporting 1.4.4.4 clients is still a priority,
> due to the widespread distribution of that version in a popular
> version of Debian, we shouldn't be rushing the index v2 or OFS_DELTA
> functionality on by default in 1.6.0.
OFS_DELTA is supported by 1.4.4.4 so that's a non issue.
> Instead we would wait until Debian stable (and most other widely
> popular distributions) are on a modern enough version of Git to
> understand this format.
I don't think we should have git development be dictated by some
discutable policy from one distribution.
IMHO git prior 1.5.0 is so horrible as general usability goes, and so
different from what everybody is discussing on the net, that no one sane
should still be using it. Even ourselves (i.e. the git community) are
not supporting git 1.4.4 anymore so this hardly can be a priority.
As far as I know, there is no other widely popular distribution other
than Debian using git prior 1.5.0 in their latest release. If Debian
people want to support git 1.4.4 although we called thatversion obsolete
_long_ ago then that's their problem. We should not be bound by that
external policy to which we never agreed with.
Now I proposed a compromise which consists of making 1.4.4.4+1 able to
cope with index v2. That should fall into Debian's "major usability
fix" category. I think that is a far better compromize than delaying
index v2 even further.
> Really. As much as I'd love to see the switch to v2 made by default
> I don't think we can/should do it unless the majority of the user
> base will be able to grok it. And Debian etch sounds like it won't.
I truly hope the majority of the user is _not_ using 1.4.4.4. Otherwise
I may only have pity for them.
Nicolas
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox