Git development
 help / color / mirror / Atom feed
* Re: [PATCH 1/2] diffcore-rename: support rename cache
From: Nguyen Thai Ngoc Duy @ 2008-11-08  4:01 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Yann Dirson, git
In-Reply-To: <7vy6zvdt25.fsf@gitster.siamese.dyndns.org>

On 11/8/08, Junio C Hamano <gitster@pobox.com> wrote:
> 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?
>

I didn't. But I think all out-of-pathspec diff pairs are removed
before it reaches diffcore_rename() so the cache has nothing to do
with it (except it still loads full cache for a commit).
-- 
Duy

^ permalink raw reply

* Re: How it was at GitTogether'08 ?
From: Johan Herland @ 2008-11-08  3:41 UTC (permalink / raw)
  To: git; +Cc: Jakub Narebski, Jeff King
In-Reply-To: <200811080254.53202.jnareb@gmail.com>

On Saturday 08 November 2008, Jakub Narebski wrote:
> GitTogether 2008, which took place October 27(Mon)-29(Wed), has ended
> more than week ago.  Therefore I'd like to ask you impressions (while it
> is still fresh) from GitTogether, describe talks which are not described
> below, correct wrong information in below, etc.

Thanks for providing this summary to those of us that didn't attend.

> * Discussion on notes

Can someone elaborate on this? AFAIK, notes have popped up on this list 
often enough that I'm convinced it would be a _really_ useful feature. The 
only drawback I was aware of, was the lack of an efficient implementation, 
but then Jeff comes out of the blue and posts some interesting numbers [1] 
a week or so ago. Does this mean there are no remaining obstacles?

[1]: http://article.gmane.org/gmane.comp.version-control.git/99415


Have fun!

...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

^ permalink raw reply

* How it was at GitTogether'08 ?
From: Jakub Narebski @ 2008-11-08  1:54 UTC (permalink / raw)
  To: git

GitTogether 2008, which took place October 27(Mon)-29(Wed), has ended more
than week ago.  Therefore I'd like to ask you impressions (while it is still
fresh) from GitTogether, describe talks which are not described below,
correct wrong information in below, etc.

Please reply to this email while GitTogether is on front of your mind...


Talks at GitTogether 2008
=========================

Mon, Oct 27, 2008
-----------------
* Dscho: Contributing with Git
  http://www.youtube.com/watch?v=j45cs5_nY2k

* Junio: Git Chronicle

  blog: Junio went though a sort of statistical history of the Git project
  that was fascinating (turns out there are still about 220 lines of code
  still around from Linus original first commit).

  http://userweb.kernel.org/~junio/200810-Chron.pdf

* Jeff: Helping new contributors join

* Petr: Renames Again and Again and Again

  IRC: detection of wholesame renames of directories (WIP) and '--follow'
  limitation were mentioned, but outcome is unclear; pasky plans to hack
  together some patch implementing explicit renames hinting

* Sam: GitTorrent

  IRC: briefly the history if GitTorrent: way underannounced, not completely
  finished, GTP/0.1 was "published" long ago. There was a GSoC project this
  year, mentored by mugwump. Unfortunately, the student turned out to be a
  procrastinator or something, and this project was mainly pushed forward by
  the mentor. Now after the summer, mugwump took a step back and analyzed
  the protocol with all the experience won from implementing a large part of
  it. Turns out that a lot of the stuff was not really necessary, but copied
  because the BitTorrent protocol had it all.  So, plan is to really strip
  down GTP.  And not have a separate protocol for GitTorrent, but rather
  have it as part of git-daemon. [...] Turns out that warthog9 had a few
  really important things to say that should end up (at least in the 2nd
  version) in having a more robust discovery/exchange.

  http://utsl.gen.nz/talks/gittorrent/start.html

* Tom: GitHub

  IRC: a tour, some history, and insight into how it works; some nice
  gimmicks, such as "gist" (a git backed pastebin), or like network graphs
  (look graphically at forks of a repository).

  blog: Tom got to demonstrate GitHub and Gist to the group, most of whom
  are very command line oriented and had not used either before.

* Shawn: JGit

* Scott: Linkable Library

  blog: got to talk about the need for a linkable git library

  http://thread.gmane.org/gmane.comp.version-control.git/99608

* Sam: perl.git

  blog: Sam demonstrated the work he went through to import 20 years of Perl
  history into the git repository that the Perl team is just now finishing
  transitioning to from Perforce.

  http://utsl.gen.nz/talks/perl-history/slides/


Tue, Oct 28, 2008
-----------------
* Shawn: Pack v4

  IRC: Shawn gave a pretty good summary of packv4, and he said what are the
  major blocks: we need a refactoring of the tree walker first, and then it
  would add a pretty complex heap of code to make use of the packv4 format
  for tree walking.  Main concern was _not_ the size of pack (for spearce at
  least), but the speed of rev-list parsing with path delimiters [...] The
  tree walking needs a different interface. (Path-delimiting must be a part
  of it, not an afterthought. Mode filtering must be a part of it, not an
  afterthought.)  Estimations for packv4: 10% decrease in size, 25% increase
  in speed.  [...] reworking packv4 to do it for OBJ_BLOB too (and the
  obvious path to speed-optimize git grep -w) has not been touched at all.

* Jeff: What needs refactoring?

* David Brown: Life with Git
  http://www.davidb.org/git/git-corp.pdf

* Lighting Round Talks
  - Scott: iGitHub - git daemon and repository browser on the iPhone
  - Jeff: Portability autobuilders
  - Petr: Quick TopGit introduction, problems, future plans

  IRC: iGitHub - its a tiny app you run on your iphone to make your iphone
  act as a git server/client. since apple doesn't let you tether your laptop
  through your iphone to get wireless internet you can instead push a
  repository to the iphone via the laptop, then push from there to the
  world, or fetch from the world to the iphone, then from the iphone to the
  laptop. it also has a small history browser and stuff built in.

* Jeffrey Altman: Discussion with OpenAFS

  IRC: was about them trying to convert their franken-CVS repo to
  Git. Basically, they use CVS, but have a layer on top of it that logically
  combines two related commits into a single "delta", and they want to
  preseve those as single git commits to the extent possible (sometimes it
  is not possible because of conflicts). They are looking for ways to script
  the conversion.

  The OpenAFS guys were also interested in helping with libgit and windows
  TortoiseGit work (separate conversation).

* Tim: Git as a Media Repository
  http://www.thousandparsec.net/~tim/media+git.pdf

* Petr: Git UI

  IRC: pasky talked a little bit about what he still misses in git.git CLI:
  commit -m piping through fmt; the "reset and checkout" vs "switch, reset
  and restore" paradigm; add vs stage; a bit about submodules [...] and
  spent most of the time in a bikeshed argument about
  add/rm/stage/unstage/staged/revert/restore/switch/. [...] Basically, reset
  and checkout sucks and has chaotic behaviour with various switches and
  argument combinations (files given vs not given etc.) So in ideal world,
  reset and checkout might've been deprecated and instead, switch, restore
  and unstage would be introduced... but that's not gonna happen.

  blog: The other important, highly visual thing that was discussed, and
  even a few patches are already in for, is for little improvements to the
  UI. The full planning document is on Gist, but already things like making
  use of the term 'stage' for things that happen in the index (such as using
  "git diff --staged" instead of "git diff --cached") is being worked
  on. I'm excited that staging files may soon be done via "git stage"
  rather-than/in-addition-to "git add". This is nice for new users who often
  have a hard time seeing why you have to keep "git add"ing to stage your
  changes.

  http://pasky.or.cz/~pasky/cp/gittogether-ui.sxi
  http://gist.github.com/20818

  http://thread.gmane.org/gmane.comp.version-control.git/99572
  http://thread.gmane.org/gmane.comp.version-control.git/99541

* Steven: Life with git-svn


Wed, Oct 29, 2008
-----------------
* Shawn: Bundle Project

  IRC: Gerrit is issue tracker / code review tool, which takes full bundles
  and moves them around, and only when it was reviewed, it is actually
  applied.

  http://android.git.kernel.org/?p=tools/gerrit.git

* Discussion on notes

* Petr: introduce the secret pickaxe project
  http://repo.or.cz/w/giddy.git

* Petr: Git in Pharma Corp
  http://pasky.or.cz/~pasky/cp/gittogether-novartis.sxi
  http://pasky.or.cz/g/gitweb.cgi?p=templatedemo.git;a=summary

* Tim: Large media in Git (Repeat)

  IRC: Large = 100M and more (e.g. audio or video assets for a game). the
  idea was that you would have shallow _and_ lazy clones. There was not much
  discussion about implementation details, though; except that there should
  be a mode where blobs are _not_ fetched, and that every non-blob has to be
  local.

  IRC: Tim Ansell (mithro) of Thousand Parsec still uses SVN for this
  because he can checkout just one file, edit it, and push it
  back. Basically he wants to clone the metadata (commits and trees and
  small blobs) and have large blobs (e.g. >25 MB or some limit) kept on the
  origin, lazily fetched over a git transport on demand. Tim started to work
  up a prototype using HTTP. folks thought the idea showed promise, but we
  need to see code and the impacts it has and where it breaks down to really
  understand if it is worth doing or not.

  blog: Tim talked about something that I think will be one of the next huge
  (highly visible) changes in Git you\u2019re likely to see in the next year
  \u2013 handling large meda well, and being able to do narrow and sparse
  clones, (and shallow clones better). This means being able to clone part
  of a Git repository, such as just the last revision (shallow), just the
  \u2018lib\u2019 directory (narrow) or just a single file
  (sparse). Importantly, you would be able to see the history of everything
  still (it would download the commit and tree objects, which are generally
  small, but not the larger blobs), and you would be able to do pushes back
  (which shallow clones can\u2019t currently do).

  http://www.thousandparsec.net/~tim/media+git.pdf  

* JH: $$$ discussion

  IRC: It was about the Google Summer of Code 2008 mentor money. It was
  decided (given most of the mentors were there) to use it to help a few
  folks cover their travel expenses to the GitTogether. Most folks actually
  had their costs covered by their employer (yay for some big companies!)
  but a few paid out of pocket, so we are helping them defray it a bit. and
  the rest is being used to order and distribute some git t-shirts.

* Sam: Git as a DB Backend

  IRC: mugwump had this idea of running a sql sort of database in git, using
  the git logs as a way to implement an ACID complaint database. to be
  honest i was busy reading email or something and didn't pay much attention
  to his talk, but folks seemed to think it was an abuse of git

* Petr: GUI Even a Designer Uses

  IRC: it was rather short and ambiguous; mostly arguing whether to and in
  what language/toolkit to reimplement git-gui


Sources:
========
* http://git.or.cz/gitwiki/GitTogether
* http://google-opensource.blogspot.com/2008/11/gittogether-08.html
* http://github.com/blog/196-gittogether-2008
* Calendar ID: amj5rbmljs66k9rtibs34kc2c8@group.calendar.google.com
* http://colabti.org/irclogger/irclogger_log_search/git
  #git channel on irc.freenode.org


-- 
Jakub Narebski

^ permalink raw reply

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

On Fri, Nov 07, 2008 at 04:47:46PM -0800, Junio C Hamano wrote:
> Yann Dirson <ydirson@altern.org> writes:
> 
> > ...  Commands
> > for saving/mailing patches could issue a bold warning if the user
> > specifies the for-human-viewing flag.
> 
> If it is a warning to the user who produces the patch, and not but a
> warning in the patch text for the user who receives it, it is not useful.

OK to get everyone warned :)

^ permalink raw reply

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

Yann Dirson <ydirson@altern.org> writes:

> ...  Commands
> for saving/mailing patches could issue a bold warning if the user
> specifies the for-human-viewing flag.

If it is a warning to the user who produces the patch, and not but a
warning in the patch text for the user who receives it, it is not useful.

> I was talking about exchanging patches with the non-git part of the
> world.

Renaming patch needs manual massaging if you want to use GNU patch and
that is not a new issue.  People know that.

The problem is in your example output were that it would break _existing
git tools_ with a backward incompatible change, when you did not have to.
That's the difference.

^ permalink raw reply

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

On Fri, Nov 07, 2008 at 03:43:19PM -0800, Junio C Hamano wrote:
> 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).

Sure in theory.  But in practice I do not remember one time when, if
all files from one dir are moved in one branch, the files added on
another in the same dir were not bound to be moved as well.

Anway, if we feel git-apply should not decide without the user
knowing, we can make it refuse by default, with options to do either
way, and one option to ask for each doubtful file instead.


>  * 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.

Right.

> 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.

OK, so I realize we need 2 things here: one format for diff-exporting
with complete info, and one for human viewing (which is, again, the
primary reason why I needed this feature, so I'm not very keen on
letting all this work finally not being useful for me :).  Commands
for saving/mailing patches could issue a bold warning if the user
specifies the for-human-viewing flag.


> > 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 ;-)).

I was talking about exchanging patches with the non-git part of the
world.  The point is that eg. GNU patch still happily accepts
git-generated files but produces nonsense using some, exactly because
it ignores meaningful data which (by design ?) appear to it to be
legal to ignore.

^ permalink raw reply

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

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

On Fri, Nov 07, 2008 at 09:37:29PM +0000, 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

> 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).

FWIW Patience diff wouldn't help at all here. Patience diff is just a
matter of preseeding your preferred diff algorithm with better (wrt
human readability) candidate for the invariant lines. IOW it helps
dividing the problem into smaller bits, but requires *unique lines* to
start with. If you haven't any, then basically, Patience diff does
nothing and calls your usual diff algorithm on the whole files.

It does so in a pseudo linear complexity, hence should not make overall
time really worse, but will not help for the ending time usually either.

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

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

^ permalink raw reply

* Re: 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


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