Junio C Hamano <gitster@xxxxxxxxx> 于2021年2月10日周三 上午2:17写道：
>
> "ZheNing Hu via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
>
> > diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt
> > index 484c485fd06c..552be097dfea 100644
> > --- a/Documentation/git-difftool.txt
> > +++ b/Documentation/git-difftool.txt
> > @@ -34,6 +34,9 @@ OPTIONS
> > This is the default behaviour; the option is provided to
> > override any configuration settings.
> >
> > +--start-from::
> > + Start viewing diff from the specified file.
> > +
>
> There is nothing that specifies a file in the above ;-)
>
> --start-from=<file>::
> Start viewing ...
>
> This is even more important as SYNOPSIS section of this manual page
> does not duplicate the list of options and their arguments.
>
I admit my mistake,thanks for reminding.
> > -t <tool>::
> > --tool=<tool>::
> > Use the diff tool specified by <tool>. Valid values include
>
> There are many things I dislike about this patch, but do not take it
> as a personal attack. I'll try to suggest an alternative at the end,
> but read along.
>
Of course I humbly accepted your criticism,I am only a sophomore in
university after all,It is extremely important to listen to your suggestions.
> > diff --git a/builtin/difftool.c b/builtin/difftool.c
> > index 6e18e623fddf..67d2befa1210 100644
> > --- a/builtin/difftool.c
> > +++ b/builtin/difftool.c
> > @@ -690,7 +690,7 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)
> > {
> > int use_gui_tool = 0, dir_diff = 0, prompt = -1, symlinks = 0,
> > tool_help = 0, no_index = 0;
> > - static char *difftool_cmd = NULL, *extcmd = NULL;
> > + static char *difftool_cmd = NULL, *extcmd = NULL, *start_file = NULL;
> > struct option builtin_difftool_options[] = {
> > OPT_BOOL('g', "gui", &use_gui_tool,
> > N_("use `diff.guitool` instead of `diff.tool`")),
> > @@ -714,6 +714,8 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)
> > OPT_STRING('x', "extcmd", &extcmd, N_("command"),
> > N_("specify a custom command for viewing diffs")),
> > OPT_ARGUMENT("no-index", &no_index, N_("passed to `diff`")),
> > + OPT_STRING(0, "start-from", &start_file, N_("start-from"),
> > + N_("start viewing diff from the specified file")),
> > OPT_END()
> > };
>
> This may be a good UI to "difftool".
>
> > @@ -724,6 +726,9 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)
> > builtin_difftool_usage, PARSE_OPT_KEEP_UNKNOWN |
> > PARSE_OPT_KEEP_DASHDASH);
> >
> > + if (start_file)
> > + setenv("START_FILE", start_file, 1);
>
> Unacceptable. Nothing gives Git the right to squat on such a
> generic name, and there is no hint that it is used to specify the
> starting point of what operation. In addition, I do not see a good
> reason why we need to use an environment variable in the first
> place. We run "diff" as an external process, with GIT_EXTERNAL_DIFF
> environment pointing back at us, no? This information should be
> passed from its command line.
Sure,I using environment variables originally hoped can be used in
git--difftool-helper.sh, later I found it was not easy to deal with, so I am
attempt to use environment variables to transmit information in
`run_external_diff`,now it seems that this method is not very good ...
>
> > diff --git a/diff.c b/diff.c
> > index 69e3bc00ed8f..cdad26f99063 100644
> > --- a/diff.c
> > +++ b/diff.c
> > @@ -4249,6 +4249,7 @@ static void run_external_diff(const char *pgm,
> > const char *xfrm_msg,
> > struct diff_options *o)
> > {
> > + const char *start_file = NULL;
> > struct strvec argv = STRVEC_INIT;
> > struct strvec env = STRVEC_INIT;
> > struct diff_queue_struct *q = &diff_queued_diff;
> > @@ -4272,9 +4273,17 @@ static void run_external_diff(const char *pgm,
> >
> > diff_free_filespec_data(one);
> > diff_free_filespec_data(two);
> > +
> > + start_file = xstrdup_or_null(getenv("START_FILE"));
> > + if (start_file) {
> > + if (strcmp(start_file, name))
> > + goto finish;
> > + unsetenv("START_FILE");
> > + }
>
> Again, an unacceptable "hack". "start the diff output showing from
> this path" would plausibly a good feature even outside the scope of
> "difftool", and the feature should not be limited to the external
> diff interface. More on this later.
What you said `from its command line` is using `git difftool
--rotate-to=<file>`,
Maybe I didn’t know whether to deal with the subcommands of `diff`, now I admit
that passing from parameters is better than passing environment variables.
>
> > if (run_command_v_opt_cd_env(argv.v, RUN_USING_SHELL, NULL, env.v))
> > die(_("external diff died, stopping at %s"), name);
> >
> > +finish:
> > remove_tempfile();
> > strvec_clear(&argv);
> > strvec_clear(&env);
> > diff --git a/diff.h b/diff.h
> > index 2ff2b1c7f2ca..f67c43f5af95 100644
> > --- a/diff.h
> > +++ b/diff.h
> > @@ -86,18 +86,18 @@ typedef void (*diff_format_fn_t)(struct diff_queue_struct *q,
> >
> > typedef struct strbuf *(*diff_prefix_fn_t)(struct diff_options *opt, void *data);
> >
> > -#define DIFF_FORMAT_RAW 0x0001
> > -#define DIFF_FORMAT_DIFFSTAT 0x0002
> > -#define DIFF_FORMAT_NUMSTAT 0x0004
> > -#define DIFF_FORMAT_SUMMARY 0x0008
> > -#define DIFF_FORMAT_PATCH 0x0010
> > -#define DIFF_FORMAT_SHORTSTAT 0x0020
> > -#define DIFF_FORMAT_DIRSTAT 0x0040
> > +#define DIFF_FORMAT_RAW (1U<<0)
> > +#define DIFF_FORMAT_DIFFSTAT (1U<<1)
> > +#define DIFF_FORMAT_NUMSTAT (1U<<2)
> > +#define DIFF_FORMAT_SUMMARY (1U<<3)
> > +#define DIFF_FORMAT_PATCH (1U<<4)
> > +#define DIFF_FORMAT_SHORTSTAT (1U<<5)
> > +#define DIFF_FORMAT_DIRSTAT (1U<<6)
> >
> > /* These override all above */
> > -#define DIFF_FORMAT_NAME 0x0100
> > -#define DIFF_FORMAT_NAME_STATUS 0x0200
> > -#define DIFF_FORMAT_CHECKDIFF 0x0400
> > +#define DIFF_FORMAT_NAME (1U<<8)
> > +#define DIFF_FORMAT_NAME_STATUS (1U<<9)
> > +#define DIFF_FORMAT_CHECKDIFF (1U<<10)
>
> Do we need these changes for the new feature to work?
It has no effect on this new feature. I should put this modification
in an additional commit, right?
>
> I also find this "skip and discard ones earlier than the given path"
> makes the utility of the feature artificially narrower than needed,
> when I imagine how else this feature, or a variant of it, would be
> useful in other situations. For example, consider that there are 5
> paths, and you've seen 3 of them so far before you went off, so you
> are restarting from 4th file. But wouldn't it be more useful, after
> showing the 4th and 5th file, if the tool wraps around to show 1st,
> 2nd, and 3rd file if the user kept going?
>
Well, it would be better if you can see 1st, 2nd, 3rd again.
> I suspect that this feature fits the overall system much better if
> it is implemented as a new step in the diffcore transformation. The
> way our diff subsystem works is roughly:
>
> * The front-end "diff", "diff-files", "diff-index" and "diff-trees"
> are given two "tree like things" to compare, and feeds bunch of
> <old, new> tuples to the diff internal machinery. Each of these
> tuples are called "filepairs", and a "pair" has two "filespecs",
> one describing the contents, the mode, and the path in the "old"
> side of the "tree like things", the other describing the
> contents, the mode, and the path in the "new" side of the "tree
> like things".
>
> * The series of filepairs are given to the "diffcore" machinery,
> where they may be broken (i.e. a filepair that says that the old
> contents of "hello.txt" was X, and the new contents of
> "hello.txt" is Y, may become two filepairs, one that says that
> the "hello.txt" file with contents X used to be in the old tree
> but there is nothing corresponding to it in the new tree, and the
> other says that a new "hello.txt" with contents Y appeared
> without corresponding thing in the old tree), matched (i.e. there
> may originally be two filepairs, one that says path A.txt appears
> on the old side but disappeared on the new side, and the other
> that says path sub/A.txt did not exist on the old side but
> appears on the new side---these two filepairs may be merged to
> express "A.txt on the old side got renamed to sub/A.txt on the
> new side"), etc.
>
Thank you for these interpretations on "git diff" for me, it will help me
understand its principle.
> * The set of filepairs processed in the "diffcore" machinery is
> given to the backend and each filepair will be shown in the
> output, as a series of patches, a diffstat, etc.
>
> There is a step in the "diffcore" machinery called "diffcore-order".
> The front-ends all feed the filepairs alphabetically to the
> "diffcore" machinery, but "diffcore-order" can reorder them, so that
> the original order that the "git diff HEAD --" frontend found the
> changes may be to "hello.c" and then "hello.h" (because .c sorts
> before .h), but the users can specify with the "-O<orderfile>"
> option that they want to view the header files before the source
> files. When the set of filepairs exits the diffcore machinery, the
> original "hello.c" then "hello.h" order may get modified to show
> "hello.h" then "hello.c". It probably is the simplest to model this
> new feature after how "diffcore-order" does it.
>
Very magical "diffcore-order", when I was looking for a solution yesterday,
I noticed that these functions similar to "diffcore_apply_filter" in
"diffcore_std",
> So, perhaps we can introduce a new "diffcore-rotate" step, where the
> filepairs before the specified location are rotated out to the end
> of the filepairs?
>
Awesome idea. In this way, `difftool --rotate-to=<file>` can call
`diff --rotate-to=<file>` , user can choose the starting file, and they can
also see previous files.
> The following is just a quick draft that is only lightly tested by
> running itself with the "--rotate-to=diffcore-rotate.c" option, but
> it should be sufficient to get you started. You should add a way to
> diagnose that the name given to the "--start-file" option actually
> exists in the diff on the "difftool" side, because a path that does
> not exist in the patch with the following code is simply ignored
> (and that is very much deliberate, because we do not want it to die
> while running "git log -p --rotate-to=X" where some changes may or
> may not touch X).
Yes, I want to know why being so cautious in git log?If the file name is
wrong, why can't I make it exit? :)
>
>
> $ ./git diff --stat -p --rotate-to=diffcore-rotate.c HEAD
>
> diffcore-rotate.c | 35 +++++++++++++++++++++++++++++++++++
> diffcore.h | 1 +
> Makefile | 1 +
> diff.c | 4 ++++
> diff.h | 1 +
> 5 files changed, 42 insertions(+)
>
> diff --git c/diffcore-rotate.c w/diffcore-rotate.c
> new file mode 100644
> index 0000000000..0d17901616
> --- /dev/null
> +++ w/diffcore-rotate.c
> @@ -0,0 +1,35 @@
> +/*
> + * Copyright (C) 2021, Google LLC.
> + * Based on diffcore-order.c, which is Copyright (C) 2005, Junio C Hamano
> + */
> +#include "cache.h"
> +#include "diff.h"
> +#include "diffcore.h"
> +
> +void diffcore_rotate(const char *rotate_to_filename)
> +{
> + struct diff_queue_struct *q = &diff_queued_diff;
> + struct diff_queue_struct outq;
> + int rotate_to, i;
> +
> + if (!q->nr)
> + return;
> +
> + for (i = 0; i < q->nr; i++)
> + if (!strcmp(rotate_to_filename, q->queue[i]->two->path))
> + break;
> + /* we did not find the specified path */
> + if (q->nr <= i)
> + return;
> +
> + DIFF_QUEUE_CLEAR(&outq);
> + rotate_to = i;
> +
> + for (i = rotate_to; i < q->nr; i++)
> + diff_q(&outq, q->queue[i]);
> + for (i = 0; i < rotate_to; i++)
> + diff_q(&outq, q->queue[i]);
> +
> + free(q->queue);
> + *q = outq;
> +}
> diff --git c/diffcore.h w/diffcore.h
> index d2a63c5c71..bd5959375b 100644
> --- c/diffcore.h
> +++ w/diffcore.h
> @@ -164,6 +164,7 @@ void diffcore_rename(struct diff_options *);
> void diffcore_merge_broken(void);
> void diffcore_pickaxe(struct diff_options *);
> void diffcore_order(const char *orderfile);
> +void diffcore_rotate(const char *rotate_to_filename);
>
> /* low-level interface to diffcore_order */
> struct obj_order {
> diff --git c/Makefile w/Makefile
> index b797033c58..031b0b88e6 100644
> --- c/Makefile
> +++ w/Makefile
> @@ -869,6 +869,7 @@ LIB_OBJS += diffcore-delta.o
> LIB_OBJS += diffcore-order.o
> LIB_OBJS += diffcore-pickaxe.o
> LIB_OBJS += diffcore-rename.o
> +LIB_OBJS += diffcore-rotate.o
> LIB_OBJS += dir-iterator.o
> LIB_OBJS += dir.o
> LIB_OBJS += editor.o
> diff --git c/diff.c w/diff.c
> index 69e3bc00ed..90a8d5abd0 100644
> --- c/diff.c
> +++ w/diff.c
> @@ -5599,6 +5599,8 @@ static void prep_parse_options(struct diff_options *options)
> DIFF_PICKAXE_REGEX, PARSE_OPT_NONEG),
> OPT_FILENAME('O', NULL, &options->orderfile,
> N_("control the order in which files appear in the output")),
> + OPT_STRING(0, "rotate-to", &options->rotate_to, N_("<path>"),
> + N_("show the change in the specified path first")),
> OPT_CALLBACK_F(0, "find-object", options, N_("<object-id>"),
> N_("look for differences that change the number of occurrences of the specified object"),
> PARSE_OPT_NONEG, diff_opt_find_object),
> @@ -6669,6 +6671,8 @@ void diffcore_std(struct diff_options *options)
> diffcore_pickaxe(options);
> if (options->orderfile)
> diffcore_order(options->orderfile);
> + if (options->rotate_to)
> + diffcore_rotate(options->rotate_to);
> if (!options->found_follow)
> /* See try_to_follow_renames() in tree-diff.c */
> diff_resolve_rename_copy();
> diff --git c/diff.h w/diff.h
> index 2ff2b1c7f2..0801469e63 100644
> --- c/diff.h
> +++ w/diff.h
> @@ -226,6 +226,7 @@ enum diff_submodule_format {
> */
> struct diff_options {
> const char *orderfile;
> + const char *rotate_to;
>
> /**
> * A constant string (can and typically does contain newlines to look for
Thanks for the patch!
After that, there was too little work I could do,do i just need to add
the following
code in `diff_flush_patch_all_file_pairs`?
if (o->rotate_to && q->nr && strcmp(q->queue[0]->one->path, o->rotate_to) &&
strcmp(q->queue[0]->one->path, o->rotate_to)) {
error(_("could not find start file name '%s'"), o->rotate_to);
return;
}
In addition, Do I need to do the documentation and tests related to
your `diff --rotate-to`?
If so, I will finish it, otherwise, I will only do corresponding tests
for `git difftool --rotate-to`.
Thank you Junio, you are like a tireless teacher,
Happy Chinese New Year!
--
ZheNing Hu
