* Re: [PATCHv2] parse-options: report uncorrupted multi-byte options
From: Junio C Hamano @ 2013-02-12 2:10 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Erik Faye-Lund, git, peff, matthieu.moy, tboegi
In-Reply-To: <CACsJy8BByNnEhhE3TieM_kOy65t75rmB45ZzjJJ8AtL2N4-UFA@mail.gmail.com>
Duy Nguyen <pclouds@gmail.com> writes:
> On Tue, Feb 12, 2013 at 6:13 AM, Erik Faye-Lund <kusmabite@gmail.com> wrote:
>> Because our command-line parser considers only one byte at the time
>> for short-options, we incorrectly report only the first byte when
>> multi-byte input was provided. This makes user-erros slightly
>> awkward to diagnose for instance under UTF-8 locale and non-English
>> keyboard layouts.
>>
>> Make the reporting code report the whole argument-string when a
>> non-ASCII short-option is detected.
>
> Similar cases:
>
> config.c:git_default_core_config() assumes core.commentchar is ascii.
> We should catch and report non-ascii chars, or simply accept it as a
> string.
That one is just an uninterpreted byte. core.commentString might be
a nice extension to the concept, but it is an entirely different
category.
> builtin/update-index.c:cmd_update_index(): error("unknown switch
> '%c'", *ctx.opt);
This one is in the same category as this topic.
> builtin/apply.c:apply_one_fragment(): error(_("invalid start of line:
> '%c'"), first); where 'first' may be a part of utf-8 from a broken
> patch.
This is where the patch is expected to have either " ", "-" or "+",
again, anything else is an uninterpreted byte. It is more like
reporting the file we found an error in, whose filename is not
encoded in UTF-8 to the user's terminal.
^ permalink raw reply
* Re: [PATCHv2] parse-options: report uncorrupted multi-byte options
From: Duy Nguyen @ 2013-02-12 1:21 UTC (permalink / raw)
To: Erik Faye-Lund; +Cc: git, gitster, peff, matthieu.moy, tboegi
In-Reply-To: <1360624428-4728-1-git-send-email-kusmabite@gmail.com>
On Tue, Feb 12, 2013 at 6:13 AM, Erik Faye-Lund <kusmabite@gmail.com> wrote:
> Because our command-line parser considers only one byte at the time
> for short-options, we incorrectly report only the first byte when
> multi-byte input was provided. This makes user-erros slightly
> awkward to diagnose for instance under UTF-8 locale and non-English
> keyboard layouts.
>
> Make the reporting code report the whole argument-string when a
> non-ASCII short-option is detected.
Similar cases:
config.c:git_default_core_config() assumes core.commentchar is ascii.
We should catch and report non-ascii chars, or simply accept it as a
string.
builtin/update-index.c:cmd_update_index(): error("unknown switch
'%c'", *ctx.opt);
builtin/apply.c:apply_one_fragment(): error(_("invalid start of line:
'%c'"), first); where 'first' may be a part of utf-8 from a broken
patch.
--
Duy
^ permalink raw reply
* Re: [PATCHv2] parse-options: report uncorrupted multi-byte options
From: Jeff King @ 2013-02-12 1:00 UTC (permalink / raw)
To: Erik Faye-Lund; +Cc: git, gitster, matthieu.moy, tboegi
In-Reply-To: <1360624428-4728-1-git-send-email-kusmabite@gmail.com>
On Tue, Feb 12, 2013 at 12:13:48AM +0100, Erik Faye-Lund wrote:
> I decided to change the text from what Jeff suggested; all we know is
> that it's non-ASCII. It might be Latin-1 or some other non-ASCII,
> single byte encoding. And since we're trying not to care, let's also
> try to not be overly specific :)
Yeah, that makes more sense (I did not put too much thought into the
original wording). Thanks.
-Peff
^ permalink raw reply
* Fetch and -t
From: Olsen, Alan R @ 2013-02-12 0:41 UTC (permalink / raw)
To: git@vger.kernel.org
I have found that if I add a remote and do a "git fetch -t -f remote_name" that it *only* pulls tags.
Reading the man page it seems like it should pull all the remotes and all the tags and the commits only reachable by tags.
Am I misreading this or it supposed to work this way. I don't mind doing two fetches to get everything, but the documentation needs to be a little clearer.
^ permalink raw reply
* Re: [PATCH v2] rebase -i: respect core.commentchar
From: Junio C Hamano @ 2013-02-12 0:13 UTC (permalink / raw)
To: John Keeping; +Cc: git, Ralf Thielow
In-Reply-To: <20130211230804.GF2270@serenity.lan>
John Keeping <john@keeping.me.uk> writes:
> @@ -179,7 +182,9 @@ die_abort () {
> }
>
> has_action () {
> - sane_grep '^[^#]' "$1" >/dev/null
> + echo "space stripped actions:" >&2
> + git stripspace --strip-comments <"$1" >&2
> + test -n "$(git stripspace --strip-comments <"$1")"
> }
I'll remove the debugging remnants while queuing.
> fixup)
> echo
> - echo "# The $(nth_string $count) commit message will be skipped:"
> + printf '%s\n' "$comment_char The $(nth_string $count) commit message will be skipped:"
> echo
> - commit_message $2 | sed -e 's/^/# /'
> + # Change the space after the comment character to TAB:
> + commit_message $2 | git stripspace --comment-lines | sed -e 's/ / /'
I think this changes the behaviour but in a good way. It used to
show an empty line in the incoming commit message to a hash followed
by a trailing HT before the end of line, but now it only emits the
comment char immediately followed by the end of line.
> @@ -942,20 +948,18 @@ test -s "$todo" || echo noop >> "$todo"
> test -n "$autosquash" && rearrange_squash "$todo"
> test -n "$cmd" && add_exec_commands "$todo"
>
> -cat >> "$todo" << EOF
> -
> -# Rebase $shortrevisions onto $shortonto
> -EOF
> +echo >>"$todo"
> +printf '%s\n' "$comment_char Rebase $shortrevisions onto $shortonto" >>"$todo"
I think you can still do
cat >>"$todo" <<EOF
$comment_char Rebase $shortrevisions onto...
EOF
here with any funny comment character. Doing this with two separate
I/O does not hurt very much, but the resulting code may be easier to
scan if left as here-text with a single cat.
Please eyeball what is in 'pu' (I have a separate squashable fixup
on top of your patch) and let me know if I made mistakes.
Thanks for working on this.
^ permalink raw reply
* Re: [PATCHv2] parse-options: report uncorrupted multi-byte options
From: Junio C Hamano @ 2013-02-11 23:51 UTC (permalink / raw)
To: Erik Faye-Lund; +Cc: git, peff, matthieu.moy, tboegi
In-Reply-To: <1360624428-4728-1-git-send-email-kusmabite@gmail.com>
Thanks.
^ permalink raw reply
* [PATCHv2] parse-options: report uncorrupted multi-byte options
From: Erik Faye-Lund @ 2013-02-11 23:13 UTC (permalink / raw)
To: git; +Cc: gitster, peff, matthieu.moy, tboegi
Because our command-line parser considers only one byte at the time
for short-options, we incorrectly report only the first byte when
multi-byte input was provided. This makes user-erros slightly
awkward to diagnose for instance under UTF-8 locale and non-English
keyboard layouts.
Make the reporting code report the whole argument-string when a
non-ASCII short-option is detected.
Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
Improved-by: Jeff King <peff@peff.net>
---
Here's a second attempt at fixing error-reporting with UTF-8 encoded
input, this time without corrupting other non-ascii multi-byte
encodings.
I decided to change the text from what Jeff suggested; all we know is
that it's non-ASCII. It might be Latin-1 or some other non-ASCII,
single byte encoding. And since we're trying not to care, let's also
try to not be overly specific :)
I wasn't entirely sure who to attribute for the improvement, so I just
picked Jeff; he provided some code. That decision might not be correct,
feel free to change it.
parse-options.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/parse-options.c b/parse-options.c
index 67e98a6..6a39446 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -461,8 +461,11 @@ int parse_options(int argc, const char **argv, const char *prefix,
default: /* PARSE_OPT_UNKNOWN */
if (ctx.argv[0][1] == '-') {
error("unknown option `%s'", ctx.argv[0] + 2);
- } else {
+ } else if (isascii(*ctx.opt)) {
error("unknown switch `%c'", *ctx.opt);
+ } else {
+ error("unknown non-ascii option in string: `%s'",
+ ctx.argv[0]);
}
usage_with_options(usagestr, options);
}
--
1.8.1.1
^ permalink raw reply related
* [PATCH v2] rebase -i: respect core.commentchar
From: John Keeping @ 2013-02-11 23:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Ralf Thielow
In-Reply-To: <7va9raldw8.fsf@alter.siamese.dyndns.org>
Commit eff80a9 (Allow custom "comment char") introduced a custom comment
character for commit messages but did not teach git-rebase--interactive
to use it.
Change git-rebase--interactive to read core.commentchar and use its
value when generating commit messages and for the command list.
Signed-off-by: John Keeping <john@keeping.me.uk>
---
Changes since v1:
- use '\' as the comment character in tests
- use "git stripspace --strip-comments" where appropriate
- use "git stripspace --comment-lines" where appropriate
- use printf instead of echo when the string contains $comment_char
- quote $comment_char in case label
On Mon, Feb 11, 2013 at 01:49:27PM -0800, Junio C Hamano wrote:
> ...
> Perhaps
>
> git stripspace --comment-lines <<EOF
> ...
> EOF
>
> is a better option that that loop.
Yes. I was confusing myself with out-of-date documentation.
---
git-rebase--interactive.sh | 88 ++++++++++++++++++++++---------------------
t/t3404-rebase-interactive.sh | 16 ++++++++
2 files changed, 62 insertions(+), 42 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 8ed7fcc..b5ce3f2 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -80,6 +80,9 @@ rewritten_pending="$state_dir"/rewritten-pending
GIT_CHERRY_PICK_HELP="$resolvemsg"
export GIT_CHERRY_PICK_HELP
+comment_char=$(git config --get core.commentchar 2>/dev/null | cut -c1)
+: ${comment_char:=#}
+
warn () {
printf '%s\n' "$*" >&2
}
@@ -105,8 +108,8 @@ mark_action_done () {
sed -e 1q < "$todo" >> "$done"
sed -e 1d < "$todo" >> "$todo".new
mv -f "$todo".new "$todo"
- new_count=$(sane_grep -c '^[^#]' < "$done")
- total=$(($new_count+$(sane_grep -c '^[^#]' < "$todo")))
+ new_count=$(git stripspace --strip-comments < "$done" | wc -l)
+ total=$(($new_count+$(git stripspace --strip-comments < "$todo" | wc -l)))
if test "$last_count" != "$new_count"
then
last_count=$new_count
@@ -116,19 +119,19 @@ mark_action_done () {
}
append_todo_help () {
- cat >> "$todo" << EOF
-#
-# Commands:
-# p, pick = use commit
-# r, reword = use commit, but edit the commit message
-# e, edit = use commit, but stop for amending
-# s, squash = use commit, but meld into previous commit
-# f, fixup = like "squash", but discard this commit's log message
-# x, exec = run command (the rest of the line) using shell
-#
-# These lines can be re-ordered; they are executed from top to bottom.
-#
-# If you remove a line here THAT COMMIT WILL BE LOST.
+ git stripspace --comment-lines >>"$todo" <<EOF
+
+Commands:
+ p, pick = use commit
+ r, reword = use commit, but edit the commit message
+ e, edit = use commit, but stop for amending
+ s, squash = use commit, but meld into previous commit
+ f, fixup = like "squash", but discard this commit's log message
+ x, exec = run command (the rest of the line) using shell
+
+These lines can be re-ordered; they are executed from top to bottom.
+
+If you remove a line here THAT COMMIT WILL BE LOST.
EOF
}
@@ -179,7 +182,9 @@ die_abort () {
}
has_action () {
- sane_grep '^[^#]' "$1" >/dev/null
+ echo "space stripped actions:" >&2
+ git stripspace --strip-comments <"$1" >&2
+ test -n "$(git stripspace --strip-comments <"$1")"
}
is_empty_commit() {
@@ -363,10 +368,10 @@ update_squash_messages () {
if test -f "$squash_msg"; then
mv "$squash_msg" "$squash_msg".bak || exit
count=$(($(sed -n \
- -e "1s/^# This is a combination of \(.*\) commits\./\1/p" \
+ -e "1s/^. This is a combination of \(.*\) commits\./\1/p" \
-e "q" < "$squash_msg".bak)+1))
{
- echo "# This is a combination of $count commits."
+ printf '%s\n' "$comment_char This is a combination of $count commits."
sed -e 1d -e '2,/^./{
/^$/d
}' <"$squash_msg".bak
@@ -375,8 +380,8 @@ update_squash_messages () {
commit_message HEAD > "$fixup_msg" || die "Cannot write $fixup_msg"
count=2
{
- echo "# This is a combination of 2 commits."
- echo "# The first commit's message is:"
+ printf '%s\n' "$comment_char This is a combination of 2 commits."
+ printf '%s\n' "$comment_char The first commit's message is:"
echo
cat "$fixup_msg"
} >"$squash_msg"
@@ -385,21 +390,22 @@ update_squash_messages () {
squash)
rm -f "$fixup_msg"
echo
- echo "# This is the $(nth_string $count) commit message:"
+ printf '%s\n' "$comment_char This is the $(nth_string $count) commit message:"
echo
commit_message $2
;;
fixup)
echo
- echo "# The $(nth_string $count) commit message will be skipped:"
+ printf '%s\n' "$comment_char The $(nth_string $count) commit message will be skipped:"
echo
- commit_message $2 | sed -e 's/^/# /'
+ # Change the space after the comment character to TAB:
+ commit_message $2 | git stripspace --comment-lines | sed -e 's/ / /'
;;
esac >>"$squash_msg"
}
peek_next_command () {
- sed -n -e "/^#/d" -e '/^$/d' -e "s/ .*//p" -e "q" < "$todo"
+ git stripspace --strip-comments <"$todo" | sed -n -e 's/ .*//p' -e q
}
# A squash/fixup has failed. Prepare the long version of the squash
@@ -464,7 +470,7 @@ do_next () {
rm -f "$msg" "$author_script" "$amend" || exit
read -r command sha1 rest < "$todo"
case "$command" in
- '#'*|''|noop)
+ "$comment_char"*|''|noop)
mark_action_done
;;
pick|p)
@@ -803,15 +809,15 @@ skip)
do_rest
;;
edit-todo)
- sed -e '/^#/d' < "$todo" > "$todo".new
+ git stripspace --strip-comments <"$todo" >"$todo".new
mv -f "$todo".new "$todo"
append_todo_help
- cat >> "$todo" << EOF
-#
-# You are editing the todo file of an ongoing interactive rebase.
-# To continue rebase after editing, run:
-# git rebase --continue
-#
+ git stripspace --comment-lines >>"$todo" <<EOF
+
+You are editing the todo file of an ongoing interactive rebase.
+To continue rebase after editing, run:
+ git rebase --continue
+
EOF
git_sequence_editor "$todo" ||
@@ -881,7 +887,7 @@ do
if test -z "$keep_empty" && is_empty_commit $shortsha1 && ! is_merge_commit $shortsha1
then
- comment_out="# "
+ comment_out="$comment_char "
else
comment_out=
fi
@@ -942,20 +948,18 @@ test -s "$todo" || echo noop >> "$todo"
test -n "$autosquash" && rearrange_squash "$todo"
test -n "$cmd" && add_exec_commands "$todo"
-cat >> "$todo" << EOF
-
-# Rebase $shortrevisions onto $shortonto
-EOF
+echo >>"$todo"
+printf '%s\n' "$comment_char Rebase $shortrevisions onto $shortonto" >>"$todo"
append_todo_help
-cat >> "$todo" << EOF
-#
-# However, if you remove everything, the rebase will be aborted.
-#
+git stripspace --comment-lines >>"$todo" <<EOF
+
+However, if you remove everything, the rebase will be aborted.
+
EOF
if test -z "$keep_empty"
then
- echo "# Note that empty commits are commented out" >>"$todo"
+ printf '%s\n' "$comment_char Note that empty commits are commented out" >>"$todo"
fi
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 8462be1..16d8160 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -934,4 +934,20 @@ test_expect_success 'rebase --edit-todo can be used to modify todo' '
test L = $(git cat-file commit HEAD | sed -ne \$p)
'
+test_expect_success 'rebase -i respects core.commentchar' '
+ git reset --hard &&
+ git checkout E^0 &&
+ git config core.commentchar "\\" &&
+ test_when_finished "git config --unset core.commentchar" &&
+ cat >comment-lines.sh <<EOF &&
+#!$SHELL_PATH
+sed -e "2,\$ s/^/\\\\\\/" "\$1" >"\$1".tmp
+mv "\$1".tmp "\$1"
+EOF
+ chmod a+x comment-lines.sh &&
+ test_set_editor "$(pwd)/comment-lines.sh" &&
+ git rebase -i B &&
+ test B = $(git cat-file commit HEAD^ | sed -ne \$p)
+'
+
test_done
--
1.8.1.3.556.gb3310b5.dirty
^ permalink raw reply related
* Re: [PATCH] Allow building with xmlparse.h
From: Junio C Hamano @ 2013-02-11 22:35 UTC (permalink / raw)
To: Matt Kraai; +Cc: git, Jeff King, Matt Kraai
In-Reply-To: <1360621855-19863-1-git-send-email-kraai@ftbfs.org>
Matt Kraai <kraai@ftbfs.org> writes:
> From: Matt Kraai <matt.kraai@amo.abbott.com>
>
> expat 1.1 and 1.2 provide xmlparse.h instead of expat.h. Include the
> former on systems that define the EXPAT_NEEDS_XMLPARSE_H variable and
> define that variable on QNX systems, which ship with expat 1.1.
>
> Signed-off-by: Matt Kraai <matt.kraai@amo.abbott.com>
> ---
> Makefile | 6 ++++++
> config.mak.uname | 1 +
> http-push.c | 4 ++++
> 3 files changed, 11 insertions(+)
>
> I've changed #ifndef to #ifdef and changed the order of the branches
> in http-push.c. If you'd also like me to rename the variable (e.g.,
> to NEEDS_XMLPARSE_H), please let me know.
I do not think renaming is necessary (the name you used in the
original and this patch is better than NEEDS_XMLPARSE_H).
I take that you also think the updated order is easier to read;
thanks for sanity-checking ;-).
^ permalink raw reply
* [PATCH] Allow building with xmlparse.h
From: Matt Kraai @ 2013-02-11 22:30 UTC (permalink / raw)
To: git, Junio C Hamano, Jeff King; +Cc: Matt Kraai
In-Reply-To: <7vy5eujybf.fsf@alter.siamese.dyndns.org>
From: Matt Kraai <matt.kraai@amo.abbott.com>
expat 1.1 and 1.2 provide xmlparse.h instead of expat.h. Include the
former on systems that define the EXPAT_NEEDS_XMLPARSE_H variable and
define that variable on QNX systems, which ship with expat 1.1.
Signed-off-by: Matt Kraai <matt.kraai@amo.abbott.com>
---
Makefile | 6 ++++++
config.mak.uname | 1 +
http-push.c | 4 ++++
3 files changed, 11 insertions(+)
I've changed #ifndef to #ifdef and changed the order of the branches
in http-push.c. If you'd also like me to rename the variable (e.g.,
to NEEDS_XMLPARSE_H), please let me know.
diff --git a/Makefile b/Makefile
index 5a2e02d..720fc18 100644
--- a/Makefile
+++ b/Makefile
@@ -43,6 +43,9 @@ all::
# Define EXPATDIR=/foo/bar if your expat header and library files are in
# /foo/bar/include and /foo/bar/lib directories.
#
+# Define EXPAT_NEEDS_XMLPARSE_H if you have an old version of expat (e.g.,
+# 1.1 or 1.2) that provides xmlparse.h instead of expat.h.
+#
# Define NO_GETTEXT if you don't want Git output to be translated.
# A translated Git requires GNU libintl or another gettext implementation,
# plus libintl-perl at runtime.
@@ -1089,6 +1092,9 @@ else
else
EXPAT_LIBEXPAT = -lexpat
endif
+ ifdef EXPAT_NEEDS_XMLPARSE_H
+ BASIC_CFLAGS += -DEXPAT_NEEDS_XMLPARSE_H
+ endif
endif
endif
diff --git a/config.mak.uname b/config.mak.uname
index bea34f0..8743a6d 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -523,6 +523,7 @@ endif
endif
ifeq ($(uname_S),QNX)
COMPAT_CFLAGS += -DSA_RESTART=0
+ EXPAT_NEEDS_XMLPARSE_H = YesPlease
HAVE_STRINGS_H = YesPlease
NEEDS_SOCKET = YesPlease
NO_FNMATCH_CASEFOLD = YesPlease
diff --git a/http-push.c b/http-push.c
index 9923441..9fa47a7 100644
--- a/http-push.c
+++ b/http-push.c
@@ -11,7 +11,11 @@
#include "list-objects.h"
#include "sigchain.h"
+#ifdef EXPAT_NEEDS_XMLPARSE_H
+#include <xmlparse.h>
+#else
#include <expat.h>
+#endif
static const char http_push_usage[] =
"git http-push [--all] [--dry-run] [--force] [--verbose] <remote> [<head>...]\n";
--
1.8.1.2.547.g7ce9def
^ permalink raw reply related
* Re: [PATCH] Allow building with xmlparse.h
From: Junio C Hamano @ 2013-02-11 22:11 UTC (permalink / raw)
To: Matt Kraai; +Cc: git, Jeff King, Matt Kraai
In-Reply-To: <1360620225-19587-1-git-send-email-kraai@ftbfs.org>
Matt Kraai <kraai@ftbfs.org> writes:
> From: Matt Kraai <matt.kraai@amo.abbott.com>
>
> expat 1.1 and 1.2 provide xmlparse.h instead of expat.h. Include the
> former on systems that define the EXPAT_NEEDS_XMLPARSE_H variable and
> define that variable on QNX systems, which ship with expat 1.1.
>
> Signed-off-by: Matt Kraai <matt.kraai@amo.abbott.com>
> ---
> ...
> diff --git a/http-push.c b/http-push.c
> index 9923441..7202e2d 100644
> --- a/http-push.c
> +++ b/http-push.c
> @@ -11,7 +11,11 @@
> #include "list-objects.h"
> #include "sigchain.h"
>
> +#ifndef EXPAT_NEEDS_XMLPARSE_H
> #include <expat.h>
> +#else
> +#include <xmlparse.h>
> +#endif
Thanks for a quick re-roll.
Is it just me who finds the above hard to read and find the below
much more natural?
#ifdef NEEDS_FOO_H
#include <foo.h>
#else
#include <bar.h>
#endif
^ permalink raw reply
* [PATCH] Allow building with xmlparse.h
From: Matt Kraai @ 2013-02-11 22:03 UTC (permalink / raw)
To: git, Junio C Hamano, Jeff King; +Cc: Matt Kraai
In-Reply-To: <20130211212411.GA19113@ftbfs.org>
From: Matt Kraai <matt.kraai@amo.abbott.com>
expat 1.1 and 1.2 provide xmlparse.h instead of expat.h. Include the
former on systems that define the EXPAT_NEEDS_XMLPARSE_H variable and
define that variable on QNX systems, which ship with expat 1.1.
Signed-off-by: Matt Kraai <matt.kraai@amo.abbott.com>
---
Makefile | 6 ++++++
config.mak.uname | 1 +
http-push.c | 4 ++++
3 files changed, 11 insertions(+)
diff --git a/Makefile b/Makefile
index 5a2e02d..720fc18 100644
--- a/Makefile
+++ b/Makefile
@@ -43,6 +43,9 @@ all::
# Define EXPATDIR=/foo/bar if your expat header and library files are in
# /foo/bar/include and /foo/bar/lib directories.
#
+# Define EXPAT_NEEDS_XMLPARSE_H if you have an old version of expat (e.g.,
+# 1.1 or 1.2) that provides xmlparse.h instead of expat.h.
+#
# Define NO_GETTEXT if you don't want Git output to be translated.
# A translated Git requires GNU libintl or another gettext implementation,
# plus libintl-perl at runtime.
@@ -1089,6 +1092,9 @@ else
else
EXPAT_LIBEXPAT = -lexpat
endif
+ ifdef EXPAT_NEEDS_XMLPARSE_H
+ BASIC_CFLAGS += -DEXPAT_NEEDS_XMLPARSE_H
+ endif
endif
endif
diff --git a/config.mak.uname b/config.mak.uname
index bea34f0..8743a6d 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -523,6 +523,7 @@ endif
endif
ifeq ($(uname_S),QNX)
COMPAT_CFLAGS += -DSA_RESTART=0
+ EXPAT_NEEDS_XMLPARSE_H = YesPlease
HAVE_STRINGS_H = YesPlease
NEEDS_SOCKET = YesPlease
NO_FNMATCH_CASEFOLD = YesPlease
diff --git a/http-push.c b/http-push.c
index 9923441..7202e2d 100644
--- a/http-push.c
+++ b/http-push.c
@@ -11,7 +11,11 @@
#include "list-objects.h"
#include "sigchain.h"
+#ifndef EXPAT_NEEDS_XMLPARSE_H
#include <expat.h>
+#else
+#include <xmlparse.h>
+#endif
static const char http_push_usage[] =
"git http-push [--all] [--dry-run] [--force] [--verbose] <remote> [<head>...]\n";
--
1.8.1.2.547.g7ce9def
^ permalink raw reply related
* Re: [PATCH] Include xmlparse.h instead of expat.h on QNX
From: Junio C Hamano @ 2013-02-11 21:53 UTC (permalink / raw)
To: Matt Kraai; +Cc: git, Jeff King
In-Reply-To: <20130211214948.GB19113@ftbfs.org>
Matt Kraai <kraai@ftbfs.org> writes:
>> Assuming that this change is about building with expat1, it would
>> probably be better to do something like this instead, I would think.
>
> expat 1.95.0 through 1.95.8 used expat.h; should I still use
> EXPAT_VERSION = 1 to signify that it should use xmlparse.h, use
> EXPAT_NEEDS_XMLPARSE_H as Jeff suggested, or something else entirely?
Oh, please do not take it as a request to use that exact name (in
case you didn't know, I am bad at naming things). It was merely an
illustration to show the direction, written without knowing that
Peff was essentially giving the same suggestion.
Thanks.
Oh, by the way, please do not deflect an attempt to directly send a
response to you with a Mail-Followup-To header.
^ permalink raw reply
* Re: [PATCH] Include xmlparse.h instead of expat.h on QNX
From: Matt Kraai @ 2013-02-11 21:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King
In-Reply-To: <7vip5ylekj.fsf@alter.siamese.dyndns.org>
On Mon, Feb 11, 2013 at 01:34:52PM -0800, Junio C Hamano wrote:
> Two points and a possibly irrelevant half:
>
> - If a fix is platform specific (i.e. tempts to use #ifdef
> PLATFORM_NAME), we would prefer to see a patch that that is
> isolated to platform-specific compatibility layer, which would
> involve:
>
> . add compat/qnx/expat.h file that #include <xmlparse.h>
> . to Makefile, add -Icompat/qnx/ to CFLAGS
>
> - Is this really a fix for a problem specific to QNX? It looks
> like this is for any platform with expat 1, no?
It should apply to anyone trying to build with expat 1.1 or 1.2, but
not with 1.95.0 or later.
> - What happens to people with QNX older than 6.3.2 or newer than
> 6.5.0 (assuming they will eventually start shipping expat 2) with
> your patch?
Git will fail to build http-push.c. I don't know if QNX will ever
update expat, though. expat 1.95.0 was released in 2000, expat 2.0.0
was released in 2006, and QNX 6.5.0 was released in 2010.
> Assuming that this change is about building with expat1, it would
> probably be better to do something like this instead, I would think.
expat 1.95.0 through 1.95.8 used expat.h; should I still use
EXPAT_VERSION = 1 to signify that it should use xmlparse.h, use
EXPAT_NEEDS_XMLPARSE_H as Jeff suggested, or something else entirely?
^ permalink raw reply
* Re: [PATCH] rebase -i: respect core.commentchar
From: Junio C Hamano @ 2013-02-11 21:49 UTC (permalink / raw)
To: John Keeping; +Cc: git, Ralf Thielow
In-Reply-To: <20130211213900.GE2270@serenity.lan>
John Keeping <john@keeping.me.uk> writes:
>> I do not think we would want to worry about comment_char being a
>> funny character that can possibly interfere with regexp. Can't we
>> do this with "git stripspace" piped to "wc -l" or something?
>
> I didn't know about "git stripspace",...
>> > -# If you remove a line here THAT COMMIT WILL BE LOST.
>> > + sed -e "s/^/$comment_char /" >>"$todo" <<EOF
>>
>> When $comment_char is slash or backslash this will break.
>> Perhaps "stripspace --comment-lines" can be used here.
>
> Not in this case - this is adding the comment char in front of each
> line. Is there a better option than this?
>
> while read -r line
> do
> printf '%s %s\n' "$comment_char" "$line"
> done >> "$todo" <<EOF
> ...
> EOF
Perhaps
git stripspace --comment-lines <<EOF
...
EOF
is a better option that that loop.
^ permalink raw reply
* Re: [PATCH] Include xmlparse.h instead of expat.h on QNX
From: Junio C Hamano @ 2013-02-11 21:41 UTC (permalink / raw)
To: Jeff King; +Cc: Matt Kraai, git, Matt Kraai
In-Reply-To: <20130211210621.GC32740@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Mon, Feb 11, 2013 at 12:59:55PM -0800, Matt Kraai wrote:
>
>> From: Matt Kraai <matt.kraai@amo.abbott.com>
>>
>> QNX 6.3.2 through 6.5.0 include Expat 1.1, which provides xmlparse.h
>> instead of expat.h, so include the former on QNX systems.
>
> So it is not just QNX, but rather older versions of expat?
>
>> diff --git a/http-push.c b/http-push.c
>> index 9923441..55c575e 100644
>> --- a/http-push.c
>> +++ b/http-push.c
>> @@ -11,7 +11,11 @@
>> #include "list-objects.h"
>> #include "sigchain.h"
>>
>> +#ifndef __QNX__
>> #include <expat.h>
>> +#else
>> +#include <xmlparse.h>
>> +#endif
>
> If that is the case, should this #ifdef look for EXPAT_NEEDS_XMLPARSE_H,
> and that macro triggered externally?
Heh, our mails crossed. Another thing neither of us mentioned is
how compatible the subset of libexpat our codebase uses to what was
offered by the older versions of expat. I would not be surprised if
nobody has tried running the resulting binary linked with expat 1.
^ permalink raw reply
* Re: [PATCH] rebase -i: respect core.commentchar
From: John Keeping @ 2013-02-11 21:39 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Ralf Thielow
In-Reply-To: <7vzjzali6a.fsf@alter.siamese.dyndns.org>
On Mon, Feb 11, 2013 at 12:17:01PM -0800, Junio C Hamano wrote:
> John Keeping <john@keeping.me.uk> writes:
> > +comment_char=$(git config --get core.commentchar 2>/dev/null | cut -c1)
> > +: ${comment_char:=#}
>
> Hmm, somewhat ugly. I wonder if we can do this without pipe and cut.
Yeah, but I can't see a better way of doing this if we want to mimic the
behaviour of the C code in taking only the first character of the
configured value.
> > @@ -105,8 +108,8 @@ mark_action_done () {
> > sed -e 1q < "$todo" >> "$done"
> > sed -e 1d < "$todo" >> "$todo".new
> > mv -f "$todo".new "$todo"
> > - new_count=$(sane_grep -c '^[^#]' < "$done")
> > - total=$(($new_count+$(sane_grep -c '^[^#]' < "$todo")))
> > + new_count=$(sane_grep -c "^[^${comment_char}]" < "$done")
> > + total=$(($new_count+$(sane_grep -c "^[^${comment_char}]" < "$todo")))
>
> I do not think we would want to worry about comment_char being a
> funny character that can possibly interfere with regexp. Can't we
> do this with "git stripspace" piped to "wc -l" or something?
I didn't know about "git stripspace", it does make a lot of this
significantly safer. I'll work up a new version that uses that instead
of grep and with printf used where necessary.
> > @@ -116,19 +119,19 @@ mark_action_done () {
> > }
> >
> > append_todo_help () {
> > - cat >> "$todo" << EOF
> > -#
> > -# Commands:
> > -# p, pick = use commit
> > -# r, reword = use commit, but edit the commit message
> > -# e, edit = use commit, but stop for amending
> > -# s, squash = use commit, but meld into previous commit
> > -# f, fixup = like "squash", but discard this commit's log message
> > -# x, exec = run command (the rest of the line) using shell
> > -#
> > -# These lines can be re-ordered; they are executed from top to bottom.
> > -#
> > -# If you remove a line here THAT COMMIT WILL BE LOST.
> > + sed -e "s/^/$comment_char /" >>"$todo" <<EOF
>
> When $comment_char is slash or backslash this will break.
> Perhaps "stripspace --comment-lines" can be used here.
Not in this case - this is adding the comment char in front of each
line. Is there a better option than this?
while read -r line
do
printf '%s %s\n' "$comment_char" "$line"
done >> "$todo" <<EOF
...
EOF
> > +
> > +Commands:
> > + p, pick = use commit
> > + r, reword = use commit, but edit the commit message
> > + e, edit = use commit, but stop for amending
> > + s, squash = use commit, but meld into previous commit
> > + f, fixup = like "squash", but discard this commit's log message
> > + x, exec = run command (the rest of the line) using shell
> > +
> > +These lines can be re-ordered; they are executed from top to bottom.
> > +
> > +If you remove a line here THAT COMMIT WILL BE LOST.
> > EOF
> > }
> >
> > @@ -179,7 +182,7 @@ die_abort () {
> > }
> >
> > has_action () {
> > - sane_grep '^[^#]' "$1" >/dev/null
> > + sane_grep "^[^${comment_char}]" "$1" >/dev/null
>
> Likewise.
>
> > @@ -363,10 +366,10 @@ update_squash_messages () {
> > if test -f "$squash_msg"; then
> > mv "$squash_msg" "$squash_msg".bak || exit
> > count=$(($(sed -n \
> > - -e "1s/^# This is a combination of \(.*\) commits\./\1/p" \
> > + -e "1s/^. This is a combination of \(.*\) commits\./\1/p" \
>
> This one is safe.
>
> > -e "q" < "$squash_msg".bak)+1))
> > {
> > - echo "# This is a combination of $count commits."
> > + echo "$comment_char This is a combination of $count commits."
>
> But you need to do "printf" to be safe here, I think, for comment_char='\'.
>
> > @@ -375,8 +378,8 @@ update_squash_messages () {
> > commit_message HEAD > "$fixup_msg" || die "Cannot write $fixup_msg"
> > count=2
> > {
> > - echo "# This is a combination of 2 commits."
> > - echo "# The first commit's message is:"
> > + echo "$comment_char This is a combination of 2 commits."
> > + echo "$comment_char The first commit's message is:"
>
> Likewise.
>
> > @@ -385,21 +388,21 @@ update_squash_messages () {
> > squash)
> > rm -f "$fixup_msg"
> > echo
> > - echo "# This is the $(nth_string $count) commit message:"
> > + echo "$comment_char This is the $(nth_string $count) commit message:"
>
> Likewise.
>
> > echo
> > commit_message $2
> > ;;
> > fixup)
> > echo
> > - echo "# The $(nth_string $count) commit message will be skipped:"
> > + echo "$comment_char The $(nth_string $count) commit message will be skipped:"
> > echo
> > - commit_message $2 | sed -e 's/^/# /'
> > + commit_message $2 | sed -e "s/^/$comment_char /"
>
> Likewise.
Again this is adding $comment_char in front of each line, so it may need
a loop like above again.
> > peek_next_command () {
> > - sed -n -e "/^#/d" -e '/^$/d' -e "s/ .*//p" -e "q" < "$todo"
> > + sed -n -e "/^$comment_char/d" -e '/^$/d' -e "s/ .*//p" -e "q" < "$todo"
> > }
>
> Likewise.
>
> > @@ -464,7 +467,7 @@ do_next () {
> > rm -f "$msg" "$author_script" "$amend" || exit
> > read -r command sha1 rest < "$todo"
> > case "$command" in
> > - '#'*|''|noop)
> > + $comment_char*|''|noop)
>
> This is OK.
>
> > @@ -803,15 +806,15 @@ skip)
> > do_rest
> > ;;
> > edit-todo)
> > - sed -e '/^#/d' < "$todo" > "$todo".new
> > + sed -e "/^$comment_char/d" < "$todo" > "$todo".new
>
> Unsafe.
>
> > mv -f "$todo".new "$todo"
> > append_todo_help
> > - cat >> "$todo" << EOF
> > -#
> > -# You are editing the todo file of an ongoing interactive rebase.
> > -# To continue rebase after editing, run:
> > -# git rebase --continue
> > -#
> > + sed -e "s/^/$comment_char /" >>"$todo" <<EOF
>
> Unsafe.
>
> > +
> > +You are editing the todo file of an ongoing interactive rebase.
> > +To continue rebase after editing, run:
> > + git rebase --continue
> > +
> > EOF
> >
> > git_sequence_editor "$todo" ||
> > @@ -881,7 +884,7 @@ do
> >
> > if test -z "$keep_empty" && is_empty_commit $shortsha1 && ! is_merge_commit $shortsha1
> > then
> > - comment_out="# "
> > + comment_out="$comment_char "
>
> OK.
>
> > else
> > comment_out=
> > fi
> > @@ -942,20 +945,20 @@ test -s "$todo" || echo noop >> "$todo"
> > test -n "$autosquash" && rearrange_squash "$todo"
> > test -n "$cmd" && add_exec_commands "$todo"
> >
> > -cat >> "$todo" << EOF
> > -
> > -# Rebase $shortrevisions onto $shortonto
> > +echo >>"$todo"
> > +sed -e "s/^/$comment_char /" >> "$todo" << EOF
>
> Unsafe.
>
> > +Rebase $shortrevisions onto $shortonto
> > EOF
> > append_todo_help
> > -cat >> "$todo" << EOF
> > -#
> > -# However, if you remove everything, the rebase will be aborted.
> > -#
> > +sed -e "s/^/$comment_char /" >> "$todo" << EOF
>
> Unsafe.
>
> > +
> > +However, if you remove everything, the rebase will be aborted.
> > +
> > EOF
> >
> > if test -z "$keep_empty"
> > then
> > - echo "# Note that empty commits are commented out" >>"$todo"
> > + echo "$comment_char Note that empty commits are commented out" >>"$todo"
> > fi
> >
> >
> > diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> > index 8462be1..1043cdc 100755
> > --- a/t/t3404-rebase-interactive.sh
> > +++ b/t/t3404-rebase-interactive.sh
> > @@ -934,4 +934,20 @@ test_expect_success 'rebase --edit-todo can be used to modify todo' '
> > test L = $(git cat-file commit HEAD | sed -ne \$p)
> > '
> >
> > +test_expect_success 'rebase -i respects core.commentchar' '
> > + git reset --hard &&
> > + git checkout E^0 &&
> > + git config core.commentchar \; &&
>
> Try setting it to '\' or '/' or '-'; they may catch some more breakages.
>
> > + test_when_finished "git config --unset core.commentchar" &&
> > + cat >comment-lines.sh <<EOF &&
> > +#!$SHELL_PATH
> > +sed -e "2,\$ s/^/;/" "\$1" >"\$1".tmp
> > +mv "\$1".tmp "\$1"
> > +EOF
> > + chmod a+x comment-lines.sh &&
> > + test_set_editor "$(pwd)/comment-lines.sh" &&
> > + git rebase -i B &&
> > + test B = $(git cat-file commit HEAD^ | sed -ne \$p)
> > +'
> > +
> > test_done
^ permalink raw reply
* Re: [PATCH] Include xmlparse.h instead of expat.h on QNX
From: Junio C Hamano @ 2013-02-11 21:34 UTC (permalink / raw)
To: Matt Kraai; +Cc: git, Matt Kraai
In-Reply-To: <1360616395-18912-1-git-send-email-kraai@ftbfs.org>
Matt Kraai <kraai@ftbfs.org> writes:
> From: Matt Kraai <matt.kraai@amo.abbott.com>
>
> QNX 6.3.2 through 6.5.0 include Expat 1.1, which provides xmlparse.h
> instead of expat.h, so include the former on QNX systems.
>
> Signed-off-by: Matt Kraai <matt.kraai@amo.abbott.com>
> ---
Two points and a possibly irrelevant half:
- If a fix is platform specific (i.e. tempts to use #ifdef
PLATFORM_NAME), we would prefer to see a patch that that is
isolated to platform-specific compatibility layer, which would
involve:
. add compat/qnx/expat.h file that #include <xmlparse.h>
. to Makefile, add -Icompat/qnx/ to CFLAGS
- Is this really a fix for a problem specific to QNX? It looks
like this is for any platform with expat 1, no?
- What happens to people with QNX older than 6.3.2 or newer than
6.5.0 (assuming they will eventually start shipping expat 2) with
your patch?
Assuming that this change is about building with expat1, it would
probably be better to do something like this instead, I would think.
Makefile | 5 +++++
config.mak.uname | 1 +
http-push.c | 4 ++++
3 files changed, 10 insertions(+)
diff --git a/Makefile b/Makefile
index 5a2e02d..57032cc 100644
--- a/Makefile
+++ b/Makefile
@@ -43,6 +43,8 @@ all::
# Define EXPATDIR=/foo/bar if your expat header and library files are in
# /foo/bar/include and /foo/bar/lib directories.
#
+# Define EXPAT_VERSION=1 if you are trying to build with expat 1.x (e.g. QNX).
+#
# Define NO_GETTEXT if you don't want Git output to be translated.
# A translated Git requires GNU libintl or another gettext implementation,
# plus libintl-perl at runtime.
@@ -1089,6 +1091,9 @@ else
else
EXPAT_LIBEXPAT = -lexpat
endif
+ ifdef EXPAT_VERSION
+ BASIC_CFLAGS += -DEXPAT_VERSION=$(EXPAT_VERSION)
+ endif
endif
endif
diff --git a/config.mak.uname b/config.mak.uname
index bea34f0..281d834 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -536,4 +536,5 @@ ifeq ($(uname_S),QNX)
NO_R_TO_GCC_LINKER = YesPlease
NO_STRCASESTR = YesPlease
NO_STRLCPY = YesPlease
+ EXPAT_VERSION = 1
endif
diff --git a/http-push.c b/http-push.c
index 3e72e84..2fdb0cd 100644
--- a/http-push.c
+++ b/http-push.c
@@ -11,7 +11,11 @@
#include "list-objects.h"
#include "sigchain.h"
+#if EXPAT_VERSION == 1
+#include <xmlparse.h>
+#else
#include <expat.h>
+#endif
static const char http_push_usage[] =
"git http-push [--all] [--dry-run] [--force] [--verbose] <remote> [<head>...]\n";
^ permalink raw reply related
* Re: [PATCH] Include xmlparse.h instead of expat.h on QNX
From: Matt Kraai @ 2013-02-11 21:24 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, Matt Kraai
In-Reply-To: <20130211210621.GC32740@sigill.intra.peff.net>
On Mon, Feb 11, 2013 at 04:06:21PM -0500, Jeff King wrote:
> On Mon, Feb 11, 2013 at 12:59:55PM -0800, Matt Kraai wrote:
>
> > From: Matt Kraai <matt.kraai@amo.abbott.com>
> >
> > QNX 6.3.2 through 6.5.0 include Expat 1.1, which provides xmlparse.h
> > instead of expat.h, so include the former on QNX systems.
>
> So it is not just QNX, but rather older versions of expat?
Yes, Expat 1.1 and 1.2 provide xmlparse.h, whereas 1.95.0 and later
provide expat.h.
> > diff --git a/http-push.c b/http-push.c
> > index 9923441..55c575e 100644
> > --- a/http-push.c
> > +++ b/http-push.c
> > @@ -11,7 +11,11 @@
> > #include "list-objects.h"
> > #include "sigchain.h"
> >
> > +#ifndef __QNX__
> > #include <expat.h>
> > +#else
> > +#include <xmlparse.h>
> > +#endif
>
> If that is the case, should this #ifdef look for EXPAT_NEEDS_XMLPARSE_H,
> and that macro triggered externally? Either in the QNX section of the
> Makefile, or potentially by an autoconf macro?
I'll submit another patch shortly that does so, defining the variable
in the QNX section of config.mak.uname.
^ permalink raw reply
* Re: [PATCH] log: re-encode commit messages before grepping
From: Jeff King @ 2013-02-11 21:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Thomas Haller, Git List
In-Reply-To: <7vmwvalfnn.fsf@alter.siamese.dyndns.org>
On Mon, Feb 11, 2013 at 01:11:24PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > 1. I suppose we could also use $LANG or one of the $LC_* variables to
> > guess at the encoding of the user's pattern. But I think using the
> > output encoding makes the most sense, since then the pattern you
> > searched for will actually be in the output.
>
> I agree. In addition, if we were to do anything with LANG/LC_CTYPE,
> it should be done at the layer that implements log-output-encoding
> (e.g. lack of configured encoding with nonstandard LANG/LC_CTYPE
> would use the locale, or something), I think.
Yeah, I had the same thought. I'll leave it to somebody who knows/cares
more about i18n. In my world view, utf8 is good enough for everyone. :)
-Peff
^ permalink raw reply
* Re: [PATCH] log: re-encode commit messages before grepping
From: Junio C Hamano @ 2013-02-11 21:11 UTC (permalink / raw)
To: Jeff King; +Cc: Thomas Haller, Git List
In-Reply-To: <20130211205958.GA32740@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> 1. I suppose we could also use $LANG or one of the $LC_* variables to
> guess at the encoding of the user's pattern. But I think using the
> output encoding makes the most sense, since then the pattern you
> searched for will actually be in the output.
I agree. In addition, if we were to do anything with LANG/LC_CTYPE,
it should be done at the layer that implements log-output-encoding
(e.g. lack of configured encoding with nonstandard LANG/LC_CTYPE
would use the locale, or something), I think.
> 2. There are still problems with utf8 normalization. E.g., my tests
> represent utf-8 é with \xc3\xa9 (the code point for that glyph),
> but it could also be represented by \x65\xcc\x81 (e + combining
> acute). But that is not a new problem; it is an inherent issue with
> grepping utf8. We might in the future want to offer an option to
> normalize utf8 (or possibly the regex library can be taught to
> handle this).
True; in either case, this caller (or any other callers) should
care. Only grep_buffer() (actually, grep_source_1()) needs to be
taught about it.
> 4. I'm still not clear on why "--graph --no-walk" wants to look at
> commit_match after we have already cleared the commit buffer. I
> agree it's nonsensical, but I wonder if it might be a symptom of an
> underlying bug or inefficiency.
Yeah, that may be something we may want to check, I agree.
The aded test is also nice. Thanks.
> diff --git a/t/t4210-log-i18n.sh b/t/t4210-log-i18n.sh
> new file mode 100755
> index 0000000..52a7472
> --- /dev/null
> +++ b/t/t4210-log-i18n.sh
> @@ -0,0 +1,58 @@
> +#!/bin/sh
> +
> +test_description='test log with i18n features'
> +. ./test-lib.sh
> +
> +# two forms of é
> +utf8_e=$(printf '\303\251')
> +latin1_e=$(printf '\351')
> +
> +test_expect_success 'create commits in different encodings' '
> + test_tick &&
> + cat >msg <<-EOF &&
> + utf8
> +
> + t${utf8_e}st
> + EOF
> + git add msg &&
> + git -c i18n.commitencoding=utf8 commit -F msg &&
> + cat >msg <<-EOF &&
> + latin1
> +
> + t${latin1_e}st
> + EOF
> + git add msg &&
> + git -c i18n.commitencoding=ISO-8859-1 commit -F msg
> +'
> +
> +test_expect_success 'log --grep searches in log output encoding (utf8)' '
> + cat >expect <<-\EOF &&
> + latin1
> + utf8
> + EOF
> + git log --encoding=utf8 --format=%s --grep=$utf8_e >actual &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'log --grep searches in log output encoding (latin1)' '
> + cat >expect <<-\EOF &&
> + latin1
> + utf8
> + EOF
> + git log --encoding=ISO-8859-1 --format=%s --grep=$latin1_e >actual &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'log --grep does not find non-reencoded values (utf8)' '
> + >expect &&
> + git log --encoding=utf8 --format=%s --grep=$latin1_e >actual &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'log --grep does not find non-reencoded values (latin1)' '
> + >expect &&
> + git log --encoding=ISO-8859-1 --format=%s --grep=$utf8_e >actual &&
> + test_cmp expect actual
> +'
> +
> +test_done
^ permalink raw reply
* Re: [PATCH] Include xmlparse.h instead of expat.h on QNX
From: Jeff King @ 2013-02-11 21:06 UTC (permalink / raw)
To: Matt Kraai; +Cc: git, Junio C Hamano, Matt Kraai
In-Reply-To: <1360616395-18912-1-git-send-email-kraai@ftbfs.org>
On Mon, Feb 11, 2013 at 12:59:55PM -0800, Matt Kraai wrote:
> From: Matt Kraai <matt.kraai@amo.abbott.com>
>
> QNX 6.3.2 through 6.5.0 include Expat 1.1, which provides xmlparse.h
> instead of expat.h, so include the former on QNX systems.
So it is not just QNX, but rather older versions of expat?
> diff --git a/http-push.c b/http-push.c
> index 9923441..55c575e 100644
> --- a/http-push.c
> +++ b/http-push.c
> @@ -11,7 +11,11 @@
> #include "list-objects.h"
> #include "sigchain.h"
>
> +#ifndef __QNX__
> #include <expat.h>
> +#else
> +#include <xmlparse.h>
> +#endif
If that is the case, should this #ifdef look for EXPAT_NEEDS_XMLPARSE_H,
and that macro triggered externally? Either in the QNX section of the
Makefile, or potentially by an autoconf macro?
-Peff
^ permalink raw reply
* Re: git-completion.bash --local
From: Jeff King @ 2013-02-11 21:04 UTC (permalink / raw)
To: Dasa Paddock; +Cc: git@vger.kernel.org
In-Reply-To: <85E0E68E8961D64E9200C534AC5E1B240A443EDC@RED-INF-EXMB-P1.esri.com>
On Sat, Feb 09, 2013 at 06:37:28PM +0000, Dasa Paddock wrote:
> I think this line should include --local:
>
> https://github.com/git/git/blob/next/contrib/completion/git-completion.bash#L1782
> "--global|--system|--file=*)"
Yeah, I think that makes sense.
Care to prepare a patch?
-Peff
^ permalink raw reply
* [PATCH] Include xmlparse.h instead of expat.h on QNX
From: Matt Kraai @ 2013-02-11 20:59 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Matt Kraai
From: Matt Kraai <matt.kraai@amo.abbott.com>
QNX 6.3.2 through 6.5.0 include Expat 1.1, which provides xmlparse.h
instead of expat.h, so include the former on QNX systems.
Signed-off-by: Matt Kraai <matt.kraai@amo.abbott.com>
---
http-push.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/http-push.c b/http-push.c
index 9923441..55c575e 100644
--- a/http-push.c
+++ b/http-push.c
@@ -11,7 +11,11 @@
#include "list-objects.h"
#include "sigchain.h"
+#ifndef __QNX__
#include <expat.h>
+#else
+#include <xmlparse.h>
+#endif
static const char http_push_usage[] =
"git http-push [--all] [--dry-run] [--force] [--verbose] <remote> [<head>...]\n";
--
1.8.1.2.547.g7ce9def
^ permalink raw reply related
* [PATCH] log: re-encode commit messages before grepping
From: Jeff King @ 2013-02-11 20:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Thomas Haller, Git List
In-Reply-To: <7v621ymxfv.fsf@alter.siamese.dyndns.org>
If you run "git log --grep=foo", we will run your regex on
the literal bytes of the commit message. This can provide
confusing results if the commit message is not in the same
encoding as your grep expression (or worse, you have commits
in multiple encodings, in which case your regex would need
to be written to match either encoding). On top of this, we
might also be grepping in the commit's notes, which are
already re-encoded, potentially leading to grepping in a
buffer with mixed encodings concatenated. This is insanity,
but most people never noticed, because their terminal and
their commit encodings all match.
Instead, let's massage the to-be-grepped commit into a
standardized encoding. There is not much point in adding a
flag for "this is the encoding I expect my grep pattern to
match"; the only sane choice is for it to use the log output
encoding. That is presumably what the user's terminal is
using, and it means that the patterns found by the grep will
match the output produced by git.
As a bonus, this fixes a potential segfault in commit_match
when commit->buffer is NULL, as we now build on logmsg_reencode,
which handles reading the commit buffer from disk if
necessary. The segfault can be triggered with:
git commit -m 'text1' --allow-empty
git commit -m 'text2' --allow-empty
git log --graph --no-walk --grep 'text2'
which arguably does not make any sense (--graph inherently
wants a connected history, and by --no-walk the command line
is telling us to show discrete points in history without
connectivity), and we probably should forbid the
combination, but that is a separate issue.
Signed-off-by: Jeff King <peff@peff.net>
---
A few notes:
1. I suppose we could also use $LANG or one of the $LC_* variables to
guess at the encoding of the user's pattern. But I think using the
output encoding makes the most sense, since then the pattern you
searched for will actually be in the output.
2. There are still problems with utf8 normalization. E.g., my tests
represent utf-8 é with \xc3\xa9 (the code point for that glyph),
but it could also be represented by \x65\xcc\x81 (e + combining
acute). But that is not a new problem; it is an inherent issue with
grepping utf8. We might in the future want to offer an option to
normalize utf8 (or possibly the regex library can be taught to
handle this).
3. If the commit does need to be re-encoded, we end up doing so here,
and then potentially again if we actually show the commit. So
there may be some room to optimize that by stashing the re-encoded
version somewhere (or it may not make a big difference, I haven't
measured).
4. I'm still not clear on why "--graph --no-walk" wants to look at
commit_match after we have already cleared the commit buffer. I
agree it's nonsensical, but I wonder if it might be a symptom of an
underlying bug or inefficiency.
revision.c | 27 ++++++++++++++++++-------
t/t4210-log-i18n.sh | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 78 insertions(+), 7 deletions(-)
create mode 100755 t/t4210-log-i18n.sh
diff --git a/revision.c b/revision.c
index d7562ee..ef60205 100644
--- a/revision.c
+++ b/revision.c
@@ -2268,7 +2268,10 @@ static int commit_match(struct commit *commit, struct rev_info *opt)
static int commit_match(struct commit *commit, struct rev_info *opt)
{
int retval;
+ const char *encoding;
+ char *message;
struct strbuf buf = STRBUF_INIT;
+
if (!opt->grep_filter.pattern_list && !opt->grep_filter.header_list)
return 1;
@@ -2279,13 +2282,23 @@ static int commit_match(struct commit *commit, struct rev_info *opt)
strbuf_addch(&buf, '\n');
}
+ /*
+ * We grep in the user's output encoding, under the assumption that it
+ * is the encoding they are most likely to write their grep pattern
+ * for. In addition, it means we will match the "notes" encoding below,
+ * so we will not end up with a buffer that has two different encodings
+ * in it.
+ */
+ encoding = get_log_output_encoding();
+ message = logmsg_reencode(commit, encoding);
+
/* Copy the commit to temporary if we are using "fake" headers */
if (buf.len)
- strbuf_addstr(&buf, commit->buffer);
+ strbuf_addstr(&buf, message);
if (opt->grep_filter.header_list && opt->mailmap) {
if (!buf.len)
- strbuf_addstr(&buf, commit->buffer);
+ strbuf_addstr(&buf, message);
commit_rewrite_person(&buf, "\nauthor ", opt->mailmap);
commit_rewrite_person(&buf, "\ncommitter ", opt->mailmap);
@@ -2294,18 +2307,18 @@ static int commit_match(struct commit *commit, struct rev_info *opt)
/* Append "fake" message parts as needed */
if (opt->show_notes) {
if (!buf.len)
- strbuf_addstr(&buf, commit->buffer);
- format_display_notes(commit->object.sha1, &buf,
- get_log_output_encoding(), 1);
+ strbuf_addstr(&buf, message);
+ format_display_notes(commit->object.sha1, &buf, encoding, 1);
}
- /* Find either in the commit object, or in the temporary */
+ /* Find either in the original commit message, or in the temporary */
if (buf.len)
retval = grep_buffer(&opt->grep_filter, buf.buf, buf.len);
else
retval = grep_buffer(&opt->grep_filter,
- commit->buffer, strlen(commit->buffer));
+ message, strlen(message));
strbuf_release(&buf);
+ logmsg_free(message, commit);
return retval;
}
diff --git a/t/t4210-log-i18n.sh b/t/t4210-log-i18n.sh
new file mode 100755
index 0000000..52a7472
--- /dev/null
+++ b/t/t4210-log-i18n.sh
@@ -0,0 +1,58 @@
+#!/bin/sh
+
+test_description='test log with i18n features'
+. ./test-lib.sh
+
+# two forms of é
+utf8_e=$(printf '\303\251')
+latin1_e=$(printf '\351')
+
+test_expect_success 'create commits in different encodings' '
+ test_tick &&
+ cat >msg <<-EOF &&
+ utf8
+
+ t${utf8_e}st
+ EOF
+ git add msg &&
+ git -c i18n.commitencoding=utf8 commit -F msg &&
+ cat >msg <<-EOF &&
+ latin1
+
+ t${latin1_e}st
+ EOF
+ git add msg &&
+ git -c i18n.commitencoding=ISO-8859-1 commit -F msg
+'
+
+test_expect_success 'log --grep searches in log output encoding (utf8)' '
+ cat >expect <<-\EOF &&
+ latin1
+ utf8
+ EOF
+ git log --encoding=utf8 --format=%s --grep=$utf8_e >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'log --grep searches in log output encoding (latin1)' '
+ cat >expect <<-\EOF &&
+ latin1
+ utf8
+ EOF
+ git log --encoding=ISO-8859-1 --format=%s --grep=$latin1_e >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'log --grep does not find non-reencoded values (utf8)' '
+ >expect &&
+ git log --encoding=utf8 --format=%s --grep=$latin1_e >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'log --grep does not find non-reencoded values (latin1)' '
+ >expect &&
+ git log --encoding=ISO-8859-1 --format=%s --grep=$utf8_e >actual &&
+ test_cmp expect actual
+'
+
+test_done
--
1.8.1.2.11.g1a2f572
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox