Re: [PATCH v4 4/4] add-patch: add diff.context command line overrides

[Leon Michalak <leonmichalak6@gmail.com>]

Hey Philip, looking again for a second time and re-reading previous
replies on this thread, I think I misunderstood a previous reply.

I will add these back as soon as I can get back to the PC, thanks for
spotting that!

On Tue, 22 Jul 2025 at 17:02, Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> Hi Leon
>
> On 19/07/2025 13:28, Leon Michalak via GitGitGadget wrote:
> > 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.
>
> Thanks for updating the quoting in the tests. Unfortunately this patch
> now deletes the tests added in the last commit which I don't think is
> correct.
>
> >
> > -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 &&
> I think there is some confusion here - why are we deleting the tests
> added in the last commit? This removes the test coverage for
> diff.context and diff.interHunkContext
>
> > +for cmd in add checkout restore 'commit -m file'
> > +do
> > +     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
> > +     '
> > +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 &&
> > -     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
>
> This is also deleting a test added in the last patch
>
> > +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 &&
> > +     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 \
> > +             stash -p -U 4 --inter-hunk-context 2 >actual &&
> > +     test_grep "@@ -2,20 +2,20 @@" 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
> > -'
>
> and so is this. The tests you're adding look good but we shouldn't be
> deleting the existing ones.
>
> > +for cmd in add checkout commit reset restore "stash save" "stash push"
> > +do
> > +     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_must_fail git $cmd -p --inter-hunk-context -3 2>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_must_fail git $cmd --inter-hunk-context 2 2>actual &&
> > +             test_grep -E ".--inter-hunk-context. requires .(--interactive/)?--patch." actual
> > +     '
> > +done
>
> This looks good as well
> >   test_done
> As I said last time I do not think the tests below add any value. They
> also do not compensate for the removal of the tests for diff.context
> that are deleted above as they all pass -U on the commandline.
>
> > diff --git a/t/t4055-diff-context.sh b/t/t4055-diff-context.sh
> > index 1384a8195705..0158fe6568cb 100755
> > --- a/t/t4055-diff-context.sh
> > +++ b/t/t4055-diff-context.sh
> > @@ -58,6 +58,36 @@ test_expect_success 'The -U option overrides diff.context' '
> >       test_grep ! "^ firstline" output
> >   '
> >
> > +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
> > +'
> > +
> > +test_expect_success 'The -U option overrides diff.context for "commit"' '
> > +     test_config diff.context 8 &&
> > +     ! git commit -U4 -p >output &&
> > +     test_grep ! "^ firstline" output
> > +'
> > +
> > +test_expect_success 'The -U option overrides diff.context for "checkout"' '
> > +     test_config diff.context 8 &&
> > +     git checkout -U4 -p >output &&
> > +     test_grep ! "^ firstline" output
> > +'
> > +
> > +test_expect_success 'The -U option overrides diff.context for "stash"' '
> > +     test_config diff.context 8 &&
> > +     ! git stash -U4 -p >output &&
> > +     test_grep ! "^ firstline" output
> > +'
> > +
> > +test_expect_success 'The -U option overrides diff.context for "restore"' '
> > +     test_config diff.context 8 &&
> > +     git restore -U4 -p >output &&
> > +     test_grep ! "^ firstline" output
> > +'
> > +
> >   test_expect_success 'diff.context honored by "diff"' '
> >       test_config diff.context 8 &&
> >       git diff >output &&
>
> Thanks
>
> Phillip
>