Skip to content

built-in add -p: add support for the same config settings as the Perl version #175

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 10 commits into from
Closed

built-in add -p: add support for the same config settings as the Perl version #175

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
e12df77
t3701: adjust difffilter test
dscho Jan 12, 2020
413a87b
built-in add -p: support interactive.diffFilter
dscho Mar 12, 2019
062c624
built-in add -p: handle diff.algorithm
dscho Mar 23, 2019
09a8946
terminal: make the code of disable_echo() reusable
dscho Apr 4, 2019
a81304c
terminal: accommodate Git for Windows' default terminal
dscho Apr 4, 2019
8d9c703
terminal: add a new function to read a single keystroke
dscho Mar 26, 2019
8ed4487
built-in add -p: respect the `interactive.singlekey` config setting
dscho Mar 26, 2019
cdc609f
built-in add -p: handle Escape sequences in interactive.singlekey mode
dscho Mar 27, 2019
80b0f25
built-in add -p: handle Escape sequences more efficiently
dscho Mar 27, 2019
7ab7ec6
ci: include the built-in `git add -i` in the `linux-gcc` job
dscho Mar 28, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions add-interactive.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,24 @@ void init_add_i_state(struct add_i_state *s, struct repository *r)
diff_get_color(s->use_color, DIFF_FILE_OLD));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, SZEDER Gábor wrote (reply to this):

On Wed, Dec 25, 2019 at 11:56:52AM +0000, Johannes Schindelin via GitGitGadget wrote:
> The Perl version supports post-processing the colored diff (that is
> generated in addition to the uncolored diff, intended to offer a
> prettier user experience) by a command configured via that config
> setting, and now the built-in version does that, too.

So this patch makes the test 'detect bogus diffFilter output' in
't3701-add-interactive.sh' succeed with the builtin interactive add,
but I stumbled upon a test failure caused by SIGPIPE in an
experimental Travis CI s390x build:

  expecting success of 3701.49 'detect bogus diffFilter output': 
          git reset --hard &&
  
          echo content >test &&
          test_config interactive.diffFilter "echo too-short" &&
          printf y >y &&
          test_must_fail force_color git add -p <y
  
  + git reset --hard
  HEAD is now at 6ee5ee5 test
  + echo content
  + test_config interactive.diffFilter echo too-short
  + printf y
  + test_must_fail force_color git add -p
  test_must_fail: died by signal 13: force_color git add -p
  error: last command exited with $?=1

Turns out it's a general issue, and

  GIT_TEST_ADD_I_USE_BUILTIN=1 ./t3701-add-interactive.sh -r 39,49 --stress

fails within 10 seconds on my Linux box, whereas the scripted 'add -p'
managed to survive a couple hundred repetitions.

> Signed-off-by: Johannes Schindelin <[email protected]>
> ---
>  add-interactive.c | 12 ++++++++++++
>  add-interactive.h |  3 +++
>  add-patch.c       | 33 +++++++++++++++++++++++++++++++++
>  3 files changed, 48 insertions(+)
> 
> diff --git a/add-interactive.c b/add-interactive.c
> index a5bb14f2f4..1786ea29c4 100644
> --- a/add-interactive.c
> +++ b/add-interactive.c
> @@ -52,6 +52,17 @@ void init_add_i_state(struct add_i_state *s, struct repository *r)
>  		diff_get_color(s->use_color, DIFF_FILE_OLD));
>  	init_color(r, s, "new", s->file_new_color,
>  		diff_get_color(s->use_color, DIFF_FILE_NEW));
> +
> +	FREE_AND_NULL(s->interactive_diff_filter);
> +	git_config_get_string("interactive.difffilter",
> +			      &s->interactive_diff_filter);
> +}
> +
> +void clear_add_i_state(struct add_i_state *s)
> +{
> +	FREE_AND_NULL(s->interactive_diff_filter);
> +	memset(s, 0, sizeof(*s));
> +	s->use_color = -1;
>  }
>  
>  /*
> @@ -1149,6 +1160,7 @@ int run_add_i(struct repository *r, const struct pathspec *ps)
>  	strbuf_release(&print_file_item_data.worktree);
>  	strbuf_release(&header);
>  	prefix_item_list_clear(&commands);
> +	clear_add_i_state(&s);
>  
>  	return res;
>  }
> diff --git a/add-interactive.h b/add-interactive.h
> index b2f23479c5..46c73867ad 100644
> --- a/add-interactive.h
> +++ b/add-interactive.h
> @@ -15,9 +15,12 @@ struct add_i_state {
>  	char context_color[COLOR_MAXLEN];
>  	char file_old_color[COLOR_MAXLEN];
>  	char file_new_color[COLOR_MAXLEN];
> +
> +	char *interactive_diff_filter;
>  };
>  
>  void init_add_i_state(struct add_i_state *s, struct repository *r);
> +void clear_add_i_state(struct add_i_state *s);
>  
>  struct repository;
>  struct pathspec;
> diff --git a/add-patch.c b/add-patch.c
> index 46c6c183d5..78bde41df0 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -398,6 +398,7 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
>  
>  	if (want_color_fd(1, -1)) {
>  		struct child_process colored_cp = CHILD_PROCESS_INIT;
> +		const char *diff_filter = s->s.interactive_diff_filter;
>  
>  		setup_child_process(s, &colored_cp, NULL);
>  		xsnprintf((char *)args.argv[color_arg_index], 8, "--color");
> @@ -407,6 +408,24 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
>  		argv_array_clear(&args);
>  		if (res)
>  			return error(_("could not parse colored diff"));
> +
> +		if (diff_filter) {
> +			struct child_process filter_cp = CHILD_PROCESS_INIT;
> +
> +			setup_child_process(s, &filter_cp,
> +					    diff_filter, NULL);
> +			filter_cp.git_cmd = 0;
> +			filter_cp.use_shell = 1;
> +			strbuf_reset(&s->buf);
> +			if (pipe_command(&filter_cp,
> +					 colored->buf, colored->len,
> +					 &s->buf, colored->len,
> +					 NULL, 0) < 0)
> +				return error(_("failed to run '%s'"),
> +					     diff_filter);
> +			strbuf_swap(colored, &s->buf);
> +		}
> +
>  		strbuf_complete_line(colored);
>  		colored_p = colored->buf;
>  		colored_pend = colored_p + colored->len;
> @@ -531,6 +550,9 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
>  						   colored_pend - colored_p);
>  			if (colored_eol)
>  				colored_p = colored_eol + 1;
> +			else if (p != pend)
> +				/* colored shorter than non-colored? */
> +				goto mismatched_output;
>  			else
>  				colored_p = colored_pend;
>  
> @@ -555,6 +577,15 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
>  		 */
>  		hunk->splittable_into++;
>  
> +	/* non-colored shorter than colored? */
> +	if (colored_p != colored_pend) {
> +mismatched_output:
> +		error(_("mismatched output from interactive.diffFilter"));
> +		advise(_("Your filter must maintain a one-to-one correspondence\n"
> +			 "between its input and output lines."));
> +		return -1;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -1612,6 +1643,7 @@ int run_add_p(struct repository *r, enum add_p_mode mode,
>  	    parse_diff(&s, ps) < 0) {
>  		strbuf_release(&s.plain);
>  		strbuf_release(&s.colored);
> +		clear_add_i_state(&s.s);
>  		return -1;
>  	}
>  
> @@ -1630,5 +1662,6 @@ int run_add_p(struct repository *r, enum add_p_mode mode,
>  	strbuf_release(&s.buf);
>  	strbuf_release(&s.plain);
>  	strbuf_release(&s.colored);
> +	clear_add_i_state(&s.s);
>  	return 0;
>  }
> -- 
> gitgitgadget
>

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Johannes Schindelin wrote (reply to this):

  This message is in MIME format.  The first part should be readable text,
  while the remaining parts are likely unreadable without MIME-aware tools.

--8323328-618752088-1578898044=:46
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

Hi G=C3=A1bor,

On Tue, 7 Jan 2020, SZEDER G=C3=A1bor wrote:

> On Wed, Dec 25, 2019 at 11:56:52AM +0000, Johannes Schindelin via GitGit=
Gadget wrote:
> > The Perl version supports post-processing the colored diff (that is
> > generated in addition to the uncolored diff, intended to offer a
> > prettier user experience) by a command configured via that config
> > setting, and now the built-in version does that, too.
>
> So this patch makes the test 'detect bogus diffFilter output' in
> 't3701-add-interactive.sh' succeed with the builtin interactive add,
> but I stumbled upon a test failure caused by SIGPIPE in an
> experimental Travis CI s390x build:
>
>   expecting success of 3701.49 'detect bogus diffFilter output':
>           git reset --hard &&
>
>           echo content >test &&
>           test_config interactive.diffFilter "echo too-short" &&
>           printf y >y &&
>           test_must_fail force_color git add -p <y
>
>   + git reset --hard
>   HEAD is now at 6ee5ee5 test
>   + echo content
>   + test_config interactive.diffFilter echo too-short
>   + printf y
>   + test_must_fail force_color git add -p
>   test_must_fail: died by signal 13: force_color git add -p
>   error: last command exited with $?=3D1
>
> Turns out it's a general issue, and
>
>   GIT_TEST_ADD_I_USE_BUILTIN=3D1 ./t3701-add-interactive.sh -r 39,49 --s=
tress
>
> fails within 10 seconds on my Linux box, whereas the scripted 'add -p'
> managed to survive a couple hundred repetitions.

You're right, of course. And I had let that slip for too long, as I saw it
sporadically happen in the Azure Pipeline, too.

This took quite a while to figure out, and I won't claim that I understand
_all_ the details: I _think_ that `stdin` being so short "breaks the pipe"
and interferes with `add -p`'s normal operation, so I needed to explicitly
use the `sigchain` feature to ignore `SIGPIPE` during `add -p`'s main
loop.

Thanks,
Dscho

--8323328-618752088-1578898044=:46--

init_color(r, s, "new", s->file_new_color,
diff_get_color(s->use_color, DIFF_FILE_NEW));

FREE_AND_NULL(s->interactive_diff_filter);
git_config_get_string("interactive.difffilter",
&s->interactive_diff_filter);

FREE_AND_NULL(s->interactive_diff_algorithm);
git_config_get_string("diff.algorithm",
&s->interactive_diff_algorithm);

git_config_get_bool("interactive.singlekey", &s->use_single_key);
}

void clear_add_i_state(struct add_i_state *s)
{
FREE_AND_NULL(s->interactive_diff_filter);
FREE_AND_NULL(s->interactive_diff_algorithm);
memset(s, 0, sizeof(*s));
s->use_color = -1;
}

/*
Expand Down Expand Up @@ -1149,6 +1167,7 @@ int run_add_i(struct repository *r, const struct pathspec *ps)
strbuf_release(&print_file_item_data.worktree);
strbuf_release(&header);
prefix_item_list_clear(&commands);
clear_add_i_state(&s);

return res;
}
4 changes: 4 additions & 0 deletions add-interactive.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,13 @@ struct add_i_state {
char context_color[COLOR_MAXLEN];
char file_old_color[COLOR_MAXLEN];
char file_new_color[COLOR_MAXLEN];

int use_single_key;
char *interactive_diff_filter, *interactive_diff_algorithm;
};

void init_add_i_state(struct add_i_state *s, struct repository *r);
void clear_add_i_state(struct add_i_state *s);

struct repository;
struct pathspec;
Expand Down
57 changes: 53 additions & 4 deletions add-patch.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#include "pathspec.h"
#include "color.h"
#include "diff.h"
#include "compat/terminal.h"

enum prompt_mode_type {
PROMPT_MODE_CHANGE = 0, PROMPT_DELETION, PROMPT_HUNK,
Expand Down Expand Up @@ -360,6 +361,7 @@ static int is_octal(const char *p, size_t len)
static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
{
struct argv_array args = ARGV_ARRAY_INIT;
const char *diff_algorithm = s->s.interactive_diff_algorithm;
struct strbuf *plain = &s->plain, *colored = NULL;
struct child_process cp = CHILD_PROCESS_INIT;
char *p, *pend, *colored_p = NULL, *colored_pend = NULL, marker = '\0';
Expand All @@ -369,6 +371,8 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
int res;

argv_array_pushv(&args, s->mode->diff_cmd);
if (diff_algorithm)
argv_array_pushf(&args, "--diff-algorithm=%s", diff_algorithm);
if (s->revision) {
struct object_id oid;
argv_array_push(&args,
Expand Down Expand Up @@ -398,6 +402,7 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)

if (want_color_fd(1, -1)) {
struct child_process colored_cp = CHILD_PROCESS_INIT;
const char *diff_filter = s->s.interactive_diff_filter;

setup_child_process(s, &colored_cp, NULL);
xsnprintf((char *)args.argv[color_arg_index], 8, "--color");
Expand All @@ -407,6 +412,24 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
argv_array_clear(&args);
if (res)
return error(_("could not parse colored diff"));

if (diff_filter) {
struct child_process filter_cp = CHILD_PROCESS_INIT;

setup_child_process(s, &filter_cp,
diff_filter, NULL);
filter_cp.git_cmd = 0;
filter_cp.use_shell = 1;
strbuf_reset(&s->buf);
if (pipe_command(&filter_cp,
colored->buf, colored->len,
&s->buf, colored->len,
NULL, 0) < 0)
return error(_("failed to run '%s'"),
diff_filter);
strbuf_swap(colored, &s->buf);
}

strbuf_complete_line(colored);
colored_p = colored->buf;
colored_pend = colored_p + colored->len;
Expand Down Expand Up @@ -531,6 +554,9 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
colored_pend - colored_p);
if (colored_eol)
colored_p = colored_eol + 1;
else if (p != pend)
/* colored shorter than non-colored? */
goto mismatched_output;
else
colored_p = colored_pend;

Expand All @@ -555,6 +581,15 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
*/
hunk->splittable_into++;

/* non-colored shorter than colored? */
if (colored_p != colored_pend) {
mismatched_output:
error(_("mismatched output from interactive.diffFilter"));
advise(_("Your filter must maintain a one-to-one correspondence\n"
"between its input and output lines."));
return -1;
}

return 0;
}

Expand Down Expand Up @@ -1115,14 +1150,27 @@ static int run_apply_check(struct add_p_state *s,
return 0;
}

static int read_single_character(struct add_p_state *s)
{
if (s->s.use_single_key) {
int res = read_key_without_echo(&s->answer);
printf("%s\n", res == EOF ? "" : s->answer.buf);
return res;
}

if (strbuf_getline(&s->answer, stdin) == EOF)
return EOF;
strbuf_trim_trailing_newline(&s->answer);
return 0;
}

static int prompt_yesno(struct add_p_state *s, const char *prompt)
{
for (;;) {
color_fprintf(stdout, s->s.prompt_color, "%s", _(prompt));
fflush(stdout);
if (strbuf_getline(&s->answer, stdin) == EOF)
if (read_single_character(s) == EOF)
return -1;
strbuf_trim_trailing_newline(&s->answer);
switch (tolower(s->answer.buf[0])) {
case 'n': return 0;
case 'y': return 1;
Expand Down Expand Up @@ -1362,9 +1410,8 @@ static int patch_update_file(struct add_p_state *s,
_(s->mode->prompt_mode[prompt_mode_type]),
s->buf.buf);
fflush(stdout);
if (strbuf_getline(&s->answer, stdin) == EOF)
if (read_single_character(s) == EOF)
break;
strbuf_trim_trailing_newline(&s->answer);

if (!s->answer.len)
continue;
Expand Down Expand Up @@ -1612,6 +1659,7 @@ int run_add_p(struct repository *r, enum add_p_mode mode,
parse_diff(&s, ps) < 0) {
strbuf_release(&s.plain);
strbuf_release(&s.colored);
clear_add_i_state(&s.s);
return -1;
}

Expand All @@ -1630,5 +1678,6 @@ int run_add_p(struct repository *r, enum add_p_mode mode,
strbuf_release(&s.buf);
strbuf_release(&s.plain);
strbuf_release(&s.colored);
clear_add_i_state(&s.s);
return 0;
}
1 change: 1 addition & 0 deletions ci/run-build-and-tests.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ linux-gcc)
export GIT_TEST_OE_DELTA_SIZE=5
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 12/25/2019 6:57 AM, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <[email protected]>
> 
> This job runs the test suite twice, once in regular mode, and once with
> a whole slew of `GIT_TEST_*` variables set.
> 
> Now that the built-in version of `git add --interactive` is
> feature-complete, let's also throw `GIT_TEST_ADD_I_USE_BUILTIN` into
> that fray.
> 
> Signed-off-by: Johannes Schindelin <[email protected]>
> ---
>  ci/run-build-and-tests.sh | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
> index ff0ef7f08e..4df54c4efe 100755
> --- a/ci/run-build-and-tests.sh
> +++ b/ci/run-build-and-tests.sh
> @@ -20,6 +20,7 @@ linux-gcc)
>  	export GIT_TEST_OE_DELTA_SIZE=5
>  	export GIT_TEST_COMMIT_GRAPH=1
>  	export GIT_TEST_MULTI_PACK_INDEX=1
> +	export GIT_TEST_ADD_I_USE_BUILTIN=1
>  	make test

I see that I need to add this to the test-coverage builds.

Will do.
-Stolee

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi Stolee,

On Thu, 26 Dec 2019, Derrick Stolee wrote:

> On 12/25/2019 6:57 AM, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <[email protected]>
> >
> > This job runs the test suite twice, once in regular mode, and once with
> > a whole slew of `GIT_TEST_*` variables set.
> >
> > Now that the built-in version of `git add --interactive` is
> > feature-complete, let's also throw `GIT_TEST_ADD_I_USE_BUILTIN` into
> > that fray.
> >
> > Signed-off-by: Johannes Schindelin <[email protected]>
> > ---
> >  ci/run-build-and-tests.sh | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
> > index ff0ef7f08e..4df54c4efe 100755
> > --- a/ci/run-build-and-tests.sh
> > +++ b/ci/run-build-and-tests.sh
> > @@ -20,6 +20,7 @@ linux-gcc)
> >  	export GIT_TEST_OE_DELTA_SIZE=5
> >  	export GIT_TEST_COMMIT_GRAPH=1
> >  	export GIT_TEST_MULTI_PACK_INDEX=1
> > +	export GIT_TEST_ADD_I_USE_BUILTIN=1
> >  	make test
>
> I see that I need to add this to the test-coverage builds.

Thank you for catching this!

It makes me wonder whether the test-coverage builds should use some `sed`
invocation on the `ci/run-build-and-tests.sh` script, though, so that you
do not have to edit the Azure Pipelines definition manually all the time?

Ciao,
Dscho

export GIT_TEST_COMMIT_GRAPH=1
export GIT_TEST_MULTI_PACK_INDEX=1
export GIT_TEST_ADD_I_USE_BUILTIN=1
make test
;;
linux-gcc-4.8)
Expand Down
Loading