* Re: builtin difftool parsing issue
From: Johannes Schindelin @ 2017-01-02 16:12 UTC (permalink / raw)
To: Paul Sbarra; +Cc: davvid, dennis, git, gitster
In-Reply-To: <CAGf+dShpkPvsC8wQN6mWmYeMZ3=i-ZOzDNSM1aa0rinKW6+-+g@mail.gmail.com>
Hi Paul,
On Wed, 21 Dec 2016, Paul Sbarra wrote:
> Sadly, I haven't been able to figure out how to get the mbox file from
> this tread into gmail, but wanted to report a parsing issue I've found
> with the builtin difftool.
>
> Original Patch:
> https://public-inbox.org/git/ac91e4818cfb5c5af6b5874662dbeb61cde1f69d.1480019834.git.johannes.schindelin@gmx.de/#t
>
> > + *status = *++p;
> > + if (!status || p[1])
> > + return error("unexpected trailer: '%s'", p);
> > + return 0;
>
> The p[1] null check assumes the status is only one character long, but
> git-diff's raw output format shows that a numeric value can follow in
> the copy-edit and rename-edit cases.
Thank you for the report! I fixed it locally.
> I'm looking forward to seeing the builtin difftool land. I came across it
> while investigating adding --submodule=diff (expanding on diff's
> recent addition) support and this looks more promising then the perl
> script. Hopefully I will make some progress. Any tips/pointers would
> be greatly appreciated.
I would have expected `git difftool --submodule=diff ...` to work... What
are the problems?
Ciao,
Johannes
^ permalink raw reply
* [PATCH v4 0/4] Show Git Mailing List: a builtin difftool
From: Johannes Schindelin @ 2017-01-02 16:16 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, David Aguilar, Dennis Kaarsemaker, Paul Sbarra
In-Reply-To: <cover.1480019834.git.johannes.schindelin@gmx.de>
I have been working on the builtin difftool for a while now, for two
reasons:
1. Perl is really not native on Windows. Not only is there a performance
penalty to be paid just for running Perl scripts, we also have to deal
with the fact that users may have different Perl installations, with
different options, and some other Perl installation may decide to set
PERL5LIB globally, wreaking havoc with Git for Windows' Perl (which we
have to use because almost all other Perl distributions lack the
Subversion bindings we need for `git svn`).
2. Perl makes for a rather large reason that Git for Windows' installer
weighs in with >30MB. While one Perl script less does not relieve us
of that burden, it is one step in the right direction.
Changes since v3:
- made path_entry_cmp static
- fixed a few bugs identified by Coverity
- fixed overzealous status parsing that did not expect any number after
C or R
Johannes Schindelin (4):
Avoid Coverity warning about unfree()d git_exec_path()
difftool: add a skeleton for the upcoming builtin
difftool: implement the functionality in the builtin
t7800: run both builtin and scripted difftool, for now
.gitignore | 1 +
Makefile | 3 +-
builtin.h | 1 +
builtin/difftool.c | 733 ++++++++++++++++++++++++++
exec_cmd.c | 5 +-
git-difftool.perl => git-legacy-difftool.perl | 0
git.c | 6 +
t/t7800-difftool.sh | 29 +
8 files changed, 776 insertions(+), 2 deletions(-)
create mode 100644 builtin/difftool.c
rename git-difftool.perl => git-legacy-difftool.perl (100%)
base-commit: e05806da9ec4aff8adfed142ab2a2b3b02e33c8c
Published-As: https://github.com/dscho/git/releases/tag/builtin-difftool-v4
Fetch-It-Via: git fetch https://github.com/dscho/git builtin-difftool-v4
Interdiff vs v3:
diff --git a/builtin/difftool.c b/builtin/difftool.c
index 3480920fea..2115e548a5 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -73,8 +73,10 @@ static int parse_index_info(char *p, int *mode1, int *mode2,
if (*p != ' ')
return error("expected ' ', got '%c'", *p);
*status = *++p;
- if (!status || p[1])
- return error("unexpected trailer: '%s'", p);
+ if (!*status)
+ return error("missing status");
+ if (p[1] && !isdigit(p[1]))
+ return error("unexpected trailer: '%s'", p + 1);
return 0;
}
@@ -107,7 +109,8 @@ static int use_wt_file(const char *workdir, const char *name,
struct object_id wt_oid;
int fd = open(buf.buf, O_RDONLY);
- if (!index_fd(wt_oid.hash, fd, &st, OBJ_BLOB, name, 0)) {
+ if (fd >= 0 &&
+ !index_fd(wt_oid.hash, fd, &st, OBJ_BLOB, name, 0)) {
if (is_null_oid(oid)) {
oidcpy(oid, &wt_oid);
use = 1;
@@ -162,7 +165,7 @@ static void add_left_or_right(struct hashmap *map, const char *path,
e->left[0] = e->right[0] = '\0';
hashmap_add(map, e);
}
- strcpy(is_right ? e->right : e->left, content);
+ strlcpy(is_right ? e->right : e->left, content, PATH_MAX);
}
struct path_entry {
@@ -170,7 +173,7 @@ struct path_entry {
char path[FLEX_ARRAY];
};
-int path_entry_cmp(struct path_entry *a, struct path_entry *b, void *key)
+static int path_entry_cmp(struct path_entry *a, struct path_entry *b, void *key)
{
return strcmp(a->path, key ? key : b->path);
}
@@ -423,17 +426,16 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
struct cache_entry *ce2 =
make_cache_entry(rmode, roid.hash,
dst_path, 0, 0);
- ce_mode_from_stat(ce2, rmode);
add_index_entry(&wtindex, ce2,
ADD_CACHE_JUST_APPEND);
- add_path(&wtdir, wtdir_len, dst_path);
add_path(&rdir, rdir_len, dst_path);
if (ensure_leading_directories(rdir.buf))
return error("could not create "
"directory for '%s'",
dst_path);
+ add_path(&wtdir, wtdir_len, dst_path);
if (symlinks) {
if (symlink(wtdir.buf, rdir.buf)) {
ret = error_errno("could not symlink '%s' to '%s'", wtdir.buf, rdir.buf);
diff --git a/exec_cmd.c b/exec_cmd.c
index 19ac2146d0..587bd7eb48 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -65,6 +65,7 @@ void git_set_argv_exec_path(const char *exec_path)
const char *git_exec_path(void)
{
const char *env;
+ static char *system_exec_path;
if (argv_exec_path)
return argv_exec_path;
@@ -74,7 +75,9 @@ const char *git_exec_path(void)
return env;
}
- return system_path(GIT_EXEC_PATH);
+ if (!system_exec_path)
+ system_exec_path = system_path(GIT_EXEC_PATH);
+ return system_exec_path;
}
static void add_path(struct strbuf *out, const char *path)
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index e94910c563..273ab55723 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -23,6 +23,20 @@ prompt_given ()
test "$prompt" = "Launch 'test-tool' [Y/n]? branch"
}
+for use_builtin_difftool in false true
+do
+
+test_expect_success 'verify we are running the correct difftool' '
+ if test true = '$use_builtin_difftool'
+ then
+ test_must_fail ok=129 git difftool -h >help &&
+ grep "g, --gui" help
+ else
+ git difftool -h >help &&
+ grep "g|--gui" help
+ fi
+'
+
# NEEDSWORK: lose all the PERL prereqs once legacy-difftool is retired.
# Create a file on master and change it on branch
@@ -606,4 +620,17 @@ test_expect_success PERL,SYMLINKS 'difftool --dir-diff symlinked directories' '
)
'
+test true != $use_builtin_difftool || break
+
+test_expect_success 'tear down for re-run' '
+ rm -rf * .[a-z]* &&
+ git init
+'
+
+# run as builtin difftool now
+GIT_CONFIG_PARAMETERS="'difftool.usebuiltin=true'"
+export GIT_CONFIG_PARAMETERS
+
+done
+
test_done
--
2.11.0.rc3.windows.1
^ permalink raw reply
* [PATCH v4 2/4] difftool: add a skeleton for the upcoming builtin
From: Johannes Schindelin @ 2017-01-02 16:22 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, David Aguilar, Dennis Kaarsemaker, Paul Sbarra
In-Reply-To: <cover.1483373635.git.johannes.schindelin@gmx.de>
This adds a builtin difftool that still falls back to the legacy Perl
version, which has been renamed to `legacy-difftool`.
The idea is that the new, experimental, builtin difftool immediately hands
off to the legacy difftool for now, unless the config variable
difftool.useBuiltin is set to true.
This feature flag will be used in the upcoming Git for Windows v2.11.0
release, to allow early testers to opt-in to use the builtin difftool and
flesh out any bugs.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
.gitignore | 1 +
Makefile | 3 +-
builtin.h | 1 +
builtin/difftool.c | 63 +++++++++++++++++++++++++++
git-difftool.perl => git-legacy-difftool.perl | 0
git.c | 6 +++
t/t7800-difftool.sh | 2 +
7 files changed, 75 insertions(+), 1 deletion(-)
create mode 100644 builtin/difftool.c
rename git-difftool.perl => git-legacy-difftool.perl (100%)
diff --git a/.gitignore b/.gitignore
index 6722f78f9a..5555ae025b 100644
--- a/.gitignore
+++ b/.gitignore
@@ -76,6 +76,7 @@
/git-init-db
/git-interpret-trailers
/git-instaweb
+/git-legacy-difftool
/git-log
/git-ls-files
/git-ls-remote
diff --git a/Makefile b/Makefile
index d861bd9985..8cf5bef034 100644
--- a/Makefile
+++ b/Makefile
@@ -522,7 +522,7 @@ SCRIPT_LIB += git-sh-setup
SCRIPT_LIB += git-sh-i18n
SCRIPT_PERL += git-add--interactive.perl
-SCRIPT_PERL += git-difftool.perl
+SCRIPT_PERL += git-legacy-difftool.perl
SCRIPT_PERL += git-archimport.perl
SCRIPT_PERL += git-cvsexportcommit.perl
SCRIPT_PERL += git-cvsimport.perl
@@ -883,6 +883,7 @@ BUILTIN_OBJS += builtin/diff-files.o
BUILTIN_OBJS += builtin/diff-index.o
BUILTIN_OBJS += builtin/diff-tree.o
BUILTIN_OBJS += builtin/diff.o
+BUILTIN_OBJS += builtin/difftool.o
BUILTIN_OBJS += builtin/fast-export.o
BUILTIN_OBJS += builtin/fetch-pack.o
BUILTIN_OBJS += builtin/fetch.o
diff --git a/builtin.h b/builtin.h
index b9122bc5f4..67f80519da 100644
--- a/builtin.h
+++ b/builtin.h
@@ -60,6 +60,7 @@ extern int cmd_diff_files(int argc, const char **argv, const char *prefix);
extern int cmd_diff_index(int argc, const char **argv, const char *prefix);
extern int cmd_diff(int argc, const char **argv, const char *prefix);
extern int cmd_diff_tree(int argc, const char **argv, const char *prefix);
+extern int cmd_difftool(int argc, const char **argv, const char *prefix);
extern int cmd_fast_export(int argc, const char **argv, const char *prefix);
extern int cmd_fetch(int argc, const char **argv, const char *prefix);
extern int cmd_fetch_pack(int argc, const char **argv, const char *prefix);
diff --git a/builtin/difftool.c b/builtin/difftool.c
new file mode 100644
index 0000000000..53870bbaf7
--- /dev/null
+++ b/builtin/difftool.c
@@ -0,0 +1,63 @@
+/*
+ * "git difftool" builtin command
+ *
+ * This is a wrapper around the GIT_EXTERNAL_DIFF-compatible
+ * git-difftool--helper script.
+ *
+ * This script exports GIT_EXTERNAL_DIFF and GIT_PAGER for use by git.
+ * The GIT_DIFF* variables are exported for use by git-difftool--helper.
+ *
+ * Any arguments that are unknown to this script are forwarded to 'git diff'.
+ *
+ * Copyright (C) 2016 Johannes Schindelin
+ */
+#include "builtin.h"
+#include "run-command.h"
+#include "exec_cmd.h"
+
+/*
+ * NEEDSWORK: this function can go once the legacy-difftool Perl script is
+ * retired.
+ *
+ * We intentionally avoid reading the config directly here, to avoid messing up
+ * the GIT_* environment variables when we need to fall back to exec()ing the
+ * Perl script.
+ */
+static int use_builtin_difftool(void) {
+ struct child_process cp = CHILD_PROCESS_INIT;
+ struct strbuf out = STRBUF_INIT;
+ int ret;
+
+ argv_array_pushl(&cp.args,
+ "config", "--bool", "difftool.usebuiltin", NULL);
+ cp.git_cmd = 1;
+ if (capture_command(&cp, &out, 6))
+ return 0;
+ strbuf_trim(&out);
+ ret = !strcmp("true", out.buf);
+ strbuf_release(&out);
+ return ret;
+}
+
+int cmd_difftool(int argc, const char **argv, const char *prefix)
+{
+ /*
+ * NEEDSWORK: Once the builtin difftool has been tested enough
+ * and git-legacy-difftool.perl is retired to contrib/, this preamble
+ * can be removed.
+ */
+ if (!use_builtin_difftool()) {
+ const char *path = mkpath("%s/git-legacy-difftool",
+ git_exec_path());
+
+ if (sane_execvp(path, (char **)argv) < 0)
+ die_errno("could not exec %s", path);
+
+ return 0;
+ }
+ prefix = setup_git_directory();
+ trace_repo_setup(prefix);
+ setup_work_tree();
+
+ die("TODO");
+}
diff --git a/git-difftool.perl b/git-legacy-difftool.perl
similarity index 100%
rename from git-difftool.perl
rename to git-legacy-difftool.perl
diff --git a/git.c b/git.c
index dce529fcbf..044958a780 100644
--- a/git.c
+++ b/git.c
@@ -424,6 +424,12 @@ static struct cmd_struct commands[] = {
{ "diff-files", cmd_diff_files, RUN_SETUP | NEED_WORK_TREE },
{ "diff-index", cmd_diff_index, RUN_SETUP },
{ "diff-tree", cmd_diff_tree, RUN_SETUP },
+ /*
+ * NEEDSWORK: Once the redirection to git-legacy-difftool.perl in
+ * builtin/difftool.c has been removed, this entry should be changed to
+ * RUN_SETUP | NEED_WORK_TREE
+ */
+ { "difftool", cmd_difftool },
{ "fast-export", cmd_fast_export, RUN_SETUP },
{ "fetch", cmd_fetch, RUN_SETUP },
{ "fetch-pack", cmd_fetch_pack, RUN_SETUP },
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 99d4123461..e94910c563 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -23,6 +23,8 @@ prompt_given ()
test "$prompt" = "Launch 'test-tool' [Y/n]? branch"
}
+# NEEDSWORK: lose all the PERL prereqs once legacy-difftool is retired.
+
# Create a file on master and change it on branch
test_expect_success PERL 'setup' '
echo master >file &&
--
2.11.0.rc3.windows.1
^ permalink raw reply related
* [PATCH v4 3/4] difftool: implement the functionality in the builtin
From: Johannes Schindelin @ 2017-01-02 16:22 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, David Aguilar, Dennis Kaarsemaker, Paul Sbarra
In-Reply-To: <cover.1483373635.git.johannes.schindelin@gmx.de>
This patch gives life to the skeleton added in the previous patch.
The motivation for converting the difftool is that Perl scripts are not at
all native on Windows, and that `git difftool` therefore is pretty slow on
that platform, when there is no good reason for it to be slow.
In addition, Perl does not really have access to Git's internals. That
means that any script will always have to jump through unnecessary
hoops.
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.
Note: to play it safe, the original difftool is still called unless the
config setting difftool.useBuiltin is set to true.
The reason: this new, experimental, builtin difftool will be shipped as
part of Git for Windows v2.11.0, to allow for easier large-scale
testing, but of course as an opt-in feature.
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.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/difftool.c | 672 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 671 insertions(+), 1 deletion(-)
diff --git a/builtin/difftool.c b/builtin/difftool.c
index 53870bbaf7..2115e548a5 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -11,9 +11,610 @@
*
* 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")) {
+ diff_gui_tool = xstrdup(value);
+ return 0;
+ }
+
+ if (!strcmp(var, "difftool.trustexitcode")) {
+ trust_exit_code = git_config_bool(var, value);
+ return 0;
+ }
+
+ return git_default_config(var, value, cb);
+}
+
+static int print_tool_help(void)
+{
+ const char *argv[] = { "mergetool", "--tool-help=diff", NULL };
+ return run_command_v_opt(argv, RUN_GIT_CMD);
+}
+
+static int parse_index_info(char *p, int *mode1, int *mode2,
+ struct object_id *oid1, struct object_id *oid2,
+ char *status)
+{
+ if (*p != ':')
+ return error("expected ':', got '%c'", *p);
+ *mode1 = (int)strtol(p + 1, &p, 8);
+ if (*p != ' ')
+ return error("expected ' ', got '%c'", *p);
+ *mode2 = (int)strtol(p + 1, &p, 8);
+ if (*p != ' ')
+ return error("expected ' ', got '%c'", *p);
+ if (get_oid_hex(++p, oid1))
+ return error("expected object ID, got '%s'", p + 1);
+ p += GIT_SHA1_HEXSZ;
+ if (*p != ' ')
+ return error("expected ' ', got '%c'", *p);
+ if (get_oid_hex(++p, oid2))
+ return error("expected object ID, got '%s'", p + 1);
+ p += GIT_SHA1_HEXSZ;
+ if (*p != ' ')
+ return error("expected ' ', got '%c'", *p);
+ *status = *++p;
+ if (!*status)
+ return error("missing status");
+ if (p[1] && !isdigit(p[1]))
+ return error("unexpected trailer: '%s'", p + 1);
+ return 0;
+}
+
+/*
+ * Remove any trailing slash from $workdir
+ * before starting to avoid double slashes in symlink targets.
+ */
+static void add_path(struct strbuf *buf, size_t base_len, const char *path)
+{
+ strbuf_setlen(buf, base_len);
+ if (buf->len && buf->buf[buf->len - 1] != '/')
+ strbuf_addch(buf, '/');
+ strbuf_addstr(buf, path);
+}
+
+/*
+ * Determine whether we can simply reuse the file in the worktree.
+ */
+static int use_wt_file(const char *workdir, const char *name,
+ struct object_id *oid)
+{
+ struct strbuf buf = STRBUF_INIT;
+ struct stat st;
+ int use = 0;
+
+ strbuf_addstr(&buf, workdir);
+ add_path(&buf, buf.len, name);
+
+ if (!lstat(buf.buf, &st) && !S_ISLNK(st.st_mode)) {
+ struct object_id wt_oid;
+ int fd = open(buf.buf, O_RDONLY);
+
+ if (fd >= 0 &&
+ !index_fd(wt_oid.hash, fd, &st, OBJ_BLOB, name, 0)) {
+ if (is_null_oid(oid)) {
+ oidcpy(oid, &wt_oid);
+ use = 1;
+ } else if (!oidcmp(oid, &wt_oid))
+ use = 1;
+ }
+ }
+
+ strbuf_release(&buf);
+
+ return use;
+}
+
+struct working_tree_entry {
+ struct hashmap_entry entry;
+ char path[FLEX_ARRAY];
+};
+
+static int working_tree_entry_cmp(struct working_tree_entry *a,
+ struct working_tree_entry *b, void *keydata)
+{
+ return strcmp(a->path, b->path);
+}
+
+/*
+ * The `left` and `right` entries hold paths for the symlinks hashmap,
+ * and a SHA-1 surrounded by brief text for submodules.
+ */
+struct pair_entry {
+ struct hashmap_entry entry;
+ char left[PATH_MAX], right[PATH_MAX];
+ const char path[FLEX_ARRAY];
+};
+
+static int pair_cmp(struct pair_entry *a, struct pair_entry *b, void *keydata)
+{
+ return strcmp(a->path, b->path);
+}
+
+static void add_left_or_right(struct hashmap *map, const char *path,
+ const char *content, int is_right)
+{
+ struct pair_entry *e, *existing;
+
+ FLEX_ALLOC_STR(e, path, path);
+ hashmap_entry_init(e, strhash(path));
+ existing = hashmap_get(map, e, NULL);
+ if (existing) {
+ free(e);
+ e = existing;
+ } else {
+ e->left[0] = e->right[0] = '\0';
+ hashmap_add(map, e);
+ }
+ strlcpy(is_right ? e->right : e->left, content, PATH_MAX);
+}
+
+struct path_entry {
+ struct hashmap_entry entry;
+ char path[FLEX_ARRAY];
+};
+
+static int path_entry_cmp(struct path_entry *a, struct path_entry *b, void *key)
+{
+ return strcmp(a->path, key ? key : b->path);
+}
+
+static void changed_files(struct hashmap *result, const char *index_path,
+ const char *workdir)
+{
+ struct child_process update_index = CHILD_PROCESS_INIT;
+ struct child_process diff_files = CHILD_PROCESS_INIT;
+ struct strbuf index_env = STRBUF_INIT, buf = STRBUF_INIT;
+ const char *git_dir = absolute_path(get_git_dir()), *env[] = {
+ NULL, NULL
+ };
+ FILE *fp;
+
+ strbuf_addf(&index_env, "GIT_INDEX_FILE=%s", index_path);
+ env[0] = index_env.buf;
+
+ argv_array_pushl(&update_index.args,
+ "--git-dir", git_dir, "--work-tree", workdir,
+ "update-index", "--really-refresh", "-q",
+ "--unmerged", NULL);
+ update_index.no_stdin = 1;
+ update_index.no_stdout = 1;
+ update_index.no_stderr = 1;
+ update_index.git_cmd = 1;
+ update_index.use_shell = 0;
+ update_index.clean_on_exit = 1;
+ update_index.dir = workdir;
+ update_index.env = env;
+ /* Ignore any errors of update-index */
+ run_command(&update_index);
+
+ argv_array_pushl(&diff_files.args,
+ "--git-dir", git_dir, "--work-tree", workdir,
+ "diff-files", "--name-only", "-z", NULL);
+ diff_files.no_stdin = 1;
+ diff_files.git_cmd = 1;
+ diff_files.use_shell = 0;
+ diff_files.clean_on_exit = 1;
+ diff_files.out = -1;
+ diff_files.dir = workdir;
+ diff_files.env = env;
+ if (start_command(&diff_files))
+ die("could not obtain raw diff");
+ fp = xfdopen(diff_files.out, "r");
+ while (!strbuf_getline_nul(&buf, fp)) {
+ struct path_entry *entry;
+ FLEX_ALLOC_STR(entry, path, buf.buf);
+ hashmap_entry_init(entry, strhash(buf.buf));
+ hashmap_add(result, entry);
+ }
+ if (finish_command(&diff_files))
+ die("diff-files did not exit properly");
+ strbuf_release(&index_env);
+ strbuf_release(&buf);
+}
+
+static NORETURN void exit_cleanup(const char *tmpdir, int exit_code)
+{
+ struct strbuf buf = STRBUF_INIT;
+ strbuf_addstr(&buf, tmpdir);
+ remove_dir_recursively(&buf, 0);
+ if (exit_code)
+ warning(_("failed: %d"), exit_code);
+ exit(exit_code);
+}
+
+static int ensure_leading_directories(char *path)
+{
+ switch (safe_create_leading_directories(path)) {
+ case SCLD_OK:
+ case SCLD_EXISTS:
+ return 0;
+ default:
+ return error(_("could not create leading directories "
+ "of '%s'"), path);
+ }
+}
+
+static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
+ int argc, const char **argv)
+{
+ char tmpdir[PATH_MAX];
+ struct strbuf info = STRBUF_INIT, lpath = STRBUF_INIT;
+ struct strbuf rpath = STRBUF_INIT, buf = STRBUF_INIT;
+ struct strbuf ldir = STRBUF_INIT, rdir = STRBUF_INIT;
+ struct strbuf wtdir = STRBUF_INIT;
+ size_t ldir_len, rdir_len, wtdir_len;
+ struct cache_entry *ce = xcalloc(1, sizeof(ce) + PATH_MAX + 1);
+ const char *workdir, *tmp;
+ int ret = 0, i;
+ FILE *fp;
+ struct hashmap working_tree_dups, submodules, symlinks2;
+ struct hashmap_iter iter;
+ struct pair_entry *entry;
+ enum object_type type;
+ unsigned long size;
+ struct index_state wtindex;
+ struct checkout lstate, rstate;
+ int rc, flags = RUN_GIT_CMD, err = 0;
+ struct child_process child = CHILD_PROCESS_INIT;
+ const char *helper_argv[] = { "difftool--helper", NULL, NULL, NULL };
+ struct hashmap wt_modified, tmp_modified;
+ int indices_loaded = 0;
+
+ workdir = get_git_work_tree();
+
+ /* Setup temp directories */
+ tmp = getenv("TMPDIR");
+ xsnprintf(tmpdir, sizeof(tmpdir), "%s/git-difftool.XXXXXX", tmp ? tmp : "/tmp");
+ if (!mkdtemp(tmpdir))
+ return error("could not create '%s'", tmpdir);
+ strbuf_addf(&ldir, "%s/left/", tmpdir);
+ strbuf_addf(&rdir, "%s/right/", tmpdir);
+ strbuf_addstr(&wtdir, workdir);
+ if (!wtdir.len || !is_dir_sep(wtdir.buf[wtdir.len - 1]))
+ strbuf_addch(&wtdir, '/');
+ mkdir(ldir.buf, 0700);
+ mkdir(rdir.buf, 0700);
+
+ memset(&wtindex, 0, sizeof(wtindex));
+
+ memset(&lstate, 0, sizeof(lstate));
+ lstate.base_dir = ldir.buf;
+ lstate.base_dir_len = ldir.len;
+ lstate.force = 1;
+ memset(&rstate, 0, sizeof(rstate));
+ rstate.base_dir = rdir.buf;
+ rstate.base_dir_len = rdir.len;
+ rstate.force = 1;
+
+ ldir_len = ldir.len;
+ rdir_len = rdir.len;
+ wtdir_len = wtdir.len;
+
+ hashmap_init(&working_tree_dups,
+ (hashmap_cmp_fn)working_tree_entry_cmp, 0);
+ hashmap_init(&submodules, (hashmap_cmp_fn)pair_cmp, 0);
+ hashmap_init(&symlinks2, (hashmap_cmp_fn)pair_cmp, 0);
+
+ child.no_stdin = 1;
+ child.git_cmd = 1;
+ child.use_shell = 0;
+ child.clean_on_exit = 1;
+ child.dir = prefix;
+ child.out = -1;
+ argv_array_pushl(&child.args, "diff", "--raw", "--no-abbrev", "-z",
+ NULL);
+ for (i = 0; i < argc; i++)
+ argv_array_push(&child.args, argv[i]);
+ if (start_command(&child))
+ die("could not obtain raw diff");
+ fp = xfdopen(child.out, "r");
+
+ /* Build index info for left and right sides of the diff */
+ i = 0;
+ while (!strbuf_getline_nul(&info, fp)) {
+ int lmode, rmode;
+ struct object_id loid, roid;
+ char status;
+ const char *src_path, *dst_path;
+ size_t src_path_len, dst_path_len;
+
+ if (starts_with(info.buf, "::"))
+ die(N_("combined diff formats('-c' and '--cc') are "
+ "not supported in\n"
+ "directory diff mode('-d' and '--dir-diff')."));
+
+ if (parse_index_info(info.buf, &lmode, &rmode, &loid, &roid,
+ &status))
+ break;
+ if (strbuf_getline_nul(&lpath, fp))
+ break;
+ src_path = lpath.buf;
+ src_path_len = lpath.len;
+
+ i++;
+ if (status != 'C' && status != 'R') {
+ dst_path = src_path;
+ dst_path_len = src_path_len;
+ } else {
+ if (strbuf_getline_nul(&rpath, fp))
+ break;
+ dst_path = rpath.buf;
+ dst_path_len = rpath.len;
+ }
+
+ if (S_ISGITLINK(lmode) || S_ISGITLINK(rmode)) {
+ strbuf_reset(&buf);
+ strbuf_addf(&buf, "Subproject commit %s",
+ oid_to_hex(&loid));
+ add_left_or_right(&submodules, src_path, buf.buf, 0);
+ strbuf_reset(&buf);
+ strbuf_addf(&buf, "Subproject commit %s",
+ oid_to_hex(&roid));
+ if (!oidcmp(&loid, &roid))
+ strbuf_addstr(&buf, "-dirty");
+ add_left_or_right(&submodules, dst_path, buf.buf, 1);
+ continue;
+ }
+
+ if (S_ISLNK(lmode)) {
+ char *content = read_sha1_file(loid.hash, &type, &size);
+ add_left_or_right(&symlinks2, src_path, content, 0);
+ free(content);
+ }
+
+ if (S_ISLNK(rmode)) {
+ char *content = read_sha1_file(roid.hash, &type, &size);
+ add_left_or_right(&symlinks2, dst_path, content, 1);
+ free(content);
+ }
+
+ if (lmode && status != 'C') {
+ ce->ce_mode = lmode;
+ oidcpy(&ce->oid, &loid);
+ strcpy(ce->name, src_path);
+ ce->ce_namelen = src_path_len;
+ if (checkout_entry(ce, &lstate, NULL))
+ return error("could not write '%s'", src_path);
+ }
+
+ if (rmode) {
+ struct working_tree_entry *entry;
+
+ /* Avoid duplicate working_tree entries */
+ FLEX_ALLOC_STR(entry, path, dst_path);
+ hashmap_entry_init(entry, strhash(dst_path));
+ if (hashmap_get(&working_tree_dups, entry, NULL)) {
+ free(entry);
+ continue;
+ }
+ hashmap_add(&working_tree_dups, entry);
+
+ if (!use_wt_file(workdir, dst_path, &roid)) {
+ ce->ce_mode = rmode;
+ oidcpy(&ce->oid, &roid);
+ strcpy(ce->name, dst_path);
+ ce->ce_namelen = dst_path_len;
+ if (checkout_entry(ce, &rstate, NULL))
+ return error("could not write '%s'",
+ dst_path);
+ } else if (!is_null_oid(&roid)) {
+ /*
+ * Changes in the working tree need special
+ * treatment since they are not part of the
+ * index.
+ */
+ struct cache_entry *ce2 =
+ make_cache_entry(rmode, roid.hash,
+ dst_path, 0, 0);
+
+ add_index_entry(&wtindex, ce2,
+ ADD_CACHE_JUST_APPEND);
+
+ add_path(&rdir, rdir_len, dst_path);
+ if (ensure_leading_directories(rdir.buf))
+ return error("could not create "
+ "directory for '%s'",
+ dst_path);
+ add_path(&wtdir, wtdir_len, dst_path);
+ if (symlinks) {
+ if (symlink(wtdir.buf, rdir.buf)) {
+ ret = error_errno("could not symlink '%s' to '%s'", wtdir.buf, rdir.buf);
+ goto finish;
+ }
+ } else {
+ struct stat st;
+ if (stat(wtdir.buf, &st))
+ st.st_mode = 0644;
+ if (copy_file(rdir.buf, wtdir.buf,
+ st.st_mode)) {
+ ret = error("could not copy '%s' to '%s'", wtdir.buf, rdir.buf);
+ goto finish;
+ }
+ }
+ }
+ }
+ }
+
+ if (finish_command(&child)) {
+ ret = error("error occurred running diff --raw");
+ goto finish;
+ }
+
+ if (!i)
+ return 0;
+
+ /*
+ * Changes to submodules require special treatment.This loop writes a
+ * temporary file to both the left and right directories to show the
+ * change in the recorded SHA1 for the submodule.
+ */
+ hashmap_iter_init(&submodules, &iter);
+ while ((entry = hashmap_iter_next(&iter))) {
+ if (*entry->left) {
+ add_path(&ldir, ldir_len, entry->path);
+ ensure_leading_directories(ldir.buf);
+ write_file(ldir.buf, "%s", entry->left);
+ }
+ if (*entry->right) {
+ add_path(&rdir, rdir_len, entry->path);
+ ensure_leading_directories(rdir.buf);
+ write_file(rdir.buf, "%s", entry->right);
+ }
+ }
+
+ /*
+ * Symbolic links require special treatment.The standard "git diff"
+ * shows only the link itself, not the contents of the link target.
+ * This loop replicates that behavior.
+ */
+ hashmap_iter_init(&symlinks2, &iter);
+ while ((entry = hashmap_iter_next(&iter))) {
+ if (*entry->left) {
+ add_path(&ldir, ldir_len, entry->path);
+ ensure_leading_directories(ldir.buf);
+ write_file(ldir.buf, "%s", entry->left);
+ }
+ if (*entry->right) {
+ add_path(&rdir, rdir_len, entry->path);
+ ensure_leading_directories(rdir.buf);
+ write_file(rdir.buf, "%s", entry->right);
+ }
+ }
+
+ strbuf_release(&buf);
+
+ strbuf_setlen(&ldir, ldir_len);
+ helper_argv[1] = ldir.buf;
+ strbuf_setlen(&rdir, rdir_len);
+ helper_argv[2] = rdir.buf;
+
+ if (extcmd) {
+ helper_argv[0] = extcmd;
+ flags = 0;
+ } else
+ setenv("GIT_DIFFTOOL_DIRDIFF", "true", 1);
+ rc = run_command_v_opt(helper_argv, flags);
+
+ /*
+ * If the diff includes working copy files and those
+ * files were modified during the diff, then the changes
+ * should be copied back to the working tree.
+ * Do not copy back files when symlinks are used and the
+ * external tool did not replace the original link with a file.
+ *
+ * These hashes are loaded lazily since they aren't needed
+ * in the common case of --symlinks and the difftool updating
+ * files through the symlink.
+ */
+ hashmap_init(&wt_modified, (hashmap_cmp_fn)path_entry_cmp,
+ wtindex.cache_nr);
+ hashmap_init(&tmp_modified, (hashmap_cmp_fn)path_entry_cmp,
+ wtindex.cache_nr);
+
+ for (i = 0; i < wtindex.cache_nr; i++) {
+ struct hashmap_entry dummy;
+ const char *name = wtindex.cache[i]->name;
+ struct stat st;
+
+ add_path(&rdir, rdir_len, name);
+ if (lstat(rdir.buf, &st))
+ continue;
+
+ if ((symlinks && S_ISLNK(st.st_mode)) || !S_ISREG(st.st_mode))
+ continue;
+
+ if (!indices_loaded) {
+ static struct lock_file lock;
+ strbuf_reset(&buf);
+ strbuf_addf(&buf, "%s/wtindex", tmpdir);
+ if (hold_lock_file_for_update(&lock, buf.buf, 0) < 0 ||
+ write_locked_index(&wtindex, &lock, COMMIT_LOCK)) {
+ ret = error("could not write %s", buf.buf);
+ rollback_lock_file(&lock);
+ goto finish;
+ }
+ changed_files(&wt_modified, buf.buf, workdir);
+ strbuf_setlen(&rdir, rdir_len);
+ changed_files(&tmp_modified, buf.buf, rdir.buf);
+ add_path(&rdir, rdir_len, name);
+ indices_loaded = 1;
+ }
+
+ hashmap_entry_init(&dummy, strhash(name));
+ if (hashmap_get(&tmp_modified, &dummy, name)) {
+ add_path(&wtdir, wtdir_len, name);
+ if (hashmap_get(&wt_modified, &dummy, name)) {
+ warning(_("both files modified: '%s' and '%s'."),
+ wtdir.buf, rdir.buf);
+ warning(_("working tree file has been left."));
+ warning("");
+ err = 1;
+ } else if (unlink(wtdir.buf) ||
+ copy_file(wtdir.buf, rdir.buf, st.st_mode))
+ warning_errno(_("could not copy '%s' to '%s'"),
+ rdir.buf, wtdir.buf);
+ }
+ }
+
+ if (err) {
+ warning(_("temporary files exist in '%s'."), tmpdir);
+ warning(_("you may want to cleanup or recover these."));
+ exit(1);
+ } else
+ exit_cleanup(tmpdir, rc);
+
+finish:
+ free(ce);
+ strbuf_release(&ldir);
+ strbuf_release(&rdir);
+ strbuf_release(&wtdir);
+ strbuf_release(&buf);
+
+ return ret;
+}
+
+static int run_file_diff(int prompt, const char *prefix,
+ int argc, const char **argv)
+{
+ struct argv_array args = ARGV_ARRAY_INIT;
+ const char *env[] = {
+ "GIT_PAGER=", "GIT_EXTERNAL_DIFF=git-difftool--helper", NULL,
+ NULL
+ };
+ int ret = 0, i;
+
+ if (prompt > 0)
+ env[2] = "GIT_DIFFTOOL_PROMPT=true";
+ else if (!prompt)
+ env[2] = "GIT_DIFFTOOL_NO_PROMPT=true";
+
+
+ argv_array_push(&args, "diff");
+ for (i = 0; i < argc; i++)
+ argv_array_push(&args, argv[i]);
+ ret = run_command_v_opt_cd_env(args.argv, RUN_GIT_CMD, prefix, env);
+ exit(ret);
+}
/*
* NEEDSWORK: this function can go once the legacy-difftool Perl script is
@@ -41,6 +642,35 @@ static int use_builtin_difftool(void) {
int cmd_difftool(int argc, const char **argv, const char *prefix)
{
+ int use_gui_tool = 0, dir_diff = 0, prompt = -1, symlinks = 0,
+ tool_help = 0;
+ static char *difftool_cmd = NULL, *extcmd = NULL;
+ struct option builtin_difftool_options[] = {
+ OPT_BOOL('g', "gui", &use_gui_tool,
+ N_("use `diff.guitool` instead of `diff.tool`")),
+ OPT_BOOL('d', "dir-diff", &dir_diff,
+ N_("perform a full-directory diff")),
+ { OPTION_SET_INT, 'y', "no-prompt", &prompt, NULL,
+ N_("do not prompt before launching a diff tool"),
+ PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, 0},
+ { OPTION_SET_INT, 0, "prompt", &prompt, NULL, NULL,
+ PARSE_OPT_NOARG | PARSE_OPT_NONEG | PARSE_OPT_HIDDEN,
+ NULL, 1 },
+ OPT_BOOL(0, "symlinks", &symlinks,
+ N_("use symlinks in dir-diff mode")),
+ OPT_STRING('t', "tool", &difftool_cmd, N_("<tool>"),
+ N_("use the specified diff tool")),
+ OPT_BOOL(0, "tool-help", &tool_help,
+ N_("print a list of diff tools that may be used with "
+ "`--tool`")),
+ OPT_BOOL(0, "trust-exit-code", &trust_exit_code,
+ N_("make 'git-difftool' exit when an invoked diff "
+ "tool returns a non - zero exit code")),
+ OPT_STRING('x', "extcmd", &extcmd, N_("<command>"),
+ N_("specify a custom command for viewing diffs")),
+ OPT_END()
+ };
+
/*
* NEEDSWORK: Once the builtin difftool has been tested enough
* and git-legacy-difftool.perl is retired to contrib/, this preamble
@@ -58,6 +688,46 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)
prefix = setup_git_directory();
trace_repo_setup(prefix);
setup_work_tree();
+ /* NEEDSWORK: once we no longer spawn anything, remove this */
+ setenv(GIT_DIR_ENVIRONMENT, absolute_path(get_git_dir()), 1);
+ setenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(get_git_work_tree()), 1);
+
+ git_config(difftool_config, NULL);
+ symlinks = has_symlinks;
+
+ argc = parse_options(argc, argv, prefix, builtin_difftool_options,
+ builtin_difftool_usage, PARSE_OPT_KEEP_UNKNOWN |
+ PARSE_OPT_KEEP_DASHDASH);
- die("TODO");
+ if (tool_help)
+ return print_tool_help();
+
+ if (use_gui_tool && diff_gui_tool && *diff_gui_tool)
+ setenv("GIT_DIFF_TOOL", diff_gui_tool, 1);
+ else if (difftool_cmd) {
+ if (*difftool_cmd)
+ setenv("GIT_DIFF_TOOL", difftool_cmd, 1);
+ else
+ die(_("no <tool> given for --tool=<tool>"));
+ }
+
+ if (extcmd) {
+ if (*extcmd)
+ setenv("GIT_DIFFTOOL_EXTCMD", extcmd, 1);
+ else
+ die(_("no <cmd> given for --extcmd=<cmd>"));
+ }
+
+ setenv("GIT_DIFFTOOL_TRUST_EXIT_CODE",
+ trust_exit_code ? "true" : "false", 1);
+
+ /*
+ * In directory diff mode, 'git-difftool--helper' is called once
+ * to compare the a / b directories. In file diff mode, 'git diff'
+ * will invoke a separate instance of 'git-difftool--helper' for
+ * each file that changed.
+ */
+ if (dir_diff)
+ return run_dir_diff(extcmd, symlinks, prefix, argc, argv);
+ return run_file_diff(prompt, prefix, argc, argv);
}
--
2.11.0.rc3.windows.1
^ permalink raw reply related
* [PATCH v4 1/4] Avoid Coverity warning about unfree()d git_exec_path()
From: Johannes Schindelin @ 2017-01-02 16:22 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, David Aguilar, Dennis Kaarsemaker, Paul Sbarra
In-Reply-To: <cover.1483373635.git.johannes.schindelin@gmx.de>
Technically, it is correct that git_exec_path() returns a possibly
malloc()ed string. Practically, it is *sometimes* not malloc()ed. So
let's just use a static variable to make it a singleton. That'll shut
Coverity up, hopefully.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
exec_cmd.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/exec_cmd.c b/exec_cmd.c
index 19ac2146d0..587bd7eb48 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -65,6 +65,7 @@ void git_set_argv_exec_path(const char *exec_path)
const char *git_exec_path(void)
{
const char *env;
+ static char *system_exec_path;
if (argv_exec_path)
return argv_exec_path;
@@ -74,7 +75,9 @@ const char *git_exec_path(void)
return env;
}
- return system_path(GIT_EXEC_PATH);
+ if (!system_exec_path)
+ system_exec_path = system_path(GIT_EXEC_PATH);
+ return system_exec_path;
}
static void add_path(struct strbuf *out, const char *path)
--
2.11.0.rc3.windows.1
^ permalink raw reply related
* [PATCH v4 4/4] t7800: run both builtin and scripted difftool, for now
From: Johannes Schindelin @ 2017-01-02 16:24 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, David Aguilar, Dennis Kaarsemaker, Paul Sbarra
In-Reply-To: <cover.1483373635.git.johannes.schindelin@gmx.de>
This is uglier than a simple
touch "$GIT_EXEC_PATH/use-builtin-difftool"
of course. But oh well.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
This patch implements the good ole' cross-validation technique
(also known as "GitHub Scientist") that I already used for my
rebase--helper work.
I am not sure whether we want to have that patch in `master`,
ever. But at least for the transition time, it may make sense.
t/t7800-difftool.sh | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index e94910c563..273ab55723 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -23,6 +23,20 @@ prompt_given ()
test "$prompt" = "Launch 'test-tool' [Y/n]? branch"
}
+for use_builtin_difftool in false true
+do
+
+test_expect_success 'verify we are running the correct difftool' '
+ if test true = '$use_builtin_difftool'
+ then
+ test_must_fail ok=129 git difftool -h >help &&
+ grep "g, --gui" help
+ else
+ git difftool -h >help &&
+ grep "g|--gui" help
+ fi
+'
+
# NEEDSWORK: lose all the PERL prereqs once legacy-difftool is retired.
# Create a file on master and change it on branch
@@ -606,4 +620,17 @@ test_expect_success PERL,SYMLINKS 'difftool --dir-diff symlinked directories' '
)
'
+test true != $use_builtin_difftool || break
+
+test_expect_success 'tear down for re-run' '
+ rm -rf * .[a-z]* &&
+ git init
+'
+
+# run as builtin difftool now
+GIT_CONFIG_PARAMETERS="'difftool.usebuiltin=true'"
+export GIT_CONFIG_PARAMETERS
+
+done
+
test_done
--
2.11.0.rc3.windows.1
^ permalink raw reply related
* Re: [PATCH v3 20/23] try_remove_empty_parents(): don't trash argument contents
From: Michael Haggerty @ 2017-01-02 16:27 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git, David Turner
In-Reply-To: <20161231064053.prvlw6x6qprzkmw7@sigill.intra.peff.net>
On 12/31/2016 07:40 AM, Jeff King wrote:
> On Sat, Dec 31, 2016 at 04:13:00AM +0100, Michael Haggerty wrote:
>
>> It's bad manners and surprising and therefore error-prone.
>
> Agreed.
>
> I wondered while reading your earlier patch whether the
> safe_create_leading_directories() function had the same problem, but it
> carefully replaces the NUL it inserts.
>
>> -static void try_remove_empty_parents(char *refname)
>> +static void try_remove_empty_parents(const char *refname)
>> {
>> + struct strbuf buf = STRBUF_INIT;
>> char *p, *q;
>> int i;
>> - p = refname;
>> +
>> + strbuf_addstr(&buf, refname);
>
> I see here you just make a copy. I think it would be enough to do:
>
>> @@ -2305,10 +2306,11 @@ static void try_remove_empty_parents(char *refname)
>> q--;
>> if (q == p)
>> break;
>> - *q = '\0';
>> - if (rmdir(git_path("%s", refname)))
>> + strbuf_setlen(&buf, q - buf.buf);
>> + if (rmdir(git_path("%s", buf.buf)))
>> break;
>
> *q = '\0';
> r = rmdir(git_path("%s", refname));
> *q = '/';
>
> if (r)
> break;
>
> or something. But I doubt the single allocation is breaking the bank,
> and it has the nice side effect that callers can pass in a const string
> (I didn't check yet whether that enables further cleanups).
The last patch in the series passes ref_update::refname to this
function, which is `const char *`. With your suggested change, either
that member would have to be made non-const, or it would have to be cast
to const at the `try_remove_empty_parents()` callsite.
Making the member non-const would be easy, though it loses a tiny bit of
documentation and safety against misuse. Also, scribbling even
temporarily over that member would mean that this code is not
thread-safe (though it seems unlikely that we would ever bother making
it multithreaded).
I think I prefer the current version because it loosens the coupling
between this function and its callers. But I don't mind either way if
somebody feels strongly about it.
As an aside, I wonder whether we would be discussing this at all if we
had stack-allocated strbufs that could be used without allocating heap
memory in the usual case.
Michael
^ permalink raw reply
* Re: [PATCH 14/17] sha1_file: introduce an nth_packed_object_oid function
From: Jeff King @ 2017-01-02 17:09 UTC (permalink / raw)
To: Michael Haggerty; +Cc: brian m. carlson, git
In-Reply-To: <8c205558-928d-42ac-d401-e73e19c96030@alum.mit.edu>
On Mon, Jan 02, 2017 at 04:30:21PM +0100, Michael Haggerty wrote:
> On 01/01/2017 08:18 PM, brian m. carlson wrote:
> > There are places in the code where we would like to provide a struct
> > object_id *, yet read the hash directly from the pack. Provide an
> > nth_packed_object_oid function that mirrors the nth_packed_object_sha1
> > function.
> >
> > The required cast is questionable, but should be safe on all known
> > platforms. The alternative of allocating an object as an intermediate
> > would be too inefficient and cause memory leaks. If necessary, an
> > intermediate union could be used; this practice is allowed by GCC and
> > explicitly sanctioned by C11. However, such a change will likely not be
> > necessary, and can be made if and when it is.
>
> I have the feeling that this design decision has been discussed on the
> mailing list. If so, could you include a URL here?
I don't recall an explicit discussion, but I think we already do this
for similar reasons in decode_tree_entry(), where we point to the sha1s
embedded in the tree buffer (of course, you may argue that the cast
there is gross, too :) ).
I'm actually not sure how bad this cast is by the C standard. Conversion
between a struct and its first member is pretty common, and the
alignment is guaranteed by the standard. I think it probably breaks
strict aliasing rules, but then so does "struct object". This is
slightly more exotic in that it's not a struct-to-struct cast, and I
could believe that compilers treat those specially.
> The obvious alternative to allocating a new object to return to the
> caller would be to have the caller of `nth_packed_object_oid()` pass a
> `struct object_id *` argument to be filled in (by copying the hash into
> it). Aside from being legal C, this would also be a more robust step
> towards a post-SHA-1 world, where the suggested hack wouldn't work.
>
> Of course, the question is what the callers want to do with the
> `object_id`. Are the return values of `nth_packed_object_sha1()` stored
> to other longer-lived structures that rely on the lifetime of the
> packfile mmap to keep the value valid? If so, then keeping track of the
> lifetime of the `struct object_id` could be a big chore, not to mention
> that needing to keep a 20-byte `struct object_id` around rather than a
> 8- or 4-byte pointer would also cost more RAM.
I agree that in general, copying the values out is a safer and saner
interface. There are definitely lifetime and memory-use issues to
consider (e.g., the loop in verify_packfile()). And I'd have a slight
worry that the performance impact might be noticeable, just because this
is really quite a low-level function that gets called a lot (but of
course measuring trumps everything there).
But I think the thing that gives me the most pause is that the oid
version of the function feels bolted-on. The nth_packed_object_sha1()
function is there specifically to give access to the mmap'd pack-index
data. And at least for now, that only stores sha1s, not any kind of
struct. If and when it does learn about other hashes, I'm not sure if
we're going to want a generic nth_packed_object_oid() function, or if
the callers would need to handle the various cases specially.
Given that this patch only converts one caller, I'd be more inclined to
just have the caller do its own hashcpy. Like:
diff --git a/sha1_file.c b/sha1_file.c
index 1173071859..16345688b5 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -3769,12 +3769,14 @@ static int for_each_object_in_pack(struct packed_git *p, each_packed_object_fn c
for (i = 0; i < p->num_objects; i++) {
const unsigned char *sha1 = nth_packed_object_sha1(p, i);
+ struct object_id oid;
if (!sha1)
return error("unable to get sha1 of object %u in %s",
i, p->pack_name);
- r = cb(sha1, p, i, data);
+ hashcpy(oid.hash, sha1);
+ r = cb(&oid, p, i, data);
if (r)
break;
}
That punts on the issue for all the other callers, but like I said, I'm
not quite sure if, when, and how it would make sense to convert them to
using a "struct oid".
-Peff
^ permalink raw reply related
* Re: [PATCH v3 20/23] try_remove_empty_parents(): don't trash argument contents
From: Jeff King @ 2017-01-02 17:10 UTC (permalink / raw)
To: Michael Haggerty; +Cc: Junio C Hamano, git, David Turner
In-Reply-To: <9215973c-8db1-8f5a-2dc7-3a0137dd5c62@alum.mit.edu>
On Mon, Jan 02, 2017 at 05:27:59PM +0100, Michael Haggerty wrote:
> > or something. But I doubt the single allocation is breaking the bank,
> > and it has the nice side effect that callers can pass in a const string
> > (I didn't check yet whether that enables further cleanups).
>
> The last patch in the series passes ref_update::refname to this
> function, which is `const char *`. With your suggested change, either
> that member would have to be made non-const, or it would have to be cast
> to const at the `try_remove_empty_parents()` callsite.
>
> Making the member non-const would be easy, though it loses a tiny bit of
> documentation and safety against misuse. Also, scribbling even
> temporarily over that member would mean that this code is not
> thread-safe (though it seems unlikely that we would ever bother making
> it multithreaded).
>
> I think I prefer the current version because it loosens the coupling
> between this function and its callers. But I don't mind either way if
> somebody feels strongly about it.
OK, let's take what you have here, then.
> As an aside, I wonder whether we would be discussing this at all if we
> had stack-allocated strbufs that could be used without allocating heap
> memory in the usual case.
I'm not sure. We still pay the memcpy(), though I don't know how
substantial that is compared to an allocation. For these small strings,
probably not very.
-Peff
^ permalink raw reply
* Re: [PATCH v3 21/23] try_remove_empty_parents(): don't accommodate consecutive slashes
From: Michael Haggerty @ 2017-01-02 18:06 UTC (permalink / raw)
To: Jeff King, Junio C Hamano; +Cc: git, David Turner
In-Reply-To: <20170101055947.7b5jxih3wlprqcil@sigill.intra.peff.net>
On 01/01/2017 06:59 AM, Jeff King wrote:
> On Sat, Dec 31, 2016 at 06:30:01PM -0800, Junio C Hamano wrote:
>
>> Michael Haggerty <mhagger@alum.mit.edu> writes:
>>
>>> "refname" has already been checked by check_refname_format(), so it
>>> cannot have consecutive slashes.
>>
>> In the endgame state, this has two callers. Both use what came in
>> the transaction->updates[] array. Presumably "has already been
>> checked by check_refname_format()" says that whoever created entries
>> in that array must have called the function, but it would be helpful
>> to be more explicit here.
>
> Hmm, yeah. This is called when we are deleting a ref, and I thought we
> explicitly _didn't_ do check_refname_format() when deleting, so that
> funny-named refs could be deleted. It's only is_refname_safe() that we
> must pass.
>
> So I have no idea if that's a problem in the code or not, but it is at
> least not immediately obvious who is responsible for calling
> check_refname_format() here.
My assumption was that only valid reference names should ever be allowed
to be inserted into a `ref_transaction` entry. But Peff is right that
sometimes the reference name is checked by `refname_is_safe()` rather
than `check_refname_format()`. Let's audit this more carefully...
* `ref_transaction_add_update()` relies on its caller doing the check
(this fact is documented). Its callers are:
* `ref_transaction_update()` (the usual codepath), which checks the
reference itself using either check_refname_format() or
refname_is_safe() depending on what kind of update it is.
* `split_head_update()` passes the literal string "HEAD".
* `split_symref_update()` picks apart reference updates that go
through existing symbolic references. As such I don't think they
are an attack surface. It doesn't do any checking itself (here
we're talking about its `referent` argument). It has only one
caller:
* `lock_ref_for_update()`, which gets `referent` from:
* `files_read_raw_ref()`, which gets the value either:
* by reading a filesystem-level symlink's contents and
checking it with `check_refname_format()`, or
* reading a symref from the filesystem. In this case, *the
value is not checked*.
Obviously this chain of custody is disconcertingly long and complicated.
And the gap for symrefs should probably be fixed, even though they are
hopefully trustworthy.
`refname_is_safe()` checks that its argument is either UPPER_CASE or
looks like a plausible filename under "refs/". Contrary to its
docstring, it *does not* accept strings that contain successive slashes
or "." or ".." components. It was made stricter in
e40f355 "refname_is_safe(): insist that the refname already be
normalized", 2016-04-27
, but the docstring wasn't updated at that time. I'll fix it.
I think the best thing to do is to drop this patch from the patch
series, and address these checks in a separate series. (There are more
known problems in this area, for example that the checks in
`check_refname_format()` are not a strict superset of the checks in
`refname_is_safe()`.)
Michael
^ permalink raw reply
* Re: [PATCH v3 00/23] Delete directories left empty after ref deletion
From: Michael Haggerty @ 2017-01-02 18:14 UTC (permalink / raw)
To: Jeff King, Jacob Keller
Cc: Philip Oakley, Junio C Hamano, Git mailing list, David Turner
In-Reply-To: <20170102041947.5jzx6og5fcpv7oso@sigill.intra.peff.net>
On 01/02/2017 05:19 AM, Jeff King wrote:
> On Sun, Jan 01, 2017 at 12:36:11PM -0800, Jacob Keller wrote:
>
>> But how likely is it to end up with differing binaries running on the
>> exact same repository concurrently? Basically, I am trying to see
>> whether or not we could accidentally end up causing problems by trying
>> to race with other git processes that haven't yet been made safe
>> against race? Is the worst case only that some git operation would
>> fail and you would have to retry?
>
> Yes, I think that is the worst case.
>
> A more likely scenario might be something like a server accepting pushes
> or other ref updates from both JGit and regular git (or maybe libgit2
> and regular git).
>
> IMHO it's not really worth worrying about too much. Certain esoteric
> setups might have a slightly higher chance of a pretty obscure race
> condition happening on a very busy repository. I hate to say "eh, ship
> it, we'll see if anybody complains". But I'd be surprised to get a
> single report about this.
I agree. I think these races would mostly affect busy hosting sites and
result in failed updates rather than corruption. And the races can
already occur whenever the user runs `git pack-refs`. By contrast, the
failure to delete empty directories is more likely to affect normal users.
That being said, if Junio wants to merge all but the last two patches in
one release, then merge the last two a release or two later, I have no
objections.
Michael
^ permalink raw reply
* Re: [PATCH 14/17] sha1_file: introduce an nth_packed_object_oid function
From: Michael Haggerty @ 2017-01-02 18:18 UTC (permalink / raw)
To: Jeff King; +Cc: brian m. carlson, git
In-Reply-To: <20170102170902.6g6bxvaanewxzm2v@sigill.intra.peff.net>
On 01/02/2017 06:09 PM, Jeff King wrote:
> [...]
> But I think the thing that gives me the most pause is that the oid
> version of the function feels bolted-on. The nth_packed_object_sha1()
> function is there specifically to give access to the mmap'd pack-index
> data. And at least for now, that only stores sha1s, not any kind of
> struct. If and when it does learn about other hashes, I'm not sure if
> we're going to want a generic nth_packed_object_oid() function, or if
> the callers would need to handle the various cases specially.
>
> Given that this patch only converts one caller, I'd be more inclined to
> just have the caller do its own hashcpy. Like:
>
> diff --git a/sha1_file.c b/sha1_file.c
> index 1173071859..16345688b5 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -3769,12 +3769,14 @@ static int for_each_object_in_pack(struct packed_git *p, each_packed_object_fn c
>
> for (i = 0; i < p->num_objects; i++) {
> const unsigned char *sha1 = nth_packed_object_sha1(p, i);
> + struct object_id oid;
>
> if (!sha1)
> return error("unable to get sha1 of object %u in %s",
> i, p->pack_name);
>
> - r = cb(sha1, p, i, data);
> + hashcpy(oid.hash, sha1);
> + r = cb(&oid, p, i, data);
> if (r)
> break;
> }
>
> That punts on the issue for all the other callers, but like I said, I'm
> not quite sure if, when, and how it would make sense to convert them to
> using a "struct oid".
Your change is not safe if any of the callback functions ("cb") tuck
away a copy of the pointer that they are passed, expecting it to contain
the same object id later.
Michael
^ permalink raw reply
* Re: [PATCH 14/17] sha1_file: introduce an nth_packed_object_oid function
From: Jeff King @ 2017-01-02 18:22 UTC (permalink / raw)
To: Michael Haggerty; +Cc: brian m. carlson, git
In-Reply-To: <eb8a3dad-9ca7-e0e6-3c31-9cd2e02e0bf9@alum.mit.edu>
On Mon, Jan 02, 2017 at 07:18:58PM +0100, Michael Haggerty wrote:
> > Given that this patch only converts one caller, I'd be more inclined to
> > just have the caller do its own hashcpy. Like:
> >
> > diff --git a/sha1_file.c b/sha1_file.c
> > index 1173071859..16345688b5 100644
> > --- a/sha1_file.c
> > +++ b/sha1_file.c
> > @@ -3769,12 +3769,14 @@ static int for_each_object_in_pack(struct packed_git *p, each_packed_object_fn c
> >
> > for (i = 0; i < p->num_objects; i++) {
> > const unsigned char *sha1 = nth_packed_object_sha1(p, i);
> > + struct object_id oid;
> >
> > if (!sha1)
> > return error("unable to get sha1 of object %u in %s",
> > i, p->pack_name);
> >
> > - r = cb(sha1, p, i, data);
> > + hashcpy(oid.hash, sha1);
> > + r = cb(&oid, p, i, data);
> > if (r)
> > break;
> > }
> >
> > That punts on the issue for all the other callers, but like I said, I'm
> > not quite sure if, when, and how it would make sense to convert them to
> > using a "struct oid".
>
> Your change is not safe if any of the callback functions ("cb") tuck
> away a copy of the pointer that they are passed, expecting it to contain
> the same object id later.
I think it's generally a given that callback functions should not assume
the lifetime of parameters extend beyond the end of the callback. That
said, I didn't audit the callers (although I'm pretty sure I wrote all
of them myself in this particular case).
-Peff
^ permalink raw reply
* Re: [PATCH v3 21/23] try_remove_empty_parents(): don't accommodate consecutive slashes
From: Jeff King @ 2017-01-02 18:26 UTC (permalink / raw)
To: Michael Haggerty; +Cc: Junio C Hamano, git, David Turner
In-Reply-To: <5051c78e-51f9-becd-e1a6-9c0b781d6912@alum.mit.edu>
On Mon, Jan 02, 2017 at 07:06:32PM +0100, Michael Haggerty wrote:
> My assumption was that only valid reference names should ever be allowed
> to be inserted into a `ref_transaction` entry. But Peff is right that
> sometimes the reference name is checked by `refname_is_safe()` rather
> than `check_refname_format()`. Let's audit this more carefully...
>
> * `ref_transaction_add_update()` relies on its caller doing the check
> (this fact is documented). Its callers are:
> * `ref_transaction_update()` (the usual codepath), which checks the
> reference itself using either check_refname_format() or
> refname_is_safe() depending on what kind of update it is.
> * `split_head_update()` passes the literal string "HEAD".
> * `split_symref_update()` picks apart reference updates that go
> through existing symbolic references. As such I don't think they
> are an attack surface. It doesn't do any checking itself (here
> we're talking about its `referent` argument). It has only one
> caller:
> * `lock_ref_for_update()`, which gets `referent` from:
> * `files_read_raw_ref()`, which gets the value either:
> * by reading a filesystem-level symlink's contents and
> checking it with `check_refname_format()`, or
> * reading a symref from the filesystem. In this case, *the
> value is not checked*.
>
> Obviously this chain of custody is disconcertingly long and complicated.
> And the gap for symrefs should probably be fixed, even though they are
> hopefully trustworthy.
Thanks as always for a careful analysis. I agree it seems like a bug
that symlinks are checked but symrefs are not.
> I think the best thing to do is to drop this patch from the patch
> series, and address these checks in a separate series. (There are more
> known problems in this area, for example that the checks in
> `check_refname_format()` are not a strict superset of the checks in
> `refname_is_safe()`.)
Sounds like a good plan. I'd be very happy if the "superset" mismatch is
fixed. It seems like it has come up in our discussions more than once.
-Peff
^ permalink raw reply
* [PATCH v2 1/2] don't use test_must_fail with grep
From: Pranit Bauva @ 2017-01-02 18:45 UTC (permalink / raw)
To: git; +Cc: sbeller, luke, j6t, Pranit Bauva
In-Reply-To: <20161231114412.23439-1-pranit.bauva@gmail.com>
test_must_fail should only be used for testing git commands. To test the
failure of other commands use `!`.
Reported-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
t/t3510-cherry-pick-sequence.sh | 6 +++---
t/t5504-fetch-receive-strict.sh | 2 +-
t/t5516-fetch-push.sh | 2 +-
t/t5601-clone.sh | 2 +-
t/t6030-bisect-porcelain.sh | 2 +-
t/t7610-mergetool.sh | 2 +-
t/t9001-send-email.sh | 2 +-
t/t9117-git-svn-init-clone.sh | 12 ++++++------
t/t9813-git-p4-preserve-users.sh | 8 ++++----
t/t9814-git-p4-rename.sh | 6 +++---
10 files changed, 22 insertions(+), 22 deletions(-)
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 372307c21..0acf4b146 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -385,7 +385,7 @@ test_expect_success '--continue respects opts' '
git cat-file commit HEAD~1 >picked_msg &&
git cat-file commit HEAD~2 >unrelatedpick_msg &&
git cat-file commit HEAD~3 >initial_msg &&
- test_must_fail grep "cherry picked from" initial_msg &&
+ ! grep "cherry picked from" initial_msg &&
grep "cherry picked from" unrelatedpick_msg &&
grep "cherry picked from" picked_msg &&
grep "cherry picked from" anotherpick_msg
@@ -426,9 +426,9 @@ test_expect_failure '--signoff is automatically propagated to resolved conflict'
git cat-file commit HEAD~1 >picked_msg &&
git cat-file commit HEAD~2 >unrelatedpick_msg &&
git cat-file commit HEAD~3 >initial_msg &&
- test_must_fail grep "Signed-off-by:" initial_msg &&
+ ! grep "Signed-off-by:" initial_msg &&
grep "Signed-off-by:" unrelatedpick_msg &&
- test_must_fail grep "Signed-off-by:" picked_msg &&
+ ! grep "Signed-off-by:" picked_msg &&
grep "Signed-off-by:" anotherpick_msg
'
diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh
index 9b19cff72..49d3621a9 100755
--- a/t/t5504-fetch-receive-strict.sh
+++ b/t/t5504-fetch-receive-strict.sh
@@ -152,7 +152,7 @@ test_expect_success 'push with receive.fsck.missingEmail=warn' '
git --git-dir=dst/.git config --add \
receive.fsck.badDate warn &&
git push --porcelain dst bogus >act 2>&1 &&
- test_must_fail grep "missingEmail" act
+ ! grep "missingEmail" act
'
test_expect_success \
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 26b2cafc4..0fc5a7c59 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1004,7 +1004,7 @@ test_expect_success 'push --porcelain' '
test_expect_success 'push --porcelain bad url' '
mk_empty testrepo &&
test_must_fail git push >.git/bar --porcelain asdfasdfasd refs/heads/master:refs/remotes/origin/master &&
- test_must_fail grep -q Done .git/bar
+ ! grep -q Done .git/bar
'
test_expect_success 'push --porcelain rejected' '
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index a43339420..4241ea5b3 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -151,7 +151,7 @@ test_expect_success 'clone --mirror does not repeat tags' '
git clone --mirror src mirror2 &&
(cd mirror2 &&
git show-ref 2> clone.err > clone.out) &&
- test_must_fail grep Duplicate mirror2/clone.err &&
+ ! grep Duplicate mirror2/clone.err &&
grep some-tag mirror2/clone.out
'
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 5e5370feb..8c2c6eaef 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -407,7 +407,7 @@ test_expect_success 'good merge base when good and bad are siblings' '
test_i18ngrep "merge base must be tested" my_bisect_log.txt &&
grep $HASH4 my_bisect_log.txt &&
git bisect good > my_bisect_log.txt &&
- test_must_fail grep "merge base must be tested" my_bisect_log.txt &&
+ ! grep "merge base must be tested" my_bisect_log.txt &&
grep $HASH6 my_bisect_log.txt &&
git bisect reset
'
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 63d36fb28..0fe7e58cf 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -602,7 +602,7 @@ test_expect_success MKTEMP 'temporary filenames are used with mergetool.writeToT
test_config mergetool.myecho.trustExitCode true &&
test_must_fail git merge master &&
git mergetool --no-prompt --tool myecho -- both >actual &&
- test_must_fail grep ^\./both_LOCAL_ actual >/dev/null &&
+ ! grep ^\./both_LOCAL_ actual >/dev/null &&
grep /both_LOCAL_ actual >/dev/null &&
git reset --hard master >/dev/null 2>&1
'
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 3dc4a3454..0f398dd16 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -50,7 +50,7 @@ test_no_confirm () {
--smtp-server="$(pwd)/fake.sendmail" \
$@ \
$patches >stdout &&
- test_must_fail grep "Send this email" stdout &&
+ ! grep "Send this email" stdout &&
>no_confirm_okay
}
diff --git a/t/t9117-git-svn-init-clone.sh b/t/t9117-git-svn-init-clone.sh
index 69a675052..044f65e91 100755
--- a/t/t9117-git-svn-init-clone.sh
+++ b/t/t9117-git-svn-init-clone.sh
@@ -55,7 +55,7 @@ test_expect_success 'clone to target directory with --stdlayout' '
test_expect_success 'init without -s/-T/-b/-t does not warn' '
test ! -d trunk &&
git svn init "$svnrepo"/project/trunk trunk 2>warning &&
- test_must_fail grep -q prefix warning &&
+ ! grep -q prefix warning &&
rm -rf trunk &&
rm -f warning
'
@@ -63,7 +63,7 @@ test_expect_success 'init without -s/-T/-b/-t does not warn' '
test_expect_success 'clone without -s/-T/-b/-t does not warn' '
test ! -d trunk &&
git svn clone "$svnrepo"/project/trunk 2>warning &&
- test_must_fail grep -q prefix warning &&
+ ! grep -q prefix warning &&
rm -rf trunk &&
rm -f warning
'
@@ -86,7 +86,7 @@ EOF
test_expect_success 'init with -s/-T/-b/-t assumes --prefix=origin/' '
test ! -d project &&
git svn init -s "$svnrepo"/project project 2>warning &&
- test_must_fail grep -q prefix warning &&
+ ! grep -q prefix warning &&
test_svn_configured_prefix "origin/" &&
rm -rf project &&
rm -f warning
@@ -95,7 +95,7 @@ test_expect_success 'init with -s/-T/-b/-t assumes --prefix=origin/' '
test_expect_success 'clone with -s/-T/-b/-t assumes --prefix=origin/' '
test ! -d project &&
git svn clone -s "$svnrepo"/project 2>warning &&
- test_must_fail grep -q prefix warning &&
+ ! grep -q prefix warning &&
test_svn_configured_prefix "origin/" &&
rm -rf project &&
rm -f warning
@@ -104,7 +104,7 @@ test_expect_success 'clone with -s/-T/-b/-t assumes --prefix=origin/' '
test_expect_success 'init with -s/-T/-b/-t and --prefix "" still works' '
test ! -d project &&
git svn init -s "$svnrepo"/project project --prefix "" 2>warning &&
- test_must_fail grep -q prefix warning &&
+ ! grep -q prefix warning &&
test_svn_configured_prefix "" &&
rm -rf project &&
rm -f warning
@@ -113,7 +113,7 @@ test_expect_success 'init with -s/-T/-b/-t and --prefix "" still works' '
test_expect_success 'clone with -s/-T/-b/-t and --prefix "" still works' '
test ! -d project &&
git svn clone -s "$svnrepo"/project --prefix "" 2>warning &&
- test_must_fail grep -q prefix warning &&
+ ! grep -q prefix warning &&
test_svn_configured_prefix "" &&
rm -rf project &&
rm -f warning
diff --git a/t/t9813-git-p4-preserve-users.sh b/t/t9813-git-p4-preserve-users.sh
index 0fe231280..798bf2b67 100755
--- a/t/t9813-git-p4-preserve-users.sh
+++ b/t/t9813-git-p4-preserve-users.sh
@@ -126,13 +126,13 @@ test_expect_success 'not preserving user with mixed authorship' '
grep "git author charlie@example.com does not match" &&
make_change_by_user usernamefile3 alice alice@example.com &&
- git p4 commit |\
- test_must_fail grep "git author.*does not match" &&
+ git p4 commit >actual 2>&1 &&
+ ! grep "git author.*does not match" actual &&
git config git-p4.skipUserNameCheck true &&
make_change_by_user usernamefile3 Charlie charlie@example.com &&
- git p4 commit |\
- test_must_fail grep "git author.*does not match" &&
+ git p4 commit >actual 2>&1 &&
+ ! grep "git author.*does not match" actual &&
p4_check_commit_author usernamefile3 alice
)
diff --git a/t/t9814-git-p4-rename.sh b/t/t9814-git-p4-rename.sh
index c89992cf9..e7e0268e9 100755
--- a/t/t9814-git-p4-rename.sh
+++ b/t/t9814-git-p4-rename.sh
@@ -141,7 +141,7 @@ test_expect_success 'detect copies' '
git diff-tree -r -C HEAD &&
git p4 submit &&
p4 filelog //depot/file8 &&
- p4 filelog //depot/file8 | test_must_fail grep -q "branch from" &&
+ ! p4 filelog //depot/file8 | grep -q "branch from" &&
echo "file9" >>file2 &&
git commit -a -m "Differentiate file2" &&
@@ -154,7 +154,7 @@ test_expect_success 'detect copies' '
git config git-p4.detectCopies true &&
git p4 submit &&
p4 filelog //depot/file9 &&
- p4 filelog //depot/file9 | test_must_fail grep -q "branch from" &&
+ ! p4 filelog //depot/file9 | grep -q "branch from" &&
echo "file10" >>file2 &&
git commit -a -m "Differentiate file2" &&
@@ -202,7 +202,7 @@ test_expect_success 'detect copies' '
git config git-p4.detectCopies $(($level + 2)) &&
git p4 submit &&
p4 filelog //depot/file12 &&
- p4 filelog //depot/file12 | test_must_fail grep -q "branch from" &&
+ ! p4 filelog //depot/file12 | grep -q "branch from" &&
echo "file13" >>file2 &&
git commit -a -m "Differentiate file2" &&
--
2.11.0
^ permalink raw reply related
* [PATCH v2 2/2] t9813: avoid using pipes
From: Pranit Bauva @ 2017-01-02 18:45 UTC (permalink / raw)
To: git; +Cc: sbeller, luke, j6t, Pranit Bauva
In-Reply-To: <20161231114412.23439-1-pranit.bauva@gmail.com>
The exit code of the upstream in a pipe is ignored thus we should avoid
using it. By writing out the output of the git command to a file, we can
test the exit codes of both the commands.
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
t/t9813-git-p4-preserve-users.sh | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/t/t9813-git-p4-preserve-users.sh b/t/t9813-git-p4-preserve-users.sh
index 798bf2b67..9d7550ff3 100755
--- a/t/t9813-git-p4-preserve-users.sh
+++ b/t/t9813-git-p4-preserve-users.sh
@@ -118,12 +118,12 @@ test_expect_success 'not preserving user with mixed authorship' '
make_change_by_user usernamefile3 Derek derek@example.com &&
P4EDITOR=cat P4USER=alice P4PASSWD=secret &&
export P4EDITOR P4USER P4PASSWD &&
- git p4 commit |\
- grep "git author derek@example.com does not match" &&
+ git p4 commit >actual 2>&1 &&
+ grep "git author derek@example.com does not match" actual &&
make_change_by_user usernamefile3 Charlie charlie@example.com &&
- git p4 commit |\
- grep "git author charlie@example.com does not match" &&
+ git p4 commit >actual 2>&1 &&
+ grep "git author charlie@example.com does not match" actual &&
make_change_by_user usernamefile3 alice alice@example.com &&
git p4 commit >actual 2>&1 &&
--
2.11.0
^ permalink raw reply related
* Re: [PATCH v3 00/23] Delete directories left empty after ref deletion
From: Jacob Keller @ 2017-01-02 18:54 UTC (permalink / raw)
To: Michael Haggerty
Cc: Jeff King, Philip Oakley, Junio C Hamano, Git mailing list,
David Turner
In-Reply-To: <ab92dc60-c623-0f6f-868b-b74b1d6dbd2e@alum.mit.edu>
On Mon, Jan 2, 2017 at 10:14 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 01/02/2017 05:19 AM, Jeff King wrote:
>> On Sun, Jan 01, 2017 at 12:36:11PM -0800, Jacob Keller wrote:
>>
>>> But how likely is it to end up with differing binaries running on the
>>> exact same repository concurrently? Basically, I am trying to see
>>> whether or not we could accidentally end up causing problems by trying
>>> to race with other git processes that haven't yet been made safe
>>> against race? Is the worst case only that some git operation would
>>> fail and you would have to retry?
>>
>> Yes, I think that is the worst case.
>>
>> A more likely scenario might be something like a server accepting pushes
>> or other ref updates from both JGit and regular git (or maybe libgit2
>> and regular git).
>>
>> IMHO it's not really worth worrying about too much. Certain esoteric
>> setups might have a slightly higher chance of a pretty obscure race
>> condition happening on a very busy repository. I hate to say "eh, ship
>> it, we'll see if anybody complains". But I'd be surprised to get a
>> single report about this.
>
> I agree. I think these races would mostly affect busy hosting sites and
> result in failed updates rather than corruption. And the races can
> already occur whenever the user runs `git pack-refs`. By contrast, the
> failure to delete empty directories is more likely to affect normal users.
>
> That being said, if Junio wants to merge all but the last two patches in
> one release, then merge the last two a release or two later, I have no
> objections.
>
> Michael
>
I only wanted to make sure that the failure mode would not result in
corruption. I believe that both you and Peff have alleviated my fears.
Thanks,
Jake
^ permalink raw reply
* Re: builtin difftool parsing issue
From: Paul Sbarra @ 2017-01-02 19:05 UTC (permalink / raw)
To: Johannes Schindelin
Cc: David Aguilar, Dennis Kaarsemaker, git, Junio C Hamano
In-Reply-To: <alpine.DEB.2.20.1701021712010.3469@virtualbox>
> I would have expected `git difftool --submodule=diff ...` to work... What
> are the problems?
The docs for difftool state...
"git difftool is a frontend to git diff and accepts the same options
and arguments."
which could lead a user to expect passing --submodule=diff to have a
similar behavior for difftool. It would be especially useful when
combined with --dir-diff.
Unfortunately, due to the way the left/right directories are built up,
difftool needs to handle this option itself. Currently a file
representing the submodule directory is created that contains the
hash.
if (S_ISGITLINK(lmode) || S_ISGITLINK(rmode)) {
strbuf_reset(&buf);
strbuf_addf(&buf, "Subproject commit %s", oid_to_hex(&loid));
add_left_or_right(&submodules, src_path, buf.buf, 0);
strbuf_reset(&buf);
strbuf_addf(&buf, "Subproject commit %s", oid_to_hex(&roid));
if (!oidcmp(&loid, &roid))
strbuf_addstr(&buf, "-dirty");
add_left_or_right(&submodules, dst_path, buf.buf, 1);
continue;
}
To achieve the desired behavior a diff command would need to be run
within the submodule. A further complication is whether submodules
should be processed recursively. I'm not sure whether or not diff
handles them recursively. I believe the logic to parse and build up
the files would need to be factored out such that it could be called
for the super-project as well as each submodule change.
This is all out of scope for your effort as the existing (perl-based)
difftool doesn't do this either. However, it's a feature that would
provide a significant simplification to the workflow used at the
office to review changes.
^ permalink raw reply
* [PATCH] archive-zip: load userdiff config
From: Jeff King @ 2017-01-02 22:25 UTC (permalink / raw)
To: git; +Cc: René Scharfe
Since 4aff646d17 (archive-zip: mark text files in archives,
2015-03-05), the zip archiver will look at the userdiff
driver to decide whether a file is text or binary. This
usually doesn't need to look any further than the attributes
themselves (e.g., "-diff", etc). But if the user defines a
custom driver like "diff=foo", we need to look at
"diff.foo.binary" in the config. Prior to this patch, we
didn't actually load it.
Signed-off-by: Jeff King <peff@peff.net>
---
I'd be surprised if anybody actually triggered this in practice. I don't
think any of the custom-driver fields except "binary" matter, and using
direct attributes is almost always easier than setting up a custom
driver. Though you could also do trickery with:
git -c diff.default.binary=true archive ...
if you wanted to be really clever.
I ran across this while investigating a case where somebody's zipfile
was all marked as binary (it turned out not to be related; the issue was
just that their Git was pre-4aff646d17).
I also happened to notice that zipfiles are created using the local
timezone (because they have no notion of the timezone, so we have to
pick _something_). That's probably the least-terrible option, but it was
certainly surprising to me when I tried to bit-for-bit reproduce a
zipfile from GitHub on my local machine.
archive-zip.c | 7 +++++++
t/t5003-archive-zip.sh | 22 ++++++++++++++++++----
2 files changed, 25 insertions(+), 4 deletions(-)
diff --git a/archive-zip.c b/archive-zip.c
index 9db47357b0..b429a8d974 100644
--- a/archive-zip.c
+++ b/archive-zip.c
@@ -554,11 +554,18 @@ static void dos_time(time_t *time, int *dos_date, int *dos_time)
*dos_time = t->tm_sec / 2 + t->tm_min * 32 + t->tm_hour * 2048;
}
+static int archive_zip_config(const char *var, const char *value, void *data)
+{
+ return userdiff_config(var, value);
+}
+
static int write_zip_archive(const struct archiver *ar,
struct archiver_args *args)
{
int err;
+ git_config(archive_zip_config, NULL);
+
dos_time(&args->time, &zip_date, &zip_time);
zip_dir = xmalloc(ZIP_DIRECTORY_MIN_SIZE);
diff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh
index 14744b2a4b..55c7870997 100755
--- a/t/t5003-archive-zip.sh
+++ b/t/t5003-archive-zip.sh
@@ -64,6 +64,12 @@ check_zip() {
test_cmp_bin $original/nodiff.crlf $extracted/nodiff.crlf &&
test_cmp_bin $original/nodiff.lf $extracted/nodiff.lf
"
+
+ test_expect_success UNZIP " validate that custom diff is unchanged " "
+ test_cmp_bin $original/custom.cr $extracted/custom.cr &&
+ test_cmp_bin $original/custom.crlf $extracted/custom.crlf &&
+ test_cmp_bin $original/custom.lf $extracted/custom.lf
+ "
}
test_expect_success \
@@ -78,6 +84,9 @@ test_expect_success \
printf "text\r" >a/nodiff.cr &&
printf "text\r\n" >a/nodiff.crlf &&
printf "text\n" >a/nodiff.lf &&
+ printf "text\r" >a/custom.cr &&
+ printf "text\r\n" >a/custom.crlf &&
+ printf "text\n" >a/custom.lf &&
printf "\0\r" >a/binary.cr &&
printf "\0\r\n" >a/binary.crlf &&
printf "\0\n" >a/binary.lf &&
@@ -112,15 +121,20 @@ test_expect_success 'add files to repository' '
test_expect_success 'setup export-subst and diff attributes' '
echo "a/nodiff.* -diff" >>.git/info/attributes &&
echo "a/diff.* diff" >>.git/info/attributes &&
+ echo "a/custom.* diff=custom" >>.git/info/attributes &&
+ git config diff.custom.binary true &&
echo "substfile?" export-subst >>.git/info/attributes &&
git log --max-count=1 "--pretty=format:A${SUBSTFORMAT}O" HEAD \
>a/substfile1
'
-test_expect_success \
- 'create bare clone' \
- 'git clone --bare . bare.git &&
- cp .git/info/attributes bare.git/info/attributes'
+test_expect_success 'create bare clone' '
+ git clone --bare . bare.git &&
+ cp .git/info/attributes bare.git/info/attributes &&
+ # Recreate our changes to .git/config rather than just copying it, as
+ # we do not want to clobber core.bare or other settings.
+ git -C bare.git config diff.custom.binary true
+'
test_expect_success \
'remove ignored file' \
--
2.11.0.519.g31435224cf
^ permalink raw reply related
* Re: [PATCH 13/17] refs: convert each_reflog_ent_fn to struct object_id
From: Michael Haggerty @ 2017-01-02 23:30 UTC (permalink / raw)
To: brian m. carlson; +Cc: git discussion list, Jeff King
In-Reply-To: <20170102191256.fjqsns3rgjyehzgp@genre.crustytoothpaste.net>
On 01/02/2017 08:12 PM, brian m. carlson wrote:
> On Mon, Jan 02, 2017 at 04:07:16PM +0100, Michael Haggerty wrote:
>> On 01/01/2017 08:18 PM, brian m. carlson wrote:
>>> /* old SP new SP name <email> SP time TAB msg LF */
>>> if (sb->len < 83 || sb->buf[sb->len - 1] != '\n' ||
>>> - get_sha1_hex(sb->buf, osha1) || sb->buf[40] != ' ' ||
>>> - get_sha1_hex(sb->buf + 41, nsha1) || sb->buf[81] != ' ' ||
>>> + get_oid_hex(sb->buf, &ooid) || sb->buf[40] != ' ' ||
>>> + get_oid_hex(sb->buf + 41, &noid) || sb->buf[81] != ' ' ||
>>
>> Some magic numbers above could be converted to use constants.
>
> Yes, I saw that. I opted to leave it as it is for the moment.
Totally understandable.
> I think
> my next series is going to include a small sscanf-style parser to parse
> these. Right now, using constants here is going to leave it extremely
> difficult to read. Something like the following for the OIDs:
>
> strbuf_git_scanf(sb, "%h %h ", &ooid, &noid);
>
> and then following up parsing the remainder.
Maybe something with an interface like skip_prefix wouldn't be too
obnoxious:
const char *p = sb.buf;
if (oid_prefix(p, &ooid, &p) &&
*p++ == ' ' &&
oid_prefix(p, &noid, &p) && ...
> [...]
Michael
^ permalink raw reply
* [PATCH] diff: add interhunk context config option
From: Vegard Nossum @ 2017-01-02 23:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Vegard Nossum, René Scharfe
The --inter-hunk-context= option was added in commit 6d0e674a5754
("diff: add option to show context between close hunks"). This patch
allows configuring a default for this option.
Cc: René Scharfe <l.s.r@web.de>
Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
---
Documentation/diff-options.txt | 2 ++
diff.c | 8 ++++++++
2 files changed, 10 insertions(+)
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index e6215c372..163b65444 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -511,6 +511,8 @@ endif::git-format-patch[]
--inter-hunk-context=<lines>::
Show the context between diff hunks, up to the specified number
of lines, thereby fusing hunks that are close to each other.
+ Defaults to `diff.interhunkcontext` or 0 if the config option
+ is unset.
-W::
--function-context::
diff --git a/diff.c b/diff.c
index 84dba60c4..40b4e6afe 100644
--- a/diff.c
+++ b/diff.c
@@ -33,6 +33,7 @@ static int diff_rename_limit_default = 400;
static int diff_suppress_blank_empty;
static int diff_use_color_default = -1;
static int diff_context_default = 3;
+static int diff_interhunk_context_default = 0;
static const char *diff_word_regex_cfg;
static const char *external_diff_cmd_cfg;
static const char *diff_order_file_cfg;
@@ -248,6 +249,12 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)
return -1;
return 0;
}
+ if (!strcmp(var, "diff.interhunkcontext")) {
+ diff_interhunk_context_default = git_config_int(var, value);
+ if (diff_interhunk_context_default < 0)
+ return -1;
+ return 0;
+ }
if (!strcmp(var, "diff.renames")) {
diff_detect_rename_default = git_config_rename(var, value);
return 0;
@@ -3371,6 +3378,7 @@ void diff_setup(struct diff_options *options)
options->rename_limit = -1;
options->dirstat_permille = diff_dirstat_permille_default;
options->context = diff_context_default;
+ options->interhunkcontext = diff_interhunk_context_default;
options->ws_error_highlight = ws_error_highlight_default;
DIFF_OPT_SET(options, RENAME_EMPTY);
--
2.11.0.1.gaa10c3f
^ permalink raw reply related
* Re: [PATCH 13/17] refs: convert each_reflog_ent_fn to struct object_id
From: Jeff King @ 2017-01-03 1:32 UTC (permalink / raw)
To: Michael Haggerty; +Cc: brian m. carlson, git discussion list
In-Reply-To: <d5fc2830-37c2-9274-77b7-97ecc5f9b763@alum.mit.edu>
On Tue, Jan 03, 2017 at 12:30:40AM +0100, Michael Haggerty wrote:
> > I think
> > my next series is going to include a small sscanf-style parser to parse
> > these. Right now, using constants here is going to leave it extremely
> > difficult to read. Something like the following for the OIDs:
> >
> > strbuf_git_scanf(sb, "%h %h ", &ooid, &noid);
> >
> > and then following up parsing the remainder.
>
> Maybe something with an interface like skip_prefix wouldn't be too
> obnoxious:
>
> const char *p = sb.buf;
> if (oid_prefix(p, &ooid, &p) &&
> *p++ == ' ' &&
> oid_prefix(p, &noid, &p) && ...
Yeah, I've used C code before that had a very similar interface for
parsing, and when used consistently it's pretty pleasant. Something
like:
if (parse_oid(p, &oid, &p) &&
skip_whitespace(p, &p) &&
parse_refname(p, &refname, &p))
etc is nicer than some of the magic numbers we end up using in the
various parsers (I don't think anybody needs to mass-convert, I just
mean that something like parse_oid() seems like a step in a good
direction).
-Peff
^ permalink raw reply
* Re: [PATCH] diff: add interhunk context config option
From: Pranit Bauva @ 2017-01-03 6:19 UTC (permalink / raw)
To: Vegard Nossum; +Cc: Junio C Hamano, Git List, René Scharfe
In-Reply-To: <20170102233114.20778-1-vegard.nossum@oracle.com>
Hey Vegard,
On Tue, Jan 3, 2017 at 5:01 AM, Vegard Nossum <vegard.nossum@oracle.com> wrote:
> The --inter-hunk-context= option was added in commit 6d0e674a5754
> ("diff: add option to show context between close hunks"). This patch
> allows configuring a default for this option.
>
> Cc: René Scharfe <l.s.r@web.de>
> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
> ---
> Documentation/diff-options.txt | 2 ++
> diff.c | 8 ++++++++
> 2 files changed, 10 insertions(+)
>
> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
> index e6215c372..163b65444 100644
> --- a/Documentation/diff-options.txt
> +++ b/Documentation/diff-options.txt
> @@ -511,6 +511,8 @@ endif::git-format-patch[]
> --inter-hunk-context=<lines>::
> Show the context between diff hunks, up to the specified number
> of lines, thereby fusing hunks that are close to each other.
> + Defaults to `diff.interhunkcontext` or 0 if the config option
> + is unset.
Seems good. But I think you have missed adding the documentation of
this to Documentation/diff-config.txt . Generally whatever config
variable one adds, is added to Documentation/config.txt but in this
case, there exists a separate Documentation/diff-config.txt for all
"diff" related things which is included in Documentation/config.txt .
Also, you need to link the link both the parts of this documentation.
Please refer to commit id aaab84203 so as to know what I am talking
about.
> -W::
> --function-context::
> diff --git a/diff.c b/diff.c
> index 84dba60c4..40b4e6afe 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -33,6 +33,7 @@ static int diff_rename_limit_default = 400;
> static int diff_suppress_blank_empty;
> static int diff_use_color_default = -1;
> static int diff_context_default = 3;
> +static int diff_interhunk_context_default = 0;
> static const char *diff_word_regex_cfg;
> static const char *external_diff_cmd_cfg;
> static const char *diff_order_file_cfg;
> @@ -248,6 +249,12 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)
> return -1;
> return 0;
> }
> + if (!strcmp(var, "diff.interhunkcontext")) {
> + diff_interhunk_context_default = git_config_int(var, value);
> + if (diff_interhunk_context_default < 0)
> + return -1;
> + return 0;
> + }
> if (!strcmp(var, "diff.renames")) {
> diff_detect_rename_default = git_config_rename(var, value);
> return 0;
> @@ -3371,6 +3378,7 @@ void diff_setup(struct diff_options *options)
> options->rename_limit = -1;
> options->dirstat_permille = diff_dirstat_permille_default;
> options->context = diff_context_default;
> + options->interhunkcontext = diff_interhunk_context_default;
> options->ws_error_highlight = ws_error_highlight_default;
> DIFF_OPT_SET(options, RENAME_EMPTY);
On a first look, it seems that we can overwrite the default config
values by using a different command line argument which is good.
Also, tests are missing. It seems that t/t4032 might be a good place
to add those tests.
Rest all is quite good! :)
Regards,
Pranit Bauva
^ permalink raw reply
* Re: [PATCH v3 02/16] dir: remove struct path_simplify
From: Duy Nguyen @ 2017-01-03 12:02 UTC (permalink / raw)
To: Brandon Williams; +Cc: Git Mailing List, Stefan Beller, Junio C Hamano
In-Reply-To: <1481670870-66754-3-git-send-email-bmwill@google.com>
On Wed, Dec 14, 2016 at 6:14 AM, Brandon Williams <bmwill@google.com> wrote:
> @@ -2010,14 +1987,11 @@ static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *d
> return root;
> }
>
> -int read_directory(struct dir_struct *dir, const char *path, int len, const struct pathspec *pathspec)
> +int read_directory(struct dir_struct *dir, const char *path,
> + int len, const struct pathspec *pathspec)
> {
> - struct path_simplify *simplify;
> struct untracked_cache_dir *untracked;
>
> - /*
> - * Check out create_simplify()
> - */
> if (pathspec)
> GUARD_PATHSPEC(pathspec,
> PATHSPEC_FROMTOP |
This GUARD_PATHSPEC macro should be moved into simplify_away() and
exclude_pathspec_matches(), so that next time somebody adds a new
pathspec magic, they can basically grep GUARD_PATHSPEC and determine
if these code can support their favorite magic or not.
--
Duy
^ permalink raw reply
* Re: [PATCH v3 06/16] pathspec: copy and free owned memory
From: Duy Nguyen @ 2017-01-03 12:06 UTC (permalink / raw)
To: Brandon Williams; +Cc: Git Mailing List, Stefan Beller, Junio C Hamano
In-Reply-To: <1481670870-66754-7-git-send-email-bmwill@google.com>
On Wed, Dec 14, 2016 at 6:14 AM, Brandon Williams <bmwill@google.com> wrote:
> void clear_pathspec(struct pathspec *pathspec)
> {
> + int i;
> +
> + for (i = 0; i < pathspec->nr; i++) {
> + free(pathspec->items[i].match);
> + free(pathspec->items[i].original);
> + }
> free(pathspec->items);
> pathspec->items = NULL;
We should set pathspec->nr to zero so that calling this function again
won't cause any harm.
> }
--
Duy
^ 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