* Re: [PATCH] Added --export option to git-send-email.
From: Junio C Hamano @ 2008-07-13 22:05 UTC (permalink / raw)
To: Avery Pennarun; +Cc: Eduard - Gabriel Munteanu, ryan, git
In-Reply-To: <32541b130807131444s5e9ea0d6v9610dd5871467fc9@mail.gmail.com>
"Avery Pennarun" <apenwarr@gmail.com> writes:
> So by encoding it this way, someone with a modern mailer could...
Heh, so by that definition Gnus is not modern enough; neither is webmail
interface at gmail.
>From which has originally zero level of >
>From which has one
>>From which has two
>>>From which has three
I am reasonably sure the above will be sent with one, one, two and three
gt before "From". Is your mailer modern enough?
^ permalink raw reply
* Re: [PATCH] bash completion: Fix the . -> .. revision range completion
From: Shawn O. Pearce @ 2008-07-13 22:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Petr Baudis, git
In-Reply-To: <7vskudpiqq.fsf@gitster.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> wrote:
> Petr Baudis <pasky@suse.cz> writes:
> > When Git sees a string with trailing dot on a place where revision
> > range could occur, it will unconditionally append another dot to
...
> > git log git-submodule.<tab>
...
> By the way, the above command line is another "dot" related frustration I
> always have. If you try:
>
> git log v1.5.6.<TAB>
>
> the completion code adds a dot unconditionally when I want to choose from
> the list of v1.5.6.X tags.
This annoys me too Junio. Its a bug I just never got around to fixing.
Based on Petr's comments and yours I'm wondering if this is just not the
better patch to apply here:
--8<--
bash completion: Don't offer "a.." as a completion for "a."
If the user is trying to complete "v1.5.3.<tab>" to see all of
the available maintenance releases for 1.5.3 we should not give
them an extra dot as the completion. Instead if the user wants
a ".." or a "..." operator they should key the two dots out on
their own. Its the same number of keystrokes either way.
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
contrib/completion/git-completion.bash | 3 ---
1 files changed, 0 insertions(+), 3 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 61581fe..9a1c66a 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -324,9 +324,6 @@ __git_complete_revlist ()
cur="${cur#*..}"
__gitcomp "$(__git_refs)" "$pfx" "$cur"
;;
- *.)
- __gitcomp "$cur."
- ;;
*)
__gitcomp "$(__git_refs)"
;;
--
1.5.6.2.393.g45096
--
Shawn.
^ permalink raw reply related
* Re: git pull is slow
From: Shawn O. Pearce @ 2008-07-13 22:11 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Stephan Hennig, Nicolas Pitre, Andreas Ericsson, git
In-Reply-To: <alpine.DEB.1.00.0807131555560.4816@eeepc-johanness>
Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> On Sun, 13 Jul 2008, Shawn O. Pearce wrote:
> > Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > ...
> > > I expect this to touch the resolve_delta() function of index-pack.c in a
> > > major way, though.
> >
> > Yea, that's going to be ugly. The "cache" you speak of above is held on
> > the call stack as resolv_delta() recurses through the delta chain to
> > reconstruct objects and generate their SHA-1s. There isn't a way to
> > release these objects when memory gets low so your worst case scenario
> > is a 100M+ blob with a delta chain of 50 or more - that will take you 5G
> > of memory to pass through index-pack.
>
> Actually, there is...
>
> You would only need to tap into the "release_pack_memory()" mechanism,
> adding a sort of a "smart pointer" that knows how to reconstruct its
> contents.
I guess you don't really know how index-pack is organized than.
It is not quite that simple.
I think we want to use the core.deltabasecachelimit parameter inside
of index-pack to limit the size of the cache, but evict based on
the callstack depth. Objects deeper back in the callstack should
evict before objects closer to the top of the stack.
Reconstruction is a bit complex as we don't have the machinary in
sha1_file.c available to us, as the index is not available, as we
are still in the process of creating the damn thing.
I'm working up a patch series to resolve this. I'm going to shutup
now and just try to post working code sometime this evening.
--
Shawn.
^ permalink raw reply
* Re: [PATCH] bash completion: Improve responsiveness of git-log completion
From: Ingo Molnar @ 2008-07-13 22:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Shawn O. Pearce, git, Petr Baudis
In-Reply-To: <7vy745piqy.fsf@gitster.siamese.dyndns.org>
* Junio C Hamano <gitster@pobox.com> wrote:
> "Shawn O. Pearce" <spearce@spearce.org> writes:
>
> > Junio noticed the bash completion has been taking a long time lately....
>
> The credit actually goes to Ingo.
>
> > Petr Baudis tracked it down to 72e5e989b ("bash: Add space after
> > unique command name is completed."). Tracing the code showed
> > we spent significant time inside of this loop within __gitcomp,
> > due to the string copying overhead....
> > ...
> > Does this make things better? Or worse? I'm not seeing a huge
> > difference on my own system. Maybe its too fast these days...
>
> Ingo, I understand you have stopped using the completion long time ago
> due to this latency issue. Together with d773c63 (bash: offer only
> paths after '--', 2008-07-08) that already is in 'maint' and 'master',
> this hopefully would make the completion usable for you again?
yeah. I've checked out the latest version and applied Shawn's patch and
added the patched contrib/completion/git-completion.bash back to my
.bashrc and i'm not seeing the latencies anymore.
Thanks! Please consider this fixed - will follow up if there's any
problem left.
Ingo
^ permalink raw reply
* Re: [PATCH] Added --export option to git-send-email.
From: Avery Pennarun @ 2008-07-13 22:21 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Eduard - Gabriel Munteanu, ryan, git
In-Reply-To: <7vvdz9o2wu.fsf@gitster.siamese.dyndns.org>
On 7/13/08, Junio C Hamano <gitster@pobox.com> wrote:
> Heh, so by that definition Gnus is not modern enough; neither is webmail
> interface at gmail.
>
> From which has originally zero level of >
> >From which has one
> >>From which has two
> >>>From which has three
>
> I am reasonably sure the above will be sent with one, one, two and three
> gt before "From". Is your mailer modern enough?
I read this message at gmail, and got zero, one, two, and three
dashes, respectively. For the sake of interest, I then connected to
gmail using IMAP from mutt, and the message was still intact. Then I
saved it to an mbox file using mutt, and it used Content-Length
instead of From munging, so it wasn't a very good experiment :)
If you're not actually delivering your mail using mbox format (as
opposed to imap, webmail, etc), it would be quite insulting for "From
" to be munged at all, so I'm happy at least that I didn't experience
that in my tests. On the other hand I'm surprised that gmail should
have corrupted it for you; gmail should be able to completely forget
about the concept of "From " munging since they don't use mbox
internally. (And it's the MDA's job to munge it, not the MTA, so the
lines should never be munged while in transit *to* gmail unless the
mailer app is extremely buggy.)
Have fun,
Avery
^ permalink raw reply
* Re: [PATCH] Added --export option to git-send-email.
From: Junio C Hamano @ 2008-07-13 22:44 UTC (permalink / raw)
To: Avery Pennarun; +Cc: Eduard - Gabriel Munteanu, ryan, git
In-Reply-To: <32541b130807131521w79cbf23m5934678e68312798@mail.gmail.com>
"Avery Pennarun" <apenwarr@gmail.com> writes:
> And it's the MDA's job to munge it, not the MTA, so the
> lines should never be munged while in transit *to* ...
Yeah, you're right. Sending side should not have any issues like that.
My receiving end (I think fetchmail slurps from my ISP and drops mails in
my local mbox, but it's a long time ago I configured my system...) does
quote only the "From " at the beginning, which is consistent with the
traditional mbox behaviour, so I see one, one, two, three in my local
mbox, and zero, one, two, three on gmane news article.
If this additional option claims to produce a mbox, I think:
(1) quoting only /^From / (not /^>*From/) to be consistent with the
standard practice is the right thing to do; and
(2) reading side might need to also pay attention to /^>From /, in case
somebody feeds an output from this option back to send-email.
However, strictly speaking,(2) may break the standard workflow of
generating patches with format-patch and feeding the result to send-email,
as format-patch does not do /^From / munging (and it shouldn't). The
issue is mostly theoretical --- it only matters if your commit log has a
line that begins with ">From ", and people who have worked with e-mail for
any nontrivial period have already learned to reword their sentences to
avoid lines that begin with "From " (or ">From ", for that matter) anyway.
^ permalink raw reply
* Re: [PATCH] bash completion: Fix the . -> .. revision range completion
From: Petr Baudis @ 2008-07-13 23:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Shawn O. Pearce, git
In-Reply-To: <7vskudpiqq.fsf@gitster.siamese.dyndns.org>
On Sun, Jul 13, 2008 at 02:38:37PM -0700, Junio C Hamano wrote:
> By the way, the above command line is another "dot" related frustration I
> always have. If you try:
>
> git log v1.5.6.<TAB>
>
> the completion code adds a dot unconditionally when I want to choose from
> the list of v1.5.6.X tags. Of course, I can work this around by dropping
> the last dot before asking for completion, so it is not really a very big
> deal, but I mention it here because this annoyance is exactly in the same
> league as your "git-submodule.<TAB>" example.
Actually, my original solution to this problem was simply to remove the
. -> .. completion altogether. Maybe this would still be the best course
of action? I don't think the . -> .. is actually very useful for anyone,
since you might as well just hit the dot another time instead of a tab.
> > contrib/completion/git-completion.bash | 7 ++++++-
> > 1 files changed, 6 insertions(+), 1 deletions(-)
> >
> > diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> > index 61581fe..fe24b8c 100755
> > --- a/contrib/completion/git-completion.bash
> > +++ b/contrib/completion/git-completion.bash
> > @@ -325,7 +325,12 @@ __git_complete_revlist ()
> > __gitcomp "$(__git_refs)" "$pfx" "$cur"
> > ;;
> > *.)
> > - __gitcomp "$cur."
> > + if ls "$cur"* >/dev/null 2>&1; then
>
> There is a slight Yuck factor for using "ls" here but I do not think of a
> better alternative offhand.
I have thought about this hard for some time, but couldn't come up with
anything better than this or
(shopt -s nullglob; completion=("$cur"*); [ -n "$completion" ])
which looks quite awful (and can waste a lot of memory in case of some
really insane completion).
--
Petr "Pasky" Baudis
GNU, n. An animal of South Africa, which in its domesticated state
resembles a horse, a buffalo and a stag. In its wild condition it is
something like a thunderbolt, an earthquake and a cyclone. -- A. Pierce
^ permalink raw reply
* Re: [PATCH] bash completion: Fix the . -> .. revision range completion
From: Junio C Hamano @ 2008-07-13 23:25 UTC (permalink / raw)
To: Petr Baudis; +Cc: Shawn O. Pearce, git
In-Reply-To: <20080713230724.GJ10151@machine.or.cz>
Petr Baudis <pasky@suse.cz> writes:
> On Sun, Jul 13, 2008 at 02:38:37PM -0700, Junio C Hamano wrote:
>> By the way, the above command line is another "dot" related frustration I
>> always have. If you try:
>>
>> git log v1.5.6.<TAB>
>>
>> the completion code adds a dot unconditionally when I want to choose from
>> the list of v1.5.6.X tags. Of course, I can work this around by dropping
>> the last dot before asking for completion, so it is not really a very big
>> deal, but I mention it here because this annoyance is exactly in the same
>> league as your "git-submodule.<TAB>" example.
>
> Actually, my original solution to this problem was simply to remove the
> . -> .. completion altogether. Maybe this would still be the best course
> of action? I don't think the . -> .. is actually very useful for anyone,
> since you might as well just hit the dot another time instead of a tab.
I think that is what Shawn sent a few minutes ago, so you two are in
agreement, and I will be happy with it, too.
^ permalink raw reply
* Re: [PATCH 2/3] add new Git::Repo API
From: Jakub Narebski @ 2008-07-13 23:28 UTC (permalink / raw)
To: Lea Wiemann; +Cc: git, John Hawley, Petr Baudis
In-Reply-To: <1215738665-5153-1-git-send-email-LeWiemann@gmail.com>
On Fri, 11 July 2008 03:11, Lea Wiemann napisał:
> 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 think it would be perhaps better to explain relationship and purpose
of each class in more detail, including Git::Repo.
No signoff - is it deliberate, or just omission?
> ---
> Here's some elaboration on why I didn't use or extend Git.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.
>
> Anyways, the following bullet points are my reasons for not extending
> Git.pm:
>
> - Git.pm doesn't do what I want: It's designed to provide access to
> working copies. Extending it to have more repository-access
> functions might have resulted in a mess.
I don't quite understand. True, Git->repository(...) has a bit strange
[optional] parameters, but you should remember that IIRC it predates
possibility of workdir / worktree / working copy separation. That
I think is why it supports non-bare repositories (default), and bare
repositories, not separating working copy from repository.
Also, from what I understand and remember, Git.pm was created to have
one place, one single implementation for safe and operating system
independent (which means that it works with ActiveState Perl on
Windows) "pipe" and "backticks". Everything else is just convenience.
> Some long-term thoughts on this issue: I don't think Git.pm's design
> approach is sensible: It tries to do (a) WC access, (b) repo access,
> and (c) frontend error handling (with sensible error messages).
I can see (b) and (c), but I have trouble seeing (a).
> Those things should really be separated; e.g. one could write a
> Git::WC class that *has* a Git::Repo instance (since every working
> copy has a repository associated with it); so you can use $wc =
> Git::WC->new to access the working copy, and $wc->repo to access its
> repository.
Please remember that the opposite relation is also true. Non-bare
repository has working copy / worktree associated with it, either
implicitly (.git/.. is working copy), or explicitly (core.worktree).
Also even if worktree and repository are separated, there are a few
files in non-bare repository which refer to worktree condition, namely
$GIT_DIR/HEAD, $GIT_DIR/logs/HEAD and $GIT_DIR/index.
> Git.pm will obviously have to stay since a few git
> commands use it,
The following commands use Git.pm: contrib/examples/git-remote.perl,
git-add--interactive.perl (helper script), git-cvsexportcommit, git-svn,
git-send-email.
The following commands are written in Perl, but *do not* use Git.pm:
git-archimport (which probably should go to contrib, and be replaced by
fast-import / fast-export based Bazaar import), git-cvsimport,
git-cvsserver, git-relink (rarely used now, I think).
> but I'd probably let it die a slow death, and (cleanly!) copy
> functionality to a Git::WC module (and perhaps a Git::Frontend
> module) in the long run.
I'm not so sure if it is a way to go. Most git commands wants to just
invoke other git commands safely, either creating pipeline, or reading
whole input; sometimes command output is not important.
Non OO things, like ability to write
print color('reset') . "\n";
is also important.
> - It would have needed refactoring and more features.
>
> - For example, the Error module should be removed (and normal die
> and eval should be used instead). At some point when I was trying
> to debug it through gitweb, Git.pm would error out somewhere and I
> would randomly get either a blank page (usually) or an error page
> (rarely). I suspect that this is because of the Error module or
> some strange interaction with another.
I'm not sure if using Error module and syntactic sugar in the form of
try { ... } catch <class> with { ... } was a good idea for (c),
frontend error handling. Or if it was a good idea (the explanation in
http://www.perl.com/pub/a/2002/11/14/exception.html is compelling)
if it was implemented fully, completely, and correctly.
How would you like to catch errors from frontend in Git::Repo and
friends?
> [...] (Besides, Error is not in
> the Perl distribution, so it would be an unnecessary dependency
> for gitweb; the only one as far as I can see.)
Git Makefile installs Error module from local copy if one is not
available. perl-Git module (which would be unfortunately required to
use gitweb; well, that or you could install it locally) has
"perl(Error)" as one of dependencies. perl-Error module is a standard
module and I think it can be found in any modern Linux distribution; if
not, see first sentence in this paragraph.
> - Also, I needed something like max_exit_code and a custom path to
> the git binary; adding max_exit_code would have been non-trivial.
What is max_exit_code (and why for example you couldn't just create your
own derivative of Error)?
> Now I'm all in favor of re-using existing code, but refactoring
> Git.pm would have taken *much* longer than simply writing a new
> module. I'm working on caching for gitweb, not on implementing the
> next great Perl API for Git. (And Git::Repo isn't great, FTR.)
Well, I can understand that.
Besides, having more than one implementation of some new feature was
quite common in early stages of feature development. See for example
wit and gitweb, git-applymbox and git-am, git-annotate and git-blame.
> - It's buggy and untested. Neither of these is a problem by itself,
> but the combination is deadly.
Haven't you added t/t9700-perl-git.sh?
> E.g. I was trying to refactor the
> 'repository' constructor (to be able to do instantaneous
> instantiation) and stumbled upon a missing semicolon that rendered
> the surrounding code syntactically correct but obviously buggy (line
> 214 on master). Adding a semicolon there seemed to cause other
> errors, and given that (a) I don't understand what the code does and
> no test or comment tells me what it should, and (b) it doesn't work
> (or maybe it half-works?), I lost all my confidence that I could do
> anything resembling a (behavior-preserving) refactoring on that
> code.
This is not that the code should not be rewritten... but git-blame (and
git log -S, aka pickaxe search) can be created to find who is the
author of said code, and ask him via email (probably Cc-ing git mailing
list).
> I might have been able to work with this particular problem, but
> such a problem (buggy and completely untested code) are indicative
> that the rest of the code might bear similar surprises.
I think that the pipe and command code might be tested best, as it is
collation and condensation of many different "safe pipe" (etc.) code
fragments.
> - It's overly lengthy, and it's a lot of code for not much
> functionality. Git.pm has 1200 LOC, with a large stack of tiny
> subroutines to handle pipes, and a complicated error handling
> system. I can make out ~400 lines of actual Git functionality, plus
> a 100-lines constructor (all including documentation). The part of
> Git::Repo that overlaps with Git.pm (in terms of duplicate
> functionality) seems to be in the range of 150-200 lines, and it's
> mostly pipe-handling. That's not a whole lot.
Pipe handling is IMHO most important part. Well, other parts such as
color(...) are important too, but not for gitweb.
> - When I decided I didn't want to use Git.pm, it took me 2-3 hours to
> replicate the functionality in Git.pm that I needed; it would have
> taken me *much* longer to extend Git.pm to do what I want. Again,
> I'm not working on the next great Git Perl API.
>
> So where do we go with Git.pm and Git::Repo? I would suggest that
> they both stay.
>
> Thus we'd have two APIs (both of them obviously incomplete). If
> there's enough objection to having two competing official APIs, I'll
> be happy to move Git::Repo to the gitweb directory, and make it a
> gitweb-specific thing (though it's not gitweb-specific at all in terms
> of functionality); I really don't care, as my goal is to get caching
> for gitweb working. Again, if you're bothered by the fact that
> there's duplicate functionality at all, please don't complain, but
> send patches to reconcile the APIs; I will not take care of that
> myself, since it's a very much non-trivial task.
What I worry about is that dependence on Git.pm or Git::Repo would make
gitweb installation too hard for some.
> Unrelatedly, should I add copyright notices at the bottom of each perl
> module so they are displayed in the perldoc/man pages? I'm not
> generally a fan of such notices, since they tend to establish code
> ownership, but if it's desired I can add them.
Well, most manpages have information about who made them... which means
who was initial author, usually, and/or who is current maintainer.
[Comments on patch itself in separate email, later]
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: [PATCH] Added --export option to git-send-email.
From: Avery Pennarun @ 2008-07-13 23:42 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Eduard - Gabriel Munteanu, ryan, git
In-Reply-To: <7vlk05o14i.fsf@gitster.siamese.dyndns.org>
On 7/13/08, Junio C Hamano <gitster@pobox.com> wrote:
> If this additional option claims to produce a mbox, I think:
>
> (1) quoting only /^From / (not /^>*From/) to be consistent with the
> standard practice is the right thing to do; and
>
> (2) reading side might need to also pay attention to /^>From /, in case
> somebody feeds an output from this option back to send-email.
>
> However, strictly speaking,(2) may break the standard workflow of
> generating patches with format-patch and feeding the result to send-email,
> as format-patch does not do /^From / munging (and it shouldn't).
Note that it's generally very bad practice to do (2) unless you always
quote /^>*From/. Quoting only /^From/ and *then* trying to dequote it
correctly actually increases the number of places where you can
corrupt a message. As a sign that very few programs do (2), I think
it's pretty clear that a lot more people see "From" rewritten as
">From" in their mail app of choice than the reverse.
There is also some debate about what "standard practice" means. See:
http://homepages.tesco.net/J.deBoynePollard/FGA/mail-mbox-formats.html
.
If git is going to start actually producing mbox files (as opposed to
just individual messages as it does now), it should probably
explicitly take a stance on the issue... or perhaps make it
configurable.
Have fun,
Avery
^ permalink raw reply
* Re: [PATCH] bash completion: Fix the . -> .. revision range completion
From: Linus Torvalds @ 2008-07-13 23:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Petr Baudis, Shawn O. Pearce, git
In-Reply-To: <7vhcatnz80.fsf@gitster.siamese.dyndns.org>
On Sun, 13 Jul 2008, Junio C Hamano wrote:
>
> I think that is what Shawn sent a few minutes ago, so you two are in
> agreement, and I will be happy with it, too.
Does it fix this one too:
git show origin/pu:Makef<tab>
which totally screws up and becomes
git show Makefile
dropping the version specifier?
I don't speak completion-script, so somebody else would have to fix this
really annoying bug (*)
Linus
(*) Ok, so it's rare. I have been bitten by it something like twice in the
last months. But when it happens it's really annoying.
^ permalink raw reply
* Re: [PATCH] Added --export option to git-send-email.
From: Eduard - Gabriel Munteanu @ 2008-07-13 23:58 UTC (permalink / raw)
To: Avery Pennarun; +Cc: Junio C Hamano, ryan, git
In-Reply-To: <32541b130807131642k1382cf84gdf5e8bb8d2ff4ffe@mail.gmail.com>
On Sun, 13 Jul 2008 19:42:35 -0400
"Avery Pennarun" <apenwarr@gmail.com> wrote:
> On 7/13/08, Junio C Hamano <gitster@pobox.com> wrote:
> > If this additional option claims to produce a mbox, I think:
> >
> > (1) quoting only /^From / (not /^>*From/) to be consistent with
> > the standard practice is the right thing to do; and
> >
> > (2) reading side might need to also pay attention to /^>From /,
> > in case somebody feeds an output from this option back to
> > send-email.
> >
> > However, strictly speaking,(2) may break the standard workflow of
> > generating patches with format-patch and feeding the result to
> > send-email, as format-patch does not do /^From / munging (and it
> > shouldn't).
>
> Note that it's generally very bad practice to do (2) unless you always
> quote /^>*From/. Quoting only /^From/ and *then* trying to dequote it
> correctly actually increases the number of places where you can
> corrupt a message. As a sign that very few programs do (2), I think
> it's pretty clear that a lot more people see "From" rewritten as
> ">From" in their mail app of choice than the reverse.
Junio is right when he says we should match against "From " instead of
"From" (i.e. we should not touch lines beginning with "Fromage" for
example). Should I resubmit or can it be corrected during the merge?
> There is also some debate about what "standard practice" means. See:
> http://homepages.tesco.net/J.deBoynePollard/FGA/mail-mbox-formats.html
> .
When I started working on this, I visited Wikipedia which provided a
link to qmail's site, which looks quite authoritative on this matter:
http://www.qmail.org/man/man5/mbox.html
I just followed that spec.
> If git is going to start actually producing mbox files (as opposed to
> just individual messages as it does now), it should probably
> explicitly take a stance on the issue... or perhaps make it
> configurable.
>
> Have fun,
>
> Avery
^ permalink raw reply
* Re: [PATCH] bash completion: Fix the . -> .. revision range completion
From: Shawn O. Pearce @ 2008-07-14 0:00 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Junio C Hamano, Petr Baudis, git
In-Reply-To: <alpine.LFD.1.10.0807131649380.3305@woody.linux-foundation.org>
Linus Torvalds <torvalds@linux-foundation.org> wrote:
> On Sun, 13 Jul 2008, Junio C Hamano wrote:
> >
> > I think that is what Shawn sent a few minutes ago, so you two are in
> > agreement, and I will be happy with it, too.
>
> Does it fix this one too:
>
> git show origin/pu:Makef<tab>
>
> which totally screws up and becomes
>
> git show Makefile
>
> dropping the version specifier?
Its unrelated to the issue you described above. But I just tested
your Makefile completion case and it worked as expected, though
I just found out it doesn't add a trailing space once there is a
unique completion made. I'll look at another patch to fix that.
--
Shawn.
^ permalink raw reply
* Re: [PATCH] Added --export option to git-send-email.
From: Eduard - Gabriel Munteanu @ 2008-07-14 0:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Avery Pennarun, ryan, git
In-Reply-To: <7vlk05o14i.fsf@gitster.siamese.dyndns.org>
On Sun, 13 Jul 2008 15:44:29 -0700
Junio C Hamano <gitster@pobox.com> wrote:
> However, strictly speaking,(2) may break the standard workflow of
> generating patches with format-patch and feeding the result to
> send-email, as format-patch does not do /^From / munging (and it
> shouldn't). The issue is mostly theoretical --- it only matters if
> your commit log has a line that begins with ">From ", and people who
> have worked with e-mail for any nontrivial period have already
> learned to reword their sentences to avoid lines that begin with
> "From " (or ">From ", for that matter) anyway.
Munging happens only when doing --export. If you don't supply that
option, it won't affect anything, so there is no breakage.
BTW, git-format-patch already produces output somewhat compatible with
mbox, as it adds a From_ line.
Eduard
^ permalink raw reply
* [PATCH] bash completion: Append space after file names have been completed
From: Shawn O. Pearce @ 2008-07-14 0:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
When completing `git show origin/maint:Makef<tab>` we should add a
space after the filename has been completed, so that the user can
immediately begin the next argument.
I also added a special case for the symlink variant so we treat it
just like a normal blob, as there are no items below it in the Git
tree structure.
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
contrib/completion/git-completion.bash | 10 +++++++++-
1 files changed, 9 insertions(+), 1 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 9a1c66a..790d1c6 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -294,9 +294,17 @@ __git_complete_file ()
ls="$ref"
;;
esac
+ local IFS=$'\n'
COMPREPLY=($(compgen -P "$pfx" \
-W "$(git --git-dir="$(__gitdir)" ls-tree "$ls" \
- | sed '/^100... blob /s,^.* ,,
+ | sed '/^100... blob /{
+ s,^.* ,,
+ s,$, ,
+ }
+ /^120000 blob /{
+ s,^.* ,,
+ s,$, ,
+ }
/^040000 tree /{
s,^.* ,,
s,$,/,
--
1.5.6.2.393.g45096
^ permalink raw reply related
* Re: [PATCH 2/3] add new Git::Repo API
From: Lea Wiemann @ 2008-07-14 1:04 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, John Hawley, Jakub Narebski, Petr Baudis
In-Reply-To: <7vmyklpiqk.fsf@gitster.siamese.dyndns.org>
Junio C Hamano wrote:
> This probably is something the person who mentors you as a GSoC student
> should tell you, and shouldn't come from me,
No, thanks for telling me. I'm actually keeping John quite busy with
the server side on kernel.org, and wouldn't currently expect him to look
at the patches I'm sending.
> I would have preferred to see the first paragraph of yours stated more
> diplomaticly; it sounds rather unilateral ultimatum to me.
Apologies; I was trying to hold off developers who'd see duplicate
functionality and run off at me screaming "unacceptable! impure!!one".
I should've phrased it more carefully.
> This patch was not signed-off, but I assume it was simply forgotten
Yup. In the unlikely case it gets applied as-is, feel free to add it.
(I'll reply to Jakub's email shortly, unless I fall asleep in the
meantime. ^^)
^ permalink raw reply
* Re: [PATCH 2/3] add new Git::Repo API
From: Petr Baudis @ 2008-07-14 1:40 UTC (permalink / raw)
To: Lea Wiemann; +Cc: git, John Hawley, Jakub Narebski
In-Reply-To: <1215738665-5153-1-git-send-email-LeWiemann@gmail.com>
Thanks to Jakub for a nice writeup too. I will try to cover mainly
design issues and do not try to dive too much into technical arguments
about current Git.pm, since I agree with the general idea of designing
a new API; Jakub covered most of that pretty well, I think.
Maybe I sound too mentoring at places; I tried to tone that down as much
as possible, and of course what Lea should be working on is her and hers
official mentor call. On the other hand, if something general gets into
the Git tree, I'd like to make sure it's something we can live happily
with for long time, not just a hack tailored for gitweb caching.
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.
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).
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();
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?
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, 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?
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.
> ---
> Here's some elaboration on why I didn't use or extend Git.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.
Frankly, I'm not really happy about this attitude. "In my dark corner,
I have developed this. Now behold - take it or leave it be; I'm not
going to argue."
That said, I do agree that better-structured object API would be
beneficial, and I think what you posted here is a good starting point.
However, I'm not going to settle with seeing a completely parallel and
independent code pop up alongside the existing Git.pm.
Instead, I believe the best course is to gradually translate all the
Git.pm functionality to the new OO model, leaving Git.pm as a
compatibility wrapper. Now, if you believe this is a non-trivial task,
please tell us why.
It should be actually very easy to start with moving all the pipe
functionality to Git::Command. This should let us get rid of a lot of
technical code, which can be reused in the cleanly rebuilt API. Aside
of the object management + error handling, the rest of Git.pm is then
actually rather boring and unimportant, especially given Jakub's
hopefully upcoming Git::Config.
I will be happy to send in patches factoring out the pipe handling to
Git::Command as soon as we agree about the missing design details and
the way of error handling. Then, making use of that in Git::Repo should
get us a long way ahead.
> Now I'm all in favor of re-using existing code, but refactoring
> Git.pm would have taken *much* longer than simply writing a new
> module. I'm working on caching for gitweb, not on implementing the
> next great Perl API for Git. (And Git::Repo isn't great, FTR.)
Wait, I can't make sense out of this paragraph. If Git.pm sucks, we can
work on new API. But we better _make_ it great. Or someone else comes by
next year and says "oh, but it's buggy and needs refactoring, let's
throw it away and redesign it!"
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.
> diff --git a/perl/Git/Commit.pm b/perl/Git/Commit.pm
> new file mode 100644
> index 0000000..a9bc304
> --- /dev/null
> +++ b/perl/Git/Commit.pm
> @@ -0,0 +1,163 @@
> +=head1 NAME
> +
> +Git::Commit - Object-oriented interface to Git commit objects.
> +
> +=cut
> +
> +use strict;
> +use warnings;
> +
> +
> +package Git::Commit;
> +
> +use base qw(Git::Object);
> +
> +use constant _MESSAGE => 'M';
> +use constant _ENCODING => 'E';
> +use constant _TREE => 'T';
> +use constant _PARENTS => 'P';
> +use constant _AUTHOR => 'A';
> +use constant _COMMITTER => 'C';
What is the idea behind this? It may make sense to create numerical
constants like this and use arrays instead of hashes for "structs" in
order to gain extra performance. But if you are going to use hashes
anyway, why not actually key by sensible name directly?
> +=head1 METHODS
> +
> +=over
> +
> +=item $commit = Git::Commit->new($repo, $sha1)
Are we sure we don't want hash-based arguments instead? This is badly
extensible and inconsistent with the rest of the API.
> +Return a new Git::Commit instance for a commit object with $sha1 in
> +repository $repo.
> +
> +Calls to this method are free, since it does not check whether $sha1
> +exists and has the right type. However, accessing any of the commit
> +object's properties will fail if $sha1 is not a valid commit object.
This is nice idea, but I'd also provide a well-defined way for the user
to verify the object's validity at a good moment; basically, make load()
a public method. The user can deal with errors then and rely on
error-free behavior later.
> +Note that $sha1 must be the SHA1 of a commit object; tag objects are
> +not dereferenced.
Why not?
> +=item $obj->repo
> +
> +Return the Git::Repo instance this object was instantiated with.
> +
> +=item $obj->sha1
> +
> +Return the SHA1 of this commit object.
> +
> +=item $commit->tree
> +
> +Return the tree this commit object refers to.
> +
> +=item $commit->parents
> +
> +Return a list of zero or more parent commit objects. Note that commit
> +objects stringify to their respective SHA1s, so you can alternatively
> +treat this as a list of SHA1 strings.
> +
> +=item $commit->authors
author
> diff --git a/perl/Git/Object.pm b/perl/Git/Object.pm
> new file mode 100644
> index 0000000..a5126f9
> --- /dev/null
> +++ b/perl/Git/Object.pm
> @@ -0,0 +1,81 @@
> +=head1 NAME
> +
> +Git::Object - Object-oriented interface to Git objects (base class).
> +
> +=head1 DESCRIPTION
> +
> +Git::Object is a base class that provides access to commit, tag and
> +(unimplemented) tree objects. See L<Git::Commit> and L<Git::Tag>.
> +
> +Objects are loaded lazily, and hence instantiation is free. Objects
> +stringify to their SHA1s.
Maybe use the term 'Git database objects'? This way, it seems as if we
are talking about all Git/*.pm objects.
> +=cut
> +
> +
> +use strict;
> +use warnings;
> +
> +
> +package Git::Object;
> +
> +use base qw(Exporter);
> +
> +our @EXPORT = qw();
> +our @EXPORT_OK = qw();
> +
> +use overload
> + '""' => \&stringify;
(For the beholders: the overload pragma comes from 5.004.)
> +# Hash indices:
> +use constant _REPO => 'R';
> +use constant _SHA1 => 'H';
> +
> +=head1 METHODS
> +
> +=over
> +
> +=item Git::Object->new($repo, $sha1)
> +
> +Return a new Git::Object instance for the object with $sha1 in the
> +repository $repo (a Git::Repo instance).
> +
> +Note that this method does not check whether the object exists in the
> +repository. Trying to accessing its properties through a subclass
> +will fail if the object doesn't exist, however.
> +
> +=cut
> +
> +sub new {
> + my($class, $repo, $sha1) = @_;
> + die "$repo is not a Git::Repo instance" unless $repo->isa('Git::Repo');
> + my $self = {_REPO() => $repo, _SHA1() => $sha1};
> + return bless $self, $class;
> +}
> +
> +=item $obj->repo
> +
> +Return the Git::Repo instance this object was instantiated with.
> +
> +=cut
> +
> +sub repo {
> + shift->{_REPO()}
> +}
> +
> +=item $obj->sha1
> +
> +Return the SHA1 of this object.
> +
> +=cut
> +
> +sub sha1 {
> + shift->{_SHA1()}
> +}
> +
> +sub stringify {
> + shift->{_SHA1()}
> +}
> +
> +1;
Why not overload "" directly to sha1()?
> diff --git a/perl/Git/Repo.pm b/perl/Git/Repo.pm
> new file mode 100644
> index 0000000..5a4c7e0
> --- /dev/null
> +++ b/perl/Git/Repo.pm
> @@ -0,0 +1,397 @@
> +=head1 NAME
> +
> +Git::Repo - Read-only access to the Git repositories.
> +
> +WARNING: This module is in active development -- do not use it in any
> +production code as the API may change at any time.
> +
> +=head1 SYNOPSIS
> +
> + use Git::Repo;
> +
> + my $repo = Git::Repo->new(
> + directory => '/path/to/repository.git',
> + git_binary => '/usr/bin/git');
> + my ($sha1, $type, $size) = $repo->get_sha1('HEAD');
> + print "Last log message:\n\n" . $repo->get_commit($sha1)->message;
> +
> +=cut
> +
> +
> +use strict;
> +use warnings;
> +use 5.006002;
> +
> +
> +package Git::Repo;
> +
> +use Git::Tag;
> +use Git::Commit;
> +
> +use IPC::Open2 qw(open2);
> +use IO::Handle;
> +
> +use base qw(Exporter);
> +
> +our @EXPORT = qw();
> +our @EXPORT_OK = qw(assert_sha1 assert_opts);
> +
> +# Auxiliary subroutines
> +
> +sub assert_opts {
> + die "must have an even number of arguments for named options"
> + unless $#_ % 2;
> +}
> +
> +sub assert_sha1 {
> + my $sha1 = shift;
> + die "'$sha1' is not a SHA1 (need to use get_sha1?)"
> + unless $sha1 && $sha1 =~ /^[a-f0-9]{40}$/;
> +}
Pretend names with underscores, since they are internal?
> +=head1 METHODS
> +
> +=head2 Instantiation and basic methods
> +
> +=over
> +
> +=item $repo = Git::Repo->new(%opts)
> +
> +Return a new Git::Repo object. The following options are supported:
> +
> +=over
> +
> +=item 'directory'
> +
> +The directory of the repository (mandatory).
> +
> +Note that this option is working-copy agnostic; you need to
> +instantiate it with the working copy's .git directory as the
> +'directory' option.
I don't think making this mandatory is reasonable, since all the git
commands can automatically figure this out by themselves too; so can
Git::Repo easily by calling git rev-parse --git-dir.
> +=item 'git_binary'
> +
> +The name or full path of the git binary (default: 'git').
> +
> +=back
> +
> +Calling this method is free, since it does not check whether the
> +repository exists. Trying to access the repository through one of the
> +instance methods will fail if it doesn't exist though.
> +
> +Examples:
> +
> + $repo = Git::Repo->new(directory => '/path/to/repository.git');
> + $repo = Git::Repo->new(directory => '/path/to/working_copy/.git');
> +
> +=cut
> +
> +sub new {
> + my $class = shift;
> + assert_opts(@_);
> + my $self = {@_};
> + bless $self, $class;
> + die 'no directory given' unless $self->{directory};
> + return $self;
> +}
> +
> +=item $repo->repo_dir
> +
> +Return the directory of the repository (.../.git in case of a working
> +copy).
> +
> +=cut
> +
> +sub repo_dir {
> + shift->{directory}
> +}
> +
> +=item $repo->git_binary
> +
> +Return the name of or path to the git binary (used with exec).
> +
> +=cut
> +
> +sub git_binary {
> + shift->{git_binary}
> +}
--- CUT ---
> +=item $repo->version
..snip..
> +# Return the first items of the git command line, for instance
> +# qw(/usr/bin/git --git-dir=/path/to/repo.git).
> +sub _get_git_cmd {
> + my $self = shift;
> + return ($self->git_binary || 'git', '--git-dir=' . $self->repo_dir);
> +}
--- CUT ---
This definitely does not belong to a Git::Repo object.
BTW, I like the idea of reuse => 1 very much.
> +=back
> +
> +=head2 Inspecting the Repository
> +
> +=over
> +
> +=item $repo->cat_file($sha1)
> +
> +Return the ($type, $content) of the object identified by $sha1, or die
> +if no such object exists in the repository.
I don't think this is good combination of semantic and name. Since we
don't do the same thing as plain git cat-file, we might as well call it
cat_object() or even better get_object().
> +Note that you may want to use the higher-level methods get_commit and
> +get_tag instead.
..snip..
> +=item $repo->get_path($tree_sha1, $file_sha1)
Now we are quickly getting messy again. This should definitely live in
Git::Tree.
> +=item $repo->get_refs
Again, the refs should be properly integrated into the object structure.
--
Petr "Pasky" Baudis
GNU, n. An animal of South Africa, which in its domesticated state
resembles a horse, a buffalo and a stag. In its wild condition it is
something like a thunderbolt, an earthquake and a cyclone. -- A. Pierce
^ permalink raw reply
* [PATCH 0/4] Honor core.deltaBaseCacheLimit during index-pack
From: Shawn O. Pearce @ 2008-07-14 2:07 UTC (permalink / raw)
To: Junio C Hamano, Junio C Hamano
Cc: git, Stephan Hennig, Nicolas Pitre, Andreas Ericsson
In-Reply-To: <20080713011512.GB31050@spearce.org>
This should resolve the obscene memory usage of index-pack when
resolving deltas for very large files.
Shawn O. Pearce (4):
index-pack: Refactor base arguments of resolve_delta into a struct
index-pack: Chain the struct base_data on the stack for traversal
index-pack: Track the object_entry that creates each base_data
index-pack: Honor core.deltaBaseCacheLimit when resolving deltas
index-pack.c | 137 +++++++++++++++++++++++++++++++++++++++++++++------------
1 files changed, 108 insertions(+), 29 deletions(-)
^ permalink raw reply
* [PATCH 1/4] index-pack: Refactor base arguments of resolve_delta into a struct
From: Shawn O. Pearce @ 2008-07-14 2:07 UTC (permalink / raw)
To: Junio C Hamano, Junio C Hamano
Cc: git, Stephan Hennig, Nicolas Pitre, Andreas Ericsson
In-Reply-To: <20080713011512.GB31050@spearce.org>
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>
---
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
^ permalink raw reply related
* [PATCH 2/4] index-pack: Chain the struct base_data on the stack for traversal
From: Shawn O. Pearce @ 2008-07-14 2:07 UTC (permalink / raw)
To: Junio C Hamano, Junio C Hamano
Cc: git, Stephan Hennig, Nicolas Pitre, Andreas Ericsson
In-Reply-To: <20080713011512.GB31050@spearce.org>
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>
---
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
^ permalink raw reply related
* [PATCH 3/4] index-pack: Track the object_entry that creates each base_data
From: Shawn O. Pearce @ 2008-07-14 2:07 UTC (permalink / raw)
To: Junio C Hamano, Junio C Hamano
Cc: git, Stephan Hennig, Nicolas Pitre, Andreas Ericsson
In-Reply-To: <20080713011512.GB31050@spearce.org>
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
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>
---
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
^ permalink raw reply related
* [PATCH 4/4] index-pack: Honor core.deltaBaseCacheLimit when resolving deltas
From: Shawn O. Pearce @ 2008-07-14 2:07 UTC (permalink / raw)
To: Junio C Hamano, Junio C Hamano
Cc: git, Stephan Hennig, Nicolas Pitre, Andreas Ericsson
In-Reply-To: <20080713011512.GB31050@spearce.org>
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>
---
index-pack.c | 43 +++++++++++++++++++++++++++++++++++++++++--
1 files changed, 41 insertions(+), 2 deletions(-)
diff --git a/index-pack.c b/index-pack.c
index 7239e89..84c9fc1 100644
--- a/index-pack.c
+++ b/index-pack.c
@@ -52,6 +52,7 @@ struct delta_entry
static struct object_entry *objects;
static struct delta_entry *deltas;
static struct base_data *base_cache;
+static size_t base_cache_used;
static int nr_objects;
static int nr_deltas;
static int nr_resolved_deltas;
@@ -220,6 +221,20 @@ static void bad_object(unsigned long offset, const char *format, ...)
die("pack has bad object at offset %lu: %s", offset, buf);
}
+static void prune_base_data(void)
+{
+ struct base_data *b = base_cache;
+ for (b = base_cache;
+ base_cache_used > delta_base_cache_limit && b && b->child;
+ b = b->child) {
+ if (b->data) {
+ free(b->data);
+ b->data = NULL;
+ base_cache_used -= b->size;
+ }
+ }
+}
+
static void link_base_data(struct base_data *base, struct base_data *c)
{
if (base)
@@ -229,6 +244,8 @@ static void link_base_data(struct base_data *base, struct base_data *c)
c->base = base;
c->child = NULL;
+ base_cache_used += c->size;
+ prune_base_data();
}
static void unlink_base_data(struct base_data *c)
@@ -238,7 +255,10 @@ static void unlink_base_data(struct base_data *c)
base->child = NULL;
else
base_cache = NULL;
- free(c->data);
+ if (c->data) {
+ free(c->data);
+ base_cache_used -= c->size;
+ }
}
static void *unpack_entry_data(unsigned long offset, unsigned long size)
@@ -456,6 +476,25 @@ static void sha1_object(const void *data, unsigned long size,
}
}
+static void *get_base_data(struct base_data *c)
+{
+ if (!c->data) {
+ 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);
+ if (!c->data)
+ bad_object(obj->idx.offset, "failed to apply delta");
+ } else
+ c->data = raw;
+ }
+ return c->data;
+}
+
static void resolve_delta(struct object_entry *delta_obj,
struct base_data *base_obj, enum object_type type)
{
@@ -468,7 +507,7 @@ static void resolve_delta(struct object_entry *delta_obj,
delta_obj->real_type = type;
delta_data = get_data_from_pack(delta_obj);
delta_size = delta_obj->size;
- result.data = patch_delta(base_obj->data, base_obj->size,
+ result.data = patch_delta(get_base_data(base_obj), base_obj->size,
delta_data, delta_size,
&result.size);
free(delta_data);
--
1.5.6.2.393.g45096
^ permalink raw reply related
* Re: [PATCH/Test] Build in merge is broken
From: Sverre Hvammen Johansen @ 2008-07-14 2:15 UTC (permalink / raw)
To: Miklos Vajna; +Cc: git, Junio C Hamano
In-Reply-To: <20080713184300.GF10347@genesis.frugalware.org>
Test cases for build in merge.
---
After applying Miklos's fix there still are some breakages. I have squashed in
another test case. c1 is merged with c1 and c2. Three parents are
recorded in the
merge commit object; c1, c1, and c2.
t/t7600-merge.sh | 22 ++++++++++++++++++++++
1 files changed, 22 insertions(+), 0 deletions(-)
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 16f4608..80cfee6 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -465,4 +465,26 @@ test_expect_success 'merge log message' '
test_debug 'gitk --all'
+test_expect_success 'merge c1 with c0, c2, c0, and c1' '
+ git reset --hard c1 &&
+ git config branch.master.mergeoptions "" &&
+ test_tick &&
+ git merge c0 c2 c0 c1 &&
+ verify_merge file result.1-5 &&
+ verify_parents $c1 $c2
+'
+
+test_debug 'gitk --all'
+
+test_expect_success 'merge c1 with c1 and c2' '
+ git reset --hard c1 &&
+ git config branch.master.mergeoptions "" &&
+ test_tick &&
+ git merge c1 c2 &&
+ verify_merge file result.1-5 &&
+ verify_parents $c1 $c2
+'
+
+test_debug 'gitk --all'
+
test_done
--
1.5.5.54.gc6550
--
Sverre Hvammen Johansen
^ permalink raw reply related
* Re: [PATCH 0/4] Honor core.deltaBaseCacheLimit during index-pack
From: Nicolas Pitre @ 2008-07-14 2:27 UTC (permalink / raw)
To: Shawn O. Pearce
Cc: Junio C Hamano, Junio C Hamano, git, Stephan Hennig,
Andreas Ericsson
In-Reply-To: <1216001267-33235-1-git-send-email-spearce@spearce.org>
On Sun, 13 Jul 2008, Shawn O. Pearce wrote:
> This should resolve the obscene memory usage of index-pack when
> resolving deltas for very large files.
>
> Shawn O. Pearce (4):
> index-pack: Refactor base arguments of resolve_delta into a struct
> index-pack: Chain the struct base_data on the stack for traversal
> index-pack: Track the object_entry that creates each base_data
> index-pack: Honor core.deltaBaseCacheLimit when resolving deltas
Well...
I don't like this. Not your patch, but rather the direction this whole
thing took in the first place, and now we have to bolt extra complexity
on top.
I have the feeling this is not the best approach, especially since many
long delta chains are going deep and all those intermediate objects are
simply useless once the _only_ delta child has been resolved and
therefore could be freed right away instead.
But I have to sleep on it first.
Nicolas
^ permalink raw reply
* Re: [PATCH 2/3] add new Git::Repo API
From: Lea Wiemann @ 2008-07-14 2:29 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git, John Hawley, Petr Baudis
In-Reply-To: <200807140128.44923.jnareb@gmail.com>
Jakub Narebski wrote:
> I think it would be perhaps better to explain relationship and purpose
> of each class in more detail, including Git::Repo.
Noted, will do.
>> [Git.pm] tries to do (a) WC access, (b) repo access,
>> and (c) frontend error handling (with sensible error messages).
>
> I can see (b) and (c), but I have trouble seeing (a).
Well, Git.pm operates on working copies in the constructor (obviously),
but also wc_{path,subdir,chdir} and hash_and_insert_object.
>> every working copy has a repository associated with it
>
> Please remember that the opposite relation is also true.
True. *nods*
>> but I'd probably let [Git.pm] die a slow death
>
> I'm not so sure if it is a way to go. Most git commands wants to just
> invoke other git commands safely,
Good point. Perhaps the command functionality of Git.pm and Git::Repo
could be extracted into something like Git::Cmd.
> Non OO things, like ability to write print color('reset') . "\n";
> is also important.
Perhaps, though you might not get around some instantiation to specify
the semantics of the color command: Honor color configuration in
.gitconfig or .git/config? Honor non-terminal stdout? Honor command
line? I suspect that in the end non-OO functions end up being wrappers
around OO interfaces that simply specify a set of reasonable defaults.
> I'm not sure if using Error module was a good idea for
> frontend error handling.
As a general rule, I'd try to not use program exceptions as a means to
do frontend error handling, unless you're trying hard to keep the
frontend minimalist. Even if you don't care about i18n, different
frontends have different needs for their error reporting styles. Also,
things like failed SHA1-lookups might be an error to one frontend but
not an error to another frontend, so you'd have to implement an
exception hierarchy to make fine-granular catching possible.
On top of that, this kind of exception handling doesn't seem very much
like typical Perl style.
> How would you like to catch errors from frontend in Git::Repo and
> friends?
Handle them yourself -- Git::Repo doesn't die unless a fatal (i.e.
unexpected) error occurs:
($sha1, $type) = $repo->get_sha1('HEAD:/my/file');
if (! defined $sha1 || $type ne 'blob') { ... handle error ... }
$contents = $repo->cat_file($sha1);
... work with contents ...
Also note how there's one well-defined (and known) error point: $sha1
being undefined, or the $type being wrong. The $repo methods *cannot*
throw errors unless they're fatal, so you can for instance call cat_file
and assume that everything goes right.
> What is max_exit_code
It allows you call the git binary without dying if it exits with
non-zero status; see the cmd_output documentation for details.
The idea is that a non-zero exit status always indicates an internal
(fatal) error, unless you specify that it's OK.
>> - It's buggy and untested. Neither of these is a problem by itself,
>> but the combination is deadly.
>
> Haven't you added t/t9700-perl-git.sh?
Yes (and it alleviated the problem), but I couldn't test the areas where
the untestedness actually hits (e.g. the missing semicolon I mentioned).
IOW, t9700 is only testing the parts that are working anyway.
> What I worry about is that dependence on Git.pm or Git::Repo would make
> gitweb installation too hard for some.
If I'm not mistaken you can always drop the perl/Git directory next to
gitweb.cgi. (I'll add that to the installation notes.)
>> Unrelatedly, should I add copyright notices at the bottom of each perl
>> module so they are displayed in the perldoc/man pages?
>
> Well, most manpages have information about who made them... which means
> who was initial author, usually, and/or who is current maintainer.
I don't really care about being credited as the initial author, and I'm
honestly not sure if I'll be able to maintain the modules in the long run.
Should I perhaps add some note along the lines of "Direct questions and
patches to git@vger"?
^ 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