* [PATCH v4 1/2] for-each-repo: new command used for multi-repo operations
From: Lars Hjemli @ 2013-01-27 12:46 UTC (permalink / raw)
To: git; +Cc: Lars Hjemli
In-Reply-To: <1359290777-5483-1-git-send-email-hjemli@gmail.com>
When working with multiple, unrelated (or loosly related) git repos,
there is often a need to locate all repos with uncommitted work and
perform some action on them (say, commit and push). Before this patch,
such tasks would require manually visiting all repositories, running
`git status` within each one and then decide what to do next.
This mundane task can now be automated by e.g. `git for-each-repo --dirty
status`, which will find all non-bare git repositories below the current
directory (even nested ones), check if they are dirty (as defined by
`git diff --quiet && git diff --cached --quiet`), and for each dirty repo
print the path to the repo and then execute `git status` within the repo.
The command also honours the option '--clean' which restricts the set of
repos to those which '--dirty' would skip, and '-x' which is used to
execute non-git commands.
Finally, the command to execute within each repo is optional. If none is
given, git-for-each-repo will just print the path to each repo found. And
since the command supports -z, this can be used for more advanced scripting
needs.
Note: since git-for-each-repo can execute both git- and nongit commands, it
must cd into the worktree of each repository before executing the command.
It is then no need for the environment variables $GIT_WORK_TREE and $GIT_DIR
to be specified, so git-for-each-repo will instead unset these variables to
stop them from interfering with the executed commands.
Signed-off-by: Lars Hjemli <hjemli@gmail.com>
---
.gitignore | 1 +
Documentation/git-for-each-repo.txt | 71 +++++++++++++++++
Makefile | 1 +
builtin.h | 1 +
builtin/for-each-repo.c | 145 ++++++++++++++++++++++++++++++++++
git.c | 1 +
t/t6400-for-each-repo.sh | 150 ++++++++++++++++++++++++++++++++++++
7 files changed, 370 insertions(+)
create mode 100644 Documentation/git-for-each-repo.txt
create mode 100644 builtin/for-each-repo.c
create mode 100755 t/t6400-for-each-repo.sh
diff --git a/.gitignore b/.gitignore
index aa258a6..0c27981 100644
--- a/.gitignore
+++ b/.gitignore
@@ -56,6 +56,7 @@
/git-filter-branch
/git-fmt-merge-msg
/git-for-each-ref
+/git-for-each-repo
/git-format-patch
/git-fsck
/git-fsck-objects
diff --git a/Documentation/git-for-each-repo.txt b/Documentation/git-for-each-repo.txt
new file mode 100644
index 0000000..fb12b3f
--- /dev/null
+++ b/Documentation/git-for-each-repo.txt
@@ -0,0 +1,71 @@
+git-for-each-repo(1)
+====================
+
+NAME
+----
+git-for-each-repo - Execute a git command in multiple non-bare repositories
+
+SYNOPSIS
+--------
+[verse]
+'git for-each-repo' [-acdxz] [command]
+
+DESCRIPTION
+-----------
+The git-for-each-repo command is used to locate all non-bare git
+repositories within the current directory tree, and optionally
+execute a git command in each of the found repos.
+
+OPTIONS
+-------
+-a::
+--all::
+ Include both clean and dirty repositories (this is the default
+ behaviour of `git-for-each-repo`).
+
+-c::
+--clean::
+ Only include repositories with a clean worktree.
+
+-d::
+--dirty::
+ Only include repositories with a dirty worktree.
+
+-x::
+ Execute a genric (non-git) command in each repo.
+
+-z::
+ Terminate each path name with the NUL character.
+
+EXAMPLES
+--------
+
+Various ways to exploit this command::
++
+------------
+$ git for-each-repo <1>
+$ git for-each-repo fetch <2>
+$ git for-each-repo -d gui <3>
+$ git for-each-repo -c push <4>
+$ git for-each-repo -x du -sh <5>
+------------
++
+<1> Print the path to all repos found below the current directory.
+
+<2> Fetch updates from default remote in all repos.
+
+<3> Start linkgit:git-gui[1] in each repo containing uncommitted changes.
+
+<4> Push the current branch in each repo with no uncommited changes.
+
+<5> Print disk-usage for each repository.
+
+NOTES
+-----
+
+For the purpose of `git-for-each-repo`, a dirty worktree is defined as a
+worktree with uncommitted changes.
+
+GIT
+---
+Part of the linkgit:git[1] suite
diff --git a/Makefile b/Makefile
index a786d4c..8c42c17 100644
--- a/Makefile
+++ b/Makefile
@@ -870,6 +870,7 @@ BUILTIN_OBJS += builtin/fetch-pack.o
BUILTIN_OBJS += builtin/fetch.o
BUILTIN_OBJS += builtin/fmt-merge-msg.o
BUILTIN_OBJS += builtin/for-each-ref.o
+BUILTIN_OBJS += builtin/for-each-repo.o
BUILTIN_OBJS += builtin/fsck.o
BUILTIN_OBJS += builtin/gc.o
BUILTIN_OBJS += builtin/grep.o
diff --git a/builtin.h b/builtin.h
index 7e7bbd6..02fc712 100644
--- a/builtin.h
+++ b/builtin.h
@@ -73,6 +73,7 @@ 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);
extern int cmd_fmt_merge_msg(int argc, const char **argv, const char *prefix);
extern int cmd_for_each_ref(int argc, const char **argv, const char *prefix);
+extern int cmd_for_each_repo(int argc, const char **argv, const char *prefix);
extern int cmd_format_patch(int argc, const char **argv, const char *prefix);
extern int cmd_fsck(int argc, const char **argv, const char *prefix);
extern int cmd_gc(int argc, const char **argv, const char *prefix);
diff --git a/builtin/for-each-repo.c b/builtin/for-each-repo.c
new file mode 100644
index 0000000..9333ae0
--- /dev/null
+++ b/builtin/for-each-repo.c
@@ -0,0 +1,145 @@
+/*
+ * "git for-each-repo" builtin command.
+ *
+ * Copyright (c) 2013 Lars Hjemli <hjemli@gmail.com>
+ */
+#include "cache.h"
+#include "color.h"
+#include "quote.h"
+#include "builtin.h"
+#include "run-command.h"
+#include "parse-options.h"
+
+#define ALL 0
+#define DIRTY 1
+#define CLEAN 2
+
+static char *color = GIT_COLOR_NORMAL;
+static int eol = '\n';
+static int match;
+static int runopt = RUN_GIT_CMD;
+
+static const char * const builtin_foreachrepo_usage[] = {
+ N_("git for-each-repo [-acdxz] [cmd]"),
+ NULL
+};
+
+static struct option builtin_foreachrepo_options[] = {
+ OPT_SET_INT('a', "all", &match, N_("match both clean and dirty repositories"), ALL),
+ OPT_SET_INT('c', "clean", &match, N_("only show clean repositories"), CLEAN),
+ OPT_SET_INT('d', "dirty", &match, N_("only show dirty repositories"), DIRTY),
+ OPT_SET_INT('x', NULL, &runopt, N_("execute generic (non-git) command"), 0),
+ OPT_SET_INT('z', NULL, &eol, N_("terminate each repo path with NUL character"), 0),
+ OPT_END(),
+};
+
+static int get_repo_state(const char *dir)
+{
+ const char *diffidx[] = {"diff", "--quiet", "--cached", NULL};
+ const char *diffwd[] = {"diff", "--quiet", NULL};
+
+ if (run_command_v_opt_cd_env(diffidx, RUN_GIT_CMD, dir, NULL) != 0)
+ return DIRTY;
+ if (run_command_v_opt_cd_env(diffwd, RUN_GIT_CMD, dir, NULL) != 0)
+ return DIRTY;
+ return CLEAN;
+}
+
+static void print_repo_path(const char *path, unsigned pretty)
+{
+ if (path[0] == '.' && path[1] == '/')
+ path += 2;
+ if (pretty)
+ color_fprintf_ln(stdout, color, "[%s]", path);
+ else
+ write_name_quoted(path, stdout, eol);
+}
+
+static void handle_repo(struct strbuf *path, const char **argv)
+{
+ const char *gitdir;
+ int len;
+
+ len = path->len;
+ strbuf_addstr(path, ".git");
+ gitdir = resolve_gitdir(path->buf);
+ strbuf_setlen(path, len - 1);
+ if (!gitdir)
+ goto done;
+ if (match != ALL && match != get_repo_state(path->buf))
+ goto done;
+ print_repo_path(path->buf, *argv != NULL);
+ if (*argv)
+ run_command_v_opt_cd_env(argv, runopt, path->buf, NULL);
+done:
+ strbuf_addstr(path, "/");
+}
+
+static int walk(struct strbuf *path, int argc, const char **argv)
+{
+ DIR *dir;
+ struct dirent *ent;
+ struct stat st;
+ size_t len;
+ int has_dotgit = 0;
+ struct string_list list = STRING_LIST_INIT_DUP;
+ struct string_list_item *item;
+
+ dir = opendir(path->buf);
+ if (!dir)
+ return errno;
+ strbuf_addstr(path, "/");
+ len = path->len;
+ while ((ent = readdir(dir))) {
+ if (!strcmp(ent->d_name, ".") || !strcmp(ent->d_name, ".."))
+ continue;
+ if (!strcmp(ent->d_name, ".git")) {
+ has_dotgit = 1;
+ continue;
+ }
+ switch (DTYPE(ent)) {
+ case DT_UNKNOWN:
+ case DT_LNK:
+ /* Use stat() to figure out if this path leads
+ * to a directory - it's not important if it's
+ * a symlink which gets us there.
+ */
+ strbuf_setlen(path, len);
+ strbuf_addstr(path, ent->d_name);
+ if (stat(path->buf, &st) || !S_ISDIR(st.st_mode))
+ break;
+ /* fallthrough */
+ case DT_DIR:
+ string_list_append(&list, ent->d_name);
+ break;
+ }
+ }
+ closedir(dir);
+ strbuf_setlen(path, len);
+ if (has_dotgit)
+ handle_repo(path, argv);
+ sort_string_list(&list);
+ for_each_string_list_item(item, &list) {
+ strbuf_setlen(path, len);
+ strbuf_addstr(path, item->string);
+ walk(path, argc, argv);
+ }
+ string_list_clear(&list, 0);
+ return 0;
+}
+
+int cmd_for_each_repo(int argc, const char **argv, const char *prefix)
+{
+ struct strbuf path = STRBUF_INIT;
+
+ unsetenv(GIT_DIR_ENVIRONMENT);
+ unsetenv(GIT_WORK_TREE_ENVIRONMENT);
+ argc = parse_options(argc, argv, prefix,
+ builtin_foreachrepo_options,
+ builtin_foreachrepo_usage,
+ PARSE_OPT_STOP_AT_NON_OPTION);
+ if (want_color(GIT_COLOR_AUTO))
+ color = GIT_COLOR_YELLOW;
+ strbuf_addstr(&path, ".");
+ return walk(&path, argc, argv);
+}
diff --git a/git.c b/git.c
index ed66c66..6b53169 100644
--- a/git.c
+++ b/git.c
@@ -337,6 +337,7 @@ static void handle_internal_command(int argc, const char **argv)
{ "fetch-pack", cmd_fetch_pack, RUN_SETUP },
{ "fmt-merge-msg", cmd_fmt_merge_msg, RUN_SETUP },
{ "for-each-ref", cmd_for_each_ref, RUN_SETUP },
+ { "for-each-repo", cmd_for_each_repo },
{ "format-patch", cmd_format_patch, RUN_SETUP },
{ "fsck", cmd_fsck, RUN_SETUP },
{ "fsck-objects", cmd_fsck, RUN_SETUP },
diff --git a/t/t6400-for-each-repo.sh b/t/t6400-for-each-repo.sh
new file mode 100755
index 0000000..af02c0c
--- /dev/null
+++ b/t/t6400-for-each-repo.sh
@@ -0,0 +1,150 @@
+#!/bin/sh
+#
+# Copyright (c) 2013 Lars Hjemli
+#
+
+test_description='Test the git-for-each-repo command'
+
+. ./test-lib.sh
+
+qname="with\"quote"
+qqname="\"with\\\"quote\""
+
+test_expect_success "setup" '
+ test_create_repo clean &&
+ (cd clean && test_commit foo1) &&
+ git init --separate-git-dir=.cleansub clean/gitfile &&
+ (cd clean/gitfile && test_commit foo2 && echo bar >>foo2.t) &&
+ test_create_repo dirty-idx &&
+ (cd dirty-idx && test_commit foo3 && git rm foo3.t) &&
+ test_create_repo dirty-wt &&
+ (cd dirty-wt && mv .git .linkedgit && ln -s .linkedgit .git &&
+ test_commit foo4 && rm foo4.t) &&
+ test_create_repo "$qname" &&
+ (cd "$qname" && test_commit foo5) &&
+ mkdir fakedir && mkdir fakedir/.git
+'
+
+test_expect_success "without filtering, all repos are included" '
+ echo "." >expect &&
+ echo "clean" >>expect &&
+ echo "clean/gitfile" >>expect &&
+ echo "dirty-idx" >>expect &&
+ echo "dirty-wt" >>expect &&
+ echo "$qqname" >>expect &&
+ git for-each-repo >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success "-z NUL-terminates each path" '
+ echo "(.)" >expect &&
+ echo "(clean)" >>expect &&
+ echo "(clean/gitfile)" >>expect &&
+ echo "(dirty-idx)" >>expect &&
+ echo "(dirty-wt)" >>expect &&
+ echo "($qname)" >>expect &&
+ git for-each-repo -z | xargs -0 printf "(%s)\n" >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success "--dirty only includes dirty repos" '
+ echo "clean/gitfile" >expect &&
+ echo "dirty-idx" >>expect &&
+ echo "dirty-wt" >>expect &&
+ git for-each-repo --dirty >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success "--clean only includes clean repos" '
+ echo "." >expect &&
+ echo "clean" >>expect &&
+ echo "$qqname" >>expect &&
+ git for-each-repo --clean >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success "run a git-command in all repos" '
+ echo "[.]" >expect &&
+ echo "[clean]" >>expect &&
+ echo "[clean/gitfile]" >>expect &&
+ echo " M foo2.t" >>expect &&
+ echo "[dirty-idx]" >>expect &&
+ echo "D foo3.t" >>expect &&
+ echo "[dirty-wt]" >>expect &&
+ echo " D foo4.t" >> expect
+ echo "[$qname]" >>expect &&
+ git for-each-repo status -suno >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success "run a git-command in dirty repos only" '
+ echo "[clean/gitfile]" >expect &&
+ echo " M foo2.t" >>expect &&
+ echo "[dirty-idx]" >>expect &&
+ echo "D foo3.t" >>expect &&
+ echo "[dirty-wt]" >>expect &&
+ echo " D foo4.t" >> expect
+ git for-each-repo -d status -suno >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success "run a git-command in clean repos only" '
+ echo "[.]" >expect &&
+ echo "[clean]" >>expect &&
+ echo "foo1.t" >>expect &&
+ echo "[$qname]" >>expect &&
+ echo "foo5.t" >>expect &&
+ git for-each-repo -c ls-files >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success "-z is disabled when a command is run" '
+ echo "[.]" >expect &&
+ echo "[clean]" >>expect &&
+ echo "foo1.t" >>expect &&
+ echo "[$qname]" >>expect &&
+ echo "foo5.t" >>expect &&
+ git for-each-repo -cz ls-files >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success "-x executes any command in each repo" '
+ echo "[.]" >expect &&
+ echo "$HOME" >>expect &&
+ echo "[clean]" >>expect &&
+ echo "$HOME/clean" >>expect &&
+ echo "[clean/gitfile]" >>expect &&
+ echo "$HOME/clean/gitfile" >>expect &&
+ echo "[dirty-idx]" >>expect &&
+ echo "$HOME/dirty-idx" >>expect &&
+ echo "[dirty-wt]" >>expect &&
+ echo "$HOME/dirty-wt" >> expect
+ echo "[$qname]" >>expect &&
+ echo "$HOME/$qname" >>expect &&
+ git for-each-repo -x pwd >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success "-cx executes any command in clean repos" '
+ echo "[.]" >expect &&
+ echo "$HOME" >>expect &&
+ echo "[clean]" >>expect &&
+ echo "$HOME/clean" >>expect &&
+ echo "[$qname]" >>expect &&
+ echo "$HOME/$qname" >>expect &&
+ git for-each-repo -cx pwd >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success "-dx executes any command in dirty repos" '
+ echo "[clean/gitfile]" >expect &&
+ echo "$HOME/clean/gitfile" >>expect &&
+ echo "[dirty-idx]" >>expect &&
+ echo "$HOME/dirty-idx" >>expect &&
+ echo "[dirty-wt]" >>expect &&
+ echo "$HOME/dirty-wt" >> expect
+ git for-each-repo -dx pwd >actual &&
+ test_cmp expect actual
+'
+
+test_done
--
1.8.1.1.349.g4cdd23e
^ permalink raw reply related
* [PATCH v4 0/2] for-each-repo: new command for multi-repo operations
From: Lars Hjemli @ 2013-01-27 12:46 UTC (permalink / raw)
To: git; +Cc: Lars Hjemli
Changes since v3:
* option -x used to execute non-git commands
* option -z used to NUL-terminate paths
* write_name_quoted() used to print repo paths
* repos are handled in sorted order (as defined by strcmp(3)) to get
predictable output from the command
* unsetenv() reintroduced to avoid problems from GIT_DIR/WORK_TREE
* more tests
Lars Hjemli (2):
for-each-repo: new command used for multi-repo operations
git: rewrite `git -a` to become a git-for-each-repo command
.gitignore | 1 +
Documentation/git-for-each-repo.txt | 71 ++++++++++++
Makefile | 1 +
builtin.h | 1 +
builtin/for-each-repo.c | 145 ++++++++++++++++++++++++
git.c | 37 +++++++
t/t6400-for-each-repo.sh | 213 ++++++++++++++++++++++++++++++++++++
7 files changed, 469 insertions(+)
create mode 100644 Documentation/git-for-each-repo.txt
create mode 100644 builtin/for-each-repo.c
create mode 100755 t/t6400-for-each-repo.sh
--
1.8.1.1.349.g4cdd23e
^ permalink raw reply
* Re: [PATCH v2] add: warn when -u or -A is used without filepattern
From: Jonathan Nieder @ 2013-01-27 12:22 UTC (permalink / raw)
To: Matthieu Moy
Cc: git, gitster, Robin Rosenberg, Piotr Krukowiecki,
Eric James Michael Ritz, Tomas Carnecky
In-Reply-To: <1359110978-20054-1-git-send-email-Matthieu.Moy@imag.fr>
Hi Matthieu,
Matthieu Moy wrote:
> --- a/builtin/add.c
> +++ b/builtin/add.c
[...]
> @@ -392,8 +420,14 @@ int cmd_add(int argc, const char **argv, const char *prefix)
> die(_("-A and -u are mutually incompatible"));
> if (!show_only && ignore_missing)
> die(_("Option --ignore-missing can only be used together with --dry-run"));
> - if ((addremove || take_worktree_changes) && !argc) {
> + if (addremove)
> + option_with_implicit_dot = "--all";
> + if (take_worktree_changes)
> + option_with_implicit_dot = "--update";
I agree with Junio that these are most often spelled as "-A" and "-u".
> + if (option_with_implicit_dot && !argc) {
> static const char *here[2] = { ".", NULL };
> + if (prefix)
> + warn_pathless_add(option_with_implicit_dot);
For what it's worth, with or without s/--all/-A/ and s/--update/-u/,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Thanks. If someone wants to preserve the spelling of the option name
passed by the user, that can happen as a patch on top.
^ permalink raw reply
* Re: [PATCH v2 3/3] branch: mark more strings for translation
From: Jonathan Nieder @ 2013-01-27 11:55 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Junio C Hamano, Matthieu Moy
In-Reply-To: <1359118225-14356-3-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy wrote:
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -466,7 +466,7 @@ static void add_verbose_info(struct strbuf *out, struct ref_item *item,
> int verbose, int abbrev)
> {
> struct strbuf subject = STRBUF_INIT, stat = STRBUF_INIT;
> - const char *sub = " **** invalid ref ****";
> + const char *sub = _(" **** invalid ref ****");
This worried me for a second --- is it an actual message that gets
emitted, a placeholder used only in code, or some combination of
the two?
Luckily it really is just a message (or rather, a value for the commit
message column in the " f7c0c00 [ahead 58, behind 197] vcs-svn: drop
obj_pool.h" message).
For what it's worth, assuming this passes tests,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
^ permalink raw reply
* Re: [PATCH v2 2/3] branch: give a more helpful message on redundant arguments
From: Jonathan Nieder @ 2013-01-27 11:58 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Junio C Hamano, Matthieu Moy
In-Reply-To: <1359118225-14356-2-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy wrote:
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -852,14 +852,14 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
> const char *branch_name;
> struct strbuf branch_ref = STRBUF_INIT;
>
> - if (detached)
> - die("Cannot give description to detached HEAD");
> - if (!argc)
> + if (!argc) {
> + if (detached)
> + die("Cannot give description to detached HEAD");
Good catch. Shouldn't this bugfix be a separate patch, so it can also
be included in maint?
^ permalink raw reply
* Behavior of stash apply vs merge
From: Robin Rosenberg @ 2013-01-27 11:35 UTC (permalink / raw)
To: Git List
In-Reply-To: <1192924141.1697155.1359285809347.JavaMail.root@dewire.com>
Hi,
What good reason is it that 'git stash apply' gives hairy conflict markers, while 'git merge stash' does not. No renames involved.
-- robin
^ permalink raw reply
* Re: [PATCH 2/2] fetch-pack: avoid repeatedly re-scanning pack directory
From: Jonathan Nieder @ 2013-01-27 10:27 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano
In-Reply-To: <20130126224043.GB20849@sigill.intra.peff.net>
Hi,
Jeff King wrote:
> When we look up a sha1 object for reading, we first check
> packfiles, and then loose objects. If we still haven't found
> it, we re-scan the list of packfiles in `objects/pack`. This
> final step ensures that we can co-exist with a simultaneous
> repack process which creates a new pack and then prunes the
> old object.
I like the context above and what follows it, but I think you forgot
to mention what the patch actually does. :)
I guess it is:
However, in the first scan over refs in fetch-pack.c::everything_local,
this double-check of packfiles is not necessary since we are only
trying to get a rough estimate of the last time we fetched from this
remote repository in order to find good candidate common commits ---
a missed object would only result in a slightly slower fetch.
Avoid that slow second scan in the common case by guarding the object
lookup with has_sha1_file().
Sounds like it would not affect most fetches except by making them
a lot faster in the many-refs case, so for what it's worth I like it.
I had not read this codepath before. I'm left with a few questions:
* Why is 49bb805e ("Do not ask for objects known to be complete",
2005-10-19) trying to do? Are we hurting that in any way?
For the sake of an example, suppose in my stalled project I
maintain 20 topic branches against an unmoving mainline I do not
advertise and you regularly fetch from me. The cutoff is the
*newest* commit date of any of my topic branches you already have.
By declaring you have that topic branch you avoid a complicated
negotiation to discover that we both have the mainline. Is that
the goal?
* Is has_sha1_file() generally succeptible to the race against repack
you mentioned? How is that normally dealt with?
* Can a slow operation get confused if an object is incorporated into
a pack and then expelled again by two repacks in sequence?
Thanks,
Jonathan
^ permalink raw reply
* Re: [PATCH] tests: turn on test-lint-shell-syntax by default
From: Jonathan Nieder @ 2013-01-27 9:31 UTC (permalink / raw)
To: Torsten Bögershausen; +Cc: Junio C Hamano, git, kraai
In-Reply-To: <51037E5F.8090506@web.de>
Hi,
Torsten Bögershausen wrote:
> On 15.01.13 21:38, Junio C Hamano wrote:
>> Torsten Bögershausen <tboegi@web.de> writes:
>>> What do we think about something like this for fishing for which:
[...]
>>> +which () {
>>> + echo >&2 "which is not portable (please use type)"
>>> + exit 1
>>> +}
[...]
>> if (
>> which frotz &&
>> test $(frobonitz --version" -le 2.0
>> )
With the above definition of "which", the only sign of a mistake would
be some extra output to stderr (which is quelled when running tests in
the normal way). The "exit" is caught by the subshell and just makes
the "if" condition false.
That's not so terrible --- it could still dissuade new test authors
from using "which". The downside I'd worry about is that it provides
a false sense of security despite not catching problems like
write_script x <<-EOF &&
# Use "foo" if possible. Otherwise use "bar".
if which foo && test $(foo --version) -le 2.0
then
...
...
EOF
./x
That's not a great tradeoff relative to the impact of the problem
being solved.
Don't get me wrong. I really do want to see more static or dynamic
analysis of git's shell scripts in the future. I fear that for the
tradeoffs to make sense, though, the analysis needs to be more
sophisticated:
* A very common error in test scripts is leaving out the "&&"
connecting adjacent statements, which causes early errors
in a test assertion to be missed and tests to pass by mistake.
Unfortunately the grammar of the dialect of shell used in tests is
not regular enough to make this easily detectable using regexps.
* Another common mistake is using "cd" without entering a subshell.
Detecting this requires counting nested parentheses and noticing
when a parenthesis is quoted.
* Another common mistake is relying on the semantics of variable
assignments in front of function calls. Detecting this requires
recognizing which commands are function calls.
In the end the analysis that works best would probably involve a
full-fledged shell script parser. Something like "sparse", except for
shell command language.
Sorry I don't have more practical advice in the short term.
My two cents,
Jonathan
^ permalink raw reply
* Re: [PATCH 2/2] mergetools: Make tortoisemerge work with
From: Sven Strickroth @ 2013-01-27 9:14 UTC (permalink / raw)
To: git; +Cc: David Aguilar, Junio C Hamano, Sebastian Schuberth, Jeff King
In-Reply-To: <CAJDDKr6OhZOitTdDkHWnhVhdAis0U+95xUtaNn6nwkQ-k+bA+w@mail.gmail.com>
Am 26.01.2013 08:10 schrieb David Aguilar:
> These patches look correct (I do not have the tool to test)
> but I think we should fixup this commit message.
>
> How about something like...
>
> mergetools: Teach tortoisemerge about TortoiseGitMerge
>
> TortoiseGitMerge improved its syntax to allow for file paths
> with spaces. Detect when it is installed and prefer it over
> TortoiseMerge.
This message implies, that I have to combine two patches again?!
--
Best regards,
Sven Strickroth
PGP key id F5A9D4C4 @ any key-server
^ permalink raw reply
* Re: [PATCH v3 6/8] git-remote-testpy: hash bytes explicitly
From: Michael Haggerty @ 2013-01-27 8:41 UTC (permalink / raw)
To: Sverre Rabbelier; +Cc: Junio C Hamano, John Keeping, Git List
In-Reply-To: <CAGdFq_icLDEdJJKHZsht8bXpZzSNProLt3F_u=0en2rFBvxLKw@mail.gmail.com>
On 01/27/2013 06:30 AM, Sverre Rabbelier wrote:
> On Sat, Jan 26, 2013 at 8:44 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> So to handle all of the cases across Python versions as closely as
>> possible to the old 2.x code, it might be necessary to make the code
>> explicitly depend on the Python version number, like:
>
> Does this all go away if we restrict ourselves to python 2.6 and just
> use the b prefix?
repo.path ultimately comes from the command line, which means that it is
a bytestring under Python 2.x and a Unicode string under Python 3.x. It
does not come from a literal that could be changed to b"value". (Nor is
a six.b()-like function helpful, if that is what you meant; that is also
intended to wrap literal strings.)
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply
* [RFC v2] git-multimail: a replacement for post-receive-email
From: Michael Haggerty @ 2013-01-27 8:37 UTC (permalink / raw)
To: git discussion list
Cc: Andy Parkins, Sitaram Chamarty, Matthieu Moy, Junio C Hamano,
Marc Branchaud, Ævar Arnfjörð Bjarmason,
Chris Hiestand
A while ago, I submitted an RFC for adding a new email notification
script to "contrib" [1]. The reaction seemed favorable and it was
suggested that the new script should replace post-receive-email rather
than be added separately, ideally with some kind of migration support.
I've been working on this on and off since then and I think it is time
for another iteration. I think I have addressed most of the points
raised earlier, including a migration script and specific migration
instructions.
Review of main advantages of git-multimail over post-receive-email:
* Can (optionally) send a separate email for each new commit (in
addition to the emails for each reference change).
* More flexible configuration, including out-of-the-box support for
running under gitolite.
* Fixed algorithm for detecting "new" commits.
* More information in emails (e.g., commit log subject lines, telling
when a push discards old commits)j.
* Written in Python rather than shell. Easier to extend.
Summary of improvements since the first version:
* Rename the project from the cumbersome "post-receive-multimail.py" to
"git-multimail".
* Push the project into a subdirectory and break it into multiple files
(script, docs, etc).
* Vastly improve the documentation and separate it out of the script
into a README file.
* Add a migration script, migrate-mailhook-config, that converts a
post-receive-email configuration into a git-multimail configuration.
Document the migration procedure and differences between the two scripts
in README.migrate-from-post-receive-email.
* Store the configuration options in namespace "multimailhook.*" rather
than "hooks.*". (The post-receive-email script's use of a too-generic
top-level name was IMHO a bad idea, so fix it now.)
* Allow the feature of sending a separate email for each individual
commit to be turned off via a configuration option (to better support
post-receive-email migrants).
* Re-implement the feature of showing a short log of commits in
announcement emails, configurable via an option.
* Make it possible to import the main code as a Python module to allow
most customization to be done via Python code without the need to edit
the original file. (Note for existing users: the Environment API has
changed since the original RFC, but I will try to keep it stable from
now on.)
* Allow the config settings that define recipient lists to be multivalued.
* Added some testing infrastructure (though the tests are still very
limited).
* Add "Auto-Submitted" headers to emails (as implemented for
post-receive-email by Chris Hiestand).
* Add option to truncate email lines to a specified length (suggested by
Matthieu Moy). By default, this option is *on* and set to 500 characters.
* Add option to force the main part of the email body to be valid UTF-8,
with invalid characters turned into the Unicode replacement character,
U+FFFD. By default, this option is *on* (arguments for turning it off
by default are welcome).
The code is in its own GitHub repository:
https://github.com/mhagger/git-multimail
The script should work with any Python 2.x starting with 2.4, though I
haven't actually tested older Python versions. It does not yet support
Python 3.x.
If it is accepted for the git project, then I would prepare a patch that
drops the git-multimail project's "git-multimail" subdirectory into the
git project as contrib/hooks/git-multimail and optionally deletes the
old post-receive-email script. I am flexible about whether future
development should occur directly in the git project's repository or in
the git-multimail repo with occasional code drops to the git project. I
am also flexible about whether the rough little test scripts should be
included in the git project or kept separate.
It would be very helpful if people would test this script in their own
environments and give me feedback/bug reports. It is rather awkward to
simulate all of the possible environment scenarios myself.
Michael
[1] http://thread.gmane.org/gmane.comp.version-control.git/201433
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply
* [PATCH v2] Reduce false positive in check-non-portable-shell.pl
From: Torsten Bögershausen @ 2013-01-27 7:46 UTC (permalink / raw)
To: git; +Cc: tboegi
check-non-portable-shell.pl is using simple regular expressions to
find illegal shell syntax.
Improve the expressions and reduce the chance for false positves:
"sed -i" must be followed by 1..n whitespace and 1 non whitespace
"declare" must be followed by 1..n whitespace and 1 non whitespace
"echo -n" must be followed by 1..n whitespace and 1 non whitespace
"which": catch lines like "if which foo"
Signed-off-by: Torsten Bögershausen <tboegi@web.de>
---
t/check-non-portable-shell.pl | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
index 8b5a71d..d9ddcdf 100755
--- a/t/check-non-portable-shell.pl
+++ b/t/check-non-portable-shell.pl
@@ -16,10 +16,10 @@ sub err {
while (<>) {
chomp;
- /^\s*sed\s+-i/ and err 'sed -i is not portable';
- /^\s*echo\s+-n/ and err 'echo -n is not portable (please use printf)';
- /^\s*declare\s+/ and err 'arrays/declare not portable';
- /^\s*[^#]\s*which\s/ and err 'which is not portable (please use type)';
+ /^\s*sed\s+-i\s+\S/ and err 'sed -i is not portable';
+ /^\s*echo\s+-n\s+\S/ and err 'echo -n is not portable (please use printf)';
+ /^\s*declare\s+\S/ and err 'arrays/declare not portable';
+ /^\s*if\s+which\s+\S/ and err 'which is not portable (please use type)';
/test\s+[^=]*==/ and err '"test a == b" is not portable (please use =)';
# this resets our $. for each file
close ARGV if eof;
--
1.8.1.1
^ permalink raw reply related
* Re: [PATCH] tests: turn on test-lint-shell-syntax by default
From: Torsten Bögershausen @ 2013-01-27 7:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Torsten Bögershausen, Jonathan Nieder, git, kraai
In-Reply-To: <7v1ud71uys.fsf@alter.siamese.dyndns.org>
On 26.01.13 22:43, Junio C Hamano wrote:
> Torsten Bögershausen <tboegi@web.de> writes:
>
>> Do we really need "which" to detect if frotz is installed?
> I think we all know the answer to that question is no, but why is
> that a relevant question in the context of this discussion? One of
> us may be very confused.
>
> I thought the topic of this discussion was that, already knowing
> that "which" should never be used anywhere in our scripts, you are
> trying to devise a mechanical way to catch newcomers' attempts to
> use it in their changes, in order to prevent patches that add use of
> "which" to be sent for review to waste our time. I was illustrating
> that the approach to override "which" in a shell function for test
> scripts will not be a useful solution for that goal.
Yes, the diskussion went away.
I would rather see the check-non-portable-shell.pl enabled per default.
It looks as if the "which" command is hard to find, when we want a minimal
risk of false positves.
I can see different solutions:
1) We can make a much simpler expression which only catches the most
common usage of which, like
"if whitch foo".
This will not catch lines like
if test -x "$(which foo 2>/dev/null)"
But I think the -x is not a useful anyway, because which should only
list command which have the executable bit set.
2) We drop the which from check-non-portable-shell.pl
I'll send a patch for 1)
/Torsten
^ permalink raw reply
* Re: [PATCH 0/2] optimizing pack access on "read only" fetch repos
From: Junio C Hamano @ 2013-01-27 6:32 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20130126224011.GA20675@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> This is a repost from here:
>
> http://thread.gmane.org/gmane.comp.version-control.git/211176
>
> which got no response initially. Basically the issue is that read-only
> repos (e.g., a CI server) whose workflow is something like:
>
> git fetch $some_branch &&
> git checkout -f $some_branch &&
> make test
>
> will never run git-gc, and will accumulate a bunch of small packs and
> loose objects, leading to poor performance.
>
> Patch 1 runs "gc --auto" on fetch, which I think is sane to do.
>
> Patch 2 optimizes our pack dir re-scanning for fetch-pack (which, unlike
> the rest of git, should expect to be missing lots of objects, since we
> are deciding what to fetch).
>
> I think 1 is a no-brainer. If your repo is packed, patch 2 matters less,
> but it still seems like a sensible optimization to me.
>
> [1/2]: fetch: run gc --auto after fetching
> [2/2]: fetch-pack: avoid repeatedly re-scanning pack directory
>
> -Peff
Both makes sense to me.
I also wonder if we would be helped by another "repack" mode that
coalesces small packs into a single one with minimum overhead, and
run that often from "gc --auto", so that we do not end up having to
have 50 packfiles.
When we have 2 or more small and young packs, we could:
- iterate over idx files for these packs to enumerate the objects
to be packed, replacing read_object_list_from_stdin() step;
- always choose to copy the data we have in these existing packs,
instead of doing a full prepare_pack(); and
- use the order the objects appear in the original packs, bypassing
compute_write_order().
The procedure cannot be a straight byte-for-byte copy, because some
objects may appear in multiple packs, and extra copies of the same
object have to be excised from the result. OFS_DELTA offsets need
to be adjusted for objects that appear later in the output and for
objects that were deltified against such an object that recorded its
base with OFS_DELTA format.
But other than such OFS_DELTA adjustments, it feels that such an
"only coalesce multiple packs into one" mode should be fairly quick.
^ permalink raw reply
* Re: [PATCH] mergetools: Simplify how we handle "vim" and "defaults"
From: Junio C Hamano @ 2013-01-27 6:05 UTC (permalink / raw)
To: David Aguilar; +Cc: John Keeping, git
In-Reply-To: <CAJDDKr692zbg+PiFWx1y81yn=s2e=C0pFhsup4z0uTRNOTMPwg@mail.gmail.com>
David Aguilar <davvid@gmail.com> writes:
>> I think that's much better.
>
> Would you like me to put this together into a proper patch?
>
> You can also squash it in (along with a removal of the
> last line of the commit message) if you prefer.
I was lazy and didn't want to think about rewriting your log
message, so I just queued it with this log message on top.
mergetools: remove mergetools/include/defaults.sh
This and its containing directory are used only to define trivial
fallback definition of five shell functions in a single script.
Defining them in-line at the location that wants to have them makes
the result much easier to read.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
But as you said, removing the last line of your log message and
squashing the change into it would be more preferrable. Let me do
that later.
Thanks.
^ permalink raw reply
* Re: [PATCH] mergetools: Simplify how we handle "vim" and "defaults"
From: David Aguilar @ 2013-01-27 5:35 UTC (permalink / raw)
To: Junio C Hamano; +Cc: John Keeping, git
In-Reply-To: <CAJDDKr5cCbNi5q5_Ds-yohXR56ZfVs7YBTgJP3THjRx1=EgG9w@mail.gmail.com>
On Sat, Jan 26, 2013 at 9:07 PM, David Aguilar <davvid@gmail.com> wrote:
> On Sat, Jan 26, 2013 at 8:57 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> John Keeping <john@keeping.me.uk> writes:
>>
>>> I'm not sure creating an 'include' directory actually buys us much over
>>> declaring that 'vimdiff' is the real script and the others just source
>>> it.
>>
>> Is 'include' really used for such a purpose? It only houses defaults.sh
>> as far as I can see.
>>
>> As that defaults.sh file is used only to define trivially empty
>> shell functions, I wonder if it is better to get rid of it, and
>> define these functions in line in git-mergetool--lib.sh. Such a
>> change would like the attached on top of the entire series.
>
> I think that's much better.
Would you like me to put this together into a proper patch?
You can also squash it in (along with a removal of the
last line of the commit message) if you prefer.
>> Makefile | 6 +-----
>> git-mergetool--lib.sh | 24 ++++++++++++++++++++++--
>> mergetools/include/defaults.sh | 22 ----------------------
>> 3 files changed, 23 insertions(+), 29 deletions(-)
>>
>> diff --git a/Makefile b/Makefile
>> index 26f217f..f69979e 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -2724,11 +2724,7 @@ install: all
>> $(INSTALL) $(install_bindir_programs) '$(DESTDIR_SQ)$(bindir_SQ)'
>> $(MAKE) -C templates DESTDIR='$(DESTDIR_SQ)' install
>> $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'
>> - $(INSTALL) -m 644 $(filter-out mergetools/include,$(wildcard mergetools/*)) \
>> - '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'
>> - $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(mergetools_instdir_SQ)/include'
>> - $(INSTALL) -m 644 mergetools/include/* \
>> - '$(DESTDIR_SQ)$(mergetools_instdir_SQ)/include'
>> + $(INSTALL) -m 644 mergetools/* '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'
>> ifndef NO_GETTEXT
>> $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(localedir_SQ)'
>> (cd po/build/locale && $(TAR) cf - .) | \
>> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
>> index 7ea7510..1d0fb12 100644
>> --- a/git-mergetool--lib.sh
>> +++ b/git-mergetool--lib.sh
>> @@ -57,8 +57,28 @@ setup_tool () {
>> return 2
>> fi
>>
>> - # Load the default functions
>> - . "$MERGE_TOOLS_DIR/include/defaults.sh"
>> + # Fallback definitions, to be overriden by tools.
>> + can_merge () {
>> + return 0
>> + }
>> +
>> + can_diff () {
>> + return 0
>> + }
>> +
>> + diff_cmd () {
>> + status=1
>> + return $status
>> + }
>> +
>> + merge_cmd () {
>> + status=1
>> + return $status
>> + }
>> +
>> + translate_merge_tool_path () {
>> + echo "$1"
>> + }
>>
>> # Load the redefined functions
>> . "$MERGE_TOOLS_DIR/$tool"
>> diff --git a/mergetools/include/defaults.sh b/mergetools/include/defaults.sh
>> deleted file mode 100644
>> index 21e63ec..0000000
>> --- a/mergetools/include/defaults.sh
>> +++ /dev/null
>> @@ -1,22 +0,0 @@
>> -# Redefined by builtin tools
>> -can_merge () {
>> - return 0
>> -}
>> -
>> -can_diff () {
>> - return 0
>> -}
>> -
>> -diff_cmd () {
>> - status=1
>> - return $status
>> -}
>> -
>> -merge_cmd () {
>> - status=1
>> - return $status
>> -}
>> -
>> -translate_merge_tool_path () {
>> - echo "$1"
>> -}
--
David
^ permalink raw reply
* Re: [PATCH 0/3] lazily load commit->buffer
From: Junio C Hamano @ 2013-01-27 5:32 UTC (permalink / raw)
To: Jeff King
Cc: Jonathon Mah, Jonathan Nieder, Duy Nguyen, Stefan Näwe,
Armin, git@vger.kernel.org
In-Reply-To: <20130126221400.GA13827@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> My HEAD has about 400/3000 non-unique commits, which matches your
> numbers percentage-wise. Dropping the lines above (and always freeing)
> takes my best-of-five for "git log -g" from 0.085s to 0.080s. Which is
> well within the noise. Doing "git log -g Makefile" ended up at 0.183s
> both before and after.
>
> ... I'd be in favor of
> dropping the lines just to decrease complexity of the code.
I think we are in agreement, then.
^ permalink raw reply
* Re: [PATCH v3 6/8] git-remote-testpy: hash bytes explicitly
From: Sverre Rabbelier @ 2013-01-27 5:30 UTC (permalink / raw)
To: Michael Haggerty; +Cc: Junio C Hamano, John Keeping, Git List
In-Reply-To: <5104B0B5.1030501@alum.mit.edu>
On Sat, Jan 26, 2013 at 8:44 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> So to handle all of the cases across Python versions as closely as
> possible to the old 2.x code, it might be necessary to make the code
> explicitly depend on the Python version number, like:
Does this all go away if we restrict ourselves to python 2.6 and just
use the b prefix?
--
Cheers,
Sverre Rabbelier
^ permalink raw reply
* Re: [PATCH v3 6/8] git-remote-testpy: hash bytes explicitly
From: Junio C Hamano @ 2013-01-27 5:30 UTC (permalink / raw)
To: Michael Haggerty; +Cc: John Keeping, git, Sverre Rabbelier
In-Reply-To: <5104B0B5.1030501@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
> This will still fail under Python 2.x if repo.path is a byte string that
> contains non-ASCII characters. And it will fail under Python 3.1 and
> later if repo.path contains characters using the surrogateescape
> encoding option [1],...
> Here you don't really need byte-for-byte correctness; it would be enough
> to get *some* byte string that is unique for a given input ...
Yeek.
As we do not care about the actual value at all, how about doing
something like this instead?
git-remote-testgit.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/git-remote-testgit.py b/git-remote-testgit.py
index 5f3ebd2..705750d 100644
--- a/git-remote-testgit.py
+++ b/git-remote-testgit.py
@@ -40,7 +40,7 @@ def get_repo(alias, url):
repo.get_head()
hasher = _digest()
- hasher.update(repo.path)
+ hasher.update(".".join([str(ord(c)) for c in repo.path]))
repo.hash = hasher.hexdigest()
repo.get_base_path = lambda base: os.path.join(
^ permalink raw reply related
* Re: [PATCH] mergetools: Simplify how we handle "vim" and "defaults"
From: David Aguilar @ 2013-01-27 5:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: John Keeping, git
In-Reply-To: <7v8v7fz0ii.fsf@alter.siamese.dyndns.org>
On Sat, Jan 26, 2013 at 8:57 PM, Junio C Hamano <gitster@pobox.com> wrote:
> John Keeping <john@keeping.me.uk> writes:
>
>> I'm not sure creating an 'include' directory actually buys us much over
>> declaring that 'vimdiff' is the real script and the others just source
>> it.
>
> Is 'include' really used for such a purpose? It only houses defaults.sh
> as far as I can see.
>
> As that defaults.sh file is used only to define trivially empty
> shell functions, I wonder if it is better to get rid of it, and
> define these functions in line in git-mergetool--lib.sh. Such a
> change would like the attached on top of the entire series.
I think that's much better.
> Makefile | 6 +-----
> git-mergetool--lib.sh | 24 ++++++++++++++++++++++--
> mergetools/include/defaults.sh | 22 ----------------------
> 3 files changed, 23 insertions(+), 29 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 26f217f..f69979e 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2724,11 +2724,7 @@ install: all
> $(INSTALL) $(install_bindir_programs) '$(DESTDIR_SQ)$(bindir_SQ)'
> $(MAKE) -C templates DESTDIR='$(DESTDIR_SQ)' install
> $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'
> - $(INSTALL) -m 644 $(filter-out mergetools/include,$(wildcard mergetools/*)) \
> - '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'
> - $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(mergetools_instdir_SQ)/include'
> - $(INSTALL) -m 644 mergetools/include/* \
> - '$(DESTDIR_SQ)$(mergetools_instdir_SQ)/include'
> + $(INSTALL) -m 644 mergetools/* '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'
> ifndef NO_GETTEXT
> $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(localedir_SQ)'
> (cd po/build/locale && $(TAR) cf - .) | \
> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> index 7ea7510..1d0fb12 100644
> --- a/git-mergetool--lib.sh
> +++ b/git-mergetool--lib.sh
> @@ -57,8 +57,28 @@ setup_tool () {
> return 2
> fi
>
> - # Load the default functions
> - . "$MERGE_TOOLS_DIR/include/defaults.sh"
> + # Fallback definitions, to be overriden by tools.
> + can_merge () {
> + return 0
> + }
> +
> + can_diff () {
> + return 0
> + }
> +
> + diff_cmd () {
> + status=1
> + return $status
> + }
> +
> + merge_cmd () {
> + status=1
> + return $status
> + }
> +
> + translate_merge_tool_path () {
> + echo "$1"
> + }
>
> # Load the redefined functions
> . "$MERGE_TOOLS_DIR/$tool"
> diff --git a/mergetools/include/defaults.sh b/mergetools/include/defaults.sh
> deleted file mode 100644
> index 21e63ec..0000000
> --- a/mergetools/include/defaults.sh
> +++ /dev/null
> @@ -1,22 +0,0 @@
> -# Redefined by builtin tools
> -can_merge () {
> - return 0
> -}
> -
> -can_diff () {
> - return 0
> -}
> -
> -diff_cmd () {
> - status=1
> - return $status
> -}
> -
> -merge_cmd () {
> - status=1
> - return $status
> -}
> -
> -translate_merge_tool_path () {
> - echo "$1"
> -}
--
David
^ permalink raw reply
* Re: [PATCH] mergetools: Simplify how we handle "vim" and "defaults"
From: Junio C Hamano @ 2013-01-27 4:57 UTC (permalink / raw)
To: John Keeping; +Cc: David Aguilar, git
In-Reply-To: <20130126121202.GH7498@serenity.lan>
John Keeping <john@keeping.me.uk> writes:
> I'm not sure creating an 'include' directory actually buys us much over
> declaring that 'vimdiff' is the real script and the others just source
> it.
Is 'include' really used for such a purpose? It only houses defaults.sh
as far as I can see.
As that defaults.sh file is used only to define trivially empty
shell functions, I wonder if it is better to get rid of it, and
define these functions in line in git-mergetool--lib.sh. Such a
change would like the attached on top of the entire series.
Makefile | 6 +-----
git-mergetool--lib.sh | 24 ++++++++++++++++++++++--
mergetools/include/defaults.sh | 22 ----------------------
3 files changed, 23 insertions(+), 29 deletions(-)
diff --git a/Makefile b/Makefile
index 26f217f..f69979e 100644
--- a/Makefile
+++ b/Makefile
@@ -2724,11 +2724,7 @@ install: all
$(INSTALL) $(install_bindir_programs) '$(DESTDIR_SQ)$(bindir_SQ)'
$(MAKE) -C templates DESTDIR='$(DESTDIR_SQ)' install
$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'
- $(INSTALL) -m 644 $(filter-out mergetools/include,$(wildcard mergetools/*)) \
- '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'
- $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(mergetools_instdir_SQ)/include'
- $(INSTALL) -m 644 mergetools/include/* \
- '$(DESTDIR_SQ)$(mergetools_instdir_SQ)/include'
+ $(INSTALL) -m 644 mergetools/* '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'
ifndef NO_GETTEXT
$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(localedir_SQ)'
(cd po/build/locale && $(TAR) cf - .) | \
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 7ea7510..1d0fb12 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -57,8 +57,28 @@ setup_tool () {
return 2
fi
- # Load the default functions
- . "$MERGE_TOOLS_DIR/include/defaults.sh"
+ # Fallback definitions, to be overriden by tools.
+ can_merge () {
+ return 0
+ }
+
+ can_diff () {
+ return 0
+ }
+
+ diff_cmd () {
+ status=1
+ return $status
+ }
+
+ merge_cmd () {
+ status=1
+ return $status
+ }
+
+ translate_merge_tool_path () {
+ echo "$1"
+ }
# Load the redefined functions
. "$MERGE_TOOLS_DIR/$tool"
diff --git a/mergetools/include/defaults.sh b/mergetools/include/defaults.sh
deleted file mode 100644
index 21e63ec..0000000
--- a/mergetools/include/defaults.sh
+++ /dev/null
@@ -1,22 +0,0 @@
-# Redefined by builtin tools
-can_merge () {
- return 0
-}
-
-can_diff () {
- return 0
-}
-
-diff_cmd () {
- status=1
- return $status
-}
-
-merge_cmd () {
- status=1
- return $status
-}
-
-translate_merge_tool_path () {
- echo "$1"
-}
^ permalink raw reply related
* Re: [PATCH v3 6/8] git-remote-testpy: hash bytes explicitly
From: Michael Haggerty @ 2013-01-27 4:44 UTC (permalink / raw)
To: Junio C Hamano; +Cc: John Keeping, git, Sverre Rabbelier
In-Reply-To: <7vwquzzkiw.fsf@alter.siamese.dyndns.org>
On 01/26/2013 10:44 PM, Junio C Hamano wrote:
> John Keeping <john@keeping.me.uk> writes:
>
>> Junio, can you replace the queued 0846b0c (git-remote-testpy: hash bytes
>> explicitly) with this?
>>
>> I hadn't realised that the "hex" encoding we chose before is a "bytes to
>> bytes" encoding so it just fails with an error on Python 3 in the same
>> way as the original code.
>>
>> Since we want to convert a Unicode string to bytes I think UTF-8 really
>> is the best option here.
>
> Ahh. I think it is already in "next", so this needs to be turned
> into an incremental to flip 'hex' to 'utf-8', with the justification
> being these five lines above.
>
> Thanks for catching.
>
>>
>> git-remote-testpy.py | 8 ++++----
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/git-remote-testpy.py b/git-remote-testpy.py
>> index d94a66a..f8dc196 100644
>> --- a/git-remote-testpy.py
>> +++ b/git-remote-testpy.py
>> @@ -31,9 +31,9 @@ from git_remote_helpers.git.exporter import GitExporter
>> from git_remote_helpers.git.importer import GitImporter
>> from git_remote_helpers.git.non_local import NonLocalGit
>>
>> -if sys.hexversion < 0x01050200:
>> - # os.makedirs() is the limiter
>> - sys.stderr.write("git-remote-testgit: requires Python 1.5.2 or later.\n")
>> +if sys.hexversion < 0x02000000:
>> + # string.encode() is the limiter
>> + sys.stderr.write("git-remote-testgit: requires Python 2.0 or later.\n")
>> sys.exit(1)
>>
>> def get_repo(alias, url):
>> @@ -45,7 +45,7 @@ def get_repo(alias, url):
>> repo.get_head()
>>
>> hasher = _digest()
>> - hasher.update(repo.path)
>> + hasher.update(repo.path.encode('utf-8'))
>> repo.hash = hasher.hexdigest()
>>
>> repo.get_base_path = lambda base: os.path.join(
This will still fail under Python 2.x if repo.path is a byte string that
contains non-ASCII characters. And it will fail under Python 3.1 and
later if repo.path contains characters using the surrogateescape
encoding option [1], as it will if the original command-line argument
contained bytes that cannot be decoded into Unicode using the user's
default encoding:
$ python3 --version
Python 3.2.3
$ python3 -c "
import sys
print(repr(sys.argv[1]))
print(repr(sys.argv[1].encode('utf-8')))
" $(echo français|iconv -t latin1)
'fran\udce7ais'
Traceback (most recent call last):
File "<string>", line 4, in <module>
UnicodeEncodeError: 'utf-8' codec can't encode character '\udce7' in
position 4: surrogates not allowed
I'm not sure what happens in Python 3.0.
I think the "modern" way to handle this situation in Python 3.1+ is via
PEP 383's surrogateescape encoding option [1]:
repo.path.encode('utf-8', 'surrogateescape')
Basically, byte strings that come from the OS are automatically decoded
into Unicode strings using
s = b.decode(sys.getfilesystemencoding(), 'surrogateescape')
If the string needs to be passed back to the filesystem as a byte string
it is via
b = s.encode(sys.getfilesystemencoding(), 'surrogateescape')
My understanding is that the surrogateescape mechanism guarantees that
the round-trip bytestring -> string -> bytestring gives back the
original byte string, which is what you want for things like filenames.
But a Unicode string that contains surrogate escape characters *cannot*
be encoded without the 'surrogateescape' option.
'surrogateescape' is not supported in Python 3.0, but I think it would
be quite acceptable only to support Python 3.x for x >= 1.
But 'surrogateescape' doesn't seem to be supported at all in Python 2.x
(I tested 2.7.3 and it's not there).
Here you don't really need byte-for-byte correctness; it would be enough
to get *some* byte string that is unique for a given input (ideally,
consistent with ASCII or UTF-8 for backwards compatibility). So you
could use
b = s.encode('utf-8', 'backslashreplace')
Unfortunately, this doesn't work under Python 2.x:
$ python2 -c "
import sys
print(repr(sys.argv[1]))
print(repr(sys.argv[1].encode('utf-8', 'backslashreplace')))
" $(echo français|iconv -t latin1)
'fran\xe7ais'
Traceback (most recent call last):
File "<string>", line 4, in <module>
UnicodeDecodeError: 'ascii' codec can't decode byte 0xe7 in position
4: ordinal not in range(128)
Apparently when you call bytestring.encode(), Python first tries to
decode the string to Unicode using the 'ascii' encoding.
So to handle all of the cases across Python versions as closely as
possible to the old 2.x code, it might be necessary to make the code
explicitly depend on the Python version number, like:
hasher = _digest()
if sys.hexversion < 0x03000000:
pathbytes = repo.path
elif sys.hexversion < 0x03010000:
# If support for Python 3.0.x is desired (note: result can
# be different in this case than under 2.x or 3.1+):
pathbytes = repo.path.encode(sys.getfilesystemencoding(),
'backslashreplace')
else
pathbytes = repo.path.encode(sys.getfilesystemencoding(),
'surrogateescape')
hasher.update(pathbytes)
repo.hash = hasher.hexdigest()
Michael
[1] http://www.python.org/dev/peps/pep-0383/
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply
* Re: [PATCH v3 2/2] mergetools: Simplify how we handle "vim" and "defaults"
From: Junio C Hamano @ 2013-01-27 4:25 UTC (permalink / raw)
To: David Aguilar; +Cc: git, John Keeping
In-Reply-To: <CAJDDKr7E1NkMV0_G+6oY9O3iCS9OCEzR1HYssKpwh77swDUcig@mail.gmail.com>
David Aguilar <davvid@gmail.com> writes:
> I have a question. John mentioned that we can replace the use of
> "$(..)" with $(..) in shell.
I think it isn't so much about $(...) but more about quoting the RHS
of assignment.
Consider these:
var="$another_var"
var="$(command)"
var="$one_var$two_var"
var="$another_var$(command)"
all of which _can_ be done without dq around the RHS, because the
result of variable substitution and command substitution will not be
subject to further word splitting.
I however find it easier to read with dq around the latter two, i.e.
substitution and then concatenation of the result of substitution.
The extra dq makes the intent of the author clearer, especially
while reviewing a script written by other people whose understanding
of the syntax I am not sure about ;-). Between var=$another and
var="$another", the latter is slightly preferrable for the same
reason.
One questionable case is:
var=$(command "with quoted parameter")
It makes it a bit harder to scan if there is an extra set of dq
around RHS, i.e.
var="$(command "with quoted parameter")"
That case is easier to read without dq around it, or you could cheat
by doing this:
var="$(command 'with quoted parameter')"
In any case, as long as the result is correct, I do not have very
strong preference either way.
^ permalink raw reply
* Re: [PATCH v3 2/2] mergetools: Simplify how we handle "vim" and "defaults"
From: David Aguilar @ 2013-01-27 3:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, John Keeping
In-Reply-To: <7vobgbz58a.fsf@alter.siamese.dyndns.org>
On Sat, Jan 26, 2013 at 7:15 PM, Junio C Hamano <gitster@pobox.com> wrote:
> David Aguilar <davvid@gmail.com> writes:
>
>> @@ -44,19 +46,9 @@ valid_tool () {
>> }
>>
>> setup_tool () {
>> - case "$1" in
>> - vim*|gvim*)
>> - tool=vim
>> - ;;
>> - *)
>> - tool="$1"
>> - ;;
>> - esac
>
> This part was an eyesore every time I looked at mergetools scripts.
> Good riddance.
>
> Is there still other special case like this, or was this the last
> one?
>
> Thanks, both of you, for working on this.
I believe that was the last special case. :-) It should be easier
to auto-generate a list of tools for use in the documentation now.
That'll be the the next topic I look into.
I have a question. John mentioned that we can replace the use of
"$(..)" with $(..) in shell.
I have a trivial style patches to replace "$(..)" with $(..)
sitting uncommitted in my tree for mergetool--lib.
Grepping the rest of the tree shows that there are many
occurrences of the "$(..)" idiom in the shell code.
Is this a general rule that should be in CodingStyle,
or is it better left as-is? Are there cases where DQ should
be used around $(..)? My understanding is "no", but I don't
know whether that is correct.
Doing the documentation stuff is more important IMO so I probably
shouldn't let myself get too distracted by it, though I am curious.
--
David
^ permalink raw reply
* [PATCHv2 21/21] git p4: introduce gitConfigBool
From: Pete Wyckoff @ 2013-01-27 3:11 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Johannes Sixt
In-Reply-To: <1359256284-5660-1-git-send-email-pw@padd.com>
Make the intent of "--bool" more obvious by returning a direct True
or False value. Convert a couple non-bool users with obvious bool
intent.
Signed-off-by: Pete Wyckoff <pw@padd.com>
---
git-p4.py | 45 ++++++++++++++++++++++++++-------------------
1 file changed, 26 insertions(+), 19 deletions(-)
diff --git a/git-p4.py b/git-p4.py
index ff3e8c9..955a5dd 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -561,17 +561,25 @@ def gitBranchExists(branch):
_gitConfig = {}
-def gitConfig(key, args=None): # set args to "--bool", for instance
+def gitConfig(key):
if not _gitConfig.has_key(key):
- cmd = [ "git", "config" ]
- if args:
- assert(args == "--bool")
- cmd.append(args)
- cmd.append(key)
+ cmd = [ "git", "config", key ]
s = read_pipe(cmd, ignore_error=True)
_gitConfig[key] = s.strip()
return _gitConfig[key]
+def gitConfigBool(key):
+ """Return a bool, using git config --bool. It is True only if the
+ variable is set to true, and False if set to false or not present
+ in the config."""
+
+ if not _gitConfig.has_key(key):
+ cmd = [ "git", "config", "--bool", key ]
+ s = read_pipe(cmd, ignore_error=True)
+ v = s.strip()
+ _gitConfig[key] = v == "true"
+ return _gitConfig[key]
+
def gitConfigList(key):
if not _gitConfig.has_key(key):
s = read_pipe(["git", "config", "--get-all", key], ignore_error=True)
@@ -722,8 +730,7 @@ def p4PathStartsWith(path, prefix):
#
# we may or may not have a problem. If you have core.ignorecase=true,
# we treat DirA and dira as the same directory
- ignorecase = gitConfig("core.ignorecase", "--bool") == "true"
- if ignorecase:
+ if gitConfigBool("core.ignorecase"):
return path.lower().startswith(prefix.lower())
return path.startswith(prefix)
@@ -959,7 +966,7 @@ class P4Submit(Command, P4UserMap):
self.usage += " [name of git branch to submit into perforce depot]"
self.origin = ""
self.detectRenames = False
- self.preserveUser = gitConfig("git-p4.preserveUser").lower() == "true"
+ self.preserveUser = gitConfigBool("git-p4.preserveUser")
self.dry_run = False
self.prepare_p4_only = False
self.conflict_behavior = None
@@ -1068,7 +1075,7 @@ class P4Submit(Command, P4UserMap):
(user,email) = self.p4UserForCommit(id)
if not user:
msg = "Cannot find p4 user for email %s in commit %s." % (email, id)
- if gitConfig('git-p4.allowMissingP4Users').lower() == "true":
+ if gitConfigBool("git-p4.allowMissingP4Users"):
print "%s" % msg
else:
die("Error: %s\nSet git-p4.allowMissingP4Users to true to allow this." % msg)
@@ -1163,7 +1170,7 @@ class P4Submit(Command, P4UserMap):
message. Return true if okay to continue with the submit."""
# if configured to skip the editing part, just submit
- if gitConfig("git-p4.skipSubmitEdit") == "true":
+ if gitConfigBool("git-p4.skipSubmitEdit"):
return True
# look at the modification time, to check later if the user saved
@@ -1179,7 +1186,7 @@ class P4Submit(Command, P4UserMap):
# If the file was not saved, prompt to see if this patch should
# be skipped. But skip this verification step if configured so.
- if gitConfig("git-p4.skipSubmitEditCheck") == "true":
+ if gitConfigBool("git-p4.skipSubmitEditCheck"):
return True
# modification time updated means user saved the file
@@ -1279,7 +1286,7 @@ class P4Submit(Command, P4UserMap):
# Patch failed, maybe it's just RCS keyword woes. Look through
# the patch to see if that's possible.
- if gitConfig("git-p4.attemptRCSCleanup","--bool") == "true":
+ if gitConfigBool("git-p4.attemptRCSCleanup"):
file = None
pattern = None
kwfiles = {}
@@ -1574,7 +1581,7 @@ class P4Submit(Command, P4UserMap):
sys.exit(128)
self.useClientSpec = False
- if gitConfig("git-p4.useclientspec", "--bool") == "true":
+ if gitConfigBool("git-p4.useclientspec"):
self.useClientSpec = True
if self.useClientSpec:
self.clientSpecDirs = getClientSpec()
@@ -1614,7 +1621,7 @@ class P4Submit(Command, P4UserMap):
commits.append(line.strip())
commits.reverse()
- if self.preserveUser or (gitConfig("git-p4.skipUserNameCheck") == "true"):
+ if self.preserveUser or gitConfigBool("git-p4.skipUserNameCheck"):
self.checkAuthorship = False
else:
self.checkAuthorship = True
@@ -1650,7 +1657,7 @@ class P4Submit(Command, P4UserMap):
else:
self.diffOpts += " -C%s" % detectCopies
- if gitConfig("git-p4.detectCopiesHarder", "--bool") == "true":
+ if gitConfigBool("git-p4.detectCopiesHarder"):
self.diffOpts += " --find-copies-harder"
#
@@ -1734,7 +1741,7 @@ class P4Submit(Command, P4UserMap):
"--format=format:%h %s", c])
print "You will have to do 'git p4 sync' and rebase."
- if gitConfig("git-p4.exportLabels", "--bool") == "true":
+ if gitConfigBool("git-p4.exportLabels"):
self.exportLabels = True
if self.exportLabels:
@@ -2834,7 +2841,7 @@ class P4Sync(Command, P4UserMap):
# will use this after clone to set the variable
self.useClientSpec_from_options = True
else:
- if gitConfig("git-p4.useclientspec", "--bool") == "true":
+ if gitConfigBool("git-p4.useclientspec"):
self.useClientSpec = True
if self.useClientSpec:
self.clientSpecDirs = getClientSpec()
@@ -3074,7 +3081,7 @@ class P4Sync(Command, P4UserMap):
sys.stdout.write("%s " % b)
sys.stdout.write("\n")
- if gitConfig("git-p4.importLabels", "--bool") == "true":
+ if gitConfigBool("git-p4.importLabels"):
self.importLabels = True
if self.importLabels:
--
1.8.1.1.460.g6fa8886
^ permalink raw reply related
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