Git development
 help / color / mirror / Atom feed
* Re: Running gitweb under mod_perl
From: Jakub Narebski @ 2006-09-05 20:32 UTC (permalink / raw)
  To: git
In-Reply-To: <20060824140525.G638085b@leonov.stosberg.net>

Dennis Stosberg wrote:

> Jakub Narebski wrote:
> 
>> What should I put in Apache configuration (Apache 2.0.54 if this
>> matters, mod_perl 2.0.1) 
> 
> From my configuration:
> 
>   <Directory /home/dennis/public_html/perl>
>     Options -Indexes +ExecCGI
>     AllowOverride None
>     PerlSendHeader On
>     SetHandler perl-script
>     PerlHandler ModPerl::Registry
>   </Directory>

I use mod_perl 2.0 version

   Alias /perl /var/www/perl
   <Directory /var/www/perl>
       SetHandler perl-script
       PerlResponseHandler ModPerl::Registry
       PerlOptions +ParseHeaders
       Options +ExecCGI
   </Directory>

What is strange that ApacheBench is showing that mod_perl is _slower_ than
CGI version: 3003.305 [ms] (mean) CGI vs 3500.589 [ms] (mean) mod_perl
for summary page for git.git repository (my copy that is).

I wonder if I misconfigured something...
-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

^ permalink raw reply

* [PATCH] autoconf: Fix copy'n'paste error
From: Jakub Narebski @ 2006-09-05 20:03 UTC (permalink / raw)
  To: Jonas Fonseca, git
In-Reply-To: <20060905162526.GA5547@diku.dk>

Signed-off-by: Jonas Fonseca <fonseca@diku.dk>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
 configure.ac |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/configure.ac b/configure.ac
index 85317a3..482c849 100644
--- a/configure.ac
+++ b/configure.ac
@@ -147,7 +147,7 @@ AC_CHECK_LIB([c], [iconv],
 [NEEDS_LIBICONV=],
 [NEEDS_LIBICONV=YesPlease])
 AC_SUBST(NEEDS_LIBICONV)
-test -n "$NEEDS_SOCKET" && LIBS="$LIBS -liconv"
+test -n "$NEEDS_LIBICONV" && LIBS="$LIBS -liconv"
 #
 # Define NEEDS_SOCKET if linking with libc is not enough (SunOS,
 # Patrick Mauritz).
-- 
1.4.2

^ permalink raw reply related

* Re: [PATCH 1/2] Add git-archive
From: Junio C Hamano @ 2006-09-05 19:23 UTC (permalink / raw)
  To: Franck Bui-Huu; +Cc: git
In-Reply-To: <cda58cb80609050516v699338b9y57fd54f50c66e49e@mail.gmail.com>

"Franck Bui-Huu" <vagabon.xyz@gmail.com> writes:

> git-archive is a command to make TAR and ZIP archives of a git tree.
> It helps prevent a proliferation of git-{format}-tree commands.

Thanks.  I like the overall structure, at least mostly.
Also dropping -tree suffix from the command name is nice, short
and sweet.

Obviously I cannot apply this patch because it is totally
whitespace damaged, but here are some comments.

> diff --git a/archive.h b/archive.h
> new file mode 100644
> index 0000000..6c69953
> --- /dev/null
> +++ b/archive.h
> @@ -0,0 +1,43 @@
> +#ifndef ARCHIVE_H
> +#define ARCHIVE_H
> +
> +typedef int (*write_archive_fn_t)(struct tree *tree,
> +				  const unsigned char *commit_sha1,
> +				  const char *prefix,
> +				  time_t time,
> +				  const char **pathspec);

The type of the first argument might have to be different,
depending on performance analysis by Rene on struct tree vs
struct tree_desc.

> +typedef int (*parse_extra_args_fn_t)(int argc,
> +				     const char **argv,
> +				     const char **reason);
> +

I do not see a way for parse_extra to record the parameter it
successfully parsed, other than in a source-file-global, static
variable.  Not a very nice design for a library, if we are
building one from scratch.

Also, you are passing "reason" around from everywhere, but that
is used by the caller to pass it to error(), so it might be
simpler to just call error() when you want to assign to *reason,
and make an error return.  The caller does not have to do
anything if you do that.  Your way might interact with the
remote protocol better, though -- I haven't thought this part
through yet so do not take this as a serious objection, but just
a comment.

> +struct archiver_struct {
> +	const char *name;
> +	write_archive_fn_t write_archive;
> +	parse_extra_args_fn_t parse_extra;
> +	const char *remote;
> +	const char *prefix;
> +};

Somehow "struct foo_struct" makes me feel uneasy, when I do not
see the reason to call it "struct foo".

Also, the first three fields are permanent property of the
archiver while two are to wrap runtime arguments of one
particular invocation.  I would have liked...

> +extern struct archiver_struct archivers[];

... this array to have only the former, and a separate structure
"struct archive_args" to be defined.

	struct archive_args {
        	const char *remote;
                const char *prefix;
	};

After parse_archive_args finds the archiver specified with
--format=*, it can call its parse_extra to retrieve a suitable
struct that has struct archive_args embedded at the beginning,
and then set remote and prefix on the returned structure.

Then a specific parse_extra implementation can be written like this:

	static struct tar_archive_args {
        	struct archive_args a;
                int z_compress;
                ...
	};

	struct archive_args *
        tar_archive_parse_extra(int ac, const char **av)
	{
        	struct tar_archive_args *args = xcalloc(1, sizeof(*args));

		while (ac--) {
			const char *arg = *++av;
                	if (arg[0] == '-' &&
                            '0' <= arg[1] && arg[1] <= '9')
				args->z_compress = arg[1] - '0';
			...
		}
		return (struct archive_args *)args;
	}

and this can be passed to tar_archive_write_archive as an
argument.

> +extern int parse_treeish_arg(const char **argv,
> +			     struct tree **tree,
> +			     const unsigned char **commit_sha1,
> +			     time_t *archive_time,
> +			     const char *prefix,
> +			     const char **reason);
> +extern int write_tar_archive(struct tree *tree,
> +			     const unsigned char *commit_sha1,
> +			     const char *prefix,
> +			     time_t time,
> +			     const char **pathspec);

I suspect we would want "struct tree_desc" based interface,
instead of "struct tree".

> +static const char archive_usage[] = \
> +"git-archive --format=<fmt> [--prefix=<prefix>/] [-0|...|-9]
> <tree-ish> [path...]";

I do not think "-[0-9]" belongs to generic "git-archive".  It
does not make much sense to run compress on zip output.  More
like:

git-archive --format=<fmt> [--prefix=<prefix>] [format specific options] <tree-ish> [path...]

It has one potential advantage, though -- git-daemon _could_
look at it and notice that the client asks for too expensive
compression level.  But I do not think it is the only way to
achive that to make "-[0-9]" a generic option.

> +static int run_remote_archiver(struct archiver_struct *ar, int argc,
> +			       const char **argv)
> +{
> +	char *url, buf[1024];
> +	pid_t pid;
> +	int fd[2];
> +	int len, rv;
> +
> +	sprintf(buf, "git-upload-%s", ar->name);

Are you calling git-upload-{tar,zip,rar,...} here?

> +	url = strdup(ar->remote);
> +	pid = git_connect(fd, url, buf);
> +	if (pid < 0)
> +		return pid;
> +
> +	concat_argv(argc, argv, buf, sizeof(buf));
> +	packet_write(fd[1], "arguments %s\n", buf);
> +	packet_flush(fd[1]);

Parameter concatenation with SP is a bad idea for two reasons.
You cannot have SP in argument.  Also packet_write() may not
like the length of the arguments.

A sequence of one argument per packet, with prefix "argument "
for future extension so that we can send other stuff if/when
needed, followed by a flush would be preferred.

> +	/* Now, start reading from fd[0] and spit it out to stdout */
> +	rv = copy_fd(fd[0], 1);
> +	close(fd[0]);
> +	rv |= finish_connect(pid);

It was painful to bolt progress indicator support onto original
upload-pack protocol, while making sure that older and newer
clients and servers interoperate with each other.  Since this is
a new protocol, we should start with the side-band support from
the beginning (see upload-pack and look for use_sideband).

Instead of sending the payload straight out, upload-archive side
would read from the underlying archiver, and send it with
one-byte prefix to say if it is a normal payload (band 1),
message to stderr used to show progress indicator and error
messages (band 2), or error exit situation (band 3).  The client
side here would receive the packetized data and do the reverse.

> +int parse_treeish_arg(const char **argv, struct tree **tree,
> +		      const unsigned char **commit_sha1,
> +		      time_t *archive_time, const char *prefix,
> +		      const char **reason)
> +{
>...
> +	if (prefix) {
> +		unsigned char tree_sha1[20];
> +		unsigned int mode;
> +		int err;
> +
> +		err = get_tree_entry((*tree)->object.sha1, prefix,
> +				     tree_sha1, &mode);
> +		if (err || !S_ISDIR(mode)) {
> +			*reason = "current working directory is untracked";
> +			goto out;
> +		}
> +		free(*tree);
> +		*tree = parse_tree_indirect(tree_sha1);
> +	}

I like the simplicity of just optionally sending one subtree (or
the whole thing), but I think this part would be made more
efficient if we go with "struct tree_desc" based interface.

Also I wonder how this interacts with the pathspec you take from
the command line.  Personally I think this single subtree
support is good enough and limiting with pathspec is not needed.

> +int parse_archive_args(int argc, const char **argv,
> +		       struct archiver_struct **ar,
> +		       const char **reason)
> +{
>...
> +		if (arg[0] == '-' && isdigit(arg[1]) && arg[2] == '\0') {
> +			zlib_compression_level = arg[1] - '0';
> +			continue;
> +		}

Commented on this part already.

> +	if (list) {
> +		if (!remote) {
> +			int i;
> +
> +			for (i = 0; i < ARRAY_SIZE(archivers); i++)
> +				printf("%s\n", archivers[i].name);
> +			exit(0);
> +		}

You do not need a different "i" that shadows the outer one here.

> +	(*ar)->remote = remote;
> +	(*ar)->prefix = prefix ? : "";

Please be nicer to other people by staying away from GNU
extension "A ? : B", especially when A is so simple.

^ permalink raw reply

* [PATCH] send pack remove remote reference limit
From: Andy Whitcroft @ 2006-09-05 19:00 UTC (permalink / raw)
  To: git
In-Reply-To: <7v64g2i9p8.fsf@assigned-by-dhcp.cox.net>

send-pack: remove remote reference limit

When build a pack for a push we query the remote copy for existant
heads.  These are used to prune unnecessary objects from the pack.
As we receive the remote references in get_remote_heads() we validate
the reference names via check_ref() which includes a length check;
rejecting those >45 characters in size.

This is a miss converted change, it was originally designed to reject
messages which were less than 45 characters in length (a 40 character
sha1 and refs/) to prevent comparing unitialised memory.  check_ref()
now gets the raw length so check for at least 5 characters.

Signed-off-by: Andy Whitcroft <apw@shadowen.org>
---
diff --git a/connect.c b/connect.c
index e501ccc..371aea1 100644
--- a/connect.c
+++ b/connect.c
@@ -17,7 +17,7 @@ static int check_ref(const char *name, i
 	if (!flags)
 		return 1;
 
-	if (len > 45 || memcmp(name, "refs/", 5))
+	if (len < 5 || memcmp(name, "refs/", 5))
 		return 0;
 
 	/* Skip the "refs/" part */

^ permalink raw reply related

* [PATCH] git-svnimport: Parse log message for Signed-off-by: lines
From: Sasha Khapyorsky @ 2006-09-05 18:46 UTC (permalink / raw)
  To: Junio C Hamano, Matthias Urlichs; +Cc: git

Hi,

This feature was useful with importing https://openib.org/svn/gen2 .

Sasha

This add '-S' option. When specified svn-import will try to parse
commit message for 'Signed-off-by: ...' line, and if found will use
the name and email address extracted at first occurrence as this commit
author name and author email address. Committer name and email are
extracted in usual way.

Signed-off-by: Sasha Khapyorsky <sashak@voltaire.com>
---
 git-svnimport.perl |   31 ++++++++++++++++++++-----------
 1 files changed, 20 insertions(+), 11 deletions(-)

diff --git a/git-svnimport.perl b/git-svnimport.perl
index 26dc454..7113cf5 100755
--- a/git-svnimport.perl
+++ b/git-svnimport.perl
@@ -31,7 +31,7 @@ die "Need SVN:Core 1.2.1 or better" if $
 $ENV{'TZ'}="UTC";
 
 our($opt_h,$opt_o,$opt_v,$opt_u,$opt_C,$opt_i,$opt_m,$opt_M,$opt_t,$opt_T,
-    $opt_b,$opt_r,$opt_I,$opt_A,$opt_s,$opt_l,$opt_d,$opt_D);
+    $opt_b,$opt_r,$opt_I,$opt_A,$opt_s,$opt_l,$opt_d,$opt_D,$opt_S);
 
 sub usage() {
 	print STDERR <<END;
@@ -39,12 +39,12 @@ Usage: ${\basename $0}     # fetch/updat
        [-o branch-for-HEAD] [-h] [-v] [-l max_rev]
        [-C GIT_repository] [-t tagname] [-T trunkname] [-b branchname]
        [-d|-D] [-i] [-u] [-r] [-I ignorefilename] [-s start_chg]
-       [-m] [-M regex] [-A author_file] [SVN_URL]
+       [-m] [-M regex] [-A author_file] [-S] [SVN_URL]
 END
 	exit(1);
 }
 
-getopts("A:b:C:dDhiI:l:mM:o:rs:t:T:uv") or usage();
+getopts("A:b:C:dDhiI:l:mM:o:rs:t:T:Suv") or usage();
 usage if $opt_h;
 
 my $tag_name = $opt_t || "tags";
@@ -531,21 +531,30 @@ sub copy_path($$$$$$$$) {
 
 sub commit {
 	my($branch, $changed_paths, $revision, $author, $date, $message) = @_;
-	my($author_name,$author_email,$dest);
+	my($committer_name,$committer_email,$dest);
+	my($author_name,$author_email);
 	my(@old,@new,@parents);
 
 	if (not defined $author or $author eq "") {
-		$author_name = $author_email = "unknown";
+		$committer_name = $committer_email = "unknown";
 	} elsif (defined $users_file) {
 		die "User $author is not listed in $users_file\n"
 		    unless exists $users{$author};
-		($author_name,$author_email) = @{$users{$author}};
+		($committer_name,$committer_email) = @{$users{$author}};
 	} elsif ($author =~ /^(.*?)\s+<(.*)>$/) {
-		($author_name, $author_email) = ($1, $2);
+		($committer_name, $committer_email) = ($1, $2);
 	} else {
 		$author =~ s/^<(.*)>$/$1/;
-		$author_name = $author_email = $author;
+		$committer_name = $committer_email = $author;
 	}
+
+	if ($opt_S && $message =~ /Signed-off-by:\s+(.*?)\s+<(.*)>\s*\n/) {
+        	($author_name, $author_email) = ($1, $2);
+	} else {
+		$author_name = $committer_name;
+		$author_email = $committer_email;
+	}
+
 	$date = pdate($date);
 
 	my $tag;
@@ -772,8 +781,8 @@ #	}
 				"GIT_AUTHOR_NAME=$author_name",
 				"GIT_AUTHOR_EMAIL=$author_email",
 				"GIT_AUTHOR_DATE=".strftime("+0000 %Y-%m-%d %H:%M:%S",gmtime($date)),
-				"GIT_COMMITTER_NAME=$author_name",
-				"GIT_COMMITTER_EMAIL=$author_email",
+				"GIT_COMMITTER_NAME=$committer_name",
+				"GIT_COMMITTER_EMAIL=$committer_email",
 				"GIT_COMMITTER_DATE=".strftime("+0000 %Y-%m-%d %H:%M:%S",gmtime($date)),
 				"git-commit-tree", $tree,@par);
 			die "Cannot exec git-commit-tree: $!\n";
@@ -825,7 +834,7 @@ #	}
 		print $out ("object $cid\n".
 		    "type commit\n".
 		    "tag $dest\n".
-		    "tagger $author_name <$author_email>\n") and
+		    "tagger $committer_name <$committer_email>\n") and
 		close($out)
 		    or die "Cannot create tag object $dest: $!\n";
 
-- 
1.4.2

^ permalink raw reply related

* Why is there a --binary option needed for git-apply?
From: Carl Worth @ 2006-09-05 18:40 UTC (permalink / raw)
  To: git

[-- Attachment #1: Type: text/plain, Size: 1152 bytes --]

I was recently emailed a patch that introduced some new binary files
to my repository. The patch included a very pleasant-looking chunk
along the lines of:

	diff --git a/new/file.png b/new/file.png
	new file mode 100644
	index 0000000000000000000000000000000000000000..e7edd8141e15f5753ea94244e7315bd1341a8c05
	GIT binary patch

I tried applying the patch with git-apply (1.4.2.rc2.gef1d9) and
received an inscrutable error:

	fatal: patch with only garbage at line 90

Where line 90 happened to be the line after the first chunk.

I was disappointed that the operation had failed and started guessing
at problem causes (git version incompatibilities? MUA whitespace
munging?).

Shawn Pearce was kind enough to direct me to the --binary option for
git-apply which solved my problem. But that left me wondering why
git-apply requires this extra command-line option to do its
job. Shouldn't git-apply simply apply the patch it is given?

If there is some reason for git-apply to only apply binary patches
when under the duress of --binary, then at the very least it could use
a better error message explaining the situation.

-Carl
--
cworth@redhat.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

^ permalink raw reply

* Re: New git commit tool
From: Jonas Fonseca @ 2006-09-05 16:46 UTC (permalink / raw)
  To: Paul Mackerras; +Cc: git, torvalds
In-Reply-To: <17660.40934.605502.248266@cargo.ozlabs.ibm.com>

Paul Mackerras <paulus@samba.org> wrote Tue, Sep 05, 2006:
> Jonas Fonseca writes:
> 
> >     I am a Cogito user, so I am no used to running git-update-index and
> >     this seems to be a problem in this case:
> > 
> > 	can't unset "indexpending(gitool)": no such element in array
> 
> Hmmm, I didn't think that could happen. :) It can only happen if some
> file gets listed twice in the output from "git-diff-index HEAD".
> Could you send me the output of "git-diff-index HEAD" and
> "git-diff-index --cached HEAD" in that repository?

~/gitool > git-diff-index HEAD
fatal: ambiguous argument 'HEAD': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions
~/gitool > git-diff-index --cached HEAD
fatal: ambiguous argument 'HEAD': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions
~/gitool > cat .git/HEAD
ref: refs/heads/master
~/gitool > ls .git/refs/heads/
~/gitool >

> And no, you don't need to run git-update-index, gitool does that for
> you.

Good to hear, but I swear the first time I ran gitool in a repo of mine
it popped up the error dialog with the details saying:

	fileX: needs update

Now, of course, I cannot reproduce it.

-- 
Jonas Fonseca

^ permalink raw reply

* Re: [PATCH 2/5] autoconf: Add -liconv to LIBS when NEEDS_LIBICONV
From: Jonas Fonseca @ 2006-09-05 16:25 UTC (permalink / raw)
  To: Jakub Narebski; +Cc: git
In-Reply-To: <200609050055.52980.jnareb@gmail.com>

Jakub Narebski <jnareb@gmail.com> wrote Tue, Sep 05, 2006:
> Signed-off-by: Jakub Narebski <jnareb@gmail.com>
> ---
> Just in case; it could matter for testing if iconv is properly
> supported (NO_ICONV test).
> 
>  configure.ac |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index 36f9cd9..fc5b813 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -147,6 +147,7 @@ AC_CHECK_LIB([c], [iconv],
>  [NEEDS_LIBICONV=],
>  [NEEDS_LIBICONV=YesPlease])
>  AC_SUBST(NEEDS_LIBICONV)
> +test -n "$NEEDS_SOCKET" && LIBS="$LIBS -liconv"

I see that this has entered already, but it looks like it needs a
s/NEEDS_SOCKET/NEEDS_LIBICONV/

>  #
>  # Define NEEDS_SOCKET if linking with libc is not enough (SunOS,
>  # Patrick Mauritz).

-- 
Jonas Fonseca

^ permalink raw reply

* Re: send-pack: limit on negative references
From: Junio C Hamano @ 2006-09-05 16:23 UTC (permalink / raw)
  To: Andy Whitcroft; +Cc: git
In-Reply-To: <44FD714F.9040003@shadowen.org>

Andy Whitcroft <apw@shadowen.org> writes:

> I've been having trouble with git push apparently resending the entire
> commit trace for the branch each and every time I push; even with short
> branch names.  This seems to be related to the changes made to handle
> the case where the remote end has a large number of branches (>900).

I think the right fix is to do one or both of the following, and
lift that 900 cut-off entirely.

One is to teach rev-list to read the information it is taking
from the command line instead from its standard input.

Another is to teach pack-object the same trick on top of my
patch last night.  This has an added benefit that we save one
pipe+fork+exec there.

These are essentially suggestions from Linus made twice
separately in the past, so they must be on the right track ;-).

If nobody does, I would do it myself, but the list is welcome to
beat me to it.  Especially, the former (giving --stdin option to
rev-list) should be trivial.

^ permalink raw reply

* Re: remote_get_heads: reference length limit
From: Junio C Hamano @ 2006-09-05 16:01 UTC (permalink / raw)
  To: Andy Whitcroft; +Cc: git
In-Reply-To: <44FD729D.4020208@shadowen.org>

Andy Whitcroft <apw@shadowen.org> writes:

> I've been having trouble with git push apparently resending the entire
> commit trace for the branch each and every time I push.  Poking at the
> source it seems this is due to a length limit on reference names as
> pulled from the remote repository.
>
> When we are building the pack to send we are sent a list of remote
> heads.  get_remote_heads() reads these in, validates them and finally
> adds them to the remote_refs list.  Part of the validation is a simple
> check for size and form; check_ref().
>
> static int check_ref(const char *name, int len, unsigned int flags)
> {
>         if (!flags)
>                 return 1;
>
>         if (len > 45 || memcmp(name, "refs/", 5))
>                 return 0;
> [...]
> }
>
> With the refs/heads/ prefix included this limits the head names to 34
> characters.  From what I can see there is no good reason for this limit
> to be so low.  I can see we don't want the remote end bloating us out of
> control, but we are already limiting the lines which contain these
> references to 1000 bytes and making no attempt to limit the number of
> them the remote server can send us.  There seems to be no limits imposed
> on the name length other than MAX_PATHLEN.
>
> Can anyone see a reason to keep this (len > 45) check?

Yes, it was a mis-conversion done with commit 2718ff0.
The data this part (and the caller) is dealing with are
peek-remote output (40-byte hex object name, tab and
"refs/...").  Before that commit we had a check like this:

+		if (ignore_funny && 45 < len && !memcmp(name, "refs/", 5) &&
+		    check_ref_format(name + 5))
+			continue;

That is, "if we are ignoring funny fake refs, continue when the
name begins with refs/ and is magic (e.g. refs/heads/master^{});
by the way that can only happen if the entire line length is
more than 45 bytes (because of 40-byte hex plus tab, anything
shorter than that cannot even fit "refs/" at the beginning of
the name).

But when the check_ref() function was written it was made to
take the name length as "len", so I do not think there is a
reason to check the name against 45 at all.  Also the direction
of the comparison is wrong in the new code.  It was meant to be
"line must be at least this long otherwise it is not right".

I think you can just drop "len > 45 || " part.

^ permalink raw reply

* remote_get_heads: reference length limit
From: Andy Whitcroft @ 2006-09-05 12:50 UTC (permalink / raw)
  To: git

I've been having trouble with git push apparently resending the entire
commit trace for the branch each and every time I push.  Poking at the
source it seems this is due to a length limit on reference names as
pulled from the remote repository.

When we are building the pack to send we are sent a list of remote
heads.  get_remote_heads() reads these in, validates them and finally
adds them to the remote_refs list.  Part of the validation is a simple
check for size and form; check_ref().

static int check_ref(const char *name, int len, unsigned int flags)
{
        if (!flags)
                return 1;

        if (len > 45 || memcmp(name, "refs/", 5))
                return 0;
[...]
}

With the refs/heads/ prefix included this limits the head names to 34
characters.  From what I can see there is no good reason for this limit
to be so low.  I can see we don't want the remote end bloating us out of
control, but we are already limiting the lines which contain these
references to 1000 bytes and making no attempt to limit the number of
them the remote server can send us.  There seems to be no limits imposed
on the name length other than MAX_PATHLEN.

Can anyone see a reason to keep this (len > 45) check?

-apw

^ permalink raw reply

* send-pack: limit on negative references
From: Andy Whitcroft @ 2006-09-05 12:45 UTC (permalink / raw)
  To: git

[-- Attachment #1: Type: text/plain, Size: 1508 bytes --]

I've been having trouble with git push apparently resending the entire
commit trace for the branch each and every time I push; even with short
branch names.  This seems to be related to the changes made to handle
the case where the remote end has a large number of branches (>900).

The problem there seemed to be that we would fill list-revs' argument
list in reference order with negative and positive limits.  This could
lead to us to exceed the limit 900 argument limit and bomb.  The changes
made in commit 797656e58ddbd82ac461a5142ed726db3a4d0ac0 split the
parameter loading into two phases.  First we load the +ve and -ve
references for the branches we are definatly trying to send; bailing if
we cannot fit those references onto the command line.  Second we load up
the remaining remote references as -ve references to limit duplicate
object sends.

This second phase loads up to 16 additional references onto the command
line.  Now this effectively limits you to 16 branchs in your remote
repository if you wish to ensure you catch a parental head to copy from.
 This can lead us to send the entire commit stream for the branch even
though the branch may contain no new commits.

It seems to me that any and all negative references we have are helpful
and as such we should be loading as many as we can into the list.  If we
cannot fit them all we should _not_ error out, but we should not limit
ourselves to any less than as many as we can fit.

Can anyone see a downside to the patch below?

-apw

[-- Attachment #2: send-pack-supply-as-many-negative-references-as-we-can-fit --]
[-- Type: text/plain, Size: 1211 bytes --]

send-pack: supply as many negative references as we can fit

In commit 797656e58ddbd82ac461a5142ed726db3a4d0ac0 list-revs' command
line was reordered to ensure the branches we are pushing are first on
the list, and then a subset of the remaining remote references are
passed to limit our push.  Given that we now have the needed positive
references on the list first, it seems reasonable to load as many
negative references onto the list as will fit without error.

Signed-off-by: Andy Whitcroft <andyw@uk.ibm.com>
---
diff --git a/send-pack.c b/send-pack.c
index 10bc8bc..5d55241 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -40,7 +40,7 @@ static void exec_rev_list(struct ref *re
 {
 	struct ref *ref;
 	static const char *args[1000];
-	int i = 0, j;
+	int i = 0;
 
 	args[i++] = "rev-list";	/* 0 */
 	if (use_thin_pack)	/* 1 */
@@ -69,8 +69,8 @@ static void exec_rev_list(struct ref *re
 	/* Then a handful of the remainder
 	 * NEEDSWORK: we would be better off if used the newer ones first.
 	 */
-	for (ref = refs, j = i + 16;
-	     i < 900 && i < j && ref;
+	for (ref = refs;
+	     i < 900 && ref;
 	     ref = ref->next) {
 		if (is_zero_sha1(ref->new_sha1) &&
 		    !is_zero_sha1(ref->old_sha1) &&

^ permalink raw reply related

* remote_get_heads: reference length limit
From: Andy Whitcroft @ 2006-09-05 12:18 UTC (permalink / raw)
  To: git

[-- Attachment #1: Type: text/plain, Size: 1225 bytes --]

I've been having trouble with git push apparently resending the entire
commit trace for the branch each and every time I push.  Poking at the
source it seems this is due to a length limit on reference names as
pulled from the remote repository.

When we are building the pack to send we are sent a list of remote
heads.  get_remote_heads() reads these in, validates them and finally
adds them to the remote_refs list.  Part of the validation is a simple
check for size and form; check_ref().

static int check_ref(const char *name, int len, unsigned int flags)
{
        if (!flags)
                return 1;

        if (len > 45 || memcmp(name, "refs/", 5))
                return 0;
[...]
}

With the refs/heads/ prefix included this limits the head names to 34
characters.  From what I can see there is no good reason for this limit
to be so low.  I can see we don't want the remote end bloating us out of
control, but we are already limiting the lines which contain these
references to 1000 bytes and making no attempt to limit the number of
them the remote server can send us.  There seems to be no limits imposed
on the name length other than MAX_PATHLEN.

Can anyone see a reason to keep this (len > 45) check?

-apw

[-- Attachment #2: send-pack-remove-remote-reference-limit --]
[-- Type: text/plain, Size: 841 bytes --]

send-pack: remove remote reference limit

When build a pack for a push we query the remote copy for existant
heads.  These are used to prune unnecessary objects from the pack.
As we receive the remote references in get_remote_heads() we validate
the reference names via check_ref() which includes a length check;
rejecting those >45 characters in size.

We appear to be able to handle reference names upto MAXPATHLEN in
size.  Relax the limit out to that size.

Signed-off-by: Andy Whitcroft <apw@shadowen.org>
---
diff --git a/connect.c b/connect.c
index 4422a0d..0a51e78 100644
--- a/connect.c
+++ b/connect.c
@@ -17,7 +17,7 @@ static int check_ref(const char *name, i
 	if (!flags)
 		return 1;
 
-	if (len > 45 || memcmp(name, "refs/", 5))
+	if (len > MAXPATHLEN || memcmp(name, "refs/", 5))
 		return 0;
 
 	/* Skip the "refs/" part */

^ permalink raw reply related

* [PATCH 2/2] Add git-upload-archive
From: Franck Bui-Huu @ 2006-09-05 12:22 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Rene Scharfe, git

This command implements the git archive protocol on the server
side. This command is not intended to be used by the end user.
Underlying git-archive command line options are sent over the
protocol from "git-archive --remote=...", just like upload-tar
currently does with "git-tar-tree=...".

As for "git-archive" command implementation, this new command
does not execute any existing "git-{tar,zip}-tree" but rely
on the archive API defined by "git-archive" patch. Hence we
get 2 good points:

 - "git-archive" and "git-upload-archive" share all option
   parsing code.

 - All kind of git-upload-{tar,zip} could be removed.

Signed-off-by: Franck Bui-Huu <vagabon.xyz@gmail.com>
---
 .gitignore               |    1
 Makefile                 |    1
 builtin-upload-archive.c |  102 ++++++++++++++++++++++++++++++++++++++++++++++
 builtin.h                |    1
 daemon.c                 |   13 +++++-
 git.c                    |    1
 6 files changed, 117 insertions(+), 2 deletions(-)

diff --git a/.gitignore b/.gitignore
index a3e7ca1..fda9bf2 100644
--- a/.gitignore
+++ b/.gitignore
@@ -119,6 +119,7 @@ git-unpack-objects
 git-update-index
 git-update-ref
 git-update-server-info
+git-upload-archive
 git-upload-pack
 git-upload-tar
 git-var
diff --git a/Makefile b/Makefile
index 51ed4dd..461e1d6 100644
--- a/Makefile
+++ b/Makefile
@@ -305,6 +305,7 @@ BUILTIN_OBJS = \
 	builtin-unpack-objects.o \
 	builtin-update-index.o \
 	builtin-update-ref.o \
+	builtin-upload-archive.o \
 	builtin-upload-tar.o \
 	builtin-verify-pack.o \
 	builtin-write-tree.o \
diff --git a/builtin-upload-archive.c b/builtin-upload-archive.c
new file mode 100644
index 0000000..5e389fc
--- /dev/null
+++ b/builtin-upload-archive.c
@@ -0,0 +1,102 @@
+/*
+ * Copyright (c) 2006 Franck Bui-Huu
+ */
+#include <time.h>
+#include "cache.h"
+#include "builtin.h"
+#include "archive.h"
+#include "pkt-line.h"
+
+#define MAX_ARGS	32
+
+static const char upload_archive_usage[] =
+	"git-upload-archive --format=fmt <repo>";
+
+static int NACK(const char *reason)
+{
+	packet_write(1, "NACK %s\n", reason);
+	packet_flush(1);
+	return -1;
+}
+
+int cmd_upload_archive(int argc, const char **argv, const char *prefix)
+{
+	struct archiver_struct *ar;
+	const char *sent_argv[MAX_ARGS];
+	const char *arg_cmd = "arguments ";
+	const char *reason;
+	char *p, buf[8192];
+	int treeish_idx;
+	int sent_argc;
+	int len;
+	int rv;
+
+	const char **pathspec;
+	struct tree *tree;
+	const unsigned char *commit_sha1;
+	time_t archive_time;
+
+	if (argc != 3)
+		usage(upload_archive_usage);
+
+	if (strlen(argv[2]) > sizeof(buf)) {
+		reason = "insanely long repository name";
+		goto nack;
+	}
+	strcpy(buf, argv[2]); /* enter-repo smudges its argument */
+
+	if (!enter_repo(buf, 0)) {
+		reason = "not a git archive";
+		goto nack;
+	}
+
+	len = packet_read_line(0, buf, sizeof(buf));
+	if (len < strlen(arg_cmd) || strncmp(arg_cmd, buf, strlen(arg_cmd))) {
+		reason = "expected 'arguments' command";
+		goto nack;
+	}
+	if (buf[len-1] == '\n')
+		buf[--len] = 0;
+
+	/* we can safely override 'arg_cmd' */
+	len = packet_read_line(0, buf, strlen(arg_cmd));
+	if (len) {
+		reason = "flush expected";
+		goto nack;
+	}
+	p = buf + strlen(arg_cmd);
+
+	/* put received line options in sent_argv[] */
+	sent_argv[0] = "git-upload-archive";
+	sent_argv[1] = argv[1]; /* --format=<fmt> */
+	sent_argc = 2;
+	while (p) {
+		if (sent_argc > MAX_ARGS - 2) {
+			reason = "Too many options (>29)";
+			goto nack;
+		}
+		sent_argv[sent_argc++] = strsep(&p, " ");
+	}
+	sent_argv[sent_argc] = NULL;
+
+	/* parse all options sent by the client */
+	treeish_idx = parse_archive_args(sent_argc, sent_argv, &ar, &reason);
+	if (treeish_idx < 0)
+		goto nack;
+
+	rv = parse_treeish_arg(sent_argv + treeish_idx, &tree, &commit_sha1,
+			       &archive_time, prefix, &reason);
+	if (rv < 0)
+		goto nack;
+
+	pathspec = get_pathspec(ar->prefix, sent_argv + treeish_idx + 1);
+
+	packet_write(1, "ACK\n");
+	packet_flush(1);
+
+	return ar->write_archive(tree, commit_sha1, ar->prefix,
+				 archive_time, pathspec);
+nack:
+	return NACK(reason);
+}
+
diff --git a/builtin.h b/builtin.h
index 2391afb..f3efb58 100644
--- a/builtin.h
+++ b/builtin.h
@@ -58,6 +58,7 @@ extern int cmd_zip_tree(int argc, const
 extern int cmd_unpack_objects(int argc, const char **argv, const char *prefix);
 extern int cmd_update_index(int argc, const char **argv, const char *prefix);
 extern int cmd_update_ref(int argc, const char **argv, const char *prefix);
+extern int cmd_upload_archive(int argc, const char **argv, const char *prefix);
 extern int cmd_upload_tar(int argc, const char **argv, const char *prefix);
 extern int cmd_version(int argc, const char **argv, const char *prefix);
 extern int cmd_whatchanged(int argc, const char **argv, const char *prefix);
diff --git a/daemon.c b/daemon.c
index a4a08f3..c09280f 100644
--- a/daemon.c
+++ b/daemon.c
@@ -324,12 +324,21 @@ static int upload_pack(void)
 	return -1;
 }

-static int upload_tar(void)
+static int upload_archive(const char *fmt)
 {
-	execl_git_cmd("upload-tar", ".", NULL);
+	char buf[64];
+
+	snprintf(buf, sizeof(buf), "--format=%s", fmt);
+
+	execl_git_cmd("upload-archive", buf, ".", NULL);
 	return -1;
 }

+static int upload_tar(void)
+{
+	return upload_archive("tar");
+}
+
 static struct daemon_service daemon_service[] = {
 	{ "upload-pack", "uploadpack", upload_pack, 1, 1 },
 	{ "upload-tar", "uploadtar", upload_tar, 0, 1 },
diff --git a/git.c b/git.c
index c62c5cf..315dc0b 100644
--- a/git.c
+++ b/git.c
@@ -261,6 +261,7 @@ static void handle_internal_command(int
 		{ "unpack-objects", cmd_unpack_objects, RUN_SETUP },
 		{ "update-index", cmd_update_index, RUN_SETUP },
 		{ "update-ref", cmd_update_ref, RUN_SETUP },
+		{ "upload-archive", cmd_upload_archive },
 		{ "upload-tar", cmd_upload_tar },
 		{ "version", cmd_version },
 		{ "whatchanged", cmd_whatchanged, RUN_SETUP | USE_PAGER },
-- 
1.4.2.gbba4

^ permalink raw reply related

* [PATCH 1/2] Add git-archive
From: Franck Bui-Huu @ 2006-09-05 12:16 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Rene Scharfe, git

git-archive is a command to make TAR and ZIP archives of a git tree.
It helps prevent a proliferation of git-{format}-tree commands.

Instead of directly calling git-{tar,zip}-tree command, it defines
a very simple API, that archiver should implement and register in
"git-archive.c". This API is made up by 2 functions whose prototype
is defined in "archive.h" file.

 - The first one is used to parse 'extra' parameters which have
   signification only for the specific archiver. That would allow
   different archive backends to have different kind of options.

 - The second one is used to ask to an archive backend to build
   the archive given some already resolved parameters.

The main reason for making this API is to avoid using
git-{tar,zip}-tree commands, hence making them useless. Maybe it's
time for them to die ?

It also implements remote operations by defining a very simple
protocol: it first sends the name of the specific uploader followed
the repository name (git-upload-tar git://example.org/repo.git).
Then it sends "arguments" key word followed by all options given
when invoking 'git-archive'.

The remote protocol is implemented in "git-archive.c" for client
side and is triggered by "--remote=<repo>" option. For example,
to fetch a TAR archive in a remote repo, you can issue:

$ git archive --format=tar --remote=git://xxx/yyy/zzz.git HEAD

We choose to not make a new command "git-fetch-archive" for example,
avoind one more GIT command which should be nice for users (less
commands to remember, keeps existing --remote option).

Signed-off-by: Franck Bui-Huu <vagabon.xyz@gmail.com>
---
 .gitignore          |    1
 Makefile            |    3 -
 archive.h           |   43 ++++++++
 builtin-archive.c   |  262 +++++++++++++++++++++++++++++++++++++++++++++++++++
 builtin-tar-tree.c  |   66 +++++++++++++
 builtin.h           |    1
 generate-cmdlist.sh |    1
 git.c               |    1
 8 files changed, 377 insertions(+), 1 deletions(-)

diff --git a/.gitignore b/.gitignore
index 78cb671..a3e7ca1 100644
--- a/.gitignore
+++ b/.gitignore
@@ -8,6 +8,7 @@ git-apply
 git-applymbox
 git-applypatch
 git-archimport
+git-archive
 git-bisect
 git-branch
 git-cat-file
diff --git a/Makefile b/Makefile
index 389daf7..51ed4dd 100644
--- a/Makefile
+++ b/Makefile
@@ -242,7 +242,7 @@ LIB_FILE=libgit.a
 XDIFF_LIB=xdiff/lib.a

 LIB_H = \
-	blob.h cache.h commit.h csum-file.h delta.h \
+	archive.h blob.h cache.h commit.h csum-file.h delta.h \
 	diff.h object.h pack.h para-walk.h pkt-line.h quote.h refs.h \
 	run-command.h strbuf.h tag.h tree.h git-compat-util.h revision.h \
 	tree-walk.h log-tree.h dir.h path-list.h unpack-trees.h builtin.h
@@ -267,6 +267,7 @@ LIB_OBJS = \
 BUILTIN_OBJS = \
 	builtin-add.o \
 	builtin-apply.o \
+	builtin-archive.o \
 	builtin-cat-file.o \
 	builtin-checkout-index.o \
 	builtin-check-ref-format.o \
diff --git a/archive.h b/archive.h
new file mode 100644
index 0000000..6c69953
--- /dev/null
+++ b/archive.h
@@ -0,0 +1,43 @@
+#ifndef ARCHIVE_H
+#define ARCHIVE_H
+
+typedef int (*write_archive_fn_t)(struct tree *tree,
+				  const unsigned char *commit_sha1,
+				  const char *prefix,
+				  time_t time,
+				  const char **pathspec);
+
+typedef int (*parse_extra_args_fn_t)(int argc,
+				     const char **argv,
+				     const char **reason);
+
+struct archiver_struct {
+	const char *name;
+	write_archive_fn_t write_archive;
+	parse_extra_args_fn_t parse_extra;
+	const char *remote;
+	const char *prefix;
+};
+
+extern struct archiver_struct archivers[];
+
+extern int parse_archive_args(int argc, const char **argv,
+			      struct archiver_struct **ar,
+			      const char **reason);
+
+extern int parse_treeish_arg(const char **argv,
+			     struct tree **tree,
+			     const unsigned char **commit_sha1,
+			     time_t *archive_time,
+			     const char *prefix,
+			     const char **reason);
+/*
+ *
+ */
+extern int write_tar_archive(struct tree *tree,
+			     const unsigned char *commit_sha1,
+			     const char *prefix,
+			     time_t time,
+			     const char **pathspec);
+
+#endif	/* ARCHIVE_H */
diff --git a/builtin-archive.c b/builtin-archive.c
new file mode 100644
index 0000000..9341813
--- /dev/null
+++ b/builtin-archive.c
@@ -0,0 +1,262 @@
+/*
+ * Copyright (c) 2006 Franck Bui-Huu
+ * Copyright (c) 2006 Rene Scharfe
+ */
+#include <time.h>
+#include "cache.h"
+#include "builtin.h"
+#include "archive.h"
+#include "commit.h"
+#include "tree-walk.h"
+#include "exec_cmd.h"
+#include "pkt-line.h"
+
+static const char archive_usage[] = \
+"git-archive --format=<fmt> [--prefix=<prefix>/] [-0|...|-9]
<tree-ish> [path...]";
+
+struct archiver_struct archivers[] = {
+	{ "tar", write_tar_archive },
+};
+
+static void concat_argv(int argc, const char **argv, char line[], int size)
+{
+	char *p;
+	int len, i;
+
+	p = line;
+	for (i = 1; i < argc; i++) {
+		/* server needn't these options */
+		if (!strncmp(argv[i], "--format=", 9) ||
+		    !strncmp(argv[i], "--remote=", 9))
+			continue;
+
+		len = strlen(argv[i]);
+		if (p + len + 1> line + size)
+			die("too many options");
+
+		strcpy(p, argv[i]);
+		p += len;
+		*p++ = ' ';
+	}
+	if (p > line)
+		p--;
+	*p = '\0';
+}
+
+static int run_remote_archiver(struct archiver_struct *ar, int argc,
+			       const char **argv)
+{
+	char *url, buf[1024];
+	pid_t pid;
+	int fd[2];
+	int len, rv;
+
+	sprintf(buf, "git-upload-%s", ar->name);
+
+	url = strdup(ar->remote);
+	pid = git_connect(fd, url, buf);
+	if (pid < 0)
+		return pid;
+
+	concat_argv(argc, argv, buf, sizeof(buf));
+
+	packet_write(fd[1], "arguments %s\n", buf);
+	packet_flush(fd[1]);
+
+	len = packet_read_line(fd[0], buf, sizeof(buf));
+	if (!len)
+		die("git-archive: expected ACK/NAK, got EOF");
+	if (buf[len-1] == '\n')
+		buf[--len] = 0;
+	if (strcmp(buf, "ACK")) {
+		if (len > 5 && !strncmp(buf, "NACK ", 5))
+			die("git-archive: NACK %s", buf + 5);
+		die("git-archive: protocol error");
+	}
+
+	len = packet_read_line(fd[0], buf, sizeof(buf));
+	if (len)
+		die("git-archive: expected a flush");
+
+	/* Now, start reading from fd[0] and spit it out to stdout */
+	rv = copy_fd(fd[0], 1);
+
+	close(fd[0]);
+	rv |= finish_connect(pid);
+
+	return !!rv;
+}
+
+static struct archiver_struct *get_archiver(const char *name)
+{
+	struct archiver_struct *ar = NULL;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(archivers); i++) {
+		if (!strcmp(name, archivers[i].name)) {
+			ar = &archivers[i];
+			break;
+		}
+	}
+	return ar;
+}
+
+int parse_treeish_arg(const char **argv, struct tree **tree,
+		      const unsigned char **commit_sha1,
+		      time_t *archive_time, const char *prefix,
+		      const char **reason)
+{
+	const char *name = argv[0];
+	struct commit *commit;
+	unsigned char sha1[20];
+	int rv = -1;
+
+	if (get_sha1(name, sha1)) {
+		*reason = "Not a valid object name";
+		goto out;
+	}
+
+	commit = lookup_commit_reference_gently(sha1, 1);
+	if (commit) {
+		*commit_sha1 = commit->object.sha1;
+		*archive_time = commit->date;
+	} else {
+		*archive_time = time(NULL);
+	}
+
+	*tree = parse_tree_indirect(sha1);
+	if (*tree == NULL) {
+		*reason = "not a tree object";
+		goto out;
+	}
+
+	if (prefix) {
+		unsigned char tree_sha1[20];
+		unsigned int mode;
+		int err;
+
+		err = get_tree_entry((*tree)->object.sha1, prefix,
+				     tree_sha1, &mode);
+		if (err || !S_ISDIR(mode)) {
+			*reason = "current working directory is untracked";
+			goto out;
+		}
+		free(*tree);
+		*tree = parse_tree_indirect(tree_sha1);
+	}
+	//free(*tree);
+	rv = 0;
+out:
+	return rv;
+}
+
+int parse_archive_args(int argc, const char **argv,
+		       struct archiver_struct **ar,
+		       const char **reason)
+{
+	const char *format = NULL;
+	const char *remote = NULL;
+	const char *prefix = NULL;
+	int list = 0;
+	int rv = -1, i;
+
+	for (i = 1; i < argc; i++) {
+		const char *arg = argv[i];
+
+		if (!strcmp(arg, "--list") || !strcmp(arg, "-l")) {
+			list = 1;
+			continue;
+		}
+		if (!strncmp(arg, "--format=", 9)) {
+			format = arg + 9;
+			continue;
+		}
+		if (!strncmp(arg, "--prefix=", 9)) {
+			prefix = arg + 9;
+			continue;
+		}
+		if (!strncmp(arg, "--remote=", 9)) {
+			remote = arg + 9;
+			continue;
+		}
+		if (arg[0] == '-' && isdigit(arg[1]) && arg[2] == '\0') {
+			zlib_compression_level = arg[1] - '0';
+			continue;
+		}
+		if (!strcmp(arg, "--")) {
+			i++;
+			break;
+		}
+		if (arg[0] == '-') {
+			*reason = archive_usage;
+			goto out;
+		}
+		break;
+	}
+	if (list) {
+		if (!remote) {
+			int i;
+
+			for (i = 0; i < ARRAY_SIZE(archivers); i++)
+				printf("%s\n", archivers[i].name);
+			exit(0);
+		}
+		*reason = "--list and --remote not supported together";
+		goto out;
+	}
+	if (argc - i < 1) {
+		*reason = archive_usage;
+		goto out;
+	}
+	if (!format){
+		*reason = "You must specify an archive format";
+		goto out;
+	}
+	*ar = get_archiver(format);
+	if (*ar == NULL) {
+		*reason = "Unknown archive format";
+		goto out;
+	}
+	if ((*ar)->parse_extra) {
+		if ((*ar)->parse_extra(argc, argv, reason) < 0)
+			goto out;
+	}
+	(*ar)->remote = remote;
+	(*ar)->prefix = prefix ? : "";
+	rv = i;
+out:
+	return rv;
+}
+
+int cmd_archive(int argc, const char **argv, const char *prefix)
+{
+	struct archiver_struct *ar;
+	const char *reason;
+	const char **pathspec;
+	struct tree *tree;
+	const unsigned char *commit_sha1;
+	time_t archive_time;
+	int rv;
+
+	rv = parse_archive_args(argc, argv, &ar, &reason);
+	if (rv < 0)
+		goto err;
+
+	if (ar->remote)
+		return run_remote_archiver(ar, argc, argv);
+
+	if (prefix == NULL)
+		prefix = setup_git_directory();
+
+	argv += rv;
+	if (parse_treeish_arg(argv, &tree, &commit_sha1,
+			      &archive_time, prefix, &reason) < 0)
+		goto err;
+
+	pathspec = get_pathspec(ar->prefix, argv + 1);
+
+	return ar->write_archive(tree, commit_sha1, ar->prefix,
+				 archive_time, pathspec);
+err:
+	return error("%s", reason);
+}
diff --git a/builtin-tar-tree.c b/builtin-tar-tree.c
index 61a4135..e0da01e 100644
--- a/builtin-tar-tree.c
+++ b/builtin-tar-tree.c
@@ -9,6 +9,7 @@ #include "strbuf.h"
 #include "tar.h"
 #include "builtin.h"
 #include "pkt-line.h"
+#include "archive.h"

 #define RECORDSIZE	(512)
 #define BLOCKSIZE	(RECORDSIZE * 20)
@@ -338,6 +339,71 @@ static int generate_tar(int argc, const
 	return 0;
 }

+static int write_tar_entry(const unsigned char *sha1,
+                           const char *base, int baselen,
+                           const char *filename, unsigned mode, int stage)
+{
+	static struct strbuf path;
+	int filenamelen = strlen(filename);
+	void *buffer;
+	char type[20];
+	unsigned long size;
+
+	if (!path.alloc) {
+		path.buf = xmalloc(PATH_MAX);
+		path.alloc = PATH_MAX;
+		path.len = path.eof = 0;
+	}
+	if (path.alloc < baselen + filenamelen) {
+		free(path.buf);
+		path.buf = xmalloc(baselen + filenamelen);
+		path.alloc = baselen + filenamelen;
+	}
+	memcpy(path.buf, base, baselen);
+	memcpy(path.buf + baselen, filename, filenamelen);
+	path.len = baselen + filenamelen;
+	if (S_ISDIR(mode)) {
+		strbuf_append_string(&path, "/");
+		buffer = NULL;
+		size = 0;
+	} else {
+		buffer = read_sha1_file(sha1, type, &size);
+		if (!buffer)
+			die("cannot read %s", sha1_to_hex(sha1));
+	}
+
+	write_entry(sha1, &path, mode, buffer, size);
+
+	return READ_TREE_RECURSIVE;
+}
+
+int write_tar_archive(struct tree *tree, const unsigned char *commit_sha1,
+                      const char *prefix, time_t time, const char **pathspec)
+{
+	int plen = strlen(prefix);
+
+	git_config(git_tar_config);
+
+	archive_time = time;
+
+	if (commit_sha1)
+		write_global_extended_header(commit_sha1);
+
+	if (prefix && plen > 0 && prefix[plen - 1] == '/') {
+		char *base = strdup(prefix);
+		int baselen = strlen(base);
+
+		while (baselen > 0 && base[baselen - 1] == '/')
+			base[--baselen] = '\0';
+		write_tar_entry(tree->object.sha1, "", 0, base, 040777, 0);
+		free(base);
+	}
+	read_tree_recursive(tree, prefix, plen, 0, pathspec, write_tar_entry);
+	write_trailer();
+
+	return 0;
+}
+
 static const char *exec = "git-upload-tar";

 static int remote_tar(int argc, const char **argv)
diff --git a/builtin.h b/builtin.h
index 8472c79..2391afb 100644
--- a/builtin.h
+++ b/builtin.h
@@ -15,6 +15,7 @@ extern int write_tree(unsigned char *sha

 extern int cmd_add(int argc, const char **argv, const char *prefix);
 extern int cmd_apply(int argc, const char **argv, const char *prefix);
+extern int cmd_archive(int argc, const char **argv, const char *prefix);
 extern int cmd_cat_file(int argc, const char **argv, const char *prefix);
 extern int cmd_checkout_index(int argc, const char **argv, const char *prefix);
 extern int cmd_check_ref_format(int argc, const char **argv, const
char *prefix);
diff --git a/generate-cmdlist.sh b/generate-cmdlist.sh
index ec1eda2..5450918 100755
--- a/generate-cmdlist.sh
+++ b/generate-cmdlist.sh
@@ -12,6 +12,7 @@ struct cmdname_help common_cmds[] = {"
 sort <<\EOF |
 add
 apply
+archive
 bisect
 branch
 checkout
diff --git a/git.c b/git.c
index 82c8fee..c62c5cf 100644
--- a/git.c
+++ b/git.c
@@ -218,6 +218,7 @@ static void handle_internal_command(int
 	} commands[] = {
 		{ "add", cmd_add, RUN_SETUP },
 		{ "apply", cmd_apply },
+		{ "archive", cmd_archive },
 		{ "cat-file", cmd_cat_file, RUN_SETUP },
 		{ "checkout-index", cmd_checkout_index, RUN_SETUP },
 		{ "check-ref-format", cmd_check_ref_format },
-- 
1.4.2.gbba4

^ permalink raw reply related

* Re: [PATCH][RFC] Add git-archive-tree
From: Franck Bui-Huu @ 2006-09-05 11:43 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Rene Scharfe, git
In-Reply-To: <7vwt8jl1dv.fsf@assigned-by-dhcp.cox.net>

2006/9/5, Junio C Hamano <junkio@cox.net>:
> Rene Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
>
> > Junio C Hamano schrieb:
> >...
> > Well, this is just _one_ of the positions I've taken on this topic, I
> > have to admit.  Franck then convinced me that merging downloader and
> > archiver into one command is nice for users (less commands to remember,
> > keeps existing --remote option) even if it doesn't make sense
> > technically, because their implementations have nothing in common.
>
> Ah, I was not following the thread closely and I agree with you
> and Franck now in that smaller number of commands the better.
> (IOW, download-archive in my 3-item list is not needed in the
> end user UI and can be implemented as "git-archive-tree
> --remote=<repo>).
>

OK, I'm sending what I've been able to do so far. It has been done
before you sent some feedbacks one day ago, so please forgive me if,
for the moment, I called "git-archive" instead of "git-archive-tree"
or if I missed some of your points; I'll change that.

I'm sending 2 patches, which are sent to get some comments. They're
not for inclusion in anyways.

The pathspec area has been pick up from Rene's initial patch. I have
not enough git's internal knowledge to improve it. But I'll be glad to
look at this later (if you have any clues for starting point BTW).

-- 
               Franck

^ permalink raw reply

* Re: Starting to think about sha-256?
From: linux @ 2006-09-05  9:05 UTC (permalink / raw)
  To: torvalds; +Cc: git, linux

For those worrying, note that even a "complete" break of SHA-1 doesn't
imply the ability to sneak a trojan into a git repository via a patch
or something.

What cryptographers consider a break is finding a collision, two messages
x and y such that hash(x) == hash(y).  But note that the attacker gets
to pick both x and y!

A so-called second pre-image attack, where x is fixed beforehand is far
harder, even if you get to choose from the 320K or so objects in the
linux kernel repository.

Merely inducing you to somehow import the trojan object y into your
object database doesn't help unless you trust and are willing to build
and run source including the object x that it is a doppelgänger for.
So, armed with only a collision-finding attack, the attacker has to
create a collision pair including an innocent source file that can get
included (without any bizarre contents getting "fixed" by a maintainer)
before anyone can attempt to substitute the trojan y.

That requirement eliminates most collision attacks, which generate
good-sized binary blobs.  There's a demo where someone did it with a
postscript file, but that was basically

	if (parity("line noise"))
		print innocent message
	else
		print trojan message

Now, admittedly, this sort of fine reasoning does reduce the possible
application domain of git to human-readable source.  If I'm using git
as a back-end for (say) an archive of statically verified but opaque
java byte-code files, then there's potential for trouble.

But I just want to remind people that even if a "complete" break of SHA-1
were announced tomorrow, it wouldn't imply that using git is dangerous.
Frankly, it would *still* probably be less work to hide a trojan in the
source code well enough that it sneaks under the maintainer's radar.


> Why? Because a git SHA1 is actually _not_ the SHA1 of the file itself, 
> it's the SHA1 of the file _with_the_git_header_added_.
> 
> So if you find two files that have the same SHA1, they would also have to 
> have the same length in order to actually generate the same object name. 
> If they have different lenths, you can just check them into git, and 
> they'll get two different git SHA1 names and you'll have a cool git 
> archive that when you check the files out, they checked-out files will 
> share the same SHA1 ;)

This has always seemed silly to me.  Every cryptographic hash
algorithm since the (catastrophically broken) MD4 has used the standard
Merkle-Damgård strengthening construction to include the length.
There's no point in doing it a second time.

It's done as a suffix, not a prefix, but it's there.  And an odd-sized
prefix makes it hard to have a fast-path aligned hash for the bulk of
the data on machines with alignment restrictions.

Now, the case can be made that a prefix is slightly stronger 
(http://cs.nyu.edu/~puniya/papers/merkle.pdf), but I don't think
that's why it was done.

Note that since the length is included, you can avoid the odd-sized prefix
problem by putting any extra data you like on the hash as a trailer.
As long as it's suffix-free, (e.g. leading null byte), you haven't
hurt anything.



Anyway, SHA-256 is tricky on x86, because it has 8 words of state and
x86 has only 7 registers.

But here's some (public domain) code which works well on x86, and doesn't
suck too badly on other processors.  It's designed to be benchmarked
against Brian Gladman's implementation; if you have that, change the
obvious #define and link against sha2.o.

The other interesting benchmark is "nm -n *.o"; on x86, it's 290 bytes
of core transform vs. 4773 bytes for sha256_compile.


#include <stdint.h>
#include <stdio.h>
#include <string.h>

#define DEBUG 0
#define HAVE_GLADMAN_CODE 0

struct sha256_state {
	uint32_t iv[8];	/* h, g, f, e, d, c, b, a */
	uint32_t w[64];	/* Fill in first 16 with ntohl(input) */
	uint32_t d2, c2, b2, a2;
	uint32_t bytes_hi, bytes_lo;
};

/* Rotate right macro.  GCC can usually get this right. */
#define ROTR(x,s) ((x)>>(s) | (x)<<(32-(s)))

/*
 * An implementation of SHA-256 for register-starved architectures like
 * x86 or perhaps the MSP430.  (Although the latter's lack of a multi-bit
 * shifter will doom its performance no matter what.)
 * This code is also quite small.
 *
 * If you have 12 32-bit registers to work with, loading the 8 state
 * variables into registers is probably faster.  If you have 28 registers
 * or so, you can put the input block into registers as well.
 *
 * The key idea is to notice that each round consumes one word from the
 * key schedule w[i], computes a new a, and shifts all the other state
 * variables down one position, discarding the old h.
 *
 * So if we store the state vector in reverse order h..a, immediately
 * before w[i], then a single base pointer can be incremented to advance
 * to the next round.
 */
void
sha256_transform(uint32_t p[76])
{
	static uint32_t const k[64] = {
		0x428a2f98, 0x71374491, 0xb5c0fbcf, 0xe9b5dba5, 0x3956c25b,
		0x59f111f1, 0x923f82a4, 0xab1c5ed5, 0xd807aa98, 0x12835b01,
		0x243185be, 0x550c7dc3, 0x72be5d74, 0x80deb1fe, 0x9bdc06a7,
		0xc19bf174, 0xe49b69c1, 0xefbe4786, 0x0fc19dc6, 0x240ca1cc,
		0x2de92c6f, 0x4a7484aa, 0x5cb0a9dc, 0x76f988da, 0x983e5152,
		0xa831c66d, 0xb00327c8, 0xbf597fc7, 0xc6e00bf3, 0xd5a79147,
		0x06ca6351, 0x14292967, 0x27b70a85, 0x2e1b2138, 0x4d2c6dfc,
		0x53380d13, 0x650a7354, 0x766a0abb, 0x81c2c92e, 0x92722c85,
		0xa2bfe8a1, 0xa81a664b, 0xc24b8b70, 0xc76c51a3, 0xd192e819,
		0xd6990624, 0xf40e3585, 0x106aa070, 0x19a4c116, 0x1e376c08,
		0x2748774c, 0x34b0bcb5, 0x391c0cb3, 0x4ed8aa4a, 0x5b9cca4f,
		0x682e6ff3, 0x748f82ee, 0x78a5636f, 0x84c87814, 0x8cc70208,
		0x90befffa, 0xa4506ceb, 0xbef9a3f7, 0xc67178f2
	};
	/*
	 * Look, ma, only 6 local variables including p!
	 * Too bad they're so overloaded it's impossible to give them
	 * meaningful names.
	 */
	register uint32_t const *kp;
	register uint32_t a, s, t, u;

	/* Step 1: Expand the 16 words of w[], at p[8..23] into 64 words */
	for (u = 8; u < 8+64-16; u++) {
		/* w[i] = s1(w[i-2]) + w[i-7] + s0(w[i-15]) + w[i-16] */
		/* Form s0(x) = (x >>> 7) ^ (x >>> 18) ^ (x >> 3) */
		s = t = p[u+1];
		s = ROTR(s, 18-7);
		s ^= t;
		s = ROTR(s, 7);
		s ^= t >> 3;
		/* Form s1(x) = (x >>> 17) ^ (x >>> 19) ^ (x >> 10) */
		a = t = p[u+14];
		a = ROTR(a, 19-17);
		a ^= t;
		a = ROTR(a, 17);
		a ^= t >> 10;

		p[u+16] = s + a + p[u] + p[u+9];
	}

	/* Step 2: Copy the initial values of d, c, b, a out of the way */
	p[72] = p[4];
	p[73] = p[5];
	p[74] = p[6];
	p[75] = a = p[7];

	/*
	 * Step 3: The big loop.
	 * We maintain p[0..7] = h..a, and p[8] is w[i]
	 * Only the variable a is actually kept in a local register.
	 */
	kp = k;

	do {
		/* T1 = h + S1(e) + Ch(e,f,g) + k[i] + w[i] */
		/* Form Ch(e,f,g) = g ^ (e & (f ^ g)) */
		s = t = p[1];	/* g */
		s ^= p[2];	/* f ^ g */
		s &= u = p[3];	/* e & (f ^ g), copy of e left in u */
		s ^= t;
		/* Form S1(e) = (e >>> 6) ^ (e >>> 11) ^ (e >>> 25) */
		t = u;
		u = ROTR(u, 25-11);
		u ^= t;
		u = ROTR(u, 11-6);
		u ^= t;
		u = ROTR(u, 6);
		s += u;
		/* Now add other things to t1 */
		s += p[0] + p[8] + *kp;	/* h + w[i] + kp[i] */
		/* Round function: e = d + T1 */
		p[4] += s;
		/* a = t1 + (t2 = S0(a) + Maj(a,b,c) */
		/* Form S0(a) = (a >>> 2) ^ (a >>> 13) ^ (a >>> 22) */
		t = a;
		t = ROTR(t, 22-13);
		t ^= a;
		t = ROTR(t, 13-2);
		t ^= a;
		t = ROTR(t, 2);
		s += t;
		/* Form Maj(a,b,c) = (a & b) + (c & (a ^ b)) */
		u = p[6];	/* b */
		t = a;
		a ^= u;		/* a ^ b */
		u &= t;		/* a & b */
		a &= p[5];	/* c & (a + b) */
		s += u;
		a += s;	/* Sum final result into a */

		/* Now store new a on top of w[i] and shift... */
		p[8] = a;
		p++;
#if DEBUG 
		/* If debugging, print out the state variables each round */
		printf("%2u:", kp-k);
		for (t = 8; t--; )
			printf(" %08x", p[t]);
		putchar('\n');
#endif
	} while (++kp != k+64);

	/* Now, do the final summation. */
	kp = p;
	p -= 64;
	/*
	 * Now, the final h..a are in p[64..71], and the initial values
	 * are in p[0..7].  Except that p[4..7] got trashed in the loop
	 * above, so use the copies we made.
	 */
	p[0] += kp[0];
	p[1] += kp[1];
	p[2] += kp[2];
	p[3] += kp[3];
	p[4] = kp[4] + kp[8];
	p[5] = kp[5] + kp[9];
	p[6] = kp[6] + kp[10];
	p[7] = a     + kp[11];
}

/* Initial values H7..H0 for SHA-256, and SHA-224. Note reverse order! */
static uint32_t const _sha256_iv[8] = {
	0x5be0cd19, 0x1f83d9ab, 0x9b05688c, 0x510e527f,
	0xa54ff53a, 0x3c6ef372, 0xbb67ae85, 0x6a09e667
};
static uint32_t const _sha224_iv[8] = {
	0xbefa4fa4, 0x64f98fa7, 0x68581511, 0xffc00b31,
	0xf70e5939, 0x3070dd17, 0x367cd507, 0xc1059ed8
};

#if HAVE_GLADMAN_CODE

#include "sha2.h"

#else

/* Compatibility wrappers */

typedef struct sha256_state sha256_ctx;

void
sha256_begin(struct sha256_state *s)
{
	memcpy(s->iv, _sha256_iv, sizeof _sha256_iv);
	s->bytes_lo = s->bytes_hi = 0;
}

#include <netinet/in.h>	/* For ntohl, htohl */

void
sha256_hash(unsigned char const *data, size_t len, struct sha256_state *s)
{
	unsigned space = 64 - (unsigned)s->bytes_lo % 64;
	unsigned i;

	s->bytes_lo += (uint32_t)len;
	s->bytes_hi += s->bytes_lo < (uint32_t)len;
	if ((size_t)-1 > (uint32_t)-1)
		s->bytes_hi += (uint32_t)(len >> 16 >> 16);

	while (len >= space) {
		memcpy((unsigned char *)s->w + 64 - space, data, space);
		len -= space;
		space = 64;
		for (i = 0; i < 16; i++)
			s->w[i] = ntohl(s->w[i]);
		sha256_transform(s->iv);
	}
	memcpy((unsigned char *)s->w + 64 - space, data, len);
}

void
sha256_end(unsigned char hash[32], struct sha256_state *s)
{
	unsigned i, pos = (unsigned)s->bytes_lo % 64;
	uint32_t *out;

	((unsigned char *)s->w)[pos++] = 0x80;
	if (pos > 56) {
		memset((unsigned char *)s->w + pos, 0, 64-pos);
		for (i = 0; i < 16; i++)
			s->w[i] = ntohl(s->w[i]);
		sha256_transform(s->iv);
		pos = 0;
	}
	memset((unsigned char *)s->w + pos, 0, 56-pos);
	for (i = 0; i < 14; i++)
		s->w[i] = ntohl(s->w[i]);
	s->w[15] = s->bytes_lo << 3;
	s->w[14] = s->bytes_hi << 3 | s->bytes_lo >> 29;
	sha256_transform(s->iv);

	out = (unsigned)hash % sizeof s->iv[0] ? s->w : (uint32_t *)hash;
	for (i = 0; i < 8; i++)
		out[i] = htonl(s->iv[7-i]);
	if (out == s->w)
		memcpy(hash, out, sizeof s->iv);
	memset(s, 0, sizeof s);	/* Good cryptographic hygiene */
}

void
sha256(unsigned char hash[32], const unsigned char *data, size_t len)
{
	struct sha256_state s;
	sha256_begin(&s);
	sha256_hash(data, len, &s);
	sha256_end(hash, &s);
}

#endif /* !HAVE_GLADMAN_CODE */

#include <string.h>
#include <sys/time.h>

#define TESTSIZE 100000000
#if TESTSIZE & 64 != 0
#error For now, TESTSIZE must be a multiple of 64.
#endif

int
main(void)
{
	uint32_t array1[76] = {
		/* Initial values, h..a */
		0x5be0cd19, 0x1f83d9ab, 0x9b05688c, 0x510e527f,
		0xa54ff53a, 0x3c6ef372, 0xbb67ae85, 0x6a09e667,
		/* First 16 w[i] values */
		0x61626380, 0, 0, 0, 0, 0, 0, 0,
		0, 0, 0, 0, 0, 0, 0, 0x00000018
	};
	uint32_t array2[76] = {
		/* Initial values, h..a */
		0x5be0cd19, 0x1f83d9ab, 0x9b05688c, 0x510e527f,
		0xa54ff53a, 0x3c6ef372, 0xbb67ae85, 0x6a09e667,
		/* First 16 w[i] values */
		0x61626364, 0x62636465, 0x63646566, 0x64656667,
		0x65666768, 0x66676869, 0x6768696a, 0x68696a6b,
		0x696a6b6c, 0x6a6b6c6d, 0x6b6c6d6e, 0x6c6d6e6f,
		0x6d6e6f70, 0x6e6f7071, 0x80000000, 0x00000000
	};
	uint32_t array3[76] = {
		/* Initial values, h..a */
		0x5be0cd19, 0x1f83d9ab, 0x9b05688c, 0x510e527f,
		0xa54ff53a, 0x3c6ef372, 0xbb67ae85, 0x6a09e667
	};
	unsigned i;
	struct timeval start, stop;
	sha256_ctx s256;
	unsigned char buf[64];

	/* The FIPS180-2 appendix B.1 example */
	printf("Appendix B.1 example:\n");
	sha256_transform(array1);
	printf("==>");
	for (i = 8; i--; )
		printf(" %08x", array1[i]);
	putchar('\n');

	printf("\nAppendix B.2 example:\n");
	sha256_transform(array2);
	printf("==>");
	for (i = 8; i--; )
		printf(" %08x", array2[i]);
	putchar('\n');
	putchar('\n');

	for(i = 8; i < 23; i++)
		array2[i] = 0;
	array2[23] = 0x000001c0;
	sha256_transform(array2);
	printf("==>");
	for (i = 8; i--; )
		printf(" %08x", array2[i]);
	putchar('\n');

	/* Now to hash 1,000,000 letters 'a' */
	printf("\nAppendix B.3 example:\n");

	gettimeofday(&start, 0);
	for (i = 0; i < TESTSIZE/64; i++) {
		memset((char *)(array3+8), 'a', 64);
		sha256_transform(array3);
	}
	array3[8] = 0x80000000;
	array3[23] = i*64*8;	/* Length in bits */
	for (i = 9; i < 23; i++)
		array3[i] = 0;
	sha256_transform(array3);
	gettimeofday(&stop, 0);
	stop.tv_sec -= start.tv_sec;
	stop.tv_usec -= start.tv_usec;
	if (stop.tv_usec < 0) {
		stop.tv_usec += 1000000;
		stop.tv_sec--;
	}
	printf("New code: %u.%06ld\n", (unsigned)stop.tv_sec, stop.tv_usec);
	printf("==>");
	for (i = 8; i--; )
		printf(" %08x", array3[i]);
	putchar('\n');


	sha256_begin(&s256);
	gettimeofday(&start, 0);
	for (i = 0; i < TESTSIZE/64; i++) {
		memset(buf, 'a', 64);
		sha256_hash(buf, 64, &s256);
	}
	sha256_end(buf, &s256);
	gettimeofday(&stop, 0);
	stop.tv_sec -= start.tv_sec;
	stop.tv_usec -= start.tv_usec;
	if (stop.tv_usec < 0) {
		stop.tv_usec += 1000000;
		stop.tv_sec--;
	}
	printf(
#if HAVE_GLADMAN_CODE
		"Gladman code: %u.%06ld\n",
#else
		"Wrapper code: %u.%06ld\n",
#endif
		(unsigned)stop.tv_sec, stop.tv_usec);

	printf("==>");
	for (i = 0; i < 32; i++) {
		if (i % 4 == 0)
			putchar(' ');
		printf("%02x", buf[i]);
	}
	putchar('\n');

	return 0;
}

^ permalink raw reply

* Re: gitweb testing and benchmarking (was: [PATCH 0/4] gitweb: Some improvements)
From: Jakub Narebski @ 2006-09-05  8:59 UTC (permalink / raw)
  To: git
In-Reply-To: <ediea9$d3d$2@sea.gmane.org>

Jakub Narebski wrote:

> Junio C Hamano wrote:
>
>> It is a while since I tried gitweb on my machine the last time
>> but was it always this slow I wonder...  We probably would need
>> a good benchmark and automated test before going too much
>> further.
> 
> The problem is that before commit 5d043a3d856bd40d8b34b8836a561e438d23573b
>   gitweb: fill in gitweb configuration by Makefile
> by Martin Waitz one had to modify gitweb script to change
> the configuration from default.
> 
> But benchmarking is good. Simple time to run script from command line,
> with environment variables GATEWAY_INTERFACE="CGI/1.1", HTTP_ACCEPT="*/*",
> REQUEST_METHOD="GET" and of course QUERY_STRING set, and perhaps using
> ApacheBench.

I think that for testing gitweb we will be better with creating some twisted
test repository (c.f. t/t4114-apply-typechange.sh), but for benchmarking it
would be better to use some larger repository, for example git repository
cut-off from above at some version (including tags!). Is there a way to do
that?
-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

^ permalink raw reply

* Re: New git commit tool
From: Karl Hasselström @ 2006-09-05  8:45 UTC (permalink / raw)
  To: Paul Mackerras; +Cc: git
In-Reply-To: <17660.39046.767944.582869@cargo.ozlabs.ibm.com>

On 2006-09-05 07:20:06 +1000, Paul Mackerras wrote:

> I was calling it "gitt" for a long time, which is at least shorter
> and easier to type. I also thought of "tick" (anagram of tk + "ci"
> for check in), but maybe all that shows I've been doing too many
> cryptic crosswords lately. :) The "gitool" was by analogy with the
> "citool" of another system we've both used. I'm happy to change it
> if someone has a better idea...

gitci?

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

^ permalink raw reply

* Re: Test names
From: Junio C Hamano @ 2006-09-05  8:28 UTC (permalink / raw)
  To: Jakub Narebski; +Cc: git
In-Reply-To: <edifa7$h29$1@sea.gmane.org>

Jakub Narebski <jnareb@gmail.com> writes:

> In t/README it is written:
>...
> Is there any naming rule, or just increment it by one when adding new test
> in the same family?
>
>> Third digit (optionally) tells the particular switch or group of switches
>>  we are testing.
>
> What tells the fourth digit?

Nothing other than just to make them aline up and ordered
nicely.

The rule was outlined long time ago but not strictly followed
(primarily because was done when git was mostly core, and it did
not anticipate the proliferation of Porcelain-ish wrappers, so
it was rather hard to follow).  I'd suggest you to declare that
t92xx- series are for gitweb, and organize the tests in that
namespace nicely.  Also you might want to see what Eric did for
the subversion interface tests (60d02cc declares that t91xx-
series are SVN tests and gives an easy way to skip heavier tests
on git-svn).

^ permalink raw reply

* Re: [PATCH] pack-objects: re-validate data we copy from elsewhere.
From: Junio C Hamano @ 2006-09-05  8:12 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: git
In-Reply-To: <Pine.LNX.4.64.0609021720500.27779@g5.osdl.org>

Linus Torvalds <torvalds@osdl.org> writes:

> On Sat, 2 Sep 2006, Junio C Hamano wrote:
>> 
>> repack without -a essentially boils down to:
>> 
>> 	rev-list --objects --all --unpacked |
>>         pack-objects $new_pack
>> 
>> which picks up all live loose objects and create a new pack.
>> 
>> If rev-list had an extention that lets you say
>> 
>> 	rev-list --objects --all --unpacked=$active_pack |
>> 	pack-objects $new_pack
>
> I have to say, that adding another pack-objects-specific flag to rev-list 
> doesn't necessarily strike me as very nice.
>
> It might be much better to just teach "git pack-objects" to walk the 
> object list by hand. The whole "git-rev-list | git-pack-objects" approach 
> made more sense back when git-rev-list was the _only_ think listing 
> objects, but now that we've librarized the whole object listing thing, 
> maybe it's time to avoid the pipeline and just do it inside the packer 
> logic.
>
> Of course, having git-pack-objects be able to take input from stdin is 
> still useful, but I'd rather start moving the obviously packing-specific 
> flags out of git-rev-list, and into git-pack-objects instead.
>
> [ That would also potentially make packing more efficient - right now the 
>   "phase 1" of packing is literally to just figure out the type and the 
>   size of the object, in order to sort the object list.
>
>   So when you do a big repack, first "git-rev-list" ends up doing all this 
>   work, and then "git-pack-object" ends up _redoing_ some of it. 
>
>   Especially for tree objects (which are one of the most common kinds), we 
>   already opened the object once when we traversed it, so opening it again 
>   just to look at its size is kind of sad.. ]

Well, I tried (see two patches from tonight, and the result is
sitting in "pu"), but it turns out that the information
pack-objects wants to get in check_object() is somewhat
different from what "struct object" family is willing to give
back easily during the traversal done by get_revision()
interface.

Since "struct object" traditionally has not had provision of
recording size (created_object() interface just takes sha1
without size, for example), and it was slimmed down to contain
absolute minimum information, I am reluctant to add fields to
record the size there.  Also having in-pack size available would
be rather nice but the functions involved in the call chain
(including process_tree and process_blob) did not even care
where the objects are stored, so it would involve rather nasty
surgery.  So I stopped short of that.  I am not quite sure how
useful tonight's patches are in practice.  It gets rid of the
pipe, but moves the revision walker memory pressure from rev-list
to pack-objects itself, so...

^ permalink raw reply

* Re: [PATCH 3/5] autoconf: Preliminary check for working mmap
From: Junio C Hamano @ 2006-09-05  7:43 UTC (permalink / raw)
  To: Shawn Pearce; +Cc: git
In-Reply-To: <20060905062531.GA30496@spearce.org>

Shawn Pearce <spearce@spearce.org> writes:

> I don't know if I've made this more complex than I really need to
> but I've permitted multiple windows per pack.  There is just one
> LRU of all windows across all packs and a maximum amount of address
> space to use for pack mappings.  Least recently used window gets
> tossed when we need a different window.  This permits us to keep
> say a window active on the front of a pack (near the commits) and
> another different active window closer to the back (near the blobs).

Sounds good. That is exactly what I was expecting it to be done.

^ permalink raw reply

* [PATCH 1/2] Separate object listing routines out of rev-list
From: Junio C Hamano @ 2006-09-05  7:30 UTC (permalink / raw)
  To: git

Create a separate file, list-objects.c, and move object listing
routines from rev-list to it.  The next round will use it in
pack-objects directly.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---
 Makefile           |    4 +-
 builtin-rev-list.c |  110 ++++++----------------------------------------------
 list-objects.c     |  107 +++++++++++++++++++++++++++++++++++++++++++++++++++
 list-objects.h     |    8 ++++
 4 files changed, 130 insertions(+), 99 deletions(-)

diff --git a/Makefile b/Makefile
index 7b3114f..18cd79e 100644
--- a/Makefile
+++ b/Makefile
@@ -233,7 +233,7 @@ XDIFF_LIB=xdiff/lib.a
 
 LIB_H = \
 	blob.h cache.h commit.h csum-file.h delta.h \
-	diff.h object.h pack.h pkt-line.h quote.h refs.h \
+	diff.h object.h pack.h pkt-line.h quote.h refs.h list-objects.h \
 	run-command.h strbuf.h tag.h tree.h git-compat-util.h revision.h \
 	tree-walk.h log-tree.h dir.h path-list.h unpack-trees.h builtin.h
 
@@ -250,7 +250,7 @@ LIB_OBJS = \
 	server-info.o setup.o sha1_file.o sha1_name.o strbuf.o \
 	tag.o tree.o usage.o config.o environment.o ctype.o copy.o \
 	fetch-clone.o revision.o pager.o tree-walk.o xdiff-interface.o \
-	write_or_die.o trace.o \
+	write_or_die.o trace.o list-objects.o \
 	alloc.o merge-file.o path-list.o help.o unpack-trees.o $(DIFF_OBJS)
 
 BUILTIN_OBJS = \
diff --git a/builtin-rev-list.c b/builtin-rev-list.c
index 8437454..d6dcba2 100644
--- a/builtin-rev-list.c
+++ b/builtin-rev-list.c
@@ -7,6 +7,7 @@ #include "blob.h"
 #include "tree-walk.h"
 #include "diff.h"
 #include "revision.h"
+#include "list-objects.h"
 #include "builtin.h"
 
 /* bits #0-15 in revision.h */
@@ -97,104 +98,19 @@ static void show_commit(struct commit *c
 	commit->buffer = NULL;
 }
 
-static void process_blob(struct blob *blob,
-			 struct object_array *p,
-			 struct name_path *path,
-			 const char *name)
+static void show_object(struct object_array_entry *p)
 {
-	struct object *obj = &blob->object;
-
-	if (!revs.blob_objects)
-		return;
-	if (obj->flags & (UNINTERESTING | SEEN))
-		return;
-	obj->flags |= SEEN;
-	name = xstrdup(name);
-	add_object(obj, p, path, name);
-}
-
-static void process_tree(struct tree *tree,
-			 struct object_array *p,
-			 struct name_path *path,
-			 const char *name)
-{
-	struct object *obj = &tree->object;
-	struct tree_desc desc;
-	struct name_entry entry;
-	struct name_path me;
-
-	if (!revs.tree_objects)
-		return;
-	if (obj->flags & (UNINTERESTING | SEEN))
-		return;
-	if (parse_tree(tree) < 0)
-		die("bad tree object %s", sha1_to_hex(obj->sha1));
-	obj->flags |= SEEN;
-	name = xstrdup(name);
-	add_object(obj, p, path, name);
-	me.up = path;
-	me.elem = name;
-	me.elem_len = strlen(name);
-
-	desc.buf = tree->buffer;
-	desc.size = tree->size;
-
-	while (tree_entry(&desc, &entry)) {
-		if (S_ISDIR(entry.mode))
-			process_tree(lookup_tree(entry.sha1), p, &me, entry.path);
-		else
-			process_blob(lookup_blob(entry.sha1), p, &me, entry.path);
-	}
-	free(tree->buffer);
-	tree->buffer = NULL;
-}
-
-static void show_commit_list(struct rev_info *revs)
-{
-	int i;
-	struct commit *commit;
-	struct object_array objects = { 0, 0, NULL };
-
-	while ((commit = get_revision(revs)) != NULL) {
-		process_tree(commit->tree, &objects, NULL, "");
-		show_commit(commit);
-	}
-	for (i = 0; i < revs->pending.nr; i++) {
-		struct object_array_entry *pending = revs->pending.objects + i;
-		struct object *obj = pending->item;
-		const char *name = pending->name;
-		if (obj->flags & (UNINTERESTING | SEEN))
-			continue;
-		if (obj->type == OBJ_TAG) {
-			obj->flags |= SEEN;
-			add_object_array(obj, name, &objects);
-			continue;
-		}
-		if (obj->type == OBJ_TREE) {
-			process_tree((struct tree *)obj, &objects, NULL, name);
-			continue;
-		}
-		if (obj->type == OBJ_BLOB) {
-			process_blob((struct blob *)obj, &objects, NULL, name);
-			continue;
-		}
-		die("unknown pending object %s (%s)", sha1_to_hex(obj->sha1), name);
-	}
-	for (i = 0; i < objects.nr; i++) {
-		struct object_array_entry *p = objects.objects + i;
-
-		/* An object with name "foo\n0000000..." can be used to
-		 * confuse downstream git-pack-objects very badly.
-		 */
-		const char *ep = strchr(p->name, '\n');
-		if (ep) {
-			printf("%s %.*s\n", sha1_to_hex(p->item->sha1),
-			       (int) (ep - p->name),
-			       p->name);
-		}
-		else
-			printf("%s %s\n", sha1_to_hex(p->item->sha1), p->name);
+	/* An object with name "foo\n0000000..." can be used to
+	 * confuse downstream git-pack-objects very badly.
+	 */
+	const char *ep = strchr(p->name, '\n');
+	if (ep) {
+		printf("%s %.*s\n", sha1_to_hex(p->item->sha1),
+		       (int) (ep - p->name),
+		       p->name);
 	}
+	else
+		printf("%s %s\n", sha1_to_hex(p->item->sha1), p->name);
 }
 
 /*
@@ -364,7 +280,7 @@ int cmd_rev_list(int argc, const char **
 	if (bisect_list)
 		revs.commits = find_bisection(revs.commits);
 
-	show_commit_list(&revs);
+	traverse_commit_list(&revs, show_commit, show_object);
 
 	return 0;
 }
diff --git a/list-objects.c b/list-objects.c
new file mode 100644
index 0000000..adaf997
--- /dev/null
+++ b/list-objects.c
@@ -0,0 +1,107 @@
+#include "cache.h"
+#include "tag.h"
+#include "commit.h"
+#include "tree.h"
+#include "blob.h"
+#include "diff.h"
+#include "tree-walk.h"
+#include "revision.h"
+#include "list-objects.h"
+
+static void process_blob(struct rev_info *revs,
+			 struct blob *blob,
+			 struct object_array *p,
+			 struct name_path *path,
+			 const char *name)
+{
+	struct object *obj = &blob->object;
+
+	if (!revs->blob_objects)
+		return;
+	if (obj->flags & (UNINTERESTING | SEEN))
+		return;
+	obj->flags |= SEEN;
+	name = xstrdup(name);
+	add_object(obj, p, path, name);
+}
+
+static void process_tree(struct rev_info *revs,
+			 struct tree *tree,
+			 struct object_array *p,
+			 struct name_path *path,
+			 const char *name)
+{
+	struct object *obj = &tree->object;
+	struct tree_desc desc;
+	struct name_entry entry;
+	struct name_path me;
+
+	if (!revs->tree_objects)
+		return;
+	if (obj->flags & (UNINTERESTING | SEEN))
+		return;
+	if (parse_tree(tree) < 0)
+		die("bad tree object %s", sha1_to_hex(obj->sha1));
+	obj->flags |= SEEN;
+	name = xstrdup(name);
+	add_object(obj, p, path, name);
+	me.up = path;
+	me.elem = name;
+	me.elem_len = strlen(name);
+
+	desc.buf = tree->buffer;
+	desc.size = tree->size;
+
+	while (tree_entry(&desc, &entry)) {
+		if (S_ISDIR(entry.mode))
+			process_tree(revs,
+				     lookup_tree(entry.sha1),
+				     p, &me, entry.path);
+		else
+			process_blob(revs,
+				     lookup_blob(entry.sha1),
+				     p, &me, entry.path);
+	}
+	free(tree->buffer);
+	tree->buffer = NULL;
+}
+
+void traverse_commit_list(struct rev_info *revs,
+			  void (*show_commit)(struct commit *),
+			  void (*show_object)(struct object_array_entry *))
+{
+	int i;
+	struct commit *commit;
+	struct object_array objects = { 0, 0, NULL };
+
+	while ((commit = get_revision(revs)) != NULL) {
+		process_tree(revs, commit->tree, &objects, NULL, "");
+		show_commit(commit);
+	}
+	for (i = 0; i < revs->pending.nr; i++) {
+		struct object_array_entry *pending = revs->pending.objects + i;
+		struct object *obj = pending->item;
+		const char *name = pending->name;
+		if (obj->flags & (UNINTERESTING | SEEN))
+			continue;
+		if (obj->type == OBJ_TAG) {
+			obj->flags |= SEEN;
+			add_object_array(obj, name, &objects);
+			continue;
+		}
+		if (obj->type == OBJ_TREE) {
+			process_tree(revs, (struct tree *)obj, &objects,
+				     NULL, name);
+			continue;
+		}
+		if (obj->type == OBJ_BLOB) {
+			process_blob(revs, (struct blob *)obj, &objects,
+				     NULL, name);
+			continue;
+		}
+		die("unknown pending object %s (%s)",
+		    sha1_to_hex(obj->sha1), name);
+	}
+	for (i = 0; i < objects.nr; i++)
+		show_object(&objects.objects[i]);
+}
diff --git a/list-objects.h b/list-objects.h
new file mode 100644
index 0000000..8a5fae6
--- /dev/null
+++ b/list-objects.h
@@ -0,0 +1,8 @@
+#ifndef LIST_OBJECTS_H
+#define LIST_OBJECTS_H
+
+void traverse_commit_list(struct rev_info *revs,
+			  void (*show_commit)(struct commit *),
+			  void (*show_object)(struct object_array_entry *));
+
+#endif
-- 
1.4.2.g552e5

^ permalink raw reply related

* [PATCH 2/2] pack-object: run rev-list equivalent internally.
From: Junio C Hamano @ 2006-09-05  7:30 UTC (permalink / raw)
  To: git

Instead of piping the rev-list output from its standard input,
you can say:

	pack-object --revs pack --all

I am not happy about the command line parameter handling of this
patch, so the syntax is very likely to change by the final
version.  Currently it understands the following:

	pack-object [options including --stdout]
	pack-object [options] pack-base-name
	pack-object [options] --revs pack-base-name [list options] refs...
	pack-object [options including --stdout] --revs [list options] refs...

The first two are for traditional behaviour and the latter two
are the new ones.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---

 * This does not contain the "pretend that objects in these
   packs are loose when handing --unpacked requests" I've sent
   earlier.  I've also considered doing without --revs but the
   parser can quickly get confused between pack-base-name and a
   ref given to (internal) rev-list.  "pack-object foobla" might
   be reading from standard input and creating foobla-*.pack, or
   the user wanted to do a rev-list starting from foobla and
   forgot to specify the base name, for example.

 builtin-pack-objects.c |  273 ++++++++++++++++++++++++++++++++++--------------
 1 files changed, 192 insertions(+), 81 deletions(-)

diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 149fa28..74daf37 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -9,10 +9,13 @@ #include "delta.h"
 #include "pack.h"
 #include "csum-file.h"
 #include "tree-walk.h"
+#include "diff.h"
+#include "revision.h"
+#include "list-objects.h"
 #include <sys/time.h>
 #include <signal.h>
 
-static const char pack_usage[] = "git-pack-objects [-q] [--no-reuse-delta] [--non-empty] [--local] [--incremental] [--window=N] [--depth=N] {--stdout | base-name} < object-list";
+static const char pack_usage[] = "git-pack-objects [-q] [--no-reuse-delta] [--non-empty] [--local] [--incremental] [--window=N] [--depth=N] {--stdout | base-name} [ --revs [--unpacked | --all | --not | <ref>]* | <object-list]";
 
 struct object_entry {
 	unsigned char sha1[20];
@@ -1326,13 +1329,109 @@ static int git_pack_config(const char *k
 	return git_default_config(k, v);
 }
 
+static void read_object_list_from_stdin(void)
+{
+	int num_preferred_base = 0;
+	char line[40 + 1 + PATH_MAX + 2];
+	unsigned char sha1[20];
+	unsigned hash;
+
+	for (;;) {
+		if (!fgets(line, sizeof(line), stdin)) {
+			if (feof(stdin))
+				break;
+			if (!ferror(stdin))
+				die("fgets returned NULL, not EOF, not error!");
+			if (errno != EINTR)
+				die("fgets: %s", strerror(errno));
+			clearerr(stdin);
+			continue;
+		}
+		if (line[0] == '-') {
+			if (get_sha1_hex(line+1, sha1))
+				die("expected edge sha1, got garbage:\n %s",
+				    line);
+			if (num_preferred_base++ < window)
+				add_preferred_base(sha1);
+			continue;
+		}
+		if (get_sha1_hex(line, sha1))
+			die("expected sha1, got garbage:\n %s", line);
+
+		hash = name_hash(line+41);
+		add_preferred_base_object(line+41, hash);
+		add_object_entry(sha1, hash, 0);
+	}
+}
+
+/* copied from rev-list but needs to do things slightly differently */
+static void mark_edge_parents_uninteresting(struct commit *commit)
+{
+	struct commit_list *parents;
+
+	for (parents = commit->parents; parents; parents = parents->next) {
+		struct commit *parent = parents->item;
+		if (!(parent->object.flags & UNINTERESTING))
+			continue;
+		mark_tree_uninteresting(parent->tree);
+	}
+}
+
+static void mark_edges_uninteresting(struct commit_list *list)
+{
+	for ( ; list; list = list->next) {
+		struct commit *commit = list->item;
+
+		if (commit->object.flags & UNINTERESTING) {
+			mark_tree_uninteresting(commit->tree);
+			continue;
+		}
+		mark_edge_parents_uninteresting(commit);
+	}
+}
+
+static void show_commit(struct commit *commit)
+{
+	unsigned hash = name_hash("");
+	add_object_entry(commit->object.sha1, hash, 0);
+}
+
+static void show_object(struct object_array_entry *p)
+{
+	unsigned hash = name_hash(p->name);
+	add_object_entry(p->item->sha1, hash, 0);
+}
+
+static void get_object_list(int ac, const char **av)
+{
+	struct rev_info revs;
+	const char **revav = xcalloc(ac + 3, sizeof(*revav));
+
+	revav[0] = "pack-objects";
+	revav[1] = "--objects";
+	memcpy(revav + 2, av, sizeof(*av) * ac);
+
+	init_revisions(&revs, NULL);
+	save_commit_buffer = 0;
+	track_object_refs = 0;
+	setup_revisions(ac + 2, revav, &revs, NULL);
+
+	/* make sure we did not get pathspecs */
+	if (revs.prune_data)
+		die("pathspec given");
+
+	prepare_revision_walk(&revs);
+	mark_edges_uninteresting(revs.commits);
+
+	traverse_commit_list(&revs, show_commit, show_object);
+}
+
 int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 {
 	SHA_CTX ctx;
-	char line[40 + 1 + PATH_MAX + 2];
 	int depth = 10;
 	struct object_entry **list;
-	int num_preferred_base = 0;
+	int read_from_stdin = 1;
 	int i;
 
 	git_config(git_pack_config);
@@ -1341,58 +1440,94 @@ int cmd_pack_objects(int argc, const cha
 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];
 
-		if (*arg == '-') {
-			if (!strcmp("--non-empty", arg)) {
-				non_empty = 1;
-				continue;
-			}
-			if (!strcmp("--local", arg)) {
-				local = 1;
-				continue;
-			}
-			if (!strcmp("--progress", arg)) {
-				progress = 1;
-				continue;
-			}
-			if (!strcmp("--incremental", arg)) {
-				incremental = 1;
-				continue;
-			}
-			if (!strncmp("--window=", arg, 9)) {
-				char *end;
-				window = strtoul(arg+9, &end, 0);
-				if (!arg[9] || *end)
-					usage(pack_usage);
-				continue;
-			}
-			if (!strncmp("--depth=", arg, 8)) {
-				char *end;
-				depth = strtoul(arg+8, &end, 0);
-				if (!arg[8] || *end)
-					usage(pack_usage);
-				continue;
-			}
-			if (!strcmp("--progress", arg)) {
-				progress = 1;
-				continue;
-			}
-			if (!strcmp("-q", arg)) {
-				progress = 0;
-				continue;
-			}
-			if (!strcmp("--no-reuse-delta", arg)) {
-				no_reuse_delta = 1;
-				continue;
-			}
-			if (!strcmp("--stdout", arg)) {
-				pack_to_stdout = 1;
+		if (*arg != '-')
+			break;
+
+		if (!strcmp("--non-empty", arg)) {
+			non_empty = 1;
+			continue;
+		}
+		if (!strcmp("--local", arg)) {
+			local = 1;
+			continue;
+		}
+		if (!strcmp("--progress", arg)) {
+			progress = 1;
+			continue;
+		}
+		if (!strcmp("--incremental", arg)) {
+			incremental = 1;
+			continue;
+		}
+		if (!strncmp("--window=", arg, 9)) {
+			char *end;
+			window = strtoul(arg+9, &end, 0);
+			if (!arg[9] || *end)
+				usage(pack_usage);
+			continue;
+		}
+		if (!strncmp("--depth=", arg, 8)) {
+			char *end;
+			depth = strtoul(arg+8, &end, 0);
+			if (!arg[8] || *end)
+				usage(pack_usage);
+			continue;
+		}
+		if (!strcmp("--progress", arg)) {
+			progress = 1;
+			continue;
+		}
+		if (!strcmp("-q", arg)) {
+			progress = 0;
+			continue;
+		}
+		if (!strcmp("--no-reuse-delta", arg)) {
+			no_reuse_delta = 1;
+			continue;
+		}
+		if (!strcmp("--stdout", arg)) {
+			pack_to_stdout = 1;
+			continue;
+		}
+		if (!strcmp("--revs", arg)) {
+			read_from_stdin = 0;
+			i++;
+			break;
+		}
+		usage(pack_usage);
+	}
+
+	/* Traditionally "pack-objects [options] base extra" failed;
+	 * we would however want to take refs parameter that would
+	 * have been given to upstream rev-list ourselves, which means
+	 * we somehow want to say what the base name is.  So the
+	 * syntax would be:
+	 *
+	 * pack-objects [options] base <refs...>
+	 *
+	 * in other words, we would treat the first non-option as the
+	 * base_name and send everything else to the internal revision
+	 * walker.
+	 */
+
+	if (!pack_to_stdout)
+		base_name = argv[i++];
+
+	if (!read_from_stdin) {
+		int j;
+		/* the rest are used as refs parameter to rev-list;
+		 * we only allow refs (including a..b and a...b notation),
+		 * --all, --not, and --unpacked.
+		 */
+		for (j = i; j < argc; j++) {
+			const char *arg = argv[j];
+			if ((arg[0] != '-') ||
+			    !strcmp(arg, "--unpacked") ||
+			    !strcmp(arg, "--all") ||
+			    !strcmp(arg, "--not"))
 				continue;
-			}
-			usage(pack_usage);
+			die("cannot use --revs with %s", arg);
 		}
-		if (base_name)
-			usage(pack_usage);
-		base_name = arg;
 	}
 
 	if (pack_to_stdout != !base_name)
@@ -1405,35 +1540,11 @@ int cmd_pack_objects(int argc, const cha
 		setup_progress_signal();
 	}
 
-	for (;;) {
-		unsigned char sha1[20];
-		unsigned hash;
-
-		if (!fgets(line, sizeof(line), stdin)) {
-			if (feof(stdin))
-				break;
-			if (!ferror(stdin))
-				die("fgets returned NULL, not EOF, not error!");
-			if (errno != EINTR)
-				die("fgets: %s", strerror(errno));
-			clearerr(stdin);
-			continue;
-		}
+	if (read_from_stdin)
+		read_object_list_from_stdin();
+	else
+		get_object_list(argc - i, argv + i);
 
-		if (line[0] == '-') {
-			if (get_sha1_hex(line+1, sha1))
-				die("expected edge sha1, got garbage:\n %s",
-				    line+1);
-			if (num_preferred_base++ < window)
-				add_preferred_base(sha1);
-			continue;
-		}
-		if (get_sha1_hex(line, sha1))
-			die("expected sha1, got garbage:\n %s", line);
-		hash = name_hash(line+41);
-		add_preferred_base_object(line+41, hash);
-		add_object_entry(sha1, hash, 0);
-	}
 	if (progress)
 		fprintf(stderr, "Done counting %d objects.\n", nr_objects);
 	sorted_by_sha = create_final_object_list();
-- 
1.4.2.g552e5

^ permalink raw reply related

* Re: [PATCH 3/5] autoconf: Preliminary check for working mmap
From: Shawn Pearce @ 2006-09-05  6:25 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <7vu03mkiei.fsf@assigned-by-dhcp.cox.net>

Junio C Hamano <junkio@cox.net> wrote:
> Shawn Pearce <spearce@spearce.org> writes:
> 
> > I'm maybe only 1/3 of the way through the sliding window mmap
> > implementation.  I've got a good chunk of sha1_file.c converted but I
> > still have to deal with the copying in pack-objects.c and the verify
> > code in verify-pack.c.  I'm hoping I can send a preliminary patch
> > series tomorrow as I'm going to work on it more tonight and tomorrow.
> 
> Thanks -- I was tempted to do this myself after finishing the
> index_64 change in "pu" branch, but have resisted the temptation
> myself so far.  Being lazy, the less I have to code the better,
> naturally ;-).

I thought that might be the case.  I should be able to finish it
up tomorrow.  :-)

I don't know if I've made this more complex than I really need to
but I've permitted multiple windows per pack.  There is just one
LRU of all windows across all packs and a maximum amount of address
space to use for pack mappings.  Least recently used window gets
tossed when we need a different window.  This permits us to keep
say a window active on the front of a pack (near the commits) and
another different active window closer to the back (near the blobs).

That multiple window feature made it a slightly non-trivial copy
and paste from fast-import but I think its worth it for tree walking
type applications.

-- 
Shawn.

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox