* 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
* [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 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
* 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 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: [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 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: [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: [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: 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: Git describe question
From: Junio C Hamano @ 2008-07-14 23:17 UTC (permalink / raw)
To: Jean-Luc Herren; +Cc: Mark Burton, git
In-Reply-To: <487BD34F.4080201@gmx.ch>
Jean-Luc Herren <jlh@gmx.ch> writes:
> Mark Burton wrote:
> ...
> I don't think people usually say "tag X is on branch Y", excepted
> maybe if Y has never been merged anywhere. Specifically, nobody
> would say v1.5.6.3 is *on* branch master. But it's part of its
> history. v1.5.6.3 is *on* maint, at best.
Actually I am somewhat sympathetic to Mark here. Probably what he wants
is to describe 10ce020 as v1.5.6-390-10ce020.
While that probably is doable by using the first-parent-only traversal, I
do not think it is such a good idea. It is not how branches in git are
designed to work. As Merlyn always says in #git at freenode, a branch is
an illusion, and it is especially true in the presense of fast-forward
merge (aka the upstream maintainer asking a subsystem lieutenant to do a
merge for him).
^ permalink raw reply
* Re: [PATCH 3/3] gitweb: use new Git::Repo API, and add optional caching
From: Jakub Narebski @ 2008-07-14 23:14 UTC (permalink / raw)
To: Lea Wiemann; +Cc: git, John Hawley, Petr Baudis
In-Reply-To: <487BDB34.7010002@gmail.com>
Lea Wiemann wrote:
> Jakub Narebski wrote:
> > Note that if caching is enabled, you can set expires to either
> > time-to-expire of cache entries (simpler), or time left to live to
> > invalidation of item in cache (better, but more complicated)
>
> Gitweb's cache is actually never out-of-date, and cache invalidation
> happens automatically. It uses some (long) expiry times to guard
> against non-standard modification of the repository, but it's nothing
> the HTTP client should be concerned with.
Could you explain then how gitweb cache is invalidated?
The _reasoning_ behind setting Expires:/Cache-Control: max-age= header
for gitweb with caching is that browser doesn't need to even try to
cache-validate or access page for the time we *know* that output would
not change[*1*] because it is from the cache.
[*1*] In significant way: changing relative dates/time doesn't count.
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: What's cooking in git.git (topics)
From: Lea Wiemann @ 2008-07-14 23:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vtzetjbif.fsf@gitster.siamese.dyndns.org>
Junio C Hamano wrote:
> * lw/gitweb (Fri Jul 11 03:11:48 2008 +0200) 3 commits
> - Add new Git::Repo API
>
> This does not pass t9710, at least for me X-<.
Yikes; I thought I had removed all instanced of Carp::Always (which I
had put in for development), but this one apparently slipped through.
It'll be fixed in the next version I post (which will also have the
dependency on the non-core Test::Exception package removed).
^ permalink raw reply
* Re: [BUG] commit walk machinery is dangerous !
From: Nicolas Pitre @ 2008-07-14 23:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vbq10f7wr.fsf@gitster.siamese.dyndns.org>
On Mon, 14 Jul 2008, Junio C Hamano wrote:
> Nicolas Pitre <nico@cam.org> writes:
>
> > However this time a corruption turned up and exposed what I think is a
> > major flaw in git's error checking. To demonstrate it, I created the
> > following test case. Turning the error() into a die() on line 772 of
> > commit.c makes this test pass but I don't know if this is the
> > appropriate fix (e.g. some attempt to parse non existing commits could
> > be valid usage, etc.). Note this is critical only for git versions
> > later than commit 8eca0b47ff15.
>
> Which probably means we should revert that commit as faulty? IIRC, before
> that commit we did check and error out correctly but you loosened the
> check to introduce "a major flaw" with that commit.
>
> $ for b in maint master next pu
> do
> echo -n $b; git cat-file blob $b:commit.c | wc -l
> done
> maint 672
> master 672
> next 779
> pu 789
>
> Hmph...
Well, most of them aren't that critical. If anything they will only
cause a segfault if ever the return value is not checked.
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.
Nicolas
^ permalink raw reply
* Re: [PATCH 3/3] gitweb: use new Git::Repo API, and add optional caching
From: Lea Wiemann @ 2008-07-14 23:03 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git, John Hawley, Petr Baudis
In-Reply-To: <200807142323.22761.jnareb@gmail.com>
Jakub Narebski wrote:
> It was suggested to split this into separate commit
Yup; I'll probably send updated patches tomorrow night (also for patch 2/3).
>> - gitweb will check for parameter correctness more aggressively,
>
> I understand that this change deals with treating invalid specifiers,
> which point to either object that do not exists, are ambiguous, or point
> to object of invalid type.
Yes, that's right. (I don't believe we have any point where ambiguity
might come up though.)
>> - Empty projects: [...]
>
> Good. The only thing that *might* be controversial is putting empty
> projects at the bottom of sorted by age (by last change) projects list,
> instead of at top.
Yup; let's see if anyone objects though. If I sort the list by "Last
Change", I usually want to see projects with recent activity, not dead
project, at the top, which is why I changed it (since I was touching
that line anyway).
>> - For HTML pages, remove the "Expires" HTTP response header, and add
>> "Cache-Control: no-cache" instead. This is because pages can
>> contain dynamic content (like the subject of the latest commit)
>
> I don't think it is a good change.
Hm; I thought transient titles could slip in (e.g. try opening the tree
of some commit and remove the hb parameter; the URL will seem cacheable,
but the page contains the title of the HEAD commit), but I can't find
any URL right now where mainline actually sets a wrong Expires header.
I'll look into it; if you don't see me posting about it again I'll
re-add the Expires header.
> Note that if caching is enabled, you can set expires to either
> time-to-expire of cache entries (simpler), or time left to live to
> invalidation of item in cache (better, but more complicated)
Gitweb's cache is actually never out-of-date, and cache invalidation
happens automatically. It uses some (long) expiry times to guard
against non-standard modification of the repository, but it's nothing
the HTTP client should be concerned with.
>> $cache = Cache::Memcached->new( { servers => ['localhost:11211'],
>
> IIRC you can use any Cache::Cache compatibile cache here;
> IMVHO it would be nice if this info would be also in commit message.
I'll add that.
>> $large_cache_root = '/home/lewiemann/gitweb-cache';
>
> Errr... I understand that it is your _private_ configuration, just
> copied here verbatim, but I don't think '/home/lewiemann/gitweb-cache'
> is a good example: '/tmp/gitweb-cache' perhaps, that I can understand.
Yup. ;-) Or /var/cache/gitweb.
>> # Invalidate cache on changes to gitweb without version number bump;
>> # useful for development.
>> $cache_key = (stat '/home/lewiemann/gitweb')[9] .
>> (stat '/home/lewiemann/gitweb/gitweb.cgi')[9];
>
> What should be used in production? "$cache_key = $version;"?
No, nothing. $version is used automatically as a cache key; I'll add
that to the documentation for $cache_key.
> You can always use $ENV{'SCRIPT_FILENAME'}, or dirname of it.
That one doesn't exist with my thttpd, or any other environment variable
that'd be usable. It's just a hack anyway, so hardcoded paths are OK.
:) I don't think gitweb should check its own mtime by default.
>> # Display detailed cache info at the bottom of each page.
>> $page_info = 2;
>
> Errr... what does "$page_info = <n>;" mean?
Display no (0) / short (1) / detailed (2) page (cache) info at the
bottom of each page. It's documented in gitweb.perl.
> [Comments on patch itself in separate email, later]
Thanks!
^ permalink raw reply
* Re: Git describe question
From: Jean-Luc Herren @ 2008-07-14 22:29 UTC (permalink / raw)
To: Mark Burton; +Cc: Junio C Hamano, git
In-Reply-To: <20080714092040.4090046b@crow>
Mark Burton wrote:
> Ok, I understand what it's doing now - but that makes me wonder if it
> would be useful/possible to be able to specify that git describe only
> considers the commits on top of the tag for the current (or some
> specified branch). i.e. at the moment, gitk shows 8 commits on top of
> v1.5.6.3 in the master branch.
Are you saying "git describe" should output v1.5.6.3-8-g10ce020?
That would be misleading and even wrong. This would be like
saying that there are only 8 commits of difference between
v1.5.6.3 and 10ce20, which is not true. The differences between
those two commits are over 300 single commits and that's also what
"git (log|diff) v1.5.6.3..10ce20" will tell you. All 300+ commits
that have been made to the branch master since the branch maint
forked off are part of 10ce20, but not part of v1.5.6.3. It seems
fine to me that "git describe" reflects this difference.
> As the master branch is the checked out branch and the v1.5.6.3 tag
> tags a commit in that branch (via the merge) [...]
I don't think people usually say "tag X is on branch Y", excepted
maybe if Y has never been merged anywhere. Specifically, nobody
would say v1.5.6.3 is *on* branch master. But it's part of its
history. v1.5.6.3 is *on* maint, at best.
jlh
^ permalink raw reply
* Re: [PATCH 2/3] add new Git::Repo API
From: Lea Wiemann @ 2008-07-14 22:19 UTC (permalink / raw)
To: Petr Baudis; +Cc: git, John Hawley, Jakub Narebski
In-Reply-To: <20080714014051.GK10151@machine.or.cz>
Petr Baudis wrote:
> Maybe I sound too mentoring at places; [OTOH] 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.
Yup, I agree (and you don't sound too mentoring ^^). Thanks for the
review and feedback.
> I really miss some more detailed writeup on the envisioned design here. [...]
> Most importantly, how is Git::Repo interacting with working copies,
Git::Repo is not interacting with working copies at all right now. Is
there anything you think should be considered for its design?
Here's a write-up about the design (I'll probably move this into
Git::Repo's man page):
----------
Git::Repo aims to provide low-level access to Git repositories. For
instance, you can resolve object names (like "HEAD~2") to SHA1s, and
inspect objects. It does not attempt to be a wrapper around the git
plumbing or porcelain commands.
Error handling is simple: On a consistent repository, the Perl interface
will never die. You can use the get_sha1 method to resolve arbitrary
object names or check the existence of SHA1 hashes; get_sha1 will return
undef if the object does not exist in the repository. Any SHA1 that is
returned by get_sha1 can be safely passed to the other Git::Repo methods.
----------
> 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.
Well, they're more like helper methods. Since they don't fit into the
design goals of the Git::Repo API at all, I'd suggest we just
underscore-prefix them and take them out of the man page. (The only
reason why I hadn't done this is that gitweb uses $repo->cmd_output
extensively, so it'd end up with a lot of underscore calls. But I
suppose we can either alias _cmd_output to cmd_output in gitweb's
CachedRepo subclass, or live with $repo->_cmd_output calls.) Does
underscore-prefixing sound good to you?
If someone wants to come up with a consistent nice interface for calling
git commands, sure. I wasn't actually trying to do that.
> Here is an idea: Introduce Git::Command object [and a Git::CommandFactory]
>
> my $c = Git::Command->new(['git', '--git-dir=.', 'cat-file', \
> '-p', 'bla'], {pipe_out=>1}) [...]
> my $cf = Git::CommandFactory->new('git', '--git-dir=.');
>
> Or am I overdoing it?
Yes, I think so. ;-) All we're talking about here is a wrapper around
"open '-|'" calls (plus some workarounds for Windows I suppose).
I don't have much of a notion of a 'command' as an object in my head;
your (snipped) example makes it look like you're trying to create a
IO::Handle-compatible interface, which I think is way too much effort
(and error-prone) for simple pipes. Also, a CommandFactory class just
to catenate lists together seems like overkill to me.
Something like a command interface *may* make sense if it's tied to
repositories or working copies, in which case it could automatically set
--git-dir or --work-tree, but it's beyond the scope of what I'm trying
to create here, and I don't think it's even overly useful.
> 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.
Well, as I said, the fact that there are untested parts in Git.pm
doesn't exactly make it trivial to refactor. Also, only very few parts
can be cleanly moved to Git::Repo. In "grep '^sub [^_]' Git.pm" I find
only cat_blob and perhaps hash_object to be eligible to be moved (though
hash_object should probably live in a working-copy/non-bare-repo class,
with an optional insert => 1 parameter). And even cat_blob is
non-trivial to move unless you want the whole blob to be read into memory.
That's a lot of non-trivialness for very little gain. I doubt I'd even
have enough time till the end of GSoC (minus vacation) to do this.
> It should be actually very easy to start with moving all the pipe
> functionality to Git::Command.
Creating a new (Git::Command) API is very much non-trivial, apart from
the fact that I'm not convinced that we need Git::Command, and that a
clean command interface neither falls out of Git.pm nor Git::Repo.
>> 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!"
Sorry, I wasn't clear with my parenthesed remark: I actually think that
Git::Repo is pretty good in terms of code and interface quality. It's
just not *complete*, even in its limited scope, and I'm not attempting
to make it complete.
I do think that someone who wants to extend Git::Repo (like Jakub with
Git::Config) won't have much trouble doing so with the existing design.
>> +use constant _MESSAGE => 'M';
>> +use constant _ENCODING => 'E'; [snip]
>
> if you are going to use hashes
> anyway, why not actually key by sensible name directly?
Embarrassingly premature optimization here. ^_^ I'll fix it.
>> $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.
*ponders* Every commit needs a repo and a SHA1, so those will never get
optional. We can always add hash-based options after the two mandatory
arguments, but I don't even see any such possible options at the moment.
(And if I turn out to be completely wrong, we can even move to a
hash-only argument list by checking the type of the first parameter.)
Really, I wouldn't worry.
>> [Git::Commit->new, Git::Tag->new:]
>> +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.
No, you should never pass in an invalid SHA1 in the first place. The
above piece of documentation is just a warning that bugs will show up
delayed. IOW, this is not the right place to have your error handling.
If you're getting a SHA1 through the user-interface, check its existence
with get_sha1 before passing it to the constructor.
>> +Note that $sha1 must be the SHA1 of a commit object; tag objects are
>> +not dereferenced.
>
> Why not?
Because the SHA1 might resolve to an object of the wrong type, which
means you have to do error handling in Git::Object objects; that's the
wrong place.
If tag-resolving is really needed, we can add an optional $type
parameter to get_sha1, which will cause get_sha1 to resolve the object
until a $type object is found, or return undef if the object is or
resolves to an object of the wrong type.
I have resolving code in gitweb's git_get_sha1_or_die (which I didn't
implement in Git::Repo since it uses some customized error reporting).
The resolving code could conceivably be extracted and moved to get_sha1.
I think there are a few things to ponder and maybe discuss, so I'd do
that in a separate patch (if I get around it before the end of the project).
>> +=item $commit->authors
>
> author
Fixed.
>> +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.
I've replaced it with: "Objects are loaded lazily, and hence
instantiation is free. Git::Object instances stringify to their SHA1s."
>> +sub sha1 {
>> +sub stringify {
>
> Why not overload "" directly to sha1()?
Done (and removed stringify).
>> +sub assert_opts {
>> +sub assert_sha1 {
>
> Pretend names with underscores, since they are internal?
Done, and removed them from @EXPORT_OK.
>> 'directory': The directory of the repository (mandatory).
>
> 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.
Sure, it can be made non-mandatory if it's needed, but there are so many
possibilities for the exact time and place at which the repo directory
should be resolved using rev-parse (if at all) that I'd rather leave
this to the person who has an actual use-case for it. I'm not a fan of
designing APIs before they are needed.
>> [Snipped a lot of quoting --LW]
>> +=item $repo->repo_dir
>> +=item $repo->git_binary
>> +=item $repo->version
>> +sub _get_git_cmd {
>
> This definitely does not belong to a Git::Repo object.
Which of those methods are you referring to? I think $repo->version
might reasonably be removed (and the code re-added to gitweb); I'll do
so unless you object. _get_git_cmd is already underscored, and repo_dir
and git_binary only access attributes passed in through the constructor,
so I think those three should stay.
>> +=item $repo->cat_file($sha1)
>
> 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().
Yup; I like get_object (I think I was planning to rename it and then
didn't remember doing so before sending off the patch). Will rename it.
>> +=item $repo->get_path($tree_sha1, $file_sha1)
>
> Now we are quickly getting messy again. This should definitely live in
> Git::Tree.
Yup, that's true. I'll move it into gitweb until we have Git::Tree
(with a comment that it can be moved to Git::Tree once it exists).
>> +=item $repo->get_refs
>> +=item $repo->get_refs($pattern)
>
> Again, the refs should be properly integrated into the object structure.
Really? I think it's generally fine for get_refs to exist and to live
in Git::Repo.
Its return value (currently an an arrayref of [$sha1, $object_type,
$ref_name] arrayrefs) might need improvement though, and I find the
$pattern parameter pretty suspect (in that it smells like a for-each-ref
wrapper). Since get_refs is unused at the moment (gitweb ended up
needing the slightly different show-ref), I'll remove it for now. (Same
thing about me not being a fan of premature API design applies.)
I keep patches of everything I remove so other people will be able to
use them as starting points; I'll probably post them as "FYI"-patches to
the list at the end of my project, to preserve them for posterity.
^ permalink raw reply
* Re: [BUG] commit walk machinery is dangerous !
From: Junio C Hamano @ 2008-07-14 21:55 UTC (permalink / raw)
To: Nicolas Pitre; +Cc: git
In-Reply-To: <alpine.LFD.1.10.0807141641110.12484@xanadu.home>
Nicolas Pitre <nico@cam.org> writes:
> However this time a corruption turned up and exposed what I think is a
> major flaw in git's error checking. To demonstrate it, I created the
> following test case. Turning the error() into a die() on line 772 of
> commit.c makes this test pass but I don't know if this is the
> appropriate fix (e.g. some attempt to parse non existing commits could
> be valid usage, etc.). Note this is critical only for git versions
> later than commit 8eca0b47ff15.
Which probably means we should revert that commit as faulty? IIRC, before
that commit we did check and error out correctly but you loosened the
check to introduce "a major flaw" with that commit.
$ for b in maint master next pu
do
echo -n $b; git cat-file blob $b:commit.c | wc -l
done
maint 672
master 672
next 779
pu 789
Hmph...
^ permalink raw reply
* [PATCH 5/5] Allow add_path() to add non-existent directories to the path
From: Johannes Sixt @ 2008-07-14 21:41 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Steffen Prohaska, git, Johannes Sixt
In-Reply-To: <1216071689-14823-5-git-send-email-johannes.sixt@telecom.at>
This function had used make_absolute_path(); but this function dies if
the directory that contains the entry whose relative path was supplied in
the argument does not exist. This is a problem if the argument is, for
example, "../libexec/git-core", and that "../libexec" does not exist.
Since the resolution of symbolic links is not required for elements in
PATH, we can fall back to using make_nonrelative_path(), which simply
prepends $PWD to the path.
We have to move make_nonrelative_path() alongside make_absolute_path() in
abspath.c so that git-shell can be linked. See 5b8e6f85f.
Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
abspath.c | 36 ++++++++++++++++++++++++++++++++++++
exec_cmd.c | 2 +-
path.c | 36 ------------------------------------
3 files changed, 37 insertions(+), 37 deletions(-)
diff --git a/abspath.c b/abspath.c
index 4f95a95..0d56124 100644
--- a/abspath.c
+++ b/abspath.c
@@ -66,3 +66,39 @@ const char *make_absolute_path(const char *path)
return buf;
}
+
+static const char *get_pwd_cwd(void)
+{
+ static char cwd[PATH_MAX + 1];
+ char *pwd;
+ struct stat cwd_stat, pwd_stat;
+ if (getcwd(cwd, PATH_MAX) == NULL)
+ return NULL;
+ pwd = getenv("PWD");
+ if (pwd && strcmp(pwd, cwd)) {
+ stat(cwd, &cwd_stat);
+ if (!stat(pwd, &pwd_stat) &&
+ pwd_stat.st_dev == cwd_stat.st_dev &&
+ pwd_stat.st_ino == cwd_stat.st_ino) {
+ strlcpy(cwd, pwd, PATH_MAX);
+ }
+ }
+ return cwd;
+}
+
+const char *make_nonrelative_path(const char *path)
+{
+ static char buf[PATH_MAX + 1];
+
+ if (is_absolute_path(path)) {
+ if (strlcpy(buf, path, PATH_MAX) >= PATH_MAX)
+ die("Too long path: %.*s", 60, path);
+ } else {
+ const char *cwd = get_pwd_cwd();
+ if (!cwd)
+ die("Cannot determine the current working directory");
+ if (snprintf(buf, PATH_MAX, "%s/%s", cwd, path) >= PATH_MAX)
+ die("Too long path: %.*s", 60, path);
+ }
+ return buf;
+}
diff --git a/exec_cmd.c b/exec_cmd.c
index c236034..0ed768d 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -50,7 +50,7 @@ static void add_path(struct strbuf *out, const char *path)
if (is_absolute_path(path))
strbuf_addstr(out, path);
else
- strbuf_addstr(out, make_absolute_path(path));
+ strbuf_addstr(out, make_nonrelative_path(path));
strbuf_addch(out, PATH_SEP);
}
diff --git a/path.c b/path.c
index 504eae0..9df447b 100644
--- a/path.c
+++ b/path.c
@@ -291,42 +291,6 @@ int adjust_shared_perm(const char *path)
return 0;
}
-static const char *get_pwd_cwd(void)
-{
- static char cwd[PATH_MAX + 1];
- char *pwd;
- struct stat cwd_stat, pwd_stat;
- if (getcwd(cwd, PATH_MAX) == NULL)
- return NULL;
- pwd = getenv("PWD");
- if (pwd && strcmp(pwd, cwd)) {
- stat(cwd, &cwd_stat);
- if (!stat(pwd, &pwd_stat) &&
- pwd_stat.st_dev == cwd_stat.st_dev &&
- pwd_stat.st_ino == cwd_stat.st_ino) {
- strlcpy(cwd, pwd, PATH_MAX);
- }
- }
- return cwd;
-}
-
-const char *make_nonrelative_path(const char *path)
-{
- static char buf[PATH_MAX + 1];
-
- if (is_absolute_path(path)) {
- if (strlcpy(buf, path, PATH_MAX) >= PATH_MAX)
- die ("Too long path: %.*s", 60, path);
- } else {
- const char *cwd = get_pwd_cwd();
- if (!cwd)
- die("Cannot determine the current working directory");
- if (snprintf(buf, PATH_MAX, "%s/%s", cwd, path) >= PATH_MAX)
- die ("Too long path: %.*s", 60, path);
- }
- return buf;
-}
-
const char *make_relative_path(const char *abs, const char *base)
{
static char buf[PATH_MAX + 1];
--
1.5.6.3.323.g1e58
^ permalink raw reply related
* [PATCH 4/5] Allow the built-in exec path to be relative to the command invocation path
From: Johannes Sixt @ 2008-07-14 21:41 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Steffen Prohaska, git, Johannes Sixt
In-Reply-To: <1216071689-14823-4-git-send-email-johannes.sixt@telecom.at>
If GIT_EXEC_PATH (the macro that is defined in the Makefile) is relative,
it is interpreted relative to the command's invocation path, which usually
is $(bindir).
The Makefile rules were written with the assumption that $(gitexecdir) is
an absolute path. We introduce a separate variable that names the
(absolute) installation directory.
Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
Makefile | 15 +++++++++++----
exec_cmd.c | 38 ++------------------------------------
2 files changed, 13 insertions(+), 40 deletions(-)
diff --git a/Makefile b/Makefile
index b9ea0ea..4df6423 100644
--- a/Makefile
+++ b/Makefile
@@ -1307,10 +1307,17 @@ template_instdir = $(template_dir)
endif
export template_instdir
+ifeq ($(firstword $(subst /, ,$(gitexecdir))),..)
+gitexec_instdir = $(bindir)/$(gitexecdir)
+else
+gitexec_instdir = $(gitexecdir)
+endif
+gitexec_instdir_SQ = $(subst ','\'',$(gitexec_instdir))
+
install: all
$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(bindir_SQ)'
- $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(gitexecdir_SQ)'
- $(INSTALL) $(ALL_PROGRAMS) '$(DESTDIR_SQ)$(gitexecdir_SQ)'
+ $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
+ $(INSTALL) $(ALL_PROGRAMS) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
$(INSTALL) git$X git-upload-pack$X git-receive-pack$X git-upload-archive$X '$(DESTDIR_SQ)$(bindir_SQ)'
$(MAKE) -C templates DESTDIR='$(DESTDIR_SQ)' install
$(MAKE) -C perl prefix='$(prefix_SQ)' DESTDIR='$(DESTDIR_SQ)' install
@@ -1319,10 +1326,10 @@ ifndef NO_TCLTK
$(MAKE) -C git-gui install
endif
ifneq (,$X)
- $(foreach p,$(patsubst %$X,%,$(filter %$X,$(ALL_PROGRAMS) $(BUILT_INS) git$X)), $(RM) '$(DESTDIR_SQ)$(gitexecdir_SQ)/$p';)
+ $(foreach p,$(patsubst %$X,%,$(filter %$X,$(ALL_PROGRAMS) $(BUILT_INS) git$X)), $(RM) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)/$p';)
endif
bindir=$$(cd '$(DESTDIR_SQ)$(bindir_SQ)' && pwd) && \
- execdir=$$(cd '$(DESTDIR_SQ)$(gitexecdir_SQ)' && pwd) && \
+ execdir=$$(cd '$(DESTDIR_SQ)$(gitexec_instdir_SQ)' && pwd) && \
if test "z$$bindir" != "z$$execdir"; \
then \
ln -f "$$bindir/git$X" "$$execdir/git$X" || \
diff --git a/exec_cmd.c b/exec_cmd.c
index 45f92eb..c236034 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -7,40 +7,6 @@ extern char **environ;
static const char *argv_exec_path;
static const char *argv0_path;
-static const char *builtin_exec_path(void)
-{
-#ifndef __MINGW32__
- return GIT_EXEC_PATH;
-#else
- int len;
- char *p, *q, *sl;
- static char *ep;
- if (ep)
- return ep;
-
- len = strlen(_pgmptr);
- if (len < 2)
- return ep = ".";
-
- p = ep = xmalloc(len+1);
- q = _pgmptr;
- sl = NULL;
- /* copy program name, turn '\\' into '/', skip last part */
- while ((*p = *q)) {
- if (*q == '\\' || *q == '/') {
- *p = '/';
- sl = p;
- }
- p++, q++;
- }
- if (sl)
- *sl = '\0';
- else
- ep[0] = '.', ep[1] = '\0';
- return ep;
-#endif
-}
-
const char *system_path(const char *path)
{
if (!is_absolute_path(path) && argv0_path) {
@@ -75,7 +41,7 @@ const char *git_exec_path(void)
return env;
}
- return builtin_exec_path();
+ return system_path(GIT_EXEC_PATH);
}
static void add_path(struct strbuf *out, const char *path)
@@ -99,7 +65,7 @@ void setup_path(void)
add_path(&new_path, argv_exec_path);
add_path(&new_path, getenv(EXEC_PATH_ENVIRONMENT));
- add_path(&new_path, builtin_exec_path());
+ add_path(&new_path, system_path(GIT_EXEC_PATH));
add_path(&new_path, argv0_path);
if (old_path)
--
1.5.6.3.323.g1e58
^ permalink raw reply related
* [PATCH 2/5] Record the command invocation path early
From: Johannes Sixt @ 2008-07-14 21:41 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Steffen Prohaska, git, Johannes Sixt
In-Reply-To: <1216071689-14823-2-git-send-email-johannes.sixt@telecom.at>
We will need the command invocation path in system_path(). This path was
passed to setup_path(), but system_path() can be called earlier, for
example via:
main
commit_pager_choice
setup_pager
git_config
git_etc_gitconfig
system_path
Therefore, we introduce git_set_argv0_path() and call it as soon as
possible.
Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
exec_cmd.c | 10 ++++++++--
exec_cmd.h | 3 ++-
git.c | 5 ++---
receive-pack.c | 2 +-
shell.c | 4 ++--
upload-pack.c | 2 +-
6 files changed, 16 insertions(+), 10 deletions(-)
diff --git a/exec_cmd.c b/exec_cmd.c
index 8899e31..dedb01d 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -5,6 +5,7 @@
extern char **environ;
static const char *argv_exec_path;
+static const char *argv0_path;
static const char *builtin_exec_path(void)
{
@@ -50,6 +51,11 @@ const char *system_path(const char *path)
return path;
}
+void git_set_argv0_path(const char *path)
+{
+ argv0_path = path;
+}
+
void git_set_argv_exec_path(const char *exec_path)
{
argv_exec_path = exec_path;
@@ -84,7 +90,7 @@ static void add_path(struct strbuf *out, const char *path)
}
}
-void setup_path(const char *cmd_path)
+void setup_path(void)
{
const char *old_path = getenv("PATH");
struct strbuf new_path;
@@ -94,7 +100,7 @@ void setup_path(const char *cmd_path)
add_path(&new_path, argv_exec_path);
add_path(&new_path, getenv(EXEC_PATH_ENVIRONMENT));
add_path(&new_path, builtin_exec_path());
- add_path(&new_path, cmd_path);
+ add_path(&new_path, argv0_path);
if (old_path)
strbuf_addstr(&new_path, old_path);
diff --git a/exec_cmd.h b/exec_cmd.h
index 7eb94e5..0c46cd5 100644
--- a/exec_cmd.h
+++ b/exec_cmd.h
@@ -2,8 +2,9 @@
#define GIT_EXEC_CMD_H
extern void git_set_argv_exec_path(const char *exec_path);
+extern void git_set_argv0_path(const char *path);
extern const char* git_exec_path(void);
-extern void setup_path(const char *);
+extern void setup_path(void);
extern int execv_git_cmd(const char **argv); /* NULL terminated */
extern int execl_git_cmd(const char *cmd, ...);
extern const char *system_path(const char *path);
diff --git a/git.c b/git.c
index 7075533..b90c358 100644
--- a/git.c
+++ b/git.c
@@ -470,7 +470,6 @@ int main(int argc, const char **argv)
{
const char *cmd = argv[0] && *argv[0] ? argv[0] : "git-help";
char *slash = (char *)cmd + strlen(cmd);
- const char *cmd_path = NULL;
int done_alias = 0;
/*
@@ -483,7 +482,7 @@ int main(int argc, const char **argv)
while (cmd <= slash && !is_dir_sep(*slash));
if (cmd <= slash) {
*slash++ = 0;
- cmd_path = cmd;
+ git_set_argv0_path(cmd);
cmd = slash;
}
@@ -527,7 +526,7 @@ int main(int argc, const char **argv)
* environment, and the $(gitexecdir) from the Makefile at build
* time.
*/
- setup_path(cmd_path);
+ setup_path();
while (1) {
/* See if it's an internal command */
diff --git a/receive-pack.c b/receive-pack.c
index fa653b4..d44c19e 100644
--- a/receive-pack.c
+++ b/receive-pack.c
@@ -482,7 +482,7 @@ int main(int argc, char **argv)
if (!dir)
usage(receive_pack_usage);
- setup_path(NULL);
+ setup_path();
if (!enter_repo(dir, 0))
die("'%s': unable to chdir or not a git archive", dir);
diff --git a/shell.c b/shell.c
index 91ca7de..6a48de0 100644
--- a/shell.c
+++ b/shell.c
@@ -15,7 +15,7 @@ static int do_generic_cmd(const char *me, char *arg)
{
const char *my_argv[4];
- setup_path(NULL);
+ setup_path();
if (!arg || !(arg = sq_dequote(arg)))
die("bad argument");
if (prefixcmp(me, "git-"))
@@ -37,7 +37,7 @@ static int do_cvs_cmd(const char *me, char *arg)
if (!arg || strcmp(arg, "server"))
die("git-cvsserver only handles server: %s", arg);
- setup_path(NULL);
+ setup_path();
return execv_git_cmd(cvsserver_argv);
}
diff --git a/upload-pack.c b/upload-pack.c
index 9f82941..c911e70 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -638,7 +638,7 @@ int main(int argc, char **argv)
if (i != argc-1)
usage(upload_pack_usage);
- setup_path(NULL);
+ setup_path();
dir = argv[i];
--
1.5.6.3.323.g1e58
^ permalink raw reply related
* [PATCH 3/5] Fix relative built-in paths to be relative to the command invocation
From: Johannes Sixt @ 2008-07-14 21:41 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Steffen Prohaska, git, Johannes Sixt
In-Reply-To: <1216071689-14823-3-git-send-email-johannes.sixt@telecom.at>
$(gitexecdir) (as defined in the Makefile) has gained another path
component, but the relative paths in the MINGW section of the Makefile,
which are interpreted relative to it, do not account for it.
Instead of adding another ../ in front of the path, we change the code that
constructs the absolute paths to do it relative to the command's directory,
which is essentially $(bindir). We do it this way because we will also
allow a relative $(gitexecdir) later.
Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
Makefile | 2 +-
exec_cmd.c | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/Makefile b/Makefile
index 4de9271..b9ea0ea 100644
--- a/Makefile
+++ b/Makefile
@@ -1301,7 +1301,7 @@ remove-dashes:
### Installation rules
ifeq ($(firstword $(subst /, ,$(template_dir))),..)
-template_instdir = $(gitexecdir)/$(template_dir)
+template_instdir = $(bindir)/$(template_dir)
else
template_instdir = $(template_dir)
endif
diff --git a/exec_cmd.c b/exec_cmd.c
index dedb01d..45f92eb 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -43,9 +43,9 @@ static const char *builtin_exec_path(void)
const char *system_path(const char *path)
{
- if (!is_absolute_path(path)) {
+ if (!is_absolute_path(path) && argv0_path) {
struct strbuf d = STRBUF_INIT;
- strbuf_addf(&d, "%s/%s", git_exec_path(), path);
+ strbuf_addf(&d, "%s/%s", argv0_path, path);
path = strbuf_detach(&d, NULL);
}
return path;
--
1.5.6.3.323.g1e58
^ permalink raw reply related
* [PATCH 0/5] replacement for the part of js/more-win that is in pu
From: Johannes Sixt @ 2008-07-14 21:41 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Steffen Prohaska, git, Johannes Sixt
In-Reply-To: <1216025226.487b128a031fd@webmail.eunet.at>
The interdiff to js/more-win is below. It is mostly the changes
of 1/5.
Johannes Sixt (5):
Makefile: Normalize $(bindir) and $(gitexecdir) before comparing
Record the command invocation path early
Fix relative built-in paths to be relative to the command
invocation
Allow the built-in exec path to be relative to the command
invocation path
Allow add_path() to add non-existent directories to the path
Makefile | 33 +++++++++++++++++-----------
abspath.c | 36 ++++++++++++++++++++++++++++++
exec_cmd.c | 54 +++++++++++----------------------------------
exec_cmd.h | 3 +-
git.c | 5 +--
path.c | 36 ------------------------------
receive-pack.c | 2 +-
shell.c | 4 +-
upload-pack.c | 2 +-
9 files changed, 77 insertions(+), 98 deletions(-)
diff --git a/Makefile b/Makefile
index 3593e6f..4df6423 100644
--- a/Makefile
+++ b/Makefile
@@ -1301,14 +1301,14 @@ remove-dashes:
### Installation rules
ifeq ($(firstword $(subst /, ,$(template_dir))),..)
-template_instdir = $(shell cd '$(bindir_SQ)/$(template_dir_SQ)' && pwd)
+template_instdir = $(bindir)/$(template_dir)
else
template_instdir = $(template_dir)
endif
export template_instdir
ifeq ($(firstword $(subst /, ,$(gitexecdir))),..)
-gitexec_instdir = $(shell cd '$(bindir_SQ)/$(gitexecdir_SQ)' && pwd)
+gitexec_instdir = $(bindir)/$(gitexecdir)
else
gitexec_instdir = $(gitexecdir)
endif
@@ -1325,18 +1325,18 @@ ifndef NO_TCLTK
$(MAKE) -C gitk-git install
$(MAKE) -C git-gui install
endif
- if test 'z$(bindir_SQ)' != 'z$(gitexec_instdir_SQ)'; \
- then \
- ln -f '$(DESTDIR_SQ)$(bindir_SQ)/git$X' \
- '$(DESTDIR_SQ)$(gitexec_instdir_SQ)/git$X' || \
- cp '$(DESTDIR_SQ)$(bindir_SQ)/git$X' \
- '$(DESTDIR_SQ)$(gitexec_instdir_SQ)/git$X'; \
- fi
- $(foreach p,$(BUILT_INS), $(RM) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)/$p' && ln '$(DESTDIR_SQ)$(gitexec_instdir_SQ)/git$X' '$(DESTDIR_SQ)$(gitexec_instdir_SQ)/$p' ;)
ifneq (,$X)
$(foreach p,$(patsubst %$X,%,$(filter %$X,$(ALL_PROGRAMS) $(BUILT_INS) git$X)), $(RM) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)/$p';)
endif
- ./check_bindir 'z$(bindir_SQ)' 'z$(gitexec_instdir_SQ)' '$(DESTDIR_SQ)$(bindir_SQ)/git-shell$X'
+ bindir=$$(cd '$(DESTDIR_SQ)$(bindir_SQ)' && pwd) && \
+ execdir=$$(cd '$(DESTDIR_SQ)$(gitexec_instdir_SQ)' && pwd) && \
+ if test "z$$bindir" != "z$$execdir"; \
+ then \
+ ln -f "$$bindir/git$X" "$$execdir/git$X" || \
+ cp "$$bindir/git$X" "$$execdir/git$X"; \
+ fi && \
+ { $(foreach p,$(BUILT_INS), $(RM) "$$execdir/$p" && ln "$$execdir/git$X" "$$execdir/$p" ;) } && \
+ ./check_bindir "z$$bindir" "z$$execdir" "$$bindir/git-shell$X"
install-doc:
$(MAKE) -C Documentation install
^ permalink raw reply related
* [PATCH 1/5] Makefile: Normalize $(bindir) and $(gitexecdir) before comparing
From: Johannes Sixt @ 2008-07-14 21:41 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Steffen Prohaska, git, Johannes Sixt
In-Reply-To: <1216071689-14823-1-git-send-email-johannes.sixt@telecom.at>
The install target needs to check whether the user has opted to make
$(gitexecdir) equal to $(bindir). It did so by a straight string
comparison. Since we are going to allow a relative $(gitexecdir), we have
to normalize paths before comparison, which we do with $(cd there && pwd).
The normalized paths are stored in shell variables. These we can now
reuse in the subsequent install statements, which conveniently shortens
the lines a bit.
Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
Makefile | 18 +++++++++---------
1 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/Makefile b/Makefile
index 4796565..4de9271 100644
--- a/Makefile
+++ b/Makefile
@@ -1318,18 +1318,18 @@ ifndef NO_TCLTK
$(MAKE) -C gitk-git install
$(MAKE) -C git-gui install
endif
- if test 'z$(bindir_SQ)' != 'z$(gitexecdir_SQ)'; \
- then \
- ln -f '$(DESTDIR_SQ)$(bindir_SQ)/git$X' \
- '$(DESTDIR_SQ)$(gitexecdir_SQ)/git$X' || \
- cp '$(DESTDIR_SQ)$(bindir_SQ)/git$X' \
- '$(DESTDIR_SQ)$(gitexecdir_SQ)/git$X'; \
- fi
- $(foreach p,$(BUILT_INS), $(RM) '$(DESTDIR_SQ)$(gitexecdir_SQ)/$p' && ln '$(DESTDIR_SQ)$(gitexecdir_SQ)/git$X' '$(DESTDIR_SQ)$(gitexecdir_SQ)/$p' ;)
ifneq (,$X)
$(foreach p,$(patsubst %$X,%,$(filter %$X,$(ALL_PROGRAMS) $(BUILT_INS) git$X)), $(RM) '$(DESTDIR_SQ)$(gitexecdir_SQ)/$p';)
endif
- ./check_bindir 'z$(bindir_SQ)' 'z$(gitexecdir_SQ)' '$(DESTDIR_SQ)$(bindir_SQ)/git-shell$X'
+ bindir=$$(cd '$(DESTDIR_SQ)$(bindir_SQ)' && pwd) && \
+ execdir=$$(cd '$(DESTDIR_SQ)$(gitexecdir_SQ)' && pwd) && \
+ if test "z$$bindir" != "z$$execdir"; \
+ then \
+ ln -f "$$bindir/git$X" "$$execdir/git$X" || \
+ cp "$$bindir/git$X" "$$execdir/git$X"; \
+ fi && \
+ { $(foreach p,$(BUILT_INS), $(RM) "$$execdir/$p" && ln "$$execdir/git$X" "$$execdir/$p" ;) } && \
+ ./check_bindir "z$$bindir" "z$$execdir" "$$bindir/git-shell$X"
install-doc:
$(MAKE) -C Documentation install
--
1.5.6.3.323.g1e58
^ permalink raw reply related
* Re: [PATCH 3/3] gitweb: use new Git::Repo API, and add optional caching
From: Jakub Narebski @ 2008-07-14 21:23 UTC (permalink / raw)
To: Lea Wiemann; +Cc: git, John Hawley, Petr Baudis
In-Reply-To: <1215738708-5212-1-git-send-email-LeWiemann@gmail.com>
On Fri, 11 July 2008, Lea Wiemann wrote:
> Gitweb now uses the Git::Repo API; this change is behavior-preserving,
> except for slightly more aggressive error handling; see below.
Good.
It was suggested to split this into separate commit from the following
change, for making it easier to test (you can check that behavior is
the same, with the exception of error handling) and review (smaller
patch to read and review).
> This patch also adds an optional caching layer for caching repository
> data in memory and (for larger cacheable items, like blobs, snapshots,
> or diffs) on disk.
As it was said, if feasible it would be good idea to put this change
into separate commit.
> Other minor changes:
>
> - Gitweb would previously accept invalid input and either (a) display
> nothing, (b) display an obscure error message, or (c) proceed as
> normal since the parameter happens to be unused in the particular
> code path used. This has changed in that gitweb will check for
> parameter correctness more aggressively, and display meaningful
> error messages. This change is only relevant if you manually edit
> gitweb's CGI parameters, since gitweb only generates valid links.
I understand that this change deals with treating invalid specifiers,
which point to either object that do not exists, are ambiguous, or point
to object of invalid type. Gitweb does check "syntactic" validity of
input (of CGI parameters) already, even those that are not used for
selected action.
BTW. such check was not feasible before implementing --batch and/or
--batch-check options to git-cat-file; I think that possibly one more
fork is not much price to pay for better error checking.
> - Empty projects:
>
> - Only display summary link for empty projects in project list to
> avoid broken links (yielding 404).
>
> - Slim down summary page for empty projects to avoid some broken
> links and unnecessary vertical space.
>
> - Sort empty projects at the bottom of the project list when sorting
> by last change.
>
> - Add test for empty projects to t9503 (the Mechanize test), now
> that there no broken links anymore.
Good. The only thing that *might* be controversial is putting empty
projects at the bottom of sorted by age (by last change) projects list,
instead of at top.
> - For HTML pages, remove the "Expires" HTTP response header, and add
> "Cache-Control: no-cache" instead. This is because pages can
> contain dynamic content (like the subject of the latest commit), so
> the Expires headers would be wrong.
>
> This makes gitweb's responsiveness slightly worse, but it will get
> much better once If-Last-Modified is implemented. It's better to be
> correct than to be convenient here, since having to press the reload
> button makes for lousy user experience (IOW, users should be able to
> always trust gitweb's output).
>
> Raw diffs and blobs still get the Expires header, where appropriate.
I don't think it is a good change.
Gitweb generates two types of views (pages): transient and immutable.
An example of transient view (transient page/action) is for example RSS
feed, or summary page. When project (repository) is updated, they can
change.
The opposite are immutable pages. They are pages/actions/views where
all specifiers are given by full SHA-1; to be more exact all specifiers
that are needed to reconstruct object are given by SHA-1. (It is
enough to have sufficient check for immutability, i.e. such that if
check succeeds, then page is immutable, but it doesn't need to be true
in reverse.)
Gitweb sets expires to '+1d' which is one day to pages it considers
immutable, while not defining expires for other pages (which results,
I think, in lack of expires header). We could have set it to "forever",
which in terms of Expires: HTTP header is half a year (from what
I remember).
Now I don't see *any* reason to not set long expires for immutable
pages; I don't know if forbidding to cache transient pages even if in
fact they are generated dynamically is a good idea... Note that if
caching is enabled, you can set expires to either time-to-expire of
cache entries (simpler), or time left to live to invalidation of item
in cache (better, but more complicated) perhaps also setting Age:
header to appropriate value.
Sidenote: we would probably want to use Expires: for HTTP/1.0 requests,
and Cache-Control: max-age=<seconds> for HTTP/1.1 requests. But that
might be left as improvement for later...
> - Add a $page_info option to display cache stats at the bottom of each
> page; the option is named generically to allow for adding non-cache
> page info there at some point (timings perhaps?).
Great idea!
> ---
> It's all documented of course :-), but for the impatient here's a
> snippet for gitweb_config.perl to activate caching:
Nice.
> use Cache::Memcached;
> $cache = Cache::Memcached->new( { servers => ['localhost:11211'],
> compress_threshold => 1000 } );
IIRC you can use any Cache::Cache compatibile (is it explained later
what it means?) cache here; IMVHO it would be nice if this info would
be also in commit message.
> $large_cache_root = '/home/lewiemann/gitweb-cache';
> $large_cache_case_sensitive = 1;
Errr... I understand that it is your _private_ configuration, just
copied here verbatim, but I don't think '/home/lewiemann/gitweb-cache'
is a good example: '/tmp/gitweb-cache' perhaps, that I can understand.
> # Invalidate cache on changes to gitweb without version number bump;
> # useful for development.
> $cache_key = (stat '/home/lewiemann/gitweb')[9] .
> (stat '/home/lewiemann/gitweb/gitweb.cgi')[9];
What should be used in production? "$cache_key = $version;"?
Besides hardcoding those paths is not a good idea. You can always
use $ENV{'SCRIPT_FILENAME'}, or dirname of it.
> # Display detailed cache info at the bottom of each page.
> $page_info = 2;
Errr... what does "$page_info = <n>;" mean?
> A live demo is here: http://odin3.kernel.org/git-lewiemann/
Nice. Thanks.
[...]
> gitweb/README | 14 +
Very good.
[Comments on patch itself in separate email, later]
--
Jakub Narebski
Poland
^ 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