Git development
 help / color / mirror / Atom feed
* Re: [PATCH] git-merge: add option --no-ff
From: Eric Wong @ 2007-09-18  6:23 UTC (permalink / raw)
  To: Lars Hjemli
  Cc: Junio C Hamano, Andreas Ericsson, Johannes Schindelin,
	Chris Shoemaker, git
In-Reply-To: <8c5c35580709172312w55613a1bw8cc58b200c526fab@mail.gmail.com>

Lars Hjemli <hjemli@gmail.com> wrote:
> On 9/18/07, Eric Wong <normalperson@yhbt.net> wrote:
> > Would automatically enabling --no-ff when it detects merging of two (or
> > more) SVN branches be a good thing?
> 
> I'd say 'git-svn merge' as a wrapper for 'git merge --no-ff' would be cleaner.

That still involves having to get the user to use something new to avoid
shooting themselves in the foot.  Perhaps putting a
"test -d $GIT_DIR/svn" condition in front of the git-svn call I proposed
in git-merge would be alright.

If anybody else is thinking about 'git-svn rebase', this is completely
different.  Using git-rebase alone doesn't allow git-svn users to shoot
themselves in the foot like git-merge does.  'git-svn rebase' only
serves to minimize typing and brain power needed to operate git-svn.

-- 
Eric Wong

^ permalink raw reply

* Re: [PATCH] git-merge: add option --no-ff
From: Junio C Hamano @ 2007-09-18  6:53 UTC (permalink / raw)
  To: Lars Hjemli
  Cc: Eric Wong, Junio C Hamano, Andreas Ericsson, Johannes Schindelin,
	Chris Shoemaker, git
In-Reply-To: <8c5c35580709172312w55613a1bw8cc58b200c526fab@mail.gmail.com>

"Lars Hjemli" <hjemli@gmail.com> writes:

> On 9/18/07, Eric Wong <normalperson@yhbt.net> wrote:
>> Would automatically enabling --no-ff when it detects merging of two (or
>> more) SVN branches be a good thing?
>
> I'd say 'git-svn merge' as a wrapper for 'git merge --no-ff' would be cleaner.

That unfortunately does not solve the problem.

^ permalink raw reply

* Re: State of Perforce importing.
From: Simon Hausmann @ 2007-09-18  6:58 UTC (permalink / raw)
  To: David Brown; +Cc: Git
In-Reply-To: <20070917193027.GA24282@old.davidb.org>

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

On Monday 17 September 2007 21:30:28 David Brown wrote:
> I'd like to track a lot of code living in a Perforce repository, so I've
> been playing with 'git-p4.py'.  Is the one in the contrib/fast-import
> directory the latest version, or is there a better place.

This is indeed the latest version (on contrib/fast-import).

> So far, it is having a couple of problems:
>
>    - The commit comment is empty.  It doesn't seem to grab the Perforce
>      description, and the user seems to be <a@b>.

This may be a problem with the python output of perforce. Can you run the 
following command?

	git-p4 debug change <a change number in your depot>

That should print a dictionary that has a 'desc' field containing the commit 
comment/log and a 'user' field that has the perforce user name.

>    - Every revision seems to check every file out of Perforce.  This means
>      that for the directory I want, every revision is going to take about
> 20 minutes.

For every revision only every _changed_ file is retrieved (using p4 
print //path/file#revision).


Simon

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

^ permalink raw reply

* Re: commit summary, --pretty=short and other tools
From: Andreas Ericsson @ 2007-09-18  7:19 UTC (permalink / raw)
  To: Benoit SIGOURE; +Cc: Mike Hommey, git
In-Reply-To: <55887C88-8523-4839-8B91-236256A5E893@lrde.epita.fr>

Benoit SIGOURE wrote:
> 
> My opinion is that it would be better to keep the first line and never 
> ever rewrite the commit messages.
> 

I've had reason to ponder this quite a lot, as I've imported 15 repos from
CVS and SVN where the commit authors did not follow the git-recommended way
of doing things, but rather put everything as one paragraph, usually without
linebreaks, in the commit message.

>From what I've read from those rather horrid commit-messages so far, it's
usually correct to grab the first sentence in case the empty line isn't
there, so:

const char *find_commit_subject_end(const char *commit_msg)
{
	const char *dot, *paragraph_end;
	
	paragraph_end = strstr(commit_msg, "\n\n");
	dot = strchr(commit_msg, '.');
	
	return min_non_null(dot, paragraph_end); 
}

would probably get it right very nearly always.

I'll submit a patch in 3 hours when I get my lunch, unless someone
beats me to it.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

^ permalink raw reply

* Re: State of Perforce importing.
From: Sam Vilain @ 2007-09-18  7:27 UTC (permalink / raw)
  To: David Brown; +Cc: Git
In-Reply-To: <20070917193027.GA24282@old.davidb.org>

David Brown wrote:
> I'd like to track a lot of code living in a Perforce repository, so I've
> been playing with 'git-p4.py'.  Is the one in the contrib/fast-import
> directory the latest version, or is there a better place.

I'm pretty close to giving a newer one a spin, that actually imports
from the raw perforce back-end files without needing the perforce
server.  I am hoping that this should give a very clean import and will
be very fast and efficient, sending files that share ancestry to gfi in
sequence so that the on-the-fly delta system works.

If you're interested, take a look at
http://utsl.gen.nz/gitweb/?p=git-p4raw;a=summary.  Expect the commands
that say "WIP" to be rebased :-).  It requires Postgres - I haven't yet
re-written the SQL queries that step outside of MySQL's little box.

It could possibly be adapted to use the p4 client (though I'd expect
that to be relatively slow per-revision), and possibly be extended to be
bidirectional as all of the upstream change number information is
recorded, a la git-svn.

Sam.

^ permalink raw reply

* [PATCH 1/2] contrib/fast-import: add simple shell example
From: Jeff King @ 2007-09-18  7:26 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Shawn O. Pearce, Nguyen Thai Ngoc Duy

From: Nguyen Thai Ngoc Duy <pclouds@gmail.com>

This example just puts a directory under git control. It is
significantly slower than using the git tools directly, but
hopefully shows a bit how fast-import works.

  [jk: added header comments]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 contrib/fast-import/git-import.sh |   38 +++++++++++++++++++++++++++++++++++++
 1 files changed, 38 insertions(+), 0 deletions(-)
 create mode 100755 contrib/fast-import/git-import.sh

diff --git a/contrib/fast-import/git-import.sh b/contrib/fast-import/git-import.sh
new file mode 100755
index 0000000..0ca7718
--- /dev/null
+++ b/contrib/fast-import/git-import.sh
@@ -0,0 +1,38 @@
+#!/bin/sh
+#
+# Performs an initial import of a directory. This is the equivalent
+# of doing 'git init; git add .; git commit'. It's a lot slower,
+# but is meant to be a simple fast-import example.
+
+if [ -z "$1" -o -z "$2" ]; then
+	echo "Usage: git-import branch import-message"
+	exit 1
+fi
+
+USERNAME="$(git config user.name)"
+EMAIL="$(git config user.email)"
+
+if [ -z "$USERNAME" -o -z "$EMAIL" ]; then
+	echo "You need to set user name and email"
+	exit 1
+fi
+
+git init
+
+(
+	cat <<EOF
+commit refs/heads/$1
+committer $USERNAME <$EMAIL> now
+data <<MSGEOF
+$2
+MSGEOF
+
+EOF
+	find * -type f|while read i;do
+		echo "M 100644 inline $i"
+		echo data $(stat -c '%s' "$i")
+		cat "$i"
+		echo
+	done
+	echo
+) | git fast-import --date-format=now
-- 
1.5.3.1.967.g6bb01

^ permalink raw reply related

* [PATCH] contrib/fast-import: add perl version of simple example
From: Jeff King @ 2007-09-18  7:26 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Shawn O. Pearce

This is based on the git-import.sh script, but is a little
more robust and efficient. More importantly, it should
serve as a quick template for interfacing fast-import with
perl scripts.

Signed-off-by: Jeff King <peff@peff.net>
---
 contrib/fast-import/git-import.perl |   64 +++++++++++++++++++++++++++++++++++
 1 files changed, 64 insertions(+), 0 deletions(-)
 create mode 100755 contrib/fast-import/git-import.perl

diff --git a/contrib/fast-import/git-import.perl b/contrib/fast-import/git-import.perl
new file mode 100755
index 0000000..f9fef6d
--- /dev/null
+++ b/contrib/fast-import/git-import.perl
@@ -0,0 +1,64 @@
+#!/usr/bin/perl
+#
+# Performs an initial import of a directory. This is the equivalent
+# of doing 'git init; git add .; git commit'. It's a little slower,
+# but is meant to be a simple fast-import example.
+
+use strict;
+use File::Find;
+
+my $USAGE = 'Usage: git-import branch import-message';
+my $branch = shift or die "$USAGE\n";
+my $message = shift or die "$USAGE\n";
+
+chomp(my $username = `git config user.name`);
+chomp(my $email = `git config user.email`);
+die 'You need to set user name and email'
+  unless $username && $email;
+
+system('git init');
+open(my $fi, '|-', qw(git fast-import --date-format=now))
+  or die "unable to spawn fast-import: $!";
+
+print $fi <<EOF;
+commit refs/heads/$branch
+committer $username <$email> now
+data <<MSGEOF
+$message
+MSGEOF
+
+EOF
+
+find(
+  sub {
+    if($File::Find::name eq './.git') {
+      $File::Find::prune = 1;
+      return;
+    }
+    return unless -f $_;
+
+    my $fn = $File::Find::name;
+    $fn =~ s#^.\/##;
+
+    open(my $in, '<', $_)
+      or die "unable to open $fn: $!";
+    my @st = stat($in)
+      or die "unable to stat $fn: $!";
+    my $len = $st[7];
+
+    print $fi "M 644 inline $fn\n";
+    print $fi "data $len\n";
+    while($len > 0) {
+      my $r = read($in, my $buf, $len < 4096 ? $len : 4096);
+      defined($r) or die "read error from $fn: $!";
+      $r > 0 or die "premature EOF from $fn: $!";
+      print $fi $buf;
+      $len -= $r;
+    }
+    print $fi "\n";
+
+  }, '.'
+);
+
+close($fi);
+exit $?;
-- 
1.5.3.1.967.g6bb01

^ permalink raw reply related

* Re: [PATCH] contrib/fast-import: add perl version of simple example
From: Jeff King @ 2007-09-18  7:27 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Shawn O. Pearce
In-Reply-To: <20070918072627.GB3506@coredump.intra.peff.net>

On Tue, Sep 18, 2007 at 03:26:27AM -0400, Jeff King wrote:

> This is based on the git-import.sh script, but is a little

Sorry, this was meant to be 2/2 (in case you were breathless in
anticipation for the second part of the series).

-Peff

^ permalink raw reply

* [PATCH] Drop UTF-8 characters in manual pages
From: Mike Hommey @ 2007-09-18  7:26 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Mike Hommey

The default character encoding for english manual pages is ISO8859-1, so
UTF-8 characters are just displayed as their sequence of bytes, which is
not very appealing.

Signed-off-by: Mike Hommey <mh@glandium.org>
---
 Documentation/git-lost-found.txt |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/Documentation/git-lost-found.txt b/Documentation/git-lost-found.txt
index e48607f..82b0699 100644
--- a/Documentation/git-lost-found.txt
+++ b/Documentation/git-lost-found.txt
@@ -65,7 +65,7 @@ $ git rev-parse not-lost-anymore
 
 Author
 ------
-Written by Junio C Hamano 濱野 純 <junkio@cox.net>
+Written by Junio C Hamano <junkio@cox.net>
 
 Documentation
 --------------
-- 
1.5.3.1

^ permalink raw reply related

* Re: [PATCH] git-merge: add option --no-ff
From: Sam Vilain @ 2007-09-18  7:30 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Lars Hjemli, Eric Wong, Andreas Ericsson, Johannes Schindelin,
	Chris Shoemaker, git
In-Reply-To: <7v4phsxy55.fsf@gitster.siamese.dyndns.org>

Junio C Hamano wrote:
> "Lars Hjemli" <hjemli@gmail.com> writes:
> 
>> On 9/18/07, Eric Wong <normalperson@yhbt.net> wrote:
>>> Would automatically enabling --no-ff when it detects merging of two (or
>>> more) SVN branches be a good thing?
>> I'd say 'git-svn merge' as a wrapper for 'git merge --no-ff' would be cleaner.
> 
> That unfortunately does not solve the problem.

I think we 'just' need to fix pushing merges back to SVN - so that they
properly set Subversion 1.5+ (and possibly SVK) merge attributes - and
if it is ambiguous which branch to push to, force the user to decide.

Sam.

^ permalink raw reply

* Re: [PATCH 1/1] git-send-email: Add a --suppress-all option
From: Andreas Ericsson @ 2007-09-18  7:40 UTC (permalink / raw)
  To: Felipe Balbi; +Cc: git, Felipe Balbi
In-Reply-To: <11900540373215-git-send-email-felipebalbi@users.sourceforge.net>

Felipe Balbi wrote:
> From: Felipe Balbi <felipe.lima@indt.org.br>
> 
> This patch adds a --suppress-all option to avoid sending emails
> to everybody but the ones listed by --to option.
> 

To my minds eye, --suppress-all is equivalent to --dry-run. Could you
rename it to "--cc-nobody" or some such?

On a side-note, I've never really understood why git-send-email *by default*
sends to a bazillion people. Does anybody ever use it without suppressing
most of the CC targets?

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

^ permalink raw reply

* Re: [EGIT PATCH] Add feature and plugin.
From: Andreas Ericsson @ 2007-09-18  7:43 UTC (permalink / raw)
  To: Ben Konrath; +Cc: git
In-Reply-To: <20070917185310.GA6042@toast.toronto.redhat.com>

Ben Konrath wrote:
> Hi,
> 
> I made a feature and associated branding plugin for Egit. Including
> these two plugins allows us to build Egit for Fedora but it also makes
> it easy to create an update site for Egit.



> These two plugins also add an
> entry for Egit in Help -> About Eclipse -> Feature Details. 
> 

When you start writing "also" in your commit messages, it's a pretty good
sign that you should have made many smaller commits rathern than one large.

Aside from that, I don't know enough about eclipse, java or xml to be of
any assistance, although I appreciate the efforts put into this.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

^ permalink raw reply

* Re: [PATCH 0/3] the return of the strbuf
From: Junio C Hamano @ 2007-09-18  7:56 UTC (permalink / raw)
  To: Pierre Habouzit; +Cc: git
In-Reply-To: <20070917125211.GA18176@artemis.corp>

Thanks, will queue in 'pu' and hopefully soon move to 'next'.

^ permalink raw reply

* Re: [PATCH] git-merge: add option --no-ff
From: Lars Hjemli @ 2007-09-18  8:02 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Eric Wong, Andreas Ericsson, Johannes Schindelin, Chris Shoemaker,
	git
In-Reply-To: <7v4phsxy55.fsf@gitster.siamese.dyndns.org>

On 9/18/07, Junio C Hamano <gitster@pobox.com> wrote:
> "Lars Hjemli" <hjemli@gmail.com> writes:
>
> > On 9/18/07, Eric Wong <normalperson@yhbt.net> wrote:
> >> Would automatically enabling --no-ff when it detects merging of two (or
> >> more) SVN branches be a good thing?
> >
> > I'd say 'git-svn merge' as a wrapper for 'git merge --no-ff' would be cleaner.
>
> That unfortunately does not solve the problem.
>

The problem we're trying to solve is to somehow avoid fast-forward
merges between git-svn branches, right?

I don't think it's a big issue in itself. If a fast-forward occurs,
what will happen is basically that git-svn will guess the wrong
upstream branch and then proceed to do nothing [1]. The user can
always recover from this state with 'git-reset' and 'git-merge
--no-ff'. So I think the result of a fast-forward merge between
git-svn branches is annoying, but not fatal [2].

But a closely related issue is that git-svn shouldn't dcommit to the
wrong upstream (even in the case of a fast-forward merge). We need a
way to explicitly show the link between the local and remote svn
branch (something like .git/config perhaps).

-- 
larsh

[1] If the merged-in branch had local commits they will be 'dcommited'
to the correct upstream of the merged-in branch, which isn't to bad

[2] if git-svn could be fixed to handle even the ff case, someone
could actually prefer to get the 'cherry-picked' history in
subversion. I don't, hence my --no-ff patch, but I'm not at all
certain this should be _forced_ on git-svn branches.

^ permalink raw reply

* [PATCH] git-push: documentation and tests for pushing only branches
From: Jeff King @ 2007-09-18  8:15 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

Commit 098e711e caused git-push to match only branches when
considering which refs to push. This patch updates the
documentation accordingly and adds a test for this behavior.

Signed-off-by: Jeff King <peff@peff.net>
---

We had discussed this behavior in April, and I worked up an
implementation and test then. We ended up not using it, but your later
commit introduced the same behavior. So here are the supplementary
changes from my original, which still apply (I have verified that the
test goes from failure to success with 098e711e).

 Documentation/git-push.txt      |    4 ++--
 Documentation/git-send-pack.txt |    4 ++--
 t/t5400-send-pack.sh            |   10 ++++++++++
 3 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 7b8e075..6bc559d 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -48,7 +48,7 @@ even if it does not result in a fast forward update.
 Note: If no explicit refspec is found, (that is neither
 on the command line nor in any Push line of the
 corresponding remotes file---see below), then all the
-refs that exist both on the local side and on the remote
+heads that exist both on the local side and on the remote
 side are updated.
 +
 `tag <tag>` means the same as `refs/tags/<tag>:refs/tags/<tag>`.
@@ -61,7 +61,7 @@ the remote repository.
 
 \--all::
 	Instead of naming each ref to push, specifies that all
-	refs be pushed.
+	refs under `$GIT_DIR/refs/heads/` be pushed.
 
 \--tags::
 	All refs under `$GIT_DIR/refs/tags` are pushed, in
diff --git a/Documentation/git-send-pack.txt b/Documentation/git-send-pack.txt
index 205bfd2..3271e88 100644
--- a/Documentation/git-send-pack.txt
+++ b/Documentation/git-send-pack.txt
@@ -32,7 +32,7 @@ OPTIONS
 
 \--all::
 	Instead of explicitly specifying which refs to update,
-	update all refs that locally exist.
+	update all heads that locally exist.
 
 \--force::
 	Usually, the command refuses to update a remote ref that
@@ -70,7 +70,7 @@ With '--all' flag, all refs that exist locally are transferred to
 the remote side.  You cannot specify any '<ref>' if you use
 this flag.
 
-Without '--all' and without any '<ref>', the refs that exist
+Without '--all' and without any '<ref>', the heads that exist
 both on the local side and on the remote side are updated.
 
 When one or more '<ref>' are specified explicitly, it can be either a
diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh
index 6c8767e..57c6397 100755
--- a/t/t5400-send-pack.sh
+++ b/t/t5400-send-pack.sh
@@ -113,4 +113,14 @@ test_expect_success \
 	! git diff .git/refs/heads/master victim/.git/refs/heads/master
 '
 
+test_expect_success \
+	'pushing does not include non-head refs' '
+	mkdir parent && cd parent &&
+	git-init && touch file && git-add file && git-commit -m add &&
+	cd .. &&
+	git-clone parent child && cd child && git-push --all &&
+	cd ../parent &&
+	git-branch -a >branches && ! grep -q origin/master branches
+'
+
 test_done
-- 
1.5.3.1.986.g4c9eb

^ permalink raw reply related

* [PATCH] git-svnimport: Use separate arguments in the pipe for git-rev-parse
From: Matthias Urlichs @ 2007-09-18  7:47 UTC (permalink / raw)
  To: Dan Libby; +Cc: git
In-Reply-To: <200709171333.48331.danda@osc.co.cr>


Signed-Off-By: Matthias Urlichs <smurf@smurf.noris.de>
---
Please tell me whether that works for you.

Somebody else, preferably its author, can fix git-svn. ;-)

diff --git a/git-svnimport.perl b/git-svnimport.perl
index d3ad5b9..aa5b3b2 100755
--- a/git-svnimport.perl
+++ b/git-svnimport.perl
@@ -633,7 +633,7 @@ sub commit {
 
 	my $rev;
 	if($revision > $opt_s and defined $parent) {
-		open(H,"git-rev-parse --verify $parent |");
+		open(H,'-|',"git-rev-parse","--verify",$parent);
 		$rev = <H>;
 		close(H) or do {
 			print STDERR "$revision: cannot find commit '$parent'!\n";
-- 
1.5.2.5

-- 
Matthias Urlichs   |   {M:U} IT Design @ m-u-it.de   |  smurf@smurf.noris.de
Disclaimer: The quote was selected randomly. Really. | http://smurf.noris.de
 - -
"Could a being create the fifty billion galaxies, each with two hundred
 billion stars, then rejoice in the smell of burning goat flesh?"
                         [Ron Patterson]

^ permalink raw reply related

* diffcore-rename performance mode
From: Jeff King @ 2007-09-18  8:23 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

[this is a resend of some comments I made deep in another thread on
rename limiting; I wanted to get your comments, Junio, but I was afraid
you didn't see it buried in the mess. If you have already read it and
have nothing to say, just tell me to shut up.

I was able to get 1000% speedup on a (perhaps pathological) diff, and I
suspect there may be more speedups possible in the spanhash lookups. So
I think it's worth pursuing.]


Hmm. Actually, doing some profiling, it looks like about 75% of the time
is spent not in the O(n^2) comparison, but in generating the hash
fingerprints of each file.

There seems to be a serious performance problem in diffcore-rename.
There is infrastructure to cache the "cnt_data" member of each filespec,
but it never gets used because we immediately free the filespec data
after use. Oops.

With this patch:

diff --git a/diffcore-rename.c b/diffcore-rename.c
index 6bde439..531a844 100644
--- a/diffcore-rename.c
+++ b/diffcore-rename.c
@@ -362,10 +362,7 @@ void diffcore_rename(struct diff_options *options)
 			m->score = estimate_similarity(one, two,
 						       minimum_score);
 			m->name_score = basename_same(one, two);
-			diff_free_filespec_data(one);
 		}
-		/* We do not need the text anymore */
-		diff_free_filespec_data(two);
 		dst_cnt++;
 	}
 	/* cost matrix sorted by most to least similar pair */

My 20-minute diff becomes a 2-minute diff. The downside is that the
memory usage is much increased (for obvious reasons, it should increase
by the dataset size, since we are keeping pointers to the data around --
in my case, around 1G extra).  However, keeping around _just_ the
cnt_data caused only about 100M of extra memory consumption (and gave
the same performance boost).

Of course, 2 minutes for a git-status is still way too slow, so there we
might still need a limiter. It also looks like 57% of my time is spent
in spanhash_find, and another 29% in diffcore_count_changes.

The spanhash data structure is a bit confusing. At first, it looked like
we were doing a linear search for a matching hash, but it's not quite,
since we seem to start at some magic spot based on the hashval we're
looking up.

But it seems to be an array of (hash, count) pairs for each file. Is
there a reason not to use a hash table mapping hash -> count? That would
make insertion and lookup O(1), presumably at the cost of a bit more
memory (since each filespec would have the full table).

Junio, can you shed some light on that decision?

-Peff

^ permalink raw reply related

* Re: diffcore-rename performance mode
From: Junio C Hamano @ 2007-09-18  8:49 UTC (permalink / raw)
  To: Jeff King; +Cc: git
In-Reply-To: <20070918082321.GA9883@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

> There seems to be a serious performance problem in diffcore-rename.
> There is infrastructure to cache the "cnt_data" member of each filespec,
> but it never gets used because we immediately free the filespec data
> after use. Oops.
>
> With this patch:
> ...
> My 20-minute diff becomes a 2-minute diff. The downside is that the
> memory usage is much increased (for obvious reasons, it should increase
> by the dataset size, since we are keeping pointers to the data around --
> in my case, around 1G extra).

Yes, these early freeing of filespec_data() were introducd later
specifically to address the memory usage issue.

> However, keeping around _just_ the
> cnt_data caused only about 100M of extra memory consumption (and gave
> the same performance boost).

That would be an interesting and relatively low-hanging optimization.

> The spanhash data structure is a bit confusing. At first, it looked like
> we were doing a linear search for a matching hash, but it's not quite,
> since we seem to start at some magic spot based on the hashval we're
> looking up.

I think it was just a hash table with linear overflow (if your
spot is occupied by somebody else, you look for the next
available vacant spot -- works only if you do not ever delete
items from the table) but sorry, I do not recall the rationale
for picking that data structure.  I vaguely recall I did some
measurement between that and the usual "an array that is indexed
with a hash value that holds heads of linked lists" and pointer
chasing appeared quite cache-unfriendly to the point that it
actually degraded performance, but did not try very hard to
optimize it.

^ permalink raw reply

* Re: [PATCH] git-svnimport: Use separate arguments in the pipe for git-rev-parse
From: Junio C Hamano @ 2007-09-18  8:54 UTC (permalink / raw)
  To: Matthias Urlichs; +Cc: Dan Libby, git
In-Reply-To: <20070918074753.GQ31176@kiste.smurf.noris.de>

Matthias Urlichs <smurf@smurf.noris.de> writes:

> Signed-Off-By: Matthias Urlichs <smurf@smurf.noris.de>
> ---
> Please tell me whether that works for you.
>
> Somebody else, preferably its author, can fix git-svn. ;-)
> 
> diff --git a/git-svnimport.perl b/git-svnimport.perl
> index d3ad5b9..aa5b3b2 100755
> --- a/git-svnimport.perl
> +++ b/git-svnimport.perl
> @@ -633,7 +633,7 @@ sub commit {
>  
>  	my $rev;
>  	if($revision > $opt_s and defined $parent) {
> -		open(H,"git-rev-parse --verify $parent |");
> +		open(H,'-|',"git-rev-parse","--verify",$parent);
>  		$rev = <H>;
>  		close(H) or do {
>  			print STDERR "$revision: cannot find commit '$parent'!\n";

I seem to be missing the context, but please describe what
problem this fixes in the commit log message.  I guess some
people use shell metacharacters and/or SP in their branch names
and this is about that problem?

^ permalink raw reply

* Re: diffcore-rename performance mode
From: Jeff King @ 2007-09-18  8:54 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <7vsl5cwe6p.fsf@gitster.siamese.dyndns.org>

On Tue, Sep 18, 2007 at 01:49:50AM -0700, Junio C Hamano wrote:

> > However, keeping around _just_ the
> > cnt_data caused only about 100M of extra memory consumption (and gave
> > the same performance boost).
> 
> That would be an interesting and relatively low-hanging optimization.

OK, I will work up a patch. Is it worth making it configurable? Since it
is a space-time tradeoff, if you are tight on memory, it might actually
hurt performance. However, I have only looked at the numbers for my
massive data set...I can produce memory usage numbers for the kernel,
too.

> I think it was just a hash table with linear overflow (if your
> spot is occupied by somebody else, you look for the next
> available vacant spot -- works only if you do not ever delete
> items from the table) but sorry, I do not recall the rationale
> for picking that data structure.  I vaguely recall I did some
> measurement between that and the usual "an array that is indexed
> with a hash value that holds heads of linked lists" and pointer
> chasing appeared quite cache-unfriendly to the point that it
> actually degraded performance, but did not try very hard to
> optimize it.

I thought we were holding counts of hashes, in which case there _is_ no
overflow. We only care if you hit the hash fingerprint or not. But
perhaps I am mistaken...I will have to look more closely at the code.

-Peff

^ permalink raw reply

* [PATCH 0/5] More builtin-fetch fixes
From: Shawn O. Pearce @ 2007-09-18  8:54 UTC (permalink / raw)
  To: Junio C Hamano, Daniel Barkalow; +Cc: git

Yet another round of builtin-fetch fixes.  This series fixes the
latest breakage in `pu` for this topic along with an annoying
warning when using the http transport.

The third patch in the series is a behavior change for git-fetch.
The commit message discusses it in detail.  Junio and I kicked this
around on #git earlier this morning.

At this point myself and a few other experienced-with-Git coworkers
are running this builtin-fetch "in production" for all daily tasks.
We don't use *everything* that the tool supports as I did not know
about this breakage in branch.$name.merge until tonight when Junio
mentioned it, but for some of the really common cases we are quite
happy with builtin-fetch.  Especially its performance as we're
seeing speedups of 25x or more on Cygwin/Windows.

I still believe there's work yet to be done on this topic as I'm
quite sure the transfer.unpackLimit is not being honored.  I meant
to look at that tonight but wound up wasting all night and morning on
the 3rd patch of this series.  I will try to work on the unpackLimit
issue Tuesday or Wednesday this week.

-- 
Shawn.

^ permalink raw reply

* [PATCH 1/5] builtin-fetch: Don't segfault on "fetch +foo"
From: Shawn O. Pearce @ 2007-09-18  8:54 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

If we are fetching something and were configured to do a forced
fetch and have no local ref to store the fetched object into we
cannot mark the local ref as having a forced update.  Instead we
should just silently discard the + request.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 remote.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/remote.c b/remote.c
index 73a34c9..af3c46b 100644
--- a/remote.c
+++ b/remote.c
@@ -875,8 +875,7 @@ int get_fetch_map(struct ref *remote_refs,
 					 refspec->src : "HEAD");
 
 		ref_map->peer_ref = get_local_ref(refspec->dst);
-
-		if (refspec->force)
+		if (ref_map->peer_ref && refspec->force)
 			ref_map->peer_ref->force = 1;
 	}
 
-- 
1.5.3.1.1000.g7319b

^ permalink raw reply related

* [PATCH 2/5] Don't attempt to merge non-existant remotes in t5515
From: Shawn O. Pearce @ 2007-09-18  8:54 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

This was actually reverted in 756373da by Junio.  We no longer
support merging the right hand side of a fetchspec in a branch's
branch.$name.merge configuration setting as we interpret these
names as being only those published by the remote we are going to
fetch from.

The older shell based implementation of git-fetch did not report an
error when branch.$name.merge was referencing a branch that does
not exist on the remote and we are running `git fetch` for the
current branch.  The new builtin-fetch does notice this failure
and aborts the fetch, thus breaking the tests.

Junio and I kicked it around on #git earlier today and decided that
the best approach here is to error out and tell the user that their
configuration is wrong, as this is likely more user friendly than
silently ignoring the user's request.  Since the new builtin-fetch
is already issuing the error there is no code change required, we
just need to remove the bad configuration from our test.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 t/t5515-fetch-merge-logic.sh |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/t/t5515-fetch-merge-logic.sh b/t/t5515-fetch-merge-logic.sh
index 6c9cc67..31c1081 100755
--- a/t/t5515-fetch-merge-logic.sh
+++ b/t/t5515-fetch-merge-logic.sh
@@ -84,8 +84,7 @@ test_expect_success setup '
 		git config branch.br-$remote-merge.merge refs/heads/three &&
 		git config branch.br-$remote-octopus.remote $remote &&
 		git config branch.br-$remote-octopus.merge refs/heads/one &&
-		git config --add branch.br-$remote-octopus.merge two &&
-		git config --add branch.br-$remote-octopus.merge remotes/rem/three
+		git config --add branch.br-$remote-octopus.merge two
 	done
 '
 
-- 
1.5.3.1.1000.g7319b

^ permalink raw reply related

* [PATCH 3/5] Correct handling of branch.$name.merge in builtin-fetch
From: Shawn O. Pearce @ 2007-09-18  8:54 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

My prior bug fix for git-push titled "Don't configure remote "." to
fetch everything to itself" actually broke t5520 as we were unable
to evaluate a branch configuration of:

  [branch "copy"]
    remote = .
    merge = refs/heads/master

as remote "." did not have a "remote...fetch" configuration entry to
offer up refs/heads/master as a possible candidate available to be
fetched and merged.  In shell script git-fetch and prior to the above
mentioned commit this was hardcoded for a url of "." to be the set of
local branches.

Chasing down this bug led me to the conclusion that our prior behavior
with regards to branch.$name.merge was incorrect.  In the shell script
based git-fetch implementation we only fetched and merged a branch if
it appeared both in branch.$name.merge *and* in remote.$r.fetch, where
$r = branch.$name.remote.  In other words in the following config file:

  [remote "origin"]
    url = git://git.kernel.org/pub/scm/git/git.git
    fetch = refs/heads/master:refs/remotes/origin/master
  [branch "master"]
    remote = origin
    merge = refs/heads/master
  [branch "pu"]
    remote = origin
    merge = refs/heads/pu

Attempting to run `git pull` while on branch "pu" would always give
the user "Already up-to-date" as git-fetch did not fetch pu and thus
did not mark it for merge in .git/FETCH_HEAD.  The configured merge
would always be ignored and the user would be left scratching her
confused head wondering why merge did not work on "pu" but worked
fine on "master".

If we are using the "default fetch" specification for the current
branch and the current branch has a branch.$name.merge configured
we now union it with the list of refs in remote.$r.fetch.  This
way the above configuration does what the user expects it to do,
which is to fetch only "master" by default but when on "pu" to
fetch both "master" and "pu".

This uncovered some breakage in the test suite where old-style Cogito
branches (.git/branches/$r) did not fetch the branches listed in
.git/config for merging and thus did not actually merge them if the
user tried to use `git pull` on that branch.  Junio and I discussed
it on list and felt that the union approach here makes more sense to
DWIM for the end-user than silently ignoring their configured request
so the test vectors for t5515 have been updated to include for-merge
lines in .git/FETCH_HEAD where they have been configured for-merge
in .git/config.

Since we are now performing a union of the fetch specification and
the merge specification and we cannot allow a branch to be listed
twice (otherwise it comes out twice in .git/FETCH_HEAD) we need to
perform a double loop here over all of the branch.$name.merge lines
and try to set their merge flag if we have already schedule that
branch for fetching by remote.$r.fetch.  If no match is found then
we must add new specifications to fetch the branch but not store it
as no local tracking branch has been designated.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 builtin-fetch.c                                    |   48 ++++++++++++++-----
 remote.c                                           |   13 ++---
 remote.h                                           |    3 +-
 t/t5515/fetch.br-branches-default-merge            |    1 +
 ...etch.br-branches-default-merge_branches-default |    1 +
 t/t5515/fetch.br-branches-default-octopus          |    2 +
 ...ch.br-branches-default-octopus_branches-default |    2 +
 t/t5515/fetch.br-branches-one-merge                |    1 +
 t/t5515/fetch.br-branches-one-merge_branches-one   |    1 +
 t/t5515/fetch.br-branches-one-octopus              |    1 +
 t/t5515/fetch.br-branches-one-octopus_branches-one |    1 +
 11 files changed, 51 insertions(+), 23 deletions(-)

diff --git a/builtin-fetch.c b/builtin-fetch.c
index 20926e0..670af0b 100644
--- a/builtin-fetch.c
+++ b/builtin-fetch.c
@@ -28,20 +28,37 @@ static void unlock_pack_on_signal(int signo)
 	raise(signo);
 }
 
-static void find_merge_config(struct ref *ref_map, struct remote *remote)
+static void add_merge_config(struct ref **head,
+		           struct ref *remote_refs,
+		           struct branch *branch,
+		           struct ref ***tail)
 {
-	struct ref *rm = ref_map;
-	struct branch *branch = branch_get(NULL);
+	int i;
 
-	for (rm = ref_map; rm; rm = rm->next) {
-		if (!branch_has_merge_config(branch)) {
-			if (remote && remote->fetch &&
-			    !strcmp(remote->fetch[0].src, rm->name))
-				rm->merge = 1;
-		} else {
-			if (branch_merges(branch, rm->name))
+	for (i = 0; i < branch->merge_nr; i++) {
+		struct ref *rm, **old_tail = *tail;
+		struct refspec refspec;
+
+		for (rm = *head; rm; rm = rm->next) {
+			if (branch_merge_matches(branch, i, rm->name)) {
 				rm->merge = 1;
+				break;
+			}
 		}
+		if (rm)
+			continue;
+
+		/* Not fetched to a tracking branch?  We need to fetch
+		 * it anyway to allow this branch's "branch.$name.merge"
+		 * to be honored by git-pull.
+		 */
+		refspec.src = branch->merge[i]->src;
+		refspec.dst = NULL;
+		refspec.pattern = 0;
+		refspec.force = 0;
+		get_fetch_map(remote_refs, &refspec, tail);
+		for (rm = *old_tail; rm; rm = rm->next)
+			rm->merge = 1;
 	}
 }
 
@@ -76,17 +93,22 @@ static struct ref *get_ref_map(struct transport *transport,
 	} else {
 		/* Use the defaults */
 		struct remote *remote = transport->remote;
-		if (remote->fetch_refspec_nr) {
+		struct branch *branch = branch_get(NULL);
+		int has_merge = branch_has_merge_config(branch);
+		if (remote && (remote->fetch_refspec_nr || has_merge)) {
 			for (i = 0; i < remote->fetch_refspec_nr; i++) {
 				get_fetch_map(remote_refs, &remote->fetch[i], &tail);
 				if (remote->fetch[i].dst &&
 				    remote->fetch[i].dst[0])
 					*autotags = 1;
+				if (!i && !has_merge && ref_map &&
+				    !strcmp(remote->fetch[0].src, ref_map->name))
+					ref_map->merge = 1;
 			}
-			find_merge_config(ref_map, remote);
+			if (has_merge)
+				add_merge_config(&ref_map, remote_refs, branch, &tail);
 		} else {
 			ref_map = get_remote_ref(remote_refs, "HEAD");
-
 			ref_map->merge = 1;
 		}
 	}
diff --git a/remote.c b/remote.c
index af3c46b..31e2b70 100644
--- a/remote.c
+++ b/remote.c
@@ -772,16 +772,13 @@ int branch_has_merge_config(struct branch *branch)
 	return branch && !!branch->merge;
 }
 
-int branch_merges(struct branch *branch, const char *refname)
+int branch_merge_matches(struct branch *branch,
+		                 int i,
+		                 const char *refname)
 {
-	int i;
-	if (!branch)
+	if (!branch || i < 0 || i >= branch->merge_nr)
 		return 0;
-	for (i = 0; i < branch->merge_nr; i++) {
-		if (ref_matches_abbrev(branch->merge[i]->src, refname))
-			return 1;
-	}
-	return 0;
+	return ref_matches_abbrev(branch->merge[i]->src, refname);
 }
 
 static struct ref *get_expanded_map(struct ref *remote_refs,
diff --git a/remote.h b/remote.h
index 8994052..b5b558f 100644
--- a/remote.h
+++ b/remote.h
@@ -88,7 +88,6 @@ struct branch {
 struct branch *branch_get(const char *name);
 
 int branch_has_merge_config(struct branch *branch);
-
-int branch_merges(struct branch *branch, const char *refname);
+int branch_merge_matches(struct branch *, int n, const char *);
 
 #endif
diff --git a/t/t5515/fetch.br-branches-default-merge b/t/t5515/fetch.br-branches-default-merge
index 828bfd8..ca2cc1d 100644
--- a/t/t5515/fetch.br-branches-default-merge
+++ b/t/t5515/fetch.br-branches-default-merge
@@ -1,5 +1,6 @@
 # br-branches-default-merge
 754b754407bf032e9a2f9d5a9ad05ca79a6b228f	not-for-merge	branch 'master' of ../
+0567da4d5edd2ff4bb292a465ba9e64dcad9536b		branch 'three' of ../
 754b754407bf032e9a2f9d5a9ad05ca79a6b228f	not-for-merge	tag 'tag-master' of ../
 8e32a6d901327a23ef831511badce7bf3bf46689	not-for-merge	tag 'tag-one' of ../
 22feea448b023a2d864ef94b013735af34d238ba	not-for-merge	tag 'tag-one-tree' of ../
diff --git a/t/t5515/fetch.br-branches-default-merge_branches-default b/t/t5515/fetch.br-branches-default-merge_branches-default
index f148673..7d947cd 100644
--- a/t/t5515/fetch.br-branches-default-merge_branches-default
+++ b/t/t5515/fetch.br-branches-default-merge_branches-default
@@ -1,5 +1,6 @@
 # br-branches-default-merge branches-default
 754b754407bf032e9a2f9d5a9ad05ca79a6b228f	not-for-merge	branch 'master' of ../
+0567da4d5edd2ff4bb292a465ba9e64dcad9536b		branch 'three' of ../
 754b754407bf032e9a2f9d5a9ad05ca79a6b228f	not-for-merge	tag 'tag-master' of ../
 8e32a6d901327a23ef831511badce7bf3bf46689	not-for-merge	tag 'tag-one' of ../
 22feea448b023a2d864ef94b013735af34d238ba	not-for-merge	tag 'tag-one-tree' of ../
diff --git a/t/t5515/fetch.br-branches-default-octopus b/t/t5515/fetch.br-branches-default-octopus
index bb1a191..ec39c54 100644
--- a/t/t5515/fetch.br-branches-default-octopus
+++ b/t/t5515/fetch.br-branches-default-octopus
@@ -1,5 +1,7 @@
 # br-branches-default-octopus
 754b754407bf032e9a2f9d5a9ad05ca79a6b228f	not-for-merge	branch 'master' of ../
+8e32a6d901327a23ef831511badce7bf3bf46689		branch 'one' of ../
+6134ee8f857693b96ff1cc98d3e2fd62b199e5a8		branch 'two' of ../
 754b754407bf032e9a2f9d5a9ad05ca79a6b228f	not-for-merge	tag 'tag-master' of ../
 8e32a6d901327a23ef831511badce7bf3bf46689	not-for-merge	tag 'tag-one' of ../
 22feea448b023a2d864ef94b013735af34d238ba	not-for-merge	tag 'tag-one-tree' of ../
diff --git a/t/t5515/fetch.br-branches-default-octopus_branches-default b/t/t5515/fetch.br-branches-default-octopus_branches-default
index 970fc93..6bf42e2 100644
--- a/t/t5515/fetch.br-branches-default-octopus_branches-default
+++ b/t/t5515/fetch.br-branches-default-octopus_branches-default
@@ -1,5 +1,7 @@
 # br-branches-default-octopus branches-default
 754b754407bf032e9a2f9d5a9ad05ca79a6b228f	not-for-merge	branch 'master' of ../
+8e32a6d901327a23ef831511badce7bf3bf46689		branch 'one' of ../
+6134ee8f857693b96ff1cc98d3e2fd62b199e5a8		branch 'two' of ../
 754b754407bf032e9a2f9d5a9ad05ca79a6b228f	not-for-merge	tag 'tag-master' of ../
 8e32a6d901327a23ef831511badce7bf3bf46689	not-for-merge	tag 'tag-one' of ../
 22feea448b023a2d864ef94b013735af34d238ba	not-for-merge	tag 'tag-one-tree' of ../
diff --git a/t/t5515/fetch.br-branches-one-merge b/t/t5515/fetch.br-branches-one-merge
index 24099fd..b4b3b35 100644
--- a/t/t5515/fetch.br-branches-one-merge
+++ b/t/t5515/fetch.br-branches-one-merge
@@ -1,5 +1,6 @@
 # br-branches-one-merge
 8e32a6d901327a23ef831511badce7bf3bf46689	not-for-merge	branch 'one' of ../
+0567da4d5edd2ff4bb292a465ba9e64dcad9536b		branch 'three' of ../
 754b754407bf032e9a2f9d5a9ad05ca79a6b228f	not-for-merge	tag 'tag-master' of ../
 8e32a6d901327a23ef831511badce7bf3bf46689	not-for-merge	tag 'tag-one' of ../
 22feea448b023a2d864ef94b013735af34d238ba	not-for-merge	tag 'tag-one-tree' of ../
diff --git a/t/t5515/fetch.br-branches-one-merge_branches-one b/t/t5515/fetch.br-branches-one-merge_branches-one
index e4b4fde..2ecef38 100644
--- a/t/t5515/fetch.br-branches-one-merge_branches-one
+++ b/t/t5515/fetch.br-branches-one-merge_branches-one
@@ -1,5 +1,6 @@
 # br-branches-one-merge branches-one
 8e32a6d901327a23ef831511badce7bf3bf46689	not-for-merge	branch 'one' of ../
+0567da4d5edd2ff4bb292a465ba9e64dcad9536b		branch 'three' of ../
 754b754407bf032e9a2f9d5a9ad05ca79a6b228f	not-for-merge	tag 'tag-master' of ../
 8e32a6d901327a23ef831511badce7bf3bf46689	not-for-merge	tag 'tag-one' of ../
 22feea448b023a2d864ef94b013735af34d238ba	not-for-merge	tag 'tag-one-tree' of ../
diff --git a/t/t5515/fetch.br-branches-one-octopus b/t/t5515/fetch.br-branches-one-octopus
index 53fe808..96e3029 100644
--- a/t/t5515/fetch.br-branches-one-octopus
+++ b/t/t5515/fetch.br-branches-one-octopus
@@ -1,5 +1,6 @@
 # br-branches-one-octopus
 8e32a6d901327a23ef831511badce7bf3bf46689		branch 'one' of ../
+6134ee8f857693b96ff1cc98d3e2fd62b199e5a8		branch 'two' of ../
 754b754407bf032e9a2f9d5a9ad05ca79a6b228f	not-for-merge	tag 'tag-master' of ../
 8e32a6d901327a23ef831511badce7bf3bf46689	not-for-merge	tag 'tag-one' of ../
 22feea448b023a2d864ef94b013735af34d238ba	not-for-merge	tag 'tag-one-tree' of ../
diff --git a/t/t5515/fetch.br-branches-one-octopus_branches-one b/t/t5515/fetch.br-branches-one-octopus_branches-one
index 41b18ff..55e0bad 100644
--- a/t/t5515/fetch.br-branches-one-octopus_branches-one
+++ b/t/t5515/fetch.br-branches-one-octopus_branches-one
@@ -1,5 +1,6 @@
 # br-branches-one-octopus branches-one
 8e32a6d901327a23ef831511badce7bf3bf46689		branch 'one' of ../
+6134ee8f857693b96ff1cc98d3e2fd62b199e5a8		branch 'two' of ../
 754b754407bf032e9a2f9d5a9ad05ca79a6b228f	not-for-merge	tag 'tag-master' of ../
 8e32a6d901327a23ef831511badce7bf3bf46689	not-for-merge	tag 'tag-one' of ../
 22feea448b023a2d864ef94b013735af34d238ba	not-for-merge	tag 'tag-one-tree' of ../
-- 
1.5.3.1.1000.g7319b

^ permalink raw reply related

* [PATCH 4/5] Avoid printing unnecessary warnings during fetch and push
From: Shawn O. Pearce @ 2007-09-18  8:54 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

If a transport doesn't support an option we already are telling
the higher level application (fetch or push) that the option is not
valid by sending back a >0 return value from transport_set_option
so there's not a strong motivation to have the function perform the
output itself.  Instead we should let the higher level application
do the output if it is necessary.  This avoids always telling the
user that depth isn't supported on HTTP urls even when they did
not pass a --depth option to git-fetch.

If the user passes an option and the option value is invalid we now
properly die in git-fetch instead of just spitting out a message
and running anyway.  This mimics prior behavior better where
incorrect/malformed options are not accepted by the process.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 builtin-fetch.c |   18 +++++++++++++++---
 transport.c     |   11 ++---------
 2 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/builtin-fetch.c b/builtin-fetch.c
index 670af0b..b9722e5 100644
--- a/builtin-fetch.c
+++ b/builtin-fetch.c
@@ -430,6 +430,17 @@ static int do_fetch(struct transport *transport,
 	return 0;
 }
 
+static void set_option(const char *name, const char *value)
+{
+	int r = transport_set_option(transport, name, value);
+	if (r < 0)
+		die("Option \"%s\" value \"%s\" is not valid for %s\n",
+			name, value, transport->url);
+	if (r > 0)
+		warning("Option \"%s\" is ignored for %s\n",
+			name, transport->url);
+}
+
 int cmd_fetch(int argc, const char **argv, const char *prefix)
 {
 	struct remote *remote;
@@ -525,10 +536,11 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 	if (quiet)
 		transport->verbose = 0;
 	if (upload_pack)
-		transport_set_option(transport, TRANS_OPT_UPLOADPACK, upload_pack);
+		set_option(TRANS_OPT_UPLOADPACK, upload_pack);
 	if (keep)
-		transport_set_option(transport, TRANS_OPT_KEEP, "yes");
-	transport_set_option(transport, TRANS_OPT_DEPTH, depth);
+		set_option(TRANS_OPT_KEEP, "yes");
+	if (depth)
+		set_option(TRANS_OPT_DEPTH, depth);
 
 	if (!transport->url)
 		die("Where do you want to fetch from today?");
diff --git a/transport.c b/transport.c
index 7f94d30..cc76e3f 100644
--- a/transport.c
+++ b/transport.c
@@ -460,16 +460,9 @@ struct transport *transport_get(struct remote *remote, const char *url)
 int transport_set_option(struct transport *transport,
 			 const char *name, const char *value)
 {
-	int ret = 1;
 	if (transport->ops->set_option)
-		ret = transport->ops->set_option(transport, name, value);
-	if (ret < 0)
-		fprintf(stderr, "For '%s' option %s cannot be set to '%s'\n",
-			transport->url, name, value);
-	if (ret > 0)
-		fprintf(stderr, "For '%s' option %s is ignored\n",
-			transport->url, name);
-	return ret;
+		return transport->ops->set_option(transport, name, value);
+	return 1;
 }
 
 int transport_push(struct transport *transport,
-- 
1.5.3.1.1000.g7319b

^ permalink raw reply related


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