* Re: What's cooking in git.git (topics)
From: Lea Wiemann @ 2008-07-14 23:20 UTC (permalink / raw)
To: Lea Wiemann; +Cc: Junio C Hamano, git
In-Reply-To: <487BDD56.7010505@gmail.com>
Lea Wiemann wrote:
> It'll be fixed in the next version I post
By the way Junio, how do you prefer to get reposts of patch sequences?
Should I repost the whole sequence under a new common parent message, or
can I simply post v2 of each patch in the sequence as a followup to its
respective v1?
^ permalink raw reply
* Re: [BUG] commit walk machinery is dangerous !
From: Junio C Hamano @ 2008-07-14 23:32 UTC (permalink / raw)
To: Nicolas Pitre; +Cc: git
In-Reply-To: <alpine.LFD.1.10.0807141904250.12484@xanadu.home>
Nicolas Pitre <nico@cam.org> writes:
> It is those with semantic meaning (e.g. object doesn't exist) which
> should be audited, especially if used in the context of repository
> modification, which pretty much limits it to the test case I produced.
I've been wondering if we should make the change 8eca0b4 (implement some
resilience against pack corruptions, 2008-06-23) less aggressive.
It makes loose objects and data from other packs to be used as fallback
where we used to just punt, which is a genuine improvement for "salvaging"
mode of operation, but at the same time, it now forbids the callers to
expect that the objects they learned to exist from has_sha1_file() or
nth_packed_object_sha1() should never result NULL return value from
read_sha1_file().
It may make it safe again to fail if you cannot salvage using fallback
method after all. Something like the attached.
This is unrelated to the issue at hand, but I also notice that there are
few callsites outside sha1_file.c that bypasses cache_or_unpack_entry()
and call unpack_entry() directly. I wonder if they should be using the
cached version, making unpack_entry() static...
sha1_file.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/sha1_file.c b/sha1_file.c
index 2df78b5..55aa361 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1649,7 +1649,7 @@ static void *unpack_delta_entry(struct packed_git *p,
mark_bad_packed_object(p, base_sha1);
base = read_sha1_file(base_sha1, type, &base_size);
if (!base)
- return NULL;
+ exit(129);
}
delta_data = unpack_compressed_entry(p, w_curs, curpos, delta_size);
@@ -1946,6 +1946,8 @@ static void *read_packed_sha1(const unsigned char *sha1,
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);
+ if (!data)
+ exit(129);
}
return data;
}
^ permalink raw reply related
* Re: [BUG] commit walk machinery is dangerous !
From: Nicolas Pitre @ 2008-07-14 23:39 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vod50dote.fsf@gitster.siamese.dyndns.org>
On Mon, 14 Jul 2008, Junio C Hamano wrote:
> Nicolas Pitre <nico@cam.org> writes:
>
> > It is those with semantic meaning (e.g. object doesn't exist) which
> > should be audited, especially if used in the context of repository
> > modification, which pretty much limits it to the test case I produced.
>
> I've been wondering if we should make the change 8eca0b4 (implement some
> resilience against pack corruptions, 2008-06-23) less aggressive.
>
> It makes loose objects and data from other packs to be used as fallback
> where we used to just punt, which is a genuine improvement for "salvaging"
> mode of operation, but at the same time, it now forbids the callers to
> expect that the objects they learned to exist from has_sha1_file() or
> nth_packed_object_sha1() should never result NULL return value from
> read_sha1_file().
>
> It may make it safe again to fail if you cannot salvage using fallback
> method after all. Something like the attached.
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.
Nicolas
^ permalink raw reply
* Re: [PATCH 2/3] add new Git::Repo API
From: Jakub Narebski @ 2008-07-14 23:41 UTC (permalink / raw)
To: Petr Baudis; +Cc: Lea Wiemann, git, John Hawley
In-Reply-To: <20080714014051.GK10151@machine.or.cz>
On Mon, 14 July 2008, Petr Baudis wrote:
> On Fri, Jul 11, 2008 at 03:11:05AM +0200, Lea Wiemann wrote:
> >
> > This also adds the Git::Commit and Git::Tag classes, which are used by
> > Git::Repo, the Git::Object base class, and the Git::RepoRoot helper
> > factory class.
>
> I really miss some more detailed writeup on the envisioned design here.
> And if we are redoing the API in a better way, we better should have
> some vision.
Once again: if you are adding some large amount of code, you'd better
describe "whys" of it.
> Most importantly, how is Git::Repo interacting with working copies, and
> how is it going to interact with them as the new OO model shapes up?
> You mention briefly Git::WC later, but it is not really clear how the
> interaction should work.
>
>
> First, I don't think it's good idea at all to put the pipe-related stuff
> to Git::Repo - this is botched up API just like the current one. This
> all is independent from any repository instances, in fact it's perfectly
> valid to call standalone remote commands (like ls-remote or, actually,
> clone).
There are three classes of git commands:
1. standalone i.e. those that doesn't require even repository, like
e.g. git-ls-remote, git-clone or git-init (or git wrapper, like
for example in "git --version").
2. those that require repository (and should use --git-dir=<path>),
like for example git-cat-file, git-log / git-rev-list,
git-for-each-ref / git-show-refs, git-diff-tree, git-ls-tree;
3. those that require both repository and working copy (and should
probably use both --git-dir=<path> and --work-tree=<path>), like
git-commit, git-clean, git-ls-files (the last one can require
only index).
3'. those that require both repository and working copy, and whose
behavior depends on where in working copy we (current directory)
is[*1*].
*All* git commands should know path to "git" wrapper binary, or where
$GIT_EXEC_PATH is.
[*1*] It is (besides pointer to Git::Repo instance) what Git::WC
differs from simple directory: the pointer where we are, and
wc_path(), wc_subdir(), [wc_cdup(),] and wc_chdir(<SUBDIR>) methods.
> Here is an idea: Introduce Git::Command object that will have very
> general interface and look like
>
> my $c = Git::Command->new(['git', '--git-dir=.', 'cat-file', \
> '-p', 'bla'], {pipe_out=>1})
> ...
> $c->close();
Errr... how do you read from such a pipe? <$c> I think wouldn't work,
unless you would use some trickery...
> and a Git::CommandFactory with a nicer interface that would look like
>
> my $cf = Git::CommandFactory->new('git', '--git-dir=.');
> my $c = $cf->output_pipe('cat-file', '-p', 'bla');
> $c->close();
>
> Then, Git::Repo would have a single Git::CommandFactory instance
> pre-initialized with the required calling convention, and returned by
> e.g. cmd() method. Then, from the user POV, you would just:
>
> my $repo = Git::Repo->new;
> $repo->cmd->output_pipe('cat-file', '-p', 'bla');
>
> Or am I overdoing it?
You are probably overdoing it.
I think it would be good to have the following interface
Git->output_pipe('ls-remotes', $URL, '--heads');
[...]
$r = Git::Repo->new(<git_dir>);
$r->output_pipe('ls_tree', 'HEAD');
[...]
$nb = Git::Repo::NonBare->new(<git_dir>[, <working_area>]);
$nb->output_pipe('ls-files');
How can it be done with minimal effort, unfortunately I don't know...
> Git::Repo considers only bare repositories. Now, since "working copy" by
> itself has nothing to do with Git and is just an ordinary directory
> tree,
Well, it does provide also current subdir pointer, pointer to git
repository it is associated with, and a few methods to examine and
change both.
> I think Git::WC does not make that much sense, but something like
> Git::Repo::Nonbare certainly would. This would be a Git::Repo subclass
> with:
>
> * Different constructor
>
> * Different Git::CommandFactory instance
>
> * Git::Index object aside the existing ones (like Git::Config,
> Git::RefTree, ...)
>
> * Some kind of wc_root() method to help directory navigation
>
> And that pretty much covers it?
Good idea, I think.
> Another thing is clearly describing how error handling is going to work.
> I have not much against ditching Error.pm, but just saying "die + eval"
> does not cut it - how about possible sideband data? E.g. the failure
> mode of Git.pm's command() method includes passing the error'd command
> output in the exception object. How are we going to handle it? Now, it
> might be actually okay to say that we _aren't_ going to handle this if
> it is deemed unuseful, but that needs to be determined too. I don't know
> off the top of my head.
I think that the solution might be some output_pipe option on how to
treat command exit status, command STDERR, and errors when invoking
command (for example command not found).
Mentioned http://http://www.perl.com/pub/a/2002/11/14/exception.html
explains why one might want to use Error.pm.
> > ---
> >
> > Please note before starting a reply to this: This is not an argument;
> > I'm just explaining why I implemented it the way I did. So please
> > don't try to argue with me about what I should or should have done.
> > I'm not going to refactor Git::Repo to use Git.pm or vice versa; it's
> > really a much more non-trivial task than you might think at first
> > glance.
[...]
> I agree that your main objective is caching for gitweb, but that's not
> what everything revolves around for the rest of us. If you chose the way
> of caching within the Git API and introducing the API to gitweb, I think
> you should spend the effort to deal with the API properly now.
I think the idea is that gitweb caching as it is implemented (data
caching) requires some kind of Perl API, and that existing Git.pm
didn't cut -- therefore Git::Repo and friends was created. But the
focus is gitweb caching, not Perl API (besides Perl API having to
be usable).
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: [PATCH 0/6] archive: refactor and cleanup
From: Lea Wiemann @ 2008-07-14 23:41 UTC (permalink / raw)
To: René Scharfe; +Cc: Junio C Hamano, Git Mailing List
In-Reply-To: <487BA74E.5070208@lsrfire.ath.cx>
René Scharfe wrote:
> This series is a collection of cleanups for git archive
A few things:
* The note quoted above probably shouldn't be in the commit message but
after the "---".
* You patch messages seem to be In-Reply-To a message that wasn't posted
on the list, not to PATCH 0/6.
* 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.)
* And most importantly, your Thunderbird introduced line-breaks, so it's
not possible to apply the patches. May I suggest you use git-send-email
instead?
Feel free to ping me on IRC (lea_w in #git) if you need help.
^ permalink raw reply
* Re: [PATCH 3/3] gitweb: use new Git::Repo API, and add optional caching
From: Lea Wiemann @ 2008-07-14 23:56 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git, John Hawley, Petr Baudis
In-Reply-To: <200807150114.44402.jnareb@gmail.com>
Jakub Narebski wrote:
> Lea Wiemann wrote:
>> Gitweb's cache is actually never out-of-date
>
> Could you explain then how gitweb cache is invalidated?
Sure; from gitweb.perl:
# 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,
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.
^ permalink raw reply
* Re: What's cooking in git.git (topics)
From: Junio C Hamano @ 2008-07-15 0:03 UTC (permalink / raw)
To: Lea Wiemann; +Cc: git
In-Reply-To: <487BDF57.4090900@gmail.com>
Lea Wiemann <lewiemann@gmail.com> writes:
> Lea Wiemann wrote:
>> It'll be fixed in the next version I post
>
> By the way Junio, how do you prefer to get reposts of patch sequences?
> Should I repost the whole sequence under a new common parent message, or
> can I simply post v2 of each patch in the sequence as a followup to its
> respective v1?
I do not have major preference either way, but for a long series, I'd
prefer a resend to be independent from the previous series, i.e.
[PATCH 0/3]
.[PATCH 1/3]
..[PATCH 2/3]
...[PATCH 3/3]
[PATCH 0/4 v2]
.[PATCH 1/4 v2]
..[PATCH 2/4 v2]
...[PATCH 3/4 v2]
....[PATCH 4/4 v2]
I can live with the first one from the new series being a follow-up to the
first one from the old series, i.e.:
[PATCH 0/3]
.[PATCH 1/3]
..[PATCH 2/3]
...[PATCH 3/3]
.[PATCH 0/4 v2]
..[PATCH 1/4 v2]
...[PATCH 2/4 v2]
....[PATCH 3/4 v2]
.....[PATCH 4/4 v2]
but _not_ with this, i.e. N/M being followup to old N/M:
[PATCH 0/3]
.[PATCH 0/4 v2]
.[PATCH 1/3]
..[PATCH 1/4 v2]
..[PATCH 2/3]
...[PATCH 2/4 v2]
...[PATCH 3/3]
....[PATCH 3/4 v2]
.....[PATCH 4/4 v2]
^ permalink raw reply
* Re: [PATCH 2/3] add new Git::Repo API
From: Lea Wiemann @ 2008-07-15 0:11 UTC (permalink / raw)
To: Jakub Narebski; +Cc: Petr Baudis, git, John Hawley
In-Reply-To: <200807150141.39186.jnareb@gmail.com>
Jakub Narebski wrote:
> Git->output_pipe('ls-remotes', $URL, '--heads');
I'd make it cmd => ['ls-remotes', $URL, '--heads'] to avoid making the
interface inflexible, and perhaps add a git_binary option to specify the
binary path. (Not that I'm suggesting something like this should be
added right now; but perhaps it'll be needed at some point.)
> I think that the solution might be some output_pipe option on how to
> treat command exit status, command STDERR, and errors when invoking
> command (for example command not found).
You read my design notes about error handling at the top of
<487BD0F3.2060508@gmail.com>, and you noticed there's max_exit_code in
cmd_output? With the approach to error handling I described, open/exit
errors can cause the method to die, and stderr can be ignored.
If you're trying to implement a more sophisticated error handling
approach, be warned that you're opening a can of worms. ;) If you're
aiming for something like that, it should be kept out of Git::Repo
(which has very coherent error handling as-is), and preferably be
discussed under a separate subject, since it's way beyond the scope of
this patch.
^ permalink raw reply
* [PATCH] t9600: allow testing with cvsps 2.2, including beta versions
From: Pavel Roskin @ 2008-07-15 0:20 UTC (permalink / raw)
To: git
Don't assume that unsupported versions are too old, they may be too new.
Signed-off-by: Pavel Roskin <proski@gnu.org>
---
t/t9600-cvsimport.sh | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
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*)
+ ;;
'')
say 'skipping cvsimport tests, cvsps not found'
test_done
exit
;;
*)
- say 'skipping cvsimport tests, cvsps too old'
+ say 'skipping cvsimport tests, unsupported cvsps version'
test_done
exit
;;
^ permalink raw reply related
* 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
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