[PATCH v4 0/4] Better support for customising context lines in --patch commands

["Leon Michalak via GitGitGadget" <gitgitgadget@gmail.com>]

This series of patches attempt to give --interactive/--patch compatible
builtins ("add", "commit", "checkout", "reset", "restore" and "stash")
better support and nicer experience for configuring how many context lines
are shown in diffs through a variety of ways.

Prior to these patches, the user could not choose how many context lines
they saw in --patch commands (apart from one workaround by using
GIT_DIFF_OPTS=-u<number> ..., however this isn't a good user experience or a
persistent solution). Additionally, the behaviour around reading from the
diff.context and diff.interHunkContext configs was also inconsistent with
other diff generating commands such as "log -p".

The summarised changes below hopefully make this experience better and fix
some inconsistencies:

 * diff.context and diff.interHunkContext configs are now respected by
   --patch compatible commands
 * --unified and --inter-hunk-context command line options have been added
   to --patch compatible commands (which take prescendence over file
   configs)
 * "add" and "commit" in --interactive mode now expose a new "context"
   subcommand which configures the amount of context lines you wish to see
   in subsequent diffs generated from other subcommands such as "patch" or
   "diff"

The original discussion for this can be read at:

 * https://lore.kernel.org/git/CAP9jKjGb-Rcr=RLJEzeFdtrekYM+qmHy+1T1fykU3n9cV4GhGw@mail.gmail.com/

Changes since v1:

 * Update commit descriptions
 * Update tests to use the more modern and robust test_grep and test_config
   utils
 * Reword some documentation / user messages
 * Ensure each commit is atomic and builds/passes tests on it's own
 * Make new command line options DRY
 * Add tests for interhunk context interaction
 * Error if context config/command line options are negative
 * Drop previous last commit to do with new subcommand for --interactive
   add/commit. My motivations behind this patch series originally where
   quite simple, just for add-patch commands to respect context configs.
   This subcommand, after the discussion in v1, will require more thought
   and a larger implementation that what I had anticipated. I would prefer
   to leave this for another time as it's the least impactful but the most
   time intensive and complicated idea.

Changes since v2:

 * Update tests to only test single command (following Philip's suggestion)
 * Add negative option checks
 * Minor commit re-wording

Changes since v3:

 * Update commit descriptions
 * Read struct properties directly instead of assigning to variables first
 * Simplify config setting / error checking
 * Remove redundant tests in later commit as they were replaced with better
   test(s)
 * Change tests to use single quotes (this messes with the grep so was
   unable to explicitly test single quotes in the error messages, so decided
   to use regex . instead, which is what some other tests that have this
   problem seem to use as well)

Leon Michalak (4):
  t: use test_grep in t3701 and t4055
  t: use test_config in t4055
  add-patch: respect diff.context configuration
  add-patch: add diff.context command line overrides

 Documentation/diff-context-options.adoc | 10 +++
 Documentation/git-add.adoc              |  2 +
 Documentation/git-checkout.adoc         |  2 +
 Documentation/git-commit.adoc           |  2 +
 Documentation/git-reset.adoc            |  2 +
 Documentation/git-restore.adoc          |  2 +
 Documentation/git-stash.adoc            |  2 +
 add-interactive.c                       | 45 ++++++++++--
 add-interactive.h                       | 17 ++++-
 add-patch.c                             | 14 ++--
 builtin/add.c                           | 21 +++++-
 builtin/checkout.c                      | 31 +++++++-
 builtin/commit.c                        | 16 +++-
 builtin/reset.c                         | 17 ++++-
 builtin/stash.c                         | 56 +++++++++++---
 commit.h                                |  3 +-
 parse-options.h                         |  2 +
 t/t3701-add-interactive.sh              | 97 +++++++++++++++++++------
 t/t4055-diff-context.sh                 | 72 ++++++++++++------
 t/t9902-completion.sh                   |  2 +
 20 files changed, 332 insertions(+), 83 deletions(-)
 create mode 100644 Documentation/diff-context-options.adoc


base-commit: cf6f63ea6bf35173e02e18bdc6a4ba41288acff9
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1915%2FNinjaInShade%2Finteractive-patch-context-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1915/NinjaInShade/interactive-patch-context-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/1915

Range-diff vs v3:

 1:  044a93014b6 ! 1:  bbb2bc7082b test: use "test_grep"
     @@ Metadata
      Author: Leon Michalak <leonmichalak6@gmail.com>
      
       ## Commit message ##
     -    test: use "test_grep"
     +    t: use test_grep in t3701 and t4055
      
     -    Use the modern "test_grep" test utility instead of regular "grep" which
     -    provides better debug information if tests fail.
     -
     -    This is a prerequisite to the commits that follow which add to both test
     -    files.
     +    As a preparatory clean-up, use the "test_grep" test utility instead of
     +    regular "grep" which provides better debug information if tests fail.
      
          Signed-off-by: Leon Michalak <leonmichalak6@gmail.com>
      
 2:  e5c40d37750 ! 2:  feace2d3676 test: use "test_config"
     @@ Metadata
      Author: Leon Michalak <leonmichalak6@gmail.com>
      
       ## Commit message ##
     -    test: use "test_config"
     +    t: use test_config in t4055
      
          Use the modern "test_config" test utility instead of manual"git config"
          as the former provides clean up on test completion.
 3:  1ec8a138486 ! 3:  994029d6602 add-patch: respect diff.context configuration
     @@ Commit message
      
          Various builtins that use add-patch infrastructure do not respect
          the user's diff.context and diff.interHunkContext file configurations.
     -    This patch fixes this inconsistency.
      
     -    This is because the plumbing commands used by "git add -p" to generate
     -    the diff do not read those config settings. Fix this by reading the
     -    config before generating the patch and passing it along to the diff
     -    command with the "-U" and "--inter-hunk-context" command-line options.
     +    The user may be used to seeing their diffs with customized context size,
     +    but not in the patches "git add -p" shows them to pick from.
     +
     +    Teach add-patch infrastructure to read these configuration variables and
     +    pass their values when spawning the underlying plumbing commands as
     +    their command line option.
      
          Signed-off-by: Leon Michalak <leonmichalak6@gmail.com>
      
       ## add-interactive.c ##
     -@@ add-interactive.c: static void init_color(struct repository *r, struct add_i_state *s,
     - void init_add_i_state(struct add_i_state *s, struct repository *r)
     - {
     +@@ add-interactive.c: void init_add_i_state(struct add_i_state *s, struct repository *r)
       	const char *value;
     -+	int context;
     -+	int interhunkcontext;
       
       	s->r = r;
      +	s->context = -1;
     @@ add-interactive.c: void init_add_i_state(struct add_i_state *s, struct repositor
       	repo_config_get_string(r, "diff.algorithm",
       			       &s->interactive_diff_algorithm);
       
     -+	if (!repo_config_get_int(r, "diff.context", &context)) {
     -+		if (context < 0)
     ++	if (!repo_config_get_int(r, "diff.context", &s->context))
     ++		if (s->context < 0)
      +			die(_("%s cannot be negative"), "diff.context");
     -+		else
     -+			s->context = context;
     -+	}
     -+	if (!repo_config_get_int(r, "diff.interHunkContext", &interhunkcontext)) {
     -+		if (interhunkcontext < 0)
     ++	if (!repo_config_get_int(r, "diff.interHunkContext", &s->interhunkcontext))
     ++		if (s->interhunkcontext < 0)
      +			die(_("%s cannot be negative"), "diff.interHunkContext");
     -+		else
     -+			s->interhunkcontext = interhunkcontext;
     -+	}
      +
       	repo_config_get_bool(r, "interactive.singlekey", &s->use_single_key);
       	if (s->use_single_key)
     @@ add-interactive.h: struct add_i_state {
       void init_add_i_state(struct add_i_state *s, struct repository *r);
      
       ## add-patch.c ##
     -@@ add-patch.c: static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
     +@@ add-patch.c: static int normalize_marker(const char *p)
     + static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
       {
       	struct strvec args = STRVEC_INIT;
     - 	const char *diff_algorithm = s->s.interactive_diff_algorithm;
     -+	int diff_context = s->s.context;
     -+	int diff_interhunkcontext = s->s.interhunkcontext;
     +-	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';
     @@ add-patch.c: static int parse_diff(struct add_p_state *s, const struct pathspec
       	int res;
       
       	strvec_pushv(&args, s->mode->diff_cmd);
     -+	if (diff_context != -1)
     -+		strvec_pushf(&args, "--unified=%i", diff_context);
     -+	if (diff_interhunkcontext != -1)
     -+		strvec_pushf(&args, "--inter-hunk-context=%i", diff_interhunkcontext);
     - 	if (diff_algorithm)
     - 		strvec_pushf(&args, "--diff-algorithm=%s", diff_algorithm);
     +-	if (diff_algorithm)
     +-		strvec_pushf(&args, "--diff-algorithm=%s", diff_algorithm);
     ++	if (s->s.context != -1)
     ++		strvec_pushf(&args, "--unified=%i", s->s.context);
     ++	if (s->s.interhunkcontext != -1)
     ++		strvec_pushf(&args, "--inter-hunk-context=%i", s->s.interhunkcontext);
     ++	if (s->s.interactive_diff_algorithm)
     ++		strvec_pushf(&args, "--diff-algorithm=%s", s->s.interactive_diff_algorithm);
       	if (s->revision) {
     + 		struct object_id oid;
     + 		strvec_push(&args,
      
       ## t/t3701-add-interactive.sh ##
      @@ t/t3701-add-interactive.sh: test_expect_success 'hunk splitting works with diff.suppressBlankEmpty' '
 4:  b68c58b667c ! 4:  2774b930406 add-patch: add diff.context command line overrides
     @@ add-interactive.c: static void init_color(struct repository *r, struct add_i_sta
      +		      struct add_p_opt *add_p_opt)
       {
       	const char *value;
     - 	int context;
     + 
      @@ add-interactive.c: void init_add_i_state(struct add_i_state *s, struct repository *r)
       	repo_config_get_bool(r, "interactive.singlekey", &s->use_single_key);
       	if (s->use_single_key)
     @@ parse-options.h: int parse_opt_tracking_mode(const struct option *, const char *
       	OPT_SET_INT_F('4', "ipv4", (v), N_("use IPv4 addresses only"), \
      
       ## t/t3701-add-interactive.sh ##
     -@@ t/t3701-add-interactive.sh: test_expect_success 'add -p rejects negative diff.context' '
     - 	test_grep "diff.context cannot be negative" output
     +@@ t/t3701-add-interactive.sh: test_expect_success 'hunk splitting works with diff.suppressBlankEmpty' '
     + 	test_cmp expect actual
       '
       
     +-test_expect_success 'add -p respects diff.context' '
     +-	test_write_lines a b c d e f g h i j k l m >file &&
      +for cmd in add checkout restore 'commit -m file'
      +do
     -+	test_expect_success "${cmd%% *} accepts -U and --inter-hunk-context" "
     ++	test_expect_success "${cmd%% *} accepts -U and --inter-hunk-context" '
      +		test_write_lines a b c d e f g h i j k l m n o p q r s t u v >file &&
      +		git add file &&
      +		test_write_lines a b c d e F g h i j k l m n o p Q r s t u v >file &&
      +		echo y | git -c diff.context=5 -c diff.interhunkcontext=1 \
      +			$cmd -p -U 4 --inter-hunk-context 2 >actual &&
     -+		test_grep \"@@ -2,20 +2,20 @@\" actual
     -+	"
     ++		test_grep "@@ -2,20 +2,20 @@" actual
     ++	'
      +done
      +
      +test_expect_success 'reset accepts -U and --inter-hunk-context' '
      +	test_write_lines a b c d e f g h i j k l m n o p q r s t u v >file &&
      +	git commit -m file file &&
      +	test_write_lines a b c d e F g h i j k l m n o p Q r s t u v >file &&
     -+	git add file &&
     + 	git add file &&
     +-	test_write_lines a b c d e f G h i j k l m >file &&
     +-	echo y | git -c diff.context=5 add -p >actual &&
     +-	test_grep "@@ -2,11 +2,11 @@" actual
      +	echo y | git -c diff.context=5 -c diff.interhunkcontext=1 \
      +		reset -p -U 4 --inter-hunk-context 2 >actual &&
      +	test_grep "@@ -2,20 +2,20 @@" actual
     -+'
     -+
     + '
     + 
     +-test_expect_success 'add -p respects diff.interHunkContext' '
     +-	test_write_lines a b c d e f g h i j k l m n o p q r s >file &&
     +-	git add file &&
     +-	test_write_lines a b c d E f g i i j k l m N o p q r s >file &&
     +-	echo y | git -c diff.interhunkcontext=2 add -p >actual &&
     +-	test_grep "@@ -2,16 +2,16 @@" actual
      +test_expect_success 'stash accepts -U and --inter-hunk-context' '
      +	test_write_lines a b c d e F g h i j k l m n o p Q r s t u v >file &&
      +	git commit -m file file &&
     @@ t/t3701-add-interactive.sh: test_expect_success 'add -p rejects negative diff.co
      +	echo y | git -c diff.context=5 -c diff.interhunkcontext=1 \
      +		stash -p -U 4 --inter-hunk-context 2 >actual &&
      +	test_grep "@@ -2,20 +2,20 @@" actual
     -+'
     -+
     -+for cmd in add checkout commit reset restore 'stash save' 'stash push'
     + '
     + 
     +-test_expect_success 'add -p rejects negative diff.context' '
     +-	test_config diff.context -1 &&
     +-	test_must_fail git add -p 2>output &&
     +-	test_grep "diff.context cannot be negative" output
     +-'
     ++for cmd in add checkout commit reset restore "stash save" "stash push"
      +do
     -+	test_expect_success "$cmd rejects invalid context options" "
     ++	test_expect_success "$cmd rejects invalid context options" '
      +		test_must_fail git $cmd -p -U -3 2>actual &&
      +		cat actual | echo &&
     -+		test_grep -e \"'--unified' cannot be negative\" actual &&
     ++		test_grep -e ".--unified. cannot be negative" actual &&
      +
      +		test_must_fail git $cmd -p --inter-hunk-context -3 2>actual &&
     -+		test_grep -e \"'--inter-hunk-context' cannot be negative\" actual &&
     ++		test_grep -e ".--inter-hunk-context. cannot be negative" actual &&
      +
      +		test_must_fail git $cmd -U 7 2>actual &&
     -+		test_grep -E \"'--unified' requires '(--interactive/)?--patch'\" actual &&
     ++		test_grep -E ".--unified. requires .(--interactive/)?--patch." actual &&
      +
      +		test_must_fail git $cmd --inter-hunk-context 2 2>actual &&
     -+		test_grep -E \"'--inter-hunk-context' requires '(--interactive/)?--patch'\" actual
     -+	"
     ++		test_grep -E ".--inter-hunk-context. requires .(--interactive/)?--patch." actual
     ++	'
      +done
     -+
     + 
       test_done
      
       ## t/t4055-diff-context.sh ##

-- 
gitgitgadget