Git development
 help / color / mirror / Atom feed
* [PATCH] Teach merge.log to "git-merge" again
From: Junio C Hamano @ 2008-07-10  7:50 UTC (permalink / raw)
  To: Miklos Vajna; +Cc: git

The command forgot the configuration variable when rewritten in C.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-merge.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/builtin-merge.c b/builtin-merge.c
index 821edcd..bc3f921 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -519,8 +519,10 @@ int git_merge_config(const char *k, const char *v, void *cb)
 	else if (!strcmp(k, "pull.twohead"))
 		return git_config_string(&pull_twohead, k, v);
 	else if (!strcmp(k, "pull.octopus"))
 		return git_config_string(&pull_octopus, k, v);
+	else if (!strcmp(k, "merge.log") || !strcmp(k, "merge.summary"))
+		option_log = git_config_bool(k, v);
 	return git_diff_ui_config(k, v, cb);
 }
 
 static int read_tree_trivial(unsigned char *common, unsigned char *head,

^ permalink raw reply related

* Re: Bug: /usr/local/bin/git-daemon starts /usr/sbin/git-upload-pack?
From: Jan Wielemaker @ 2008-07-10  7:38 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git Mailing List
In-Reply-To: <7vlk0a7obl.fsf@gitster.siamese.dyndns.org>

Hi Junio,

On Wednesday 09 July 2008 23:14:06 Junio C Hamano wrote:
> Jan Wielemaker <J.Wielemaker@uva.nl> writes:
> > After upgrade to SuSE 11.0 I was forced to update GIT (changed libcurl).
> > I did (with some trickery) a pull of the latest git and built it using
> > make prefix=/usr/local ..., to find out that /usr/local/bin/git-daemon
> > starts /usr/sbin/git-upload-pack.
> >
> > After creating a symlink to /usr/local/bin/git-upload-pack all works fine
> > again, but I guess this is a mistake?
>
> I think this is a distro issue.
>
> Pre-1.6.0 git installs git-everything in $(bindir) and knows things are
> there.  Current 'master' we are preparing for 1.6.0 instead installs
> git-mostofthem in $(prefix)/libexec/git-core and git-someofthem in
> $(bindir); our executables know which binaries are installed in $(bindir)
> and which ones are installed in $(prefix)/libexec/git-core.
>
> The point is we never install $anywhere/sbin unless the person who is
> building explicitly does so (either by futzing the Makefile or giving
> bindir=$somewhere from the command line of "make").  The reason your
> /usr/local/bin/git-daemon (we do not know who compiled it and how) spawns
> something in /usr/sbin is not our doing.  Find out who configured it and
> why, and more importantly, what _other_ changes are made in the build and
> installation procedure to support that change in location.

If you read carefully you see some "I ..." :-)  Summarising to avoid all
confusion:

	* I had git installed in /usr/local/bin, this was git from git,
	close to 1.5.6.  The machine was running SuSE 10.2.
	* I installed SuSE 11.0, whiping /, but git remained as 
	/usr/local is on a seperate partition.
	* Git refused to run (dependency on libcurl.so.3; 11.0 ships with
	libcurl.so.4).
	* I copied libcurl.so.3 from backup into /usr/lib64, which at least
	made git run again.
	* Then I did 

		% git pull
		% git clean -x -f
		% make prefix=/usr/local all doc info 
		# make prefix=/usr/local install install-doc install-info
		
	* All went smooth, but clients to git:// complained that the other site
	hung up unexpectedly.
	* Ran git-daemon --verbose --export-all --user=git --port=4000 /some/path
	* Ran strace -f -p <pid>
	* Ran git clone git://localhost:4000/some/path to find it was trying to
	start /usr/sbin/git-upload-pack.

I understand bit changes are underway, so it might not be worthwhile to try 
and
get to the bottom of this.

	Cheers --- Jan

^ permalink raw reply

* Re: [PATCH] revisions: refactor handle_revision_opt into  parse_revision_opt.
From: Pierre Habouzit @ 2008-07-10  7:40 UTC (permalink / raw)
  To: Jeff King, git, gitster
In-Reply-To: <20080710073450.GA24819@artemis.madism.org>

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

On Thu, Jul 10, 2008 at 07:34:50AM +0000, Pierre Habouzit wrote:
> and have a full parse-opt thing from top to bottom. the "--reverse" hack
> could be done really differently, because we really know what
> "--children" does and we could directly do what the revision option
> parser does.

  I mean in a clean way, given what --children does, it would probably
implemented by a callback in revision.c, so it's easy for git-blame to
call it too.


-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

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

^ permalink raw reply

* Re: [PATCH] revisions: refactor handle_revision_opt into parse_revision_opt.
From: Junio C Hamano @ 2008-07-10  7:36 UTC (permalink / raw)
  To: Jeff King; +Cc: Pierre Habouzit, git, gitster
In-Reply-To: <20080710071418.GD3195@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

> but it also seems like the top bit of that for loop is boilerplate, too:
>
>>  	for (;;) {
>>  		switch (parse_options_step(&ctx, options, shortlog_usage)) {
>>  		case PARSE_OPT_HELP:
>>  			exit(129);
>>  		case PARSE_OPT_DONE:
>>  			goto parse_done;
>>  		}

Another thing we should be able to refactor is the option help formatter.
Cf. 071438a (Teach git-merge -X<option> again., 2008-07-09)

^ permalink raw reply

* Re: [PATCH] revisions: refactor handle_revision_opt into  parse_revision_opt.
From: Pierre Habouzit @ 2008-07-10  7:34 UTC (permalink / raw)
  To: Jeff King; +Cc: git, gitster
In-Reply-To: <20080710071418.GD3195@sigill.intra.peff.net>

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

On Thu, Jul 10, 2008 at 07:14:18AM +0000, Jeff King wrote:
> On Wed, Jul 09, 2008 at 11:38:34PM +0200, Pierre Habouzit wrote:
> 
> > It seems we're using handle_revision_opt the same way each time, have a
> > wrapper around it that does the 9-liner we copy each time instead.
> > 
> > handle_revision_opt can be static in the module for now, it's always
> > possible to make it public again if needed.
> 
> I have been on the road, and I finally got the chance to read through
> your whole parseopt/blame refactoring. I think it looks good overall, as
> do these patches.
> 
> I have one comment, though I am not sure it is worth implementing.
> 
> I was happy to see this refactoring, which I think improves readability:
> 
> > -		n = handle_revision_opt(&revs, ctx.argc, ctx.argv,
> > -					&ctx.cpidx, ctx.out);
> > -		if (n <= 0) {
> > -			error("unknown option `%s'", ctx.argv[0]);
> > -			usage_with_options(blame_opt_usage, options);
> > -		}
> > -		ctx.argv += n;
> > -		ctx.argc -= n;
> > +		parse_revision_opt(&revs, &ctx, options, blame_opt_usage);
> 
> but it also seems like the top bit of that for loop is boilerplate, too:
> 
> >  	for (;;) {
> >  		switch (parse_options_step(&ctx, options, shortlog_usage)) {
> >  		case PARSE_OPT_HELP:
> >  			exit(129);
> >  		case PARSE_OPT_DONE:
> >  			goto parse_done;
> >  		}
> 
> AFAICT, the main reason for not folding this into your refactored
> function is that after the parse_options_step, but before we handle the
> revision arg to parse_revision_opt, there needs to be an opportunity for
> the caller to intercept and do something based on revision opts (like
> blame does with --reverse):
> 
> 	if (!strcmp(ctx.argv[0], "--reverse")) {
> 		ctx.argv[0] = "--children";
> 		reverse = 1;
> 	}

  The other thing I would like to do is remove the exit(129) and replace
it with proper documentation for the rev-list options, and this will
depend upon the fact that we parse revisions or something else inside
the loop. Of course this is boilerplate, but well, I wouldn't like to
hide it and prevent people from thinking they can hook other things in
there.

  And I'm not very keen on adding more options to parse-options like you
propose, our endgame is to get rid of parse_revision_opt in this form
and have a full parse-opt thing from top to bottom. the "--reverse" hack
could be done really differently, because we really know what
"--children" does and we could directly do what the revision option
parser does. But oh well... For now I'm more interested to see more
commands migrated thanks to this facility, and see what we can refactor
to get rid of the old parsers at once.

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

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

^ permalink raw reply

* Re: [PATCH] Fix problem with authentification on http repository.
From: Junio C Hamano @ 2008-07-10  7:32 UTC (permalink / raw)
  To: Jeff King
  Cc: Johannes Schindelin, Mike Hommey, jean.guyader, git, Jean Guyader
In-Reply-To: <20080710072420.GA5167@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

> On Wed, Jul 09, 2008 at 01:43:07PM +0200, Johannes Schindelin wrote:
>
>> > Note that http://user:pass@server/path/ /should/ work (but that would 
>> > need validation), though not a good idea on command line.
>> 
>> Well, now that the programs using URLs are all builtins, we can actually 
>> do something about it.  We can edit out the "user[:pass]@" part out of 
>> argv, which usually means that "ps" will not see it anymore.
>
> Wouldn't there still be a race condition for publicly broadcasting your
> password via ps?

In addition, I think on some Unices overwriting argv[0] does not have the
effect Dscho is aiming for.

I thought we do discourage [:pass] part in our documentation.

^ permalink raw reply

* Re: Bug: /usr/local/bin/git-daemon starts /usr/sbin/git-upload-pack?
From: Brian Foster @ 2008-07-10  7:32 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Jan Wielemaker
In-Reply-To: <200807092220.10655.J.Wielemaker@uva.nl>

On Wednesday 09 July 2008 22:20:10 Jan Wielemaker wrote:
>[ ... ]
> P.s.	This [ git-daemon using a weird path to exec git-upload-pack
>       was ] quite nasty to debug.  I was forced to to run git-daemon
> 	stand-alone (not xinetd) another port and run strace -f -p <pid>
> 	to discover the	cause of this problem.  Even with --verbose, the
> 	only error response was the client complaining on unexpected EOF.

 Just a hint:  I had some odd exec problems in the past
 (as it happens, also from xinetd and also path-related,
 but not(?) your problem), and found that exporting

    GIT_TRACE=/tmp/LOG-git-daemon

 gave some valuable clews.  I'm unaware of GIT_TRACE
 being documented.  I also concur it's annoying neither
 --syslog nor --verbose seems to cause these sort of
 errors to be logged.

cheers!
	-blf-

-- 
“How many surrealists does it take to   | Brian Foster
 change a lightbulb? Three. One calms   | somewhere in south of France
 the warthog, and two fill the bathtub  |   Stop E$$o (ExxonMobil)!
 with brightly-coloured machine tools.” |      http://www.stopesso.com

^ permalink raw reply

* Re: [PATCH] Fix problem with authentification on http repository.
From: Jeff King @ 2008-07-10  7:24 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: Mike Hommey, Junio C Hamano, jean.guyader, git, Jean Guyader
In-Reply-To: <alpine.DEB.1.00.0807091341230.5277@eeepc-johanness>

On Wed, Jul 09, 2008 at 01:43:07PM +0200, Johannes Schindelin wrote:

> > Note that http://user:pass@server/path/ /should/ work (but that would 
> > need validation), though not a good idea on command line.
> 
> Well, now that the programs using URLs are all builtins, we can actually 
> do something about it.  We can edit out the "user[:pass]@" part out of 
> argv, which usually means that "ps" will not see it anymore.

Wouldn't there still be a race condition for publicly broadcasting your
password via ps?

-Peff

^ permalink raw reply

* Re: [PATCH] revisions: refactor handle_revision_opt into parse_revision_opt.
From: Jeff King @ 2008-07-10  7:14 UTC (permalink / raw)
  To: Pierre Habouzit; +Cc: git, gitster
In-Reply-To: <1215639514-1612-2-git-send-email-madcoder@debian.org>

On Wed, Jul 09, 2008 at 11:38:34PM +0200, Pierre Habouzit wrote:

> It seems we're using handle_revision_opt the same way each time, have a
> wrapper around it that does the 9-liner we copy each time instead.
> 
> handle_revision_opt can be static in the module for now, it's always
> possible to make it public again if needed.

I have been on the road, and I finally got the chance to read through
your whole parseopt/blame refactoring. I think it looks good overall, as
do these patches.

I have one comment, though I am not sure it is worth implementing.

I was happy to see this refactoring, which I think improves readability:

> -		n = handle_revision_opt(&revs, ctx.argc, ctx.argv,
> -					&ctx.cpidx, ctx.out);
> -		if (n <= 0) {
> -			error("unknown option `%s'", ctx.argv[0]);
> -			usage_with_options(blame_opt_usage, options);
> -		}
> -		ctx.argv += n;
> -		ctx.argc -= n;
> +		parse_revision_opt(&revs, &ctx, options, blame_opt_usage);

but it also seems like the top bit of that for loop is boilerplate, too:

>  	for (;;) {
>  		switch (parse_options_step(&ctx, options, shortlog_usage)) {
>  		case PARSE_OPT_HELP:
>  			exit(129);
>  		case PARSE_OPT_DONE:
>  			goto parse_done;
>  		}

AFAICT, the main reason for not folding this into your refactored
function is that after the parse_options_step, but before we handle the
revision arg to parse_revision_opt, there needs to be an opportunity for
the caller to intercept and do something based on revision opts (like
blame does with --reverse):

	if (!strcmp(ctx.argv[0], "--reverse")) {
		ctx.argv[0] = "--children";
		reverse = 1;
	}

But I wonder if it would be a suitable alternative to just add
"--reverse" in this case to the blame options, but with an option flag
for "parse me, but also pass me along to the next parser" (which would
be added). Then we could do our thing in a callback.

Of course, in this case, we do something a bit tricky by actually
_rewriting_ the argument to "--children". So we would have to have
support for callbacks rewriting arguments, or it would have to manually
do what "--children" should do. So perhaps it isn't worth the trouble.
This particular boilerplate is at least not very error-prone.

So food for thought, mainly, I suppose. Apologies if you already thought
of this and I missed the discussion. I think I am up to date on my
back-reading of the git list, but it is easy to lose some threads. :)

-Peff

^ permalink raw reply

* Re: [PATCH 2/4] git-imap-send: Add support for SSL.
From: Jeff King @ 2008-07-10  6:39 UTC (permalink / raw)
  To: Rob Shearman; +Cc: Josh Triplett, git
In-Reply-To: <1096648c0807091424g1e10d0ccrae0be929ec428b89@mail.gmail.com>

On Wed, Jul 09, 2008 at 10:24:27PM +0100, Rob Shearman wrote:

> >>  [imap]
> >> +    sslverify = false
> > [...]
> >> +     else if (!strcmp( "ssl_verify", key ))
> >
> > The example and the code disagree on the name of the
> > sslverify/ssl_verify option.
> 
> I wouldn't exactly call it "disagree". The config variable is limited
> by not allowing underscores, whereas the C language does allow them.

I think his point is that the example says "sslverify" but the code is
looking for the config variable "ssl_verify". So that config won't work.

However as you mention, underscore isn't allowed, so your strcmp line is
totally bogus anyway, but you silently fix it in your 3/4 "style" fix.

-Peff

^ permalink raw reply

* [JGIT PATCH 5/5] Explicitly capture the stderr from a failed SSH fetch or push
From: Shawn O. Pearce @ 2008-07-10  6:13 UTC (permalink / raw)
  To: Robin Rosenberg, Marek Zawirski; +Cc: git
In-Reply-To: <1215670403-19191-5-git-send-email-spearce@spearce.org>

If the remote command name is not found on the remote system we are
likely to get a shell error sent to the channel's stderr stream,
and yet the channel is connected.  So we don't see the problem
until we try to read the advertised refs, which is somewhat late.

If we get EOF before the first advertised ref it is a very good
indication that the remote side did not start the process we wanted
it to.  Tossing a specialized exception allows the SSH transport
to offer up the contents of the stderr channel (if any) as possible
indication of why the repository does not exist.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../spearce/egit/ui/EclipseSshSessionFactory.java  |    5 +-
 .../jgit/errors/NoRemoteRepositoryException.java   |   59 ++++++++++++++++++++
 .../spearce/jgit/transport/BasePackConnection.java |    3 +-
 .../jgit/transport/DefaultSshSessionFactory.java   |   30 ++++++++++-
 .../spearce/jgit/transport/TransportGitSsh.java    |   34 +++++++++++-
 5 files changed, 126 insertions(+), 5 deletions(-)
 create mode 100644 org.spearce.jgit/src/org/spearce/jgit/errors/NoRemoteRepositoryException.java

diff --git a/org.spearce.egit.ui/src/org/spearce/egit/ui/EclipseSshSessionFactory.java b/org.spearce.egit.ui/src/org/spearce/egit/ui/EclipseSshSessionFactory.java
index 144d47d..8f80373 100644
--- a/org.spearce.egit.ui/src/org/spearce/egit/ui/EclipseSshSessionFactory.java
+++ b/org.spearce.egit.ui/src/org/spearce/egit/ui/EclipseSshSessionFactory.java
@@ -55,7 +55,10 @@ class EclipseSshSessionFactory extends SshSessionFactory {
 			StringBuilder sb = new StringBuilder();
 
 			public String toString() {
-				return all.toString();
+				String r = all.toString();
+				while (r.endsWith("\n"))
+					r = r.substring(0, r.length() - 1);
+				return r;
 			}
 
 			@Override
diff --git a/org.spearce.jgit/src/org/spearce/jgit/errors/NoRemoteRepositoryException.java b/org.spearce.jgit/src/org/spearce/jgit/errors/NoRemoteRepositoryException.java
new file mode 100644
index 0000000..604ec4d
--- /dev/null
+++ b/org.spearce.jgit/src/org/spearce/jgit/errors/NoRemoteRepositoryException.java
@@ -0,0 +1,59 @@
+/*
+ * Copyright (C) 2008, Shawn O. Pearce <spearce@spearce.org>
+ *
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * - Redistributions of source code must retain the above copyright
+ *   notice, this list of conditions and the following disclaimer.
+ *
+ * - Redistributions in binary form must reproduce the above
+ *   copyright notice, this list of conditions and the following
+ *   disclaimer in the documentation and/or other materials provided
+ *   with the distribution.
+ *
+ * - Neither the name of the Git Development Community nor the
+ *   names of its contributors may be used to endorse or promote
+ *   products derived from this software without specific prior
+ *   written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
+ * CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES,
+ * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
+ * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
+ * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+ * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
+ * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
+ * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
+ * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+package org.spearce.jgit.errors;
+
+import org.spearce.jgit.transport.URIish;
+
+/**
+ * Indicates a remote repository does not exist.
+ */
+public class NoRemoteRepositoryException extends TransportException {
+	private static final long serialVersionUID = 1L;
+
+	/**
+	 * Constructs an exception indicating a repository does not exist.
+	 *
+	 * @param uri
+	 *            URI used for transport
+	 * @param s
+	 *            message
+	 */
+	public NoRemoteRepositoryException(final URIish uri, final String s) {
+		super(uri, s);
+	}
+}
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackConnection.java b/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackConnection.java
index 7dc4620..52f3f48 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackConnection.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackConnection.java
@@ -49,6 +49,7 @@ import java.util.HashSet;
 import java.util.LinkedHashMap;
 import java.util.Set;
 
+import org.spearce.jgit.errors.NoRemoteRepositoryException;
 import org.spearce.jgit.errors.PackProtocolException;
 import org.spearce.jgit.errors.TransportException;
 import org.spearce.jgit.lib.ObjectId;
@@ -129,7 +130,7 @@ abstract class BasePackConnection extends BaseConnection {
 				line = pckIn.readString();
 			} catch (EOFException eof) {
 				if (avail.isEmpty())
-					throw new TransportException(uri, "not found.");
+					throw new NoRemoteRepositoryException(uri, "not found.");
 				throw eof;
 			}
 
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java b/org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java
index 5924a04..b4578d4 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java
@@ -253,6 +253,34 @@ class DefaultSshSessionFactory extends SshSessionFactory {
 
 	@Override
 	public OutputStream getErrorStream() {
-		return System.err;
+		return new OutputStream() {
+			private StringBuilder all = new StringBuilder();
+
+			private StringBuilder sb = new StringBuilder();
+
+			public String toString() {
+				String r = all.toString();
+				while (r.endsWith("\n"))
+					r = r.substring(0, r.length() - 1);
+				return r;
+			}
+
+			@Override
+			public void write(final int b) throws IOException {
+				if (b == '\r') {
+					System.err.print('\r');
+					return;
+				}
+
+				sb.append((char) b);
+
+				if (b == '\n') {
+					final String line = sb.toString();
+					System.err.print(line);
+					all.append(line);
+					sb = new StringBuilder();
+				}
+			}
+		};
 	}
 }
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/TransportGitSsh.java b/org.spearce.jgit/src/org/spearce/jgit/transport/TransportGitSsh.java
index 9a6c719..3f2cd37 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/TransportGitSsh.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/TransportGitSsh.java
@@ -44,6 +44,7 @@ import java.io.OutputStream;
 import java.net.ConnectException;
 import java.net.UnknownHostException;
 
+import org.spearce.jgit.errors.NoRemoteRepositoryException;
 import org.spearce.jgit.errors.TransportException;
 import org.spearce.jgit.lib.Repository;
 
@@ -217,6 +218,25 @@ class TransportGitSsh extends PackTransport {
 		}
 	}
 
+	NoRemoteRepositoryException cleanNotFound(NoRemoteRepositoryException nf) {
+		String why = errStream.toString();
+		if (why == null || why.length() == 0)
+			return nf;
+
+		String path = uri.getPath();
+		if (uri.getScheme() != null && uri.getPath().startsWith("/~"))
+			path = uri.getPath().substring(1);
+
+		final StringBuilder pfx = new StringBuilder();
+		pfx.append("fatal: ");
+		sqAlways(pfx, path);
+		pfx.append(": ");
+		if (why.startsWith(pfx.toString()))
+			why = why.substring(pfx.length());
+
+		return new NoRemoteRepositoryException(uri, why);
+	}
+
 	class SshFetchConnection extends BasePackFetchConnection {
 		private ChannelExec channel;
 
@@ -238,7 +258,12 @@ class TransportGitSsh extends PackTransport {
 				throw new TransportException(uri,
 						"remote hung up unexpectedly", err);
 			}
-			readAdvertisedRefs();
+
+			try {
+				readAdvertisedRefs();
+			} catch (NoRemoteRepositoryException notFound) {
+				throw cleanNotFound(notFound);
+			}
 		}
 
 		@Override
@@ -277,7 +302,12 @@ class TransportGitSsh extends PackTransport {
 				throw new TransportException(uri,
 						"remote hung up unexpectedly", err);
 			}
-			readAdvertisedRefs();
+
+			try {
+				readAdvertisedRefs();
+			} catch (NoRemoteRepositoryException notFound) {
+				throw cleanNotFound(notFound);
+			}
 		}
 
 		@Override
-- 
1.5.6.2.393.g45096

^ permalink raw reply related

* [JGIT PATCH 3/5] Reuse the same SSH connection when automatically fetching tags
From: Shawn O. Pearce @ 2008-07-10  6:13 UTC (permalink / raw)
  To: Robin Rosenberg, Marek Zawirski; +Cc: git
In-Reply-To: <1215670403-19191-3-git-send-email-spearce@spearce.org>

If we have to open a second connection to git-upload-pack in order
to automatically follow and fetch annotated tags we may be able to
reuse the same JSch Session object and simply run the second stream
over the already established, encrypted TCP stream.

Reusing the Session avoids the overheads associated with performing
public/private key authentication and reduces the overall latency
of the fetch process.

To make reuse work we cache the Session at the Transport instance
level and ask Transport API users to close the Transport once they
are completely done talking to this remote repository.  While the
Transport instance is unclosed however multiple FetchConnection
and/or PushConnections can be established, with each subsequent
connection being cheaper to setup.

In the future we may also support reusing the same Session onto
different Transport instances, permitting access to multiple
repositories hosted on the same remote SSH server.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../org/spearce/egit/core/op/CloneOperation.java   |   10 +++-
 .../egit/ui/internal/clone/SourceBranchPage.java   |    1 +
 .../spearce/jgit/transport/PushProcessTest.java    |    5 ++
 .../org/spearce/jgit/transport/TransportTest.java  |   10 +++
 .../src/org/spearce/jgit/pgm/Fetch.java            |   17 ++++--
 .../src/org/spearce/jgit/pgm/LsRemote.java         |    1 +
 .../src/org/spearce/jgit/transport/Transport.java  |   10 +++
 .../spearce/jgit/transport/TransportAmazonS3.java  |    5 ++
 .../spearce/jgit/transport/TransportBundle.java    |    5 ++
 .../spearce/jgit/transport/TransportGitAnon.java   |    5 ++
 .../spearce/jgit/transport/TransportGitSsh.java    |   60 +++++++++-----------
 .../org/spearce/jgit/transport/TransportHttp.java  |    5 ++
 .../org/spearce/jgit/transport/TransportLocal.java |    5 ++
 .../org/spearce/jgit/transport/TransportSftp.java  |   52 +++++++++---------
 14 files changed, 124 insertions(+), 67 deletions(-)

diff --git a/org.spearce.egit.core/src/org/spearce/egit/core/op/CloneOperation.java b/org.spearce.egit.core/src/org/spearce/egit/core/op/CloneOperation.java
index 7600e3b..656f3cb 100644
--- a/org.spearce.egit.core/src/org/spearce/egit/core/op/CloneOperation.java
+++ b/org.spearce.egit.core/src/org/spearce/egit/core/op/CloneOperation.java
@@ -85,8 +85,14 @@ public class CloneOperation implements IRunnableWithProgress {
 
 	private void doFetch(final IProgressMonitor monitor)
 			throws NotSupportedException, TransportException {
-		fetchResult = Transport.open(local, remote).fetch(
-				new EclipseGitProgressTransformer(monitor), null);
+		final Transport tn = Transport.open(local, remote);
+		try {
+			final EclipseGitProgressTransformer pm;
+			pm = new EclipseGitProgressTransformer(monitor);
+			fetchResult = tn.fetch(pm, null);
+		} finally {
+			tn.close();
+		}
 	}
 
 	private void doCheckout(final IProgressMonitor monitor) throws IOException {
diff --git a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/clone/SourceBranchPage.java b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/clone/SourceBranchPage.java
index b704aaa..b0aba1e 100644
--- a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/clone/SourceBranchPage.java
+++ b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/clone/SourceBranchPage.java
@@ -241,6 +241,7 @@ class SourceBranchPage extends WizardPage {
 							adv = fn.getRefs();
 						} finally {
 							fn.close();
+							tn.close();
 						}
 
 						final Ref idHEAD = fn.getRef(Constants.HEAD);
diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/transport/PushProcessTest.java b/org.spearce.jgit.test/tst/org/spearce/jgit/transport/PushProcessTest.java
index fae1cbb..357e6b7 100644
--- a/org.spearce.jgit.test/tst/org/spearce/jgit/transport/PushProcessTest.java
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/transport/PushProcessTest.java
@@ -379,6 +379,11 @@ public class PushProcessTest extends RepositoryTestCase {
 				TransportException {
 			return new MockPushConnection();
 		}
+
+		@Override
+		public void close() {
+			// nothing here
+		}
 	}
 
 	private class MockPushConnection extends BaseConnection implements
diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/transport/TransportTest.java b/org.spearce.jgit.test/tst/org/spearce/jgit/transport/TransportTest.java
index 10f6651..dc1cb21 100644
--- a/org.spearce.jgit.test/tst/org/spearce/jgit/transport/TransportTest.java
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/transport/TransportTest.java
@@ -56,6 +56,16 @@ public class TransportTest extends RepositoryTestCase {
 		final RepositoryConfig config = db.getConfig();
 		remoteConfig = new RemoteConfig(config, "test");
 		remoteConfig.addURI(new URIish("http://everyones.loves.git/u/2"));
+		transport = null;
+	}
+
+	@Override
+	protected void tearDown() throws Exception {
+		if (transport != null) {
+			transport.close();
+			transport = null;
+		}
+		super.tearDown();
 	}
 
 	/**
diff --git a/org.spearce.jgit/src/org/spearce/jgit/pgm/Fetch.java b/org.spearce.jgit/src/org/spearce/jgit/pgm/Fetch.java
index 36a0592..c361c26 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/pgm/Fetch.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/pgm/Fetch.java
@@ -63,12 +63,17 @@ class Fetch extends TextBuiltin {
 			args = new String[] { "origin" };
 
 		final Transport tn = Transport.open(db, args[argi++]);
-		final List<RefSpec> toget = new ArrayList<RefSpec>();
-		for (; argi < args.length; argi++)
-			toget.add(new RefSpec(args[argi]));
-		final FetchResult r = tn.fetch(new TextProgressMonitor(), toget);
-		if (r.getTrackingRefUpdates().isEmpty())
-			return;
+		final FetchResult r;
+		try {
+			final List<RefSpec> toget = new ArrayList<RefSpec>();
+			for (; argi < args.length; argi++)
+				toget.add(new RefSpec(args[argi]));
+			r = tn.fetch(new TextProgressMonitor(), toget);
+			if (r.getTrackingRefUpdates().isEmpty())
+				return;
+		} finally {
+			tn.close();
+		}
 
 		out.print("From ");
 		out.print(tn.getURI().setPass(null));
diff --git a/org.spearce.jgit/src/org/spearce/jgit/pgm/LsRemote.java b/org.spearce.jgit/src/org/spearce/jgit/pgm/LsRemote.java
index dbdfeb3..21e02ec 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/pgm/LsRemote.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/pgm/LsRemote.java
@@ -70,6 +70,7 @@ class LsRemote extends TextBuiltin {
 			}
 		} finally {
 			c.close();
+			tn.close();
 		}
 	}
 
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/Transport.java b/org.spearce.jgit/src/org/spearce/jgit/transport/Transport.java
index b962162..5bec4d2 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/Transport.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/Transport.java
@@ -527,6 +527,16 @@ public abstract class Transport {
 	public abstract PushConnection openPush() throws NotSupportedException,
 			TransportException;
 
+	/**
+	 * Close any resources used by this transport.
+	 * <p>
+	 * If the remote repository is contacted by a network socket this method
+	 * must close that network socket, disconnecting the two peers. If the
+	 * remote repository is actually local (same system) this method must close
+	 * any open file handles used to read the "remote" repository.
+	 */
+	public abstract void close();
+
 	private Collection<RefSpec> expandPushWildcardsFor(
 			final Collection<RefSpec> specs) {
 		final Map<String, Ref> localRefs = local.getAllRefs();
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/TransportAmazonS3.java b/org.spearce.jgit/src/org/spearce/jgit/transport/TransportAmazonS3.java
index cd62c5b..9aa2567 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/TransportAmazonS3.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/TransportAmazonS3.java
@@ -159,6 +159,11 @@ class TransportAmazonS3 extends WalkTransport {
 		return r;
 	}
 
+	@Override
+	public void close() {
+		// No explicit connections are maintained.
+	}
+
 	class DatabaseS3 extends WalkRemoteObjectDatabase {
 		private final String bucketName;
 
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/TransportBundle.java b/org.spearce.jgit/src/org/spearce/jgit/transport/TransportBundle.java
index 6169179..24d49eb 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/TransportBundle.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/TransportBundle.java
@@ -101,6 +101,11 @@ class TransportBundle extends PackTransport {
 				"Push is not supported for bundle transport");
 	}
 
+	@Override
+	public void close() {
+		// Resources must be established per-connection.
+	}
+
 	class BundleFetchConnection extends BaseFetchConnection {
 		FileInputStream in;
 
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/TransportGitAnon.java b/org.spearce.jgit/src/org/spearce/jgit/transport/TransportGitAnon.java
index 8a78099..a80c335 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/TransportGitAnon.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/TransportGitAnon.java
@@ -77,6 +77,11 @@ class TransportGitAnon extends PackTransport {
 		return new TcpPushConnection();
 	}
 
+	@Override
+	public void close() {
+		// Resources must be established per-connection.
+	}
+
 	Socket openConnection() throws TransportException {
 		final int port = uri.getPort() > 0 ? uri.getPort() : GIT_PORT;
 		try {
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/TransportGitSsh.java b/org.spearce.jgit/src/org/spearce/jgit/transport/TransportGitSsh.java
index 1bbdf04..b169f4c 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/TransportGitSsh.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/TransportGitSsh.java
@@ -78,7 +78,10 @@ class TransportGitSsh extends PackTransport {
 		return false;
 	}
 
-	final SshSessionFactory sch;
+	private final SshSessionFactory sch;
+
+	private Session sock;
+
 	OutputStream errStream;
 
 	TransportGitSsh(final Repository local, final URIish uri) {
@@ -96,6 +99,17 @@ class TransportGitSsh extends PackTransport {
 		return new SshPushConnection();
 	}
 
+	@Override
+	public void close() {
+		if (sock != null) {
+			try {
+				sch.releaseSession(sock);
+			} finally {
+				sock = null;
+			}
+		}
+	}
+
 	private static void sqMinimal(final StringBuilder cmd, final String val) {
 		if (val.matches("^[a-zA-Z0-9._/-]*$")) {
 			// If the string matches only generally safe characters
@@ -152,17 +166,18 @@ class TransportGitSsh extends PackTransport {
 		cmd.append('\'');
 	}
 
-	Session openSession() throws TransportException {
+	private void initSession() throws TransportException {
+		if (sock != null)
+			return;
+
 		final String user = uri.getUser();
 		final String pass = uri.getPass();
 		final String host = uri.getHost();
 		final int port = uri.getPort();
 		try {
-			final Session session;
-			session = sch.getSession(user, pass, host, port);
-			if (!session.isConnected())
-				session.connect();
-			return session;
+			sock = sch.getSession(user, pass, host, port);
+			if (!sock.isConnected())
+				sock.connect();
 		} catch (JSchException je) {
 			final Throwable c = je.getCause();
 			if (c instanceof UnknownHostException)
@@ -173,8 +188,9 @@ class TransportGitSsh extends PackTransport {
 		}
 	}
 
-	ChannelExec exec(final Session sock, final String exe)
-			throws TransportException {
+	ChannelExec exec(final String exe) throws TransportException {
+		initSession();
+
 		try {
 			final ChannelExec channel = (ChannelExec) sock.openChannel("exec");
 			String path = uri.getPath();
@@ -202,15 +218,12 @@ class TransportGitSsh extends PackTransport {
 	}
 
 	class SshFetchConnection extends BasePackFetchConnection {
-		private Session session;
-
 		private ChannelExec channel;
 
 		SshFetchConnection() throws TransportException {
 			super(TransportGitSsh.this);
 			try {
-				session = openSession();
-				channel = exec(session, getOptionUploadPack());
+				channel = exec(getOptionUploadPack());
 
 				if (channel.isConnected())
 					init(channel.getInputStream(), channel.getOutputStream());
@@ -240,27 +253,16 @@ class TransportGitSsh extends PackTransport {
 					channel = null;
 				}
 			}
-
-			if (session != null) {
-				try {
-					sch.releaseSession(session);
-				} finally {
-					session = null;
-				}
-			}
 		}
 	}
 
 	class SshPushConnection extends BasePackPushConnection {
-		private Session session;
-
 		private ChannelExec channel;
 
 		SshPushConnection() throws TransportException {
 			super(TransportGitSsh.this);
 			try {
-				session = openSession();
-				channel = exec(session, getOptionReceivePack());
+				channel = exec(getOptionReceivePack());
 				init(channel.getInputStream(), channel.getOutputStream());
 			} catch (TransportException err) {
 				close();
@@ -285,14 +287,6 @@ class TransportGitSsh extends PackTransport {
 					channel = null;
 				}
 			}
-
-			if (session != null) {
-				try {
-					sch.releaseSession(session);
-				} finally {
-					session = null;
-				}
-			}
 		}
 	}
 }
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/TransportHttp.java b/org.spearce.jgit/src/org/spearce/jgit/transport/TransportHttp.java
index 9351a12..1357e58 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/TransportHttp.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/TransportHttp.java
@@ -106,6 +106,11 @@ class TransportHttp extends WalkTransport {
 		return r;
 	}
 
+	@Override
+	public void close() {
+		// No explicit connections are maintained.
+	}
+
 	class HttpObjectDB extends WalkRemoteObjectDatabase {
 		private final URL objectsUrl;
 
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/TransportLocal.java b/org.spearce.jgit/src/org/spearce/jgit/transport/TransportLocal.java
index 155d59f..d74f1b3 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/TransportLocal.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/TransportLocal.java
@@ -93,6 +93,11 @@ class TransportLocal extends PackTransport {
 		return new LocalPushConnection();
 	}
 
+	@Override
+	public void close() {
+		// Resources must be established per-connection.
+	}
+
 	protected Process startProcessWithErrStream(final String cmd)
 			throws TransportException {
 		try {
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/TransportSftp.java b/org.spearce.jgit/src/org/spearce/jgit/transport/TransportSftp.java
index c2cbe6a..6a5df07 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/TransportSftp.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/TransportSftp.java
@@ -91,7 +91,9 @@ class TransportSftp extends WalkTransport {
 		return uri.isRemote() && "sftp".equals(uri.getScheme());
 	}
 
-	final SshSessionFactory sch;
+	private final SshSessionFactory sch;
+
+	private Session sock;
 
 	TransportSftp(final Repository local, final URIish uri) {
 		super(local, uri);
@@ -114,17 +116,29 @@ class TransportSftp extends WalkTransport {
 		return r;
 	}
 
-	Session openSession() throws TransportException {
+	@Override
+	public void close() {
+		if (sock != null) {
+			try {
+				sch.releaseSession(sock);
+			} finally {
+				sock = null;
+			}
+		}
+	}
+
+	private void initSession() throws TransportException {
+		if (sock != null)
+			return;
+
 		final String user = uri.getUser();
 		final String pass = uri.getPass();
 		final String host = uri.getHost();
 		final int port = uri.getPort();
 		try {
-			final Session session;
-			session = sch.getSession(user, pass, host, port);
-			if (!session.isConnected())
-				session.connect();
-			return session;
+			sock = sch.getSession(user, pass, host, port);
+			if (!sock.isConnected())
+				sock.connect();
 		} catch (JSchException je) {
 			final Throwable c = je.getCause();
 			if (c instanceof UnknownHostException)
@@ -135,7 +149,9 @@ class TransportSftp extends WalkTransport {
 		}
 	}
 
-	ChannelSftp open(final Session sock) throws TransportException {
+	ChannelSftp newSftp() throws TransportException {
+		initSession();
+
 		try {
 			final Channel channel = sock.openChannel("sftp");
 			channel.connect();
@@ -148,10 +164,6 @@ class TransportSftp extends WalkTransport {
 	class SftpObjectDB extends WalkRemoteObjectDatabase {
 		private final String objectsPath;
 
-		private final boolean sessionOwner;
-
-		private Session session;
-
 		private ChannelSftp ftp;
 
 		SftpObjectDB(String path) throws TransportException {
@@ -160,9 +172,7 @@ class TransportSftp extends WalkTransport {
 			if (path.startsWith("~/"))
 				path = path.substring(2);
 			try {
-				session = openSession();
-				sessionOwner = true;
-				ftp = TransportSftp.this.open(session);
+				ftp = newSftp();
 				ftp.cd(path);
 				ftp.cd("objects");
 				objectsPath = ftp.pwd();
@@ -177,10 +187,8 @@ class TransportSftp extends WalkTransport {
 
 		SftpObjectDB(final SftpObjectDB parent, final String p)
 				throws TransportException {
-			sessionOwner = false;
-			session = parent.session;
 			try {
-				ftp = TransportSftp.this.open(session);
+				ftp = newSftp();
 				ftp.cd(parent.objectsPath);
 				ftp.cd(p);
 				objectsPath = ftp.pwd();
@@ -452,14 +460,6 @@ class TransportSftp extends WalkTransport {
 					ftp = null;
 				}
 			}
-
-			if (sessionOwner && session != null) {
-				try {
-					sch.releaseSession(session);
-				} finally {
-					session = null;
-				}
-			}
 		}
 	}
 }
-- 
1.5.6.2.393.g45096

^ permalink raw reply related

* [JGIT PATCH 2/5] Don't display passwords on the console in fetch/push output
From: Shawn O. Pearce @ 2008-07-10  6:13 UTC (permalink / raw)
  To: Robin Rosenberg, Marek Zawirski; +Cc: git
In-Reply-To: <1215670403-19191-2-git-send-email-spearce@spearce.org>

When we show the URI we just fetched or pushed against there may
be a user password embedded in that URI, as saved in the user's
.git/config file.  We shouldn't display that in public to prying
eyes so nulling it out will give us a copy of the URI without that
field in it.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../src/org/spearce/jgit/pgm/Fetch.java            |    2 +-
 .../src/org/spearce/jgit/pgm/Push.java             |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/org.spearce.jgit/src/org/spearce/jgit/pgm/Fetch.java b/org.spearce.jgit/src/org/spearce/jgit/pgm/Fetch.java
index c9c997e..36a0592 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/pgm/Fetch.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/pgm/Fetch.java
@@ -71,7 +71,7 @@ class Fetch extends TextBuiltin {
 			return;
 
 		out.print("From ");
-		out.print(tn.getURI());
+		out.print(tn.getURI().setPass(null));
 		out.println();
 		for (final TrackingRefUpdate u : r.getTrackingRefUpdates()) {
 			final char type = shortTypeOf(u.getResult());
diff --git a/org.spearce.jgit/src/org/spearce/jgit/pgm/Push.java b/org.spearce.jgit/src/org/spearce/jgit/pgm/Push.java
index cbdf465..8411a11 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/pgm/Push.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/pgm/Push.java
@@ -149,7 +149,7 @@ class Push extends TextBuiltin {
 			final RemoteRefUpdate rru) {
 		if (first) {
 			first = false;
-			out.format("To %s\n", transport.getURI());
+			out.format("To %s\n", transport.getURI().setPass(null));
 		}
 
 		final String remoteName = rru.getRemoteName();
-- 
1.5.6.2.393.g45096

^ permalink raw reply related

* [JGIT PATCH 4/5] Report remote SSH execution errors during push via TransportException
From: Shawn O. Pearce @ 2008-07-10  6:13 UTC (permalink / raw)
  To: Robin Rosenberg, Marek Zawirski; +Cc: git
In-Reply-To: <1215670403-19191-4-git-send-email-spearce@spearce.org>

If the remote side failed to execute git-receive-pack we may
have the reason why on the stderr stream of the channel, as
the remote shell may have failed execution.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../spearce/jgit/transport/TransportGitSsh.java    |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/TransportGitSsh.java b/org.spearce.jgit/src/org/spearce/jgit/transport/TransportGitSsh.java
index b169f4c..9a6c719 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/TransportGitSsh.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/TransportGitSsh.java
@@ -228,7 +228,7 @@ class TransportGitSsh extends PackTransport {
 				if (channel.isConnected())
 					init(channel.getInputStream(), channel.getOutputStream());
 				else
-					throw new TransportException(errStream.toString());
+					throw new TransportException(uri, errStream.toString());
 
 			} catch (TransportException err) {
 				close();
@@ -263,7 +263,12 @@ class TransportGitSsh extends PackTransport {
 			super(TransportGitSsh.this);
 			try {
 				channel = exec(getOptionReceivePack());
-				init(channel.getInputStream(), channel.getOutputStream());
+
+				if (channel.isConnected())
+					init(channel.getInputStream(), channel.getOutputStream());
+				else
+					throw new TransportException(uri, errStream.toString());
+
 			} catch (TransportException err) {
 				close();
 				throw err;
-- 
1.5.6.2.393.g45096

^ permalink raw reply related

* [JGIT PATCH 1/5] Include a progress meter for large uploads to Amazon S3
From: Shawn O. Pearce @ 2008-07-10  6:13 UTC (permalink / raw)
  To: Robin Rosenberg, Marek Zawirski; +Cc: git
In-Reply-To: <1215670403-19191-1-git-send-email-spearce@spearce.org>

If we are uploading a sizable pack file or idx file to the S3
service the upload happens after we have finished writing the pack
to a local temporary file.  This causes a long pause for the user
while they wait for the data transfer to complete, with no real
indication of progress happening.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../src/org/spearce/jgit/transport/AmazonS3.java   |   29 +++++++++++++++----
 .../spearce/jgit/transport/TransportAmazonS3.java  |    7 +++-
 .../org/spearce/jgit/transport/TransportSftp.java  |    5 +++-
 .../spearce/jgit/transport/WalkPushConnection.java |    5 ++-
 .../jgit/transport/WalkRemoteObjectDatabase.java   |   11 ++++++-
 5 files changed, 44 insertions(+), 13 deletions(-)

diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/AmazonS3.java b/org.spearce.jgit/src/org/spearce/jgit/transport/AmazonS3.java
index e8575aa..59337f8 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/AmazonS3.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/AmazonS3.java
@@ -75,6 +75,8 @@ import javax.crypto.spec.SecretKeySpec;
 
 import org.spearce.jgit.awtui.AwtAuthenticator;
 import org.spearce.jgit.lib.Constants;
+import org.spearce.jgit.lib.NullProgressMonitor;
+import org.spearce.jgit.lib.ProgressMonitor;
 import org.spearce.jgit.util.Base64;
 import org.spearce.jgit.util.HttpSupport;
 import org.spearce.jgit.util.TemporaryBuffer;
@@ -376,7 +378,7 @@ public class AmazonS3 {
 			// We have to copy to produce the cipher text anyway so use
 			// the large object code path as it supports that behavior.
 			//
-			final OutputStream os = beginPut(bucket, key);
+			final OutputStream os = beginPut(bucket, key, null, null);
 			os.write(data);
 			os.close();
 			return;
@@ -430,11 +432,17 @@ public class AmazonS3 {
 	 *            name of the bucket storing the object.
 	 * @param key
 	 *            key of the object within its bucket.
+	 * @param monitor
+	 *            (optional) progress monitor to post upload completion to
+	 *            during the stream's close method.
+	 * @param monitorTask
+	 *            (optional) task name to display during the close method.
 	 * @return a stream which accepts the new data, and transmits once closed.
 	 * @throws IOException
 	 *             if encryption was enabled it could not be configured.
 	 */
-	public OutputStream beginPut(final String bucket, final String key)
+	public OutputStream beginPut(final String bucket, final String key,
+			final ProgressMonitor monitor, final String monitorTask)
 			throws IOException {
 		final MessageDigest md5 = newMD5();
 		final TemporaryBuffer buffer = new TemporaryBuffer() {
@@ -442,7 +450,8 @@ public class AmazonS3 {
 			public void close() throws IOException {
 				super.close();
 				try {
-					putImpl(bucket, key, md5.digest(), this);
+					putImpl(bucket, key, md5.digest(), this, monitor,
+							monitorTask);
 				} finally {
 					destroy();
 				}
@@ -452,7 +461,13 @@ public class AmazonS3 {
 	}
 
 	private void putImpl(final String bucket, final String key,
-			final byte[] csum, final TemporaryBuffer buf) throws IOException {
+			final byte[] csum, final TemporaryBuffer buf,
+			ProgressMonitor monitor, String monitorTask) throws IOException {
+		if (monitor == null)
+			monitor = NullProgressMonitor.INSTANCE;
+		if (monitorTask == null)
+			monitorTask = "Uploading " + key;
+
 		final String md5str = Base64.encodeBytes(csum);
 		final long len = buf.length();
 		final String lenstr = String.valueOf(len);
@@ -465,10 +480,12 @@ public class AmazonS3 {
 			authorize(c);
 			c.setDoOutput(true);
 			c.setFixedLengthStreamingMode((int) len);
+			monitor.beginTask(monitorTask, (int) (len / 1024));
 			final OutputStream os = c.getOutputStream();
 			try {
-				buf.writeTo(os, null);
+				buf.writeTo(os, monitor);
 			} finally {
+				monitor.endTask();
 				os.close();
 			}
 
@@ -641,7 +658,7 @@ public class AmazonS3 {
 		} else if ("rm".equals(op) || "delete".equals(op)) {
 			s3.delete(bucket, key);
 		} else if ("put".equals(op)) {
-			final OutputStream os = s3.beginPut(bucket, key);
+			final OutputStream os = s3.beginPut(bucket, key, null, null);
 			final byte[] tmp = new byte[2048];
 			int n;
 			while ((n = System.in.read(tmp)) > 0)
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/TransportAmazonS3.java b/org.spearce.jgit/src/org/spearce/jgit/transport/TransportAmazonS3.java
index d2f4c83..cd62c5b 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/TransportAmazonS3.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/TransportAmazonS3.java
@@ -54,6 +54,7 @@ import java.util.TreeMap;
 import org.spearce.jgit.errors.NotSupportedException;
 import org.spearce.jgit.errors.TransportException;
 import org.spearce.jgit.lib.ObjectId;
+import org.spearce.jgit.lib.ProgressMonitor;
 import org.spearce.jgit.lib.Ref;
 import org.spearce.jgit.lib.Repository;
 import org.spearce.jgit.lib.Ref.Storage;
@@ -236,8 +237,10 @@ class TransportAmazonS3 extends WalkTransport {
 		}
 
 		@Override
-		OutputStream writeFile(final String path) throws IOException {
-			return s3.beginPut(bucket, resolveKey(path));
+		OutputStream writeFile(final String path,
+				final ProgressMonitor monitor, final String monitorTask)
+				throws IOException {
+			return s3.beginPut(bucket, resolveKey(path), monitor, monitorTask);
 		}
 
 		@Override
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/TransportSftp.java b/org.spearce.jgit/src/org/spearce/jgit/transport/TransportSftp.java
index a33406b..c2cbe6a 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/TransportSftp.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/TransportSftp.java
@@ -54,6 +54,7 @@ import java.util.TreeMap;
 
 import org.spearce.jgit.errors.TransportException;
 import org.spearce.jgit.lib.ObjectId;
+import org.spearce.jgit.lib.ProgressMonitor;
 import org.spearce.jgit.lib.Ref;
 import org.spearce.jgit.lib.Repository;
 import org.spearce.jgit.lib.Ref.Storage;
@@ -294,7 +295,9 @@ class TransportSftp extends WalkTransport {
 		}
 
 		@Override
-		OutputStream writeFile(final String path) throws IOException {
+		OutputStream writeFile(final String path,
+				final ProgressMonitor monitor, final String monitorTask)
+				throws IOException {
 			try {
 				return ftp.put(path);
 			} catch (SftpException je) {
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/WalkPushConnection.java b/org.spearce.jgit/src/org/spearce/jgit/transport/WalkPushConnection.java
index bb5a653..904a699 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/WalkPushConnection.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/WalkPushConnection.java
@@ -230,14 +230,15 @@ class WalkPushConnection extends BaseConnection implements PushConnection {
 			// Write the pack file, then the index, as readers look the
 			// other direction (index, then pack file).
 			//
-			OutputStream os = dest.writeFile(pathPack);
+			final String wt = "Put " + base.substring(0, 12);
+			OutputStream os = dest.writeFile(pathPack, monitor, wt + "..pack");
 			try {
 				pw.writePack(os);
 			} finally {
 				os.close();
 			}
 
-			os = dest.writeFile(pathIdx);
+			os = dest.writeFile(pathIdx, monitor, wt + "..idx");
 			try {
 				pw.writeIndex(os);
 			} finally {
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/WalkRemoteObjectDatabase.java b/org.spearce.jgit/src/org/spearce/jgit/transport/WalkRemoteObjectDatabase.java
index 915faac..c5a5199 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/WalkRemoteObjectDatabase.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/WalkRemoteObjectDatabase.java
@@ -52,6 +52,7 @@ import java.util.Map;
 import org.spearce.jgit.errors.TransportException;
 import org.spearce.jgit.lib.Constants;
 import org.spearce.jgit.lib.ObjectId;
+import org.spearce.jgit.lib.ProgressMonitor;
 import org.spearce.jgit.lib.Ref;
 import org.spearce.jgit.util.NB;
 
@@ -213,11 +214,17 @@ abstract class WalkRemoteObjectDatabase {
 	 *         complete the write request. The stream is not buffered and each
 	 *         write may cause a network request/response so callers should
 	 *         buffer to smooth out small writes.
+	 * @param monitor
+	 *            (optional) progress monitor to post write completion to during
+	 *            the stream's close method.
+	 * @param monitorTask
+	 *            (optional) task name to display during the close method.
 	 * @throws IOException
 	 *             writing is not supported, or attempting to write the file
 	 *             failed, possibly due to permissions or remote disk full, etc.
 	 */
-	OutputStream writeFile(final String path) throws IOException {
+	OutputStream writeFile(final String path, final ProgressMonitor monitor,
+			final String monitorTask) throws IOException {
 		throw new IOException("Writing of '" + path + "' not supported.");
 	}
 
@@ -247,7 +254,7 @@ abstract class WalkRemoteObjectDatabase {
 	 *             failed, possibly due to permissions or remote disk full, etc.
 	 */
 	void writeFile(final String path, final byte[] data) throws IOException {
-		final OutputStream os = writeFile(path);
+		final OutputStream os = writeFile(path, null, null);
 		try {
 			os.write(data);
 		} finally {
-- 
1.5.6.2.393.g45096

^ permalink raw reply related

* [JGIT PATCH 0/5] Yet another round of transport fixes
From: Shawn O. Pearce @ 2008-07-10  6:13 UTC (permalink / raw)
  To: Robin Rosenberg, Marek Zawirski; +Cc: git

Apparently tweaking minor details is never done.  This short
series improves minor issues I have with the transport code
in relation to error and progress reporting.  We also get a
small performance boost for tag fetching over SSH.

Shawn O. Pearce (5):
  Include a progress meter for large uploads to Amazon S3
  Don't display passwords on the console in fetch/push output
  Reuse the same SSH connection when automatically fetching tags
  Report remote SSH execution errors during push via TransportException
  Explicitly capture the stderr from a failed SSH fetch or push

 .../org/spearce/egit/core/op/CloneOperation.java   |   10 ++-
 .../spearce/egit/ui/EclipseSshSessionFactory.java  |    5 +-
 .../egit/ui/internal/clone/SourceBranchPage.java   |    1 +
 .../spearce/jgit/transport/PushProcessTest.java    |    5 +
 .../org/spearce/jgit/transport/TransportTest.java  |   10 ++
 .../jgit/errors/NoRemoteRepositoryException.java   |   59 +++++++++++
 .../src/org/spearce/jgit/pgm/Fetch.java            |   19 +++--
 .../src/org/spearce/jgit/pgm/LsRemote.java         |    1 +
 .../src/org/spearce/jgit/pgm/Push.java             |    2 +-
 .../src/org/spearce/jgit/transport/AmazonS3.java   |   29 +++++-
 .../spearce/jgit/transport/BasePackConnection.java |    3 +-
 .../jgit/transport/DefaultSshSessionFactory.java   |   30 ++++++-
 .../src/org/spearce/jgit/transport/Transport.java  |   10 ++
 .../spearce/jgit/transport/TransportAmazonS3.java  |   12 ++-
 .../spearce/jgit/transport/TransportBundle.java    |    5 +
 .../spearce/jgit/transport/TransportGitAnon.java   |    5 +
 .../spearce/jgit/transport/TransportGitSsh.java    |  103 +++++++++++++-------
 .../org/spearce/jgit/transport/TransportHttp.java  |    5 +
 .../org/spearce/jgit/transport/TransportLocal.java |    5 +
 .../org/spearce/jgit/transport/TransportSftp.java  |   57 ++++++-----
 .../spearce/jgit/transport/WalkPushConnection.java |    5 +-
 .../jgit/transport/WalkRemoteObjectDatabase.java   |   11 ++-
 22 files changed, 303 insertions(+), 89 deletions(-)
 create mode 100644 org.spearce.jgit/src/org/spearce/jgit/errors/NoRemoteRepositoryException.java

^ permalink raw reply

* Re: [PATCH] Correct pack memory leak causing git gc to try to exceed ulimit
From: Nicolas Pitre @ 2008-07-10  5:37 UTC (permalink / raw)
  To: Shawn O. Pearce; +Cc: Junio C Hamano, git
In-Reply-To: <20080709071007.GA31974@spearce.org>

On Wed, 9 Jul 2008, Shawn O. Pearce wrote:

> When recursing to unpack a delta base we must unuse_pack() so that
> the pack window for the current object does not remain pinned in
> memory while the delta base is itself being unpacked and materialized
> for our use.
> 
> On a long delta chain of 50 objects we may need to access 6 different
> windows from a very large (>3G) pack file in order to obtain all
> of the delta base content.  If the process ulimit permits us to
> map/allocate only 1.5G we must release windows during this recursion
> to ensure we stay within the ulimit and transition memory from pack
> cache to standard malloc, or other mmap needs.
> 
> Inserting an unuse_pack() call prior to the recursion allows us to
> avoid pinning the current window, making it available for garbage
> collection if memory runs low.
> 
> This has been broken since at least before 1.5.1-rc1, and very
> likely earlier than that.  Its fixed now.  :)

Well well...  I suspect this might have been the cause of our strange 
out-of-memory issue when trying to repack the gcc repository a while 
ago.

I updated my copy of git://git.infradead.org/gcc.git and re-attempted a 
full repack with window=500 and depth=500, and this time it worked for 
me on a 32-bit machine!  Took about 6h30 single-threaded, and produced a 
mere 331MB pack file containing 1254664 objects across 142328 commits.


Nicolas

^ permalink raw reply

* Re: [PATCH 2/4] git-imap-send: Add support for SSL.
From: Mike Hommey @ 2008-07-10  5:33 UTC (permalink / raw)
  To: Robert Shearman; +Cc: git, Robert Shearman
In-Reply-To: <1215638942-25010-2-git-send-email-robertshearman@gmail.com>

On Wed, Jul 09, 2008 at 10:29:00PM +0100, Robert Shearman wrote:
> From: Robert Shearman <rob@codeweavers.com>
> 
> Allow SSL to be used when a imaps:// URL is used for the host name.
> 
> Also, automatically use TLS when not using imaps:// by using the IMAP STARTTLS command, if the server supports it.
> 
> Tested with Courier and Gimap IMAP servers.

Please do an alternative implementation using GnuTLS. Why ? Because curl
can be built against openssl or gnutls, and it would be sad if git
ended up depending on both indirectly.

Mike

^ permalink raw reply

* Re: [PATCH] apply: fix copy/rename breakage
From: Stephan Beyer @ 2008-07-10  4:21 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Linus Torvalds, Don Zickus
In-Reply-To: <7vy74aqvr1.fsf@gitster.siamese.dyndns.org>

Hi,

On Wed, Jul 09, 2008 at 08:10:58PM -0700, Junio C Hamano wrote:
> 
> 	diff --git a/A b/A
> 	--- a/A
> 	+++ b/A
> 	... change text here ...
> 	diff --git a/A b/B
> 	copy from A
> 	copy to B
> 	--- a/A
> 	+++ b/B
> 	... change text here ...

Big thanks! Now my patch applies cleanly again and many others, too. So:

Tested-by: Stephan Beyer <s-beyer@gmx.net>

;)

Regards,
  Stephan

-- 
Stephan Beyer <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F

^ permalink raw reply

* [PATCH] bisect: test merge base if good rev is not an ancestor of bad rev
From: Christian Couder @ 2008-07-10  3:41 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Johannes Schindelin, Michael Haggerty, Jeff King, git

Before this patch, "git bisect", when it was given some good revs that
are not ancestor of the bad rev, didn't check if the merge bases were
good. "git bisect" just supposed that the user knew what he was doing,
and that, when he said the revs were good, he knew that it meant that
all the revs in the history leading to the good revs were also
considered good.

But in pratice, the user may not know that a good rev is not an
ancestor of the bad rev, or he may not know/remember that all revs
leading to the good rev will be considered good. So he may give a good
rev that is a sibling, instead of an ancestor, of the bad rev, when in
fact there can be one rev becoming good in the branch of the good rev
(because the bug was already fixed there for example) instead of one
rev becoming bad in the branch of the bad rev.

For example, if there is the following history:

A-B-C-D
   \E-F

and we launch "git bisect start D F" then only C and D would have been
considered as possible first bad commit before this patch. This may be
wrong because A, B and E may be bad too if the bug exists everywhere
except in F that fixes it.

The purpose of this patch is to detect when "git bisect" is passed
some good revs that are not ancestor of the bad rev, and then to first
ask the user to test the merge bases between the good and bad revs.

If the merge bases are good then all is fine, we can continue
bisecting. Otherwise, if one merge base is bad, it means that the
assumption that all revs leading to the good one are good too is
wrong and we error out. In the case where one merge base is skipped we
issue a warning and then continue bisecting anyway.

These checks will also catch the case where good and bad have been
mistaken. This means that we can remove the check that was done latter
on the output of "git rev-list --bisect-vars".

In the current implementation though, only one merge base between a
good and a bad rev is tested.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 git-bisect.sh               |  137 +++++++++++++++++++++++++++++++++++--------
 t/t6030-bisect-porcelain.sh |   41 +++++++++++++
 2 files changed, 153 insertions(+), 25 deletions(-)

	Le mardi 1 juillet 2008, Junio C Hamano a écrit :
	> Aside from the "test a trial merge" idea I floated in the other message,
	> when we detect such a fork, perhaps we can suggest testing the merge base
	> version (B in your picture) first?  We would immediately know as the user
	> would say "B is bad" if the topology is problematic.
	>
	> Then, we can suggest the user that breakage at D may not be a regression
	> but a longstanding bug that was recently fixed somewhere between B and F.

	Here is a patch to do that.

diff --git a/git-bisect.sh b/git-bisect.sh
index 991b2ef..50f912f 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -242,33 +242,18 @@ bisect_auto_next() {
 	bisect_next_check && bisect_next || :
 }
 
-eval_rev_list() {
-	_eval="$1"
-
-	eval $_eval
-	res=$?
-
-	if [ $res -ne 0 ]; then
-		echo >&2 "'git rev-list --bisect-vars' failed:"
-		echo >&2 "maybe you mistake good and bad revs?"
-		exit $res
-	fi
-
-	return $res
-}
-
 filter_skipped() {
 	_eval="$1"
 	_skip="$2"
 
 	if [ -z "$_skip" ]; then
-		eval_rev_list "$_eval"
+		eval "$_eval"
 		return
 	fi
 
 	# Let's parse the output of:
 	# "git rev-list --bisect-vars --bisect-all ..."
-	eval_rev_list "$_eval" | while read hash line
+	eval "$_eval" | while read hash line
 	do
 		case "$VARS,$FOUND,$TRIED,$hash" in
 			# We display some vars.
@@ -331,20 +316,122 @@ exit_if_skipped_commits () {
 	fi
 }
 
+checkout() {
+	_rev="$1"
+	_msg="$2"
+	echo "Bisecting: $_msg"
+	git checkout -q "$_rev" || exit
+	git show-branch "$_rev"
+}
+
+is_among() {
+	_rev="$1"
+	_list="$2"
+	case "$_list" in *$_rev*) return 0 ;; esac
+	return 1
+}
+
+is_merge_base_ok() {
+	grep "^$1 $2 ok$" "$GIT_DIR/BISECT_MERGE_BASES" >/dev/null 2>&1
+}
+
+mark_merge_base_ok() {
+	echo "$1 $2 ok" >> "$GIT_DIR/BISECT_MERGE_BASES"
+}
+
+is_testing_merge_base() {
+	grep "^testing $1$" "$GIT_DIR/BISECT_MERGE_BASES" >/dev/null 2>&1
+}
+
+mark_testing_merge_base() {
+	echo "testing $1" >> "$GIT_DIR/BISECT_MERGE_BASES"
+}
+
+handle_bad_merge_base() {
+	_badmb="$1"
+	_g="$2"
+	if is_testing_merge_base "$_badmb"; then
+		cat >&2 <<EOF
+The merge base $_badmb is bad.
+This means the bug has been fixed between $_badmb and $_g.
+EOF
+		exit 3
+	else
+		cat >&2 <<EOF
+Some good revs are not ancestor of the bad rev.
+git bisect cannot work properly in this case.
+Maybe you mistake good and bad revs?
+EOF
+		exit 1
+	fi
+}
+
+handle_skipped_merge_base() {
+	_bad="$1"
+	_g="$2"
+	_mb="$3"
+	cat >&2 <<EOF
+Warning: the merge base between $_bad and $_g must be skipped.
+So we cannot be sure the first bad commit is between $_mb and $_bad.
+We continue anyway.
+EOF
+	mark_merge_base_ok "$_bad" "$_g"
+}
+
+check_merge_bases() {
+	_bad="$1"
+	_good="$2"
+	_skip="$3"
+	for _g in $_good; do
+		is_merge_base_ok "$_bad" "$_g" && continue
+		_mb=$(git merge-base $_g $_bad)
+		if test "$_mb" = "$_g" || is_among "$_mb" "$_good"; then
+			mark_merge_base_ok "$_bad" "$_g"
+		elif test "$_mb" = "$_bad"; then
+			handle_bad_merge_base "$_bad" "$_g"
+		elif is_among "$_mb" "$_skip"; then
+			handle_skipped_merge_base "$_bad" "$_g" "_mb"
+		else
+			mark_testing_merge_base "$_mb"
+			checkout "$_mb" "a merge base must be tested"
+			checkout_done=1
+			break
+		fi
+	done
+}
+
+check_good_are_ancestors_of_bad() {
+	test -f "$GIT_DIR/BISECT_ANCESTORS_OK" && return
+	_bad="$1"
+	_good=$(echo $2 | sed -e 's/\^//g')
+	_skip="$3"
+	_side=$(git rev-list $_good ^$_bad)
+	if test -n "$_side"; then
+		check_merge_bases "$_bad" "$_good" "$_skip" || return
+		test "$checkout_done" -eq "1" && return
+	fi
+	: > "$GIT_DIR/BISECT_ANCESTORS_OK"
+}
+
 bisect_next() {
 	case "$#" in 0) ;; *) usage ;; esac
 	bisect_autostart
 	bisect_next_check good
 
+	# Get bad, good and skipped revs
+	bad=$(git rev-parse --verify refs/bisect/bad) &&
+	good=$(git for-each-ref --format='^%(objectname)' \
+		"refs/bisect/good-*" | tr '\012' ' ') &&
 	skip=$(git for-each-ref --format='%(objectname)' \
-		"refs/bisect/skip-*" | tr '\012' ' ') || exit
+		"refs/bisect/skip-*" | tr '\012' ' ') &&
 
+	# Maybe some merge bases must be tested first
+	check_good_are_ancestors_of_bad "$bad" "$good" "$skip" || exit
+	test "$checkout_done" -eq "1" && checkout_done='' && return
+
+	# Get bisection information
 	BISECT_OPT=''
 	test -n "$skip" && BISECT_OPT='--bisect-all'
-
-	bad=$(git rev-parse --verify refs/bisect/bad) &&
-	good=$(git for-each-ref --format='^%(objectname)' \
-		"refs/bisect/good-*" | tr '\012' ' ') &&
 	eval="git rev-list --bisect-vars $BISECT_OPT $good $bad --" &&
 	eval="$eval $(cat "$GIT_DIR/BISECT_NAMES")" &&
 	eval=$(filter_skipped "$eval" "$skip") &&
@@ -365,9 +452,7 @@ bisect_next() {
 	# commit is also a "skip" commit (see above).
 	exit_if_skipped_commits "$bisect_rev"
 
-	echo "Bisecting: $bisect_nr revisions left to test after this"
-	git checkout -q "$bisect_rev" || exit
-	git show-branch "$bisect_rev"
+	checkout "$bisect_rev" "$bisect_nr revisions left to test after this"
 }
 
 bisect_visualize() {
@@ -414,6 +499,8 @@ bisect_clean_state() {
 	do
 		git update-ref -d $ref $hash || exit
 	done
+	rm -f "$GIT_DIR/BISECT_MERGE_BASES" &&
+	rm -f "$GIT_DIR/BISECT_ANCESTORS_OK" &&
 	rm -f "$GIT_DIR/BISECT_LOG" &&
 	rm -f "$GIT_DIR/BISECT_NAMES" &&
 	rm -f "$GIT_DIR/BISECT_RUN" &&
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 0626544..bf24b9a 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -350,6 +350,47 @@ test_expect_success 'bisect does not create a "bisect" branch' '
 	git branch -D bisect
 '
 
+# This creates a "side" branch to test "siblings" cases.
+test_expect_success 'side branch creation' '
+	git bisect reset &&
+	git checkout -b side $HASH4 &&
+	add_line_into_file "5(side): first line on a side branch" hello &&
+	SIDE_HASH5=$(git rev-parse --verify HEAD) &&
+	add_line_into_file "6(side): second line on a side branch" hello &&
+	SIDE_HASH6=$(git rev-parse --verify HEAD) &&
+	add_line_into_file "7(side): third line on a side branch" hello &&
+	SIDE_HASH7=$(git rev-parse --verify HEAD)
+'
+
+test_expect_success 'good merge base when good and bad are siblings' '
+	git bisect start "$HASH7" "$SIDE_HASH7" > my_bisect_log.txt &&
+	grep "merge base must be tested" my_bisect_log.txt &&
+	grep $HASH4 my_bisect_log.txt &&
+	git bisect good > my_bisect_log.txt &&
+	test_must_fail grep "merge base must be tested" my_bisect_log.txt &&
+	grep $HASH6 my_bisect_log.txt &&
+	git bisect reset
+'
+test_expect_success 'skipped merge base when good and bad are siblings' '
+	git bisect start "$SIDE_HASH7" "$HASH7" > my_bisect_log.txt &&
+	grep "merge base must be tested" my_bisect_log.txt &&
+	grep $HASH4 my_bisect_log.txt &&
+	git bisect skip > my_bisect_log.txt 2>&1 &&
+	grep "Warning" my_bisect_log.txt &&
+	grep $SIDE_HASH6 my_bisect_log.txt &&
+	git bisect reset
+'
+
+test_expect_success 'bad merge base when good and bad are siblings' '
+	git bisect start "$HASH7" HEAD > my_bisect_log.txt &&
+	grep "merge base must be tested" my_bisect_log.txt &&
+	grep $HASH4 my_bisect_log.txt &&
+	test_must_fail git bisect bad > my_bisect_log.txt 2>&1 &&
+	grep "merge base $HASH4 is bad" my_bisect_log.txt &&
+	grep "fixed between $HASH4 and $SIDE_HASH7" my_bisect_log.txt &&
+	git bisect reset
+'
+
 #
 #
 test_done
-- 
1.5.6.2.221.g6ad9a

^ permalink raw reply related

* [PATCH] cherry: cache patch-ids to avoid repeating work
From: Geoffrey Irving @ 2008-07-10  3:34 UTC (permalink / raw)
  To: Junio C Hamano, Johannes Schindelin, git@vger.kernel.org
In-Reply-To: <7vprpnlglh.fsf@gitster.siamese.dyndns.org>

Add cached-sha-map.[ch] implementing a persistent hash map from sha1 to
sha1.  The map is read with mmap, and completely rewritten if any entries
change.  It would be good to add incremental update to handle the usual case
where only a few entries change.

This structure is used by patch-ids.c to cache the mapping from commit to
patch-id into $GIT_DIR/patch-id-cache.  In the one case I've tested so far,
this speeds up the second invocation of git-cherry by two orders of
magnitude.  The caching can be disabled by setting cherry.cachepatchids to
false.

Original code cannibalized from Johannes Schindelin's notes-index structure.

Signed-off-by: Geoffrey Irving <irving@naml.us>
---

Note: there are at least two "holes" in this code.  First, it is impossible
to verify the validity of the entries (this is impossible to fix).  Second,
it is possible to write a malicious patch-id-cache file that causes git-cherry
to go into an infinite loop.  Fixing the loop requires either traversing every
entry on load (bad) or adding a second loop termination condition to
find_helper.  Since looping forever is better than returning incorrect
results, I figured fixing the weaker hole would just result in a false sense
of security.

I'll await the next round of comments. :)

 Documentation/config.txt |    5 +
 Makefile                 |    2 +
 builtin-log.c            |   12 ++
 cached-sha1-map.c        |  269 ++++++++++++++++++++++++++++++++++++++++++++++
 cached-sha1-map.h        |   45 ++++++++
 patch-ids.c              |   26 +++++-
 patch-ids.h              |    2 +
 7 files changed, 360 insertions(+), 1 deletions(-)
 create mode 100644 cached-sha1-map.c
 create mode 100644 cached-sha1-map.h

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 838794d..02b8113 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -468,6 +468,11 @@ browser.<tool>.path::
 	browse HTML help (see '-w' option in linkgit:git-help[1]) or a
 	working repository in gitweb (see linkgit:git-instaweb[1]).

+cherry.cachepatchids::
+	If true, linkgit:git-cherry will store a cache of computed patch-ids
+	in $GIT_DIR/patch-id-cache in order to make repeated invocations faster.
+	Defaults to true.
+
 clean.requireForce::
 	A boolean to make git-clean do nothing unless given -f
 	or -n.   Defaults to true.
diff --git a/Makefile b/Makefile
index 4796565..f7360e1 100644
--- a/Makefile
+++ b/Makefile
@@ -356,6 +356,7 @@ LIB_H += pack-refs.h
 LIB_H += pack-revindex.h
 LIB_H += parse-options.h
 LIB_H += patch-ids.h
+LIB_H += cached-sha1-map.h
 LIB_H += path-list.h
 LIB_H += pkt-line.h
 LIB_H += progress.h
@@ -436,6 +437,7 @@ LIB_OBJS += pager.o
 LIB_OBJS += parse-options.o
 LIB_OBJS += patch-delta.o
 LIB_OBJS += patch-ids.o
+LIB_OBJS += cached-sha1-map.o
 LIB_OBJS += path-list.o
 LIB_OBJS += path.o
 LIB_OBJS += pkt-line.o
diff --git a/builtin-log.c b/builtin-log.c
index 430d876..fbfefbd 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -1081,6 +1081,16 @@ static int add_pending_commit(const char *arg,
struct rev_info *revs, int flags)
 	return -1;
 }

+static int git_cherry_config(const char *var, const char *value, void *cb)
+{
+	if (!strcmp(var, "cherry.cachepatchids")) {
+		cache_patch_ids = git_config_bool(var, value);
+		return 0;
+	}
+
+	return 0;
+}
+
 static const char cherry_usage[] =
 "git-cherry [-v] <upstream> [<head>] [<limit>]";
 int cmd_cherry(int argc, const char **argv, const char *prefix)
@@ -1094,6 +1104,8 @@ int cmd_cherry(int argc, const char **argv,
const char *prefix)
 	const char *limit = NULL;
 	int verbose = 0;

+	git_config(git_cherry_config, NULL);
+
 	if (argc > 1 && !strcmp(argv[1], "-v")) {
 		verbose = 1;
 		argc--;
diff --git a/cached-sha1-map.c b/cached-sha1-map.c
new file mode 100644
index 0000000..3ac5474
--- /dev/null
+++ b/cached-sha1-map.c
@@ -0,0 +1,269 @@
+#include "cached-sha1-map.h"
+
+union cached_sha1_map_header {
+	struct {
+		char signature[4]; /* CS1M */
+		uint32_t version;
+		uint32_t count;
+		uint32_t size;
+	} u;
+	struct cached_sha1_entry pad; /* pad header out to 40 bytes */
+};
+
+static const char *signature = "CS1M";
+static const uint32_t version = 1;
+
+static int init_empty_map(struct cached_sha1_map *cache, uint32_t size)
+{
+	cache->count = 0;
+	cache->size = size;
+	cache->initialized = 1;
+	cache->mmapped = 0;
+	cache->dirty = 1;
+
+	cache->entries = calloc(size, sizeof(struct cached_sha1_entry));
+	if (!cache->entries) {
+		warning("failed to allocate empty map of size %"PRIu32" for %s",
+			size, git_path(cache->filename));
+		cache->size = 0;
+		cache->dirty = 0;
+		return -1;
+	}
+	return 0;
+}
+
+static int grow_map(struct cached_sha1_map *cache)
+{
+	struct cached_sha1_map new_cache;
+	uint32_t i;
+
+	if (cache->size * 2 == 0) {
+		warning("%s overflowed, so resetting to empty",
+			git_path(cache->filename));
+		return init_empty_map(cache, 64);
+	}
+
+	/* allocate cache with twice the size */
+	new_cache.filename = cache->filename;
+	if (init_empty_map(&new_cache, cache->size * 2)) {
+		warning("failed to grow %s to size %"PRIu32,
+			git_path(cache->filename), cache->size * 2);
+		return init_empty_map(cache, 64);
+	}
+
+	/* reinsert all entries */
+ 	for (i = 0; i < cache->size; i++)
+		if (!is_null_sha1(cache->entries[i].key))
+			set_cached_sha1_entry(&new_cache,
+				cache->entries[i].key, cache->entries[i].value);
+	/* finish */
+	free_cached_sha1_map(cache);
+	*cache = new_cache;
+	return 0;
+}
+
+/* Any errors that occur result in the cache being initialized to empty */
+static int init_cached_sha1_map(struct cached_sha1_map *cache)
+{
+	int fd;
+	union cached_sha1_map_header header;
+	const char *filename;
+	size_t map_size;
+
+	if (cache->initialized)
+		return cache->size ? 0 : -1;
+
+	filename = git_path(cache->filename);
+	fd = open(filename, O_RDONLY);
+	if (fd < 0) {
+		if (errno != ENOENT)
+			warning("failed to read '%s': %s", filename,
+				strerror(errno));
+		goto empty;
+	}
+
+	if (read_in_full(fd, &header, sizeof(header)) != sizeof(header))
+	{
+		warning("cannot read %s header", filename);
+		goto empty;
+	}
+
+	if (memcmp(header.u.signature, signature, 4))
+	{
+		warning("%s has invalid header", filename);
+		goto empty;
+	}
+
+	if (ntohl(header.u.version) != version)
+	{
+		warning("%s has unrecognized version %"PRIu32, filename,
+			ntohl(header.u.version));
+		goto empty;
+	}
+
+	cache->count = ntohl(header.u.count);
+	cache->size = ntohl(header.u.size);
+
+	if (cache->size & (cache->size-1))
+	{
+		warning("%s is corrupt: size %"PRIu32" is not a power of two",
+			filename, cache->size);
+		goto empty;
+	}
+
+	if (cache->count >= cache->size)
+	{
+		warning("%s is corrupt: count %"PRIu32" >= size %"PRIu32,
+			filename, cache->count, cache->size);
+		goto empty;
+	}
+
+	cache->dirty = 0;
+	cache->initialized = 1;
+	cache->mmapped = 1;
+
+	/* mmap entire file so that file / memory blocks are aligned */
+	map_size = sizeof(struct cached_sha1_entry) * (cache->size + 1);
+	cache->entries = mmap(NULL, map_size,
+		PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);
+	if (cache->entries == MAP_FAILED) {
+		/* this is just a cache, so don't free pack memory and retry */
+		warning("%s mmap failed: %s", filename, strerror(errno));
+		goto empty;
+	}
+	cache->entries += 1; /* skip header */
+	return 0;
+
+empty:
+	if (fd >= 0)
+		close(fd);
+	return init_empty_map(cache, 64);
+}
+
+int write_cached_sha1_map(struct cached_sha1_map *cache)
+{
+	union cached_sha1_map_header header;
+	struct lock_file update_lock;
+	int fd;
+	size_t map_size;
+	const char *filename;
+
+	if (!cache->initialized || !cache->dirty)
+		return 0;
+
+	filename = git_path(cache->filename);
+	fd = hold_lock_file_for_update(&update_lock, filename, 0);
+
+	if (fd < 0)
+	{
+		warning("could not construct %s", filename);
+		return -1;
+	}
+
+	memcpy(header.u.signature, signature, 4);
+	header.u.version = htonl(version);
+	header.u.count = htonl(cache->count);
+	header.u.size = htonl(cache->size);
+	map_size = sizeof(struct cached_sha1_entry) * cache->size;
+	if (write_in_full(fd, &header, sizeof(header)) != sizeof(header)
+		|| write_in_full(fd, cache->entries, map_size) != map_size)
+	{
+		warning("could not write %s", filename);
+		return -1;
+	}
+
+	if (commit_lock_file(&update_lock) < 0)
+	{
+		warning("could not write %s", filename);
+		return -1;
+	}
+
+	cache->dirty = 0;
+	return 0;
+}
+
+void free_cached_sha1_map(struct cached_sha1_map *cache)
+{
+	if (!cache->initialized)
+		return;
+
+	if (cache->mmapped)
+		munmap(cache->entries - 1,
+			sizeof(struct cached_sha1_entry) * (cache->size + 1));
+	else
+		free(cache->entries);
+}
+
+/* The fact that size is a power of two means count-1 <= INT32_MAX, so it
+ * is safe to return signed integers here. */
+static int32_t get_hash_index(const unsigned char *sha1)
+{
+	/* this is alignment safe since 40 is a multiple of 4 */
+	return ntohl(*(uint32_t*)sha1);
+}
+
+/*
+ * Returns the index if the entry exists, and the complemented index of
+ * the next free entry otherwise.
+ */
+static int32_t find_helper(struct cached_sha1_map *cache,
+	const unsigned char *key)
+{
+	int32_t i, mask;
+
+	mask = cache->size - 1;
+
+	for (i = get_hash_index(key) & mask; ; i = (i+1) & mask) {
+		if (!hashcmp(key, cache->entries[i].key))
+			return i;
+		else if (is_null_sha1(cache->entries[i].key))
+			return ~i;
+	}
+}
+
+int get_cached_sha1_entry(struct cached_sha1_map *cache,
+	const unsigned char *key, unsigned char *value)
+{
+	int32_t i;
+
+	if (init_cached_sha1_map(cache))
+		return -1;
+
+	i = find_helper(cache, key);
+	if(i < 0)
+		return -1;
+
+	/* entry found, return value */
+	hashcpy(value, cache->entries[i].value);
+	return 0;
+}
+
+int set_cached_sha1_entry(struct cached_sha1_map *cache,
+	const unsigned char *key, const unsigned char *value)
+{
+	int32_t i;
+	struct cached_sha1_entry *entry;
+
+	if (init_cached_sha1_map(cache))
+		return -1;
+
+	i = find_helper(cache, key);
+
+	if (i < 0) { /* write new entry */
+		entry = cache->entries + ~i;
+		hashcpy(entry->key, key);
+		hashcpy(entry->value, value);
+		cache->count++;
+		cache->dirty = 1;
+	} else { /* overwrite existing entry */
+		entry = cache->entries + i;
+		if (hashcmp(value, entry->value)) {
+			hashcpy(entry->value, value);
+			cache->dirty = 1;
+		}
+	}
+
+	if (4*cache->count >= 3*cache->size)
+		return grow_map(cache);
+	return 0;
+}
diff --git a/cached-sha1-map.h b/cached-sha1-map.h
new file mode 100644
index 0000000..296c17c
--- /dev/null
+++ b/cached-sha1-map.h
@@ -0,0 +1,45 @@
+#ifndef CACHED_SHA1_MAP_H
+#define CACHED_SHA1_MAP_H
+
+#include "cache.h"
+
+/*
+ * A cached-sha1-map is a file storing a hash map from sha1 to sha1.
+ *
+ * The file is mmap'ed, updated in memory during operation, and flushed
+ * back to disk when freed.  Currently the entire file is rewritten for
+ * any change.  This could be a significant bottleneck for common uses,
+ * so it would be good to fix this later if possible.
+ *
+ * The performance of a hash map depends highly on a good hashing
+ * algorithm, to avoid collisions.  Lucky us!  SHA-1 is a pretty good
+ * hashing algorithm.
+ */
+
+struct cached_sha1_entry {
+	unsigned char key[20];
+	unsigned char value[20];
+};
+
+struct cached_sha1_map {
+	const char *filename; /* relative to GIT_DIR */
+
+	/* rest is for internal use */
+	uint32_t count, size;
+	unsigned int initialized : 1;
+	unsigned int dirty : 1;
+	unsigned int mmapped : 1;
+	struct cached_sha1_entry *entries; /* pointer to mmap'ed memory + 1 */
+};
+
+extern int get_cached_sha1_entry(struct cached_sha1_map *cache,
+	const unsigned char *key,unsigned char *value);
+
+extern int set_cached_sha1_entry(struct cached_sha1_map *cache,
+	const unsigned char *key, const unsigned char *value);
+
+extern int write_cached_sha1_map(struct cached_sha1_map *cache);
+
+extern void free_cached_sha1_map(struct cached_sha1_map *cache);
+
+#endif
diff --git a/patch-ids.c b/patch-ids.c
index 3be5d31..663ffee 100644
--- a/patch-ids.c
+++ b/patch-ids.c
@@ -2,17 +2,36 @@
 #include "diff.h"
 #include "commit.h"
 #include "patch-ids.h"
+#include "cached-sha1-map.h"
+
+int cache_patch_ids = 1;
+static struct cached_sha1_map patch_id_cache;

 static int commit_patch_id(struct commit *commit, struct diff_options *options,
 		    unsigned char *sha1)
 {
+	int ret;
+
+	/* pull patch-id out of the cache if possible */
+	patch_id_cache.filename = "patch-id-cache";
+	if (cache_patch_ids && !get_cached_sha1_entry(&patch_id_cache,
+			commit->object.sha1, sha1))
+		return 0;
+
 	if (commit->parents)
 		diff_tree_sha1(commit->parents->item->object.sha1,
 		               commit->object.sha1, "", options);
 	else
 		diff_root_tree_sha1(commit->object.sha1, "", options);
 	diffcore_std(options);
-	return diff_flush_patch_id(options, sha1);
+	ret = diff_flush_patch_id(options, sha1);
+	if (ret)
+		return ret;
+
+	/* record commit, patch-id pair in cache */
+	if (cache_patch_ids)
+		set_cached_sha1_entry(&patch_id_cache, commit->object.sha1, sha1);
+	return 0;
 }

 static uint32_t take2(const unsigned char *id)
@@ -136,6 +155,11 @@ int free_patch_ids(struct patch_ids *ids)
 		next = patches->next;
 		free(patches);
 	}
+
+	/* write cached patch-ids and ignore any errors that arise
+	 * (e.g. if the repository is write protected) */
+	if (cache_patch_ids)
+		write_cached_sha1_map(&patch_id_cache);
 	return 0;
 }

diff --git a/patch-ids.h b/patch-ids.h
index c8c7ca1..c0ebdc1 100644
--- a/patch-ids.h
+++ b/patch-ids.h
@@ -18,4 +18,6 @@ int free_patch_ids(struct patch_ids *);
 struct patch_id *add_commit_patch_id(struct commit *, struct patch_ids *);
 struct patch_id *has_commit_patch_id(struct commit *, struct patch_ids *);

+extern int cache_patch_ids;
+
 #endif /* PATCH_IDS_H */
-- 
1.5.6.2.256.g47cb9.dirty

^ permalink raw reply related

* Re: [PATCH] apply: fix copy/rename breakage
From: Junio C Hamano @ 2008-07-10  3:13 UTC (permalink / raw)
  To: git; +Cc: Linus Torvalds, Don Zickus
In-Reply-To: <7vy74aqvr1.fsf@gitster.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> writes:

> Recently, 7ebd52a (Merge branch 'dz/apply-again', 2008-07-01) taught
> "git-apply" to grok a (non-git) patch that is a concatenation of separate
> patches that touch the same file number of files, by recording the

Eh, s/files/times/;

> postimage of patch application of previous round and using it as the
> preimage for later rounds.
>
> However, this "incremental" mode of patch application contradicts with the
> way git rename/copy patches are fundamentally designed....
> a patch to produce new files out of existing file by copying to fix this
> issue.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>
>  * Applies to 'master'.  I am CC'ing Linus not because he is in any way
>    responsible for this breakage, but because this breakage can affect
>    heavy users of "git apply".

^ permalink raw reply

* [PATCH] apply: fix copy/rename breakage
From: Junio C Hamano @ 2008-07-10  3:10 UTC (permalink / raw)
  To: git; +Cc: Linus Torvalds, Don Zickus

Recently, 7ebd52a (Merge branch 'dz/apply-again', 2008-07-01) taught
"git-apply" to grok a (non-git) patch that is a concatenation of separate
patches that touch the same file number of files, by recording the
postimage of patch application of previous round and using it as the
preimage for later rounds.

However, this "incremental" mode of patch application contradicts with the
way git rename/copy patches are fundamentally designed.  When a git patch
talks about a file A getting modified, and a new file B created out of B,
like this:

	diff --git a/A b/A
	--- a/A
	+++ b/A
	... change text here ...
	diff --git a/A b/B
	copy from A
	copy to B
	--- a/A
	+++ b/B
	... change text here ...

the second change to produce B does not depend on what is done to A with
the first change (this is explicitly done so for reviewability of
individual patches).

With this patch, we disable the postimage record 'fn_table' when applying
a patch to produce new files out of existing file by copying to fix this
issue.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * Applies to 'master'.  I am CC'ing Linus not because he is in any way
   responsible for this breakage, but because this breakage can affect
   heavy users of "git apply".

 builtin-apply.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/builtin-apply.c b/builtin-apply.c
index b3fc290..d13313f 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -2296,7 +2296,8 @@ static int apply_data(struct patch *patch, struct stat *st, struct cache_entry *
 
 	strbuf_init(&buf, 0);
 
-	if ((tpatch = in_fn_table(patch->old_name)) != NULL) {
+	if (!(patch->is_copy || patch->is_rename) &&
+	    ((tpatch = in_fn_table(patch->old_name)) != NULL)) {
 		if (tpatch == (struct patch *) -1) {
 			return error("patch %s has been renamed/deleted",
 				patch->old_name);
@@ -2375,7 +2376,7 @@ static int verify_index_match(struct cache_entry *ce, struct stat *st)
 static int check_preimage(struct patch *patch, struct cache_entry **ce, struct stat *st)
 {
 	const char *old_name = patch->old_name;
-	struct patch *tpatch;
+	struct patch *tpatch = NULL;
 	int stat_ret = 0;
 	unsigned st_mode = 0;
 
@@ -2389,7 +2390,9 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s
 		return 0;
 
 	assert(patch->is_new <= 0);
-	if ((tpatch = in_fn_table(old_name)) != NULL) {
+
+	if (!(patch->is_copy || patch->is_rename) &&
+	    (tpatch = in_fn_table(old_name)) != NULL) {
 		if (tpatch == (struct patch *) -1) {
 			return error("%s: has been deleted/renamed", old_name);
 		}
@@ -2399,6 +2402,7 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s
 		if (stat_ret && errno != ENOENT)
 			return error("%s: %s", old_name, strerror(errno));
 	}
+
 	if (check_index && !tpatch) {
 		int pos = cache_name_pos(old_name, strlen(old_name));
 		if (pos < 0) {
-- 
1.5.6.2.291.g7eef3

^ permalink raw reply related

* What's cooking in git.git (topics)
From: Junio C Hamano @ 2008-07-10  2:32 UTC (permalink / raw)
  To: git
In-Reply-To: <7vtzf1w0rj.fsf@gitster.siamese.dyndns.org>

Here are the topics that have been cooking.  Commits prefixed
with '-' are only in 'pu' while commits prefixed with '+' are
in 'next'.

The topics list the commits in reverse chronological order.  The topics
meant to be applied to the maintenance series have "maint-" in their
names.

It already is beginning to become clear what 1.6.0 will look like.  What's
already in 'next' all are well intentioned (I do not guarantee they are
already bug-free --- that is what cooking them in 'next' is for) and are
good set of feature enhancements.  Bigger changes will be:

 * Port for MinGW.

 * With the default Makefile settings, most of the programs will be
   installed outside your $PATH, except for "git", "gitk", "git-gui" and
   some server side programs that need to be accessible for technical
   reasons.  Invoking a git subcommand as "git-xyzzy" from the command
   line has been deprecated since early 2006 (and officially announced in
   1.5.4 release notes); use of them from your scripts after adding
   output from "git --exec-path" to the $PATH will still be supported in
   1.6.0, but users are again strongly encouraged to adjust their
   scripts to use "git xyzzy" form, as we will stop installing
   "git-xyzzy" hardlinks for built-in commands in later releases.

 * git-merge will be rewritten in C.

 * default pack and idx versions will be updated as scheduled for some
   time ago.

 * GIT_CONFIG, which was only documented as affecting "git config", but
   actually affected all git commands, now only affects "git config".
   GIT_LOCAL_CONFIG, also only documented as affecting "git config" and
   not different from GIT_CONFIG in a useful way, is removed.

----------------------------------------------------------------
[New Topics]

* xx/merge-in-c-into-next (Wed Jul 9 13:51:46 2008 -0700) 4 commits
 + Teach git-merge -X<option> again.
 + Merge branch 'jc/merge-theirs' into xx/merge-in-c-into-next
 + builtin-merge.c: use parse_options_step() "incremental parsing"
   machinery
 + Merge branch 'ph/parseopt-step-blame' into xx/merge-in-c-into-next

I've described what this is in a separate message.

* jc/branch-merged (Tue Jul 8 17:55:47 2008 -0700) 3 commits
 + branch --merged/--no-merged: allow specifying arbitrary commit
 + branch --contains: default to HEAD
 + parse-options: add PARSE_OPT_LASTARG_DEFAULT flag

This builds on top of the parse-options enhancement series that
has been cooking in 'next' for some time.

* rs/imap (Wed Jul 9 22:29:02 2008 +0100) 5 commits
 + Documentation: Improve documentation for git-imap-send(1)
 + imap-send.c: more style fixes
 + imap-send.c: style fixes
 + git-imap-send: Support SSL
 + git-imap-send: Allow the program to be run from subdirectories of
   a git tree

* om/rerere-careful (Mon Jul 7 14:42:48 2008 +0200) 1 commit
 + builtin-rerere: more carefully find conflict markers

----------------------------------------------------------------
[Will merge to master soon]

* js/pick-root (Fri Jul 4 16:19:52 2008 +0100) 1 commit
 + Allow cherry-picking root commits

* ab/bundle (Sat Jul 5 17:26:40 2008 -0400) 1 commit
 + Teach git-bundle to read revision arguments from stdin like git-
   rev-list.

* sg/stash-k-i (Tue Jul 8 00:40:56 2008 -0700) 2 commits
 + Documentation: tweak use case in "git stash save --keep-index"
 + stash: introduce 'stash save --keep-index' option

One weakness of our "partial commit" workflow support used to be that the
user can incrementally build what is to be committed in the index but that
state cannot be tested as a whole in the working tree.  This allows you to
temporarily stash the remaining changes in the working tree so that the
index state before running "stash save --keep-index" can be seen in the
working tree to be tested and then committed.

* am/stash-branch (Mon Jul 7 02:50:10 2008 +0530) 2 commits
 + Add a test for "git stash branch"
 + Implement "git stash branch <newbranch> <stash>"

Creates a new branch out of the stashed state, after returning from the
interrupt that forced you to create the stash in the first place.

* tr/add-i-e (Thu Jul 3 00:00:00 2008 +0200) 3 commits
 + git-add--interactive: manual hunk editing mode
 + git-add--interactive: remove hunk coalescing
 + git-add--interactive: replace hunk recounting with apply --recount

Adds 'e/dit' action to interactive add command.

* jc/report-tracking (Sun Jul 6 02:54:56 2008 -0700) 5 commits
 + branch -r -v: do not spit out garbage
 + stat_tracking_info(): clear object flags used during counting
 + git-branch -v: show the remote tracking statistics
 + git-status: show the remote tracking statistics
 + Refactor "tracking statistics" code used by "git checkout"

Makes the "your branch is ahead of the tracked one by N commits" logic and
messages available to other commands; status and branch are updated.

----------------------------------------------------------------
[Actively Cooking]

* jc/rebase-orig-head (Tue Jul 8 00:12:22 2008 -0400) 2 commits
 + Documentation: mention ORIG_HEAD in am, merge, and rebase
 + Teach "am" and "rebase" to mark the original position with
   ORIG_HEAD

* ph/parseopt-step-blame (Wed Jul 9 23:38:34 2008 +0200) 18 commits
 + revisions: refactor handle_revision_opt into parse_revision_opt.
 + git-shortlog: migrate to parse-options partially.
 + git-blame: fix lapsus
 + git-blame: migrate to incremental parse-option [2/2]
 + git-blame: migrate to incremental parse-option [1/2]
 + revisions: split handle_revision_opt() from setup_revisions()
 + Merge branch 'jc/blame' (early part) into HEAD
 + parse-opt: add PARSE_OPT_KEEP_ARGV0 parser option.
 + parse-opt: fake short strings for callers to believe in.
 + parse-opt: do not print errors on unknown options, return -2
   intead.
 + parse-opt: create parse_options_step.
 + parse-opt: Export a non NORETURN usage dumper.
 + parse-opt: have parse_options_{start,end}.
 + git-blame --reverse
 + builtin-blame.c: allow more than 16 parents
 + builtin-blame.c: move prepare_final() into a separate function.
 + rev-list --children
 + revision traversal: --children option

Became active again ;-) This probably is ready for 'master' already,
except for the last two which I only looked at the patch and have not
used heavily in production yet.

* jc/merge-theirs (Mon Jun 30 22:18:57 2008 -0700) 5 commits
 + Make "subtree" part more orthogonal to the rest of merge-
   recursive.
 + Teach git-pull to pass -X<option> to git-merge
 + Teach git-merge to pass -X<option> to the backend strategy module
 + git-merge-recursive-{ours,theirs}
 + git-merge-file --ours, --theirs

Punting a merge by discarding your own work in conflicting parts but still
salvaging the parts that are cleanly automerged.  It is likely that this
will result in nonsense mishmash, but somehow often people want this, so
here they are.  The interface to the backends is updated so that you can
say "git merge -Xours -Xsubtree=foo/bar/baz -s recursive other" now.

* mv/merge-in-c (Mon Jul 7 19:24:20 2008 +0200) 15 commits
 + Build in merge
 + Fix t7601-merge-pull-config.sh on AIX
 + git-commit-tree: make it usable from other builtins
 + Add new test case to ensure git-merge prepends the custom merge
   message
 + Add new test case to ensure git-merge reduces octopus parents when
   possible
 + Introduce reduce_heads()
 + Introduce get_merge_bases_many()
 + Add new test to ensure git-merge handles more than 25 refs.
 + Introduce get_octopus_merge_bases() in commit.c
 + git-fmt-merge-msg: make it usable from other builtins
 + Move read_cache_unmerged() to read-cache.c
 + Add new test to ensure git-merge handles pull.twohead and
   pull.octopus
 + Move parse-options's skip_prefix() to git-compat-util.h
 + Move commit_list_count() to commit.c
 + Move split_cmdline() to alias.c

----------------------------------------------------------------
[Graduated to "master"]

* js/apply-root (Sun Jul 6 18:36:01 2008 -0700) 3 commits
 + git-apply --directory: make --root more similar to GNU diff
 + apply --root: thinkofix.
 + Teach "git apply" to prepend a prefix with "--root=<root>"

* jc/reflog-expire (Sat Jun 28 22:24:49 2008 -0700) 2 commits
 + Make default expiration period of reflog used for stash infinite
 + Per-ref reflog expiry configuration

As 1.6.0 will be a good time to make backward incompatible changes, the
tip commit makes the default expiry period of stash 'never', unless you
configure them to expire explicitly using gc.refs/stash.* variables.
Needs consensus, but I am guessing that enough people would want stash
that does not expire.

* jk/pager-config (Thu Jul 3 07:46:57 2008 -0400) 1 commit
 + Allow per-command pager config

----------------------------------------------------------------
[On Hold]

* sg/merge-options (Sun Apr 6 03:23:47 2008 +0200) 1 commit
 + merge: remove deprecated summary and diffstat options and config
   variables

This was previously in "will be in master soon" category, but it turns out
that the synonyms to the ones this one deletes are fairly new invention
that happend in 1.5.6 timeframe, and we cannot do this just yet.  Perhaps
in 1.7.0.

* jc/dashless (Thu Jun 26 16:43:34 2008 -0700) 2 commits
 + Revert "Make clients ask for "git program" over ssh and local
   transport"
 + Make clients ask for "git program" over ssh and local transport

This is the "botched" one.  Will be resurrected during 1.7.0 or 1.8.0
timeframe.

* jk/renamelimit (Sat May 3 13:58:42 2008 -0700) 1 commit
 - diff: enable "too large a rename" warning when -M/-C is explicitly
   asked for

This would be the right thing to do for command line use, but gitk will be
hit due to tcl/tk's limitation, so I am holding this back for now.

----------------------------------------------------------------
[Stalled/Needs more work]

* sb/sequencer (Tue Jul 1 04:38:34 2008 +0200) 4 commits
 . Migrate git-am to use git-sequencer
 . Add git-sequencer test suite (t3350)
 . Add git-sequencer prototype documentation
 . Add git-sequencer shell prototype

* jc/grafts (Wed Jul 2 17:14:12 2008 -0700) 1 commit
 - [BROKEN wrt shallow clones] Ignore graft during object transfer

Cloning or fetching from a repository from grafts did not send objects
that are hidden by grafts, but the commits in the resulting repository do
need these to pass fsck.  This fixes object transfer to ignore grafts.

Another fix is needed to git-prune so that it ignores grafts but treats
commits that are mentioned in grafts as reachable.

* jc/blame (Wed Jun 4 22:58:40 2008 -0700) 7 commits
 - blame: show "previous" information in --porcelain/--incremental
   format
 - git-blame: refactor code to emit "porcelain format" output
 + git-blame --reverse
 + builtin-blame.c: allow more than 16 parents
 + builtin-blame.c: move prepare_final() into a separate function.
 + rev-list --children
 + revision traversal: --children option

The blame that finds where each line in the original lines moved to.  This
may help a GSoC project that wants to gather statistical overview of the
history.  The final presentation may need tweaking (see the log message of
the commit ""git-blame --reverse" on the series).

The tip two commits are for peeling to see what's behind the blamed
commit, which we should be able to separate out into an independent topic
from the rest.

^ permalink raw reply

* Re: git protocol specification
From: Shawn O. Pearce @ 2008-07-10  2:32 UTC (permalink / raw)
  To: Tom Werner; +Cc: Git Mailing List
In-Reply-To: <48750231.4020805@rubyisawesome.com>

Tom Werner <pubsub@rubyisawesome.com> wrote:
> I'm working on a flexible Erlang replacement for git-daemon and would  
> like to know if there is a specification available for the protocol that  
> git-upload-pack and git-receive-pack use. I've reverse engineered it but  
> have come across some points that seem inconsistent. Before I dig into  
> the C code in earnest I wanted to see if there's anything like spec  
> document for the system. Thanks!

I'm curious, are you planning on implementing git-upload-pack and
git-receive-pack in Erlang as well?  Or are you just doing the
git-daemon frontend and spawn the C git binaries as port processes?

git-receive-pack isn't too much trouble, the protocol on the server
side without the tell-me-more is fairly simple to implement, once
you have something like index-pack or unpack-objects implemented.
But that is somewhat non-trivial to do.

git-upload-pack is much more complex on the server side as you
must actually create the packfile, which means you also need to
compute the set of objects the client is missing, as well as tell
the client when you have enough "have" lines in order to compute
the smallest reasonable pack possible for the data transfer.

I know Erlang is really good at protocol handling and binary
formatting/parsing, but I don't know if you have a good git object
navigation library available to you in Erlang.  Its been years
since I last worked with it, or was involved in its community...  ;-)

-- 
Shawn.

^ permalink raw reply


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