Git development
 help / color / mirror / Atom feed
* Re: GitWeb: Adding fork visualization based on objects/info/alternates
From: Petr Baudis @ 2007-05-28 13:00 UTC (permalink / raw)
  To: Marius Storm-Olsen; +Cc: Git Mailing List
In-Reply-To: <46587CE5.1030501@trolltech.com>

  Hi,

On Sat, May 26, 2007 at 08:31:01PM CEST, Marius Storm-Olsen wrote:
> In an effort to tidy up the project list, and given that the
> current fork support in GitWeb doesn't exactly fit my
> structure (where forks are in subdirs of parent repo, and has
> to be first level down; alas no seek unlike when not showing
> forks), I set out to add fork support based on
>     objects/info/alternates
> of each repo. The result can be seen here:
>     http://chaos.troll.no/~marius/git/forks_collapsed.png
>     http://chaos.troll.no/~marius/git/forks_expanded.png
> 
> I doubt you'd want it in the mainline, as you can't turn the
> feature off; it's just like another column which you can sort
> by. If you do not sort by the 'fork' column, the forks will
> still be colored with the first level color, but not ordered
> under it's parent repo.
> 
> Just wanted to feed back what I did so others can get
> inspiration, tweak it, and add it to their own gitwebs.
> 
> It cleans up the project list real nicely though.

  I think this patch is quite an interesting idea, and I kind of like
the possibility to show forks in the summary project list, if it's
optional (I actually don't think if I would enable this say at
repo.or.cz - some projects have quite a lot of forks and it would
clutter it up a lot e.g. for text-mode browsers that don't grok
enough javascript). OTOH, taking fork info from alternates is more
controversial; I don't want to force the repo.or.cz forks model on
everyone but having $proj in alternates may not mean it is a fork... but
if more people think it's a reasonable heuristic, we might optionally
support it too.

  If you would be willing to split this patch to two and make both
features optional (and sign off the patch), I think at least the
forks-in-summary part might be quite a worthy addition.

> @@ -1909,8 +1919,9 @@ EOF
>  		print qq(<link rel="shortcut icon" href="$favicon" type="image/png"/>\n);
>  	}
> 
> +	print "<script src=\"gitweb_scripts.js\" type=\"text/javascript\"></script>\n";
>  	print "</head>\n" .
> -	      "<body>\n";
> +	      "<body onload=\"collapseAllRows();\">\n";
> 
>  	if (-f $site_header) {
>  		open (my $fd, $site_header);

Would it be more reasonable to just set the style for these rows by
default to display: none?

> --- /dev/null
> +++ b/gitweb/gitweb_scripts.js

We have other patch introducing blame.js, so maybe call this forks.js?
Or do we have a strong reason to prefer a single .js file with all the
lot?

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
Ever try. Ever fail. No matter. // Try again. Fail again. Fail better.
		-- Samuel Beckett

^ permalink raw reply

* Re: [PATCH 00/15] git-note: A mechanisim for providing free-form after-the-fact annotations on commits
From: Johan Herland @ 2007-05-28 10:54 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: git, Junio C Hamano
In-Reply-To: <alpine.LFD.0.98.0705272131370.26602@woody.linux-foundation.org>

On Monday 28 May 2007, Linus Torvalds wrote:
> 
> On Sun, 27 May 2007, Johan Herland wrote:
> > 
> > I've been working on combining tag objects and --decorate into a useful
> > proof-of-concept that provides the after-the-fact commit annotations I
> > requested above, and here's the result:
> 
> Ok, looks fine to me. I do have a few questions:
>  - why don't you just let people name their notes, the same way we name 
>    tags (and then actually using it as the note name?)
> 
>    Putting them in the refs/notes/ filesystem by their SHA1 seems a bit 
>    wasteful, and it would seem that it could be quite nice to name the 
>    notes some way?

Well, when I first designed them, I thought of notes more as an extension
to the commit message than as a special case of tags. Therefore naming
them didn't seem natural at that point. But now that it all seems to
blend together (cf. Junio's answer), I'm not opposed to putting names
on notes at all. But I don't want to make naming mandatory. It should
still be possible to add a note like "Fixes bug #12345" in a jiffy,
without having to think up a note name as well (or having a badly
thought-up name pollute some namespace).

So I guess we end up with two kinds of objects/entities:
1. tags, with mandatory naming and optional comment/message
2. notes, with optional naming and mandatory comment/message

To some degree we can say that the name is the ref, and the
comment/message is the object. We therefore get:

1. tags, with mandatory ref and optional tag object (we have this today)
2. notes, with optional ref and mandatory object

Today, a note needs its ref in order to stay alive, but this is purely a
(badly designed) technical measure. When we get the proper reverse mapping
done (as discussed in Junio's answer), notes will stop polluting refs/ and
we can instead allow naming of notes by adding simple (tag-type) refs to
note objects.

Also, as mentioned by Junio, we want to do the reverse mapping for regular
tags as well.

At this point, there's no real difference between tags and notes (some
would say there never were a difference), and we can stop caring about
whether a note is a note or a tag, etc, at least in the plumbing.

We may still want to enforce a difference in the procelain, though.
When presenting tags/notes (e.g. with --decorate), users might be
interested in classifying and filtering their tags/notes in different
categories. Categories might be ("note", "tag"), or we can make it
user-extensible, like Junio's "keyword" idea). Either way, we'll have
to make room for this classification in the tag object by adding another
header ("tagtype", "keyword", whatever...).

And while we're on the subject of changing the tag object, I'd like for
the "tag" header (the one holding the tag name) to become optional.
When doing my ref <-> name trick above, I conveniently forgot this little
bugger. Basically the only reason for this one to exist is to include the
name of the tag in the data passed to gpg for signing. This is of course
necesssary in order to make renaming a signed tag impossible. (Allowing
renaming would make it possible to replace it with a malicious tag with
the original name.) Therefore the "tag" header must be mandatory for
signed tags. But for all other tags (including notes) this header is
pretty much useless.

>  - This will probably scale horribly badly if you have tens of thousands 
>    of notes, even when they are packed. Do we care?

I can't see why the current implementation would scale any worse than an
equivalent number of (annotated/signed) tags. But then again, the tag
system might not have been designed with tens of thousands of tag objects
in mind. :)

> Other than that, it looks straightforward and sane.

Thanks for the feedback


Have fun!

...Johan

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

^ permalink raw reply

* Re: [PATCH] gitweb: Update copyright information
From: Jan-Benedict Glaw @ 2007-05-28 10:27 UTC (permalink / raw)
  To: Jakub Narebski; +Cc: git
In-Reply-To: <200705281158.47128.jnareb@gmail.com>

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

On Mon, 2007-05-28 11:58:46 +0200, Jakub Narebski <jnareb@gmail.com> wrote:
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -2,6 +2,7 @@
>  
>  # gitweb - simple web interface to track changes in git repositories
>  #
> +# (C) 2006-2007, Git Development Community <git@vger.kernel.org>
>  # (C) 2005-2006, Kay Sievers <kay.sievers@vrfy.org>
>  # (C) 2005, Christian Gierke
>  #

Am I the only person that doesn't think that there's a community based
copyright? Isn't it a matter of people (or a company in case of paid
work by an employee)???

MfG, JBG

-- 
      Jan-Benedict Glaw      jbglaw@lug-owl.de              +49-172-7608481
Signature of:              What we do for ourselves dies with us. What we do for
the second  :         others and the world remains and is immortal. (Albert Pine)

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

^ permalink raw reply

* [PATCH] gitweb: Update copyright information
From: Jakub Narebski @ 2007-05-28  9:58 UTC (permalink / raw)
  To: git

Update copyright information in file header, and remove obsolete
copyright information from the comment in HTML head, in the output.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
Just a thought...

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index f73f184..d51ae84 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2,6 +2,7 @@
 
 # gitweb - simple web interface to track changes in git repositories
 #
+# (C) 2006-2007, Git Development Community <git@vger.kernel.org>
 # (C) 2005-2006, Kay Sievers <kay.sievers@vrfy.org>
 # (C) 2005, Christian Gierke
 #
@@ -2072,7 +2073,7 @@ sub git_header_html {
 <?xml version="1.0" encoding="utf-8"?>
 <!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">
 <html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en-US" lang="en-US">
-<!-- git web interface version $version, (C) 2005-2006, Kay Sievers <kay.sievers\@vrfy.org>, Christian Gierke -->
+<!-- git web interface version $version -->
 <!-- git core binaries version $git_version -->
 <head>
 <meta http-equiv="content-type" content="$content_type; charset=utf-8"/>
-- 
1.5.2

^ permalink raw reply related

* [PATCH (tig)] Infrastructure for tig rpm builds.
From: Jakub Narebski @ 2007-05-28  8:54 UTC (permalink / raw)
  To: Jonas Fonseca; +Cc: git

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

Adds RPM_VERSION to Makefile and new make targets: tig.spec, dist, and
rpm. A simple 'make rpm' will build the rpm. Also adds tig.spec.in
which is used to generate tig.spec.

Accidentally VERSION (and adding -DVERSION=$(VERSION) to CFLAGS) is
now defined always, even if we do not compile from the "live" tig
repository.

Minimally tested (meaning "make rpm" from tig repository works).

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
'tig.spec.in' is based on 'git.spec.in' from git repository.

This is the initial, minimal version.  For example it does not add
TIG-VERSION-GEN file and the rest of versioning infrastructure that is
used in git Makefile.

Commands used to get version string (in RPM_VERSION) suitable as a
version field in the rpm can, most probably, be improved.  Perhaps
(with the new git-describe output) we should put closest version tag
as version of tig in rpm, and distance from the tag (perhaps plus 1)
as release number (in the rpm sense).

TODO: Change '--without docs' to '--with rebuild-docs' and use
pre-generated docs when creating rpm, instead of generating them (and
requiring asciidoc, xmlto and docbook2pdf to build rpm if we want tig
documentation).


P.S. I have build tig-0.6.4.g9eded37-1.i386.rpm using "make rpm" from
tig repository, and installed it without problems, but when trying to
build it again _without network_ it failed on building
manual.pdf. Error log attached.

  asciidoc-7.1.2-2.fc3.rf
  docbook-dtds-1.0-26
  docbook-style-dsssl-1.79-1
  docbook-style-xsl-1.68.1-1
  docbook-utils-0.6.14-4
  docbook-utils-pdf-0.6.14-4
  jadetex-3.12-13
  openjade-1.3.2-16

I think we can skip generation of manual.pdf, and generate it only on
request (e.g. "make doc-pdf"), or using doc-man and doc-html in place
of doc in the spec file... but this is better left for the next
commit.

By the way, why do you use xmlto and docbook2pdf, instead of a2x from
asciidoc package?

 .gitignore  |    2 ++
 Makefile    |   34 ++++++++++++++++++++++++++++++++--
 tig.spec.in |   56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 90 insertions(+), 2 deletions(-)
 create mode 100644 tig.spec.in

diff --git a/.gitignore b/.gitignore
index 8f6aa93..f7ab840 100644
--- a/.gitignore
+++ b/.gitignore
@@ -2,3 +2,5 @@ manual.html-chunked
 manual.pdf
 manual.toc
 tig
+tig.spec
+tig-*.tar.gz
diff --git a/Makefile b/Makefile
index 06a5d6a..45c833b 100644
--- a/Makefile
+++ b/Makefile
@@ -18,8 +18,16 @@ DOCS	= $(DOCS_MAN) $(DOCS_HTML) \
 ifneq (,$(wildcard .git))
 VERSION = $(shell git-describe)
 WTDIRTY = $(shell git-diff-index --name-only HEAD 2>/dev/null)
-CFLAGS += '-DVERSION="$(VERSION)$(if $(WTDIRTY),-dirty)"'
+else
+GEN_VER="tig-0.6.TIG"
+VERSION = $(shell test -f version && cat version || echo "$(GEN_VER)")
 endif
+CFLAGS += '-DVERSION="$(VERSION)$(if $(WTDIRTY),-dirty)"'
+RPM_VERSION  = $(subst tig-,,$(VERSION))
+RPM_VERSION := $(subst -,.,$(RPM_VERSION))
+
+TARNAME=tig-$(RPM_VERSION)
+
 
 all: $(PROGS)
 all-debug: $(PROGS)
@@ -57,6 +65,9 @@ install-doc: install-doc-man install-doc-html
 clean:
 	rm -rf manual.html-chunked
 	rm -f $(PROGS) $(DOCS) core *.xml
+	rm -f *.spec
+	rm -rf $(TARNAME)
+	rm -f $(TARNAME).tar.gz
 
 spell-check:
 	aspell --lang=en --check tig.1.txt tigrc.5.txt manual.txt
@@ -64,7 +75,7 @@ spell-check:
 strip: all
 	strip $(PROGS)
 
-.PHONY: all all-debug doc doc-man doc-html install install-doc install-doc-man install-doc-html clean spell-check
+.PHONY: all all-debug doc doc-man doc-html install install-doc install-doc-man install-doc-html clean spell-check dist rpm
 
 manual.html: manual.toc
 manual.toc: manual.txt
@@ -119,3 +130,22 @@ sync-docs:
 	make doc
 	cg commit -m "Sync docs"
 	cg switch master
+
+tig.spec: tig.spec.in
+	sed -e 's/@@VERSION@@/$(RPM_VERSION)/g' < $< > $@+
+	mv $@+ $@
+
+dist: tig.spec
+	git-archive --format=tar \
+		--prefix=$(TARNAME)/ HEAD^{tree} > $(TARNAME).tar
+	@mkdir -p $(TARNAME)
+	@cp tig.spec $(TARNAME)
+	@echo $(VERSION) > $(TARNAME)/version
+	tar rf $(TARNAME).tar \
+	       $(TARNAME)/tig.spec \
+	       $(TARNAME)/version
+	@rm -rf $(TARNAME)
+	gzip -f -9 $(TARNAME).tar
+
+rpm: dist
+	rpmbuild -ta $(TARNAME).tar.gz
diff --git a/tig.spec.in b/tig.spec.in
new file mode 100644
index 0000000..2ce6cdb
--- /dev/null
+++ b/tig.spec.in
@@ -0,0 +1,56 @@
+# -*- mode: rpm-spec-mode; encoding: utf-8; -*-
+# Pass '--without docs' to rpmbuild if you don't want the documentation
+
+Summary: 	Tig: text-mode interface for git
+Name: 		tig
+Version: 	@@VERSION@@
+Release: 	1%{?dist}
+License: 	GPL
+Group: 		Development/Tools
+Vendor: 	Jonas Fonseca <fonseca@diku.dk>
+URL: 		http://jonas.nitro.dk/tig/
+Source: 	http://jonas.nitro.dk/tig/releases/%{name}-%{version}.tar.gz
+BuildRequires: 	ncurses-devel%{!?_without_docs:, xmlto, asciidoc > 6.0.3, /usr/bin/docbook2pdf}
+BuildRoot: 	%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
+Requires: 	git-core, ncurses
+
+%description
+Tig is a git repository browser that additionally can act as a pager
+for output from various git commands.
+
+When browsing repositories, it uses the underlying git commands to
+present the user with various views, such as summarized revision log
+and showing the commit with the log message, diffstat, and the diff.
+
+Using it as a pager, it will display input from stdin and colorize it.
+
+%prep
+%setup -q
+
+%build
+CFLAGS="$RPM_OPT_FLAGS -DVERSION=tig-%{version}-%{release}"
+%{__make} %{_smp_mflags} \
+	prefix=%{_prefix} \
+	all %{!?_without_docs: doc}
+
+%install
+[ "$RPM_BUILD_ROOT" != "/" ] && rm -rf $RPM_BUILD_ROOT
+CFLAGS="$RPM_OPT_FLAGS -DVERSION=tig-%{version}-%{release}"
+%{__make} %{_smp_mflags} DESTDIR=$RPM_BUILD_ROOT \
+	prefix=%{_prefix} bindir=%{_bindir} mandir=%{_mandir} \
+	install %{!?_without_docs: install-doc-man}
+
+%clean
+[ "$RPM_BUILD_ROOT" != "/" ] && rm -rf $RPM_BUILD_ROOT
+
+%files
+%defattr(-,root,root)
+%{_bindir}/*
+%doc README COPYING INSTALL SITES BUGS TODO tigrc
+%{!?_without_docs: %{_mandir}/man1/*.1*}
+%{!?_without_docs: %{_mandir}/man5/*.5*}
+%{!?_without_docs: %doc *.html *.pdf}
+
+%changelog
+* Mon May 28 2007 Jakub Narebski <jnareb@gmail.com>
+- Initial tig spec file
-- 
1.5.2


[-- Attachment #2: Selected parts of error log for "make rpm" --]
[-- Type: text/plain, Size: 2208 bytes --]

docbook2pdf manual.xml
Using catalogs: /etc/sgml/xml-docbook-4.2-1.0-26.cat
Using stylesheet: /usr/share/sgml/docbook/utils-0.6.14/docbook-utils.dsl#print
Working on: /home/local/builddir/BUILD/tig-0.6.4.g9eded37/manual.xml
jade:/home/local/builddir/BUILD/tig-0.6.4.g9eded37/manual.xml:2:118:E: could not resolve host "www.oasis-open.org" (try again later)
jade:/home/local/builddir/BUILD/tig-0.6.4.g9eded37/manual.xml:2:118:E: DTD did not contain element declaration for document type name
jade:/home/local/builddir/BUILD/tig-0.6.4.g9eded37/manual.xml:4:14:E: there is no attribute "lang"
jade:/home/local/builddir/BUILD/tig-0.6.4.g9eded37/manual.xml:4:18:E: element "article" undefined

[...]

jade:/home/local/builddir/BUILD/tig-0.6.4.g9eded37/manual.xml:400:10:E: element "entry" undefined
jade:/home/local/builddir/BUILD/tig-0.6.4.g9eded37/manual.xml:401:172:E: element "emphasis" undefined
jade:/home/local/builddir/BUILD/tig-0.6.4.g9eded37/manual.xml:404:6:E: element "row" undefined
jade:I: maximum number of errors (200) reached; change with -E option
jade:/usr/share/sgml/docbook/dsssl-stylesheets-1.79/print/../common/dbtable.dsl:224:13:E: 2nd argument for primitive "ancestor" of wrong type: "#<unknown object 152549536>" not a singleton node list
jade:/usr/share/sgml/docbook/dsssl-stylesheets-1.79/print/../common/dbtable.dsl:224:13:E: 2nd argument for primitive "ancestor" of wrong type: "#<unknown object 152549536>" not a singleton node list
jade:/usr/share/sgml/docbook/dsssl-stylesheets-1.79/print/../common/dbtable.dsl:224:13:E: 2nd argument for primitive "ancestor" of wrong type: "#<unknown object 152549536>" not a singleton node list

[...]

jade:/usr/share/sgml/docbook/dsssl-stylesheets-1.79/print/../common/dbtable.dsl:224:13:E: 2nd argument for primitive "ancestor" of wrong type: "#<unknown object 152549536>" not a singleton node list
jade:/usr/share/sgml/docbook/dsssl-stylesheets-1.79/print/../common/dbtable.dsl:224:13:E: 2nd argument for primitive "ancestor" of wrong type: "#<unknown object 152549536>" not a singleton node list
make[1]: *** [manual.pdf] Błąd 8
rm manual.xml tigrc.5.xml tig.1.xml
make[1]: Leaving directory `/home/local/builddir/BUILD/tig-0.6.4.g9eded37'

^ permalink raw reply related

* Re: [RFC/PATCH] gitweb: Create special from-file/to-file header for combined diff
From: Jakub Narebski @ 2007-05-28  6:50 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <7v1wh1en8k.fsf@assigned-by-dhcp.cox.net>

Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
>> Instead of using default, diff(1) like from-file/to-file header for
>> combined diff (for a merge commit), which looks like:
>>
>>   --- a/git-gui/git-gui.sh
>>   +++ b/_git-gui/git-gui.sh_
>>
>> (where _link_ denotes [hidden] hyperlink), create from-file(n)/to-file
>> header, using n/file for each or parents, e.g.:
>>
>>   --- 1/_git-gui/git-gui.sh_
>>   --- 2/_git-gui.sh_
>>   +++ b/_git-gui/git-gui.sh_
> 
> Sounds quite straightforward to implement, and diff with 1/
> would be useful to recreate what the person who did the merge
> pulled in, for most of the time.  I suspect diff with 2/ is
> almost always uninteresting, though.

Errr... it _is_ implemented in this patch, although code is not perfect
and has some unnecessary repetitions. But I thought before improving
code would it make sense to do this... and perhaps even add generated
extended diff headers for renames:

  rename at _2_ parent from _git-gui.sh_
  rename to _git-gui/git-gui.sh_

where as before _link_ denotes hyperlinked part. Hmmm?

-- 
Jakub Narebski
Poland

^ permalink raw reply

* Re: [PATCH 00/15] git-note: A mechanisim for providing free-form after-the-fact annotations on commits
From: Linus Torvalds @ 2007-05-28  4:37 UTC (permalink / raw)
  To: Johan Herland; +Cc: git, Junio C Hamano
In-Reply-To: <200705271608.02122.johan@herland.net>



On Sun, 27 May 2007, Johan Herland wrote:
> 
> I've been working on combining tag objects and --decorate into a useful
> proof-of-concept that provides the after-the-fact commit annotations I
> requested above, and here's the result:

Ok, looks fine to me. I do have a few questions:
 - why don't you just let people name their notes, the same way we name 
   tags (and then actually using it as the note name?)

   Putting them in the refs/notes/ filesystem by their SHA1 seems a bit 
   wasteful, and it would seem that it could be quite nice to name the 
   notes some way?

 - This will probably scale horribly badly if you have tens of thousands 
   of notes, even when they are packed. Do we care?

Other than that, it looks straightforward and sane.

		Linus

^ permalink raw reply

* Re: [PATCH 1/3] Lazily open pack index files on demand
From: A Large Angry SCM @ 2007-05-28  2:30 UTC (permalink / raw)
  To: Dana How; +Cc: Shawn O. Pearce, Nicolas Pitre, Junio C Hamano, git
In-Reply-To: <56b7f5510705271835m5a375324p3a908fe766fdf902@mail.gmail.com>

Dana How wrote:
[...]
> 
> Some history of what I've been doing with git:
> First I simply had to import the repo,
> which led to split packs (this was before index v2).
> Then maintaining the repo led to the unfinished maxblobsize stuff.
> Distributing the repo included users pulling (usually) from the central 
> repo,
> which would be trivial since it was also an alternate.
> Local repacking would avoid heavy load on it.
> 
> Now I've started looking into how to push back into the
> central repo from a user's repo (not everything will be central;
> some pulling between users will occur
> otherwise I wouldn't be as interested).
> 
> It looks like the entire sequence is:
> A. git add file [compute SHA-1 & compress file into objects/xx]
> B. git commit [write some small objects locally]
> C. git push {using PROTO_LOCAL}:
> 1. read & uncompress objects
> 2. recompress objects into a pack and send through a pipe
> 3. read pack on other end of pipe and uncompress each object
> 4. compute SHA-1 for each object and compress file into objects/xx
> 
> So, after creating an object in the local working tree,
> to get it into the central repo,  we must:
> compress -> uncompress -> compress -> uncompress -> compress.
> In responsiveness this won't compare very well to Perforce,
> which has only one compress step.
> 
> The sequence above could be somewhat different currently in git.
> The user might have repacked their repo before pushing,
> but this just moves C1 and C2 back earlier in time,
> it doesn't remove the need for them.  Besides,  the blobs in
> a push are more likely to be recent and hence unpacked.
> 
> Also,  C3 and C4 might not happen if more than 100 blobs get pushed.
> But this seems very unusual; only 0.3% of commits in the history
> had 100+ new files/file contents.  If the 100 level is reduced,
> then the central repo fills up with packfiles and their index files,
> reducing performance for everybody (using the central repo as an 
> alternate).
> 
> Thus there really is 5X more compression activity going on
> compared to Perforce.  How can this be reduced?
> 
> One way is to restore the ability to write the "new" loose object format.
> Then C1, C2, and C4 disappear.  C3 must remain because we need
> to uncompress the object to compute its SHA-1;  we don't need
> to recompress since we were already given the compressed form.
> 
> And that final sentence is why I sent this email:  if the packfile
> contained the SHA-1s,  either at the beginning or before each object,
> then they wouldn't need to be recomputed at the receiving end
> and the extra decompression could be skipped as well.  This would
> make the total zlib effort the same as Perforce.
> 
> The fact that a loose object is never overwritten would still be retained.
> Is that sufficient security?  Or does the SHA-1 always need to be
> recomputed on the receiving end?  Could that be skipped just for
> specific connections and/or protocols (presumably "trusted" ones)?
[...]

So how do you want to decide when to trust the sender and when to 
validate that the objects received have the SHA-1's claimed? A _central_ 
repository, being authoritative, would need to _always_ validate _all_ 
objects it receives. An since, with a central repository setup, the 
central repository is where the CPU resources are the most in demand, 
validating the object IDs when received at the developers repositories 
should not be a problem. And just to be fair, how does Perforce 
guarantee that the retrieved version of a file matches what was checked in?

^ permalink raw reply

* Re: [PATCH 1/3] Lazily open pack index files on demand
From: Nicolas Pitre @ 2007-05-28  2:18 UTC (permalink / raw)
  To: Shawn O. Pearce; +Cc: Dana How, Junio C Hamano, git
In-Reply-To: <20070527213525.GC28023@spearce.org>

On Sun, 27 May 2007, Shawn O. Pearce wrote:

> Nicolas Pitre <nico@cam.org> wrote:
> > On Sat, 26 May 2007, Dana How wrote:
> > 
> > > On 5/26/07, Shawn O. Pearce <spearce@spearce.org> wrote:
> > > > In pack v4 we're likely to move the SHA-1 table from the .idx file
> > > > into the front of the .pack file.  This makes the .idx file hold
> > > > only the offsets and the CRC checkums of each object.  If we start
> > > > making a super index, we have to duplicate the SHA-1 table twice
> > > > (once in the .pack, again in the super index).
> > > 
> > > Hmm, hopefully the SHA-1 table can go at the _end_
> > > since with split packs that's the only time we know the number
> > > of objects in the pack... ;-)
> > 
> > Hmmm good point to consider.
> 
> The problem with putting the SHA-1 table at the end of the pack is
> it ruins the streaming for both unpack-objects and index-pack if
> we were to ever use pack v4 as a transport format.  Or just try
> to run a pack v4 packfile through unpack-objects, just locally,
> say to extract megablobs.  ;-)

Right.  In fact I think the SHA1 table could still remain at the 
beginning even if we don't know yet that the pack will be split. It 
would just happen to contain redundent entries.

In fact it would be impossible to store it at the end in the hope of 
trimming it down according to the written objects because the idea is to 
have commit and tree objects index into that table.  So if you cull 
entries from the table then indices in already written out objects won't 
match.


Nicolas

^ permalink raw reply

* Re: [RFC/PATCH] gitweb: Create special from-file/to-file header for combined diff
From: Junio C Hamano @ 2007-05-28  1:43 UTC (permalink / raw)
  To: Jakub Narebski; +Cc: git
In-Reply-To: <11803077792064-git-send-email-jnareb@gmail.com>

Jakub Narebski <jnareb@gmail.com> writes:

> Instead of using default, diff(1) like from-file/to-file header for
> combined diff (for a merge commit), which looks like:
>
>   --- a/git-gui/git-gui.sh
>   +++ b/_git-gui/git-gui.sh_
>
> (where _link_ denotes [hidden] hyperlink), create from-file(n)/to-file
> header, using n/file for each or parents, e.g.:
>
>   --- 1/_git-gui/git-gui.sh_
>   --- 2/_git-gui.sh_
>   +++ b/_git-gui/git-gui.sh_

Sounds quite straightforward to implement, and diff with 1/
would be useful to recreate what the person who did the merge
pulled in, for most of the time.  I suspect diff with 2/ is
almost always uninteresting, though.

^ permalink raw reply

* Re: [PATCH 1/3] Lazily open pack index files on demand
From: Dana How @ 2007-05-28  1:35 UTC (permalink / raw)
  To: Shawn O. Pearce; +Cc: Nicolas Pitre, Junio C Hamano, git, danahow
In-Reply-To: <20070527213525.GC28023@spearce.org>

On 5/27/07, Shawn O. Pearce <spearce@spearce.org> wrote:
> Nicolas Pitre <nico@cam.org> wrote:
> > On Sat, 26 May 2007, Dana How wrote:
> > > On 5/26/07, Shawn O. Pearce <spearce@spearce.org> wrote:
> > > > In pack v4 we're likely to move the SHA-1 table from the .idx file
> > > > into the front of the .pack file.  This makes the .idx file hold
> > > > only the offsets and the CRC checkums of each object.  If we start
> > > > making a super index, we have to duplicate the SHA-1 table twice
> > > > (once in the .pack, again in the super index).
> > > Hmm, hopefully the SHA-1 table can go at the _end_
> > > since with split packs that's the only time we know the number
> > > of objects in the pack... ;-)
> > Hmmm good point to consider.
> The problem with putting the SHA-1 table at the end of the pack is
> it ruins the streaming for both unpack-objects and index-pack if
> we were to ever use pack v4 as a transport format.  Or just try
> to run a pack v4 packfile through unpack-objects, just locally,
> say to extract megablobs.  ;-)

Perhaps I have stumbled on another issue related to
including SHA-1s in packfiles.  This is completely independent
of the handling of megablobs (my current focus),  but the presence
of large blobs do make this issue more apparent.

Some history of what I've been doing with git:
First I simply had to import the repo,
which led to split packs (this was before index v2).
Then maintaining the repo led to the unfinished maxblobsize stuff.
Distributing the repo included users pulling (usually) from the central repo,
which would be trivial since it was also an alternate.
Local repacking would avoid heavy load on it.

Now I've started looking into how to push back into the
central repo from a user's repo (not everything will be central;
some pulling between users will occur
otherwise I wouldn't be as interested).

It looks like the entire sequence is:
A. git add file [compute SHA-1 & compress file into objects/xx]
B. git commit [write some small objects locally]
C. git push {using PROTO_LOCAL}:
 1. read & uncompress objects
 2. recompress objects into a pack and send through a pipe
 3. read pack on other end of pipe and uncompress each object
 4. compute SHA-1 for each object and compress file into objects/xx

So, after creating an object in the local working tree,
to get it into the central repo,  we must:
compress -> uncompress -> compress -> uncompress -> compress.
In responsiveness this won't compare very well to Perforce,
which has only one compress step.

The sequence above could be somewhat different currently in git.
The user might have repacked their repo before pushing,
but this just moves C1 and C2 back earlier in time,
it doesn't remove the need for them.  Besides,  the blobs in
a push are more likely to be recent and hence unpacked.

Also,  C3 and C4 might not happen if more than 100 blobs get pushed.
But this seems very unusual; only 0.3% of commits in the history
had 100+ new files/file contents.  If the 100 level is reduced,
then the central repo fills up with packfiles and their index files,
reducing performance for everybody (using the central repo as an alternate).

Thus there really is 5X more compression activity going on
compared to Perforce.  How can this be reduced?

One way is to restore the ability to write the "new" loose object format.
Then C1, C2, and C4 disappear.  C3 must remain because we need
to uncompress the object to compute its SHA-1;  we don't need
to recompress since we were already given the compressed form.

And that final sentence is why I sent this email:  if the packfile
contained the SHA-1s,  either at the beginning or before each object,
then they wouldn't need to be recomputed at the receiving end
and the extra decompression could be skipped as well.  This would
make the total zlib effort the same as Perforce.

The fact that a loose object is never overwritten would still be retained.
Is that sufficient security?  Or does the SHA-1 always need to be
recomputed on the receiving end?  Could that be skipped just for
specific connections and/or protocols (presumably "trusted" ones)?

Note that none of this depends on how megablobs are handled,
but large blobs certainly do make the 5X more zlib activity more apparent.

Shawn:  I think you mentioned something related to this a few days ago.
Also, you didn't like split packs putting SHA-1s at the end because
that messed up streaming for transport, but packs are not split for transport.

Thanks,
-- 
Dana L. How  danahow@gmail.com  +1 650 804 5991 cell

^ permalink raw reply

* Re: [PATCH 00/15] git-note: A mechanisim for providing free-form after-the-fact annotations on commits
From: Jakub Narebski @ 2007-05-28  0:59 UTC (permalink / raw)
  To: git
In-Reply-To: <7vejl2do5w.fsf@assigned-by-dhcp.cox.net>

Junio C Hamano wrote:

> You can create a tag object that points at another tag.  When we
> peel a tag using "^{}" notation, currently it is peeled all the
> way until we hit a non tag, so if you need to look at
> intermediate tags, you need to parse them yourself.  IOW, there
> is no Porcelain feature to let you take advantage of this
> capability, easily.  But we could use this to say "I attest that
> he tagged that object".

Hmmm... I wonder why "git show exaples/tag" (where 'examples/tag' is
tag to tag) does show empty output, and why "git show examples/tag^{tag}"
does not work...

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

^ permalink raw reply

* Re: svn-cp equivalent for history on a single file from a git-svn user.
From: Jakub Narebski @ 2007-05-28  0:41 UTC (permalink / raw)
  To: git
In-Reply-To: <20070527101430.GA27013@curie-int.orbis-terrarum.net>

Robin H. Johnson wrote:

> The Git docs say to use plain 'cp' where svn-cp would be used, as Git
> detects copies after-the-fact.

But I think you need to _request_ detecting copies with '-C' option.

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

^ permalink raw reply

* Re: GIT on MinGW problem
From: Han-Wen Nienhuys @ 2007-05-28  0:31 UTC (permalink / raw)
  To: git; +Cc: Jakub Narebski, Steven Grimm, Shawn O. Pearce
In-Reply-To: <Pine.LNX.4.64.0705280059520.4648@racer.site>

Johannes Schindelin escreveu:
>>> but it does not do this, and does not check chache by default. Of course
>>> tests have to be written to make use of cache, IIRC...
>> Slowness is a misguided argument as well.  Yes, configure is slow, but 
>> you only have to run it if configure.in , config.h.in or config.make.in 
>> chagnes. And that doesn't happen very often during development.
> 
> Slowness is a good argument. The config does change from time to time (you 
> should know, IICC LilyPond had 36 changes in configure.in through 2006). 

Compared to the 100 recompiles during a typical debug/compile day, that's 
not very significant, IMO.


-- 
 Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen

^ permalink raw reply

* Re: [PATCH 00/15] git-note: A mechanisim for providing free-form after-the-fact annotations on commits
From: Johan Herland @ 2007-05-28  0:29 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Linus Torvalds
In-Reply-To: <7vejl2do5w.fsf@assigned-by-dhcp.cox.net>

On Sunday 27 May 2007, Junio C Hamano wrote:
> Johan Herland <johan@herland.net> writes:
> 
> > I've been working on combining tag objects and --decorate into a useful
> > proof-of-concept that provides the after-the-fact commit annotations I
> > requested above, and here's the result:
> > ...
> 
> Very nicely presented. I appreciate [PATCH 0/N] summary for a
> series like this that talks about not just what and how it does,
> but focus more on why it does it in a particular way, on design
> issues, and decisions.  It makes it very pleasant to comment on
> the series to have a well written summary like this, because in
> the early stage of a review cycle, we would want to talk about
> the design, ignoring implementation details.

Thanks, no problem. You're doing a really good job keeping all this crazy
development on track. The very least I can do is make your job easier. :)

> > However, there are still some remaining questions:
> >
> > - Is the creation of a unique tag name (i.e. the 'tag' field _inside_ the
> >   object) for note objects really necessary? Which parts of the system rely
> >   on these names to be unique across tag objects?
> 
> None.  Although your use of note-X{40} would make "recovering"
> easier, I would imagine.

Yeah, I'd rather just use _one_ identifier for all notes, instead of
generating yet another SHA1 sum, just to make each note (unecessarily)
unique.

> > - Should notes have their own object type instead of piggy-backing on "tag"?
> >
> > - What about having a note object type with minimal header fields, and make
> >   tags just a special case of a note object with an extra tag name header?
> 
> Two things struck me:
> 
>  - The "reverse" mapping 'note' tag uses uses FS as a database.
>    Clever but would not scale well.

Agreed.

>  - *BUT* the reverse mapping 'note' tag uses is exactly what we
>     would want to use for tags that are not notes, for tools
>     like gitk to attach little flags to commits.

For notes, it's obvious, as they aren't very useful unless we have a fast
reverse mapping, but it makes sense that other types of tag objects would
want this as well.

> So maybe we _should_ not even need to call this a new 'notes'
> system.  I am just saying your second point above in a different
> way, but I suspect all what is needed is to make the filtering
> and presentation of tag objects a bit more flexible, and making
> use of the 'peeled' (see .git/packed-refs, for example) mapping
> ultra cheap for ordinary tags.

Agreed. The peeled mapping makes a lot of sense here. I didn't know
about it when I first designed this, and I somehow managed to miss it
when I made the patches to pack-refs and show-refs.

Actually, I thought about something similar to the peeled mapping when
I designed the note refs. I basically had 3 reverse mapping options lined
up:

1. Put _all_ note refs in a single two-column file, noted objects
   ("notees") in the first column, and note objects in the second.
   Keep the file sorted by object name (first column), and keep the
   notes per object sorted chronologically. This would provide one of
   the fastest possible lookups for "git-note -l". However, the file
   would probably grow extremely large, slowing down insertion (and
   lookup) considerably.

2. One file per object with notes. Notes sorted chronologically within
   that file. This is sort of an intermediate between (1) and (3).

3. Keep a two-level hierarchy with one subdir per noted object, and one
   file per note. This is what I ended up with, mostly because it fit
   perfectly into the existing layout of refs/, and required very little
   code to implement.

Of course, now that I know about the peeled mapping, it makes more sense
to use a variation on that (maybe together with (1) or (2)).

> What you would need are:
> 
>  - Ultra-cheap reverse lookup: "I have an object; which tags
>    point at this?"
> 
>    You solved it with refs/notes/<sha1-of-tagged>/, but it would
>    be useful to extend this to any tag in general.
> 
>  - Easy way to filter the above question: "I have an object;
>    which tags of this particlar class point at this?"
> 
>    You are essentially prefiltering by only making notes tags
>    available via refs/notes/ hierarchy, but you can also filter
>    with your naming convention "tag notes-<sha1>".  We _could_
>    (I am not seriously suggesting this yet as I haven't thought
>    through the issues yet) allow "keyword" header to tag
>    objects, and allow --decorate='kwd1,kwd2,...' to limit the
>    tags we take object decorations from the ones that has one of
>    the specified keywords.  If we do that, I suspect your
>    'notes' system would naturally fall out as regular tags that
>    happen to have a keyword header with 'note' on it.

Yeah, I was thinking somewhat similarly; basically making tag objects
simpler and more versatile by replacing the "tag" header (which is
fundamentally unimportant to my notes) with a "tagtype" header which
could be set to "tag" for tag objects, "note" for note objects, etc.
Each tagtype could then further define headers specific to that tagtype.
For example could "tagtype == tag" objects reintroduce the "tag" header.

The tagtype field could then serve pretty much the same purpose as the
keyword header you suggest above.

> > - What about notes on notes? How are tags on tags treated? How should they be
> >   treated?
> 
> You can create a tag object that points at another tag.  When we
> peel a tag using "^{}" notation, currently it is peeled all the
> way until we hit a non tag, so if you need to look at
> intermediate tags, you need to parse them yourself.  IOW, there
> is no Porcelain feature to let you take advantage of this
> capability, easily.  But we could use this to say "I attest that
> he tagged that object".

Yeah, I'm still undecided on whether we want to allow notes on notes.
Some might argue that a note on a note should really be just another note
on the original noted object, and that nested notes doesn't make sense.
But then, on the other hand, there might be a use for nested notes.
Say, what about allowing threaded discussion on commits represented as
nested note objects? ;-P

In my current implementation, "git-note <tag>" will create a note on the
tag object, and not on the commit pointed to by the tag object. I'm not
sure that's the default behaviour I want; at least I would probably want
the note to show up when looking at the commit pointed to by that tag

> > - Currently noted objects (notees?) are treated as reachable from their
> >   associated notes, i.e. like tags. This means that an otherwise dangling
> >   object will not be detected and removed if it has associated notes.
> 
> I think it largely depends on how you intend to use 'notes' if
> this is a problem.
> 
> The reachability issue is not limited to cruft removal; it also
> affects the fetching.
> 
> I would imagine the intended use of a 'note' is to annotate
> objects (mostly commits, but not necessarily) after the fact,
> e.g. "this is reported to fix the issue 35833".

Yep, sounds about right, although other people might have other,
just as valid, ideas.

> You may or may 
> not want to propagate this across repositories, and the way you
> implemented it is to have a ref pointing at the note -- then it
> would make it fetchable by other people.  Otherwise it would not.
> With your implementation, unfortunately, not having a ref under
> refs/notes/ would also make the information unavailable to
> yourself.
> 
> I think the semantics you would want, when a 'note' object that
> points at another object (pointee) exists, is to allow creating
> a fake (reverse) reachability that says "pointee points at the
> note".  When such a reachability exists:
> 
>  - A fetch from such a repository that transfers out the pointee
>    will drag the note attached to it along with it (so you would
>    teach rev-list/upload-pack about the extended reachability).
> 
>  - Incidentally, having a ref that points at the pointee is
>    enough to protect the note that points at the pointee from
>    getting pruned (so you would teach fsck/prune about the
>    extended reachability).
> 
> Othewise, the 'note' is subject to garbage collection.

Yeah, the current implementation does not allow for this reverse/weaker
relationship at all.

I agree that fetchability and prunability of a note depends on the
fetch-/prunability of its pointee. Furthermore, a note on an otherwise
dangling object should not keep that object alive, but rather allow that
object to be pruned (automatically causing the note to be pruned as well).

> This is 
> pretty much an extension to the existing 'grafts' mechanism,
> which can only override commit and its parents; what is done in
> the above is to add (not override) to existing reachability to
> any object (not just commit, but anything that can get 'noted').

I'll have to read up on the 'grafts' mechanism, but it already sounds
like a much better way to encode these relationships than my crude
attempts in 'refs/notes'.

Thanks a lot for the feedback.


Have fun!

...Johan

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

^ permalink raw reply

* Re: GIT on MinGW problem
From: Johannes Schindelin @ 2007-05-28  0:04 UTC (permalink / raw)
  To: Han-Wen Nienhuys; +Cc: Jakub Narebski, Steven Grimm, git, Shawn O. Pearce
In-Reply-To: <465A11AB.6060906@xs4all.nl>

Hi,

On Sun, 27 May 2007, Han-Wen Nienhuys wrote:

> Jakub Narebski escreveu:
> > On Sun, 27 May 2007, Steven Grimm wrote:
> >> Han-Wen Nienhuys wrote:
> >>> Shawn O. Pearce escreveu:
> >>>> On systems like Cygwin the fork+exec overheads are very high
> >>> A well written configure script is able to detect presence
> >>> of a linkable libcurl.
> >> IMO the reasons configure is so unwieldy, at least as it's set up in 
> >> most open source projects, are that a) it spends 95% of its time 
> >> checking for things that basically never vary (yes, I have stdlib.h, 
> >> thank you) and that b) it doesn't remember the results from previous 
> >> runs on the same host (I'm just changing the install path; my ints won't 
> >> have stopped being 32 bits as a result.)
> > 
> > ./configure _can_ cache tests results:
> > 
> > $ ./configure --help
> > [...]
> >       --cache-file=FILE   cache test results in FILE [disabled]
> >   -C, --config-cache      alias for `--cache-file=config.cache'
> > 
> > but it does not do this, and does not check chache by default. Of course
> > tests have to be written to make use of cache, IIRC...
> 
> Slowness is a misguided argument as well.  Yes, configure is slow, but 
> you only have to run it if configure.in , config.h.in or config.make.in 
> chagnes. And that doesn't happen very often during development.

Slowness is a good argument. The config does change from time to time (you 
should know, IICC LilyPond had 36 changes in configure.in through 2006). 
And it is annoying.

Especially when you are trying to debug ./configure.

Ciao,
Dscho

^ permalink raw reply

* Re: GIT on MinGW - No symbolic links support
From: Johannes Schindelin @ 2007-05-27 23:56 UTC (permalink / raw)
  To: Aaron Gray; +Cc: hanwen, git
In-Reply-To: <012d01c7a0b2$374eea50$0200a8c0@AMD2500>

Hi,

On Sun, 27 May 2007, Aaron Gray wrote:

> Bit of a dampener on GIT on MinGW :-
> 
>        $ git clone git://git.kernel.org/pub/scm/git/git.git
>        Initialized empty Git repository in C:/MSYS/src/git/.git/
>        error: git-checkout-index: unable to create symlink RelNotes (Function
> not implemented)
> 
> No Symbolic links !
> 
> There are symbolic links provided by Windows by SFU (Services For Unix)
> apparently.

Does not work on FAT. Has lots of problems.

That's why Johannes Sixt pushed for core.symlinks, and got it. So maybe 
the templates should set core.symlinks=false?

Ciao,
Dscho

^ permalink raw reply

* Re: [PATCH 1/3] Lazily open pack index files on demand
From: Nicolas Pitre @ 2007-05-27 23:35 UTC (permalink / raw)
  To: Shawn O. Pearce; +Cc: Dana How, Junio C Hamano, git
In-Reply-To: <20070527215245.GD28023@spearce.org>

On Sun, 27 May 2007, Shawn O. Pearce wrote:

> Nicolas Pitre <nico@cam.org> wrote:
> > BTW I think the Newton-Raphson based index lookup approach should be 
> > revived at some point.
> 
> That doesn't help with 10 packfiles though, does it?

It helps irrespective of the number of pack files.  With the current 
binary search the lookup cost is O(log n).  With a Newton method this 
cost is almost O(1).  If you have 10 pack files then you may have to do 
5 separate lookups on average, but those lookups are still faster with a 
Newton method.


Nicolas

^ permalink raw reply

* Re: [PATCH] gitweb: Create special from-file/to-file header for combined diff
From: Jakub Narebski @ 2007-05-27 23:33 UTC (permalink / raw)
  To: git
In-Reply-To: <11803077803470-git-send-email-jnareb@gmail.com>

Ooops

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

^ permalink raw reply

* Re: GIT on MinGW problem
From: Han-Wen Nienhuys @ 2007-05-27 23:18 UTC (permalink / raw)
  To: Jakub Narebski; +Cc: Steven Grimm, git, Shawn O. Pearce
In-Reply-To: <200705271109.11942.jnareb@gmail.com>

Jakub Narebski escreveu:
> On Sun, 27 May 2007, Steven Grimm wrote:
>> Han-Wen Nienhuys wrote:
>>> Shawn O. Pearce escreveu:
>>>> On systems like Cygwin the fork+exec overheads are very high
>>> A well written configure script is able to detect presence
>>> of a linkable libcurl.
>> IMO the reasons configure is so unwieldy, at least as it's set up in 
>> most open source projects, are that a) it spends 95% of its time 
>> checking for things that basically never vary (yes, I have stdlib.h, 
>> thank you) and that b) it doesn't remember the results from previous 
>> runs on the same host (I'm just changing the install path; my ints won't 
>> have stopped being 32 bits as a result.)
> 
> ./configure _can_ cache tests results:
> 
> $ ./configure --help
> [...]
>       --cache-file=FILE   cache test results in FILE [disabled]
>   -C, --config-cache      alias for `--cache-file=config.cache'
> 
> but it does not do this, and does not check chache by default. Of course
> tests have to be written to make use of cache, IIRC...

Slowness is a misguided argument as well.  Yes, configure is slow, but you
only have to run it if configure.in , config.h.in or config.make.in chagnes.
And that doesn't happen very often during development.


-- 
 Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen

^ permalink raw reply

* Re: GIT on MinGW problem
From: Jakub Narebski @ 2007-05-27  9:09 UTC (permalink / raw)
  To: Steven Grimm; +Cc: Han-Wen Nienhuys, git, Shawn O. Pearce
In-Reply-To: <4659318B.20801@midwinter.com>

On Sun, 27 May 2007, Steven Grimm wrote:
> Han-Wen Nienhuys wrote:
>> Shawn O. Pearce escreveu:
>>>
>>> On systems like Cygwin the fork+exec overheads are very high
>>
>> A well written configure script is able to detect presence
>> of a linkable libcurl.
> 
> IMO the reasons configure is so unwieldy, at least as it's set up in 
> most open source projects, are that a) it spends 95% of its time 
> checking for things that basically never vary (yes, I have stdlib.h, 
> thank you) and that b) it doesn't remember the results from previous 
> runs on the same host (I'm just changing the install path; my ints won't 
> have stopped being 32 bits as a result.)

./configure _can_ cache tests results:

$ ./configure --help
[...]
      --cache-file=FILE   cache test results in FILE [disabled]
  -C, --config-cache      alias for `--cache-file=config.cache'

but it does not do this, and does not check chache by default. Of course
tests have to be written to make use of cache, IIRC...

> I wonder if we could satisfy  
> most people with a configure script -- maybe not based on autoconf -- 
> that is limited in scope to just the things that are currently tweakable 
> in the git Makefile.

The problem with handcrafted configure script lies in the portability
of it. There was an attempt to add such script, IIRC based on mplayer's
configure.sh script, but it turned out it was not portable enough.
The conclusion was that sice so many manhours were put into making
autoconf generate ultra-portable ./configure shell script, it would
be better to use it.

> If configure ran only, say, 10-15 tests, I bet the fork+exec overhead on 
> Cygwin would be perfectly tolerable.

That's not only fork+exec, that is also the fact that large number
of tests relies on compiling snippets of code...


P.S. What do you think about separating the guessing appropriate
values of build variables based on uname to separate file
config.mak.guess, included in Makefile?

-- 
Jakub Narebski
Poland

^ permalink raw reply

* [PATCH] gitweb: Create special from-file/to-file header for combined diff
From: Jakub Narebski @ 2007-05-27 23:16 UTC (permalink / raw)
  To: git; +Cc: Jakub Narebski
In-Reply-To: <11803077792064-git-send-email-jnareb@gmail.com>

Instead of using default, diff(1) like from-file/to-file header for
combined diff (for a merge commit), which looks like:

  --- a/git-gui/git-gui.sh
  +++ b/_git-gui/git-gui.sh_

(where _link_ denotes [hidden] hyperlink), create from-file(n)/to-file
header, using n/file for each or parents, e.g.:

  --- 1/_git-gui/git-gui.sh_
  --- 2/_git-gui.sh_
  +++ b/_git-gui/git-gui.sh_

Test it on one of merge commits involving rename, e.g.
  95f97567c1887d77f3a46b42d8622c76414d964d (rename at top)
  5bac4a671907604b5fb4e24ff682d5b0e8431931 (file from one branch)

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
 gitweb/gitweb.perl |   32 ++++++++++++++++++++++++--------
 1 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 795af92..f73f184 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1045,17 +1045,33 @@ sub format_diff_from_to_header {
 	#assert($line =~ m/^---/) if DEBUG;
 	# no extra formatting "^--- /dev/null"
 	if ($line =~ m!^--- "?a/!) {
-		if (!$diffinfo->{'nparents'} && # multiple 'from'
-		    $from->{'href'}) {
-			$line = '--- a/' .
-			        $cgi->a({-href=>$from->{'href'}, -class=>"path"},
-			                esc_path($from->{'file'}));
+		if (!$diffinfo->{'nparents'}) {
+			if ($from->{'href'}) {
+				$line = '--- a/' .
+				        $cgi->a({-href=>$from->{'href'}, -class=>"path"},
+				                esc_path($from->{'file'}));
+			} else {
+				$line = '--- a/' .
+				        esc_path($from->{'file'});
+			}
+			$result .= qq!<div class="diff from_file">$line</div>\n!;
 		} else {
-			$line = '--- a/' .
-			        esc_path($from->{'file'});
+			for (my $i = 0; $i < $diffinfo->{'nparents'}; $i++) {
+				if ($from->{'href'}[$i]) {
+					$result .= qq!<div class="diff from_file">--- ! .
+					           ($i+1) . "/" .
+					           $cgi->a({-href=>$from->{'href'}[$i], -class=>"path"},
+					                   esc_path($from->{'file'}[$i])) .
+					           qq!</div>\n!;
+				} else {
+					$result .= qq!<div class="diff from_file">--- /dev/null</div>\n!;
+				}
+			}
 		}
+	} else {
+		$result .= qq!<div class="diff from_file">$line</div>\n!;
 	}
-	$result .= qq!<div class="diff from_file">$line</div>\n!;
+
 
 	$line = $to_line;
 	#assert($line =~ m/^\+\+\+/) if DEBUG;
-- 
1.5.2

^ permalink raw reply related

* [RFC/PATCH] gitweb: Create special from-file/to-file header for combined diff
From: Jakub Narebski @ 2007-05-27 23:16 UTC (permalink / raw)
  To: git; +Cc: Jakub Narebski
In-Reply-To: <11803077771867-git-send-email-jnareb@gmail.com>

Instead of using default, diff(1) like from-file/to-file header for
combined diff (for a merge commit), which looks like:

  --- a/git-gui/git-gui.sh
  +++ b/_git-gui/git-gui.sh_

(where _link_ denotes [hidden] hyperlink), create from-file(n)/to-file
header, using n/file for each or parents, e.g.:

  --- 1/_git-gui/git-gui.sh_
  --- 2/_git-gui.sh_
  +++ b/_git-gui/git-gui.sh_

Test it on one of merge commits involving rename, e.g.
  95f97567c1887d77f3a46b42d8622c76414d964d (rename at top)
  5bac4a671907604b5fb4e24ff682d5b0e8431931 (file from one branch)

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
This is just an idea, not seriously meant to be applied... unless of
course you think that this is great. What do you think about it?

This patch depends on my earlier patch
  "gitweb: Split git_patchset_body into separate subroutines"

 gitweb/gitweb.perl |   32 ++++++++++++++++++++++++--------
 1 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 795af92..f73f184 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1045,17 +1045,33 @@ sub format_diff_from_to_header {
 	#assert($line =~ m/^---/) if DEBUG;
 	# no extra formatting "^--- /dev/null"
 	if ($line =~ m!^--- "?a/!) {
-		if (!$diffinfo->{'nparents'} && # multiple 'from'
-		    $from->{'href'}) {
-			$line = '--- a/' .
-			        $cgi->a({-href=>$from->{'href'}, -class=>"path"},
-			                esc_path($from->{'file'}));
+		if (!$diffinfo->{'nparents'}) {
+			if ($from->{'href'}) {
+				$line = '--- a/' .
+				        $cgi->a({-href=>$from->{'href'}, -class=>"path"},
+				                esc_path($from->{'file'}));
+			} else {
+				$line = '--- a/' .
+				        esc_path($from->{'file'});
+			}
+			$result .= qq!<div class="diff from_file">$line</div>\n!;
 		} else {
-			$line = '--- a/' .
-			        esc_path($from->{'file'});
+			for (my $i = 0; $i < $diffinfo->{'nparents'}; $i++) {
+				if ($from->{'href'}[$i]) {
+					$result .= qq!<div class="diff from_file">--- ! .
+					           ($i+1) . "/" .
+					           $cgi->a({-href=>$from->{'href'}[$i], -class=>"path"},
+					                   esc_path($from->{'file'}[$i])) .
+					           qq!</div>\n!;
+				} else {
+					$result .= qq!<div class="diff from_file">--- /dev/null</div>\n!;
+				}
+			}
 		}
+	} else {
+		$result .= qq!<div class="diff from_file">$line</div>\n!;
 	}
-	$result .= qq!<div class="diff from_file">$line</div>\n!;
+
 
 	$line = $to_line;
 	#assert($line =~ m/^\+\+\+/) if DEBUG;
-- 
1.5.2

^ permalink raw reply related

* [PATCH] gitweb: Split git_patchset_body into separate subroutines
From: Jakub Narebski @ 2007-05-27 23:16 UTC (permalink / raw)
  To: git; +Cc: Jakub Narebski

Separate formatting "git diff" header into format_git_diff_header_line.
While at it fix it so it always escapes pathname. It would be even more
useful if we decide to use `--cc' for merges, and need to generate by
hand empty patches for anchors.

Separate formatting extended (git) diff header lines into
format_extended_diff_header_line. This one is copied without changes.

Separate formatting two-lines from-file/to-file diff header into
format_diff_from_to_header subroutine. While at it fix it so it always
escapes pathname. Beware calling convention: it takes _two_ lines.

Separate generating %from and %to hashes (with info used among others to
generate hyperlinks) into parse_from_to_diffinfo subroutine. This one is
copied without changes.

Separate checking if file was deleted (and among others therefore does
not have link to the result file) into is_deleted subroutine. This would
allow us to easily change the algotithm to find if file is_deleted in
the result.


This commit makes git_patchset_body easier to read, and reduces level of
nesting and indent level. It adds more lines that it removes because of
extra parameter passing in subroutines, and subroutine calls in
git_patchset_body. Also because there are few added comments.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
 gitweb/gitweb.perl |  313 +++++++++++++++++++++++++++++++++------------------
 1 files changed, 202 insertions(+), 111 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 999353d..795af92 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -933,7 +933,149 @@ sub format_subject_html {
 	}
 }
 
-# format patch (diff) line (rather not to be used for diff headers)
+# format git diff header line, i.e. "diff --(git|combined|cc) ..."
+sub format_git_diff_header_line {
+	my $line = shift;
+	my $diffinfo = shift;
+	my ($from, $to) = @_;
+
+	if ($diffinfo->{'nparents'}) {
+		# combined diff
+		$line =~ s!^(diff (.*?) )"?.*$!$1!;
+		if ($to->{'href'}) {
+			$line .= $cgi->a({-href => $to->{'href'}, -class => "path"},
+			                 esc_path($to->{'file'}));
+		} else { # file was deleted (no href)
+			$line .= esc_path($to->{'file'});
+		}
+	} else {
+		# "ordinary" diff
+		$line =~ s!^(diff (.*?) )"?a/.*$!$1!;
+		if ($from->{'href'}) {
+			$line .= $cgi->a({-href => $from->{'href'}, -class => "path"},
+			                 'a/' . esc_path($from->{'file'}));
+		} else { # file was added (no href)
+			$line .= 'a/' . esc_path($from->{'file'});
+		}
+		$line .= ' ';
+		if ($to->{'href'}) {
+			$line .= $cgi->a({-href => $to->{'href'}, -class => "path"},
+			                 'b/' . esc_path($to->{'file'}));
+		} else { # file was deleted
+			$line .= 'b/' . esc_path($to->{'file'});
+		}
+	}
+
+	return "<div class=\"diff header\">$line</div>\n";
+}
+
+# format extended diff header line, before patch itself
+sub format_extended_diff_header_line {
+	my $line = shift;
+	my $diffinfo = shift;
+	my ($from, $to) = @_;
+
+	# match <path>
+	if ($line =~ s!^((copy|rename) from ).*$!$1! && $from->{'href'}) {
+		$line .= $cgi->a({-href=>$from->{'href'}, -class=>"path"},
+		                       esc_path($from->{'file'}));
+	}
+	if ($line =~ s!^((copy|rename) to ).*$!$1! && $to->{'href'}) {
+		$line .= $cgi->a({-href=>$to->{'href'}, -class=>"path"},
+		                 esc_path($to->{'file'}));
+	}
+	# match single <mode>
+	if ($line =~ m/\s(\d{6})$/) {
+		$line .= '<span class="info"> (' .
+		         file_type_long($1) .
+		         ')</span>';
+	}
+	# match <hash>
+	if ($line =~ m/^index [0-9a-fA-F]{40},[0-9a-fA-F]{40}/) {
+		# can match only for combined diff
+		$line = 'index ';
+		for (my $i = 0; $i < $diffinfo->{'nparents'}; $i++) {
+			if ($from->{'href'}[$i]) {
+				$line .= $cgi->a({-href=>$from->{'href'}[$i],
+				                  -class=>"hash"},
+				                 substr($diffinfo->{'from_id'}[$i],0,7));
+			} else {
+				$line .= '0' x 7;
+			}
+			# separator
+			$line .= ',' if ($i < $diffinfo->{'nparents'} - 1);
+		}
+		$line .= '..';
+		if ($to->{'href'}) {
+			$line .= $cgi->a({-href=>$to->{'href'}, -class=>"hash"},
+			                 substr($diffinfo->{'to_id'},0,7));
+		} else {
+			$line .= '0' x 7;
+		}
+
+	} elsif ($line =~ m/^index [0-9a-fA-F]{40}..[0-9a-fA-F]{40}/) {
+		# can match only for ordinary diff
+		my ($from_link, $to_link);
+		if ($from->{'href'}) {
+			$from_link = $cgi->a({-href=>$from->{'href'}, -class=>"hash"},
+			                     substr($diffinfo->{'from_id'},0,7));
+		} else {
+			$from_link = '0' x 7;
+		}
+		if ($to->{'href'}) {
+			$to_link = $cgi->a({-href=>$to->{'href'}, -class=>"hash"},
+			                   substr($diffinfo->{'to_id'},0,7));
+		} else {
+			$to_link = '0' x 7;
+		}
+		my ($from_id, $to_id) = ($diffinfo->{'from_id'}, $diffinfo->{'to_id'});
+		$line =~ s!$from_id\.\.$to_id!$from_link..$to_link!;
+	}
+
+	return $line . "<br/>\n";
+}
+
+# format from-file/to-file diff header
+sub format_diff_from_to_header {
+	my ($from_line, $to_line, $diffinfo, $from, $to) = @_;
+	my $line;
+	my $result = '';
+
+	$line = $from_line;
+	#assert($line =~ m/^---/) if DEBUG;
+	# no extra formatting "^--- /dev/null"
+	if ($line =~ m!^--- "?a/!) {
+		if (!$diffinfo->{'nparents'} && # multiple 'from'
+		    $from->{'href'}) {
+			$line = '--- a/' .
+			        $cgi->a({-href=>$from->{'href'}, -class=>"path"},
+			                esc_path($from->{'file'}));
+		} else {
+			$line = '--- a/' .
+			        esc_path($from->{'file'});
+		}
+	}
+	$result .= qq!<div class="diff from_file">$line</div>\n!;
+
+	$line = $to_line;
+	#assert($line =~ m/^\+\+\+/) if DEBUG;
+	# no extra formatting for "^+++ /dev/null"
+	if ($line =~ m!^\+\+\+ "?b/!) {
+		if ($to->{'href'}) {
+			$line = '+++ b/' .
+			        $cgi->a({-href=>$to->{'href'}, -class=>"path"},
+			                esc_path($to->{'file'}));
+		} else {
+			$line = '+++ b/' .
+			        esc_path($to->{'file'});
+		}
+	}
+	$result .= qq!<div class="diff to_file">$line</div>\n!;
+
+	return $result;
+}
+
+# format patch (diff) line (not to be used for diff headers)
 sub format_diff_line {
 	my $line = shift;
 	my ($from, $to) = @_;
@@ -1659,6 +1801,48 @@ sub parse_ls_tree_line ($;%) {
 	return wantarray ? %res : \%res;
 }
 
+# generates _two_ hashes, references to which are passed as 2 and 3 argument
+sub parse_from_to_diffinfo {
+	my ($diffinfo, $from, $to, @parents) = @_;
+
+	if ($diffinfo->{'nparents'}) {
+		# combined diff
+		$from->{'file'} = [];
+		$from->{'href'} = [];
+		fill_from_file_info($diffinfo, @parents)
+			unless exists $diffinfo->{'from_file'};
+		for (my $i = 0; $i < $diffinfo->{'nparents'}; $i++) {
+			$from->{'file'}[$i] = $diffinfo->{'from_file'}[$i] || $diffinfo->{'to_file'};
+			if ($diffinfo->{'status'}[$i] ne "A") { # not new (added) file
+				$from->{'href'}[$i] = href(action=>"blob",
+				                           hash_base=>$parents[$i],
+				                           hash=>$diffinfo->{'from_id'}[$i],
+				                           file_name=>$from->{'file'}[$i]);
+			} else {
+				$from->{'href'}[$i] = undef;
+			}
+		}
+	} else {
+		$from->{'file'} = $diffinfo->{'from_file'} || $diffinfo->{'file'};
+		if ($diffinfo->{'status'} ne "A") { # not new (added) file
+			$from->{'href'} = href(action=>"blob", hash_base=>$hash_parent,
+			                       hash=>$diffinfo->{'from_id'},
+			                       file_name=>$from->{'file'});
+		} else {
+			delete $from->{'href'};
+		}
+	}
+
+	$to->{'file'} = $diffinfo->{'to_file'} || $diffinfo->{'file'};
+	if (!is_deleted($diffinfo)) { # file exists in result
+		$to->{'href'} = href(action=>"blob", hash_base=>$hash,
+		                     hash=>$diffinfo->{'to_id'},
+		                     file_name=>$to->{'file'});
+	} else {
+		delete $to->{'href'};
+	}
+}
+
 ## ......................................................................
 ## parse to array of hashes functions
 
@@ -2366,6 +2550,11 @@ sub from_ids_eq {
 	}
 }
 
+sub is_deleted {
+	my $diffinfo = shift;
+
+	return $diffinfo->{'to_id'} eq ('0' x 40);
+}
 
 sub git_difftree_body {
 	my ($difftree, $hash, @parents) = @_;
@@ -2422,7 +2611,7 @@ sub git_difftree_body {
 			fill_from_file_info($diff, @parents)
 				unless exists $diff->{'from_file'};
 
-			if ($diff->{'to_id'} ne ('0' x 40)) {
+			if (!is_deleted($diff)) {
 				# file exists in the result (child) commit
 				print "<td>" .
 				      $cgi->a({-href => href(action=>"blob", hash=>$diff->{'to_id'},
@@ -2742,6 +2931,8 @@ sub git_patchset_body {
 			} else {
 				$diffinfo = parse_difftree_raw_line($difftree->[$patch_idx]);
 			}
+			# modifies %from, %to hashes
+			parse_from_to_diffinfo($diffinfo, \%from, \%to, @hash_parents);
 			if ($diffinfo->{'nparents'}) {
 				# combined diff
 				$from{'file'} = [];
@@ -2771,7 +2962,7 @@ sub git_patchset_body {
 			}
 
 			$to{'file'} = $diffinfo->{'to_file'} || $diffinfo->{'file'};
-			if ($diffinfo->{'to_id'} ne ('0' x 40)) { # file exists in result
+			if (!is_deleted($diffinfo)) { # file exists in result
 				$to{'href'} = href(action=>"blob", hash_base=>$hash,
 				                   hash=>$diffinfo->{'to_id'},
 				                   file_name=>$to{'file'});
@@ -2785,105 +2976,15 @@ sub git_patchset_body {
 
 		# print "git diff" header
 		$patch_line = shift @diff_header;
-		if ($diffinfo->{'nparents'}) {
-
-			# combined diff
-			$patch_line =~ s!^(diff (.*?) )"?.*$!$1!;
-			if ($to{'href'}) {
-				$patch_line .= $cgi->a({-href => $to{'href'}, -class => "path"},
-				                       esc_path($to{'file'}));
-			} else { # file was deleted
-				$patch_line .= esc_path($to{'file'});
-			}
-
-		} else {
-
-			$patch_line =~ s!^(diff (.*?) )"?a/.*$!$1!;
-			if ($from{'href'}) {
-				$patch_line .= $cgi->a({-href => $from{'href'}, -class => "path"},
-				                       'a/' . esc_path($from{'file'}));
-			} else { # file was added
-				$patch_line .= 'a/' . esc_path($from{'file'});
-			}
-			$patch_line .= ' ';
-			if ($to{'href'}) {
-				$patch_line .= $cgi->a({-href => $to{'href'}, -class => "path"},
-				                       'b/' . esc_path($to{'file'}));
-			} else { # file was deleted
-				$patch_line .= 'b/' . esc_path($to{'file'});
-			}
-
-		}
-		print "<div class=\"diff header\">$patch_line</div>\n";
+		print format_git_diff_header_line($patch_line, $diffinfo,
+		                                  \%from, \%to);
 
 		# print extended diff header
 		print "<div class=\"diff extended_header\">\n" if (@diff_header > 0);
 	EXTENDED_HEADER:
 		foreach $patch_line (@diff_header) {
-			# match <path>
-			if ($patch_line =~ s!^((copy|rename) from ).*$!$1! && $from{'href'}) {
-				$patch_line .= $cgi->a({-href=>$from{'href'}, -class=>"path"},
-				                       esc_path($from{'file'}));
-			}
-			if ($patch_line =~ s!^((copy|rename) to ).*$!$1! && $to{'href'}) {
-				$patch_line .= $cgi->a({-href=>$to{'href'}, -class=>"path"},
-				                       esc_path($to{'file'}));
-			}
-			# match single <mode>
-			if ($patch_line =~ m/\s(\d{6})$/) {
-				$patch_line .= '<span class="info"> (' .
-				               file_type_long($1) .
-				               ')</span>';
-			}
-			# match <hash>
-			if ($patch_line =~ m/^index [0-9a-fA-F]{40},[0-9a-fA-F]{40}/) {
-				# can match only for combined diff
-				$patch_line = 'index ';
-				for (my $i = 0; $i < $diffinfo->{'nparents'}; $i++) {
-					if ($from{'href'}[$i]) {
-						$patch_line .= $cgi->a({-href=>$from{'href'}[$i],
-						                        -class=>"hash"},
-						                       substr($diffinfo->{'from_id'}[$i],0,7));
-					} else {
-						$patch_line .= '0' x 7;
-					}
-					# separator
-					$patch_line .= ',' if ($i < $diffinfo->{'nparents'} - 1);
-				}
-				$patch_line .= '..';
-				if ($to{'href'}) {
-					$patch_line .= $cgi->a({-href=>$to{'href'}, -class=>"hash"},
-					                       substr($diffinfo->{'to_id'},0,7));
-				} else {
-					$patch_line .= '0' x 7;
-				}
-
-			} elsif ($patch_line =~ m/^index [0-9a-fA-F]{40}..[0-9a-fA-F]{40}/) {
-				# can match only for ordinary diff
-				my ($from_link, $to_link);
-				if ($from{'href'}) {
-					$from_link = $cgi->a({-href=>$from{'href'}, -class=>"hash"},
-					                     substr($diffinfo->{'from_id'},0,7));
-				} else {
-					$from_link = '0' x 7;
-				}
-				if ($to{'href'}) {
-					$to_link = $cgi->a({-href=>$to{'href'}, -class=>"hash"},
-					                   substr($diffinfo->{'to_id'},0,7));
-				} else {
-					$to_link = '0' x 7;
-				}
-				#affirm {
-				#	my ($from_hash, $to_hash) =
-				#		($patch_line =~ m/^index ([0-9a-fA-F]{40})..([0-9a-fA-F]{40})/);
-				#	my ($from_id, $to_id) =
-				#		($diffinfo->{'from_id'}, $diffinfo->{'to_id'});
-				#	($from_hash eq $from_id) && ($to_hash eq $to_id);
-				#} if DEBUG;
-				my ($from_id, $to_id) = ($diffinfo->{'from_id'}, $diffinfo->{'to_id'});
-				$patch_line =~ s!$from_id\.\.$to_id!$from_link..$to_link!;
-			}
-			print $patch_line . "<br/>\n";
+			print format_extended_diff_header_line($patch_line, $diffinfo,
+			                                       \%from, \%to);
 		}
 		print "</div>\n"  if (@diff_header > 0); # class="diff extended_header"
 
@@ -2895,24 +2996,14 @@ sub git_patchset_body {
 		}
 		next PATCH if ($patch_line =~ m/^diff /);
 		#assert($patch_line =~ m/^---/) if DEBUG;
-		if (!$diffinfo->{'nparents'} && # not from-file line for combined diff
-		    $from{'href'} && $patch_line =~ m!^--- "?a/!) {
-			$patch_line = '--- a/' .
-			              $cgi->a({-href=>$from{'href'}, -class=>"path"},
-			                      esc_path($from{'file'}));
-		}
-		print "<div class=\"diff from_file\">$patch_line</div>\n";
+		#assert($patch_line eq $last_patch_line) if DEBUG;
 
 		$patch_line = <$fd>;
 		chomp $patch_line;
+		#assert($patch_line =~ m/^\+\+\+/) if DEBUG;
 
-		#assert($patch_line =~ m/^+++/) if DEBUG;
-		if ($to{'href'} && $patch_line =~ m!^\+\+\+ "?b/!) {
-			$patch_line = '+++ b/' .
-			              $cgi->a({-href=>$to{'href'}, -class=>"path"},
-			                      esc_path($to{'file'}));
-		}
-		print "<div class=\"diff to_file\">$patch_line</div>\n";
+		print format_diff_from_to_header($last_patch_line, $patch_line,
+		                                 $diffinfo, \%from, \%to);
 
 		# the patch itself
 	LINE:
-- 
1.5.2

^ permalink raw reply related

* Re: [PATCH] Fix git-svn to handle svn not reporting the md5sum of a file, and test.
From: Eric Wong @ 2007-05-27 23:04 UTC (permalink / raw)
  To: A Large Angry SCM; +Cc: James Y Knight, git, Junio C Hamano
In-Reply-To: <4659DBC8.2000105@gmail.com>

A Large Angry SCM <gitzilla@gmail.com> wrote:
> Eric Wong wrote:
> > A Large Angry SCM <gitzilla@gmail.com> wrote:
> >> James Y Knight wrote:
> >>> ---
> >>> git-svn.perl                    |    2 +-
> >>> t/t9112-git-svn-md5less-file.sh |   45
> >>> +++++++++++++++++++++++++++++++++++++++
> >>> 2 files changed, 46 insertions(+), 1 deletions(-)
> >>> create mode 100755 t/t9112-git-svn-md5less-file.sh
> >> [...]
> >>
> >> The new test fails here (Suse 9.3 fully patched) w/ the following:
> >>
> >> *** t9112-git-svn-md5less-file.sh ***
> >> *   ok 1: load svn dumpfile
> >> *   ok 2: initialize git-svn
> >> * FAIL 3: fetch revisions from svn
> >>         git-svn fetch
> >> * failed 1 among 3 test(s)
> >> make[1]: *** [t9112-git-svn-md5less-file.sh] Error 1
> >
> > I can't reproduce it here (on Debian Etch, SVN 1.4.2).  Can you run with
> > the test with the -v switch?  Thanks.
> >
> 
> 
> ~/GIT/git/t> sh ./t9112-git-svn-md5less-file.sh -v
> * expecting success: svnadmin load /home/test/GIT/git/t/trash/svnrepo < 
> dumpfile.svn
> <<< Started new transaction, based on original revision 1
>      * adding path : md5less-file ... done.
> 
> ------- Committed revision 1 >>>
> 
> *   ok 1: load svn dumpfile
> 
> * expecting success: git-svn init file:///home/test/GIT/git/t/trash/svnrepo
> *   ok 2: initialize git-svn
> 
> * expecting success: git-svn fetch
> ./test-lib.sh: line 141:  8163 Segmentation fault      git-svn fetch
> * FAIL 3: fetch revisions from svn
>         git-svn fetch
> 
> * failed 1 among 3 test(s)
> ~/GIT/git/t>
> 
> 
> And here is the failing part of the test using sh -x:
> 
> + test_expect_success 'fetch revisions from svn' 'git-svn fetch'
> + test 2 = 2
> + test_skip 'fetch revisions from svn' 'git-svn fetch'
> ++ expr ././t9112-git-svn-md5less-file.sh : '.*/\(t[0-9]*\)-[^/]*$'
> + this_test=t9112
> ++ expr 2 + 1
> + this_test=t9112.3
> + to_skip=
> + case "$to_skip" in
> + false
> + say 'expecting success: git-svn fetch'
> + echo '* expecting success: git-svn fetch'
> * expecting success: git-svn fetch
> + test_run_ 'git-svn fetch'
> + eval 'git-svn fetch'
> ++ git-svn fetch
> ./test-lib.sh: line 141:  8276 Segmentation fault      git-svn fetch
> + eval_ret=139
> + return 0
> + '[' 0 = 0 -a 139 = 0 ']'
> + test_failure_ 'fetch revisions from svn' 'git-svn fetch'
> ++ expr 2 + 1
> + test_count=3
> ++ expr 0 + 1
> + test_failure=1
> + say 'FAIL 3: fetch revisions from svn'
> + echo '* FAIL 3: fetch revisions from svn'
> * FAIL 3: fetch revisions from svn
> + shift
> + echo 'git-svn fetch'
> + sed -e 's/^/  /'
>         git-svn fetch
> + test '' = ''
> + echo ''
> 
> This began after the 18bece4..99b5a79 update to master. Prior to that 
> the svn tests were passing.

Thanks.

I'm definitely not able to reproduce this here, and I'm sure Junio
wouldn't have pushed out if he could, either...  Which versions of SVN
and Perl (MD5) do you have?

A backtrace with debugging symbols could be helpful if the below
stab in the dark doesn't work out:

Maybe there's an off chance that the MD5 implementation you're using
can't handle zero-sized files?

Junio: can you apply the following patch regardless of whether or not it
fixes this issue?  It just makes more sense, thanks.

>From 3229470be27589a0428994475b0a597cc549cf78 Mon Sep 17 00:00:00 2001
From: Eric Wong <normalperson@yhbt.net>
Date: Sun, 27 May 2007 15:59:01 -0700
Subject: [PATCH] git-svn: avoid md5 calculation entirely if SVN doesn't provide one

There's no point in calculating an MD5 if we're not going to use
it.  We'll also avoid the possibility of there being a bug in the
Perl MD5 library not being able to handle zero-sized files.

This is a followup to 20b3d206acbbb042c7ad5f42d36ff8d036a538c5,
which allows us to track repositories that do not provide MD5
checksums.

Signed-off-by: Eric Wong <normalperson@yhbt.net>
---
 git-svn.perl |   16 ++++++++++------
 1 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index eeaeb2d..58f7dd0 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -2472,12 +2472,16 @@ sub close_file {
 	my $hash;
 	my $path = $self->git_path($fb->{path});
 	if (my $fh = $fb->{fh}) {
-		seek($fh, 0, 0) or croak $!;
-		my $md5 = Digest::MD5->new;
-		$md5->addfile($fh);
-		my $got = $md5->hexdigest;
-		die "Checksum mismatch: $path\n",
-		    "expected: $exp\n    got: $got\n" if (defined $exp && $got ne $exp);
+		if (defined $exp) {
+			seek($fh, 0, 0) or croak $!;
+			my $md5 = Digest::MD5->new;
+			$md5->addfile($fh);
+			my $got = $md5->hexdigest;
+			if ($got ne $exp) {
+				die "Checksum mismatch: $path\n",
+				    "expected: $exp\n    got: $got\n";
+			}
+		}
 		sysseek($fh, 0, 0) or croak $!;
 		if ($fb->{mode_b} == 120000) {
 			sysread($fh, my $buf, 5) == 5 or croak $!;
-- 
Eric Wong

^ 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