Subject: Re: [PATCH] git-add--interactive: manual hunk editing mode

From: Junio C Hamano <[email-masked]>
Date: Tue, 01 Jul 2008 22:39:58 -0700

Thomas Rast <[email-masked]> writes:

> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> index 903953e..6bb117a 100755
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -2,6 +2,7 @@
>  
>  use strict;
>  use Git;
> +use File::Temp;

People with minimum Perl installation should still be able to use "add -i"
as long as they do not use 'e' subcommand, shouldn't they?  Shouldn't we
do something like:

	my $can_use_temp = eval {
        	require File::Temp;
                1;
	};

and disable 'e' subcommand unless $can_use_temp?

> +sub edit_hunk_manually {
> +	my ($oldtext) = @_;
> +
> +	my $t = File::Temp->new(
> +		TEMPLATE => $repo->repo_path . "/git-hunk-edit.XXXXXX",
> +		SUFFIX => '.diff'
> +	);
> +	print $t "# Manual hunk edit mode -- see bottom for a quick guide\n";
> +	print $t @$oldtext;
> +	print $t <<EOF;
> +# ---
> +# To remove '-' lines, make them ' ' lines (context).
> +# To remove '+' lines, delete them.
> +# Lines starting with # will be removed.

Don't you want to say "Do not touch lines that begin with ' '"?

> +	# Reinsert the first hunk header if the user accidentally deleted it
> +	if ($newtext[0] !~ /^@/) {
> +		unshift @newtext, $oldtext->[0];
> +	}

Hmm, perhaps not even giving the "@@ ... @@" lines to the editor would be
a more robust solution?

> +sub diff_applies {
> +	my $fh;
> +	open $fh, '| git apply --recount --cached --check';
> +	for my $h (@_) {
> +		print $fh @{$h->{TEXT}};
> +	}
> +	return close $fh;

Have to wonder where the potential error message would go, and if it would
confuse the end users...

> @@ -1002,7 +1123,8 @@ sub patch_update_file {
>  	if (@result) {
>  		my $fh;
>  
> -		open $fh, '| git apply --cached';
> +		open $fh, '| git apply --cached'
> +			. ($need_recount ? ' --recount' : '');
>  		for (@{$head->{TEXT}}, @result) {
>  			print $fh $_;
>  		}

I recall that the original "add--interactive" carefully counted numbers in
hunks it reassembles (as it can let you split and then you can choose to
use both parts, which requires it to merge overlapping hunks back), but if
you are going to use --recount anyway, perhaps we can discard that logic?
It may make the patch application less robust, though.  I dunno.

An alternative, and probably more robust, approach would be to recount
what we have in @{$mode->{TEXT}}, after letting the user edit some of
them, so that "add--interactive" still knows what it is doing after
applying your patch without having to rely on "apply --recount".