Junio C Hamano wrote:
William Pursell <bill.pursell@xxxxxxxxx> writes:
Here's a new patch. Instead of displaying the summary and then
the current hunk, it implements a 'goto' command.
I take it that this is for discussion not for immediate inclusion.
Yes. I tend to think of all of my patches as being merely
for discussion since I'm not terribly familiar with the code
base and expect to miss many things. I'm flattered that
you would even consider them for inclusion. For that matter,
I'm flattered that you have even responded to my submissions!
@@ -799,6 +801,7 @@ sub help_patch_cmd {
y - stage this hunk
n - do not stage this hunk
a - stage this and all the remaining hunks in the file
+g - select a hunk to jump to
d - do not stage this hunk nor any of the remaining hunks in the file
j - leave this hunk undecided, see next undecided hunk
J - leave this hunk undecided, see next hunk
Since you took 'g' after "go to", help text should also say "go to",
instead of "jump to" for the mnemonics value, iow, to help people
remember.
Agreed.
@@ -836,6 +839,27 @@ sub patch_update_cmd {
}
}
+sub select_new_hunk {
+ my $ri = shift;
+ my @hunk = @_;
+ my ($i, $response);
+ print " '+' stage, '-' don't stage\n";
+ for ( $i = 0; $i < @hunk; $i++ ) {
+ my $status = " ";
+ if( defined $hunk[$i]{USE} ) {
+ $status = $hunk[$i]{USE} ? "+" : "-";
+ }
Style.
(1) SP between language construct and open parenthesis, as opposed to
no extra SP between function name and open parenthesis;
(2) No extra SP around what is enclosed in parentheses.
My apologies for that. I do try to conform, but this
sort of habit is hard to change. Especially in perl,
where code so often looks like a cartoon character's
speech bubble while swearing (eg #@$!%#@@), I
like to put space inside my parens. I'm fully aware
that this is not the preferred style here, and I
am trying to conform. Is there a style validating
pre-commit hook script available?
+ printf "%s%3d: %s",
+ $status,
+ $i + 1,
+ $hunk[$i]{SUMMARY};
+ }
I think this "for ()" loop part, including the comment about +/- notation,
should be separated into a function so that you can implement a separate
"l"ist command like you did in the other patch, using the same function.
My thought is that 'g' would replace 'l', but as per your previous
email 'l' would be a reasonable status command, so it
makes sense to factor it out.
+ printf "goto which hunk? ";
+ $response = <STDIN>;
+ chomp $response;
+ $$ri = $response - 1;
What happens when $response is (1) a non number, (2) outside range (both
negative and positive), or (3) EOF?
Sending ref to scalar and returning the value by assigning is a bad taste.
Why shouldn't this function just return an integer to be assigned to $ix
by the caller? If you want to use pass-by-ref to show off your Perl-fu, I
think \@hunk would be what you would want to for performance reasons.
Ack. I have no Perl-fu, I'm just not familiar with the
idioms and thought this was accepted in perl. I agree
that it's poor judgement, and can only attribute my
usage of it here to laziness. ( At one point I was passing
$ix, and when I realized I wanted to modify it at the caller
it was just easier to pass by reference.)
@@ -919,7 +943,7 @@ sub patch_update_file {
for (@{$hunk[$ix]{DISPLAY}}) {
print;
}
- print colored $prompt_color, "Stage this hunk [y/n/a/d$other/?]? ";
+ print colored $prompt_color, "Stage this hunk [y/n/a/d/g$other/?]? ";
When there is only one hunk, we do not give j nor k. Should we give g in
such a case? Why?
I would agree that g should be invalid when only one hunk is
available. I hadn't considered that case.
@@ -937,6 +961,16 @@ sub patch_update_file {
}
next;
}
+ elsif ($line =~ /^g/) {
+ chomp ($line);
+ if ($line =~ /^g$/) {
+ select_new_hunk (\$ix, @hunk);
+ }
+ else {
+ $ix = (substr $line, 1) - 1;
+ }
The same "input validation" issue exists here. it would make sense to:
- Make choose_hunk(@hunk) that calls list_hunks(@hunk) that gives the
summary, reads one line, and returns that line;
- Make the caller here to look like this:
elsif ($line =~ s/^g//) {
chomp($line);
if ($line eq '') {
$line = choose_hunk(@hunk);
}
if ($line !~ /^\d+$/) {
print STDERR "Eh '$line', what number is that?\n";
next;
} elsif (0 < $line && $line <= $num) {
$ix = $line - 1;
} else {
print STDERR "Sorry, you have only $num hunks\n";
}
}
+ next;
+ }
elsif ($line =~ /^d/i) {
while ($ix < $num) {
if (!defined $hunk[$ix]{USE}) {
I will try to incorporate your ideas into a workable,
includable patch. I think the '/' regex search can
become part of the choose_hunk() routine so that
an integer response means select by number while
a '/re' response means jump forward to next matching
hunk. Also, instead of storing the summary line in
the hunk, it will probably be better to generate on
the fly during the display routine.
Thanks for the feedback.
--
William Pursell
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
