Re: summaries in git add --patch
- From
William Pursell <bill.pursell@gmail.com>
- Date
- Nov 29, 2008, 00:22 UTC
- Message-ID
- <49308B4B.3070703@gmail.com>
- In-Reply-To
- <7v8wr48g98.fsf@gitster.siamese.dyndns.org>
Junio C Hamano wrote:
Show 7 quoted lines
> William Pursell <bill.pursell@gmail.com> 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!
Show 12 quoted lines
>> @@ -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.
Show 21 quoted lines
>> @@ -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?
Show 9 quoted lines
>> + 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.
Show 13 quoted lines
> >> + 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.)
Show 10 quoted lines
>
>> @@ -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.
Show 41 quoted lines
>
>> @@ -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