Git development
 help / color / mirror / Atom feed
* Re: Start handling references internally as a sorted in-memory list
From: Junio C Hamano @ 2006-09-11 23:57 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: git
In-Reply-To: <Pine.LNX.4.64.0609111632050.27779@g5.osdl.org>

Linus Torvalds <torvalds@osdl.org> writes:

> This also adds some very rudimentary support for the notion of packed
> And yeah, I know that the "sorting" code is O(n**2) thanks to doing an 
> insertion sort into a simple linked list. Tough. I didn't care enough to 
> do it well. With "n" usually being a few hundred at most, we really don't 
> care, and if we ever do, we _can_ fix it later on to use a heap or 
> something.

I thought what triggered the restructuring of this part was N
actually being large enough to cause pain to some people, so I
suspect later might need to be reasonably soon ;-).

> +static const char *parse_ref_line(char *line, unsigned char *sha1)
> +{
> +	/*
> +	 * 42: the answer to everything.
> +	 *
> +	 * In this case, it happens to be the answer to
>...

Rof,l.

I've been wondering what happens if you pack "refs/heads/foo",
delete it, and create "refs/heads/foo/bar" with your proposal.
I'll find the answer (which is _not_ "42") in the updated
do_for_each_ref(), I guess.

^ permalink raw reply

* Re: Start handling references internally as a sorted in-memory list
From: Linus Torvalds @ 2006-09-11 23:50 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git Mailing List
In-Reply-To: <Pine.LNX.4.64.0609111632050.27779@g5.osdl.org>



On Mon, 11 Sep 2006, Linus Torvalds wrote:
> 
> And yeah, I know that the "sorting" code is O(n**2) thanks to doing an 
> insertion sort into a simple linked list. Tough. I didn't care enough to 
> do it well. With "n" usually being a few hundred at most, we really don't 
> care, and if we ever do, we _can_ fix it later on to use a heap or 
> something.

Btw, to expand on that - one of the nice things from this whole re-org of 
how we handle refs is that when you do

	git-rev-parse --all

it now sorts them in a reliable order - by name. Before, they came out in 
some random order that totally depended on the filesystem that the 
references were on.

Use the "--symbolic --all" to see this in a more obvious way, before and 
after.

		Linus

^ permalink raw reply

* Re: What's in git.git
From: Junio C Hamano @ 2006-09-11 23:48 UTC (permalink / raw)
  To: Petr Baudis; +Cc: git
In-Reply-To: <20060911221411.GG23891@pasky.or.cz>

Petr Baudis <pasky@suse.cz> writes:

> Dear diary, on Mon, Sep 11, 2006 at 11:06:03PM CEST, I got a letter
> where Jakub Narebski <jnareb@gmail.com> said that...
>
>> I still think it is better, easier and faster for someone who makes a new
>> feature to document it too.
>
> Especially since we _DON'T_ have good track record in other people
> quickly documenting newly introduced undocumented features.

Well,...

I miss the days the lead of this project had _me_ as a
contributor ;-).

^ permalink raw reply

* Re: Change set based shallow clone
From: Junio C Hamano @ 2006-09-11 23:47 UTC (permalink / raw)
  To: Paul Mackerras; +Cc: linux, git, torvalds
In-Reply-To: <17669.55963.930152.564529@cargo.ozlabs.ibm.com>

Paul Mackerras <paulus@samba.org> writes:

> Junio C Hamano writes:
>
>> That's a dubious idea.
>> 
>>  - Why assume a tag points directly at a commit, or if it is
>>    not, why assume "foo^{}" (dereferencing repeatedly until we
>>    get a non-tag) is special?
>
> Umm, I'm not sure what you're getting at here - if one shouldn't make
> those assumptions, why does git ls-remote output both the tag and
> tag^{} lines?

This was originally done to support Cogito's tag following which
was in its infancy.  So in that sense it is already special (iow
we know one user that can take advantage of it), but my point
was that its usefulness for a commit chain fetching application
(i.e. Cogito) does not automatically mean it is also useful for
visualizers like gitk and gitweb.

^ permalink raw reply

* Start handling references internally as a sorted in-memory list
From: Linus Torvalds @ 2006-09-11 23:37 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git Mailing List
In-Reply-To: <Pine.LNX.4.64.0609111158390.3960@g5.osdl.org>


This also adds some very rudimentary support for the notion of packed
refs.  HOWEVER! At this point it isn't used to actually look up a ref
yet, only for listing them (ie "for_each_ref()" and friends see the
packed refs, but none of the other single-ref lookup routines).

Note how we keep two separate lists: one for the loose refs, and one for 
the packed refs we read. That's so that we can easily keep the two apart, 
and read only one set or the other (and still always make sure that the 
loose refs take precedence).

[ From this, it's not actually obvious why we'd keep the two separate 
  lists, but it's important to have the packed refs on their own list 
  later on, when I add support for looking up a single loose one.

  For that case, we will want to read _just_ the packed refs in case the 
  single-ref lookup fails, yet we may end up needing the other list at 
  some point in the future, so keeping them separated is important ]

Signed-off-by: Linus Torvalds <torvalds@osdl.org>
----

This passes all the tests, but largely because it doesn't yet actually use 
and test the ref-packing code. The code exists, but it doesn't remove old 
refs yet. It will be easy enough to do with this background code, though: 
I think this is all set up to be reasonably efficient.

And yeah, I know that the "sorting" code is O(n**2) thanks to doing an 
insertion sort into a simple linked list. Tough. I didn't care enough to 
do it well. With "n" usually being a few hundred at most, we really don't 
care, and if we ever do, we _can_ fix it later on to use a heap or 
something.

diff --git a/Makefile b/Makefile
index 7b3114f..5a2e946 100644
--- a/Makefile
+++ b/Makefile
@@ -295,7 +295,8 @@ BUILTIN_OBJS = \
 	builtin-upload-tar.o \
 	builtin-verify-pack.o \
 	builtin-write-tree.o \
-	builtin-zip-tree.o
+	builtin-zip-tree.o \
+	builtin-pack-refs.o
 
 GITLIBS = $(LIB_FILE) $(XDIFF_LIB)
 LIBS = $(GITLIBS) -lz
diff --git a/builtin-pack-refs.c b/builtin-pack-refs.c
new file mode 100644
index 0000000..0f5d827
--- /dev/null
+++ b/builtin-pack-refs.c
@@ -0,0 +1,41 @@
+#include "cache.h"
+#include "refs.h"
+
+static FILE *refs_file;
+static const char *result_path, *lock_path;
+
+static void remove_lock_file(void)
+{
+	if (lock_path)
+		unlink(lock_path);
+}
+
+static int handle_one_ref(const char *path, const unsigned char *sha1)
+{
+	fprintf(refs_file, "%s %s\n", sha1_to_hex(sha1), path);
+	return 0;
+}
+
+int cmd_pack_refs(int argc, const char **argv, const char *prefix)
+{
+	int fd;
+
+	result_path = xstrdup(git_path("packed-refs"));
+	lock_path = xstrdup(mkpath("%s.lock", result_path));
+
+	fd = open(lock_path, O_CREAT | O_EXCL | O_WRONLY, 0666);
+	if (fd < 0)
+		die("unable to create new ref-pack file (%s)", strerror(errno));
+	atexit(remove_lock_file);
+
+	refs_file = fdopen(fd, "w");
+	if (!refs_file)
+		die("unable to create ref-pack file structure (%s)", strerror(errno));
+	for_each_ref(handle_one_ref);
+	fsync(fd);
+	fclose(refs_file);
+	if (rename(lock_path, result_path) < 0)
+		die("unable to overwrite old ref-pack file (%s)", strerror(errno));
+	lock_path = NULL;
+	return 0;
+}
diff --git a/builtin.h b/builtin.h
index 25431d7..34ed7b9 100644
--- a/builtin.h
+++ b/builtin.h
@@ -61,5 +61,6 @@ extern int cmd_version(int argc, const c
 extern int cmd_whatchanged(int argc, const char **argv, const char *prefix);
 extern int cmd_write_tree(int argc, const char **argv, const char *prefix);
 extern int cmd_verify_pack(int argc, const char **argv, const char *prefix);
+extern int cmd_pack_refs(int argc, const char **argv, const char *prefix);
 
 #endif
diff --git a/git.c b/git.c
index 335f405..63d1ba3 100644
--- a/git.c
+++ b/git.c
@@ -266,6 +266,7 @@ static void handle_internal_command(int 
 		{ "whatchanged", cmd_whatchanged, RUN_SETUP | USE_PAGER },
 		{ "write-tree", cmd_write_tree, RUN_SETUP },
 		{ "verify-pack", cmd_verify_pack },
+		{ "pack-refs", cmd_pack_refs, RUN_SETUP },
 	};
 	int i;
 
diff --git a/refs.c b/refs.c
index 5e65314..5f80a68 100644
--- a/refs.c
+++ b/refs.c
@@ -3,6 +3,145 @@ #include "cache.h"
 
 #include <errno.h>
 
+struct ref_list {
+	struct ref_list *next;
+	unsigned char sha1[20];
+	char name[FLEX_ARRAY];
+};
+
+static const char *parse_ref_line(char *line, unsigned char *sha1)
+{
+	/*
+	 * 42: the answer to everything.
+	 *
+	 * In this case, it happens to be the answer to
+	 *  40 (length of sha1 hex representation)
+	 *  +1 (space in between hex and name)
+	 *  +1 (newline at the end of the line)
+	 */
+	int len = strlen(line) - 42;
+
+	if (len <= 0)
+		return NULL;
+	if (get_sha1_hex(line, sha1) < 0)
+		return NULL;
+	if (!isspace(line[40]))
+		return NULL;
+	line += 41;
+	if (line[len] != '\n')
+		return NULL;
+	line[len] = 0;
+	return line;
+}
+
+static struct ref_list *add_ref(const char *name, const unsigned char *sha1, struct ref_list *list)
+{
+	int len;
+	struct ref_list **p = &list, *entry;
+
+	/* Find the place to insert the ref into.. */
+	while ((entry = *p) != NULL) {
+		int cmp = strcmp(entry->name, name);
+		if (cmp > 0)
+			break;
+
+		/* Same as existing entry? */
+		if (!cmp)
+			return list;
+		p = &entry->next;
+	}
+
+	/* Allocate it and add it in.. */
+	len = strlen(name) + 1;
+	entry = xmalloc(sizeof(struct ref_list) + len);
+	hashcpy(entry->sha1, sha1);
+	memcpy(entry->name, name, len);
+	entry->next = *p;
+	*p = entry;
+	return list;
+}
+
+static struct ref_list *get_packed_refs(void)
+{
+	static int did_refs = 0;
+	static struct ref_list *refs = NULL;
+
+	if (!did_refs) {
+		FILE *f = fopen(git_path("packed-refs"), "r");
+		if (f) {
+			struct ref_list *list = NULL;
+			char refline[PATH_MAX];
+			while (fgets(refline, sizeof(refline), f)) {
+				unsigned char sha1[20];
+				const char *name = parse_ref_line(refline, sha1);
+				if (!name)
+					continue;
+				list = add_ref(name, sha1, list);
+			}
+			fclose(f);
+			refs = list;
+		}
+		did_refs = 1;
+	}
+	return refs;
+}
+
+static struct ref_list *get_ref_dir(const char *base, struct ref_list *list)
+{
+	DIR *dir = opendir(git_path("%s", base));
+
+	if (dir) {
+		struct dirent *de;
+		int baselen = strlen(base);
+		char *path = xmalloc(baselen + 257);
+
+		memcpy(path, base, baselen);
+		if (baselen && base[baselen-1] != '/')
+			path[baselen++] = '/';
+
+		while ((de = readdir(dir)) != NULL) {
+			unsigned char sha1[20];
+			struct stat st;
+			int namelen;
+
+			if (de->d_name[0] == '.')
+				continue;
+			namelen = strlen(de->d_name);
+			if (namelen > 255)
+				continue;
+			if (has_extension(de->d_name, ".lock"))
+				continue;
+			memcpy(path + baselen, de->d_name, namelen+1);
+			if (stat(git_path("%s", path), &st) < 0)
+				continue;
+			if (S_ISDIR(st.st_mode)) {
+				list = get_ref_dir(path, list);
+				continue;
+			}
+			if (read_ref(git_path("%s", path), sha1) < 0) {
+				error("%s points nowhere!", path);
+				continue;
+			}
+			list = add_ref(path, sha1, list);
+		}
+		free(path);
+		closedir(dir);
+	}
+	return list;
+}
+
+static struct ref_list *get_loose_refs(void)
+{
+	static int did_refs = 0;
+	static struct ref_list *refs = NULL;
+
+	if (!did_refs) {
+		refs = get_ref_dir("refs", NULL);
+		did_refs = 1;
+	}
+	return refs;
+}
+
 /* We allow "recursive" symbolic refs. Only within reason, though */
 #define MAXDEPTH 5
 
@@ -121,60 +260,41 @@ int read_ref(const char *filename, unsig
 
 static int do_for_each_ref(const char *base, int (*fn)(const char *path, const unsigned char *sha1), int trim)
 {
-	int retval = 0;
-	DIR *dir = opendir(git_path("%s", base));
-
-	if (dir) {
-		struct dirent *de;
-		int baselen = strlen(base);
-		char *path = xmalloc(baselen + 257);
-
-		if (!strncmp(base, "./", 2)) {
-			base += 2;
-			baselen -= 2;
+	int retval;
+	struct ref_list *packed = get_packed_refs();
+	struct ref_list *loose = get_loose_refs();
+
+	while (packed && loose) {
+		struct ref_list *entry;
+		int cmp = strcmp(packed->name, loose->name);
+		if (!cmp) {
+			packed = packed->next;
+			continue;
 		}
-		memcpy(path, base, baselen);
-		if (baselen && base[baselen-1] != '/')
-			path[baselen++] = '/';
-
-		while ((de = readdir(dir)) != NULL) {
-			unsigned char sha1[20];
-			struct stat st;
-			int namelen;
+		if (cmp > 0) {
+			entry = loose;
+			loose = loose->next;
+		} else {
+			entry = packed;
+			packed = packed->next;
+		}
+		if (strncmp(base, entry->name, trim))
+			continue;
+		retval = fn(entry->name + trim, entry->sha1);
+		if (retval)
+			return retval;
+	}
 
-			if (de->d_name[0] == '.')
-				continue;
-			namelen = strlen(de->d_name);
-			if (namelen > 255)
-				continue;
-			if (has_extension(de->d_name, ".lock"))
-				continue;
-			memcpy(path + baselen, de->d_name, namelen+1);
-			if (stat(git_path("%s", path), &st) < 0)
-				continue;
-			if (S_ISDIR(st.st_mode)) {
-				retval = do_for_each_ref(path, fn, trim);
-				if (retval)
-					break;
-				continue;
-			}
-			if (read_ref(git_path("%s", path), sha1) < 0) {
-				error("%s points nowhere!", path);
-				continue;
-			}
-			if (!has_sha1_file(sha1)) {
-				error("%s does not point to a valid "
-				      "commit object!", path);
-				continue;
-			}
-			retval = fn(path + trim, sha1);
+	packed = packed ? packed : loose;
+	while (packed) {
+		if (!strncmp(base, packed->name, trim)) {
+			retval = fn(packed->name + trim, packed->sha1);
 			if (retval)
-				break;
+				return retval;
 		}
-		free(path);
-		closedir(dir);
+		packed = packed->next;
 	}
-	return retval;
+	return 0;
 }
 
 int head_ref(int (*fn)(const char *path, const unsigned char *sha1))
@@ -187,22 +307,22 @@ int head_ref(int (*fn)(const char *path,
 
 int for_each_ref(int (*fn)(const char *path, const unsigned char *sha1))
 {
-	return do_for_each_ref("refs", fn, 0);
+	return do_for_each_ref("refs/", fn, 0);
 }
 
 int for_each_tag_ref(int (*fn)(const char *path, const unsigned char *sha1))
 {
-	return do_for_each_ref("refs/tags", fn, 10);
+	return do_for_each_ref("refs/tags/", fn, 10);
 }
 
 int for_each_branch_ref(int (*fn)(const char *path, const unsigned char *sha1))
 {
-	return do_for_each_ref("refs/heads", fn, 11);
+	return do_for_each_ref("refs/heads/", fn, 11);
 }
 
 int for_each_remote_ref(int (*fn)(const char *path, const unsigned char *sha1))
 {
-	return do_for_each_ref("refs/remotes", fn, 13);
+	return do_for_each_ref("refs/remotes/", fn, 13);
 }
 
 int get_ref_sha1(const char *ref, unsigned char *sha1)

^ permalink raw reply related

* [PATCH] contrib/vim: add syntax highlighting file for commits
From: Jeff King @ 2006-09-11 23:22 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git


Signed-off-by: Jeff King <peff@peff.net>
---
There were at least a few positive comments on this, and hey, we have to
balance the contrib/emacs directory!

 contrib/vim/README            |    7 +++++++
 contrib/vim/commit-syntax.vim |   18 ++++++++++++++++++
 2 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/contrib/vim/README b/contrib/vim/README
new file mode 100644
index 0000000..bad0a05
--- /dev/null
+++ b/contrib/vim/README
@@ -0,0 +1,7 @@
+To syntax highlight git's commit messages, you need to:
+  1. Copy commit-syntax.vim to vim's syntax directory:
+     $ cp commit-syntax.vim $HOME/.vim/syntax/gitcommit.vim
+  2. Auto-detect the editing of git commit files:
+     $ cat >>$HOME/.vimrc <<'EOF'
+     autocmd BufNewFile,BufRead COMMIT_EDITMSG set filetype=gitcommit
+     EOF
diff --git a/contrib/vim/commit-syntax.vim b/contrib/vim/commit-syntax.vim
new file mode 100644
index 0000000..a9de09f
--- /dev/null
+++ b/contrib/vim/commit-syntax.vim
@@ -0,0 +1,18 @@
+syn region gitLine start=/^#/ end=/$/
+syn region gitCommit start=/^# Updated but not checked in:$/ end=/^#$/ contains=gitHead,gitCommitFile
+syn region gitHead contained start=/^#   (.*)/ end=/^#$/
+syn region gitChanged start=/^# Changed but not updated:/ end=/^#$/ contains=gitHead,gitChangedFile
+syn region gitUntracked start=/^# Untracked files:/ end=/^#$/ contains=gitHead,gitUntrackedFile
+
+syn match gitCommitFile contained /^#\t.*/hs=s+2
+syn match gitChangedFile contained /^#\t.*/hs=s+2
+syn match gitUntrackedFile contained /^#\t.*/hs=s+2
+
+hi def link gitLine Comment
+hi def link gitCommit Comment
+hi def link gitChanged Comment
+hi def link gitHead Comment
+hi def link gitUntracked Comment
+hi def link gitCommitFile Type
+hi def link gitChangedFile Constant
+hi def link gitUntrackedFile Constant
-- 
1.4.2.g39f1

^ permalink raw reply related

* [PATCH] git-status: document colorization config options
From: Jeff King @ 2006-09-11 23:21 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git


Signed-off-by: Jeff King <peff@peff.net>
---
Applies to next.  This is the documentation from the original 'sh'
version which got dropped (but it's the same for the C version).

 Documentation/config.txt |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index ce722a2..844cae4 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -225,6 +225,20 @@ showbranch.default::
 	The default set of branches for gitlink:git-show-branch[1].
 	See gitlink:git-show-branch[1].
 
+status.color::
+	A boolean to enable/disable color in the output of
+	gitlink:git-status[1]. May be set to `true` (or `always`),
+	`false` (or `never`) or `auto`, in which case colors are used
+	only when the output is to a terminal. Defaults to false.
+
+status.color.<slot>::
+	Use customized color for status colorization. `<slot>` is
+	one of `header` (the header text of the status message),
+	`updated` (files which are updated but not committed),
+	`changed` (files which are changed but not updated in the index),
+	or `untracked` (files which are not tracked by git). The values of
+	these variables may be specified as in diff.color.<slot>.
+
 tar.umask::
 	By default, gitlink:git-tar-tree[1] sets file and directories modes
 	to 0666 or 0777. While this is both useful and acceptable for projects
-- 
1.4.2.g39f1

^ permalink raw reply related

* Re: What's in git.git
From: Petr Baudis @ 2006-09-11 22:14 UTC (permalink / raw)
  To: Jakub Narebski; +Cc: git
In-Reply-To: <ee4j3j$mli$1@sea.gmane.org>

Dear diary, on Mon, Sep 11, 2006 at 11:06:03PM CEST, I got a letter
where Jakub Narebski <jnareb@gmail.com> said that...
> Junio C Hamano wrote:
> 
> >>  * git-rev-list: --stdin to feed list of <commits>; it is not clear from 
> >>    the manpage if one can use ^<commit>, and commit related options
> >>    and shortcuts like --not, <commit>..<commit>, <commit>...<commit>
> >>  * git-pack-objects: --revs to provide arguments to rev-list from stdin,
> >>    instead of list of objects. UNDOCUMENTED.
> > 
> > Time spent whining about it is better spent finding it out
> > yourself (UTSL) and writing it I suspect.  You'll learn how
> > things actually work while doing so ;-).
> 
> I still think it is better, easier and faster for someone who makes a new
> feature to document it too.

Especially since we _DON'T_ have good track record in other people
quickly documenting newly introduced undocumented features.

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
Snow falling on Perl. White noise covering line noise.
Hides all the bugs too. -- J. Putnam

^ permalink raw reply

* Re: Change set based shallow clone
From: Paul Mackerras @ 2006-09-11 21:52 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: linux, git, torvalds
In-Reply-To: <7vy7sqic4e.fsf@assigned-by-dhcp.cox.net>

Junio C Hamano writes:

> That's a dubious idea.
> 
>  - Why assume a tag points directly at a commit, or if it is
>    not, why assume "foo^{}" (dereferencing repeatedly until we
>    get a non-tag) is special?

Umm, I'm not sure what you're getting at here - if one shouldn't make
those assumptions, why does git ls-remote output both the tag and
tag^{} lines?

>  - Why assume the user wants access to only the object name of
>    what the tag points at?  Perhaps most users would want to
>    have its type, dates (committer and author), and probably the
>    first line of the commit message if it is (and most likely it
>    is) a commit?  -- at least gitweb and gitk would want these.

There are two things here.  Gitk needs to know which IDs have tags
when displaying the graph, and their names.  It doesn't need to know
the other details until someone clicks on the commit or the tag.  Thus
the information that needs to be collected for every tag at startup is
just the name and the sha1 id of the commit it eventually points to
(if it doesn't eventually point to a commit it's basically not
interesting).

Paul.

^ permalink raw reply

* Re: [PATCH] git-upload-archive: add config option to allow only specified formats
From: Rene Scharfe @ 2006-09-11 21:55 UTC (permalink / raw)
  To: Franck Bui-Huu; +Cc: Junio C Hamano, git
In-Reply-To: <cda58cb80609101207y420bd034n9a76b661374e1edf@mail.gmail.com>

Franck Bui-Huu schrieb:
> just out of curiousity, why "free(daemon_formats)" right before a
> "die()" ?

Hmm, do I show signs of a cleaning fetish? ;-)

A bit more seriously, it helps reduce the number of false positives
emitted by automatic memory checkers.

René

^ permalink raw reply

* Re: [PATCH] git-upload-archive: add config option to allow only specified formats
From: Jakub Narebski @ 2006-09-11 21:50 UTC (permalink / raw)
  To: git
In-Reply-To: <4505D7F4.8050207@lsrfire.ath.cx>

Rene Scharfe wrote:

>> Fine -- do we have any other "list-ish" configuration variable,
>> by the way?  I am just wondering if we earlier established a
>> convention to use some delimiter to list out things and if we do
>> have such a convention if delimiter is a comma or not.
> 
> I didn't find any.  The code would accept whitespace as a separator, too
> (in order to recognize lists like "one, two").  I found myself abusing
> this during my testing: I always left out the comma.  So I dropped the
> comma from the list of delimiters in this patch, going with shell-like
> lists now.

Alternative would be to use multiple valued variable (multiple values with
the same configuration variable).

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

^ permalink raw reply

* Re: [PATCH] git-upload-archive: add config option to allow only specified formats
From: Rene Scharfe @ 2006-09-11 21:41 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Franck Bui-Huu, git
In-Reply-To: <7vpse3sitc.fsf@assigned-by-dhcp.cox.net>

Add a config options, uploadarchive.daemonformats, which can be used to
specify the archive formats that are OK to be uploaded using the
upload-archive service of git-daemon.  The list can be restricted
further using the environment variable GIT_DAEMON_ARCHIVE_FORMATS.

These settings are properly taken into account for --list output, so a
user can run git-archive --remote=<url> --list to inquire the allowed
and supported archive formats on the remote end.

git-daemon is changed to always set the environment variable GIT_DAEMON
before it spawns children.  This variable can thus be used to detect
the presence of the (more restrictive) daemon environment.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---

Junio C Hamano schrieb:
> Rene Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
>> +uploadarchive.daemonformats::
>> +	A comma-separated list of the git-archive formats allowed for upload
>> +	via git-daemon.  If this parameter is missing all formats are allowed
>> +	for upload.
>> +
> 
> Fine -- do we have any other "list-ish" configuration variable,
> by the way?  I am just wondering if we earlier established a
> convention to use some delimiter to list out things and if we do
> have such a convention if delimiter is a comma or not.

I didn't find any.  The code would accept whitespace as a separator, too
(in order to recognize lists like "one, two").  I found myself abusing
this during my testing: I always left out the comma.  So I dropped the
comma from the list of delimiters in this patch, going with shell-like
lists now.

> So we could enhance "--remote --list" to show what are supported
> (both codewise and policywise) on the remote end, with a bit of
> code restructuring?

Yes, of course.  D'oh!

>> diff --git a/daemon.c b/daemon.c
>> index a2954a0..2d58abe 100644
>> --- a/daemon.c
>> +++ b/daemon.c
>> @@ -304,6 +304,8 @@ static int run_service(char *dir, struct
>>  		return -1;
>>  	}
>>  
>> +	setenv("GIT_DAEMON", "I am your father.", 1);
> 
> I suspect "upload_format_allowed()" can be taught to see what is
> in this environment variable and sometimes take that as
> daemon_format without letting the repository to override it, so
> that the site administrator can limit the formats that can be
> used further, just like daemon service mechanism lets them be in
> control.

GIT_DAEMON is meant to indicate one thing, only: that git-daemon started
the process and that it thus has to obey stricter rules.  We can add
another variable which can override the archive config, though.

This patch applies to the 'next' branch (61af0aa).

 Documentation/config.txt |    5 +++++
 archive.h                |    2 ++
 builtin-archive.c        |   27 +++++++++++++++++++++++++--
 builtin-upload-archive.c |    6 ++++++
 cache.h                  |    4 ++++
 config.c                 |   22 ++++++++++++++++++++++
 daemon.c                 |    2 ++
 environment.c            |    1 +
 8 files changed, 67 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index ce722a2..e4123f1 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -236,6 +236,11 @@ tar.umask::
 	the same permissions as gitlink:git-checkout[1] would use. The default
 	value remains 0, which means world read-write.
 
+uploadarchive.daemonformats::
+	A whitespace-separated list of the git-archive formats allowed for
+	upload via git-daemon.  If this parameter is missing or is equal to
+	"*" then all formats are allowed for upload.
+
 user.email::
 	Your email address to be recorded in any newly created commits.
 	Can be overridden by the 'GIT_AUTHOR_EMAIL' and 'GIT_COMMITTER_EMAIL'
diff --git a/archive.h b/archive.h
index 16dcdb8..9ffd300 100644
--- a/archive.h
+++ b/archive.h
@@ -27,6 +27,8 @@ struct archiver {
 
 extern struct archiver archivers[];
 
+extern int is_allowed_archive_format(const char *format);
+
 extern int parse_archive_args(int argc,
 			      const char **argv,
 			      struct archiver *ar);
diff --git a/builtin-archive.c b/builtin-archive.c
index cb883df..5060811 100644
--- a/builtin-archive.c
+++ b/builtin-archive.c
@@ -169,8 +169,10 @@ int parse_archive_args(int argc, const c
 		const char *arg = argv[i];
 
 		if (!strcmp(arg, "--list") || !strcmp(arg, "-l")) {
-			for (i = 0; i < ARRAY_SIZE(archivers); i++)
-				printf("%s\n", archivers[i].name);
+			for (i = 0; i < ARRAY_SIZE(archivers); i++) {
+				if (is_allowed_archive_format(archivers[i].name))
+					printf("%s\n", archivers[i].name);
+			}
 			exit(0);
 		}
 		if (!strcmp(arg, "--verbose") || !strcmp(arg, "-v")) {
@@ -217,6 +219,25 @@ int parse_archive_args(int argc, const c
 	return i;
 }
 
+int is_allowed_archive_format(const char *format)
+{
+	char *formats;
+
+	if (!getenv("GIT_DAEMON"))
+		return 1;
+
+	formats = getenv("GIT_DAEMON_ARCHIVE_FORMATS");
+	if (formats && !git_config_list_contains(formats, format))
+		return 0;
+
+	if (git_config_list_contains(daemon_archive_formats, "*"))
+		return 1;
+	if (!git_config_list_contains(daemon_archive_formats, format))
+		return 0;
+
+	return 1;
+}
+
 static const char *remote_request(int *ac, const char **av)
 {
 	int ix, iy, cnt = *ac;
@@ -260,6 +281,8 @@ int cmd_archive(int argc, const char **a
 
 	setlinebuf(stderr);
 
+	git_config(git_default_config);
+
 	memset(&ar, 0, sizeof(ar));
 	tree_idx = parse_archive_args(argc, argv, &ar);
 	if (prefix == NULL)
diff --git a/builtin-upload-archive.c b/builtin-upload-archive.c
index 42cb9f8..ff31c04 100644
--- a/builtin-upload-archive.c
+++ b/builtin-upload-archive.c
@@ -67,6 +67,9 @@ static int run_upload_archive(int argc, 
 	/* parse all options sent by the client */
 	treeish_idx = parse_archive_args(sent_argc, sent_argv, &ar);
 
+	if (!is_allowed_archive_format(ar.name))
+		die("upload of %s format forbidden\n", ar.name);
+
 	parse_treeish_arg(sent_argv + treeish_idx, &ar.args, prefix);
 	parse_pathspec_arg(sent_argv + treeish_idx + 1, &ar.args);
 
@@ -77,6 +80,9 @@ int cmd_upload_archive(int argc, const c
 {
 	pid_t writer;
 	int fd1[2], fd2[2];
+
+	git_config(git_default_config);
+
 	/*
 	 * Set up sideband subprocess.
 	 *
diff --git a/cache.h b/cache.h
index ac51ed1..9f8d58b 100644
--- a/cache.h
+++ b/cache.h
@@ -399,6 +399,7 @@ extern int git_config_int(const char *, 
 extern int git_config_bool(const char *, const char *);
 extern int git_config_set(const char *, const char *);
 extern int git_config_set_multivar(const char *, const char *, const char *, int);
+extern int git_config_list_contains(const char *haystack, const char *needle);
 extern int check_repository_format_version(const char *var, const char *value);
 
 #define MAX_GITNAME (1000)
@@ -408,6 +409,9 @@ extern char git_default_name[MAX_GITNAME
 #define MAX_ENCODING_LENGTH 64
 extern char git_commit_encoding[MAX_ENCODING_LENGTH];
 
+#define MAX_FORMATS_LENGTH 256
+extern char daemon_archive_formats[MAX_FORMATS_LENGTH];
+
 extern int copy_fd(int ifd, int ofd);
 extern void write_or_die(int fd, const void *buf, size_t count);
 extern int write_or_whine(int fd, const void *buf, size_t count, const char *msg);
diff --git a/config.c b/config.c
index e8f0caf..0f111ab 100644
--- a/config.c
+++ b/config.c
@@ -251,6 +251,23 @@ int git_config_bool(const char *name, co
 	return git_config_int(name, value) != 0;
 }
 
+int git_config_list_contains(const char *haystack, const char *needle)
+{
+	const char delim[] = " \t";
+	int len = strlen(needle);
+	const char *begin = haystack;
+
+	for (;;) {
+		char *pos = strstr(haystack, needle);
+		if (!pos)
+			return 0;
+		haystack++;
+		if ((pos == begin || strchr(delim, pos[-1])) &&
+		    (pos[len] == '\0' || strchr(delim, pos[len])))
+			return 1;
+	}
+}
+
 int git_default_config(const char *var, const char *value)
 {
 	/* This needs a better name */
@@ -314,6 +331,11 @@ int git_default_config(const char *var, 
 		return 0;
 	}
 
+	if (!strcmp(var, "uploadarchive.daemonformats")) {
+		strlcpy(daemon_archive_formats, value, sizeof(daemon_archive_formats));
+		return 0;
+	}
+
 	/* Add other config variables here and to Documentation/config.txt. */
 	return 0;
 }
diff --git a/daemon.c b/daemon.c
index a2954a0..2d58abe 100644
--- a/daemon.c
+++ b/daemon.c
@@ -304,6 +304,8 @@ static int run_service(char *dir, struct
 		return -1;
 	}
 
+	setenv("GIT_DAEMON", "I am your father.", 1);
+
 	/*
 	 * We'll ignore SIGTERM from now on, we have a
 	 * good client.
diff --git a/environment.c b/environment.c
index 84d870c..1796937 100644
--- a/environment.c
+++ b/environment.c
@@ -24,6 +24,7 @@ const char *apply_default_whitespace;
 int zlib_compression_level = Z_DEFAULT_COMPRESSION;
 int pager_in_use;
 int pager_use_color = 1;
+char daemon_archive_formats[MAX_FORMATS_LENGTH] = "*";
 
 static const char *git_dir;
 static char *git_object_dir, *git_index_file, *git_refs_dir, *git_graft_file;

^ permalink raw reply related

* Re: What's in git.git
From: Jakub Narebski @ 2006-09-11 21:06 UTC (permalink / raw)
  To: git
In-Reply-To: <7v7j0ajrfh.fsf@assigned-by-dhcp.cox.net>

Junio C Hamano wrote:

>>  * git-rev-list: --stdin to feed list of <commits>; it is not clear from 
>>    the manpage if one can use ^<commit>, and commit related options
>>    and shortcuts like --not, <commit>..<commit>, <commit>...<commit>
>>  * git-pack-objects: --revs to provide arguments to rev-list from stdin,
>>    instead of list of objects. UNDOCUMENTED.
> 
> Time spent whining about it is better spent finding it out
> yourself (UTSL) and writing it I suspect.  You'll learn how
> things actually work while doing so ;-).

I still think it is better, easier and faster for someone who makes a new
feature to document it too.

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

^ permalink raw reply

* Allow multiple "git_path()" uses
From: Linus Torvalds @ 2006-09-11 19:03 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git Mailing List


This allows you to maintain a few filesystem pathnames concurrently, by 
simply replacing the single static "pathname" buffer with a LRU of four 
buffers.

We did exactly the same thing with sha1_to_hex(), for pretty much exactly 
the same reason. Sometimes you want to use two pathnames, and while it's 
easy enough to xstrdup() them, why not just do the LU buffer thing.

Signed-off-by: Linus Torvalds <torvalds@osdl.org>
---

[ This actually came up when I was playing with git_path("refs-packed") 
  when following refs, so while it doesn't hit us right now, it's for some 
  future work.. ]

diff --git a/path.c b/path.c
index db8905f..bb89fb0 100644
--- a/path.c
+++ b/path.c
@@ -13,9 +13,15 @@
 #include "cache.h"
 #include <pwd.h>
 
-static char pathname[PATH_MAX];
 static char bad_path[] = "/bad-path/";
 
+static char *get_pathname(void)
+{
+	static char pathname_array[4][PATH_MAX];
+	static int index;
+	return pathname_array[3 & ++index];
+}
+
 static char *cleanup_path(char *path)
 {
 	/* Clean it up */
@@ -31,6 +37,7 @@ char *mkpath(const char *fmt, ...)
 {
 	va_list args;
 	unsigned len;
+	char *pathname = get_pathname();
 
 	va_start(args, fmt);
 	len = vsnprintf(pathname, PATH_MAX, fmt, args);
@@ -43,6 +50,7 @@ char *mkpath(const char *fmt, ...)
 char *git_path(const char *fmt, ...)
 {
 	const char *git_dir = get_git_dir();
+	char *pathname = get_pathname();
 	va_list args;
 	unsigned len;
 

^ permalink raw reply related

* Re: Change set based shallow clone
From: Junio C Hamano @ 2006-09-11 18:54 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Paul Mackerras, linux, jonsmirl, git
In-Reply-To: <Pine.LNX.4.64.0609102002300.27779@g5.osdl.org>

Linus Torvalds <torvalds@osdl.org> writes:

> On Sun, 10 Sep 2006, Linus Torvalds wrote:
>> 
>> If we did the same pack-file approach that we do for objects, the problem 
>> ends up being that _updating_ things is really hard. What we could do (and 
>> might work) is that a "git repack" would create a "packed representation 
>> of the heads too".
>
> To clarify: I'm _not_ suggesting actually using the "pack-file" 
> representation itself for the references.
>
> I'm saying that we could have something like a
>
> 	.git/refs-packed
>
> file, which could be (for example) just a plain linear text-file, of the 
> form
>
> 	19ed368b17abfb9ad5c7467ea74fd8a045d96b43	refs/heads/html
> 	60a6bf5f53635005f4f68d8b8a33172309193623	refs/heads/maint
> 	2d32e76f893b2ac432201ce7596c8bab691691e6	refs/heads/man
>...
> 	9165ec17fde255a1770886189359897dbb541012	refs/tags/v0.99.7c
> 	02b2acff8bafb6d73c6513469cdda0c6c18c4138	refs/tags/v0.99.7d
> 	...
>
> ie it would contain just a linear file with the "<hex></tab><refname>"
> format.

This depends on how expensive it is to read a file backwards,
but it might make sense to have a single append-only file.

${GIT_DIR}/refs-packed file would be a sequence of records, each
record is of form:

	type SP timestamp LF payload LF length LF

where length and timestamp are ASCII integers, the former is the
length of this record and you would use to compute the file
offset you give lseek() to seek back to the beginning of this
record.  The latter is the creation time of this record.  The
type of the record is either full or incremental (perhaps 'F'
and 'I').

A full record describes in the above "object-name refname" tuple
format.  An incremental record has entries only for the subset
of refs, with 0{40} object name denoting deletion of a ref as in
your suggestion.

A few nice things about this representation:

 - It gives reflog for free (we need to add 'author' and
   'reason' encoded after timestamp, though); it also gives
   reflog to tags which I sometimes miss.

 - Garbage collection is straightforward, although it requires
   locking the whole refs.

 - You can now have 'foo' and 'foo/a', 'foo/b', branches at the
   same time.  It is natural to organize a topic branch 'foo'
   which consists of a merge of subtopics 'foo/a', 'foo/b', ...,
   but this cannot be expressed with the current directory-file
   based scheme.

Some minor details:

 - After every N (I do not think we would need to make this
   configurable, but we need to decide on a sane value) updates,
   we would want to write a full record so that we do not have
   to traverse too long a chain.

 - An incremental record could be delta since the last full,
   instead of delta since the last record, as the above
   introduction hints.  If we start from A, B, and C, and then A
   and B are modified in turn, we might have:

	Full: A B C
        Incr: A'
        Incr: B'

   but we could have instead:

	Full: A B C
        Incr: A'
        Incr: A' B'

   For this to make sense, the record header needs to record a
   back-pointer to the last full record before the current
   incremental record.  With that, we would need to read at most
   two records to find out the current status.

Locking between writers and readers can become problematic.  The
reader would read the last line to find the record length, seek
that much back to find the beginning of the "then-latest"
record, but when there is a simultanous writer, there is no
guarantee that what at the last line of the file _is_ the length
of the last record.  The writer may be in the middle of
appending something.  We may be able to work it around by:

 - Add hash of the record at the end;

 - The reader assumes that there is usually no collision, reads
   the hash and the length, seeks back and reads the record and
   validate the hash -- if the hash does not match, it is likely
   that it detected the writer-reader collision so it should
   wait until the file grows and stops growing and then retry.

But writer-writer contention obviously needs a file lock.  Maybe
we can use flock/lockf/fcntl(F_SETLKW)?  If we are going to do
that then protecting the reader-writer side with the same
mechanism may be simpler and cleaner.

> NOTE! It's important that whatever sceme used gets locking right. The 
> above suggestion gets it right simply because it doesn't really _change_ 
> anything. Any new or modified ref ends up using the old code, and using a 
> ".lock" file and renaming it automatically does the same thing it ever 
> did.

The above changes things quite a bit, so locking aspect of it
needs to be thought through thouroughly.

^ permalink raw reply

* Re: Change set based shallow clone
From: Shawn Pearce @ 2006-09-11 17:52 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <7vr6yikhdj.fsf@assigned-by-dhcp.cox.net>

Junio C Hamano <junkio@cox.net> wrote:
> As you know I've shelved the 64-bit offset stuff.  My preference
> is to base it on 106d710b but basing it on 'next' would be fine.
> Between 106d710b and next there is only one unrelated change to
> sha1_file.c, which is the hexval[] change that is in 'master'.

OK.  I'll rebase on 106d710b and try to get the series out in a day
or two.
 
> So far I liked the per-object-type dictionary conjecture the
> most, primarily because my gut feeling tells me that it would
> involve the least amount of hassle in the interoperability area.

Agreed, except I don't think its going to save us very much (~4%)
and its enough of a change still to make things a little ugly.
If the "true" dictionary compression idea has any value it may
save us even more and make it easier to implement fast full text
searching across files.  But its definately more complex.  So I'm
going to shut up now.
 
> But honestly speaking I am not looking forward to a packfile
> format update at this moment.  I'd like things to settle down a
> bit now, after merging good bits from 'next' to 'master' and
> declare 1.4.3 first, perhaps by the end of the month.

No worry there.  I won't have enough time between now and the end
of this month to create a dictionary pack that's even ready for pu,
let alone next.  I hope we see 1.4.3 before then.  :-)

-- 
Shawn.

^ permalink raw reply

* Re: Change set based shallow clone
From: Junio C Hamano @ 2006-09-11 16:47 UTC (permalink / raw)
  To: linux; +Cc: git, paulus, torvalds
In-Reply-To: <20060911142644.32313.qmail@science.horizon.com>

linux@horizon.com writes:

>> Ideally we would have two sha1 values in the cache - the sha1 in the
>> file, and if that is the ID of a tag object, we would also put the
>> sha1 of the commit that the tag points to in the cache.
>
> Now that's not a bad idea.  Hacking it in to Linus's scheme, that's
>
> <foo sha>\t<foo^{} sha>\tfoo

That's a dubious idea.

 - Why assume a tag points directly at a commit, or if it is
   not, why assume "foo^{}" (dereferencing repeatedly until we
   get a non-tag) is special?

 - Why assume the user wants access to only the object name of
   what the tag points at?  Perhaps most users would want to
   have its type, dates (committer and author), and probably the
   first line of the commit message if it is (and most likely it
   is) a commit?  -- at least gitweb and gitk would want these.

You should separate the issue of the internal data structure
implementation and programming interface for Porcelains.  From
the internal data structure point-of-view, the second one is a
redundant piece of information.  Caching it _would_ speed up the
access to it, but then the issue becomes where we draw the line
to stop.

It is probably more useful to think about what kind of
information formatted in what way is often wanted by Porcelains
who want to grab many refs in one-go.  If you can come up with a
set that can satisfy everybody using that as the cache file
format would be fine, but I strongly doubt you can satisfy
everybody.  In which case, thinking about the ways for the
Porcelain to express flexibly what information is wanted and
formatted in what way, and have a command to access that
(git-show-refs, anybody?) would be more fruitful, and at that
point, the internal representation of the cached data becomes
the implementation detail.

^ permalink raw reply

* Re: What's in git.git
From: Junio C Hamano @ 2006-09-11 16:31 UTC (permalink / raw)
  To: Jakub Narebski; +Cc: git
In-Reply-To: <ee3hac$n57$1@sea.gmane.org>

Jakub Narebski <jnareb@gmail.com> writes:

> Perhaps those two options, --stdin to feed arguments from standard input,
> and -revs to run rev-list logic internally should be used whenever possible
> in all the git commands? This would allow to avoid forks and/or command
> line length limit.

Some make more sense than others, from usability point of view.
It all depends on how much sense it makes to be able to run them
on sequence of commits.

>  * git-rev-list: --stdin to feed list of <commits>; it is not clear from 
>    the manpage if one can use ^<commit>, and commit related options
>    and shortcuts like --not, <commit>..<commit>, <commit>...<commit>
>  * git-pack-objects: --revs to provide arguments to rev-list from stdin,
>    instead of list of objects. UNDOCUMENTED.

Time spent whining about it is better spent finding it out
yourself (UTSL) and writing it I suspect.  You'll learn how
things actually work while doing so ;-).

> It would be nice if the following commands had --stdin or had it's --stdin
> usage extended:
>  * git-diff-tree: --stdin to allow to provide path limits, separated 
>    by ' -- ' from <tree-ish> or pair of <tree-ish>

Something like this could be used to follow renames and do more
interesting stuff.  The caller would create a pair of pipes,
throw first tree-pair at it, receive and examine the output, and
decide what pathspec to use for the next one and continue.  If
you do not limit yourself to pathspec but e.g. allow -S to be
also specified per invocation, then you can make it to follow
line movements, not just renames.  So in the bigger picture, I
like what something like this can offer to the Porcelain writers.

It is a separate story if it is worth building that _into_
diff-tree itself.  A sane way to see if it is is to start by
writing a rough equivalent that is:

	#!/bin/sh
	while read stuff;
        do
		# Note: this really needs to do shell quote
        	git-diff-tree $stuff
		echo I am done with one record.
        done

Then, write such a driver program to drive it and see if
per-line invocation of diff-tree is really the bottleneck.  For
any useful and interesting application of this pattern, I
suspect the process that drives this diff-tree loop will have
enough computation in it, and the only thing you are saving is
the start-up cost of diff-tree (i.e. fork+exec).  I am not sure
how much of the bottleneck it would be in the while thing.

>  * git-ls-tree: --stdin to loop over <tree-ish>, one tree per line.

Likewise.  Also you would need to decide the record delimiter.

>  * git-cat-object: --stdin to loop over objects, plus -z to change separator
>    between records to NULL (or have it turned on by default).

I would suggest TYPE SP BYTE-COUNT LF followed by payload
instead.  You cannot otherwise handle binary files with NUL
(lesson of the day: NULL is a pointer, the character's name is
NUL) in it.  If you do not mind redundancy to help the consumer
of this output, TYPE SP SHA-1 SP BYTE-COUNT LF followed by
payload might even be a better choice.

> For all "loop" --stdin, the output should begin with the line which was
> arguments, like git-diff-tree outputs first <tree-ish> used for diff.

That's something you cannot decide on a whim without knowing how
they are used and what convention is the most useful.  I suspect
that single record delimiter without frill might turn out to be
more useful.  Parrotting the arguments means you would need to
make sure the output format can easily parsable -- the issues
include that you need deal with embedded newlines in them.  To
quote them in the output routine is easy but the consumer now
needs to know they need to be dequoted.

> I think it is quite often to use git-rev-list ...| git-diff-tree ...
> pipeline, so it might be worth to add --revs option to git-diff-tree.

Are you talking about "git rev-list | git diff-tree --stdin"???

> P.S. does git-merge take -F <file> option?

No; it is not a Porcelain so ease of typing is not its goal.
You can say longhand: git-merge "`cat $file`" ...
Merge messages are supposed to be mostly automated and short,
so command line length limit is not much of an issue here.

^ permalink raw reply

* Re: Change set based shallow clone
From: Jon Smirl @ 2006-09-11 15:01 UTC (permalink / raw)
  To: linux@horizon.com; +Cc: paulus, torvalds, git
In-Reply-To: <20060911142644.32313.qmail@science.horizon.com>

On 11 Sep 2006 10:26:44 -0400, linux@horizon.com <linux@horizon.com> wrote:
> > Could we do a cache of the refs that stores the stat information for
> > each of the files under .git/refs plus the sha1 that the ref points
> > to?  In other words this cache would do for the refs what the index
> > does for the working directory.  Reading all the refs would mean we
> > still had to stat each of the files, but that's much quicker than
> > reading them in the cold-cache case.  In the common case when most of
> > the stat information matches, we don't have to read the file because
> > we have the sha1 that the file contains right there in the cache.
>
> Well, that could save one of two seeks, but that's not *much* quicker.
> (Indeed, a git ref would fit into the 60 bytes of block pointer space
> in an ext2/3 inode if regular files were stuffed there as well as symlinks.)

Does everyone hate my idea of putting the sha in the file name and
using the directory structure as a simple database with locking? No
one made any comments.

------------------------------------------------
Here's a hack, instead of of putting the sha inside the file, put the
sha into the filename.

master_86a8534ba23a5532f6d0ddd01ecd8f02f662cf78

Now you can just do a directory listing and get all of the data
quickly. To keep the existing porcelain working add a symlink.

ln -s master_86a8534ba23a5532f6d0ddd01ecd8f02f662cf78 master

You might want the sha1 encoded names in a new director
-- 
Jon Smirl
jonsmirl@gmail.com

^ permalink raw reply

* Re: Change set based shallow clone
From: linux @ 2006-09-11 14:26 UTC (permalink / raw)
  To: paulus, torvalds; +Cc: git, jonsmirl, linux
In-Reply-To: <17669.8191.778645.311304@cargo.ozlabs.ibm.com>

> Could we do a cache of the refs that stores the stat information for
> each of the files under .git/refs plus the sha1 that the ref points
> to?  In other words this cache would do for the refs what the index
> does for the working directory.  Reading all the refs would mean we
> still had to stat each of the files, but that's much quicker than
> reading them in the cold-cache case.  In the common case when most of
> the stat information matches, we don't have to read the file because
> we have the sha1 that the file contains right there in the cache.

Well, that could save one of two seeks, but that's not *much* quicker.
(Indeed, a git ref would fit into the 60 bytes of block pointer space
in an ext2/3 inode if regular files were stuffed there as well as symlinks.)

> Ideally we would have two sha1 values in the cache - the sha1 in the
> file, and if that is the ID of a tag object, we would also put the
> sha1 of the commit that the tag points to in the cache.

Now that's not a bad idea.  Hacking it in to Linus's scheme, that's

<foo sha>\t<foo^{} sha>\tfoo


A couple of thoughts:
1) I bet Hans Reiser is enjoying this; he's been agitating for better
   lots-of-small-files support for years.

2) Since I've written about two caches in a few minutes (here
   and in git-rev-list), a standardized cache validation hook for
   git-fsck-objects and git-prune's use might be useful.

3) If we use Linus's idea of a flat "static refs" file overridden by loose
   refs (presumably, refs would be stuffed in if their mod times got old
   enough, and on initial import you'd use the timestamp of the commit
   they point to), we'll have to do a bit of a dance to move refs to and
   from it.

   Basically, to move refs into the refs file, it's
   - Read all the old refs and loose refs and write the new refs file.
   - Rename the new refs file into place.
   - For each loose ref moved in, lock it, verify it hasn'd changed,
     and delete it.
   with some more locking to prevent two people from doing this at once.

   Folks looking up tags will do an FS search, then validate their refs
   file cache, then if necessary, suck in the refs file.

   Now, exploding a refs file into loose refs is tricky.  There's
   the possible race condition with a reader:

   A: Looks for loose ref "foo", doesn't find it.
		B: Write out loose ref "foo"
		B: Deletes now-unpacked refs file
   A: Looks for refs file, doesn't find it.
   A: Concludes that ref "foo" doesn't exist.

   The only solution I can think of is to stat the refs file at the
   start of the operation and restart from the beginning if it changes
   by the time it actually opens and read it.

^ permalink raw reply

* Re: Change set based shallow clone
From: linux @ 2006-09-11 12:39 UTC (permalink / raw)
  To: linux, paulus; +Cc: git, jonsmirl, junkio, mcostalba, torvalds
In-Reply-To: <17669.13028.198886.947400@cargo.ozlabs.ibm.com>

>> I'm trying to figure out the gitk code, but I'm not fluent in tcl, and
>> it has 39 non-boilerplate comment lines in 229 functions and 6308 lines
>> of source, so it requires fairly intensive grokking.
>
> Sorry :)

I was more making excuses for my failure to understand its properties.
Still, if you get the chance, a couple of paragraphs of comments on the
primary data structures would be an immense help.

Anyway, thanks for the summary.

>> For example, is fixing a small number of out-of-place commits practical,
>> or is it better to purge and restart?  The former avoids deleting
>> already-existing objects, while the latter avoids moving them.

> For gitk, I'm thinking of a reorder buffer of say 10 entries at the
> front end to cope with minor misorderings; then if misordering occurs
> that is outside the scope of the reorder buffer to fix, freeze the
> layout algorithms at that point, read in the rest of the commits and
> reorder as necessary, then at the end restart the layout algorithm
> from scratch, probably with a popup to inform the user what happened.
> If the user could set the size of the reorder buffer then they could
> avoid having that happen in future, at the cost of it taking longer to
> show the first screenful of commits.

My thoughts were that anything you could do, git-rev-list could do better,
including the re-order buffer, so don't even bother.  (For example, I
was thinking of a time- or depth-based one rather than a count-based one.)

So I read your suggestion as a "purge and restart" preference, with
git-rev-list keeping a cache of problems encountered so that on most
runs, it doesn't have to do it.  Putting that logic in git-rev-list
means that you don't have to detect ordering problems or do any sorting,
just handle an "oops!" line in the output.

Basically, given the example graph (with letters indicating timestamps)

...G--D--C--B---A
          \    /
           F--E

You'd get something like
A B E
B C
C D
D G
E F
*** ERROR -  restarting - please wait
A B E
B C
E F
F C
C D
D G
G ...

^ permalink raw reply

* Re: What's in git.git
From: Jakub Narebski @ 2006-09-11 11:29 UTC (permalink / raw)
  To: git
In-Reply-To: <7vk64bnnxl.fsf@assigned-by-dhcp.cox.net>

Junio C Hamano wrote:

>  - Andy Whitcroft taught send-pack to use git-rev-list --stdin
>    so that we do not have to be limited by the number of refs
>    exec() command-line can hold.
[...]
>  - Pack-objects learned to run rev-list logic internally when
>    given --revs parameter; the refs arguments you would normally
>    give the upstream rev-list can be fed from its standard
>    input, instead of usual list of objects.
BTW. could you please document the above?

Perhaps those two options, --stdin to feed arguments from standard input,
and -revs to run rev-list logic internally should be used whenever possible
in all the git commands? This would allow to avoid forks and/or command
line length limit.
 
In 'next' currently the following commands have --stdin implemented:
 * git-update-index: --stdin to feed list of paths, one per line
 * git-diff-tree: --stdin to loop over <tree-ish>, or pairs of
   <tree-ish>[*1*]
 * git-hash-object: --stdin is equivalent of '-' special file
 * git-http-fetch and git-local-fetch have some strange --stdin
 * git-name-rev with --stdin functions as filter
 * git-rev-list: --stdin to feed list of <commits>; it is not clear from 
   the manpage if one can use ^<commit>, and commit related options
   and shortcuts like --not, <commit>..<commit>, <commit>...<commit>
And the following have --revs implemented
 * git-pack-objects: --revs to provide arguments to rev-list from stdin,
   instead of list of objects. UNDOCUMENTED.

It would be nice if the following commands had --stdin or had it's --stdin
usage extended:
 * git-diff-tree: --stdin to allow to provide path limits, separated 
   by ' -- ' from <tree-ish> or pair of <tree-ish> (does git-diff-tree allow
   for diff3-like behavior? then perhaps also three <tree-ish>)
 * git-ls-tree: --stdin to loop over <tree-ish>, one tree per line.
 * git-cat-object: --stdin to loop over objects, plus -z to change separator
   between records to NULL (or have it turned on by default).
For all "loop" --stdin, the output should begin with the line which was
arguments, like git-diff-tree outputs first <tree-ish> used for diff.

I think it is quite often to use git-rev-list ...| git-diff-tree ...
pipeline, so it might be worth to add --revs option to git-diff-tree.
Or it might not.

P.S. does git-merge take -F <file> option?
-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

^ permalink raw reply

* Re: [PATCH 3/3] Add sideband status report to git-archive protocol
From: Franck Bui-Huu @ 2006-09-11 10:34 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Franck Bui-Huu, git, Rene Scharfe
In-Reply-To: <7v1wqkt2v4.fsf_-_@assigned-by-dhcp.cox.net>

Junio C Hamano wrote:
> Using the refactored sideband code from existing upload-pack protocol,
> this lets the error condition and status output sent from the remote
> process to be shown locally.
> 
> Signed-off-by: Junio C Hamano <junkio@cox.net>
> ---

[snip]

> -}
> +	while (1) {
> +		struct pollfd pfd[2];
> +		char buf[16384];
> +		ssize_t sz;
> +		pid_t pid;
> +		int status;
> +
> +		pfd[0].fd = fd1[0];
> +		pfd[0].events = POLLIN;
> +		pfd[1].fd = fd2[0];
> +		pfd[1].events = POLLIN;
> +		if (poll(pfd, 2, -1) < 0) {
> +			if (errno != EINTR) {
> +				error("poll failed resuming: %s",
> +				      strerror(errno));
> +				sleep(1);
> +			}
> +			continue;
> +		}
> +		if (pfd[0].revents & (POLLIN|POLLHUP)) {
> +			/* Data stream ready */
> +			sz = read(pfd[0].fd, buf, sizeof(buf));
> +			send_sideband(1, 1, buf, sz, DEFAULT_PACKET_MAX);
> +		}
> +		if (pfd[1].revents & (POLLIN|POLLHUP)) {
> +			/* Status stream ready */
> +			sz = read(pfd[1].fd, buf, sizeof(buf));
> +			send_sideband(1, 2, buf, sz, DEFAULT_PACKET_MAX);
> +		}
>  
> +		if (((pfd[0].revents | pfd[1].revents) & POLLHUP) == 0)
> +			continue;
> +		/* did it die? */
> +		pid = waitpid(writer, &status, WNOHANG);
> +		if (!pid) {
> +			fprintf(stderr, "Hmph, HUP?\n");
> +			continue;
> +		}

I get a lot of "Hmph, HUP?" messages when testing "git-archive
--remote" command. One guess: this can be due to the fact that when
the writer process exits, it first closes its fd but do not send a
SIGCHLD signal right after to its parent.

Therefore poll() can return POLLHUP flag to the parent process but
waitpid still returns 0 because the writer process has still not sent
SIGCHLD signal to its parent.

How about this patch ? If poll() doesn't only return the single POLLIN
flag, then either the pipe has been closed because the write process
died or something wrong happened. In all these cases we can wait for
the writer process to exit then die.

-- >8 --

diff --git a/builtin-upload-archive.c b/builtin-upload-archive.c
index 42cb9f8..2ebe9a0 100644
--- a/builtin-upload-archive.c
+++ b/builtin-upload-archive.c
@@ -114,7 +114,6 @@ int cmd_upload_archive(int argc, const c
 		struct pollfd pfd[2];
 		char buf[16384];
 		ssize_t sz;
-		pid_t pid;
 		int status;
 
 		pfd[0].fd = fd1[0];
@@ -140,13 +139,11 @@ int cmd_upload_archive(int argc, const c
 			send_sideband(1, 2, buf, sz, LARGE_PACKET_MAX);
 		}
 
-		if (((pfd[0].revents | pfd[1].revents) & POLLHUP) == 0)
-			continue;
-		/* did it die? */
-		pid = waitpid(writer, &status, WNOHANG);
-		if (!pid) {
-			fprintf(stderr, "Hmph, HUP?\n");
+		if ((pfd[0].revents | pfd[1].revents) == POLLIN)
 			continue;
+
+		if (waitpid(writer, &status, 0) < 0) {
+			die("waitpid failed: %s", strerror(errno));
 		}
 		if (!WIFEXITED(status) || WEXITSTATUS(status) > 0)
 			send_sideband(1, 3, deadchild, strlen(deadchild),

^ permalink raw reply related

* Re: Change set based shallow clone
From: Paul Mackerras @ 2006-09-11  9:56 UTC (permalink / raw)
  To: linux; +Cc: junkio, mcostalba, git, jonsmirl, torvalds
In-Reply-To: <20060910152122.31694.qmail@science.horizon.com>

linux@horizon.com writes:

> I'm trying to figure out the gitk code, but I'm not fluent in tcl, and
> it has 39 non-boilerplate comment lines in 229 functions and 6308 lines
> of source, so it requires fairly intensive grokking.

Sorry :)

> Still, while it obviously doesn't render bitmaps until the data
> appears in the window, it appears as though at least part of the
> layout (the "layoutmore" function) code is performed eagerly as
> soon as new data arrives via the getcommitlines callback.

It doesn't render bitmaps as such at the Tcl level, what it does is
create items on canvases, add text to text widgets, etc.  Tk takes
care of turning it into bitmaps.

The work of drawing the graph is done in three stages - layout,
optimization and drawing.  The layout and optimization stages are done
when we have enough data because, at least for the layout stage,
earlier rows can affect later rows arbitrarily far away.  For the
optimization stage, I don't think the changes it makes can propagate
arbitrarily far, so it would be possible to defer that stage, and I am
looking into implementing that.  The drawing stage is done on demand,
just for the part that's actually visible.

> (Indeed, it appears that Tk does not inform it of window scroll
> events, so it can't wait any longer to decide on the layout.)

The Tcl code does get to know when the canvas has been scrolled - see
the scrollcanv procedure.

> In case 2, I utterly fail to see how delaying emitting the out-of-order
> commit is of the slightest help to the UI.  The simplest way to merge

Indeed.  I would want to see it as early as possible.

> out-of-order data is with an insertion sort (a.k.a. roll back and
> reprocess forward), and the cost of that is minimized if the distance
> to back up is minimized.

That part isn't too hard; I already have modifications to gitk to
handle that much of it.  The harder part is backing up the graph
drawing algorithm.

> For example, is fixing a small number of out-of-place commits practical,
> or is it better to purge and restart?  The former avoids deleting
> already-existing objects, while the latter avoids moving them.

For gitk, I'm thinking of a reorder buffer of say 10 entries at the
front end to cope with minor misorderings; then if misordering occurs
that is outside the scope of the reorder buffer to fix, freeze the
layout algorithms at that point, read in the rest of the commits and
reorder as necessary, then at the end restart the layout algorithm
from scratch, probably with a popup to inform the user what happened.
If the user could set the size of the reorder buffer then they could
avoid having that happen in future, at the cost of it taking longer to
show the first screenful of commits.

Paul.

^ permalink raw reply

* Re: git web view doesn't show short log in chronological order ?
From: Andy Whitcroft @ 2006-09-11  9:19 UTC (permalink / raw)
  To: Toralf Förster; +Cc: git
In-Reply-To: <200609111101.00801.toralf.foerster@gmx.de>

Toralf Förster wrote:
> http://www.kernel.org/git/?p=linux%2Fkernel%2Fgit%2Ftorvalds%2Flinux-2.6.git;a=summary
> lists the entry from Kirill Korotaev at position 12 (all first 12 positions are
> 2 days old), but Kirill Korotaev did the latest commit (if I do a "$>git log" at
> my local clone of Linus' tree) and therefore should be listed at position 1,
> isn't it ?

The dates on commits can always be suspect as they are generally in the
originators local clock.  You also have to remember the date which is
shown is the authors original create time.  Not the time it was commited
to linus' tree.

/me pulls the dates for 11 & 12:

Author: Thomas Gleixner <tglx@linutronix.de>  2006-09-08 17:47:15
Committer: Linus Torvalds <torvalds@g5.osdl.org>  2006-09-08 18:22:50

Author: Kirill Korotaev <dev@openvz.org>  2006-09-07 11:17:04
Committer: Linus Torvalds <torvalds@g5.osdl.org>  2006-09-08 16:40:46

From what I can see both dates here are older in Kirill's change.  So by
any sort they are in the right order (where right is reverse chronological).

?

-apw

^ 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