| Message ID | 2fb58fd30ae730ccd3e88ec51b5fe6d80ab7a8c7.camel@guriev.su (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] mergetools: support difftool.tabbed setting | expand |
On Tue, Jan 12, 2021 at 10:07 PM Nicholas Guriev <nicholas@guriev.su> wrote: > > I was asked how to configure "git difftool" to open files using several > tabs and stop spawning diff application on every modified file. I looked > into Git source and found no possibility to run diff tool at one step. > > The patch allows a user to view diffs in single window at one go. The > current implementation is still poor and it can be used solely for > demonstration purposes. To see it in action, tweak the local gitconfig: > > git config difftool.prompt false > git config difftool.tabbed true > > Then run: > > git difftool -t vimdiff > > Or: > > git difftool -t meld > > The solution has some restrictions, diffing up to ten files works now (I > did not bother with dynamic memory allocation), and it does not handle spaces > in file names (I do not know how to pass them correctly to underlying tools > without "xargs -0"). > > I think the git-difftool--helper should be changed so that it could > process many files in single invocation and it would not use a temporary > file by itself. A similar behaviour can be done in git-mergetool, too. > > Do you have ideas how to better implement such a feature? Any comments > are welcome. > > P.S.: I'm attaching screenshots for a clear demo what I mean. > --- > diff.c | 4 ++-- > git-mergetool--lib.sh | 36 +++++++++++++++++++++++++++++++++++- > mergetools/meld | 4 ++++ > mergetools/vimdiff | 17 +++++++++++++++++ > 4 files changed, 58 insertions(+), 3 deletions(-) > > > I'm not really sure if "tabbed" is the best name for what's going on, though. It's really more of a "diff everything in one shot" mode, and it just so happens that the tools in question use tabs. General note -- similar to the convention followed by mergetool.hideResolved and other difftool things I think it would make sense for tools to be able to override this on a per-tool basis. That said, I wonder whether we need this new feature, or whether we should instead improve an existing one. I'm leaning towards improving the existing dir-diff feature as a better alternative. It's unfortunate that the "git difftool --dir-diff" feature doesn't seem to mesh well with vimdiff. It does work well with other tools that support diffing arbitrary directories, notably meld, xxdiff, etc. Regarding vimdiff + git difftool -d, there is this advice: https://stackoverflow.com/questions/8156493/git-vimdiff-and-dirdiff meld works just fine with "git difftool -d" (arguably nicer, since it gives you a directory view and a diff view in separate tabs), so if the only improvement is for vimdiff, then maybe the advice with the DirDiff plugin might be a better way to go. Having something like a "difftool.vimdiff.useDirDiff" configuration variable could be a way for us to adopt the advice that they offer there. We could have the dir-diff difftool mode set a variable that the vimdiff scriptlet could use to detect that we're in dir-diff mode. Then, when that variable is set, vimdiff could use, vim -f '+next' '+execute \"DirDiff\" argv(0) argv(1)' $LOCAL $REMOTE (as mentioned in the SO page) to invoke dir-diff mode in vim. That way the user only needs to set: git config difftool.vimdiff.useDirDiff ... and then "git diffftool -d" will integrate with the DirDiff plugin. What do you think about improving the vimdiff scriptlet to better integrate with "git difftool -d" instead?
David Aguilar <davvid@gmail.com> writes: > I'm not really sure if "tabbed" is the best name for what's going on, > though. It's really more of a "diff everything in one shot" mode, and > it just so happens that the tools in question use tabs. This statement matches my reaction to this new feature exactly. The way the external commands are triggered via GIT_EXTERNAL_DIFF mechanism makes it "easy" to show changes for one path at a time and "hard" to do so for all paths at once, but the resulting end-user experience that is forced to view one path at a time may be awkward. > That said, I wonder whether we need this new feature, or whether we > should instead improve an existing one. I'm leaning towards improving > the existing dir-diff feature as a better alternative. ... As a non-user, I have no strong opinion on the "new feature"; other than that I trust your judgement on the "difftool" design issues, that is. Thanks.
diff --git a/diff.c b/diff.c index 2253ec880..8a265e0b0 100644 --- a/diff.c +++ b/diff.c @@ -532,7 +532,7 @@ static struct diff_tempfile { * this tempfile object is used to manage its lifetime. */ struct tempfile *tempfile; -} diff_temp[2]; +} diff_temp[20]; struct emit_callback { int color_diff; @@ -4275,7 +4275,7 @@ static void run_external_diff(const char *pgm, if (run_command_v_opt_cd_env(argv.v, RUN_USING_SHELL, NULL, env.v)) die(_("external diff died, stopping at %s"), name); - remove_tempfile(); + //remove_tempfile(); strvec_clear(&argv); strvec_clear(&env); } diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh index 7225abd81..e599e4243 100644 --- a/git-mergetool--lib.sh +++ b/git-mergetool--lib.sh @@ -193,6 +193,8 @@ setup_tool () { false } + unset -f diff_combo_cmd + if test -f "$MERGE_TOOLS_DIR/$tool" then . "$MERGE_TOOLS_DIR/$tool" @@ -248,6 +250,25 @@ trust_exit_code () { fi } +is_difftool_tabbed () { + : "${GIT_DIFFTOOL_TABBED:=$(git config --type=bool --default=false difftool.tabbed)}" + case $(printf "%s" "$GIT_DIFFTOOL_TABBED" | tr '[:upper:]' '[:lower:]') in + yes|on|true|1) + GIT_DIFFTOOL_TABBED=true + ;; + no|off|false|0) + GIT_DIFFTOOL_TABBED=false + ;; + *) + echo "error: bad boolean value of GIT_DIFFTOOL_TABBED" >&2 + exit 1 + ;; + esac + + test "$GIT_DIFFTOOL_TABBED" = true && test "$GIT_DIFF_PATH_TOTAL" -gt 1 \ + && type diff_combo_cmd >/dev/null 2>&1 +} + # Entry point for running tools run_merge_tool () { @@ -272,7 +293,20 @@ run_merge_tool () { # Run a either a configured or built-in diff tool run_diff_cmd () { - diff_cmd "$1" + if is_difftool_tabbed + then + temp_file="${TMPDIR:-/tmp}/git-${PPID}_tabbed-queue" + test "$GIT_DIFF_PATH_COUNTER" -eq 1 && > "$temp_file" + printf "%s " "$LOCAL" "$REMOTE" >> "$temp_file" + + if [ "$GIT_DIFF_PATH_COUNTER" -eq "$GIT_DIFF_PATH_TOTAL" ] + then + diff_combo_cmd 3< "$temp_file" + rm -f -- "$temp_file" + fi + else + diff_cmd "$1" + fi } # Run a either a configured or built-in merge tool diff --git a/mergetools/meld b/mergetools/meld index aab4ebb93..6570bf0f8 100644 --- a/mergetools/meld +++ b/mergetools/meld @@ -2,6 +2,10 @@ diff_cmd () { "$merge_tool_path" "$LOCAL" "$REMOTE" } +diff_combo_cmd () { + ( IFS=' '; "$merge_tool_path" $(printf ' --diff %s %s' `cat <&3`) 3<&- ) +} + merge_cmd () { check_meld_for_features diff --git a/mergetools/vimdiff b/mergetools/vimdiff index abc8ce4ec..31d6e1eaa 100644 --- a/mergetools/vimdiff +++ b/mergetools/vimdiff @@ -3,6 +3,23 @@ diff_cmd () { -c 'wincmd l' -c 'cd $GIT_PREFIX' "$LOCAL" "$REMOTE" } +multitabbed_script=' + let i = 1 + while i < argc() + execute "tabedit" fnameescape(argv(i - 1)) + execute "diffsplit" fnameescape(argv(i)) + wincmd L + let i = i + 2 + endwhile + tabfirst + tabclose + unlet i +' +diff_combo_cmd () { + ( IFS=' '; cd "$GIT_PREFIX" && "$merge_tool_path" -R -f \ + -c "$multitabbed_script" `cat <&3` 3<&- ) +} + merge_cmd () { case "$1" in *vimdiff)
I was asked how to configure "git difftool" to open files using several tabs and stop spawning diff application on every modified file. I looked into Git source and found no possibility to run diff tool at one step. The patch allows a user to view diffs in single window at one go. The current implementation is still poor and it can be used solely for demonstration purposes. To see it in action, tweak the local gitconfig: git config difftool.prompt false git config difftool.tabbed true Then run: git difftool -t vimdiff Or: git difftool -t meld The solution has some restrictions, diffing up to ten files works now (I did not bother with dynamic memory allocation), and it does not handle spaces in file names (I do not know how to pass them correctly to underlying tools without "xargs -0"). I think the git-difftool--helper should be changed so that it could process many files in single invocation and it would not use a temporary file by itself. A similar behaviour can be done in git-mergetool, too. Do you have ideas how to better implement such a feature? Any comments are welcome. P.S.: I'm attaching screenshots for a clear demo what I mean. --- diff.c | 4 ++-- git-mergetool--lib.sh | 36 +++++++++++++++++++++++++++++++++++- mergetools/meld | 4 ++++ mergetools/vimdiff | 17 +++++++++++++++++ 4 files changed, 58 insertions(+), 3 deletions(-)