cherry-pick -Xrenormalize fails with formerly CRLF files
I'm working with a repo that used to be all CRLF. At some point it was changed to all LF, with `text=auto` in .gitattributes for the sake of Windows devs. I'm on Linux and have never touched any twiddles relating to line endings. I'm trying to cherry-pick some commits from before the switchover. Straightforward cherry-picking causes entire files at a time to conflict, which I've seen before when switching from tabs to spaces. So I tried -Xrenormalize and got: fatal: CRLF would be replaced by LF in [path] The error comes from check_safe_crlf, which warns if checksafe is CRLF_SAFE_WARN and dies if it's (presumably) CRLF_SAFE_FAIL. The funny thing is that it's CRLF_SAFE_RENORMALIZE. I don't know what the semantics of this value are, but the caller (crlf_to_git) explicitly checks for CRLF_SAFE_RENORMALIZE and changes it to CRLF_SAFE_FALSE instead. But that check only happens if crlf_action is CRLF_AUTO*, and for me it's CRLF_TEXT_INPUT. I moved the check to happen regardless of the value of crlf_action, and at least in this case, git appears to happily do the right thing. So I think this is a bug, but line endings are such a tangle that I'm really not sure. :) The repository in question is ZDoom: https://github.com/rheit/zdoom I'm trying to cherry-pick commits from the 3dfloors3 branch (e.g., 9fb2daf58e9d512170859302a1ac0ea9c2ec5993) onto a slightly outdated master, 6384e81d0f135a2c292ac3e874f6fe26093f45b1.
did you receive my previous email ?
Friedrich MayrhoferThis is the second time i am sending you this mail.I, Friedrich Mayrhofer Donate $ 1,000,000.00 to You, Email Me personally for more details. Regards. Friedrich Mayrhofer
Re: trustExitCode doesn't apply to vimdiff mergetool
On Sun, Nov 27, 2016 at 05:45:38PM -0800, David Aguilar wrote: > On Sun, Nov 27, 2016 at 11:55:59AM -0500, Jeff King wrote: > > On Sun, Nov 27, 2016 at 08:46:40AM -0500, Dun Peal wrote: > > > > > Ignoring a non-zero exit code from the merge tool, and assuming a > > > successful merge in that case, seems like the wrong default behavior > > > to me. > > > > Yeah, I'm inclined to agree. But like I said, I'm not too familiar with > > this area, so maybe there are subtle things I'm missing. > > I think this may have been an oversight in how the > trust-exit-code feature is implemented across builtins. > > Right now, specific builtin tools could in theory opt-in to the > feature, but I think it should be handled in a central place. > For vimdiff, the exit code is not considered because the > scriptlet calls check_unchanged(), which only cares about > modifciation time. > > I have a patch that makes it so that none of the tools do the > check_unchanged logic themselves and instead rely on the > library code to handle it for them. This makes the > implementation uniform across all tools, and allows tools to > opt-in to trustExitCode=true. > > This means that all of the builtin tools will default to > trustExitCode=false, and they can opt-in by setting the > configuration variable. > > For tkdiff and kdiff3, this is a subtle change in behavior, but > not one that should be problematic, and the upside is that we'll > have consistency across all tools. > > In this scenario specifically, what happens is that the > scriptlet is calling check_unchanged(), which checks the > modification time of the file, and if the file is new then it > assumes that the merge succeeded. check_unchanged() is clearing > the exit code. > > Try the patch below. I tested it with vimdiff and it seems to > provide the desired behavior: > - the modificaiton time behavior is the default > - setting mergetool.vimdiff.trustExitCode = true will make it > honor vim's exit code via :cq > > One possible idea that could avoid the subtle tkdiff/kdiff3 > change in behavior would be to allow the scriptlets to advertise > their preference for the default trustExitCode setting. These > tools could say, "default to true", and the rest can assume > false. > > If others feel that this is worth the extra machinery, and the > mental burden of tools having different defaults, then that > could be implemented as a follow-up patch. IMO I'd be okay with > not needing it and only adding it if someone notices, but if > others feel otherwise we can do it sooner rather than later. > > Thoughts? For the curious, here is what that patch might look like. This allows scriptlets to redefine trust_exit_code() so that they can advertise that they prefer default=true. The main benefit of this is that we're preserving the original behavior before these patches. I'll let this sit out here for comments for a few days to see what others think. --- >8 --- Date: Sun, 27 Nov 2016 18:08:08 -0800 Subject: [PATCH] mergetool: restore trustExitCode behavior for builtins tools deltawalker, diffmerge, emerge, kdiff3, kompare, and tkdiff originally provided behavior that matched trustExitCode=true. The default for all tools is trustExitCode=false, which conflicts with these tools' defaults. Allow tools to advertise their own default value for trustExitCode so that users do not need to opt-in to the original behavior. While this makes the default inconsistent between tools, it can still be overridden, and it makes it consistent with the current Git behavior. Signed-off-by: David Aguilar --- git-mergetool--lib.sh | 15 +-- mergetools/deltawalker | 6 ++ mergetools/diffmerge | 6 ++ mergetools/emerge | 6 ++ mergetools/kdiff3 | 6 ++ mergetools/kompare | 6 ++ mergetools/tkdiff | 6 ++ 7 files changed, 49 insertions(+), 2 deletions(-) diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh index 3d8a873ab..be079723a 100644 --- a/git-mergetool--lib.sh +++ b/git-mergetool--lib.sh @@ -120,6 +120,12 @@ setup_user_tool () { merge_tool_cmd=$(get_merge_tool_cmd "$tool") test -n "$merge_tool_cmd" || return 1 + trust_exit_code () { + # user-defined tools default to trustExitCode = false + git config --bool "mergetool.$1.trustExitCode" || + echo false + } + diff_cmd () { ( eval $merge_tool_cmd ) } @@ -153,6 +159,12 @@ setup_tool () { echo "$1" } + trust_exit_code () { + # built-in tools default to trustExitCode = false + git config --bool "mergetool.$1.trustExitCode" || + echo false + } + if ! test -f "$MERGE_TOOLS_DIR/$tool" then setup_user_tool @@ -221,8 +233,7 @@ run_merge_cmd () { merge_cmd "$1" status=$? - trust_exit_code=$(git config --bool \ - "mergetoo
Re: trustExitCode doesn't apply to vimdiff mergetool
On Sun, Nov 27, 2016 at 05:45:38PM -0800, David Aguilar wrote: > I have a patch that makes it so that none of the tools do the > check_unchanged logic themselves and instead rely on the > library code to handle it for them. This makes the > implementation uniform across all tools, and allows tools to > opt-in to trustExitCode=true. > > This means that all of the builtin tools will default to > trustExitCode=false, and they can opt-in by setting the > configuration variable. FWIW, that was the refactoring that came to mind when I looked at the code yesterday. This is the first time I've looked at the mergetool code, though, so you can take that with the appropriate grain of salt. Your patch looks mostly good to me. One minor comment: > merge_cmd () { > - trust_exit_code=$(git config --bool \ > - "mergetool.$1.trustExitCode" || echo false) > - if test "$trust_exit_code" = "false" > - then > - touch "$BACKUP" > - ( eval $merge_tool_cmd ) > - check_unchanged > - else > - ( eval $merge_tool_cmd ) > - fi > + ( eval $merge_tool_cmd ) > } > } > > @@ -225,7 +216,20 @@ run_diff_cmd () { > > # Run a either a configured or built-in merge tool > run_merge_cmd () { > + touch "$BACKUP" > + > merge_cmd "$1" > + status=$? > + > + trust_exit_code=$(git config --bool \ > + "mergetool.$1.trustExitCode" || echo false) > + if test "$trust_exit_code" = "false" > + then > + check_unchanged > + status=$? > + fi > + In the original, we only touch $BACKUP if we care about timestamps. I can't think of a reason it would matter to do the touch in the trustExitCode=true case, but you could also write it as: if test "$trust_exit_code" = "false" then touch "$BACKUP" merge_cmd "$1" check_unchanged else merge_cmd "$1" fi # now $? is from either merge_cmd or check_unchanged Yours is arguably less subtle, though, which may be a good thing. -Peff
Re: trustExitCode doesn't apply to vimdiff mergetool
On Sun, Nov 27, 2016 at 11:55:59AM -0500, Jeff King wrote: > On Sun, Nov 27, 2016 at 08:46:40AM -0500, Dun Peal wrote: > > > Ignoring a non-zero exit code from the merge tool, and assuming a > > successful merge in that case, seems like the wrong default behavior > > to me. > > Yeah, I'm inclined to agree. But like I said, I'm not too familiar with > this area, so maybe there are subtle things I'm missing. I think this may have been an oversight in how the trust-exit-code feature is implemented across builtins. Right now, specific builtin tools could in theory opt-in to the feature, but I think it should be handled in a central place. For vimdiff, the exit code is not considered because the scriptlet calls check_unchanged(), which only cares about modifciation time. I have a patch that makes it so that none of the tools do the check_unchanged logic themselves and instead rely on the library code to handle it for them. This makes the implementation uniform across all tools, and allows tools to opt-in to trustExitCode=true. This means that all of the builtin tools will default to trustExitCode=false, and they can opt-in by setting the configuration variable. For tkdiff and kdiff3, this is a subtle change in behavior, but not one that should be problematic, and the upside is that we'll have consistency across all tools. In this scenario specifically, what happens is that the scriptlet is calling check_unchanged(), which checks the modification time of the file, and if the file is new then it assumes that the merge succeeded. check_unchanged() is clearing the exit code. Try the patch below. I tested it with vimdiff and it seems to provide the desired behavior: - the modificaiton time behavior is the default - setting mergetool.vimdiff.trustExitCode = true will make it honor vim's exit code via :cq One possible idea that could avoid the subtle tkdiff/kdiff3 change in behavior would be to allow the scriptlets to advertise their preference for the default trustExitCode setting. These tools could say, "default to true", and the rest can assume false. If others feel that this is worth the extra machinery, and the mental burden of tools having different defaults, then that could be implemented as a follow-up patch. IMO I'd be okay with not needing it and only adding it if someone notices, but if others feel otherwise we can do it sooner rather than later. Thoughts? --- 8< --- Date: Sun, 27 Nov 2016 17:26:55 -0800 Subject: [PATCH] mergetool: honor mergetool..trustExitCode for all tools The built-in mergetools originally required that each tool scriptlet opt-in to the trustExitCode behavior, based on whether or not the tool called check_unchanged() itself. Refactor the functions so that run_merge_cmd() (rather than merge_cmd()) takes care of calling check_unchanged() so that all tools handle the trustExitCode behavior uniformly. Remove the check_unchanged() calls from the scriptlets. A subtle benefit of this change is that the responsibility of merge_cmd() has been narrowed to running the command only, rather than also needing to deal with the backup file and checking for changes. Reported-by: Dun Peal Signed-off-by: David Aguilar --- git-mergetool--lib.sh| 24 ++-- mergetools/araxis| 2 -- mergetools/bc| 2 -- mergetools/codecompare | 2 -- mergetools/diffuse | 2 -- mergetools/ecmerge | 2 -- mergetools/examdiff | 2 -- mergetools/meld | 3 +-- mergetools/opendiff | 2 -- mergetools/p4merge | 2 -- mergetools/tortoisemerge | 2 -- mergetools/vimdiff | 2 -- mergetools/winmerge | 2 -- mergetools/xxdiff| 2 -- 14 files changed, 15 insertions(+), 36 deletions(-) diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh index 9abd00be2..3d8a873ab 100644 --- a/git-mergetool--lib.sh +++ b/git-mergetool--lib.sh @@ -125,16 +125,7 @@ setup_user_tool () { } merge_cmd () { - trust_exit_code=$(git config --bool \ - "mergetool.$1.trustExitCode" || echo false) - if test "$trust_exit_code" = "false" - then - touch "$BACKUP" - ( eval $merge_tool_cmd ) - check_unchanged - else - ( eval $merge_tool_cmd ) - fi + ( eval $merge_tool_cmd ) } } @@ -225,7 +216,20 @@ run_diff_cmd () { # Run a either a configured or built-in merge tool run_merge_cmd () { + touch "$BACKUP" + merge_cmd "$1" + status=$? + + trust_exit_code=$(git config --bool \ + "mergetool.$1.trustExitCode" || echo false) + if test "$trust_exit_code" = "false" + then + check_unchanged + status=$? + fi + + return $status } list_merge_tool_candidates () { diff --git a/mergetools/araxis b/mergetools/araxis index 64f97c5e
[PATCH] contrib/subtree: added --no-show-signature to git log invocation
When having log.showSignature enabled by default (as is good practice with signed commits), it still shows the signature when git-subtree passes custom format specifiers to git-log. This causes an error when trying to push a subtree when signed commits are involved. Adding this command line flag fixes the above bug. The command line flag was added to all invocations of git-log so that it would behave as expected by the original developers. The flag could be more judiciously applied, but that requires a deeper understanding of the code. It may be more desirable to disable --show-signature when any custom format specifier is given, but that change has far more wide reaching consequences, as well as a change to the documentation. Signed-off-by: Kieran Colford --- contrib/subtree/git-subtree.sh | 12 ++-- 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh index dec085a..d9e89d1 100755 --- a/contrib/subtree/git-subtree.sh +++ b/contrib/subtree/git-subtree.sh @@ -296,7 +296,7 @@ find_latest_squash () { sq= main= sub= - git log --grep="^git-subtree-dir: $dir/*\$" \ + git log --no-show-signature --grep="^git-subtree-dir: $dir/*\$" \ --pretty=format:'START %H%n%s%n%n%b%nEND%n' HEAD | while read a b junk do @@ -340,7 +340,7 @@ find_existing_splits () { revs="$2" main= sub= - git log --grep="^git-subtree-dir: $dir/*\$" \ + git log --no-show-signature --grep="^git-subtree-dir: $dir/*\$" \ --pretty=format:'START %H%n%s%n%n%b%nEND%n' $revs | while read a b junk do @@ -382,7 +382,7 @@ copy_commit () { # We're going to set some environment vars here, so # do it in a subshell to get rid of them safely later debug copy_commit "{$1}" "{$2}" "{$3}" - git log -1 --pretty=format:'%an%n%ae%n%aD%n%cn%n%ce%n%cD%n%B' "$1" | + git log --no-show-signature -1 --pretty=format:'%an%n%ae%n%aD%n%cn%n%ce%n%cD%n%B' "$1" | ( read GIT_AUTHOR_NAME read GIT_AUTHOR_EMAIL @@ -462,8 +462,8 @@ squash_msg () { oldsub_short=$(git rev-parse --short "$oldsub") echo "Squashed '$dir/' changes from $oldsub_short..$newsub_short" echo - git log --pretty=tformat:'%h %s' "$oldsub..$newsub" - git log --pretty=tformat:'REVERT: %h %s' "$newsub..$oldsub" + git log --no-show-signature --pretty=tformat:'%h %s' "$oldsub..$newsub" + git log --no-show-signature --pretty=tformat:'REVERT: %h %s' "$newsub..$oldsub" else echo "Squashed '$dir/' content from commit $newsub_short" fi @@ -475,7 +475,7 @@ squash_msg () { toptree_for_commit () { commit="$1" - git log -1 --pretty=format:'%T' "$commit" -- || exit $? + git log --no-show-signature -1 --pretty=format:'%T' "$commit" -- || exit $? } subtree_for_commit () { -- 2.10.2
Re: trustExitCode doesn't apply to vimdiff mergetool
On Sun, Nov 27, 2016 at 08:46:40AM -0500, Dun Peal wrote: > Ignoring a non-zero exit code from the merge tool, and assuming a > successful merge in that case, seems like the wrong default behavior > to me. Yeah, I'm inclined to agree. But like I said, I'm not too familiar with this area, so maybe there are subtle things I'm missing. > Finally, if you're not using mergetools, how do you resolve conflicts? I just edit the conflicted sections in vim. I do use git-jump (see contrib/git-jump), but that's just to get to them quickly. -Peff
Re: [PATCH v3 1/2] difftool: add a skeleton for the upcoming builtin
On Sat, Nov 26, 2016 at 02:01:36PM +0100, Johannes Schindelin wrote: > > If you want to control it from outside the test script, you'd need > > something like: > > > > if test "$GIT_TEST_DIFFTOOL" = "builtin" > > That is a bit magic. I first used "GIT_USE_BUILTIN_DIFFTOOL" and it did > not work. My name is arguably more correct (see also Jakub's note about > "naming is hard"), but yours works because there is a "TEST" substring in > it. Yes. You are free to add an exception to the env list in test-lib.sh, but we usually use GIT_TEST_* to avoid having to do so. -Peff
[PATCH/RFC v1 1/1] New way to normalize the line endings
From: Torsten Bögershausen Sincec commit 6523728499e7 'convert: unify the "auto" handling of CRLF' the normalization instruction in Documentation/gitattributes.txt doesn't work any more. Update the documentation and add a test case. Reported by Kristian Adrup https://github.com/git-for-windows/git/issues/954 Signed-off-by: Torsten Bögershausen --- Documentation/gitattributes.txt | 7 +++ t/t0025-crlf-auto.sh| 29 + 2 files changed, 32 insertions(+), 4 deletions(-) diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt index 976243a..1f7529a 100644 --- a/Documentation/gitattributes.txt +++ b/Documentation/gitattributes.txt @@ -227,11 +227,10 @@ From a clean working directory: - $ echo "* text=auto" >.gitattributes -$ rm .git/index # Remove the index to force Git to -$ git reset # re-scan the working directory +$ git ls-files --eol | egrep "i/(crlf|mixed)" # find not normalized files +$ rm .git/index # Remove the index to re-scan the working directory +$ git add . $ git status# Show files that will be normalized -$ git add -u -$ git add .gitattributes $ git commit -m "Introduce end-of-line normalization" - diff --git a/t/t0025-crlf-auto.sh b/t/t0025-crlf-auto.sh index d0bee08..4ad4d02 100755 --- a/t/t0025-crlf-auto.sh +++ b/t/t0025-crlf-auto.sh @@ -152,4 +152,33 @@ test_expect_success 'eol=crlf _does_ normalize binary files' ' test -z "$LFwithNULdiff" ' +test_expect_success 'prepare unnormalized' ' + + > .gitattributes && + git config core.autocrlf false && + printf "LINEONE\nLINETWO\r\n" >mixed && + git add mixed .gitattributes && + git commit -m "Add mixed" && + git ls-files --eol | egrep "i/crlf" && + git ls-files --eol | egrep "i/mixed" + +' + +test_expect_success 'normalize unnormalized' ' + echo "* text=auto" >.gitattributes && + rm .git/index && + git add . && + git commit -m "Introduce end-of-line normalization" && + git ls-files --eol | tr "\\t" " " | sort >act && +cat >exp <
Re: trustExitCode doesn't apply to vimdiff mergetool
Thanks, Jeff. Ignoring a non-zero exit code from the merge tool, and assuming a successful merge in that case, seems like the wrong default behavior to me. If your merge tool quit with an error, it is more sensible to assume that the resolution you were working on has not been successfully concluded. In the rare case where one did successfully conclude the resolution, you can always quickly mark the file resolved. I'm not even sure how to do that short of `git checkout -m -- file`, which would lose any work you've already done towards the merge. Long story short, I hope the developers change this default, or at least let us override it for the builtin invocations. Finally, if you're not using mergetools, how do you resolve conflicts? On Sun, Nov 27, 2016 at 12:08 AM, Jeff King wrote: > On Sat, Nov 26, 2016 at 09:44:36PM -0500, Dun Peal wrote: > >> I'm using vimdiff as my mergetool, and have the following lines in >> ~/.gitconfig: >> >> [merge] >> tool = vimdiff >> [mergetool "vimdiff"] >> trustExitCode = true >> >> >> My understanding from the docs is that this sets >> mergetool.vimdiff.trustExitCode to true, thereby concluding that a >> merge hasn't been successful if vimdiff's exit code is non-zero. >> >> Unfortunately, when I exit Vim using `:cq` - which returns code 1 - >> the merge is still presumed to have succeeded. >> >> Is there a way to accomplish the desired effect, such that exiting >> vimdiff with a non-zero code would prevent git from resolving the >> conflict in the merged file? > > I don't use mergetool myself, but peeking at the code, it looks like > trustExitCode is used only for a "user" tool, not for the builtin tool > profiles. That sounds kind of confusing to me, but I'll leave discussion > of that to people more interested in mergetool. > > However, I think you can work around it by defining your own tool that > runs vimdiff: > > git config merge.tool foo > git config mergetool.foo.cmd 'vimdiff "$LOCAL" "$BASE" "$REMOTE" "$MERGED"' > git config mergetool.foo.trustExitCode true > > Though note that the builtin vimdiff invocation is a little more > complicated than that. You may want to adapt what is in git.git's > mergetools/vimdiff to your liking. > > -Peff
E-Book "Kredit"
Hallo, ich biete auf http://www.kreditzentrale.com/thema/online-kredit ein E-Book mit dem Titel "Kredit" zum kostenlosen Download an. Da ich bei meiner Themen-Recherche auch Ihre Webseite gefunden habe, dachte ich, das könnte Sie interessieren. Wäre es möglich, dass Sie meine Webseite bzw. das E-Book verlinken, z. B. von http://git.661346.n2.nabble.com/Darlehen-Angebot-td7601293.html? Um den Aufwand für Sie gering zu halten, kann ich Ihnen gerne einen Artikel zum Thema zusenden, zur Veröffentlichung auf Ihrer Webseite. Als Dankeschön biete ich Ihnen an, Ihre Webseite von einem meiner Blogs zu verlinken :-) Viele Grüße, Marcel Ziegler
Did you get my message this time?
This is the second time i am sending you this mail. I, Friedrich Mayrhofer Donate $ 1,000,000.00 to You, Email Me personally for more details. Regards. Friedrich Mayrhofer
Re: [PATCH v3 2/2] difftool: implement the functionality in the builtin
Hello Johannes, On 27 November 2016 at 12:10, Johannes Schindelin wrote: > On Fri, 25 Nov 2016, Jakub Narębski wrote: > > W dniu 24.11.2016 o 21:55, Johannes Schindelin pisze: [...] > > > +static int difftool_config(const char *var, const char *value, void *cb) > > > +{ > > > + if (!strcmp(var, "diff.guitool")) { > > > > Shouldn't you also read other configuration variables, like "diff.tool", > > and it's mergetool fallbacks ("merge.guitool", "merge.tool")? > > No, as those configuration variables are not used by the builtin difftool > directly but read by subsequently spawned commands separately. There would > be no use reading them here, for now. Ah, all right then. Though NEEDSWORK comment would be nice to have here (for when we don't spawn commands). [...] > I read the rest of your review, but it appears that it is more about > style than about substance, while I am only willing to address the latter > issues at the moment. You see, I want to focus on getting difftool correct > first before attempting to make it pretty. > > Ciao, > Dscho Well, excet for the submodule-relates stuff, which I have skipped, it looks good to me. -- Jakub Narębski
Re: [PATCH v3 2/2] difftool: implement the functionality in the builtin
Hi Jakub, On Fri, 25 Nov 2016, Jakub Narębski wrote: > W dniu 24.11.2016 o 21:55, Johannes Schindelin pisze: > > > The current version of the builtin difftool does not, however, make > > full use of the internals but instead chooses to spawn a couple of Git > > processes, still, to make for an easier conversion. There remains a > > lot of room for improvement, left for a later date. > [...] > > > Sadly, the speedup is more noticable on Linux than on Windows: a quick > > test shows that t7800-difftool.sh runs in (2.183s/0.052s/0.108s) > > (real/user/sys) in a Linux VM, down from (6.529s/3.112s/0.644s), > > while on Windows, it is (36.064s/2.730s/7.194s), down from > > (47.637s/2.407s/6.863s). The culprit is most likely the overhead > > incurred from *still* having to shell out to mergetool-lib.sh and > > difftool--helper.sh. > > Does this mean that our shell-based testsuite is not well suited to be > benchmark suite for comparing performance on MS Windows? It is quite likely the case that shell-based testing will always be inappropriate for performance testing. Even on Linux. > Or does it mean that "builtin-difftool" spawning Git processes is the > problem? At the moment I would have to guess, and I'd rather not. > > Signed-off-by: Johannes Schindelin > > --- > > builtin/difftool.c | 670 > > - > > 1 file changed, 669 insertions(+), 1 deletion(-) > > > > diff --git a/builtin/difftool.c b/builtin/difftool.c > > index 53870bb..3480920 100644 > > --- a/builtin/difftool.c > > +++ b/builtin/difftool.c > > @@ -11,9 +11,608 @@ > > * > > * Copyright (C) 2016 Johannes Schindelin > > */ > > +#include "cache.h" > > #include "builtin.h" > > #include "run-command.h" > > #include "exec_cmd.h" > > +#include "parse-options.h" > > +#include "argv-array.h" > > +#include "strbuf.h" > > +#include "lockfile.h" > > +#include "dir.h" > > + > > +static char *diff_gui_tool; > > +static int trust_exit_code; > > + > > +static const char *const builtin_difftool_usage[] = { > > + N_("git difftool [] [ []] [--] [...]"), > > + NULL > > +}; > > + > > +static int difftool_config(const char *var, const char *value, void *cb) > > +{ > > + if (!strcmp(var, "diff.guitool")) { > > Shouldn't you also read other configuration variables, like "diff.tool", > and it's mergetool fallbacks ("merge.guitool", "merge.tool")? No, as those configuration variables are not used by the builtin difftool directly but read by subsequently spawned commands separately. There would be no use reading them here, for now. > > +static int print_tool_help(void) > > +{ > > + const char *argv[] = { "mergetool", "--tool-help=diff", NULL }; > > + return run_command_v_opt(argv, RUN_GIT_CMD); > > This looks a bit strange to me, but I guess this is to avoid recursively > invoking ourself, and { "legacy-difftool", "--tool-help", NULL }; isn't > that much better. This is obviously a straight translation of the Perl script (see https://github.com/git/git/blob/v2.10.2/git-difftool.perl#L40-L46): sub print_tool_help { # See the comment at the bottom of file_diff() for the reason # behind # using system() followed by exit() instead of exec(). my $rc = system(qw(git mergetool --tool-help=diff)); exit($rc | ($rc >> 8)); } I read the rest of your review, but it appears that it is more about style than about substance, while I am only willing to address the latter issues at the moment. You see, I want to focus on getting difftool correct first before attempting to make it pretty. Ciao, Dscho
Talent Scout
Dear Concern, I am Talent Scout For BLUE SKY FILM STUDIO, Present Blue sky Studio a Film Corporation Located in the United State, is Soliciting for the Right to use Your Photo/Face and Personality as One of the Semi -Major Role/ Character in our Upcoming ANIMATED Stereoscope 3D Movie-The Story of Ferdinand (Ferdinand 2017) The Movie is Currently Filming (In Production) Please Note That There Will Be No Auditions, Traveling or Any Special / Professional Acting Skills, Since the Production of This Movie Will Be Done with our State of Art Computer -Generating Imagery Equipment. We Are Prepared to Pay the Total Sum of $620,000.00 USD. For More Information/Understanding, Please Write us on the E-Mail Below. CONTACT EMAIL: blueskystu...@usa.com All Reply to: blueskystu...@usa.com Note: Only the Response send to this mail will be Given a Prior Consideration. Talent Scout Camilia Brunnet
Re: [PATCH v3 1/2] difftool: add a skeleton for the upcoming builtin
Hi Peff, On Sat, 26 Nov 2016, Jeff King wrote: > On Sat, Nov 26, 2016 at 01:22:28PM +0100, Johannes Schindelin wrote: > > > In other words, GIT_CONFIG_PARAMETERS is *explicitly scrubbed* from the > > environment when we run our tests (by the code block between the "before" > > and the "after" statements in the diff above). > > Sorry if I wasn't clear. I meant to modify t7800 to run the tests twice, > once with the existing script and once with the builtin. I.e., to set > the variable after test-lib.sh has done its scrubbing, and then use a > loop or similar to go through the tests twice. > > If you want to control it from outside the test script, you'd need > something like: > > if test "$GIT_TEST_DIFFTOOL" = "builtin" That is a bit magic. I first used "GIT_USE_BUILTIN_DIFFTOOL" and it did not work. My name is arguably more correct (see also Jakub's note about "naming is hard"), but yours works because there is a "TEST" substring in it. Ciao, Dscho
Xmas Offer
You are a recipient to Mrs Julie Leach Donation of $3 million USD. Contact ( julieleac...@gmail.com ) for claims.
Talent Scout
Dear Concern, I am Talent Scout For BLUE SKY FILM STUDIO, Present Blue sky Studio a Film Corporation Located in the United State, is Soliciting for the Right to use Your Photo/Face and Personality as One of the Semi -Major Role/ Character in our Upcoming ANIMATED Stereoscope 3D Movie-The Story of Ferdinand (Ferdinand 2017) The Movie is Currently Filming (In Production) Please Note That There Will Be No Auditions, Traveling or Any Special / Professional Acting Skills, Since the Production of This Movie Will Be Done with our State of Art Computer -Generating Imagery Equipment. We Are Prepared to Pay the Total Sum of $620,000.00 USD. For More Information/Understanding, Please Write us on the E-Mail Below. CONTACT EMAIL: blueskystu...@usa.com All Reply to: blueskystu...@usa.com Note: Only the Response send to this mail will be Given a Prior Consideration. Talent Scout Camilia Brunnet