Junio C Hamano <gitster@xxxxxxxxx> 于2021年2月8日周一 上午3:04写道：
>
> Sorry, but a not-yet-written reply went out by accident; please
> discard it.
>
Never mind. I have synchronized different signatures of git, gmail,
github.
> > `git difftool` only allow us to select file to view In turn.
>
> Funny capitalization "In"?
>
> > If there is a commit with many files and we exit in search,
> > We will have to traverse list again to get the file diff which
> > we want to see.Therefore,here is a new method:every time before
>
> It makes it hard to lack SP after punctuation like '.', ',', and
> ':'.
>
> > we view the file diff,the current coordinates will be stored in
> > `GIT_DIR/difftool_skip_to`,this file will be deleted after
> > successful traversing.But if an unexpected exit occurred midway,
> > git will view the coordinates in the save point,ask user if they
> > want continue from the last saved point.This will improve the
> > user experience.
>
> I think the idea sounds good. Admittedly I do not use difftool
> myself, so I do not even know if and how the current end user
> experience is so bad to require a patch like this (e.g. I do not
> know how "unexpected exit" is "unexpected"---isn't it the end user
> initiated action to "quit", or does the tool crash or something?).
>
Generally speaking, It is the user of git manually use [Ctrl+c].
However, if the program itself fails and causes the exit, I think
this "save point" can also be well recorded, because it will be
stored before view the diff.
> So I won't be the best qualified person to judge if the solution
> presented is the best one for the problem.
>
> $ git shortlog --no-merges git-diff-helper.sh
>
> might be a good way to find whom to ask for review and help.
>
Thanks for reminding, I will -cc these authors.
> Having said that, I do have one opinion on the "skip-to" filename.
> I do not think it is wise to call it after the purpose you want to
> use it for (i.e. "I want to use it to skip to the recorded
> position"). Instead, if the file records "the last visited
> position", it is better to name it after that
> (e.g. "difftool-last-position". If it records "the next file to be
> visited", then "difftool-next-file" may be a good name).
>
Indeed, "last-position" can better express this patch function.
I will modify it according to your suggestions.
> The reason is because your first design may be to visit the file the
> user was visiting before the "crash" happened, but you may later
> want to revise the design to allow the user to say "start at one
> file before the file I was visiting" etc. The location recorded in
> the file may still be used to decide where the code skips to when
> restarting, but no longer exactly where the code "skips to". If you
> name it after what it is, not what it is (currently) used for, the
> design would become clearer.
>
You are right,But I think based on this patch, the function of "skip to"
may can be added later.
>
> > diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh
> > index 46af3e60b718..56ec1d38a7a1 100755
> > --- a/git-difftool--helper.sh
> > +++ b/git-difftool--helper.sh
> > @@ -6,6 +6,7 @@
> > # Copyright (c) 2009, 2010 David Aguilar
> >
> > TOOL_MODE=diff
> > +GIT_DIFFTOOL_SKIP_TO_FILE="$GIT_DIR/difftool-skip-to"
> > . git-mergetool--lib
> >
> > # difftool.prompt controls the default prompt/no-prompt behavior
> > @@ -40,6 +41,31 @@ launch_merge_tool () {
> > # the user with the real $MERGED name before launching $merge_tool.
> > if should_prompt
> > then
> > + if test -f "$GIT_DIFFTOOL_SKIP_TO_FILE"
> > + then
> > + SAVE_POINT_NUM=$(cat "$GIT_DIFFTOOL_SKIP_TO_FILE")
>
> You can avoid the TOCTTOU race by
>
> if SAVE_POINT=$(cat 2>/dev/null "$GIT_DIFFTOOL_SKIP_TO_FILE")
> then
>
> but that wouldn't probably matter in this application.
>
> > + if test $SAVE_POINT_NUM -le $GIT_DIFF_PATH_TOTAL &&
> > + test $SAVE_POINT_NUM -gt $GIT_DIFF_PATH_COUNTER
>
> Think what happens if the file is corrupt and SAVE_POINT_NUM has (1)
> an empty string, (2) garbage that has $IFS whitespace, (3) non
> number, in it. At least, quoting the variable inside double-quotes,
> i.e. "$SAVE_POINT_NUM", would help an error condition reported
> correctly at the runtime.
Understand now.A variable with '""'can show correct error usage when
these error conditions occur.
>
> > + then
> > + # choice skip or not skip when check first file.
>
> A bit funny language. Isn't the code clear enough without this comment?
>
> > + if test $GIT_DIFF_PATH_COUNTER -eq "1"
>
> No need to quote the constant "1"; quoting the variable side may be
> a good practice, even though I think in this codepath we know
> GIT_DIFF_PATH_COUNTER is a well-formatted number.
Truly. I will use "DIFFTOOL_FIRST_NUM" instread of "1".
>
> > + then
> > + printf "do you want to skip to last time difftool save point($SAVE_POINT_NUM) [Y/n]?"
>
> "Skip" is probably an implementation detail that the user does not
> have to know. "Do you want to start from the last file you were
> viewing?", perhaps?
Yeah. Because users may choice another totally different diff,
I will use "Do you want to start from the possible last file you
were viewing?".
>
> > + read skip_ans || return
> > + if test "$skip_ans" = y
> > + then
> > + return
> > + fi
> > + else
> > + return
> > + fi
> > + fi
> > + fi
> > + # write the current coordinates to .git/difftool-skip-to
> > + if test !$SAVE_POINT_NUM || $SAVE_POINT_NUM -ne $GIT_DIFF_PATH_COUNTER
>
> Have this code been tested? I think "test" is missing after the
> "||", and I am not quite sure what you are trying to check with
> "test !$SAVE_POINT_NUM", either. The "test" utility, when given a
> non-operator string (like "!23" this one is checking when the last
> visited path was the 23rd one), returns true if the string is not an
> empty string, and by definition a string made by appending anything
> after "!" would not be empty, so the entire "|| $SAVE_POINT_NUM ..."
> have been skipped in your test, I think.
>
Is indeed a mistake of mine, `test -z "$SAVE_POINT_NUM"` will be fine.
Shell script syntax I will pay more attention.
> Is writing the current position to the file unconditionally good
> enough? After all, we are about to go interactive with the user, so
> the body of this "if" statement won't be performance critical in any
> sense, no? Or is there something more subtle going on and
> correctness of the code depends on this condition? I cannot quite
> tell.
>
> > + then
> > + echo $GIT_DIFF_PATH_COUNTER > $GIT_DIFFTOOL_SKIP_TO_FILE
>
> echo "$GIT_DIFF_PATH_COUNTER" >"$GIT_DIFFTOOL_SKIP_TO_FILE"
>
> cf. Documentation/CodingGuidelines
>
> - Redirection operators should be written with space before, but no
> space after them. In other words, write 'echo test >"$file"'
> instead of 'echo test> $file' or 'echo test > $file'. Note that
> even though it is not required by POSIX to double-quote the
> redirection target in a variable (as shown above), our code does so
> because some versions of bash issue a warning without the quotes.
>
> (incorrect)
> cat hello > world < universe
> echo hello >$world
>
> (correct)
> cat hello >world <universe
> echo hello >"$world"
>
>
>
>
OK, I will read Documentation/CodingGuidelines more times.
> > + fi
> > printf "\nViewing (%s/%s): '%s'\n" "$GIT_DIFF_PATH_COUNTER" \
> > "$GIT_DIFF_PATH_TOTAL" "$MERGED"
> > if use_ext_cmd
> > @@ -102,4 +128,10 @@ else
> > done
> > fi
> >
> > +if test -f $GIT_DIFFTOOL_SKIP_TO_FILE &&
> > + test $GIT_DIFF_PATH_COUNTER -eq $GIT_DIFF_PATH_TOTAL
> > +then
> > + rm $GIT_DIFFTOOL_SKIP_TO_FILE
> > +
> > +fi
> > exit 0
>
> Wouldn't it be simpler to clear when we have reached at the end, i.e.
>
> if test "$GIT_DIFF_PATH_COUNTER" -eq "$GIT_DIFF_PATH_TOTAL"
> then
> rm -f "$GIT_DIFFTOOL_SKIP_TO_FILE"
> fi
>
> Thanks.
Thanks for the advice and correct, Junio.
