Git development
 help / color / mirror / Atom feed
* [PATCH 1/2] diff.c: second war on whitespace.
From: Junio C Hamano @ 2006-09-23  7:47 UTC (permalink / raw)
  To: git

This adds DIFF_WHITESPACE color class (default = reverse red) to
colored diff output to let you catch common whitespace errors.

 - trailing whitespaces at the end of line
 - a space followed by a tab in the indent

Signed-off-by: Junio C Hamano <junkio@cox.net>
---
 diff.c |  172 +++++++++++++++++++++++++++++++++++++++++++++++++---------------
 diff.h |    1 
 2 files changed, 132 insertions(+), 41 deletions(-)

diff --git a/diff.c b/diff.c
index 443e248..2464238 100644
--- a/diff.c
+++ b/diff.c
@@ -20,12 +20,13 @@ static int diff_use_color_default;
 
 static char diff_colors[][COLOR_MAXLEN] = {
 	"\033[m",	/* reset */
-	"",		/* normal */
-	"\033[1m",	/* bold */
-	"\033[36m",	/* cyan */
-	"\033[31m",	/* red */
-	"\033[32m",	/* green */
-	"\033[33m"	/* yellow */
+	"",		/* PLAIN (normal) */
+	"\033[1m",	/* METAINFO (bold) */
+	"\033[36m",	/* FRAGINFO (cyan) */
+	"\033[31m",	/* OLD (red) */
+	"\033[32m",	/* NEW (green) */
+	"\033[33m",	/* COMMIT (yellow) */
+	"\033[41m",	/* WHITESPACE (red background) */
 };
 
 static int parse_diff_color_slot(const char *var, int ofs)
@@ -42,6 +43,8 @@ static int parse_diff_color_slot(const c
 		return DIFF_FILE_NEW;
 	if (!strcasecmp(var+ofs, "commit"))
 		return DIFF_COMMIT;
+	if (!strcasecmp(var+ofs, "whitespace"))
+		return DIFF_WHITESPACE;
 	die("bad config variable '%s'", var);
 }
 
@@ -383,9 +386,89 @@ const char *diff_get_color(int diff_use_
 	return "";
 }
 
+static void emit_line(const char *set, const char *reset, const char *line, int len)
+{
+	if (len > 0 && line[len-1] == '\n')
+		len--;
+	fputs(set, stdout);
+	fwrite(line, len, 1, stdout);
+	puts(reset);
+}
+
+static void emit_add_line(const char *reset, struct emit_callback *ecbdata, const char *line, int len)
+{
+	int col0 = ecbdata->nparents;
+	int last_tab_in_indent = -1;
+	int last_space_in_indent = -1;
+	int i;
+	int tail = len;
+	int need_highlight_leading_space = 0;
+	const char *ws = diff_get_color(ecbdata->color_diff, DIFF_WHITESPACE);
+	const char *set = diff_get_color(ecbdata->color_diff, DIFF_FILE_NEW);
+
+	if (!*ws) {
+		emit_line(set, reset, line, len);
+		return;
+	}
+
+	/* The line is a newly added line.  Does it have funny leading
+	 * whitespaces?  In indent, SP should never precede a TAB.
+	 */
+	for (i = col0; i < len; i++) {
+		if (line[i] == '\t') {
+			last_tab_in_indent = i;
+			if (0 <= last_space_in_indent)
+				need_highlight_leading_space = 1;
+		}
+		else if (line[i] == ' ')
+			last_space_in_indent = i;
+		else
+			break;
+	}
+	fputs(set, stdout);
+	fwrite(line, col0, 1, stdout);
+	fputs(reset, stdout);
+	if (((i == len) || line[i] == '\n') && i != col0) {
+		/* The whole line was indent */
+		emit_line(ws, reset, line + col0, len - col0);
+		return;
+	}
+	i = col0;
+	if (need_highlight_leading_space) {
+		while (i < last_tab_in_indent) {
+			if (line[i] == ' ') {
+				fputs(ws, stdout);
+				putchar(' ');
+				fputs(reset, stdout);
+			}
+			else
+				putchar(line[i]);
+			i++;
+		}
+	}
+	tail = len - 1;
+	if (line[tail] == '\n' && i < tail)
+		tail--;
+	while (i < tail) {
+		if (!isspace(line[tail]))
+			break;
+		tail--;
+	}
+	if ((i < tail && line[tail + 1] != '\n')) {
+		/* This has whitespace between tail+1..len */
+		fputs(set, stdout);
+		fwrite(line + i, tail - i + 1, 1, stdout);
+		fputs(reset, stdout);
+		emit_line(ws, reset, line + tail + 1, len - tail - 1);
+	}
+	else
+		emit_line(set, reset, line + i, len - i);
+}
+
 static void fn_out_consume(void *priv, char *line, unsigned long len)
 {
 	int i;
+	int color;
 	struct emit_callback *ecbdata = priv;
 	const char *set = diff_get_color(ecbdata->color_diff, DIFF_METAINFO);
 	const char *reset = diff_get_color(ecbdata->color_diff, DIFF_RESET);
@@ -403,45 +486,52 @@ static void fn_out_consume(void *priv, c
 		;
 	if (2 <= i && i < len && line[i] == ' ') {
 		ecbdata->nparents = i - 1;
-		set = diff_get_color(ecbdata->color_diff, DIFF_FRAGINFO);
+		emit_line(diff_get_color(ecbdata->color_diff, DIFF_FRAGINFO),
+			  reset, line, len);
+		return;
 	}
-	else if (len < ecbdata->nparents)
+
+	if (len < ecbdata->nparents) {
 		set = reset;
-	else {
-		int nparents = ecbdata->nparents;
-		int color = DIFF_PLAIN;
-		if (ecbdata->diff_words && nparents != 1)
-			/* fall back to normal diff */
-			free_diff_words_data(ecbdata);
-		if (ecbdata->diff_words) {
-			if (line[0] == '-') {
-				diff_words_append(line, len,
-						&ecbdata->diff_words->minus);
-				return;
-			} else if (line[0] == '+') {
-				diff_words_append(line, len,
-						&ecbdata->diff_words->plus);
-				return;
-			}
-			if (ecbdata->diff_words->minus.text.size ||
-					ecbdata->diff_words->plus.text.size)
-				diff_words_show(ecbdata->diff_words);
-			line++;
-			len--;
-		} else
-			for (i = 0; i < nparents && len; i++) {
-				if (line[i] == '-')
-					color = DIFF_FILE_OLD;
-				else if (line[i] == '+')
-					color = DIFF_FILE_NEW;
-			}
-		set = diff_get_color(ecbdata->color_diff, color);
+		emit_line(reset, reset, line, len);
+		return;
 	}
-	if (len > 0 && line[len-1] == '\n')
+
+	color = DIFF_PLAIN;
+	if (ecbdata->diff_words && ecbdata->nparents != 1)
+		/* fall back to normal diff */
+		free_diff_words_data(ecbdata);
+	if (ecbdata->diff_words) {
+		if (line[0] == '-') {
+			diff_words_append(line, len,
+					  &ecbdata->diff_words->minus);
+			return;
+		} else if (line[0] == '+') {
+			diff_words_append(line, len,
+					  &ecbdata->diff_words->plus);
+			return;
+		}
+		if (ecbdata->diff_words->minus.text.size ||
+		    ecbdata->diff_words->plus.text.size)
+			diff_words_show(ecbdata->diff_words);
+		line++;
 		len--;
-	fputs (set, stdout);
-	fwrite (line, len, 1, stdout);
-	puts (reset);
+		emit_line(set, reset, line, len);
+		return;
+	}
+	for (i = 0; i < ecbdata->nparents && len; i++) {
+		if (line[i] == '-')
+			color = DIFF_FILE_OLD;
+		else if (line[i] == '+')
+			color = DIFF_FILE_NEW;
+	}
+
+	if (color != DIFF_FILE_NEW) {
+		emit_line(diff_get_color(ecbdata->color_diff, color),
+			  reset, line, len);
+		return;
+	}
+	emit_add_line(reset, ecbdata, line, len);
 }
 
 static char *pprint_rename(const char *a, const char *b)
diff --git a/diff.h b/diff.h
index b60a02e..3435fe7 100644
--- a/diff.h
+++ b/diff.h
@@ -86,6 +86,7 @@ enum color_diff {
 	DIFF_FILE_OLD = 4,
 	DIFF_FILE_NEW = 5,
 	DIFF_COMMIT = 6,
+	DIFF_WHITESPACE = 7,
 };
 const char *diff_get_color(int diff_use_color, enum color_diff ix);
 
-- 
1.4.2.1.gf2bba

^ permalink raw reply related

* Keep it
From: Lindsey Currie @ 2006-09-23  7:07 UTC (permalink / raw)
  To: git; +Cc: ddissettqldf

I can redo your h0me r8te at 4.5percent or less insts.

Last 3 I've given below:

U. Caldwell / 1.2 million at 4.1
R. Abraham / 270 thousand at 4.4
S. Workman / 515 thousand at 3.9

http://phuXK.greatexpetitions.com

^ permalink raw reply

* Re: Git user survey and `git pull`
From: Matthias Urlichs @ 2006-09-22 21:05 UTC (permalink / raw)
  To: git
In-Reply-To: <20060921162401.GD3934@spearce.org>

For what it's worth, I'm in favor of renaming the things.

IMHO, we do not need that kind of baggage. Sure, you explain it once and
people hopefully remember -- except that they don't, not always anyway,
and novice users can't be expected to be notice, let alone repair, that
kind of damage.

*I* make that stupid pull/fetch/merge mistake sometimes, 
and I'm not exactly new to git...


On Thu, 21 Sep 2006 12:24:01 -0400, Shawn Pearce wrote:

>   Current            Shoulda Been
>   ---------------    ----------------
>   git-push           git-push
>   git-fetch          git-pull
>   git-pull . foo     git-merge foo
>   git-pull           git-pull --merge
>   git-merge          git-merge-driver
> 
The new programs can (for the most part) recognize when they're called with
"old" semantics, and spit out a warning.

-- 
Matthias Urlichs

^ permalink raw reply

* Re: [PATCH] Remove branch by putting a null sha1 into the ref file.
From: Christian Couder @ 2006-09-23  4:45 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Linus Torvalds, git
In-Reply-To: <7v3baj365g.fsf@assigned-by-dhcp.cox.net>

Junio C Hamano wrote:
> Linus Torvalds <torvalds@osdl.org> writes:
> > It's entirely possible that the proper way to do branch deletion with
> > packed branches is to simply re-pack without the old branch, rather
> > than the negative branch model. I couldn't really decide.
>
> After playing with branch deletion issues for some time, I
> started to think it would be a lot simpler if we do not mark
> a deleted branch with 0{40}.

I also came to the same conclusion and I wonder if we could delete a branch 
by moving the file ".git/refs/heads/frotz" 
to ".git/deleted-refs/heads/frotz".
If the branch is packed we could perhaps just create 
".git/deleted-refs/heads/frotz". 

It means everytime we are looking up a ref we have to check 
in ".git/deleted-refs/" first to see if it exists.

> But the latter falls apart if we use 0{40} convention to mark a
> deleted branch. Removing .git/refs/heads/frotz/nitfol file which
> has 0{40} and creating .git/refs/heads/frotz file resurrects
> frotz/nitfol branch that is still packed.

If we move ".git/refs/heads/frotz/nitfol" 
to ".git/deleted-refs/heads/frotz/nitfol" when we remove this ref, we only 
need to try to rmdir all subdirectories under ".git/refs/heads/frotz/" and 
then ".git/refs/heads/frotz/" to see if we can 
create ".git/refs/heads/frotz", and if we can, we won't 
resurect "frotz/nitfol" because ".git/deleted-refs/heads/frotz/nitfol" 
still exists.

> Not allowing frotz 
> branch to be created only because we had deleted frotz/nitfol
> previously is not what we want either, so at that point we need
> to repack without frotz/nitfol anyway.
>
> Which makes me think that we would better repack when removing
> any existing ref.

Yes but it may not be fast and may be corruption prone.

Christian.

^ permalink raw reply

* [PATCH] pack-refs: fix git_path() usage.
From: Junio C Hamano @ 2006-09-23  4:34 UTC (permalink / raw)
  To: git
In-Reply-To: <20060923011634.GL13132@pasky.or.cz>


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

 * A valid ref name can contain %.

 builtin-pack-refs.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin-pack-refs.c b/builtin-pack-refs.c
index 246dd63..db57fee 100644
--- a/builtin-pack-refs.c
+++ b/builtin-pack-refs.c
@@ -56,7 +56,7 @@ static void prune_ref(struct ref_to_prun
 	struct ref_lock *lock = lock_ref_sha1(r->name + 5, r->sha1, 1);
 
 	if (lock) {
-		unlink(git_path(r->name));
+		unlink(git_path("%s", r->name));
 		unlock_ref(lock);
 	}
 }
-- 
1.4.2.1.gf2bba

^ permalink raw reply related

* Re: git pull for update of netdev fails.
From: Petr Baudis @ 2006-09-23  4:18 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Johannes Schindelin, git, Junio C Hamano, Stephen Hemminger,
	Jeff Garzik
In-Reply-To: <Pine.LNX.4.64.0609200920290.4388@g5.osdl.org>

I'm joining several fibres of the thread since we are talking about the
same thing per partes and that's rather confusing.


Dear diary, on Wed, Sep 20, 2006 at 06:26:32PM CEST, I got a letter
where Linus Torvalds <torvalds@osdl.org> said that...
> Think about it. You and somebody else works on a common branch, using a 
> common source repo. When you "fetch", you want to get all the work that 
> the other person has done. But you sure as hell don't want that work to 
> overwrite your own work.
> 
> So what does git do? It notices if you have a local commit on that shared 
> branch (because it no longer fast-forwards to the other end), and it tells 
> you exactly that: it says that branch so-and-so doesn't fast-forward, and 
> refuses to overwrite it.

I'd bite here that if you commit to a branch that you also simultanously
fetch from somewhere else, you're asking for it - but I've never really
used [plain Git]'s branches so perhaps it is a blessed workflow there.
(Cogito won't let you do it.)


Dear diary, on Wed, Sep 20, 2006 at 06:33:42PM CEST, I got a letter
where Linus Torvalds <torvalds@osdl.org> said that...
> I would be ok with a "anonymous read-only" approach IF GIT ACTUALLY 
> ENFORCED IT. In other words, we could easily have a read-only clone that 
> added the "+" to all branches, but then we should also make sure that 
> nobody ever commits _anything_ in such a repo.
> 
> No merges (because you can not rely on the merge result being meaningful: 
> the sources of the merge may be "ephemeral"), no local commits (because 
> you can never "pull" any more after that, since that now becomes a merge 
> with something you can't trust any more).

Well, yes, it gets bad if you just prepend + to all branches:

Dear diary, on Wed, Sep 20, 2006 at 06:15:04PM CEST, I got a letter
where Linus Torvalds <torvalds@osdl.org> said that...
> The thing is, if you don't understand how rebasing etc destroys history, 
> you may do things like do a "git pull" or a "git merge" of a branch that 
> the other side WILL THROW AWAY! That will later result in major pain, 
> because when you then try to merge it later, you will get all kinds of 
> nasty behaviour, because the history you merged earlier no longer matches 
> the history you're now trying to merge again, and the work you merged 
> earlier is simply not there any more.

This is a very good point.

But this just means that, as others in the thread noted, there needs to
be a reliable way for marking floating branches in the public
repositories and enforcing the implications of that locally (not letting
the user to merge with those).

When I've said "seamless support for floating branches", generally what
I've had on my mind was that it's seamless for the user, that his the
one who pulls them - the developer can be required to do something small
extra to mark those as such.


When you have that, you can just:

  (i) Disallow three-way merges with +branches

  (ii) Fast-forward of +branches makes you follow the branch' rebase if
the original commit in the branch before fetching equaled your HEAD
commit, and taints your branch against committing unless you switch to
a non-+ branch (or manually untaint your branch)

  (iii) Of course enforce the fast-forward restriction for non-+
branches

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
#!/bin/perl -sp0777i<X+d*lMLa^*lN%0]dsXx++lMlN/dsM0<j]dsj
$/=unpack('H*',$_);$_=`echo 16dio\U$k"SK$/SM$n\EsN0p[lN*1
lK[d2%Sa2/d0$^Ixp"|dc`;s/\W//g;$_=pack('H*',/((..)*)$/)

^ permalink raw reply

* Re: [PATCH] Make cvsexportcommit work with filenames containing spaces.
From: Junio C Hamano @ 2006-09-23  4:17 UTC (permalink / raw)
  To: Robin Rosenberg; +Cc: git
In-Reply-To: <20060922223506.3377.34859.stgit@lathund.dewire.com>

Robin Rosenberg <robin.rosenberg@dewire.com> writes:

> Binary files are except to this so far.

Funny.  This works on my home machine (CVS 1.12.13) while it
does not pass test #2 "with spaces" on another machine that has
CVS 1.11.22.

> +. ./test-lib.sh
> +
> +export CVSROOT=$(dirname $(pwd))/cvsroot
> +export CVSWORK=$(dirname $(pwd))/cvswork

You are creating t/{cvsroot,cvswork} directories.  Do not
contaminate outside the test/trash directory your tests is
started in.

> +rm -rf $CVSROOT $CVSWORK

People's $(pwd) can contain shell $IFS characters, especially on
Cygwin.   Be careful and quote them when in doubt.

^ permalink raw reply

* Re: git pull for update of netdev fails.
From: Petr Baudis @ 2006-09-23  4:09 UTC (permalink / raw)
  To: Shawn Pearce; +Cc: Johannes Schindelin, catalin.marinas, Linus Torvalds, git
In-Reply-To: <20060923040035.GB18105@spearce.org>

Dear diary, on Sat, Sep 23, 2006 at 06:00:35AM CEST, I got a letter
where Shawn Pearce <spearce@spearce.org> said that...
> I can see the same concept of ref history being useful even for
> core git-rebase and doing it this way would also give it to StGIT
> without Catalin needing to change code.

In my StGIT tree, I don't want to have arbitrary N-days cutoff point,
I want all the patches history preserved at least as long as I carry
the patch, because it's just as valuable as the "regular" project
history to me.

> But it doesn't help git bisect.

It's not really all that clear how should bisect work in case of
multi-dimensional history. To do it sensibly, I guess you would have to
have another command like "stg commitseries" which declares "right now I
have the series (patches, their applied/unapplied state and their
ordering) in a clearly defined state" and then bisect between those.

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
#!/bin/perl -sp0777i<X+d*lMLa^*lN%0]dsXx++lMlN/dsM0<j]dsj
$/=unpack('H*',$_);$_=`echo 16dio\U$k"SK$/SM$n\EsN0p[lN*1
lK[d2%Sa2/d0$^Ixp"|dc`;s/\W//g;$_=pack('H*',/((..)*)$/)

^ permalink raw reply

* Re: git pull for update of netdev fails.
From: Shawn Pearce @ 2006-09-23  4:00 UTC (permalink / raw)
  To: Petr Baudis; +Cc: Johannes Schindelin, catalin.marinas, Linus Torvalds, git
In-Reply-To: <20060923034407.GF8259@pasky.or.cz>

Petr Baudis <pasky@suse.cz> wrote:
> Yes, I agree that this really is a problem, but that's a fundamental
> limitation. At least for StGIT-maintained floating branches the latest
> bleeding edge StGIT could fix that. (Except that the problem outlined by
> Linus is present here as well, first prune will wipe your older patch
> versions and your patch log will be useless - Catalin? Can we store the
> older patch versions references in something like
> .git/refs/patches-old/?) And except that it does that only for you -
> there should be a way to conveniently mirror (clone+pull) the patch
> stack setup.

Why not change reflog to store the last n values or the last n days
(obtained from .git/config file) as refs under refs/prior-heads ?

I can see the same concept of ref history being useful even for
core git-rebase and doing it this way would also give it to StGIT
without Catalin needing to change code.

But it doesn't help git bisect.


Of course a looooong time ago when I first implemented reflog support
this was suggested but not implemented at the time due to the high
cost per ref.  Now that we have Linus' packed refs available this
may not be nearly as much of a problem.

-- 
Shawn.

^ permalink raw reply

* Re: git pull for update of netdev fails.
From: Petr Baudis @ 2006-09-23  3:44 UTC (permalink / raw)
  To: Johannes Schindelin, catalin.marinas; +Cc: Linus Torvalds, git
In-Reply-To: <Pine.LNX.4.63.0609202304270.19042@wbgn013.biozentrum.uni-wuerzburg.de>

Dear diary, on Wed, Sep 20, 2006 at 11:14:25PM CEST, I got a letter
where Johannes Schindelin <Johannes.Schindelin@gmx.de> said that...
> Another, even more serious problems with rebasing: You can introduce a bug 
> by rebasing. Meaning: git-rebase can succeed, even compilation is fine, 
> but the sum of your patches, and the patches you are rebasing on, is 
> buggy. And there is _no_ way to bisect this, since the "good" version can 
> be gone for good.

Yes, I agree that this really is a problem, but that's a fundamental
limitation. At least for StGIT-maintained floating branches the latest
bleeding edge StGIT could fix that. (Except that the problem outlined by
Linus is present here as well, first prune will wipe your older patch
versions and your patch log will be useless - Catalin? Can we store the
older patch versions references in something like
.git/refs/patches-old/?) And except that it does that only for you -
there should be a way to conveniently mirror (clone+pull) the patch
stack setup.

> As for the problem git-rebase tries to solve: you can get a clean branch 
> by cherry-picking what you have into a temporary branch, for the sole 
> purpose of being history clean.

That's not really a reliable option because after getting your patches
through Christopher Hellwig you _will_ need to go back and poke some
patches in that history.

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
#!/bin/perl -sp0777i<X+d*lMLa^*lN%0]dsXx++lMlN/dsM0<j]dsj
$/=unpack('H*',$_);$_=`echo 16dio\U$k"SK$/SM$n\EsN0p[lN*1
lK[d2%Sa2/d0$^Ixp"|dc`;s/\W//g;$_=pack('H*',/((..)*)$/)

^ permalink raw reply

* Re: [RFC/PATCH] gitweb: Add committags support
From: Petr Baudis @ 2006-09-23  3:29 UTC (permalink / raw)
  To: Jakub Narebski; +Cc: git, Sham Chukoury
In-Reply-To: <200609212356.31806.jnareb@gmail.com>

Dear diary, on Thu, Sep 21, 2006 at 11:56:31PM CEST, I got a letter
where Jakub Narebski <jnareb@gmail.com> said that...
> Below there is preliminary (hence RFC) committag support for gitweb,
> based on the idea introduced by Sham Chukoury to gitweb-xmms2.
> 
> The code has all the possible committags I could think of enabled;
> not all are tested, though. This includes existing commitsha tag
> support (full sha links to commit view, is sha is sha of commit),
> mantis bug and feature tags from gitweb-xmms2 (there is no release
> committag of gitweb-xmms2, but it should be fairly easy to add it),
> bugzilla committag for the Linux kernel, plain text URL committag
> (probably doesn't work that well, marking as links examples, and
> sometimes protocol specification), and Message-Id committag via
> GMane git mailing list (and not only) archive -- not tested.

I think that's a good test. People will certainly want more but that can
be added over time.

> -- >8 --
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 7ed963c..5eb0dd0 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -173,6 +173,118 @@ sub feature_pickaxe {
>  	return ($_[0]);
>  }
>  
> +# You define site-wide comittags defaults here; override them with
> +# $GITWEB_CONFIG as necessary.
> +our %committags = (
> +	# 'committag' => {
> +	# 	'pattern' => regexp (use 'qr' quote-like operator)
> +	# 	'sub' => committag-sub (subroutine),
> +	# 	'enabled' => is given committag enabled,
> +	# fields below can be defined, but don't need to
> +	# 	'options' => [ default options...] (array reference),
> +	# 	'insubject' => should given committag be enabled in commit/tag subject,
> +	# 	'islink' => if the result is hyperlink,
> +	# }
> +	#
> +	# You should ensure that enabled committags cannot overlap
> +	#
> +	# The committag subroutine is called with match for pattern,
> +	# and options if they are defined. Match is replaced by return
> +	# value of committag-sub.

You should note that the pattern matches in an already HTML-escaped
text. Which is actually quite ugly especially wrt. the &nbsp;, perhaps
it would be worth considering converting the spaces after this? (The
Marc links would need fixing tho'.)

..snip..
> +	'message_id' => {
> +		'pattern' => qr/(message|msg)[- ]?id&nbsp;&lt;([^&]*)&rt;/i,
> +		'enabled' => 1,
> +		'options' => [
> +			'http://news.gmane.org/find-root.php?message_id=',
> +			\&quote_msgid_gmane ],
> +		'sub' => \&tag_msgid},

Mention quote_msgid_marc() (and the URL) in a comment so that it's not
completely unreferenced?

> +);
..snip..
> +sub quote_msgid_gmane {
> +	my $msgid = shift || return;
> +
> +	return '<'.(quotemeta $msgid).'>';
> +}

<> should be HTML-escaped (unless CGI::a() does that).

> @@ -577,18 +693,43 @@ ## which don't beling to other sections
..snip..
> -	if ($line =~ m/([0-9a-fA-F]{40})/) {
> -		my $hash_text = $1;
> -		if (git_get_type($hash_text) eq "commit") {
> -			my $link =
> -				$cgi->a({-href => href(action=>"commit", hash=>$hash_text),
> -				        -class => "text"}, $hash_text);
> -			$line =~ s/$hash_text/$link/;
> +
> +	foreach my $ct (@tags) {
> +		next unless exists $committags{$ct};

At this point, I'd complain (even die) rather than silently pass, that's
definitely a bug.

> +		my $wrap = $a_attr && %$a_attr && $committags{$ct}{'islink'};

$a_attr can't be but a hashref, that test is redundant.

> @@ -626,12 +767,13 @@ sub format_subject_html {
>  	$extra = '' unless defined($extra);
>  
>  	if (length($short) < length($long)) {
> -		return $cgi->a({-href => $href, -class => "list subject",
> -		                -title => $long},
> -		       esc_html($short) . $extra);
> +		my $a_attr = {-href => $href, -class => "list subject", -title => $long};
> +		return $cgi->a($a_attr,
> +		       format_log_line_html($short, $a_attr, @subjecttags) . $extra);
>  	} else {
> -		return $cgi->a({-href => $href, -class => "list subject"},
> -		       esc_html($long)  . $extra);
> +		my $a_attr = {-href => $href, -class => "list subject"};
> +		return $cgi->a($a_attr,
> +		       format_log_line_html($long,  $a_attr, @subjecttags) . $extra);
>  	}
>  }
>  

Subjects are often clickable and we don't want links in those.

> @@ -693,9 +835,9 @@ sub git_get_type {
>  
>  	open my $fd, "-|", git_cmd(), "cat-file", '-t', $hash or return;
>  	my $type = <$fd>;
> -	close $fd or return;
> +	close $fd or return "unknown";
>  	chomp $type;
> -	return $type;
> +	return ($type || "unknown");
>  }
>  
>  sub git_get_project_config {

> P.S. I've corrected git_get_type for comparison
>   git_get_type($hash) eq "commit"
> to work without Perl syntax errors.

Hey it's just a warning. ;-) And if you are calling git_get_type() on a
non-existing object, don't you have a problem you should better know
about? Couldn't this break git_get_refs_list()?

> @@ -1585,7 +1727,7 @@ sub git_print_log ($;%) {
>  			$empty = 0;
>  		}
>  
> -		print format_log_line_html($line) . "<br/>\n";
> +		print format_log_line_html($line, @committags) . "<br/>\n";
>  	}
>  
>  	if ($opts{'-final_empty_line'}) {

What about the tags? Or perhaps even blobs, for that matter?

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
#!/bin/perl -sp0777i<X+d*lMLa^*lN%0]dsXx++lMlN/dsM0<j]dsj
$/=unpack('H*',$_);$_=`echo 16dio\U$k"SK$/SM$n\EsN0p[lN*1
lK[d2%Sa2/d0$^Ixp"|dc`;s/\W//g;$_=pack('H*',/((..)*)$/)

^ permalink raw reply

* [PATCH 7/6] make pack data reuse compatible with both delta types
From: Nicolas Pitre @ 2006-09-23  1:25 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <Pine.LNX.4.64.0609210008360.2627@xanadu.home>

This is the missing part to git-pack-objects allowing it to reuse delta
data to/from any of the two delta types.  It can reuse delta from any 
type, and it outputs base offsets when --allow-delta-base-offset is
provided and the base is also included in the pack.  Otherwise it 
outputs base sha1 references just like it always did.

Signed-off-by: Nicolas Pitre <nico@cam.org>

---

On Thu, 21 Sep 2006, Nicolas Pitre wrote:

> Delta data reuse is possible, of course, but that will come through a
> separate patch.  I need to think about it some more in order to implement
> it as efficiently as possible.  The issue is to get back to the
> corresponding object entry given an object offset in an existing pack.

This was done with the addition of a sequence number in the revindex 
entry which is 4 additional bytes per object that has to live in memory.  
If ever this is considered too much memory usage then there is always 
the possibility of using an array of pointers to pack index entries 
instead, but then the runtime cost might be higher since the sorting and 
searching will have to dereference those pointers to get to the offset 
and endian swap it every time.  I think that memory usage should not be 
an issue in most cases, and if it is then this 4 bytes per object is 
probably not going to save much anyway.

So I think support for delta base offset is now complete.

diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 2212649..6db97b6 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -29,6 +29,7 @@ struct object_entry {
 	enum object_type type;
 	enum object_type in_pack_type;	/* could be delta */
 	unsigned long delta_size;	/* delta data size (uncompressed) */
+#define in_pack_header_size delta_size	/* only when reusing pack data */
 	struct object_entry *delta;	/* delta base object */
 	struct packed_git *in_pack; 	/* already in pack */
 	unsigned int in_pack_offset;
@@ -86,17 +87,25 @@ static int object_ix_hashsz;
  * Pack index for existing packs give us easy access to the offsets into
  * corresponding pack file where each object's data starts, but the entries
  * do not store the size of the compressed representation (uncompressed
- * size is easily available by examining the pack entry header).  We build
- * a hashtable of existing packs (pack_revindex), and keep reverse index
- * here -- pack index file is sorted by object name mapping to offset; this
- * pack_revindex[].revindex array is an ordered list of offsets, so if you
- * know the offset of an object, next offset is where its packed
- * representation ends.
+ * size is easily available by examining the pack entry header).  It is
+ * also rather expensive to find the sha1 for an object given its offset.
+ *
+ * We build a hashtable of existing packs (pack_revindex), and keep reverse
+ * index here -- pack index file is sorted by object name mapping to offset;
+ * this pack_revindex[].revindex array is a list of offset/index_nr pairs
+ * ordered by offset, so if you know the offset of an object, next offset
+ * is where its packed representation ends and the index_nr can be used to
+ * get the object sha1 from the main index.
  */
+struct revindex_entry {
+	unsigned int offset;
+	unsigned int nr;
+};
 struct pack_revindex {
 	struct packed_git *p;
-	unsigned long *revindex;
-} *pack_revindex = NULL;
+	struct revindex_entry *revindex;
+};
+static struct  pack_revindex *pack_revindex;
 static int pack_revindex_hashsz;
 
 /*
@@ -143,14 +152,9 @@ static void prepare_pack_ix(void)
 
 static int cmp_offset(const void *a_, const void *b_)
 {
-	unsigned long a = *(unsigned long *) a_;
-	unsigned long b = *(unsigned long *) b_;
-	if (a < b)
-		return -1;
-	else if (a == b)
-		return 0;
-	else
-		return 1;
+	const struct revindex_entry *a = a_;
+	const struct revindex_entry *b = b_;
+	return (a->offset < b->offset) ? -1 : (a->offset > b->offset) ? 1 : 0;
 }
 
 /*
@@ -163,25 +167,27 @@ static void prepare_pack_revindex(struct
 	int i;
 	void *index = p->index_base + 256;
 
-	rix->revindex = xmalloc(sizeof(unsigned long) * (num_ent + 1));
+	rix->revindex = xmalloc(sizeof(*rix->revindex) * (num_ent + 1));
 	for (i = 0; i < num_ent; i++) {
 		unsigned int hl = *((unsigned int *)((char *) index + 24*i));
-		rix->revindex[i] = ntohl(hl);
+		rix->revindex[i].offset = ntohl(hl);
+		rix->revindex[i].nr = i;
 	}
 	/* This knows the pack format -- the 20-byte trailer
 	 * follows immediately after the last object data.
 	 */
-	rix->revindex[num_ent] = p->pack_size - 20;
-	qsort(rix->revindex, num_ent, sizeof(unsigned long), cmp_offset);
+	rix->revindex[num_ent].offset = p->pack_size - 20;
+	rix->revindex[num_ent].nr = -1;
+	qsort(rix->revindex, num_ent, sizeof(*rix->revindex), cmp_offset);
 }
 
-static unsigned long find_packed_object_size(struct packed_git *p,
-					     unsigned long ofs)
+static struct revindex_entry * find_packed_object(struct packed_git *p,
+						  unsigned int ofs)
 {
 	int num;
 	int lo, hi;
 	struct pack_revindex *rix;
-	unsigned long *revindex;
+	struct revindex_entry *revindex;
 	num = pack_revindex_ix(p);
 	if (num < 0)
 		die("internal error: pack revindex uninitialized");
@@ -193,10 +199,10 @@ static unsigned long find_packed_object_
 	hi = num_packed_objects(p) + 1;
 	do {
 		int mi = (lo + hi) / 2;
-		if (revindex[mi] == ofs) {
-			return revindex[mi+1] - ofs;
+		if (revindex[mi].offset == ofs) {
+			return revindex + mi;
 		}
-		else if (ofs < revindex[mi])
+		else if (ofs < revindex[mi].offset)
 			hi = mi;
 		else
 			lo = mi + 1;
@@ -204,6 +210,20 @@ static unsigned long find_packed_object_
 	die("internal error: pack revindex corrupt");
 }
 
+static unsigned long find_packed_object_size(struct packed_git *p,
+					     unsigned long ofs)
+{
+	struct revindex_entry *entry = find_packed_object(p, ofs);
+	return entry[1].offset - ofs;
+}
+
+static unsigned char *find_packed_object_name(struct packed_git *p,
+					      unsigned long ofs)
+{
+	struct revindex_entry *entry = find_packed_object(p, ofs);
+	return (unsigned char *)(p->index_base + 256) + 24 * entry->nr + 4;
+}
+
 static void *delta_against(void *buf, unsigned long size, struct object_entry *entry)
 {
 	unsigned long othersize, delta_size;
@@ -249,6 +269,10 @@ static int encode_header(enum object_typ
 	return n;
 }
 
+/*
+ * we are going to reuse the existing object data as is.  make
+ * sure it is not corrupt.
+ */
 static int check_inflate(unsigned char *data, unsigned long len, unsigned long expect)
 {
 	z_stream stream;
@@ -280,32 +304,6 @@ static int check_inflate(unsigned char *
 	return st;
 }
 
-/*
- * we are going to reuse the existing pack entry data.  make
- * sure it is not corrupt.
- */
-static int revalidate_pack_entry(struct object_entry *entry, unsigned char *data, unsigned long len)
-{
-	enum object_type type;
-	unsigned long size, used;
-
-	if (pack_to_stdout)
-		return 0;
-
-	/* the caller has already called use_packed_git() for us,
-	 * so it is safe to access the pack data from mmapped location.
-	 * make sure the entry inflates correctly.
-	 */
-	used = unpack_object_header_gently(data, len, &type, &size);
-	if (!used)
-		return -1;
-	if (type == OBJ_REF_DELTA)
-		used += 20; /* skip base object name */
-	data += used;
-	len -= used;
-	return check_inflate(data, len, entry->size);
-}
-
 static int revalidate_loose_object(struct object_entry *entry,
 				   unsigned char *map,
 				   unsigned long mapsize)
@@ -339,7 +337,7 @@ static unsigned long write_object(struct
 	obj_type = entry->type;
 	if (! entry->in_pack)
 		to_reuse = 0;	/* can't reuse what we don't have */
-	else if (obj_type == OBJ_REF_DELTA)
+	else if (obj_type == OBJ_REF_DELTA || obj_type == OBJ_OFS_DELTA)
 		to_reuse = 1;	/* check_object() decided it for us */
 	else if (obj_type != entry->in_pack_type)
 		to_reuse = 0;	/* pack has delta which is unusable */
@@ -415,18 +413,38 @@ static unsigned long write_object(struct
 	}
 	else {
 		struct packed_git *p = entry->in_pack;
-		use_packed_git(p);
 
-		datalen = find_packed_object_size(p, entry->in_pack_offset);
-		buf = (char *) p->pack_base + entry->in_pack_offset;
+		if (entry->delta) {
+			obj_type = (allow_ofs_delta && entry->delta->offset) ?
+				OBJ_OFS_DELTA : OBJ_REF_DELTA;
+			reused_delta++;
+		}
+		hdrlen = encode_header(obj_type, entry->size, header);
+		sha1write(f, header, hdrlen);
+		if (obj_type == OBJ_OFS_DELTA) {
+			unsigned long ofs = entry->offset - entry->delta->offset;
+			unsigned pos = sizeof(header) - 1;
+			header[pos] = ofs & 127;
+			while (ofs >>= 7)
+				header[--pos] = 128 | (--ofs & 127);
+			sha1write(f, header + pos, sizeof(header) - pos);
+			hdrlen += sizeof(header) - pos;
+		} else if (obj_type == OBJ_REF_DELTA) {
+			sha1write(f, entry->delta->sha1, 20);
+			hdrlen += 20;
+		}
 
-		if (revalidate_pack_entry(entry, buf, datalen))
+		use_packed_git(p);
+		buf = (char *) p->pack_base
+			+ entry->in_pack_offset
+			+ entry->in_pack_header_size;
+		datalen = find_packed_object_size(p, entry->in_pack_offset)
+				- entry->in_pack_header_size;
+//fprintf(stderr, "reusing %d at %d header %d size %d\n", obj_type, entry->in_pack_offset, entry->in_pack_header_size, datalen);
+		if (!pack_to_stdout && check_inflate(buf, datalen, entry->size))
 			die("corrupt delta in pack %s", sha1_to_hex(entry->sha1));
 		sha1write(f, buf, datalen);
 		unuse_packed_git(p);
-		hdrlen = 0; /* not really */
-		if (obj_type == OBJ_REF_DELTA)
-			reused_delta++;
 		reused++;
 	}
 	if (entry->delta)
@@ -914,26 +932,64 @@ static void check_object(struct object_e
 	char type[20];
 
 	if (entry->in_pack && !entry->preferred_base) {
-		unsigned char base[20];
-		unsigned long size;
-		struct object_entry *base_entry;
+		struct packed_git *p = entry->in_pack;
+		unsigned long left = p->pack_size - entry->in_pack_offset;
+		unsigned long size, used;
+		unsigned char *buf;
+		struct object_entry *base_entry = NULL;
+
+		use_packed_git(p);
+		buf = p->pack_base;
+		buf += entry->in_pack_offset;
 
 		/* We want in_pack_type even if we do not reuse delta.
 		 * There is no point not reusing non-delta representations.
 		 */
-		check_reuse_pack_delta(entry->in_pack,
-				       entry->in_pack_offset,
-				       base, &size,
-				       &entry->in_pack_type);
+		used = unpack_object_header_gently(buf, left,
+						   &entry->in_pack_type, &size);
+		if (!used || left - used <= 20)
+			die("corrupt pack for %s", sha1_to_hex(entry->sha1));
 
 		/* Check if it is delta, and the base is also an object
 		 * we are going to pack.  If so we will reuse the existing
 		 * delta.
 		 */
-		if (!no_reuse_delta &&
-		    entry->in_pack_type == OBJ_REF_DELTA &&
-		    (base_entry = locate_object_entry(base)) &&
-		    (!base_entry->preferred_base)) {
+		if (!no_reuse_delta) {
+			unsigned char c, *base_name;
+			unsigned long ofs;
+			/* there is at least 20 bytes left in the pack */
+			switch (entry->in_pack_type) {
+			case OBJ_REF_DELTA:
+				base_name = buf + used;
+				used += 20;
+				break;
+			case OBJ_OFS_DELTA:
+				c = buf[used++];
+				ofs = c & 127;
+				while (c & 128) {
+					ofs += 1;
+					if (!ofs || ofs & ~(~0UL >> 7))
+						die("delta base offset overflow in pack for %s",
+						    sha1_to_hex(entry->sha1));
+					c = buf[used++];
+					ofs = (ofs << 7) + (c & 127);
+				}
+				if (ofs >= entry->in_pack_offset)
+					die("delta base offset out of bound for %s",
+					    sha1_to_hex(entry->sha1));
+				ofs = entry->in_pack_offset - ofs;
+				base_name = find_packed_object_name(p, ofs);
+				break;
+			default:
+				base_name = NULL;
+			}
+			if (base_name)
+				base_entry = locate_object_entry(base_name);
+		}
+		unuse_packed_git(p);
+		entry->in_pack_header_size = used;
+
+		if (base_entry && !base_entry->preferred_base) {
 
 			/* Depth value does not matter - find_deltas()
 			 * will never consider reused delta as the
@@ -942,9 +998,9 @@ static void check_object(struct object_e
 			 */
 
 			/* uncompressed size of the delta data */
-			entry->size = entry->delta_size = size;
+			entry->size = size;
 			entry->delta = base_entry;
-			entry->type = OBJ_REF_DELTA;
+			entry->type = entry->in_pack_type;
 
 			entry->delta_sibling = base_entry->delta_child;
 			base_entry->delta_child = entry;
@@ -1500,7 +1556,7 @@ int cmd_pack_objects(int argc, const cha
 			continue;
 		}
 		if (!strcmp("--delta-base-offset", arg)) {
-			allow_ofs_delta = no_reuse_delta = 1;
+			allow_ofs_delta = 1;
 			continue;
 		}
 		if (!strcmp("--stdout", arg)) {
diff --git a/pack.h b/pack.h
index 05557da..4c9bddd 100644
--- a/pack.h
+++ b/pack.h
@@ -16,7 +16,4 @@ struct pack_header {
 };
 
 extern int verify_pack(struct packed_git *, int);
-extern int check_reuse_pack_delta(struct packed_git *, unsigned long,
-				  unsigned char *, unsigned long *,
-				  enum object_type *);
 #endif
diff --git a/sha1_file.c b/sha1_file.c
index 66ebdde..e4bc4ae 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1004,25 +1004,6 @@ static unsigned long unpack_object_heade
 	return offset + used;
 }
 
-int check_reuse_pack_delta(struct packed_git *p, unsigned long offset,
-			   unsigned char *base, unsigned long *sizep,
-			   enum object_type *kindp)
-{
-	unsigned long ptr;
-	int status = -1;
-
-	use_packed_git(p);
-	ptr = offset;
-	ptr = unpack_object_header(p, ptr, kindp, sizep);
-	if (*kindp != OBJ_REF_DELTA)
-		goto done;
-	hashcpy(base, (unsigned char *) p->pack_base + ptr);
-	status = 0;
- done:
-	unuse_packed_git(p);
-	return status;
-}
-
 void packed_object_info_detail(struct packed_git *p,
 			       unsigned long offset,
 			       char *type,

^ permalink raw reply related

* Re: [PATCH] Fix buggy ref recording
From: Petr Baudis @ 2006-09-23  1:16 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <7vzmcrxvgw.fsf@assigned-by-dhcp.cox.net>

Dear diary, on Sat, Sep 23, 2006 at 02:44:31AM CEST, I got a letter
where Junio C Hamano <junkio@cox.net> said that...
> I've never seen you send out a corrupt patch over e-mail.
> What's different this time?
> 
> > diff --git a/refs.c b/refs.c
> > index 40f16af..5fdf9c4 100644
> > --- a/refs.c
> > +++ b/refs.c
> > @@ -472,7 +472,7 @@ static struct ref_lock *lock_ref_sha1_ba
> >
> >  	lock->ref_name = xstrdup(ref);
> >  	lock->log_file = xstrdup(git_path("logs/%s", ref));
> 
> The empty line at the beginning of the hunk is totally empty,
> not even with a SP to show it is a context line.

Sorry, I've cut'n'pasted from an ssh session on repo.or.cz and *thought*
that I fixed the whitespaces...

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
#!/bin/perl -sp0777i<X+d*lMLa^*lN%0]dsXx++lMlN/dsM0<j]dsj
$/=unpack('H*',$_);$_=`echo 16dio\U$k"SK$/SM$n\EsN0p[lN*1
lK[d2%Sa2/d0$^Ixp"|dc`;s/\W//g;$_=pack('H*',/((..)*)$/)

^ permalink raw reply

* Re: [PATCH] Fetch: get the remote branches to merge from the branch properties.
From: Santi @ 2006-09-23  0:53 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <7vac4rxvep.fsf@assigned-by-dhcp.cox.net>

> Your patch has a very strange whitespace damage in it, and I am
> not talking about quoted-printable encoding.

Last minute hand-editing, sorry. Did I already say it is very late here? :(

   Santi

^ permalink raw reply

* Re: [PATCH] Fetch: get the remote branches to merge from the branch properties.
From: Santi @ 2006-09-23  0:47 UTC (permalink / raw)
  To: git
In-Reply-To: <874puza2qq.fsf@gmail.com>

2006/9/23, Santi Béjar <sbejar@gmail.com>:
> If in branch "foo" and this in config:
>
> [branch "foo"]
>        merge=bar
>
> "git fetch": fetch from the default repository and program the "bar"
>              branch to be merged with pull.
>
> Signed-off-by: Santi Béjar <sbejar@gmail.com>

Just to comments (it's late here):

.- the branch.foo.merge must be the same as specified in the remotes/$remote.

.- I've already found a bug (it's late and I need to sleep, sorry).
This must only be done for the default repository (origin, or in
branch.foo.remote).

Santi

^ permalink raw reply

* Re: [PATCH] Fetch: get the remote branches to merge from the branch properties.
From: Junio C Hamano @ 2006-09-23  0:45 UTC (permalink / raw)
  To: Santi; +Cc: git
In-Reply-To: <874puza2qq.fsf@gmail.com>

> If in branch "foo" and this in config:
>
> [branch "foo"]
>        merge=3Dbar
>
> "git fetch": fetch from the default repository and program the "bar"
>              branch to be merged with pull.
>

Your patch has a very strange whitespace damage in it, and I am
not talking about quoted-printable encoding.

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index fa20e28..b4de243 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -122,6 +122,10 @@ apply.whitespace::
>  branch.<name>.remote::
>         When in branch <name>, it tells `git pull` which remote to fetc=
> h.
>

A tab in context before "When" is expanded into spaces, and the
empty line in the context does not have the leading SP.

> +branch.<name>.merge::
> +       When in branch <name>, it tells `git fetch` the remote branch t=
> o be
> +       merged.
> +

I suspect you wanted to start the new line with a leading tab
before "When" but it is expanded into spaces here.

However, the diff for the other file in the same patch does not
have this problem.

No need to resend, as I hand-applied it to a new topic, but I am
a bit curious why one diff is broken when the other one is not
in the same e-mail message.

^ permalink raw reply

* Re: [PATCH] Fix snapshot link in tree view
From: Junio C Hamano @ 2006-09-23  0:44 UTC (permalink / raw)
  To: Petr Baudis; +Cc: git
In-Reply-To: <20060922232120.596.63045.stgit@rover>

Petr Baudis <pasky@suse.cz> writes:

> It would just give HEAD snapshot instead of one of the particular tree.
>
> Perhaps we should also include snapshot in the global navbar? And perhaps
> stgit should have a way to edit the mail before sending it so that I could
> note this below the three dashes? ;-)

Personally, I do not mind having the first "Perhaps" in the log
message.

Will apply.

^ permalink raw reply

* Re: [PATCH 4/6] gitweb: Link to associated tree from a particular log item in full log view
From: Junio C Hamano @ 2006-09-23  0:44 UTC (permalink / raw)
  To: Petr Baudis; +Cc: git
In-Reply-To: <20060922231315.GD8259@pasky.or.cz>

Petr Baudis <pasky@suse.cz> writes:

> Oops. It's trivial typo:
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index d2366c7..c5f3810 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -2880,7 +2880,7 @@ sub git_log {
>  		      " | " .
>  		      $cgi->a({-href => href(action=>"commitdiff", hash=>$commit)}, "commitdiff") .
>  		      " | " .
> -		      $cgi->a({-href => href(action=>"tree", hash=>$commit), hash_base=>$commit}, "tree") .
> +		      $cgi->a({-href => href(action=>"tree", hash=>$commit, hash_base=>$commit)}, "tree") .
>  		      "<br/>\n" .
>  		      "</div>\n" .
>  		      "<i>" . esc_html($co{'author_name'}) .  " [$ad{'rfc2822'}]</i><br/>\n" .
>
>> You do not have the tree object name available in git_log to
>> generate an URL with both h and hb, and getting to it is an
>> extra work.
>
> Well using commit hash as a tree id works just fine...

I agree that change is much safer.  Will apply with an
appropriate log message and attribution.  No need to resend.

^ permalink raw reply

* [RFC/PATCH] git-diff/git-apply: make diff output a bit friendlier to GNU patch
From: Junio C Hamano @ 2006-09-23  0:44 UTC (permalink / raw)
  To: git; +Cc: Linus Torvalds

Somebody was wondering on #git channel why a git generated diff
does not apply with GNU patch when the filename contains a SP.
It is because GNU patch expects to find TAB (and trailing timestamp)
on ---/+++ (old_name and new_name) lines after the filenames.

The "diff --git" output format was carefully designed to be
compatible with GNU patch where it can, but whitespace
characters were always a pain.

This adds an extra TAB (but not trailing timestamp) to old_name
and new_name lines of git-diff output when the filename has a SP
in it.  When a filename contains a real tab, "diff --git" format
always c-quotes it as discussed on the list with GNU patch
maintainer previously:

	http://marc.theaimsgroup.com/?l=git&m=112927316408690&w=2

so there is no real room for confusion.

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

 * This patch, if people like its general direction, should be
   split into two parts, one to update "git-apply" so that it
   understands that old/new name lines may contain the extra TAB
   at the end, and another to update "git-diff" to actually
   generate such a patch.  We apply the former immediately to
   get people prepared, and when everybody's git-apply is ready
   then we can apply the latter to actually produce the new
   format.

 builtin-apply.c |    4 ++--
 diff.c          |   20 +++++++++++++++++---
 2 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/builtin-apply.c b/builtin-apply.c
index 25e90d8..84dbbc3 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -360,7 +360,7 @@ static int gitdiff_hdrend(const char *li
 static char *gitdiff_verify_name(const char *line, int isnull, char *orig_name, const char *oldnew)
 {
 	if (!orig_name && !isnull)
-		return find_name(line, NULL, 1, 0);
+		return find_name(line, NULL, 1, TERM_TAB);
 
 	if (orig_name) {
 		int len;
@@ -370,7 +370,7 @@ static char *gitdiff_verify_name(const c
 		len = strlen(name);
 		if (isnull)
 			die("git-apply: bad git-diff - expected /dev/null, got %s on line %d", name, linenr);
-		another = find_name(line, NULL, 1, 0);
+		another = find_name(line, NULL, 1, TERM_TAB);
 		if (!another || memcmp(another, name, len))
 			die("git-apply: bad git-diff - inconsistent %s filename on line %d", oldnew, linenr);
 		free(another);
diff --git a/diff.c b/diff.c
index 443e248..37a4ce1 100644
--- a/diff.c
+++ b/diff.c
@@ -201,11 +201,18 @@ static void emit_rewrite_diff(const char
 			      struct diff_filespec *two)
 {
 	int lc_a, lc_b;
+	const char *name_a_tab, *name_b_tab;
+
+	name_a_tab = strchr(name_a, ' ') ? "\t" : "";
+	name_b_tab = strchr(name_b, ' ') ? "\t" : "";
+
 	diff_populate_filespec(one, 0);
 	diff_populate_filespec(two, 0);
 	lc_a = count_lines(one->data, one->size);
 	lc_b = count_lines(two->data, two->size);
-	printf("--- %s\n+++ %s\n@@ -", name_a, name_b);
+	printf("--- %s%s\n+++ %s%s\n@@ -",
+	       name_a, name_a_tab,
+	       name_b, name_b_tab);
 	print_line_count(lc_a);
 	printf(" +");
 	print_line_count(lc_b);
@@ -391,8 +398,15 @@ static void fn_out_consume(void *priv, c
 	const char *reset = diff_get_color(ecbdata->color_diff, DIFF_RESET);
 
 	if (ecbdata->label_path[0]) {
-		printf("%s--- %s%s\n", set, ecbdata->label_path[0], reset);
-		printf("%s+++ %s%s\n", set, ecbdata->label_path[1], reset);
+		const char *name_a_tab, *name_b_tab;
+
+		name_a_tab = strchr(ecbdata->label_path[0], ' ') ? "\t" : "";
+		name_b_tab = strchr(ecbdata->label_path[1], ' ') ? "\t" : "";
+
+		printf("%s--- %s%s%s\n",
+		       set, ecbdata->label_path[0], reset, name_a_tab);
+		printf("%s+++ %s%s%s\n",
+		       set, ecbdata->label_path[1], reset, name_b_tab);
 		ecbdata->label_path[0] = ecbdata->label_path[1] = NULL;
 	}
 
-- 
1.4.2.1.gb6052

^ permalink raw reply related

* Re: [PATCH] Fix buggy ref recording
From: Junio C Hamano @ 2006-09-23  0:44 UTC (permalink / raw)
  To: Petr Baudis; +Cc: git
In-Reply-To: <20060922230845.GB8259@pasky.or.cz>

Petr Baudis <pasky@suse.cz> writes:

> And since just reporting it did not magically result in a fix... ;-)

Yes, please always send in a patch to be applied to get the
attribution right.

I've never seen you send out a corrupt patch over e-mail.
What's different this time?

> diff --git a/refs.c b/refs.c
> index 40f16af..5fdf9c4 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -472,7 +472,7 @@ static struct ref_lock *lock_ref_sha1_ba
>
>  	lock->ref_name = xstrdup(ref);
>  	lock->log_file = xstrdup(git_path("logs/%s", ref));

The empty line at the beginning of the hunk is totally empty,
not even with a SP to show it is a context line.

Will hand-apply, no need to resend.

^ permalink raw reply

* [PATCH] Fetch: get the remote branches to merge from the branch properties.
From: Santi Béjar @ 2006-09-22 23:41 UTC (permalink / raw)
  To: git

If in branch "foo" and this in config:

[branch "foo"]
       merge=bar

"git fetch": fetch from the default repository and program the "bar"
             branch to be merged with pull.

Signed-off-by: Santi Béjar <sbejar@gmail.com>
---
 Documentation/config.txt |    4 ++++
 git-parse-remote.sh      |   13 ++++++++++---
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index fa20e28..b4de243 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -122,6 +122,10 @@ apply.whitespace::
 branch.<name>.remote::
        When in branch <name>, it tells `git pull` which remote to fetch.

+branch.<name>.merge::
+       When in branch <name>, it tells `git fetch` the remote branch to be
+       merged.
+
 pager.color::
        A boolean to enable/disable colored output when the pager is in
        use (default is true).
diff --git a/git-parse-remote.sh b/git-parse-remote.sh
index 187f088..0b14606 100755
--- a/git-parse-remote.sh
+++ b/git-parse-remote.sh
@@ -86,9 +86,12 @@ get_remote_default_refs_for_push () {
 
 # Subroutine to canonicalize remote:local notation.
 canon_refs_list_for_fetch () {
-	# Leave only the first one alone; add prefix . to the rest
+	# Leave the branches in branch.${curr_branch}.merge alone,
+	# or the first one; add prefix . to the rest
 	# to prevent the secondary branches to be merged by default.
-	dot_prefix=
+	curr_branch=$(git-symbolic-ref HEAD | sed -e 's|^refs/heads/||')
+	merge_branches=$(git-repo-config --get-all "branch.${curr_branch}.merge")
+	[ -z "$merge_branches" ] && merge_branches=$1
 	for ref
 	do
 		force=
@@ -101,6 +104,11 @@ canon_refs_list_for_fetch () {
 		expr "z$ref" : 'z.*:' >/dev/null || ref="${ref}:"
 		remote=$(expr "z$ref" : 'z\([^:]*\):')
 		local=$(expr "z$ref" : 'z[^:]*:\(.*\)')
+		dot_prefix=.
+		for merge_branch in $merge_branches
+		do
+		    [ "$remote" = "$merge_branch" ] && dot_prefix= && break
+		done
 		case "$remote" in
 		'') remote=HEAD ;;
 		refs/heads/* | refs/tags/* | refs/remotes/*) ;;
@@ -120,7 +128,6 @@ canon_refs_list_for_fetch () {
 		   die "* refusing to create funny ref '$local_ref_name' locally"
 		fi
 		echo "${dot_prefix}${force}${remote}:${local}"
-		dot_prefix=.
 	done
 }
 
-- 
1.4.2.1.g4b5cd

^ permalink raw reply related

* [PATCH] Fetch: default remote repository from config
From: Santi Béjar @ 2006-09-22 23:26 UTC (permalink / raw)
  To: git


If in branch "foo" and this in config:

[branch "foo"]
        remote=bar

"git fetch" = "git fetch bar"
"git  pull" = "git pull  bar"

Signed-off-by: Santi Béjar <sbejar@gmail.com>
---
 Documentation/config.txt |    3 +++
 git-fetch.sh             |   11 ++++++-----
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index bb2fbc3..fa20e28 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -119,6 +119,9 @@ apply.whitespace::
 	Tells `git-apply` how to handle whitespaces, in the same way
 	as the '--whitespace' option. See gitlink:git-apply[1].
 
+branch.<name>.remote::
+	When in branch <name>, it tells `git pull` which remote to fetch.
+
 pager.color::
 	A boolean to enable/disable colored output when the pager is in
 	use (default is true).
diff --git a/git-fetch.sh b/git-fetch.sh
index 09a5d6c..e3b8f26 100755
--- a/git-fetch.sh
+++ b/git-fetch.sh
@@ -68,11 +68,12 @@ done
 
 case "$#" in
 0)
-	test -f "$GIT_DIR/branches/origin" ||
-		test -f "$GIT_DIR/remotes/origin" ||
-			git-repo-config --get remote.origin.url >/dev/null ||
-				die "Where do you want to fetch from today?"
-	set origin ;;
+	curr_branch=$(git-symbolic-ref HEAD | sed -e 's|^refs/heads/||')
+	origin=$(git-repo-config --get "branch.$curr_branch.remote")
+	origin=${origin:-origin}
+	test -n "$(get_remote_url ${origin})" ||
+		die "Where do you want to fetch from today?"
+	set x $origin ; shift ;;
 esac
 
 remote_nick="$1"
-- 
1.4.2.1.g4b5cd

^ permalink raw reply related

* Re: Git user survey and `git pull`
From: Santi @ 2006-09-22 23:24 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <7v4puz4t3z.fsf@assigned-by-dhcp.cox.net>

2006/9/22, Junio C Hamano <junkio@cox.net>:
> Santi <sbejar@gmail.com> writes:
>
> > Something like
> > http://marc.theaimsgroup.com/?l=git&m=115398815111209&w=2
> > ?
>
> I do not have access to marc at the moment but I have saved a
> patch from you back from July in one of my freezer mbox.  IIRC
> your message was not for inclusion but more as a firestarter for
> discussions, and the discussion never came to conclusion with
> appliable set of patches.
>

Yes, it was. But I think now will be different :)

In a moment I'll send two little patches for your two first points
(pull w/o arg + pull with just the remote).

Santi

^ permalink raw reply

* [PATCH] Fix snapshot link in tree view
From: Petr Baudis @ 2006-09-22 23:21 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

It would just give HEAD snapshot instead of one of the particular tree.

Perhaps we should also include snapshot in the global navbar? And perhaps
stgit should have a way to edit the mail before sending it so that I could
note this below the three dashes? ;-)

Signed-off-by: Petr Baudis <pasky@suse.cz>
---

 gitweb/gitweb.perl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index c5f3810..fc31ac3 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2771,7 +2771,7 @@ sub git_tree {
 		if ($have_snapshot) {
 			# FIXME: Should be available when we have no hash base as well.
 			push @views_nav,
-				$cgi->a({-href => href(action=>"snapshot")},
+				$cgi->a({-href => href(action=>"snapshot", hash=>$hash)},
 					"snapshot");
 		}
 		git_print_page_nav('tree','', $hash_base, undef, undef, join(' | ', @views_nav));

^ permalink raw reply related

* [PATCH] gitweb: Fix @git_base_url_list usage
From: Petr Baudis @ 2006-09-22 23:15 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <20060920004935.GJ8259@pasky.or.cz>

As it is now, that array was never used because the customurl accessor was
broken and ''unless @url_list'' never happenned.
---

 gitweb/gitweb.perl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index f7e7d10..a357604 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -752,7 +752,7 @@ sub git_get_project_description {
 sub git_get_project_url_list {
 	my $path = shift;
 
-	open my $fd, "$projectroot/$path/cloneurl" or return undef;
+	open my $fd, "$projectroot/$path/cloneurl" or return;
 	my @git_project_url_list = map { chomp; $_ } <$fd>;
 	close $fd;
 

^ 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