diff mbox series

git-difftool-helper.sh: learn a new way skip to save point

Message ID pull.870.git.1612711153591.gitgitgadget@gmail.com (mailing list archive)
State New
Headers show
Series git-difftool-helper.sh: learn a new way skip to save point | expand

Commit Message

ZheNing Hu Feb. 7, 2021, 3:19 p.m. UTC
From: ZheNing Hu <adlternative@gmail.com>

`git difftool` only allow us to select file to view In turn.
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
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.

Signed-off-by: ZheNing Hu <adlternative@gmail.com>
---
    git-difftool-helper.sh: learn a new way skip to save point
    
    this patch's origin discuss is here:
    https://lore.kernel.org/git/gOXOaoqn-E9A2ob7ykWEcDc7ZxmSwAjcP5CCFKfr5ejCOWZQ1lfAUZcbgYT9AyQCcDgJvCrnrtziXiels-Hxol3xlkGTVHk24SvAdaSUtKQ=@rtzoeller.com/
    
    git user may should travel the diff list to choice file diff to view, if
    they exit in midway,they must travel it again. I’m on the basis of the
    "difftool_skip_to" suggested by Junio,Provides a possibility for this
    user-friendly solution.
    
    Thanks!

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-870%2Fadlternative%2Fdifftool_save_point-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-870/adlternative/difftool_save_point-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/870

 git-difftool--helper.sh | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)


base-commit: e6362826a0409539642a5738db61827e5978e2e4

Comments

Junio C Hamano Feb. 7, 2021, 6:29 p.m. UTC | #1
"阿德烈 via GitGitGadget"  <gitgitgadget@gmail.com> writes:

> From: ZheNing Hu <adlternative@gmail.com>

I am a bit confused.  Are 胡哲宁 and 阿德烈 and ZeNing Hu all the
same person (I am asking that an earlier question came under the
name first listed in this sentence, and the patch uses the latter
two names, where I guess )?

>
> `git difftool` only allow us to select file to view In turn.
> 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
> 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.
>
> Signed-off-by: ZheNing Hu <adlternative@gmail.com>
> ---
>     git-difftool-helper.sh: learn a new way skip to save point
>     
>     this patch's origin discuss is here:
>     https://lore.kernel.org/git/gOXOaoqn-E9A2ob7ykWEcDc7ZxmSwAjcP5CCFKfr5ejCOWZQ1lfAUZcbgYT9AyQCcDgJvCrnrtziXiels-Hxol3xlkGTVHk24SvAdaSUtKQ=@rtzoeller.com/
>     
>     git user may should travel the diff list to choice file diff to view, if
>     they exit in midway,they must travel it again. I’m on the basis of the
>     "difftool_skip_to" suggested by Junio,Provides a possibility for this
>     user-friendly solution.
>     
>     Thanks!
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-870%2Fadlternative%2Fdifftool_save_point-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-870/adlternative/difftool_save_point-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/870
>
>  git-difftool--helper.sh | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
>
> 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")
> +			if test $SAVE_POINT_NUM -le $GIT_DIFF_PATH_TOTAL &&
> +				test $SAVE_POINT_NUM -gt $GIT_DIFF_PATH_COUNTER
> +			then
> +				# choice skip or not skip when check first file.
> +				if test $GIT_DIFF_PATH_COUNTER -eq "1"
> +				then
> +					printf "do you want to skip to last time difftool save point($SAVE_POINT_NUM) [Y/n]?"
> +					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
> +		then
> +			echo $GIT_DIFF_PATH_COUNTER > $GIT_DIFFTOOL_SKIP_TO_FILE
> +		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
>
> base-commit: e6362826a0409539642a5738db61827e5978e2e4
Junio C Hamano Feb. 7, 2021, 7:04 p.m. UTC | #2
Sorry, but a not-yet-written reply went out by accident; please
discard it.

> `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?).

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.

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).

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.


> 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.

> +			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.

> +				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?

> +					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 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"




> +		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.
ZheNing Hu Feb. 8, 2021, 8:06 a.m. UTC | #3
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.
diff mbox series

Patch

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")
+			if test $SAVE_POINT_NUM -le $GIT_DIFF_PATH_TOTAL &&
+				test $SAVE_POINT_NUM -gt $GIT_DIFF_PATH_COUNTER
+			then
+				# choice skip or not skip when check first file.
+				if test $GIT_DIFF_PATH_COUNTER -eq "1"
+				then
+					printf "do you want to skip to last time difftool save point($SAVE_POINT_NUM) [Y/n]?"
+					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
+		then
+			echo $GIT_DIFF_PATH_COUNTER > $GIT_DIFFTOOL_SKIP_TO_FILE
+		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