Better support for customising context lines in --patch commands
#1915
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Welcome to GitGitGadgetHi @NinjaInShade, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests. Please make sure that either:
You can CC potential reviewers by adding a footer to the PR description with the following syntax: NOTE: DO NOT copy/paste your CC list from a previous GGG PR's description, Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:
It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code. Contributing the patchesBefore you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a comment to your PR of the form Both the person who commented An alternative is the channel Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment If you want to see what email(s) would be sent for a After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions (while the comments and suggestions will be mirrored into the PR by GitGitGadget, you will still want to reply via mail). If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox from the Git mailing list archive (click the curl -g --user "<EMailAddress>:<Password>" \
--url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txtTo iterate on your change, i.e. send a revised patch or patch series, you will first want to (force-)push to the same branch. You probably also want to modify your Pull Request description (or title). It is a good idea to summarize the revision by adding something like this to the cover letter (read: by editing the first comment on the PR, i.e. the PR description): To send a new iteration, just add another PR comment with the contents: Need help?New contributors who want advice are encouraged to join git-mentoring@googlegroups.com, where volunteers who regularly contribute to Git are willing to answer newbie questions, give advice, or otherwise provide mentoring to interested contributors. You must join in order to post or view messages, but anyone can join. You may also be able to find help in real time in the developer IRC channel, |
NinjaInShade
force-pushed
the
interactive-patch-context
branch
2 times, most recently
from
0484407 to
ff64c39
Compare
|
/allow |
|
User NinjaInShade is now allowed to use GitGitGadget. WARNING: NinjaInShade has no public email address set on GitHub; GitGitGadget needs an email address to Cc: you on your contribution, so that you receive any feedback on the Git mailing list. Go to https://github.com/settings/profile to make your preferred email public to let GitGitGadget know which email address to use. |
NinjaInShade
changed the title
--patch commands--patch commands
NinjaInShade
force-pushed
the
interactive-patch-context
branch
2 times, most recently
from
9b90dc6 to
b4b7854
Compare
|
/submit |
|
Submitted as pull.1915.git.1746436719.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
|
User |
|
User |
|
Thanks for letting me know, I'll make sure to amend that in v2 :)
…On Mon, 5 May 2025 at 21:33, gitgitgadget[bot] ***@***.***> wrote:
*gitgitgadget[bot]* left a comment (gitgitgadget/git#1915)
<#1915 (comment)>
User Eric Sunshine ***@***.***> has been added to the cc:
list.
—
Reply to this email directly, view it on GitHub
<#1915 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/APFXCEW367PGDBXHMJDZTHT247DJ7AVCNFSM6AAAAAB4M4W3LOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDQNJSGI3DIOBUGQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
User |
|
User |
NinjaInShade
force-pushed
the
interactive-patch-context
branch
2 times, most recently
from
cf726da to
973dfad
Compare
|
/submit |
|
Submitted as pull.1915.v2.git.1746884789.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
| @@ -63,7 +63,7 @@ test_expect_success 'setup (initial)' ' | |||
| ' | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Leon Michalak via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Leon Michalak <leonmichalak6@gmail.com>
>
> Refactor to 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.
>
> Signed-off-by: Leon Michalak <leonmichalak6@gmail.com>
> ---
These mostly look sensible, but I would title & phrase the commit
description to 'use "test_grep"', not 'refactor to &'. It's shorter
and more direct ;-)
Thanks.There was a problem hiding this comment.
On the Git mailing list, Leon Michalak wrote (reply to this):
On Mon, 12 May 2025 at 14:42, Junio C Hamano <gitster@pobox.com> wrote:
> These mostly look sensible, but I would title & phrase the commit
> description to 'use "test_grep"', not 'refactor to &'. It's shorter
> and more direct ;-)
Thanks - will make sure to change that in v3 :)
| @@ -0,0 +1,10 @@ | |||
| `-U<n>`:: | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Leon Michalak via GitGitGadget" <gitgitgadget@gmail.com> writes:
> diff --git a/Documentation/diff-context-options.adoc b/Documentation/diff-context-options.adoc
> new file mode 100644
> index 000000000000..e161260358ff
> --- /dev/null
> +++ b/Documentation/diff-context-options.adoc
> @@ -0,0 +1,10 @@
> +`-U<n>`::
> +`--unified=<n>`::
> + Generate diffs with _<n>_ lines of context. Defaults to `diff.context`
> + or 3 if the config option is unset.
> +
> +`--inter-hunk-context=<n>`::
> + Show the context between diff hunks, up to the specified _<number>_
> + of lines, thereby fusing hunks that are close to each other.
> + Defaults to `diff.interHunkContext` or 0 if the config option
> + is unset.
It might not be trivial to do but I wonder if we cannot do better
than this to share more of the same text across manual pages. These
two being options understood by `git diff`, we certainly have an
existing description for them, no?
Other than that, looking good to me.
Thanks.There was a problem hiding this comment.
On the Git mailing list, Leon Michalak wrote (reply to this):
On Mon, 12 May 2025 at 17:45, Junio C Hamano <gitster@pobox.com> wrote:
> It might not be trivial to do but I wonder if we cannot do better
> than this to share more of the same text across manual pages. These
> two being options understood by `git diff`, we certainly have an
> existing description for them, no?
Yes, I did of course notice documentation for `git diff` also has
these; ultimately my justification for not changing that to use this
new .adoc include as well was for a couple reasons:
- these two options are not together in the `git diff` documentation
(not *so* important, and they probably should actually be together?)
- there is an extra if def which adds on "implies --patch" text in the
`git diff` documentation which isn't the behaviour the add-patch
commands are going for, so that makes the intent a little different
here
But would be good to hear if anyone else has any thoughts.
|
This patch series was integrated into seen via git@0945e76. |
|
There was a status update in the "Cooking" section about the branch "git add/etc -p" now honors diff.context configuration variable, and learns to honor -U<n> option. Expecting a reroll. cf. <CAP9jKjEYgEBBGrPnJ8fkaWuS8RPzBeBqFLE7aTJd5x9PcWu=7Q@mail.gmail.com> source: <pull.1915.v2.git.1746884789.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@841fb98. |
|
This patch series was integrated into seen via git@86214de. |
|
This patch series was integrated into seen via git@7c97f36. |
|
There was a status update in the "Cooking" section about the branch "git add/etc -p" now honors diff.context configuration variable, and learns to honor -U<n> option. Expecting a reroll. cf. <CAP9jKjEYgEBBGrPnJ8fkaWuS8RPzBeBqFLE7aTJd5x9PcWu=7Q@mail.gmail.com> source: <pull.1915.v2.git.1746884789.gitgitgadget@gmail.com> |
NinjaInShade
force-pushed
the
interactive-patch-context
branch
3 times, most recently
from
2042995 to
b534069
Compare
|
/submit |
|
There is a merge commit in this Pull Request: Please rebase the branch and force-push. |
1 similar comment
|
There is a merge commit in this Pull Request: Please rebase the branch and force-push. |
NinjaInShade
force-pushed
the
interactive-patch-context
branch
from
f675df4 to
b534069
Compare
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. Signed-off-by: Leon Michalak <leonmichalak6@gmail.com>
Use the modern "test_config" test utility instead of manual"git config" as the former provides clean up on test completion. This is a prerequisite to the commits that follow which add to this test file. Signed-off-by: Leon Michalak <leonmichalak6@gmail.com>
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. Signed-off-by: Leon Michalak <leonmichalak6@gmail.com>
This patch compliments the previous commit, where builtins that use add-patch infrastructure now respect diff.context and diff.interHunkContext file configurations. In particular, this patch helps users who don't want to set persistent context configurations or just want a way to override them on a one-time basis, by allowing the relevant builtins to accept corresponding command line options that override the file configurations. This mimics commands such as diff and log, which allow for both context file configuration and command line overrides. Signed-off-by: Leon Michalak <leonmichalak6@gmail.com>
NinjaInShade
force-pushed
the
interactive-patch-context
branch
from
b534069 to
b68c58b
Compare
|
/submit |
|
Submitted as pull.1915.v3.git.1751128486.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
@NinjaInShade please note that nobody will see this reply; You need to reply on the Git mailing list. |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "Leon Michalak via GitGitGadget" <gitgitgadget@gmail.com> writes:
> Leon Michalak (4):
> test: use "test_grep"
> test: use "test_config"
These make it sound as if they touch all test scripts under t/ but
apparently that is not what this series is doing (and we do not want
to see a huge churn like that anyway). Would something like
t: use test_grep in t3701, t4055, and t9902
work better?
> add-patch: respect diff.context configuration
> add-patch: add diff.context command line overrides |
| @@ -63,7 +63,7 @@ test_expect_success 'setup (initial)' ' | |||
| ' | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Leon Michalak via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Leon Michalak <leonmichalak6@gmail.com>
>
> 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.
Just a terminology thing, but we would phrase the last paragraph
more like
As a preparatory clean-up, use the "test_grep" test utility
instead of regular "grep" which provides better debug
information if tests fail.
to avoid saying "This does X", "This commit is Y", etc. It also
avoids giving a wrong impression by misusing the word "prerequisite"
which we almost always use for a step that cannot be skipped. While
we add new tests to the same file, we _could_ leave these existing
tests as-is, but there is a good reason to making this change
beforehand, which we often call is a "preparatory clean-up".
> @@ -86,7 +86,7 @@ test_expect_success 'revert works (initial)' '
> git add file &&
> test_write_lines r 1 | git add -i &&
> git ls-files >output &&
> - ! grep . output
> + test_grep ! . output
> '
Good (we sometime see people got the negation wrong).
Thanks.
| test_grep ! "^ d" output && | ||
| test_grep "^ e" output && | ||
| test_grep "^ j" output && | ||
| test_grep ! "^ k" output | ||
| ' | ||
|
|
||
| test_expect_success 'diff.context honored by "log"' ' |
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Leon Michalak via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Leon Michalak <leonmichalak6@gmail.com>
>
> Use the modern "test_config" test utility instead of manual"git config"
> as the former provides clean up on test completion.
Here "completion" is not "all the tests in the script are done", but
"each of the test_expect_{success,failure} piece that uses test_config".
Drop "modern". It was invented 14 years ago (the same can be said
for test_grep which was called test_i18ngrep and had an extra
purpose, which was invented in the same year).
This conversion, unlike "test_grep" needs to be done a bit
carefully. The fact the configuration is removed after the test
piece "test_config" was used means any tests after the test that was
originally using "git config" needs to be inspected to make sure
it was *not* relying on the value that was left by the previous
test piece.
For example...
> test_expect_success 'diff.context honored by "log"' '
> git log -1 -p >output &&
> test_grep ! firstline output &&
> - git config diff.context 8 &&
> + test_config diff.context 8 &&
> git log -1 -p >output &&
> test_grep "^ firstline" output
> '
... the test piece after this one may have assumed (wrongly! The
assumption does not hold if this test failed before reaching "git
config") that diff.context is still set to 8 but that is no longer
the case. But that one is OK because ...
> test_expect_success 'The -U option overrides diff.context' '
> - git config diff.context 8 &&
> + test_config diff.context 8 &&
> git log -U4 -1 >output &&
> test_grep ! "^ firstline" output
> '
... it was setting it for itself. The same can be said with other
tests (not quoted in this reply). They all set things up for
themselves, which is a good hygiene.
That means the last one in the patch needs to be inspected
carefully. We do not know from the post-context of the patch what
the next test used to expect.
> test_expect_success '-U0 is valid, so is diff.context=0' '
> - git config diff.context 0 &&
> + test_config diff.context 0 &&
> git diff >output &&
> test_grep "^-ADDED" output &&
> test_grep "^+MODIFIED" output
The one that comes after this one is about giving an explicit
command line option -U<n>. It should not be affected by a
diff.context conffiguration variable that is lower-precedence
so we should be OK.
Thanks.
add-interactive.c
Outdated
| @@ -39,8 +39,12 @@ 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) | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Leon Michalak via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Leon Michalak <leonmichalak6@gmail.com>
>
> Various builtins that use add-patch infrastructure do not respect
> the user's diff.context and diff.interHunkContext file configurations.
Great. "add-patch.c" invokes "diff-files", "diff-index" plumbing
commands to do its thing, and these plumbing commands deliberately
ignore such configuration variables, unlike "diff" Porcelain command
that is meant for end-user consumption.
> This patch fixes this inconsistency.
If we were spelling it out, we would say "Fix this inconsistency" in
imperative. But you never talked about "this inconsistency" so far,
so it is not just confusing. It hints an incorrect conclusion that
the difference between plumbing diff-{files,index,tree} and
Porcelain diff is an inconsistency that needs to be "fixed", which
is not true.
Follow the first paragraph with an explanation why it is a bad
thing. For example:
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.
That would implicitly tell readers that we would want the patch
shown by "add -p" generated with diff.context given by the user.
So we can outline the solution next.
Teach add-patch infrastructure to read these configuration
variables and pass their values when spawning the underlying
plumbing commands as their command line option.
or something.
> @@ -39,8 +39,12 @@ 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)
> {
> const char *value;
> + int context;
> + int interhunkcontext;
>
> s->r = r;
> + s->context = -1;
> + s->interhunkcontext = -1;
Hmph, context/interhunkcontext variables serve no purpose other than
peeking into the value before assigning it to s->{context,interhunkcontext}
members. In a sense, they may be confusing than they are worth.
> if (repo_config_get_value(r, "color.interactive", &value))
> s->use_color = -1;
> @@ -78,6 +82,19 @@ void init_add_i_state(struct add_i_state *s, struct repository *r)
> repo_config_get_string(r, "diff.algorithm",
> &s->interactive_diff_algorithm);
>
> + if (!repo_config_get_int(r, "diff.context", &context)) {
> + if (context < 0)
> + die(_("%s cannot be negative"), "diff.context");
> + else
> + s->context = context;
> + }
Would the code be easier to understand if it is written more like
if (!repo_config_get_int(r, "diff.context", &s->context)) {
if (s->context < 0)
die(...);
}
with or without {braces} around the (technically) single statement block?
> + if (!repo_config_get_int(r, "diff.interHunkContext", &interhunkcontext)) {
> + if (interhunkcontext < 0)
> + die(_("%s cannot be negative"), "diff.interHunkContext");
> + else
> + s->interhunkcontext = interhunkcontext;
> + }
Ditto.
> diff --git a/add-interactive.h b/add-interactive.h
> index 693f125e8e4b..c63f35b14be8 100644
> --- a/add-interactive.h
> +++ b/add-interactive.h
> @@ -18,6 +18,7 @@ struct add_i_state {
>
> int use_single_key;
> char *interactive_diff_filter, *interactive_diff_algorithm;
> + int context, interhunkcontext;
> };
>
> void init_add_i_state(struct add_i_state *s, struct repository *r);
> diff --git a/add-patch.c b/add-patch.c
> index 95c67d8c80c4..b43ca1600738 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -415,6 +415,8 @@ 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;
> 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';
> @@ -424,6 +426,10 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
> 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);
Ditto. What does it buy us to have these two local variables? We
have the state object 's' available to us here, right?
Thanks.
| @@ -0,0 +1,10 @@ | |||
| `-U<n>`:: | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Leon Michalak via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Leon Michalak <leonmichalak6@gmail.com>
>
> This patch compliments the previous commit, where builtins that use
> add-patch infrastructure now respect diff.context and
> diff.interHunkContext file configurations.
>
> In particular, this patch helps users who don't want to set persistent
> context configurations or just want a way to override them on a one-time
> basis, by allowing the relevant builtins to accept corresponding command
> line options that override the file configurations.
>
> This mimics commands such as diff and log, which allow for both context
> file configuration and command line overrides.
I skimmed the patch briefly. I am not sure if it is a good idea to
* add OPT_DIFF_*() macros to parse-options API, as its utility is
very narrow, and forces those who are learning parse-options API
to learn one more thing.
* validation of the value range to be duplicated for each and every
users of the new OPT_DIFF_*() macros.
but other than that, looked reasonable to me.
Thanks.There was a problem hiding this comment.
On the Git mailing list, Phillip Wood wrote (reply to this):
On 30/06/2025 18:03, Junio C Hamano wrote:
> "Leon Michalak via GitGitGadget" <gitgitgadget@gmail.com> writes:
> >> From: Leon Michalak <leonmichalak6@gmail.com>
>>
>> This patch compliments the previous commit, where builtins that use
>> add-patch infrastructure now respect diff.context and
>> diff.interHunkContext file configurations.
>>
>> In particular, this patch helps users who don't want to set persistent
>> context configurations or just want a way to override them on a one-time
>> basis, by allowing the relevant builtins to accept corresponding command
>> line options that override the file configurations.
>>
>> This mimics commands such as diff and log, which allow for both context
>> file configuration and command line overrides.
> > I skimmed the patch briefly. I am not sure if it is a good idea to
> > * add OPT_DIFF_*() macros to parse-options API, as its utility is
> very narrow, and forces those who are learning parse-options API
> to learn one more thing.
It means that we have consistent help for all the commands with these options which I think is valuable. We have a number of other macros that define options that are shared between commands and I think that works quite well.
> > * validation of the value range to be duplicated for each and every
> users of the new OPT_DIFF_*() macros.
Yes the validation is awkward. If we changed the OPT_DIFF_* to use a callback that rejected negative values that would reduce the duplication.
> but other than that, looked reasonable to me.
I've left a couple of comments on the tests but the code changes look reasonable to me too
Thanks
Phillip
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Phillip Wood <phillip.wood123@gmail.com> writes:
>> * add OPT_DIFF_*() macros to parse-options API, as its utility is
>> very narrow, and forces those who are learning parse-options API
>> to learn one more thing.
>
> It means that we have consistent help for all the commands with these
> options which I think is valuable. We have a number of other macros
> that define options that are shared between commands and I think that
> works quite well.
I understand that principe. What I was wondering was if there are
enough places to use these particular ones to make it worthwhile to
enlarge the set of OPT_* macros.
>> * validation of the value range to be duplicated for each and
>> every
>> users of the new OPT_DIFF_*() macros.
>
> Yes the validation is awkward. If we changed the OPT_DIFF_* to use a
> callback that rejected negative values that would reduce the
> duplication.
Yeah, I was wondering about that approach, too. Another benefit
with the "validate just after we parse the value before we assign
the result to a variable or a struct member" approach is that we can
also complain about -1 that is given from the command line (which
the current code ignores, if I am not mistaken, because it needs to
be silent if that -1 is there merely because it is the "not set yet"
sentinel value).
Or perhaps the valid value range Réne has been workingon canbe used
here?
|
This patch series was integrated into seen via git@d738e2b. |
| @@ -0,0 +1,10 @@ | |||
| `-U<n>`:: | |||
There was a problem hiding this comment.
On the Git mailing list, Phillip Wood wrote (reply to this):
Hi Leon
On 28/06/2025 17:34, Leon Michalak via GitGitGadget wrote:
> From: Leon Michalak <leonmichalak6@gmail.com>
> > +for cmd in add checkout restore 'commit -m file'
> +do
> + test_expect_success "${cmd%% *} accepts -U and --inter-hunk-context" "
Looking at this again, I think the test bodies here and below should be wrapped in single quotes because they are passed to eval and we want to expand $cmd when the body is evaluated, not before. That would also simplify the quoting inside the tests as we don't need to escape double quotes. That's not your fault - you've just copied what I suggested before.
> +test_expect_success 'The -U option overrides diff.context for "add"' '
> + test_config diff.context 8 &&
> + git add -U4 -p >output &&
> + test_grep ! "^ firstline" output
> +'
Don't the tests above check this as they set diff.context and diff.interhunkcontext and pass different values to -U and --inter-hunk-context?
Thanks
Phillip
add-interactive.c
Outdated
| @@ -39,8 +39,12 @@ 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) | |||
There was a problem hiding this comment.
On the Git mailing list, Phillip Wood wrote (reply to this):
Hi Leon
On 28/06/2025 17:34, Leon Michalak via GitGitGadget wrote:
> From: Leon Michalak <leonmichalak6@gmail.com>
> > 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.
This looks good to me, thanks for working on it
Phillip
> Signed-off-by: Leon Michalak <leonmichalak6@gmail.com>
> ---
> add-interactive.c | 17 +++++++++++++++++
> add-interactive.h | 1 +
> add-patch.c | 6 ++++++
> t/t3701-add-interactive.sh | 22 ++++++++++++++++++++++
> 4 files changed, 46 insertions(+)
> > diff --git a/add-interactive.c b/add-interactive.c
> index 97ff35b6f12a..e0aafb8dd02a 100644
> --- a/add-interactive.c
> +++ b/add-interactive.c
> @@ -39,8 +39,12 @@ 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)
> {
> const char *value;
> + int context;
> + int interhunkcontext;
> > s->r = r;
> + s->context = -1;
> + s->interhunkcontext = -1;
> > if (repo_config_get_value(r, "color.interactive", &value))
> s->use_color = -1;
> @@ -78,6 +82,19 @@ void init_add_i_state(struct add_i_state *s, struct repository *r)
> repo_config_get_string(r, "diff.algorithm",
> &s->interactive_diff_algorithm);
> > + if (!repo_config_get_int(r, "diff.context", &context)) {
> + if (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)
> + 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)
> setbuf(stdin, NULL);
> diff --git a/add-interactive.h b/add-interactive.h
> index 693f125e8e4b..c63f35b14be8 100644
> --- a/add-interactive.h
> +++ b/add-interactive.h
> @@ -18,6 +18,7 @@ struct add_i_state {
> > int use_single_key;
> char *interactive_diff_filter, *interactive_diff_algorithm;
> + int context, interhunkcontext;
> };
> > void init_add_i_state(struct add_i_state *s, struct repository *r);
> diff --git a/add-patch.c b/add-patch.c
> index 95c67d8c80c4..b43ca1600738 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -415,6 +415,8 @@ 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;
> 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';
> @@ -424,6 +426,10 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
> 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 (s->revision) {
> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index b088ee141ff4..18dc329ea1f6 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -1230,4 +1230,26 @@ 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 &&
> + 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
> +'
> +
> +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 '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
> +'
> +
> test_done
|
This patch series was integrated into seen via git@e275e24. |
|
There was a status update in the "Cooking" section about the branch "git add/etc -p" now honors diff.context configuration variable, and learns to honor -U<n> option. Ready? source: <pull.1915.v3.git.1751128486.gitgitgadget@gmail.com> |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This series of patches attempt to give
--interactive/--patchcompatible 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
--patchcommands (apart from one workaround by usingGIT_DIFF_OPTS=-u<number> ..., however this isn't a good user experience or a persistent solution). Additionally, the behaviour around reading from thediff.contextanddiff.interHunkContextconfigs 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.contextanddiff.interHunkContextconfigs are now respected by--patchcompatible commands--unifiedand--inter-hunk-contextcommand line options have been added to--patchcompatible commands (which take prescendence over file configs)--interactivemode 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:
Changes since v1:
test_grepandtest_configutilsChanges since v2:
cc: "Kristoffer Haugsbakk" kristofferhaugsbakk@fastmail.com
cc: Eric Sunshine sunshine@sunshineco.com
cc: Christian Couder christian.couder@gmail.com
cc: Phillip Wood phillip.wood123@gmail.com