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

From: ZheNing Hu <[email protected]>

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 <[email protected]>

---
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(+)

git-difftool--helper.sh +32 -0

index 46af3e60b718..56ec1d38a7a1 100755
@@ -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

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.

git-difftool--helper.sh +1 -0

index 46af3e60b718..56ec1d38a7a1 100755
@@ -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

git-difftool--helper.sh +1 -0

+		fi
 		printf "\nViewing (%s/%s): '%s'\n" "$GIT_DIFF_PATH_COUNTER" \
 			"$GIT_DIFF_PATH_TOTAL" "$MERGED"
 		if use_ext_cmd

Sorry, but a not-yet-written reply went out by accident; please discard it.

Quoted 1 line

git difftool only allow us to select file to view In turn.

Funny capitalization "In"?

Quoted 3 lines

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

Quoted 6 lines

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.

Quoted 14 lines

git-difftool--helper.sh +1 -0

index 46af3e60b718..56ec1d38a7a1 100755
@@ -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

git-difftool--helper.sh launch_merge_tool () { · line 46

Quoted 6 lines

git-difftool--helper.sh +3 -0

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

git-difftool--helper.sh launch_merge_tool () { · line 48

Quoted 2 lines

git-difftool--helper.sh +2 -0

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

git-difftool--helper.sh launch_merge_tool () { · line 50

Quoted 3 lines

git-difftool--helper.sh +2 -0

+                     then
+                             # choice skip or not skip when check first file.

A bit funny language. Isn't the code clear enough without this comment?

git-difftool--helper.sh launch_merge_tool () { · line 51

Quoted 1 line

git-difftool--helper.sh +1 -0

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

git-difftool--helper.sh launch_merge_tool () { · line 53

Quoted 3 lines

git-difftool--helper.sh +2 -0

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

git-difftool--helper.sh launch_merge_tool () { · line 65

Quoted 13 lines

git-difftool--helper.sh +12 -0

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

git-difftool--helper.sh launch_merge_tool () { · line 67

Quoted 2 lines

git-difftool--helper.sh +2 -0

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

Quoted 5 lines

git-difftool--helper.sh +1 -0

+             fi
              printf "\nViewing (%s/%s): '%s'\n" "$GIT_DIFF_PATH_COUNTER" \
                      "$GIT_DIFF_PATH_TOTAL" "$MERGED"
              if use_ext_cmd

git-difftool--helper.sh else · line 137

Quoted 10 lines

git-difftool--helper.sh +6 -0

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

From: ZheNing Hu <[email protected]>

git difftool only allow us to select file to view in turn. If there is a commit with many files and we exit in the 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-last-position, this file will be deleted after successful traversing. But if an unexpected exit occurred midway or users similar to using "ctrl+c" kill the process,and the user wants to redo the same git difftoool, git will view the coordinates in the save point, ask user if they want continue from the last position. This will improve the user experience.

Signed-off-by ZheNing Hu <[email protected]>

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

git user may should travel the diff list to choice file diff to view, if they exit in midway,they must travel it again. By saving current coordinates in GIT_DIR/difftool-last-position method, provides a possibility for this user-friendly solution.

this patch's origin discuss is here:
https://lore.kernel.org/git/gOXOaoqn-E9A2ob7ykWEcDc7ZxmSwAjcP5CCFKfr5ejCOWZQ1lfAUZcbgYT9AyQCcDgJvCrnrtziXiels-Hxol3xlkGTVHk24SvAdaSUtKQ=@rtzoeller.com/

Thanks!

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

Range-diff vs v1:

1: e77c3e33ba85 ! 1: 2468eaff322b git-difftool-helper.sh: learn a new way skip to save point
@@ Metadata
Author: ZheNing Hu <[email protected]>

## Commit message ##
- git-difftool-helper.sh: learn a new way skip to save point
+ git-difftool-helper.sh: learn a new way go back to last save point

- 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,
+ git difftool only allow us to select file to view in turn.
+ If there is a commit with many files and we exit in the 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.
+ 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-last-position, this file will be deleted after
+ successful traversing. But if an unexpected exit occurred midway or
+ users similar to using "ctrl+c" kill the process,and the user wants
+ to redo the same git difftoool, git will view the coordinates in
+ the save point, ask user if they want continue from the last position.
+ This will improve the user experience.

Signed-off-by ZheNing Hu <[email protected]>

@@ git-difftool--helper.sh
# Copyright (c) 2009, 2010 David Aguilar

TOOL_MODE=diff
-+GIT_DIFFTOOL_SKIP_TO_FILE="$GIT_DIR/difftool-skip-to" ++GIT_DIFFTOOL_LAST_POSITION="$GIT_DIR/difftool-last-position" ++DIFFTOOL_FIRST_NUM="1"
. git-mergetool--lib

# difftool.prompt controls the default prompt/no-prompt behavior
@@ git-difftool--helper.sh: 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"
++ if test -f "$GIT_DIFFTOOL_LAST_POSITION"
+ 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
++ 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
-+ # choice skip or not skip when check first file.
-+ if test $GIT_DIFF_PATH_COUNTER -eq "1"
++ if test "$GIT_DIFF_PATH_COUNTER" -eq "$DIFFTOOL_FIRST_NUM"
+ then
-+ printf "do you want to skip to last time difftool save point($SAVE_POINT_NUM) [Y/n]?"

++ printf "Do you want to start from the possible last file you were viewing? [Y/n]?"
+ read skip_ans || return
+ if test "$skip_ans" = y
+ then
@@ git-difftool--helper.sh: launch_merge_tool () {
+ fi
+ fi
+ fi

What if instead of global state, maybe the user could specify a path
that difftool could skip forward to? For example, we could teach
difftool to resume by telling it where we last left off:

   git difftool --resume-from=foo/bar099.txt

Then we don't need the global counter state file?

Finally, I'm going to plug what I believe to be the right tool for the job here. Have you tried git cola?[1] Difftool is tightly integrated, and the UI is such that you can trivially choose any of the modified/staged files and difftool them by using the Ctrl-d hotkey.

https://github.com/git-cola/git-cola/

Cola is purpose-built for driving difftool, and for interactive staging, so not mentioning it in the context of wanting a better UI for difftool would be a disservice to difftool users.

(cc'd Ryan since the thread involving him was mentioned in the commit message)

Quoted 95 lines · ZheNing Hu via GitGitGadget

From: ZheNing Hu <[email protected]>

git difftool only allow us to select file to view in turn. If there is a commit with many files and we exit in the 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-last-position, this file will be deleted after successful traversing. But if an unexpected exit occurred midway or users similar to using "ctrl+c" kill the process,and the user wants to redo the same git difftoool, git will view the coordinates in the save point, ask user if they want continue from the last position. This will improve the user experience.

Signed-off-by ZheNing Hu <[email protected]>

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

git user may should travel the diff list to choice file diff to view, if they exit in midway,they must travel it again. By saving current coordinates in GIT_DIR/difftool-last-position method, provides a possibility for this user-friendly solution.

this patch's origin discuss is here:
https://lore.kernel.org/git/gOXOaoqn-E9A2ob7ykWEcDc7ZxmSwAjcP5CCFKfr5ejCOWZQ1lfAUZcbgYT9AyQCcDgJvCrnrtziXiels-Hxol3xlkGTVHk24SvAdaSUtKQ=@rtzoeller.com/

Thanks!

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

Range-diff vs v1:

1: e77c3e33ba85 ! 1: 2468eaff322b git-difftool-helper.sh: learn a new way skip to save point
@@ Metadata
Author: ZheNing Hu <[email protected]>

## Commit message ##
- git-difftool-helper.sh: learn a new way skip to save point
+ git-difftool-helper.sh: learn a new way go back to last save point

- 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,
+ git difftool only allow us to select file to view in turn.
+ If there is a commit with many files and we exit in the 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.
+ 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-last-position, this file will be deleted after
+ successful traversing. But if an unexpected exit occurred midway or
+ users similar to using "ctrl+c" kill the process,and the user wants
+ to redo the same git difftoool, git will view the coordinates in
+ the save point, ask user if they want continue from the last position.
+ This will improve the user experience.

Signed-off-by ZheNing Hu <[email protected]>

@@ git-difftool--helper.sh
# Copyright (c) 2009, 2010 David Aguilar

TOOL_MODE=diff
-+GIT_DIFFTOOL_SKIP_TO_FILE="$GIT_DIR/difftool-skip-to" ++GIT_DIFFTOOL_LAST_POSITION="$GIT_DIR/difftool-last-position" ++DIFFTOOL_FIRST_NUM="1"
. git-mergetool--lib

# difftool.prompt controls the default prompt/no-prompt behavior
@@ git-difftool--helper.sh: 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"
++ if test -f "$GIT_DIFFTOOL_LAST_POSITION"
+ 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
++ 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
-+ # choice skip or not skip when check first file.
-+ if test $GIT_DIFF_PATH_COUNTER -eq "1"
++ if test "$GIT_DIFF_PATH_COUNTER" -eq "$DIFFTOOL_FIRST_NUM"
+ then
-+ printf "do you want to skip to last time difftool save point($SAVE_POINT_NUM) [Y/n]?"

Quoted 8 lines

++ printf "Do you want to start from the possible last file you were viewing? [Y/n]?"
+ read skip_ans || return
+ if test "$skip_ans" = y
+ then
@@ git-difftool--helper.sh: launch_merge_tool () {
+ fi
+ fi
+ fi

Similar to Junio's question about, "where does this possible come from?", I wasn't able to make out the behavior in the following situation.

What about when the user switches branches or specifies a pathspec on the command-line or some other avenue that ends up with the number of files to diff being very different than the last difftool invocation?

Will difftool, for example, skip over a smaller set of files on invocation 2 if invocation 1 involved many files and we exited out with a counter number that is very high?

One thing that's not too good about having state files in .git/ is that they're global data and we also have to think about, "what if the user has multiple difftools running?" and those kind of complexities.

I don't want this to seem like I'm trying to be dismissive of this feature which does seem like a useful thing in general, so I'll try to come up with an alternative interface that is slightly more general but a admittedly a little bit more cumbersome because it's not as automatic.

What if instead of global state, maybe the user could specify a path
that difftool could skip forward to? For example, we could teach
difftool to resume by telling it where we last left off:

   git difftool --resume-from=foo/bar099.txt

Then we don't need the global counter state file?

Finally, I'm going to plug what I believe to be the right tool for the job here. Have you tried git cola?[1] Difftool is tightly integrated, and the UI is such that you can trivially choose any of the modified/staged files and difftool them by using the Ctrl-d hotkey.

https://github.com/git-cola/git-cola/

Cola is purpose-built for driving difftool, and for interactive staging, so not mentioning it in the context of wanting a better UI for difftool would be a disservice to difftool users.

--
David

Quoted 4 lines · David Aguilar

What if instead of global state, maybe the user could specify a path
that difftool could skip forward to? For example, we could teach
difftool to resume by telling it where we last left off:

Quoted 3 lines

   git difftool --resume-from=foo/bar099.txt

Then we don't need the global counter state file?

Does it have to be the second and subsequent invocation to pass the new "resume-from" option? As we do not have "global" state, I would presume that we do not even know if it is the first invocation, so perhaps a better name would be "--start-from=$pathname"?

Quoted 6 lines

Finally, I'm going to plug what I believe to be the right tool for the job here. Have you tried git cola?[1] Difftool is tightly integrated, and the UI is such that you can trivially choose any of the modified/staged files and difftool them by using the Ctrl-d hotkey.

Quoted 5 lines

https://github.com/git-cola/git-cola/

Cola is purpose-built for driving difftool, and for interactive staging, so not mentioning it in the context of wanting a better UI for difftool would be a disservice to difftool users.

;-)

builtin/difftool.c +1 -1

index 6e18e623fddf..67d2befa1210 100644
@@ -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`")),

diff.c +1 -0

index 69e3bc00ed8f..cdad26f99063 100644
@@ -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;

diff.c +1 -0

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

index 2ff2b1c7f2ca..f67c43f5af95 100644
@@ -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)

Documentation/git-difftool.txt OPTIONS · line 39

Quoted 11 lines · ZheNing Hu via GitGitGadget

Documentation/git-difftool.txt +3 -0

index 484c485fd06c..552be097dfea 100644
@@ -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.

Documentation/git-difftool.txt OPTIONS · line 42

Quoted 3 lines

Documentation/git-difftool.txt +0 -0

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

Quoted 14 lines

builtin/difftool.c +1 -1

index 6e18e623fddf..67d2befa1210 100644
@@ -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`")),

builtin/difftool.c int cmd_difftool(int argc, const char **argv, const char *prefix) · line 720

Quoted 7 lines

builtin/difftool.c +2 -0

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

Quoted 4 lines

builtin/difftool.c +0 -0

@@ -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);
 

builtin/difftool.c int cmd_difftool(int argc, const char **argv, const char *prefix) · line 730

Quoted 2 lines

builtin/difftool.c +2 -0

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

Quoted 15 lines

diff.c +1 -0

index 69e3bc00ed8f..cdad26f99063 100644
@@ -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.c static void run_external_diff(const char *pgm, · line 4282

Quoted 9 lines

diff.c +7 -0

      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.

Quoted 31 lines

diff.c +1 -0

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

index 2ff2b1c7f2ca..f67c43f5af95 100644
@@ -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)
 

diff.h typedef void (*diff_format_fn_t)(struct diff_queue_struct *q, · line 100

Quoted 7 lines

diff.h +3 -3

 /* 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?

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?

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.

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

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?

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

$ ./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(+)

diffcore-rotate.c new file +35 -0

new file mode 100644
index 0000000000..0d17901616
@@ -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;
+}

diffcore.h +1 -0

index d2a63c5c71..bd5959375b 100644
@@ -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 {

Makefile +1 -0

index b797033c58..031b0b88e6 100644
@@ -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.c +4 -0

index 69e3bc00ed..90a8d5abd0 100644
@@ -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.h +0 -0

index 2ff2b1c7f2..0801469e63 100644

It has no effect on this new feature. I should put this modification in an additional commit, right?

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? :)

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.

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?

So "difftool --start-at=<file>" can of use "diff --rotate-to=<file>"

Quoted 2 lines · 胡哲宁

It has no effect on this new feature. I should put this modification in an additional commit, right?

Or you can just drop it. It certainly is a distracting change to be part of this topic.

Quoted 2 lines

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? :)

Imagine a history where file1 and file2 are in the initial commit. The second commit adds file3 and modifies file2, and then the third commit modifies file1. What would happen when you did this?

  $ git log -p --rotate-to=file2

For the commit at HEAD, the set of paths shown would be file1 and nothing else (because it is the only path that gets modified). You cannot start showing from "file2". Dying (and not showing HEAD~1 and HEAD~2) is the last thing you want to do in this situation. We do not even want to give a warning or an error, as it is totally expected that some commits do not touch a given path---it would be very unusual if a path is touched by every commit ;-).

For "difftool --start-at=file2", the equation is different. It does not traverse history where each commit may or may not modify file2, and when the user says s/he wants to resume from file2, file2 must be in the set of paths that have changes, or something is wrong (i.e. the user meant file3 but mistyped it as file2).

Quoted 3 lines

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.

So "difftool --start-at=<file>" can of use "diff --rotate-to=<file>" to implement the feature (after all, that is why I wrote it), but the error condition between the two are quite different. And ...

Quoted 10 lines

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;
}

... that is why an unconditional change like this in diff.c is not acceptable, as it would break other codepaths like "git log -p". If we were to add an error there, it has to be very limited to exclude at least "log -p"---there may be other features that share the code that should not trigger an error for similar reasons.

If diffcore-rotate chooses "missing rotate-to file makes it a no-op" as its semantics, and if "difftool --start-at" does not want to see a misspelt filename making it a no-op, then the latter needs to ensure that the name it got from the user is indeed in the set of paths that have changes before running "diff --rotate-to" to implement its "difftool --start-at" feature.

The "missing rotate-to file in the diff_queue MUST NOT cause diffcore-rotate to error out" rule is probably unnegitiable, but there are other ways to make it easier to use, though.

For example, we could change the rotate-to logic to mean "start at the given path, or if such a path does not exist in the diff_queue, then the first path that sorts after the given path is where we start". That way, if the diff_queue has paths A B C D E and rotate-to gives C, then we rotate the output to C D E A B. And if the diff_queue has A B D E and rotate-to gives C, then the output would become D E A B (instead of becoming a no-op). Then, a mistyped filename may not do what the user wanted to do (after all, that is the definition of MIStyping), but it would do something noticeable by the user, which may be useful enough and at least would let the user notice the mistake.

if (!strcmp(rotate_to_filename, q->queue[i]->two->path))

if (strcmp(rotate_to_filename, q->queue[i]->two->path) <= 0)

OPT_STRING(0, "start-with", &options->rotate-to, N_("<path>"),
N_("pass from difftool to diff, has the same effort as rotate-to")),

Quoted 2 lines

In addition, Do I need to do the documentation and tests related to your diff --rotate-to?

Once we know how we want "diff --rotate-to" to behave exactly, I can help that part further, if you want. And then you can build on top.

But we need to design exactly what the desired semantics would be before any of that.

Thanks.

From: ZheNing Hu <[email protected]>

git difftool only allow us to select file to view in turn. If there is a commit with many files and we exit in the search, We will have to traverse list again to get the file diff which we want to see. Therefore, here is a new method: user can use git difftool --start-from=<filename> to start viewing from the specified file, This will improve the user experience.

Junio C Hamano (1):
diff: --{rotate,skip}-to=<path>

ZheNing Hu (1):
difftool.c: learn a new way start at specified file

Quoted 2 lines · ZheNing Hu via GitGitGadget

Junio C Hamano (1):
diff: --{rotate,skip}-to=<path>

That's not part of your series (didn't I ask you not to include it)?

Quoted 2 lines

ZheNing Hu (1):
difftool.c: learn a new way start at specified file

Will see what I find, but I may not be able to get to it today.

Thanks.

From: ZheNing Hu <[email protected]>

git difftool only allow us to select file to view in turn. If there is a commit with many files and we exit in the search,

We will have to traverse list again to get the file diff which

we want to see. Therefore, here is a new method: user can use git difftool --rotate-to=<filename> or git difftool --skip-to=<filename> to start viewing from the specified file, This will improve the user experience.

Documentation/git-difftool.txt +0 -0

index 484c485fd06c..c64dff69c976 100644
@@ -34,6 +34,16 @@ OPTIONS
 	This is the default behaviour; the option is provided to
 	override any configuration settings.
 

Oh, I am sorry.
Then I only need to squash the two commit, right?

Quoted 14 lines · Junio C Hamano <[email protected]> 于2021年2月17日周三 上午2:45写道：

Quoted 2 lines · ZheNing Hu via GitGitGadget

Junio C Hamano (1):
diff: --{rotate,skip}-to=<path>

That's not part of your series (didn't I ask you not to include it)?

Quoted 2 lines

ZheNing Hu (1):
difftool.c: learn a new way start at specified file

Will see what I find, but I may not be able to get to it today.

Thanks.

Hi ZheNing,

Quoted 2 lines · ZheNing Hu

Oh, I am sorry.
Then I only need to squash the two commit, right?

I've never used GGG before but I suspect that in your GitHub PR, you need to set the PR base to 'master' instead of 'jc/diffcore-rotate'.

CCing the creator of GGG, please correct me if I'm wrong.

-Denton

Quoted 15 lines

Quoted 14 lines · Junio C Hamano <[email protected]> 于2021年2月17日周三 上午2:45写道：

Quoted 2 lines · ZheNing Hu via GitGitGadget

Junio C Hamano (1):
diff: --{rotate,skip}-to=<path>

That's not part of your series (didn't I ask you not to include it)?

Quoted 2 lines

ZheNing Hu (1):
difftool.c: learn a new way start at specified file

Will see what I find, but I may not be able to get to it today.

Thanks.

Quoted 4 lines · ZheNing Hu via GitGitGadget

From: ZheNing Hu <[email protected]>

git difftool only allow us to select file to view in turn. If there is a commit with many files and we exit in the search,

I am not sure what "in the search" refers to. "in the middle" I would understand, though.

Quoted 1 line

We will have to traverse list again to get the file diff which

Let's downcase this "We".

Quoted 4 lines

we want to see. Therefore, here is a new method: user can use git difftool --rotate-to=<filename> or git difftool --skip-to=<filename> to start viewing from the specified file, This will improve the user experience.

Do we need both? I'd rather not to give end-user-facing commands too many knobs that would do similar things. Too many choices to choose from without clear answer to "which one should I prefer to use?" is a bad combination for end-users.

Quoted 7 lines

Documentation/diff-options.txt +0 -0

index 7c5b3cf42bcc..aa2b5c11f20b 100644
@@ -701,7 +701,7 @@ components matches the pattern.  For example, the pattern "`foo*bar`"
 matches "`fooasdfbar`" and "`foo/bar/baz/asdf`" but not "`foobarx`".
 

Documentation/diff-options.txt components matches the pattern. For example, the pattern "`foo*bar`" · line 707

Quoted 6 lines

Documentation/diff-options.txt +1 -1

 --skip-to=<file>::
---rotate-to=<file::
+--rotate-to=<file>::
      Discard the files before the named <file> from the output
      (i.e. 'skip to'), or move them to the end of the output
      (i.e. 'rotate to').  These were invented primarily for use

Thanks for correcting, but this change should not be a part of this patch. Instead, you help the other's topic by giving a review (and you could just have said "there there is closing '>' missing").

Quoted 8 lines

Documentation/git-difftool.txt +0 -0

index 484c485fd06c..c64dff69c976 100644
@@ -34,6 +34,16 @@ OPTIONS
      This is the default behaviour; the option is provided to
      override any configuration settings.
 

Documentation/git-difftool.txt OPTIONS · line 46

Quoted 10 lines

Documentation/git-difftool.txt +10 -0

+--rotate-to=<file>::
+     Internally call `git diff --rotate-to=<file>`,
+     show the change in the specified path first.
+     Files before the specified path will be moved to the last output.
+
+--skip-to=<file>::
+     Internally call `git diff --skip-to=<file>`,
+     skip the output to the specified path.
+     Files before the specified path will not output.
+

This, unlike the "diffcore" stuff, is end-user facing, and it is better not to force the readers even know what --skip-to option to the diff does (after all, difftool users are using 'git difftool' and they are not necessarily 'git diff' users).

--skip-to=<file>::
Start showing the diff for the given path, skipping all the paths before it.

or something, perhaps.

Documentation/git-difftool.txt OPTIONS

Quoted 11 lines

Documentation/git-difftool.txt +11 -0

+test_expect_success 'difftool --skip-to' '
+     difftool_test_setup &&
+     test_when_finished git reset --hard &&
+     git difftool --no-prompt --extcmd=cat --skip-to="2" HEAD^ >output &&
+     cat >expect <<-\EOF &&
+     2
+     4
+     EOF
+     test_cmp output expect &&
+     test_must_fail git difftool --no-prompt --extcmd=cat --skip-to="3" HEAD^
+'

This probably should be split into two independent tests. One to check that the non-failing case works as expected, the other to check that a bogus command line option errors out as expected.

Thanks.

int cmd_diff_tree(...)
...
if (read_stdin) {
...
opt->diffopt.rotate_to_strict = 0;
...

Quoted 10 lines · Denton Liu <[email protected]> 于2021年2月17日周三 下午7:14写道：

Hi ZheNing,

Quoted 2 lines · ZheNing Hu

Oh, I am sorry.
Then I only need to squash the two commit, right?

I've never used GGG before but I suspect that in your GitHub PR, you need to set the PR base to 'master' instead of 'jc/diffcore-rotate'.

Quoted 3 lines

CCing the creator of GGG, please correct me if I'm wrong.

-Denton

Hi Denton Liu,
You mean I should cherry-pick Junio's patch to my topic branch, right?

Quoted 11 lines · ZheNing Hu

Quoted 10 lines · Denton Liu <[email protected]> 于2021年2月17日周三 下午7:14写道：

Hi ZheNing,

Quoted 2 lines · ZheNing Hu

Oh, I am sorry.
Then I only need to squash the two commit, right?

I've never used GGG before but I suspect that in your GitHub PR, you need to set the PR base to 'master' instead of 'jc/diffcore-rotate'.

Quoted 6 lines

Quoted 3 lines

CCing the creator of GGG, please correct me if I'm wrong.

-Denton

Hi Denton Liu,
You mean I should cherry-pick Junio's patch to my topic branch, right?

Thanks, Denton, for helping.

ZheNing, the end result we want to see on the list is just a single patch, your 2/2 alone, that says "this patch depends on the diffcore-rotate topic" _under_ its "---" three-dash lines (where "meta" comments on the patch to explain how it fits the rest of the world, etc.). As a single patch "topic", there won't be even 1/1 marking, i.e. something like:

Subject: [PATCH v6] difftool.c: learn a new way start at specified file
From: ZheNing Hu <[email protected]>

git difftool only allow us to ...
...
Teach the command an option '--skip-to=<path>' to allow the user to say that diffs for earlier paths are not interesting (because they were already seen in an earlier session) and start this session with the named path.

Signed-off-by ZheNing Hu <[email protected]>

---

* An earlier round tried to implement the skipping all in the GIT_EXTERNAL_DIFF, but this round takes advantage of the new "diff --skip-to=<path>" feature implemented by gitster (therefore, the patch depends on that topic).

Documentation/git-difftool.txt | 10 ++++++++++
t/t7800-difftool.sh | 30 ++++++++++++++++++++++++++++++
2 files changed, 40 insertions(+)

... patch here ...

I do not know how to achieve that end result with GGG and I do not know if GGG allows its users to do so easily, though.

Thanks.

Quoted 11 lines · ZheNing Hu

Quoted 10 lines · Denton Liu <[email protected]> 于2021年2月17日周三 下午7:14写道：

Hi ZheNing,

Quoted 2 lines · ZheNing Hu

Oh, I am sorry.
Then I only need to squash the two commit, right?

I've never used GGG before but I suspect that in your GitHub PR, you need to set the PR base to 'master' instead of 'jc/diffcore-rotate'.

Quoted 6 lines

Quoted 3 lines

CCing the creator of GGG, please correct me if I'm wrong.

-Denton

Hi Denton Liu,
You mean I should cherry-pick Junio's patch to my topic branch, right?

Thanks, Denton, for helping.

ZheNing, the end result we want to see on the list is just a single patch, your 2/2 alone, that says "this patch depends on the diffcore-rotate topic" _under_ its "---" three-dash lines (where "meta" comments on the patch to explain how it fits the rest of the world, etc.). As a single patch "topic", there won't be even 1/1 marking, i.e. something like:

Subject: [PATCH v6] difftool.c: learn a new way start at specified file
From: ZheNing Hu <[email protected]>

git difftool only allow us to ...
...
Teach the command an option '--skip-to=<path>' to allow the user to say that diffs for earlier paths are not interesting (because they were already seen in an earlier session) and start this session with the named path.

Signed-off-by ZheNing Hu <[email protected]>

---

* An earlier round tried to implement the skipping all in the GIT_EXTERNAL_DIFF, but this round takes advantage of the new "diff --skip-to=<path>" feature implemented by gitster (therefore, the patch depends on that topic).

Documentation/git-difftool.txt | 10 ++++++++++
t/t7800-difftool.sh | 30 ++++++++++++++++++++++++++++++
2 files changed, 40 insertions(+)

... patch here ...

I do not know how to achieve that end result with GGG and I do not know if GGG allows its users to do so easily, though.

Thanks.

I think my patch is stuck in GGG, and the current version is after I cherry-pick your patch and my patch on the master. Because I don’t know how to based on your patch but not submit your patch. Is there any good way?

Quoted 4 lines · ZheNing Hu

I think my patch is stuck in GGG, and the current version is after I cherry-pick your patch and my patch on the master. Because I don’t know how to based on your patch but not submit your patch. Is there any good way?

I am not capable of doing helpdesk for GGG but I think you can send two of them anyway with the title of the first one munged for reading humans to tell them not to use or even look at it ;-)

GGG may send both of them out, but the ultimate objective here in the exercise is to avoid the unwanted first one to waste people's time, so until such a feature is implemented in GGG (or there may already be one, but we do not know it), that would serve as a workaround.

Thanks.

From: ZheNing Hu <[email protected]>

git difftool only allow us to select file to view in turn. If there is a commit with many files and we exit in the middle, we will have to traverse list again to get the file diff which we want to see. Therefore,teach the command an option --skip-to=<path> to allow the user to say that diffs for earlier paths are not interesting (because they were already seen in an earlier session) and start this session with the named path.

Signed-off-by ZheNing Hu <[email protected]>

---
difftool.c: learn a new way start at specified file

* The patch of the previous version implemented the jump through environment variables. The current version is based on the "diff --skip-to=" feature implemented by gitster, which implements a possible solution for the jump of difftool.

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

Range-diff vs v5:

 1:  fb4bfd0f8b16 < -:  ------------ diff: --{rotate,skip}-to=<path>
 2:  98e2707ee2fa ! 1:  4377a917ca9e difftool.c: learn a new way start at specified file

@@ Commit message
difftool.c: learn a new way start at specified file

git difftool only allow us to select file to view in turn.
- If there is a commit with many files and we exit in the search,
- We will have to traverse list again to get the file diff which
- we want to see. Therefore, here is a new method: user can use
- git difftool --rotate-to=<filename> or git difftool --skip-to=<filename>
- to start viewing from the specified file, This will improve the
- user experience.
-
- git difftool --rotate-to=<file> or git difftool --skip-to=<filename>
- will pass the path to diffcore-rotate, and diff-core will
- adjust the order of files, make the specified file sorted to
- the first.git difftool --rotate-to=<file> will move files before
- the specified path to the last output, and
- git difftool --skip-to=<filename> will ignore these files output.
- It is an error when there is no patch for specified file is shown.
+ If there is a commit with many files and we exit in the middle,
+ we will have to traverse list again to get the file diff which
+ we want to see. Therefore,teach the command an option
+ --skip-to=<path> to allow the user to say that diffs for earlier
+ paths are not interesting (because they were already seen in an
+ earlier session) and start this session with the named path.

Signed-off-by ZheNing Hu <[email protected]>

- ## Documentation/diff-options.txt ##
-@@ Documentation/diff-options.txt: components matches the pattern. For example, the pattern "foo*bar"
- matches "fooasdfbar" and "foo/bar/baz/asdf" but not "foobarx".
-
- --skip-to=<file>::
----rotate-to=<file::
-+--rotate-to=<file>::
- Discard the files before the named <file> from the output
- (i.e. 'skip to'), or move them to the end of the output
- (i.e. 'rotate to'). These were invented primarily for use
-
## Documentation/git-difftool.txt ##
@@ Documentation/git-difftool.txt: OPTIONS
This is the default behaviour; the option is provided to
override any configuration settings.

+--rotate-to=<file>::
-+ Internally call git diff --rotate-to=<file>,
-+ show the change in the specified path first.
-+ Files before the specified path will be moved to the last output. ++ Start showing the diff for the given path,
++ the paths before it will move to end and output.
+
+--skip-to=<file>::
-+ Internally call git diff --skip-to=<file>,
-+ skip the output to the specified path.
-+ Files before the specified path will not output.
++ Start showing the diff for the given path, skipping all
++ the paths before it.
+
-t <tool>::
--tool=<tool>::
@@ t/t7800-difftool.sh: test_expect_success 'difftool --gui, --tool and --extcmd ar
+ 4
+ 1
+ EOF
-+ test_cmp output expect &&
-+ test_must_fail git difftool --no-prompt --extcmd=cat --rotate-to="3" HEAD^ ++ test_cmp output expect
+'
+
+test_expect_success 'difftool --skip-to' '
@@ t/t7800-difftool.sh: test_expect_success 'difftool --gui, --tool and --extcmd ar
+ 2
+ 4
+ EOF
-+ test_cmp output expect &&
-+ test_must_fail git difftool --no-prompt --extcmd=cat --skip-to="3" HEAD^ ++ test_cmp output expect
+'
+
++test_expect_success 'difftool --rotate/skip-to error condition' ' ++ test_must_fail git difftool --no-prompt --extcmd=cat --rotate-to="3" HEAD^ && ++ test_must_fail git difftool --no-prompt --extcmd=cat --skip-to="3" HEAD^ ++'
test_done

 Documentation/git-difftool.txt |  8 ++++++++
 t/t7800-difftool.sh            | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+)

Documentation/git-difftool.txt +8 -0

index 484c485fd06c..143b0c49d739 100644
@@ -34,6 +34,14 @@ OPTIONS
        This is the default behaviour; the option is provided to
        override any configuration settings.
 
+--rotate-to=<file>::
+       Start showing the diff for the given path,
+       the paths before it will move to end and output.
+
+--skip-to=<file>::
+       Start showing the diff for the given path, skipping all
+       the paths before it.
+
 -t <tool>::
 --tool=<tool>::
        Use the diff tool specified by <tool>.  Valid values include

t/t7800-difftool.sh +32 -0

index 9192c141ffc6..3e041e83aede 100755
@@ -762,4 +762,36 @@ test_expect_success 'difftool --gui, --tool and --extcmd are mutually exclusive'
        test_must_fail git difftool --gui --tool=test-tool --extcmd=cat
 '
 
+test_expect_success 'difftool --rotate-to' '
+       difftool_test_setup &&
+       test_when_finished git reset --hard &&
+       echo 1 >1 &&
+       echo 2 >2 &&
+       echo 4 >4 &&
+       git add 1 2 4 &&
+       git commit -a -m "124" &&
+       git difftool --no-prompt --extcmd=cat --rotate-to="2" HEAD^ >output&&
+       cat >expect <<-\EOF &&
+       2
+       4
+       1
+       EOF
+       test_cmp output expect
+'
+
+test_expect_success 'difftool --skip-to' '
+       difftool_test_setup &&
+       test_when_finished git reset --hard &&
+       git difftool --no-prompt --extcmd=cat --skip-to="2" HEAD^ >output &&
+       cat >expect <<-\EOF &&
+       2
+       4
+       EOF
+       test_cmp output expect
+'
+
+test_expect_success 'difftool --rotate/skip-to error condition' '
+       test_must_fail git difftool --no-prompt --extcmd=cat --rotate-to="3" HEAD^ &&
+       test_must_fail git difftool --no-prompt --extcmd=cat --skip-to="3" HEAD^
+'
 test_done

base-commit: 1eb4136ac2a24764257567b930535fcece01719f --
gitgitgadget

From: ZheNing Hu <[email protected]>

git difftool only allow us to select file to view in turn. If there is a commit with many files and we exit in the middle, we will have to traverse list again to get the file diff which we want to see. Therefore,teach the command an option --skip-to=<path> to allow the user to say that diffs for earlier paths are not interesting (because they were already seen in an earlier session) and start this session with the named path.

Signed-off-by ZheNing Hu <[email protected]>

---
difftool.c: learn a new way start at specified file

* The patch of the previous version implemented the jump through environment variables. The current version is based on the "diff --skip-to=" feature implemented by gitster, which implements a possible solution for the jump of difftool.

Quoted 8 lines · ZheNing Hu

Quoted 7 lines · Denton Liu <[email protected]> 于2021年2月17日周三 下午7:14写道：

Quoted 2 lines · ZheNing Hu

Oh, I am sorry.
Then I only need to squash the two commit, right?

I've never used GGG before but I suspect that in your GitHub PR, you need to set the PR base to 'master' instead of 'jc/diffcore-rotate'.

Quoted 1 line

You mean I should cherry-pick Junio's patch to my topic branch, right?

ZheNing, the end result we want to see on the list is just a single patch, your 2/2 alone, that says "this patch depends on the diffcore-rotate topic" _under_ its "---" three-dash lines (where "meta" comments on the patch to explain how it fits the rest of the world, etc.). As a single patch "topic", there won't be even 1/1 marking, i.e. something like:

Subject: [PATCH v6] difftool.c: learn a new way start at specified file
From: ZheNing Hu <[email protected]>

git difftool only allow us to ...
...
Teach the command an option '--skip-to=<path>' to allow the user to say that diffs for earlier paths are not interesting (because they were already seen in an earlier session) and start this session with the named path.

Signed-off-by ZheNing Hu <[email protected]>

---

* An earlier round tried to implement the skipping all in the GIT_EXTERNAL_DIFF, but this round takes advantage of the new "diff --skip-to=<path>" feature implemented by gitster (therefore, the patch depends on that topic).

Documentation/git-difftool.txt | 10 ++++++++++
t/t7800-difftool.sh | 30 ++++++++++++++++++++++++++++++
2 files changed, 40 insertions(+)

... patch here ...

I do not know how to achieve that end result with GGG and I do not know if GGG allows its users to do so easily, though.

Hi Junio, ZheNing & Denton,

Quoted 4 lines · Junio C Hamano

Quoted 2 lines · ZheNing Hu

Quoted 1 line · Denton Liu <[email protected]> 于2021年2月17日周三 下午7:14写道：

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines · ZheNing Hu

Quoted 2 lines

Oh, I am sorry.
Then I only need to squash the two commit, right?

I've never used GGG before but I suspect that in your GitHub PR, you need to set the PR base to 'master' instead of 'jc/diffcore-rotate'.

Yes, that is my understanding of what needed to be done.

Quoted 1 line

Quoted 1 line

You mean I should cherry-pick Junio's patch to my topic branch, right?

That, too.

Quoted 9 lines

ZheNing, the end result we want to see on the list is just a single patch, your 2/2 alone, that says "this patch depends on the diffcore-rotate topic" _under_ its "---" three-dash lines (where "meta" comments on the patch to explain how it fits the rest of the world, etc.). As a single patch "topic", there won't be even 1/1 marking, i.e. something like:
...
I do not know how to achieve that end result with GGG and I do not know if GGG allows its users to do so easily, though.

For single-patch contributions, the PR description is not turned into a separate cover letter (per your request, Junio), but it is put between the commit message and the diff as you illustrated.

So yes, the comment can go into the PR description (AKA the first comment on the PR) and the next /submit will include it in the single mail.