* Re: [PATCH 1/8] gitweb: Extract print_sidebyside_diff_lines()
From: Michał Kiedrowicz @ 2012-02-11 23:03 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git
In-Reply-To: <m3obt5tn0g.fsf@localhost.localdomain>
Jakub Narebski <jnareb@gmail.com> wrote:
> Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:
>
> > Currently, print_sidebyside_diff_chunk() does two things: it
> > accumulates diff lines and prints them. Accumulation may be used to
> > perform additional operations on diff lines, so it makes sense to split
> > these two things. Thus, the code that prints diff lines in a side-by-side
> > manner is moved out of print_sidebyside_diff_chunk() to a separate
> > subroutine.
> >
> > The outcome of this patch is that print_sidebyside_diff_chunk() is now
> > much shorter and easier to read.
> >
> > This change is meant to be a simple code movement. No behavior change is
> > intended.
> >
> > Signed-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>
>
> As it is pure refactoring, and improves readibility of code quite a
> bit, I am all for it, but...
>
> You replace the following set of conditions
>
> > - ## print from accumulator when type of class of lines change
> > - # empty contents block on start rem/add block, or end of chunk
> > - if (@ctx && (!$class || $class eq 'rem' || $class eq 'add')) {
>
> and
>
> > - # empty add/rem block on start context block, or end of chunk
> > - if ((@rem || @add) && (!$class || $class eq 'ctx')) {
> > - if (!@add) {
> [...]
> > - } elsif (!@rem) {
> [...]
> > - } else {
>
>
> with these (I have reindented the code to match)
>
> > + ## print from accumulator when have some add/rem lines or end
> > + # of chunk (flush context lines)
> > + if (((@rem || @add) && $class eq 'ctx') || !$class) {
>
> > + if (@$ctx) {
> [...]
> > + }
> > + if (!@$add) {
> [...]
> > + } elsif (!@$rem) {
> [...]
> > + } else {
>
> It is not obvious that the final result is the same.
You are right. I should have described it in the commit message.
>
> BTW doesn't new code print context when printing added and removed
> lines, and not as soon as class changes? This doesn't change the
> output, but it slightly changes behavior.
>
This is also true. I'll describe it in the commit message. I could also
create two functions: one that prints @ctx and second one that prints
@rem and @add but then I would have to create two other functions for
inline diff.
^ permalink raw reply
* Re: [PATCH 2/8] gitweb: Use print_diff_chunk() for both side-by-side and inline diffs
From: Michał Kiedrowicz @ 2012-02-11 23:16 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git
In-Reply-To: <m3k43ttlh9.fsf@localhost.localdomain>
Jakub Narebski <jnareb@gmail.com> wrote:
> Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:
>
> > This renames print_sidebyside_diff_chunk() to print_diff_chunk() and
> > makes use of it for both side-by-side and inline diffs. Now diff lines
> > are always accumulated before they are printed. This opens the
> > possibility to preprocess diff output before it's printed.
> >
> > The new function print_inline_diff_lines() could reorder diff lines.
>
> I think you meant here "if left as is", isn't it?
Yes.
> > It first prints all context lines, then all removed lines and
> > finally all added lines. If the diff output consisted of mixed added
> > and removed lines, gitweb would reorder these lines. This is
> > especially true for combined diff output, for example:
> >
> > - removed line for first parent
> > + added line for first parent
> > -removed line for second parent
> > ++added line for both parents
> >
> > would be rendered as:
> >
> > - removed line for first parent
> > -removed line for second parent
> > + added line for first parent
> > ++added line for both parents
>
> This is not "is especially true"; it can happen (though: can it
> really?)
Yeah. See
http://git.kernel.org/?p=git/git.git;a=commitdiff;h=5de89d3abfca98b0dfd0280d28576940c913d60d
> in combined diff output; in ordinary diff you would always
> have context or end/beginning of chunk between added and removed
> lines.
OK, I'll reword this part of the commit message.
>
> >
> > To prevent gitweb from reordering lines, print_diff_chunk() calls
> > print_diff_lines() as soon as it detects that both added and removed
> > lines are present and there was a class change.
>
> You really should mention, in my opinion, that this change is
> preparation for diff refinement highlighting (higlighting the
> differing segments).
>
> Otherwise I don't see the reason for it: ordinary diff didn't need the
> fancy stuff with accumulating then printing, and could be send
> straightly to output. This adds complexity for non-sidebyside diff,
> unnecessary if not for diff refinement highlighting.
I completely agree with that. This patch doesn't makes sense without
later patches.
>
> > Signed-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>
> > ---
> > gitweb/gitweb.perl | 53 +++++++++++++++++++++++++++++++++++++--------------
> > 1 files changed, 38 insertions(+), 15 deletions(-)
> >
> > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> > index 1247607..ed63261 100755
> > --- a/gitweb/gitweb.perl
> > +++ b/gitweb/gitweb.perl
> > @@ -4908,9 +4908,31 @@ sub print_sidebyside_diff_lines {
> > }
> > }
> >
> > -sub print_sidebyside_diff_chunk {
> > - my @chunk = @_;
> > +# Print context lines and then rem/add lines in inline manner.
> > +sub print_inline_diff_lines {
> > + my ($ctx, $rem, $add) = @_;
> > +
> > + print foreach (@$ctx);
> > + print foreach (@$rem);
> > + print foreach (@$add);
> > +}
> > +
> > +# Print context lines and then rem/add lines.
> > +sub print_diff_lines {
> > + my ($ctx, $rem, $add, $diff_style, $is_combined) = @_;
> > +
> > + if ($diff_style eq 'sidebyside' && !$is_combined) {
> > + print_sidebyside_diff_lines($ctx, $rem, $add);
> > + } else {
> > + # default 'inline' style and unknown styles
> > + print_inline_diff_lines($ctx, $rem, $add);
> > + }
> > +}
>
> That looks nice.
>
> BTW. I didn't examine the final code, but what happens for binary
> diffs that git supports? Is it handled outside print_diff_chunk()?
Good question. Will check.
>
> > +
> > +sub print_diff_chunk {
> > + my ($diff_style, $is_combined, @chunk) = @_;
> > my (@ctx, @rem, @add);
> > + my $prev_class = '';
>
> Is $prev_class here class of previous chunk fragment or _previous class_
> (i.e. always different from $class), or is it class of previous line?
> It is not obvious here, from variable name. Better name or at least
> a comment would be appreciated.
>
It's a class of previous line.
> >
> > return unless @chunk;
> >
> > @@ -4935,9 +4957,13 @@ sub print_sidebyside_diff_chunk {
> > }
> >
> > ## print from accumulator when have some add/rem lines or end
> > - # of chunk (flush context lines)
> > - if (((@rem || @add) && $class eq 'ctx') || !$class) {
> > - print_sidebyside_diff_lines(\@ctx, \@rem, \@add);
> > + # of chunk (flush context lines), or when have add and rem
> > + # lines and new block is reached (otherwise add/rem lines could
> > + # be reordered)
> > + if (((@rem || @add) && $class eq 'ctx') || !$class ||
> > + (@rem && @add && $class ne $prev_class)) {
>
> We usually use tabs to align, but spaces to indent in gitweb, so it
> would be
>
> > + if (((@rem || @add) && $class eq 'ctx') || !$class ||
> > + (@rem && @add && $class ne $prev_class)) {
>
OK.
> Anyway, this conditional gets ridicously complicated. I wonder if we
> could simplify it.
>
> If we allow printing context together widh added/removed lines, and
> not as soon as possible, perhaps good conditional would be: class
> changed, and accumulator for current class is full:
>
> > + if ($class ne $prev_class && @{ $acc{$class} }) {
>
> Or something like that, where
>
> my %acc = (ctx => \@ctx, add => \@add, rem => \@rem);
>
> If we want to print context as soon as possible, like it was before
> 1st patch in this series, we could uuse the following conditional
>
> > + if ($class ne $prev_class &&
> > + ($prev_class eq 'ctx' || @{ $acc{$class} })) {
>
> Perhaps with "scalar @{ $acc{$class} }" if it would make things more
> clear.
>
> Though probably to avoid "Use of uninitialized value in string ne"
> we would probably have to use empty string instead of undefined
> value for "no class" case, i.e. beginning or end of chunk.
>
OK, I'll try to work out something.
> > + print_diff_lines(\@ctx, \@rem, \@add, $diff_style,
> > + $is_combined);
> > @ctx = @rem = @add = ();
> > }
> >
> > @@ -4954,6 +4980,8 @@ sub print_sidebyside_diff_chunk {
> > if ($class eq 'ctx') {
> > push @ctx, $line;
> > }
> > +
> > + $prev_class = $class;
> > }
> > }
> [...]
>
^ permalink raw reply
* Re: [PATCH 4/8] gitweb: Push formatting diff lines to print_diff_chunk()
From: Michał Kiedrowicz @ 2012-02-11 23:20 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git
In-Reply-To: <m3bop5tjt6.fsf@localhost.localdomain>
Jakub Narebski <jnareb@gmail.com> wrote:
> Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:
>
> > Now git_patchset_body() only calls diff_line_class() (removed from
> > process_diff_line()). The latter function is renamed to
> > format_diff_line() and is called from print_diff_chunk().
> >
> > This is a pure code movement, needed for processing raw diff lines in
> > the accumulator in print_diff_chunk(). No behavior change is intended by
> > this change.
>
> Well, this is not "pure code movement" per se; it is meant to be
> refactoring that doesn't change gitweb output nor behavior.
>
> If I understand correctly the change is from
>
> read
> format
> accumulate
> print
>
> to
>
> read
> accumulate
> format
> print
>
> Isn't it?
Yeah, this is what I meant :).
>
> As a note I would add also that process_diff_line got renamed to
> format_diff_line,
I thought I wrote that ("The latter function is renamed...")?
> and its output changed to returning only
> HTML-formatted line, which bringg it in line with other format_*
> subroutines.
>
OK.
> > Signed-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>
>
> I think it is a good change even without subsequent patches.
>
> Acked-by: Jakub Narębski <jnareb@gmail.com>
>
Thanks.
> > ---
> > gitweb/gitweb.perl | 25 ++++++++++++-------------
> > 1 files changed, 12 insertions(+), 13 deletions(-)
> >
> > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> > index d2f75c4..cae9dfa 100755
> > --- a/gitweb/gitweb.perl
> > +++ b/gitweb/gitweb.perl
> > @@ -2320,12 +2320,9 @@ sub format_cc_diff_chunk_header {
> > }
> >
> > # process patch (diff) line (not to be used for diff headers),
> > -# returning class and HTML-formatted (but not wrapped) line
> > -sub process_diff_line {
> > - my $line = shift;
> > - my ($from, $to) = @_;
> > -
> > - my $diff_class = diff_line_class($line, $from, $to);
> > +# returning HTML-formatted (but not wrapped) line
> > +sub format_diff_line {
> > + my ($line, $diff_class, $from, $to) = @_;
> >
> > chomp $line;
> > $line = untabify($line);
> > @@ -2343,7 +2340,7 @@ sub process_diff_line {
> > $diff_classes .= " $diff_class" if ($diff_class);
> > $line = "<div class=\"$diff_classes\">$line</div>\n";
> >
> > - return $diff_class, $line;
> > + return $line;
> > }
> >
> > # Generates undef or something like "_snapshot_" or "snapshot (_tbz2_ _zip_)",
> > @@ -4934,7 +4931,7 @@ sub print_diff_lines {
> > }
> >
> > sub print_diff_chunk {
> > - my ($diff_style, $is_combined, @chunk) = @_;
> > + my ($diff_style, $is_combined, $from, $to, @chunk) = @_;
> > my (@ctx, @rem, @add);
> > my $prev_class = '';
> >
> > @@ -4954,6 +4951,8 @@ sub print_diff_chunk {
> > foreach my $line_info (@chunk) {
> > my ($class, $line) = @$line_info;
> >
> > + $line = format_diff_line($line, $class, $from, $to);
> > +
> > # print chunk headers
> > if ($class && $class eq 'chunk_header') {
> > print $line;
> > @@ -5107,19 +5106,19 @@ sub git_patchset_body {
> >
> > next PATCH if ($patch_line =~ m/^diff /);
> >
> > - my ($class, $line) = process_diff_line($patch_line, \%from, \%to);
> > + my $class = diff_line_class($patch_line, \%from, \%to);
> >
> > if ($class eq 'chunk_header') {
> > - print_diff_chunk($diff_style, $is_combined, @chunk);
> > - @chunk = ( [ $class, $line ] );
> > + print_diff_chunk($diff_style, $is_combined, \%from, \%to, @chunk);
> > + @chunk = ( [ $class, $patch_line ] );
> > } else {
> > - push @chunk, [ $class, $line ];
> > + push @chunk, [ $class, $patch_line ];
> > }
> > }
> >
> > } continue {
> > if (@chunk) {
> > - print_diff_chunk($diff_style, $is_combined, @chunk);
> > + print_diff_chunk($diff_style, $is_combined, \%from, \%to, @chunk);
> > @chunk = ();
> > }
> > print "</div>\n"; # class="patch"
> > --
> > 1.7.3.4
> >
>
> Nice!
>
^ permalink raw reply
* Re: [PATCH 4/8] gitweb: Push formatting diff lines to print_diff_chunk()
From: Michał Kiedrowicz @ 2012-02-11 23:30 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git
In-Reply-To: <20120212002025.214b3fb3@gmail.com>
Michał Kiedrowicz <michal.kiedrowicz@gmail.com> wrote:
> Jakub Narebski <jnareb@gmail.com> wrote:
>
> > Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:
> >
> > > Now git_patchset_body() only calls diff_line_class() (removed from
> > > process_diff_line()). The latter function is renamed to
> > > format_diff_line() and is called from print_diff_chunk().
> > >
> > > This is a pure code movement, needed for processing raw diff lines in
> > > the accumulator in print_diff_chunk(). No behavior change is intended by
> > > this change.
> >
> > Well, this is not "pure code movement" per se; it is meant to be
> > refactoring that doesn't change gitweb output nor behavior.
> >
> > If I understand correctly the change is from
> >
> > read
> > format
> > accumulate
> > print
> >
> > to
> >
> > read
> > accumulate
> > format
> > print
> >
> > Isn't it?
>
> Yeah, this is what I meant :).
Minor correction: The order *does* change, in the sense that first a
chunk is read, then (in print_diff_chunk) formatted and printed,
but in print_diff_chunk() the order remains:
read line
format
put into accumulator
print
>
> >
> > As a note I would add also that process_diff_line got renamed to
> > format_diff_line,
>
> I thought I wrote that ("The latter function is renamed...")?
>
> > and its output changed to returning only
> > HTML-formatted line, which bringg it in line with other format_*
> > subroutines.
> >
>
> OK.
>
> > > Signed-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>
> >
> > I think it is a good change even without subsequent patches.
> >
> > Acked-by: Jakub Narębski <jnareb@gmail.com>
> >
>
> Thanks.
>
> > > ---
> > > gitweb/gitweb.perl | 25 ++++++++++++-------------
> > > 1 files changed, 12 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> > > index d2f75c4..cae9dfa 100755
> > > --- a/gitweb/gitweb.perl
> > > +++ b/gitweb/gitweb.perl
> > > @@ -2320,12 +2320,9 @@ sub format_cc_diff_chunk_header {
> > > }
> > >
> > > # process patch (diff) line (not to be used for diff headers),
> > > -# returning class and HTML-formatted (but not wrapped) line
> > > -sub process_diff_line {
> > > - my $line = shift;
> > > - my ($from, $to) = @_;
> > > -
> > > - my $diff_class = diff_line_class($line, $from, $to);
> > > +# returning HTML-formatted (but not wrapped) line
> > > +sub format_diff_line {
> > > + my ($line, $diff_class, $from, $to) = @_;
> > >
> > > chomp $line;
> > > $line = untabify($line);
> > > @@ -2343,7 +2340,7 @@ sub process_diff_line {
> > > $diff_classes .= " $diff_class" if ($diff_class);
> > > $line = "<div class=\"$diff_classes\">$line</div>\n";
> > >
> > > - return $diff_class, $line;
> > > + return $line;
> > > }
> > >
> > > # Generates undef or something like "_snapshot_" or "snapshot (_tbz2_ _zip_)",
> > > @@ -4934,7 +4931,7 @@ sub print_diff_lines {
> > > }
> > >
> > > sub print_diff_chunk {
> > > - my ($diff_style, $is_combined, @chunk) = @_;
> > > + my ($diff_style, $is_combined, $from, $to, @chunk) = @_;
> > > my (@ctx, @rem, @add);
> > > my $prev_class = '';
> > >
> > > @@ -4954,6 +4951,8 @@ sub print_diff_chunk {
> > > foreach my $line_info (@chunk) {
> > > my ($class, $line) = @$line_info;
> > >
> > > + $line = format_diff_line($line, $class, $from, $to);
> > > +
> > > # print chunk headers
> > > if ($class && $class eq 'chunk_header') {
> > > print $line;
> > > @@ -5107,19 +5106,19 @@ sub git_patchset_body {
> > >
> > > next PATCH if ($patch_line =~ m/^diff /);
> > >
> > > - my ($class, $line) = process_diff_line($patch_line, \%from, \%to);
> > > + my $class = diff_line_class($patch_line, \%from, \%to);
> > >
> > > if ($class eq 'chunk_header') {
> > > - print_diff_chunk($diff_style, $is_combined, @chunk);
> > > - @chunk = ( [ $class, $line ] );
> > > + print_diff_chunk($diff_style, $is_combined, \%from, \%to, @chunk);
> > > + @chunk = ( [ $class, $patch_line ] );
> > > } else {
> > > - push @chunk, [ $class, $line ];
> > > + push @chunk, [ $class, $patch_line ];
> > > }
> > > }
> > >
> > > } continue {
> > > if (@chunk) {
> > > - print_diff_chunk($diff_style, $is_combined, @chunk);
> > > + print_diff_chunk($diff_style, $is_combined, \%from, \%to, @chunk);
> > > @chunk = ();
> > > }
> > > print "</div>\n"; # class="patch"
> > > --
> > > 1.7.3.4
> > >
> >
> > Nice!
> >
^ permalink raw reply
* Re: [PATCH 5/8] gitweb: Format diff lines just before printing
From: Michał Kiedrowicz @ 2012-02-11 23:38 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git
In-Reply-To: <m37gztthrx.fsf@localhost.localdomain>
Jakub Narebski <jnareb@gmail.com> wrote:
> Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:
>
> > Now we're ready to insert highlightning to diff output.
> >
> > The call to untabify() remains in the main loop in print_diff_chunk().
> > The motivation is that it must be called before any call to esc_html()
> > (because that converts spaces to ) and to call it only once.
> >
> > This is a refactoring patch. It's not meant to change gitweb output.
>
> I'm not sure about this patch.
>
> First, in my opinion it doesn't make much sense standalone, and not
> squashed with subsequent patch.
True. I wanted to separate "preparation" from "adding new feature" but
maybe went few steps too far.
>
> Second, it makes format_diff_line an odd duck among all other format_*
> subroutines in that it post-processes HTML output, rather than
> generating HTML from data.
>
> Why "diff refinement highlighting" cannot be part of format_diff_line()?
> If it does need additional data, just pass it as additional parameters
> to this subroutine.
>
> Another solution could be to borrow idea from stalled and inactive
> committags feature, namely that parts that are HTML are to be passed
> as scalar reference (\$str), while plain text (unescaped yet) is to be
> passed as string ($str).
I'll look into it.
>
> > -# process patch (diff) line (not to be used for diff headers),
> > -# returning HTML-formatted (but not wrapped) line
> > +# wrap patch (diff) line into a <div> (not to be used for diff headers),
> > +# the line must be esc_html()'ed
> > sub format_diff_line {
>
> I just don't like this error-prone "the line must be esc_html()'ed".
>
> > +# HTML-format diff context, removed and added lines.
> > +sub format_ctx_rem_add_lines {
> > + my ($ctx, $rem, $add) = @_;
> > + my (@new_ctx, @new_rem, @new_add);
> > +
> > + @new_ctx = map { format_diff_line(esc_html($_, -nbsp=>1), 'ctx') } @$ctx;
> > + @new_rem = map { format_diff_line(esc_html($_, -nbsp=>1), 'rem') } @$rem;
> > + @new_add = map { format_diff_line(esc_html($_, -nbsp=>1), 'add') } @$add;
> > +
> > + return (\@new_ctx, \@new_rem, \@new_add);
> > +}
> > +
> > # Print context lines and then rem/add lines.
> > sub print_diff_lines {
> > my ($ctx, $rem, $add, $diff_style, $is_combined) = @_;
> >
> > + ($ctx, $rem, $add) = format_ctx_rem_add_lines($ctx, $rem, $add);
> > +
>
> Nice trick.
>
^ permalink raw reply
* Re: [RFC/PATCHv2 2/2] git-p4: initial demonstration of possible RCS keyword fixup
From: Pete Wyckoff @ 2012-02-11 23:42 UTC (permalink / raw)
To: Luke Diamand; +Cc: git, Eric Scouten
In-Reply-To: <1328829442-12550-3-git-send-email-luke@diamand.org>
luke@diamand.org wrote on Thu, 09 Feb 2012 23:17 +0000:
> This change has a go at showing a possible way to fixup RCS
> keyword handling in git-p4.
>
> It does not cope with deleted files.
> It does not have good test coverage.
> It does not solve the problem of the incorrect error messages.
> But it does at least work after a fashion, and could provide
> a starting point.
I think this has great promise. Since p4 can always be trusted
to regenerate the keywords, we should be able to scrub them off
at will.
I'm debating whether it's best just _always_ to scrub the files
affected by a diff rather than trying the patch, checking for
failure, then scrubbing and trying again.
I'll send along a bunch of test cases I wrote to play around
with this. Your case had too many moving parts for me to
understand. If there's something in there that isn't covered,
maybe you can factor it out into something small? Feel free
to merge any of my code in with a future resubmission.
Some comments in this code below:
> @@ -753,6 +753,23 @@ class P4Submit(Command, P4UserMap):
>
> return result
>
> + def patchRCSKeywords(self, file):
> + # Attempt to zap the RCS keywords in a p4 controlled file
> + p4_edit(file)
> + (handle, outFileName) = tempfile.mkstemp()
> + outFile = os.fdopen(handle, "w+")
> + inFile = open(file, "r")
> + for line in inFile.readlines():
> + line = re.sub(r'\$(Id|Header|Author|Date|DateTime|Change|File|Revision)[^$]*\$',
> + r'$\1$', line)
I added a couple test cases that show how text+k, text+ko and
everything else need to be handled differently.
> + outFile.write(line)
> + inFile.close()
> + outFile.close()
> + # Forcibly overwrite the original file
> + system("cat %s" % outFileName)
> + system(["mv", "-f", outFileName, file])
Will need a good bit of error handling in the final version. I
like the way you do it a line at a time, avoiding any issues with
gigantic files.
> @@ -964,9 +982,30 @@ class P4Submit(Command, P4UserMap):
> patchcmd = diffcmd + " | git apply "
> tryPatchCmd = patchcmd + "--check -"
> applyPatchCmd = patchcmd + "--check --apply -"
> + patch_succeeded = True
>
> if os.system(tryPatchCmd) != 0:
> + fixed_rcs_keywords = False
> + patch_succeeded = False
> print "Unfortunately applying the change failed!"
> +
> + # Patch failed, maybe it's just RCS keyword woes. Look through
> + # the patch to see if that's possible.
> + if gitConfig("git-p4.attemptRCSCleanup","--bool") == "true":
> + file = None
> + for line in read_pipe_lines(diffcmd):
> + m = re.match(r'^diff --git a/(.*)\s+b/(.*)', line)
> + if m:
> + file = m.group(1)
> + if re.match(r'.*\$(Id|Header|Author|Date|DateTime|Change|File|Revision)[^$]*\$', line):
> + self.patchRCSKeywords(file)
> + fixed_rcs_keywords = True
This is a novel approach too. Instead of just guessing that
keywords are causing the conflict, inspect the diff for context
or edited lines containing keywords.
Or we could just always scrub every file before even trying to
apply patches.
> + if fixed_rcs_keywords:
> + print "Retrying the patch with RCS keywords cleaned up"
> + if os.system(tryPatchCmd) == 0:
> + patch_succeeded = True
> +
> + if not patch_succeeded:
> print "What do you want to do?"
> response = "x"
> while response != "s" and response != "a" and response != "w":
> @@ -1588,11 +1627,11 @@ class P4Sync(Command, P4UserMap):
> if type_base in ("text", "unicode", "binary"):
> if "ko" in type_mods:
> text = ''.join(contents)
> - text = re.sub(r'\$(Id|Header):[^$]*\$', r'$\1$', text)
> + text = re.sub(r'\$(Id|Header)[^$]*\$', r'$\1$', text)
In a few spots I see you've taken the ":" out of the regex. This
will match strings like $Idiot$ that shouldn't be keyword
expanded.
Impressed.
-- Pete
^ permalink raw reply
* Re: [PATCHv2 1/4] refs: add common refname_match_patterns()
From: Junio C Hamano @ 2012-02-11 23:43 UTC (permalink / raw)
To: Tom Grennan; +Cc: pclouds, git, jasampler
In-Reply-To: <20120211193742.GD4903@tgrennan-laptop>
Tom Grennan <tmgrennan@gmail.com> writes:
> Yes, I should have stated that this emphasized containment over
> efficiency. If instead we stipulate that the caller must list exclusion
> patterns before others, this could simply be:
No.
You have to pre-parse and rearrange the pattern[] list *only once* before
matching them against dozens of refs, so instead of forcing the callers do
anything funky, you give a function that gets a pattern[] list and returns
something that can be efficiently used by the match_pattern() function,
and have the caller pass that thing, not the original pattern[] list, to
the match_pattern() function.
That is how pathspec matching side of the logic is arranged.
I keep saying that it is probably not a good idea to directly reuse the
pathspec code, but you would want to study and learn from the overall
structure of it.
^ permalink raw reply
* [PATCH] git-p4: more RCS tests
From: Pete Wyckoff @ 2012-02-11 23:44 UTC (permalink / raw)
To: Luke Diamand; +Cc: git, Eric Scouten
In-Reply-To: <20120211234248.GA16691@padd.com>
Add a few smaller test cases.
Signed-off-by: Pete Wyckoff <pw@padd.com>
---
t/t9810-git-p4-rcs.sh | 201 +++++++++++++++++++++++++++++++++++++++++++++++-
1 files changed, 196 insertions(+), 5 deletions(-)
diff --git a/t/t9810-git-p4-rcs.sh b/t/t9810-git-p4-rcs.sh
index a235913..013a951 100755
--- a/t/t9810-git-p4-rcs.sh
+++ b/t/t9810-git-p4-rcs.sh
@@ -8,6 +8,198 @@ test_expect_success 'start p4d' '
start_p4d
'
+#
+# Make one file with keyword lines at the top, and
+# enough plain text to be able to test modifications
+# far away from the keywords.
+#
+test_expect_success 'init depot' '
+ (
+ cd "$cli" &&
+ cat <<-\EOF >filek &&
+ $Id$
+ /* $Revision$ */
+ # $Change$
+ line4
+ line5
+ line6
+ line7
+ line8
+ EOF
+ cp filek fileko &&
+ sed -i "s/Revision/Revision: do not scrub me/" fileko
+ cp fileko file_text &&
+ sed -i "s/Id/Id: do not scrub me/" file_text
+ p4 add -t text+k filek &&
+ p4 submit -d "filek" &&
+ p4 add -t text+ko fileko &&
+ p4 submit -d "fileko" &&
+ p4 add -t text file_text &&
+ p4 submit -d "file_text"
+ )
+'
+
+#
+# Generate these in a function to make it easy to use single quote marks.
+#
+write_scrub_scripts() {
+ cat >"$TRASH_DIRECTORY/scrub_k.py" <<-\EOF &&
+ import re, sys
+ sys.stdout.write(re.sub(r'(?i)\$(Id|Header|Author|Date|DateTime|Change|File|Revision):[^$]*\$', r'$\1$', sys.stdin.read()))
+ EOF
+ cat >"$TRASH_DIRECTORY/scrub_ko.py" <<-\EOF
+ import re, sys
+ sys.stdout.write(re.sub(r'(?i)\$(Id|Header):[^$]*\$', r'$\1$', sys.stdin.read()))
+ EOF
+}
+
+test_expect_success 'scrub scripts' '
+ write_scrub_scripts
+'
+
+#
+# Compare $cli/file to its scrubbed version, should be different.
+# Compare scrubbed $cli/file to $git/file, should be same.
+#
+scrub_k_check() {
+ file=$1 &&
+ scrub="$TRASH_DIRECTORY/$file" &&
+ "$PYTHON_PATH" "$TRASH_DIRECTORY/scrub_k.py" <"$git/$file" >"$scrub" &&
+ ! test_cmp "$cli/$file" "$scrub" &&
+ test_cmp "$git/$file" "$scrub" &&
+ rm "$scrub"
+}
+scrub_ko_check() {
+ file=$1 &&
+ scrub="$TRASH_DIRECTORY/$file" &&
+ "$PYTHON_PATH" "$TRASH_DIRECTORY/scrub_ko.py" <"$git/$file" >"$scrub" &&
+ ! test_cmp "$cli/$file" "$scrub" &&
+ test_cmp "$git/$file" "$scrub" &&
+ rm "$scrub"
+}
+
+#
+# Modify far away from keywords. If no RCS lines show up
+# in the diff, there is no conflict.
+#
+test_expect_success 'edit far away from RCS lines' '
+ test_when_finished cleanup_git &&
+ "$GITP4" clone --dest="$git" //depot &&
+ (
+ cd "$git" &&
+ git config git-p4.skipSubmitEdit true &&
+ sed -i "s/^line7/line7 edit/" filek &&
+ git commit -m "filek line7 edit" filek &&
+ "$GITP4" submit &&
+ scrub_k_check filek
+ )
+'
+
+#
+# Modify near the keywords. This will require RCS scrubbing.
+#
+test_expect_success 'edit near RCS lines' '
+ test_when_finished cleanup_git &&
+ "$GITP4" clone --dest="$git" //depot &&
+ (
+ cd "$git" &&
+ git config git-p4.skipSubmitEdit true &&
+ git config git-p4.attemptRCSCleanup true &&
+ sed -i "s/^line4/line4 edit/" filek &&
+ git commit -m "filek line4 edit" filek &&
+ "$GITP4" submit &&
+ scrub_k_check filek
+ )
+'
+
+#
+# Modify the keywords themselves. This also will require RCS scrubbing.
+#
+test_expect_success 'edit keyword lines' '
+ test_when_finished cleanup_git &&
+ "$GITP4" clone --dest="$git" //depot &&
+ (
+ cd "$git" &&
+ git config git-p4.skipSubmitEdit true &&
+ git config git-p4.attemptRCSCleanup true &&
+ sed -i "/Revision/d" filek &&
+ git commit -m "filek remove Revision line" filek &&
+ "$GITP4" submit &&
+ scrub_k_check filek
+ )
+'
+
+#
+# Scrubbing text+ko files should not alter all keywords, just Id, Header.
+#
+test_expect_failure 'scrub ko files differently' '
+ test_when_finished cleanup_git &&
+ "$GITP4" clone --dest="$git" //depot &&
+ (
+ cd "$git" &&
+ git config git-p4.skipSubmitEdit true &&
+ git config git-p4.attemptRCSCleanup true &&
+ sed -i "s/^line4/line4 edit/" fileko &&
+ git commit -m "fileko line4 edit" fileko &&
+ "$GITP4" submit &&
+ scrub_ko_check fileko &&
+ ! scrub_k_check fileko
+ )
+'
+
+# hack; git-p4 submit should do it on its own
+test_expect_success 'cleanup after failure' '
+ (
+ cd "$cli" &&
+ p4 revert ...
+ )
+'
+
+#
+# Do not scrub anything but +k or +ko files. Sneak a change into
+# the cli file so that submit will get a conflict. Make sure that
+# scrubbing doesn't make a mess of things.
+#
+# Assumes that git-p4 exits leaving the p4 file open, with the
+# conflict-generating patch unapplied.
+#
+# This might happen only if the git repo is behind the p4 repo at
+# submit time, and there is a conflict.
+#
+test_expect_failure 'do not scrub plain text' '
+ test_when_finished cleanup_git &&
+ "$GITP4" clone --dest="$git" //depot &&
+ (
+ cd "$git" &&
+ git config git-p4.skipSubmitEdit true &&
+ git config git-p4.attemptRCSCleanup true &&
+ sed -i "s/^line4/line4 edit/" file_text &&
+ git commit -m "file_text line4 edit" file_text &&
+ (
+ cd "$cli" &&
+ p4 open file_text &&
+ sed -i "s/^line5/line5 p4 edit/" file_text &&
+ p4 submit -d "file5 p4 edit"
+ ) &&
+ ! "$GITP4" submit &&
+ (
+ # exepct something like:
+ # file_text - file(s) not opened on this client
+ # but not copious diff output
+ cd "$cli" &&
+ p4 diff file_text >wc &&
+ test_line_count = 1 wc
+ )
+ )
+'
+
+# hack; git-p4 submit should do it on its own
+test_expect_success 'cleanup after failure 2' '
+ (
+ cd "$cli" &&
+ p4 revert ...
+ )
+'
create_kw_file () {
cat <<\EOF >"$1"
/* A file
@@ -21,12 +213,12 @@ int main(int argc, const char **argv) {
EOF
}
-test_expect_success 'init depot' '
+test_expect_success 'add kwfile' '
(
cd "$cli" &&
echo file1 >file1 &&
p4 add file1 &&
- p4 submit -d "change 1" &&
+ p4 submit -d "file1" &&
create_kw_file kwfile1.c &&
p4 add kwfile1.c &&
p4 submit -d "Add rcw kw file" kwfile1.c
@@ -45,6 +237,7 @@ p4_append_to_file () {
# results in a merge conflict if we touch the RCS kw lines,
# even though the change itself would otherwise apply cleanly.
test_expect_failure 'cope with rcs keyword expansion damage' '
+ test_when_finished cleanup_git &&
"$GITP4" clone --dest="$git" //depot &&
(
cd "$git" &&
@@ -69,11 +262,9 @@ test_expect_failure 'cope with rcs keyword expansion damage' '
git commit -m "Add line in git at the top" kwfile1.c &&
"$GITP4" rebase &&
"$GITP4" submit
- ) &&
- rm -rf "$git" && mkdir "$git"
+ )
'
-
test_expect_success 'kill p4d' '
kill_p4d
'
--
1.7.9.213.g57e99
^ permalink raw reply related
* Re: [PATCH 6/8] gitweb: Highlight interesting parts of diff
From: Jakub Narebski @ 2012-02-11 23:45 UTC (permalink / raw)
To: Michał Kiedrowicz; +Cc: git
In-Reply-To: <1328865494-24415-7-git-send-email-michal.kiedrowicz@gmail.com>
Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:
> Reading diff output is sometimes very hard, even if it's colored,
> especially if lines differ only in few characters. This is often true
> when a commit fixes a typo or renames some variables or functions.
>
This is certainly nice feature to have. I think most tools that
implement side-by-side diff implement this too.
> This commit teaches gitweb to highlight characters that are different
> between old and new line. This should work in the similar manner as in
> Trac or GitHub.
>
Doe you have URLs with good examples of such diff refinement
highlighting (GNU Emacs ediff/emerge terminology) / highlighting
differing segments (diff-highlight terminology)?
> The code that compares lines is based on
> contrib/diff-highlight/diff-highlight, except that it works with
> multiline changes too. It also won't highlight lines that are
> completely different because that would only make the output unreadable.
>
So what does it look like? From the contrib/diff-highlight/README I
guess that it finds common prefix and common suffix, and highlights
the rest, which includes change. It doesn't implement LCS / diff
algorithm like e.g. tkdiff does for its diff refinement highlighting,
isn't it?
By completly different you mean that they do not have common prefix or
common suffix (at least one of them), isn't it?
> Combined diffs are not supported but a following commit will change it.
>
O.K.
> Signed-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>
> ---
> gitweb/gitweb.perl | 82 ++++++++++++++++++++++++++++++++++++++++++++++++---
> 1 files changed, 77 insertions(+), 5 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index db61553..1a5b454 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -2322,7 +2322,7 @@ sub format_cc_diff_chunk_header {
> # wrap patch (diff) line into a <div> (not to be used for diff headers),
> # the line must be esc_html()'ed
> sub format_diff_line {
> - my ($line, $diff_class, $from, $to) = @_;
> + my ($line, $diff_class) = @_;
Why that change?
>
> my $diff_classes = "diff";
> $diff_classes .= " $diff_class" if ($diff_class);
> @@ -4923,14 +4923,85 @@ sub print_inline_diff_lines {
> print foreach (@$add);
> }
>
> +# Highlight characters from $prefix to $suffix and escape HTML.
> +# $str is a reference to the array of characters.
> +sub esc_html_mark_range {
> + my ($str, $prefix, $suffix) = @_;
> +
> + # Don't generate empty <span> element.
> + if ($prefix == $suffix + 1) {
> + return esc_html(join('', @$str), -nbsp=>1);
> + }
> +
> + my $before = join('', @{$str}[0..($prefix - 1)]);
> + my $marked = join('', @{$str}[$prefix..$suffix]);
> + my $after = join('', @{$str}[($suffix + 1)..$#{$str}]);
Eeeeeek! First you split into letters, in caller at that, then join?
Why not pass striung ($str suggests string not array of characters),
and use substr instead?
[Please disregard this and the next paragraph at first reading]
> +
> + return esc_html($before, -nbsp=>1) .
> + $cgi->span({-class=>'marked'}, esc_html($marked, -nbsp=>1)) .
> + esc_html($after,-nbsp=>1);
> +}
Anyway I have send to git mailing list a patch series, which in one of
patches adds esc_html_match_hl($str, $regexp) to highlight matches in
a string. Your esc_html_mark_range(), after a generalization, could
be used as underlying "engine".
Something like this, perhaps (untested):
# Highlight selected fragments of string, using given CSS class,
# and escape HTML. It is assumed that fragments do not overlap.
# Regions are passed as list of pairs (array references).
sub esc_html_hl {
my ($str, $css_class, @sel) = @_;
return esc_html($str) unless @sel;
my $out = '';
my $pos = 0;
for my $s (@sel) {
$out .= esc_html(substr($str, $pos, $s->[0] - $pos))
if ($s->[0] - $pos > 0);
$out .= $cgi->span({-class => $css_class},
esc_html(substr($str, $s->[0], $s->[1] - $s->[0])));
$pos = $m->[1];
}
$out .= esc_html(substr($str, $pos))
if ($pos < length($str));
return $out;
}
> +
> +# Format removed and added line, mark changed part and HTML-format them.
You should probably ad here that this code is taken from
diff-highlight in contrib area, isn't it?
> +sub format_rem_add_line {
> + my ($rem, $add) = @_;
> + my @r = split(//, $rem);
> + my @a = split(//, $add);
> + my ($esc_rem, $esc_add);
> + my ($prefix, $suffix_rem, $suffix_add) = (1, $#r, $#a);
It is not as much $prefix, as $prefix_len, isn't it? Shouldn't
$prefix / $prefix_len start from 0, not from 1?
> + my ($prefix_is_space, $suffix_is_space) = (1, 1);
> +
> + while ($prefix < @r && $prefix < @a) {
> + last if ($r[$prefix] ne $a[$prefix]);
> +
> + $prefix_is_space = 0 if ($r[$prefix] !~ /\s/);
> + $prefix++;
> + }
Ah, I see that it is easier to find common prefix by treating string
as array of characters.
Though I wonder if it wouldn't be easier to use trick of XOR-ing two
strings (see "Bitwise String Operators" in perlop(1)):
my $xor = "$rem" ^ "$add";
and finding starting sequence of "\0", which denote common prefix.
Though this and the following is a nice implementation of
algorithm... as it would be implemented in C. Nevermind, it might be
good enough...
> +
> + while ($suffix_rem >= $prefix && $suffix_add >= $prefix) {
> + last if ($r[$suffix_rem] ne $a[$suffix_add]);
> +
> + $suffix_is_space = 0 if ($r[$suffix_rem] !~ /\s/);
> + $suffix_rem--;
> + $suffix_add--;
> + }
BTW., perhaps using single negative $suffix_len instead of separate
$suffix_rem_pos and $suffix_add_pos would make code simpler and easier
to understand?
> +
> + # Mark lines that are different from each other, but have some common
> + # part that isn't whitespace. If lines are completely different, don't
> + # mark them because that would make output unreadable, especially if
> + # diff consists of multiple lines.
> + if (($prefix == 1 && $suffix_rem == $#r && $suffix_add == $#a)
> + || ($prefix_is_space && $suffix_is_space)) {
Micronit about style: in gitweb we put boolean operator at the end of
continued line, not at beginning of next one.
So this would be:
+ if (($prefix == 1 && $suffix_rem == $#r && $suffix_add == $#a) ||
+ ($prefix_is_space && $suffix_is_space)) {
> + $esc_rem = esc_html($rem);
> + $esc_add = esc_html($add);
> + } else {
> + $esc_rem = esc_html_mark_range(\@r, $prefix, $suffix_rem);
> + $esc_add = esc_html_mark_range(\@a, $prefix, $suffix_add);
> + }
> +
> + return format_diff_line($esc_rem, 'rem'),
> + format_diff_line($esc_add, 'add');
Please use spaces to align.
> +}
> +
> # HTML-format diff context, removed and added lines.
> sub format_ctx_rem_add_lines {
> - my ($ctx, $rem, $add) = @_;
> + my ($ctx, $rem, $add, $is_combined) = @_;
> my (@new_ctx, @new_rem, @new_add);
> + my $num_add_lines = @$add;
Why is this temporary variable needed? If you are not sure that ==
operator enforces scalar context on both arguments you can always
write
scalar @$add == scalar @$rem
> +
> + if (!$is_combined && $num_add_lines > 0 && $num_add_lines == @$rem) {
> + for (my $i = 0; $i < $num_add_lines; $i++) {
> + my ($line_rem, $line_add) = format_rem_add_line(
> + $rem->[$i], $add->[$i]);
> + push @new_rem, $line_rem;
> + push @new_add, $line_add;
The original contrib/diff-highlight works only for single changed line
(single removed and single added). You make this code work also for
the case where number of aded lines is equal to the number of removed
lines, and assume that subsequent changed lines in preimage
correcponds to subsequent changed lines in postimage, which is not
always true:
-foo
-bar
+baz
+fooo
This is not described in commit message, I think.
> + }
> + } else {
> + @new_rem = map { format_diff_line(esc_html($_, -nbsp=>1), 'rem') } @$rem;
> + @new_add = map { format_diff_line(esc_html($_, -nbsp=>1), 'add') } @$add;
> + }
>
> @new_ctx = map { format_diff_line(esc_html($_, -nbsp=>1), 'ctx') } @$ctx;
> - @new_rem = map { format_diff_line(esc_html($_, -nbsp=>1), 'rem') } @$rem;
> - @new_add = map { format_diff_line(esc_html($_, -nbsp=>1), 'add') } @$add;
>
> return (\@new_ctx, \@new_rem, \@new_add);
> }
> @@ -4939,7 +5010,8 @@ sub format_ctx_rem_add_lines {
> sub print_diff_lines {
> my ($ctx, $rem, $add, $diff_style, $is_combined) = @_;
>
> - ($ctx, $rem, $add) = format_ctx_rem_add_lines($ctx, $rem, $add);
> + ($ctx, $rem, $add) = format_ctx_rem_add_lines($ctx, $rem, $add,
> + $is_combined);
O.K., now the code depends on $is_combined
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH 8/8] gitweb: Highlight combined diffs
From: Jakub Narebski @ 2012-02-12 0:03 UTC (permalink / raw)
To: Michał Kiedrowicz; +Cc: git
In-Reply-To: <1328865494-24415-9-git-send-email-michal.kiedrowicz@gmail.com>
Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:
> The highlightning of combined diffs is currently disabled. This is
> because output from a combined diff is much harder to highlight because
> it's not obvious which removed and added lines should be compared.
>
> Moreover, code that compares added and removed lines doesn't care about
> combined diffs. It only skips first +/- character, treating second +/-
> as a line content.
>
> Let's start with a simple case: only highlight changes that come from
> one parent, i.e. when every removed line has a corresponding added line
> for the same parent. This way the highlightning cannot get wrong. For
> example, following diffs would be highlighted:
>
> - removed line for first parent
> + added line for first parent
> context line
> -removed line for second parent
> +added line for second parent
>
> or
>
> - removed line for first parent
> -removed line for second parent
> + added line for first parent
> +added line for second parent
>
> but following output will not:
>
> - removed line for first parent
> -removed line for second parent
> +added line for second parent
> ++added line for both parents
>
That is a very reasonable approach.
> Further changes may introduce more intelligent approach that better
> handles combined diffs.
>
> Signed-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>
> ---
> gitweb/gitweb.perl | 40 +++++++++++++++++++++++++++++++++++++---
> 1 files changed, 37 insertions(+), 3 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 1a5b454..2b6cb9e 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -4944,13 +4944,16 @@ sub esc_html_mark_range {
>
> # Format removed and added line, mark changed part and HTML-format them.
> sub format_rem_add_line {
> - my ($rem, $add) = @_;
> + my ($rem, $add, $is_combined) = @_;
> my @r = split(//, $rem);
> my @a = split(//, $add);
> my ($esc_rem, $esc_add);
> my ($prefix, $suffix_rem, $suffix_add) = (1, $#r, $#a);
> my ($prefix_is_space, $suffix_is_space) = (1, 1);
>
> + # In combined diff we must ignore two +/- characters.
> + $prefix = 2 if ($is_combined);
> +
Errr... actually number of prefix is equalt to number of parents, so
it might be in case of octopus merge more than 2.
> while ($prefix < @r && $prefix < @a) {
> last if ($r[$prefix] ne $a[$prefix]);
>
> @@ -4988,11 +4991,42 @@ sub format_ctx_rem_add_lines {
> my ($ctx, $rem, $add, $is_combined) = @_;
> my (@new_ctx, @new_rem, @new_add);
> my $num_add_lines = @$add;
> + my $can_highlight;
> +
> + # Highlight if every removed line has a corresponding added line.
> + if ($num_add_lines > 0 && $num_add_lines == @$rem) {
> + $can_highlight = 1;
> +
> + # Highlight lines in combined diff only if the chunk contains
> + # diff between the same version, e.g.
> + #
> + # - a
> + # - b
> + # + c
> + # + d
> + #
> + # Otherwise the highlightling would be confusing.
> + if ($is_combined) {
> + for (my $i = 0; $i < $num_add_lines; $i++) {
> + my $prefix_rem = substr($rem->[$i], 0, 2);
> + my $prefix_add = substr($add->[$i], 0, 2);
> +
> + $prefix_rem =~ s/-/+/g;
> +
> + if ($prefix_rem ne $prefix_add) {
> + $can_highlight = 0;
> + last;
Nb. this assumes that we cannot refine and highlight something like
this:
# - a
# - b
# + c
# ++ d
But perhaps this is better left for future improvemnt.
> + }
> + }
> + }
> + } else {
> + $can_highlight = 0;
> + }
This 'else' would be not necessary if $can_highlight was initialized
to 0.
>
> - if (!$is_combined && $num_add_lines > 0 && $num_add_lines == @$rem) {
> + if ($can_highlight) {
> for (my $i = 0; $i < $num_add_lines; $i++) {
> my ($line_rem, $line_add) = format_rem_add_line(
> - $rem->[$i], $add->[$i]);
> + $rem->[$i], $add->[$i], $is_combined);
> push @new_rem, $line_rem;
> push @new_add, $line_add;
O.K.
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH] Makefile: Change the default compiler from "gcc" to "cc"
From: Ævar Arnfjörð Bjarmason @ 2012-02-12 0:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Linus Torvalds
In-Reply-To: <7vr4zyiyih.fsf@alter.siamese.dyndns.org>
On Wed, Dec 21, 2011 at 01:01, Junio C Hamano <gitster@pobox.com> wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> However unlike Linux Git is written in ANSI C and supports a multitude
>> of compilers, including Clang, Sun Studio, xlc etc. On my Linux box
>> "cc" is a symlink to clang, and on a Solaris box I have access to "cc"
>> is Sun Studio's CC.
>>
>> Both of these are perfectly capable of compiling Git, and it's
>> annoying to have to specify CC=cc on the command-line when compiling
>> Git when that's the default behavior of most other portable programs.
>
> Would this affect folks in BSD land negatively?
I see my mail back it December didn't include a reply to that for some
reason.
Anyway like Linus said probably not, also the BSD's I use have cc as a
symlink to whatever the default compiler is, which is usually gcc.
I've recently been hacking git on Solaris again and keep getting bit
by this, I'd like to propose it for inclusion again, the patch still
applies.
^ permalink raw reply
* Re: [PATCH] Remove Git's support for smoke testing
From: Ævar Arnfjörð Bjarmason @ 2012-02-12 0:09 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason
In-Reply-To: <1324660098-26666-1-git-send-email-avarab@gmail.com>
On Fri, Dec 23, 2011 at 18:08, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> I'm no longer running the Git smoke testing service at
> smoke.git.nix.is due to Smolder being a fragile piece of software not
> having time to follow through on making it easy for third parties to
> run and submit their own smoke tests.
Junio, could you please apply this? The current release's t/README
file is pointing to a service I'm not running anymore.
^ permalink raw reply
* Re: [PATCH 7/8] gitweb: Use different colors to present marked changes
From: Jakub Narebski @ 2012-02-12 0:11 UTC (permalink / raw)
To: Michał Kiedrowicz; +Cc: git
In-Reply-To: <1328865494-24415-8-git-send-email-michal.kiedrowicz@gmail.com>
Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:
> This makes use of the highlight diff feature.
>
> Signed-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>
> ---
> I decided to split mechanism (generate HTML page with <span> elements that
> mark interesting fragments of diff output) from politics (use these
> particular colors for this <span> elements), but otherwise this commit
> may be squashed with the previous one. These colors work for me but if
> someone comes out with better ones, I'd be happy.
I think it would be better squashed with previous patch, otherwise it
is a bit not visible change...
> gitweb/static/gitweb.css | 8 ++++++++
> 1 files changed, 8 insertions(+), 0 deletions(-)
>
> diff --git a/gitweb/static/gitweb.css b/gitweb/static/gitweb.css
> index c7827e8..4f87d16 100644
> --- a/gitweb/static/gitweb.css
> +++ b/gitweb/static/gitweb.css
> @@ -438,6 +438,10 @@ div.diff.add {
> color: #008800;
> }
>
> +div.diff.add span.marked {
> + background-color: #77ff77;
> +}
> +
> div.diff.from_file a.path,
> div.diff.from_file {
> color: #aa0000;
> @@ -447,6 +451,10 @@ div.diff.rem {
> color: #cc0000;
> }
>
> +div.diff.rem span.marked {
> + background-color: #ff7777;
> +}
> +
> div.diff.chunk_header a,
> div.diff.chunk_header {
> color: #990099;
> --
I'd have to see those colors in use. BTW what colors other
highlighting diff GUIs use?
--
Jakub Narębski
^ permalink raw reply
* [PATCH v2 1/2] git-svn: remove redundant porcelain option to rev-list
From: Ævar Arnfjörð Bjarmason @ 2012-02-12 0:23 UTC (permalink / raw)
To: git; +Cc: Eric Wong, Jonathan Nieder,
Ævar Arnfjörð Bjarmason
In-Reply-To: <CACBZZX5cwZ4Xz3-C8B3v4eEmyO0B-JiohfRATu1UhxzST0ar5w@mail.gmail.com>
Change an invocation of git-rev-list(1) to not use --no-color,
git-rev-list(1) will always ignore that option and the --color option,
so there's no need to pass it.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
git-svn.perl | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/git-svn.perl b/git-svn.perl
index eeb83d3..712eeeb 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -3920,7 +3920,7 @@ sub rebuild {
my ($base_rev, $head) = ($partial ? $self->rev_map_max_norebuild(1) :
(undef, undef));
my ($log, $ctx) =
- command_output_pipe(qw/rev-list --pretty=raw --no-color --reverse/,
+ command_output_pipe(qw/rev-list --pretty=raw --reverse/,
($head ? "$head.." : "") . $self->refname,
'--');
my $metadata_url = $self->metadata_url;
--
1.7.9
^ permalink raw reply related
* [PATCH v2 2/2] git-svn: un-break "git svn rebase" when log.abbrevCommit=true
From: Ævar Arnfjörð Bjarmason @ 2012-02-12 0:23 UTC (permalink / raw)
To: git; +Cc: Eric Wong, Jonathan Nieder,
Ævar Arnfjörð Bjarmason
In-Reply-To: <1329006186-21346-1-git-send-email-avarab@gmail.com>
Change git-svn to use git-rev-list(1) instead of git-log(1) since the
latter is porcelain that'll cause "git svn rebase" to fail completely
if log.abbrevCommit is set to true in the configuration.
Without this patch the code will fail to parse a SHA1, and then just
spew a bunch of "Use of uninitialized value $hash in string eq"
warnings at "if ($c && $c eq $hash) { ..." and never do anything
useful.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Helped-by: Jonathan Nieder <jrnieder@gmail.com>
---
git-svn.perl | 3 +--
1 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/git-svn.perl b/git-svn.perl
index 712eeeb..bebe38b 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -1878,8 +1878,7 @@ sub cmt_sha2rev_batch {
sub working_head_info {
my ($head, $refs) = @_;
- my @args = qw/log --no-color --no-decorate --first-parent
- --pretty=medium/;
+ my @args = qw/rev-list --first-parent --pretty=medium/;
my ($fh, $ctx) = command_output_pipe(@args, $head);
my $hash;
my %max;
--
1.7.9
^ permalink raw reply related
* Re: [PATCH v2 2/2] git-svn: un-break "git svn rebase" when log.abbrevCommit=true
From: Jonathan Nieder @ 2012-02-12 0:31 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: git, Eric Wong, Junio C Hamano
In-Reply-To: <1329006186-21346-2-git-send-email-avarab@gmail.com>
Ævar Arnfjörð Bjarmason wrote:
> Change git-svn to use git-rev-list(1) instead of git-log(1) since the
> latter is porcelain that'll cause "git svn rebase" to fail completely
> if log.abbrevCommit is set to true in the configuration.
[...]
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -1878,8 +1878,7 @@ sub cmt_sha2rev_batch {
>
> sub working_head_info {
> my ($head, $refs) = @_;
> - my @args = qw/log --no-color --no-decorate --first-parent
> - --pretty=medium/;
> + my @args = qw/rev-list --first-parent --pretty=medium/;
Thanks! The other caller to "git log" in this script uses
--pretty=raw and should be safe.
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
^ permalink raw reply
* Re: [PATCH 12/10] support pager.* for external commands
From: Ævar Arnfjörð Bjarmason @ 2012-02-12 0:46 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, Steffen Daode Nurpmeso, Ingo Brückl
In-Reply-To: <20110818220132.GB7799@sigill.intra.peff.net>
On Fri, Aug 19, 2011 at 00:01, Jeff King <peff@peff.net> wrote:
> +test_expect_success TTY 'command-specific pager works for external commands' '
> + sane_unset PAGER GIT_PAGER &&
> + echo "foo:initial" >expect &&
> + >actual &&
> + test_config pager.external "sed s/^/foo:/ >actual" &&
> + test_terminal git --exec-path="`pwd`" external log --format=%s -1 &&
> + test_cmp expect actual
For reasons that I haven't looked into using sed like that breaks
under /usr/bin/ksh on Solaris. Just using:
sed -e \"s/^/foo:/\"
Instead fixes it, it's not broken with /usr/xpg4/bin/sh, so it's some
ksh peculiarity.
The error it gives is:
sed s/^/foo:/ >actual: Not found
Indicating that for some reason it's considering that whole "sed
s/^/foo:/ >actual" string to be a single command.
^ permalink raw reply
* Re: 1.7.9, libcharset missing from EXTLIBS
From: Дилян Палаузов @ 2012-02-12 0:55 UTC (permalink / raw)
To: git
In-Reply-To: <7vd39mph9x.fsf@alter.siamese.dyndns.org>
[-- Attachment #1: Type: text/plain, Size: 2983 bytes --]
Hello,
please consider the patch below as fix for the problem. It checks if
locale_charset is not in libiconv, but in libcharset and in this case
appends -lcharset to EXTLIBS.
Със здраве
Дилян
diff -u git-1.7.9.orig/config.mak.in git-1.7.9/config.mak.in
--- git-1.7.9.orig/config.mak.in 2012-01-27 20:51:04.000000000 +0000
+++ git-1.7.9/config.mak.in 2012-02-12 00:52:41.457968080 +0000
@@ -74,3 +74,4 @@
NO_PTHREADS=@NO_PTHREADS@
PTHREAD_CFLAGS=@PTHREAD_CFLAGS@
PTHREAD_LIBS=@PTHREAD_LIBS@
+LINK_CHARSET=@LINK_CHARSET@
diff -u git-1.7.9.orig/configure.ac git-1.7.9/configure.ac
--- git-1.7.9.orig/configure.ac 2012-01-27 20:51:04.000000000 +0000
+++ git-1.7.9/configure.ac 2012-02-12 00:44:29.222967868 +0000
@@ -836,6 +836,18 @@
[HAVE_LIBCHARSET_H=YesPlease],
[HAVE_LIBCHARSET_H=])
AC_SUBST(HAVE_LIBCHARSET_H)
+# Define LINK_LIBCHARSET if libiconv does not export the locale_charset
symbol
+# and liblibcharset does
+LINK_CHARSET=
+AC_CHECK_LIB([iconv], [locale_charset],
+ [],
+ [AC_CHECK_LIB([charset], [locale_charset],
+ [LINK_CHARSET=Yes])
+ ]
+)
+AC_SUBST(LINK_CHARSET)
+
+
#
# Define NO_STRCASESTR if you don't have strcasestr.
GIT_CHECK_FUNC(strcasestr,
diff -u git-1.7.9.orig/Makefile git-1.7.9/Makefile
--- git-1.7.9.orig/Makefile 2012-01-27 20:51:04.000000000 +0000
+++ git-1.7.9/Makefile 2012-02-12 00:35:23.982967555 +0000
@@ -1692,6 +1692,9 @@
ifdef HAVE_LIBCHARSET_H
BASIC_CFLAGS += -DHAVE_LIBCHARSET_H
+ifdef LINK_CHARSET
+ EXTLIBS += -lcharset
+endif
endif
ifdef HAVE_DEV_TTY
On 10.02.2012 21:25, Junio C Hamano wrote:
> Junio C Hamano<gitster@pobox.com> writes:
>
>> Дилян Палаузов<dilyan.palauzov@aegee.org> writes:
>>
>>>>> What I am wondering is there are systems that need to include the header,
>>>>> but locale_charset() does not live in /lib/libcharset.a, in which case we
>>>>> cannot make HAVE_LIBCHARSET_H imply use of -lcharset.
>>>
>>> I do not understand this. If you want to use a function from
>>> libcharset, you have to use both #include<libcharset.h> and
>>> -lcharset.
>>
>> You are mistaken.
>>
>> The only constraint is that you have to "#include<libcharset.h>" and need
>> to link with the library that has locale_charset() defined.
>
> I think the follow-ups in this thread already demonstrated why it is an
> insufficient solution to make HAVE_LIBCHARSET_H imply -lcharset.
>
> We would instead need:
>
> ifeq ($(uname_S),MyHomeBrewLinux)
> HAVE_LIBCHARSET_H = YesPlease
> EXTLIBS += -lcharset
> endif
>
> or
>
> # Define NEEDS_CHARSETLIB if you use HAVE_LIBCHARSET_H and
> # need to link with -lcharset
> NEEDS_CHARSETLIB =
>
> ifeq ($(uname_S),MyHomeBrewLinux)
> HAVE_LIBCHARSET_H = YesPlease
> NEEDS_CHARSETLIB = YesPlease
> endif
>
> ifdef NEEDS_CHARSETLIB
> EXTLIBS += -lcharset
> endif
>
> or something like that, I guess.
>
[-- Attachment #2: dilyan_palauzov.vcf --]
[-- Type: text/x-vcard, Size: 381 bytes --]
begin:vcard
fn;quoted-printable:=D0=94=D0=B8=D0=BB=D1=8F=D0=BD =D0=9F=D0=B0=D0=BB=D0=B0=D1=83=D0=B7=D0=BE=
=D0=B2
n;quoted-printable;quoted-printable:=D0=9F=D0=B0=D0=BB=D0=B0=D1=83=D0=B7=D0=BE=D0=B2;=D0=94=D0=B8=D0=BB=D1=8F=D0=BD
email;internet:dilyan.palauzov@aegee.org
tel;home:+49-721-94193270
tel;cell:+49-162-4091172
note:sip:8372@aegee.org
version:2.1
end:vcard
^ permalink raw reply
* Re: 1.7.9, libcharset missing from EXTLIBS
From: Ævar Arnfjörð Bjarmason @ 2012-02-12 1:03 UTC (permalink / raw)
To: Дилян Палаузов
Cc: git
In-Reply-To: <4F370DE5.70400@aegee.org>
2012/2/12 Дилян Палаузов <dilyan.palauzov@aegee.org>:
This looks good to me, could you please re-submit it as described in
Documentation/SubmittingPatches? I.e. with a signed-off-by etc.
^ permalink raw reply
* [PATCH] t: use sane_unset instead of unset
From: Ævar Arnfjörð Bjarmason @ 2012-02-12 1:05 UTC (permalink / raw)
To: git; +Cc: Ævar Arnfjörð Bjarmason
Change several tests to use the sane_unset function introduced in
v1.7.3.1-35-g00648ba instead of the built-in unset function.
This fixes a failure I was having on t9130-git-svn-authors-file.sh on
Solaris, and prevents several other issues from occurring.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
t/t4150-am.sh | 2 +-
t/t5560-http-backend-noserver.sh | 6 +++---
t/t6032-merge-large-rename.sh | 2 +-
t/t9130-git-svn-authors-file.sh | 4 ++--
t/t9200-git-cvsexportcommit.sh | 2 +-
t/t9808-git-p4-chdir.sh | 4 ++--
6 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/t/t4150-am.sh b/t/t4150-am.sh
index 8807b60..f1b60b8 100755
--- a/t/t4150-am.sh
+++ b/t/t4150-am.sh
@@ -136,7 +136,7 @@ test_expect_success setup '
git format-patch -M --stdout lorem^ >rename-add.patch &&
# reset time
- unset test_tick &&
+ sane_unset test_tick &&
test_tick
'
diff --git a/t/t5560-http-backend-noserver.sh b/t/t5560-http-backend-noserver.sh
index 0ad7ce0..ef98d95 100755
--- a/t/t5560-http-backend-noserver.sh
+++ b/t/t5560-http-backend-noserver.sh
@@ -17,7 +17,7 @@ run_backend() {
GET() {
REQUEST_METHOD="GET" && export REQUEST_METHOD &&
run_backend "/repo.git/$1" &&
- unset REQUEST_METHOD &&
+ sane_unset REQUEST_METHOD &&
if ! grep "Status" act.out >act
then
printf "Status: 200 OK\r\n" >act
@@ -30,8 +30,8 @@ POST() {
REQUEST_METHOD="POST" && export REQUEST_METHOD &&
CONTENT_TYPE="application/x-$1-request" && export CONTENT_TYPE &&
run_backend "/repo.git/$1" "$2" &&
- unset REQUEST_METHOD &&
- unset CONTENT_TYPE &&
+ sane_unset REQUEST_METHOD &&
+ sane_unset CONTENT_TYPE &&
if ! grep "Status" act.out >act
then
printf "Status: 200 OK\r\n" >act
diff --git a/t/t6032-merge-large-rename.sh b/t/t6032-merge-large-rename.sh
index fdb6c25..94f010b 100755
--- a/t/t6032-merge-large-rename.sh
+++ b/t/t6032-merge-large-rename.sh
@@ -95,7 +95,7 @@ test_expect_success 'setup large simple rename' '
'
test_expect_success 'massive simple rename does not spam added files' '
- unset GIT_MERGE_VERBOSITY &&
+ sane_unset GIT_MERGE_VERBOSITY &&
git merge --no-stat simple-rename | grep -v Removing >output &&
test 5 -gt "$(wc -l < output)"
'
diff --git a/t/t9130-git-svn-authors-file.sh b/t/t9130-git-svn-authors-file.sh
index b324c49..c3443ce 100755
--- a/t/t9130-git-svn-authors-file.sh
+++ b/t/t9130-git-svn-authors-file.sh
@@ -96,8 +96,8 @@ test_expect_success 'fresh clone with svn.authors-file in config' '
rm -r "$GIT_DIR" &&
test x = x"$(git config svn.authorsfile)" &&
test_config="$HOME"/.gitconfig &&
- unset GIT_DIR &&
- unset GIT_CONFIG &&
+ sane_unset GIT_DIR &&
+ sane_unset GIT_CONFIG &&
git config --global \
svn.authorsfile "$HOME"/svn-authors &&
test x"$HOME"/svn-authors = x"$(git config svn.authorsfile)" &&
diff --git a/t/t9200-git-cvsexportcommit.sh b/t/t9200-git-cvsexportcommit.sh
index 518358a..b59be9a 100755
--- a/t/t9200-git-cvsexportcommit.sh
+++ b/t/t9200-git-cvsexportcommit.sh
@@ -321,7 +321,7 @@ test_expect_success 'use the same checkout for Git and CVS' '
(mkdir shared &&
cd shared &&
- unset GIT_DIR &&
+ sane_unset GIT_DIR &&
cvs co . &&
git init &&
git add " space" &&
diff --git a/t/t9808-git-p4-chdir.sh b/t/t9808-git-p4-chdir.sh
index eb8cc95..f002283 100755
--- a/t/t9808-git-p4-chdir.sh
+++ b/t/t9808-git-p4-chdir.sh
@@ -25,7 +25,7 @@ test_expect_success 'P4CONFIG and absolute dir clone' '
test_when_finished cleanup_git &&
(
P4CONFIG=p4config && export P4CONFIG &&
- unset P4PORT P4CLIENT &&
+ sane_unset P4PORT P4CLIENT &&
"$GITP4" clone --verbose --dest="$git" //depot
)
'
@@ -37,7 +37,7 @@ test_expect_success 'P4CONFIG and relative dir clone' '
test_when_finished cleanup_git &&
(
P4CONFIG=p4config && export P4CONFIG &&
- unset P4PORT P4CLIENT &&
+ sane_unset P4PORT P4CLIENT &&
"$GITP4" clone --verbose --dest="git" //depot
)
'
--
1.7.9
^ permalink raw reply related
* Re: [PATCH] column: Fix an incorrect parse of the 'nodense' option token
From: Nguyen Thai Ngoc Duy @ 2012-02-12 3:37 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Junio C Hamano, GIT Mailing-list
In-Reply-To: <4F36B64D.4030000@ramsay1.demon.co.uk>
On Sun, Feb 12, 2012 at 1:41 AM, Ramsay Jones
<ramsay@ramsay1.demon.co.uk> wrote:
>
> The parse_option() function always saw the 'nodense' token as
> 'dense', since it stripped the 'no' off the input argument string
> earlier when checking for the presence of the 'color' token.
>
> In order to fix the parse, we use local variables (within the loop)
> initialised to the function input arguments instead, which allows
> each token check to be independent.
>
> Also, add some 'nodense' tests to t/t9002-column.sh.
>
> Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
Acked-by: Nguyen Thai Ngoc Duy <pclouds@gmail.com>
> Note that the code supports the 'nocolor' token, but it is not documented
> in config.txt.
>
> If I understand correctly (and it's quite possible that I don't!), the
> 'nodense' token has the same meaning as the absence of the 'dense' token
> (and I assume a similar comment applies to the '[no]color' tokens). If that
> is the case, then maybe a better solution would be to simply remove the
> 'nodense' and 'nocolor' tokens. I will leave it to you to decide which
> solution is more appropriate.
It's about overriding config. If you set "dense" by default in
column.ui but do not want it in this particular run, you can say
--column=nodense.
The [no]color is for plumbing only. If a command produces colored
output, "color" is required to calculate text length correctly.
Overriding it with "nocolor" would break the layout badly so it's no
use there. It does not make sense (to me) for users to put "color" in
column.ui. Which is why it's not mentioned in document.
--
Duy
^ permalink raw reply
* Re: [PATCH v2 1/2] git-svn: remove redundant porcelain option to rev-list
From: Eric Wong @ 2012-02-12 6:49 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: git, Jonathan Nieder
In-Reply-To: <1329006186-21346-1-git-send-email-avarab@gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> Change an invocation of git-rev-list(1) to not use --no-color,
> git-rev-list(1) will always ignore that option and the --color option,
> so there's no need to pass it.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Acked-by: Eric Wong <normalperson@yhbt.net>
Thanks, will push this series.
^ permalink raw reply
* Re: [PATCH 1/2] git-svn.perl: perform deletions before anything else
From: Eric Wong @ 2012-02-12 7:03 UTC (permalink / raw)
To: Steven Walter; +Cc: gitster, git, Steven Walter
In-Reply-To: <1328820742-4795-2-git-send-email-stevenrwalter@gmail.com>
Steven Walter <stevenrwalter@gmail.com> wrote:
> Signed-off-by: Steven Walter <stevenrwalter@gmail.com>
Thanks, shall I fixup 2/2 and assume you meant to Sign-off on that, too?
^ permalink raw reply
* Re: [PATCH] git-svn: Fix time zone in --localtime
From: Eric Wong @ 2012-02-12 8:15 UTC (permalink / raw)
To: Wei-Yin Chen (陳威尹); +Cc: git, Pete Harlan, gitster
In-Reply-To: <4EEEF199.4000404@gmail.com>
"\"Wei-Yin Chen (陳威尹)\"" <chen.weiyin@gmail.com> wrote:
> Use numerical form of time zone to replace alphabetic time zone
> abbreviation generated by "%Z". "%Z" is not portable and contain
> ambiguity for many areas. For example, CST could be "Central
> Standard Time" (GMT-0600) and "China Standard Time" (GMT+0800).
> Alphabetic time zone abbreviation is meant for human readability,
> not for specifying a time zone for machines.
>
> Failed case can be illustrated like this in linux shell:
> > echo $TZ
> Asia/Taipei
> > date +%Z
> CST
> > env TZ=`date +%Z` date
> Mon Dec 19 06:03:04 CST 2011
> > date
> Mon Dec 19 14:03:04 CST 2011
>
> Signed-off-by: Wei-Yin Chen (陳威尹) <chen.weiyin@gmail.com>
> ---
Hi, sorry for the late reply, this got lost in the shuffle.
This issue seems reasonable, but did you test this change at all?
> sub format_svn_date {
> - # some systmes don't handle or mishandle %z, so be creative.
> my $t = shift || time;
> - my $gm = timelocal(gmtime($t));
> - my $sign = qw( + + - )[ $t <=> $gm ];
> - my $gmoff = sprintf("%s%02d%02d", $sign, (gmtime(abs($t - $gm)))[2,1]);
> + my $gmoff = get_tz($t);
> return strftime("%Y-%m-%d %H:%M:%S $gmoff (%a, %d %b %Y)", localtime($t));
> }
I needed the following additional change to get things working:
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -6111,7 +6111,7 @@ sub run_pager {
sub format_svn_date {
my $t = shift || time;
- my $gmoff = get_tz($t);
+ my $gmoff = Git::SVN::get_tz($t);
return strftime("%Y-%m-%d %H:%M:%S $gmoff (%a, %d %b %Y)", localtime($t));
}
If there are no objections, I'll squash this and push for Junio
tomorrow.
^ permalink raw reply
* Re: [PATCH 2/3] help.c: make term_columns() cached and export it
From: Junio C Hamano @ 2012-02-12 9:40 UTC (permalink / raw)
To: Zbigniew Jędrzejewski-Szmek
Cc: Nguyen Thai Ngoc Duy, git, Michael J Gruber, Ramsay Jones
In-Reply-To: <4F3647B4.8090803@in.waw.pl>
Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl> writes:
> Junio suggested that "a new file, term.c or something, be a lot more
> suitable home for the function you will be reusing from diff and other
> parts of the system". Nevertheless, I think that adding two files (.c
> and .h) to hold one function isn't worth it. It can live in pager.c.
> Terminal size is logically connected to paging after all.
I do not have any objection to the above reasoning.
Given that Nguyen's columns topic hasn't been merged to 'next' and I
expect it will be re-rolled anyway, I would prefer a patch that does the
move from help.c to pager.c that is based directly on v1.7.9, on top of
which your work and the columns topic can both be built independently.
Thanks.
^ permalink raw reply
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