Git development
 help / color / mirror / Atom feed
* Re: absurdly slow git-diff
From: Linus Torvalds @ 2008-11-07 23:57 UTC (permalink / raw)
  To: Davide Libenzi, Junio C Hamano
  Cc: Abhijit Menon-Sen, Pierre Habouzit, Git Mailing List
In-Reply-To: <alpine.DEB.1.10.0811071547080.8736@alien.or.mcafeemobile.com>



On Fri, 7 Nov 2008, Davide Libenzi wrote:
> 
> Yeah, similar. Mine is below. There's one less branch in the for loops.

..and has a comment and made the magic constant be named.

Junio, the time difference is quite big for Abhijit's admittedly odd 
test-case:

 - Before:

	[torvalds@nehalem slow-diff]$ time git diff 1 2 > out.old
	
	real	2m19.912s
	user	2m19.885s
	sys	0m0.024s

 - After:

	[torvalds@nehalem slow-diff]$ time ~/git/git diff 1 2 > out

	real	0m0.841s
	user	0m0.816s
	sys	0m0.024s

with no difference in output.

			Linus

^ permalink raw reply

* Re: absurdly slow git-diff
From: Davide Libenzi @ 2008-11-07 23:48 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Abhijit Menon-Sen, Pierre Habouzit, Git Mailing List
In-Reply-To: <alpine.LFD.2.00.0811071540340.3468@nehalem.linux-foundation.org>

On Fri, 7 Nov 2008, Linus Torvalds wrote:

> 
> 
> On Fri, 7 Nov 2008, Davide Libenzi wrote:
> > 
> > With +/- 100 lines (200 lines window):
> > 
> > davide@alien:~$ time ./xdiff_test --diff 1 2 > /dev/null 
> > 
> > real    0m1.534s
> > user    0m1.466s
> > sys     0m0.040s
> 
> I assume the patch is something like the appended?
> 
> 		Linus
> 
> ---
>  xdiff/xprepare.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xdiff/xprepare.c b/xdiff/xprepare.c
> index e87ab57..4bebd76 100644
> --- a/xdiff/xprepare.c
> +++ b/xdiff/xprepare.c
> @@ -318,7 +318,7 @@ static int xdl_clean_mmatch(char const *dis, long i, long s, long e) {
>  	 * Note that we always call this function with dis[i] > 1, so the
>  	 * current line (i) is already a multimatch line.
>  	 */
> -	for (r = 1, rdis0 = 0, rpdis0 = 1; (i - r) >= s; r++) {
> +	for (r = 1, rdis0 = 0, rpdis0 = 1; r < 100 && (i - r) >= s; r++) {
>  		if (!dis[i - r])
>  			rdis0++;
>  		else if (dis[i - r] == 2)
> @@ -334,7 +334,7 @@ static int xdl_clean_mmatch(char const *dis, long i, long s, long e) {
>  	 */
>  	if (rdis0 == 0)
>  		return 0;
> -	for (r = 1, rdis1 = 0, rpdis1 = 1; (i + r) <= e; r++) {
> +	for (r = 1, rdis1 = 0, rpdis1 = 1; r < 100 && (i + r) <= e; r++) {
>  		if (!dis[i + r])
>  			rdis1++;
>  		else if (dis[i + r] == 2)
> 

Yeah, similar. Mine is below. There's one less branch in the for loops.



- Davide


diff --git a/xdiff/xprepare.c b/xdiff/xprepare.c
index deba25a..3ebd87c 100644
--- a/xdiff/xprepare.c
+++ b/xdiff/xprepare.c
@@ -23,10 +23,9 @@
 #include "xinclude.h"
 
 
-
 #define XDL_KPDIS_RUN 4
 #define XDL_MAX_EQLIMIT 1024
-
+#define XDL_SIMSCAN_WINDOWN 100
 
 
 typedef struct s_xdlclass {
@@ -246,6 +245,18 @@ static int xdl_clean_mmatch(char const *dis, long i, long s, long e) {
 	long r, rdis0, rpdis0, rdis1, rpdis1;
 
 	/*
+	 * Limits the window the is examined during the similar-lines
+	 * scan. The loops below stops when dis[i - r] == 1 (line that
+	 * has no match), but there are corner cases where the loop
+	 * proceed all the way to the extremities by causing huge
+	 * performance penalties in case of big files.
+	 */
+	if (i - s > XDL_SIMSCAN_WINDOWN)
+		s = i - XDL_SIMSCAN_WINDOWN;
+	if (e - i > XDL_SIMSCAN_WINDOWN)
+		e = i + XDL_SIMSCAN_WINDOWN;
+
+	/*
 	 * Scans the lines before 'i' to find a run of lines that either
 	 * have no match (dis[j] == 0) or have multiple matches (dis[j] > 1).
 	 * Note that we always call this function with dis[i] > 1, so the

^ permalink raw reply related

* Re: [PATCH 2/3] Introduce rename factorization in diffcore.
From: Junio C Hamano @ 2008-11-07 23:43 UTC (permalink / raw)
  To: Yann Dirson; +Cc: git
In-Reply-To: <20081107221224.GK5158@nan92-1-81-57-214-146.fbx.proxad.net>

Yann Dirson <ydirson@altern.org> writes:

> I hope I just miss your point.  Letting unaware tools handle such a
> patch "the right way" would imply just adding the information "dir foo
> moved to bar", and not removing the individual file moves, which goes
> in the way of the exact reason why I have started to work on this.

If your change is to move a/{1,2,3} to b/{1,2,3} and without content
change to a/{1,2} to b/{1,2}, then do you want to say "a/ moved to b/
and by the way here is the content change from a/3 to b/3" without saying
anything about a/{1,2} and b/{1,2}?

Two points.

 * I do not think it is a good idea to begin with.  If you are to apply
   such a patch (with git-apply that is updated with your patch to
   understand that notation) to the exact tree that has only {1,2,3} under
   a/, you would get an expected result.  But if the recipient of your
   patch has a/4 (or lacks a/2), there is no cue in the patch that
   automatically moving a/4 to b/4 may or may not be what is sane (or the
   patch is unapplicable in general).

 * If you give at least the names of paths that were moved without any
   content changes as I suggested, at least the recipient of your patch
   can catch the case where his tree is structurally different from what
   you used to prepare the patch for by noticing the a/2 in the patch that
   he does not have.

In addition, if you keep the movements for the paths whose contents did
not change, existing tools are perfectly capable of applying (or showing)
the output.  I seriously doubt that keeping 4 lines per perfectly moved
paths is too much a price to pay to keep backward compatibility.

> Compare this to the addition of the "file rename" feature (correct me
> if I'm wrong): it was added without bothering whether plain old
> "patch" can deal with it,...

Sorry, but that's an old history whie git-diff output format was rapidly
being developed, when we did not have that many users, and when we did not
have an old version of git-apply that did not understand the new feature
in majority of user's hands.

We do not have that kind of luxury anymore.  git is much more widespread
now and the majority of people use pre-1.6.1 git now (including me ;-)).

^ permalink raw reply

* Re: absurdly slow git-diff
From: Linus Torvalds @ 2008-11-07 23:42 UTC (permalink / raw)
  To: Davide Libenzi; +Cc: Abhijit Menon-Sen, Pierre Habouzit, Git Mailing List
In-Reply-To: <alpine.DEB.1.10.0811071517280.8736@alien.or.mcafeemobile.com>



On Fri, 7 Nov 2008, Davide Libenzi wrote:
> 
> With +/- 100 lines (200 lines window):
> 
> davide@alien:~$ time ./xdiff_test --diff 1 2 > /dev/null 
> 
> real    0m1.534s
> user    0m1.466s
> sys     0m0.040s

I assume the patch is something like the appended?

		Linus

---
 xdiff/xprepare.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/xdiff/xprepare.c b/xdiff/xprepare.c
index e87ab57..4bebd76 100644
--- a/xdiff/xprepare.c
+++ b/xdiff/xprepare.c
@@ -318,7 +318,7 @@ static int xdl_clean_mmatch(char const *dis, long i, long s, long e) {
 	 * Note that we always call this function with dis[i] > 1, so the
 	 * current line (i) is already a multimatch line.
 	 */
-	for (r = 1, rdis0 = 0, rpdis0 = 1; (i - r) >= s; r++) {
+	for (r = 1, rdis0 = 0, rpdis0 = 1; r < 100 && (i - r) >= s; r++) {
 		if (!dis[i - r])
 			rdis0++;
 		else if (dis[i - r] == 2)
@@ -334,7 +334,7 @@ static int xdl_clean_mmatch(char const *dis, long i, long s, long e) {
 	 */
 	if (rdis0 == 0)
 		return 0;
-	for (r = 1, rdis1 = 0, rpdis1 = 1; (i + r) <= e; r++) {
+	for (r = 1, rdis1 = 0, rpdis1 = 1; r < 100 && (i + r) <= e; r++) {
 		if (!dis[i + r])
 			rdis1++;
 		else if (dis[i + r] == 2)

^ permalink raw reply related

* Re: absurdly slow git-diff
From: Davide Libenzi @ 2008-11-07 23:18 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Abhijit Menon-Sen, Pierre Habouzit, Git Mailing List
In-Reply-To: <alpine.DEB.1.10.0811071503120.8736@alien.or.mcafeemobile.com>

On Fri, 7 Nov 2008, Davide Libenzi wrote:

> On Fri, 7 Nov 2008, Linus Torvalds wrote:
> 
> > On Sat, 8 Nov 2008, Abhijit Menon-Sen wrote:
> > >
> > > If anyone's interested, the files are http://toroid.org/misc/1 and
> > > http://toroid.org/misc/2
> > 
> > Btw, you can see this by just doing
> > 
> > 	git diff 1 2
> > 
> > without even doing "git init" or doing any actual git repository.
> > 
> > > Does anyone understand why this slowdown might happen or have
> > > suggestions about where I should look for it?
> > 
> > Sure. It's actually fairly simple. You're hitting a O(n^2) thing (possibly 
> > higher), and it's triggered by the fact that almost all your lines are 
> > identical, ie you have a file that basically has 40,000 lines of each of
> > 
> > 	xxxx: xxx, xx xxx xxxx xx:xx:xx +xxxx
> > 	xx: xxxx xxxxxxxxxxx <xxxx@xxxx.xxx>
> > 	xxxx: xxxx xxxxxxxxxxx <xxxx@xxxx.xxx>
> > 
> > and 30,000 of 
> > 
> > 	* xxxxx xxxxx (xxxxxx.xxxx xxx xxxxx (\Xxxxxx) xxxxxx.xxxxxx {xxx}
> > 
> > with a smattering of others. And this is a case where the internal git 
> > implementation does really badly. And nobody has really cared before, 
> > because nobody has ever had a case that mattered.
> > 
> > There's a number of different 'diff' algorithms, and it looks like GNU 
> > diff has one that avoids the O(n^2) case for this case.
> > 
> > I'm adding Davide as the original author of the diff library to the cc. 
> > I'm also adding Pierre, since he was talking about trying to implement
> > another diff algorithm (although I'm not at all sure that the patience 
> > diff really would help this case at all).
> 
> That should be an easy fix. Just need to limit the window by which 
> xdl_clean_mmatch() scans the current position.

With +/- 100 lines (200 lines window):

davide@alien:~$ time ./xdiff_test --diff 1 2 > /dev/null 

real    0m1.534s
user    0m1.466s
sys     0m0.040s



- Davide

^ permalink raw reply

* Re: [PATCH 1/2] diffcore-rename: support rename cache
From: Junio C Hamano @ 2008-11-07 23:17 UTC (permalink / raw)
  To: Yann Dirson; +Cc: Nguy???n Thái Ng???c Duy, git
In-Reply-To: <20081107222128.GB4030@nan92-1-81-57-214-146.fbx.proxad.net>

Yann Dirson <ydirson@altern.org> writes:

> On Fri, Nov 07, 2008 at 09:35:32PM +0700, Nguy???n Thái Ng???c Duy wrote:
>> This patch teaches diffcore_rename() to look into
>> $GIT_DIR/rename-cache and make use of it to recreate diff_filepair.
>> With proper cache, there should be no available entry for estimation
>> after exact matching.
>
> This is something I have thought about in the past, good to see that
> implemented :)
>
>> Rename caching is per commit. I don't think abitrary tree-tree caching
>> is worth it.
>
> That could be a nice complement to my directory-rename patch.

Has anybody thought about interaction between that caching and pathspec
limited operation?

^ permalink raw reply

* Re: [RFC PATCH 0/4] deny push to current branch of non-bare repo
From: Junio C Hamano @ 2008-11-07 23:16 UTC (permalink / raw)
  To: Jeff King; +Cc: git, Sam Vilain
In-Reply-To: <20081107220730.GA15942@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

> The FAQ even says "don't do this until you know what you are doing." So
> the safety valve is configurable, so that those who know what they are
> doing can switch it off.

"We are breaking your existing working setup but you can add a new
configuration to unbreak it" should not be done lightly.  I think as the
end result it is a reasonable thing to aim for for this particular
feature, but we do need a transition plan patch in between that introduces
a step that warns but not forbids.  We can ship 1.6.1 with it and then
switch the default to forbid in 1.6.3, for example.

> Patch 4/4 is the interesting one. 1/4 is a cleanup I saw while fixing
> tests. 2/4 is a cleanup to prepare for 3/4. And 3/4 fixes a bunch of
> tests which were inadvertently doing such a push (but didn't care
> because they didn't look at the working directory).

I wonder if you can use the tests 3/4 touches as the test for your "keep
existing setup" configuration variable, pretending that they are old
timer's repositories?

^ permalink raw reply

* Re: [PATCH] checkout: Don't crash when switching away from an invalid branch.
From: Junio C Hamano @ 2008-11-07 23:06 UTC (permalink / raw)
  To: Alexandre Julliard; +Cc: Johannes Schindelin, git
In-Reply-To: <87od0r9nnj.fsf@wine.dyndns.org>

Alexandre Julliard <julliard@winehq.org> writes:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> ...
>> Why not
>> 		old_desc ? old_desc : "(invalid)"
>> ?
>
> IMO it looks more friendly to not display a branch that doesn't exist,
> rather than printing something like (invalid) or (null).

Actually I think it is a good idea to remind that you were in a funny
state.

For that matter, dying without removing the trace of that funny state
might be even preferrable if you need to do postmortem to figure out why
you got into such a funny state to begin with, but not everybody uses git
to debug git.  I think Dscho's suggestion is a reasonable middle ground.

^ permalink raw reply

* Re: absurdly slow git-diff
From: Davide Libenzi @ 2008-11-07 23:04 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Abhijit Menon-Sen, Pierre Habouzit, Git Mailing List
In-Reply-To: <alpine.LFD.2.00.0811071335010.3468@nehalem.linux-foundation.org>

On Fri, 7 Nov 2008, Linus Torvalds wrote:

> On Sat, 8 Nov 2008, Abhijit Menon-Sen wrote:
> >
> > If anyone's interested, the files are http://toroid.org/misc/1 and
> > http://toroid.org/misc/2
> 
> Btw, you can see this by just doing
> 
> 	git diff 1 2
> 
> without even doing "git init" or doing any actual git repository.
> 
> > Does anyone understand why this slowdown might happen or have
> > suggestions about where I should look for it?
> 
> Sure. It's actually fairly simple. You're hitting a O(n^2) thing (possibly 
> higher), and it's triggered by the fact that almost all your lines are 
> identical, ie you have a file that basically has 40,000 lines of each of
> 
> 	xxxx: xxx, xx xxx xxxx xx:xx:xx +xxxx
> 	xx: xxxx xxxxxxxxxxx <xxxx@xxxx.xxx>
> 	xxxx: xxxx xxxxxxxxxxx <xxxx@xxxx.xxx>
> 
> and 30,000 of 
> 
> 	* xxxxx xxxxx (xxxxxx.xxxx xxx xxxxx (\Xxxxxx) xxxxxx.xxxxxx {xxx}
> 
> with a smattering of others. And this is a case where the internal git 
> implementation does really badly. And nobody has really cared before, 
> because nobody has ever had a case that mattered.
> 
> There's a number of different 'diff' algorithms, and it looks like GNU 
> diff has one that avoids the O(n^2) case for this case.
> 
> I'm adding Davide as the original author of the diff library to the cc. 
> I'm also adding Pierre, since he was talking about trying to implement
> another diff algorithm (although I'm not at all sure that the patience 
> diff really would help this case at all).

That should be an easy fix. Just need to limit the window by which 
xdl_clean_mmatch() scans the current position.



- Davide

^ permalink raw reply

* Re: [KORG] Master backend downtime - hardware swap
From: J.H. @ 2008-11-07 23:00 UTC (permalink / raw)
  To: users, git, linux-kernel, ftpadmin
In-Reply-To: <49139D4C.2010004@kernel.org>

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Just a heads up to everyone, ran slightly over (rsync took longer to
verify the move than I expected).  But we are moved over to the much
bigger and faster new master.kernel.org server.

For those curious it's a:
HP DL380 G5
	2 x Intel Quad Core Xeon's
	16G ram
	Pile more disk (including an external MSA chassis)

Thanks go out to HP for the new hardware, and their continued support of
Kernel.org!!!

If there are problems, issues or concerns please report them to
ftpadmin@kernel.org, a quick look around seems to indicate everything
looks good however.

- - John 'Warthog9' Hawley
Chief Kernel.org Administrator

J.H. wrote:
> Hey everyone,
> 
> Just a heads up to everyone, I'm going to be taking the master (hera)
> backend server offline for a couple of hours tomorrow (Friday, CST)
> afternoon.  Downtime is going to get used for finishing flipping over
> the master to new hardware, which was very generously donated by
> Hewlett-Packard.  We are moving up to a new DL380 G5.  That said I will
> need time to shut everything down, get a current rsync from the old
> hardware to the new hardware and confirm that serves are restored on the
> new machine.
> 
> I'm expecting the entire downtime to only take a couple of hours, but
> since we all know things will take longer I'm going to declare the
> window from 1200 CST (1800 UTC Friday Nov 7th).  Expect the window to
> run through 2000 UTC, and if things are up and running before that I'll
> let people know.  Otherwise if there are problems you'll see another
> e-mail from me.
> 
> If you have questions, comments, concerns, etc - please send them to me
> CC ftpadmin@kernel.org
> 
> - John 'Warthog9' Hawley
> Chief Kernel.org Administrator
- --
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (GNU/Linux)
Comment: Using GnuPG with Remi - http://enigmail.mozdev.org

iD8DBQFJFMip/E3kyWU9dicRAv4aAJ4l6dSodeAT6eGx9sb0wpsv44KY0gCePJsD
gNdijp6x/oEHXwkxkbqxWB8=
=CWoY
-----END PGP SIGNATURE-----

^ permalink raw reply

* Re: [PATCH 2/3] Introduce rename factorization in diffcore.
From: Yann Dirson @ 2008-11-07 22:12 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <7vskq3fdgz.fsf@gitster.siamese.dyndns.org>

On Fri, Nov 07, 2008 at 01:11:40PM -0800, Junio C Hamano wrote:
> Yann Dirson <ydirson@altern.org> writes:
> 
> > On Fri, Nov 07, 2008 at 12:19:14PM -0800, Junio C Hamano wrote:
> >> I am afraid that this is totally unacceptable, as you yourself mentioned,
> >> the end result is unapplicable with any existing tool and would confuse
> >> viewers like gitk and gitweb.
> >
> > Well, other tools will still have to be taught about a new syntax, if
> > they're going to use the new flag - just like it was for --rename.
> 
> You are mistaken.  For a patch, you are dealing with two different
> parties: producer and consumer.  If you are adding new feature to the
> producer, the output format should be desigend to allow the consumer tell
> that it is something it does not know how to handle.

Agreed.

> Marking a non patch with "diff --git" to trigger the logic to signal the
> beginning of a patch to git-apply (and perhaps other tools) is a non
> starter.

This is not what I meant.

> And for this "we are giving a patch that your git-apply can apply and gitk
> can show, but by the way we also think the whole directory foo moved to
> new location bar" is merely an additional information.  You should still
> be able to apply the patch with tools that are unaware of this new
> directory move detection feature.

I hope I just miss your point.  Letting unaware tools handle such a
patch "the right way" would imply just adding the information "dir foo
moved to bar", and not removing the individual file moves, which goes
in the way of the exact reason why I have started to work on this.

Compare this to the addition of the "file rename" feature (correct me
if I'm wrong): it was added without bothering whether plain old
"patch" can deal with it, and when feeding a git diff to patch(1) I
cannot expect it to DTRT (indeed it applies the content change but not
the rename part, without complaining, and an unsuspecting user would
just be shot in the foot): we are precisely *not* able to apply the
patch with tools that are unaware of this new file rename feature.

This is precisely to limit this problem in the future that I proposed
this "diff extension" stuff in my last mail: limit how the "backward
compatibility" argument can cripple innovation.

^ permalink raw reply

* Re: [RFC PATCH 0/4] deny push to current branch of non-bare repo
From: Mark Burton @ 2008-11-07 22:39 UTC (permalink / raw)
  To: git
In-Reply-To: <20081107220730.GA15942@coredump.intra.peff.net>

Jeff King <peff <at> peff.net> writes:

> 
> The short of it is that it's dangerous, we see people confused by it
> (there was another one just yesterday), and it's a FAQ:
> 
>   http://git.or.cz/gitwiki/GitFaq#head-b96f48bc9c925074be9f95c0fce69bcece5f6e73
> 
> The FAQ even says "don't do this until you know what you are doing." So
> the safety valve is configurable, so that those who know what they are
> doing can switch it off.

When I first tried to use git I was bitten by exactly this problem. I know,
RTFM, but when everything is new, it's easy to undervalue the words of wisdom
when you don't understand the bigger picture and the rational behind the advice.

I now happily work with non-bare repositories on my main machine that I push to
from my satellite development machines but, of course, I don't push to the head
branches but, instead, to remote branches and then merge on the main machine.

I wouldn't have wasted as much time getting my head around this if git had
refused to accept the push to the current branch but, instead, issued a suitable
message telling me I probably didn't want to be doing that.

So, from my own experience, I would say this would be a good feature to add.

Cheers,

Mark

^ permalink raw reply

* [PATCH 4/4] receive-pack: deny push to current branch of non-bare repo
From: Jeff King @ 2008-11-07 22:28 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Sam Vilain
In-Reply-To: <20081107220730.GA15942@coredump.intra.peff.net>

Pushing into the currently checked out branch of a non-bare
repository can be dangerous; the HEAD then loses sync with
the index and working tree, and it looks in the receiving
repo as if the pushed changes have been reverted in the
index (since they were never there in the first place).

This patch adds a safety valve that checks for this
condition and denies the push. We trigger the check only on
a non-bare repository, since a bare does not have a working
tree (and in fact, pushing to the HEAD branch is a common
workflow for publishing repositories).

This behavior is still configurable, though, since some very
specific setups may want to allow such a push if they know
they will take action to reconcile the working tree and HEAD
afterwards (e.g., a post-receive hook that does "git reset
--hard").

Signed-off-by: Jeff King <peff@peff.net>
---
My feeling is that this is dangerous behavior that we see new users
confused by, so it is worth addressing. The other obvious route is to
at least _try_ the merge, and if it comes out cleanly, to allow it.

But it looks like Sam is promoting that as a hook, which makes a lot
more sense to me. And we can still support that, but the user of the
hook must now not only install the hook, but also set the config value.

I am open to comments on the name of the config value. Somebody at the
GitTogether suggested (possibly under the influence of beer) that it be
receive.PEBKAC (since you should only turn it off if you really know
what you're doing, you would set PEBKAC to "false"), but I didn't want
to give the impression that git wasn't user-friendly. ;)

One final issue: do we need to make a special exception for "branch yet
to be born"? I believe we do so for the analagous "fetch" situation.

 Documentation/config.txt |    8 ++++++++
 builtin-receive-pack.c   |   16 ++++++++++++++++
 t/t5516-fetch-push.sh    |   21 +++++++++++++++++++++
 3 files changed, 45 insertions(+), 0 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 965ed74..971f01e 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1198,6 +1198,14 @@ receive.denyNonFastForwards::
 	even if that push is forced. This configuration variable is
 	set when initializing a shared repository.
 
+receive.denyCurrentBranch::
+	If set to true, receive-pack will deny a ref update to the
+	currently checked out branch of a non-bare repository. Such a
+	push is potentially dangerous because it brings the HEAD out of
+	sync with the index and working tree; only set this to "false"
+	if you know what you are doing (e.g., you have a post-receive
+	hook which resets the working tree). Defaults to "true".
+
 transfer.unpackLimit::
 	When `fetch.unpackLimit` or `receive.unpackLimit` are
 	not set, the value of this variable is used instead.
diff --git a/builtin-receive-pack.c b/builtin-receive-pack.c
index 7f9f134..06ad545 100644
--- a/builtin-receive-pack.c
+++ b/builtin-receive-pack.c
@@ -13,6 +13,7 @@ static const char receive_pack_usage[] = "git-receive-pack <git-dir>";
 
 static int deny_deletes = 0;
 static int deny_non_fast_forwards = 0;
+static int deny_current_branch = 1;
 static int receive_fsck_objects;
 static int receive_unpack_limit = -1;
 static int transfer_unpack_limit = -1;
@@ -49,6 +50,11 @@ static int receive_pack_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
+	if (!strcmp(var, "receive.denycurrentbranch")) {
+		deny_current_branch = git_config_bool(var, value);
+		return 0;
+	}
+
 	return git_default_config(var, value, cb);
 }
 
@@ -186,6 +192,16 @@ static const char *update(struct command *cmd)
 		return "funny refname";
 	}
 
+	if (deny_current_branch && !is_bare_repository()) {
+		unsigned char sha1[20];
+		const char *head = resolve_ref("HEAD", sha1, 0, NULL);
+		if (!strcmp(head, name)) {
+			error("refusing to update checked out branch: %s",
+				name);
+			return "branch is currently checked out";
+		}
+	}
+
 	if (!is_null_sha1(new_sha1) && !has_sha1_file(new_sha1)) {
 		error("unpack should have generated %s, "
 		      "but I can't find it!", sha1_to_hex(new_sha1));
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 7070171..579c3d8 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -487,4 +487,25 @@ test_expect_success 'allow deleting an invalid remote ref' '
 
 '
 
+test_expect_success 'deny push to HEAD to non-bare repository' '
+	mk_test heads/master
+	(cd testrepo && git checkout master) &&
+	test_must_fail git push testrepo master
+'
+
+test_expect_success 'allow push to HEAD of bare repository' '
+	mk_test heads/master
+	(cd testrepo && git checkout master && git config core.bare true) &&
+	git push testrepo master
+'
+
+test_expect_success 'allow push to HEAD of non-bare repository w/ config' '
+	mk_test heads/master
+	(cd testrepo &&
+		git checkout master &&
+		git config receive.denyCurrentBranch false
+	) &&
+	git push testrepo master
+'
+
 test_done
-- 
1.6.0.3.866.gc189b

^ permalink raw reply related

* Re: [PATCH] checkout: Don't crash when switching away from an invalid branch.
From: Alexandre Julliard @ 2008-11-07 22:28 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: git
In-Reply-To: <alpine.DEB.1.00.0811071903300.30769@pacific.mpi-cbg.de>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> diff --git a/builtin-checkout.c b/builtin-checkout.c
>> index 57b94d2..7c1b8cd 100644
>> --- a/builtin-checkout.c
>> +++ b/builtin-checkout.c
>> @@ -47,7 +47,8 @@ static int post_checkout_hook(struct commit *old, struct commit *new,
>>  
>>  	memset(&proc, 0, sizeof(proc));
>>  	argv[0] = name;
>> -	argv[1] = xstrdup(sha1_to_hex(old->object.sha1));
>> +	argv[1] = old ? xstrdup(sha1_to_hex(old->object.sha1))
>> +		      : "0000000000000000000000000000000000000000";
>
> I guess you want to use the variable null_sha1 here.

I could, though it seemed to me a bit silly to format and strdup a
string that is a known constant. But I'm happy to change it if needed.

>> @@ -492,10 +493,13 @@ static void update_refs_for_switch(struct checkout_opts *opts,
>>  	}
>>  
>>  	old_desc = old->name;
>> -	if (!old_desc)
>> +	if (!old_desc && old->commit)
>>  		old_desc = sha1_to_hex(old->commit->object.sha1);
>> -	strbuf_addf(&msg, "checkout: moving from %s to %s",
>> -		    old_desc, new->name);
>> +	if (old_desc)
>> +		strbuf_addf(&msg, "checkout: moving from %s to %s",
>> +			    old_desc, new->name);
>> +	else
>> +		strbuf_addf(&msg, "checkout: moving to %s", new->name);
>
> Why not
> 		old_desc ? old_desc : "(invalid)"
> ?

IMO it looks more friendly to not display a branch that doesn't exist,
rather than printing something like (invalid) or (null).

-- 
Alexandre Julliard
julliard@winehq.org

^ permalink raw reply

* Re: git-push and $GIT_DIR/branches
From: Junio C Hamano @ 2008-11-07 22:27 UTC (permalink / raw)
  To: Martin Koegler; +Cc: git
In-Reply-To: <20081107204451.GA10394@auto.tuwien.ac.at>

mkoegler@auto.tuwien.ac.at (Martin Koegler) writes:

> I asume, that you [and probably many other] are not very interrested
> in this feature. But would you accept a patch changing the push
> refspec to HEAD:refs/heads/<head>?

I believe "git-push foo" with .git/branches/foo has worked the way it does
now from day one, so the above change you suggest breaks peoples'
expectations and documented behaviour, _if_ people are using "branches"
with git-push.  It definitely is a no-go to just change the behaviour
without warning and deprecation period, but with a careful transition
plan, I personally do not think there is a fundamental reason not to
improve it (others may disagree).

I do not know what Cogito did when you told it to push with such a
configuration, though.  Its "branches" had quite different semantics from
the concept of "remote" in git.

^ permalink raw reply

* [PATCH 3/4] tests: avoid pushing to current branch of non-bare repo
From: Jeff King @ 2008-11-07 22:22 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Sam Vilain
In-Reply-To: <20081107220730.GA15942@coredump.intra.peff.net>

Many tests create a new repo, and then push into the master
branch (sometimes after making some commits on that branch).
After such a push the index and working tree of the
receiving repo are out of sync with the HEAD. This isn't a
problem for most tests, since they don't bother looking at
the working tree after such a push.  But this is generally a
dangerous behavior, and the tests would break if we later
decided to put in a safety valve.

Depending on the situation, this patch takes one of two
approaches:

  - creates the pushed-to repo as a bare repository. This
    works if we don't actually want to create our own
    commits in the repo.

  - switches the pushed-to repo to another branch before
    pushing. Since we never look at the working tree after
    the push anyway, this doesn't impact the test results.

Signed-off-by: Jeff King <peff@peff.net>
---
This is not the _most_ minimal patch, since when changing a non-bare
repo to a bare one, the name of the git dir changed (e.g.,
s{victim/.git}{victim}), causing a lot of textual changes. We could
technically call the bare clone "victim/.git", but I think this is less
confusing (if a bit harder to read the diff).

 t/t5400-send-pack.sh        |   30 ++++++++++++----------
 t/t5401-update-hooks.sh     |   58 +++++++++++++++++++++---------------------
 t/t5405-send-pack-rewind.sh |    3 +-
 t/t5516-fetch-push.sh       |    3 +-
 t/t5517-push-mirror.sh      |    2 +-
 5 files changed, 50 insertions(+), 46 deletions(-)

diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh
index da69f08..6bcb4df 100755
--- a/t/t5400-send-pack.sh
+++ b/t/t5400-send-pack.sh
@@ -31,7 +31,7 @@ test_expect_success setup '
 	    parent=$commit || return 1
 	done &&
 	git update-ref HEAD "$commit" &&
-	git clone ./. victim &&
+	git clone --bare ./. victim &&
 	cd victim &&
 	git log &&
 	cd .. &&
@@ -68,7 +68,7 @@ test_expect_success 'pack the destination repository' '
 test_expect_success \
         'pushing rewound head should not barf but require --force' '
 	# should not fail but refuse to update.
-	if git send-pack ./victim/.git/ master
+	if git send-pack ./victim/ master
 	then
 		# now it should fail with Pasky patch
 		echo >&2 Gaah, it should have failed.
@@ -77,7 +77,7 @@ test_expect_success \
 		echo >&2 Thanks, it correctly failed.
 		true
 	fi &&
-	if cmp victim/.git/refs/heads/master .git/refs/heads/master
+	if cmp victim/refs/heads/master .git/refs/heads/master
 	then
 		# should have been left as it was!
 		false
@@ -85,8 +85,8 @@ test_expect_success \
 		true
 	fi &&
 	# this should update
-	git send-pack --force ./victim/.git/ master &&
-	cmp victim/.git/refs/heads/master .git/refs/heads/master
+	git send-pack --force ./victim/ master &&
+	cmp victim/refs/heads/master .git/refs/heads/master
 '
 
 test_expect_success \
@@ -94,14 +94,14 @@ test_expect_success \
 	cd victim &&
 	git branch extra master &&
 	cd .. &&
-	test -f victim/.git/refs/heads/extra &&
-	git send-pack ./victim/.git/ :extra master &&
-	! test -f victim/.git/refs/heads/extra
+	test -f victim/refs/heads/extra &&
+	git send-pack ./victim/ :extra master &&
+	! test -f victim/refs/heads/extra
 '
 
 unset GIT_CONFIG GIT_CONFIG_LOCAL
 HOME=`pwd`/no-such-directory
-export HOME ;# this way we force the victim/.git/config to be used.
+export HOME ;# this way we force the victim/config to be used.
 
 test_expect_success \
 	'pushing a delete should be denied with denyDeletes' '
@@ -109,10 +109,10 @@ test_expect_success \
 	git config receive.denyDeletes true &&
 	git branch extra master &&
 	cd .. &&
-	test -f victim/.git/refs/heads/extra &&
-	test_must_fail git send-pack ./victim/.git/ :extra master
+	test -f victim/refs/heads/extra &&
+	test_must_fail git send-pack ./victim/ :extra master
 '
-rm -f victim/.git/refs/heads/extra
+rm -f victim/refs/heads/extra
 
 test_expect_success \
         'pushing with --force should be denied with denyNonFastforwards' '
@@ -120,14 +120,15 @@ test_expect_success \
 	git config receive.denyNonFastforwards true &&
 	cd .. &&
 	git update-ref refs/heads/master master^ || return 1
-	git send-pack --force ./victim/.git/ master && return 1
-	! test_cmp .git/refs/heads/master victim/.git/refs/heads/master
+	git send-pack --force ./victim/ master && return 1
+	! test_cmp .git/refs/heads/master victim/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 &&
+	git checkout -b otherbranch &&
 	cd .. &&
 	git clone parent child && cd child && git push --all &&
 	cd ../parent &&
@@ -139,6 +140,7 @@ rewound_push_setup() {
 	mkdir parent && cd parent &&
 	git init && echo one >file && git add file && git commit -m one &&
 	echo two >file && git commit -a -m two &&
+	git checkout -b otherbranch
 	cd .. &&
 	git clone parent child && cd child && git reset --hard HEAD^
 }
diff --git a/t/t5401-update-hooks.sh b/t/t5401-update-hooks.sh
index 64f66c9..ae1aa77 100755
--- a/t/t5401-update-hooks.sh
+++ b/t/t5401-update-hooks.sh
@@ -17,22 +17,22 @@ test_expect_success setup '
 	commit1=$(echo modify | git commit-tree $tree1 -p $commit0) &&
 	git update-ref refs/heads/master $commit0 &&
 	git update-ref refs/heads/tofail $commit1 &&
-	git clone ./. victim &&
-	GIT_DIR=victim/.git git update-ref refs/heads/tofail $commit1 &&
+	git clone --bare ./. victim &&
+	GIT_DIR=victim git update-ref refs/heads/tofail $commit1 &&
 	git update-ref refs/heads/master $commit1 &&
 	git update-ref refs/heads/tofail $commit0
 '
 
-cat >victim/.git/hooks/pre-receive <<'EOF'
+cat >victim/hooks/pre-receive <<'EOF'
 #!/bin/sh
 printf %s "$@" >>$GIT_DIR/pre-receive.args
 cat - >$GIT_DIR/pre-receive.stdin
 echo STDOUT pre-receive
 echo STDERR pre-receive >&2
 EOF
-chmod u+x victim/.git/hooks/pre-receive
+chmod u+x victim/hooks/pre-receive
 
-cat >victim/.git/hooks/update <<'EOF'
+cat >victim/hooks/update <<'EOF'
 #!/bin/sh
 echo "$@" >>$GIT_DIR/update.args
 read x; printf %s "$x" >$GIT_DIR/update.stdin
@@ -40,77 +40,77 @@ echo STDOUT update $1
 echo STDERR update $1 >&2
 test "$1" = refs/heads/master || exit
 EOF
-chmod u+x victim/.git/hooks/update
+chmod u+x victim/hooks/update
 
-cat >victim/.git/hooks/post-receive <<'EOF'
+cat >victim/hooks/post-receive <<'EOF'
 #!/bin/sh
 printf %s "$@" >>$GIT_DIR/post-receive.args
 cat - >$GIT_DIR/post-receive.stdin
 echo STDOUT post-receive
 echo STDERR post-receive >&2
 EOF
-chmod u+x victim/.git/hooks/post-receive
+chmod u+x victim/hooks/post-receive
 
-cat >victim/.git/hooks/post-update <<'EOF'
+cat >victim/hooks/post-update <<'EOF'
 #!/bin/sh
 echo "$@" >>$GIT_DIR/post-update.args
 read x; printf %s "$x" >$GIT_DIR/post-update.stdin
 echo STDOUT post-update
 echo STDERR post-update >&2
 EOF
-chmod u+x victim/.git/hooks/post-update
+chmod u+x victim/hooks/post-update
 
 test_expect_success push '
-	test_must_fail git send-pack --force ./victim/.git \
+	test_must_fail git send-pack --force ./victim \
 		master tofail >send.out 2>send.err
 '
 
 test_expect_success 'updated as expected' '
-	test $(GIT_DIR=victim/.git git rev-parse master) = $commit1 &&
-	test $(GIT_DIR=victim/.git git rev-parse tofail) = $commit1
+	test $(GIT_DIR=victim git rev-parse master) = $commit1 &&
+	test $(GIT_DIR=victim git rev-parse tofail) = $commit1
 '
 
 test_expect_success 'hooks ran' '
-	test -f victim/.git/pre-receive.args &&
-	test -f victim/.git/pre-receive.stdin &&
-	test -f victim/.git/update.args &&
-	test -f victim/.git/update.stdin &&
-	test -f victim/.git/post-receive.args &&
-	test -f victim/.git/post-receive.stdin &&
-	test -f victim/.git/post-update.args &&
-	test -f victim/.git/post-update.stdin
+	test -f victim/pre-receive.args &&
+	test -f victim/pre-receive.stdin &&
+	test -f victim/update.args &&
+	test -f victim/update.stdin &&
+	test -f victim/post-receive.args &&
+	test -f victim/post-receive.stdin &&
+	test -f victim/post-update.args &&
+	test -f victim/post-update.stdin
 '
 
 test_expect_success 'pre-receive hook input' '
 	(echo $commit0 $commit1 refs/heads/master;
 	 echo $commit1 $commit0 refs/heads/tofail
-	) | test_cmp - victim/.git/pre-receive.stdin
+	) | test_cmp - victim/pre-receive.stdin
 '
 
 test_expect_success 'update hook arguments' '
 	(echo refs/heads/master $commit0 $commit1;
 	 echo refs/heads/tofail $commit1 $commit0
-	) | test_cmp - victim/.git/update.args
+	) | test_cmp - victim/update.args
 '
 
 test_expect_success 'post-receive hook input' '
 	echo $commit0 $commit1 refs/heads/master |
-	test_cmp - victim/.git/post-receive.stdin
+	test_cmp - victim/post-receive.stdin
 '
 
 test_expect_success 'post-update hook arguments' '
 	echo refs/heads/master |
-	test_cmp - victim/.git/post-update.args
+	test_cmp - victim/post-update.args
 '
 
 test_expect_success 'all hook stdin is /dev/null' '
-	! test -s victim/.git/update.stdin &&
-	! test -s victim/.git/post-update.stdin
+	! test -s victim/update.stdin &&
+	! test -s victim/post-update.stdin
 '
 
 test_expect_success 'all *-receive hook args are empty' '
-	! test -s victim/.git/pre-receive.args &&
-	! test -s victim/.git/post-receive.args
+	! test -s victim/pre-receive.args &&
+	! test -s victim/post-receive.args
 '
 
 test_expect_success 'send-pack produced no output' '
diff --git a/t/t5405-send-pack-rewind.sh b/t/t5405-send-pack-rewind.sh
index cb9aacc..2ad080f 100755
--- a/t/t5405-send-pack-rewind.sh
+++ b/t/t5405-send-pack-rewind.sh
@@ -16,7 +16,8 @@ test_expect_success setup '
 	) &&
 
 	>file2 && git add file2 && test_tick &&
-	git commit -m Second
+	git commit -m Second &&
+	git checkout -b otherbranch
 
 '
 
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 3411107..7070171 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -12,7 +12,8 @@ mk_empty () {
 	(
 		cd testrepo &&
 		git init &&
-		mv .git/hooks .git/hooks-disabled
+		mv .git/hooks .git/hooks-disabled &&
+		git symbolic-ref HEAD refs/heads/nonexistent
 	)
 }
 
diff --git a/t/t5517-push-mirror.sh b/t/t5517-push-mirror.sh
index ea49ded..5536077 100755
--- a/t/t5517-push-mirror.sh
+++ b/t/t5517-push-mirror.sh
@@ -19,7 +19,7 @@ mk_repo_pair () {
 	mkdir mirror &&
 	(
 		cd mirror &&
-		git init
+		git --bare init
 	) &&
 	mkdir master &&
 	(
-- 
1.6.0.3.866.gc189b

^ permalink raw reply related

* Re: [PATCH 1/2] diffcore-rename: support rename cache
From: Yann Dirson @ 2008-11-07 22:21 UTC (permalink / raw)
  To: Nguy???n Thái Ng???c Duy; +Cc: git
In-Reply-To: <1226068533-10152-1-git-send-email-pclouds@gmail.com>

On Fri, Nov 07, 2008 at 09:35:32PM +0700, Nguy???n Thái Ng???c Duy wrote:
> This patch teaches diffcore_rename() to look into
> $GIT_DIR/rename-cache and make use of it to recreate diff_filepair.
> With proper cache, there should be no available entry for estimation
> after exact matching.

This is something I have thought about in the past, good to see that
implemented :)

> Rename caching is per commit. I don't think abitrary tree-tree caching
> is worth it.

That could be a nice complement to my directory-rename patch.

^ permalink raw reply

* [PATCH 2/4] t5516: refactor oddball tests
From: Jeff King @ 2008-11-07 22:20 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Sam Vilain
In-Reply-To: <20081107220730.GA15942@coredump.intra.peff.net>

t5516 sets up some utility functions for starting each test
with a clean slate. However, there were a few tests added
that do not use these functions, but instead make their own
repositories.

Let's bring these in line with the rest of the tests. Not
only do we reduce the number of lines, but these tests will
benefit from any further enhancements to the utility
scripts.

The conversion is pretty straightforward. Most of the tests
created a parent/child clone relationship, for which we now
use 'testrepo' as the parent.  One test looked in testrepo,
but relied on previous tests to have set it up; it now sets
up testrepo explicitly, which makes it a bit more robust to
changes in the script, as well.

Signed-off-by: Jeff King <peff@peff.net>
---
This is on top of 'next' to pick up the recent test from Clemens
Buchacher.

A few oddities here while I was digging in the history:

  - I actually introduced the first of these tests for local tracking
    refs in 09fba7a59 (and the others, being related, copied the style).
    But then I reverted them in 0673c96, because Alex had added other
    similar tests in t5404.  However, these tests ended up being
    re-added by Dscho in 28391a80, which adds a totally unrelated test.
    I think it's the result of a bad patch application (IIRC, he marked
    up my tests to avoid having them chdir for the whole test script.
    During application, Junio would see them going from tweaks to whole
    creation, and presumably just resolved the conflict that way).

    So my initial thought was to simply delete these tests. But since
    then, other related tests have been added to this script, and we do
    want to keep those. So I decided to keep them all, as they form a
    logical progression related to tracking refs. So while there is some
    duplication with t5404, I don't think it is a problem.

  - One of the tests called 'pwd', and I can't see that it would do
    anything useful. I assume it was just leftover debugging cruft
    (especially since it is from that same commit by Dscho).

 t/t5516-fetch-push.sh |   50 ++++++++++++++++++++----------------------------
 1 files changed, 21 insertions(+), 29 deletions(-)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 598664c..3411107 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -39,6 +39,11 @@ mk_test () {
 	)
 }
 
+mk_child() {
+	rm -rf "$1" &&
+	git clone testrepo "$1"
+}
+
 check_push_result () {
 	(
 		cd testrepo &&
@@ -425,13 +430,10 @@ test_expect_success 'push with dry-run' '
 
 test_expect_success 'push updates local refs' '
 
-	rm -rf parent child &&
-	mkdir parent &&
-	(cd parent && git init &&
-		echo one >foo && git add foo && git commit -m one) &&
-	git clone parent child &&
+	mk_test heads/master &&
+	mk_child child &&
 	(cd child &&
-		echo two >foo && git commit -a -m two &&
+		git pull .. master &&
 		git push &&
 	test $(git rev-parse master) = $(git rev-parse remotes/origin/master))
 
@@ -439,15 +441,10 @@ test_expect_success 'push updates local refs' '
 
 test_expect_success 'push updates up-to-date local refs' '
 
-	rm -rf parent child &&
-	mkdir parent &&
-	(cd parent && git init &&
-		echo one >foo && git add foo && git commit -m one) &&
-	git clone parent child1 &&
-	git clone parent child2 &&
-	(cd child1 &&
-		echo two >foo && git commit -a -m two &&
-		git push) &&
+	mk_test heads/master &&
+	mk_child child1 &&
+	mk_child child2 &&
+	(cd child1 && git pull .. master && git push) &&
 	(cd child2 &&
 		git pull ../child1 master &&
 		git push &&
@@ -457,11 +454,8 @@ test_expect_success 'push updates up-to-date local refs' '
 
 test_expect_success 'push preserves up-to-date packed refs' '
 
-	rm -rf parent child &&
-	mkdir parent &&
-	(cd parent && git init &&
-		echo one >foo && git add foo && git commit -m one) &&
-	git clone parent child &&
+	mk_test heads/master &&
+	mk_child child &&
 	(cd child &&
 		git push &&
 	! test -f .git/refs/remotes/origin/master)
@@ -470,15 +464,13 @@ test_expect_success 'push preserves up-to-date packed refs' '
 
 test_expect_success 'push does not update local refs on failure' '
 
-	rm -rf parent child &&
-	mkdir parent &&
-	(cd parent && git init &&
-		echo one >foo && git add foo && git commit -m one &&
-		echo exit 1 >.git/hooks/pre-receive &&
-		chmod +x .git/hooks/pre-receive) &&
-	git clone parent child &&
+	mk_test heads/master &&
+	mk_child child &&
+	mkdir testrepo/.git/hooks &&
+	echo exit 1 >testrepo/.git/hooks/pre-receive &&
+	chmod +x testrepo/.git/hooks/pre-receive &&
 	(cd child &&
-		echo two >foo && git commit -a -m two &&
+		git pull .. master
 		test_must_fail git push &&
 		test $(git rev-parse master) != \
 			$(git rev-parse remotes/origin/master))
@@ -487,7 +479,7 @@ test_expect_success 'push does not update local refs on failure' '
 
 test_expect_success 'allow deleting an invalid remote ref' '
 
-	pwd &&
+	mk_test heads/master &&
 	rm -f testrepo/.git/objects/??/* &&
 	git push testrepo :refs/heads/master &&
 	(cd testrepo && test_must_fail git rev-parse --verify refs/heads/master)
-- 
1.6.0.3.866.gc189b

^ permalink raw reply related

* [PATCH 1/4] t5400: expect success for denying deletion
From: Jeff King @ 2008-11-07 22:09 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Sam Vilain, Jan Krüger
In-Reply-To: <20081107220730.GA15942@coredump.intra.peff.net>

Commit a240de11 introduced this test and the code to make it
successful.

Signed-off-by: Jeff King <peff@peff.net>
---
Reading over the mailing list postings which led to a240de11, I think it
is simply a case that Jan didn't fully understand what expect_failure
meant (it means "this is a test that is currently broken, but we hope to
fix in the future", and not anything to do with the test_must_fail in
the test itself).

 t/t5400-send-pack.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh
index 6fe2f87..da69f08 100755
--- a/t/t5400-send-pack.sh
+++ b/t/t5400-send-pack.sh
@@ -103,7 +103,7 @@ unset GIT_CONFIG GIT_CONFIG_LOCAL
 HOME=`pwd`/no-such-directory
 export HOME ;# this way we force the victim/.git/config to be used.
 
-test_expect_failure \
+test_expect_success \
 	'pushing a delete should be denied with denyDeletes' '
 	cd victim &&
 	git config receive.denyDeletes true &&
-- 
1.6.0.3.866.gc189b

^ permalink raw reply related

* [EGIT PATCH 6/7] Add tags to the graphical history display.
From: Robin Rosenberg @ 2008-11-07 22:07 UTC (permalink / raw)
  To: spearce; +Cc: git, Robin Rosenberg
In-Reply-To: <1226095664-13759-6-git-send-email-robin.rosenberg@dewire.com>

Both the SWT (Eclipse) drawing and Swing versions are updated.
The coloring and shapes are intentionally not the same as for gitk.

Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
---
 .../egit/ui/internal/history/SWTCommit.java        |    5 +-
 .../egit/ui/internal/history/SWTPlotRenderer.java  |   71 +++++++++++++++++++-
 .../spearce/egit/ui/internal/history/SWTWalk.java  |    2 +-
 .../org/spearce/jgit/awtui/AWTPlotRenderer.java    |   46 +++++++++++++
 .../spearce/jgit/revplot/AbstractPlotRenderer.java |   23 ++++++-
 .../src/org/spearce/jgit/revplot/PlotCommit.java   |    8 ++-
 .../src/org/spearce/jgit/revplot/PlotWalk.java     |   60 ++++++++++++++++-
 7 files changed, 207 insertions(+), 8 deletions(-)

diff --git a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/history/SWTCommit.java b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/history/SWTCommit.java
index fa0d25d..2341fbd 100644
--- a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/history/SWTCommit.java
+++ b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/history/SWTCommit.java
@@ -10,12 +10,13 @@
 import org.eclipse.swt.widgets.Widget;
 import org.spearce.jgit.lib.AnyObjectId;
 import org.spearce.jgit.revplot.PlotCommit;
+import org.spearce.jgit.lib.Ref;
 
 class SWTCommit extends PlotCommit<SWTCommitList.SWTLane> {
 	Widget widget;
 
-	SWTCommit(final AnyObjectId id) {
-		super(id);
+	SWTCommit(final AnyObjectId id, final Ref[] tags) {
+		super(id, tags);
 	}
 
 	@Override
diff --git a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/history/SWTPlotRenderer.java b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/history/SWTPlotRenderer.java
index c4ee282..b008df7 100644
--- a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/history/SWTPlotRenderer.java
+++ b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/history/SWTPlotRenderer.java
@@ -15,7 +15,10 @@
 import org.eclipse.swt.widgets.Display;
 import org.eclipse.swt.widgets.Event;
 import org.eclipse.swt.widgets.TableItem;
+import org.eclipse.ui.themes.ColorUtil;
 import org.spearce.egit.ui.internal.history.SWTCommitList.SWTLane;
+import org.spearce.jgit.lib.Constants;
+import org.spearce.jgit.lib.Ref;
 import org.spearce.jgit.revplot.AbstractPlotRenderer;
 import org.spearce.jgit.revplot.PlotCommit;
 
@@ -26,7 +29,13 @@
 
 	private final Color sys_gray;
 
-	private Color sys_darkblue;
+	private final Color sys_darkblue;
+
+	private final Color sys_yellow;
+
+	private final Color sys_green;
+
+	private final Color sys_white;
 
 	GC g;
 
@@ -43,6 +52,9 @@ SWTPlotRenderer(final Display d) {
 		sys_black = d.getSystemColor(SWT.COLOR_BLACK);
 		sys_gray = d.getSystemColor(SWT.COLOR_GRAY);
 		sys_darkblue = d.getSystemColor(SWT.COLOR_DARK_BLUE);
+		sys_yellow = d.getSystemColor(SWT.COLOR_YELLOW);
+		sys_green = d.getSystemColor(SWT.COLOR_GREEN);
+		sys_white = d.getSystemColor(SWT.COLOR_WHITE);
 	}
 
 	void paint(final Event event) {
@@ -92,7 +104,64 @@ protected void drawText(final String msg, final int x, final int y) {
 		g.drawString(msg, cellX + x, cellY + texty, true);
 	}
 
+	@Override
+	protected int drawLabel(int x, int y, Ref ref) {
+		String txt;
+		String name = ref.getOrigName();
+		if (name.startsWith(Constants.R_HEADS)) {
+			g.setBackground(sys_green);
+			txt = name.substring(Constants.R_HEADS.length());
+		} else if (name.startsWith(Constants.R_REMOTES)){
+			g.setBackground(sys_gray);
+			txt = name.substring(Constants.R_REMOTES.length());
+		} else if (name.startsWith(Constants.R_TAGS)){
+			g.setBackground(sys_yellow);
+			txt = name.substring(Constants.R_TAGS.length());
+		} else {
+			// Whatever this would be
+			g.setBackground(sys_white);
+			if (name.startsWith(Constants.R_REFS))
+				txt = name.substring(Constants.R_REFS.length());
+			else
+				txt = name; // HEAD and such
+		}
+
+		// Make peeled objects, i.e. via annotated tags come out in a paler color
+		Color peeledColor = null;
+		if (ref.getPeeledObjectId() == null || !ref.getPeeledObjectId().equals(ref.getObjectId())) {
+			peeledColor = new Color(g.getDevice(), ColorUtil.blend(g.getBackground().getRGB(), sys_white.getRGB()));
+			g.setBackground(peeledColor);
+		}
+
+		if (txt.length() > 12)
+			txt = txt.substring(0,11) + "\u2026"; // ellipsis "…" (in UTF-8)
+
+		Point textsz = g.stringExtent(txt);
+		int arc = textsz.y/2;
+		final int texty = (y * 2 - textsz.y) / 2;
+
+		// Draw backgrounds
+		g.fillRoundRectangle(x + 1, cellY + texty -1, textsz.x + 3, textsz.y + 1, arc, arc);
+		g.setForeground(sys_black);
+		g.drawString(txt, x + 2, cellY + texty, true);
+		g.setLineWidth(2);
+
+		// And a two color shaded border, blend with whatever background there already is
+		g.setAlpha(128);
+		g.setForeground(sys_gray);
+		g.drawRoundRectangle(x, cellY + texty -2, textsz.x + 5, textsz.y + 3, arc, arc);
+		g.setLineWidth(2);
+		g.setForeground(sys_black);
+		g.drawRoundRectangle(x + 1, cellY + texty -1, textsz.x + 3, textsz.y + 1, arc, arc);
+		g.setAlpha(255);
+
+		if (peeledColor != null)
+			peeledColor.dispose();
+		return 8 + textsz.x;
+	}
+
 	protected Color laneColor(final SWTLane myLane) {
 		return myLane != null ? myLane.color : sys_black;
 	}
+
 }
diff --git a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/history/SWTWalk.java b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/history/SWTWalk.java
index 527d284..57039b5 100644
--- a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/history/SWTWalk.java
+++ b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/history/SWTWalk.java
@@ -19,6 +19,6 @@ SWTWalk(final Repository repo) {
 
 	@Override
 	protected RevCommit createCommit(final AnyObjectId id) {
-		return new SWTCommit(id);
+		return new SWTCommit(id, getTags(id));
 	}
 }
diff --git a/org.spearce.jgit/src/org/spearce/jgit/awtui/AWTPlotRenderer.java b/org.spearce.jgit/src/org/spearce/jgit/awtui/AWTPlotRenderer.java
index b6b715c..5dcddf5 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/awtui/AWTPlotRenderer.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/awtui/AWTPlotRenderer.java
@@ -44,6 +44,8 @@
 
 import org.spearce.jgit.awtui.CommitGraphPane.GraphCellRender;
 import org.spearce.jgit.awtui.SwingCommitList.SwingLane;
+import org.spearce.jgit.lib.Constants;
+import org.spearce.jgit.lib.Ref;
 import org.spearce.jgit.revplot.AbstractPlotRenderer;
 import org.spearce.jgit.revplot.PlotCommit;
 
@@ -134,4 +136,48 @@ void paintTriangleDown(final int cx, final int y, final int h) {
 		g.drawPolygon(triangle);
 	}
 
+	@Override
+	protected int drawLabel(int x, int y, Ref ref) {
+		String txt;
+		String name = ref.getOrigName();
+		if (name.startsWith(Constants.R_HEADS)) {
+			g.setBackground(Color.GREEN);
+			txt = name.substring(Constants.R_HEADS.length());
+		} else if (name.startsWith(Constants.R_REMOTES)){
+			g.setBackground(Color.LIGHT_GRAY);
+			txt = name.substring(Constants.R_REMOTES.length());
+		} else if (name.startsWith(Constants.R_TAGS)){
+			g.setBackground(Color.YELLOW);
+			txt = name.substring(Constants.R_TAGS.length());
+		} else {
+			// Whatever this would be
+			g.setBackground(Color.WHITE);
+			if (name.startsWith(Constants.R_REFS))
+				txt = name.substring(Constants.R_REFS.length());
+			else
+				txt = name; // HEAD and such
+		}
+		if (ref.getPeeledObjectId() != null) {
+			float[] colorComponents = g.getBackground().getRGBColorComponents(null);
+			colorComponents[0] *= 0.9;
+			colorComponents[1] *= 0.9;
+			colorComponents[2] *= 0.9;
+			g.setBackground(new Color(colorComponents[0],colorComponents[1],colorComponents[2]));
+		}
+		if (txt.length() > 12)
+			txt = txt.substring(0,11) + "\u2026"; // ellipsis "…" (in UTF-8)
+
+		final int texth = g.getFontMetrics().getHeight();
+		int textw = g.getFontMetrics().stringWidth(txt);
+		g.setColor(g.getBackground());
+		int arcHeight = texth/4;
+		int y0 = y - texth/2 + (cell.getHeight() - texth)/2;
+		g.fillRoundRect(x , y0, textw + arcHeight*2, texth -1, arcHeight, arcHeight);
+		g.setColor(g.getColor().darker());
+		g.drawRoundRect(x, y0, textw + arcHeight*2, texth -1 , arcHeight, arcHeight);
+		g.setColor(Color.BLACK);
+		g.drawString(txt, x + arcHeight, y0 + texth - g.getFontMetrics().getDescent());
+
+		return arcHeight * 3 + textw;
+	}
 }
\ No newline at end of file
diff --git a/org.spearce.jgit/src/org/spearce/jgit/revplot/AbstractPlotRenderer.java b/org.spearce.jgit/src/org/spearce/jgit/revplot/AbstractPlotRenderer.java
index f175c9d..911dd68 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/revplot/AbstractPlotRenderer.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/revplot/AbstractPlotRenderer.java
@@ -37,6 +37,7 @@
 
 package org.spearce.jgit.revplot;
 
+import org.spearce.jgit.lib.Ref;
 import org.spearce.jgit.revwalk.RevFlag;
 
 /**
@@ -140,11 +141,29 @@ protected void paintCommit(final PlotCommit<TLane> commit, final int h) {
 		else
 			drawCommitDot(dotX, dotY, dotSize, dotSize);
 
+		int textx = Math.max(maxCenter + LANE_WIDTH / 2, dotX + dotSize) + 8;
+		int n = commit.refs == null ? 0 : commit.refs.length;
+		for (int i = 0; i < n; ++i) {
+			textx += drawLabel(textx + dotSize, h/2, commit.refs[i]);
+		}
+
 		final String msg = commit.getShortMessage();
-		final int textx = Math.max(maxCenter + LANE_WIDTH / 2, dotX + dotSize) + 8;
-		drawText(msg, textx, h / 2);
+		drawText(msg, textx + dotSize + n*2, h / 2);
 	}
 
+	/**
+	 * Draw a decoration for the Ref ref at x,y
+	 *
+	 * @param x
+	 *            left
+	 * @param y
+	 *            top
+	 * @param ref
+	 *            A peeled ref
+	 * @return width of label in pixels
+	 */
+	protected abstract int drawLabel(int x, int y, Ref ref);
+
 	private int computeDotSize(final int h) {
 		int d = (int) (Math.min(h, LANE_WIDTH) * 0.50f);
 		d += (d & 1);
diff --git a/org.spearce.jgit/src/org/spearce/jgit/revplot/PlotCommit.java b/org.spearce.jgit/src/org/spearce/jgit/revplot/PlotCommit.java
index 5a5ef1e..c885a44 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/revplot/PlotCommit.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/revplot/PlotCommit.java
@@ -39,6 +39,7 @@
 
 import org.spearce.jgit.lib.AnyObjectId;
 import org.spearce.jgit.revwalk.RevCommit;
+import org.spearce.jgit.lib.Ref;
 
 /**
  * A commit reference to a commit in the DAG.
@@ -58,14 +59,19 @@
 
 	PlotCommit[] children;
 
+	final Ref[] refs;
+
 	/**
 	 * Create a new commit.
 	 * 
 	 * @param id
 	 *            the identity of this commit.
+	 * @param tags
+	 *            the tags associated with this commit, null for no tags
 	 */
-	protected PlotCommit(final AnyObjectId id) {
+	protected PlotCommit(final AnyObjectId id, final Ref[] tags) {
 		super(id);
+		this.refs = tags;
 		passingLanes = NO_LANES;
 		children = NO_CHILDREN;
 	}
diff --git a/org.spearce.jgit/src/org/spearce/jgit/revplot/PlotWalk.java b/org.spearce.jgit/src/org/spearce/jgit/revplot/PlotWalk.java
index e5e8aba..8801850 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/revplot/PlotWalk.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/revplot/PlotWalk.java
@@ -37,14 +37,33 @@
 
 package org.spearce.jgit.revplot;
 
+import java.io.IOException;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Comparator;
+import java.util.Map;
+import java.util.Set;
+
 import org.spearce.jgit.lib.AnyObjectId;
+import org.spearce.jgit.lib.Commit;
+import org.spearce.jgit.lib.Ref;
 import org.spearce.jgit.lib.Repository;
+import org.spearce.jgit.lib.Tag;
 import org.spearce.jgit.revwalk.RevCommit;
 import org.spearce.jgit.revwalk.RevSort;
 import org.spearce.jgit.revwalk.RevWalk;
 
 /** Specialized RevWalk for visualization of a commit graph. */
 public class PlotWalk extends RevWalk {
+
+	private Map<AnyObjectId, Set<Ref>> reverseRefMap;
+
+	@Override
+	public void dispose() {
+		super.dispose();
+		reverseRefMap.clear();
+	}
+
 	/**
 	 * Create a new revision walker for a given repository.
 	 * 
@@ -54,6 +73,7 @@
 	public PlotWalk(final Repository repo) {
 		super(repo);
 		super.sort(RevSort.TOPO, true);
+		reverseRefMap = repo.getAllRefsByPeeledObjectId();
 	}
 
 	@Override
@@ -65,6 +85,44 @@ public void sort(final RevSort s, final boolean use) {
 
 	@Override
 	protected RevCommit createCommit(final AnyObjectId id) {
-		return new PlotCommit(id);
+		return new PlotCommit(id, getTags(id));
+	}
+
+	protected Ref[] getTags(final AnyObjectId commitId) {
+		Collection<Ref> list = reverseRefMap.get(commitId);
+		Ref[] tags;
+		if (list == null)
+			tags = null;
+		else {
+			tags = list.toArray(new Ref[list.size()]);
+			Arrays.sort(tags, new PlotRefComparator());
+		}
+		return tags;
+	}
+
+	class PlotRefComparator implements Comparator<Ref> {
+		public int compare(Ref o1, Ref o2) {
+			try {
+				Object obj1 = getRepository().mapObject(o1.getObjectId(), o1.getName());
+				Object obj2 = getRepository().mapObject(o2.getObjectId(), o2.getName());
+				long t1 = timeof(obj1);
+				long t2 = timeof(obj2);
+				if (t1 > t2)
+					return -1;
+				if (t1 < t2)
+					return 1;
+				return 0;
+			} catch (IOException e) {
+				// ignore
+				return 0;
+			}
+		}
+		long timeof(Object o) {
+			if (o instanceof Commit)
+				return ((Commit)o).getCommitter().getWhen().getTime();
+			if (o instanceof Tag)
+				return ((Tag)o).getTagger().getWhen().getTime();
+			return 0;
+		}
 	}
 }
-- 
1.6.0.3.578.g6a50

^ permalink raw reply related

* [EGIT PATCH 5/7] Add a method to get refs by object Id
From: Robin Rosenberg @ 2008-11-07 22:07 UTC (permalink / raw)
  To: spearce; +Cc: git, Robin Rosenberg
In-Reply-To: <1226095664-13759-5-git-send-email-robin.rosenberg@dewire.com>

Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
---
 .../src/org/spearce/jgit/lib/Repository.java       |   34 ++++++++++++++++++++
 1 files changed, 34 insertions(+), 0 deletions(-)

diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/Repository.java b/org.spearce.jgit/src/org/spearce/jgit/lib/Repository.java
index 4d6e6fd..82a7454 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/Repository.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/Repository.java
@@ -47,10 +47,13 @@
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 import java.util.Vector;
 
 import org.spearce.jgit.errors.IncorrectObjectTypeException;
@@ -952,6 +955,37 @@ public Ref peel(final Ref ref) {
 	}
 
 	/**
+	 * @return a map with all objects referenced by a peeled ref.
+	 */
+	public Map<AnyObjectId, Set<Ref>> getAllRefsByPeeledObjectId() {
+		Map<String, Ref> allRefs = getAllRefs();
+		Map<AnyObjectId, Set<Ref>> ret = new HashMap<AnyObjectId, Set<Ref>>(allRefs.size());
+		for (Ref ref : allRefs.values()) {
+			if (ref == null)
+				continue;
+			if (!ref.isPeeled()) {
+				ref = peel(ref);
+				allRefs.put(ref.getOrigName(), ref);
+			}
+			AnyObjectId target = ref.getPeeledObjectId();
+			if (target == null)
+				target = ref.getObjectId();
+			// We assume most Sets here are singletons
+			Set<Ref> oset = ret.put(target, Collections.singleton(ref));
+			if (oset != null) {
+				// that was not the case (rare)
+				if (oset.size() == 1) {
+					// Was a read-only singleton, we must copy to a new Set
+					oset = new HashSet<Ref>(oset);
+				}
+				ret.put(target, oset);
+				oset.add(ref);
+			}
+		}
+		return ret;
+	}
+
+	/**
 	 * @return true if HEAD points to a StGit patch.
 	 */
 	public boolean isStGitMode() {
-- 
1.6.0.3.578.g6a50

^ permalink raw reply related

* [EGIT PATCH 3/7] Keep original ref name when reading refs
From: Robin Rosenberg @ 2008-11-07 22:07 UTC (permalink / raw)
  To: spearce; +Cc: git, Robin Rosenberg
In-Reply-To: <1226095664-13759-3-git-send-email-robin.rosenberg@dewire.com>

We want to know the original name of refs, not just the target name.

Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
---
 org.spearce.jgit/src/org/spearce/jgit/lib/Ref.java |   63 +++++++++++++++++++-
 .../src/org/spearce/jgit/lib/RefDatabase.java      |   45 +++++++++-----
 .../src/org/spearce/jgit/lib/RefUpdate.java        |   14 ++---
 3 files changed, 94 insertions(+), 28 deletions(-)

diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/Ref.java b/org.spearce.jgit/src/org/spearce/jgit/lib/Ref.java
index db94875..2f102af 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/Ref.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/Ref.java
@@ -43,6 +43,11 @@
  * A ref in Git is (more or less) a variable that holds a single object
  * identifier. The object identifier can be any valid Git object (blob, tree,
  * commit, annotated tag, ...).
+ * <p>
+ * The ref name has the attributes of the ref that was asked for as well as
+ * the ref it was resolved to for symbolic refs plus the object id it points
+ * to and (for tags) the peeled target object id, i.e. the tag resolved
+ * recursively until a non-tag object is referenced.
  */
 public class Ref {
 	/** Location where a {@link Ref} is stored. */
@@ -119,19 +124,24 @@ public boolean isPacked() {
 
 	private ObjectId peeledObjectId;
 
+	private final String origName;
+
 	/**
 	 * Create a new ref pairing.
 	 * 
 	 * @param st
 	 *            method used to store this ref.
+	 * @param origName
+	 *            The name used to resolve this ref
 	 * @param refName
 	 *            name of this ref.
 	 * @param id
 	 *            current value of the ref. May be null to indicate a ref that
 	 *            does not exist yet.
 	 */
-	public Ref(final Storage st, final String refName, final ObjectId id) {
+	public Ref(final Storage st, final String origName, final String refName, final ObjectId id) {
 		storage = st;
+		this.origName = origName;
 		name = refName;
 		objectId = id;
 	}
@@ -146,19 +156,56 @@ public Ref(final Storage st, final String refName, final ObjectId id) {
 	 * @param id
 	 *            current value of the ref. May be null to indicate a ref that
 	 *            does not exist yet.
+	 */
+	public Ref(final Storage st, final String refName, final ObjectId id) {
+		this(st, refName, refName, id);
+	}
+
+	/**
+	 * Create a new ref pairing.
+	 * 
+	 * @param st
+	 *            method used to store this ref.
+	 * @param origName
+	 *            The name used to resolve this ref
+	 * @param refName
+	 *            name of this ref.
+	 * @param id
+	 *            current value of the ref. May be null to indicate a ref that
+	 *            does not exist yet.
 	 * @param peel
 	 *            peeled value of the ref's tag. May be null if this is not a
 	 *            tag or the peeled value is not known.
 	 */
-	public Ref(final Storage st, final String refName, final ObjectId id,
+	public Ref(final Storage st, final String origName, final String refName, final ObjectId id,
 			final ObjectId peel) {
 		storage = st;
+		this.origName = origName;
 		name = refName;
 		objectId = id;
 		peeledObjectId = peel;
 	}
 
 	/**
+	 * Create a new ref pairing.
+	 * 
+	 * @param st
+	 *            method used to store this ref.
+	 * @param refName
+	 *            name of this ref.
+	 * @param id
+	 *            current value of the ref. May be null to indicate a ref that
+	 *            does not exist yet.
+	 * @param peel
+	 *            peeled value of the ref's tag. May be null if this is not a
+	 *            tag or the peeled value is not known.
+	 */
+	public Ref(final Storage st, final String refName, final ObjectId id,
+			final ObjectId peel) {
+		this(st, refName, refName, id, peel);
+	}
+
+	/**
 	 * What this ref is called within the repository.
 	 * 
 	 * @return name of this ref.
@@ -168,6 +215,13 @@ public String getName() {
 	}
 
 	/**
+	 * @return the originally resolved name
+	 */
+	public String getOrigName() {
+		return origName;
+	}
+
+	/**
 	 * Cached value of this ref.
 	 * 
 	 * @return the value of this ref at the last time we read it.
@@ -200,6 +254,9 @@ public Storage getStorage() {
 	}
 
 	public String toString() {
-		return "Ref[" + name + "=" + ObjectId.toString(getObjectId()) + "]";
+		String o = "";
+		if (!origName.equals(name))
+			o = "(" + origName + ")";
+		return "Ref[" + o + name + "=" + ObjectId.toString(getObjectId()) + "]";
 	}
 }
diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java b/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java
index 5c1f060..5a1b85f 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java
@@ -51,6 +51,7 @@
 import java.util.Map;
 
 import org.spearce.jgit.errors.ObjectWritingException;
+import org.spearce.jgit.lib.Ref.Storage;
 import org.spearce.jgit.util.FS;
 import org.spearce.jgit.util.NB;
 
@@ -135,8 +136,8 @@ RefUpdate newUpdate(final String name) throws IOException {
 		return new RefUpdate(this, r, fileForRef(r.getName()));
 	}
 
-	void stored(final String name, final ObjectId id, final long time) {
-		looseRefs.put(name, new Ref(Ref.Storage.LOOSE, name, id));
+	void stored(final String origName, final String name, final ObjectId id, final long time) {
+		looseRefs.put(name, new Ref(Ref.Storage.LOOSE, origName, name, id));
 		looseRefsMTime.put(name, time);
 		setModified();
 		db.fireRefsMaybeChanged();
@@ -222,12 +223,12 @@ private void readLooseRefs(final Map<String, Ref> avail,
 			final String entName = ent.getName();
 			if (".".equals(entName) || "..".equals(entName))
 				continue;
-			readOneLooseRef(avail, prefix + entName, ent);
+			readOneLooseRef(avail, prefix + entName, prefix + entName, ent);
 		}
 	}
 
 	private void readOneLooseRef(final Map<String, Ref> avail,
-			final String refName, final File ent) {
+			final String origName, final String refName, final File ent) {
 		// Unchanged and cached? Don't read it again.
 		//
 		Ref ref = looseRefs.get(refName);
@@ -270,7 +271,7 @@ private void readOneLooseRef(final Map<String, Ref> avail,
 					return;
 				}
 
-				ref = new Ref(Ref.Storage.LOOSE, refName, id);
+				ref = new Ref(Ref.Storage.LOOSE, origName, refName, id);
 				looseRefs.put(ref.getName(), ref);
 				looseRefsMTime.put(ref.getName(), ent.lastModified());
 				avail.put(ref.getName(), ref);
@@ -293,27 +294,35 @@ private File fileForRef(final String name) {
 		return new File(gitDir, name);
 	}
 
-	private Ref readRefBasic(final String name, final int depth)
+	private Ref readRefBasic(final String name, final int depth) throws IOException {
+		return readRefBasic(name, name, depth);
+	}
+
+	private Ref readRefBasic(final String origName, final String name, final int depth)
 			throws IOException {
 		// Prefer loose ref to packed ref as the loose
 		// file can be more up-to-date than a packed one.
 		//
-		Ref ref = looseRefs.get(name);
+		Ref ref = looseRefs.get(origName);
 		final File loose = fileForRef(name);
 		final long mtime = loose.lastModified();
 		if (ref != null) {
 			Long cachedlastModified = looseRefsMTime.get(name);
 			if (cachedlastModified != null && cachedlastModified == mtime)
 				return ref;
-			looseRefs.remove(name);
-			looseRefsMTime.remove(name);
+			looseRefs.remove(origName);
+			looseRefsMTime.remove(origName);
 		}
 
 		if (mtime == 0) {
 			// If last modified is 0 the file does not exist.
 			// Try packed cache.
 			//
-			return packedRefs.get(name);
+			ref = packedRefs.get(name);
+			if (ref != null)
+				if (!ref.getOrigName().equals(origName))
+					ref = new Ref(Storage.LOOSE_PACKED, origName, name, ref.getObjectId());
+			return ref;
 		}
 
 		final String line;
@@ -324,7 +333,7 @@ private Ref readRefBasic(final String name, final int depth)
 		}
 
 		if (line == null || line.length() == 0)
-			return new Ref(Ref.Storage.LOOSE, name, null);
+			return new Ref(Ref.Storage.LOOSE, origName, name, null);
 
 		if (line.startsWith("ref: ")) {
 			if (depth >= 5) {
@@ -333,12 +342,16 @@ private Ref readRefBasic(final String name, final int depth)
 			}
 
 			final String target = line.substring("ref: ".length());
-			final Ref r = readRefBasic(target, depth + 1);
+			Ref r = readRefBasic(target, target, depth + 1);
 			Long cachedMtime = looseRefsMTime.get(name);
 			if (cachedMtime != null && cachedMtime != mtime)
 				setModified();
 			looseRefsMTime.put(name, mtime);
-			return r != null ? r : new Ref(Ref.Storage.LOOSE, target, null);
+			if (r == null)
+				return new Ref(Ref.Storage.LOOSE, origName, target, null);
+			if (!origName.equals(r.getName()))
+				r = new Ref(Ref.Storage.LOOSE_PACKED, origName, r.getName(), r.getObjectId(), r.getPeeledObjectId());
+			return r; 
 		}
 
 		setModified();
@@ -350,7 +363,7 @@ private Ref readRefBasic(final String name, final int depth)
 			throw new IOException("Not a ref: " + name + ": " + line);
 		}
 
-		ref = new Ref(Ref.Storage.LOOSE, name, id);
+		ref = new Ref(Ref.Storage.LOOSE, origName, name, id);
 		looseRefs.put(name, ref);
 		looseRefsMTime.put(name, mtime);
 		return ref;
@@ -384,7 +397,7 @@ private void refreshPackedRefs() {
 
 						final ObjectId id = ObjectId.fromString(p.substring(1));
 						last = new Ref(Ref.Storage.PACKED, last.getName(), last
-								.getObjectId(), id);
+								.getName(), last.getObjectId(), id);
 						newPackedRefs.put(last.getName(), last);
 						continue;
 					}
@@ -392,7 +405,7 @@ private void refreshPackedRefs() {
 					final int sp = p.indexOf(' ');
 					final ObjectId id = ObjectId.fromString(p.substring(0, sp));
 					final String name = new String(p.substring(sp + 1));
-					last = new Ref(Ref.Storage.PACKED, name, id);
+					last = new Ref(Ref.Storage.PACKED, name, name, id);
 					newPackedRefs.put(last.getName(), last);
 				}
 			} finally {
diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/RefUpdate.java b/org.spearce.jgit/src/org/spearce/jgit/lib/RefUpdate.java
index 86b44c5..235c2fd 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/RefUpdate.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/RefUpdate.java
@@ -131,9 +131,6 @@
 	/** Repository the ref is stored in. */
 	private final RefDatabase db;
 
-	/** Name of the ref. */
-	private final String name;
-
 	/** Location of the loose file holding the value of this ref. */
 	private final File looseFile;
 
@@ -160,7 +157,6 @@
 	RefUpdate(final RefDatabase r, final Ref ref, final File f) {
 		db = r;
 		this.ref = ref;
-		name = ref.getName();
 		oldValue = ref.getObjectId();
 		looseFile = f;
 	}
@@ -171,7 +167,7 @@ RefUpdate(final RefDatabase r, final Ref ref, final File f) {
 	 * @return name of this ref.
 	 */
 	public String getName() {
-		return name;
+		return ref.getName();
 	}
 
 	/**
@@ -349,9 +345,9 @@ public Result delete() throws IOException {
 	 * @throws IOException
 	 */
 	public Result delete(final RevWalk walk) throws IOException {
-		if (name.startsWith(Constants.R_HEADS)) {
+		if (getName().startsWith(Constants.R_HEADS)) {
 			final Ref head = db.readRef(Constants.HEAD);
-			if (head != null && name.equals(head.getName()))
+			if (head != null && getName().equals(head.getName()))
 				return Result.REJECTED_CURRENT_BRANCH;
 		}
 
@@ -373,7 +369,7 @@ private Result updateImpl(final RevWalk walk, final Store store)
 		if (!lock.lock())
 			return Result.LOCK_FAILURE;
 		try {
-			oldValue = db.idOf(name);
+			oldValue = db.idOf(getName());
 			if (oldValue == null)
 				return store.store(lock, Result.NEW);
 
@@ -428,7 +424,7 @@ else if (status == Result.NEW)
 				getName());
 		if (!lock.commit())
 			return Result.LOCK_FAILURE;
-		db.stored(name, newValue, lock.getCommitLastModified());
+		db.stored(this.ref.getOrigName(),  ref.getName(), newValue, lock.getCommitLastModified());
 		return status;
 	}
 
-- 
1.6.0.3.578.g6a50

^ permalink raw reply related

* [EGIT PATCH 7/7] Add decorate option to log program
From: Robin Rosenberg @ 2008-11-07 22:07 UTC (permalink / raw)
  To: spearce; +Cc: git, Robin Rosenberg
In-Reply-To: <1226095664-13759-7-git-send-email-robin.rosenberg@dewire.com>

Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
---
 .../src/org/spearce/jgit/pgm/Log.java              |   33 ++++++++++++++++++++
 1 files changed, 33 insertions(+), 0 deletions(-)

diff --git a/org.spearce.jgit.pgm/src/org/spearce/jgit/pgm/Log.java b/org.spearce.jgit.pgm/src/org/spearce/jgit/pgm/Log.java
index e3a32c8..bb424d4 100644
--- a/org.spearce.jgit.pgm/src/org/spearce/jgit/pgm/Log.java
+++ b/org.spearce.jgit.pgm/src/org/spearce/jgit/pgm/Log.java
@@ -40,11 +40,19 @@
 
 import java.text.DateFormat;
 import java.text.SimpleDateFormat;
+import java.util.Collection;
+import java.util.Iterator;
 import java.util.Locale;
+import java.util.Map;
+import java.util.Set;
 import java.util.TimeZone;
 
+import org.kohsuke.args4j.Option;
+import org.spearce.jgit.lib.AnyObjectId;
 import org.spearce.jgit.lib.PersonIdent;
+import org.spearce.jgit.lib.Ref;
 import org.spearce.jgit.revwalk.RevCommit;
+import org.spearce.jgit.revwalk.RevWalk;
 
 @Command(common = true, usage = "View commit history")
 class Log extends RevWalkTextBuiltin {
@@ -52,14 +60,39 @@
 
 	private final DateFormat fmt;
 
+	private Map<AnyObjectId, Set<Ref>> allRefsByPeeledObjectId;
+
+	@Option(name="--decorate", usage="Show ref names matching commits")
+	private boolean decorate;
+
 	Log() {
 		fmt = new SimpleDateFormat("EEE MMM dd HH:mm:ss yyyy ZZZZZ", Locale.US);
 	}
 
 	@Override
+	protected RevWalk createWalk() {
+		RevWalk ret = super.createWalk();
+		if (decorate)
+			allRefsByPeeledObjectId = getRepository().getAllRefsByPeeledObjectId();
+		return ret;
+	}
+
+	@Override
 	protected void show(final RevCommit c) throws Exception {
 		out.print("commit ");
 		c.getId().copyTo(outbuffer, out);
+		if (decorate) {
+			Collection<Ref> list = allRefsByPeeledObjectId.get(c.copy());
+			if (list != null) {
+				out.print(" (");
+				for (Iterator<Ref> i = list.iterator(); i.hasNext(); ) {
+					out.print(i.next().getOrigName());
+					if (i.hasNext())
+						out.print(" ");
+				}
+				out.print(")");
+			}
+		}
 		out.println();
 
 		final PersonIdent author = c.getAuthorIdent();
-- 
1.6.0.3.578.g6a50

^ permalink raw reply related

* [EGIT PATCH 4/7] Handle peeling of loose refs.
From: Robin Rosenberg @ 2008-11-07 22:07 UTC (permalink / raw)
  To: spearce; +Cc: git, Robin Rosenberg
In-Reply-To: <1226095664-13759-4-git-send-email-robin.rosenberg@dewire.com>

For packed refs we got peeling automatically from packed-refs,
but for loose tags we have to follow the tags and get the leaf
object in order to comply with the documentation.

Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
---
 org.spearce.jgit/src/org/spearce/jgit/lib/Ref.java |   22 +++++++++++----
 .../src/org/spearce/jgit/lib/RefDatabase.java      |   28 +++++++++++++++++++-
 .../src/org/spearce/jgit/lib/Repository.java       |   13 +++++++++
 3 files changed, 56 insertions(+), 7 deletions(-)

diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/Ref.java b/org.spearce.jgit/src/org/spearce/jgit/lib/Ref.java
index 2f102af..1a6cc4c 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/Ref.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/Ref.java
@@ -140,10 +140,7 @@ public boolean isPacked() {
 	 *            does not exist yet.
 	 */
 	public Ref(final Storage st, final String origName, final String refName, final ObjectId id) {
-		storage = st;
-		this.origName = origName;
-		name = refName;
-		objectId = id;
+		this(st, origName, refName, id, ObjectId.zeroId());
 	}
 
 	/**
@@ -158,7 +155,7 @@ public Ref(final Storage st, final String origName, final String refName, final 
 	 *            does not exist yet.
 	 */
 	public Ref(final Storage st, final String refName, final ObjectId id) {
-		this(st, refName, refName, id);
+		this(st, refName, refName, id, ObjectId.zeroId());
 	}
 
 	/**
@@ -175,7 +172,7 @@ public Ref(final Storage st, final String refName, final ObjectId id) {
 	 *            does not exist yet.
 	 * @param peel
 	 *            peeled value of the ref's tag. May be null if this is not a
-	 *            tag or the peeled value is not known.
+	 *            tag or the zero id if the peeled value is not known.
 	 */
 	public Ref(final Storage st, final String origName, final String refName, final ObjectId id,
 			final ObjectId peel) {
@@ -238,10 +235,19 @@ public ObjectId getObjectId() {
 	 *         refer to an annotated tag.
 	 */
 	public ObjectId getPeeledObjectId() {
+		if (peeledObjectId == ObjectId.zeroId())
+			return null;
 		return peeledObjectId;
 	}
 
 	/**
+	 * @return whether the Ref represents a peeled tag
+	 */
+	public boolean isPeeled() {
+		return peeledObjectId != null && peeledObjectId != ObjectId.zeroId();
+	}
+
+	/**
 	 * How was this ref obtained?
 	 * <p>
 	 * The current storage model of a Ref may influence how the ref must be
@@ -259,4 +265,8 @@ public String toString() {
 			o = "(" + origName + ")";
 		return "Ref[" + o + name + "=" + ObjectId.toString(getObjectId()) + "]";
 	}
+
+	void setPeeledObjectId(final ObjectId id) {
+		peeledObjectId = id;
+	}
 }
diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java b/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java
index 5a1b85f..0d73191 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java
@@ -271,7 +271,8 @@ private void readOneLooseRef(final Map<String, Ref> avail,
 					return;
 				}
 
-				ref = new Ref(Ref.Storage.LOOSE, origName, refName, id);
+				ref = new Ref(Ref.Storage.LOOSE, origName, refName, id, null); // unpeeled
+
 				looseRefs.put(ref.getName(), ref);
 				looseRefsMTime.put(ref.getName(), ent.lastModified());
 				avail.put(ref.getName(), ref);
@@ -288,6 +289,28 @@ private void readOneLooseRef(final Map<String, Ref> avail,
 		}
 	}
 
+	Ref peel(final Ref ref) {
+		if (ref.isPeeled())
+			return ref;
+		try {
+			Object tt = db.mapObject(ref.getObjectId(), ref.getName());
+			if (tt != null && tt instanceof Tag) {
+				Tag t = (Tag)tt;
+				while (t != null && t.getType().equals(Constants.TYPE_TAG))
+					t = db.mapTag(t.getTag(), t.getObjId());
+				if (t != null)
+					ref.setPeeledObjectId(t.getObjId());
+				else
+					ref.setPeeledObjectId(null);
+			} else
+				ref.setPeeledObjectId(ref.getObjectId());
+		} catch (IOException e) {
+			// Serious error. Caller knows a ref should never be null
+			ref.setPeeledObjectId(null);
+		}
+		return ref;
+	}
+
 	private File fileForRef(final String name) {
 		if (name.startsWith(REFS_SLASH))
 			return new File(refsDir, name.substring(REFS_SLASH.length()));
@@ -364,6 +387,9 @@ private Ref readRefBasic(final String origName, final String name, final int dep
 		}
 
 		ref = new Ref(Ref.Storage.LOOSE, origName, name, id);
+
+		looseRefs.put(origName, ref);
+		ref = new Ref(Ref.Storage.LOOSE, origName, id);
 		looseRefs.put(name, ref);
 		looseRefsMTime.put(name, mtime);
 		return ref;
diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/Repository.java b/org.spearce.jgit/src/org/spearce/jgit/lib/Repository.java
index 26748e2..4d6e6fd 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/Repository.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/Repository.java
@@ -939,6 +939,19 @@ public String getBranch() throws IOException {
 	}
 
 	/**
+	 * Peel a possibly unpeeled ref and updates it. If the ref cannot be peeled
+	 * the peeled id is set to {@link ObjectId#zeroId()}
+	 * 
+	 * @param ref
+	 *            The ref to peel
+	 * @return The same, an updated ref with peeled info or a new instance with
+	 *         more information
+	 */
+	public Ref peel(final Ref ref) {
+		return refs.peel(ref);
+	}
+
+	/**
 	 * @return true if HEAD points to a StGit patch.
 	 */
 	public boolean isStGitMode() {
-- 
1.6.0.3.578.g6a50

^ permalink raw reply related

* [EGIT PATCH 1/7] Test the origName part of the key vs the ref itself
From: Robin Rosenberg @ 2008-11-07 22:07 UTC (permalink / raw)
  To: spearce; +Cc: git, Robin Rosenberg
In-Reply-To: <1226095664-13759-1-git-send-email-robin.rosenberg@dewire.com>

Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
---
 .../tst/org/spearce/jgit/lib/RefUpdateTest.java    |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RefUpdateTest.java b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RefUpdateTest.java
index 6e2cfa8..12f9ada 100644
--- a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RefUpdateTest.java
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RefUpdateTest.java
@@ -39,6 +39,8 @@
 
 import java.io.File;
 import java.io.IOException;
+import java.util.Map;
+import java.util.Map.Entry;
 
 import org.spearce.jgit.lib.RefUpdate.Result;
 
@@ -127,4 +129,12 @@ public void testDeleteEmptyDirs() throws IOException {
 	private void assertExists(final boolean expected, final String name) {
 		assertEquals(expected, new File(db.getDirectory(), name).exists());
 	}
+
+	public void testRefKeySameAsOrigName() {
+		Map<String, Ref> allRefs = db.getAllRefs();
+		for (Entry<String, Ref> e : allRefs.entrySet()) {
+			assertEquals(e.getKey(), e.getValue().getOrigName());
+
+		}
+	}
 }
-- 
1.6.0.3.578.g6a50

^ 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