* [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/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
* 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] 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 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/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] 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] Allow to specify the editor used for git rebase -i by config/environment var
From: Junio C Hamano @ 2011-10-17 21:36 UTC (permalink / raw)
To: Peter Oberndorfer; +Cc: git, Phil Hord
In-Reply-To: <57346812.Rh0UlzroDp@soybean>
Peter Oberndorfer <kumbayo84@arcor.de> writes:
> 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.
Thanks. I'll reword the proposed commit log message before queuing,
because it won't make _any_ sense to talk about the search order before
telling what these things that are searched would do, or why they are
useful things to have.
"rebase -i": support special-purpose editor to edit insn sheet
The insn sheet used by "rebase -i" is designed to be easily editable by
any text editor, but an editor that is specifically meant for it (but
is otherwise unsuitable for editing regular text files) could be useful
by allowing drag & drop reordering in a GUI environment, for example.
The GIT_SEQUENCE_EDITOR environment variable and/or the sequence.editor
configuration variable can be used to specify such an editor, while
allowing the usual editor to be used to edit commit log messages. As
usual, the environment variable takes precedence over the configuration
variable.
It is envisioned that other "sequencer" based tools will use the same
mechanism.
Signed-off-by: Peter Oberndorfer <kumbayo84@arcor.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
^ permalink raw reply
* Re: [PATCH/RFC 3/2] Refactor Git::config_*
From: Jakub Narebski @ 2011-10-17 21:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Eric Wong, Cord Seele, Matthieu Moy, git, Cord Seele
In-Reply-To: <7vsjmrl4ur.fsf@alter.siamese.dyndns.org>
On Mon, 17 Oct 2011, Junio C Hamano wrote:
> 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.
Sorry about that, I have ran only t9700-perl-git.sh, which runs all right.
The problem was with duplicated _maybe_self(@_), which should be run only
once. t9700-perl-git.sh passed because it uses only (recommended) object
form, and not procedural form like git-svn.
The following amend fixes the issue for me:
-- >8 --
diff --git i/perl/Git.pm w/perl/Git.pm
index 8e52290..32f6533 100644
--- i/perl/Git.pm
+++ w/perl/Git.pm
@@ -653,7 +653,7 @@ sub config_int {
# Common subroutine to implement bulk of what the config* family of methods
# do. This wraps command('config') so it is not so fast.
sub _config_common {
- my ($self, $var, $opts) = _maybe_self(@_);
+ my ($self, $var, $opts) = @_;
try {
my @cmd = ('config', $opts->{'kind'} ? $opts->{'kind'} : ());
-- 8< --
I'll resend amended commit.
--
Jakub Narebski
Poland
^ permalink raw reply related
* Re: What should "git fetch origin +next" should do?
From: Marc Branchaud @ 2011-10-17 22:02 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <7vipnnmppx.fsf@alter.siamese.dyndns.org>
On 11-10-17 02:34 PM, Junio C Hamano wrote:
> 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.
Indeed. Apologies for missing that subtlety.
So now I think option (2) is the best choice. To support one-off fetches,
teach fetch to accept "foo:" refspecs as "fetch ref foo from the remote and
only update FETCH_HEAD" -- maybe allow "foo:FETCH_HEAD" too, for folks who
like to be explicit and can't remember the shorthand syntax.
The rest of this post explains my reasoning, which I think pretty much just
rehashes what Junio said more efficiently in his initial message.
Overall I'd expect "git fetch origin next" to be a subset of "git fetch
origin". That is, since the default fetch refspec is
+refs/heads/*:refs/remotes/origin/*
normally "git fetch origin" gets all of origin's updated refs (ff or not) and
puts them under the local remotes/origin/ space. So I would expect "git
fetch origin next" to only fetch the "next" ref from origin and update the
local remotes/origin/next ref.
Given the default fetch refspec, I'd expect "git fetch origin +next" to do
the exact same thing. The + on the command line is basically redundant.
But removing the + from the fetch refspec changes things. Now I'd expect
plain "git fetch origin" to fail if there are any non-ff updates, and "git
fetch origin next" should also fail if origin's next ref is non-ff. But "git
fetch origin +next" would succeed.
In all cases if the command-line refspec has no RHS then git should try to
figure out which local ref to update from the config, and it should die if it
can't figure out a local ref to create or update. (As I said above, maybe
allow "git fetch origin foo:" to let the user put the tip of origin's foo ref
into FETCH_HEAD.)
All this gets a bit more complicated if the user has currently checked out
the a ref that should be updated (regardless of the presence of a LHS +).
But really only old-style git gurus should run in to that problem.
M.
^ 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 22:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Kato Kazuyoshi, git
In-Reply-To: <7v62jnl3ef.fsf@alter.siamese.dyndns.org>
On Mon, 17 Oct 2011, Junio C Hamano wrote:
> 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.
Well, cover letter for 2-patch series might be overkill, but having
patches in series connected e.g. chain-replied-to, or all replies to
first patch in series, is IMHO a good idea.
>>> 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
Wouldn't that cover not only combined diff, but an "ordinary" diff as
well then (i.e. comment should change)? I think that was the intent,
isn't it?
> my $num_sign = @{$from->{'href'}} + 1;
$from->{'href'} is array reference only for combined diff (>= 2 parents).
For ordinary diff it is a scalar.
> my @cc_classifier = (
> ["\@{$num_sign} ", "chunk_header"],
O.K.
Nb., it is "chunk_header", or "hunk_header"?
> ['\\', "incomplete"],
O.K.
> [" {$num_sign}", ""],
O.K.
> ["[+ ]{$num_sign}", "add"],
> ["[- ]{$num_sign}", "rem"],
It would be slightly different to what current code does.
Current code for combined diff uses "add" if there is at least one '+',
"rem" if there are no '+' and at least one '-', and context otherwise.
I wonder if with sufficiently evil merge we can have a line that is
added (changed) in some children, and removed in other, i.e. pluses
and minuses combined.
Nb. we can put regexp here, not only stringification of regexp.
i.e.
[qr/[+ ]{$num_sign}/, "add"],
[qr/[- ]{$num_sign}/, "rem"],
> );
> for my $cls (@cc_classifier) {
> if ($line =~ /^$cls->[0]$/) {
> $diff_class = $cls->[1];
> last;
> }
> }
Nice trick.
> 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?
Right. We use "diff" class without anything else for context, but probably
it would be better to state this explicitly.
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: [PATCH] resolve_gitlink_packed_ref(): fix mismerge
From: Mark Levedahl @ 2011-10-17 22:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michael Haggerty, git
In-Reply-To: <7vbotfmpbh.fsf_-_@alter.siamese.dyndns.org>
On 10/17/2011 02:43 PM, Junio C Hamano wrote:
> 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>
> ---
>
Thank you, this fixes the problem for me.
Mark
^ permalink raw reply
* [PATCH] Git-p4: Add "git p4 change" command.
From: Andrei Warkentin @ 2011-10-17 22:16 UTC (permalink / raw)
To: git, gitster; +Cc: Andrei Warkentin
Many users of p4/sd use changelists for review, regression
tests and batch builds.
"p4 change" is almost equivalent to "p4 submit", yet will
just create the changelist and not submit it.
Signed-off-by: Andrei Warkentin <andreiw@vmware.com>
---
contrib/fast-import/git-p4 | 16 ++++++++++++----
contrib/fast-import/git-p4.txt | 10 ++++++++++
2 files changed, 22 insertions(+), 4 deletions(-)
diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index 2f7b270..19c295b 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -959,7 +959,10 @@ class P4Submit(Command, P4UserMap):
submitTemplate = message[:message.index(separatorLine)]
if self.isWindows:
submitTemplate = submitTemplate.replace("\r\n", "\n")
- p4_write_pipe("submit -i", submitTemplate)
+ if gitConfig("git-p4.changeOnSubmit"):
+ p4_write_pipe("change -i", submitTemplate)
+ else:
+ p4_write_pipe("subadasdmit -i", submitTemplate)
if self.preserveUser:
if p4User:
@@ -981,9 +984,14 @@ class P4Submit(Command, P4UserMap):
file = open(fileName, "w+")
file.write(self.prepareLogMessage(template, logMessage))
file.close()
- print ("Perforce submit template written as %s. "
- + "Please review/edit and then use p4 submit -i < %s to submit directly!"
- % (fileName, fileName))
+ if gitConfig("git-p4.changeOnSubmit"):
+ print ("Perforce submit template written as %s. "
+ + "Please review/edit and then use p4 change -i < %s to create changelist!"
+ % (fileName, fileName))
+ else:
+ print ("Perforce submit template written as %s. "
+ + "Please review/edit and then use p4 submit -i < %s to submit directly!"
+ % (fileName, fileName))
def run(self, args):
if len(args) == 0:
diff --git a/contrib/fast-import/git-p4.txt b/contrib/fast-import/git-p4.txt
index 52003ae..3a3a815 100644
--- a/contrib/fast-import/git-p4.txt
+++ b/contrib/fast-import/git-p4.txt
@@ -180,6 +180,16 @@ git-p4.allowSubmit
git config [--global] git-p4.allowSubmit false
+git-p4.changeOnSubmit
+
+ git config [--global] git-p4.changeOnSubmit false
+
+Most places using p4/sourcedepot don't actually want you submit
+changes directly, and changelists are used to do regression testing,
+batch builds and review, hence, by setting this parameter to
+true you acknowledge you end up creating a changelist which you
+must then manually commit.
+
git-p4.syncFromOrigin
A useful setup may be that you have a periodically updated git repository
--
1.7.4.1
^ permalink raw reply related
* [PATCH] Git-p4: Add "git p4 change" command.
From: Andrei Warkentin @ 2011-10-17 22:18 UTC (permalink / raw)
To: git, gitster; +Cc: Andrei Warkentin
Many users of p4/sd use changelists for review, regression
tests and batch builds.
"p4 change" is almost equivalent to "p4 submit", yet will
just create the changelist and not submit it.
Signed-off-by: Andrei Warkentin <andreiw@vmware.com>
---
contrib/fast-import/git-p4 | 23 ++++++++++++++++++-----
1 files changed, 18 insertions(+), 5 deletions(-)
diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index 2f7b270..dd084b9 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -950,7 +950,10 @@ class P4Submit(Command, P4UserMap):
if checkModTime and (os.stat(fileName).st_mtime <= mtime):
response = "x"
while response != "y" and response != "n":
- response = raw_input("Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) ")
+ if (self.cmdname == "change"):
+ response = raw_input("Change template unchanged. Create changelist anyway? [y]es, [n]o (skip this patch) ")
+ else:
+ response = raw_input("Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) ")
if response == "y":
tmpFile = open(fileName, "rb")
@@ -959,7 +962,10 @@ class P4Submit(Command, P4UserMap):
submitTemplate = message[:message.index(separatorLine)]
if self.isWindows:
submitTemplate = submitTemplate.replace("\r\n", "\n")
- p4_write_pipe("submit -i", submitTemplate)
+ if (self.cmdname == "change"):
+ p4_write_pipe("change -i", submitTemplate)
+ else:
+ p4_write_pipe("submit -i", submitTemplate)
if self.preserveUser:
if p4User:
@@ -981,9 +987,14 @@ class P4Submit(Command, P4UserMap):
file = open(fileName, "w+")
file.write(self.prepareLogMessage(template, logMessage))
file.close()
- print ("Perforce submit template written as %s. "
- + "Please review/edit and then use p4 submit -i < %s to submit directly!"
- % (fileName, fileName))
+ if (self.cmdname == "change"):
+ print ("Perforce change template written as %s. "
+ + "Please review/edit and then use p4 change -i < %s to submit directly!"
+ % (fileName, fileName))
+ else:
+ print ("Perforce submit template written as %s. "
+ + "Please review/edit and then use p4 submit -i < %s to submit directly!"
+ % (fileName, fileName))
def run(self, args):
if len(args) == 0:
@@ -2177,6 +2188,7 @@ commands = {
"debug" : P4Debug,
"submit" : P4Submit,
"commit" : P4Submit,
+ "change" : P4Submit,
"sync" : P4Sync,
"rebase" : P4Rebase,
"clone" : P4Clone,
@@ -2202,6 +2214,7 @@ def main():
sys.exit(2)
options = cmd.options
+ cmd.cmdname = cmdName
cmd.gitdir = os.environ.get("GIT_DIR", None)
args = sys.argv[2:]
--
1.7.4.1
^ permalink raw reply related
* Re: [PATCH] Git-p4: Add "git p4 change" command.
From: Andrei Warkentin @ 2011-10-17 22:20 UTC (permalink / raw)
To: Andrei Warkentin; +Cc: git, gitster
In-Reply-To: <1318889798-17334-1-git-send-email-andreiw@vmware.com>
----- Original Message -----
> From: "Andrei Warkentin" <andreiw@vmware.com>
> To: git@vger.kernel.org, gitster@pobox.com
> Cc: "Andrei Warkentin" <andreiw@vmware.com>
> Sent: Monday, October 17, 2011 6:16:38 PM
> Subject: [PATCH] Git-p4: Add "git p4 change" command.
>
> Many users of p4/sd use changelists for review, regression
> tests and batch builds.
>
> "p4 change" is almost equivalent to "p4 submit", yet will
> just create the changelist and not submit it.
>
> Signed-off-by: Andrei Warkentin <andreiw@vmware.com>
> ---
Sorry for the spam, this is the wrong patch :-(.
A
^ permalink raw reply
* Re: [PATCH] Git-p4: Add "git p4 change" command.
From: Andrei Warkentin @ 2011-10-17 22:21 UTC (permalink / raw)
To: Andrei Warkentin; +Cc: git, gitster
In-Reply-To: <1318889937-17693-1-git-send-email-andreiw@vmware.com>
----- Original Message -----
> From: "Andrei Warkentin" <andreiw@vmware.com>
> To: git@vger.kernel.org, gitster@pobox.com
> Cc: "Andrei Warkentin" <andreiw@vmware.com>
> Sent: Monday, October 17, 2011 6:18:57 PM
> Subject: [PATCH] Git-p4: Add "git p4 change" command.
>
> Many users of p4/sd use changelists for review, regression
> tests and batch builds.
>
> "p4 change" is almost equivalent to "p4 submit", yet will
> just create the changelist and not submit it.
>
> Signed-off-by: Andrei Warkentin <andreiw@vmware.com>
> ---
> contrib/fast-import/git-p4 | 23 ++++++++++++++++++-----
> 1 files changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
> index 2f7b270..dd084b9 100755
> --- a/contrib/fast-import/git-p4
> +++ b/contrib/fast-import/git-p4
> @@ -950,7 +950,10 @@ class P4Submit(Command, P4UserMap):
> if checkModTime and (os.stat(fileName).st_mtime <=
> mtime):
> response = "x"
> while response != "y" and response != "n":
> - response = raw_input("Submit template unchanged.
> Submit anyway? [y]es, [n]o (skip this patch) ")
> + if (self.cmdname == "change"):
> + response = raw_input("Change template
> unchanged. Create changelist anyway? [y]es, [n]o (skip this patch)
> ")
> + else:
> + response = raw_input("Submit template
> unchanged. Submit anyway? [y]es, [n]o (skip this patch) ")
>
> if response == "y":
> tmpFile = open(fileName, "rb")
> @@ -959,7 +962,10 @@ class P4Submit(Command, P4UserMap):
> submitTemplate =
> message[:message.index(separatorLine)]
> if self.isWindows:
> submitTemplate = submitTemplate.replace("\r\n",
> "\n")
> - p4_write_pipe("submit -i", submitTemplate)
> + if (self.cmdname == "change"):
> + p4_write_pipe("change -i", submitTemplate)
> + else:
> + p4_write_pipe("submit -i", submitTemplate)
>
> if self.preserveUser:
> if p4User:
> @@ -981,9 +987,14 @@ class P4Submit(Command, P4UserMap):
> file = open(fileName, "w+")
> file.write(self.prepareLogMessage(template, logMessage))
> file.close()
> - print ("Perforce submit template written as %s. "
> - + "Please review/edit and then use p4 submit -i <
> %s to submit directly!"
> - % (fileName, fileName))
> + if (self.cmdname == "change"):
> + print ("Perforce change template written as %s. "
> + + "Please review/edit and then use p4 change
> -i < %s to submit directly!"
> + % (fileName, fileName))
> + else:
> + print ("Perforce submit template written as %s. "
> + + "Please review/edit and then use p4 submit
> -i < %s to submit directly!"
> + % (fileName, fileName))
>
> def run(self, args):
> if len(args) == 0:
> @@ -2177,6 +2188,7 @@ commands = {
> "debug" : P4Debug,
> "submit" : P4Submit,
> "commit" : P4Submit,
> + "change" : P4Submit,
> "sync" : P4Sync,
> "rebase" : P4Rebase,
> "clone" : P4Clone,
> @@ -2202,6 +2214,7 @@ def main():
> sys.exit(2)
>
> options = cmd.options
> + cmd.cmdname = cmdName
> cmd.gitdir = os.environ.get("GIT_DIR", None)
>
> args = sys.argv[2:]
> --
> 1.7.4.1
>
This is the change I would like to have reviewed.
Sorry again for the spam.
A
^ permalink raw reply
* Re: [PATCH] git-gui: fix multi selected file operation
From: Pat Thoyts @ 2011-10-17 22:26 UTC (permalink / raw)
To: Bert Wesarg; +Cc: git
In-Reply-To: <CAKPyHN0iaS301r_d+kc-AVRNVKUprhdMwDpdD0HDf7nKbsPR3Q@mail.gmail.com>
Bert Wesarg <bert.wesarg@googlemail.com> writes:
>Hi,
>
>On Sun, Oct 16, 2011 at 00:48, Pat Thoyts
><patthoyts@users.sourceforge.net> wrote:
>> Bert Wesarg <bert.wesarg@googlemail.com> writes:
>>
>>>The current path for what we see the diff is not in the list of selected
>>>paths. But when we add single paths (with Ctrl) to the set the current path
>>>would not be used when the action is performed.
>>>
>>>Fix this by explicitly putting the path into the list before we start
>>>showing the diff.
>>>
>>>Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
>>>---
>>> git-gui.sh | 1 +
>>> 1 files changed, 1 insertions(+), 0 deletions(-)
>>>
>>>diff --git a/git-gui.sh b/git-gui.sh
>>>index f897160..e5dd8bc 100755
>>>--- a/git-gui.sh
>>>+++ b/git-gui.sh
>>>@@ -2474,6 +2474,7 @@ proc toggle_or_diff {w x y} {
>>> [concat $after [list ui_ready]]
>>> }
>>> } else {
>>>+ set selected_paths($path) 1
>>> show_diff $path $w $lno
>>> }
>>> }
>>
>> It is not clear what I should be looking for to test this. Can you
>> re-write the commit message to be more clear about what you are
>> fixing. Is this multiple unstaged files in the staging box? If so I
>> don't see what path display is changing.
>
>Sorry, for this bad description. I will give you a recipe here what to
>do to expose the problem. I try later to form this into a new commit
>message:
>
>You have 2 modified, not staged files A and B. Your current view shows
>the diff for A. Adding B to the selection via Ctrl+Button1 and than
>perform the "Stage To Commit" action from the "Commit" menu results
>only in the staging of B.
>
>Note, using Shift+Button1 (i.e. 'adding a range of files to the
>selection') results in the staging of both files A and B.
>
>Bert
Ah ok - that explains things and I can see the issue now. I think
something like:
"When staging a selection of files using Shift-Click to choose a range
of files then using Ctrl-T or the Stage To Commit menu item will stage
all the selected files. However if a non-sequential range is selected
using Ctrl-Click then only the last name selected gets staged. This
commit fixes this to properly stage all selected files by explicitly
adding the path to the list before showing the diff."
will do.
--
Pat Thoyts http://www.patthoyts.tk/
PGP fingerprint 2C 6E 98 07 2C 59 C8 97 10 CE 11 E6 04 E0 B9 DD
^ permalink raw reply
* Re: [PATCH] Git-p4: git-p4.changeOnSubmit to do 'change' instead of 'submit'.
From: Pete Wyckoff @ 2011-10-17 22:32 UTC (permalink / raw)
To: Andrei Warkentin
Cc: git, Andrei Warkentin, Tor Arvid Lund, Luke Diamand, gitster
In-Reply-To: <4E9C799E.70700@diamand.org>
luke@diamand.org wrote on Mon, 17 Oct 2011 19:53 +0100:
> 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.
This seems like a useful thing to do, but needs some care.
Git can have multiple commits outstanding that touch the same
file, but p4 cannot really have multiple pending changes in the
same workspace that touch the same file.
If you call "git-p4 change", it would build a p4 change for each
of those commits. If the commits happen to touch the same file,
the changes get rearranged as far as p4 is concerned so that all
changes to a given file are lumped in the first change that sees
the file. This is highly counterintuitive from a git mindset.
The most restrictive implementation would have to:
1. ensure no pending changes in the P4 clientPath
2. ensure number of commits ("git rev-list") is 1
You could be more permissive, allowing multiple pending changes
if the file sets do not conflict. In that case, the first test
would look at the files in pending changes and allow the
operation if they did not intersect with files in origin..master.
The second would make sure that each file appears in no more than
1 commit in origin..master.
Also make sure this works with preserveUser. Not sure if an
unsubmitted change can be handled the same way.
Because it feels like a delicate operation that could have big
negative consequences, this needs a few unit tests.
For the code structure, I'd like to see a proper subclass instead
of the dictionary idea. Something like, e.g.:
class P4Submit(...):
def __init__(self, change_only=0)
...
self.change_only = change_only
class P4Change(P4Submit):
def __init__(self):
P4Submit.__init__(self, change_only=1)
Sorry this is looking so difficult now.
-- Pete
^ permalink raw reply
* Re: [PATCH v4 2/2] push: teach --recurse-submodules the on-demand option
From: Junio C Hamano @ 2011-10-17 22:33 UTC (permalink / raw)
To: Heiko Voigt; +Cc: Jens Lehmann, Fredrik Gustafsson, git
In-Reply-To: <20111017190749.GA3126@sandbox-rc>
Heiko Voigt <hvoigt@hvoigt.net> writes:
> since we have not heard anything from Fredrik I will probably look into
> cleaning this up. Should I do that with follow-up patches since this
> patch is already in next?
I thought we kicked it back to 'pu' after 1.7.7 cycle.
I would personally want to put a freeze on "recursively do anything to
submodule" topic (including but not limited to "checkout") for now, until
we know how we would want to support "floating submodule" model. For
existing code in-flight, I would like to see us at least have a warm and
fuzzy feeling that we know which part of the code such a support would
need to undo and how the update would look like before moving forward.
There are two camps that use submodules in their large-ish projects.
One is mostly happy with the traditional "submodule trees checked out must
match what the superproject says, otherwise you have local changes and the
build product cannot be called to have emerged from that particular
superproject commit" model. Let's call this "exact submodules" model.
The other prefers "submodule trees checked out are whatever submodule
commits that happen to sit at the tips of the designated branches the
superproject wants to use" model. The superproject tree does not exactly
know or care what commit to use from each of its submodules, and I would
imagine that it may be more convenient for developers. They do not have to
care the entire build product while they commit---only the integration
process that could be separate and perhaps automated needs to know.
We haven't given any explicit support to the latter "floating submodules"
model so far. There may be easy workarounds to many of the potential
issues, (e.g. at "git diff/status" level, there may be some configuration
variables to tell the tools to ignore differences between the commit the
superproject records for the submodule path and the HEAD in the
submodule), but with recent work on submodules such as "allow pushing
superproject only after submodule commits are pushed out", I am afraid
that we seem to be piling random new things with the assumption that we
would never support anything but "exact submodules" model. Continuing the
development that way would require retrofitting support for "floating
submodules" model to largely undo the unwarranted assumptions existing
code makes. That is the reason why I would like to see people think about
the need to support the other "floating submodules" model, before making
the existing mess even worse.
The very first step for floating submodules support would be relatively
simple. We could declare that an entry in the .gitmodules file in the
superproject can optionally specify which branch needs to be checked out
with something like:
[submodule "libfoo"]
branch = master
path = include/foo
url = git://foo.com/git/lib.git
and when such an entry is defined, a command at the superproject level
would largely ignore what is at include/foo in the tree object recorded in
the superproject commit and in the index. When we show "git status" in the
superproject, instead of using the commit bound to the superproject, we
would use include/foo/.git/HEAD as the basis for detecting "local" changes
to the submodule. We could even declare that the gitlink for such a
submodule should record 0{40} SHA-1 in the superproject, but I do not
think that is necessary.
^ permalink raw reply
* Re: [PATCH] Git-p4: git-p4.changeOnSubmit to do 'change' instead of 'submit'.
From: Andrei Warkentin @ 2011-10-17 22:37 UTC (permalink / raw)
To: Pete Wyckoff; +Cc: git, Andrei Warkentin, Tor Arvid Lund, Luke Diamand, gitster
In-Reply-To: <20111017223202.GA1834@arf.padd.com>
Hi,
----- Original Message -----
> From: "Pete Wyckoff" <pw@padd.com>
> To: "Andrei Warkentin" <awarkentin@vmware.com>
> Cc: git@vger.kernel.org, "Andrei Warkentin" <andreiw@vmware.com>, "Tor Arvid Lund" <torarvid@gmail.com>, "Luke
> Diamand" <luke@diamand.org>, gitster@pobox.com
> Sent: Monday, October 17, 2011 6:32:02 PM
> Subject: Re: [PATCH] Git-p4: git-p4.changeOnSubmit to do 'change' instead of 'submit'.
>
> Git can have multiple commits outstanding that touch the same
> file, but p4 cannot really have multiple pending changes in the
> same workspace that touch the same file.
>
> If you call "git-p4 change", it would build a p4 change for each
> of those commits. If the commits happen to touch the same file,
> the changes get rearranged as far as p4 is concerned so that all
> changes to a given file are lumped in the first change that sees
> the file. This is highly counterintuitive from a git mindset.
>
> The most restrictive implementation would have to:
>
> 1. ensure no pending changes in the P4 clientPath
> 2. ensure number of commits ("git rev-list") is 1
>
> You could be more permissive, allowing multiple pending changes
> if the file sets do not conflict. In that case, the first test
> would look at the files in pending changes and allow the
> operation if they did not intersect with files in origin..master.
> The second would make sure that each file appears in no more than
> 1 commit in origin..master.
Hmmm...I see. I'll think some more about it, then!
>
> Also make sure this works with preserveUser. Not sure if an
> unsubmitted change can be handled the same way.
>
> Because it feels like a delicate operation that could have big
> negative consequences, this needs a few unit tests.
>
> For the code structure, I'd like to see a proper subclass instead
> of the dictionary idea. Something like, e.g.:
>
> class P4Submit(...):
> def __init__(self, change_only=0)
> ...
> self.change_only = change_only
>
> class P4Change(P4Submit):
> def __init__(self):
> P4Submit.__init__(self, change_only=1)
>
> Sorry this is looking so difficult now.
No problem!
A
^ permalink raw reply
* Re: [PATCH 3/3] git-gui: new config to control staging of untracked files
From: Pat Thoyts @ 2011-10-17 22:51 UTC (permalink / raw)
To: Heiko Voigt; +Cc: Bert Wesarg, git
In-Reply-To: <20111017192706.GB3168@sandbox-rc>
Heiko Voigt <hvoigt@hvoigt.net> writes:
>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
>
This set of 3 patches looks fine. I was a bit dubious of the new
phrasing for the ask condition but it is growing on me. I wonder it it
might be worth including the number of untracked files to be staged too
eg: "Stage 15 untracked files?"
set reply [ask_popup [mc "Stage %d untracked files?" \
[llength $untracked_paths]]]
Loosening the check we can do using
switch -glob -- [get_config gui.stageuntracked] {
[Nn]* { set reply 0}
[Yy]* { set reply 1}
default { ... }
}
--
Pat Thoyts http://www.patthoyts.tk/
PGP fingerprint 2C 6E 98 07 2C 59 C8 97 10 CE 11 E6 04 E0 B9 DD
^ permalink raw reply
* Re: [PATCH 1/4] git-gui: handle config booleans without value
From: Pat Thoyts @ 2011-10-17 23:13 UTC (permalink / raw)
To: Bert Wesarg; +Cc: git
In-Reply-To: <94b050c4cf7ae8df8d79112e13613244ebff4037.1318579823.git.bert.wesarg@googlemail.com>
Bert Wesarg <bert.wesarg@googlemail.com> writes:
>When git interprets a config variable without a value as bool it is considered
>as true. But git-gui doesn't so until yet.
>
>The value for boolean configs are also case-insensitive.
>
>Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
>---
> git-gui.sh | 16 ++++++++++++++--
> 1 files changed, 14 insertions(+), 2 deletions(-)
>
>diff --git a/git-gui.sh b/git-gui.sh
>index f897160..d687871 100755
>--- a/git-gui.sh
>+++ b/git-gui.sh
>@@ -299,7 +299,9 @@ proc is_config_true {name} {
> global repo_config
> if {[catch {set v $repo_config($name)}]} {
> return 0
>- } elseif {$v eq {true} || $v eq {1} || $v eq {yes}} {
>+ }
>+ set v [string tolower $v]
>+ if {$v eq {} || $v eq {true} || $v eq {1} || $v eq {yes} || $v eq {on}} {
> return 1
> } else {
> return 0
This looks ok - we actually have a [string is boolean $v] test we could
use eg:
if {[string is boolean $v]} {
return [expr {$v eq {} ? 1 : !!$v}]
}
although I'm not sure it gains us much. This would match everything Tcl
believes to be a boolean - yes/no, on/off, true/false and 1/0. Without
-strict the [string is] test will consider {} to be a boolean.
>@@ -310,7 +312,9 @@ proc is_config_false {name} {
> global repo_config
> if {[catch {set v $repo_config($name)}]} {
> return 0
>- } elseif {$v eq {false} || $v eq {0} || $v eq {no}} {
>+ }
>+ set v [string tolower $v]
>+ if {$v eq {false} || $v eq {0} || $v eq {no} || $v eq {off}} {
> return 1
> } else {
> return 0
>@@ -1060,6 +1064,10 @@ git-version proc _parse_config {arr_name args} {
> } else {
> set arr($name) $value
> }
>+ } elseif {[regexp {^([^\n]+)$} $line line name]} {
>+ # no value given, but interpreting them as
>+ # boolean will be handled as true
>+ set arr($name) {}
> }
> }
> }
>@@ -1075,6 +1083,10 @@ git-version proc _parse_config {arr_name args} {
> } else {
> set arr($name) $value
> }
>+ } elseif {[regexp {^([^=]+)$} $line line name]} {
>+ # no value given, but interpreting them as
>+ # boolean will be handled as true
>+ set arr($name) {}
> }
> }
> close $fd_rc
Is this really how git treats boolean config values? I can't seem to
demonstrate that to myself:
pat@frog:/opt/src/git-gui$ git config --unset core.xyzzy
pat@frog:/opt/src/git-gui$ git config --bool --get core.xyzzy
pat@frog:/opt/src/git-gui$ git config --bool core.xyzzy 1
pat@frog:/opt/src/git-gui$ git config --bool --get core.xyzzy
true
I assume I'm using the wrong test for this.
--
Pat Thoyts http://www.patthoyts.tk/
PGP fingerprint 2C 6E 98 07 2C 59 C8 97 10 CE 11 E6 04 E0 B9 DD
^ permalink raw reply
* Re: [PATCH] resolve_gitlink_packed_ref(): fix mismerge
From: Junio C Hamano @ 2011-10-17 23:14 UTC (permalink / raw)
To: Mark Levedahl; +Cc: Michael Haggerty, git
In-Reply-To: <4E9CA853.10107@gmail.com>
Mark Levedahl <mlevedahl@gmail.com> writes:
> On 10/17/2011 02:43 PM, Junio C Hamano wrote:
>> 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>
>> ---
>>
> Thank you, this fixes the problem for me.
Thanks for a quick bug report and helping with the test.
^ 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 23:20 UTC (permalink / raw)
To: Jakub Narebski; +Cc: Kato Kazuyoshi, git
In-Reply-To: <201110180007.31008.jnareb@gmail.com>
Jakub Narebski <jnareb@gmail.com> writes:
> O.K.
>
>> [" {$num_sign}", ""],
>
> O.K.
>
>> ["[+ ]{$num_sign}", "add"],
>> ["[- ]{$num_sign}", "rem"],
>
> It would be slightly different to what current code does.
> Current code for combined diff uses "add" if there is at least one '+',
> "rem" if there are no '+' and at least one '-', and context otherwise.
The only possible difference would be for a line with all blank, but
because there is an additional explicit rule for context, the behaviour is
the same. In a combined diff, you will never see + and - together on the
same line.
> I wonder if with sufficiently evil merge we can have a line that is
> added (changed) in some children, and removed in other, i.e. pluses
> and minuses combined.
The logic in combine-diff.c::dump_sline() was written in such a way to
avoid such a confusing output.
> Nb. we can put regexp here, not only stringification of regexp.
> i.e.
>
> [qr/[+ ]{$num_sign}/, "add"],
> [qr/[- ]{$num_sign}/, "rem"],
That would be a good change.
>> 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?
>
> Right. We use "diff" class without anything else for context, but probably
> it would be better to state this explicitly.
^ permalink raw reply
* Re: [PATCH/RFC 3/2] Refactor Git::config_*
From: Junio C Hamano @ 2011-10-17 23:25 UTC (permalink / raw)
To: Jakub Narebski; +Cc: Eric Wong, Cord Seele, Matthieu Moy, git, Cord Seele
In-Reply-To: <201110172347.42568.jnareb@gmail.com>
Jakub Narebski <jnareb@gmail.com> writes:
> The problem was with duplicated _maybe_self(@_), which should be run only
> once. t9700-perl-git.sh passed because it uses only (recommended) object
> form, and not procedural form like git-svn.
Thanks.
I have a suspicion that a more standard implementation of _maybe_self()
would instead unshift a dummy singleton Git() instance instead of undef to
lift such a limitation, but perhaps the code relies on undef'ness of self
when called as a vanilla subroutine.
^ 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