* Re: [PATCH] git-rerere.txt: Mention rr-cache directory
From: Junio C Hamano @ 2008-07-09 1:09 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Stephan Beyer, git
In-Reply-To: <alpine.DEB.1.00.0807090230560.5277@eeepc-johanness>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Of course, this only holds true when the config is read, i.e. when
> setup_rerere() was called in time. Which is the case when you call
> rerere() (as is done both from cmd_rerere() as well as cmd_commit()).
>
> Of course, I haven't tested it. Other than running the test script, that
> is.
>
> So care to elaborate what is going wrong?
Very interesting question indeed.
^ permalink raw reply
* Re: Git, merging, and News/Relnotes files
From: Edward Z. Yang @ 2008-07-09 1:14 UTC (permalink / raw)
To: git; +Cc: dpotapov, torvalds, pdebie
In-Reply-To: <37fcd2780807060753h26d9391crff5f9ba5531db654@mail.gmail.com>
Dmitry Potapov wrote:
> Having one file changed on almost every commit is not a good idea, and
> not only because it will cause unnecessary conflicts but also it may
> considerable increase the size of the whole repository. By default, the
> delta compression has limit 50, which means that every 50 change of file
> will become its full copy. If the changelog file is changed very often
> and it is long, it may turn out that changelog alone takes as much space
> as the rest of the source tree.
That is certainly a good technical point, and I will certainly look into
building a log parser after we wrap up our next release cycle.
P.S. Linus, we ended up manually merging the NEWS file; in some cases
there were branch specific changes in the file which would have been
completely inappropriate with a union merge. Thank you for the
suggestion, however.
^ permalink raw reply
* Re: [PATCH 1/4] git-imap-send: Allow the program to be run from subdirectories of a git tree.
From: Junio C Hamano @ 2008-07-09 1:15 UTC (permalink / raw)
To: Robert Shearman; +Cc: git
In-Reply-To: <1215555496-21335-1-git-send-email-robertshearman@gmail.com>
Robert Shearman <robertshearman@gmail.com> writes:
> Call setup_git_directory_gently to allow git-imap-send to be used from subdirectories of a git tree.
> ---
> imap-send.c | 1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/imap-send.c b/imap-send.c
> index 1ec1310..89a1532 100644
> --- a/imap-send.c
> +++ b/imap-send.c
> @@ -1296,6 +1296,7 @@ main(int argc, char **argv)
> /* init the random number generator */
> arc4_init();
>
> + setup_git_directory_gently( NULL );
> git_config(git_imap_config, NULL);
>
> if (!imap_folder) {
I thought Jeff already explained why this NULL was a bad idea, but perhaps
I was dreaming...
Besides, the extra spaces around the argument is very distracting.
^ permalink raw reply
* Re: [PATCH] parse-options: add PARSE_OPT_FAKELASTARG flag.
From: Junio C Hamano @ 2008-07-09 1:15 UTC (permalink / raw)
To: Pierre Habouzit; +Cc: git, Lars Hjemli
In-Reply-To: <20080708103408.GC19202@artemis.madism.org>
Pierre Habouzit <madcoder@debian.org> writes:
> If you set this for a given flag, and the flag appears without a value on
> the command line, then the `defval' is used to fake a new argument.
>
> Note that this flag is meaningless in presence of OPTARG or NOARG flags.
> (in the current implementation it will be ignored, but don't rely on it).
>
> Signed-off-by: Pierre Habouzit <madcoder@debian.org>
> ---
>
> > (3) inspired from (1) and (2), have a flag for options that says
> > "I do take an argument, but if I'm the last option on the
> > command line, please fake this argument for me.
> >
> > I really like (3) more FWIW as it doesn't generate ambiguous
> > parsers like (2) would, and it's not horrible like (1). And cherry
> > on top it's pretty trivial to implement I think.
Yeah, I do not particularly want a major rewrite that only introduces
possible ambiguity to the option parser (even though arguably it would add
to the usability very much, just like we accept revs and paths when
unambiguous), so I think this is a reasonable compromise.
It feels more like LASTARG_DEFAULT but that is bikeshedding ;-)
But I see one thinko (fix below) and another issue I am not sure what the
best fix would be.
---
diff --git a/parse-options.c b/parse-options.c
index b6735a5..cba20d7 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -26,11 +26,11 @@ static int get_arg(struct parse_opt_ctx_t *p, const struct option *opt,
if (p->opt) {
*arg = p->opt;
p->opt = NULL;
+ } else if (p->argc == 1 && (opt->flags & PARSE_OPT_FAKELASTARG)) {
+ *arg = (const char *)opt->defval;
} else if (p->argc) {
p->argc--;
*arg = *++p->argv;
- } else if (opt->flags & PARSE_OPT_FAKELASTARG) {
- *arg = (const char *)opt->defval;
} else
return opterror(opt, "requires a value", flags);
return 0;
^ permalink raw reply related
* [PATCH 1/2] branch --contains: default to HEAD
From: Junio C Hamano @ 2008-07-09 1:17 UTC (permalink / raw)
To: Pierre Habouzit; +Cc: git, Lars Hjemli
In-Reply-To: <7vvdzfoo1s.fsf@gitster.siamese.dyndns.org>
We used to require the name of the commit to limit the branches shown to
the --contains option, but more recent --merged/--no-meregd defaults to
HEAD (and they do not allow arbitrary commit, which is a separate issue).
This teaches --contains to default to HEAD when no parameter is given.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* This comes on top of the FAKELASTARG patch
builtin-branch.c | 12 ++++++++----
1 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/builtin-branch.c b/builtin-branch.c
index d279702..375e5e0 100644
--- a/builtin-branch.c
+++ b/builtin-branch.c
@@ -438,13 +438,17 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
OPT_BOOLEAN( 0 , "color", &branch_use_color, "use colored output"),
OPT_SET_INT('r', NULL, &kinds, "act on remote-tracking branches",
REF_REMOTE_BRANCH),
- OPT_CALLBACK(0, "contains", &with_commit, "commit",
- "print only branches that contain the commit",
- opt_parse_with_commit),
+ {
+ OPTION_CALLBACK, 0, "contains", &with_commit, "commit",
+ "print only branches that contain the commit",
+ PARSE_OPT_FAKELASTARG,
+ opt_parse_with_commit, (intptr_t)"HEAD",
+ },
{
OPTION_CALLBACK, 0, "with", &with_commit, "commit",
"print only branches that contain the commit",
- PARSE_OPT_HIDDEN, opt_parse_with_commit,
+ PARSE_OPT_HIDDEN | PARSE_OPT_FAKELASTARG,
+ opt_parse_with_commit, (intptr_t) "HEAD",
},
OPT__ABBREV(&abbrev),
--
1.5.6.2.255.gbed62
^ permalink raw reply related
* [PATCH 2/2] branch --merged/--not-merged: allow specifying arbitrary commit
From: Junio C Hamano @ 2008-07-09 1:22 UTC (permalink / raw)
To: Pierre Habouzit; +Cc: git, Lars Hjemli
In-Reply-To: <7vvdzfoo1s.fsf@gitster.siamese.dyndns.org>
"git-branch --merged" is a handy way to list all the branches that have
already been merged to the current branch, but it did not allow checking
against anything but the current branch. Having to check out only for
that purpose made the command practically useless.
This updates the option parser so that "git branch --merged next" is
accepted when you are on 'master' branch.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* This does have an issue. --no-<option>=<value> is often nonsense and
parse-options does not accept it (and I do not think we would want to
change it). The use of "--no-merged" was a mistake, but nobody has
perfect foresight.
This adds --not-merged <commit> and allows the <commit> to default to
HEAD if not given to work it around. This and the previous one are not
for application but primarily meant for discussion on what further
flexibility we may want to have in parse-options.
builtin-branch.c | 50 ++++++++++++++++++++++++++++++++++++++++++--------
1 files changed, 42 insertions(+), 8 deletions(-)
diff --git a/builtin-branch.c b/builtin-branch.c
index 375e5e0..151f519 100644
--- a/builtin-branch.c
+++ b/builtin-branch.c
@@ -46,7 +46,12 @@ enum color_branch {
COLOR_BRANCH_CURRENT = 4,
};
-static int mergefilter = -1;
+static enum merge_filter {
+ NO_FILTER = 0,
+ SHOW_NOT_MERGED,
+ SHOW_MERGED,
+} merge_filter;
+static unsigned char merge_filter_ref[20];
static int parse_branch_color_slot(const char *var, int ofs)
{
@@ -234,13 +239,15 @@ static int append_ref(const char *refname, const unsigned char *sha1, int flags,
if ((kind & ref_list->kinds) == 0)
return 0;
- if (mergefilter > -1) {
+ if (merge_filter != NO_FILTER) {
branch.item = lookup_commit_reference_gently(sha1, 1);
if (!branch.item)
die("Unable to lookup tip of branch %s", refname);
- if (mergefilter == 0 && has_commit(head_sha1, &branch))
+ if (merge_filter == SHOW_NOT_MERGED &&
+ has_commit(merge_filter_ref, &branch))
return 0;
- if (mergefilter == 1 && !has_commit(head_sha1, &branch))
+ if (merge_filter == SHOW_MERGED &&
+ !has_commit(merge_filter_ref, &branch))
return 0;
}
@@ -421,6 +428,20 @@ static int opt_parse_with_commit(const struct option *opt, const char *arg, int
return 0;
}
+static int opt_parse_merge_filter(const struct option *opt, const char *arg, int unset)
+{
+ merge_filter = ((opt->long_name[0] == 'n')
+ ? SHOW_NOT_MERGED
+ : SHOW_MERGED);
+ if (unset)
+ merge_filter = SHOW_NOT_MERGED; /* b/c for --no-merged */
+ if (!arg)
+ arg = "HEAD";
+ if (get_sha1(arg, merge_filter_ref))
+ die("malformed object name %s", arg);
+ return 0;
+}
+
int cmd_branch(int argc, const char **argv, const char *prefix)
{
int delete = 0, rename = 0, force_create = 0;
@@ -461,7 +482,18 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
OPT_BIT('M', NULL, &rename, "move/rename a branch, even if target exists", 2),
OPT_BOOLEAN('l', NULL, &reflog, "create the branch's reflog"),
OPT_BOOLEAN('f', NULL, &force_create, "force creation (when already exists)"),
- OPT_SET_INT(0, "merged", &mergefilter, "list only merged branches", 1),
+ {
+ OPTION_CALLBACK, 0, "merged", &merge_filter_ref,
+ "commit", "print only merged branches",
+ PARSE_OPT_FAKELASTARG,
+ opt_parse_merge_filter, (intptr_t) "HEAD",
+ },
+ {
+ OPTION_CALLBACK, 0, "not-merged", &merge_filter_ref,
+ "commit", "print only not merged branches",
+ PARSE_OPT_FAKELASTARG,
+ opt_parse_merge_filter, (intptr_t) "HEAD",
+ },
OPT_END(),
};
@@ -471,9 +503,6 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
branch_use_color = git_use_color_default;
track = git_branch_track;
- argc = parse_options(argc, argv, options, builtin_branch_usage, 0);
- if (!!delete + !!rename + !!force_create > 1)
- usage_with_options(builtin_branch_usage, options);
head = resolve_ref("HEAD", head_sha1, 0, NULL);
if (!head)
@@ -486,6 +515,11 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
die("HEAD not found below refs/heads!");
head += 11;
}
+ hashcpy(merge_filter_ref, head_sha1);
+
+ argc = parse_options(argc, argv, options, builtin_branch_usage, 0);
+ if (!!delete + !!rename + !!force_create > 1)
+ usage_with_options(builtin_branch_usage, options);
if (delete)
return delete_branches(argc, argv, delete > 1, kinds);
--
1.5.6.2.255.gbed62
^ permalink raw reply related
* Re: [PATCH 1/4] git-imap-send: Allow the program to be run from subdirectories of a git tree.
From: Jay Soffian @ 2008-07-09 1:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Robert Shearman, git
In-Reply-To: <7v3amjq2mj.fsf@gitster.siamese.dyndns.org>
On Tue, Jul 8, 2008 at 9:15 PM, Junio C Hamano <gitster@pobox.com> wrote:
> I thought Jeff already explained why this NULL was a bad idea, but perhaps
> I was dreaming...
http://article.gmane.org/gmane.comp.version-control.git/87701
j.
^ permalink raw reply
* Re: [PATCH 4/4] Documentation: Improve documentation for git-imap-send(1).
From: Brian Gernhardt @ 2008-07-09 1:35 UTC (permalink / raw)
To: Robert Shearman; +Cc: git
In-Reply-To: <1215555496-21335-4-git-send-email-robertshearman@gmail.com>
On Jul 8, 2008, at 6:18 PM, Robert Shearman wrote:
> +Using direct mode:
>
> +.........................
> [imap]
> - Host = imaps://imap.example.com
> - User = bob
> - Pass = pwd
> - Port = 993
> + folder = "[Gmail]/Drafts"
> + host = imaps://imap.example.com
> + user = bob
> + pass = p4ssw0rd
> + port = 123
> sslverify = false
> ..........................
If you're going to use [Gmail]/Drafts as the example folder, shouldn't
you just use mail.google.com as your example? Google themselves use username@gmail.com
as an example[1], so that should be safe. So:
..............................
[imap]
folder = "[Gmail]/Drafts"
host = imaps://imap.gmail.com
port = 993
user = username
pass = password
..............................
And I also assume that someone has tried this with Gmail and it
doesn't mangle the Drafts. I'd also move the sslverify option into
the tunneling example, as it's more likely to be needed there.
[1] http://mail.google.com/support/bin/answer.py?answer=78799
~~ Brian
^ permalink raw reply
* [PATCH] git-svn: find-rev and rebase for SVN::Mirror repositories
From: João Abecasis @ 2008-07-09 2:08 UTC (permalink / raw)
To: git, Eric Wong
find-rev and rebase error out on svm because git-svn doesn't trace the original
svn revision numbers back to git commits. The updated test case, included in the
patch, shows the issue and passes with the rest of the patch applied.
This fixes Git::SVN::find_by_url to find branches based on the svm:source URL,
where useSvmProps is set. Also makes sure cmd_find_rev and working_head_info use
the information they have to correctly track the source repository. This is
enough to get find-rev and rebase working.
Signed-off-by: João Abecasis <joao@abecasis.name>
---
Incidentally, I've tried submitting these fixes before, but failed to
get much attention, so I could be doing something wrong... Any input
on the patch or my approach to this list is appreciated. This time I
reworded the commit message and added Eric Wong to the CC list, since
he seems to be Ack'ing git-svn patches.
Anyway, the patch does get things working for me.
Cheers,
João
git-svn.perl | 37 +++++++++++++++++++++++++++++++++----
t/t9110-git-svn-use-svm-props.sh | 9 +++++++++
2 files changed, 42 insertions(+), 4 deletions(-)
diff --git a/git-svn.perl b/git-svn.perl
index a366c89..f5baec1 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -537,13 +537,13 @@ sub cmd_find_rev {
my $head = shift;
$head ||= 'HEAD';
my @refs;
- my (undef, undef, undef, $gs) = working_head_info($head, \@refs);
+ my (undef, undef, $uuid, $gs) = working_head_info($head, \@refs);
unless ($gs) {
die "Unable to determine upstream SVN information from ",
"$head history\n";
}
my $desired_revision = substr($revision_or_hash, 1);
- $result = $gs->rev_map_get($desired_revision);
+ $result = $gs->rev_map_get($desired_revision, $uuid);
} else {
my (undef, $rev, undef) = cmt_metadata($revision_or_hash);
$result = $rev;
@@ -1162,7 +1162,7 @@ sub working_head_info {
if (defined $url && defined $rev) {
next if $max{$url} and $max{$url} < $rev;
if (my $gs = Git::SVN->find_by_url($url)) {
- my $c = $gs->rev_map_get($rev);
+ my $c = $gs->rev_map_get($rev, $uuid);
if ($c && $c eq $hash) {
close $fh; # break the pipe
return ($url, $rev, $uuid, $gs);
@@ -1416,11 +1416,17 @@ sub fetch_all {
sub read_all_remotes {
my $r = {};
+ my $usesvmprops = eval { command_oneline(qw/config --bool
+ svn.useSvmProps/) };
+ $usesvmprops = $usesvmprops eq 'true' if $usesvmprops;
foreach (grep { s/^svn-remote\.// } command(qw/config -l/)) {
if (m!^(.+)\.fetch=\s*(.*)\s*:\s*refs/remotes/(.+)\s*$!) {
my ($remote, $local_ref, $remote_ref) = ($1, $2, $3);
$local_ref =~ s{^/}{};
$r->{$remote}->{fetch}->{$local_ref} = $remote_ref;
+ $r->{$remote}->{svm} = {} if $usesvmprops;
+ } elsif (m!^(.+)\.usesvmprops=\s*(.*)\s*$!) {
+ $r->{$1}->{svm} = {};
} elsif (m!^(.+)\.url=\s*(.*)\s*$!) {
$r->{$1}->{url} = $2;
} elsif (m!^(.+)\.(branches|tags)=
@@ -1437,6 +1443,21 @@ sub read_all_remotes {
}
}
}
+
+ map {
+ if (defined $r->{$_}->{svm}) {
+ my $svm;
+ eval {
+ my $section = "svn-remote.$_";
+ $svm = {
+ source => tmp_config('--get', "$section.svm-source"),
+ replace => tmp_config('--get', "$section.svm-replace"),
+ }
+ };
+ $r->{$_}->{svm} = $svm;
+ }
+ } keys %$r;
+
$r;
}
@@ -1563,13 +1584,21 @@ sub find_by_url { # repos_root and, path are optional
}
my $p = $path;
my $rwr = rewrite_root({repo_id => $repo_id});
+ my $svm = $remotes->{$repo_id}->{svm}
+ if defined $remotes->{$repo_id}->{svm};
unless (defined $p) {
$p = $full_url;
my $z = $u;
+ my $prefix = '';
if ($rwr) {
$z = $rwr;
+ } elsif (defined $svm) {
+ $z = $svm->{source};
+ $prefix = $svm->{replace};
+ $prefix =~ s#^\Q$u\E(?:/|$)##;
+ $prefix =~ s#/$##;
}
- $p =~ s#^\Q$z\E(?:/|$)## or next;
+ $p =~ s#^\Q$z\E(?=/|$)#$prefix# or next;
}
foreach my $f (keys %$fetch) {
next if $f ne $p;
diff --git a/t/t9110-git-svn-use-svm-props.sh b/t/t9110-git-svn-use-svm-props.sh
index 047659f..04d2a65 100755
--- a/t/t9110-git-svn-use-svm-props.sh
+++ b/t/t9110-git-svn-use-svm-props.sh
@@ -49,4 +49,13 @@ test_expect_success 'verify metadata for /dir' "
grep '^git-svn-id: $dir_url@1 $uuid$'
"
+test_expect_success 'find commit based on SVN revision number' "
+ git-svn find-rev r12 |
+ grep `git rev-parse HEAD`
+ "
+
+test_expect_success 'empty rebase' "
+ git-svn rebase
+ "
+
test_done
--
1.5.6
^ permalink raw reply related
* Re: [PATCH 2/4] git-imap-send: Add support for SSL.
From: Abhijit Menon-Sen @ 2008-07-09 2:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Robert Shearman, git
In-Reply-To: <7vbq18q7yk.fsf@gitster.siamese.dyndns.org>
At 2008-07-08 16:20:19 -0700, gitster@pobox.com wrote:
>
> > - Port = 143
> > + Port = 993
> > + sslverify = false
> > ..........................
>
> Don't we also want to keep a vanilla configuration in the example, or
> is imaps the norm and unencrypted imap is exception these days?
The norm is IMAP+STARTTLS on port 143, not IMAPS on port 993. The latter
is also widely deployed for compatibility with older clients, but it is
non-standard and its use isn't exactly encouraged.
-- ams
^ permalink raw reply
* Re: git push to amazon s3 [was: [GSoC] What is status of Git's Google Summer of Code 2008 projects?]
From: Shawn O. Pearce @ 2008-07-09 3:22 UTC (permalink / raw)
To: Tarmigan
Cc: Marek Zawirski, git, Daniel Barkalow, Nick Hengeveld,
Johannes Schindelin
In-Reply-To: <905315640807072248w44ccdc4y2f1cf54a10c50c43@mail.gmail.com>
Tarmigan <tarmigan+git@gmail.com> wrote:
> (trimmed cc list to folks who've touched http-push.c)
> On Mon, Jul 7, 2008 at 9:19 PM, Shawn O. Pearce <spearce@spearce.org> wrote:
> > Using Marek's pack generation code I added support for push over
> > the dumb sftp:// and amazon-s3:// protocols, with the latter also
> > supporting transparent client side encryption.
>
> Can you describe the s3 support that you added? Did you do any
> locking when you pushed? The objects and packs seem likely to be
> naturally OK, but I was worried about refs/ and especially
> objects/info/packs and info/refs (fetch over http works currently out
> of the box with publicly accessable s3 repos).
It behaves like http push does in C git in that it is pretty
transparent to the end-user:
# Create a bucket using other S3 tools.
# I used jets3t's cockpit tool to creat "gitney".
# Create a configuration file for jgit's S3 client:
#
$ touch ~/.jgit_s3_public
$ chmod 600 ~/.jgit_s3_public
$ cat >>~/.jgit_s3_public
accesskey: AWSAccessKeyId
secretkey: AWSSecretAccessKey
acl: public
EOF
# Configure the remote and push
#
$ git remote add s3 amazon-s3://.jgit_s3_public@gitney/projects/egit.git/
$ jgit push s3 refs/heads/master
$ jgit push --tags s3
# Future incremental updates are just as easy
#
$ jgit push s3 refs/heads/master
(or)
$ git config remote.s3.push refs/heads/master
$ jgit push s3
This is now cloneable[*1*]:
$ git clone http://gitney.s3.amazonaws.com/projects/egit.git
Pushes are incremental, rather than the approach you outlined, as
that causes a full re-upload of the repository. Consequently there
is relatively little bandwidth usage during subsequent pushes.
A jgit amazon-s3 URL is organized as:
amazon-s3://$config@$bucket/$prefix
where:
$config = path to configuration in $GIT_DIR/$config or $HOME/$config
$bucket = name of the Amazon S3 bucket holding the objects
$prefix = prefix to apply to all objects, implicitly ends in "/"
Amazon S3 atomically replaces a file, but offers no locking support.
Our crude remote VFS abstraction offers two types of file write
operations:
- Atomic write for small (in-memory) files <~1M
- Stream write for large (e.g. pack) files >~1M
In the S3 implementation both operations are basically the same
code, since even large streams are atomic updates. But in sftp://
our remote VFS writes to a "$path.lock" for the atomic case and
renames to "$path". This is not the same as a real lock, but it
avoids readers from seeing an in-progress update.
We are very carefully to order the update operations to try and
avoid any sorts of race conditions:
- Delete loose refs which are being deleted.
- Upload new pack:
- If same pack already exists:
- (atomic) Remove it from objects/info/packs
- Delete .idx
- Delete .pack
- Upload new .pack
- Upload new .idx
- (atomic) Add to front of objects/info/packs.
- (atomic) Create/update loose refs.
- (atomic) Update (if necessary) packed-refs.
- (atomic) Update info/refs.
Since we are pushing over a dumb transport we assume readers
are pulling over the same dumb transport and thus rely upon the
objects/info/packs and info/refs files to obtain the listing of
what is available. This isn't true though for jgit's sftp://
and amazon-s3:// protocols as both support navigation of the
objects/packs and refs/{heads,tags} tree directly.
Locking on S3 is difficult. Multi-object writes may not sync
across the S3 cluster immediately. This means you can write to A,
then to B, then read A and see the old content still there, then
seconds later read A again and see the new content suddenly arrive.
It all depends upon when the replicas update and which replica
the load-balancer sends you into during the request. So despite
our attempts to order writes to S3 it is still possible for an S3
write to appear "late" and for a client to see a ref in info/refs
for which the corresponding pack is not listed in object/info/packs.
However, this is the same mirroring problem that kernel.org has for
its git trees. I believe they are moved out to the public mirrors
by dumb rsync and not some sort of smart git-aware transport.
As rsync is free to order the writes out of order kernel.org has
the same issue. ;-)
Actually I suspect the S3 replica update occurs more quickly than
the kernel.org mirrors update, so the window under which a client
can see out-of-order writes is likely smaller.
*1* You need a bug fix in jgit to correctly initialize HEAD during
push to a new, non-existant repository stored on S3. The patch
is going to be posted later this evening, its still in my tree.
--
Shawn.
^ permalink raw reply
* Re: git push to amazon s3 [was: [GSoC] What is status of Git's Google Summer of Code 2008 projects?]
From: Shawn O. Pearce @ 2008-07-09 3:26 UTC (permalink / raw)
To: Mike Hommey
Cc: Tarmigan, Marek Zawirski, git, Daniel Barkalow, Nick Hengeveld,
Johannes Schindelin
In-Reply-To: <20080708055610.GA12591@glandium.org>
Mike Hommey <mh@glandium.org> wrote:
> FWIW, I'm starting to work again on the http backend overhaul. My idea
> is to provide a generic dumb protocol vfs-like interface, so that other
> dumb protocols could be built out of it.
jgit has a vfs abstraction for the different dumb protocols. Not sure
if you would find it of any value to read as we are also able to use a
number of Java standard abstractions like InputStream/OutputStream,
but here it is:
WalkRemoteObjectDatabase:
http://repo.or.cz/w/egit.git?a=blob;f=org.spearce.jgit/src/org/spearce/jgit/transport/WalkRemoteObjectDatabase.java;h=915faac9eb85e59c0ed2c08b9631d03cbc4c6bf8;hb=8d085723b260f3b51a70d11b723608779160b090
Thus far this abstraction has worked for sftp:// and amazon-s3://.
WebDAV may make it more complicated due to locking being available
(and something we would want to use to protect writes) but S3 uses
HTTP PUT much like DAV does to upload content so there wouldn't be
too many changes to actually implement DAV support.
--
Shawn.
^ permalink raw reply
* [PATCH 1/3] cherry: cache patch-ids to avoid repeating work
From: Geoffrey Irving @ 2008-07-09 3:53 UTC (permalink / raw)
To: Johannes Schindelin, git@vger.kernel.org, Junio C Hamano
>From a3afd1455d215a541e1481e0f064df743d9219cc Mon Sep 17 00:00:00 2001
From: Geoffrey Irving <irving@naml.us>
Date: Sat, 7 Jun 2008 16:03:49 -0700
Subject: [PATCH 1/3] cherry: cache patch-ids to avoid repeating work
Added cached-sha-map.[hc] implementing a persistent hash map from sha1 to
sha1. The map is read with mmap, and completely rewritten if any entries
change. It would be good to add incremental update to handle the usual case
where only a few entries change.
This structure is used by patch-ids.c to cache the mapping from commit to
patch-id into $GIT_DIR/patch-id-cache. In the one case I've tested so far,
this speeds up the second invocation of git-cherry by two orders of
magnitude.
Original code cannibalized from Johannes Schindelin's notes-index structure.
---
Here's another (hopefully final) version of the patch-id-cache code,
since I finally got around to updating it with Dscho's suggestions.
Makefile | 2 +
cached-sha1-map.c | 182 +++++++++++++++++++++++++++++++++++++++++++++++++++++
cached-sha1-map.h | 45 +++++++++++++
patch-ids.c | 18 +++++-
4 files changed, 246 insertions(+), 1 deletions(-)
create mode 100644 cached-sha1-map.c
create mode 100644 cached-sha1-map.h
diff --git a/Makefile b/Makefile
index 4796565..f7360e1 100644
--- a/Makefile
+++ b/Makefile
@@ -356,6 +356,7 @@ LIB_H += pack-refs.h
LIB_H += pack-revindex.h
LIB_H += parse-options.h
LIB_H += patch-ids.h
+LIB_H += cached-sha1-map.h
LIB_H += path-list.h
LIB_H += pkt-line.h
LIB_H += progress.h
@@ -436,6 +437,7 @@ LIB_OBJS += pager.o
LIB_OBJS += parse-options.o
LIB_OBJS += patch-delta.o
LIB_OBJS += patch-ids.o
+LIB_OBJS += cached-sha1-map.o
LIB_OBJS += path-list.o
LIB_OBJS += path.o
LIB_OBJS += pkt-line.o
diff --git a/cached-sha1-map.c b/cached-sha1-map.c
new file mode 100644
index 0000000..e363745
--- /dev/null
+++ b/cached-sha1-map.c
@@ -0,0 +1,182 @@
+#include "cached-sha1-map.h"
+
+union cached_sha1_map_header {
+ struct {
+ char signature[4]; /* HASH */
+ off_t count, size;
+ };
+ struct cached_sha1_entry padding; /* pad header out to 40 bytes */
+};
+
+static const char *signature = "HASH";
+
+static void init_empty_map(struct cached_sha1_map *cache, size_t size)
+{
+ cache->count = 0;
+ cache->size = size;
+ cache->initialized = 1;
+ cache->dirty = 1;
+ cache->mmapped = 0;
+ cache->entries = xcalloc(size, sizeof(struct cached_sha1_entry));
+}
+
+static void grow_map(struct cached_sha1_map *cache)
+{
+ struct cached_sha1_map new_cache;
+ size_t i;
+
+ /* allocate cache with twice the size */
+ new_cache.filename = cache->filename;
+ init_empty_map(&new_cache, cache->size * 2);
+
+ /* reinsert all entries */
+ for (i = 0; i < cache->size; i++)
+ if (!is_null_sha1(cache->entries[i].key))
+ set_cached_sha1_entry(&new_cache,
+ cache->entries[i].key, cache->entries[i].value);
+ /* finish */
+ free_cached_sha1_map(cache);
+ *cache = new_cache;
+}
+
+static void init_cached_sha1_map(struct cached_sha1_map *cache)
+{
+ int fd;
+ union cached_sha1_map_header header;
+
+ if (cache->initialized)
+ return;
+
+ fd = open(git_path(cache->filename), O_RDONLY);
+ if (fd < 0) {
+ init_empty_map(cache, 64);
+ return;
+ }
+
+ if (read_in_full(fd, &header, sizeof(header)) != sizeof(header))
+ die("cannot read %s header", cache->filename);
+
+ if (memcmp(header.signature, signature, 4))
+ die("%s has invalid header", cache->filename);
+
+ if (header.size & (header.size-1))
+ die("%s size %lld is not a power of two", cache->filename,
+ (long long)header.size);
+
+ cache->count = header.count;
+ cache->size = header.size;
+ cache->dirty = 0;
+ cache->initialized = 1;
+ cache->mmapped = 1;
+
+ /* check off_t to size_t conversion */
+ if (cache->count != header.count || cache->size != header.size)
+ die("%s is too large to hold in memory", cache->filename);
+
+ /* mmap entire file so that file / memory blocks are aligned */
+ cache->entries = xmmap(NULL,
+ sizeof(struct cached_sha1_entry) * (header.size + 1),
+ PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);
+ cache->entries += 1; /* skip header */
+ close(fd);
+}
+
+int write_cached_sha1_map(struct cached_sha1_map *cache)
+{
+ union cached_sha1_map_header header;
+ struct lock_file update_lock;
+ int fd;
+ size_t entry_size;
+
+ if (!cache->initialized || !cache->dirty)
+ return 0;
+
+ fd = hold_lock_file_for_update(&update_lock,
+ git_path(cache->filename), 0);
+
+ if (fd < 0)
+ return error("could not construct %s", cache->filename);
+
+ memcpy(header.signature, signature, 4);
+ header.count = cache->count;
+ header.size = cache->size;
+ entry_size = sizeof(struct cached_sha1_entry) * cache->size;
+ if (write_in_full(fd, &header, sizeof(header)) != sizeof(header)
+ || write_in_full(fd, cache->entries, entry_size) != entry_size)
+ return error("could not write %s", cache->filename);
+
+ if (commit_lock_file(&update_lock) < 0)
+ return error("could not write %s", cache->filename);
+
+ cache->dirty = 0;
+ return 0;
+}
+
+void free_cached_sha1_map(struct cached_sha1_map *cache)
+{
+ if (!cache->initialized)
+ return;
+
+ if (cache->mmapped)
+ munmap(cache->entries - 1,
+ sizeof(struct cached_sha1_entry) * (cache->size + 1));
+ else
+ free(cache->entries);
+}
+
+static size_t get_hash_index(const unsigned char *sha1)
+{
+ return ntohl(*(size_t*)sha1);
+}
+
+int get_cached_sha1_entry(struct cached_sha1_map *cache,
+ const unsigned char *key, unsigned char *value)
+{
+ size_t i, mask;
+
+ if (!cache->initialized)
+ init_cached_sha1_map(cache);
+
+ mask = cache->size - 1;
+
+ for (i = get_hash_index(key) & mask; ; i = (i+1) & mask) {
+ if (!hashcmp(key, cache->entries[i].key)) {
+ hashcpy(value, cache->entries[i].value);
+ return 0;
+ } else if (is_null_sha1(cache->entries[i].key))
+ return -1;
+ }
+}
+
+void set_cached_sha1_entry(struct cached_sha1_map *cache,
+ const unsigned char *key, const unsigned char *value)
+{
+ size_t i, mask;
+ struct cached_sha1_entry *entry;
+
+ if (!cache->initialized)
+ init_cached_sha1_map(cache);
+
+ if (4*cache->count >= 3*cache->size)
+ grow_map(cache);
+
+ mask = cache->size - 1;
+
+ for (i = get_hash_index(key) & mask; ; i = (i+1) & mask) {
+ entry = cache->entries+i;
+
+ if (is_null_sha1(entry->key)) {
+ hashcpy(entry->key, key);
+ hashcpy(entry->value, value);
+ cache->count++;
+ cache->dirty = 1;
+ return;
+ } else if(!hashcmp(key, entry->key)) {
+ if (hashcmp(value, entry->value)) {
+ hashcpy(entry->value, value);
+ cache->dirty = 1;
+ }
+ return;
+ }
+ }
+}
diff --git a/cached-sha1-map.h b/cached-sha1-map.h
new file mode 100644
index 0000000..f592d07
--- /dev/null
+++ b/cached-sha1-map.h
@@ -0,0 +1,45 @@
+#ifndef CACHED_SHA1_MAP_H
+#define CACHED_SHA1_MAP_H
+
+#include "cache.h"
+
+/*
+ * A cached-sha1-map is a file storing a hash map from sha1 to sha1.
+ *
+ * The file is mmap'ed, updated in memory during operation, and flushed
+ * back to disk when freed. Currently the entire file is rewritten for
+ * any change. This could be a significant bottleneck for common uses,
+ * so it would be good to fix this later if possible.
+ *
+ * The performance of a hash map depends highly on a good hashing
+ * algorithm, to avoid collisions. Lucky us! SHA-1 is a pretty good
+ * hashing algorithm.
+ */
+
+struct cached_sha1_entry {
+ unsigned char key[20];
+ unsigned char value[20];
+};
+
+struct cached_sha1_map {
+ const char *filename; /* relative to GIT_DIR */
+
+ /* rest is for internal use */
+ size_t count, size;
+ unsigned int initialized : 1;
+ unsigned int dirty : 1;
+ unsigned int mmapped : 1;
+ struct cached_sha1_entry *entries; /* pointer to mmap'ed memory + 1 */
+};
+
+extern int get_cached_sha1_entry(struct cached_sha1_map *cache,
+ const unsigned char *key,unsigned char *value);
+
+extern void set_cached_sha1_entry(struct cached_sha1_map *cache,
+ const unsigned char *key, const unsigned char *value);
+
+extern int write_cached_sha1_map(struct cached_sha1_map *cache);
+
+extern void free_cached_sha1_map(struct cached_sha1_map *cache);
+
+#endif
diff --git a/patch-ids.c b/patch-ids.c
index 3be5d31..36332f3 100644
--- a/patch-ids.c
+++ b/patch-ids.c
@@ -2,17 +2,31 @@
#include "diff.h"
#include "commit.h"
#include "patch-ids.h"
+#include "cached-sha1-map.h"
+
+struct cached_sha1_map patch_id_cache;
static int commit_patch_id(struct commit *commit, struct diff_options *options,
unsigned char *sha1)
{
+ /* pull patch-id out of the cache if possible */
+ patch_id_cache.filename = "patch-id-cache";
+ if (!get_cached_sha1_entry(&patch_id_cache, commit->object.sha1, sha1))
+ return 0;
+
if (commit->parents)
diff_tree_sha1(commit->parents->item->object.sha1,
commit->object.sha1, "", options);
else
diff_root_tree_sha1(commit->object.sha1, "", options);
diffcore_std(options);
- return diff_flush_patch_id(options, sha1);
+ int ret = diff_flush_patch_id(options, sha1);
+ if (ret)
+ return ret;
+
+ /* record commit, patch-id pair in cache */
+ set_cached_sha1_entry(&patch_id_cache, commit->object.sha1, sha1);
+ return 0;
}
static uint32_t take2(const unsigned char *id)
@@ -136,6 +150,8 @@ int free_patch_ids(struct patch_ids *ids)
next = patches->next;
free(patches);
}
+
+ write_cached_sha1_map(&patch_id_cache);
return 0;
}
--
1.5.6.2.258.g7a51a
^ permalink raw reply related
* [PATCH 2/3] cached-sha1-map: refactoring hash traversal code
From: Geoffrey Irving @ 2008-07-09 3:56 UTC (permalink / raw)
To: Johannes Schindelin, git@vger.kernel.org, Junio C Hamano
>From c4e60c28fe66985ac8224da832589c982010744e Mon Sep 17 00:00:00 2001
From: Geoffrey Irving <irving@naml.us>
Date: Tue, 8 Jul 2008 19:47:22 -0700
Subject: [PATCH 2/3] cached-sha1-map: refactoring hash traversal code
Pulling common code from get_cached_sha1_entry and set_cached_sha1_entry
into static find_helper function.
---
cached-sha1-map.c | 68 +++++++++++++++++++++++++++++-----------------------
1 files changed, 38 insertions(+), 30 deletions(-)
diff --git a/cached-sha1-map.c b/cached-sha1-map.c
index e363745..147c7a2 100644
--- a/cached-sha1-map.c
+++ b/cached-sha1-map.c
@@ -129,10 +129,14 @@ static size_t get_hash_index(const unsigned char *sha1)
return ntohl(*(size_t*)sha1);
}
-int get_cached_sha1_entry(struct cached_sha1_map *cache,
- const unsigned char *key, unsigned char *value)
+/*
+ * Returns the index if the entry exists, and the complemented index of
+ * the next free entry otherwise.
+ */
+static long find_helper(struct cached_sha1_map *cache,
+ const unsigned char *key)
{
- size_t i, mask;
+ long i, mask;
if (!cache->initialized)
init_cached_sha1_map(cache);
@@ -140,43 +144,47 @@ int get_cached_sha1_entry(struct cached_sha1_map *cache,
mask = cache->size - 1;
for (i = get_hash_index(key) & mask; ; i = (i+1) & mask) {
- if (!hashcmp(key, cache->entries[i].key)) {
- hashcpy(value, cache->entries[i].value);
- return 0;
- } else if (is_null_sha1(cache->entries[i].key))
- return -1;
+ if (!hashcmp(key, cache->entries[i].key))
+ return i;
+ else if (is_null_sha1(cache->entries[i].key))
+ return ~i;
}
}
+int get_cached_sha1_entry(struct cached_sha1_map *cache,
+ const unsigned char *key, unsigned char *value)
+{
+ long i = find_helper(cache, key);
+ if(i < 0)
+ return -1;
+
+ /* entry found, return value */
+ hashcpy(value, cache->entries[i].value);
+ return 0;
+}
+
void set_cached_sha1_entry(struct cached_sha1_map *cache,
const unsigned char *key, const unsigned char *value)
{
- size_t i, mask;
+ long i;
struct cached_sha1_entry *entry;
- if (!cache->initialized)
- init_cached_sha1_map(cache);
-
- if (4*cache->count >= 3*cache->size)
- grow_map(cache);
-
- mask = cache->size - 1;
-
- for (i = get_hash_index(key) & mask; ; i = (i+1) & mask) {
- entry = cache->entries+i;
-
- if (is_null_sha1(entry->key)) {
- hashcpy(entry->key, key);
+ i = find_helper(cache, key);
+
+ if (i < 0) { /* write new entry */
+ entry = cache->entries + ~i;
+ hashcpy(entry->key, key);
+ hashcpy(entry->value, value);
+ cache->count++;
+ cache->dirty = 1;
+ } else { /* overwrite existing entry */
+ entry = cache->entries + i;
+ if (hashcmp(value, entry->value)) {
hashcpy(entry->value, value);
- cache->count++;
cache->dirty = 1;
- return;
- } else if(!hashcmp(key, entry->key)) {
- if (hashcmp(value, entry->value)) {
- hashcpy(entry->value, value);
- cache->dirty = 1;
- }
- return;
}
}
+
+ if (4*cache->count >= 3*cache->size)
+ grow_map(cache);
}
--
1.5.6.2.258.g7a51a
^ permalink raw reply related
* [PATCH 3/3] cherry: add cherry.cachepatchids option
From: Geoffrey Irving @ 2008-07-09 3:57 UTC (permalink / raw)
To: Johannes Schindelin, git@vger.kernel.org, Junio C Hamano
>From 7a51a1808fb440b1aca58780ccc09ffe11d4d3d6 Mon Sep 17 00:00:00 2001
From: Geoffrey Irving <irving@naml.us>
Date: Tue, 8 Jul 2008 20:25:53 -0700
Subject: [PATCH 3/3] cherry: add cherry.cachepatchids option
The patch-id caching optimization in git-cherry is still enabled by default,
but now it can be disabled by setting cherry.cachepatchids = false.
---
Documentation/config.txt | 5 +++++
builtin-log.c | 12 ++++++++++++
patch-ids.c | 12 +++++++++---
patch-ids.h | 2 ++
4 files changed, 28 insertions(+), 3 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 838794d..02b8113 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -468,6 +468,11 @@ browser.<tool>.path::
browse HTML help (see '-w' option in linkgit:git-help[1]) or a
working repository in gitweb (see linkgit:git-instaweb[1]).
+cherry.cachepatchids::
+ If true, linkgit:git-cherry will store a cache of computed patch-ids
+ in $GIT_DIR/patch-id-cache in order to make repeated invocations faster.
+ Defaults to true.
+
clean.requireForce::
A boolean to make git-clean do nothing unless given -f
or -n. Defaults to true.
diff --git a/builtin-log.c b/builtin-log.c
index 430d876..fbfefbd 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -1081,6 +1081,16 @@ static int add_pending_commit(const char *arg,
struct rev_info *revs, int flags)
return -1;
}
+static int git_cherry_config(const char *var, const char *value, void *cb)
+{
+ if (!strcmp(var, "cherry.cachepatchids")) {
+ cache_patch_ids = git_config_bool(var, value);
+ return 0;
+ }
+
+ return 0;
+}
+
static const char cherry_usage[] =
"git-cherry [-v] <upstream> [<head>] [<limit>]";
int cmd_cherry(int argc, const char **argv, const char *prefix)
@@ -1094,6 +1104,8 @@ int cmd_cherry(int argc, const char **argv,
const char *prefix)
const char *limit = NULL;
int verbose = 0;
+ git_config(git_cherry_config, NULL);
+
if (argc > 1 && !strcmp(argv[1], "-v")) {
verbose = 1;
argc--;
diff --git a/patch-ids.c b/patch-ids.c
index 36332f3..7e3a563 100644
--- a/patch-ids.c
+++ b/patch-ids.c
@@ -4,6 +4,7 @@
#include "patch-ids.h"
#include "cached-sha1-map.h"
+int cache_patch_ids = 1;
struct cached_sha1_map patch_id_cache;
static int commit_patch_id(struct commit *commit, struct diff_options *options,
@@ -11,7 +12,8 @@ static int commit_patch_id(struct commit *commit,
struct diff_options *options,
{
/* pull patch-id out of the cache if possible */
patch_id_cache.filename = "patch-id-cache";
- if (!get_cached_sha1_entry(&patch_id_cache, commit->object.sha1, sha1))
+ if (cache_patch_ids && !get_cached_sha1_entry(&patch_id_cache,
+ commit->object.sha1, sha1))
return 0;
if (commit->parents)
@@ -25,7 +27,8 @@ static int commit_patch_id(struct commit *commit,
struct diff_options *options,
return ret;
/* record commit, patch-id pair in cache */
- set_cached_sha1_entry(&patch_id_cache, commit->object.sha1, sha1);
+ if (cache_patch_ids)
+ set_cached_sha1_entry(&patch_id_cache, commit->object.sha1, sha1);
return 0;
}
@@ -151,7 +154,10 @@ int free_patch_ids(struct patch_ids *ids)
free(patches);
}
- write_cached_sha1_map(&patch_id_cache);
+ /* write cached patch-ids and ignore any errors that arise
+ * (e.g. if the repository is write protected) */
+ if (cache_patch_ids)
+ write_cached_sha1_map(&patch_id_cache);
return 0;
}
diff --git a/patch-ids.h b/patch-ids.h
index c8c7ca1..c0ebdc1 100644
--- a/patch-ids.h
+++ b/patch-ids.h
@@ -18,4 +18,6 @@ int free_patch_ids(struct patch_ids *);
struct patch_id *add_commit_patch_id(struct commit *, struct patch_ids *);
struct patch_id *has_commit_patch_id(struct commit *, struct patch_ids *);
+extern int cache_patch_ids;
+
#endif /* PATCH_IDS_H */
--
1.5.6.2.258.g7a51a
^ permalink raw reply related
* [PATCH] git-submodule - make "submodule add" more strict, and document it
From: Mark Levedahl @ 2008-07-09 3:59 UTC (permalink / raw)
To: gitster; +Cc: git, sylvain.joyeux, hjemli, pkufranky, Mark Levedahl
In-Reply-To: <7vhcb0x6ak.fsf@gitster.siamese.dyndns.org>
This change makes "submodule add" much more strict in the arguments it
takes, and is intended to address confusion as recently noted on the
git-list. With this change, the required syntax is:
$ git submodule add URL path
Specifically, this eliminates the form
$git submodule add URL
which was confused by more than one person as
$git submodule add path
The change eliminates one form of URL: a path relative to the current
working directory. This form was confusing, and does not seem to
correspond to an important use-case. Specifically, no-one has identified
the use to clone from a repository already in the tree, but if this is
needed it is easily done using an absolute URL: $(pwd)/relative-path. So,
no functionality is lost.
Following this change, there are exactly four variants of
submodule-add, as both arguments have two flavors:
URL can be absolute, or can begin with ./|../ and thus name the origin
relative to the top-level origin.
Note: With this patch, "submodule add" discerns an absolute URL as
matching /*|*:*: e.g., URL begins with /, or it contains a :. This works
for all valid URLs, an absolute path in POSIX, as well as an absolute
path on Windows).
path can either already exist as a valid git repo, or will be cloned from
the given URL. The first form here allows easy addition of a new
submodule to an existing project as the module can be added and tested
in-tree before pushing to the public repository. However, the more
usual form is the second, where the repo is cloned from the given URL.
This specifically addresses the issue of
$ git submodule add a/b/c
attempting to clone from a repository at "a/b/c" to create a new module
in "c". This also simplifies description of "relative URL" as there is now
exactly *one* form: a URL relative to the parent's origin repo.
Signed-off-by: Mark Levedahl <mlevedahl@gmail.com>
---
Documentation/git-submodule.txt | 31 +++++++++++++++------
git-submodule.sh | 55 ++++++++++++++------------------------
2 files changed, 42 insertions(+), 44 deletions(-)
diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 105fc2d..6f24f92 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -9,7 +9,7 @@ git-submodule - Initialize, update or inspect submodules
SYNOPSIS
--------
[verse]
-'git submodule' [--quiet] add [-b branch] [--] <repository> [<path>]
+'git submodule' [--quiet] add [-b branch] [--] <repository> <path>
'git submodule' [--quiet] status [--cached] [--] [<path>...]
'git submodule' [--quiet] init [--] [<path>...]
'git submodule' [--quiet] update [--init] [--] [<path>...]
@@ -20,14 +20,26 @@ COMMANDS
--------
add::
Add the given repository as a submodule at the given path
- to the changeset to be committed next. If path is a valid
- repository within the project, it is added as is. Otherwise,
- repository is cloned at the specified path. path is added to the
- changeset and registered in .gitmodules. If no path is
- specified, the path is deduced from the repository specification.
- If the repository url begins with ./ or ../, it is stored as
- given but resolved as a relative path from the main project's
- url when cloning.
+ to the changeset to be committed next. This requires two arguments,
+ <repository> and <path>. <repository> is the URL of the new
+ submodule's origin repository. This may be either an absolute URL,
+ or (if it begins with ./ or ../), the location relative
+ to the parent repository's origin.
+
+ <path> is the relative location for the cloned submodule to
+ exist in the current tree. If <path> does not exist, then the
+ module is created by cloning from the named URL. If <path> does
+ exist and is already a valid git repository, then this is added
+ to the changeset without cloning. This second form is provided
+ to ease adding a submodule to a project the first time, and presumes
+ the user will later push the new submodule to the given URL.
+
+ In either case, the given URL is recorded into .gitmodules for
+ use by subsequent users cloning the project. If the URL is given
+ relative to the parent, the presumption is the main and sub-modules
+ will be kept together in the same relative location, and only the
+ top-level URL need be provided: git-submodule will correctly
+ locat the submodules using the hint in .gitmodules.
status::
Show the status of the submodules. This will print the SHA-1 of the
@@ -85,6 +97,7 @@ OPTIONS
<path>::
Path to submodule(s). When specified this will restrict the command
to only operate on the submodules found at the specified paths.
+ (This argument is required with add).
FILES
-----
diff --git a/git-submodule.sh b/git-submodule.sh
index 3eb78cc..d227907 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -5,7 +5,7 @@
# Copyright (c) 2007 Lars Hjemli
USAGE="[--quiet] [--cached] \
-[add <repo> [-b branch]|status|init|update [-i|--init]|summary [-n|--summary-limit <n>] [<commit>]] \
+[add <repo> [-b branch] <path>]|[status|init|update [-i|--init]|summary [-n|--summary-limit <n>] [<commit>]] \
[--] [<path>...]"
OPTIONS_SPEC=
. git-sh-setup
@@ -27,18 +27,6 @@ say()
fi
}
-# NEEDSWORK: identical function exists in get_repo_base in clone.sh
-get_repo_base() {
- (
- cd "`/bin/pwd`" &&
- cd "$1" || cd "$1.git" &&
- {
- cd .git
- pwd
- }
- ) 2>/dev/null
-}
-
# Resolve relative url by appending to parent's url
resolve_relative_url ()
{
@@ -115,7 +103,7 @@ module_clone()
#
# Add a new submodule to the working tree, .gitmodules and the index
#
-# $@ = repo [path]
+# $@ = repo path
#
# optional branch is stored in global branch variable
#
@@ -150,16 +138,27 @@ cmd_add()
repo=$1
path=$2
- if test -z "$repo"; then
+ if test -z "$repo" -o -z "$path"; then
usage
fi
- # Guess path from repo if not specified or strip trailing slashes
- if test -z "$path"; then
- path=$(echo "$repo" | sed -e 's|/*$||' -e 's|:*/*\.git$||' -e 's|.*[/:]||g')
- else
- path=$(echo "$path" | sed -e 's|/*$||')
- fi
+ # assure repo is absolute or relative to parent
+ case "$repo" in
+ ./*|../*)
+ # dereference source url relative to parent's url
+ realrepo=$(resolve_relative_url "$repo") || exit
+ ;;
+ *:*|/*)
+ # absolute url
+ realrepo=$repo
+ ;;
+ *)
+ die "repo URL: '$repo' must be absolute or begin with ./|../"
+ ;;
+ esac
+
+ # strip trailing slashes from path
+ path=$(echo "$path" | sed -e 's|/*$||')
git ls-files --error-unmatch "$path" > /dev/null 2>&1 &&
die "'$path' already exists in the index"
@@ -175,20 +174,6 @@ cmd_add()
die "'$path' already exists and is not a valid git repo"
fi
else
- case "$repo" in
- ./*|../*)
- # dereference source url relative to parent's url
- realrepo=$(resolve_relative_url "$repo") || exit
- ;;
- *)
- # Turn the source into an absolute path if
- # it is local
- if base=$(get_repo_base "$repo"); then
- repo="$base"
- fi
- realrepo=$repo
- ;;
- esac
module_clone "$path" "$realrepo" || exit
(unset GIT_DIR; cd "$path" && git checkout -q ${branch:+-b "$branch" "origin/$branch"}) ||
--
1.5.6.2.271.g73ad8
^ permalink raw reply related
* Re: [StGit PATCH 0/2] stg status rename fix (stable)
From: Karl Hasselström @ 2008-07-09 4:05 UTC (permalink / raw)
To: Catalin Marinas; +Cc: git, Jon Smirl
In-Reply-To: <20080708195214.24866.61663.stgit@yoghurt>
On 2008-07-08 21:54:19 +0200, Karl Hasselström wrote:
> This fixes the bug on the stable branch. (stgit.diff-opts doesn't
> exist there, so it's less of an issue, but still.)
>
> Also available at
>
> git://repo.or.cz/stgit/kha.git safe
s/safe/stable/ here, of course.
--
Karl Hasselström, kha@treskal.com
www.treskal.com/kalle
^ permalink raw reply
* [JGIT PATCH 0/4] Misc. push transport fixes
From: Shawn O. Pearce @ 2008-07-09 4:15 UTC (permalink / raw)
To: Robin Rosenberg, Marek Zawirski; +Cc: git
Few minor bugs related to our push implementation. A deadlock
is fixed in the local fetch case and pack generation to include
annotated tags is also fixed.
We also now do essentially "git init" on the remote repository
if we are creating it over a dumb transport during the first push
request made to it.
Shawn O. Pearce (4):
Avoid deadlock while fetching from local repository
Fix pushing of annotated tags to actually include the tag object
Automatically initialize a new dumb repository during push
Use a singleton for the NullProgressMonitor implementation
.../org/spearce/jgit/lib/NullProgressMonitor.java | 3 +
.../src/org/spearce/jgit/revwalk/ObjectWalk.java | 13 ++++-
.../spearce/jgit/transport/BasePackConnection.java | 7 ++-
.../jgit/transport/BasePackFetchConnection.java | 1 +
.../jgit/transport/BasePackPushConnection.java | 1 +
.../spearce/jgit/transport/WalkPushConnection.java | 54 ++++++++++++++++++++
.../src/org/spearce/jgit/util/TemporaryBuffer.java | 2 +-
7 files changed, 77 insertions(+), 4 deletions(-)
^ permalink raw reply
* [JGIT PATCH 1/4] Avoid deadlock while fetching from local repository
From: Shawn O. Pearce @ 2008-07-09 4:15 UTC (permalink / raw)
To: Robin Rosenberg, Marek Zawirski; +Cc: git
In-Reply-To: <1215576931-4174-1-git-send-email-spearce@spearce.org>
We cannot send a packet line end command to git-upload-pack after we
have sent our want list and obtained a pack back from it. Once the
want list has ended git-upload-pack wants no further data sent to
it, and attempting to write more may cause us to deadlock as the
pipe won't accept the data.
Not sending the packet line end if we don't send a want list is a
(minor) protocol error. To avoid these protocol errors (which
may display on stderr from git-upload-pack) we still send the end
during close if we have not yet sent a want list.
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
.../spearce/jgit/transport/BasePackConnection.java | 7 ++++++-
.../jgit/transport/BasePackFetchConnection.java | 1 +
.../jgit/transport/BasePackPushConnection.java | 1 +
3 files changed, 8 insertions(+), 1 deletions(-)
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackConnection.java b/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackConnection.java
index a878f01..7dc4620 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackConnection.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackConnection.java
@@ -83,6 +83,9 @@ abstract class BasePackConnection extends BaseConnection {
/** Packet line encoder around {@link #out}. */
protected PacketLineOut pckOut;
+ /** Send {@link PacketLineOut#end()} before closing {@link #out}? */
+ protected boolean outNeedsEnd;
+
/** Capability tokens advertised by the remote side. */
private final Set<String> remoteCapablities = new HashSet<String>();
@@ -99,6 +102,7 @@ abstract class BasePackConnection extends BaseConnection {
pckIn = new PacketLineIn(in);
pckOut = new PacketLineOut(out);
+ outNeedsEnd = true;
}
protected void readAdvertisedRefs() throws TransportException {
@@ -195,7 +199,8 @@ abstract class BasePackConnection extends BaseConnection {
public void close() {
if (out != null) {
try {
- pckOut.end();
+ if (outNeedsEnd)
+ pckOut.end();
out.close();
} catch (IOException err) {
// Ignore any close errors.
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackFetchConnection.java b/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackFetchConnection.java
index 04a91bf..12b36f2 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackFetchConnection.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackFetchConnection.java
@@ -253,6 +253,7 @@ abstract class BasePackFetchConnection extends BasePackConnection implements
pckOut.writeString(line.toString());
}
pckOut.end();
+ outNeedsEnd = false;
return !first;
}
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackPushConnection.java b/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackPushConnection.java
index 784a578..6d95eaf 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackPushConnection.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackPushConnection.java
@@ -148,6 +148,7 @@ class BasePackPushConnection extends BasePackConnection implements
if (monitor.isCancelled())
throw new TransportException(uri, "push cancelled");
pckOut.end();
+ outNeedsEnd = false;
}
private String enableCapabilties() {
--
1.5.6.74.g8a5e
^ permalink raw reply related
* [JGIT PATCH 2/4] Fix pushing of annotated tags to actually include the tag object
From: Shawn O. Pearce @ 2008-07-09 4:15 UTC (permalink / raw)
To: Robin Rosenberg, Marek Zawirski; +Cc: git
In-Reply-To: <1215576931-4174-2-git-send-email-spearce@spearce.org>
When we pushed an annotated tag to a remote repository jgit was
failing to include the annotated tag object itself as ObjectWalk
nextObject() failed to pop the object out of the internal pending
queue and return it for inclusion in the generated pack file.
We need to use two different flags for SEEN and IN_PENDING as a blob
or tree object can be SEEN during a TreeWalk, yet may also have been
inserted into the pending object list. SEEN means we have output
the object to the caller, IN_PENDING means we have placed it into
the pending object list. Together these prevent duplicates in the
queue and duplicate return values.
Noticed by Robin while trying to push an annotated tag:
$ git tag -m foo X
$ git rev-parse X
49aa0e93621...
$ jgit push somerepo refs/tags/X
error: unpack should have generated 49aa0e93621...
To somerepo.git
! [remote rejected] X -> X (bad pack)
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
.../src/org/spearce/jgit/revwalk/ObjectWalk.java | 13 +++++++++++--
1 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/org.spearce.jgit/src/org/spearce/jgit/revwalk/ObjectWalk.java b/org.spearce.jgit/src/org/spearce/jgit/revwalk/ObjectWalk.java
index 6a5b857..1ba21eb 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/revwalk/ObjectWalk.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/revwalk/ObjectWalk.java
@@ -66,6 +66,15 @@ import org.spearce.jgit.treewalk.TreeWalk;
* commits that are returned first.
*/
public class ObjectWalk extends RevWalk {
+ /**
+ * Indicates a non-RevCommit is in {@link #pendingObjects}.
+ * <p>
+ * We can safely reuse {@link RevWalk#REWRITE} here for the same value as it
+ * is only set on RevCommit and {@link #pendingObjects} never has RevCommit
+ * instances inserted into it.
+ */
+ private static final int IN_PENDING = RevWalk.REWRITE;
+
private final TreeWalk treeWalk;
private BlockObjQueue pendingObjects;
@@ -361,8 +370,8 @@ public class ObjectWalk extends RevWalk {
}
private void addObject(final RevObject o) {
- if ((o.flags & SEEN) == 0) {
- o.flags |= SEEN;
+ if ((o.flags & IN_PENDING) == 0) {
+ o.flags |= IN_PENDING;
pendingObjects.add(o);
}
}
--
1.5.6.74.g8a5e
^ permalink raw reply related
* [JGIT PATCH 4/4] Use a singleton for the NullProgressMonitor implementation
From: Shawn O. Pearce @ 2008-07-09 4:15 UTC (permalink / raw)
To: Robin Rosenberg, Marek Zawirski; +Cc: git
In-Reply-To: <1215576931-4174-4-git-send-email-spearce@spearce.org>
No reason to create a new instance every time we need to shove
a null monitor into a variable because someone passed us null.
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
.../org/spearce/jgit/lib/NullProgressMonitor.java | 3 +++
.../src/org/spearce/jgit/util/TemporaryBuffer.java | 2 +-
2 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/NullProgressMonitor.java b/org.spearce.jgit/src/org/spearce/jgit/lib/NullProgressMonitor.java
index de75b90..191dc00 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/NullProgressMonitor.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/NullProgressMonitor.java
@@ -42,6 +42,9 @@ package org.spearce.jgit.lib;
* A NullProgressMonitor does not report progress anywhere.
*/
public class NullProgressMonitor implements ProgressMonitor {
+ /** Immutable instance of a null progress monitor. */
+ public static final NullProgressMonitor INSTANCE = new NullProgressMonitor();
+
public void start(int totalTasks) {
// Do not report.
}
diff --git a/org.spearce.jgit/src/org/spearce/jgit/util/TemporaryBuffer.java b/org.spearce.jgit/src/org/spearce/jgit/util/TemporaryBuffer.java
index 20db580..d597c38 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/util/TemporaryBuffer.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/util/TemporaryBuffer.java
@@ -200,7 +200,7 @@ public class TemporaryBuffer extends OutputStream {
public void writeTo(final OutputStream os, ProgressMonitor pm)
throws IOException {
if (pm == null)
- pm = new NullProgressMonitor();
+ pm = NullProgressMonitor.INSTANCE;
if (blocks != null) {
// Everything is in core so we can stream directly to the output.
//
--
1.5.6.74.g8a5e
^ permalink raw reply related
* [JGIT PATCH 3/4] Automatically initialize a new dumb repository during push
From: Shawn O. Pearce @ 2008-07-09 4:15 UTC (permalink / raw)
To: Robin Rosenberg, Marek Zawirski; +Cc: git
In-Reply-To: <1215576931-4174-3-git-send-email-spearce@spearce.org>
If we are pushing into a repository which has no refs and no packs
we can reasonably assume the repository is brand new. In such
cases we must ensure there is a valid HEAD and config file in the
repository, otherwise Git clients reading the repository may not
be able to recognize its contents correctly.
By default we try to create HEAD from refs/heads/master, if that
is among the branches being pushed by the user. This mirrors what
git-init would have initialized HEAD to be. However if master was
not among the branches pushed then another branch is selected as a
reasonable default.
We initialize .git/config to a very bare configuration, listing
only the repository version. We avoid setting up core.filemode as
we cannot genrally detect if the remote side understands executable
mode bits.
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
.../spearce/jgit/transport/WalkPushConnection.java | 54 ++++++++++++++++++++
1 files changed, 54 insertions(+), 0 deletions(-)
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/WalkPushConnection.java b/org.spearce.jgit/src/org/spearce/jgit/transport/WalkPushConnection.java
index e11b85a..bb5a653 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/WalkPushConnection.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/WalkPushConnection.java
@@ -48,6 +48,7 @@ import java.util.TreeMap;
import org.spearce.jgit.errors.TransportException;
import org.spearce.jgit.lib.AnyObjectId;
+import org.spearce.jgit.lib.Constants;
import org.spearce.jgit.lib.ObjectId;
import org.spearce.jgit.lib.PackWriter;
import org.spearce.jgit.lib.ProgressMonitor;
@@ -151,6 +152,12 @@ class WalkPushConnection extends BaseConnection implements PushConnection {
for (final RemoteRefUpdate u : updates)
updateCommand(u);
+ // Is this a new repository? If so we should create additional
+ // metadata files so it is properly initialized during the push.
+ //
+ if (!updates.isEmpty() && isNewRepository())
+ createNewRepository(updates);
+
if (!packedRefUpdates.isEmpty()) {
try {
dest.writePackedRefs(newRefs.values());
@@ -307,4 +314,51 @@ class WalkPushConnection extends BaseConnection implements PushConnection {
u.setMessage(e.getMessage());
}
}
+
+ private boolean isNewRepository() {
+ return getRefsMap().isEmpty() && packNames != null
+ && packNames.isEmpty();
+ }
+
+ private void createNewRepository(final List<RemoteRefUpdate> updates)
+ throws TransportException {
+ try {
+ final String ref = "ref: " + pickHEAD(updates) + "\n";
+ final byte[] bytes = ref.getBytes(Constants.CHARACTER_ENCODING);
+ dest.writeFile("../HEAD", bytes);
+ } catch (IOException e) {
+ throw new TransportException(uri, "cannot create HEAD", e);
+ }
+
+ try {
+ final String config = "[core]\n"
+ + "\trepositoryformatversion = 0\n";
+ final byte[] bytes = config.getBytes(Constants.CHARACTER_ENCODING);
+ dest.writeFile("../config", bytes);
+ } catch (IOException e) {
+ throw new TransportException(uri, "cannot create config", e);
+ }
+ }
+
+ private static String pickHEAD(final List<RemoteRefUpdate> updates) {
+ // Try to use master if the user is pushing that, it is the
+ // default branch and is likely what they want to remain as
+ // the default on the new remote.
+ //
+ for (final RemoteRefUpdate u : updates) {
+ final String n = u.getRemoteName();
+ if (n.equals(Constants.HEADS_PREFIX + "/" + Constants.MASTER))
+ return n;
+ }
+
+ // Pick any branch, under the assumption the user pushed only
+ // one to the remote side.
+ //
+ for (final RemoteRefUpdate u : updates) {
+ final String n = u.getRemoteName();
+ if (n.startsWith(Constants.HEADS_PREFIX + "/"))
+ return n;
+ }
+ return updates.get(0).getRemoteName();
+ }
}
--
1.5.6.74.g8a5e
^ permalink raw reply related
* Re: [PATCH 1/3] cherry: cache patch-ids to avoid repeating work
From: Junio C Hamano @ 2008-07-09 5:14 UTC (permalink / raw)
To: Geoffrey Irving; +Cc: Johannes Schindelin, git@vger.kernel.org, Junio C Hamano
In-Reply-To: <7f9d599f0807082053w4603d0bbgfead9127c33b78b5@mail.gmail.com>
"Geoffrey Irving" <irving@naml.us> writes:
> From a3afd1455d215a541e1481e0f064df743d9219cc Mon Sep 17 00:00:00 2001
Please drop this line.
> From: Geoffrey Irving <irving@naml.us>
> Date: Sat, 7 Jun 2008 16:03:49 -0700
> Subject: [PATCH 1/3] cherry: cache patch-ids to avoid repeating work
These are Ok _if_ the difference between them and what you have in your
e-mail header really matter (e.g. you are forwarding somebody else's
patch). I don't think it is in this case, though.
> Added cached-sha-map.[hc] implementing a persistent hash map from sha1 to
> sha1.
"Add cached-sha1-map.[ch]" (imperative mood), not "(here is what I) _did_".
> diff --git a/cached-sha1-map.c b/cached-sha1-map.c
> new file mode 100644
> index 0000000..e363745
> --- /dev/null
> +++ b/cached-sha1-map.c
> @@ -0,0 +1,182 @@
> +#include "cached-sha1-map.h"
> +
> +union cached_sha1_map_header {
> + struct {
> + char signature[4]; /* HASH */
> + off_t count, size;
> + };
> + struct cached_sha1_entry padding; /* pad header out to 40 bytes */
> +};
> +
> +static const char *signature = "HASH";
That sounds a bit too generic, doesn't it, to protect ourselves from
getting confused by some other filetype?
> +static void init_cached_sha1_map(struct cached_sha1_map *cache)
> +{
> + int fd;
> + union cached_sha1_map_header header;
> +
> + if (cache->initialized)
> + return;
> +
> + fd = open(git_path(cache->filename), O_RDONLY);
> + if (fd < 0) {
> + init_empty_map(cache, 64);
> + return;
Check errno and do this only when ENOENT. Other errors should be caught
and reported.
> + }
> +
> + if (read_in_full(fd, &header, sizeof(header)) != sizeof(header))
> + die("cannot read %s header", cache->filename);
> +
> + if (memcmp(header.signature, signature, 4))
> + die("%s has invalid header", cache->filename);
> +
> + if (header.size & (header.size-1))
> + die("%s size %lld is not a power of two", cache->filename,
> + (long long)header.size);
Two issues and a half:
- Isn't it gcc extension to be able to say header.signature, bypassing
the anonymous structure inside the union that the "header" itself is?
- The signature header (count and size) is defined to be off_t, which
makes the cached file unusable across architectures. The map header
structure should be specified with explicit size:
union {
struct {
char sig[4];
uint32_t version;
uint32_t count;
unit32_t size;
} u;
struct cached_sha1_entry pad;
};
the uint32_t fields should be treated as network byte order integers,
e.g.
cache->count = ntohl(header.u.count);
header.u.size = htonl(cache->size);
- If this file is truly intended as "cache", shouldn't corruption of it
be detected, reported but otherwise ignored, so that the lookup would
continue in degraded uncached mode?
> + /* check off_t to size_t conversion */
> + if (cache->count != header.count || cache->size != header.size)
> + die("%s is too large to hold in memory", cache->filename);
This does not make sense to me. What you are checking does not match the
error message.
If you are making the file format architecture dependent (which I would
suggest strongly against), you can use the same type and be done with it.
Otherwise, if you are making the format portable across architectures,
then you would know how large the on-disk integer will be, so as long as
you use appropriate type that is large enough for cache->count you should
be Ok.
What you may want to check is that (header.u.size + 1) * sizeof(entry)
does not wrap around, but you don't.
> + /* mmap entire file so that file / memory blocks are aligned */
> + cache->entries = xmmap(NULL,
> + sizeof(struct cached_sha1_entry) * (header.size + 1),
> + PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);
I think this will die() if the file is too large to map. Ideally you
would want to allow this mmap to fail if the cache is too large, in which
case you can gracefully degrade to cacheless mode of operation, but that
can probably be left to 47th round.
> +int write_cached_sha1_map(struct cached_sha1_map *cache)
> +{
> + union cached_sha1_map_header header;
> + struct lock_file update_lock;
> + int fd;
> + size_t entry_size;
> +
> + if (!cache->initialized || !cache->dirty)
> + return 0;
> +
> + fd = hold_lock_file_for_update(&update_lock,
> + git_path(cache->filename), 0);
> +
> + if (fd < 0)
> + return error("could not construct %s", cache->filename);
Use a "const char *" to hold git_path(cache->filename) upfront in the
function, use it to obtain lock _and_ for reporting.
> + memcpy(header.signature, signature, 4);
> + header.count = cache->count;
> + header.size = cache->size;
And here will be htonl().
> + entry_size = sizeof(struct cached_sha1_entry) * cache->size;
Typically "entry_size" means the size of individual entry; this is the
size of the whole thing.
> + if (write_in_full(fd, &header, sizeof(header)) != sizeof(header)
> + || write_in_full(fd, cache->entries, entry_size) != entry_size)
> + return error("could not write %s", cache->filename);
> +
> + if (commit_lock_file(&update_lock) < 0)
> + return error("could not write %s", cache->filename);
> +
> + cache->dirty = 0;
> + return 0;
> +}
But it is good that you used this intermediate variable; the above
write_in_full() is much easier to read than the xmmap() above at the end
of init_cached_sha1_map() function.
> +static size_t get_hash_index(const unsigned char *sha1)
> +{
> + return ntohl(*(size_t*)sha1);
> +}
Two issues:
- I do not see any guarantee that sha1 is suitably aligned for reading
size_t bytes off of;
- size_t is architecture dependent, so you would get different hash value
depending on the architecture, which again makes this file format
unportable.
> diff --git a/patch-ids.c b/patch-ids.c
> index 3be5d31..36332f3 100644
> --- a/patch-ids.c
> +++ b/patch-ids.c
> @@ -2,17 +2,31 @@
> #include "diff.h"
> #include "commit.h"
> #include "patch-ids.h"
> +#include "cached-sha1-map.h"
> +
> +struct cached_sha1_map patch_id_cache;
Does this have to be extern?
> static int commit_patch_id(struct commit *commit, struct diff_options *options,
> unsigned char *sha1)
> {
> + /* pull patch-id out of the cache if possible */
> + patch_id_cache.filename = "patch-id-cache";
> + if (!get_cached_sha1_entry(&patch_id_cache, commit->object.sha1, sha1))
> + return 0;
> +
> if (commit->parents)
> diff_tree_sha1(commit->parents->item->object.sha1,
> commit->object.sha1, "", options);
> else
> diff_root_tree_sha1(commit->object.sha1, "", options);
> diffcore_std(options);
> - return diff_flush_patch_id(options, sha1);
> + int ret = diff_flush_patch_id(options, sha1);
Decl-after-statement.
> + if (ret)
> + return ret;
> +
> + /* record commit, patch-id pair in cache */
> + set_cached_sha1_entry(&patch_id_cache, commit->object.sha1, sha1);
> + return 0;
> }
^ permalink raw reply
* Re: [PATCH v2 2/4] Add git-sequencer prototype documentation
From: Karl Hasselström @ 2008-07-09 5:20 UTC (permalink / raw)
To: Jakub Narebski
Cc: Stephan Beyer, git, Christian Couder, Daniel Barkalow,
Johannes Schindelin, Junio C Hamano
In-Reply-To: <200807081237.51456.jnareb@gmail.com>
(I've hijacked this thread and started talking mostly about StGit
instead of git-sequencer; if you're not interested, you can stop
reading now.)
On 2008-07-08 12:37:50 +0200, Jakub Narebski wrote:
> 1. Splitting a patch
>
> I cannot comment well on git-sequencer, as I have started using
> StGIT patch management interface instead of git-rebase in times when
> there were no "git rebase --interactive". Nevertheless working with
> StGIT is a bit similar to working with interactive rebase...
>
> I don't find myself wanting to join two patches into one (to squadh
> a commit) perhaps because when I want to add something to a commit
> (to a patch) I simply go to this patch, edit files, and refresh the
> patch.
You can do this without having to manually go to the right patch with
the -p <patchname> flag to stg refresh. In my experimental branch,
this even works together with path limited refresh, or refresh of just
the index.
My own workflow is different: I generally make a large number of
rather small "work-in-progress" commits without much of a commit
message, and every now and then (while I still have everything in
short-term memory) I use "stg coalesce" to make one or more "real"
patches out of them. Because I've committed such small pieces in the
first place, I rarely need to split a patch.
> From time to time however I find myself SPLITTING a patch, for
> example extracting something added "by the way"/"while at it" into
> separate commit (like late separate better documenting project_index
> file format from adding optional description field to project_index
> file format).
The best way I've found of splitting a patch in StGit is to open the
diff in an Emacs buffer, then pop the patch, and then use Emacs' cool
diff-mode features to apply hunks selectively, split hunks, edit files
in place, etc., and committing at the points where I want patch
boundaries. Occasionally, I'll push and pop the patch to get a new
diff with only the remaining stuff.
I imagine something like this could work without StGit as well, since
it's mostly Emacs doing all the hard work. (And I suppose there are
other tools besides Emacs that can do this?)
--
Karl Hasselström, kha@treskal.com
www.treskal.com/kalle
^ permalink raw reply
* Re: [PATCH 1/3] cherry: cache patch-ids to avoid repeating work
From: Geoffrey Irving @ 2008-07-09 5:26 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, git@vger.kernel.org
In-Reply-To: <7vfxqjmyg2.fsf@gitster.siamese.dyndns.org>
On Tue, Jul 8, 2008 at 10:14 PM, Junio C Hamano <gitster@pobox.com> wrote:
> "Geoffrey Irving" <irving@naml.us> writes:
>
>> From a3afd1455d215a541e1481e0f064df743d9219cc Mon Sep 17 00:00:00 2001
>
> Please drop this line.
>
>> From: Geoffrey Irving <irving@naml.us>
>> Date: Sat, 7 Jun 2008 16:03:49 -0700
>> Subject: [PATCH 1/3] cherry: cache patch-ids to avoid repeating work
>
> These are Ok _if_ the difference between them and what you have in your
> e-mail header really matter (e.g. you are forwarding somebody else's
> patch). I don't think it is in this case, though.
>
>> Added cached-sha-map.[hc] implementing a persistent hash map from sha1 to
>> sha1.
>
> "Add cached-sha1-map.[ch]" (imperative mood), not "(here is what I) _did_".
>
>> diff --git a/cached-sha1-map.c b/cached-sha1-map.c
>> new file mode 100644
>> index 0000000..e363745
>> --- /dev/null
>> +++ b/cached-sha1-map.c
>> @@ -0,0 +1,182 @@
>> +#include "cached-sha1-map.h"
>> +
>> +union cached_sha1_map_header {
>> + struct {
>> + char signature[4]; /* HASH */
>> + off_t count, size;
>> + };
>> + struct cached_sha1_entry padding; /* pad header out to 40 bytes */
>> +};
>> +
>> +static const char *signature = "HASH";
>
> That sounds a bit too generic, doesn't it, to protect ourselves from
> getting confused by some other filetype?
>
>> +static void init_cached_sha1_map(struct cached_sha1_map *cache)
>> +{
>> + int fd;
>> + union cached_sha1_map_header header;
>> +
>> + if (cache->initialized)
>> + return;
>> +
>> + fd = open(git_path(cache->filename), O_RDONLY);
>> + if (fd < 0) {
>> + init_empty_map(cache, 64);
>> + return;
>
> Check errno and do this only when ENOENT. Other errors should be caught
> and reported.
>
>> + }
>> +
>> + if (read_in_full(fd, &header, sizeof(header)) != sizeof(header))
>> + die("cannot read %s header", cache->filename);
>> +
>> + if (memcmp(header.signature, signature, 4))
>> + die("%s has invalid header", cache->filename);
>> +
>> + if (header.size & (header.size-1))
>> + die("%s size %lld is not a power of two", cache->filename,
>> + (long long)header.size);
>
> Two issues and a half:
>
> - Isn't it gcc extension to be able to say header.signature, bypassing
> the anonymous structure inside the union that the "header" itself is?
>
> - The signature header (count and size) is defined to be off_t, which
> makes the cached file unusable across architectures. The map header
> structure should be specified with explicit size:
>
> union {
> struct {
> char sig[4];
> uint32_t version;
> uint32_t count;
> unit32_t size;
> } u;
> struct cached_sha1_entry pad;
> };
>
> the uint32_t fields should be treated as network byte order integers,
> e.g.
>
> cache->count = ntohl(header.u.count);
> header.u.size = htonl(cache->size);
>
> - If this file is truly intended as "cache", shouldn't corruption of it
> be detected, reported but otherwise ignored, so that the lookup would
> continue in degraded uncached mode?
>
>> + /* check off_t to size_t conversion */
>> + if (cache->count != header.count || cache->size != header.size)
>> + die("%s is too large to hold in memory", cache->filename);
>
> This does not make sense to me. What you are checking does not match the
> error message.
>
> If you are making the file format architecture dependent (which I would
> suggest strongly against), you can use the same type and be done with it.
> Otherwise, if you are making the format portable across architectures,
> then you would know how large the on-disk integer will be, so as long as
> you use appropriate type that is large enough for cache->count you should
> be Ok.
>
> What you may want to check is that (header.u.size + 1) * sizeof(entry)
> does not wrap around, but you don't.
>
>> + /* mmap entire file so that file / memory blocks are aligned */
>> + cache->entries = xmmap(NULL,
>> + sizeof(struct cached_sha1_entry) * (header.size + 1),
>> + PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);
>
> I think this will die() if the file is too large to map. Ideally you
> would want to allow this mmap to fail if the cache is too large, in which
> case you can gracefully degrade to cacheless mode of operation, but that
> can probably be left to 47th round.
>
>> +int write_cached_sha1_map(struct cached_sha1_map *cache)
>> +{
>> + union cached_sha1_map_header header;
>> + struct lock_file update_lock;
>> + int fd;
>> + size_t entry_size;
>> +
>> + if (!cache->initialized || !cache->dirty)
>> + return 0;
>> +
>> + fd = hold_lock_file_for_update(&update_lock,
>> + git_path(cache->filename), 0);
>> +
>> + if (fd < 0)
>> + return error("could not construct %s", cache->filename);
>
> Use a "const char *" to hold git_path(cache->filename) upfront in the
> function, use it to obtain lock _and_ for reporting.
>
>> + memcpy(header.signature, signature, 4);
>> + header.count = cache->count;
>> + header.size = cache->size;
>
> And here will be htonl().
>
>> + entry_size = sizeof(struct cached_sha1_entry) * cache->size;
>
> Typically "entry_size" means the size of individual entry; this is the
> size of the whole thing.
>
>> + if (write_in_full(fd, &header, sizeof(header)) != sizeof(header)
>> + || write_in_full(fd, cache->entries, entry_size) != entry_size)
>> + return error("could not write %s", cache->filename);
>> +
>> + if (commit_lock_file(&update_lock) < 0)
>> + return error("could not write %s", cache->filename);
>> +
>> + cache->dirty = 0;
>> + return 0;
>> +}
>
> But it is good that you used this intermediate variable; the above
> write_in_full() is much easier to read than the xmmap() above at the end
> of init_cached_sha1_map() function.
>
>> +static size_t get_hash_index(const unsigned char *sha1)
>> +{
>> + return ntohl(*(size_t*)sha1);
>> +}
>
> Two issues:
>
> - I do not see any guarantee that sha1 is suitably aligned for reading
> size_t bytes off of;
>
> - size_t is architecture dependent, so you would get different hash value
> depending on the architecture, which again makes this file format
> unportable.
>
>> diff --git a/patch-ids.c b/patch-ids.c
>> index 3be5d31..36332f3 100644
>> --- a/patch-ids.c
>> +++ b/patch-ids.c
>> @@ -2,17 +2,31 @@
>> #include "diff.h"
>> #include "commit.h"
>> #include "patch-ids.h"
>> +#include "cached-sha1-map.h"
>> +
>> +struct cached_sha1_map patch_id_cache;
>
> Does this have to be extern?
>
>> static int commit_patch_id(struct commit *commit, struct diff_options *options,
>> unsigned char *sha1)
>> {
>> + /* pull patch-id out of the cache if possible */
>> + patch_id_cache.filename = "patch-id-cache";
>> + if (!get_cached_sha1_entry(&patch_id_cache, commit->object.sha1, sha1))
>> + return 0;
>> +
>> if (commit->parents)
>> diff_tree_sha1(commit->parents->item->object.sha1,
>> commit->object.sha1, "", options);
>> else
>> diff_root_tree_sha1(commit->object.sha1, "", options);
>> diffcore_std(options);
>> - return diff_flush_patch_id(options, sha1);
>> + int ret = diff_flush_patch_id(options, sha1);
>
> Decl-after-statement.
>
>> + if (ret)
>> + return ret;
>> +
>> + /* record commit, patch-id pair in cache */
>> + set_cached_sha1_entry(&patch_id_cache, commit->object.sha1, sha1);
>> + return 0;
>> }
Thanks. I'll fix these in the next few days.
Should I rewrite the patch sequence to incorporate these changes into
the first commit, or add them as a forth commit off the end?
Geoffrey
^ 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