Re: [PATCH] git-difftool-helper.sh: learn a new way skip to save point

From: 胡哲宁 <adlternative@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: "阿德烈 via GitGitGadget" <gitgitgadget@gmail.com>,
	"Git List" <git@vger.kernel.org>
Subject: Re: [PATCH] git-difftool-helper.sh: learn a new way skip to save point
Date: Mon, 8 Feb 2021 16:06:20 +0800	[thread overview]
Message-ID: <CAOLTT8TnkzU397Bnx1NdpJY-P4fYpTPzjtuzwPzLEpE_Si4Fjw@mail.gmail.com> (raw)
In-Reply-To: <xmqq8s7z8zsg.fsf@gitster.c.googlers.com>

Junio C Hamano <gitster@pobox.com> 于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.