Re: [PATCH 1/3] add-patch: respect diff.context configuration

From: Junio C Hamano <gitster@pobox.com>
Date: May 6, 2025, 17:29 UTC
Message-ID: <xmqqzffpk5q9.fsf@gitster.g>
In-Reply-To: <8b91eef8120b8f92db953ec983fddce8a442abcc.1746436719.git.gitgitgadget@gmail.com>

"Leon Michalak via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 8 quoted lines

> From: Leon Michalak <leonmichalak6@gmail.com>
>
> This aims to teach relevant builtins (that take in `--patch`) to respect
> the user's diff.context and diff.interHunkContext file configurations.
>
> Since these are both UI options and `--patch` is designed for the end user,
> I believe this was previously just an inconsistency, which this patch hopes
> to address.

The usual way to compose a log message of this project is to

 - Give an observation on how the current system works in the present
   tense (so no need to say "Currently X is Y", just "X is Y"), and
   discuss what you perceive as a problem in it.

 - Propose a solution (optional---often, problem description
   trivially leads to an obvious solution in reader's minds).

 - Give commands to the codebase to "become like so".

in this order. "Various built-in commands that use add-patch infrastructure do not honor diff.context and diff.interHunkContext configuration variables." Would be the first sentence to explain the current situation. Follow that sentence with an explanation why it is a bad thing (i.e. it is possible that "do not honor" may be a deliberate design decision---perhaps the implementation of "add -p" is impossible if it has to honor diff.context that is set to 0; show evidence that no such deliberate design decision was there when the feature was introduced from the list archive, and say that not honoring the configuration is an inconvenient inconsistency).

Such an explanation would be sufficient for readers to guess the "solution", so stopping there should be fine.

Thanks.

Show 15 quoted lines

> @@ -1014,10 +1019,13 @@ static int run_diff(struct add_i_state *s, const struct pathspec *ps,
>  	if (count > 0) {
>  		struct child_process cmd = CHILD_PROCESS_INIT;
>  
> -		strvec_pushl(&cmd.args, "git", "diff", "-p", "--cached",
> -			     oid_to_hex(!is_initial ? &oid :
> -					s->r->hash_algo->empty_tree),
> -			     "--", NULL);
> +		strvec_pushl(&cmd.args, "git", "diff", "-p", "--cached", NULL);
> +		if (s->context != -1)
> +			strvec_pushf(&cmd.args, "--unified=%i", s->context);
> +		if (s->interhunkcontext != -1)
> +			strvec_pushf(&cmd.args, "--inter-hunk-context=%i", s->interhunkcontext);
> +		strvec_pushl(&cmd.args, oid_to_hex(!is_initial ? &oid :
> +			     s->r->hash_algo->empty_tree), "--", NULL);

OK.

> +test_expect_success 'diff.context honored by "add"' '
> +	git add -p >output &&
> +	! grep firstline output &&

	test_grep ! firstline output

> +	git config diff.context 8 &&
> +	git add -p >output &&
> +	grep "^ firstline" output &&

Likewise.

	test_grep "^firstline" output

> +	git config --unset diff.context

Not like this. Either use test_when_finished to arrange that this unset is run when you leave, or use test_config.

	test_expect_success title '
		test_config diff.context 8 &&
		do your tests
	'

will reset diff.conftext when the above piece is done.

	test_expect_success title '
		test_when_finished "git config unset diff.context" &&
		git config set diff.context 8 &&
		do your tests
	'

is an equivalent.