cherry-pick -Xrenormalize fails with formerly CRLF files

[Eevee (Lexy Munroe)]

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 Mayrhofer]

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

[David Aguilar]

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

Re: trustExitCode doesn't apply to vimdiff mergetool

[Jeff King]

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

[David Aguilar]

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 

[PATCH] contrib/subtree: added --no-show-signature to git log invocation

[Kieran Colford]

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

[Jeff King]

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

[Jeff King]

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

[tboegi]

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

[Dun Peal]

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"

[Marcel Ziegler]

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?

[Friedrich Mayrhofer]

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

[Jakub Narębski]

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

[Johannes Schindelin]

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

[Camilia Brunnet]

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

[Johannes Schindelin]

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

[Mrs Julie Leach]

You are a recipient to Mrs Julie Leach Donation of $3 million USD. Contact ( 
julieleac...@gmail.com ) for claims.

Talent Scout

[Camilia Brunnet]

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