* Re: [PATCH] gitweb: export options
From: Junio C Hamano @ 2006-09-17 10:34 UTC (permalink / raw)
To: Matthias Lederhofer; +Cc: git
In-Reply-To: <20060917090710.GA18153@moooo.ath.cx>
Matthias Lederhofer <matled@gmx.net> writes:
> Junio C Hamano <junkio@cox.net> wrote:
>> Matthias Lederhofer <matled@gmx.net> writes:
>> Is this replacement for your earlier two patches?
> Yes, it is, they did not apply on the current master. (I fetched the
> new heads, had a short look at the log but forgot to update my working
> heads :) ).
>
>> [PATCH] gitweb: export-ok option
>> [PATCH] gitweb: option 'strict export'
>>
>> What's the difference between this and the previous two?
>
> Now I decided to split it in another way: first remove the
> no-such-directory error and then implement the two new features. The
> patches should be equivalent to those before.
I see.
The PATHINFO stuff Martin Waitz did mucks with $project somewhat
later than this part of the patch, possibly bypassing your
checks. Could you check what's in 'master' to see if it is
reasonable and if not fix it up please?
^ permalink raw reply
* Re: [PATCH 2/2 (take 2)] gitweb: Make git_get_refs_list do work of git_get_references
From: Jakub Narebski @ 2006-09-17 10:06 UTC (permalink / raw)
To: git
In-Reply-To: <eej6dr$i3o$2@sea.gmane.org>
Jakub Narebski wrote:
> Jakub Narebski wrote:
>
>> Corrected in "take 3" of this patch.
>
> Which somehow didn't get to the list...
And "take 2" didn't get to the list either.
Stupid filters, or should I just wait a bit?
--
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git
^ permalink raw reply
* Re: [PATCH] Add virtualization support to git-daemon
From: Junio C Hamano @ 2006-09-17 10:07 UTC (permalink / raw)
To: Jon Loeliger; +Cc: git
In-Reply-To: <E1GOint-00048W-2A@jdl.com>
I have a few stylistic comments but the contents look quite sane
and fine.
> I use an inetd invocation like this example:
>
> git stream tcp nowait nobody /usr/bin/git-daemon git-daemon --inetd
> --verbose --syslog
> --export-all --interpolated-path=/pub/%H/%D
> /pub/software /software
> /pub/www.example.com/software
> /pub/www.example.org/software
> /pub
This would be nice to have as an example in the documentation.
> @@ -45,6 +51,23 @@ static const char *user_path;
> static unsigned int timeout;
> static unsigned int init_timeout;
>
> +/*
> + * Static table for now. Ugh.
> + * Feel free to make dynamic as needed.
> + */
> +#define INTERP_SLOT_HOST (0)
> +#define INTERP_SLOT_DIR (1)
> +#define INTERP_SLOT_PERCENT (2)
> +
> +struct interp interp_table[] = {
> + { "%H", 0},
> + { "%D", 0},
> + { "%%", "%"},
> +};
> +
> +#define N_INTERPS (sizeof(interp_table) / sizeof(struct interp))
Shouldn't this variable static to the file?
We seem to use ARRAY_SIZE() macro, which is not particularly my
favorite but other places already use it so probably we would
want to keep consistency.
> + loginfo("Before interpolation '%s'", dir);
> + loginfo("Interp slot 0 (%s,%s)",
> + interp_table[0].name, interp_table[0].value);
> + loginfo("Interp slot 1 (%s,%s)",
> + interp_table[1].name, interp_table[1].value);
> + interpolate(interp_path, PATH_MAX, interpolated_path,
> + interp_table, N_INTERPS);
> + loginfo("After interpolation '%s'", interp_path);
> + dir = interp_path;
Do we really want to see each step of interpolation in the log?
Sounds more like logdebug than loginfo to me.
> + snprintf(rpath, PATH_MAX, "%s%s", base_path, dir);
> + loginfo("dir was %s", dir);
> + loginfo("base_path is %s", base_path);
> + loginfo("rpath now %s", rpath);
> + dir = rpath;
Likewise.
> @@ -351,6 +398,29 @@ static void make_service_overridable(con
> die("No such service %s", name);
> }
>
> +void parse_extra_args(char *extra_args, int buflen)
static?
> @@ -391,13 +461,19 @@ #endif
> if (len && line[len-1] == '\n')
> line[--len] = 0;
>
> + if (len != pktlen) {
> + parse_extra_args(line + len + 1, pktlen - len - 1);
> + }
> +
No { } around single statement.
> +int
> +interpolate(char *result, int reslen,
> + char *orig,
> + struct interp *interps, int ninterps)
> +{
"int interpolate(..."; the return type and function name on the
same line (again, not my preference but just doing as Romans do).
> + bzero(result, reslen);
memset(result, 0, reslen);
> diff --git a/interpolate.h b/interpolate.h
> new file mode 100644
> index 0000000..241af7c
> --- /dev/null
> +++ b/interpolate.h
> @@ -0,0 +1,13 @@
#ifndef INTERPOLATE_H
#define INTERPOLATE_H
...
#endif
> +/*
> + * Copyright 2006 Jon Loeliger
> + */
> +
> +struct interp {
> + char *name;
> + char *value;
> +};
> +
> +extern int interpolate(char *result, int reslen,
> + char *orig,
> + struct interp *interps, int ninterps);
> +
^ permalink raw reply
* Re: [PATCH 2/2 (take 2)] gitweb: Make git_get_refs_list do work of git_get_references
From: Jakub Narebski @ 2006-09-17 10:02 UTC (permalink / raw)
To: git
In-Reply-To: <200609171158.48358.jnareb@gmail.com>
Jakub Narebski wrote:
> Corrected in "take 3" of this patch.
Which somehow didn't get to the list...
--
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git
^ permalink raw reply
* Re: [PATCH 2/2 (take 2)] gitweb: Make git_get_refs_list do work of git_get_references
From: Jakub Narebski @ 2006-09-17 10:00 UTC (permalink / raw)
To: git
In-Reply-To: <7vfyeq6dn3.fsf@assigned-by-dhcp.cox.net>
Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
>
> > This was supposed to make gitweb performance better, by
> > (in the case of git_summary) replacing three calls to git-peek-remote
> > (or one reading info/refs and two calls to git-peek-remote) by only
> > one such a call. ApacheBench shows that after changes summary and tags
> > views are slower, while heads remains the same or even faster (?).
>
> That does not give us much confidence in the patch, does it? If
> it results in cleaner code that is fine, though.
This was because the additional test you proposed for "it is tag if
it is followed by deref" was implemented incorrectly. Corrected in "take 3"
of this patch.
> > @@ -1107,8 +1115,8 @@ sub git_get_refs_list {
> > push @reflist, \%ref_item;
> > }
> > # sort refs by age
> > - @reflist = sort {$b->{'epoch'} <=> $a->{'epoch'}} @reflist;
> > - return \@reflist;
> > + #@reflist = sort {$b->{'epoch'} <=> $a->{'epoch'}} @reflist;
> > + return (\@reflist, \%refs);
> > }
>
> I've already said I do not like commented out lines that do not
> say why they are commented out, but I sense something more
> serious here. Doesn't this removal of sorting affect the
> callers?
Corrected in "take 3" of this patch.
> > @@ -2072,14 +2080,14 @@ sub git_tags_body {
> >
> > sub git_heads_body {
> > # uses global variable $project
> > - my ($taglist, $head, $from, $to, $extra) = @_;
> > + my ($headlist, $head, $from, $to, $extra) = @_;
> > $from = 0 unless defined $from;
> > - $to = $#{$taglist} if (!defined $to || $#{$taglist} < $to);
> > + $to = $#{$headlist} if (!defined $to || $#{$headlist} < $to);
> >
> > print "<table class=\"heads\" cellspacing=\"0\">\n";
> > my $alternate = 0;
> > for (my $i = $from; $i <= $to; $i++) {
> > - my $entry = $taglist->[$i];
> > + my $entry = $headlist->[$i];
> > my %tag = %$entry;
> > my $curr = $tag{'id'} eq $head;
> > if ($alternate) {
>
> Unrelated but indeed is a good clean-up. It was quite confusing
> before (I think this was a result of cut and paste reuse).
And I see that this cleanup is incomplete (%tag = %$entry). Gaah.
Should I split cleanup patch?
> > @@ -2500,7 +2518,7 @@ sub git_tags {
> > git_print_page_nav('','', $head,undef,$head);
> > git_print_header_div('summary', $project);
> >
> > - my $taglist = git_get_refs_list("refs/tags");
> > + my ($taglist) = git_get_refs_list("tags");
> > if (defined @$taglist) {
> > git_tags_body($taglist);
> > }
> > @@ -2513,9 +2531,9 @@ sub git_heads {
> > git_print_page_nav('','', $head,undef,$head);
> > git_print_header_div('summary', $project);
> >
> > - my $taglist = git_get_refs_list("refs/heads");
> > - if (defined @$taglist) {
> > - git_heads_body($taglist, $head);
> > + my ($headlist) = git_get_refs_list("heads");
> > + if (defined @$headlist) {
> > + git_heads_body($headlist, $head);
> > }
> > git_footer_html();
> > }
>
> It always confuses me when people say:
>
> if (defined @$arrayref)
Gaah. It should be either
if (defined $arrayref)
or
if (@$arrayref)
or (if one is truly paranoid) ;-)
if (defined $arrayref && ref($arrayref) eq "ARRAY" && @$arrayref)
[...]
> You probably would want to grep for 'defined @' and fix them all
> at once.
Will do. Unless someone will do this first.
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: gitweb on OpenBSD (-T not supported on filesystems...)
From: Jakub Narebski @ 2006-09-17 9:51 UTC (permalink / raw)
To: git
In-Reply-To: <op.tf0n3bjjqpav6l@crystal.sienna.ath.cx>
Andrew Pamment wrote:
> On Sun, 17 Sep 2006 18:42:12 +0930, Jakub Narebski <jnareb@gmail.com>
> wrote:
>
>> What Perl version do you use? Does 'perldoc -f -X' has the following
>> lines?
>>
>> -X FILEHANDLE
>>
>> (...) If "-T" or "-B" is used on a filehandle, the current IO
>> buffer
>> is examined rather than the first block. (...)
>
> The version of perl is the one that comes with OpenBSD 3.9, it is v5.8.6.
>
> I do get the above text when I run 'perldoc -f -X', does this mean it
> should work?
I thought that OpenBSD has perhaps distributed some old Perl version that
doesn't have -X tests (including -T/-B) on filehandles.
> When I try the original -T $fd I get:
>
> Software error:
> -T and -B not implemented on filehandles at /var/www/htdocs/git/gitweb.cgi
> line 1598.
So this means that Perl implementation in OpenBSD is flawed.
Does anyone else has problem with gitweb on other systems? (I guess that
gitweb doesn't work with ActivePerl).
>> The solution for you would be to skip -T test.
>
> That works fine. Thanks.
The -T test is just a fallback for when we cannot derive mimetype. I don't
know if you should default to 'application/octet-stream' or 'text/plain' in
your case; that perhaps depends on the repository.
--
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git
^ permalink raw reply
* Re: [PATCH] apply --unidiff-zero: loosen sanity checks for --unidiff=0 patches
From: Junio C Hamano @ 2006-09-17 9:50 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git
In-Reply-To: <eej2s7$97c$2@sea.gmane.org>
Jakub Narebski <jnareb@gmail.com> writes:
> By the way, could you document this option (usage and manpage), please?
Will not do this late at night. The list is welcome to come up
with a janotorial patch while I take a nap ;-).
^ permalink raw reply
* Re: gitweb on OpenBSD (-T not supported on filesystems...)
From: Andrew Pamment @ 2006-09-17 9:46 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git@vger.kernel.org
In-Reply-To: <eej3g3$d9j$1@sea.gmane.org>
On Sun, 17 Sep 2006 18:42:12 +0930, Jakub Narebski <jnareb@gmail.com>
wrote:
> What Perl version do you use? Does 'perldoc -f -X' has the following
> lines?
>
> -X FILEHANDLE
>
> (...) If "-T" or "-B" is used on a filehandle, the current IO
> buffer
> is examined rather than the first block. (...)
The version of perl is the one that comes with OpenBSD 3.9, it is v5.8.6.
I do get the above text when I run 'perldoc -f -X', does this mean it
should work? When I try the original -T $fd I get:
Software error:
-T and -B not implemented on filehandles at /var/www/htdocs/git/gitweb.cgi
line 1598.
> The solution for you would be to skip -T test.
That works fine. Thanks.
Andrew
--
Mort - http://www.mort-os.com/
^ permalink raw reply
* Re: gitweb on OpenBSD (-T not supported on filesystems...)
From: Jakub Narebski @ 2006-09-17 9:12 UTC (permalink / raw)
To: git
In-Reply-To: <op.tf0k9fdvqpav6l@crystal.sienna.ath.cx>
Andrew Pamment wrote:
> I'm not sure if this is useful but I thought I would post it anyway,
>
> in gitweb.cgi on line 1560 where you have -T testing a file descripter, it
> doesn't work on OpenBSD 3.9, which makes viewing blobs not work.
>
> I solved this (or I think I have) by replacing $fd with $filename.
This doesn't work. $filename is file name relative to the git repository,
i.e. full path would be $projectroot/$project/../$file_name _if_ the
repository is full repository and not bare repository, and when projects
are symlinked .. goes up linked directory.
We need '-T $fd', where $fd is open pipe from git-cat-file, to avoid
creating temporary files (we just removed need for $tmp_dir and temporary
files for creating diffs).
What Perl version do you use? Does 'perldoc -f -X' has the following lines?
-X FILEHANDLE
(...) If "-T" or "-B" is used on a filehandle, the current IO buffer
is examined rather than the first block. (...)
The solution for you would be to skip -T test.
--
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git
^ permalink raw reply
* Re: [PATCH] gitweb: use correct mime type even if filename has multiple dots.
From: Junio C Hamano @ 2006-09-17 9:07 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git
In-Reply-To: <eej2i1$97c$1@sea.gmane.org>
Jakub Narebski <jnareb@gmail.com> writes:
> Is it really useful? Usually the suffix in mime.types doesn't contain dot
> itself. Besides, to be said we need correct mimetype only for files which
> can be displayed in browser (HTML, XHTML, images: png, gif, jpeg, perhaps
> XML). All other can get generic mimetype, i.e. application/octet-stream for
> binary files (to be saved) and text/plain for text file (to be displayer
> as-is in browser).
Sorry, I do not think that is quite correct. The browser can
launch an appropriate application on the downloaded file, as
long as you send the data labelled with proper mimetype. If you
send out application/octet-stream, of course that would not work.
Having said that, I am not sure what special things Martin
wanted to do with x.8.html that is different from y.html, so
maybe discussing the patch does not have merit at all.
^ permalink raw reply
* Re: [PATCH] gitweb: export options
From: Matthias Lederhofer @ 2006-09-17 9:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v64fm7u2q.fsf@assigned-by-dhcp.cox.net>
Junio C Hamano <junkio@cox.net> wrote:
> Matthias Lederhofer <matled@gmx.net> writes:
> Is this replacement for your earlier two patches?
Yes, it is, they did not apply on the current master. (I fetched the
new heads, had a short look at the log but forgot to update my working
heads :) ).
> [PATCH] gitweb: export-ok option
> [PATCH] gitweb: option 'strict export'
>
> What's the difference between this and the previous two?
Now I decided to split it in another way: first remove the
no-such-directory error and then implement the two new features. The
patches should be equivalent to those before.
^ permalink raw reply
* Re: [PATCH] apply --unidiff-zero: loosen sanity checks for --unidiff=0 patches
From: Jakub Narebski @ 2006-09-17 9:01 UTC (permalink / raw)
To: git
In-Reply-To: <7vac4y7u8n.fsf@assigned-by-dhcp.cox.net>
Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
>
>> Junio C Hamano wrote:
>>
>>> This adds a new flag, --unified-zero, to allow bypassing these
>>> checks. [...]
>>
>> Doesn't -C0 option to git-apply bypass those checks?
>
> Only the head/tail anchor check, but not the creation/deletion
> check.
Ah. Well, --unified-zero (or --context-free) flag makes sense then.
By the way, could you document this option (usage and manpage), please?
--
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git
^ permalink raw reply
* Re: [PATCH] gitweb: fix uninitialized variable warning.
From: Junio C Hamano @ 2006-09-17 9:00 UTC (permalink / raw)
To: Martin Waitz; +Cc: git
In-Reply-To: <20060916210902.GW17042@admingilde.org>
Martin Waitz <tali@admingilde.org> writes:
> @@ -2436,7 +2432,11 @@ sub git_blob {
> } else {
> die_error(undef, "No file name defined");
> }
> + } elsif ($hash =~ m/^[0-9a-fA-F]{40}$/) {
> + # blobs defined by non-textual hash id's can be cached
> + $expires = "+1d";
> }
> +
> my $have_blame = gitweb_check_feature('blame');
> open my $fd, "-|", git_cmd(), "cat-file", "blob", $hash
> or die_error(undef, "Couldn't cat $file_name, $hash");
Will apply, but you seem to be out of sync.
^ permalink raw reply
* Re: [PATCH] gitweb: use correct mime type even if filename has multiple dots.
From: Jakub Narebski @ 2006-09-17 8:56 UTC (permalink / raw)
To: git
In-Reply-To: <7vejua7um3.fsf@assigned-by-dhcp.cox.net>
Junio C Hamano wrote:
>> Looking at /etc/mime.types, it only contains pcf.Z but perhaps
>> it should also contain tar.gz or similiar.
>
> Probably. But that makes me think it might be better to:
>
> - read in mime.types, sort the entries with length of the
> suffixes (longer first);
>
> - try matching the suffixes from longer to shorter and pick the
> first match.
>
> Without that, you would not be able to cope with a /etc/mime.types
> that looks like this, no?
>
> application/a a
> application/b b.a
>
> Perhaps something like the attached.
Is it really useful? Usually the suffix in mime.types doesn't contain dot
itself. Besides, to be said we need correct mimetype only for files which
can be displayed in browser (HTML, XHTML, images: png, gif, jpeg, perhaps
XML). All other can get generic mimetype, i.e. application/octet-stream for
binary files (to be saved) and text/plain for text file (to be displayer
as-is in browser).
Besides, performance will suffer for "blob_plain" view. One hash lookup
vs. nested loops.
--
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git
^ permalink raw reply
* Re: [PATCH] gitweb: export options
From: Junio C Hamano @ 2006-09-17 8:53 UTC (permalink / raw)
To: Matthias Lederhofer; +Cc: git
In-Reply-To: <20060916223101.GB32679@moooo.ath.cx>
Matthias Lederhofer <matled@gmx.net> writes:
> $export_ok: If this variable evaluates to true it is checked
> if a file with this name exists in the repository. If it
> does not exist the repository cannot be viewed from gitweb.
> (Similar to git-daemon-export-ok for git-daemon).
>
> $strict_export: If this variable evaluates to true only
> repositories listed on the project-list-page of gitweb can
> be accessed.
Is this replacement for your earlier two patches?
[PATCH] gitweb: export-ok option
[PATCH] gitweb: option 'strict export'
What's the difference between this and the previous two?
I am asking this question because I am lazy and do not want to
review two sets of similar patches ;-).
^ permalink raw reply
* Re: [PATCH] apply --unidiff-zero: loosen sanity checks for --unidiff=0 patches
From: Junio C Hamano @ 2006-09-17 8:49 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git
In-Reply-To: <eej12j$36t$2@sea.gmane.org>
Jakub Narebski <jnareb@gmail.com> writes:
> Junio C Hamano wrote:
>
>> This adds a new flag, --unified-zero, to allow bypassing these
>> checks. If you are in control of the patch generation process,
>> you should not use --unified=0 patch and fix it up with this
>> flag; rather you should try work with a patch with context. But
>> if all you have to work with is a patch without context, this
>> flag may come handy as the last resort.
>
> Doesn't -C0 option to git-apply bypass those checks?
Only the head/tail anchor check, but not the creation/deletion
check.
Also -CNUM's meaning is "you are allowed to reduce context down
to only this many lines if you need to (but you do not have to
if you can find the right match with context recorded in the
patch)" not "what I am feeding you is a context-free patch so
loosen sanity checks", so it does not make much sense to
overload the flag with such a new meaning (if that is what you
are implying, that is).
^ permalink raw reply
* gitweb on OpenBSD (-T not supported on filesystems...)
From: Andrew Pamment @ 2006-09-17 8:45 UTC (permalink / raw)
To: git
Hi git people
I'm not sure if this is useful but I thought I would post it anyway,
in gitweb.cgi on line 1560 where you have -T testing a file descripter, it
doesn't work on OpenBSD 3.9, which makes viewing blobs not work.
I solved this (or I think I have) by replacing $fd with $filename.
Andrew
--
Mort - http://www.mort-os.com/
^ permalink raw reply
* Re: [PATCH] gitweb: use correct mime type even if filename has multiple dots.
From: Junio C Hamano @ 2006-09-17 8:41 UTC (permalink / raw)
To: Martin Waitz; +Cc: git, Jakub Narebski
In-Reply-To: <20060917075157.GY17042@admingilde.org>
Martin Waitz <tali@admingilde.org> writes:
>> > - $filename =~ /\.(.*?)$/;
>> > - return $mimemap{$1};
>>
>> Actually, that is non-greedy match, so the above code insist that
>> extension starts at the _last_ dot.
>
> hmm, but it didn't work for me.
> I had filenames like "man/program.8.html" which got served as
> "text/html" with the old code.
It based its decision on '.html' part, which is expected from
non-greedy match (if I were writing the pattern, I would have
written /\.[^.]+$/ instead, though). Are you trying to have it
behave differently between "x.8.html" and "x.html"?
> Looking at /etc/mime.types, it only contains pcf.Z but perhaps
> it should also contain tar.gz or similiar.
Probably. But that makes me think it might be better to:
- read in mime.types, sort the entries with length of the
suffixes (longer first);
- try matching the suffixes from longer to shorter and pick the
first match.
Without that, you would not be able to cope with a /etc/mime.types
that looks like this, no?
application/a a
application/b b.a
Perhaps something like the attached.
---
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index a81c8d4..4994a33 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1140,23 +1140,42 @@ sub mimetype_guess_file {
my $filename = shift;
my $mimemap = shift;
-r $mimemap or return undef;
+ local($_);
- my %mimemap;
- open(MIME, $mimemap) or return undef;
+ open MIME, $mimemap
+ or return undef;
+
+ # Under mod_perl caching this may make a lot of sense...
+ my @mime = ();
+ my $maxlen = 0;
while (<MIME>) {
- next if m/^#/; # skip comments
- my ($mime, $exts) = split(/\t+/);
- if (defined $exts) {
- my @exts = split(/\s+/, $exts);
- foreach my $ext (@exts) {
- $mimemap{$ext} = $mime;
+ next if /^#/;
+ chomp;
+ my ($mimetype, @ext) = split(/\s+/);
+ for (@ext) {
+ my $len = length;
+ my $map = $mime[$len];
+ if (!$map) {
+ $mime[$len] = $map = {};
+ $maxlen = $len if ($maxlen < $len);
}
+ # We could detect duplicate definition here... i.e.
+ # onetype ext
+ # anothertype ext
+ $map->{$_} = $mimetype;
}
}
- close(MIME);
+ close MIME;
- $filename =~ /\.(.*?)$/;
- return $mimemap{$1};
+ for ($filename) {
+ for (my $len = $maxlen; 0 < $len; $len--) {
+ my $map = $mime[$len];
+ while (my ($ext, $type) = each %$map) {
+ return $type if (/\.\Q$ext\E$/);
+ }
+ }
+ }
+ return undef;
}
sub mimetype_guess {
^ permalink raw reply related
* Re: [PATCH] apply --unidiff-zero: loosen sanity checks for --unidiff=0 patches
From: Jakub Narebski @ 2006-09-17 8:30 UTC (permalink / raw)
To: git
In-Reply-To: <7vsliq7vqr.fsf_-_@assigned-by-dhcp.cox.net>
Junio C Hamano wrote:
> This adds a new flag, --unified-zero, to allow bypassing these
> checks. If you are in control of the patch generation process,
> you should not use --unified=0 patch and fix it up with this
> flag; rather you should try work with a patch with context. But
> if all you have to work with is a patch without context, this
> flag may come handy as the last resort.
Doesn't -C0 option to git-apply bypass those checks?
--
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git
^ permalink raw reply
* Re: [PATCH] gitweb: use correct mime type even if filename has multiple dots.
From: Jakub Narebski @ 2006-09-17 8:23 UTC (permalink / raw)
To: git
In-Reply-To: <20060917075157.GY17042@admingilde.org>
Martin Waitz wrote:
> hoi :)
>
> On Sat, Sep 16, 2006 at 11:44:52PM +0200, Jakub Narebski wrote:
>> Martin Waitz wrote:
>>
>> > Match the last part of the filename agains the extention from the
>> > mime database instead of insisting that it starts at the first dot.
>> [...]
>> > - $filename =~ /\.(.*?)$/;
>> > - return $mimemap{$1};
>>
>> Actually, that is non-greedy match, so the above code insist that
>> extension starts at the _last_ dot.
>
> hmm, but it didn't work for me.
> I had filenames like "man/program.8.html" which got served as
> "text/html" with the old code.
And why it shouldn't? From the extension it is HTML page, I would guess
manpage converted to HTML (pretty-printed manpage). And it should be served
with text/html mimetype.
> Besides, the new code would cope with extentions that contain a
> dot itself.
But is unnecessary complicated, and I guess performance suffers a bit.
> Looking at /etc/mime.types, it only contains pcf.Z but perhaps
So the *.pcf.Z file wouldn't get correct mimetype. No big.
> it should also contain tar.gz or similiar.
You can't properly serve tar.gz as something other than with
application/x-gzip mimetype. If you want to serve it as/with
application/x-tar mimetype, you would need to add
Content-Encoding: x-gzip
in addition to
Content-Type: application/x-tar
And your code doesn't do that.
The _last_ extension defines the type.
Besides, with the exception of files which can be displayed in browser,
i.e. HTML files and images it doesn't matter what the mimetype is, if only
binary files get binary mimetype (e.g. generic application/octet-stream).
--
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git
^ permalink raw reply
* [PATCH] apply --unidiff-zero: loosen sanity checks for --unidiff=0 patches
From: Junio C Hamano @ 2006-09-17 8:17 UTC (permalink / raw)
To: Gerrit Pape; +Cc: 386495, git
In-Reply-To: <7vzmcz6mrq.fsf@assigned-by-dhcp.cox.net>
One request and one question:
(1) I think the attached patch should fix this. Could you
see if it works for you?
(2) Is it absolutely necessary to work with a context-free
patch in the workflow of the particular job (pciutils
maintenance)? If so what is the reason?
-- >8 --
In "git-apply", we have a few sanity checks and heuristics that
expects that the patch fed to us is a unified diff with at least
one line of context.
* When there is no leading context line in a hunk, the hunk
must apply at the beginning of the preimage. Similarly, no
trailing context means that the hunk is anchored at the end.
* We learn a patch deletes the file from a hunk that has no
resulting line (i.e. all lines are prefixed with '-') if it
has not otherwise been known if the patch deletes the file.
Similarly, no old line means the file is being created.
And we declare an error condition when the file created by a
creation patch already exists, and/or when a deletion patch
still leaves content in the file.
These sanity checks are good safety measures, but breaks down
when people feed a diff generated with --unified=0. This was
recently noticed first by Matthew Wilcox and Gerrit Pape.
This adds a new flag, --unified-zero, to allow bypassing these
checks. If you are in control of the patch generation process,
you should not use --unified=0 patch and fix it up with this
flag; rather you should try work with a patch with context. But
if all you have to work with is a patch without context, this
flag may come handy as the last resort.
Signed-off-by: Junio C Hamano <junkio@cox.net>
---
builtin-apply.c | 112 +++++++++++++++++++++++++++++++-------------
t/t3403-rebase-skip.sh | 4 +-
t/t4104-apply-boundary.sh | 115 +++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 197 insertions(+), 34 deletions(-)
diff --git a/builtin-apply.c b/builtin-apply.c
index 6e0864c..25e90d8 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -27,6 +27,7 @@ static const char *prefix;
static int prefix_length = -1;
static int newfd = -1;
+static int unidiff_zero;
static int p_value = 1;
static int check_index;
static int write_index;
@@ -854,11 +855,10 @@ static int find_header(char *line, unsig
}
/*
- * Parse a unified diff. Note that this really needs
- * to parse each fragment separately, since the only
- * way to know the difference between a "---" that is
- * part of a patch, and a "---" that starts the next
- * patch is to look at the line counts..
+ * Parse a unified diff. Note that this really needs to parse each
+ * fragment separately, since the only way to know the difference
+ * between a "---" that is part of a patch, and a "---" that starts
+ * the next patch is to look at the line counts..
*/
static int parse_fragment(char *line, unsigned long size, struct patch *patch, struct fragment *fragment)
{
@@ -875,31 +875,14 @@ static int parse_fragment(char *line, un
leading = 0;
trailing = 0;
- if (patch->is_new < 0) {
- patch->is_new = !oldlines;
- if (!oldlines)
- patch->old_name = NULL;
- }
- if (patch->is_delete < 0) {
- patch->is_delete = !newlines;
- if (!newlines)
- patch->new_name = NULL;
- }
-
- if (patch->is_new && oldlines)
- return error("new file depends on old contents");
- if (patch->is_delete != !newlines) {
- if (newlines)
- return error("deleted file still has contents");
- fprintf(stderr, "** warning: file %s becomes empty but is not deleted\n", patch->new_name);
- }
-
/* Parse the thing.. */
line += len;
size -= len;
linenr++;
added = deleted = 0;
- for (offset = len; size > 0; offset += len, size -= len, line += len, linenr++) {
+ for (offset = len;
+ 0 < size;
+ offset += len, size -= len, line += len, linenr++) {
if (!oldlines && !newlines)
break;
len = linelen(line, size);
@@ -972,12 +955,18 @@ static int parse_fragment(char *line, un
patch->lines_added += added;
patch->lines_deleted += deleted;
+
+ if (0 < patch->is_new && oldlines)
+ return error("new file depends on old contents");
+ if (0 < patch->is_delete && newlines)
+ return error("deleted file still has contents");
return offset;
}
static int parse_single_patch(char *line, unsigned long size, struct patch *patch)
{
unsigned long offset = 0;
+ unsigned long oldlines = 0, newlines = 0, context = 0;
struct fragment **fragp = &patch->fragments;
while (size > 4 && !memcmp(line, "@@ -", 4)) {
@@ -988,9 +977,11 @@ static int parse_single_patch(char *line
len = parse_fragment(line, size, patch, fragment);
if (len <= 0)
die("corrupt patch at line %d", linenr);
-
fragment->patch = line;
fragment->size = len;
+ oldlines += fragment->oldlines;
+ newlines += fragment->newlines;
+ context += fragment->leading + fragment->trailing;
*fragp = fragment;
fragp = &fragment->next;
@@ -999,6 +990,46 @@ static int parse_single_patch(char *line
line += len;
size -= len;
}
+
+ /*
+ * If something was removed (i.e. we have old-lines) it cannot
+ * be creation, and if something was added it cannot be
+ * deletion. However, the reverse is not true; --unified=0
+ * patches that only add are not necessarily creation even
+ * though they do not have any old lines, and ones that only
+ * delete are not necessarily deletion.
+ *
+ * Unfortunately, a real creation/deletion patch do _not_ have
+ * any context line by definition, so we cannot safely tell it
+ * apart with --unified=0 insanity. At least if the patch has
+ * more than one hunk it is not creation or deletion.
+ */
+ if (patch->is_new < 0 &&
+ (oldlines || (patch->fragments && patch->fragments->next)))
+ patch->is_new = 0;
+ if (patch->is_delete < 0 &&
+ (newlines || (patch->fragments && patch->fragments->next)))
+ patch->is_delete = 0;
+ if (!unidiff_zero || context) {
+ /* If the user says the patch is not generated with
+ * --unified=0, or if we have seen context lines,
+ * then not having oldlines means the patch is creation,
+ * and not having newlines means the patch is deletion.
+ */
+ if (patch->is_new < 0 && !oldlines)
+ patch->is_new = 1;
+ if (patch->is_delete < 0 && !newlines)
+ patch->is_delete = 1;
+ }
+
+ if (0 < patch->is_new && oldlines)
+ die("new file %s depends on old contents", patch->new_name);
+ if (0 < patch->is_delete && newlines)
+ die("deleted file %s still has contents", patch->old_name);
+ if (!patch->is_delete && !newlines && context)
+ fprintf(stderr, "** warning: file %s becomes empty but "
+ "is not deleted\n", patch->new_name);
+
return offset;
}
@@ -1556,9 +1587,19 @@ static int apply_one_fragment(struct buf
/*
* If we don't have any leading/trailing data in the patch,
* we want it to match at the beginning/end of the file.
+ *
+ * But that would break if the patch is generated with
+ * --unified=0; sane people wouldn't do that to cause us
+ * trouble, but we try to please not so sane ones as well.
*/
- match_beginning = !leading && (frag->oldpos == 1);
- match_end = !trailing;
+ if (unidiff_zero) {
+ match_beginning = (!leading && !frag->oldpos);
+ match_end = 0;
+ }
+ else {
+ match_beginning = !leading && (frag->oldpos == 1);
+ match_end = !trailing;
+ }
lines = 0;
pos = frag->newpos;
@@ -1804,7 +1845,7 @@ static int apply_data(struct patch *patc
patch->result = desc.buffer;
patch->resultsize = desc.size;
- if (patch->is_delete && patch->resultsize)
+ if (0 < patch->is_delete && patch->resultsize)
return error("removal patch leaves file contents");
return 0;
@@ -1876,7 +1917,7 @@ static int check_patch(struct patch *pat
old_name, st_mode, patch->old_mode);
}
- if (new_name && prev_patch && prev_patch->is_delete &&
+ if (new_name && prev_patch && 0 < prev_patch->is_delete &&
!strcmp(prev_patch->old_name, new_name))
/* A type-change diff is always split into a patch to
* delete old, immediately followed by a patch to
@@ -1889,7 +1930,8 @@ static int check_patch(struct patch *pat
else
ok_if_exists = 0;
- if (new_name && (patch->is_new | patch->is_rename | patch->is_copy)) {
+ if (new_name &&
+ ((0 < patch->is_new) | (0 < patch->is_rename) | patch->is_copy)) {
if (check_index &&
cache_name_pos(new_name, strlen(new_name)) >= 0 &&
!ok_if_exists)
@@ -1906,7 +1948,7 @@ static int check_patch(struct patch *pat
return error("%s: %s", new_name, strerror(errno));
}
if (!patch->new_mode) {
- if (patch->is_new)
+ if (0 < patch->is_new)
patch->new_mode = S_IFREG | 0644;
else
patch->new_mode = patch->old_mode;
@@ -1957,7 +1999,7 @@ static void show_index_list(struct patch
const char *name;
name = patch->old_name ? patch->old_name : patch->new_name;
- if (patch->is_new)
+ if (0 < patch->is_new)
sha1_ptr = null_sha1;
else if (get_sha1(patch->old_sha1_prefix, sha1))
die("sha1 information is lacking or useless (%s).",
@@ -2543,6 +2585,10 @@ int cmd_apply(int argc, const char **arg
apply_in_reverse = 1;
continue;
}
+ if (!strcmp(arg, "--unidiff-zero")) {
+ unidiff_zero = 1;
+ continue;
+ }
if (!strcmp(arg, "--reject")) {
apply = apply_with_reject = apply_verbosely = 1;
continue;
diff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh
index 8ab63c5..bb25315 100755
--- a/t/t3403-rebase-skip.sh
+++ b/t/t3403-rebase-skip.sh
@@ -37,7 +37,9 @@ test_expect_success setup '
git branch skip-merge skip-reference
'
-test_expect_failure 'rebase with git am -3 (default)' 'git rebase master'
+test_expect_failure 'rebase with git am -3 (default)' '
+ git rebase master
+'
test_expect_success 'rebase --skip with am -3' '
git reset --hard HEAD &&
diff --git a/t/t4104-apply-boundary.sh b/t/t4104-apply-boundary.sh
new file mode 100755
index 0000000..2ff800c
--- /dev/null
+++ b/t/t4104-apply-boundary.sh
@@ -0,0 +1,115 @@
+#!/bin/sh
+#
+# Copyright (c) 2005 Junio C Hamano
+#
+
+test_description='git-apply boundary tests
+
+'
+. ./test-lib.sh
+
+L="c d e f g h i j k l m n o p q r s t u v w x"
+
+test_expect_success setup '
+ for i in b '"$L"' y
+ do
+ echo $i
+ done >victim &&
+ cat victim >original &&
+ git update-index --add victim &&
+
+ : add to the head
+ for i in a b '"$L"' y
+ do
+ echo $i
+ done >victim &&
+ cat victim >add-a-expect &&
+ git diff victim >add-a-patch.with &&
+ git diff --unified=0 >add-a-patch.without &&
+
+ : modify at the head
+ for i in a '"$L"' y
+ do
+ echo $i
+ done >victim &&
+ cat victim >mod-a-expect &&
+ git diff victim >mod-a-patch.with &&
+ git diff --unified=0 >mod-a-patch.without &&
+
+ : remove from the head
+ for i in '"$L"' y
+ do
+ echo $i
+ done >victim &&
+ cat victim >del-a-expect &&
+ git diff victim >del-a-patch.with
+ git diff --unified=0 >del-a-patch.without &&
+
+ : add to the tail
+ for i in b '"$L"' y z
+ do
+ echo $i
+ done >victim &&
+ cat victim >add-z-expect &&
+ git diff victim >add-z-patch.with &&
+ git diff --unified=0 >add-z-patch.without &&
+
+ : modify at the tail
+ for i in a '"$L"' y
+ do
+ echo $i
+ done >victim &&
+ cat victim >mod-z-expect &&
+ git diff victim >mod-z-patch.with &&
+ git diff --unified=0 >mod-z-patch.without &&
+
+ : remove from the tail
+ for i in b '"$L"'
+ do
+ echo $i
+ done >victim &&
+ cat victim >del-z-expect &&
+ git diff victim >del-z-patch.with
+ git diff --unified=0 >del-z-patch.without &&
+
+ : done
+'
+
+for with in with without
+do
+ case "$with" in
+ with) u= ;;
+ without) u='--unidiff-zero ' ;;
+ esac
+ for kind in add-a add-z mod-a mod-z del-a del-z
+ do
+ test_expect_success "apply $kind-patch $with context" '
+ cat original >victim &&
+ git update-index victim &&
+ git apply --index '"$u$kind-patch.$with"' || {
+ cat '"$kind-patch.$with"'
+ (exit 1)
+ } &&
+ diff -u '"$kind"'-expect victim
+ '
+ done
+done
+
+for kind in add-a add-z mod-a mod-z del-a del-z
+do
+ rm -f $kind-ng.without
+ sed -e "s/^diff --git /diff /" \
+ -e '/^index /d' \
+ <$kind-patch.without >$kind-ng.without
+ test_expect_success "apply non-git $kind-patch without context" '
+ cat original >victim &&
+ git update-index victim &&
+ git apply --unidiff-zero --index '"$kind-ng.without"' || {
+ cat '"$kind-ng.without"'
+ (exit 1)
+ } &&
+ diff -u '"$kind"'-expect victim
+ '
+done
+
+test_done
--
1.4.2.1.gca4e5c
^ permalink raw reply related
* Re: [PATCH] gitweb: use correct mime type even if filename has multiple dots.
From: Martin Waitz @ 2006-09-17 7:51 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git
In-Reply-To: <eehr7e$5i0$1@sea.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 1051 bytes --]
hoi :)
On Sat, Sep 16, 2006 at 11:44:52PM +0200, Jakub Narebski wrote:
> Martin Waitz wrote:
>
> > Match the last part of the filename agains the extention from the
> > mime database instead of insisting that it starts at the first dot.
> [...]
> > - $filename =~ /\.(.*?)$/;
> > - return $mimemap{$1};
>
> Actually, that is non-greedy match, so the above code insist that
> extension starts at the _last_ dot.
hmm, but it didn't work for me.
I had filenames like "man/program.8.html" which got served as
"text/html" with the old code.
Besides, the new code would cope with extentions that contain a
dot itself.
Looking at /etc/mime.types, it only contains pcf.Z but perhaps
it should also contain tar.gz or similiar.
>
> --
> Jakub Narebski
> Warsaw, Poland
> ShadeHawk on #git
>
>
> -
> 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
--
Martin Waitz
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply
* Re: [PATCH 1/2] Add [-s|--hash] option to Linus' show-ref.
From: Jakub Narebski @ 2006-09-17 7:15 UTC (permalink / raw)
To: git
In-Reply-To: <20060917062024.c476e07d.chriscool@tuxfamily.org>
Christian Couder wrote:
> With this option only the sha1 hash of the ref should
> be printed.
Could you please update manpage as well?
--
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git
^ permalink raw reply
* Re: git-apply fails to apply some patches
From: Junio C Hamano @ 2006-09-17 6:16 UTC (permalink / raw)
To: Gerrit Pape; +Cc: 386495, git
In-Reply-To: <20060915132225.12040.qmail@a0896f98af57b2.315fe32.mid.smarden.org>
Gerrit Pape <pape@smarden.org> writes:
>...
> On top of that, I try to apply this interdiff generated patch:
>
> diff -u pciutils-2.1.11/debian/dirs pciutils-2.1.11/debian/dirs
> --- pciutils-2.1.11/debian/dirs
> +++ pciutils-2.1.11/debian/dirs
> @@ -6,0 +7 @@
> +var/lib/pciutils
>
> and git-apply says:
>
> error: debian/dirs: already exists in working directory
>
> I suspect it's confused by the '-x,0' thinking that means "file does not
> exist" rather than "we have 0 context for this diff".
There are a few safety checks we perform assuming that the patch
has a few lines of context, and this is falsely triggering one
of them. It incorrectly thinks that seeing _no_ context nor old
lines in the patch means this is a file creation patch, and
complains because it knows the file being patched already
exists.
I am looking into a way to handle a context-less patch line the
one quoted, without breaking the sanity check we have.
In the meantime you should be able to work things around by not
feeding --unified=0 diff output.
^ permalink raw reply
* [PATCH 2/2] Use Linus' show ref in "git-branch.sh".
From: Christian Couder @ 2006-09-17 4:32 UTC (permalink / raw)
To: Junio Hamano; +Cc: git
In-Reply-To: <20060917062024.c476e07d.chriscool@tuxfamily.org>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
git-branch.sh | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/git-branch.sh b/git-branch.sh
index e0501ec..2600e9c 100755
--- a/git-branch.sh
+++ b/git-branch.sh
@@ -21,7 +21,7 @@ delete_branch () {
,,)
die "What branch are you on anyway?" ;;
esac
- branch=$(cat "$GIT_DIR/refs/heads/$branch_name") &&
+ branch=$(git-show-ref --verify --hash -- "refs/heads/$branch_name") &&
branch=$(git-rev-parse --verify "$branch^0") ||
die "Seriously, what branch are you talking about?"
case "$option" in
@@ -112,7 +112,7 @@ rev=$(git-rev-parse --verify "$head") ||
git-check-ref-format "heads/$branchname" ||
die "we do not like '$branchname' as a branch name."
-if [ -e "$GIT_DIR/refs/heads/$branchname" ]
+if git-show-ref --verify --quiet -- "refs/heads/$branchname"
then
if test '' = "$force"
then
--
1.4.2.1.gea00f-dirty
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox