Re: [PATCH v2] git-difftool-helper.sh: learn a new way go back to last save point

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 8, 2021, 20:13 UTC
Message-ID: <xmqqczxa5nct.fsf@gitster.c.googlers.com>
In-Reply-To: <pull.870.v2.git.1612803744188.gitgitgadget@gmail.com>

"ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 10 quoted lines

> diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh
> index 46af3e60b718..a01aa7c9d551 100755
> --- a/git-difftool--helper.sh
> +++ b/git-difftool--helper.sh
> @@ -6,6 +6,8 @@
>  # Copyright (c) 2009, 2010 David Aguilar
>  
>  TOOL_MODE=diff
> +GIT_DIFFTOOL_LAST_POSITION="$GIT_DIR/difftool-last-position"
> +DIFFTOOL_FIRST_NUM="1"

Do we need this constant? I do not think it makes the resulting code easier to follow.

Show 13 quoted lines

>  . git-mergetool--lib
>  
>  # difftool.prompt controls the default prompt/no-prompt behavior
> @@ -40,6 +42,30 @@ launch_merge_tool () {
>  	# the user with the real $MERGED name before launching $merge_tool.
>  	if should_prompt
>  	then
> +		if test -f "$GIT_DIFFTOOL_LAST_POSITION"
> +		then
> +			if SAVE_POINT_NUM=$(cat 2>/dev/null "$GIT_DIFFTOOL_LAST_POSITION") &&
> +				test "$SAVE_POINT_NUM" -le "$GIT_DIFF_PATH_TOTAL" &&
> +					test "$SAVE_POINT_NUM" -gt "$GIT_DIFF_PATH_COUNTER"
> +			then

No need to push the subsequent lines that far to the right, especially when your variable names are already overly long. It just makes things harder to read.

			if test -r "$GIT_DIFFTOOL_LAST_POSITION" &&
			   SAVE_POINT_NUM=$(cat "$GIT_DIFFTOOL_LAST_POSITION") &&
			   test "$SAVE_POINT_NUM" -le "$GIT_DIFF_PATH_TOTAL" &&
			   test "$SAVE_POINT_NUM" -gt "$GIT_DIFF_PATH_COUNTER"
			then

> +				if test "$GIT_DIFF_PATH_COUNTER" -eq "$DIFFTOOL_FIRST_NUM"
> +				then
> +					printf "Do you want to start from the possible last file you were viewing? [Y/n]?"

Where does that "possible" come from? If the reason is "We might have miscomputed or misrecorded an incorrect last position", we probably should work harder to make sure we don't ;-)

At this point in the code, do we have the _name_ of the file we are going to skip to readily available, or do we actually need to seek to that position before we can find it out?

	You were looking at 'hello-world.txt' the last time.
	Do you want to restart from there [Y/n]?

would be far easier to answer for an end-user than an unspecified "possible last file".

> +		fi
> +		if test -z "$SAVE_POINT_NUM" ||
> +			test "$SAVE_POINT_NUM" -ne "$GIT_DIFF_PATH_COUNTER"

Ditto about indentation.

Is this behaviour something we can write a test for in t/t7800-difftool.sh, by the way?

Other than these, i.e. (cosmetic) indentation and overly long lines (ui) giving filename is easier to work with for the users and (dev) lack of tests, looking quite good.

Thanks.