* Re: [PATCH 1/2] fetch: run gc --auto after fetching
From: Jonathan Nieder @ 2013-01-27 1:51 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano
In-Reply-To: <20130126224038.GA20849@sigill.intra.peff.net>
Jeff King wrote:
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -959,6 +959,9 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
> struct string_list list = STRING_LIST_INIT_NODUP;
> struct remote *remote;
> int result = 0;
> + static const char *argv_gc_auto[] = {
> + "gc", "--auto", NULL,
> + };
>
> packet_trace_identity("fetch");
>
> @@ -1026,5 +1029,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
> list.strdup_strings = 1;
> string_list_clear(&list, 0);
>
> + run_command_v_opt(argv_gc_auto, RUN_GIT_CMD);
> +
> return result;
Good idea, and the execution is obviously correct. Thanks.
^ permalink raw reply
* [PATCH v3 2/2] mergetools: Simplify how we handle "vim" and "defaults"
From: David Aguilar @ 2013-01-27 0:46 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, John Keeping
Remove the exceptions for "vim" and "defaults" in the mergetool library
so that every filename in mergetools/ matches 1:1 with the name of a
valid built-in tool.
Make common functions available in $MERGE_TOOLS_DIR/include/.
Signed-off-by: David Aguilar <davvid@gmail.com>
---
v2 used "include" instead of "mergetools/include"
in the Makefile. Please ignore it.
Makefile | 6 +++-
git-mergetool--lib.sh | 41 ++++++++--------------------
mergetools/gvimdiff | 1 +
mergetools/gvimdiff2 | 1 +
mergetools/{defaults => include/defaults.sh} | 0
mergetools/{vim => vimdiff} | 0
mergetools/vimdiff2 | 1 +
7 files changed, 19 insertions(+), 31 deletions(-)
create mode 100644 mergetools/gvimdiff
create mode 100644 mergetools/gvimdiff2
rename mergetools/{defaults => include/defaults.sh} (100%)
rename mergetools/{vim => vimdiff} (100%)
create mode 100644 mergetools/vimdiff2
diff --git a/Makefile b/Makefile
index f69979e..26f217f 100644
--- a/Makefile
+++ b/Makefile
@@ -2724,7 +2724,11 @@ 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 mergetools/* '$(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'
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 f1bb372..7ea7510 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -1,5 +1,7 @@
#!/bin/sh
# git-mergetool--lib is a library for common merge tool functions
+MERGE_TOOLS_DIR=$(git --exec-path)/mergetools
+
diff_mode() {
test "$TOOL_MODE" = diff
}
@@ -44,19 +46,9 @@ valid_tool () {
}
setup_tool () {
- case "$1" in
- vim*|gvim*)
- tool=vim
- ;;
- *)
- tool="$1"
- ;;
- esac
- mergetools="$(git --exec-path)/mergetools"
+ tool="$1"
- # Load the default definitions
- . "$mergetools/defaults"
- if ! test -f "$mergetools/$tool"
+ if ! test -f "$MERGE_TOOLS_DIR/$tool"
then
# Use a special return code for this case since we want to
# source "defaults" even when an explicit tool path is
@@ -65,8 +57,11 @@ setup_tool () {
return 2
fi
+ # Load the default functions
+ . "$MERGE_TOOLS_DIR/include/defaults.sh"
+
# Load the redefined functions
- . "$mergetools/$tool"
+ . "$MERGE_TOOLS_DIR/$tool"
if merge_mode && ! can_merge
then
@@ -194,24 +189,10 @@ list_merge_tool_candidates () {
show_tool_help () {
unavailable= available= LF='
'
-
- scriptlets="$(git --exec-path)"/mergetools
- for i in "$scriptlets"/*
+ for i in "$MERGE_TOOLS_DIR"/*
do
- . "$scriptlets"/defaults
- . "$i"
-
- tool="$(basename "$i")"
- if test "$tool" = "defaults"
- then
- continue
- elif merge_mode && ! can_merge
- then
- continue
- elif diff_mode && ! can_diff
- then
- continue
- fi
+ tool=$(basename "$i")
+ setup_tool "$tool" 2>/dev/null || continue
merge_tool_path=$(translate_merge_tool_path "$tool")
if type "$merge_tool_path" >/dev/null 2>&1
diff --git a/mergetools/gvimdiff b/mergetools/gvimdiff
new file mode 100644
index 0000000..04a5bb0
--- /dev/null
+++ b/mergetools/gvimdiff
@@ -0,0 +1 @@
+. "$MERGE_TOOLS_DIR/vimdiff"
diff --git a/mergetools/gvimdiff2 b/mergetools/gvimdiff2
new file mode 100644
index 0000000..04a5bb0
--- /dev/null
+++ b/mergetools/gvimdiff2
@@ -0,0 +1 @@
+. "$MERGE_TOOLS_DIR/vimdiff"
diff --git a/mergetools/defaults b/mergetools/include/defaults.sh
similarity index 100%
rename from mergetools/defaults
rename to mergetools/include/defaults.sh
diff --git a/mergetools/vim b/mergetools/vimdiff
similarity index 100%
rename from mergetools/vim
rename to mergetools/vimdiff
diff --git a/mergetools/vimdiff2 b/mergetools/vimdiff2
new file mode 100644
index 0000000..04a5bb0
--- /dev/null
+++ b/mergetools/vimdiff2
@@ -0,0 +1 @@
+. "$MERGE_TOOLS_DIR/vimdiff"
--
1.8.0.8.gd6b90fb.dirty
^ permalink raw reply related
* [PATCH v2 2/2] mergetools: Simplify how we handle "vim" and "defaults"
From: David Aguilar @ 2013-01-27 0:40 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, John Keeping
In-Reply-To: <1359247207-71819-1-git-send-email-davvid@gmail.com>
Remove the exceptions for "vim" and "defaults" in the mergetool library
so that every filename in mergetools/ matches 1:1 with the name of a
valid built-in tool.
Make common functions available in $MERGE_TOOLS_DIR/include/.
Signed-off-by: David Aguilar <davvid@gmail.com>
---
This diffstat is much nicer now thanks to John's setup_tool rework in 1/1.
Makefile | 6 +++-
git-mergetool--lib.sh | 41 ++++++++--------------------
mergetools/gvimdiff | 1 +
mergetools/gvimdiff2 | 1 +
mergetools/{defaults => include/defaults.sh} | 0
mergetools/{vim => vimdiff} | 0
mergetools/vimdiff2 | 1 +
7 files changed, 19 insertions(+), 31 deletions(-)
create mode 100644 mergetools/gvimdiff
create mode 100644 mergetools/gvimdiff2
rename mergetools/{defaults => include/defaults.sh} (100%)
rename mergetools/{vim => vimdiff} (100%)
create mode 100644 mergetools/vimdiff2
diff --git a/Makefile b/Makefile
index f69979e..0f89032 100644
--- a/Makefile
+++ b/Makefile
@@ -2724,7 +2724,11 @@ 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 mergetools/* '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'
+ $(INSTALL) -m 644 $(filter-out 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'
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 f1bb372..7ea7510 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -1,5 +1,7 @@
#!/bin/sh
# git-mergetool--lib is a library for common merge tool functions
+MERGE_TOOLS_DIR=$(git --exec-path)/mergetools
+
diff_mode() {
test "$TOOL_MODE" = diff
}
@@ -44,19 +46,9 @@ valid_tool () {
}
setup_tool () {
- case "$1" in
- vim*|gvim*)
- tool=vim
- ;;
- *)
- tool="$1"
- ;;
- esac
- mergetools="$(git --exec-path)/mergetools"
+ tool="$1"
- # Load the default definitions
- . "$mergetools/defaults"
- if ! test -f "$mergetools/$tool"
+ if ! test -f "$MERGE_TOOLS_DIR/$tool"
then
# Use a special return code for this case since we want to
# source "defaults" even when an explicit tool path is
@@ -65,8 +57,11 @@ setup_tool () {
return 2
fi
+ # Load the default functions
+ . "$MERGE_TOOLS_DIR/include/defaults.sh"
+
# Load the redefined functions
- . "$mergetools/$tool"
+ . "$MERGE_TOOLS_DIR/$tool"
if merge_mode && ! can_merge
then
@@ -194,24 +189,10 @@ list_merge_tool_candidates () {
show_tool_help () {
unavailable= available= LF='
'
-
- scriptlets="$(git --exec-path)"/mergetools
- for i in "$scriptlets"/*
+ for i in "$MERGE_TOOLS_DIR"/*
do
- . "$scriptlets"/defaults
- . "$i"
-
- tool="$(basename "$i")"
- if test "$tool" = "defaults"
- then
- continue
- elif merge_mode && ! can_merge
- then
- continue
- elif diff_mode && ! can_diff
- then
- continue
- fi
+ tool=$(basename "$i")
+ setup_tool "$tool" 2>/dev/null || continue
merge_tool_path=$(translate_merge_tool_path "$tool")
if type "$merge_tool_path" >/dev/null 2>&1
diff --git a/mergetools/gvimdiff b/mergetools/gvimdiff
new file mode 100644
index 0000000..04a5bb0
--- /dev/null
+++ b/mergetools/gvimdiff
@@ -0,0 +1 @@
+. "$MERGE_TOOLS_DIR/vimdiff"
diff --git a/mergetools/gvimdiff2 b/mergetools/gvimdiff2
new file mode 100644
index 0000000..04a5bb0
--- /dev/null
+++ b/mergetools/gvimdiff2
@@ -0,0 +1 @@
+. "$MERGE_TOOLS_DIR/vimdiff"
diff --git a/mergetools/defaults b/mergetools/include/defaults.sh
similarity index 100%
rename from mergetools/defaults
rename to mergetools/include/defaults.sh
diff --git a/mergetools/vim b/mergetools/vimdiff
similarity index 100%
rename from mergetools/vim
rename to mergetools/vimdiff
diff --git a/mergetools/vimdiff2 b/mergetools/vimdiff2
new file mode 100644
index 0000000..04a5bb0
--- /dev/null
+++ b/mergetools/vimdiff2
@@ -0,0 +1 @@
+. "$MERGE_TOOLS_DIR/vimdiff"
--
1.8.0.8.g9bc9422
^ permalink raw reply related
* [PATCH 1/2] mergetool--lib: don't call "exit" in setup_tool
From: David Aguilar @ 2013-01-27 0:40 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, John Keeping
From: John Keeping <john@keeping.me.uk>
This will make it easier to use setup_tool in places where we expect
that the selected tool will not support the current mode.
We need to introduce a new return code for setup_tool to differentiate
between the case of "the selected tool is invalid" and "the selected
tool is not a built-in" since we must call setup_tool when a custom
'merge.<tool>.path' is configured for a built-in tool but avoid failing
when the configured tool is not a built-in.
Signed-off-by: John Keeping <john@keeping.me.uk>
Signed-off-by: David Aguilar <davvid@gmail.com>
---
This series is based on jk/mergetool in "pu".
This patch is unchanged from $gmane/214624.
git-mergetool--lib.sh | 23 ++++++++++++++++++++---
1 file changed, 20 insertions(+), 3 deletions(-)
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index aa38bd1..f1bb372 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -58,7 +58,11 @@ setup_tool () {
. "$mergetools/defaults"
if ! test -f "$mergetools/$tool"
then
- return 1
+ # Use a special return code for this case since we want to
+ # source "defaults" even when an explicit tool path is
+ # configured since the user can use that to override the
+ # default path in the scriptlet.
+ return 2
fi
# Load the redefined functions
@@ -67,11 +71,11 @@ setup_tool () {
if merge_mode && ! can_merge
then
echo "error: '$tool' can not be used to resolve merges" >&2
- exit 1
+ return 1
elif diff_mode && ! can_diff
then
echo "error: '$tool' can only be used to resolve merges" >&2
- exit 1
+ return 1
fi
return 0
}
@@ -101,6 +105,19 @@ run_merge_tool () {
# Bring tool-specific functions into scope
setup_tool "$1"
+ exitcode=$?
+ case $exitcode in
+ 0)
+ :
+ ;;
+ 2)
+ # The configured tool is not a built-in tool.
+ test -n "$merge_tool_path" || return 1
+ ;;
+ *)
+ return $exitcode
+ ;;
+ esac
if merge_mode
then
--
1.8.0.8.g9bc9422
^ permalink raw reply related
* [PATCH] git-remote-testpy: fix patch hashing on Python 3
From: John Keeping @ 2013-01-26 23:32 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Sverre Rabbelier
In-Reply-To: <7vwquzzkiw.fsf@alter.siamese.dyndns.org>
When this change was originally made (0846b0c - git-remote-testpy: hash bytes
explicitly , I didn't realised that the "hex" encoding we chose 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, UTF-8 really is the best
option here.
Signed-off-by: John Keeping <john@keeping.me.uk>
---
On Sat, Jan 26, 2013 at 01:44:55PM -0800, Junio C Hamano wrote:
> 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.
Here it is, based on next obviously.
git-remote-testpy.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/git-remote-testpy.py b/git-remote-testpy.py
index c7a04ec..4713363 100644
--- a/git-remote-testpy.py
+++ b/git-remote-testpy.py
@@ -45,7 +45,7 @@ def get_repo(alias, url):
repo.get_head()
hasher = _digest()
- hasher.update(repo.path.encode('hex'))
+ hasher.update(repo.path.encode('utf-8'))
repo.hash = hasher.hexdigest()
repo.get_base_path = lambda base: os.path.join(
--
1.8.1.1
^ permalink raw reply related
* [PATCH 2/2] fetch-pack: avoid repeatedly re-scanning pack directory
From: Jeff King @ 2013-01-26 22:40 UTC (permalink / raw)
To: git
In-Reply-To: <20130126224011.GA20675@sigill.intra.peff.net>
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.
This extra re-scan usually does not have a performance
impact for two reasons:
1. If an object is missing, then typically the re-scan
will find a new pack, then no more misses will occur.
Or if it truly is missing, then our next step is
usually to die().
2. Re-scanning is cheap enough that we do not even notice.
However, these do not always hold. The assumption in (1) is
that the caller is expecting to find the object. This is
usually the case, but the call to `parse_object` in
`everything_local` does not follow this pattern. It is
looking to see whether we have objects that the remote side
is advertising, not something we expect to have. Therefore
if we are fetching from a remote which has many refs
pointing to objects we do not have, we may end up
re-scanning the pack directory many times.
Even with this extra re-scanning, the impact is often not
noticeable due to (2); we just readdir() the packs directory
and skip any packs that are already loaded. However, if
there are a large number of packs, then even enumerating the
directory directory can be expensive (especially if we do it
repeatedly). Having this many packs is a good sign the user
should run `git gc`, but it would still be nice to avoid
having to scan the directory at all.
Signed-off-by: Jeff King <peff@peff.net>
---
fetch-pack.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/fetch-pack.c b/fetch-pack.c
index f0acdf7..6d8926a 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -594,6 +594,9 @@ static int everything_local(struct fetch_pack_args *args,
for (ref = *refs; ref; ref = ref->next) {
struct object *o;
+ if (!has_sha1_file(ref->old_sha1))
+ continue;
+
o = parse_object(ref->old_sha1);
if (!o)
continue;
--
1.8.0.2.16.g72e2fc9
^ permalink raw reply related
* [PATCH 1/2] fetch: run gc --auto after fetching
From: Jeff King @ 2013-01-26 22:40 UTC (permalink / raw)
To: git
In-Reply-To: <20130126224011.GA20675@sigill.intra.peff.net>
We generally try to run "gc --auto" after any commands that
might introduce a large number of new objects. An obvious
place to do so is after running "fetch", which may introduce
new loose objects or packs (depending on the size of the
fetch).
While an active developer repository will probably
eventually trigger a "gc --auto" on another action (e.g.,
git-rebase), there are two good reasons why it is nicer to
do it at fetch time:
1. Read-only repositories which track an upstream (e.g., a
continuous integration server which fetches and builds,
but never makes new commits) will accrue loose objects
and small packs, but never coalesce them into a more
efficient larger pack.
2. Fetching is often already perceived to be slow to the
user, since they have to wait on the network. It's much
more pleasant to include a potentially slow auto-gc as
part of the already-long network fetch than in the
middle of productive work with git-rebase or similar.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/fetch.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 4b5a898..1ddbf0d 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -959,6 +959,9 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
struct string_list list = STRING_LIST_INIT_NODUP;
struct remote *remote;
int result = 0;
+ static const char *argv_gc_auto[] = {
+ "gc", "--auto", NULL,
+ };
packet_trace_identity("fetch");
@@ -1026,5 +1029,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
list.strdup_strings = 1;
string_list_clear(&list, 0);
+ run_command_v_opt(argv_gc_auto, RUN_GIT_CMD);
+
return result;
}
--
1.8.0.2.16.g72e2fc9
^ permalink raw reply related
* [PATCH 0/2] optimizing pack access on "read only" fetch repos
From: Jeff King @ 2013-01-26 22:40 UTC (permalink / raw)
To: git
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
^ permalink raw reply
* Re: [PATCH 0/3] lazily load commit->buffer
From: Jeff King @ 2013-01-26 22:14 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jonathon Mah, Jonathan Nieder, Duy Nguyen, Stefan Näwe,
Armin, git@vger.kernel.org
In-Reply-To: <7v8v7f1vqa.fsf@alter.siamese.dyndns.org>
On Sat, Jan 26, 2013 at 01:26:53PM -0800, Junio C Hamano wrote:
> This looks very good.
>
> I wonder if this lets us get rid of the hack in cmd_log_walk() that
> does this:
>
> while ((commit = get_revision(rev)) != NULL) {
> if (!log_tree_commit(rev, commit) &&
> rev->max_count >= 0)
> rev->max_count++;
> ! if (!rev->reflog_info) {
> ! /* we allow cycles in reflog ancestry */
> free(commit->buffer);
> commit->buffer = NULL;
> ! }
> free_commit_list(commit->parents);
> commit->parents = NULL;
>
> After log_tree_commit() handles the commit, using the buffer, we
> discard the memory associated to it because we know we no longer
> will use it in normal cases.
> [...]
> But that is a performance thing, not a correctness issue, so "we
> allow cycles" implying "therefore if we discard the buffer, we will
> show wrong output" becomes an incorrect justification.
Right. I think the correctness issue goes away with my patches, and it
is just a question of estimating the workload for performance. I doubt
it makes a big difference either way, especially when compared to
actually showing the commit (even a single pathspec limiter, or doing
"-p", would likely dwarf a few extra commit decompressions).
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.
So I suspect it does not matter at all in normal cases, and the time is
indeed dwarfed by adding even a rudimentary pathspec. I'd be in favor of
dropping the lines just to decrease complexity of the code.
-Peff
^ permalink raw reply
* Re: [PATCH v3 6/8] git-remote-testpy: hash bytes explicitly
From: Junio C Hamano @ 2013-01-26 21:44 UTC (permalink / raw)
To: John Keeping; +Cc: git, Sverre Rabbelier
In-Reply-To: <20130126175158.GK7498@serenity.lan>
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(
^ permalink raw reply
* Re: [PATCH] tests: turn on test-lint-shell-syntax by default
From: Junio C Hamano @ 2013-01-26 21:43 UTC (permalink / raw)
To: Torsten Bögershausen; +Cc: Jonathan Nieder, git, kraai
In-Reply-To: <51037E5F.8090506@web.de>
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.
^ permalink raw reply
* Re: [PATCH 0/3] lazily load commit->buffer
From: Junio C Hamano @ 2013-01-26 21:26 UTC (permalink / raw)
To: Jeff King
Cc: Jonathon Mah, Jonathan Nieder, Duy Nguyen, Stefan Näwe,
Armin, git@vger.kernel.org
In-Reply-To: <20130126094026.GA9646@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Yeah, agreed. I started to fix this up with a use/unuse pattern and
> realized something: all of the call sites are calling logmsg_reencode
> anyway, because that is the next logical step in doing anything with the
> buffer that is not just parsing out the parent/timestamp/tree info. And
> since that function already might allocate (for the re-encoded copy),
> callers have to handle the maybe-borrowed-maybe-free situation already.
>
> So I came up with this patch series, which I think should fix the
> problem, and actually makes the call-sites easier to read, rather than
> harder.
>
> [1/3]: commit: drop useless xstrdup of commit message
> [2/3]: logmsg_reencode: never return NULL
> [3/3]: logmsg_reencode: lazily load missing commit buffers
>
> Here's the diffstat:
>
> builtin/blame.c | 22 ++-------
> builtin/commit.c | 14 +-----
> commit.h | 1 +
> pretty.c | 93 ++++++++++++++++++++++++++---------
> t/t4042-diff-textconv-caching.sh | 8 +++
> 5 files changed, 85 insertions(+), 53 deletions(-)
>
> Not too bad, and 27 of the lines added in pretty.c are new comments
> explaining the flow of logmsg_reencode. So even if this doesn't get
> every case, I think it's a nice cleanup.
This looks very good.
I wonder if this lets us get rid of the hack in cmd_log_walk() that
does this:
while ((commit = get_revision(rev)) != NULL) {
if (!log_tree_commit(rev, commit) &&
rev->max_count >= 0)
rev->max_count++;
! if (!rev->reflog_info) {
! /* we allow cycles in reflog ancestry */
free(commit->buffer);
commit->buffer = NULL;
! }
free_commit_list(commit->parents);
commit->parents = NULL;
After log_tree_commit() handles the commit, using the buffer, we
discard the memory associated to it because we know we no longer
will use it in normal cases.
The "do not do that if rev->reflog_info is true" was added in
a6c7306 (--walk-reflogs: do not crash with cyclic reflog ancestry,
2007-01-20) because the second and subsequent display of "commit"
(which happens to occur only when walking reflogs) needs to look at
commit->buffer again, and this hack forces us to retain the buffer
for _all_ commit objects.
But your patches could be seen as a different (and more correct) way
to fix the same issue. Once the display side learns how to re-read
the log text of the commit object, the above becomes unnecessary, no?
We may still be helped if majority of commit objects that appear in
the reflog appear more than once, in which case retaining the buffer
for _all_ commits could be an overall win. Not having to read the
buffer for the same commit each time it is shown for majority of
multiply-appearing commits, in exchange for having to keep the
buffer for commits that appears only once that are minority and
suffering increasted page cache pressure. That could be seen as an
optimization.
But that is a performance thing, not a correctness issue, so "we
allow cycles" implying "therefore if we discard the buffer, we will
show wrong output" becomes an incorrect justification.
I happen to have HEAD reflog that is 30k entries long; more than 26k
represent a checkout of unique commit. So I suspect that the above
hack to excessive retain commit->buffer for already used commits will
not help us performance-wise, either.
^ permalink raw reply
* Re: [PATCH] mergetools: Simplify how we handle "vim" and "defaults"
From: John Keeping @ 2013-01-26 20:54 UTC (permalink / raw)
To: David Aguilar; +Cc: Junio C Hamano, git
In-Reply-To: <CAJDDKr5UMyJOthSOPuChx7BCpcGwtmYcnVMh83q9kgCWSxDp4w@mail.gmail.com>
On Sat, Jan 26, 2013 at 12:40:23PM -0800, David Aguilar wrote:
> On Sat, Jan 26, 2013 at 4:12 AM, John Keeping <john@keeping.me.uk> wrote:
> > On Fri, Jan 25, 2013 at 10:50:58PM -0800, David Aguilar wrote:
> >> diff --git a/Makefile b/Makefile
> >> index f69979e..3bc6eb5 100644
> >> --- a/Makefile
> >> +++ b/Makefile
> >> @@ -2724,7 +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 mergetools/* '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'
> >> + cp -R mergetools/* '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'
> >
> > Shouldn't this be more like this?
> >
> > $(INSTALL) -m 644 $(subst 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'
> >
> > 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. Either way there is a single special entry in the mergetools
> > directory.
>
> Thanks (and thanks for the Makefile hint.. I knew it was wrong! ;-).
I think that version's still not right actually, the first line should
probably use filter-out not subst:
$(INSTALL) -m 644 $(filter-out include,$(wildcard mergetools/*)) \
'$(DESTDIR_SQ)$(mergetools_instdir_SQ)'
John
^ permalink raw reply
* Re: [PATCH] mergetools: Simplify how we handle "vim" and "defaults"
From: David Aguilar @ 2013-01-26 20:40 UTC (permalink / raw)
To: John Keeping; +Cc: Junio C Hamano, git
In-Reply-To: <20130126121202.GH7498@serenity.lan>
On Sat, Jan 26, 2013 at 4:12 AM, John Keeping <john@keeping.me.uk> wrote:
> On Fri, Jan 25, 2013 at 10:50:58PM -0800, David Aguilar wrote:
>> Remove the exceptions for "vim" and "defaults" in the mergetool library
>> so that every filename in mergetools/ matches 1:1 with the name of a
>> valid built-in tool.
>>
>> Make common functions available in $MERGE_TOOLS_DIR/include/.
>>
>> Signed-off-by: David Aguilar <davvid@gmail.com>
>> ---
>>
>> diff --git a/Makefile b/Makefile
>> index f69979e..3bc6eb5 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -2724,7 +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 mergetools/* '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'
>> + cp -R mergetools/* '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'
>
> Shouldn't this be more like this?
>
> $(INSTALL) -m 644 $(subst 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'
>
> 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. Either way there is a single special entry in the mergetools
> directory.
>
>> 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 aa38bd1..c866ed8 100644
>> --- a/git-mergetool--lib.sh
>> +++ b/git-mergetool--lib.sh
>> @@ -1,5 +1,7 @@
>> #!/bin/sh
>> # git-mergetool--lib is a library for common merge tool functions
>> +MERGE_TOOLS_DIR="$(git --exec-path)/mergetools"
>> +
>> diff_mode() {
>> test "$TOOL_MODE" = diff
>> }
>> @@ -44,25 +46,14 @@ valid_tool () {
>> }
>>
>> setup_tool () {
>> - case "$1" in
>> - vim*|gvim*)
>> - tool=vim
>> - ;;
>> - *)
>> - tool="$1"
>> - ;;
>> - esac
>> - mergetools="$(git --exec-path)/mergetools"
>> + tool="$1"
>
> Unnecessary quoting.
>
>> - # Load the default definitions
>> - . "$mergetools/defaults"
>> - if ! test -f "$mergetools/$tool"
>> + if ! test -f "$MERGE_TOOLS_DIR/$tool"
>> then
>> return 1
>> fi
>> -
>> - # Load the redefined functions
>> - . "$mergetools/$tool"
>> + . "$MERGE_TOOLS_DIR/include/defaults.sh"
>> + . "$MERGE_TOOLS_DIR/$tool"
>>
>> if merge_mode && ! can_merge
>> then
>> @@ -99,7 +90,7 @@ run_merge_tool () {
>> base_present="$2"
>> status=0
>>
>> - # Bring tool-specific functions into scope
>> + # Bring tool specific functions into scope
>
> This isn't related to this change (and I think tool-specific is more
> correct anyway).
>
>> setup_tool "$1"
>>
>> if merge_mode
>> @@ -177,18 +168,17 @@ list_merge_tool_candidates () {
>> show_tool_help () {
>> unavailable= available= LF='
>> '
>> -
>> - scriptlets="$(git --exec-path)"/mergetools
>> - for i in "$scriptlets"/*
>> + for i in "$MERGE_TOOLS_DIR"/*
>> do
>> - . "$scriptlets"/defaults
>> - . "$i"
>> -
>> - tool="$(basename "$i")"
>> - if test "$tool" = "defaults"
>> + if test -d "$i"
>> then
>> continue
>> - elif merge_mode && ! can_merge
>> + fi
>> +
>> + . "$MERGE_TOOLS_DIR"/include/defaults.sh
>> + . "$i"
>> +
>> + if merge_mode && ! can_merge
>> then
>> continue
>> elif diff_mode && ! can_diff
>> @@ -196,6 +186,7 @@ show_tool_help () {
>> continue
>> fi
>
>
> I'd prefer to see my change to setup_tool done before this so that we
> can just use:
>
> setup_tool "$tool" 2>/dev/null || continue
>
> for the above block. I'll send a fixed version in a couple of minutes.
I'll reroll this patch with your notes on top of your new patch later today.
Thanks (and thanks for the Makefile hint.. I knew it was wrong! ;-).
>
>> + tool=$(basename "$i")
>> merge_tool_path=$(translate_merge_tool_path "$tool")
>> if type "$merge_tool_path" >/dev/null 2>&1
>> then
>> diff --git a/mergetools/gvimdiff b/mergetools/gvimdiff
>> new file mode 100644
>> index 0000000..f5890b1
>> --- /dev/null
>> +++ b/mergetools/gvimdiff
>> @@ -0,0 +1 @@
>> +. "$MERGE_TOOLS_DIR/include/vim.sh"
>> diff --git a/mergetools/gvimdiff2 b/mergetools/gvimdiff2
>> new file mode 100644
>> index 0000000..f5890b1
>> --- /dev/null
>> +++ b/mergetools/gvimdiff2
>> @@ -0,0 +1 @@
>> +. "$MERGE_TOOLS_DIR/include/vim.sh"
>> diff --git a/mergetools/defaults b/mergetools/include/defaults.sh
>> similarity index 100%
>> rename from mergetools/defaults
>> rename to mergetools/include/defaults.sh
>> diff --git a/mergetools/vim b/mergetools/include/vim.sh
>> similarity index 100%
>> rename from mergetools/vim
>> rename to mergetools/include/vim.sh
>> diff --git a/mergetools/vimdiff b/mergetools/vimdiff
>> new file mode 100644
>> index 0000000..f5890b1
>> --- /dev/null
>> +++ b/mergetools/vimdiff
>> @@ -0,0 +1 @@
>> +. "$MERGE_TOOLS_DIR/include/vim.sh"
>> diff --git a/mergetools/vimdiff2 b/mergetools/vimdiff2
>> new file mode 100644
>> index 0000000..f5890b1
>> --- /dev/null
>> +++ b/mergetools/vimdiff2
>> @@ -0,0 +1 @@
>> +. "$MERGE_TOOLS_DIR/include/vim.sh"
--
David
^ permalink raw reply
* [PATCH v2 2/2] git-p4.py: support Python 2.4
From: Brandon Casey @ 2013-01-26 19:14 UTC (permalink / raw)
To: gitster; +Cc: pw, git, esr, john, Brandon Casey, Brandon Casey
In-Reply-To: <1359227673-5673-1-git-send-email-bcasey@nvidia.com>
From: Brandon Casey <drafnel@gmail.com>
Python 2.4 lacks the following features:
subprocess.check_call
struct.pack_into
Take a cue from 460d1026 and provide an implementation of the
CalledProcessError exception. Then replace the calls to
subproccess.check_call with calls to subprocess.call that check the return
status and raise a CalledProcessError exception if necessary.
The struct.pack_into in t/9802 can be converted into a single struct.pack
call which is available in Python 2.4.
Signed-off-by: Brandon Casey <bcasey@nvidia.com>
Acked-by: Pete Wyckoff <pw@padd.com>
---
On 1/26/2013 4:48 AM, Pete Wyckoff wrote:
> One stray semicolon.
Fixed.
-Brandon
INSTALL | 2 +-
git-p4.py | 27 ++++++++++++++++++++++++---
t/t9802-git-p4-filetype.sh | 11 ++++++-----
3 files changed, 31 insertions(+), 9 deletions(-)
diff --git a/INSTALL b/INSTALL
index fc723b3..b96e16d 100644
--- a/INSTALL
+++ b/INSTALL
@@ -131,7 +131,7 @@ Issues of note:
use English. Under autoconf the configure script will do this
automatically if it can't find libintl on the system.
- - Python version 2.5 or later is needed to use the git-p4
+ - Python version 2.4 or later is needed to use the git-p4
interface to Perforce.
- Some platform specific issues are dealt with Makefile rules,
diff --git a/git-p4.py b/git-p4.py
index de1a0b9..625a396 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -18,6 +18,21 @@ import optparse, os, marshal, subprocess, shelve
import tempfile, getopt, os.path, time, platform
import re, shutil
+try:
+ from subprocess import CalledProcessError
+except ImportError:
+ # from python2.7:subprocess.py
+ # Exception classes used by this module.
+ class CalledProcessError(Exception):
+ """This exception is raised when a process run by check_call() returns
+ a non-zero exit status. The exit status will be stored in the
+ returncode attribute."""
+ def __init__(self, returncode, cmd):
+ self.returncode = returncode
+ self.cmd = cmd
+ def __str__(self):
+ return "Command '%s' returned non-zero exit status %d" % (self.cmd, self.returncode)
+
verbose = False
# Only labels/tags matching this will be imported/exported
@@ -158,13 +173,17 @@ def system(cmd):
expand = isinstance(cmd,basestring)
if verbose:
sys.stderr.write("executing %s\n" % str(cmd))
- subprocess.check_call(cmd, shell=expand)
+ retcode = subprocess.call(cmd, shell=expand)
+ if retcode:
+ raise CalledProcessError(retcode, cmd)
def p4_system(cmd):
"""Specifically invoke p4 as the system command. """
real_cmd = p4_build_cmd(cmd)
expand = isinstance(real_cmd, basestring)
- subprocess.check_call(real_cmd, shell=expand)
+ retcode = subprocess.call(real_cmd, shell=expand)
+ if retcode:
+ raise CalledProcessError(retcode, real_cmd)
def p4_integrate(src, dest):
p4_system(["integrate", "-Dt", wildcard_encode(src), wildcard_encode(dest)])
@@ -3174,7 +3193,9 @@ class P4Clone(P4Sync):
init_cmd = [ "git", "init" ]
if self.cloneBare:
init_cmd.append("--bare")
- subprocess.check_call(init_cmd)
+ retcode = subprocess.call(init_cmd)
+ if retcode:
+ raise CalledProcessError(retcode, init_cmd)
if not P4Sync.run(self, depotPaths):
return False
diff --git a/t/t9802-git-p4-filetype.sh b/t/t9802-git-p4-filetype.sh
index 21924df..aae1a3f 100755
--- a/t/t9802-git-p4-filetype.sh
+++ b/t/t9802-git-p4-filetype.sh
@@ -105,12 +105,13 @@ build_gendouble() {
cat >gendouble.py <<-\EOF
import sys
import struct
- import array
- s = array.array("c", '\0' * 26)
- struct.pack_into(">L", s, 0, 0x00051607) # AppleDouble
- struct.pack_into(">L", s, 4, 0x00020000) # version 2
- s.tofile(sys.stdout)
+ s = struct.pack(">LL18s",
+ 0x00051607, # AppleDouble
+ 0x00020000, # version 2
+ "" # pad to 26 bytes
+ )
+ sys.stdout.write(s)
EOF
}
--
1.8.1.1.442.g413e803
-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information. Any unauthorized review, use, disclosure or distribution
is prohibited. If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------
^ permalink raw reply related
* [PATCH v2 1/2] git-p4.py: support Python 2.5
From: Brandon Casey @ 2013-01-26 19:14 UTC (permalink / raw)
To: gitster; +Cc: pw, git, esr, john, Brandon Casey, Brandon Casey
From: Brandon Casey <drafnel@gmail.com>
Python 2.5 and older do not accept None as the first argument to
translate() and complain with:
TypeError: expected a character buffer object
As suggested by Pete Wyckoff, let's just replace the call to translate()
with a regex search which should be more clear and more portable.
This allows git-p4 to be used with Python 2.5.
Signed-off-by: Brandon Casey <bcasey@nvidia.com>
---
INSTALL | 2 +-
git-p4.py | 3 ++-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/INSTALL b/INSTALL
index 28f34bd..fc723b3 100644
--- a/INSTALL
+++ b/INSTALL
@@ -131,7 +131,7 @@ Issues of note:
use English. Under autoconf the configure script will do this
automatically if it can't find libintl on the system.
- - Python version 2.6 or later is needed to use the git-p4
+ - Python version 2.5 or later is needed to use the git-p4
interface to Perforce.
- Some platform specific issues are dealt with Makefile rules,
diff --git a/git-p4.py b/git-p4.py
index 2da5649..de1a0b9 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -768,7 +768,8 @@ def wildcard_encode(path):
return path
def wildcard_present(path):
- return path.translate(None, "*#@%") != path
+ m = re.search("[*#@%]", path)
+ return m is not None
class Command:
def __init__(self):
--
1.8.1.1.442.g413e803
-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information. Any unauthorized review, use, disclosure or distribution
is prohibited. If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------
^ permalink raw reply related
* Re: [PATCH 2/2] git-p4.py: support Python 2.4
From: Brandon Casey @ 2013-01-26 19:02 UTC (permalink / raw)
To: Pete Wyckoff; +Cc: git, esr, john, Brandon Casey
In-Reply-To: <20130126124854.GB31052@padd.com>
On Sat, Jan 26, 2013 at 4:48 AM, Pete Wyckoff <pw@padd.com> wrote:
> drafnel@gmail.com wrote on Fri, 25 Jan 2013 12:44 -0800:
>> + sys.stdout.write(s);
> One stray semicolon.
Whoops. Thanks.
> In terms of maintenance, I'll not run tests with 2.4 or 2.5
> myself, but maybe you would be willing to check an RC candidate
> each release?
Not a problem. I'm sure it will get run much more often then that.
-Brandon
^ permalink raw reply
* Port 22
From: Craig Christensen @ 2013-01-26 18:56 UTC (permalink / raw)
To: git
I am currently a student at Brigham Young University - Idaho and we are use Pagoda Box and Git for our Mobile Apps class. However, the school's network has blocked incoming trafic on port 22. I have been searching through all the tutorials and documents provided by Pagoda Box and Git but have not been able to find a solution to solve this problem. We can use sftp but we then have to manually deploy the latest using the admin panel. Can you help provide a simple solution?
Thanks,
Craig W Christensen
cwcraigo@gmail.com
chr07035@byui.edu
^ permalink raw reply
* Re: [PATCH 1/2] git-p4.py: support Python 2.5
From: Brandon Casey @ 2013-01-26 18:19 UTC (permalink / raw)
To: Pete Wyckoff; +Cc: git, esr, john, Brandon Casey
In-Reply-To: <20130126124510.GA31052@padd.com>
On Sat, Jan 26, 2013 at 4:45 AM, Pete Wyckoff <pw@padd.com> wrote:
> drafnel@gmail.com wrote on Fri, 25 Jan 2013 12:44 -0800:
>> Python 2.5 and older do not accept None as the first argument to
>> translate() and complain with:
>>
>> TypeError: expected a character buffer object
>>
>> Satisfy this older python by calling maketrans() to generate an empty
>> translation table and supplying that to translate().
>>
>> This allows git-p4 to be used with Python 2.5.
>
> This was a lot easier than I imagined!
>
>> def wildcard_present(path):
>> - return path.translate(None, "*#@%") != path
>> + from string import maketrans
>> + return path.translate(maketrans("",""), "*#@%") != path
>
> translate() was a bit too subtle already. Could you try
> something like this instead?
>
> m = re.search("[*#@%]", path)
> return m is not None
>
> I think that'll work everywhere and not force people to look
> up how translate and maketrans work.
Yes that's simpler and works fine.
-Brandon
^ permalink raw reply
* Re: [PATCH v3 6/8] git-remote-testpy: hash bytes explicitly
From: John Keeping @ 2013-01-26 17:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Sverre Rabbelier
In-Reply-To: <611a44568bdc969bcfa3d7d870560855e00baf1e.1358686905.git.john@keeping.me.uk>
Under Python 3 'hasher.update(...)' must take a byte string and not a
unicode string. Explicitly encode the argument to this method as UTF-8
bytes. This is safe since we are encoding a Python Unicode string to a
Unicode encoding.
This changes the directory used by git-remote-testpy for its git mirror
of the remote repository, but this tool should not have any serious
users as it is used primarily to test the Python remote helper
framework.
The use of encode() moves the required Python version forward to 2.0.
Signed-off-by: John Keeping <john@keeping.me.uk>
---
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.
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(
--
1.8.1.1
^ permalink raw reply related
* [feature request] git add completion should exclude staged content
From: wookietreiber @ 2013-01-26 17:21 UTC (permalink / raw)
To: git
Dear Git Hackers,
I have a feature request for `git add` auto completion:
`git add` auto completion suggests all files / directories, filtered by nothing. I guess it would be
much nicer (as in increasing productivity) if it would only suggest unstaged content, as reported by
`git status`, because that would be the only content one would be able to add.
Example:
$ git status
# On branch develop
# Changes to be committed:
# (use "git reset HEAD <file>..." to unstage)
#
# modified: src/main/scala/XYChart.scala
#
# Untracked files:
# (use "git add <file>..." to include in what will be committed)
#
# notes/0.2.0.markdown
$ git add <auto-complete>
build.sbt .ensime .git/ project/ scalastyle-config.xml todo
COPYING .ensime_lucene/ notes/ README.md src/
$ git add
Where it should be:
$ git add <auto-complete>
$ git add notes/0.2.0.markdown
... because `notes/0.2.0.markdown` is the only thing I can add.
--
Beste Grüße / Best Regards
Christian Krause aka wookietreiber
-----------------------------------------------------------------------
EGAL WIE DICHT DU BIST, GOETHE WAR DICHTER.
^ permalink raw reply
* Re: [PATCH/RFC] mingw: rename WIN32 cpp macro to NATIVE_WINDOWS
From: Torsten Bögershausen @ 2013-01-26 17:21 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Eric Blake, Junio C Hamano, Mark Levedahl, Ramsay Jones,
Alex Riesen, Jason Pyeron, git, Torsten Bögershausen,
Stephen & Linda Smith
In-Reply-To: <20130126010359.GH3341@elie.Belkin>
On 26.01.13 02:03, Jonathan Nieder wrote:
> Throughout git, it is assumed that the WIN32 preprocessor symbol is
> defined on native Windows setups (mingw and msvc) and not on Cygwin.
> On Cygwin, most of the time git can pretend this is just another Unix
> machine, and Windows-specific magic is generally counterproductive.
>
> Unfortunately Cygwin *does* define the WIN32 symbol in some headers.
> Best to rely on a new git-specific symbol NATIVE_WINDOWS instead,
> defined as follows:
>
> #if defined(WIN32) && !defined(__CYGWIN__)
> # define NATIVE_WINDOWS
> #endif
>
> After this change, it should be possible to drop the
> CYGWIN_V15_WIN32API setting without any negative effect.
>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> Eric Blake wrote:
>
>> Which is why other projects, like gnulib, have
>>
>> # if (defined _WIN32 || defined __WIN32__) && ! defined __CYGWIN__
>>
>> all over the place.
> So, how about this?
>
> Completely untested.
>
> abspath.c | 2 +-
> compat/terminal.c | 4 ++--
> compat/win32.h | 2 +-
> diff-no-index.c | 2 +-
> git-compat-util.h | 3 ++-
> help.c | 2 +-
> run-command.c | 10 +++++-----
> test-chmtime.c | 2 +-
> thread-utils.c | 2 +-
> 9 files changed, 15 insertions(+), 14 deletions(-)
>
> diff --git a/abspath.c b/abspath.c
> index 40cdc462..c7d5458e 100644
> --- a/abspath.c
> +++ b/abspath.c
> @@ -216,7 +216,7 @@ const char *absolute_path(const char *path)
> const char *prefix_filename(const char *pfx, int pfx_len, const char *arg)
> {
> static char path[PATH_MAX];
> -#ifndef WIN32
> +#ifndef WINDOWS_NATIVE
> if (!pfx_len || is_absolute_path(arg))
> return arg;
> memcpy(path, pfx, pfx_len);
> diff --git a/compat/terminal.c b/compat/terminal.c
> index 9b5e3d1b..136e4a74 100644
> --- a/compat/terminal.c
> +++ b/compat/terminal.c
> @@ -3,7 +3,7 @@
> #include "sigchain.h"
> #include "strbuf.h"
>
> -#if defined(HAVE_DEV_TTY) || defined(WIN32)
> +#if defined(HAVE_DEV_TTY) || defined(WINDOWS_NATIVE)
>
> static void restore_term(void);
>
> @@ -53,7 +53,7 @@ error:
> return -1;
> }
>
> -#elif defined(WIN32)
> +#elif defined(WINDOWS_NATIVE)
>
> #define INPUT_PATH "CONIN$"
> #define OUTPUT_PATH "CONOUT$"
> diff --git a/compat/win32.h b/compat/win32.h
> index 8ce91048..31dd30f7 100644
> --- a/compat/win32.h
> +++ b/compat/win32.h
> @@ -2,7 +2,7 @@
> #define WIN32_H
>
> /* common Win32 functions for MinGW and Cygwin */
> -#ifndef WIN32 /* Not defined by Cygwin */
> +#ifndef WINDOWS_NATIVE /* Not defined for Cygwin */
> #include <windows.h>
> #endif
>
> diff --git a/diff-no-index.c b/diff-no-index.c
> index 74da6593..a0bc9c50 100644
> --- a/diff-no-index.c
> +++ b/diff-no-index.c
> @@ -45,7 +45,7 @@ static int get_mode(const char *path, int *mode)
>
> if (!path || !strcmp(path, "/dev/null"))
> *mode = 0;
> -#ifdef _WIN32
> +#ifdef WINDOWS_NATIVE
> else if (!strcasecmp(path, "nul"))
> *mode = 0;
> #endif
> diff --git a/git-compat-util.h b/git-compat-util.h
> index e5a4b745..ebbdff53 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -85,10 +85,11 @@
> #define _NETBSD_SOURCE 1
> #define _SGI_SOURCE 1
>
> -#ifdef WIN32 /* Both MinGW and MSVC */
> +#if defined(WIN32) && !defined(__CYGWIN__) /* Both MinGW and MSVC */
> #define WIN32_LEAN_AND_MEAN /* stops windows.h including winsock.h */
> #include <winsock2.h>
> #include <windows.h>
> +#define WINDOWS_NATIVE
> #endif
>
> #include <unistd.h>
> diff --git a/help.c b/help.c
> index 2a42ec6d..cc1e63f7 100644
> --- a/help.c
> +++ b/help.c
> @@ -106,7 +106,7 @@ static int is_executable(const char *name)
> !S_ISREG(st.st_mode))
> return 0;
>
> -#if defined(WIN32) || defined(__CYGWIN__)
> +#if defined(WINDOWS_NATIVE) || defined(__CYGWIN__)
> #if defined(__CYGWIN__)
> if ((st.st_mode & S_IXUSR) == 0)
> #endif
> diff --git a/run-command.c b/run-command.c
> index 04712191..04ac6181 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -72,7 +72,7 @@ static inline void close_pair(int fd[2])
> close(fd[1]);
> }
>
> -#ifndef WIN32
> +#ifndef WINDOWS_NATIVE
> static inline void dup_devnull(int to)
> {
> int fd = open("/dev/null", O_RDWR);
> @@ -159,7 +159,7 @@ static const char **prepare_shell_cmd(const char **argv)
> die("BUG: shell command is empty");
>
> if (strcspn(argv[0], "|&;<>()$`\\\"' \t\n*?[#~=%") != strlen(argv[0])) {
> -#ifndef WIN32
> +#ifndef WINDOWS_NATIVE
> nargv[nargc++] = SHELL_PATH;
> #else
> nargv[nargc++] = "sh";
> @@ -182,7 +182,7 @@ static const char **prepare_shell_cmd(const char **argv)
> return nargv;
> }
>
> -#ifndef WIN32
> +#ifndef WINDOWS_NATIVE
> static int execv_shell_cmd(const char **argv)
> {
> const char **nargv = prepare_shell_cmd(argv);
> @@ -193,7 +193,7 @@ static int execv_shell_cmd(const char **argv)
> }
> #endif
>
> -#ifndef WIN32
> +#ifndef WINDOWS_NATIVE
> static int child_err = 2;
> static int child_notifier = -1;
>
> @@ -330,7 +330,7 @@ fail_pipe:
> trace_argv_printf(cmd->argv, "trace: run_command:");
> fflush(NULL);
>
> -#ifndef WIN32
> +#ifndef WINDOWS_NATIVE
> {
> int notify_pipe[2];
> if (pipe(notify_pipe))
> diff --git a/test-chmtime.c b/test-chmtime.c
> index 92713d16..803b6055 100644
> --- a/test-chmtime.c
> +++ b/test-chmtime.c
> @@ -87,7 +87,7 @@ int main(int argc, const char *argv[])
> return -1;
> }
>
> -#ifdef WIN32
> +#ifdef WINDOWS_NATIVE
> if (!(sb.st_mode & S_IWUSR) &&
> chmod(argv[i], sb.st_mode | S_IWUSR)) {
> fprintf(stderr, "Could not make user-writable %s: %s",
> diff --git a/thread-utils.c b/thread-utils.c
> index 7f4b76a9..4c4cf2fa 100644
> --- a/thread-utils.c
> +++ b/thread-utils.c
> @@ -24,7 +24,7 @@ int online_cpus(void)
> long ncpus;
> #endif
>
> -#ifdef _WIN32
> +#ifdef WINDOWS_NATIVE
> SYSTEM_INFO info;
> GetSystemInfo(&info);
>
Thanks, that looks good.
I run the test suite under XP, the same test cases are broken as on 1.8.1.1
/Torsten
^ permalink raw reply
* Re: [PATCH/RFC] mingw: rename WIN32 cpp macro to NATIVE_WINDOWS
From: Mark Levedahl @ 2013-01-26 14:11 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Eric Blake, Junio C Hamano, Ramsay Jones, Alex Riesen,
Jason Pyeron, git, Torsten Bögershausen,
Stephen & Linda Smith
In-Reply-To: <20130126010359.GH3341@elie.Belkin>
On 01/25/2013 08:03 PM, Jonathan Nieder wrote:
> diff --git a/abspath.c b/abspath.c
> index 40cdc462..c7d5458e 100644
> --- a/abspath.c
> +++ b/abspath.c
> @@ -216,7 +216,7 @@ const char *absolute_path(const char *path)
> const char *prefix_filename(const char *pfx, int pfx_len, const char *arg)
> {
> static char path[PATH_MAX];
> -#ifndef WIN32
> +#ifndef WINDOWS_NATIVE
> if (!pfx_len || is_absolute_path(arg))
> return arg;
> memcpy(path, pfx, pfx_len);
> diff --git a/compat/terminal.c b/compat/terminal.c
> index 9b5e3d1b..136e4a74 100644
Maybe WINDOWS_NATIVE should be defined in config.mak.uname?
Otherwise, I tested the patch and it does build / run on Cygwin, but I
cannot run a test suite until next week. I am concerned about subtle
changes due to the various WIN32 tests that were not guarded by
__CYGWIN__ before, haven't stared at the code enough to see if there
could be an issue.
Mark
^ permalink raw reply
* [PATCH 1/2 v3] mergetool--lib: don't call "exit" in setup_tool
From: John Keeping @ 2013-01-26 13:50 UTC (permalink / raw)
To: Junio C Hamano; +Cc: David Aguilar, git
In-Reply-To: <20130126121721.GI7498@serenity.lan>
This will make it easier to use setup_tool in places where we expect
that the selected tool will not support the current mode.
We need to introduce a new return code for setup_tool to differentiate
between the case of "the selected tool is invalid" and "the selected
tool is not a built-in" since we must call setup_tool when a custom
'merge.<tool>.path' is configured for a built-in tool but avoid failing
when the configured tool is not a built-in.
Signed-off-by: John Keeping <john@keeping.me.uk>
---
> On Fri, Jan 25, 2013 at 04:24:03PM -0800, Junio C Hamano wrote:
> > Applying this one on top of 1/7 thru 5/7 and 7/7 seems to break
> > t7610 rather badly.
>
> Sorry about that. The 'setup_tool' function should really be called
> 'setup_builtin_tool' - it isn't necessary when a custom mergetool is
> configured and will return 1 when called with an argument that isn't a
> builtin tool from $GIT_EXEC_PATH/mergetools.
>
> The change is the second hunk below which now wraps the call to
> setup_tool in an if block as well as adding the "|| return".
Now that I've run the entire test suite, that still wasn't correct since
it did not correctly handle the case where the user overrides the path
for one of the built-in mergetools.
git-mergetool--lib.sh | 23 ++++++++++++++++++++---
1 file changed, 20 insertions(+), 3 deletions(-)
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 4c1e129..dd4f088 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -58,7 +58,11 @@ setup_tool () {
. "$mergetools/defaults"
if ! test -f "$mergetools/$tool"
then
- return 1
+ # Use a special return code for this case since we want to
+ # source "defaults" even when an explicit tool path is
+ # configured since the user can use that to override the
+ # default path in the scriptlet.
+ return 2
fi
# Load the redefined functions
@@ -67,11 +71,11 @@ setup_tool () {
if merge_mode && ! can_merge
then
echo "error: '$tool' can not be used to resolve merges" >&2
- exit 1
+ return 1
elif diff_mode && ! can_diff
then
echo "error: '$tool' can only be used to resolve merges" >&2
- exit 1
+ return 1
fi
return 0
}
@@ -101,6 +105,19 @@ run_merge_tool () {
# Bring tool-specific functions into scope
setup_tool "$1"
+ exitcode=$?
+ case $exitcode in
+ 0)
+ :
+ ;;
+ 2)
+ # The configured tool is not a built-in tool.
+ test -n "$merge_tool_path" || return 1
+ ;;
+ *)
+ return $exitcode
+ ;;
+ esac
if merge_mode
then
--
1.8.1
^ permalink raw reply related
* Re: [PATCH 2/2] git-p4.py: support Python 2.4
From: Pete Wyckoff @ 2013-01-26 12:48 UTC (permalink / raw)
To: Brandon Casey; +Cc: git, esr, john, Brandon Casey
In-Reply-To: <1359146641-27810-3-git-send-email-drafnel@gmail.com>
drafnel@gmail.com wrote on Fri, 25 Jan 2013 12:44 -0800:
> Python 2.4 lacks the following features:
>
> subprocess.check_call
> struct.pack_into
>
> Take a cue from 460d1026 and provide an implementation of the
> CalledProcessError exception. Then replace the calls to
> subproccess.check_call with calls to subprocess.call that check the return
> status and raise a CalledProcessError exception if necessary.
>
> The struct.pack_into in t/9802 can be converted into a single struct.pack
> call which is available in Python 2.4.
Excellent. Should have used struct.pack() from the get-go.
Acked-by: Pete Wyckoff <pw@padd.com>
> diff --git a/t/t9802-git-p4-filetype.sh b/t/t9802-git-p4-filetype.sh
> index 21924df..be299dc 100755
> --- a/t/t9802-git-p4-filetype.sh
> +++ b/t/t9802-git-p4-filetype.sh
> @@ -105,12 +105,13 @@ build_gendouble() {
> cat >gendouble.py <<-\EOF
> import sys
> import struct
> - import array
>
> - s = array.array("c", '\0' * 26)
> - struct.pack_into(">L", s, 0, 0x00051607) # AppleDouble
> - struct.pack_into(">L", s, 4, 0x00020000) # version 2
> - s.tofile(sys.stdout)
> + s = struct.pack(">LL18s",
> + 0x00051607, # AppleDouble
> + 0x00020000, # version 2
> + "" # pad to 26 bytes
> + )
> + sys.stdout.write(s);
> EOF
One stray semicolon.
In terms of maintenance, I'll not run tests with 2.4 or 2.5
myself, but maybe you would be willing to check an RC candidate
each release?
-- Pete
^ 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