Re: summaries in git add --patch

[Junio C Hamano <gitster@pobox.com>]

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.

> @@ -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.

> @@ -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.

> +		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.

> +	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.

> @@ -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?

> @@ -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}) {
>
>
> -- 
> William Pursell