* [PATCH v2 1/1] difftool: add the builtin
From: Johannes Schindelin @ 2016-11-23 22:03 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, David Aguilar, Dennis Kaarsemaker
In-Reply-To: <cover.1479938494.git.johannes.schindelin@gmx.de>
This adds a builtin difftool that represents a conversion of the current
Perl script version of the difftool.
The motivation 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: the original difftool is now called `git legacy-difftool`, but to
play it safe, it is still called by difftool unless the config setting
core.useBuiltinDifftool=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>
---
.gitignore | 1 +
Makefile | 3 +-
builtin.h | 1 +
builtin/difftool.c | 692 ++++++++++++++++++++++++++
git-difftool.perl => git-legacy-difftool.perl | 0
git.c | 1 +
6 files changed, 697 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 05cb58a..f96e50e 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 f53fcc9..7863bc2 100644
--- a/Makefile
+++ b/Makefile
@@ -527,7 +527,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
@@ -888,6 +888,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 b9122bc..67f8051 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 0000000..f845879
--- /dev/null
+++ b/builtin/difftool.c
@@ -0,0 +1,692 @@
+/*
+ * "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 "cache.h"
+#include "builtin.h"
+#include "parse-options.h"
+#include "run-command.h"
+#include "argv-array.h"
+#include "strbuf.h"
+#include "lockfile.h"
+#include "dir.h"
+#include "exec_cmd.h"
+
+static char *diff_gui_tool;
+static int trust_exit_code;
+static int use_builtin_difftool;
+
+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;
+ }
+
+ if (!strcmp(var, "core.usebuiltindifftool")) {
+ use_builtin_difftool = 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 || p[1])
+ return error("unexpected trailer: '%s'", p);
+ 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 (!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);
+ }
+ strcpy(is_right ? e->right : e->left, content);
+}
+
+struct path_entry {
+ struct hashmap_entry entry;
+ char path[FLEX_ARRAY];
+};
+
+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,
+ 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;
+
+ setup_work_tree();
+ 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.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 */
+ 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;
+
+ 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);
+ 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);
+ 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;
+ }
+
+ /*
+ * 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, 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, NULL, env);
+ exit(ret);
+}
+
+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()
+ };
+
+ git_config(difftool_config, NULL);
+ symlinks = has_symlinks;
+ 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;
+ }
+
+ argc = parse_options(argc, argv, prefix, builtin_difftool_options,
+ builtin_difftool_usage, PARSE_OPT_KEEP_UNKNOWN |
+ PARSE_OPT_KEEP_DASHDASH);
+
+ 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, argc, argv);
+ return run_file_diff(prompt, argc, argv);
+}
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 efa1059..0e6bbee 100644
--- a/git.c
+++ b/git.c
@@ -424,6 +424,7 @@ 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 },
+ { "difftool", cmd_difftool, RUN_SETUP | NEED_WORK_TREE },
{ "fast-export", cmd_fast_export, RUN_SETUP },
{ "fetch", cmd_fetch, RUN_SETUP },
{ "fetch-pack", cmd_fetch_pack, RUN_SETUP },
--
2.10.1.583.g721a9e0
^ permalink raw reply related
* [PATCH v2 0/1] Show Git Mailing List: a builtin difftool
From: Johannes Schindelin @ 2016-11-23 22:03 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, David Aguilar, Dennis Kaarsemaker
In-Reply-To: <cover.1479834051.git.johannes.schindelin@gmx.de>
I have been working on the builtin difftool for almost two weeks,
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.
This patch serves two purposes: to ask for reviews, and to show what I
plan to release as part of Git for Windows v2.11.0 (which is due this
coming Wednesday, if Git v2.11.0 is released on Tuesday, as planned).
Changes since v1:
- fixed the usage (pointed out by David Aguilar)
- moved the stray #include "dir.h" to cuddle with the other #include's
(also pointed out by David Aguilar)
- changed the `sprintf()` call to a much safer `xsnprintf()` call
(again, pointed out by David Aguilar)
- changed an error message to include the offending path (need I say,
pointed out by David Aguilar?)
- used more restrictive permissions for the temporary directories (once
again, David Aguilar's suggestion)
- fixed a comment that lacked a space after a period (another fix thanks
to David Aguilar).
- switched the opt-in feature flag triggering the use of the builtin
difftool to a config variable (this suggestion came from Junio
Hamano).
- made difftool respect core.symlinks by moving the usage of
has_symlinks after the config was parsed.
Johannes Schindelin (1):
difftool: add the builtin
.gitignore | 1 +
Makefile | 3 +-
builtin.h | 1 +
builtin/difftool.c | 692 ++++++++++++++++++++++++++
git-difftool.perl => git-legacy-difftool.perl | 0
git.c | 1 +
6 files changed, 697 insertions(+), 1 deletion(-)
create mode 100644 builtin/difftool.c
rename git-difftool.perl => git-legacy-difftool.perl (100%)
base-commit: 1e37181391e305a7ab0c382ca3c3b2de998d4138
Published-As: https://github.com/dscho/git/releases/tag/builtin-difftool-v2
Fetch-It-Via: git fetch https://github.com/dscho/git builtin-difftool-v2
Interdiff vs v1:
diff --git a/.gitignore b/.gitignore
index 91bfd09..f96e50e 100644
--- a/.gitignore
+++ b/.gitignore
@@ -1,4 +1,3 @@
-/use-builtin-difftool
/GIT-BUILD-OPTIONS
/GIT-CFLAGS
/GIT-LDFLAGS
@@ -52,7 +51,6 @@
/git-diff-tree
/git-difftool
/git-difftool--helper
-/git-builtin-difftool
/git-describe
/git-fast-export
/git-fast-import
@@ -78,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 f764174..7863bc2 100644
--- a/Makefile
+++ b/Makefile
@@ -527,7 +527,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
@@ -888,7 +888,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/builtin-difftool.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 409a61e..67f8051 100644
--- a/builtin.h
+++ b/builtin.h
@@ -60,7 +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_builtin_difftool(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/builtin-difftool.c b/builtin/difftool.c
similarity index 95%
rename from builtin/builtin-difftool.c
rename to builtin/difftool.c
index 9feefcd..f845879 100644
--- a/builtin/builtin-difftool.c
+++ b/builtin/difftool.c
@@ -18,12 +18,15 @@
#include "argv-array.h"
#include "strbuf.h"
#include "lockfile.h"
+#include "dir.h"
+#include "exec_cmd.h"
static char *diff_gui_tool;
static int trust_exit_code;
+static int use_builtin_difftool;
-static const char * const builtin_difftool_usage[] = {
- N_("git add [<options>] [--] <pathspec>..."),
+static const char *const builtin_difftool_usage[] = {
+ N_("git difftool [<options>] [<commit> [<commit>]] [--] [<path>...]"),
NULL
};
@@ -39,6 +42,11 @@ static int difftool_config(const char *var, const char *value, void *cb)
return 0;
}
+ if (!strcmp(var, "core.usebuiltindifftool")) {
+ use_builtin_difftool = git_config_bool(var, value);
+ return 0;
+ }
+
return git_default_config(var, value, cb);
}
@@ -227,8 +235,6 @@ static void changed_files(struct hashmap *result, const char *index_path,
strbuf_release(&buf);
}
-#include "dir.h"
-
static NORETURN void exit_cleanup(const char *tmpdir, int exit_code)
{
struct strbuf buf = STRBUF_INIT;
@@ -282,16 +288,16 @@ static int run_dir_diff(const char *extcmd, int symlinks,
/* Setup temp directories */
tmp = getenv("TMPDIR");
- sprintf(tmpdir, "%s/git-difftool.XXXXXX", tmp ? tmp : "/tmp");
+ xsnprintf(tmpdir, sizeof(tmpdir), "%s/git-difftool.XXXXXX", tmp ? tmp : "/tmp");
if (!mkdtemp(tmpdir))
- return error("could not create temporary directory");
+ 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, 0777);
- mkdir(rdir.buf, 0777);
+ mkdir(ldir.buf, 0700);
+ mkdir(rdir.buf, 0700);
memset(&wtindex, 0, sizeof(wtindex));
@@ -606,12 +612,11 @@ static int run_file_diff(int prompt, int argc, const char **argv)
exit(ret);
}
-int cmd_builtin_difftool(int argc, const char ** argv, const char * prefix)
+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`")),
@@ -638,9 +643,16 @@ int cmd_builtin_difftool(int argc, const char ** argv, const char * prefix)
OPT_END()
};
+ git_config(difftool_config, NULL);
symlinks = has_symlinks;
+ if (!use_builtin_difftool) {
+ const char *path = mkpath("%s/git-legacy-difftool", git_exec_path());
- git_config(difftool_config, NULL);
+ if (sane_execvp(path, (char **)argv) < 0)
+ die_errno("could not exec %s", path);
+
+ return 0;
+ }
argc = parse_options(argc, argv, prefix, builtin_difftool_options,
builtin_difftool_usage, PARSE_OPT_KEEP_UNKNOWN |
@@ -670,7 +682,7 @@ int cmd_builtin_difftool(int argc, const char ** argv, const char * prefix)
/*
* In directory diff mode, 'git-difftool--helper' is called once
- * to compare the a / b directories.In file diff mode, 'git diff'
+ * 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.
*/
diff --git a/git-difftool.perl b/git-legacy-difftool.perl
similarity index 98%
rename from git-difftool.perl
rename to git-legacy-difftool.perl
index 28e47d8..a5790d0 100755
--- a/git-difftool.perl
+++ b/git-legacy-difftool.perl
@@ -23,13 +23,6 @@ use File::Temp qw(tempdir);
use Getopt::Long qw(:config pass_through);
use Git;
-if (-e Git::exec_path() . '/use-builtin-difftool') {
- unshift(@ARGV, "builtin-difftool");
- unshift(@ARGV, "git");
- exec(@ARGV);
- die("Could not execute builtin difftool");
-}
-
sub usage
{
my $exitcode = shift;
diff --git a/git.c b/git.c
index 7a0df7a..0e6bbee 100644
--- a/git.c
+++ b/git.c
@@ -2,7 +2,6 @@
#include "exec_cmd.h"
#include "help.h"
#include "run-command.h"
-#include "dir.h"
const char git_usage_string[] =
"git [--version] [--help] [-C <path>] [-c name=value]\n"
@@ -425,7 +424,7 @@ 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 },
- { "builtin-difftool", cmd_builtin_difftool, RUN_SETUP | NEED_WORK_TREE },
+ { "difftool", cmd_difftool, RUN_SETUP | NEED_WORK_TREE },
{ "fast-export", cmd_fast_export, RUN_SETUP },
{ "fetch", cmd_fetch, RUN_SETUP },
{ "fetch-pack", cmd_fetch_pack, RUN_SETUP },
@@ -543,22 +542,6 @@ static void strip_extension(const char **argv)
#define strip_extension(cmd)
#endif
-static int use_builtin_difftool(void)
-{
- static int initialized, use;
-
- if (!initialized) {
- struct strbuf buf = STRBUF_INIT;
- strbuf_addf(&buf, "%s/%s", git_exec_path(),
- "use-builtin-difftool");
- use = file_exists(buf.buf);
- strbuf_release(&buf);
- initialized = 1;
- }
-
- return use;
-}
-
static void handle_builtin(int argc, const char **argv)
{
struct argv_array args = ARGV_ARRAY_INIT;
@@ -568,9 +551,6 @@ static void handle_builtin(int argc, const char **argv)
strip_extension(argv);
cmd = argv[0];
- if (!strcmp("difftool", cmd) && use_builtin_difftool())
- cmd = "builtin-difftool";
-
/* Turn "git cmd --help" into "git help --exclude-guides cmd" */
if (argc > 1 && !strcmp(argv[1], "--help")) {
int i;
--
2.10.1.583.g721a9e0
^ permalink raw reply
* Re: [PATCH 2/2] difftool: add a feature flag for the builtin vs scripted version
From: Johannes Schindelin @ 2016-11-23 22:01 UTC (permalink / raw)
To: Dennis Kaarsemaker; +Cc: git, Junio C Hamano
In-Reply-To: <alpine.DEB.2.20.1611231824530.3746@virtualbox>
Hi Dennis,
On Wed, 23 Nov 2016, Johannes Schindelin wrote:
> On Wed, 23 Nov 2016, Dennis Kaarsemaker wrote:
>
> > On Tue, 2016-11-22 at 18:01 +0100, Johannes Schindelin wrote:
> > > The original idea was to use an environment variable
> > > GIT_USE_BUILTIN_DIFFTOOL, but the test suite resets those variables, and
> > > we do want to use that feature flag to run the tests with, and without,
> > > the feature flag.
> > >
> > > Besides, the plan is to add an opt-in flag in Git for Windows'
> > > installer. If we implemented the feature flag as an environment
> > > variable, we would have to modify the user's environment, in order to
> > > make the builtin difftool the default when called from Git Bash, Git CMD
> > > or third-party tools.
> >
> > Why is this not a normal configuration variable (as in git config
> > difftool.builtin true or something)? It doesn't make much sense to me
> > to introduce a way of configuring git by introducing magic files, when
> > a normal configuration variable would do just fine, and the GfW
> > installer can also set such variables, like it does for the crlf config
> > I believe.
>
> I considered that. Adding a config setting would mean we simply test for
> it in git-difftool.perl and call the builtin if the setting is active,
> right?
>
> The downside is that we actually *do* go through Perl to do that. Only to
> go back to a builtin. Which is exactly the thing I intended to avoid.
Okay, I reconsidered. Junio's comment about how git-am did it made me
rethink the issue: I need not keep the name "difftool" for the script. So
what I do now is rename the Perl script to git-legacy-difftool and always
read the config in the builtin difftool, handing off to the legacy
difftool unless core.useBuiltinDifftool=true.
This is an easy way to do it, and a portable and clean blueprint for
similar feature-flags in the future.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH] git-gui: pass the branch name to git merge
From: Junio C Hamano @ 2016-11-23 20:05 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Pat Thoyts, René Scharfe, Git Mailing List
In-Reply-To: <5baaf25b-6f15-8002-97ea-97c5c6a4b4e4@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
> Am 22.11.2016 um 21:40 schrieb Johannes Sixt:
>> Am 22.11.2016 um 20:16 schrieb Junio C Hamano:
>>> Can't this be handled on the "git merge FETCH_HEAD" codepath
>>> instead?
>>
>> Absolutely. Any takers? ;)
>
> I attempted to fix git merge FETCH_HEAD, but I do not see a trivial
> solution.
>
> But on second thought, we have an excuse to pick my proposed git-gui
> change anyway: Without that change and a fix in git-merge only, there
> is still a regression for all users who use the latest git-gui but
> some git version between 2.5.0 and the fixed git-merge...
I'll leave it up to Pat, as I do not read tcl very well ;-)
^ permalink raw reply
* Re: [PATCH 2/2] difftool: add a feature flag for the builtin vs scripted version
From: Junio C Hamano @ 2016-11-23 20:04 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Dennis Kaarsemaker, git
In-Reply-To: <alpine.DEB.2.20.1611232051420.3746@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Hi Junio,
>
> On Wed, 23 Nov 2016, Junio C Hamano wrote:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>> > Can't you route the control upon seeing "git difftool" to your
>> > experimental "C" difftool and check the configuration there? Then
>> > you can decide to run_command() a non-builtin one depending what the
>> > configuration says---that way, you would incur cost of spawning Perl
>> > only when you need it, no?
>>
>> FWIW, the approach taken by 73c2779f42 ("builtin-am: implement
>> skeletal builtin am", 2015-08-04) is what I had in mind when I wrote
>> the above.
>
> Maybe that worked back then. But I doubt it, because checking out that
> revision, I get this "warning":
>
> Makefile:1732: warning: overriding recipe for target 'git-am'
> Makefile:1696: warning: ignoring old recipe for target 'git-am'
>
> Seems like a matter of luck whether the `make` executable you happen to
> use guesses what we want: to munge git-am.sh into git-am, as opposed to
> hard-linking git to git-am.
You do not need to keep two copies of "git-cmd", though. commands[]
table can have an entry "difftool" that points at cmd_difftool(),
which switches between run_command("difftool-scripted") or makes a
function call to a static difftool_builtin() that you wrote in 1/2.
^ permalink raw reply
* Re: [PATCH 2/2] difftool: add a feature flag for the builtin vs scripted version
From: Johannes Schindelin @ 2016-11-23 19:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Dennis Kaarsemaker, git
In-Reply-To: <xmqqd1hmm54f.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Wed, 23 Nov 2016, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Can't you route the control upon seeing "git difftool" to your
> > experimental "C" difftool and check the configuration there? Then
> > you can decide to run_command() a non-builtin one depending what the
> > configuration says---that way, you would incur cost of spawning Perl
> > only when you need it, no?
>
> FWIW, the approach taken by 73c2779f42 ("builtin-am: implement
> skeletal builtin am", 2015-08-04) is what I had in mind when I wrote
> the above.
Maybe that worked back then. But I doubt it, because checking out that
revision, I get this "warning":
Makefile:1732: warning: overriding recipe for target 'git-am'
Makefile:1696: warning: ignoring old recipe for target 'git-am'
Seems like a matter of luck whether the `make` executable you happen to
use guesses what we want: to munge git-am.sh into git-am, as opposed to
hard-linking git to git-am.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH] git-gui: pass the branch name to git merge
From: Johannes Sixt @ 2016-11-23 19:23 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Pat Thoyts, René Scharfe, Git Mailing List
In-Reply-To: <1dc28731-9000-c3bf-fbed-0cb17c230d8b@kdbg.org>
Am 22.11.2016 um 21:40 schrieb Johannes Sixt:
> Am 22.11.2016 um 20:16 schrieb Junio C Hamano:
>> Can't this be handled on the "git merge FETCH_HEAD" codepath
>> instead?
>
> Absolutely. Any takers? ;)
I attempted to fix git merge FETCH_HEAD, but I do not see a trivial
solution.
But on second thought, we have an excuse to pick my proposed git-gui
change anyway: Without that change and a fix in git-merge only, there is
still a regression for all users who use the latest git-gui but some git
version between 2.5.0 and the fixed git-merge...
-- Hannes
^ permalink raw reply
* Re: [PATCH 2/2] difftool: add a feature flag for the builtin vs scripted version
From: Junio C Hamano @ 2016-11-23 18:18 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Dennis Kaarsemaker, git
In-Reply-To: <xmqqh96ym6x6.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Can't you route the control upon seeing "git difftool" to your
> experimental "C" difftool and check the configuration there? Then
> you can decide to run_command() a non-builtin one depending what the
> configuration says---that way, you would incur cost of spawning Perl
> only when you need it, no?
FWIW, the approach taken by 73c2779f42 ("builtin-am: implement
skeletal builtin am", 2015-08-04) is what I had in mind when I wrote
the above.
^ permalink raw reply
* Re: [PATCH 2/2] difftool: add a feature flag for the builtin vs scripted version
From: Junio C Hamano @ 2016-11-23 17:40 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Dennis Kaarsemaker, git
In-Reply-To: <alpine.DEB.2.20.1611231824530.3746@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> The downside is that we actually *do* go through Perl to do that. Only to
> go back to a builtin. Which is exactly the thing I intended to avoid.
>
> If we do not go through Perl, we have to set up the git directory and
> parse the config in git.c *just* to figure out whether we want to
> magically forward difftool to builtin-difftool. That is not only ugly, but
> has potential side effects I was not willing to risk.
I won't accept the latter anyway, so do not worry ;-)
> In any case, this feature flag will be there only for one or two Git for
> Windows releases, to give early adopters a chance to send me bug reports
> about any regressions.
I think that is sensible. I suspect that for early detection of
breakages, you do not need to invent and force people to use a
completely new "mechanism" to switch between two implementations.
Can't you route the control upon seeing "git difftool" to your
experimental "C" difftool and check the configuration there? Then
you can decide to run_command() a non-builtin one depending what the
configuration says---that way, you would incur cost of spawning Perl
only when you need it, no?
^ permalink raw reply
* Re: [PATCH v1 15/19] config: add git_config_get_date_string() from gc.c
From: Junio C Hamano @ 2016-11-23 17:34 UTC (permalink / raw)
To: Christian Couder
Cc: git, Nguyen Thai Ngoc Duy, Ævar Arnfjörð Bjarmason,
Christian Couder
In-Reply-To: <CAP8UFD0iToVxU+maNL9BFXacp3sER+AfrqAnQXWf7EAwURKmdQ@mail.gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
> Ok it will appear like this in cache.h:
>
> /* This dies if the configured or default date is in the future */
> extern int git_config_get_expire_date_string(const char *key, const
> char **output);
Those who imitate existing callsites never read comments, and you
need to spend effort to get the name right to protect the codebase
from them.
"get-expiry" may be shorter. Neither still does not say it will
die, though.
^ permalink raw reply
* Re: [PATCH 2/2] difftool: add a feature flag for the builtin vs scripted version
From: Johannes Schindelin @ 2016-11-23 17:29 UTC (permalink / raw)
To: Dennis Kaarsemaker; +Cc: git, Junio C Hamano
In-Reply-To: <1479912693.5181.27.camel@kaarsemaker.net>
Hi Dennis,
On Wed, 23 Nov 2016, Dennis Kaarsemaker wrote:
> On Tue, 2016-11-22 at 18:01 +0100, Johannes Schindelin wrote:
> > The original idea was to use an environment variable
> > GIT_USE_BUILTIN_DIFFTOOL, but the test suite resets those variables, and
> > we do want to use that feature flag to run the tests with, and without,
> > the feature flag.
> >
> > Besides, the plan is to add an opt-in flag in Git for Windows'
> > installer. If we implemented the feature flag as an environment
> > variable, we would have to modify the user's environment, in order to
> > make the builtin difftool the default when called from Git Bash, Git CMD
> > or third-party tools.
>
> Why is this not a normal configuration variable (as in git config
> difftool.builtin true or something)? It doesn't make much sense to me
> to introduce a way of configuring git by introducing magic files, when
> a normal configuration variable would do just fine, and the GfW
> installer can also set such variables, like it does for the crlf config
> I believe.
I considered that. Adding a config setting would mean we simply test for
it in git-difftool.perl and call the builtin if the setting is active,
right?
The downside is that we actually *do* go through Perl to do that. Only to
go back to a builtin. Which is exactly the thing I intended to avoid.
If we do not go through Perl, we have to set up the git directory and
parse the config in git.c *just* to figure out whether we want to
magically forward difftool to builtin-difftool. That is not only ugly, but
has potential side effects I was not willing to risk.
In any case, this feature flag will be there only for one or two Git for
Windows releases, to give early adopters a chance to send me bug reports
about any regressions.
To be crystal-clear: I never expected this patch to enter git.git.
In that light, I am okay with taking the heat for introducing a temporary,
Git for Windows-only feature flag that is implemented as a "does the file
<xyz> exist?" test.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH] merge-recursive.c: use QSORT macro
From: Junio C Hamano @ 2016-11-23 17:21 UTC (permalink / raw)
To: Jeff King; +Cc: Nguyễn Thái Ngọc Duy, git, René Scharfe
In-Reply-To: <20161122174946.jy5at4g7rifu3und@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Another possibility is:
>
> df_sorted_entries.cmp = string_list_df_name_compare;
> string_list_sort(&df_sorted_entries);
>
> It's not any shorter, but maybe it's conceptually simpler.
My first reaction to Duy's patch was: it is moronic for the
string-list API not to offer "I've done _append() to add many items
while avoiding the overhead of doing insertion sort all the time;
now I finished adding and I want the result sorted".
And then I looked at string-list.h and there it was ;-)
^ permalink raw reply
* Re: [PATCH 3/3] worktree list: keep the list sorted
From: Junio C Hamano @ 2016-11-23 17:16 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, rappazzo
In-Reply-To: <20161122100046.8341-4-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> + for (i = nr = 0; worktrees[i]; i++)
> + nr++;
> +
> + /*
> + * don't sort the first item (main worktree), which will
> + * always be the first
> + */
> + QSORT(worktrees + 1, nr - 1, compare_worktree);
> +
This is somewhat curious.
for (i = 0; worktrees[i]; i++)
; /* just counting */
QSORT(worktrees + 1, i - 1, compare_worktree);
would have been a lot more idiomatic (you do not use nr after sorting).
More importantly, perhaps get_worktrees() should learn to take an
optional pointer to int that returns how many items are in the list?
Alternatively, other existing callers of the function do not care
about the order, so it may not be such a good idea to always sort
the result, but it may be a good idea to teach it to take a boolean
that signals that the list should be sorted in a "natural order",
which is how "worktree list" would show them to the user?
This should be easily protectable with a new test. Please do.
^ permalink raw reply
* Re: [PATCH 2/3] get_worktrees() must return main worktree as first item even on error
From: Junio C Hamano @ 2016-11-23 17:06 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, rappazzo
In-Reply-To: <20161122100046.8341-3-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 5c4854d..b835b91 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -388,7 +388,7 @@ static void show_worktree_porcelain(struct worktree *wt)
> printf("HEAD %s\n", sha1_to_hex(wt->head_sha1));
> if (wt->is_detached)
> printf("detached\n");
> - else
> + else if (wt->head_ref)
> printf("branch %s\n", wt->head_ref);
This change looks somewhat unrelated to what the title and the log
message claims to do, but the fix is to indicate an error condition
by leaving wt->head_ref as NULL, so this is a necessary adjustment.
Good.
> @@ -406,10 +406,12 @@ static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len)
> else {
> strbuf_addf(&sb, "%-*s ", abbrev_len,
> find_unique_abbrev(wt->head_sha1, DEFAULT_ABBREV));
> - if (!wt->is_detached)
> + if (wt->is_detached)
> + strbuf_addstr(&sb, "(detached HEAD)");
> + else if (wt->head_ref)
> strbuf_addf(&sb, "[%s]", shorten_unambiguous_ref(wt->head_ref, 0));
> else
> - strbuf_addstr(&sb, "(detached HEAD)");
> + strbuf_addstr(&sb, "(error)");
> }
Likewise.
> diff --git a/worktree.c b/worktree.c
> index f7c1b5e..a674efa 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -89,7 +89,7 @@ static struct worktree *get_main_worktree(void)
> strbuf_addf(&path, "%s/HEAD", get_git_common_dir());
>
> if (parse_ref(path.buf, &head_ref, &is_detached) < 0)
> - goto done;
> + strbuf_reset(&head_ref);
>
> worktree = xcalloc(1, sizeof(*worktree));
> worktree->path = strbuf_detach(&worktree_path, NULL);
> @@ -97,7 +97,6 @@ static struct worktree *get_main_worktree(void)
> worktree->is_detached = is_detached;
> add_head_info(&head_ref, worktree);
OK. The earlier call to _reset() and add_head_info() function
itself may want to be clarified that a zero-length strbuf signals an
error condition with additional comment. It is all too unclear in
the code with this patch as it stands.
> -done:
> strbuf_release(&path);
> strbuf_release(&worktree_path);
> strbuf_release(&head_ref);
After this there is "return worktree" which used to return NULL
because of the "goto", but we never return NULL from the function
after this change, which is the whole point of this change. Good.
> @@ -173,8 +172,7 @@ struct worktree **get_worktrees(void)
>
> list = xmalloc(alloc * sizeof(struct worktree *));
>
> - if ((list[counter] = get_main_worktree()))
> - counter++;
> + list[counter++] = get_main_worktree();
Hence the conditional, while it does not hurt, becomes unnecessary
and we can unconditionally throw the primary one to the list.
Good.
Other than the "these need in-code commenting", and also that this
should have a new test, the patch makes sense to me.
Thanks.
^ permalink raw reply
* Re: [PATCH 1/3] worktree.c: zero new 'struct worktree' on allocation
From: Junio C Hamano @ 2016-11-23 16:52 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, rappazzo
In-Reply-To: <20161122100046.8341-2-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> This keeps things a bit simpler when we add more fields, knowing that
> default values are always zero.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
Looks sensible. Thanks.
> worktree.c | 14 ++------------
> 1 file changed, 2 insertions(+), 12 deletions(-)
>
> diff --git a/worktree.c b/worktree.c
> index f7869f8..f7c1b5e 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -91,16 +91,11 @@ static struct worktree *get_main_worktree(void)
> if (parse_ref(path.buf, &head_ref, &is_detached) < 0)
> goto done;
>
> - worktree = xmalloc(sizeof(struct worktree));
> + worktree = xcalloc(1, sizeof(*worktree));
> worktree->path = strbuf_detach(&worktree_path, NULL);
> - worktree->id = NULL;
> worktree->is_bare = is_bare;
> - worktree->head_ref = NULL;
> worktree->is_detached = is_detached;
> - worktree->is_current = 0;
> add_head_info(&head_ref, worktree);
> - worktree->lock_reason = NULL;
> - worktree->lock_reason_valid = 0;
>
> done:
> strbuf_release(&path);
> @@ -138,16 +133,11 @@ static struct worktree *get_linked_worktree(const char *id)
> if (parse_ref(path.buf, &head_ref, &is_detached) < 0)
> goto done;
>
> - worktree = xmalloc(sizeof(struct worktree));
> + worktree = xcalloc(1, sizeof(*worktree));
> worktree->path = strbuf_detach(&worktree_path, NULL);
> worktree->id = xstrdup(id);
> - worktree->is_bare = 0;
> - worktree->head_ref = NULL;
> worktree->is_detached = is_detached;
> - worktree->is_current = 0;
> add_head_info(&head_ref, worktree);
> - worktree->lock_reason = NULL;
> - worktree->lock_reason_valid = 0;
>
> done:
> strbuf_release(&path);
^ permalink raw reply
* Re: [PATCH 0/3] Minor fixes on 'git worktree'
From: Junio C Hamano @ 2016-11-23 16:52 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, rappazzo
In-Reply-To: <20161122100046.8341-1-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> This fixes two things:
>
> - make sure the first item is always the main worktree even if we
> fail to retrieve some info
>
> - keep 'worktree list' order stable (which in turn fixes the random
> failure on my 'worktree-move' series
> Nguyễn Thái Ngọc Duy (3):
> worktree.c: zero new 'struct worktree' on allocation
> get_worktrees() must return main worktree as first item even on error
> worktree list: keep the list sorted
>
> builtin/worktree.c | 26 ++++++++++++++++++++++----
> worktree.c | 20 ++++----------------
> 2 files changed, 26 insertions(+), 20 deletions(-)
Any tests?
^ permalink raw reply
* Re: [PATCH v2 2/2] push: fix --dry-run to not push submodules
From: Junio C Hamano @ 2016-11-23 16:51 UTC (permalink / raw)
To: Brandon Williams
Cc: Stefan Beller, git@vger.kernel.org, Heiko Voigt, Johannes Sixt
In-Reply-To: <20161122181839.GF149321@google.com>
Brandon Williams <bmwill@google.com> writes:
> On 11/22, Junio C Hamano wrote:
>> Brandon Williams <bmwill@google.com> writes:
>>
>> > On 11/17, Stefan Beller wrote:
>> >> On Thu, Nov 17, 2016 at 10:46 AM, Brandon Williams <bmwill@google.com> wrote:
>> >>
>> >> > sha1_array_clear(&commits);
>> >> > - die("Failed to push all needed submodules!");
>> >> > + die ("Failed to push all needed submodules!");
>> >>
>> >> huh? Is this a whitespace change?
>> >
>> > That's odd...I didn't mean to add that lone space.
>>
>> Is that the only glitch in this round? IOW, is the series OK to be
>> picked up as long as I treak this out while queuing?
>
> It looks that way. And I did fix this in my local series. Let me know
> if you would rather I resend the series. Otherwise I think it looks
> good.
OK, queued with trivial fix for now.
Thanks.
^ permalink raw reply
* Re: [PATCH v1 15/19] config: add git_config_get_date_string() from gc.c
From: Christian Couder @ 2016-11-23 15:04 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Nguyen Thai Ngoc Duy, Ævar Arnfjörð Bjarmason,
Christian Couder
In-Reply-To: <xmqqziljngod.fsf@gitster.mtv.corp.google.com>
On Tue, Nov 1, 2016 at 8:28 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <christian.couder@gmail.com> writes:
>
>> This function will be used in a following commit to get the expiration
>> time of the shared index files from the config, and it is generic
>> enough to be put in "config.c".
>
> Is it generic enough that a helper that sounds as if it can get any
> date string dies if it is given a future date? I somehow doubt it.
>
> At the minimum, it must be made clear that there is an artificial
> limitation that the current set of callers find useful in cache.h as
> a one-liner comment next to the added declaration. Then people with
> the same need (i.e. they want to reject future timestamps) can
> decide to use it, while others would stay away from it.
>
> If you can come up with a better word to use to encode that
> artificial limitation in its name, renaming it is even better.
Ok it will appear like this in cache.h:
/* This dies if the configured or default date is in the future */
extern int git_config_get_expire_date_string(const char *key, const
char **output);
Thanks,
Christian.
^ permalink raw reply
* Re: [PATCH 2/2] difftool: add a feature flag for the builtin vs scripted version
From: Dennis Kaarsemaker @ 2016-11-23 14:51 UTC (permalink / raw)
To: Johannes Schindelin, git; +Cc: Junio C Hamano
In-Reply-To: <598dcfdbeef4e15d2d439053a0423589182e5f30.1479834051.git.johannes.schindelin@gmx.de>
On Tue, 2016-11-22 at 18:01 +0100, Johannes Schindelin wrote:
> The original idea was to use an environment variable
> GIT_USE_BUILTIN_DIFFTOOL, but the test suite resets those variables, and
> we do want to use that feature flag to run the tests with, and without,
> the feature flag.
>
> Besides, the plan is to add an opt-in flag in Git for Windows'
> installer. If we implemented the feature flag as an environment
> variable, we would have to modify the user's environment, in order to
> make the builtin difftool the default when called from Git Bash, Git CMD
> or third-party tools.
Hi Johannes,
Why is this not a normal configuration variable (as in git config
difftool.builtin true or something)? It doesn't make much sense to me
to introduce a way of configuring git by introducing magic files, when
a normal configuration variable would do just fine, and the GfW
installer can also set such variables, like it does for the crlf config
I believe.
--
Dennis Kaarsemaker
http://www.kaarsemaker.net
^ permalink raw reply
* Re: dangling commits in worktree
From: Luc Van Oostenryck @ 2016-11-23 14:15 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List
In-Reply-To: <CACsJy8CPX3PDfhcaftDHy_U37rEACr7Q1gj_un4ALen45J9GZQ@mail.gmail.com>
On Wed, Nov 23, 2016 at 04:45:56PM +0700, Duy Nguyen wrote:
>
> It's a known issue that gc (and maybe some others that do rev-list
> --all, like fsck) "forgets" about some worktree's refs and you will
> see what you see.
Good. I just wanted to be sure it was a known problem.
Thanks for the info.
Luc
^ permalink raw reply
* Re: [PATCH 1/2] difftool: add the builtin
From: Johannes Schindelin @ 2016-11-23 11:34 UTC (permalink / raw)
To: David Aguilar; +Cc: git, Junio C Hamano
In-Reply-To: <20161123080850.GA23742@gmail.com>
Hi David,
On Wed, 23 Nov 2016, David Aguilar wrote:
> On Tue, Nov 22, 2016 at 06:01:23PM +0100, Johannes Schindelin wrote:
>
> > +static const char * const builtin_difftool_usage[] = {
> > + N_("git add [<options>] [--] <pathspec>..."),
> > + NULL
> > +};
>
> The usage should probably say "difftool" (or "builtin-difftool").
Ah, my dirty secret was spilled. I copy-edited this. *pours ashes over his
head*
> > [...]
> > +static void changed_files(struct hashmap *result, const char *index_path,
> > + const char *workdir)
> > +{
> > +[...]
> > +}
> > +
> > +#include "dir.h"
>
> Can this mid-file #include go to the top of the file?
Yep, thanks.
In case you are interested: You probably guessed it, it was left for a
later clean-up. I worked a bit over the last weeks on getting Git to build
in Visual Studio, to be able to benefit from its quite nice features (I
was always a fan of Visual Studio, long before I started working at
Microsoft). I used the conversion of the difftool as an excuse to make use
of this myself: I did the entire conversion in Visual Studio, reverting to
the old, tedious command-line driven workflow to fix the bugs identified
by t7800-difftool.sh.
> > +static int run_dir_diff(const char *extcmd, int symlinks,
> > + 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;
> > +
> > + setup_work_tree();
> > + workdir = get_git_work_tree();
> > +
> > + /* Setup temp directories */
> > + tmp = getenv("TMPDIR");
> > + sprintf(tmpdir, "%s/git-difftool.XXXXXX", tmp ? tmp : "/tmp");
>
> Maybe snprintf instead?
>
> getenv() won't return anything longer than PATH_MAX for most
> users, but users are weird.
True.
> > + if (!mkdtemp(tmpdir))
> > + return error("could not create temporary directory");
>
> Mention the tmpdir here?
Sure thing.
> > + 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, 0777);
> > + mkdir(rdir.buf, 0777);
>
> Seeing the perl mkpath() default 0777 spelled out this way
> makes me wonder whether 0700 would be safer.
>
> The mkdtemp() above is already using 0700 so it's ok, but it
> might be worth making it consistent (later, perhaps).
Ah, of course! I stupidly imitated other `mkdir()` calls elsewhere, but
they refer to directories within the Git worktree...
> > + /*
> > + * 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.
> > + */
>
> Missing space after "." in the comment above.
Yep. It was two spaces and I deleted one too many (we are so way past
actual print, where the two spaces may have made sense...).
> > + if (dir_diff)
> > + return run_dir_diff(extcmd, symlinks, argc, argv);
> > + return run_file_diff(prompt, argc, argv);
> > +}
> > diff --git a/git.c b/git.c
> > index efa1059..eaa0f67 100644
> > --- a/git.c
> > +++ b/git.c
> > @@ -424,6 +424,7 @@ 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 },
> > + { "builtin-difftool", cmd_builtin_difftool, RUN_SETUP | NEED_WORK_TREE },
> > { "fast-export", cmd_fast_export, RUN_SETUP },
> > { "fetch", cmd_fetch, RUN_SETUP },
> > { "fetch-pack", cmd_fetch_pack, RUN_SETUP },
>
> This isn't alphabetical anymore, but it actually is if you
> consider that the final plan is to change "builtin-difftool" to
> "difftool".
Exactly, that was my thinking.
> If we want to minimize that future diff we could name
> cmd_builtin_difftool() as cmd_difftool() for consistency now so
> that the future commit only needs to tweak the string here.
Yes!
For the record, this is a left-over from an impatient attempt at avoiding
problems with `make` overwriting the Perl version of `git difftool` by the
builtin version; I had originally assumed that a list of builtins was
generated from parsing git.c or builtin.h, but it turns out that the
BUILTIN_OBJS are actually responsible, i.e. the file name.
Fixed.
Thank you for your review!
Dscho
^ permalink raw reply
* Re: [PATCH 1/3] rebase -i: highlight problems with core.commentchar
From: Johannes Schindelin @ 2016-11-23 11:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqwpfvqwbq.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Tue, 22 Nov 2016, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > On Mon, 21 Nov 2016, Junio C Hamano wrote:
> >
> >> diff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh
> >> index 29e91d861c..c1f6411eb2 100755
> >> --- a/t/t0030-stripspace.sh
> >> +++ b/t/t0030-stripspace.sh
> >> @@ -432,6 +432,15 @@ test_expect_success '-c with changed comment char' '
> >> test_cmp expect actual
> >> '
> >>
> >> +test_expect_failure '-c with comment char defined in .git/config' '
> >> + test_config core.commentchar = &&
> >> + printf "= foo\n" >expect &&
> >> + printf "foo" | (
> >
> > Could I ask you to sneak in a \n here?
>
> The one before that _is_ about fixing incomplete line, but this and
> the one before this one are not, so I think we should fix them at
> the same time.
>
> But does it break anything to leave it as-is? If not, I'd prefer to
> leave this (and one before this one) for a later clean-up patch post
> release.
Fair enough.
As a matter of fact, we do not even need to change it later. If it ain't
broke, don't fix it.
Ciao,
Dscho
^ permalink raw reply
* Re: dangling commits in worktree
From: Duy Nguyen @ 2016-11-23 9:45 UTC (permalink / raw)
To: Van Oostenryck Luc; +Cc: Git Mailing List
In-Reply-To: <CAExDi1SYOuq7GJC69+5yDmzaw--vKMmmqv0Jsm80hU1L5phDUg@mail.gmail.com>
On Wed, Nov 23, 2016 at 7:52 AM, Van Oostenryck Luc
<luc.vanoostenryck@gmail.com> wrote:
> Hi,
>
> More or less by error I used the fsck command in a worktree and I had
> the surprised to see that it reported a lot of dangling commits while it was
> not supposed to have one.
> I quickly realized that it was the case only in the worktree, in the main dir
> things were OK. While experimenting a bit I also saw that git gc had not the
> same effect in a worktree than in the main tree (the pack was smaller, more
> files were left in objects/xx/ dirs), which is even more odd and a bit
> scary when thinking to the pruning.
>
> This seems like a bug to me and googling about it didn't returned anything.
It's a known issue that gc (and maybe some others that do rev-list
--all, like fsck) "forgets" about some worktree's refs and you will
see what you see. Work on it was postponed because the "refs"
subsystem was being refactored. I think I'm resuming it soon.
--
Duy
^ permalink raw reply
* Re: [PATCH] merge-recursive.c: use QSORT macro
From: Duy Nguyen @ 2016-11-23 9:43 UTC (permalink / raw)
To: Jeff King; +Cc: Git Mailing List, René Scharfe
In-Reply-To: <20161122174946.jy5at4g7rifu3und@sigill.intra.peff.net>
On Wed, Nov 23, 2016 at 12:49 AM, Jeff King <peff@peff.net> wrote:
> On Tue, Nov 22, 2016 at 07:30:19PM +0700, Nguyễn Thái Ngọc Duy wrote:
>
>> This is the follow up of rs/qsort series, merged in b8688ad (Merge
>> branch 'rs/qsort' - 2016-10-10), where coccinelle was used to do
>> automatic transformation.
>>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> ---
>> coccinelle missed this place, understandably, because it can't know
>> that
>>
>> sizeof(*entries->items)
>>
>> is the same as
>>
>> sizeof(*df_name_compare.items)
>>
>> without some semantic analysis.
>
> That made me wonder why "entries" is used at all. Does it point to the
> same struct? But no, df_name_compare is a string list we create with the
> same list of strings.
>
> Which is why...
>
>> - qsort(df_sorted_entries.items, entries->nr, sizeof(*entries->items),
>> + QSORT(df_sorted_entries.items, entries->nr,
>> string_list_df_name_compare);
>
> ...it's OK to use entries->nr here, and not df_sorted_entries.nr. It
> still seems a bit odd, though.
Argh.. I completely overlooked that entries->nr !
> Maybe it's worth making this:
>
> QSORT(df_sorted_entries.items, df_sorted_entries.nr,
> string_list_df_name_compare);
>
> while we're at it. Another possibility is:
>
> df_sorted_entries.cmp = string_list_df_name_compare;
> string_list_sort(&df_sorted_entries);
>
> It's not any shorter, but maybe it's conceptually simpler.
Agreed. Shall I re-roll with this?
--
Duy
^ permalink raw reply
* Re: [PATCH 31/35] pathspec: allow querying for attributes
From: Duy Nguyen @ 2016-11-23 9:38 UTC (permalink / raw)
To: Stefan Beller; +Cc: Junio C Hamano, Brandon Williams, Git Mailing List
In-Reply-To: <CAGZ79kYm4LfXK=1j-ayLawt+BojnkyM4h2RLQ=kfpPgMQbdBag@mail.gmail.com>
On Wed, Nov 23, 2016 at 12:26 AM, Stefan Beller <sbeller@google.com> wrote:
> On Tue, Nov 22, 2016 at 2:41 AM, Duy Nguyen <pclouds@gmail.com> wrote:
>> On Fri, Nov 11, 2016 at 3:34 AM, Stefan Beller <sbeller@google.com> wrote:
>>> @@ -139,7 +140,8 @@ static size_t common_prefix_len(const struct pathspec *pathspec)
>>> PATHSPEC_LITERAL |
>>> PATHSPEC_GLOB |
>>> PATHSPEC_ICASE |
>>> - PATHSPEC_EXCLUDE);
>>> + PATHSPEC_EXCLUDE |
>>> + PATHSPEC_ATTR);
>>
>> Hmm.. common_prefix_len() has always been a bit relaxing and can cover
>> more than needed. It's for early pruning. Exact pathspec matching
>> _will_ be done later anyway.
>>
>> Is that obvious?
>
> Yes it is.
> Not sure what your concern is, though.
None really. I was just thinking out loud and trying not to make
assumptions, because I know this code quite well and I don't know how
people see this code anymore :D So all is good then.
--
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