* [PATCH] mergetools: Simplify how we handle "vim" and "defaults"
From: David Aguilar @ 2013-01-26 6:50 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>
---
This should make things ok on platforms where we don't have symlinks.
Makefile | 2 +-
git-mergetool--lib.sh | 41 +++++++++++-----------------
mergetools/gvimdiff | 1 +
mergetools/gvimdiff2 | 1 +
mergetools/{defaults => include/defaults.sh} | 0
mergetools/{vim => include/vim.sh} | 0
mergetools/vimdiff | 1 +
mergetools/vimdiff2 | 1 +
8 files changed, 21 insertions(+), 26 deletions(-)
create mode 100644 mergetools/gvimdiff
create mode 100644 mergetools/gvimdiff2
rename mergetools/{defaults => include/defaults.sh} (100%)
rename mergetools/{vim => include/vim.sh} (100%)
create mode 100644 mergetools/vimdiff
create mode 100644 mergetools/vimdiff2
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)'
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"
- # 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
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
+ 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"
--
1.8.0.6.ge6f188f.dirty
^ permalink raw reply related
* Re: [PATCH] tests: turn on test-lint-shell-syntax by default
From: Torsten Bögershausen @ 2013-01-26 6:57 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Torsten Bögershausen, Jonathan Nieder, git, kraai
In-Reply-To: <7vk3re2ncb.fsf@alter.siamese.dyndns.org>
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:
>>
>> --- a/t/test-lib.sh
>> +++ b/t/test-lib.sh
>> @@ -644,6 +644,10 @@ yes () {
>> :
>> done
>> }
>> +which () {
>> + echo >&2 "which is not portable (please use type)"
>> + exit 1
>> +}
>>
>>
>> This will happen in runtime, which might be good enough ?
>
> if (
> which frotz &&
> test $(frobonitz --version" -le 2.0
> )
> then
> test_set_prereq FROTZ_FROBONITZ
> else
> echo >&2 "suitable Frotz/Frobonitz combo not available;"
> echo >&2 "some tests may be skipped"
> fi
>
> I somehow think this is a lost cause.
I found different ways to detect if frotz is installed in the test suite:
a) use "type" (Should be the fastest ?)
b) call the command directly, check the exit code
c) "! test_have_prereq" (easy to understand, propably most expensive ?)
Do we really need "which" to detect if frotz is installed?
/Torsten
=============
if ! type cvs >/dev/null 2>&1
then
skip_all='skipping cvsimport tests, cvs not found'
test_done
fi
===========
if test -n "$BASH" && test -z "$POSIXLY_CORRECT"; then
# we are in full-on bash mode
true
elif type bash >/dev/null 2>&1; then
# execute in full-on bash mode
unset POSIXLY_CORRECT
exec bash "$0" "$@"
else
echo '1..0 #SKIP skipping bash completion tests; bash not available'
exit 0
fi
===============
svn >/dev/null 2>&1
if test $? -ne 1
then
skip_all='skipping git svn tests, svn not found'
test_done
fi
===============
( p4 -h && p4d -h ) >/dev/null 2>&1 || {
skip_all='skipping git p4 tests; no p4 or p4d'
test_done
}
===============
if ! test_have_prereq PERL; then
skip_all='skipping gitweb tests, perl not available'
test_done
fi
^ permalink raw reply
* Re: [PATCH 8/7] mergetool--lib: don't call "exit" in setup_tool
From: David Aguilar @ 2013-01-26 7:01 UTC (permalink / raw)
To: Junio C Hamano; +Cc: John Keeping, git
In-Reply-To: <7vip6k23mk.fsf@alter.siamese.dyndns.org>
On Fri, Jan 25, 2013 at 4:24 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Applying this one on top of 1/7 thru 5/7 and 7/7 seems to break
> t7610 rather badly.
I just sent a replacement for the vim/symlink issue stuff.
I tried to keep the patch small. John, can you rebase this
patch on top of it?
> --- >8 ------ >8 ------ >8 ------ >8 ------ >8 ------ >8 ---
> ...
> ok 1 - setup
>
> expecting success:
> git checkout -b test1 branch1 &&
> git submodule update -N &&
> test_must_fail git merge master >/dev/null 2>&1 &&
> ( yes "" | git mergetool both >/dev/null 2>&1 ) &&
> ( yes "" | git mergetool file1 file1 ) &&
> ( yes "" | git mergetool file2 "spaced name" >/dev/null 2>&1 ) &&
> ( yes "" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&
> ( yes "d" | git mergetool file11 >/dev/null 2>&1 ) &&
> ( yes "d" | git mergetool file12 >/dev/null 2>&1 ) &&
> ( yes "l" | git mergetool submod >/dev/null 2>&1 ) &&
> test "$(cat file1)" = "master updated" &&
> test "$(cat file2)" = "master new" &&
> test "$(cat subdir/file3)" = "master new sub" &&
> test "$(cat submod/bar)" = "branch1 submodule" &&
> git commit -m "branch1 resolved with mergetool"
>
> M submod
> Switched to a new branch 'test1'
> Submodule path 'submod': checked out '39c7f044ed2e6a9cebd5266529badd181c8762b5'
> not ok - 2 custom mergetool
> #
> # git checkout -b test1 branch1 &&
> # git submodule update -N &&
> # test_must_fail git merge master >/dev/null 2>&1 &&
> # ( yes "" | git mergetool both >/dev/null 2>&1 ) &&
> # ( yes "" | git mergetool file1 file1 ) &&
> # ( yes "" | git mergetool file2 "spaced name" >/dev/null 2>&1 ) &&
> # ( yes "" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&
> # ( yes "d" | git mergetool file11 >/dev/null 2>&1 ) &&
> # ( yes "d" | git mergetool file12 >/dev/null 2>&1 ) &&
> # ( yes "l" | git mergetool submod >/dev/null 2>&1 ) &&
> # test "$(cat file1)" = "master updated" &&
> # test "$(cat file2)" = "master new" &&
> # test "$(cat subdir/file3)" = "master new sub" &&
> # test "$(cat submod/bar)" = "branch1 submodule" &&
> # git commit -m "branch1 resolved with mergetool"
> #
> --- 8< ------ 8< ------ 8< ------ 8< ------ 8< ------ 8< ---
>
> Due to ">dev/null 2>&1", all of the error clues are hidden, and I
> didn't dig further to see which one was failing (this is why tests
> shouldn't do these in general).
--
David
^ permalink raw reply
* Re: [PATCH 2/2] mergetools: Make tortoisemerge work with
From: David Aguilar @ 2013-01-26 7:10 UTC (permalink / raw)
To: Sven Strickroth; +Cc: Junio C Hamano, git, Sebastian Schuberth, Jeff King
In-Reply-To: <51032E96.2040209@tu-clausthal.de>
On Fri, Jan 25, 2013 at 5:17 PM, Sven Strickroth
<sven.strickroth@tu-clausthal.de> wrote:
> TortoiseGitMerge now separates cli parameter key-values by space instead
> of colons as TortoiseSVN TortoiseMerge does and supports filesnames
> with spaces in it this way now.
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.
--
David
^ permalink raw reply
* [PATCH 0/3] lazily load commit->buffer
From: Jeff King @ 2013-01-26 9:40 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: <7vip6l5l71.fsf@alter.siamese.dyndns.org>
On Fri, Jan 25, 2013 at 07:36:18AM -0800, Junio C Hamano wrote:
> Jonathon Mah <jmah@me.com> writes:
>
> > Just to note, the proposals so far don't prevent a "smart-ass"
> > function from freeing the buffer when it's called underneath the
> > use/release scope, as in:
> >
> > with_commit_buffer(commit); {
> > fn1_needing_buffer(commit);
> > walk_rev_tree_or_something();
> > fn2_needing_buffer(commit);
> > } done_with_commit_buffer(commit);
>
> I think the goal of everybody discussing these ideas is to make sure
> that all code follows the simple ownership policy proposed at the
> beginning of this subthread: commit->buffer belongs to the revision
> traversal machinery, and other users could borrow it when available.
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.
-Peff
^ permalink raw reply
* [PATCH 1/3] commit: drop useless xstrdup of commit message
From: Jeff King @ 2013-01-26 9:42 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: <20130126094026.GA9646@sigill.intra.peff.net>
When git-commit is asked to reuse a commit message via "-c",
we call read_commit_message, which looks up the commit and
hands back either the re-encoded result, or a copy of the
original. We make a copy in the latter case so that the
ownership semantics of the return value are clear (in either
case, it can be freed).
However, since we return a "const char *", and since the
resulting buffer's lifetime is the same as that of the whole
program, we never bother to free it at all.
Let's just drop the copy. That saves us a copy in the common
case. While it does mean we leak in the re-encode case, it
doesn't matter, since we are relying on program exit to free
the memory anyway.
Signed-off-by: Jeff King <peff@peff.net>
---
This one isn't strictly necessary, but it makes it a lot more obvious
what is going on with the memory ownership of this code in the next
patch.
builtin/commit.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 38b9a9c..fbbb40f 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -962,7 +962,7 @@ static const char *read_commit_message(const char *name)
* encodings are identical.
*/
if (out == NULL)
- out = xstrdup(commit->buffer);
+ out = commit->buffer;
return out;
}
--
1.8.0.2.16.g72e2fc9
^ permalink raw reply related
* [PATCH 2/3] logmsg_reencode: never return NULL
From: Jeff King @ 2013-01-26 9:44 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: <20130126094026.GA9646@sigill.intra.peff.net>
The logmsg_reencode function will return the reencoded
commit buffer, or NULL if reencoding failed or no reencoding
was necessary. Since every caller then ends up checking for NULL
and just using the commit's original buffer, anyway, we can
be a bit more helpful and just return that buffer when we
would have returned NULL.
Since the resulting string may or may not need to be freed,
we introduce a logmsg_free, which checks whether the buffer
came from the commit object or not (callers either
implemented the same check already, or kept two separate
pointers, one to mark the buffer to be used, and one for the
to-be-freed string).
Pushing this logic into logmsg_* simplifies the callers, and
will let future patches lazily load the commit buffer in a
single place.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/blame.c | 9 ++++-----
builtin/commit.c | 14 ++------------
commit.h | 1 +
pretty.c | 38 ++++++++++++++++++++++----------------
4 files changed, 29 insertions(+), 33 deletions(-)
diff --git a/builtin/blame.c b/builtin/blame.c
index b431ba3..962e4e3 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1420,7 +1420,7 @@ static void get_commit_info(struct commit *commit,
{
int len;
const char *subject, *encoding;
- char *reencoded, *message;
+ char *message;
commit_info_init(ret);
@@ -1438,14 +1438,13 @@ static void get_commit_info(struct commit *commit,
sha1_to_hex(commit->object.sha1));
}
encoding = get_log_output_encoding();
- reencoded = logmsg_reencode(commit, encoding);
- message = reencoded ? reencoded : commit->buffer;
+ message = logmsg_reencode(commit, encoding);
get_ac_line(message, "\nauthor ",
&ret->author, &ret->author_mail,
&ret->author_time, &ret->author_tz);
if (!detailed) {
- free(reencoded);
+ logmsg_free(message, commit);
return;
}
@@ -1459,7 +1458,7 @@ static void get_commit_info(struct commit *commit,
else
strbuf_addf(&ret->summary, "(%s)", sha1_to_hex(commit->object.sha1));
- free(reencoded);
+ logmsg_free(message, commit);
}
/*
diff --git a/builtin/commit.c b/builtin/commit.c
index fbbb40f..6169f1e 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -946,24 +946,14 @@ static const char *read_commit_message(const char *name)
static const char *read_commit_message(const char *name)
{
- const char *out_enc, *out;
+ const char *out_enc;
struct commit *commit;
commit = lookup_commit_reference_by_name(name);
if (!commit)
die(_("could not lookup commit %s"), name);
out_enc = get_commit_output_encoding();
- out = logmsg_reencode(commit, out_enc);
-
- /*
- * If we failed to reencode the buffer, just copy it
- * byte for byte so the user can try to fix it up.
- * This also handles the case where input and output
- * encodings are identical.
- */
- if (out == NULL)
- out = commit->buffer;
- return out;
+ return logmsg_reencode(commit, out_enc);
}
static int parse_and_validate_options(int argc, const char *argv[],
diff --git a/commit.h b/commit.h
index c16c8a7..e770649 100644
--- a/commit.h
+++ b/commit.h
@@ -101,6 +101,7 @@ extern char *logmsg_reencode(const struct commit *commit,
struct rev_info; /* in revision.h, it circularly uses enum cmit_fmt */
extern char *logmsg_reencode(const struct commit *commit,
const char *output_encoding);
+extern void logmsg_free(char *msg, const struct commit *commit);
extern void get_commit_format(const char *arg, struct rev_info *);
extern const char *format_subject(struct strbuf *sb, const char *msg,
const char *line_separator);
diff --git a/pretty.c b/pretty.c
index 07fc062..c675349 100644
--- a/pretty.c
+++ b/pretty.c
@@ -524,10 +524,11 @@ static char *get_header(const struct commit *commit, const char *key)
strbuf_addch(sb, '\n');
}
-static char *get_header(const struct commit *commit, const char *key)
+static char *get_header(const struct commit *commit, const char *msg,
+ const char *key)
{
int key_len = strlen(key);
- const char *line = commit->buffer;
+ const char *line = msg;
while (line) {
const char *eol = strchr(line, '\n'), *next;
@@ -588,17 +589,18 @@ char *logmsg_reencode(const struct commit *commit,
static const char *utf8 = "UTF-8";
const char *use_encoding;
char *encoding;
+ char *msg = commit->buffer;
char *out;
if (!output_encoding || !*output_encoding)
- return NULL;
- encoding = get_header(commit, "encoding");
+ return msg;
+ encoding = get_header(commit, msg, "encoding");
use_encoding = encoding ? encoding : utf8;
if (same_encoding(use_encoding, output_encoding))
if (encoding) /* we'll strip encoding header later */
out = xstrdup(commit->buffer);
else
- return NULL; /* nothing to do */
+ return msg; /* nothing to do */
else
out = reencode_string(commit->buffer,
output_encoding, use_encoding);
@@ -606,7 +608,17 @@ char *logmsg_reencode(const struct commit *commit,
out = replace_encoding_header(out, output_encoding);
free(encoding);
- return out;
+ /*
+ * If the re-encoding failed, out might be NULL here; in that
+ * case we just return the commit message verbatim.
+ */
+ return out ? out : msg;
+}
+
+void logmsg_free(char *msg, const struct commit *commit)
+{
+ if (msg != commit->buffer)
+ free(msg);
}
static int mailmap_name(const char **email, size_t *email_len,
@@ -1278,14 +1290,11 @@ void format_commit_message(const struct commit *commit,
context.pretty_ctx = pretty_ctx;
context.wrap_start = sb->len;
context.message = logmsg_reencode(commit, output_enc);
- if (!context.message)
- context.message = commit->buffer;
strbuf_expand(sb, format, format_commit_item, &context);
rewrap_message_tail(sb, &context, 0, 0, 0);
- if (context.message != commit->buffer)
- free(context.message);
+ logmsg_free(context.message, commit);
free(context.signature.gpg_output);
free(context.signature.signer);
}
@@ -1432,7 +1441,7 @@ void pretty_print_commit(const struct pretty_print_context *pp,
{
unsigned long beginning_of_body;
int indent = 4;
- const char *msg = commit->buffer;
+ const char *msg;
char *reencoded;
const char *encoding;
int need_8bit_cte = pp->need_8bit_cte;
@@ -1443,10 +1452,7 @@ void pretty_print_commit(const struct pretty_print_context *pp,
}
encoding = get_log_output_encoding();
- reencoded = logmsg_reencode(commit, encoding);
- if (reencoded) {
- msg = reencoded;
- }
+ msg = reencoded = logmsg_reencode(commit, encoding);
if (pp->fmt == CMIT_FMT_ONELINE || pp->fmt == CMIT_FMT_EMAIL)
indent = 0;
@@ -1503,7 +1509,7 @@ void pretty_print_commit(const struct pretty_print_context *pp,
if (pp->fmt == CMIT_FMT_EMAIL && sb->len <= beginning_of_body)
strbuf_addch(sb, '\n');
- free(reencoded);
+ logmsg_free(reencoded, commit);
}
void pp_commit_easy(enum cmit_fmt fmt, const struct commit *commit,
--
1.8.0.2.16.g72e2fc9
^ permalink raw reply related
* [PATCH 3/3] logmsg_reencode: lazily load missing commit buffers
From: Jeff King @ 2013-01-26 9:44 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: <20130126094026.GA9646@sigill.intra.peff.net>
Usually a commit that makes it to logmsg_reencode will have
been parsed, and the commit->buffer struct member will be
valid. However, some code paths will free commit buffers
after having used them (for example, the log traversal
machinery will do so to keep memory usage down).
Most of the time this is fine; log should only show a commit
once, and then exits. However, there are some code paths
where this does not work. At least two are known:
1. A commit may be shown as part of a regular ref, and
then it may be shown again as part of a submodule diff
(e.g., if a repo contains refs to both the superproject
and subproject).
2. A notes-cache commit may be shown during "log --all",
and then later used to access a textconv cache during a
diff.
Lazily loading in logmsg_reencode does not necessarily catch
all such cases, but it should catch most of them. Users of
the commit buffer tend to be either parsing for structure
(in which they will call parse_commit, and either we will
already have parsed, or we will load commit->buffer lazily
there), or outputting (either to the user, or fetching a
part of the commit message via format_commit_message). In
the latter case, we should always be using logmsg_reencode
anyway (and typically we do so via the pretty-print
machinery).
If there are any cases that this misses, we can fix them up
to use logmsg_reencode (or handle them on a case-by-case
basis if that is inappropriate).
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/blame.c | 13 ---------
pretty.c | 57 ++++++++++++++++++++++++++++++++++------
t/t4042-diff-textconv-caching.sh | 8 ++++++
3 files changed, 57 insertions(+), 21 deletions(-)
diff --git a/builtin/blame.c b/builtin/blame.c
index 962e4e3..86100e9 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1424,19 +1424,6 @@ static void get_commit_info(struct commit *commit,
commit_info_init(ret);
- /*
- * We've operated without save_commit_buffer, so
- * we now need to populate them for output.
- */
- if (!commit->buffer) {
- enum object_type type;
- unsigned long size;
- commit->buffer =
- read_sha1_file(commit->object.sha1, &type, &size);
- if (!commit->buffer)
- die("Cannot read commit %s",
- sha1_to_hex(commit->object.sha1));
- }
encoding = get_log_output_encoding();
message = logmsg_reencode(commit, encoding);
get_ac_line(message, "\nauthor ",
diff --git a/pretty.c b/pretty.c
index c675349..eae57ad 100644
--- a/pretty.c
+++ b/pretty.c
@@ -592,18 +592,59 @@ char *logmsg_reencode(const struct commit *commit,
char *msg = commit->buffer;
char *out;
+ if (!msg) {
+ enum object_type type;
+ unsigned long size;
+
+ msg = read_sha1_file(commit->object.sha1, &type, &size);
+ if (!msg)
+ die("Cannot read commit object %s",
+ sha1_to_hex(commit->object.sha1));
+ if (type != OBJ_COMMIT)
+ die("Expected commit for '%s', got %s",
+ sha1_to_hex(commit->object.sha1), typename(type));
+ }
+
if (!output_encoding || !*output_encoding)
return msg;
encoding = get_header(commit, msg, "encoding");
use_encoding = encoding ? encoding : utf8;
- if (same_encoding(use_encoding, output_encoding))
- if (encoding) /* we'll strip encoding header later */
- out = xstrdup(commit->buffer);
- else
- return msg; /* nothing to do */
- else
- out = reencode_string(commit->buffer,
- output_encoding, use_encoding);
+ if (same_encoding(use_encoding, output_encoding)) {
+ /*
+ * No encoding work to be done. If we have no encoding header
+ * at all, then there's nothing to do, and we can return the
+ * message verbatim (whether newly allocated or not).
+ */
+ if (!encoding)
+ return msg;
+
+ /*
+ * Otherwise, we still want to munge the encoding header in the
+ * result, which will be done by modifying the buffer. If we
+ * are using a fresh copy, we can reuse it. But if we are using
+ * the cached copy from commit->buffer, we need to duplicate it
+ * to avoid munging commit->buffer.
+ */
+ out = msg;
+ if (out == commit->buffer)
+ out = xstrdup(out);
+ }
+ else {
+ /*
+ * There's actual encoding work to do. Do the reencoding, which
+ * still leaves the header to be replaced in the next step. At
+ * this point, we are done with msg. If we allocated a fresh
+ * copy, we can free it.
+ */
+ out = reencode_string(msg, output_encoding, use_encoding);
+ if (out && msg != commit->buffer)
+ free(msg);
+ }
+
+ /*
+ * This replacement actually consumes the buffer we hand it, so we do
+ * not have to worry about freeing the old "out" here.
+ */
if (out)
out = replace_encoding_header(out, output_encoding);
diff --git a/t/t4042-diff-textconv-caching.sh b/t/t4042-diff-textconv-caching.sh
index 91f8198..04a44d5 100755
--- a/t/t4042-diff-textconv-caching.sh
+++ b/t/t4042-diff-textconv-caching.sh
@@ -106,4 +106,12 @@ test_expect_success 'switching diff driver produces correct results' '
test_cmp expect actual
'
+# The point here is to test that we can log the notes cache and still use it to
+# produce a diff later (older versions of git would segfault on this). It's
+# much more likely to come up in the real world with "log --all -p", but using
+# --no-walk lets us reliably reproduce the order of traversal.
+test_expect_success 'log notes cache and still use cache for -p' '
+ git log --no-walk -p refs/notes/textconv/magic HEAD
+'
+
test_done
--
1.8.0.2.16.g72e2fc9
^ permalink raw reply related
* Re: git merge error question: The following untracked working tree files would be overwritten by merge
From: Carsten Fuchs @ 2013-01-26 10:46 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vehh95e7u.fsf@alter.siamese.dyndns.org>
Am 2013-01-25 19:07, schrieb Junio C Hamano:
> Carsten Fuchs <carsten.fuchs@cafu.de> writes:
>>> [...]
>>> $ git merge origin/master --ff-only
>>> Updating f419d57..2da6052
>>> error: The following untracked working tree files would be overwritten by merge:
>>> obsolete/e107/Readme.txt
>>> obsolete/e107/article.php
>>> obsolete/e107/backend.php
>>> [...]
>> ...
>> Compare with what Subversion did in an analogous case: When I ran "svn
>> update" and the update brought new files for which there already was
>> an untracked copy in the working directory, Subversion:
>> - started to consider the file as tracked,
>> - but left the file in the working-copy alone.
>>
>> As a result, a subsequent "svn status" might
>> a) no longer show the file at all, if the foreign copy in the
>> working directory happened to be the same as the one brought by the
>> "svn update", or
>> b) flag the file as modified, if different from the one that "svn
>> update" would have created in its place.
>
> Interesting. So before running "status", the merge is recorded (in this
> particular case you are doing ff-only so there is nothing new to
> record, but if the rest of the tree merges cleanly, the new tree
> that contains "obsolete" from the other branch you just merged will
> be the contents you record in the merge commit), and working tree is
> immediately dirty?
Yes. But I don't think it's the (svn) "status" command that does anything special.
In Git, if I understand it correctly, the final step of a "merge", "checkout", "reset",
etc. is a move of the HEAD to the resulting or specified commit. I imagine that it is
here where the diff of the dirty working tree is re-applied to the newly checked out
commit (and if this is not possible cleanly, probably [a] the whole operation must
abort, or [b] leave files in the working tree with conflict markers), and where a
decision must be made about "obstructing paths" (svn lingo): [c] abort the whole
operation, or [d] "version" them (but don't modify them in any way).
I'm not sure if Subversion does [a] and [c] ("abort") without the --force option, and
[b] and [d] with --force, or any other combination, but at least TortoiseSVN seems to
use [d] by default (which seems safe enough).
Despite a thorough search, I've not been able to find much reference about this behaviour:
http://svnbook.red-bean.com/en/1.6/svn.ref.svn.c.switch.html
http://markphip.blogspot.de/2007/01/subversion-15-tolerate-obstructions.html
However, as the blog article mentions, I too have found this treatment of obstructing
paths very natural and helpful in several occasions.
(Because without it, we must manually rename the obstructing paths, re-start the
previously aborted operation, and then take diffs or somehow else compare the renamed
obstructing and newly added paths manually, and possible merge them manually; or at
least copy the renamed edition over the newly added edition to get back into Git for the
job.)
>> So my real question is, why does Git not do something analogous?
>
> The answer to that question is "because nobody thought that doing so
> would help users very much and bothered to implement it"; it is not
> "people thought about doing so and found reasons why that would not
> help users".
Thanks, it's very good to hear this!
:-)
Best regards,
Carsten
^ permalink raw reply
* [PATCH 1/2 v2] mergetool--lib: don't call "exit" in setup_tool
From: John Keeping @ 2013-01-26 12:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: David Aguilar, git
In-Reply-To: <7vip6k23mk.fsf@alter.siamese.dyndns.org>
This will make it easier to use setup_tool in places where we expect
that the selected tool will not support the current mode.
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".
git-mergetool--lib.sh | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 4c1e129..8a5eaff 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -67,11 +67,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
}
@@ -100,7 +100,10 @@ run_merge_tool () {
status=0
# Bring tool-specific functions into scope
- setup_tool "$1"
+ if test -z "$merge_tool_path"
+ then
+ setup_tool "$1" || return
+ fi
if merge_mode
then
--
1.8.1.1.367.ga9c3dd4.dirty
^ permalink raw reply related
* Re: [PATCH] mergetools: Simplify how we handle "vim" and "defaults"
From: John Keeping @ 2013-01-26 12:12 UTC (permalink / raw)
To: David Aguilar; +Cc: Junio C Hamano, git
In-Reply-To: <1359183058-51835-1-git-send-email-davvid@gmail.com>
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.
> + 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"
^ permalink raw reply
* Re: [PATCH 1/2] git-p4.py: support Python 2.5
From: Pete Wyckoff @ 2013-01-26 12:45 UTC (permalink / raw)
To: Brandon Casey; +Cc: git, esr, john, Brandon Casey
In-Reply-To: <1359146641-27810-2-git-send-email-drafnel@gmail.com>
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.
-- Pete
^ permalink raw reply
* 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
* [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/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
* 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
* [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 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
* 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
* 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 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
* [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
* [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
* 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
* 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
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