Re: [PATCH v5 2/2] difftool.c: learn a new way start at specified file

Junio C Hamano [off-list ref] 于2021年2月17日周三 下午6:31写道：

"ZheNing Hu via GitGitGadget" [off-list ref] writes:

quoted

From: ZheNing Hu <redacted>

`git difftool` only allow us to select file to view in turn.
If there is a commit with many files and we exit in the search,

I am not sure what "in the search" refers to.  "in the middle" I
would understand, though.

quoted

We will have to traverse list again to get the file diff which

Let's downcase this "We".

quoted

we want to see. Therefore, here is a new method: user can use
`git difftool --rotate-to=<filename>` or `git difftool --skip-to=<filename>`
to start viewing from the specified file, This will improve the
user experience.

Do we need both?  I'd rather not to give end-user-facing commands
too many knobs that would do similar things.  Too many choices to
choose from without clear answer to "which one should I prefer to
use?" is a bad combination for end-users.

So users will not need to use `git difftool --skip-to`? Then I am confused
about the meaning of `git difftool --skip-to`.

quoted

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 7c5b3cf42bcc..aa2b5c11f20b 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt

@@ -701,7 +701,7 @@ components matches the pattern.  For example, the pattern "`foo*bar`"
 matches "`fooasdfbar`" and "`foo/bar/baz/asdf`" but not "`foobarx`".

 --skip-to=<file>::
---rotate-to=<file::
+--rotate-to=<file>::
      Discard the files before the named <file> from the output
      (i.e. 'skip to'), or move them to the end of the output
      (i.e. 'rotate to').  These were invented primarily for use

Thanks for correcting, but this change should not be a part of this
patch.  Instead, you help the other's topic by giving a review (and
you could just have said "there there is closing '>' missing").

Okay, I will pay attention later.

quoted

diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt
index 484c485fd06c..c64dff69c976 100644
--- a/Documentation/git-difftool.txt
+++ b/Documentation/git-difftool.txt

@@ -34,6 +34,16 @@ OPTIONS
      This is the default behaviour; the option is provided to
      override any configuration settings.

+--rotate-to=<file>::
+     Internally call `git diff --rotate-to=<file>`,
+     show the change in the specified path first.
+     Files before the specified path will be moved to the last output.
+
+--skip-to=<file>::
+     Internally call `git diff --skip-to=<file>`,
+     skip the output to the specified path.
+     Files before the specified path will not output.
+

This, unlike the "diffcore" stuff, is end-user facing, and it is
better not to force the readers even know what --skip-to option
to the diff does (after all, difftool users are using 'git difftool'
and they are not necessarily 'git diff' users).

    --skip-to=<file>::
            Start showing the diff for the given path, skipping all
            the paths before it.

or something, perhaps.

quoted

+test_expect_success 'difftool --skip-to' '
+     difftool_test_setup &&
+     test_when_finished git reset --hard &&
+     git difftool --no-prompt --extcmd=cat --skip-to="2" HEAD^ >output &&
+     cat >expect <<-\EOF &&
+     2
+     4
+     EOF
+     test_cmp output expect &&
+     test_must_fail git difftool --no-prompt --extcmd=cat --skip-to="3" HEAD^
+'

This probably should be split into two independent tests.  One to
check that the non-failing case works as expected, the other to
check that a bogus command line option errors out as expected.

I will finish it.

Thanks.

Besides, I have some curiosity about one place in the code in your
patch:

int cmd_diff_tree(...)
...
if (read_stdin) {
...
opt->diffopt.rotate_to_strict = 0;
...

}
This is the only place where rotate_to_strict is set to zero,
So the "git log -p" you mentioned earlier is here Called this code
to avoid exiting  the program because of the wrong path, right?

Thanks.