| Message ID | pull.870.v2.git.1612803744188.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] git-difftool-helper.sh: learn a new way go back to last save point | expand |
"ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes: > 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. > . 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.
(cc'd Ryan since the thread involving him was mentioned in the commit message) On Mon, Feb 8, 2021 at 9:02 AM ZheNing Hu via GitGitGadget <gitgitgadget@gmail.com> wrote: > > 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 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 <adlternative@gmail.com> > --- > 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 <adlternative@gmail.com> > > ## 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 <adlternative@gmail.com> > > @@ 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 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 Aguilar <davvid@gmail.com> writes: > 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? 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"? > 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 Aguilar <davvid@gmail.com> 于2021年2月9日周二 上午6:16写道： > > (cc'd Ryan since the thread involving him was mentioned in the commit message) > > On Mon, Feb 8, 2021 at 9:02 AM ZheNing Hu via GitGitGadget > <gitgitgadget@gmail.com> wrote: > > > > 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 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 <adlternative@gmail.com> > > --- > > 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 <adlternative@gmail.com> > > > > ## 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 <adlternative@gmail.com> > > > > @@ 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 > > > 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? > This is what I worry about. > 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 admit that I did not consider the situation where multiple `git difftool` processes are going on at the same time. > 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? > Wonderful idea.But as Junio said, there may be no global state support, `start-from` will be more applicable. > > 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. I saw the difftool UI of git-cola, and it is great to view the differences by selecting files.I have been using vscode's git plugin before, and it works well tool. > -- > David Thanks for help! -- ZheNing Hu
Junio C Hamano <gitster@pobox.com> 于2021年2月9日周二 上午7:34写道： > > David Aguilar <davvid@gmail.com> writes: > > > 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? > > 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"? > Thank you for your thinking, I agree with your point of view. As you said before: an accurate file name may be more suitable for users than `possible last file`. However, without the support of the global `difftool-save-point`, it may not be possible to know the last exit point of the user. Even if the global state is allowed, it may need to do more work to avoid the competition for the global state under multiple processes. So "--start-from=$pathname" is more suitable to provide users with a way to quickly index to specified files. Then I shift the front and start thinking about how to realize it! :) > > 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. > > ;-) Thanks again!
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" . 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 + 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]?" + read skip_ans || return + if test "$skip_ans" = y + then + return + fi + else + return + fi + fi + fi + if test -z "$SAVE_POINT_NUM" || + test "$SAVE_POINT_NUM" -ne "$GIT_DIFF_PATH_COUNTER" + then + echo "$GIT_DIFF_PATH_COUNTER" >"$GIT_DIFFTOOL_LAST_POSITION" + fi printf "\nViewing (%s/%s): '%s'\n" "$GIT_DIFF_PATH_COUNTER" \ "$GIT_DIFF_PATH_TOTAL" "$MERGED" if use_ext_cmd @@ -102,4 +128,9 @@ else done fi +if test "$GIT_DIFF_PATH_COUNTER" -eq "$GIT_DIFF_PATH_TOTAL" +then + rm -f "$GIT_DIFFTOOL_LAST_POSITION" + +fi exit 0