[PATCH] t7610: clean up foo.* tmpdir
[resend; the original subject with foo.XX was bounced by vger for being too sexy] -- >8 -- Subject: [PATCH] t7610: clean up foo.XX 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.XX" 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 --- 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.XX) && - 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
Re: trustExitCode doesn't apply to vimdiff mergetool
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
[PATCH] common-main: stop munging argv[0] path
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
---
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
trustExitCode doesn't apply to vimdiff mergetool
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?
Re: git-daemon regression: 650c449250d7 common-main: call git_extract_argv0_path()
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
Re: git-daemon regression: 650c449250d7 common-main: call git_extract_argv0_path()
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
Re: git-daemon regression: 650c449250d7 common-main: call git_extract_argv0_path()
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.
Re: [PATCH v3 1/2] difftool: add a skeleton for the upcoming builtin
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
git-daemon regression: 650c449250d7 common-main: call git_extract_argv0_path()
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
[pid 18804] close(8
[pid 18830] <... set_robust_list resumed> ) = 0
[pid 18804] <... close resumed> ) = 0
[pid 18804] read(7,
[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_A
RE: [char-misc-next] mei: request async autosuspend at the end of enumeration
> > W dniu 25.11.2016 o 04:14, Jeff King pisze: > > On Thu, Nov 24, 2016 at 10:37:14PM +, Winkler, Tomas wrote: > > > > Cc: # 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 : > 'sta...@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 > > sta...@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 # comment > > to the following address: > > John Doe (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
Re: [PATCH v2 2/2] Avoid a segmentation fault with renaming merges
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
[PATCH v2 2/2] Avoid a segmentation fault with renaming merges
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
---
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
[PATCH v2 1/2] cherry-pick: demonstrate a segmentation fault
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
---
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
[PATCH v2 0/2] Fix segmentation fault with cherry-pick
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
Re: [PATCH 1/2] cherry-pick: demonstrate a segmentation fault
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
Re: [PATCH v3 1/2] difftool: add a skeleton for the upcoming builtin
Hi Peff,
On Fri, 25 Nov 2016, Jeff King wrote:
> On Fri, Nov 25, 2016 at 06:41:23PM +0100, Johannes Schindelin wrote:
>
> > > Ah, I didn't realize that was a requirement. If this is going to be part
> > > of a release and real end-users are going to see it, that does make me
> > > think the config option is the better path (than the presence of some
> > > file), as it's our standard way of tweaking run-time behavior.
> >
> > So how do you easily switch back and forth between testing the old vs the
> > new difftool via the test suite?
>
> If it's for a specific tool, I'd consider teaching the test suite to run
> the whole script twice: once with the flag set and once without.
>
> That is sometimes more complicated, though, if the script creates many
> sub-repos. An environment variable might be more natural. If you already
> support flipping the default via config, you can probably do:
>
> GIT_CONFIG_PARAMETERS="'difftool.usebuiltin=true'"
> export GIT_CONFIG_PARAMETERS
Except that that does not work, of course. To figure out why, apply this
diff:
-- snip --
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 17f3008277..27159f65f3 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -10,6 +10,9 @@ Testing basic diff tool invocation
. ./test-lib.sh
+echo "config $(git config difftool.usebuiltin)." >&2
+exit 1
+
difftool_test_setup ()
{
test_config diff.tool test-tool &&
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 9980a46133..0ddeded92b 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -86,6 +86,7 @@ EDITOR=:
# /usr/xpg4/bin/sh and /bin/ksh to bail out. So keep the unsets
# deriving from the command substitution clustered with the other
# ones.
+echo "before $(git config difftool.usebuiltin)." >&2
unset VISUAL EMAIL LANGUAGE COLUMNS $("$PERL_PATH" -e '
my @env = keys %ENV;
my $ok = join("|", qw(
@@ -104,6 +105,7 @@ unset VISUAL EMAIL LANGUAGE COLUMNS $("$PERL_PATH" -e
'
my @vars = grep(/^GIT_/ && !/^GIT_($ok)/o, @env);
print join("\n", @vars);
')
+echo "after $(git config difftool.usebuiltin)." >&2
unset XDG_CONFIG_HOME
unset GITPERLLIB
GIT_AUTHOR_EMAIL=aut...@example.com
-- snap --
and then weep at this output:
GIT_CONFIG_PARAMETERS="'difftool.usebuiltin=true'"; export
GIT_CONFIG_PARAMETERS; bash t7800-difftool.sh -i -v -x
before true.
after .
Initialized empty Git repository in
/home/virtualbox/git/git-for-windows/t/trash
directory.t7800-difftool/.git/
config .
FATAL: Unexpected exit with code 1
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).
Ciao,
Dscho
Re: What's cooking in git.git (Nov 2016, #05; Wed, 23)
On Thu, Nov 24, 2016 at 6:21 AM, Junio C Hamano wrote: > * nd/rebase-forget (2016-10-28) 1 commit > - rebase: add --forget to cleanup rebase, leave HEAD untouched > > "git rebase" learned "--forget" option, which allows a user to > remove the metadata left by an earlier "git rebase" that was > manually aborted without using "git rebase --abort". > > Waiting for a reroll. The reroll was http://public-inbox.org/git/%3c20161112020041.2335-1-pclo...@gmail.com%3E/ -- Duy
