* Re: Undo last commit?
From: Neal Kreitzinger @ 2012-02-11 18:19 UTC (permalink / raw)
To: git
Cc: Massimo Manca, Jakub Narebski, Jonathan Nieder, Mike, Ben Walton,
Ramkumar Ramachandra, Nguyễn Thái Ngọc Duy
In-Reply-To: <4E09DDAE.30801@ira.uka.de>
On 6/28/2011 8:57 AM, Holger Hellmuth wrote:
>
> It would be a nice companion to the not-yet-realized "git unadd" ;-)
>
or perhaps "git unstage"...
v/r,
neal
^ permalink raw reply
* Re: git-subtree Ready #2
From: Junio C Hamano @ 2012-02-11 18:03 UTC (permalink / raw)
To: David A. Greene; +Cc: git
In-Reply-To: <877gztmfwy.fsf@smith.obbligato.org>
greened@obbligato.org (David A. Greene) writes:
> I might need to fiddle with permissions, let me know.
Everybody's client can talk smart http these days. Please don't
host/publish the code behind a dumb HTTP server.
^ permalink raw reply
* Re: [PATCH 1/2] pack-refs: remove all empty directories under $GIT_DIR/refs
From: Junio C Hamano @ 2012-02-11 17:59 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: git, Jeff King
In-Reply-To: <CACsJy8Bh=FZ6kNN5hERK5_H7XnZ83BZ_EfsZ5XmJbrnn+CfgcQ@mail.gmail.com>
Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
> 2012/2/11 Junio C Hamano <gitster@pobox.com>:
>> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> ...
>> Would it make more sense to note the
>> directory for which rmdir() fails in try_remove_empty_parents(), and
>> revisit only these directories, at least?
>
> That would leave empty directories not sharing the ref's path until
> the failed rmdir() unexamined, I think.
True. Thanks.
>>> + subpath = xmalloc(pathlen + 257);
>>
>> What is this 257 about?
>
> This function is a ripoff from get_ref_dir(). I think 257 is 255 below
> plus '/' and NIL.
I do not think there is any justification to copy-and-paste from code that
predates the strbuf infrastructure these days.
^ permalink raw reply
* git-subtree Ready #2
From: David A. Greene @ 2012-02-11 17:35 UTC (permalink / raw)
To: git
[This bounced for some reason.]
Ok, I have http access now:
git clone http://sources.obbligato.org/git/git.git
git pull origin subtree
I might need to fiddle with permissions, let me know.
-Dave
^ permalink raw reply
* Re: [PATCH 5/8] gitweb: Format diff lines just before printing
From: Jakub Narebski @ 2012-02-11 17:14 UTC (permalink / raw)
To: Michał Kiedrowicz; +Cc: git
In-Reply-To: <1328865494-24415-6-git-send-email-michal.kiedrowicz@gmail.com>
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.
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).
> -# 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.
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH 4/8] gitweb: Push formatting diff lines to print_diff_chunk()
From: Jakub Narebski @ 2012-02-11 16:29 UTC (permalink / raw)
To: Michał Kiedrowicz; +Cc: git
In-Reply-To: <1328865494-24415-5-git-send-email-michal.kiedrowicz@gmail.com>
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?
As a note I would add also that process_diff_line got renamed to
format_diff_line, and its output changed to returning only
HTML-formatted line, which bringg it in line with other format_*
subroutines.
> 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>
> ---
> 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!
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH 3/8] gitweb: Move HTML-formatting diff line back to process_diff_line()
From: Jakub Narebski @ 2012-02-11 16:02 UTC (permalink / raw)
To: Michał Kiedrowicz; +Cc: git
In-Reply-To: <1328865494-24415-4-git-send-email-michal.kiedrowicz@gmail.com>
Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:
> Commit 6ba1eb51b (gitweb: Add a feature to show side-by-side diff,
> 2011-10-31) for no special reason moved wrapping diff line in <div> out
> of format_diff_line(). Bring back old behavior.
>
> This simplifies code in git_patchset_body() and keeps formatting of a
> diff line in one place.
>
> The more long-term purpose of this patch is to move formatting diff
> lines down to print_diff_chunk(), to allow processing lines without
> HTML-formatting.
>
> This is just a refactoring patch. It's not meant to change gitweb
> output.
Gah, I don't remember why we ended with change in behavior when adding
side-by-side diff support to gitweb.
> Signed-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>
Acked-by: Jakub Narębski <jnareb@gmail.com>
> ---
> gitweb/gitweb.perl | 15 ++++++++-------
> 1 files changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index ed63261..d2f75c4 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -2332,14 +2332,18 @@ sub process_diff_line {
>
> if ($from && $to && $line =~ m/^\@{2} /) {
> $line = format_unidiff_chunk_header($line, $from, $to);
> - return $diff_class, $line;
>
> } elsif ($from && $to && $line =~ m/^\@{3}/) {
> $line = format_cc_diff_chunk_header($line, $from, $to);
> - return $diff_class, $line;
> -
> + } else {
> + $line = esc_html($line, -nbsp=>1);
> }
> - return $diff_class, esc_html($line, -nbsp=>1);
> +
> + my $diff_classes = "diff";
> + $diff_classes .= " $diff_class" if ($diff_class);
> + $line = "<div class=\"$diff_classes\">$line</div>\n";
> +
> + return $diff_class, $line;
> }
>
> # Generates undef or something like "_snapshot_" or "snapshot (_tbz2_ _zip_)",
> @@ -5104,9 +5108,6 @@ sub git_patchset_body {
> next PATCH if ($patch_line =~ m/^diff /);
>
> my ($class, $line) = process_diff_line($patch_line, \%from, \%to);
> - my $diff_classes = "diff";
> - $diff_classes .= " $class" if ($class);
> - $line = "<div class=\"$diff_classes\">$line</div>\n";
>
> if ($class eq 'chunk_header') {
> print_diff_chunk($diff_style, $is_combined, @chunk);
> --
> 1.7.3.4
>
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH 2/8] gitweb: Use print_diff_chunk() for both side-by-side and inline diffs
From: Jakub Narebski @ 2012-02-11 15:53 UTC (permalink / raw)
To: Michał Kiedrowicz; +Cc: git
In-Reply-To: <1328865494-24415-3-git-send-email-michal.kiedrowicz@gmail.com>
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?
> 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?) in combined diff output; in ordinary diff you would always
have context or end/beginning of chunk between added and removed
lines.
>
> 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.
> 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()?
> +
> +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.
>
> 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)) {
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.
> + 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;
> }
> }
[...]
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH 1/8] gitweb: Extract print_sidebyside_diff_lines()
From: Jakub Narebski @ 2012-02-11 15:20 UTC (permalink / raw)
To: Michał Kiedrowicz; +Cc: git
In-Reply-To: <1328865494-24415-2-git-send-email-michal.kiedrowicz@gmail.com>
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.
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.
--
Jakub Narębski
^ permalink raw reply
* Re: [RFC/PATCH] tag: make list exclude !<pattern>
From: Nguyen Thai Ngoc Duy @ 2012-02-11 14:06 UTC (permalink / raw)
To: Jakub Narebski; +Cc: Junio C Hamano, Tom Grennan, git, krh, jasampler
In-Reply-To: <m3sjihu18t.fsf@localhost.localdomain>
On Sat, Feb 11, 2012 at 5:13 PM, Jakub Narebski <jnareb@gmail.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> In a future versions, we may want to have "branch/tag --list" also ask for
>> FNM_PATHNAME (this *is* a backward incompatible change, so it needs to be
>> performed across major version boundary, with backward compatibility
>> configurations, deprecation warnings and whole nine yards). Under the new
>> match function, today's "branch --list 'maint*'" needs to be spelled as
>> "branch --list 'maint*/*'" or something.
One of the reasons I proposed pathspec is to avoid another set of
matching syntax in addition to gitattr, gitignore and pathspec,
Pathspec magic makes it extensible, FNM_PATHNAME be set that way.
> Or "branch --list 'maint**'
But this comes up a few times already on gitignore discussions. We
have fnmatch.c in compat for Windows. Someone just needs to step up
and implement "**". It would also a nice thing to add to pathspec
syntax, if we can forget the backward compatibility nightmare.
--
Duy
^ permalink raw reply
* Re: [RFD] Rewriting safety - warn before/when rewriting published history
From: Jakub Narebski @ 2012-02-11 13:45 UTC (permalink / raw)
To: Johan Herland; +Cc: Philip Oakley, git
In-Reply-To: <CALKQrgdWOgG3y2HzM694zDykGJWa4QDetsEVXf0AGpf=FNFaVg@mail.gmail.com>
On Sat, 11 Feb 2012, Johan Herland wrote:
> On Fri, Feb 10, 2012 at 20:38, Jakub Narebski <jnareb@gmail.com> wrote:
> > On Tue, 7 Feb 2012, Johan Herland wrote:
[...]
> > > I am unsure whether the 'secret'-ness of a commit should follow across
> > > the push, but if you do (assuming we store the 'secret' flag using
> > > git-notes) this is simply a matter of synchronizing the
> > > refs/notes/secret to the same remote.
> >
> > I think it should, so that 'secret' commit would not escape by accident
> > via a group secret repository.
> >
> > What makes it hard (I think) is that we would prefer to transfer
> > 'secret'-ness only for pushed commits. That might be problem for notes
> > based implementation of 'secret' annotation and 'secret'-ness transfer...
> > though I guess knowing that there exist 'secret' commit with given SHA1
> > which we do not have and should not have is not much breach of
> > confidentiality. Still...
>
> If you don't want to transfer all of refs/notes/secret, you would
> probably have to extend the git protocol with a per-commit 'secret'
> flag (which would then be applied to the receiving repo's
> refs/notes/secret).
Or create per-remote custom notes ref, for example for 'foo' remote it
would be 'refs/remotes/foo/notes/secret'... assuming that canonalization
of remote-tracking refs goes in (refs/remotes/foo/{heads,tags,notes,replace})
This would be updated with 'secret'-ness state of comits being pushed
before actual push, and secretness notes ref would be pushed together
with actual push.
> Still, this is all specific to the 'secret' feature, which IMHO is
> much less important then the 'public' feature. Implementing the
> barebones 'public' feature (i.e. refuse rewrite of commits reachable
> from upstream) is much less work, and would be enough for 90% of git
> users, I believe.
Hmmm... I thought that is 'public' feature that is more work, especially
in full incarnation. But perhaps bare bones (no 'pre-push' or 'pre-rewrite'
hooks, no traits info in "git log" output, per remote tracking of 'public'
status only, no support for bundles or send-email, etc.) could be as easy
or easier.
As to what is more important for git users... perhaps short survey would
help here? I wonder if asking Mercurial users about their use of "phases"
on their mailing would help us...
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: [RFD] Rewriting safety - warn before/when rewriting published history
From: Johan Herland @ 2012-02-11 13:10 UTC (permalink / raw)
To: Jakub Narebski; +Cc: Philip Oakley, git
In-Reply-To: <201202102038.55710.jnareb@gmail.com>
On Fri, Feb 10, 2012 at 20:38, Jakub Narebski <jnareb@gmail.com> wrote:
> On Tue, 7 Feb 2012, Johan Herland wrote:
>> On Tue, Feb 7, 2012 at 15:31, Jakub Narebski <jnareb@gmail.com> wrote:
>> I am unsure whether the 'secret'-ness of a commit should follow across
>> the push, but if you do (assuming we store the 'secret' flag using
>> git-notes) this is simply a matter of synchronizing the
>> refs/notes/secret to the same remote.
>
> I think it should, so that 'secret' commit would not escape by accident
> via a group secret repository.
>
> What makes it hard (I think) is that we would prefer to transfer
> 'secret'-ness only for pushed commits. That might be problem for notes
> based implementation of 'secret' annotation and 'secret'-ness transfer...
> though I guess knowing that there exist 'secret' commit with given SHA1
> which we do not have and should not have is not much breach of
> confidentiality. Still...
If you don't want to transfer all of refs/notes/secret, you would
probably have to extend the git protocol with a per-commit 'secret'
flag (which would then be applied to the receiving repo's
refs/notes/secret).
Still, this is all specific to the 'secret' feature, which IMHO is
much less important then the 'public' feature. Implementing the
barebones 'public' feature (i.e. refuse rewrite of commits reachable
from upstream) is much less work, and would be enough for 90% of git
users, I believe.
...Johan
--
Johan Herland, <johan@herland.net>
www.herland.net
^ permalink raw reply
* [PATCH] gitweb: Silence stderr in parse_commit*() subroutines
From: Jakub Narebski @ 2012-02-11 13:02 UTC (permalink / raw)
To: rajesh boyapati; +Cc: git
In-Reply-To: <201202092114.40832.jnareb@gmail.com>
On Thu, 9 Feb 2012, Jakub Narebski wrote:
> On Wed, 8 Feb 2012, rajesh boyapati wrote:
> > 2012/2/8 Jakub Narebski <jnareb@gmail.com>
>
> [...]
> > > Does the following patch help, and does it fix the issue?
> > >
> > > (Nb. you can try to simply change filename, and apply it with fuzz
> > > against index.cgi file).
> > > -- >8 -- ----- ----- ----- ----- ----- -- >8 --
> > > From: Jakub Narebski <jnareb@gmail.com>
> > > Subject: [PATCH] gitweb: Harden parse_commit and parse_commits
> [...]
> > When I applied the above patch and also the patch from your previous
> > e-mail, I am getting this error
> > >>>>>>>>>>>>>
> > [2012-02-08 14:09:58,396] ERROR
> > com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: fatal: bad revision
> > 'HEAD'
> > [2012-02-08 14:10:06,732] ERROR
> > com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: fatal: bad revision
> > 'HEAD'
> > [2012-02-08 14:10:11,404] ERROR
> > com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: fatal: bad revision
> > 'HEAD'
> > [2012-02-08 14:10:15,270] ERROR
> > com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: fatal: Not a valid
> > object name HEAD
> > <<<<<<<<<<<<<<
> > With these patches, the previous errors at line numbers are gone.
>
> Thanks for information.
>
>
> This final issue will be a bit harder to fix. This error message
>
> fatal: bad revision 'HEAD'
>
> comes from git (I think from "git rev-list" command), and not from gitweb.
> It is printed on STDERR of git command. What has to be done to fix it is
> to capture stderr of a process, or silence it.
>
> Unfortunately it is not that easy. We use list form of open, which avoids
> using a shell interpreter to run command, and is safer wrt. shell escaping.
>
> The only place where gitweb cares about redirecting standard error from git
> command is git_object(). It is a bit hacky, and might be not entirely safe.
> To fix this issue we would have to do the same in parse_commit*() as in
> git_object(), or provide some kind of wrapper like IPC::Run provides
> for redirecting stderr of called command.
>
> Note that this issue was not considered very important, because this message
> doesn't goes into web server logs when running gitweb via mod_cgi with
> Apache... and probably also with other web servers. Gerrit (or rather
> whatever it uses for serving CGI scripts) might be exception here.
Anyway, here is the patch that should fix those "CGI: fatal: Not a valid
object name HEAD" errors for you.
I'll resend the all the patches as single patch series for inclusion in
git, but I am not sure if this latest patch will be accepted because of
drawbacks of its implementation.
-- >8 ---- ----- ----- ----- >8 --
From: Jakub Narebski <jnareb@gmail.com>
Subject: [PATCH] gitweb: Silence stderr in parse_commit*() subroutines
git-rev-list command in parse_commit() and parse_commits() can fail
because $commit_id doesn't point to a valid commit; for example if we
are on unborn branch HEAD doesn't point to a valid commit.
In this case git-rev-list prints
fatal: bad revision 'HEAD'
on its standard error. This commit silences this warning, at the cost
of using shell to redirect it to /dev/null, and relying on
quote_command() to protect against code injection.
Note however that such error message from git (from extrenal command)
usually but not always does not appear in web server logs.
Reported-by: rajesh boyapati <boyapatisrajesh@gmail.com>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
gitweb/gitweb.perl | 10 ++++++----
1 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 1181aeb..081ac45 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3338,12 +3338,13 @@ sub parse_commit {
local $/ = "\0";
- open my $fd, "-|", git_cmd(), "rev-list",
+ open my $fd, "-|", quote_command(
+ git_cmd(), "rev-list",
"--parents",
"--header",
"--max-count=1",
$commit_id,
- "--",
+ "--") . ' 2>/dev/null',
or die_error(500, "Open git-rev-list failed");
my $commit_text = <$fd>;
%co = parse_commit_text($commit_text, 1)
@@ -3363,7 +3364,8 @@ sub parse_commits {
local $/ = "\0";
- open my $fd, "-|", git_cmd(), "rev-list",
+ open my $fd, "-|", quote_command(
+ git_cmd(), "rev-list",
"--header",
@args,
("--max-count=" . $maxcount),
@@ -3371,7 +3373,7 @@ sub parse_commits {
@extra_options,
$commit_id,
"--",
- ($filename ? ($filename) : ())
+ ($filename ? ($filename) : ())) . ' 2>/dev/null'
or die_error(500, "Open git-rev-list failed");
while (my $line = <$fd>) {
my %co = parse_commit_text($line);
--
1.7.9
^ permalink raw reply related
* Re: [PATCH] pack-refs: remove all empty dirs under .git/{refs,logs/refs}
From: Thomas Adam @ 2012-02-11 11:27 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Junio C Hamano, Jeff King
In-Reply-To: <1328958484-4202-1-git-send-email-pclouds@gmail.com>
2012/2/11 Nguyễn Thái Ngọc Duy <pclouds@gmail.com>:
> static void prune_refs(struct ref_to_prune *r)
> {
> + struct string_list *list = get_empty_ref_directories();;
Double ";;" at end of line.
-- Thomas Adam
^ permalink raw reply
* [PATCH] pack-refs: remove all empty dirs under .git/{refs,logs/refs}
From: Nguyễn Thái Ngọc Duy @ 2012-02-11 11:08 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Nguyễn Thái Ngọc Duy
In-Reply-To: <1328946907-31650-1-git-send-email-pclouds@gmail.com>
"git pack-refs" tries to remove directory that becomes empty but it
does not try to do so hard enough. Only empty directories created
because a ref is packed are considered.
This patch introduces a global switch, which instructs ref machinery
to collect all empty directories (or ones containing only empty
directories) in removable order. "git pack-refs" uses this information
to clean $GIT_DIR/refs and $GIT_DIR/logs/refs.
Some directories are kept by this patch even if they are empty: refs,
refs/heads and refs/tags. The first one is one of git repository
signature. The rest is created by init-db, one may expect them to always
be there.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
v3, no second look at $GIT_DIR/refs and also clean
$GIT_DIR/logs/refs. Not really fond of the global switch, but it does
not look very intrusive to refs.c
builtin/pack-refs.c | 2 ++
pack-refs.c | 10 ++++++++++
refs.c | 35 +++++++++++++++++++++++++++++++----
refs.h | 4 ++++
t/t3210-pack-refs.sh | 10 ++++++++++
5 files changed, 57 insertions(+), 4 deletions(-)
diff --git a/builtin/pack-refs.c b/builtin/pack-refs.c
index 39a9d89..044ae8f 100644
--- a/builtin/pack-refs.c
+++ b/builtin/pack-refs.c
@@ -1,6 +1,7 @@
#include "builtin.h"
#include "parse-options.h"
#include "pack-refs.h"
+#include "refs.h"
static char const * const pack_refs_usage[] = {
"git pack-refs [options]",
@@ -15,6 +16,7 @@ int cmd_pack_refs(int argc, const char **argv, const char *prefix)
OPT_BIT(0, "prune", &flags, "prune loose refs (default)", PACK_REFS_PRUNE),
OPT_END(),
};
+ save_empty_ref_directories = 1;
if (parse_options(argc, argv, prefix, opts, pack_refs_usage, 0))
usage_with_options(pack_refs_usage, opts);
return pack_refs(flags);
diff --git a/pack-refs.c b/pack-refs.c
index f09a054..76d3408 100644
--- a/pack-refs.c
+++ b/pack-refs.c
@@ -2,6 +2,7 @@
#include "refs.h"
#include "tag.h"
#include "pack-refs.h"
+#include "string-list.h"
struct ref_to_prune {
struct ref_to_prune *next;
@@ -105,10 +106,19 @@ static void prune_ref(struct ref_to_prune *r)
static void prune_refs(struct ref_to_prune *r)
{
+ struct string_list *list = get_empty_ref_directories();;
+ int i;
+
while (r) {
prune_ref(r);
r = r->next;
}
+
+ for (i = 0; i < list->nr; i++) {
+ const char *s = list->items[i].string;
+ rmdir(git_path("%s", s));
+ rmdir(git_path("logs/%s", s));
+ }
}
static struct lock_file packed;
diff --git a/refs.c b/refs.c
index b8843bb..7e9a250 100644
--- a/refs.c
+++ b/refs.c
@@ -3,6 +3,7 @@
#include "object.h"
#include "tag.h"
#include "dir.h"
+#include "string-list.h"
/* ISSYMREF=0x01, ISPACKED=0x02 and ISBROKEN=0x04 are public interfaces */
#define REF_KNOWS_PEELED 0x10
@@ -29,6 +30,8 @@ struct ref_array {
struct ref_entry **refs;
};
+int save_empty_ref_directories;
+
/*
* Parse one line from a packed-refs file. Write the SHA1 to sha1.
* Return a pointer to the refname within the line (null-terminated),
@@ -177,6 +180,7 @@ static struct ref_cache {
char did_packed;
struct ref_array loose;
struct ref_array packed;
+ struct string_list empty_dirs;
/* The submodule name, or "" for the main repo. */
char name[FLEX_ARRAY];
} *ref_cache;
@@ -326,7 +330,7 @@ void add_packed_ref(const char *refname, const unsigned char *sha1)
}
static void get_ref_dir(struct ref_cache *refs, const char *base,
- struct ref_array *array)
+ struct ref_array *array, int *would_be_empty)
{
DIR *dir;
const char *path;
@@ -343,6 +347,7 @@ static void get_ref_dir(struct ref_cache *refs, const char *base,
struct dirent *de;
int baselen = strlen(base);
char *refname = xmalloc(baselen + 257);
+ int nr = 0;
memcpy(refname, base, baselen);
if (baselen && base[baselen-1] != '/')
@@ -355,8 +360,13 @@ static void get_ref_dir(struct ref_cache *refs, const char *base,
int namelen;
const char *refdir;
- if (de->d_name[0] == '.')
+ if (de->d_name[0] == '.') {
+ if (strcmp(de->d_name, "..") &&
+ strcmp(de->d_name, "."))
+ nr++;
continue;
+ }
+ nr++;
namelen = strlen(de->d_name);
if (namelen > 255)
continue;
@@ -369,7 +379,10 @@ static void get_ref_dir(struct ref_cache *refs, const char *base,
if (stat(refdir, &st) < 0)
continue;
if (S_ISDIR(st.st_mode)) {
- get_ref_dir(refs, refname, array);
+ int empty = 0;
+ get_ref_dir(refs, refname, array, &empty);
+ if (empty)
+ nr--;
continue;
}
if (*refs->name) {
@@ -387,6 +400,15 @@ static void get_ref_dir(struct ref_cache *refs, const char *base,
}
free(refname);
closedir(dir);
+ if (save_empty_ref_directories &&
+ nr == 0 &&
+ strcmp(base, "refs") &&
+ strcmp(base, "refs/heads") &&
+ strcmp(base, "refs/tags")) {
+ string_list_append(&refs->empty_dirs, xstrdup(base));
+ if (would_be_empty)
+ *would_be_empty = 1;
+ }
}
}
@@ -427,12 +449,17 @@ void warn_dangling_symref(FILE *fp, const char *msg_fmt, const char *refname)
static struct ref_array *get_loose_refs(struct ref_cache *refs)
{
if (!refs->did_loose) {
- get_ref_dir(refs, "refs", &refs->loose);
+ get_ref_dir(refs, "refs", &refs->loose, NULL);
refs->did_loose = 1;
}
return &refs->loose;
}
+struct string_list *get_empty_ref_directories()
+{
+ return &get_ref_cache(NULL)->empty_dirs;
+}
+
/* We allow "recursive" symbolic refs. Only within reason, though */
#define MAXDEPTH 5
#define MAXREFLEN (1024)
diff --git a/refs.h b/refs.h
index 00ba1e2..21a2a00 100644
--- a/refs.h
+++ b/refs.h
@@ -14,6 +14,10 @@ struct ref_lock {
#define REF_ISPACKED 0x02
#define REF_ISBROKEN 0x04
+struct string_list;
+extern int save_empty_ref_directories;
+extern struct string_list *get_empty_ref_directories();
+
/*
* Calls the specified function for each ref file until it returns nonzero,
* and returns the value
diff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh
index cd04361..40fcd54 100755
--- a/t/t3210-pack-refs.sh
+++ b/t/t3210-pack-refs.sh
@@ -66,6 +66,16 @@ test_expect_success 'see if git pack-refs --prune removes empty dirs' '
! test -e .git/refs/heads/r
'
+test_expect_success 'pack-refs --prune removes all empty dirs in refs and logs' '
+ mkdir -p .git/refs/empty/outside/heads &&
+ mkdir -p .git/refs/heads/empty/dir/ectory &&
+ mkdir -p .git/logs/refs/heads/empty/dir/ectory &&
+ git pack-refs --all --prune &&
+ ! test -e .git/refs/empty &&
+ ! test -e .git/refs/heads/empty &&
+ ! test -e .git/logs/refs/heads/empty
+'
+
test_expect_success \
'git branch g should work when git branch g/h has been deleted' \
'git branch g/h &&
--
1.7.8.36.g69ee2
^ permalink raw reply related
* Re: [PATCH 2/3] help.c: make term_columns() cached and export it
From: Zbigniew Jędrzejewski-Szmek @ 2012-02-11 10:49 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: git, gitster, Michael J Gruber, Ramsay Jones
In-Reply-To: <CACsJy8AjqqEpJr64DJUqVumw1sB2g3pvuz-g4DxhmS-ZbGhY3w@mail.gmail.com>
On 02/11/2012 05:36 AM, Nguyen Thai Ngoc Duy wrote:
> 2012/2/10 Zbigniew Jędrzejewski-Szmek<zbyszek@in.waw.pl>:
>> Since term_columns() will usually fail, when a pager is installed,
>> the cache is primed before the pager is installed. If a pager is not
>> installed, then the cache will be set on first use.
>
> Conflict alert. term_columns() is also moved out of help.c in
> nd/columns series on pu, commit cb0850f (Save terminal width before
> setting up pager - 2012-02-04)
Thanks for the heads-up. I think that the two patches should be
merged, especially because there's an error in cb0850f (a variable is
read-only, never written).
Tweaks to cb0850f (Save terminal width before setting up pager -
2012-02-04):
[This actually was done on top of today's pu, so it will not apply
cleanly to cb0850f].
- term_columns() lives in pager.c so it should be declared in
cache.h like other public functions in pager.c. It has nothing to do
with columns.h.
- simplify logic to use a static variable instead of two global
variables (cb0850f actually doesn't work at all because
spawned_pager wasn't ever set).
- check the cache first, then do getenv(COLUMNS) + atoi, then do
ioctl(). The behaviour is equivalent to checking COLUMNS first, but
is slightly more efficient.
- document the function
- remove #include "column.h" added in 88c9754c4097 column: Fix some
compiler and sparse warnings (Wed Feb 8 2012).
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.
---
cache.h | 1 +
column.h | 1 -
pager.c | 64 +++++++++++++++++++++++-----------------
3 files changed, 38 insertions(+), 28 deletions(-)
diff --git a/cache.h b/cache.h
index 6c70dbc..b4422d4 100644
--- a/cache.h
+++ b/cache.h
@@ -1196,6 +1196,7 @@ extern void setup_pager(void);
extern const char *pager_program;
extern int pager_in_use(void);
extern int pager_use_color;
+extern int term_columns(void);
extern const char *editor_program;
extern const char *askpass_program;
diff --git a/column.h b/column.h
index b9dec64..142299e 100644
--- a/column.h
+++ b/column.h
@@ -17,7 +17,6 @@ struct column_options {
const char *nl;
};
-extern int term_columns(void);
extern void print_columns(const struct string_list *list,
unsigned int mode,
struct column_options *opts);
diff --git a/pager.c b/pager.c
index fe203a7..d105761 100644
--- a/pager.c
+++ b/pager.c
@@ -7,21 +7,6 @@
#define DEFAULT_PAGER "less"
#endif
-static int spawned_pager;
-static int max_columns;
-
-static int retrieve_terminal_width(void)
-{
-#ifdef TIOCGWINSZ
- struct winsize ws;
- if (ioctl(1, TIOCGWINSZ, &ws)) /* e.g., ENOSYS */
- return 0;
- return ws.ws_col;
-#else
- return 0;
-#endif
-}
-
/*
* This is split up from the rest of git so that we can do
* something different on Windows.
@@ -88,16 +73,15 @@ const char *git_pager(int stdout_is_tty)
void setup_pager(void)
{
const char *pager = git_pager(isatty(1));
- int width;
if (!pager || pager_in_use())
return;
- setenv("GIT_PAGER_IN_USE", "true", 1);
+ /* prime the term_columns() cache before it is too
+ * late and stdout is replaced */
+ (void) term_columns();
- width = retrieve_terminal_width();
- if (width)
- max_columns = width;
+ setenv("GIT_PAGER_IN_USE", "true", 1);
/* spawn the pager */
pager_argv[0] = pager;
@@ -132,17 +116,43 @@ int pager_in_use(void)
return env ? git_config_bool("GIT_PAGER_IN_USE", env) : 0;
}
+/*
+ * Return cached value (if set) or $COLUMNS (if set and positive) or
+ * ioctl(1, TIOCGWINSZ).ws_col (if positive) or 80.
+ *
+ * $COLUMNS even if set, is usually not exported, so
+ * the variable can be used to override autodection.
+ * This behaviour conforms to The Single UNIX Specification, Version 2
+ *
(http://pubs.opengroup.org/onlinepubs/7908799/xbd/envvar.html#tag_002_003).
+ */
int term_columns(void)
{
- char *col_string = getenv("COLUMNS");
+ static int term_columns_cache;
+
+ char *col_string;
int n_cols;
- if (col_string && (n_cols = atoi(col_string)) > 0)
- return n_cols;
+ if (term_columns_cache)
+ return term_columns_cache;
+
+ col_string = getenv("COLUMNS");
+ if (col_string && (n_cols = atoi(col_string)) > 0) {
+ term_columns_cache = n_cols;
+ return term_columns_cache;
+ }
- if (spawned_pager && max_columns)
- return max_columns;
+#ifdef TIOCGWINSZ
+ {
+ struct winsize ws;
+ if (!ioctl(1, TIOCGWINSZ, &ws)) {
+ if (ws.ws_col) {
+ term_columns_cache = ws.ws_col;
+ return term_columns_cache;
+ }
+ }
+ }
+#endif
- n_cols = retrieve_terminal_width();
- return n_cols ? n_cols : 80;
+ term_columns_cache = 80;
+ return term_columns_cache;
}
--
1.7.9.310.g883d84c.dirty
^ permalink raw reply related
* Re: Git SSH Authentication
From: Sitaram Chamarty @ 2012-02-11 10:23 UTC (permalink / raw)
To: Junio C Hamano; +Cc: isawk, git
In-Reply-To: <7vty2xn6tl.fsf@alter.siamese.dyndns.org>
On Sat, Feb 11, 2012 at 1:24 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Sitaram Chamarty <sitaramc@gmail.com> writes:
>
>> Common causes of pubkey access fail:
>>
>> - wrong pubkey being offered: if you are using ssh-agent, make sure
>> you have 'ssh-add'ed the key you want to offer. Confirm with 'ssh-add
>> -l'
>
> A failure related to this I saw is to have (too) many keys in ssh-agent,
> and running ssh without telling it which exact key to use. The client
> tries each key in turn and the server rejects the connection attempt after
> seeing too many keys tried. "ssh -v" is useful to diagnose this mode of
> failure, and an entry in ~/.ssh/config like:
>
> Host example.com
> User myusernameoverthere
> IdentityFile ~/.ssh/id_rsa-for-example.com
>
> would fix it.
Interesting... I didn't know this, so I checked man sshd_config
looking for exactly how many keys it would fail after. The setting
seems to be MaxAuthTries, and defaults to 6.
--
Sitaram
^ permalink raw reply
* Re: git svn problem
From: Serhat Sevki Dincer @ 2012-02-11 10:19 UTC (permalink / raw)
To: git
In-Reply-To: <CAPqC6xSJ7pfUQJz8FQ743mJMNmZyfJfWxutOnt+FVkP76eXOGw@mail.gmail.com>
Ok, I figured out the rest, in case someone needs it:
> I have the following at the moment:
>
> rm -rf plone.app.locales ; mkdir plone.app.locales ; cd plone.app.locales
> git svn init -T trunk http://svn.plone.org/svn/plone/plone.app.locales
> touch start ; git add start ; git commit -m start
> git svn fetch -r49624:HEAD
> git rebase --onto master --root trunk --preserve-merges
> git checkout -b plone
# edit .git/config: s#/plone/#/collective/# s#remotes/trunk#remotes/trunkcol#
git checkout master
git svn fetch -r248302:HEAD
git rebase --onto plone --root trunkcol --preserve-merges
git checkout -b collective
git branch -d master plone
^ permalink raw reply
* Re: [RFC/PATCH] tag: make list exclude !<pattern>
From: Jakub Narebski @ 2012-02-11 10:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Tom Grennan, pclouds, git, krh, jasampler
In-Reply-To: <7vy5s9n70x.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> In a future versions, we may want to have "branch/tag --list" also ask for
> FNM_PATHNAME (this *is* a backward incompatible change, so it needs to be
> performed across major version boundary, with backward compatibility
> configurations, deprecation warnings and whole nine yards). Under the new
> match function, today's "branch --list 'maint*'" needs to be spelled as
> "branch --list 'maint*/*'" or something.
Or "branch --list 'maint**'
--
Jakub Narebski
^ permalink raw reply
* [PATCH] strbuf: move strbuf_readline_fd() from bundle.c to strbuf.{c,h}
From: 徐迪 @ 2012-02-11 9:50 UTC (permalink / raw)
To: Git 邮件列表
strbuf_readline_fd() existed in bundle.c since e9ee84cf, but this
function can be used elsewhere, and since it's relevant to strbuf, it
should be in strbuf.{c,h}.
Signed-off-by: Xu Di <xudifsd@gmail.com>
---
bundle.c | 18 +-----------------
strbuf.c | 16 ++++++++++++++++
strbuf.h | 1 +
3 files changed, 18 insertions(+), 17 deletions(-)
diff --git a/bundle.c b/bundle.c
index b8acf3c..9344a91 100644
--- a/bundle.c
+++ b/bundle.c
@@ -7,6 +7,7 @@
#include "list-objects.h"
#include "run-command.h"
#include "refs.h"
+#include "strbuf.h"
static const char bundle_signature[] = "# v2 git bundle\n";
@@ -23,23 +24,6 @@ static void add_to_ref_list(const unsigned char
*sha1, const char *name,
list->nr++;
}
-/* Eventually this should go to strbuf.[ch] */
-static int strbuf_readline_fd(struct strbuf *sb, int fd)
-{
- strbuf_reset(sb);
-
- while (1) {
- char ch;
- ssize_t len = xread(fd, &ch, 1);
- if (len <= 0)
- return len;
- strbuf_addch(sb, ch);
- if (ch == '\n')
- break;
- }
- return 0;
-}
-
static int parse_bundle_header(int fd, struct bundle_header *header,
const char *report_path)
{
diff --git a/strbuf.c b/strbuf.c
index ff0b96b..7532a13 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -282,6 +282,22 @@ void strbuf_addbuf_percentquote(struct strbuf
*dst, const struct strbuf *src)
}
}
+int strbuf_readline_fd(struct strbuf *sb, int fd)
+{
+ strbuf_reset(sb);
+
+ while (1) {
+ char ch;
+ ssize_t len = xread(fd, &ch, 1);
+ if (len <= 0)
+ return len;
+ strbuf_addch(sb, ch);
+ if (ch == '\n')
+ break;
+ }
+ return 0;
+}
+
size_t strbuf_fread(struct strbuf *sb, size_t size, FILE *f)
{
size_t res;
diff --git a/strbuf.h b/strbuf.h
index fbf059f..ecebd11 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -109,6 +109,7 @@ static inline void strbuf_complete_line(struct strbuf *sb)
}
extern size_t strbuf_fread(struct strbuf *, size_t, FILE *);
+extern int strbuf_readline_fd(struct strbuf *sb, int fd);
/* XXX: if read fails, any partial read is undone */
extern ssize_t strbuf_read(struct strbuf *, int fd, size_t hint);
extern int strbuf_read_file(struct strbuf *sb, const char *path, size_t hint);
--
1.7.8.1.749.gb6b3b
^ permalink raw reply related
* Re: [PATCH 1/2] pack-refs: remove all empty directories under $GIT_DIR/refs
From: Nguyen Thai Ngoc Duy @ 2012-02-11 8:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King
In-Reply-To: <7vhayxn5cg.fsf@alter.siamese.dyndns.org>
2012/2/11 Junio C Hamano <gitster@pobox.com>:
> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>
>> Deleting refs does not remove parent directories if they are empty.
>> Empty directories add extra overhead to startup time of most of git
>> commands because they have to traverse $GIT_DIR/refs.
>
> Perhaps drop the first line and replace with the description of what you
> do differently from the first round?
>
> "git pack-refs" tries to remove directory that becomes empty but it
> does not try to do so hard enough, leaving a parent directory full of
> empty children directories without removing.
Sure.
> While I agree with Peff that people would expect doing other things while
> pack-refs is running would be much "riskier" and doing this inside
> pack-refs is far more preferable than doing so during normal read-only
> operation, I wonder why we would want a completely separate pass that
> scans the entire hierarchy
Less complex code. Doing it in one pass, I think get_ref_dir() needs
to learn read-only vs read-write mode and I haven't figured out a
non-ugly way to do it.
> Would it make more sense to note the
> directory for which rmdir() fails in try_remove_empty_parents(), and
> revisit only these directories, at least?
That would leave empty directories not sharing the ref's path until
the failed rmdir() unexamined, I think.
> Wouldn't we want to rmdir() the corresponding logs/ hierarchy while at it
> to be consistent?
Good idea.
>> + subpath = xmalloc(pathlen + 257);
>
> What is this 257 about?
This function is a ripoff from get_ref_dir(). I think 257 is 255 below
plus '/' and NIL.
>> + if (namelen > 255)
>> + continue;
--
Duy
^ permalink raw reply
* Re: [PATCH 1/2] pack-refs: remove all empty directories under $GIT_DIR/refs
From: Junio C Hamano @ 2012-02-11 8:26 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Junio C Hamano, Jeff King
In-Reply-To: <1328946907-31650-1-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> Deleting refs does not remove parent directories if they are empty.
> Empty directories add extra overhead to startup time of most of git
> commands because they have to traverse $GIT_DIR/refs.
Perhaps drop the first line and replace with the description of what you
do differently from the first round?
"git pack-refs" tries to remove directory that becomes empty but it
does not try to do so hard enough, leaving a parent directory full of
empty children directories without removing.
or something?
> Some directories are kept by this patch even if they are empty (refs,
> refs/heads and refs/tags). The first one is one of git repository
> signature. The rest is created by init-db, one may expect them to always
> be there.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> v2, no more refs code change.
>
> Part of the reason I do not want to update delete_ref() is because it
> won't remove empty directories in existing repositories.
While I agree with Peff that people would expect doing other things while
pack-refs is running would be much "riskier" and doing this inside
pack-refs is far more preferable than doing so during normal read-only
operation, I wonder why we would want a completely separate pass that
scans the entire hierarchy. Would it make more sense to note the
directory for which rmdir() fails in try_remove_empty_parents(), and
revisit only these directories, at least?
Wouldn't we want to rmdir() the corresponding logs/ hierarchy while at it
to be consistent?
>
> pack-refs.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 files changed, 53 insertions(+), 0 deletions(-)
>
> diff --git a/pack-refs.c b/pack-refs.c
> index f09a054..bb3a9c4 100644
> --- a/pack-refs.c
> +++ b/pack-refs.c
> @@ -91,6 +91,58 @@ static void try_remove_empty_parents(char *name)
> }
> }
>
> +static int prune_empty_dirs(const char *path)
> +{
> + int nr_entries = 0, pathlen = strlen(path);
> + DIR *dir;
> + struct dirent *de;
> + char *subpath;
> +
> + dir = opendir(git_path("%s", path));
> +
> + if (!dir)
> + return 0;
> +
> + subpath = xmalloc(pathlen + 257);
What is this 257 about?
> + memcpy(subpath, path, pathlen);
> + if (pathlen && path[pathlen-1] != '/')
> + subpath[pathlen++] = '/';
> +
> + while ((de = readdir(dir)) != NULL) {
> + struct stat st;
> + int namelen;
> +
> + if (de->d_name[0] == '.') {
> + if (strcmp(de->d_name, "..") && strcmp(de->d_name, "."))
> + nr_entries++;
> + continue;
> + }
> + nr_entries++;
> + namelen = strlen(de->d_name);
> + if (namelen > 255)
> + continue;
> + if (has_extension(de->d_name, ".lock"))
> + continue;
This is a sign that somebody else might be actively accessing this
repository.
> + memcpy(subpath + pathlen, de->d_name, namelen+1);
> + if (stat(git_path("%s", subpath), &st) < 0)
> + continue;
> + if (S_ISDIR(st.st_mode)) {
> + int removed = prune_empty_dirs(subpath);
> + if (removed)
> + nr_entries--;
> + continue;
> + }
> + }
> + free(subpath);
> + closedir(dir);
> + if (nr_entries == 0 &&
> + strcmp(path, "refs") &&
> + strcmp(path, "refs/heads") &&
> + strcmp(path, "refs/tags"))
> + return rmdir(git_path("%s", path)) == 0;
> + return 0;
> +}
> +
> /* make sure nobody touched the ref, and unlink */
> static void prune_ref(struct ref_to_prune *r)
> {
> @@ -109,6 +161,7 @@ static void prune_refs(struct ref_to_prune *r)
> prune_ref(r);
> r = r->next;
> }
> + prune_empty_dirs("refs");
> }
>
> static struct lock_file packed;
^ permalink raw reply
* Re: [RFC/PATCH] tag: make list exclude !<pattern>
From: Junio C Hamano @ 2012-02-11 8:13 UTC (permalink / raw)
To: Michael Haggerty; +Cc: Tom Grennan, pclouds, git, krh, jasampler
In-Reply-To: <4F361DD4.9020108@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
> The proposal, amended to use "^" instead of "!", is that
>
> git for-each-ref A B ^C
>
> should mean "the reference names A and B but not C". Therefore, the command
>
> git rev-list $(git for-each-ref A B ^C)
>
> , which consistency suggests should do the same thing as the first
> command,...
That is an utter rubbish that does not even deserve a response.
Your argument is like saying
git for-each-ref A
and
git for-each-ref $(git rev-parse A)
should somehow magically produce the same (or related) result. The
for-each-ref command operates on refname patterns, while rev-list and
rev-parse takes object names.
^ permalink raw reply
* Re: [PATCHv2 1/4] refs: add common refname_match_patterns()
From: Junio C Hamano @ 2012-02-11 8:06 UTC (permalink / raw)
To: Tom Grennan; +Cc: pclouds, git, krh, jasampler
In-Reply-To: <1328926618-17167-2-git-send-email-tmgrennan@gmail.com>
Tom Grennan <tmgrennan@gmail.com> writes:
> +int refname_match_patterns(const char **patterns, const char *refname)
> +{
> + int given_match_pattern = 0, had_match = 0;
> +
> + for (; *patterns; patterns++)
> + if (**patterns != '!') {
> + given_match_pattern = 1;
> + if (!fnmatch(*patterns, refname, 0))
> + had_match = 1;
> + } else if (!fnmatch(*patterns+1, refname, 0))
> + return 0;
> + return given_match_pattern ? had_match : 1;
> +}
This, while its semantics seem sane, is highly inefficient when you have
many patterns, and you will be calling this to filter dozens of refs. And
it can trivially improved by first pre-parsing the pattern[] array.
* If you know the patterns do not have any negative entry, you can return
true upon seeing the first match. Because you do not pre-parse the
pattern[] array, this loop does not know if there is any negative one,
and has to scan it always all the way.
* If you arrange the pattern[] array so that it has negative ones early,
again, you can return false upon seeing the first hit with a negative
one. If your input has negative ones at the end, the loop ends up
scanning all the way, noting the positive matches, only to discard upon
seeing the negative match at the end.
That is why I said Nguyen's idea of reusing pathspec matching logic
somewhat attractive, even though I think it has downsides (the exact
matching logic for pathspec is more similar to that of for-each-ref
and very different from branch/tag).
^ permalink raw reply
* Re: Git SSH Authentication
From: Junio C Hamano @ 2012-02-11 7:54 UTC (permalink / raw)
To: Sitaram Chamarty; +Cc: isawk, git
In-Reply-To: <CAMK1S_jmY5KvBH8z6YKszroMai4O5ULeCBYGAGFT4CgVUAfmwg@mail.gmail.com>
Sitaram Chamarty <sitaramc@gmail.com> writes:
> Common causes of pubkey access fail:
>
> - wrong pubkey being offered: if you are using ssh-agent, make sure
> you have 'ssh-add'ed the key you want to offer. Confirm with 'ssh-add
> -l'
A failure related to this I saw is to have (too) many keys in ssh-agent,
and running ssh without telling it which exact key to use. The client
tries each key in turn and the server rejects the connection attempt after
seeing too many keys tried. "ssh -v" is useful to diagnose this mode of
failure, and an entry in ~/.ssh/config like:
Host example.com
User myusernameoverthere
IdentityFile ~/.ssh/id_rsa-for-example.com
would fix it.
^ 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