* Re: [PATCH 1/2] cherry-pick: demonstrate a segmentation fault
From: Johannes Schindelin @ 2016-11-26 12:47 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Markus Klein
In-Reply-To: <89ffd6eaf4e1e121426c84f31dfc9c289f2a948b.1480091758.git.johannes.schindelin@gmx.de>
Hi,
On Fri, 25 Nov 2016, Johannes Schindelin wrote:
> +test_expect_failure 'cherry-pick fails gracefully with dirty renamed file' '
Woops. This title is wrong. It should say instead: 'cherry-pick succeeds
with unrelated renamed, dirty file'.
> + test_commit to-rename &&
> + git checkout -b unrelated &&
> + test_commit unrelated &&
> + git checkout @{-1} &&
> + git mv to-rename.t renamed &&
> + test_tick &&
> + git commit -m renamed &&
> + echo modified >renamed &&
> + git cherry-pick unrelated
And this actually warns about an ambiguous refname.
Will send out v2 in a moment.
Ciao,
Dscho
^ permalink raw reply
* [PATCH v2 0/2] Fix segmentation fault with cherry-pick
From: Johannes Schindelin @ 2016-11-26 12:47 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Markus Klein
In-Reply-To: <cover.1480091758.git.johannes.schindelin@gmx.de>
The culprit is actually not cherry-pick, but a special code path that
expects refresh_cache_entry() not to return NULL. And the fix is to
teach it to handle NULL there.
This bug was brought to my attention by Markus Klein via
https://github.com/git-for-windows/git/issues/952.
Changes since v1:
- changed test title
- avoided ambiguous refname in test
Johannes Schindelin (2):
cherry-pick: demonstrate a segmentation fault
Avoid a segmentation fault with renaming merges
merge-recursive.c | 2 ++
t/t3501-revert-cherry-pick.sh | 12 ++++++++++++
2 files changed, 14 insertions(+)
base-commit: e2b2d6a172b76d44cb7b1ddb12ea5bfac9613a44
Published-As: https://github.com/dscho/git/releases/tag/cherry-pick-segfault-v2
Fetch-It-Via: git fetch https://github.com/dscho/git cherry-pick-segfault-v2
Interdiff vs v1:
diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
index 8e21840f11..4f2a263b63 100755
--- a/t/t3501-revert-cherry-pick.sh
+++ b/t/t3501-revert-cherry-pick.sh
@@ -141,7 +141,7 @@ test_expect_success 'cherry-pick "-" works with arguments' '
test_cmp expect actual
'
-test_expect_success 'cherry-pick fails gracefully with dirty renamed file' '
+test_expect_success 'cherry-pick works with dirty renamed file' '
test_commit to-rename &&
git checkout -b unrelated &&
test_commit unrelated &&
@@ -150,7 +150,7 @@ test_expect_success 'cherry-pick fails gracefully with dirty renamed file' '
test_tick &&
git commit -m renamed &&
echo modified >renamed &&
- git cherry-pick unrelated
+ git cherry-pick refs/heads/unrelated
'
test_done
--
2.11.0.rc3.windows.1
^ permalink raw reply
* [PATCH v2 1/2] cherry-pick: demonstrate a segmentation fault
From: Johannes Schindelin @ 2016-11-26 12:48 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Markus Klein
In-Reply-To: <cover.1480164459.git.johannes.schindelin@gmx.de>
In https://github.com/git-for-windows/git/issues/952, a complicated
scenario was described that leads to a segmentation fault in
cherry-pick.
It boils down to a certain code path involving a renamed file that is
dirty, for which `refresh_cache_entry()` returns `NULL`, and that
`NULL` not being handled properly.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
t/t3501-revert-cherry-pick.sh | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
index 394f0005a1..d7b4251234 100755
--- a/t/t3501-revert-cherry-pick.sh
+++ b/t/t3501-revert-cherry-pick.sh
@@ -141,4 +141,16 @@ test_expect_success 'cherry-pick "-" works with arguments' '
test_cmp expect actual
'
+test_expect_failure 'cherry-pick works with dirty renamed file' '
+ test_commit to-rename &&
+ git checkout -b unrelated &&
+ test_commit unrelated &&
+ git checkout @{-1} &&
+ git mv to-rename.t renamed &&
+ test_tick &&
+ git commit -m renamed &&
+ echo modified >renamed &&
+ git cherry-pick refs/heads/unrelated
+'
+
test_done
--
2.11.0.rc3.windows.1
^ permalink raw reply related
* [PATCH v2 2/2] Avoid a segmentation fault with renaming merges
From: Johannes Schindelin @ 2016-11-26 12:48 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Markus Klein
In-Reply-To: <cover.1480164459.git.johannes.schindelin@gmx.de>
Under very particular circumstances, merge-recursive's `add_cacheinfo()`
function gets a `NULL` returned from `refresh_cache_entry()` without
expecting it, and subsequently passes it to `add_cache_entry()` which
consequently crashes.
Let's not crash.
This fixes https://github.com/git-for-windows/git/issues/952
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
merge-recursive.c | 2 ++
t/t3501-revert-cherry-pick.sh | 2 +-
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/merge-recursive.c b/merge-recursive.c
index 9041c2f149..609061f58a 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -235,6 +235,8 @@ static int add_cacheinfo(struct merge_options *o,
struct cache_entry *nce;
nce = refresh_cache_entry(ce, CE_MATCH_REFRESH | CE_MATCH_IGNORE_MISSING);
+ if (!nce)
+ return err(o, _("addinfo: '%s' is not up-to-date"), path);
if (nce != ce)
ret = add_cache_entry(nce, options);
}
diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
index d7b4251234..4f2a263b63 100755
--- a/t/t3501-revert-cherry-pick.sh
+++ b/t/t3501-revert-cherry-pick.sh
@@ -141,7 +141,7 @@ test_expect_success 'cherry-pick "-" works with arguments' '
test_cmp expect actual
'
-test_expect_failure 'cherry-pick works with dirty renamed file' '
+test_expect_success 'cherry-pick works with dirty renamed file' '
test_commit to-rename &&
git checkout -b unrelated &&
test_commit unrelated &&
--
2.11.0.rc3.windows.1
^ permalink raw reply related
* Re: [PATCH v2 2/2] Avoid a segmentation fault with renaming merges
From: Johannes Schindelin @ 2016-11-26 12:53 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Markus Klein
In-Reply-To: <d1571a25e8f3860a2867b00994d4d6938aa602ec.1480164459.git.johannes.schindelin@gmx.de>
Hi,
On Sat, 26 Nov 2016, Johannes Schindelin wrote:
> diff --git a/merge-recursive.c b/merge-recursive.c
> index 9041c2f149..609061f58a 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -235,6 +235,8 @@ static int add_cacheinfo(struct merge_options *o,
> struct cache_entry *nce;
>
> nce = refresh_cache_entry(ce, CE_MATCH_REFRESH | CE_MATCH_IGNORE_MISSING);
> + if (!nce)
> + return err(o, _("addinfo: '%s' is not up-to-date"), path);
> if (nce != ce)
> ret = add_cache_entry(nce, options);
> }
BTW I was not quite sure why we need to refresh the cache entry here, and
1335d76e45 (merge: avoid "safer crlf" during recording of merge results,
2016-07-08) has a commit message for which I need some time to wrap my
head around.
Also, an error here may be overkill. Maybe we should simply change the "if
(nce != ce)" to an "if (nce && nce != ce)" here, as a locally-modified
file will give a nicer message later, anyway.
Dunno,
Dscho
^ permalink raw reply
* RE: [char-misc-next] mei: request async autosuspend at the end of enumeration
From: Winkler, Tomas @ 2016-11-26 13:02 UTC (permalink / raw)
To: jnareb@gmail.com, Jeff King, Jiri Slaby,
Greg KH (gregkh@linuxfoundation.org), Ben Hutchings
Cc: Matthieu Moy, git@vger.kernel.org, Usyskin, Alexander,
linux-kernel@vger.kernel.org
In-Reply-To: <e11d28d3-c1b5-2c04-643f-0b3bd96cb4d3@gmail.com>
>
> W dniu 25.11.2016 o 04:14, Jeff King pisze:
> > On Thu, Nov 24, 2016 at 10:37:14PM +0000, Winkler, Tomas wrote:
> >
> >>>>> Cc: <stable@vger.kernel.org> # 4.4+
> >>>>
> >>>> Looks like git send-email is not able to parse this address
> >>>> correctly though this is suggested format by
> Documentation/stable_kernel_rules.txt.
> >>>> Create wrong address If git parsers is used : 'stable@vger.kernel.org#4.4+'
> [...]
>
> > The patch just brings parity to the Mail::Address behavior and git's
> > fallback parser, so that you don't end up with the broken
> > stable@vger.kernel.org#4.4+ address. Instead, that content goes into
> > the name part of the address.
> >
> > It sounds like you want the "# 4.4+" to be dropped entirely in the
> > rfc822 header. It looks like send-email used to do that, but stopped
> > in
> > b1c8a11c8 (send-email: allow multiple emails using --cc, --to and
> > --bcc, 2015-06-30).
> >
> > So perhaps there are further fixes required, but it's hard to know.
> > The input isn't a valid rfc822 header, so it's not entirely clear what
> > the output is supposed to be. I can buy either "drop it completely" or
> > "stick it in the name field of the cc header" as reasonable.
>
> Well, we could always convert it to email address comment, converting for
> example the following trailer:
>
> Cc: John Doe <john@example.com> # comment
>
> to the following address:
>
> John Doe <john@example.com> (comment)
>
> Just FYI. Though I'm not sure how well this would work...
>
Yep, it actually looks as right place to put this kind of info,
though I'm not on the receiving side.
I'm not sure if and how is this used by stable maintainers.
Thanks
Tomas
^ permalink raw reply
* git-daemon regression: 650c449250d7 common-main: call git_extract_argv0_path()
From: Mike Galbraith @ 2016-11-26 14:03 UTC (permalink / raw)
To: Jeff King; +Cc: git
Greetings,
git-daemon went broke on me post v2.9.3 due to binaries being installed
in /usr/lib/git, which is not in PATH. Reverting 650c449250d7 fixes it
up, as does ln -s /usr/lib/git/git-daemon /usr/bin/git-daemon 'course,
but thought I should report it, since it used to work without that.
Process 18804 attached
restart_syscall(<... resuming interrupted call ...>) = 1
accept(4, {sa_family=AF_INET6, sin6_port=htons(44400), inet_pton(AF_INET6, "::1", &sin6_addr), sin6_flowinfo=0, sin6_scope_id=0}, [28]) = 5
dup(5) = 6
pipe([7, 8]) = 0
clone(Process 18830 attached
child_stack=0, flags=CLONE_CHILD_CLEARTID|CLONE_CHILD_SETTID|SIGCHLD, child_tidptr=0x7f0e6061c9d0) = 18830
[pid 18830] set_robust_list(0x7f0e6061c9e0, 24 <unfinished ...>
[pid 18804] close(8 <unfinished ...>
[pid 18830] <... set_robust_list resumed> ) = 0
[pid 18804] <... close resumed> ) = 0
[pid 18804] read(7, <unfinished ...>
[pid 18830] close(7) = 0
[pid 18830] fcntl(8, F_GETFD) = 0
[pid 18830] fcntl(8, F_SETFD, FD_CLOEXEC) = 0
[pid 18830] dup2(5, 0) = 0
[pid 18830] close(5) = 0
[pid 18830] dup2(6, 1) = 1
[pid 18830] close(6) = 0
[pid 18830] execve("/usr/local/sbin/git-daemon", ["git-daemon", "--serve", "--syslog", "--detach", "--reuseaddr", "--user=git", "--group=daemon", "--pid-file=/var/run/git-daemon.p"..., "--export-all", "--user-path"], ["CONSOLE=/dev/console", "LC_ALL=POSIX", "REDIRECT=/dev/tty7", "COLUMNS=320", "PATH=/usr/local/sbin:/usr/local/"..., "PWD=/", "LINES=90", "SHLVL=1", "LC_CTYPE=en_US.UTF-8", "_=/sbin/startproc", "PREVLEVEL=", "RUNLEVEL=5", "DAEMON=/usr/lib/git/git-daemon", "REMOTE_ADDR=[::1]", "REMOTE_PORT=44400"]) = -1 ENOENT (No such file or directory)
[pid 18830] execve("/usr/local/bin/git-daemon", ["git-daemon", "--serve", "--syslog", "--detach", "--reuseaddr", "--user=git", "--group=daemon", "--pid-file=/var/run/git-daemon.p"..., "--export-all", "--user-path"], ["CONSOLE=/dev/console", "LC_ALL=POSIX", "REDIRECT=/dev/tty7", "COLUMNS=320", "PATH=/usr/local/sbin:/usr/local/"..., "PWD=/", "LINES=90", "SHLVL=1", "LC_CTYPE=en_US.UTF-8", "_=/sbin/startproc", "PREVLEVEL=", "RUNLEVEL=5", "DAEMON=/usr/lib/git/git-daemon", "REMOTE_ADDR=[::1]", "REMOTE_PORT=44400"]) = -1 ENOENT (No such file or directory)
[pid 18830] execve("/usr/sbin/git-daemon", ["git-daemon", "--serve", "--syslog", "--detach", "--reuseaddr", "--user=git", "--group=daemon", "--pid-file=/var/run/git-daemon.p"..., "--export-all", "--user-path"], ["CONSOLE=/dev/console", "LC_ALL=POSIX", "REDIRECT=/dev/tty7", "COLUMNS=320", "PATH=/usr/local/sbin:/usr/local/"..., "PWD=/", "LINES=90", "SHLVL=1", "LC_CTYPE=en_US.UTF-8", "_=/sbin/startproc", "PREVLEVEL=", "RUNLEVEL=5", "DAEMON=/usr/lib/git/git-daemon", "REMOTE_ADDR=[::1]", "REMOTE_PORT=44400"]) = -1 ENOENT (No such file or directory)
[pid 18830] execve("/usr/bin/git-daemon", ["git-daemon", "--serve", "--syslog", "--detach", "--reuseaddr", "--user=git", "--group=daemon", "--pid-file=/var/run/git-daemon.p"..., "--export-all", "--user-path"], ["CONSOLE=/dev/console", "LC_ALL=POSIX", "REDIRECT=/dev/tty7", "COLUMNS=320", "PATH=/usr/local/sbin:/usr/local/"..., "PWD=/", "LINES=90", "SHLVL=1", "LC_CTYPE=en_US.UTF-8", "_=/sbin/startproc", "PREVLEVEL=", "RUNLEVEL=5", "DAEMON=/usr/lib/git/git-daemon", "REMOTE_ADDR=[::1]", "REMOTE_PORT=44400"]) = -1 ENOENT (No such file or directory)
[pid 18830] execve("/sbin/git-daemon", ["git-daemon", "--serve", "--syslog", "--detach", "--reuseaddr", "--user=git", "--group=daemon", "--pid-file=/var/run/git-daemon.p"..., "--export-all", "--user-path"], ["CONSOLE=/dev/console", "LC_ALL=POSIX", "REDIRECT=/dev/tty7", "COLUMNS=320", "PATH=/usr/local/sbin:/usr/local/"..., "PWD=/", "LINES=90", "SHLVL=1", "LC_CTYPE=en_US.UTF-8", "_=/sbin/startproc", "PREVLEVEL=", "RUNLEVEL=5", "DAEMON=/usr/lib/git/git-daemon", "REMOTE_ADDR=[::1]", "REMOTE_PORT=44400"]) = -1 ENOENT (No such file or directory)
[pid 18830] execve("/bin/git-daemon", ["git-daemon", "--serve", "--syslog", "--detach", "--reuseaddr", "--user=git", "--group=daemon", "--pid-file=/var/run/git-daemon.p"..., "--export-all", "--user-path"], ["CONSOLE=/dev/console", "LC_ALL=POSIX", "REDIRECT=/dev/tty7", "COLUMNS=320", "PATH=/usr/local/sbin:/usr/local/"..., "PWD=/", "LINES=90", "SHLVL=1", "LC_CTYPE=en_US.UTF-8", "_=/sbin/startproc", "PREVLEVEL=", "RUNLEVEL=5", "DAEMON=/usr/lib/git/git-daemon", "REMOTE_ADDR=[::1]", "REMOTE_PORT=44400"]) = -1 ENOENT (No such file or directory)
[pid 18830] fstat(2, {st_dev=makedev(0, 6), st_ino=1029, st_mode=S_IFCHR|0666, st_nlink=1, st_uid=0, st_gid=0, st_blksize=4096, st_blocks=0, st_rdev=makedev(1, 3), st_atime=2016/11/26-00:42:47, st_mtime=2016/11/26-00:42:47, st_ctime=2016/11/26-00:42:47}) = 0
[pid 18830] ioctl(2, TCGETS, 0x7ffe6dbd09b0) = -1 ENOTTY (Inappropriate ioctl for device)
[pid 18830] mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f0e60632000
[pid 18830] write(2, "error: cannot run git-daemon: No"..., 56) = 56
[pid 18830] write(8, "\0", 1) = 1
[pid 18804] <... read resumed> "\0", 1) = 1
[pid 18830] exit_group(127) = ?
[pid 18804] wait4(18830, <unfinished ...>
[pid 18830] +++ exited with 127 +++
<... wait4 resumed> [{WIFEXITED(s) && WEXITSTATUS(s) == 127}], 0, NULL) = 18830
--- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=18830, si_uid=1002, si_status=127, si_utime=0, si_stime=0} ---
rt_sigaction(SIGCHLD, {0x404e10, [CHLD], SA_RESTORER|SA_RESTART, 0x7f0e5f6a7140}, {0x404e10, [CHLD], SA_RESTORER|SA_RESTART, 0x7f0e5f6a7140}, 8) = 0
rt_sigreturn({mask=[]}) = 18830
close(7) = 0
close(5) = 0
close(6) = 0
open("/etc/localtime", O_RDONLY|O_CLOEXEC) = 5
fstat(5, {st_dev=makedev(8, 5), st_ino=6031966, st_mode=S_IFREG|0644, st_nlink=2, st_uid=0, st_gid=0, st_blksize=4096, st_blocks=8, st_size=2335, st_atime=2016/11/26-01:00:02, st_mtime=2016/11/08-18:09:53, st_ctime=2016/11/09-21:15:24}) = 0
fstat(5, {st_dev=makedev(8, 5), st_ino=6031966, st_mode=S_IFREG|0644, st_nlink=2, st_uid=0, st_gid=0, st_blksize=4096, st_blocks=8, st_size=2335, st_atime=2016/11/26-01:00:02, st_mtime=2016/11/08-18:09:53, st_ctime=2016/11/09-21:15:24}) = 0
mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f0e60632000
read(5, "TZif2\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\t\0\0\0\t\0\0\0\0"..., 4096) = 2335
lseek(5, -1476, SEEK_CUR) = 859
read(5, "TZif2\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\t\0\0\0\t\0\0\0\0"..., 4096) = 1476
close(5) = 0
munmap(0x7f0e60632000, 4096) = 0
socket(PF_LOCAL, SOCK_DGRAM|SOCK_CLOEXEC, 0) = 5
connect(5, {sa_family=AF_LOCAL, sun_path="/dev/log"}, 110) = 0
sendto(5, "<27>Nov 26 14:25:55 git-daemon[1"..., 53, MSG_NOSIGNAL, NULL, 0) = 53
poll([{fd=3, events=POLLIN}, {fd=4, events=POLLIN}], 2, 4294967295
revert 650c449250d7
Process 18934 attached
restart_syscall(<... resuming interrupted call ...>) = 1
accept(4, {sa_family=AF_INET6, sin6_port=htons(44404), inet_pton(AF_INET6, "::1", &sin6_addr), sin6_flowinfo=0, sin6_scope_id=0}, [28]) = 5
dup(5) = 6
pipe([7, 8]) = 0
clone(Process 19010 attached
child_stack=0, flags=CLONE_CHILD_CLEARTID|CLONE_CHILD_SETTID|SIGCHLD, child_tidptr=0x7f62fcb5e9d0) = 19010
[pid 18934] close(8 <unfinished ...>
[pid 19010] set_robust_list(0x7f62fcb5e9e0, 24) = 0
[pid 18934] <... close resumed> ) = 0
[pid 18934] read(7, <unfinished ...>
[pid 19010] close(7) = 0
[pid 19010] fcntl(8, F_GETFD) = 0
[pid 19010] fcntl(8, F_SETFD, FD_CLOEXEC) = 0
[pid 19010] dup2(5, 0) = 0
[pid 19010] close(5) = 0
[pid 19010] dup2(6, 1) = 1
[pid 19010] close(6) = 0
[pid 19010] execve("/usr/lib/git/git-daemon", ["/usr/lib/git/git-daemon", "--serve", "--syslog", "--detach", "--reuseaddr", "--user=git", "--group=daemon", "--pid-file=/var/run/git-daemon.p"..., "--export-all", "--user-path"], ["CONSOLE=/dev/console", "LC_ALL=POSIX", "REDIRECT=/dev/tty7", "COLUMNS=320", "PATH=/usr/local/sbin:/usr/local/"..., "PWD=/", "LINES=90", "SHLVL=1", "LC_CTYPE=en_US.UTF-8", "_=/sbin/startproc", "PREVLEVEL=", "RUNLEVEL=5", "DAEMON=/usr/lib/git/git-daemon", "REMOTE_ADDR=[::1]", "REMOTE_PORT=44404"] <unfinished ...>
[pid 18934] <... read resumed> "", 1) = 0
[pid 19010] <... execve resumed> ) = 0
.... works
ln -s /usr/lib/git/git-daemon /usr/bin/git-daemon
Process 19862 attached
restart_syscall(<... resuming interrupted call ...>) = 1
accept(4, {sa_family=AF_INET6, sin6_port=htons(44412), inet_pton(AF_INET6, "::1", &sin6_addr), sin6_flowinfo=0, sin6_scope_id=0}, [28]) = 5
dup(5) = 6
pipe([7, 8]) = 0
clone(Process 19915 attached
child_stack=0, flags=CLONE_CHILD_CLEARTID|CLONE_CHILD_SETTID|SIGCHLD, child_tidptr=0x7fc97962e9d0) = 19915
[pid 19862] close(8) = 0
[pid 19915] set_robust_list(0x7fc97962e9e0, 24 <unfinished ...>
[pid 19862] read(7, <unfinished ...>
[pid 19915] <... set_robust_list resumed> ) = 0
[pid 19915] close(7) = 0
[pid 19915] fcntl(8, F_GETFD) = 0
[pid 19915] fcntl(8, F_SETFD, FD_CLOEXEC) = 0
[pid 19915] dup2(5, 0) = 0
[pid 19915] close(5) = 0
[pid 19915] dup2(6, 1) = 1
[pid 19915] close(6) = 0
[pid 19915] execve("/usr/local/sbin/git-daemon", ["git-daemon", "--serve", "--syslog", "--detach", "--reuseaddr", "--user=git", "--group=daemon", "--pid-file=/var/run/git-daemon.p"..., "--export-all", "--user-path"], ["CONSOLE=/dev/console", "LC_ALL=POSIX", "REDIRECT=/dev/tty7", "COLUMNS=320", "PATH=/usr/local/sbin:/usr/local/"..., "PWD=/", "LINES=90", "SHLVL=1", "LC_CTYPE=en_US.UTF-8", "_=/sbin/startproc", "PREVLEVEL=", "RUNLEVEL=5", "DAEMON=/usr/lib/git/git-daemon", "REMOTE_ADDR=[::1]", "REMOTE_PORT=44412"]) = -1 ENOENT (No such file or directory)
[pid 19915] execve("/usr/local/bin/git-daemon", ["git-daemon", "--serve", "--syslog", "--detach", "--reuseaddr", "--user=git", "--group=daemon", "--pid-file=/var/run/git-daemon.p"..., "--export-all", "--user-path"], ["CONSOLE=/dev/console", "LC_ALL=POSIX", "REDIRECT=/dev/tty7", "COLUMNS=320", "PATH=/usr/local/sbin:/usr/local/"..., "PWD=/", "LINES=90", "SHLVL=1", "LC_CTYPE=en_US.UTF-8", "_=/sbin/startproc", "PREVLEVEL=", "RUNLEVEL=5", "DAEMON=/usr/lib/git/git-daemon", "REMOTE_ADDR=[::1]", "REMOTE_PORT=44412"]) = -1 ENOENT (No such file or directory)
[pid 19915] execve("/usr/sbin/git-daemon", ["git-daemon", "--serve", "--syslog", "--detach", "--reuseaddr", "--user=git", "--group=daemon", "--pid-file=/var/run/git-daemon.p"..., "--export-all", "--user-path"], ["CONSOLE=/dev/console", "LC_ALL=POSIX", "REDIRECT=/dev/tty7", "COLUMNS=320", "PATH=/usr/local/sbin:/usr/local/"..., "PWD=/", "LINES=90", "SHLVL=1", "LC_CTYPE=en_US.UTF-8", "_=/sbin/startproc", "PREVLEVEL=", "RUNLEVEL=5", "DAEMON=/usr/lib/git/git-daemon", "REMOTE_ADDR=[::1]", "REMOTE_PORT=44412"]) = -1 ENOENT (No such file or directory)
[pid 19915] execve("/usr/bin/git-daemon", ["git-daemon", "--serve", "--syslog", "--detach", "--reuseaddr", "--user=git", "--group=daemon", "--pid-file=/var/run/git-daemon.p"..., "--export-all", "--user-path"], ["CONSOLE=/dev/console", "LC_ALL=POSIX", "REDIRECT=/dev/tty7", "COLUMNS=320", "PATH=/usr/local/sbin:/usr/local/"..., "PWD=/", "LINES=90", "SHLVL=1", "LC_CTYPE=en_US.UTF-8", "_=/sbin/startproc", "PREVLEVEL=", "RUNLEVEL=5", "DAEMON=/usr/lib/git/git-daemon", "REMOTE_ADDR=[::1]", "REMOTE_PORT=44412"] <unfinished ...>
[pid 19862] <... read resumed> "", 1) = 0
[pid 19915] <... execve resumed> ) = 0
.... works
^ permalink raw reply
* Re: [PATCH v3 1/2] difftool: add a skeleton for the upcoming builtin
From: Jeff King @ 2016-11-26 16:19 UTC (permalink / raw)
To: Johannes Schindelin
Cc: git, Junio C Hamano, David Aguilar, Dennis Kaarsemaker
In-Reply-To: <alpine.DEB.2.20.1611261320050.117539@virtualbox>
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"
then
GIT_CONFIG_PARAMETERS=...
fi
-Peff
^ permalink raw reply
* Re: git-daemon regression: 650c449250d7 common-main: call git_extract_argv0_path()
From: Dennis Kaarsemaker @ 2016-11-26 16:56 UTC (permalink / raw)
To: Mike Galbraith, Jeff King; +Cc: git
In-Reply-To: <1480169028.3830.24.camel@gmail.com>
Hi Mike,
On Sat, 2016-11-26 at 15:03 +0100, Mike Galbraith wrote:
> Greetings,
>
> git-daemon went broke on me post v2.9.3 due to binaries being installed
> in /usr/lib/git, which is not in PATH. Reverting 650c449250d7 fixes it
> up, as does ln -s /usr/lib/git/git-daemon /usr/bin/git-daemon 'course,
> but thought I should report it, since it used to work without that.
I don't know how you usually install git, but git-daemon is not
supposed to be in $PATH, the correct way to invoke the git daemon is
'git daemon' not 'git-daemon'
Having all subcommands of git as separate binaries in your $PATH is an
ancient git.git practice that stopped being used/supported quite a
while ago.
I don't know why this patch broke that obsolete practice, but hopefully
this can help you forward.
D.
^ permalink raw reply
* Re: git-daemon regression: 650c449250d7 common-main: call git_extract_argv0_path()
From: Jeff King @ 2016-11-26 17:09 UTC (permalink / raw)
To: Mike Galbraith; +Cc: git
In-Reply-To: <1480169028.3830.24.camel@gmail.com>
On Sat, Nov 26, 2016 at 03:03:48PM +0100, Mike Galbraith wrote:
> git-daemon went broke on me post v2.9.3 due to binaries being installed
> in /usr/lib/git, which is not in PATH. Reverting 650c449250d7 fixes it
> up, as does ln -s /usr/lib/git/git-daemon /usr/bin/git-daemon 'course,
> but thought I should report it, since it used to work without that.
Generally /usr/lib/git _should_ be in your PATH, as it is added by the
git wrapper when you run "git daemon".
The only behavior difference caused by 650c449250d7 is that we replace
argv[0] with the output of git_extract_argv0_path(argv[0]), which will
give the basename, not a full path. So presumably you are running:
/usr/lib/git/git-daemon
directly. I'm not sure that's even supposed to work these days, and it
was not just a happy accident that it did.
On the other hand, I am sympathetic that something used to work and now
doesn't. It probably wouldn't be that hard to work around it.
The reason for the behavior change is that one of the cmd_main()
functions was relying on the basename side-effect of the
extract_argv0_path function, so 650c449250d7 just feeds the munged
argv[0] to all of the programs. The cleanest fix would probably be
something like:
diff --git a/common-main.c b/common-main.c
index 44a29e8b1..c654f9555 100644
--- a/common-main.c
+++ b/common-main.c
@@ -33,7 +33,7 @@ int main(int argc, const char **argv)
git_setup_gettext();
- argv[0] = git_extract_argv0_path(argv[0]);
+ git_extract_argv0_path(argv[0]);
restore_sigpipe_to_default();
diff --git a/git.c b/git.c
index bd66a2e0a..05986680c 100644
--- a/git.c
+++ b/git.c
@@ -730,6 +730,11 @@ int cmd_main(int argc, const char **argv)
cmd = argv[0];
if (!cmd)
cmd = "git-help";
+ else {
+ const char *base = find_last_dir_sep(cmd);
+ if (base)
+ cmd = base + 1;
+ }
trace_command_performance(argv);
trace_stdin();
-Peff
^ permalink raw reply related
* Re: git-daemon regression: 650c449250d7 common-main: call git_extract_argv0_path()
From: Mike Galbraith @ 2016-11-26 17:51 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20161126170933.6tge6j5etuchqy33@sigill.intra.peff.net>
On Sat, 2016-11-26 at 12:09 -0500, Jeff King wrote:
> On Sat, Nov 26, 2016 at 03:03:48PM +0100, Mike Galbraith wrote:
>
> > git-daemon went broke on me post v2.9.3 due to binaries being installed
> > in /usr/lib/git, which is not in PATH. Reverting 650c449250d7 fixes it
> > up, as does ln -s /usr/lib/git/git-daemon /usr/bin/git-daemon 'course,
> > but thought I should report it, since it used to work without that.
>
> Generally /usr/lib/git _should_ be in your PATH, as it is added by the
> git wrapper when you run "git daemon".
>
> The only behavior difference caused by 650c449250d7 is that we replace
> argv[0] with the output of git_extract_argv0_path(argv[0]), which will
> give the basename, not a full path. So presumably you are running:
>
> /usr/lib/git/git-daemon
>
> directly. I'm not sure that's even supposed to work these days, and it
> was not just a happy accident that it did.
Ah. I'm using suse's rpm glue to package my modified source, and its
startup script still calls it directly, so wants some modernization.
> On the other hand, I am sympathetic that something used to work and now
> doesn't. It probably wouldn't be that hard to work around it.
>
> The reason for the behavior change is that one of the cmd_main()
> functions was relying on the basename side-effect of the
> extract_argv0_path function, so 650c449250d7 just feeds the munged
> argv[0] to all of the programs. The cleanest fix would probably be
> something like:
That did fix it up, thanks. I'll try twiddling the script instead.
> diff --git a/common-main.c b/common-main.c
> index 44a29e8b1..c654f9555 100644
> --- a/common-main.c
> +++ b/common-main.c
> @@ -33,7 +33,7 @@ int main(int argc, const char **argv)
>
> > > git_setup_gettext();
>
> -> > argv[0] = git_extract_argv0_path(argv[0]);
> +> > git_extract_argv0_path(argv[0]);
>
> > > restore_sigpipe_to_default();
>
> diff --git a/git.c b/git.c
> index bd66a2e0a..05986680c 100644
> --- a/git.c
> +++ b/git.c
> @@ -730,6 +730,11 @@ int cmd_main(int argc, const char **argv)
> > > cmd = argv[0];
> > > if (!cmd)
> > > > cmd = "git-help";
> +> > else {
> +> > > const char *base = find_last_dir_sep(cmd);
> +> > > if (base)
> +> > > > cmd = base + 1;
> +> > }
>
> > > trace_command_performance(argv);
> > > trace_stdin();
>
> -Peff
^ permalink raw reply
* trustExitCode doesn't apply to vimdiff mergetool
From: Dun Peal @ 2016-11-27 2:44 UTC (permalink / raw)
To: Git ML
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?
^ permalink raw reply
* [PATCH] common-main: stop munging argv[0] path
From: Jeff King @ 2016-11-27 4:31 UTC (permalink / raw)
To: Mike Galbraith; +Cc: git
In-Reply-To: <1480182671.3830.38.camel@gmail.com>
On Sat, Nov 26, 2016 at 06:51:11PM +0100, Mike Galbraith wrote:
> > On the other hand, I am sympathetic that something used to work and now
> > doesn't. It probably wouldn't be that hard to work around it.
> >
> > The reason for the behavior change is that one of the cmd_main()
> > functions was relying on the basename side-effect of the
> > extract_argv0_path function, so 650c449250d7 just feeds the munged
> > argv[0] to all of the programs. The cleanest fix would probably be
> > something like:
>
> That did fix it up, thanks. I'll try twiddling the script instead.
Thanks for confirming. Here it is, with a commit message and a little
bit of polish.
-- >8 --
Subject: [PATCH] common-main: stop munging argv[0] path
Since 650c44925 (common-main: call git_extract_argv0_path(),
2016-07-01), the argv[0] that is seen in cmd_main() of
individual programs is always the basename of the
executable, as common-main strips off the full path. This
can produce confusing results for git-daemon, which wants to
re-exec itself.
For instance, if the program was originally run as
"/usr/lib/git/git-daemon", it will try just re-execing
"git-daemon", which will find the first instance in $PATH.
If git's exec-path has not been prepended to $PATH, we may
find the git-daemon from a different version (or no
git-daemon at all).
Normally this isn't a problem. Git commands are run as "git
daemon", the git wrapper puts the exec-path at the front of
$PATH, and argv[0] is already "daemon" anyway. But running
git-daemon via its full exec-path, while not really a
recommended method, did work prior to 650c44925. Let's make
it work again.
The real goal of 650c44925 was not to munge argv[0], but to
reliably set the argv0_path global. The only reason it
munges at all is that one caller, the git.c wrapper,
piggy-backed on that computation to find the command
basename. Instead, let's leave argv[0] untouched in
common-main, and have git.c do its own basename computation.
While we're at it, let's drop the return value from
git_extract_argv0_path(). It was only ever used in this one
callsite, and its dual purposes is what led to this
confusion in the first place.
Note that by changing the interface, the compiler can
confirm for us that there are no other callers storing the
return value. But the compiler can't tell us whether any of
the cmd_main() functions (besides git.c) were relying on the
basename munging. However, we can observe that prior to
650c44925, no other cmd_main() functions did that munging,
and no new cmd_main() functions have been introduced since
then. So we can't be regressing any of those cases.
Signed-off-by: Jeff King <peff@peff.net>
---
common-main.c | 2 +-
exec_cmd.c | 10 +++-------
exec_cmd.h | 2 +-
git.c | 5 +++++
4 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/common-main.c b/common-main.c
index 44a29e8b1..c654f9555 100644
--- a/common-main.c
+++ b/common-main.c
@@ -33,7 +33,7 @@ int main(int argc, const char **argv)
git_setup_gettext();
- argv[0] = git_extract_argv0_path(argv[0]);
+ git_extract_argv0_path(argv[0]);
restore_sigpipe_to_default();
diff --git a/exec_cmd.c b/exec_cmd.c
index 9d5703a15..19ac2146d 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -38,21 +38,17 @@ char *system_path(const char *path)
return strbuf_detach(&d, NULL);
}
-const char *git_extract_argv0_path(const char *argv0)
+void git_extract_argv0_path(const char *argv0)
{
const char *slash;
if (!argv0 || !*argv0)
- return NULL;
+ return;
slash = find_last_dir_sep(argv0);
- if (slash) {
+ if (slash)
argv0_path = xstrndup(argv0, slash - argv0);
- return slash + 1;
- }
-
- return argv0;
}
void git_set_argv_exec_path(const char *exec_path)
diff --git a/exec_cmd.h b/exec_cmd.h
index 1f6b43378..ff0b48048 100644
--- a/exec_cmd.h
+++ b/exec_cmd.h
@@ -4,7 +4,7 @@
struct argv_array;
extern void git_set_argv_exec_path(const char *exec_path);
-extern const char *git_extract_argv0_path(const char *path);
+extern void git_extract_argv0_path(const char *path);
extern const char *git_exec_path(void);
extern void setup_path(void);
extern const char **prepare_git_cmd(struct argv_array *out, const char **argv);
diff --git a/git.c b/git.c
index e8b2baf2d..dce529fcb 100644
--- a/git.c
+++ b/git.c
@@ -654,6 +654,11 @@ int cmd_main(int argc, const char **argv)
cmd = argv[0];
if (!cmd)
cmd = "git-help";
+ else {
+ const char *slash = find_last_dir_sep(cmd);
+ if (slash)
+ cmd = slash + 1;
+ }
trace_command_performance(argv);
--
2.11.0.rc3.313.g1055eca
^ permalink raw reply related
* Re: trustExitCode doesn't apply to vimdiff mergetool
From: Jeff King @ 2016-11-27 5:08 UTC (permalink / raw)
To: Dun Peal; +Cc: Git ML
In-Reply-To: <CAD03jn5PAZcFeesaq2osjo7cYd1frWZeN0odNqTh+AMxSEmLgQ@mail.gmail.com>
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
^ permalink raw reply
* [PATCH] t7610: clean up foo.* tmpdir
From: Jeff King @ 2016-11-27 6:34 UTC (permalink / raw)
To: git; +Cc: Armin Kunaschik
[resend; the original subject with foo.XXXXXX was bounced by vger for
being too sexy]
-- >8 --
Subject: [PATCH] t7610: clean up foo.XXXXXX tmpdir
The lazy prereq for MKTEMP uses "mktemp -t" to see if
mergetool's internal mktemp call will be able to run. But
unlike the call inside mergetool, we do not ever bother to
clean up the result, and the /tmp of git developers will
slowly fill up with "foo.XXXXXX" directories as they run the
test suite over and over. Let's clean up the directory
after we've verified its creation.
Note that we don't use test_when_finished here, and instead
just make rmdir part of the &&-chain. We should only remove
something that we're confident we just created. A failure in
the middle of the chain either means there's nothing to
clean up, or we are very confused and should err on the side
of caution.
Signed-off-by: Jeff King <peff@peff.net>
---
This has been happening since c578a09bd (t7610: test for mktemp before
test execution, 2016-07-02). I have noticed the foo.* directories
building in /tmp, but I never bothered to track it down before. I just
assumed from the name it was one of my personal hacky scripts. :)
It does make me wonder if test-lib.sh ought to just set $TMPDIR inside
the trash directory so that any cruft we fail to cleanup is contained.
t/t7610-mergetool.sh | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 6d9f21511..63d36fb28 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -591,7 +591,8 @@ test_expect_success 'filenames seen by tools start with ./' '
test_lazy_prereq MKTEMP '
tempdir=$(mktemp -d -t foo.XXXXXX) &&
- test -d "$tempdir"
+ test -d "$tempdir" &&
+ rmdir "$tempdir"
'
test_expect_success MKTEMP 'temporary filenames are used with mergetool.writeToTemp' '
--
2.11.0.rc3.315.gde8259a
^ permalink raw reply related
* Re: [PATCH v3 1/2] difftool: add a skeleton for the upcoming builtin
From: Johannes Schindelin @ 2016-11-26 13:01 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, David Aguilar, Dennis Kaarsemaker
In-Reply-To: <20161126161907.xol62zytn2jb45gh@sigill.intra.peff.net>
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
^ permalink raw reply
* Re: [PATCH v3 2/2] difftool: implement the functionality in the builtin
From: Johannes Schindelin @ 2016-11-27 11:10 UTC (permalink / raw)
To: Jakub Narębski
Cc: git, Junio C Hamano, David Aguilar, Dennis Kaarsemaker
In-Reply-To: <2994b0d6-4b6c-84e7-d0d5-257bcae3be98@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3725 bytes --]
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 <johannes.schindelin@gmx.de>
> > ---
> > 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 [<options>] [<commit> [<commit>]] [--] [<path>...]"),
> > + 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
^ permalink raw reply
* Re: [PATCH v3 2/2] difftool: implement the functionality in the builtin
From: Jakub Narębski @ 2016-11-27 11:20 UTC (permalink / raw)
To: Johannes Schindelin
Cc: git, Junio C Hamano, David Aguilar, Dennis Kaarsemaker
In-Reply-To: <alpine.DEB.2.20.1611261407520.117539@virtualbox>
Hello Johannes,
On 27 November 2016 at 12:10, Johannes Schindelin
<Johannes.Schindelin@gmx.de> 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
^ permalink raw reply
* Re: trustExitCode doesn't apply to vimdiff mergetool
From: Dun Peal @ 2016-11-27 13:46 UTC (permalink / raw)
To: Jeff King; +Cc: Git ML
In-Reply-To: <20161127050818.rmjpvha64y4wosq2@sigill.intra.peff.net>
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 <peff@peff.net> 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
^ permalink raw reply
* [PATCH/RFC v1 1/1] New way to normalize the line endings
From: tboegi @ 2016-11-27 16:22 UTC (permalink / raw)
To: git; +Cc: Torsten Bögershausen
In-Reply-To: <5502e894-bb22-e8b9-ab7a-49346d238283@web.de>
From: Torsten Bögershausen <tboegi@web.de>
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 <tboegi@web.de>
---
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 <<EOF &&
+i/-text w/-text attr/text=auto LFwithNUL
+i/lf w/crlf attr/text=auto CRLFonly
+i/lf w/crlf attr/text=auto LFonly
+i/lf w/lf attr/text=auto .gitattributes
+i/lf w/mixed attr/text=auto mixed
+EOF
+ test_cmp exp act
+
+'
+
test_done
--
2.10.0
^ permalink raw reply related
* Re: [PATCH v3 1/2] difftool: add a skeleton for the upcoming builtin
From: Jeff King @ 2016-11-27 16:50 UTC (permalink / raw)
To: Johannes Schindelin
Cc: git, Junio C Hamano, David Aguilar, Dennis Kaarsemaker
In-Reply-To: <alpine.DEB.2.20.1611261400300.117539@virtualbox>
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
^ permalink raw reply
* Re: trustExitCode doesn't apply to vimdiff mergetool
From: Jeff King @ 2016-11-27 16:55 UTC (permalink / raw)
To: Dun Peal; +Cc: Git ML
In-Reply-To: <CAD03jn7gU9g7NyDk_3wYTKsYQUtRGg6msvumZqUDs44hMOVX7w@mail.gmail.com>
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
^ permalink raw reply
* [PATCH] contrib/subtree: added --no-show-signature to git log invocation
From: Kieran Colford @ 2016-11-27 21:21 UTC (permalink / raw)
To: git; +Cc: 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 <kieran@kcolford.com>
---
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
^ permalink raw reply related
* Re: trustExitCode doesn't apply to vimdiff mergetool
From: David Aguilar @ 2016-11-28 1:45 UTC (permalink / raw)
To: Jeff King; +Cc: Dun Peal, Git ML
In-Reply-To: <20161127165559.j5okxyztwescheug@sigill.intra.peff.net>
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.<tool>.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 <dunpealer@gmail.com>
Signed-off-by: David Aguilar <davvid@gmail.com>
---
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 64f97c5e9..e2407b65b 100644
--- a/mergetools/araxis
+++ b/mergetools/araxis
@@ -3,7 +3,6 @@ diff_cmd () {
}
merge_cmd () {
- touch "$BACKUP"
if $base_present
then
"$merge_tool_path" -wait -merge -3 -a1 \
@@ -12,7 +11,6 @@ merge_cmd () {
"$merge_tool_path" -wait -2 \
"$LOCAL" "$REMOTE" "$MERGED" >/dev/null 2>&1
fi
- check_unchanged
}
translate_merge_tool_path() {
diff --git a/mergetools/bc b/mergetools/bc
index b6319d206..3a69e60fa 100644
--- a/mergetools/bc
+++ b/mergetools/bc
@@ -3,7 +3,6 @@ diff_cmd () {
}
merge_cmd () {
- touch "$BACKUP"
if $base_present
then
"$merge_tool_path" "$LOCAL" "$REMOTE" "$BASE" \
@@ -12,7 +11,6 @@ merge_cmd () {
"$merge_tool_path" "$LOCAL" "$REMOTE" \
-mergeoutput="$MERGED"
fi
- check_unchanged
}
translate_merge_tool_path() {
diff --git a/mergetools/codecompare b/mergetools/codecompare
index 3f0486bc8..9f60e8da6 100644
--- a/mergetools/codecompare
+++ b/mergetools/codecompare
@@ -3,7 +3,6 @@ diff_cmd () {
}
merge_cmd () {
- touch "$BACKUP"
if $base_present
then
"$merge_tool_path" -MF="$LOCAL" -TF="$REMOTE" -BF="$BASE" \
@@ -12,7 +11,6 @@ merge_cmd () {
"$merge_tool_path" -MF="$LOCAL" -TF="$REMOTE" \
-RF="$MERGED"
fi
- check_unchanged
}
translate_merge_tool_path() {
diff --git a/mergetools/diffuse b/mergetools/diffuse
index 02e0843f4..5a3ae8b56 100644
--- a/mergetools/diffuse
+++ b/mergetools/diffuse
@@ -3,7 +3,6 @@ diff_cmd () {
}
merge_cmd () {
- touch "$BACKUP"
if $base_present
then
"$merge_tool_path" \
@@ -13,5 +12,4 @@ merge_cmd () {
"$merge_tool_path" \
"$LOCAL" "$MERGED" "$REMOTE" | cat
fi
- check_unchanged
}
diff --git a/mergetools/ecmerge b/mergetools/ecmerge
index 13c2e439d..6c5101c4f 100644
--- a/mergetools/ecmerge
+++ b/mergetools/ecmerge
@@ -3,7 +3,6 @@ diff_cmd () {
}
merge_cmd () {
- touch "$BACKUP"
if $base_present
then
"$merge_tool_path" "$BASE" "$LOCAL" "$REMOTE" \
@@ -12,5 +11,4 @@ merge_cmd () {
"$merge_tool_path" "$LOCAL" "$REMOTE" \
--default --mode=merge2 --to="$MERGED"
fi
- check_unchanged
}
diff --git a/mergetools/examdiff b/mergetools/examdiff
index 7b524d408..e72b06fc4 100644
--- a/mergetools/examdiff
+++ b/mergetools/examdiff
@@ -3,14 +3,12 @@ diff_cmd () {
}
merge_cmd () {
- touch "$BACKUP"
if $base_present
then
"$merge_tool_path" -merge "$LOCAL" "$BASE" "$REMOTE" -o:"$MERGED" -nh
else
"$merge_tool_path" -merge "$LOCAL" "$REMOTE" -o:"$MERGED" -nh
fi
- check_unchanged
}
translate_merge_tool_path() {
diff --git a/mergetools/meld b/mergetools/meld
index 83ebdfb4c..bc178e888 100644
--- a/mergetools/meld
+++ b/mergetools/meld
@@ -7,7 +7,7 @@ merge_cmd () {
then
check_meld_for_output_version
fi
- touch "$BACKUP"
+
if test "$meld_has_output_option" = true
then
"$merge_tool_path" --output "$MERGED" \
@@ -15,7 +15,6 @@ merge_cmd () {
else
"$merge_tool_path" "$LOCAL" "$MERGED" "$REMOTE"
fi
- check_unchanged
}
# Check whether we should use 'meld --output <file>'
diff --git a/mergetools/opendiff b/mergetools/opendiff
index 0942b2a73..b608dd6de 100644
--- a/mergetools/opendiff
+++ b/mergetools/opendiff
@@ -3,7 +3,6 @@ diff_cmd () {
}
merge_cmd () {
- touch "$BACKUP"
if $base_present
then
"$merge_tool_path" "$LOCAL" "$REMOTE" \
@@ -12,5 +11,4 @@ merge_cmd () {
"$merge_tool_path" "$LOCAL" "$REMOTE" \
-merge "$MERGED" | cat
fi
- check_unchanged
}
diff --git a/mergetools/p4merge b/mergetools/p4merge
index 5a608abf9..7a5b291dd 100644
--- a/mergetools/p4merge
+++ b/mergetools/p4merge
@@ -20,14 +20,12 @@ diff_cmd () {
}
merge_cmd () {
- touch "$BACKUP"
if ! $base_present
then
cp -- "$LOCAL" "$BASE"
create_virtual_base "$BASE" "$REMOTE"
fi
"$merge_tool_path" "$BASE" "$REMOTE" "$LOCAL" "$MERGED"
- check_unchanged
}
create_empty_file () {
diff --git a/mergetools/tortoisemerge b/mergetools/tortoisemerge
index 3b89f1c82..d7ab666a5 100644
--- a/mergetools/tortoisemerge
+++ b/mergetools/tortoisemerge
@@ -5,7 +5,6 @@ can_diff () {
merge_cmd () {
if $base_present
then
- touch "$BACKUP"
basename="$(basename "$merge_tool_path" .exe)"
if test "$basename" = "tortoisegitmerge"
then
@@ -17,7 +16,6 @@ merge_cmd () {
-base:"$BASE" -mine:"$LOCAL" \
-theirs:"$REMOTE" -merged:"$MERGED"
fi
- check_unchanged
else
echo "$merge_tool_path cannot be used without a base" 1>&2
return 1
diff --git a/mergetools/vimdiff b/mergetools/vimdiff
index 74ea6d547..a841ffdb4 100644
--- a/mergetools/vimdiff
+++ b/mergetools/vimdiff
@@ -4,7 +4,6 @@ diff_cmd () {
}
merge_cmd () {
- touch "$BACKUP"
case "$1" in
gvimdiff|vimdiff)
if $base_present
@@ -31,7 +30,6 @@ merge_cmd () {
fi
;;
esac
- check_unchanged
}
translate_merge_tool_path() {
diff --git a/mergetools/winmerge b/mergetools/winmerge
index f3819d316..74d03259f 100644
--- a/mergetools/winmerge
+++ b/mergetools/winmerge
@@ -6,10 +6,8 @@ diff_cmd () {
merge_cmd () {
# mergetool.winmerge.trustExitCode is implicitly false.
# touch $BACKUP so that we can check_unchanged.
- touch "$BACKUP"
"$merge_tool_path" -u -e -dl Local -dr Remote \
"$LOCAL" "$REMOTE" "$MERGED"
- check_unchanged
}
translate_merge_tool_path() {
diff --git a/mergetools/xxdiff b/mergetools/xxdiff
index 05b443394..e284811ff 100644
--- a/mergetools/xxdiff
+++ b/mergetools/xxdiff
@@ -6,7 +6,6 @@ diff_cmd () {
}
merge_cmd () {
- touch "$BACKUP"
if $base_present
then
"$merge_tool_path" -X --show-merged-pane \
@@ -21,5 +20,4 @@ merge_cmd () {
-R 'Accel.SearchForward: "Ctrl-G"' \
--merged-file "$MERGED" "$LOCAL" "$REMOTE"
fi
- check_unchanged
}
--
2.11.0.rc3.dirty
^ permalink raw reply related
* Re: trustExitCode doesn't apply to vimdiff mergetool
From: Jeff King @ 2016-11-28 2:01 UTC (permalink / raw)
To: David Aguilar; +Cc: Dun Peal, Git ML
In-Reply-To: <20161128014538.GA18691@gmail.com>
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
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox