* Re: [PATCH 2/2] gitweb: add a feature to show side-by-side diff
From: Jakub Narebski @ 2011-10-17 21:24 UTC (permalink / raw)
To: Kato Kazuyoshi; +Cc: git
In-Reply-To: <CAFo4x0L4BAKnCDa1uEK0Rskd9kTsR-94D4mkYKnLGqVDnuyuBA@mail.gmail.com>
Kato Kazuyoshi <kato.kazuyoshi@gmail.com> writes:
> gitweb currently has a feature to show diff but it doesn't support
> "side-by-side" style. This modification introduces:
>
> * The "ds" query parameter to specify the style of diff.
> * The format_diff_chunk() to reorganize an output of diff.
> * The diff_nav() for form.
I would state it a bit differently.
I would say that this patch introduces support for side-by-side diff
in git_patchset_body, and that style of diff is controlled by newly
introduces 'diff_style' ("ds") parameter.
I would say a bit later that navigation bar got extended to make it
easy to switch between 'inline' and 'sidebyside' diff.
> ---
> gitweb/gitweb.perl | 81 +++++++++++++++++++++++++++++++++++++++++----
> gitweb/static/gitweb.css | 15 ++++++++
> 2 files changed, 88 insertions(+), 8 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 095adda..dca09dc 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -757,6 +757,7 @@ our @cgi_param_mapping = (
> extra_options => "opt",
> search_use_regexp => "sr",
> ctag => "by_tag",
> + diff_style => "ds",
> # this must be last entry (for manipulation from JavaScript)
> javascript => "js"
> );
O.K.
> @@ -1072,6 +1073,8 @@ sub evaluate_and_validate_params {
> }
> $search_regexp = $search_use_regexp ? $searchtext : quotemeta $searchtext;
> }
> +
> + $input_params{diff_style} ||= 'inline';
> }
O.K.
> # path to the current git repository
> @@ -2276,7 +2279,7 @@ sub format_diff_line {
> }
> $line = "<span class=\"chunk_info\">@@ $from_text $to_text @@</span>" .
> "<span class=\"section\">" . esc_html($section, -nbsp=>1) .
> "</span>";
> - return "$div_open$line</div>\n";
> + return $diff_class, "$div_open$line</div>\n";
> } elsif ($from && $to && $line =~ m/^\@{3}/) {
> my ($prefix, $ranges, $section) = $line =~ m/^(\@+) (.*?) \@+(.*)$/;
> my (@from_text, @from_start, @from_nlines, $to_text, $to_start, $to_nlines);
If you are changing behavior of format_diff_line, by having it return
<class>, <formatted line> line pair, I think you should document it,
and change the name of function.
All format_* functions in gitweb return a single string, so that
print format_*(...);
works.
> @@ -4828,8 +4831,32 @@ sub git_difftree_body {
> print "</table>\n";
> }
>
It would be good idea to add some comment about this subroutine,
e.g. what parameters it accepts. And perhaps how it works, as it is
not obvious.
> +sub format_diff_chunk {
> + my @chunk = @_;
> +
> + my $first_class = $chunk[0]->[0];
> + my @partial = map { $_->[1] } grep { $_->[0] eq $first_class } @chunk;
> +
> + if (scalar @partial < scalar @chunk) {
> + return join '', ("<div class='chunk'><div class='old'>",
> + @partial,
> + "</div>",
> + "<div class='new'>",
> + (map {
> + $_->[1];
> + } @chunk[scalar @partial..scalar @chunk-1]),
> + "</div></div>");
> + } else {
> + return join '', ("<div class='chunk'><div class='",
> + ($first_class eq 'add' ? 'new' : 'old'),
> + "'>",
> + @partial,
> + "</div></div>");
> + }
> +}
> +
> sub git_patchset_body {
> - my ($fd, $difftree, $hash, @hash_parents) = @_;
> + my ($fd, $is_inline, $difftree, $hash, @hash_parents) = @_;
Hmmm... shouldn't changing git_patchset_body signature (calling
convention) be mentioned in commit message?
> my ($hash_parent) = $hash_parents[0];
>
> my $is_combined = (@hash_parents > 1);
> @@ -4940,12 +4967,31 @@ sub git_patchset_body {
>
> # the patch itself
> LINE:
> + my @chunk;
> while ($patch_line = <$fd>) {
> chomp $patch_line;
>
> next PATCH if ($patch_line =~ m/^diff /);
>
> - print format_diff_line($patch_line, \%from, \%to);
> + my ($class, $line) = format_diff_line($patch_line, \%from, \%to);
> + if ($is_inline) {
> + print $line;
> + } elsif ($class eq 'add' || $class eq 'rem') {
> + push @chunk, [ $class, $line ];
> + } else {
> + if (@chunk) {
> + print format_diff_chunk(@chunk);
> + @chunk = ();
> + } elsif ($class eq 'chunk_header') {
> + print $line;
> + } else {
> + print '<div class="chunk"><div class="old">',
> + $line,
> + '</div><div class="new">',
> + $line,
> + '</div></div>';
> + }
> + }
I wonder why you have output of context lines in side-by-side diff in
git_patchset_body, instead of having it all in format_diff_chunk.
Unless by chunk you mean something different than "unified format hunk"
that is defined e.g. in (diff.info)"Detailed Unified".
> }
>
> } continue {
> @@ -7053,7 +7099,8 @@ sub git_blobdiff {
> if ($format eq 'html') {
> print "<div class=\"page_body\">\n";
>
> - git_patchset_body($fd, [ \%diffinfo ], $hash_base, $hash_parent_base);
> + git_patchset_body($fd, $input_params{diff_style} eq 'inline',
> + [ \%diffinfo ], $hash_base, $hash_parent_base);
> close $fd;
>
> print "</div>\n"; # class="page_body"
> @@ -7078,6 +7125,22 @@ sub git_blobdiff_plain {
> git_blobdiff('plain');
> }
>
> +sub diff_nav {
> + my ($style) = @_;
> +
> + my %pairs = (inline => 'inline', 'sidebyside' => 'side by side');
> + join '', ($cgi->start_form({ method => 'get' }),
> + $cgi->hidden('p'),
> + $cgi->hidden('a'),
> + $cgi->hidden('h'),
> + $cgi->hidden('hp'),
> + $cgi->hidden('hb'),
> + $cgi->hidden('hpb'),
> + $cgi->popup_menu('ds', [keys %pairs], $style, \%pairs),
> + $cgi->submit('change'),
> + $cgi->end_form);
> +}
Why do you feel the need for form to select between diff formats,
instead of links switching between them, like for interactive and
non-interactive blame?
> +
> sub git_commitdiff {
> my %params = @_;
> my $format = $params{-format} || 'html';
> @@ -7230,7 +7293,8 @@ sub git_commitdiff {
> my $ref = format_ref_marker($refs, $co{'id'});
>
> git_header_html(undef, $expires);
> - git_print_page_nav('commitdiff','', $hash,$co{'tree'},$hash, $formats_nav);
> + git_print_page_nav('commitdiff','', $hash,$co{'tree'},$hash,
> + $formats_nav . diff_nav($input_params{diff_style}));
> git_print_header_div('commit', esc_html($co{'title'}) . $ref, $hash);
> print "<div class=\"title_text\">\n" .
> "<table class=\"object_header\">\n";
> @@ -7284,7 +7348,8 @@ sub git_commitdiff {
> $use_parents ? @{$co{'parents'}} : $hash_parent);
> print "<br/>\n";
>
> - git_patchset_body($fd, \@difftree, $hash,
> + git_patchset_body($fd, $input_params{diff_style} eq 'inline',
> + \@difftree, $hash,
> $use_parents ? @{$co{'parents'}} : $hash_parent);
> close $fd;
> print "</div>\n"; # class="page_body"
Only commitdiff? What about blobdiff?
> diff --git a/gitweb/static/gitweb.css b/gitweb/static/gitweb.css
> index 7d88509..dc84db2 100644
> --- a/gitweb/static/gitweb.css
> +++ b/gitweb/static/gitweb.css
> @@ -618,6 +618,21 @@ div.remote {
> cursor: pointer;
> }
>
> +/* side-by-side diff */
> +div.chunk {
> + overflow: hidden;
???
> +}
> +
> +div.chunk div.old {
> + float: left;
> + width: 50%;
> + overflow: hidden;
> +}
> +
> +div.chunk div.new {
> + margin-left: 50%;
> + width: 50%;
> +}
Hmmm... I think the way side-by-side diff is styled should be
explained in commit message, or in comments.
I also wonder if it wouldn't be simpler to use table here, instead of
fiddling with floats, widths and margins.
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH/RFC 1/2] gitweb: change format_diff_line() to remove leading SP from $diff_class
From: Junio C Hamano @ 2011-10-17 21:22 UTC (permalink / raw)
To: Jakub Narebski; +Cc: Kato Kazuyoshi, git
In-Reply-To: <m38voj72xy.fsf@localhost.localdomain>
Jakub Narebski <jnareb@gmail.com> writes:
> By the way, it is common to either have following patches to be chain
> reply to first patch, or better provide cover letter for patch series
> and have all patches be reply to cover letter.
It may be a good discipline for 14 patch series, but it doesn't matter for
something this small.
>> gitweb/gitweb.perl | 24 +++++++++++++-----------
>> 1 files changed, 13 insertions(+), 11 deletions(-)
>>
>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
>> index 85d64b2..095adda 100755
>> --- a/gitweb/gitweb.perl
>> +++ b/gitweb/gitweb.perl
>> @@ -2235,28 +2235,30 @@ sub format_diff_line {
>> # combined diff
>> my $prefix = substr($line, 0, scalar @{$from->{'href'}});
>> if ($line =~ m/^\@{3}/) {
>> - $diff_class = " chunk_header";
>> + $diff_class = "chunk_header";
>> } elsif ($line =~ m/^\\/) {
>> - $diff_class = " incomplete";
>> + $diff_class = "incomplete";
>> } elsif ($prefix =~ tr/+/+/) {
>> - $diff_class = " add";
>> + $diff_class = "add";
>> } elsif ($prefix =~ tr/-/-/) {
>> - $diff_class = " rem";
>> + $diff_class = "rem";
>> }
>
> Hmmm... that reminds me: this if-elsif chain is a bit ugly, but would
> be hard to replace without given ... when, I think.
I wasn't reading the existing context line, but now that you mention it,
they are not just ugly but are borderline of being incorrect (e.g. it does
not catch a broken hunk-header that begins with "@@@@" for a two-way
merge).
Assuming that $from->{'href'} has all the parents (i.e. 2 for two-way
merge), shouldn't the code be more like this?
# combined diff
my $num_sign = @{$from->{'href'}} + 1;
my @cc_classifier = (
["\@{$num_sign} ", "chunk_header"],
['\\', "incomplete"],
[" {$num_sign}", ""],
["[+ ]{$num_sign}", "add"],
["[- ]{$num_sign}", "rem"],
);
for my $cls (@cc_classifier) {
if ($line =~ /^$cls->[0]$/) {
$diff_class = $cls->[1];
last;
}
}
Also don't we want to use "context" or something for the css class for the
context lines, instead of assuming that we won't want to paint it in any
special color?
^ permalink raw reply
* Re: [PATCH 2/2] daemon: report permission denied error to clients
From: Junio C Hamano @ 2011-10-17 21:03 UTC (permalink / raw)
To: Jeff King; +Cc: Clemens Buchacher, git
In-Reply-To: <20111017020912.GB18536@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Mon, Oct 17, 2011 at 12:11:16AM +0200, Clemens Buchacher wrote:
>
> I like the intent. This actually does leak a little more information
> than the existing --informative-errors, as before you couldn't tell the
> difference between "not found" and "not exported".
I personally think this is going a bit too far, even for "informative"
option, by allowing to fish for possible list of usernames. It would make
it a tougher sell to later default to "informative", I am afraid.
Suppose you are setting up your own repository (either on your own box or
on a box with a separate administrator), and youare wondering why your
attempted access failed. You know /pub/repo/sito.git exists (you created
it after all) and you get "no such repository: /repo/sito.git" when you
ran:
$ git clone git://host/repo/sito.git/
If you have another repository in /pub/repo/ that does already work, and
if you know /pub/repo/sito.git/ is fine locally (e.g. you can see local
command like "git log" works fine there), then even if you see "not found"
you would know to compare what the difference between these two are.
If there is no other repositories in /pub/repo/ or you are setting up many
repositories on this same box for the first time, wouldn't it be plausible
that you _are_ the administrator of the box and have access to the daemon
log to diagnose the problem more easily anyway?
I can see how this is "leaking a little more information", but I am not
convinced that leak is helping legit users more than helping unwanted
snoopers.
> The new calling conventions for this function seem a little weird. I
> would expect either "return negative, and set errno" for usual library
> code, or possibly "return negative error value". But "return -1, or a
> positive error code" seems unusual to me.
>
> One of:
>
> errno = EACCESS;
> return -1;
>
> or
>
> return -EACCESS;
>
> would be more idiomatic, I think.
Yes, the former would probably be easier to handle.
^ permalink raw reply
* Re: [PATCH] use test number as port number
From: Junio C Hamano @ 2011-10-17 20:57 UTC (permalink / raw)
To: Clemens Buchacher; +Cc: Jeff King, git, Junio C Hamano
In-Reply-To: <20111017195547.GB29479@ecki>
Clemens Buchacher <drizzd@aon.at> writes:
> Test 5550 was apparently using the default port number by mistake.
>
> Signed-off-by: Clemens Buchacher <drizzd@aon.at>
> ---
>
> On Sun, Oct 16, 2011 at 10:01:03PM -0400, Jeff King wrote:
>>
>> LIB_DAEMON_PORT=${LIB_DAEMON_PORT-'5570'}
>
> Thanks, I missed that.
>
> Clemens
>
> t/t5550-http-fetch.sh | 2 +-
> t/t5570-git-daemon.sh | 1 +
> 2 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh
> index a1883ca..8a77750 100755
> --- a/t/t5550-http-fetch.sh
> +++ b/t/t5550-http-fetch.sh
> @@ -8,8 +8,8 @@ if test -n "$NO_CURL"; then
> test_done
> fi
>
> -. "$TEST_DIRECTORY"/lib-httpd.sh
> LIB_HTTPD_PORT=${LIB_HTTPD_PORT-'5550'}
> +. "$TEST_DIRECTORY"/lib-httpd.sh
> start_httpd
Good eyes. This is the only one in the 55xx series that gets the order
wrong.
I'll drop the patch to 5570 for now as that should be done in the change
that is still not in 'next' that adds 5570.
I've fixed and queued the previous one as aa0b028 (daemon: add tests,
2011-10-17); does that look good enough?
> diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
> index e6482eb..a92d996 100755
> --- a/t/t5570-git-daemon.sh
> +++ b/t/t5570-git-daemon.sh
> @@ -3,6 +3,7 @@
> test_description='test fetching over git protocol'
> . ./test-lib.sh
>
> +LIB_DAEMON_PORT=${LIB_DAEMON_PORT-'5570'}
> . "$TEST_DIRECTORY"/lib-daemon.sh
> start_daemon
^ permalink raw reply
* Re: [PATCH/RFC 1/2] gitweb: change format_diff_line() to remove leading SP from $diff_class
From: Jakub Narebski @ 2011-10-17 20:56 UTC (permalink / raw)
To: Kato Kazuyoshi; +Cc: git, Jakub Narebski
In-Reply-To: <CAFo4x0LP4fXgSNAnss_WRLo-TH_qe=esYn7P+=iS6t87tdzcbw@mail.gmail.com>
By the way, it is common to either have following patches to be chain
reply to first patch, or better provide cover letter for patch series
and have all patches be reply to cover letter.
Kato Kazuyoshi <kato.kazuyoshi@gmail.com> writes:
This commit needs some more explanation in the commit message, like
Junio said.
> The format_diff_line() will return $diff_class and HTML in upcoming changes.
This is not necessary, I think; this change stands alone as a style
(semantic) cleanup.
Signoff? See Documentation/SubmittingPatches
> ---
> gitweb/gitweb.perl | 24 +++++++++++++-----------
> 1 files changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 85d64b2..095adda 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -2235,28 +2235,30 @@ sub format_diff_line {
> # combined diff
> my $prefix = substr($line, 0, scalar @{$from->{'href'}});
> if ($line =~ m/^\@{3}/) {
> - $diff_class = " chunk_header";
> + $diff_class = "chunk_header";
> } elsif ($line =~ m/^\\/) {
> - $diff_class = " incomplete";
> + $diff_class = "incomplete";
> } elsif ($prefix =~ tr/+/+/) {
> - $diff_class = " add";
> + $diff_class = "add";
> } elsif ($prefix =~ tr/-/-/) {
> - $diff_class = " rem";
> + $diff_class = "rem";
> }
Hmmm... that reminds me: this if-elsif chain is a bit ugly, but would
be hard to replace without given ... when, I think.
> } else {
> # assume ordinary diff
> my $char = substr($line, 0, 1);
> if ($char eq '+') {
> - $diff_class = " add";
> + $diff_class = "add";
> } elsif ($char eq '-') {
> - $diff_class = " rem";
> + $diff_class = "rem";
> } elsif ($char eq '@') {
> - $diff_class = " chunk_header";
> + $diff_class = "chunk_header";
> } elsif ($char eq "\\") {
> - $diff_class = " incomplete";
> + $diff_class = "incomplete";
> }
This is also ugly, but this one could in theory be replaced by hash
lookup. But this would remove symmetry...
> }
> $line = untabify($line);
> +
> + my $div_open = '<div class="' . (join ' ', ('diff', $diff_class)) . '">';
Another solution would be to use
+
+ my $diff_classes = "diff";
+ $diff_classes .= " $diff_class" if ($diff_class);
+
> if ($from && $to && $line =~ m/^\@{2} /) {
> my ($from_text, $from_start, $from_lines, $to_text, $to_start,
> $to_lines, $section) =
> $line =~ m/^\@{2} (-(\d+)(?:,(\d+))?) (\+(\d+)(?:,(\d+))?) \@{2}(.*)$/;
> @@ -2274,7 +2276,7 @@ sub format_diff_line {
> }
> $line = "<span class=\"chunk_info\">@@ $from_text $to_text @@</span>" .
> "<span class=\"section\">" . esc_html($section, -nbsp=>1) .
> "</span>";
The above is word-wrapped. Please turn off word wrapping in the
future to avoid such damage to the patch.
> - return "<div class=\"diff$diff_class\">$line</div>\n";
> + return "$div_open$line</div>\n";
This would be instead
+ return "<div class=\"$diff_classes\">$line</div>\n";
which is a bit more symmetric.
But that is just a matter of taste.
> } elsif ($from && $to && $line =~ m/^\@{3}/) {
> my ($prefix, $ranges, $section) = $line =~ m/^(\@+) (.*?) \@+(.*)$/;
> my (@from_text, @from_start, @from_nlines, $to_text, $to_start, $to_nlines);
> @@ -2307,9 +2309,9 @@ sub format_diff_line {
> }
> $line .= " $prefix</span>" .
> "<span class=\"section\">" . esc_html($section, -nbsp=>1)
> . "</span>";
> - return "<div class=\"diff$diff_class\">$line</div>\n";
> + return "$div_open$line</div>\n";
> }
> - return "<div class=\"diff$diff_class\">" . esc_html($line, -nbsp=>1)
> . "</div>\n";
> + return $div_open . esc_html($line, -nbsp=>1) . "</div>\n";
> }
>
> # Generates undef or something like "_snapshot_" or "snapshot (_tbz2_ _zip_)",
> --
> 1.7.7.213.g8b0e1.dirty
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH/RFC 3/2] Refactor Git::config_*
From: Junio C Hamano @ 2011-10-17 20:50 UTC (permalink / raw)
To: Jakub Narebski; +Cc: Eric Wong, Cord Seele, Matthieu Moy, git, Cord Seele
In-Reply-To: <201110072317.17436.jnareb@gmail.com>
Jakub Narebski <jnareb@gmail.com> writes:
> This is version which has fixed style to be more Perl-ish, and which
> actually works (i.e. t9700 passes).
>
> I have also moved _config_common() after commands that use it, just like
> it is done with other "private" methods (methods with names starting with
> '_'), and excluded this private detail of implementation from docs.
It seems that this breaks many tests in t9xxx series for me, especially
the 9100 series that cover git-svn.
^ permalink raw reply
* [PATCH] Allow to specify the editor used for git rebase -i by config/environment var
From: Peter Oberndorfer @ 2011-10-17 20:26 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Phil Hord
The search order for choosing the sequence editor is:
$GIT_SEQUENCE_EDITOR
git config sequence.editor
git var GIT_EDITOR (default editor for commit messages)
With this change is it possible to have a separate
(possibly graphical) editor that helps the user
during a interactive rebase.
Using $GIT_EDITOR or core.editor config var for this is not possible
since they is also used to start the commit message editor for reword action.
Signed-off-by: Peter Oberndorfer <kumbayo84@arcor.de>
---
I reworded the commit message and the config description a bit.
renamed to sequence.editor / $GIT_SEQUENCE_EDITOR
and moved the helper to git-rebase--interactive.sh
Documentation/config.txt | 7 +++++++
git-rebase--interactive.sh | 15 ++++++++++++++-
2 files changed, 21 insertions(+), 1 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 03296b7..048c5f9 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -473,6 +473,13 @@ core.editor::
variable when it is set, and the environment variable
`GIT_EDITOR` is not set. See linkgit:git-var[1].
+sequence.editor::
+ Text editor used by git rebase -i for editing the rebase todo file.
+ The value is meant to be interpreted by the shell when it is used.
+ It can be overridden by the 'GIT_SEQUENCE_EDITOR' environment variable.
+ When not configured the default commit message editor is used instead.
+ See linkgit:git-var[1]
+
core.pager::
The command that git will use to paginate output. Can
be overridden with the `GIT_PAGER` environment
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 94f36c2..13a0661 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -161,6 +161,19 @@ do_with_author () {
)
}
+git_sequence_editor() {
+ if test -z "${GIT_SEQUENCE_EDITOR:+set}"
+ then
+ GIT_SEQUENCE_EDITOR="$(git config sequence.editor)"
+ if [ -z "$GIT_SEQUENCE_EDITOR" ]
+ then
+ GIT_SEQUENCE_EDITOR="$(git var GIT_EDITOR)" || return $?
+ fi
+ fi
+
+ eval "$GIT_SEQUENCE_EDITOR" '"$@"'
+}
+
pick_one () {
ff=--ff
case "$1" in -n) sha1=$2; ff= ;; *) sha1=$1 ;; esac
@@ -832,7 +845,7 @@ has_action "$todo" ||
die_abort "Nothing to do"
cp "$todo" "$todo".backup
-git_editor "$todo" ||
+git_sequence_editor "$todo" ||
die_abort "Could not execute editor"
has_action "$todo" ||
--
1.7.7.329.g2140c
^ permalink raw reply related
* Re: [PATCH 1/2] daemon: add tests
From: Jeff King @ 2011-10-17 20:08 UTC (permalink / raw)
To: Clemens Buchacher; +Cc: git, Junio C Hamano
In-Reply-To: <20111017200528.GA19054@ecki>
On Mon, Oct 17, 2011 at 10:05:28PM +0200, Clemens Buchacher wrote:
> On Sun, Oct 16, 2011 at 10:01:03PM -0400, Jeff King wrote:
> >
> > Thanks, it's nice to have some tests. Overall, some of the tests feel a
> > little silly, because the results should be exactly the same as fetching
> > or pushing a local repository (so the "set-head" thing, for example,
> > really has little to do with git-daemon).
>
> Hmm, yes. Actually, I thought I had found a bug with the failure of
> "set-head -a". But now I see that in t5505 this treated like a
> feature.
It's not a feature, exactly. It's just documenting that we fail in the
face of ambiguous HEADs. Arguably, the test should be switched to use
text_expect_failure to document that we would prefer it the other way,
but it doesn't work now.
> Would it be difficult to support this over the git protocol? Maybe
> I will have a look.
It needs a protocol extension to communicate symbolic ref destinations.
The topic has come up a few times, and I think Junio even had patches at
one point.
-Peff
^ permalink raw reply
* Re: [PATCH 1/2] daemon: add tests
From: Clemens Buchacher @ 2011-10-17 20:05 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano
In-Reply-To: <20111017020103.GA18536@sigill.intra.peff.net>
On Sun, Oct 16, 2011 at 10:01:03PM -0400, Jeff King wrote:
>
> Thanks, it's nice to have some tests. Overall, some of the tests feel a
> little silly, because the results should be exactly the same as fetching
> or pushing a local repository (so the "set-head" thing, for example,
> really has little to do with git-daemon).
Hmm, yes. Actually, I thought I had found a bug with the failure of
"set-head -a". But now I see that in t5505 this treated like a
feature.
Would it be difficult to support this over the git protocol? Maybe
I will have a look.
Clemens
^ permalink raw reply
* [PATCH v2 2/2] daemon: report permission denied error to clients
From: Clemens Buchacher @ 2011-10-17 19:58 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <1318803076-4229-2-git-send-email-drizzd@aon.at>
If passed an inaccessible url, git daemon returns the
following error:
$ git clone git://host/repo
fatal: remote error: no such repository: /repo
In case of a permission denied error, return the following
instead:
fatal: remote error: permission denied: /repo
Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
Compared to v1 of this patch, the calling convention of path_ok are
back to what they were previously. Now the only change is that it
sets errno.
daemon.c | 15 +++++++++++++--
path.c | 31 +++++++++++++++++++++----------
t/t5570-git-daemon.sh | 2 +-
3 files changed, 35 insertions(+), 13 deletions(-)
diff --git a/daemon.c b/daemon.c
index 72fb53a..2f7f84e 100644
--- a/daemon.c
+++ b/daemon.c
@@ -120,12 +120,14 @@ static char *path_ok(char *directory)
if (daemon_avoid_alias(dir)) {
logerror("'%s': aliased", dir);
+ errno = 0;
return NULL;
}
if (*dir == '~') {
if (!user_path) {
logerror("'%s': User-path not allowed", dir);
+ errno = EACCES;
return NULL;
}
if (*user_path) {
@@ -158,6 +160,7 @@ static char *path_ok(char *directory)
if (*dir != '/') {
/* Allow only absolute */
logerror("'%s': Non-absolute path denied (interpolated-path active)", dir);
+ errno = EACCES;
return NULL;
}
@@ -173,6 +176,7 @@ static char *path_ok(char *directory)
if (*dir != '/') {
/* Allow only absolute */
logerror("'%s': Non-absolute path denied (base-path active)", dir);
+ errno = EACCES;
return NULL;
}
snprintf(rpath, PATH_MAX, "%s%s", base_path, dir);
@@ -190,7 +194,9 @@ static char *path_ok(char *directory)
}
if (!path) {
+ int err = errno;
logerror("'%s' does not appear to be a git repository", dir);
+ errno = err;
return NULL;
}
@@ -221,6 +227,7 @@ static char *path_ok(char *directory)
}
logerror("'%s': not in whitelist", path);
+ errno = EACCES;
return NULL; /* Fallthrough. Deny by default */
}
@@ -269,8 +276,12 @@ static int run_service(char *dir, struct daemon_service *service)
return daemon_error(dir, "service not enabled");
}
- if (!(path = path_ok(dir)))
- return daemon_error(dir, "no such repository");
+ if (!(path = path_ok(dir))) {
+ if (errno == EACCES)
+ return daemon_error(dir, "permission denied");
+ else
+ return daemon_error(dir, "no such repository");
+ }
/*
* Security on the cheap.
diff --git a/path.c b/path.c
index 6f3f5d5..227d8d7 100644
--- a/path.c
+++ b/path.c
@@ -288,6 +288,7 @@ char *enter_repo(char *path, int strict)
static char used_path[PATH_MAX];
static char validated_path[PATH_MAX];
+ errno = 0;
if (!path)
return NULL;
@@ -301,12 +302,15 @@ char *enter_repo(char *path, int strict)
path[len-1] = 0;
len--;
}
- if (PATH_MAX <= len)
+ if (PATH_MAX <= len) {
+ errno = ENAMETOOLONG;
return NULL;
+ }
if (path[0] == '~') {
char *newpath = expand_user_path(path);
if (!newpath || (PATH_MAX - 10 < strlen(newpath))) {
free(newpath);
+ errno = 0;
return NULL;
}
/*
@@ -319,9 +323,10 @@ char *enter_repo(char *path, int strict)
strcpy(validated_path, path);
path = used_path;
}
- else if (PATH_MAX - 10 < len)
+ else if (PATH_MAX - 10 < len) {
+ errno = ENAMETOOLONG;
return NULL;
- else {
+ } else {
path = strcpy(used_path, path);
strcpy(validated_path, path);
}
@@ -331,23 +336,29 @@ char *enter_repo(char *path, int strict)
if (!access(path, F_OK)) {
strcat(validated_path, suffix[i]);
break;
+ } else if (errno == EACCES) {
+ return NULL;
}
}
- if (!suffix[i] || chdir(path))
+ if (!suffix[i])
+ return NULL;
+ if (chdir(path))
return NULL;
path = validated_path;
}
else if (chdir(path))
return NULL;
- if (access("objects", X_OK) == 0 && access("refs", X_OK) == 0 &&
- validate_headref("HEAD") == 0) {
- set_git_dir(".");
- check_repository_format();
- return path;
+ if (access("objects", X_OK) || access("refs", X_OK))
+ return NULL;
+ if (validate_headref("HEAD")) {
+ errno = 0;
+ return NULL;
}
- return NULL;
+ set_git_dir(".");
+ check_repository_format();
+ return path;
}
int set_shared_perm(const char *path, int mode)
diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
index aa5771a..e6482eb 100755
--- a/t/t5570-git-daemon.sh
+++ b/t/t5570-git-daemon.sh
@@ -141,7 +141,7 @@ start_daemon --informative-errors
test_expect_success 'clone non-existent' "test_remote_error clone nowhere.git 'no such repository'"
test_expect_success 'push disabled' "test_remote_error push repo.git 'service not enabled'"
-test_expect_success 'read access denied' "test_remote_error -x fetch repo.git 'no such repository'"
+test_expect_success 'read access denied' "test_remote_error -x fetch repo.git 'permission denied'"
test_expect_success 'not exported' "test_remote_error -n fetch repo.git 'repository not exported'"
stop_daemon
--
1.7.7
^ permalink raw reply related
* [PATCH] use test number as port number
From: Clemens Buchacher @ 2011-10-17 19:55 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano
In-Reply-To: <20111017020103.GA18536@sigill.intra.peff.net>
Test 5550 was apparently using the default port number by mistake.
Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
On Sun, Oct 16, 2011 at 10:01:03PM -0400, Jeff King wrote:
>
> LIB_DAEMON_PORT=${LIB_DAEMON_PORT-'5570'}
Thanks, I missed that.
Clemens
t/t5550-http-fetch.sh | 2 +-
t/t5570-git-daemon.sh | 1 +
2 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh
index a1883ca..8a77750 100755
--- a/t/t5550-http-fetch.sh
+++ b/t/t5550-http-fetch.sh
@@ -8,8 +8,8 @@ if test -n "$NO_CURL"; then
test_done
fi
-. "$TEST_DIRECTORY"/lib-httpd.sh
LIB_HTTPD_PORT=${LIB_HTTPD_PORT-'5550'}
+. "$TEST_DIRECTORY"/lib-httpd.sh
start_httpd
test_expect_success 'setup repository' '
diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
index e6482eb..a92d996 100755
--- a/t/t5570-git-daemon.sh
+++ b/t/t5570-git-daemon.sh
@@ -3,6 +3,7 @@
test_description='test fetching over git protocol'
. ./test-lib.sh
+LIB_DAEMON_PORT=${LIB_DAEMON_PORT-'5570'}
. "$TEST_DIRECTORY"/lib-daemon.sh
start_daemon
--
1.7.7
^ permalink raw reply related
* Re: [PATCH 2/2] daemon: report permission denied error to clients
From: Jeff King @ 2011-10-17 19:51 UTC (permalink / raw)
To: Clemens Buchacher; +Cc: git, Junio C Hamano
In-Reply-To: <20111017194821.GA29479@ecki>
On Mon, Oct 17, 2011 at 09:48:21PM +0200, Clemens Buchacher wrote:
> > I like the intent. This actually does leak a little more information
> > than the existing --informative-errors, as before you couldn't tell the
> > difference between "not found" and "not exported".
>
> I think you mean that before, you couldn't tell the difference
> between "not found" and "permission denied".
Ah, right. Sorry, I was thinking path_ok handled the export-ok flag, but
I already handled it in my patch to run_service. So it is leaking a
little more, but even less than I indicated. And at any rate, I think it
is consistent with what --informative-errors is meant to do, so it's a
good change.
> > The new calling conventions for this function seem a little weird. I
> > would expect either "return negative, and set errno" for usual library
> > code, or possibly "return negative error value". But "return -1, or a
> > positive error code" seems unusual to me.
>
> Yes indeed, will fix.
Thanks.
-Peff
^ permalink raw reply
* Re: [PATCH 2/2] daemon: report permission denied error to clients
From: Clemens Buchacher @ 2011-10-17 19:48 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano
In-Reply-To: <20111017020912.GB18536@sigill.intra.peff.net>
On Sun, Oct 16, 2011 at 10:09:12PM -0400, Jeff King wrote:
> On Mon, Oct 17, 2011 at 12:11:16AM +0200, Clemens Buchacher wrote:
>
> > If passed an inaccessible url, git daemon returns the
> > following error:
> >
> > $ git clone git://host/repo
> > fatal: remote error: no such repository: /repo
> >
> > In case of a permission denied error, return the following
> > instead:
> >
> > fatal: remote error: permission denied: /repo
> >
> > Signed-off-by: Clemens Buchacher <drizzd@aon.at>
> > ---
>
> I like the intent. This actually does leak a little more information
> than the existing --informative-errors, as before you couldn't tell the
> difference between "not found" and "not exported".
I think you mean that before, you couldn't tell the difference
between "not found" and "permission denied".
> > -static char *path_ok(char *directory)
> > +static int path_ok(char *directory, const char **return_path)
> > {
> > static char rpath[PATH_MAX];
> > static char interp_path[PATH_MAX];
> > @@ -120,13 +120,13 @@ static char *path_ok(char *directory)
> >
> > if (daemon_avoid_alias(dir)) {
> > logerror("'%s': aliased", dir);
> > - return NULL;
> > + return -1;
> > }
> >
> > if (*dir == '~') {
> > if (!user_path) {
> > logerror("'%s': User-path not allowed", dir);
> > - return NULL;
> > + return EACCES;
>
> The new calling conventions for this function seem a little weird. I
> would expect either "return negative, and set errno" for usual library
> code, or possibly "return negative error value". But "return -1, or a
> positive error code" seems unusual to me.
Yes indeed, will fix.
Clemens
^ permalink raw reply
* Re: [PATCH v4 0/7] Provide API to invalidate refs cache
From: Michael Haggerty @ 2011-10-17 19:43 UTC (permalink / raw)
To: Heiko Voigt
Cc: Junio C Hamano, git, Jeff King, Drew Northup, Jakub Narebski,
Johan Herland, Julian Phillips
In-Reply-To: <20111017184708.GB2540@sandbox-rc>
On 10/17/2011 08:47 PM, Heiko Voigt wrote:
> On Mon, Oct 17, 2011 at 04:38:04AM +0200, mhagger@alum.mit.edu wrote:
>> [1] http://marc.info/?l=git&m=131827641227965&w=2
>> In this mailing list thread, Heiko Voigt stated that git-submodule
>> does not modify any references, so it should not have to use the
>> API.
>
> This is not entirely true. I was saying that my submodule-merge code is
> currently the only one using the refs api for submodules and that does
> not need to modify submodule refs. I imagine that there will be some
> users when submodule support matures (e.g. recursive push).
Sorry for misunderstanding/misrepresenting you; thanks for the
clarification.
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply
* Re: [PATCH 2/2] gitweb: add a feature to show side-by-side diff
From: Junio C Hamano @ 2011-10-17 19:37 UTC (permalink / raw)
To: Kato Kazuyoshi; +Cc: git
In-Reply-To: <CAFo4x0L4BAKnCDa1uEK0Rskd9kTsR-94D4mkYKnLGqVDnuyuBA@mail.gmail.com>
Kato Kazuyoshi <kato.kazuyoshi@gmail.com> writes:
> @@ -2276,7 +2279,7 @@ sub format_diff_line {
> }
> $line = "<span class=\"chunk_info\">@@ $from_text $to_text @@</span>" .
> "<span class=\"section\">" . esc_html($section, -nbsp=>1) .
> "</span>";
Corrupt patch (wrapped long line); please fix.
I suspect you have similar issues in the [PATCH 1/2] message, too.
^ permalink raw reply
* Re: Re: [PATCH 3/3] git-gui: new config to control staging of untracked files
From: Heiko Voigt @ 2011-10-17 19:27 UTC (permalink / raw)
To: Bert Wesarg; +Cc: Pat Thoyts, git
In-Reply-To: <CAKPyHN3pKUSLTs8_5QMo5i+=3w7KXAHJjDOfQ1XYG92ZbQ1SeA@mail.gmail.com>
Hi,
On Mon, Oct 17, 2011 at 08:47:50PM +0200, Bert Wesarg wrote:
> On Mon, Oct 17, 2011 at 20:34, Heiko Voigt <hvoigt@hvoigt.net> wrote:
> > Here I am wondering whether we have a similar mechanism in git gui like
> > in core git that makes yes,true,1 equivalents (and similar with other
> > values) ?
>
> But it is not only yes,true,1 or no,false,0 its a tristate with the
> third state 'ask'. For booleans, there is such functionality in git
> gui. See is_config_true and is_config_false. Reusing these for this
> tristate wouldn't work. The current check here is indeed very strict
> and should be loosen by at least ignoring the case, surrounding
> spaces, and allow also true/false. But also note, that this variable
> can be set via the Options menu, so you can't mistype it.
Well if using git config you can ;-). I just wanted to ask whether we
may already have machinery which supports such tristate.
If we do not I think the current "strict" configuration is fine. In most
cases the user will use the gui itself to configure such behavior so
thats no big deal.
If someone needs that it can be added later on.
Thanks, Heiko
^ permalink raw reply
* Re: [PATCH v2 00/14] Tidying up references code
From: Junio C Hamano @ 2011-10-17 19:12 UTC (permalink / raw)
To: mhagger
Cc: git, Jeff King, Drew Northup, Jakub Narebski, Heiko Voigt,
Johan Herland, Julian Phillips
In-Reply-To: <1318837163-27112-1-git-send-email-mhagger@alum.mit.edu>
mhagger@alum.mit.edu writes:
> From: Michael Haggerty <mhagger@alum.mit.edu>
>
> Patch series re-rolled against v4 of "Provide API to invalidate refs
> cache"...
Thanks; queued (but not pushed out yet).
> BTW, whenever I add comments to existing code, it is just an attempt
> to record information that I have inferred from reverse-engineering.
Thanks again. I often find me scratching head while reading other people's
code, long after I reviewed (or read other's reviews) and accepted their
patches. It often is not the lack of review that caused undercommented
code to get in my tree. During the review process, the issue the code is
trying to solve is so fresh in everybody's mind, that certain things do
not need to be explained to be understood. But that kind of memory
eventually fades and only the code remains.
It is a rather unfortunate result of the human nature that the next person
who touches that code is in the best position to find out what aspect of
the code is hard to understand and deserves comment.
^ permalink raw reply
* Re: [PATCH/RFC 1/2] gitweb: change format_diff_line() to remove leading SP from $diff_class
From: Junio C Hamano @ 2011-10-17 19:02 UTC (permalink / raw)
To: Kato Kazuyoshi; +Cc: git
In-Reply-To: <CAFo4x0LP4fXgSNAnss_WRLo-TH_qe=esYn7P+=iS6t87tdzcbw@mail.gmail.com>
Kato Kazuyoshi <kato.kazuyoshi@gmail.com> writes:
> The format_diff_line() will return $diff_class and HTML in upcoming changes.
An auxiliary piece of information like this is fine at the end of the
commit log message, but the patch itself wants to be justified
standalone. Perhaps this should be sufficient:
The $diff_class variable to classify the kind of line in the diff
output was prefixed with a SP, only so that the code to synthesize
value for "class" attribute can blindly concatenate it with
another value "diff". This made the code unnecessarily ugly.
Instead, add SP that separates the value of $diff_class from
another class value "diff" where <div class="..."> string is
created and drop the leading SP from the value of $diff_class.
Explained this way, it does not even have to mention that the return value
will be changed in a different patch.
We all know that the hidden motivation of this change is that the caller
of this function, after it starts using the returned value of $diff_class,
does not want to worry about the ugly SP prefix in that variable. Saying
only "This function will return this variable in the future" still does
not fully explain that hidden motivation unless you say "and the caller
shouldn't have to worry about the leading SP", so let's not even mention
it in the log message of this change.
> ---
Sign-off?
> gitweb/gitweb.perl | 24 +++++++++++++-----------
> 1 files changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 85d64b2..095adda 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -2235,28 +2235,30 @@ sub format_diff_line {
> ...
> +
> + my $div_open = '<div class="' . (join ' ', ('diff', $diff_class)) . '">';
I think using a separate helper variable like this is a good change. You
do not have to worry about the issue in three different places.
But doesn't join(" ", ("frotz", "")) still give you "frotz "? It is OK to
punt and say
my $div_open = '<div class="diff $diff_class">';
which would be far easier to read. It may sacrifice a bit of tidiness in
the resulting HTML but the tidiness of the source outweighs it.
Of course, if you have tons of classes, it may be worth doing something
like
join(" ", grep { defined $_ && $_ ne ""} @diff_classes);
but I do not think it is worth it in this particular case.
Thanks.
^ permalink raw reply
* Re: [PATCH] Git-p4: git-p4.changeOnSubmit to do 'change' instead of 'submit'.
From: Luke Diamand @ 2011-10-17 18:53 UTC (permalink / raw)
To: Andrei Warkentin; +Cc: git, gitster, Pete Wyckoff, Andrei Warkentin
In-Reply-To: <83923897.7841.1318868319131.JavaMail.root@zimbra-prod-mbox-2.vmware.com>
On 17/10/11 17:18, Andrei Warkentin wrote:
> Hi,
>
> ----- Original Message -----
> Anyway, the other suggestion I had was to create a new command
> instead of overriding behaviour of an existing one. Of course,
> copy-pasting P4Submit into P4Change is silly, so...
>
> How about something like this?
>
> The commands dict maps command name to class and optional dict passed to cmd.run(). That way 'change'
> can really mean P4Submit with an extra parameter not to submit but to do a changelist instead. The
> reason why I initially made the config flag was because I didn't want to copy-paste P4Submit into P4Change.
>
> commands = {
> "debug" : [ P4Debug, {} ]
> "submit" : [ P4Submit, { "doChange" : 0 } ]
> "commit" : [ P4Submit, { "doChange" : 0 } ]
> "change" : [ P4Submit, { "doChange" : 1 } ]
> "sync" : [ P4Sync, {} ],
> "rebase" : [ P4Rebase, {} ],
> "clone" : [ P4Clone, {} ],
> "rollback" : [ P4RollBack, {} ],
> "branches" : [ P4Branches, {} ]
> }
>
> Thanks for the review,
> A
>
Sounds plausible to me. The alternative would be a command line
parameter, although that could get annoying and error prone, especially
as you can't easily unsubmit a perforce change.
^ permalink raw reply
* Re: [PATCH 3/3] git-gui: new config to control staging of untracked files
From: Bert Wesarg @ 2011-10-17 18:47 UTC (permalink / raw)
To: Heiko Voigt; +Cc: Pat Thoyts, git
In-Reply-To: <20111017183430.GA2540@sandbox-rc>
On Mon, Oct 17, 2011 at 20:34, Heiko Voigt <hvoigt@hvoigt.net> wrote:
> Hi,
>
> what the series tries to achieve looks good to me. Just one comment.
Thanks.
>
> On Fri, Oct 14, 2011 at 09:25:21PM +0200, Bert Wesarg wrote:
> [...]
>> diff --git a/lib/index.tcl b/lib/index.tcl
>> index 014acf9..45094c2 100644
>> --- a/lib/index.tcl
>> +++ b/lib/index.tcl
>> @@ -367,7 +367,19 @@ proc do_add_all {} {
>> }
>> }
>> if {[llength $untracked_paths]} {
>> - set reply [ask_popup [mc "Stage also untracked files?"]]
>> + set reply 0
>> + switch -- [get_config gui.stageuntracked] {
>> + no {
>> + set reply 0
>> + }
> [...]
>
> Here I am wondering whether we have a similar mechanism in git gui like
> in core git that makes yes,true,1 equivalents (and similar with other
> values) ?
But it is not only yes,true,1 or no,false,0 its a tristate with the
third state 'ask'. For booleans, there is such functionality in git
gui. See is_config_true and is_config_false. Reusing these for this
tristate wouldn't work. The current check here is indeed very strict
and should be loosen by at least ignoring the case, surrounding
spaces, and allow also true/false. But also note, that this variable
can be set via the Options menu, so you can't mistype it.
Bert
> If we don't I think the series is fine as it is otherwise it
> probably makes sense to use that mechanism.
>
> Cheers Heiko
>
^ permalink raw reply
* Re: [PATCH v4 0/7] Provide API to invalidate refs cache
From: Heiko Voigt @ 2011-10-17 18:47 UTC (permalink / raw)
To: mhagger
Cc: Junio C Hamano, git, Jeff King, Drew Northup, Jakub Narebski,
Johan Herland, Julian Phillips
In-Reply-To: <1318819091-7066-1-git-send-email-mhagger@alum.mit.edu>
Hi,
On Mon, Oct 17, 2011 at 04:38:04AM +0200, mhagger@alum.mit.edu wrote:
> I won't myself have time to figure out who, outside of refs.c, has to
> *call* invalidate_ref_cache(). The candidates that I know off the top
> of my head are git-clone, git-submodule [1], and git-pack-refs. It
> would be great if experts in those areas would insert calls to
> invalidate_ref_cache() where needed.
[...]
> [1] http://marc.info/?l=git&m=131827641227965&w=2
> In this mailing list thread, Heiko Voigt stated that git-submodule
> does not modify any references, so it should not have to use the
> API.
This is not entirely true. I was saying that my submodule-merge code is
currently the only one using the refs api for submodules and that does
not need to modify submodule refs. I imagine that there will be some
users when submodule support matures (e.g. recursive push).
Cheers Heiko
^ permalink raw reply
* [PATCH] resolve_gitlink_packed_ref(): fix mismerge
From: Junio C Hamano @ 2011-10-17 18:43 UTC (permalink / raw)
To: Michael Haggerty; +Cc: Mark Levedahl, git
In-Reply-To: <7v4nz7o7mg.fsf@alter.siamese.dyndns.org>
2c5c66b (Merge branch 'jp/get-ref-dir-unsorted', 2011-10-10) merged a
topic that forked from the mainline before a new helper function
get_packed_refs() refactored code to read packed-refs file. The merge made
the call to the helper function with an incorrect argument. The parameter
to the function has to be a path to the submodule.
Fix the mismerge.
Helped-by: Mark Levedahl <mlevedahl@gmail.com>
Helped-by: Michael Haggerty <mhagger@alum.mit.edu>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Junio C Hamano <gitster@pobox.com> writes:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> ...
>> The problem is that the parameter "name" is not NUL-terminated. The old
>> code turned it into a (NUL-terminated) filename via
>>
>> strcpy(name + pathlen, "packed-refs");
>>
>> but the new code passes it (unterminated) to get_packed_refs()
Let's do this on the 'master' front while mh/refs-api and mh/refs-api-2
(the new 14 patch series) are cooking in the 'next' branch. The added
test does not pass until the second-to-last patch in your new series.
I made trial merges of this with mh/refs-api and mh/refs-api-2 and both
were trivial to resolve (merge with mh/refs-api will keep the fix, and
merge with mh/refs-api-2 can be made by dropping this change), so this is
purely as interim fix plus---the value of the patch lies primarily in the
test for regression avoidance.
refs.c | 12 +++++++++++-
t/t3000-ls-files-others.sh | 19 +++++++++++++++++++
2 files changed, 30 insertions(+), 1 deletions(-)
diff --git a/refs.c b/refs.c
index 9911c97..cab4394 100644
--- a/refs.c
+++ b/refs.c
@@ -393,12 +393,22 @@ static struct ref_array *get_loose_refs(const char *submodule)
#define MAXDEPTH 5
#define MAXREFLEN (1024)
+/*
+ * Called by resolve_gitlink_ref_recursive() after it failed to read
+ * from "name", which is "module/.git/<refname>". Find <refname> in
+ * the packed-refs file for the submodule.
+ */
static int resolve_gitlink_packed_ref(char *name, int pathlen, const char *refname, unsigned char *result)
{
int retval = -1;
struct ref_entry *ref;
- struct ref_array *array = get_packed_refs(name);
+ struct ref_array *array;
+ /* being defensive: resolve_gitlink_ref() did this for us */
+ if (pathlen < 6 || memcmp(name + pathlen - 6, "/.git/", 6))
+ die("Oops");
+ name[pathlen - 6] = '\0'; /* make it path to the submodule */
+ array = get_packed_refs(name);
ref = search_ref_array(array, refname);
if (ref != NULL) {
memcpy(result, ref->sha1, 20);
diff --git a/t/t3000-ls-files-others.sh b/t/t3000-ls-files-others.sh
index 2eec011..e9160df 100755
--- a/t/t3000-ls-files-others.sh
+++ b/t/t3000-ls-files-others.sh
@@ -65,4 +65,23 @@ test_expect_success '--no-empty-directory hides empty directory' '
test_cmp expected3 output
'
+test_expect_success SYMLINKS 'ls-files --others with symlinked submodule' '
+ git init super &&
+ git init sub &&
+ (
+ cd sub &&
+ >a &&
+ git add a &&
+ git commit -m sub &&
+ git pack-refs --all
+ ) &&
+ (
+ cd super &&
+ "$TEST_DIRECTORY/../contrib/workdir/git-new-workdir" ../sub sub
+ git ls-files --others --exclude-standard >../actual
+ ) &&
+ echo sub/ >expect &&
+ test_cmp expect actual
+'
+
test_done
--
1.7.7.370.gefe43
^ permalink raw reply related
* Re: What should "git fetch origin +next" should do?
From: Junio C Hamano @ 2011-10-17 18:34 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20111017171041.GA12837@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I think the exact same confusion exists. I told git to update 'next'
> from origin, but it didn't touch refs/remotes/origin/next.
Except that you didn't tell git to *update* the remote tracking branch for
'next'; you merely told it to fetch 'next' at the remote.
> ... But I suspect that is not how many git users think of it.
I am inclined to agree that it might be the case; see my other message in
this thread.
> We've discussed this before, of course:
>
> http://thread.gmane.org/gmane.comp.version-control.git/127163/focus=127215
Yes, you brought up the "remote state as of the time I told git to record
it is a precious piece of information" issue, and I share the reasoning,
hence I am somewhat torn.
We might be better off biting the bullet and do the "rewrite a command
line colon-less refspec using a matching configured refspec iff exists"
and defer the history of remote tracking branches to its reflog in the
longer term.
^ permalink raw reply
* Re: [PATCH 3/3] git-gui: new config to control staging of untracked files
From: Heiko Voigt @ 2011-10-17 18:34 UTC (permalink / raw)
To: Bert Wesarg; +Cc: Pat Thoyts, git
In-Reply-To: <03727ea04f20c953e7de3f84ab1724a8360ca2c4.1318620267.git.bert.wesarg@googlemail.com>
Hi,
what the series tries to achieve looks good to me. Just one comment.
On Fri, Oct 14, 2011 at 09:25:21PM +0200, Bert Wesarg wrote:
[...]
> diff --git a/lib/index.tcl b/lib/index.tcl
> index 014acf9..45094c2 100644
> --- a/lib/index.tcl
> +++ b/lib/index.tcl
> @@ -367,7 +367,19 @@ proc do_add_all {} {
> }
> }
> if {[llength $untracked_paths]} {
> - set reply [ask_popup [mc "Stage also untracked files?"]]
> + set reply 0
> + switch -- [get_config gui.stageuntracked] {
> + no {
> + set reply 0
> + }
[...]
Here I am wondering whether we have a similar mechanism in git gui like
in core git that makes yes,true,1 equivalents (and similar with other
values) ? If we don't I think the series is fine as it is otherwise it
probably makes sense to use that mechanism.
Cheers Heiko
^ permalink raw reply
* Re: [PATCH v3 2/7] invalidate_ref_cache(): take the submodule as parameter
From: Junio C Hamano @ 2011-10-17 18:00 UTC (permalink / raw)
To: Michael Haggerty
Cc: git, Jeff King, Drew Northup, Jakub Narebski, Heiko Voigt,
Johan Herland, Julian Phillips
In-Reply-To: <4E960F91.5020103@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
> On 10/12/2011 09:19 PM, Junio C Hamano wrote:
>> Michael Haggerty <mhagger@alum.mit.edu> writes:
>> ...
>> Probably that is what all the existing callers want, but I would have
>> expected that an existing feature would be kept, perhaps like this
>> instead:
>>
>> if (!submodule) {
>> struct ref_cache *c;
>> for (c = ref_cache; c; c = c->next)
>> clear_ref_cache(c);
>> } else {
>> clear_ref_cache(get_ref_cache(submodule);
>> }
> ...
> Your specific suggestion would not work because currently
> submodule==NULL signifies the main module. However, it would be easy to
> add the few-line function when/if it is needed.
I think "submodule==NULL" is probably a mistake; "" would make more sense
given that you are storing the string in name[FLEX_ARRAY] field.
^ 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