Git development
 help / color / mirror / Atom feed
* [PATCH 3/7] Documentation: rework SHA1 description in git push
From: Anders Melchiorsen @ 2008-10-29 20:25 UTC (permalink / raw)
  To: git; +Cc: gitster, Anders Melchiorsen
In-Reply-To: <1225311945-17100-3-git-send-email-mail@cup.kalibalik.dk>

Get rid of a double pair of parentheses. The arbitrary SHA1 is a
special case, so it can be postponed a bit.

Also mention HEAD, which is possibly the most useful SHA1 in this
situation.

Mention that a SHA1 cannot be automatically matched to a <dst>.

Add HEAD as an example of an arbitrary SHA1.

Signed-off-by: Anders Melchiorsen <mail@cup.kalibalik.dk>
---
 Documentation/git-push.txt |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 02c7dae..fb9fb97 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -38,9 +38,7 @@ OPTIONS
 	by the source ref, followed by a colon `:`, followed by
 	the destination ref.
 +
-The <src> side represents the source branch (or arbitrary
-"SHA1 expression", such as `master~4` (four parents before the
-tip of `master` branch); see linkgit:git-rev-parse[1]) that you
+The <src> side represents the source branch that you
 want to push.  The <dst> side represents the destination location.
 +
 The local ref that matches <src> is used
@@ -63,6 +61,10 @@ the local side, the remote side is updated if a head of the same name
 already exists on the remote side.  This is the default operation mode
 if no explicit refspec is found (that is neither on the command line
 nor in any Push line of the corresponding remotes file---see below).
++
+The <src> can be an arbitrary "SHA1 expression", such as `HEAD`
+or `master~4` (see linkgit:git-rev-parse[1]). In this case, a
+<dst> location has to be named.
 
 --all::
 	Instead of naming each ref to push, specifies that all
@@ -193,6 +195,10 @@ git push origin master::
 	with it.  If `master` did not exist remotely, it would be
 	created.
 
+git push origin HEAD:master::
+	Push the current head to the remote ref matching `master` in
+	the `origin` repository.
+
 git push origin :experimental::
 	Find a ref that matches `experimental` in the `origin` repository
 	(e.g. `refs/heads/experimental`), and delete it.
-- 
1.6.0.2.514.g23abd3

^ permalink raw reply related

* [PATCH 2/7] Documentation: git push repository can also be a remote
From: Anders Melchiorsen @ 2008-10-29 20:25 UTC (permalink / raw)
  To: git; +Cc: gitster, Anders Melchiorsen
In-Reply-To: <1225311945-17100-2-git-send-email-mail@cup.kalibalik.dk>

This is copied from pull-fetch-param.txt and helps the reader
to not get stuck in the URL section.

Signed-off-by: Anders Melchiorsen <mail@cup.kalibalik.dk>
---
 Documentation/git-push.txt |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index df99c0b..02c7dae 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -28,7 +28,9 @@ OPTIONS
 -------
 <repository>::
 	The "remote" repository that is destination of a push
-	operation.  See the section <<URLS,GIT URLS>> below.
+	operation.  This parameter can be either a URL
+	(see the section <<URLS,GIT URLS>> below) or the name
+	of a remote (see the section <<REMOTES,REMOTES>> below).
 
 <refspec>...::
 	The canonical format of a <refspec> parameter is
-- 
1.6.0.2.514.g23abd3

^ permalink raw reply related

* [PATCH 6/7] Documentation: mention branches rather than heads
From: Anders Melchiorsen @ 2008-10-29 20:25 UTC (permalink / raw)
  To: git; +Cc: gitster, Anders Melchiorsen
In-Reply-To: <1225311945-17100-6-git-send-email-mail@cup.kalibalik.dk>

Most of the git push page talks about branches, so make it consistent
also in this paragraph.

Signed-off-by: Anders Melchiorsen <mail@cup.kalibalik.dk>
---
 Documentation/git-push.txt |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 52035db..4e339c7 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -53,8 +53,8 @@ Pushing an empty <src> allows you to delete the <dst> ref from
 the remote repository.
 +
 The special refspec `:` (or `+:` to allow non-fast forward updates)
-directs git to push "matching" heads: for every head that exists on
-the local side, the remote side is updated if a head of the same name
+directs git to push "matching" branches: for every branch that exists on
+the local side, the remote side is updated if a branch of the same name
 already exists on the remote side.  This is the default operation mode
 if no explicit refspec is found (that is neither on the command line
 nor in any Push line of the corresponding remotes file---see below).
-- 
1.6.0.2.514.g23abd3

^ permalink raw reply related

* [PATCH 5/7] Documentation: elaborate on pushing tags
From: Anders Melchiorsen @ 2008-10-29 20:25 UTC (permalink / raw)
  To: git; +Cc: gitster, Anders Melchiorsen
In-Reply-To: <1225311945-17100-5-git-send-email-mail@cup.kalibalik.dk>

Make the description of pushing tags easier to read, but move the
shorthand notation towards the end of the description. This gives
a better flow.

Signed-off-by: Anders Melchiorsen <mail@cup.kalibalik.dk>
---
 Documentation/git-push.txt |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 9788d49..52035db 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -38,7 +38,7 @@ OPTIONS
 	by the source ref, followed by a colon `:`, followed by
 	the destination ref.
 +
-The <src> side represents the source branch that you
+The <src> side represents the source branch or tag that you
 want to push.  The <dst> side represents the destination location.
 +
 The local ref that matches <src> is used
@@ -46,8 +46,6 @@ to fast forward the remote ref that matches <dst>.  If
 the optional leading plus `+` is used, the remote ref is updated
 even if it does not result in a fast forward update.
 +
-`tag <tag>` means the same as `refs/tags/<tag>:refs/tags/<tag>`.
-+
 A parameter <ref> without a colon pushes the <ref> from the source
 repository to the destination repository under the same name.
 +
@@ -64,6 +62,10 @@ nor in any Push line of the corresponding remotes file---see below).
 The <src> can be an arbitrary "SHA1 expression", such as `HEAD`
 or `master~4` (see linkgit:git-rev-parse[1]). In this case, a
 <dst> location has to be named.
++
+When pushing tags, `tag <tag>` can be used as a shorthand for
+`refs/tags/<tag>:refs/tags/<tag>`. This should be specified as
+two arguments (that is, without quotes in the shell).
 
 --all::
 	Instead of naming each ref to push, specifies that all
-- 
1.6.0.2.514.g23abd3

^ permalink raw reply related

* [PATCH 4/7] Documentation: remove a redundant elaboration
From: Anders Melchiorsen @ 2008-10-29 20:25 UTC (permalink / raw)
  To: git; +Cc: gitster, Anders Melchiorsen
In-Reply-To: <1225311945-17100-4-git-send-email-mail@cup.kalibalik.dk>

The comment in parentheses is wrong, as one has to leave out both the
colon and <dst>. This situation is covered by the section a few lines
down:

  A parameter <ref> without a colon pushes the <ref> from the source
  repository to the destination repository under the same name.

So, just remove the parentheses.

Signed-off-by: Anders Melchiorsen <mail@cup.kalibalik.dk>
---
 Documentation/git-push.txt |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index fb9fb97..9788d49 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -42,8 +42,7 @@ The <src> side represents the source branch that you
 want to push.  The <dst> side represents the destination location.
 +
 The local ref that matches <src> is used
-to fast forward the remote ref that matches <dst> (or, if no <dst> was
-specified, the same ref that <src> referred to locally).  If
+to fast forward the remote ref that matches <dst>.  If
 the optional leading plus `+` is used, the remote ref is updated
 even if it does not result in a fast forward update.
 +
-- 
1.6.0.2.514.g23abd3

^ permalink raw reply related

* [PATCH 7/7] Documentation: avoid using undefined parameters
From: Anders Melchiorsen @ 2008-10-29 20:25 UTC (permalink / raw)
  To: git; +Cc: gitster, Anders Melchiorsen
In-Reply-To: <1225311945-17100-7-git-send-email-mail@cup.kalibalik.dk>

The <ref> parameter has not been introduced, so rewrite to
avoid it.

Signed-off-by: Anders Melchiorsen <mail@cup.kalibalik.dk>
---
 Documentation/git-push.txt |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 4e339c7..e848ff9 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -46,8 +46,8 @@ to fast forward the remote ref that matches <dst>.  If
 the optional leading plus `+` is used, the remote ref is updated
 even if it does not result in a fast forward update.
 +
-A parameter <ref> without a colon pushes the <ref> from the source
-repository to the destination repository under the same name.
+A lonely <src> parameter (without a colon and a destination) pushes
+the <src> to the same name in the destination repository.
 +
 Pushing an empty <src> allows you to delete the <dst> ref from
 the remote repository.
-- 
1.6.0.2.514.g23abd3

^ permalink raw reply related

* Re: [PATCH] increase git el1T3nEss
From: Daniel Barkalow @ 2008-10-29 20:28 UTC (permalink / raw)
  To: Jeff King
  Cc: Petr Baudis, git, Johannes Schindelin, Scott Chacon,
	Tom Preston-Werner, J.H., Sam Vilain, Christian Couder, Kai Blin
In-Reply-To: <20081029170631.GA12078@sigill.intra.peff.net>

On Wed, 29 Oct 2008, Jeff King wrote:

> The uptake of git by script kiddies has been disappointingly
> minimal. Let's make it more palatable by allowing mixed-case
> and l33t-speak commands.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> This commit was made by "git c0mM1t".
> 
>  git.c |   27 ++++++++++++++++++++++++++-
>  1 files changed, 26 insertions(+), 1 deletions(-)
> 
> diff --git a/git.c b/git.c
> index 89feb0b..fd0ca67 100644
> --- a/git.c
> +++ b/git.c
> @@ -261,6 +261,31 @@ static int run_command(struct cmd_struct *p, int argc, const char **argv)
>  	return 0;
>  }
>  
> +static char deelite(char in) {
> +	if (isalpha(in))
> +		return tolower(in);
> +	switch (in) {
> +	case '0': return 'o';
> +	case '1': return 'i';
> +	case '3': return 'e';
> +	case '5': return 's';
> +	case '7': return 'l';

Your mapping doesn't comply with ISO 1337.5p34k; 1 can be l (in addition 
to i), and 7 is T. Also, you're missing 4.

	-Daniel
*This .sig left intentionally blank*

^ permalink raw reply

* Re: Using the --track option when creating a branch
From: Bill Lear @ 2008-10-29 20:33 UTC (permalink / raw)
  To: Santi Béjar; +Cc: git
In-Reply-To: <adf1fd3d0810290925s493cdc6oc7534904c864db28@mail.gmail.com>

On Wednesday, October 29, 2008 at 17:25:37 (+0100) Santi Béjar writes:
>On Wed, Oct 29, 2008 at 4:23 PM, Bill Lear <rael@zopyra.com> wrote:
>> We have had a few "crossed stream" problems when developers are
>> working on a local branch and they do an unguarded git push/pull,
>> when they really intended to do git push/pull origin branchname.
>>
>> We use git in a way that makes it desirable for us to only push/pull
>> to the same remote branch.  So, if I'm in branch X, I want 'git push'
>> to push to origin/X, and 'git pull' to fetch into origin/X and then
>> merge into X from origin/X.
>>
>> In other words, we want git push/pull to behave in branches other than
>> master the same way it does when in master.
>>
>> I have discovered the '--track' option when creating a local branch,
>> and this appears to me to be the thing that gives us the desired
>> behavior.
>
>branch.autosetupmerge controls if --track is used by default (it is
>true by default since a long time)
>(See "git help config" for details)

Ah, problem solved then.  I'll just have everyone upgrade to the
latest git.  Thanks very much, Santi.


Bill

^ permalink raw reply

* Re: [PATCH 5/7] Documentation: elaborate on pushing tags
From: Daniel Barkalow @ 2008-10-29 20:40 UTC (permalink / raw)
  To: Anders Melchiorsen; +Cc: git, gitster
In-Reply-To: <1225311945-17100-6-git-send-email-mail@cup.kalibalik.dk>

On Wed, 29 Oct 2008, Anders Melchiorsen wrote:

> Make the description of pushing tags easier to read, but move the
> shorthand notation towards the end of the description. This gives
> a better flow.
> 
> Signed-off-by: Anders Melchiorsen <mail@cup.kalibalik.dk>
> ---
>  Documentation/git-push.txt |    8 +++++---
>  1 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
> index 9788d49..52035db 100644
> --- a/Documentation/git-push.txt
> +++ b/Documentation/git-push.txt
> @@ -38,7 +38,7 @@ OPTIONS
>  	by the source ref, followed by a colon `:`, followed by
>  	the destination ref.
>  +
> -The <src> side represents the source branch that you
> +The <src> side represents the source branch or tag that you
>  want to push.  The <dst> side represents the destination location.
>  +
>  The local ref that matches <src> is used
> @@ -46,8 +46,6 @@ to fast forward the remote ref that matches <dst>.  If
>  the optional leading plus `+` is used, the remote ref is updated
>  even if it does not result in a fast forward update.
>  +
> -`tag <tag>` means the same as `refs/tags/<tag>:refs/tags/<tag>`.
> -+
>  A parameter <ref> without a colon pushes the <ref> from the source
>  repository to the destination repository under the same name.
>  +
> @@ -64,6 +62,10 @@ nor in any Push line of the corresponding remotes file---see below).
>  The <src> can be an arbitrary "SHA1 expression", such as `HEAD`
>  or `master~4` (see linkgit:git-rev-parse[1]). In this case, a
>  <dst> location has to be named.
> ++
> +When pushing tags, `tag <tag>` can be used as a shorthand for
> +`refs/tags/<tag>:refs/tags/<tag>`. This should be specified as
> +two arguments (that is, without quotes in the shell).

I think this should be "When pushing a tag, ...", to avoid confusion with 
"--tags", which pushes (all) tags and doesn't need the "tag <tag>" 
arguments.

	-Daniel
*This .sig left intentionally blank*

^ permalink raw reply

* Re: Some updates to refspec documentation
From: Anders Melchiorsen @ 2008-10-29 20:41 UTC (permalink / raw)
  To: git
In-Reply-To: <1225311945-17100-1-git-send-email-mail@cup.kalibalik.dk>

During my struggle, I also found a git push usability problem. The
refspec ":dst" will delete the remote branch without warning.

While this is indeed documented, you cannot blame somebody for
thinking that a left out <src> will default to HEAD, as it does for so
many other commands.

In a CVS-like setup, with everybody pushing to master, this could
cause havoc.

As the "git push <repository> :<dst>" operation is destructive, I
think it should be protected by the -f option.

For the record, I did realize this before trying to push ":master" :-).



Anders.

^ permalink raw reply

* Re: Some updates to refspec documentation
From: Anders Melchiorsen @ 2008-10-29 20:44 UTC (permalink / raw)
  To: git
In-Reply-To: <878ws7b0d2.fsf@cup.kalibalik.dk>

Anders Melchiorsen <mail@cup.kalibalik.dk> writes:

> While this is indeed documented, you cannot blame somebody for
> thinking that a left out <src> will default to HEAD, as it does for
> so many other commands.

I actually think that <src> defaulting to HEAD would be reasonable,
but it is probably less reasonable to change the meaning at this
point.



Anders

^ permalink raw reply

* a few git notes numbers
From: Jeff King @ 2008-10-29 21:40 UTC (permalink / raw)
  To: Johannes Schindelin, Shawn O. Pearce; +Cc: git

I looked a little bit at how bad tree lookup speed would be for notes.

My test implementation was a notes tree made like this:

 blob=`echo this is a note | git hash-object -w --stdin`
 export GIT_INDEX_FILE=.git/notes
 # attach the note to every commit;
 # note that this uses a less-efficient ascii encoding
 git rev-list HEAD | while read commit; do
   echo -e "100644 blob $blob\t$commit"
 done | git update-index --index-info
 tree=`git write-tree`
 commit=`echo notes | git commit-tree $tree`
 git update-ref refs/notes $tree

Then I added a %N format specifier that looked up the note in the
refs/notes tree for every commit. For a baseline, here is just looking
at the subject of a commit message:

   /usr/bin/time git log --pretty=format:'%s' >/dev/null
   0.57user 0.00system 0:00.58elapsed 100%CPU (0avgtext+0avgdata 0maxresident)k
   0inputs+0outputs (0major+3497minor)pagefaults 0swaps

So it takes about half a second without notes. When I used the regular
init_tree_desc() and tree_entry() walk, that bumped to about 37 seconds
(sorry, I didn't save the /usr/bin/time output). Converting the stupid
ascii filename format to binary brought it down to 27 seconds. So both
obviously crappy due to the linear walk.

Then at somebody (David Reiss?)'s suggestion, I used our hash table
implementation and pre-seeded the table. I kept with the ascii
filenames in the tree, since they are a little easier to create, and
since we only pay the price for conversion when seeding the hash table.
And I got these numbers:

  $ /usr/bin/time git log --pretty=format:'%s: %N' >/dev/null
  0.85user 0.12system 0:00.98elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k
  0inputs+0outputs (0major+20914minor)pagefaults 0swaps

So not too bad, though check out those minor page faults. Patch is
below:

---
diff --git a/pretty.c b/pretty.c
index f6ff312..c4930ac 100644
--- a/pretty.c
+++ b/pretty.c
@@ -498,6 +498,67 @@ static void format_decoration(struct strbuf *sb, const struct commit *commit)
 		strbuf_addch(sb, ')');
 }
 
+struct note_entry {
+	unsigned char from[20];
+	unsigned char to[20];
+	struct note_entry *next;
+};
+
+static struct hash_table *notes_tree(void)
+{
+	static int initialized;
+	static struct hash_table table;
+
+	if (!initialized) {
+		unsigned char sha1[20];
+		struct tree *tree;
+		struct tree_desc desc;
+		struct name_entry entry;
+
+		init_hash(&table);
+		if(get_sha1("refs/notes", sha1) == 0)
+			tree = parse_tree_indirect(sha1);
+		init_tree_desc(&desc, tree->buffer, tree->size);
+		while (tree_entry(&desc, &entry)) {
+			struct note_entry *note;
+			unsigned int hash;
+			void **pos;
+
+			note = malloc(sizeof *note);
+			if (get_sha1_hex(entry.path, note->from) < 0) {
+				free(note);
+				continue;
+			}
+			memcpy(note->to, entry.sha1, 20);
+			memcpy(&hash, note->from, sizeof(hash));
+			pos = insert_hash(hash, note, &table);
+			if (pos) {
+				note->next = *pos;
+				*pos = note;
+			}
+		}
+
+		initialized = 1;
+	}
+	return &table;
+}
+
+static char *lookup_note(const unsigned char *sha1, unsigned long *size) {
+	struct hash_table *table = notes_tree();
+	struct note_entry *note;
+	unsigned int hash;
+
+	memcpy(&hash, sha1, sizeof(hash));
+	note = lookup_hash(hash, table);
+	while (note) {
+		if (!memcmp(sha1, note->from, 20)) {
+			enum object_type type;
+			return read_sha1_file(note->to, &type, size);
+		}
+	}
+	return NULL;
+}
+
 static size_t format_commit_item(struct strbuf *sb, const char *placeholder,
                                void *context)
 {
@@ -537,6 +598,17 @@ static size_t format_commit_item(struct strbuf *sb, const char *placeholder,
 			return 3;
 		} else
 			return 0;
+	case 'N':
+		{
+			unsigned long size;
+			char *buf;
+			buf = lookup_note(commit->object.sha1, &size);
+			if (buf) {
+				strbuf_add(sb, buf, size);
+				free(buf);
+			}
+		}
+		return 1;
 	}
 
 	/* these depend on the commit */

^ permalink raw reply related

* Re: [PATCH] increase git el1T3nEss
From: Jeff King @ 2008-10-29 21:50 UTC (permalink / raw)
  To: Daniel Barkalow
  Cc: Petr Baudis, git, Johannes Schindelin, Scott Chacon,
	Tom Preston-Werner, J.H., Sam Vilain, Christian Couder, Kai Blin
In-Reply-To: <alpine.LNX.1.00.0810291623360.19665@iabervon.org>

On Wed, Oct 29, 2008 at 04:28:16PM -0400, Daniel Barkalow wrote:

> Your mapping doesn't comply with ISO 1337.5p34k; 1 can be l (in addition 
> to i), and 7 is T. Also, you're missing 4.

Patches welcome.

-Peff

^ permalink raw reply

* Re: [PATCH] Use find instead of perl in t5000 to get file modification time
From: Jeff King @ 2008-10-29 21:54 UTC (permalink / raw)
  To: Alex Riesen; +Cc: Git Mailing List, Junio C Hamano, René Scharfe
In-Reply-To: <81b0412b0810290338j1beaa25bx9fb373a69f5dfe7@mail.gmail.com>

On Wed, Oct 29, 2008 at 11:38:32AM +0100, Alex Riesen wrote:

> There can be a problem with "-printf": Open Group SUS does not specify
> -printf for find(1), so it is probably a problem somewhere. I just don't know.
> [...]
> +     find extract/a/a -printf "%T@\\n" >b.mtime &&

$ uname -sr
FreeBSD 6.1-RELEASE-p17-jc1
$ find . -printf "%T@\\n"
find: -printf: unknown option

> There is always a fallback, which is to write a small program which calls
> native stat(2). Or modify test-chmtime to just print mtime when asked.

I think that makes the most sense.

-Peff

^ permalink raw reply

* request for pre-generated git.info pages
From: Leo Razoumov @ 2008-10-29 22:00 UTC (permalink / raw)
  To: git, Junio C Hamano

I am an emacs user but with my current setup I have difficulties
generating git.info pages. Main Git repo already provides
pre-generated man and html  documentation in "man" and "html"
branches. I would kindly beg to extend this privilege to "info" pages
to benefit all the emacs users out there.

--Leo--

^ permalink raw reply

* Re: A typesetting problem with git man pages
From: Teemu Likonen @ 2008-10-29 22:22 UTC (permalink / raw)
  To: Jonas Fonseca; +Cc: git
In-Reply-To: <2c6b72b30810291235j554cc21dw4e3da4fdbfe633ee@mail.gmail.com>

Jonas Fonseca (2008-10-29 20:35 +0100) wrote:

> On Wed, Oct 29, 2008 at 20:16, Teemu Likonen <tlikonen@iki.fi> wrote:
>> Does anybody know why "man" prints those ".ft" commands? The
>> corresponding code in git-log.1 file is this:

> I had a similar problem after upgrading on Ubuntu and came up with a
> patch to optionally disable some of asciidoc.conf (commit
> 7f55cf451c9e7). Try putting DOCBOOK_XSL_172=Yes in your config.mak.

Ah, thank you. That fixed it.

In case someone is interested there is still a minor flaw that an
example command and the following paragraph is printed with no empty
line between them. Like in the beginning of "git help tutorial", for
example:

    First, note that you can get documentation for a command such as git
    log --graph with:

        $ man git-log             
    It is a good idea to introduce yourself to git [...]

It would be nicer if there was empty line after "$ man git-log". I can't
remember if this is new issue or not. This applies only to man pages; in
html pages there are nice boxes around example commands and equal
spacing before and after them.

^ permalink raw reply

* Re: [PATCH 1/1] diff: support making output friendlier for fine, grand users
From: Petr Baudis @ 2008-10-29 22:24 UTC (permalink / raw)
  To: git, A Large Angry SCM
  Cc: Johannes Schindelin, Scott Chacon, Tom Preston-Werner, Jeff King,
	J.H., Sam Vilain, Christian Couder, Kai Blin
In-Reply-To: <1225257832-29086-1-git-send-email-pasky@suse.cz>

On Tue, Oct 28, 2008 at 10:23:52PM -0700, Petr Baudis wrote:
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> Signed-off-by: Scott Chacon <schacon@gmail.com>
> Signed-off-by: Tom Preston-Werner <tom@github.com>
> Signed-off-by: Jeff King <peff@peff.net>
> Signed-off-by: J.H. <warthog19@eaglescrag.net>
> Signed-off-by: Sam Vilain <sam@vilain.net>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> Signed-off-by: Kai Blin <kai@samba.org>
> Acked-by: Petr Baudis <pasky@suse.cz>
> Enabled-by: Cascade "Smooth" Amber <clarity@tiedhouse.com>

Sponsored-by: A Large Angry SCM <gitzilla@gmail.com>

^ permalink raw reply

* jgit as a jira plugin
From: J. Longman @ 2008-10-29 23:02 UTC (permalink / raw)
  To: git

Hey there,

I've integrated jgit into a plugin for the Jira Issue tracking  
system.  There is more information here: http://confluence.atlassian.com/display/JIRAEXT/Jira+Git+Plugin

I'm new to git, jgit and jira plugin writing, so I have a number of  
questions.  These are the ones I have for you:

1) I noticed that there is a maven pom file.  Are you present in a  
maven repository?  Also any problem with embedding a working snapshot  
in my plugin?
2) I'd like to find out the jgit way to achieve the equivalent of 'svn  
update'.  I understand that fetch can do this but being new to git, I  
don't really understand quite what I need yet.  The goal is to have  
git the latest commits from the origin before indexing.

Thanks for jgit - it took me a day or two to wrap my head around  
getting the list of files changed in a commit but otherwise its great  
to have something that can be integrated into jira.

later, jl

--
J. Longman
longman@xiplink.com

The information transmitted is intended only for the person or entity to
which it is addressed and may contain confidential and/or privileged
material.  If you have received this in error, please contact the sender
and delete this communication and any copy immediately.  Thank you.

^ permalink raw reply

* [PATCH 0/9] more robustness against pack corruptions
From: Nicolas Pitre @ 2008-10-29 23:02 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

A few months ago I produced a set of patches to allow git to work even
in the presence of pack corruption given that the corrupted objects have
a good duplicate in the object store.  Turns out that this work was
rather incomplete and covered only a limited set of cases.

This series extend coverage to all cases I could think about, and make
repack-objects able to create a good pack in such conditions to "fix"
the corruption without having to perform a full repack.

Yes, this is all about the small and trivial patch I posted a while ago
that I intended to repost with a test case.  Well, the test failed
miserably, resulting in this series before it finally all passed.  ;-)

 builtin-pack-objects.c                |   79 +++++++++++++++++-----
 builtin-unpack-objects.c              |    2 +
 cache.h                               |    2 +-
 index-pack.c                          |    2 +-
 pack-revindex.c                       |    3 +-
 sha1_file.c                           |   85 ++++++++++++++++++-----
 t/t5302-pack-index.sh                 |    3 +-
 t/t5303-pack-corruption-resilience.sh |   96 +++++++++++++++++++++++++--
 8 files changed, 223 insertions(+), 49 deletions(-)


Nicolas

^ permalink raw reply

* [PATCH 1/9] close another possibility for propagating pack corruption
From: Nicolas Pitre @ 2008-10-29 23:02 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <1225321372-6570-1-git-send-email-nico@cam.org>

Abstract
--------

With index v2 we have a per object CRC to allow quick and safe reuse of
pack data when repacking.  this, however, doesn't currently prevent a
stealth corruption from being propagated into a new pack when _not_
reusing pack data as demonstrated by the modification to t5302 included
here.

The Context
-----------

The Git database is all checksumed with SHA1 hashes.  Any kind of
corruption can be confirmed by verifying this per object hash against
corresponding data.  However this can be costly to perform systematically
and therefore this check is often not performed at run time when
accessing the object database.

First, the loose object format is entirely compressed with zlib which
already provide a CRC verification of its own when inflating data.  Any
disk corruption would be caught already in this case.

Then, packed objects are also compressed with zlib but only for their
actual payload.  The object headers and delta base references are not
deflated for obvious performance reasons, however this leave them
vulnerable to potentially undetected disk corruptions.  Object types
are often validated against the expected type when they're requested,
and deflated size must always match the size recorded in the object header,
so those cases are pretty much covered as well.

Where corruptions could go unnoticed is in the delta base reference.
Of course, in the OBJ_REF_DELTA case,  the odds for a SHA1 reference to
get corrupted so it actually matches the SHA1 of another object with the
same size (the delta header stores the expected size of the base object
to apply against) are virtually zero.  In the OBJ_OFS_DELTA case, the
reference is a pack offset which would have to match the start boundary
of a different base object but still with the same size, and although this
is relatively much more "probable" than in the OBJ_REF_DELTA case, the
probability is also about zero in absolute terms.  Still, the possibility
exists as demonstrated in t5302 and is certainly greater than a SHA1
collision, especially in the OBJ_OFS_DELTA case which is now the default
when repacking.

Again, repacking by reusing existing pack data is OK since the per object
CRC provided by index v2 guards against any such corruptions. What t5302
failed to test is a full repack in such case.

The Solution
------------

As unlikely as this kind of stealth corruption can be in practice, it
certainly isn't acceptable to propagate it into a freshly created pack.
But, because this is so unlikely, we don't want to pay the run time cost
associated with extra validation checks all the time either.  Furthermore,
consequences of such corruption in anything but repacking should be rather
visible, and even if it could be quite unpleasant, it still has far less
severe consequences than actively creating bad packs.

So the best compromize is to check packed object CRC when unpacking
objects, and only during the compression/writing phase of a repack, and
only when not streaming the result.  The cost of this is minimal (less
than 1% CPU time), and visible only with a full repack.

Someone with a stats background could provide an objective evaluation of
this, but I suspect that it's bad RAM that has more potential for data
corruptions at this point, even in those cases where this extra check
is not performed.  Still, it is best to prevent a known hole for
corruption when recreating object data into a new pack.

What about the streamed pack case?  Well, any client receiving a pack
must always consider that pack as untrusty and perform full validation
anyway, hence no such stealth corruption could be propagated to remote
repositoryes already.  It is therefore worthless doing local validation
in that case.

Signed-off-by: Nicolas Pitre <nico@cam.org>
---
 builtin-pack-objects.c |   12 ++++++++++++
 sha1_file.c            |   15 +++++++++++++++
 t/t5302-pack-index.sh  |    3 ++-
 3 files changed, 29 insertions(+), 1 deletions(-)

diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 59c30d1..0366277 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -1689,6 +1689,8 @@ static int add_ref_tag(const char *path, const unsigned char *sha1, int flag, vo
 	return 0;
 }
 
+extern int do_check_packed_object_crc;
+
 static void prepare_pack(int window, int depth)
 {
 	struct object_entry **delta_list;
@@ -1697,6 +1699,16 @@ static void prepare_pack(int window, int depth)
 
 	get_object_details();
 
+	/*
+	 * If we're locally repacking then we need to be doubly careful
+	 * from now on in order to make sure no stealth corruption gets
+	 * propagated to the new pack.  Clients receiving streamed packs
+	 * should validate everything they get anyway so no need to incure
+	 * the additional cost here in that case.
+	 */
+	if (!pack_to_stdout)
+		do_check_packed_object_crc = 1;
+
 	if (!nr_objects || !window || !depth)
 		return;
 
diff --git a/sha1_file.c b/sha1_file.c
index ab2b520..88d9cf3 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1694,6 +1694,8 @@ static void *unpack_delta_entry(struct packed_git *p,
 	return result;
 }
 
+int do_check_packed_object_crc;
+
 void *unpack_entry(struct packed_git *p, off_t obj_offset,
 		   enum object_type *type, unsigned long *sizep)
 {
@@ -1701,6 +1703,19 @@ void *unpack_entry(struct packed_git *p, off_t obj_offset,
 	off_t curpos = obj_offset;
 	void *data;
 
+	if (do_check_packed_object_crc && p->index_version > 1) {
+		struct revindex_entry *revidx = find_pack_revindex(p, obj_offset);
+		unsigned long len = revidx[1].offset - obj_offset;
+		if (check_pack_crc(p, &w_curs, obj_offset, len, revidx->nr)) {
+			const unsigned char *sha1 =
+				nth_packed_object_sha1(p, revidx->nr);
+			error("bad packed object CRC for %s",
+			      sha1_to_hex(sha1));
+			mark_bad_packed_object(p, sha1);
+			return NULL;
+		}
+	}
+
 	*type = unpack_object_header(p, &w_curs, &curpos, sizep);
 	switch (*type) {
 	case OBJ_OFS_DELTA:
diff --git a/t/t5302-pack-index.sh b/t/t5302-pack-index.sh
index b0b0fda..884e242 100755
--- a/t/t5302-pack-index.sh
+++ b/t/t5302-pack-index.sh
@@ -196,7 +196,8 @@ test_expect_success \
 
 test_expect_success \
     '[index v2] 5) pack-objects refuses to reuse corrupted data' \
-    'test_must_fail git pack-objects test-5 <obj-list'
+    'test_must_fail git pack-objects test-5 <obj-list &&
+     test_must_fail git pack-objects --no-reuse-object test-6 <obj-list'
 
 test_expect_success \
     '[index v2] 6) verify-pack detects CRC mismatch' \
-- 
1.6.0.3.757.g01be.dirty

^ permalink raw reply related

* [PATCH 2/9] better validation on delta base object offsets
From: Nicolas Pitre @ 2008-10-29 23:02 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <1225321372-6570-2-git-send-email-nico@cam.org>

In one case, it was possible to have a bad offset equal to 0 effectively
pointing a delta onto itself and crashing git after too many recursions.
In the other cases, a negative offset could result due to off_t being
signed.  Catch those.

Signed-off-by: Nicolas Pitre <nico@cam.org>
---
 builtin-pack-objects.c   |    4 ++--
 builtin-unpack-objects.c |    2 ++
 index-pack.c             |    2 +-
 sha1_file.c              |    2 +-
 4 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 0366277..d4c721b 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -1038,10 +1038,10 @@ static void check_object(struct object_entry *entry)
 				c = buf[used_0++];
 				ofs = (ofs << 7) + (c & 127);
 			}
-			if (ofs >= entry->in_pack_offset)
+			ofs = entry->in_pack_offset - ofs;
+			if (ofs <= 0 || ofs >= entry->in_pack_offset)
 				die("delta base offset out of bound for %s",
 				    sha1_to_hex(entry->idx.sha1));
-			ofs = entry->in_pack_offset - ofs;
 			if (reuse_delta && !entry->preferred_base) {
 				struct revindex_entry *revidx;
 				revidx = find_pack_revindex(p, ofs);
diff --git a/builtin-unpack-objects.c b/builtin-unpack-objects.c
index 9f4bdd3..47ed610 100644
--- a/builtin-unpack-objects.c
+++ b/builtin-unpack-objects.c
@@ -370,6 +370,8 @@ static void unpack_delta_entry(enum object_type type, unsigned long delta_size,
 			base_offset = (base_offset << 7) + (c & 127);
 		}
 		base_offset = obj_list[nr].offset - base_offset;
+		if (base_offset <= 0 || base_offset >= obj_list[nr].offset)
+			die("offset value out of bound for delta base object");
 
 		delta_data = get_data(delta_size);
 		if (dry_run || !delta_data) {
diff --git a/index-pack.c b/index-pack.c
index fe75332..60ed41a 100644
--- a/index-pack.c
+++ b/index-pack.c
@@ -338,7 +338,7 @@ static void *unpack_raw_entry(struct object_entry *obj, union delta_base *delta_
 			base_offset = (base_offset << 7) + (c & 127);
 		}
 		delta_base->offset = obj->idx.offset - base_offset;
-		if (delta_base->offset >= obj->idx.offset)
+		if (delta_base->offset <= 0 || delta_base->offset >= obj->idx.offset)
 			bad_object(obj->idx.offset, "delta base offset is out of bound");
 		break;
 	case OBJ_COMMIT:
diff --git a/sha1_file.c b/sha1_file.c
index 88d9cf3..e57949b 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1355,7 +1355,7 @@ static off_t get_delta_base(struct packed_git *p,
 			base_offset = (base_offset << 7) + (c & 127);
 		}
 		base_offset = delta_obj_offset - base_offset;
-		if (base_offset >= delta_obj_offset)
+		if (base_offset <= 0 || base_offset >= delta_obj_offset)
 			return 0;  /* out of bound */
 		*curpos += used;
 	} else if (type == OBJ_REF_DELTA) {
-- 
1.6.0.3.757.g01be.dirty

^ permalink raw reply related

* [PATCH 3/9] make unpack_object_header() non fatal
From: Nicolas Pitre @ 2008-10-29 23:02 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <1225321372-6570-3-git-send-email-nico@cam.org>

It is possible to have pack corruption in the object header.  Currently
unpack_object_header() simply die() on them instead of letting the caller
deal with that gracefully.

So let's have unpack_object_header() return an error instead, and find
a better name for unpack_object_header_gently() in that context.  All
callers of unpack_object_header() are ready for it.

Signed-off-by: Nicolas Pitre <nico@cam.org>
---
 builtin-pack-objects.c |    2 +-
 cache.h                |    2 +-
 sha1_file.c            |   20 +++++++++++---------
 3 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index d4c721b..9e249c9 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -1002,7 +1002,7 @@ static void check_object(struct object_entry *entry)
 		 * We want in_pack_type even if we do not reuse delta
 		 * since non-delta representations could still be reused.
 		 */
-		used = unpack_object_header_gently(buf, avail,
+		used = unpack_object_header_buffer(buf, avail,
 						   &entry->in_pack_type,
 						   &entry->size);
 
diff --git a/cache.h b/cache.h
index a3c77f0..1a9edf3 100644
--- a/cache.h
+++ b/cache.h
@@ -751,7 +751,7 @@ extern const unsigned char *nth_packed_object_sha1(struct packed_git *, uint32_t
 extern off_t nth_packed_object_offset(const struct packed_git *, uint32_t);
 extern off_t find_pack_entry_one(const unsigned char *, struct packed_git *);
 extern void *unpack_entry(struct packed_git *, off_t, enum object_type *, unsigned long *);
-extern unsigned long unpack_object_header_gently(const unsigned char *buf, unsigned long len, enum object_type *type, unsigned long *sizep);
+extern unsigned long unpack_object_header_buffer(const unsigned char *buf, unsigned long len, enum object_type *type, unsigned long *sizep);
 extern unsigned long get_size_from_delta(struct packed_git *, struct pack_window **, off_t);
 extern const char *packed_object_info_detail(struct packed_git *, off_t, unsigned long *, unsigned long *, unsigned int *, unsigned char *);
 extern int matches_pack_name(struct packed_git *p, const char *name);
diff --git a/sha1_file.c b/sha1_file.c
index e57949b..7698177 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1110,7 +1110,8 @@ static int legacy_loose_object(unsigned char *map)
 		return 0;
 }
 
-unsigned long unpack_object_header_gently(const unsigned char *buf, unsigned long len, enum object_type *type, unsigned long *sizep)
+unsigned long unpack_object_header_buffer(const unsigned char *buf,
+		unsigned long len, enum object_type *type, unsigned long *sizep)
 {
 	unsigned shift;
 	unsigned char c;
@@ -1122,10 +1123,10 @@ unsigned long unpack_object_header_gently(const unsigned char *buf, unsigned lon
 	size = c & 15;
 	shift = 4;
 	while (c & 0x80) {
-		if (len <= used)
-			return 0;
-		if (sizeof(long) * 8 <= shift)
+		if (len <= used || sizeof(long) * 8 <= shift) {
+			error("bad object header");
 			return 0;
+		}
 		c = buf[used++];
 		size += (c & 0x7f) << shift;
 		shift += 7;
@@ -1164,7 +1165,7 @@ static int unpack_sha1_header(z_stream *stream, unsigned char *map, unsigned lon
 	 * really worth it and we don't write it any longer.  But we
 	 * can still read it.
 	 */
-	used = unpack_object_header_gently(map, mapsize, &type, &size);
+	used = unpack_object_header_buffer(map, mapsize, &type, &size);
 	if (!used || !valid_loose_object_type[type])
 		return -1;
 	map += used;
@@ -1411,10 +1412,11 @@ static int unpack_object_header(struct packed_git *p,
 	 * insane, so we know won't exceed what we have been given.
 	 */
 	base = use_pack(p, w_curs, *curpos, &left);
-	used = unpack_object_header_gently(base, left, &type, sizep);
-	if (!used)
-		die("object offset outside of pack file");
-	*curpos += used;
+	used = unpack_object_header_buffer(base, left, &type, sizep);
+	if (!used) {
+		type = OBJ_BAD;
+	} else
+		*curpos += used;
 
 	return type;
 }
-- 
1.6.0.3.757.g01be.dirty

^ permalink raw reply related

* [PATCH 4/9] make packed_object_info() resilient to pack corruptions
From: Nicolas Pitre @ 2008-10-29 23:02 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <1225321372-6570-4-git-send-email-nico@cam.org>

In the same spirit as commit 8eca0b47ff, let's try to survive a pack
corruption by making packed_object_info() able to fall back to alternate
packs or loose objects.

Signed-off-by: Nicolas Pitre <nico@cam.org>
---
 sha1_file.c |   36 ++++++++++++++++++++++++++++++------
 1 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 7698177..384a430 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1314,8 +1314,10 @@ unsigned long get_size_from_delta(struct packed_git *p,
 	} while ((st == Z_OK || st == Z_BUF_ERROR) &&
 		 stream.total_out < sizeof(delta_head));
 	inflateEnd(&stream);
-	if ((st != Z_STREAM_END) && stream.total_out != sizeof(delta_head))
-		die("delta data unpack-initial failed");
+	if ((st != Z_STREAM_END) && stream.total_out != sizeof(delta_head)) {
+		error("delta data unpack-initial failed");
+		return 0;
+	}
 
 	/* Examine the initial part of the delta to figure out
 	 * the result size.
@@ -1382,15 +1384,29 @@ static int packed_delta_info(struct packed_git *p,
 	off_t base_offset;
 
 	base_offset = get_delta_base(p, w_curs, &curpos, type, obj_offset);
+	if (!base_offset)
+		return OBJ_BAD;
 	type = packed_object_info(p, base_offset, NULL);
+	if (type <= OBJ_NONE) {
+		struct revindex_entry *revidx = find_pack_revindex(p, base_offset);
+		const unsigned char *base_sha1 =
+					nth_packed_object_sha1(p, revidx->nr);
+		mark_bad_packed_object(p, base_sha1);
+		type = sha1_object_info(base_sha1, NULL);
+		if (type <= OBJ_NONE)
+			return OBJ_BAD;
+	}
 
 	/* We choose to only get the type of the base object and
 	 * ignore potentially corrupt pack file that expects the delta
 	 * based on a base with a wrong size.  This saves tons of
 	 * inflate() calls.
 	 */
-	if (sizep)
+	if (sizep) {
 		*sizep = get_size_from_delta(p, w_curs, curpos);
+		if (*sizep == 0)
+			type = OBJ_BAD;
+	}
 
 	return type;
 }
@@ -1500,8 +1516,9 @@ static int packed_object_info(struct packed_git *p, off_t obj_offset,
 			*sizep = size;
 		break;
 	default:
-		die("pack %s contains unknown object type %d",
-		    p->pack_name, type);
+		error("unknown object type %i at offset %"PRIuMAX" in %s",
+		      type, (uintmax_t)obj_offset, p->pack_name);
+		type = OBJ_BAD;
 	}
 	unuse_pack(&w_curs);
 	return type;
@@ -1971,7 +1988,14 @@ int sha1_object_info(const unsigned char *sha1, unsigned long *sizep)
 		if (!find_pack_entry(sha1, &e, NULL))
 			return status;
 	}
-	return packed_object_info(e.p, e.offset, sizep);
+
+	status = packed_object_info(e.p, e.offset, sizep);
+	if (status < 0) {
+		mark_bad_packed_object(e.p, sha1);
+		status = sha1_object_info(sha1, sizep);
+	}
+
+	return status;
 }
 
 static void *read_packed_sha1(const unsigned char *sha1,
-- 
1.6.0.3.757.g01be.dirty

^ permalink raw reply related

* [PATCH 5/9] make check_object() resilient to pack corruptions
From: Nicolas Pitre @ 2008-10-29 23:02 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <1225321372-6570-5-git-send-email-nico@cam.org>

The check_object() function tries to get away with the least amount of
pack access possible when it already has partial information on given
object rather than calling the more costly packed_object_info().

When things don't look right, it should just give up and fall back to
packed_object_info() directly instead of die()'ing.

Signed-off-by: Nicolas Pitre <nico@cam.org>
---
 builtin-pack-objects.c |   23 +++++++++++++++++------
 1 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 9e249c9..b595d04 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -1005,6 +1005,8 @@ static void check_object(struct object_entry *entry)
 		used = unpack_object_header_buffer(buf, avail,
 						   &entry->in_pack_type,
 						   &entry->size);
+		if (used == 0)
+			goto give_up;
 
 		/*
 		 * Determine if this is a delta and if so whether we can
@@ -1016,6 +1018,8 @@ static void check_object(struct object_entry *entry)
 			/* Not a delta hence we've already got all we need. */
 			entry->type = entry->in_pack_type;
 			entry->in_pack_header_size = used;
+			if (entry->type < OBJ_COMMIT || entry->type > OBJ_BLOB)
+				goto give_up;
 			unuse_pack(&w_curs);
 			return;
 		case OBJ_REF_DELTA:
@@ -1032,16 +1036,20 @@ static void check_object(struct object_entry *entry)
 			ofs = c & 127;
 			while (c & 128) {
 				ofs += 1;
-				if (!ofs || MSB(ofs, 7))
-					die("delta base offset overflow in pack for %s",
-					    sha1_to_hex(entry->idx.sha1));
+				if (!ofs || MSB(ofs, 7)) {
+					error("delta base offset overflow in pack for %s",
+					      sha1_to_hex(entry->idx.sha1));
+					goto give_up;
+				}
 				c = buf[used_0++];
 				ofs = (ofs << 7) + (c & 127);
 			}
 			ofs = entry->in_pack_offset - ofs;
-			if (ofs <= 0 || ofs >= entry->in_pack_offset)
-				die("delta base offset out of bound for %s",
-				    sha1_to_hex(entry->idx.sha1));
+			if (ofs <= 0 || ofs >= entry->in_pack_offset) {
+				error("delta base offset out of bound for %s",
+				      sha1_to_hex(entry->idx.sha1));
+				goto give_up;
+			}
 			if (reuse_delta && !entry->preferred_base) {
 				struct revindex_entry *revidx;
 				revidx = find_pack_revindex(p, ofs);
@@ -1078,6 +1086,8 @@ static void check_object(struct object_entry *entry)
 			 */
 			entry->size = get_size_from_delta(p, &w_curs,
 					entry->in_pack_offset + entry->in_pack_header_size);
+			if (entry->size == 0)
+				goto give_up;
 			unuse_pack(&w_curs);
 			return;
 		}
@@ -1087,6 +1097,7 @@ static void check_object(struct object_entry *entry)
 		 * with sha1_object_info() to find about the object type
 		 * at this point...
 		 */
+		give_up:
 		unuse_pack(&w_curs);
 	}
 
-- 
1.6.0.3.757.g01be.dirty

^ permalink raw reply related

* [PATCH 6/9] make find_pack_revindex() aware of the nasty world
From: Nicolas Pitre @ 2008-10-29 23:02 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <1225321372-6570-6-git-send-email-nico@cam.org>

It currently calls die() whenever given offset is not found thinking
that such thing should never happen.  But this offset may come from a
corrupted pack whych _could_ happen and not be found.  Callers should
deal with this possibility gracefully instead.

Signed-off-by: Nicolas Pitre <nico@cam.org>
---
 builtin-pack-objects.c |    2 ++
 pack-revindex.c        |    3 ++-
 sha1_file.c            |   18 ++++++++++++------
 3 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index b595d04..963b432 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -1053,6 +1053,8 @@ static void check_object(struct object_entry *entry)
 			if (reuse_delta && !entry->preferred_base) {
 				struct revindex_entry *revidx;
 				revidx = find_pack_revindex(p, ofs);
+				if (!revidx)
+					goto give_up;
 				base_ref = nth_packed_object_sha1(p, revidx->nr);
 			}
 			entry->in_pack_header_size = used + used_0;
diff --git a/pack-revindex.c b/pack-revindex.c
index 6096b62..1de53c8 100644
--- a/pack-revindex.c
+++ b/pack-revindex.c
@@ -140,7 +140,8 @@ struct revindex_entry *find_pack_revindex(struct packed_git *p, off_t ofs)
 		else
 			lo = mi + 1;
 	} while (lo < hi);
-	die("internal error: pack revindex corrupt");
+	error("bad offset for revindex");
+	return NULL;
 }
 
 void discard_revindex(void)
diff --git a/sha1_file.c b/sha1_file.c
index 384a430..9ce1df0 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1388,9 +1388,12 @@ static int packed_delta_info(struct packed_git *p,
 		return OBJ_BAD;
 	type = packed_object_info(p, base_offset, NULL);
 	if (type <= OBJ_NONE) {
-		struct revindex_entry *revidx = find_pack_revindex(p, base_offset);
-		const unsigned char *base_sha1 =
-					nth_packed_object_sha1(p, revidx->nr);
+		struct revindex_entry *revidx;
+		const unsigned char *base_sha1;
+		revidx = find_pack_revindex(p, base_offset);
+		if (!revidx)
+			return OBJ_BAD;
+		base_sha1 = nth_packed_object_sha1(p, revidx->nr);
 		mark_bad_packed_object(p, base_sha1);
 		type = sha1_object_info(base_sha1, NULL);
 		if (type <= OBJ_NONE)
@@ -1682,9 +1685,12 @@ static void *unpack_delta_entry(struct packed_git *p,
 		 * This is costly but should happen only in the presence
 		 * of a corrupted pack, and is better than failing outright.
 		 */
-		struct revindex_entry *revidx = find_pack_revindex(p, base_offset);
-		const unsigned char *base_sha1 =
-					nth_packed_object_sha1(p, revidx->nr);
+		struct revindex_entry *revidx;
+		const unsigned char *base_sha1;
+		revidx = find_pack_revindex(p, base_offset);
+		if (!revidx)
+			return NULL;
+		base_sha1 = nth_packed_object_sha1(p, revidx->nr);
 		error("failed to read delta base object %s"
 		      " at offset %"PRIuMAX" from %s",
 		      sha1_to_hex(base_sha1), (uintmax_t)base_offset,
-- 
1.6.0.3.757.g01be.dirty

^ permalink raw reply related


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