* git: regression with mergetool and answering "n" (backport fix / add tests)
@ 2014-12-23 19:08 Daniel Hahler
2014-12-26 1:00 ` David Aguilar
0 siblings, 1 reply; 4+ messages in thread
From: Daniel Hahler @ 2014-12-23 19:08 UTC (permalink / raw)
To: David Aguilar, Junio C Hamano; +Cc: git
[-- Attachment #1: Type: text/plain, Size: 3826 bytes --]
Hi,
this is in reply to the commits from David:
commit 0ddedd4d6b9b3e8eb3557d8ed28e1a0b354a25f8
Refs: v2.2.0-60-g0ddedd4
Merge: e886efd 1e86d5b
Author: Junio C Hamano <gitster@pobox.com>
AuthorDate: Fri Dec 12 14:31:39 2014 -0800
Commit: Junio C Hamano <gitster@pobox.com>
CommitDate: Fri Dec 12 14:31:39 2014 -0800
Merge branch 'da/difftool-mergetool-simplify-reporting-status'
Code simplification.
* da/difftool-mergetool-simplify-reporting-status:
mergetools: stop setting $status in merge_cmd()
mergetool: simplify conditionals
difftool--helper: add explicit exit statement
mergetool--lib: remove use of $status global
mergetool--lib: remove no-op assignment to $status from setup_user_tool
I've ran into a problem, where "git mergetool" (using vimdiff) would add
the changes to the index, although you'd answered "n" after not changing/saving
the merged file.
This regression has been introduced in:
commit 99474b6340dbcbe58f6c256fdee231cbadb060f4
Author: David Aguilar <davvid@gmail.com>
Date: Fri Nov 14 13:33:55 2014 -0800
difftool: honor --trust-exit-code for builtin tools
run_merge_tool() was not setting $status, which prevented the
exit code for builtin tools from being forwarded to the caller.
Capture the exit status and add a test to guarantee the behavior.
Reported-by: Adria Farres <14farresa@gmail.com>
Signed-off-by: David Aguilar <davvid@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index c45a020..cce4f8c 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -221,6 +221,7 @@ run_merge_tool () {
else
run_diff_cmd "$1"
fi
+ status=$?
return $status
}
My fix has been the following, but I agree that the changes from David
are much better in general.
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index cce4f8c..fa9acb1 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -105,6 +105,7 @@ check_unchanged () {
esac
done
fi
+ return $status
}
valid_tool () {
I haven't verified if it really fixes the regression, but if it does it
should get backported into the branches where the regression is present.
Also, there should be some tests for this. I've came up with the
following, which is not finished and left in a state of debugging why it
did not trigger the bug.
In t/t7610-mergetool.sh:
test_expect_success 'return code with untrusted mergetool' '
# git config merge.tool untrusted &&
# git config mergetool.untrusted.cmd "true" &&
# git config mergetool.untrusted.trustExitCode false &&
test_config mergetool.vimdiff.cmd "cat \"\$REMOTE\"" &&
git config merge.tool vimdiff &&
git config --get-regexp mergetool.
git config --get-regexp merge\.tool.
git checkout -b test_untrusted branch1 &&
test_must_fail git merge master >/dev/null 2>&1 &&
git status -s &&
test "$(git status --short both)" = "AA both" &&
( echo "n" | test_must_fail git mergetool both ) &&
git status -s &&
test "$(git status --short both)" = "AA both" &&
( echo "y" | git mergetool both ) &&
git status -s &&
test "$(git status --short both)" = "M both" &&
git reset --hard &&
git config merge.tool mytool
Cheers,
Daniel.
--
http://daniel.hahler.de/
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 173 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: git: regression with mergetool and answering "n" (backport fix / add tests)
2014-12-23 19:08 git: regression with mergetool and answering "n" (backport fix / add tests) Daniel Hahler
@ 2014-12-26 1:00 ` David Aguilar
2014-12-26 1:12 ` Daniel Hahler
0 siblings, 1 reply; 4+ messages in thread
From: David Aguilar @ 2014-12-26 1:00 UTC (permalink / raw)
To: Daniel Hahler; +Cc: Junio C Hamano, git
On Tue, Dec 23, 2014 at 08:08:34PM +0100, Daniel Hahler wrote:
> Hi,
>
> this is in reply to the commits from David:
>
> commit 0ddedd4d6b9b3e8eb3557d8ed28e1a0b354a25f8
> Refs: v2.2.0-60-g0ddedd4
> Merge: e886efd 1e86d5b
> Author: Junio C Hamano <gitster@pobox.com>
> AuthorDate: Fri Dec 12 14:31:39 2014 -0800
> Commit: Junio C Hamano <gitster@pobox.com>
> CommitDate: Fri Dec 12 14:31:39 2014 -0800
>
> Merge branch 'da/difftool-mergetool-simplify-reporting-status'
>
> Code simplification.
>
> * da/difftool-mergetool-simplify-reporting-status:
> mergetools: stop setting $status in merge_cmd()
> mergetool: simplify conditionals
> difftool--helper: add explicit exit statement
> mergetool--lib: remove use of $status global
> mergetool--lib: remove no-op assignment to $status from setup_user_tool
>
> I've ran into a problem, where "git mergetool" (using vimdiff) would add
> the changes to the index, although you'd answered "n" after not changing/saving
> the merged file.
Thanks for the heads-up.
Do you perhaps have mergetool.vimdiff.trustExitCode defined, or
a similar setting?
If you saw the prompt then it should have aborted right after
you answered "n".
The very last thing merge_cmd() for vimdiff does is call
check_unchanged(). We'll come back to check_unchanged() later.
I tried to reproduce this issue. Here's a transcript:
....
$ git status -s
UU file.txt
$ git mergetool -t vimdiff file.txt
Merging:
file.txt
Normal merge conflict for 'file.txt':
{local}: modified file
{remote}: modified file
4 files to edit
#### Enter :qall inside vim
file.txt seems unchanged.
Was the merge successful? [y/n] n
merge of file.txt failed
Continue merging other unresolved paths (y/n) ? n
$ git status -s
UU file.txt
....
That seemed to work fine. Any clues?
More notes below...
> This regression has been introduced in:
>
> commit 99474b6340dbcbe58f6c256fdee231cbadb060f4
> Author: David Aguilar <davvid@gmail.com>
> Date: Fri Nov 14 13:33:55 2014 -0800
>
> difftool: honor --trust-exit-code for builtin tools
>
> run_merge_tool() was not setting $status, which prevented the
> exit code for builtin tools from being forwarded to the caller.
>
> Capture the exit status and add a test to guarantee the behavior.
>
> Reported-by: Adria Farres <14farresa@gmail.com>
> Signed-off-by: David Aguilar <davvid@gmail.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>
> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> index c45a020..cce4f8c 100644
> --- a/git-mergetool--lib.sh
> +++ b/git-mergetool--lib.sh
> @@ -221,6 +221,7 @@ run_merge_tool () {
> else
> run_diff_cmd "$1"
> fi
> + status=$?
> return $status
> }
>
>
> My fix has been the following, but I agree that the changes from David
> are much better in general.
>
> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> index cce4f8c..fa9acb1 100644
> --- a/git-mergetool--lib.sh
> +++ b/git-mergetool--lib.sh
> @@ -105,6 +105,7 @@ check_unchanged () {
> esac
> done
> fi
> + return $status
> }
I don't think this fix does anything.
Here is all of check_unchanged() for context:
check_unchanged () {
if test "$MERGED" -nt "$BACKUP"
then
return 0
else
while true
do
echo "$MERGED seems unchanged."
printf "Was the merge successful? [y/n] "
read answer || return 1
case "$answer" in
y*|Y*) return 0 ;;
n*|N*) return 1 ;;
esac
done
fi
}
The addition of "return $status" after the "fi" in the above fix
won't do anything because that code is unreachable.
We either return 0 or 1.
> I haven't verified if it really fixes the regression, but if it does it
> should get backported into the branches where the regression is present.
Also, the $status variable doesn't even exist anymore, so the
fix is suspect.
What platform are you on?
> Also, there should be some tests for this.
I don't disagree with that ;-)
Let me know if you have any clues. I don't see anything obvious.
cheers,
--
David
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: git: regression with mergetool and answering "n" (backport fix / add tests)
2014-12-26 1:00 ` David Aguilar
@ 2014-12-26 1:12 ` Daniel Hahler
2015-01-23 13:19 ` Daniel Hahler
0 siblings, 1 reply; 4+ messages in thread
From: Daniel Hahler @ 2014-12-26 1:12 UTC (permalink / raw)
To: David Aguilar; +Cc: Junio C Hamano, git
[-- Attachment #1: Type: text/plain, Size: 4965 bytes --]
Hi David,
sorry for the confusion - the patch / fix I've mentioned was meant to be
applied on the commit that caused the regression and not current master.
Cheers,
Daniel.
On 26.12.2014 02:00, David Aguilar wrote:
> On Tue, Dec 23, 2014 at 08:08:34PM +0100, Daniel Hahler wrote:
>> Hi,
>>
>> this is in reply to the commits from David:
>>
>> commit 0ddedd4d6b9b3e8eb3557d8ed28e1a0b354a25f8
>> Refs: v2.2.0-60-g0ddedd4
>> Merge: e886efd 1e86d5b
>> Author: Junio C Hamano <gitster@pobox.com>
>> AuthorDate: Fri Dec 12 14:31:39 2014 -0800
>> Commit: Junio C Hamano <gitster@pobox.com>
>> CommitDate: Fri Dec 12 14:31:39 2014 -0800
>>
>> Merge branch 'da/difftool-mergetool-simplify-reporting-status'
>>
>> Code simplification.
>>
>> * da/difftool-mergetool-simplify-reporting-status:
>> mergetools: stop setting $status in merge_cmd()
>> mergetool: simplify conditionals
>> difftool--helper: add explicit exit statement
>> mergetool--lib: remove use of $status global
>> mergetool--lib: remove no-op assignment to $status from setup_user_tool
>>
>> I've ran into a problem, where "git mergetool" (using vimdiff) would add
>> the changes to the index, although you'd answered "n" after not changing/saving
>> the merged file.
>
> Thanks for the heads-up.
>
> Do you perhaps have mergetool.vimdiff.trustExitCode defined, or
> a similar setting?
>
> If you saw the prompt then it should have aborted right after
> you answered "n".
>
> The very last thing merge_cmd() for vimdiff does is call
> check_unchanged(). We'll come back to check_unchanged() later.
>
> I tried to reproduce this issue. Here's a transcript:
>
> ....
> $ git status -s
> UU file.txt
>
> $ git mergetool -t vimdiff file.txt
> Merging:
> file.txt
>
> Normal merge conflict for 'file.txt':
> {local}: modified file
> {remote}: modified file
> 4 files to edit
> #### Enter :qall inside vim
> file.txt seems unchanged.
> Was the merge successful? [y/n] n
> merge of file.txt failed
> Continue merging other unresolved paths (y/n) ? n
>
> $ git status -s
> UU file.txt
> ....
>
> That seemed to work fine. Any clues?
> More notes below...
>
>> This regression has been introduced in:
>>
>> commit 99474b6340dbcbe58f6c256fdee231cbadb060f4
>> Author: David Aguilar <davvid@gmail.com>
>> Date: Fri Nov 14 13:33:55 2014 -0800
>>
>> difftool: honor --trust-exit-code for builtin tools
>>
>> run_merge_tool() was not setting $status, which prevented the
>> exit code for builtin tools from being forwarded to the caller.
>>
>> Capture the exit status and add a test to guarantee the behavior.
>>
>> Reported-by: Adria Farres <14farresa@gmail.com>
>> Signed-off-by: David Aguilar <davvid@gmail.com>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>>
>> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
>> index c45a020..cce4f8c 100644
>> --- a/git-mergetool--lib.sh
>> +++ b/git-mergetool--lib.sh
>> @@ -221,6 +221,7 @@ run_merge_tool () {
>> else
>> run_diff_cmd "$1"
>> fi
>> + status=$?
>> return $status
>> }
>>
>>
>> My fix has been the following, but I agree that the changes from David
>> are much better in general.
>>
>> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
>> index cce4f8c..fa9acb1 100644
>> --- a/git-mergetool--lib.sh
>> +++ b/git-mergetool--lib.sh
>> @@ -105,6 +105,7 @@ check_unchanged () {
>> esac
>> done
>> fi
>> + return $status
>> }
>
> I don't think this fix does anything.
> Here is all of check_unchanged() for context:
>
> check_unchanged () {
> if test "$MERGED" -nt "$BACKUP"
> then
> return 0
> else
> while true
> do
> echo "$MERGED seems unchanged."
> printf "Was the merge successful? [y/n] "
> read answer || return 1
> case "$answer" in
> y*|Y*) return 0 ;;
> n*|N*) return 1 ;;
> esac
> done
> fi
> }
>
> The addition of "return $status" after the "fi" in the above fix
> won't do anything because that code is unreachable.
> We either return 0 or 1.
>
>> I haven't verified if it really fixes the regression, but if it does it
>> should get backported into the branches where the regression is present.
>
> Also, the $status variable doesn't even exist anymore, so the
> fix is suspect.
>
> What platform are you on?
>
>> Also, there should be some tests for this.
>
> I don't disagree with that ;-)
>
> Let me know if you have any clues. I don't see anything obvious.
>
> cheers,
>
--
http://daniel.hahler.de/
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 173 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: git: regression with mergetool and answering "n" (backport fix / add tests)
2014-12-26 1:12 ` Daniel Hahler
@ 2015-01-23 13:19 ` Daniel Hahler
0 siblings, 0 replies; 4+ messages in thread
From: Daniel Hahler @ 2015-01-23 13:19 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
[-- Attachment #1: Type: text/plain, Size: 5307 bytes --]
Hi,
I am a bit surprised that this bug still exists in "maint" / v2.2.2.
Cherry-picking/merging 0ddedd4 fixes it.
Regards,
Daniel.
On 26.12.2014 02:12, Daniel Hahler wrote:
> Hi David,
>
> sorry for the confusion - the patch / fix I've mentioned was meant to be
> applied on the commit that caused the regression and not current master.
>
>
> Cheers,
> Daniel.
>
> On 26.12.2014 02:00, David Aguilar wrote:
>> On Tue, Dec 23, 2014 at 08:08:34PM +0100, Daniel Hahler wrote:
>>> Hi,
>>>
>>> this is in reply to the commits from David:
>>>
>>> commit 0ddedd4d6b9b3e8eb3557d8ed28e1a0b354a25f8
>>> Refs: v2.2.0-60-g0ddedd4
>>> Merge: e886efd 1e86d5b
>>> Author: Junio C Hamano <gitster@pobox.com>
>>> AuthorDate: Fri Dec 12 14:31:39 2014 -0800
>>> Commit: Junio C Hamano <gitster@pobox.com>
>>> CommitDate: Fri Dec 12 14:31:39 2014 -0800
>>>
>>> Merge branch 'da/difftool-mergetool-simplify-reporting-status'
>>>
>>> Code simplification.
>>>
>>> * da/difftool-mergetool-simplify-reporting-status:
>>> mergetools: stop setting $status in merge_cmd()
>>> mergetool: simplify conditionals
>>> difftool--helper: add explicit exit statement
>>> mergetool--lib: remove use of $status global
>>> mergetool--lib: remove no-op assignment to $status from setup_user_tool
>>>
>>> I've ran into a problem, where "git mergetool" (using vimdiff) would add
>>> the changes to the index, although you'd answered "n" after not changing/saving
>>> the merged file.
>>
>> Thanks for the heads-up.
>>
>> Do you perhaps have mergetool.vimdiff.trustExitCode defined, or
>> a similar setting?
>>
>> If you saw the prompt then it should have aborted right after
>> you answered "n".
>>
>> The very last thing merge_cmd() for vimdiff does is call
>> check_unchanged(). We'll come back to check_unchanged() later.
>>
>> I tried to reproduce this issue. Here's a transcript:
>>
>> ....
>> $ git status -s
>> UU file.txt
>>
>> $ git mergetool -t vimdiff file.txt
>> Merging:
>> file.txt
>>
>> Normal merge conflict for 'file.txt':
>> {local}: modified file
>> {remote}: modified file
>> 4 files to edit
>> #### Enter :qall inside vim
>> file.txt seems unchanged.
>> Was the merge successful? [y/n] n
>> merge of file.txt failed
>> Continue merging other unresolved paths (y/n) ? n
>>
>> $ git status -s
>> UU file.txt
>> ....
>>
>> That seemed to work fine. Any clues?
>> More notes below...
>>
>>> This regression has been introduced in:
>>>
>>> commit 99474b6340dbcbe58f6c256fdee231cbadb060f4
>>> Author: David Aguilar <davvid@gmail.com>
>>> Date: Fri Nov 14 13:33:55 2014 -0800
>>>
>>> difftool: honor --trust-exit-code for builtin tools
>>>
>>> run_merge_tool() was not setting $status, which prevented the
>>> exit code for builtin tools from being forwarded to the caller.
>>>
>>> Capture the exit status and add a test to guarantee the behavior.
>>>
>>> Reported-by: Adria Farres <14farresa@gmail.com>
>>> Signed-off-by: David Aguilar <davvid@gmail.com>
>>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>>>
>>> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
>>> index c45a020..cce4f8c 100644
>>> --- a/git-mergetool--lib.sh
>>> +++ b/git-mergetool--lib.sh
>>> @@ -221,6 +221,7 @@ run_merge_tool () {
>>> else
>>> run_diff_cmd "$1"
>>> fi
>>> + status=$?
>>> return $status
>>> }
>>>
>>>
>>> My fix has been the following, but I agree that the changes from David
>>> are much better in general.
>>>
>>> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
>>> index cce4f8c..fa9acb1 100644
>>> --- a/git-mergetool--lib.sh
>>> +++ b/git-mergetool--lib.sh
>>> @@ -105,6 +105,7 @@ check_unchanged () {
>>> esac
>>> done
>>> fi
>>> + return $status
>>> }
>>
>> I don't think this fix does anything.
>> Here is all of check_unchanged() for context:
>>
>> check_unchanged () {
>> if test "$MERGED" -nt "$BACKUP"
>> then
>> return 0
>> else
>> while true
>> do
>> echo "$MERGED seems unchanged."
>> printf "Was the merge successful? [y/n] "
>> read answer || return 1
>> case "$answer" in
>> y*|Y*) return 0 ;;
>> n*|N*) return 1 ;;
>> esac
>> done
>> fi
>> }
>>
>> The addition of "return $status" after the "fi" in the above fix
>> won't do anything because that code is unreachable.
>> We either return 0 or 1.
>>
>>> I haven't verified if it really fixes the regression, but if it does it
>>> should get backported into the branches where the regression is present.
>>
>> Also, the $status variable doesn't even exist anymore, so the
>> fix is suspect.
>>
>> What platform are you on?
>>
>>> Also, there should be some tests for this.
>>
>> I don't disagree with that ;-)
>>
>> Let me know if you have any clues. I don't see anything obvious.
>>
>> cheers,
>>
>
--
http://daniel.hahler.de/
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 173 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-01-23 13:19 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-12-23 19:08 git: regression with mergetool and answering "n" (backport fix / add tests) Daniel Hahler
2014-12-26 1:00 ` David Aguilar
2014-12-26 1:12 ` Daniel Hahler
2015-01-23 13:19 ` Daniel Hahler
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).