* [PATCH v2 4/6] git-gui: Add Japanese language code
From: Satoshi Yasushima @ 2016-09-06 16:02 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jakub Narębski, Pat Thoyts,
Satoshi Yasushima
In-Reply-To: <1473177741-9576-1-git-send-email-s.yasushima@gmail.com>
Signed-off-by: Satoshi Yasushima <s.yasushima@gmail.com>
---
po/ja.po | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/po/ja.po b/po/ja.po
index b140e8b..23974cc 100644
--- a/po/ja.po
+++ b/po/ja.po
@@ -11,7 +11,7 @@ msgstr ""
"PO-Revision-Date: 2010-02-02 19:03+0900\n"
"Last-Translator: しらいし ななこ <nanako3@lavabit.com>\n"
"Language-Team: Japanese\n"
-"Language: \n"
+"Language: ja\n"
"MIME-Version: 1.0\n"
"Content-Type: text/plain; charset=UTF-8\n"
"Content-Transfer-Encoding: 8bit\n"
--
2.8.2.windows.1
^ permalink raw reply related
* [PATCH v2 5/6] git-gui: Update Japanese translation
From: Satoshi Yasushima @ 2016-09-06 16:02 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jakub Narębski, Pat Thoyts,
Satoshi Yasushima
In-Reply-To: <1473177741-9576-1-git-send-email-s.yasushima@gmail.com>
Signed-off-by: Satoshi Yasushima <s.yasushima@gmail.com>
---
po/ja.po | 77 +++++++++++++++++++++++++++++-----------------------------------
1 file changed, 35 insertions(+), 42 deletions(-)
diff --git a/po/ja.po b/po/ja.po
index 23974cc..deaf8e3 100644
--- a/po/ja.po
+++ b/po/ja.po
@@ -102,6 +102,8 @@ msgstr "準備完了"
msgid ""
"Display limit (gui.maxfilesdisplayed = %s) reached, not showing all %s files."
msgstr ""
+"表示可能な限界 (gui.maxfilesdisplayed = %s) に達しため、全体で%s個のファイル"
+"を表示できません"
#: git-gui.sh:2101
msgid "Unmodified"
@@ -128,23 +130,20 @@ msgid "File type changed, not staged"
msgstr "ファイル型変更、コミット未予定"
#: git-gui.sh:2109 git-gui.sh:2110
-#, fuzzy
msgid "File type changed, old type staged for commit"
-msgstr "ファイル型変更、コミット未予定"
+msgstr "ファイル型変更、旧型コミット予定済"
#: git-gui.sh:2111
msgid "File type changed, staged"
msgstr "ファイル型変更、コミット予定済"
#: git-gui.sh:2112
-#, fuzzy
msgid "File type change staged, modification not staged"
-msgstr "ファイル型変更、コミット未予定"
+msgstr "ファイル型変更コミット予定済、変更コミット未予定"
#: git-gui.sh:2113
-#, fuzzy
msgid "File type change staged, file missing"
-msgstr "ファイル型変更、コミット予定済"
+msgstr "ファイル型変更コミット予定済、ファイル無し"
#: git-gui.sh:2115
msgid "Untracked, not staged"
@@ -408,10 +407,9 @@ msgstr "SSH キーを表示"
#: git-gui.sh:3014 git-gui.sh:3146
msgid "Usage"
-msgstr ""
+msgstr "使い方"
#: git-gui.sh:3095 lib/blame.tcl:573
-#, fuzzy
msgid "Error"
msgstr "エラー"
@@ -1112,9 +1110,8 @@ msgid "Find Text..."
msgstr "テキストを検索"
#: lib/blame.tcl:288
-#, fuzzy
msgid "Goto Line..."
-msgstr "複製…"
+msgstr "指定行に移動…"
#: lib/blame.tcl:297
msgid "Do Full Copy Detection"
@@ -1310,7 +1307,7 @@ msgstr "共有(最高速・非推奨・バックアップ無し)"
#: lib/choose_repository.tcl:545
msgid "Recursively clone submodules too"
-msgstr ""
+msgstr "サブモジュールも再帰的に複製する"
#: lib/choose_repository.tcl:579 lib/choose_repository.tcl:626
#: lib/choose_repository.tcl:772 lib/choose_repository.tcl:842
@@ -1435,12 +1432,11 @@ msgstr "ファイル"
#: lib/choose_repository.tcl:981
msgid "Cannot clone submodules."
-msgstr ""
+msgstr "サブモジュールが複製できません。"
#: lib/choose_repository.tcl:990
-#, fuzzy
msgid "Cloning submodules"
-msgstr "%s から複製しています"
+msgstr "サブモジュールを複製しています"
#: lib/choose_repository.tcl:1015
msgid "Initial file checkout failed."
@@ -1515,11 +1511,11 @@ msgstr "前"
#: lib/search.tcl:52
msgid "RegExp"
-msgstr ""
+msgstr "正規表現"
#: lib/search.tcl:54
msgid "Case"
-msgstr ""
+msgstr "大文字小文字を区別"
#: lib/status_bar.tcl:87
#, tcl-format
@@ -1635,9 +1631,9 @@ msgid "Running %s requires a selected file."
msgstr "ファイルを選択してから %s を起動してください。"
#: lib/tools.tcl:91
-#, fuzzy, tcl-format
+#, tcl-format
msgid "Are you sure you want to run %1$s on file \"%2$s\"?"
-msgstr "本当に %s を起動しますか?"
+msgstr "本当にファイル \"%2$s\"で %1$s を起動しますか?"
#: lib/tools.tcl:95
#, tcl-format
@@ -1817,16 +1813,15 @@ msgstr "トラッキングブランチを合わせる"
#: lib/option.tcl:151
msgid "Use Textconv For Diffs and Blames"
-msgstr ""
+msgstr "diff と注釈に textconv を使う"
#: lib/option.tcl:152
msgid "Blame Copy Only On Changed Files"
msgstr "変更されたファイルのみコピー検知を行なう"
#: lib/option.tcl:153
-#, fuzzy
msgid "Maximum Length of Recent Repositories List"
-msgstr "最近使ったリポジトリ"
+msgstr "最近使ったリポジトリ一覧の上限"
#: lib/option.tcl:154
msgid "Minimum Letters To Blame Copy On"
@@ -1842,7 +1837,7 @@ msgstr "diff の文脈行数"
#: lib/option.tcl:157
msgid "Additional Diff Parameters"
-msgstr ""
+msgstr "diff の追加引数"
#: lib/option.tcl:158
msgid "Commit Message Text Width"
@@ -1858,19 +1853,19 @@ msgstr "ファイル内容のデフォールトエンコーディング"
#: lib/option.tcl:161
msgid "Warn before committing to a detached head"
-msgstr ""
+msgstr "分離 HEAD のコミット前に警告する"
#: lib/option.tcl:162
msgid "Staging of untracked files"
-msgstr ""
+msgstr "管理外のファイルをコミット予定する"
#: lib/option.tcl:163
msgid "Show untracked files"
-msgstr ""
+msgstr "管理外のファイルを表示する"
#: lib/option.tcl:164
msgid "Tab spacing"
-msgstr ""
+msgstr "タブ幅"
#: lib/option.tcl:210
msgid "Change"
@@ -1979,22 +1974,19 @@ msgstr "%s から削除されたトラッキング・ブランチを刈ってい
#: lib/transport.tcl:25
msgid "fetch all remotes"
-msgstr ""
+msgstr "すべてのリモートを取得"
#: lib/transport.tcl:26
-#, fuzzy
msgid "Fetching new changes from all remotes"
-msgstr "%s から新しい変更をフェッチしています"
+msgstr "すべてのリモートから新しい変更をフェッチしています"
#: lib/transport.tcl:40
-#, fuzzy
msgid "remote prune all remotes"
-msgstr "リモート刈込 %s"
+msgstr "リモート刈込 すべてのリモート"
#: lib/transport.tcl:41
-#, fuzzy
msgid "Pruning tracking branches deleted from all remotes"
-msgstr "%s から削除されたトラッキング・ブランチを刈っています"
+msgstr "すべてのリモートから削除されたトラッキング・ブランチを刈っています"
#: lib/transport.tcl:54 lib/transport.tcl:92 lib/transport.tcl:110
#: lib/remote_add.tcl:162
@@ -2247,7 +2239,7 @@ msgstr "コミットに %s を加えています"
#: lib/index.tcl:380
#, tcl-format
msgid "Stage %d untracked files?"
-msgstr ""
+msgstr "管理外の %d ファイルをコミット予定としますか？"
#: lib/index.tcl:428
#, tcl-format
@@ -2452,6 +2444,13 @@ msgid ""
" \n"
" Do you really want to proceed with your Commit?"
msgstr ""
+"分離 HEAD での変更をコミットしようとしています。"
+"これは潜在的に危険な行為で、理由は別のブランチへの切り替えで"
+"変更が消失し、reflog からの事後復旧も困難となるためです。"
+"おそらくこのコミットはキャンセルし新しく作成したブランチで"
+"行うべきです。\n"
+"\n"
+" 本当にコミットを続行しますか？"
#: lib/commit.tcl:290
msgid "Calling commit-msg hook..."
@@ -2593,11 +2592,11 @@ msgstr "%2$s にある %1$s をセットアップします"
#: lib/line.tcl:17
msgid "Goto Line:"
-msgstr ""
+msgstr "行番号"
#: lib/line.tcl:23
msgid "Go"
-msgstr ""
+msgstr "移動"
#: lib/branch_create.tcl:23
msgid "Create Branch"
@@ -2681,9 +2680,3 @@ msgstr "スペルチェッカーが予想外の EOF を返しました"
#: lib/spellcheck.tcl:392
msgid "Spell Checker Failed"
msgstr "スペルチェック失敗"
-
-#~ msgid "Displaying only %s of %s files."
-#~ msgstr "全体で%s個の内の%sファイルだけ表示しています"
-
-#~ msgid "Case-Sensitive"
-#~ msgstr "大文字小文字を区別"
--
2.8.2.windows.1
^ permalink raw reply related
* Re: [PATCH 3/3] diff_grep: add assertions verifying that the buffers are NUL-terminated
From: Johannes Schindelin @ 2016-09-06 16:04 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano
In-Reply-To: <20160906070836.7brjtijxq7nukjkq@sigill.intra.peff.net>
Hi Peff,
On Tue, 6 Sep 2016, Jeff King wrote:
> On Mon, Sep 05, 2016 at 05:45:09PM +0200, Johannes Schindelin wrote:
>
> > Before calling regexec() on the file contents, we better be certain that
> > the strings fulfill the contract of C strings assumed by said function.
>
> If you have a buffer that is exactly "size" bytes and you are worried
> about regexec reading off the end, then...
>
> > diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
> > index 55067ca..88820b6 100644
> > --- a/diffcore-pickaxe.c
> > +++ b/diffcore-pickaxe.c
> > @@ -49,6 +49,8 @@ static int diff_grep(mmfile_t *one, mmfile_t *two,
> > xpparam_t xpp;
> > xdemitconf_t xecfg;
> >
> > + assert(!one || one->ptr[one->size] == '\0');
> > + assert(!two || two->ptr[two->size] == '\0');
> > if (!one)
> > return !regexec(regexp, two->ptr, 1, ®match, 0);
>
> ...don't your asserts also read off the end?
Yes, they would read off the end, *unless* a NUL was somehow appended to
the buffers.
> So you might still segfault, though you do catch a case where we have N
> bytes of junk before the end of the page (and you have a 255/256 chance
> of catching it).
Right. The assertion may fail, or a segfault happen. In both cases,
assumptions are violated and we need to fix the code.
Ciao,
Dscho
^ permalink raw reply
* [PATCH v2 2/6] git-gui: consistently use the same word for "blame" in Japanese
From: Satoshi Yasushima @ 2016-09-06 16:02 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jakub Narębski, Pat Thoyts,
Satoshi Yasushima
In-Reply-To: <1473177741-9576-1-git-send-email-s.yasushima@gmail.com>
Signed-off-by: Satoshi Yasushima <s.yasushima@gmail.com>
---
po/ja.po | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/po/ja.po b/po/ja.po
index 8a2c16f..b692b5c 100644
--- a/po/ja.po
+++ b/po/ja.po
@@ -598,7 +598,7 @@ msgstr "文脈を見せる"
#: lib/blame.tcl:291
msgid "Blame Parent Commit"
-msgstr "親コミットを註釈"
+msgstr "親コミットを注釈"
#: lib/blame.tcl:450
#, tcl-format
@@ -2052,7 +2052,7 @@ msgstr "コピーを検知する最少文字数"
#: lib/option.tcl:151
msgid "Blame History Context Radius (days)"
-msgstr "註釈する履歴半径（日数）"
+msgstr "注釈する履歴半径（日数）"
#: lib/option.tcl:152
msgid "Number of Diff Context Lines"
--
2.8.2.windows.1
^ permalink raw reply related
* [PATCH v2 1/6] git-gui: consistently use the same word for "remote" in Japanese
From: Satoshi Yasushima @ 2016-09-06 16:02 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jakub Narębski, Pat Thoyts,
Satoshi Yasushima
In-Reply-To: <1472913822-9088-1-git-send-email-s.yasushima@gmail.com>
Signed-off-by: Satoshi Yasushima <s.yasushima@gmail.com>
---
po/ja.po | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/po/ja.po b/po/ja.po
index 9aff249..8a2c16f 100644
--- a/po/ja.po
+++ b/po/ja.po
@@ -765,7 +765,8 @@ msgstr "トラッキング・ブランチを選択して下さい。"
#: lib/branch_create.tcl:140
#, tcl-format
msgid "Tracking branch %s is not a branch in the remote repository."
-msgstr "トラッキング・ブランチ %s は遠隔リポジトリのブランチではありません。"
+msgstr ""
+"トラッキング・ブランチ %s はリモートリポジトリのブランチではありません。"
#: lib/branch_create.tcl:153 lib/branch_rename.tcl:86
msgid "Please supply a branch name."
@@ -2192,7 +2193,7 @@ msgstr "%2$s にある %1$s をセットアップします"
#: lib/remote_branch_delete.tcl:29 lib/remote_branch_delete.tcl:34
msgid "Delete Branch Remotely"
-msgstr "遠隔でブランチ削除"
+msgstr "リモートブランチ削除"
#: lib/remote_branch_delete.tcl:47
msgid "From Repository"
@@ -2504,7 +2505,7 @@ msgstr "%s から新しい変更をフェッチしています"
#: lib/transport.tcl:18
#, tcl-format
msgid "remote prune %s"
-msgstr "遠隔刈込 %s"
+msgstr "リモート刈込 %s"
#: lib/transport.tcl:19
#, tcl-format
--
2.8.2.windows.1
^ permalink raw reply related
* Re: [PATCH 2/3] diff_populate_filespec: NUL-terminate buffers
From: Johannes Schindelin @ 2016-09-06 16:02 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano
In-Reply-To: <20160906070604.i5rojh3kyc7x7kso@sigill.intra.peff.net>
Hi Peff,
On Tue, 6 Sep 2016, Jeff King wrote:
> On Mon, Sep 05, 2016 at 05:45:06PM +0200, Johannes Schindelin wrote:
>
> > It is true that many code paths populate the mmfile_t structure
> > silently appending a NUL, e.g. when running textconv on a temporary
> > file and reading the results back into an strbuf.
> >
> > The assumption is most definitely wrong, however, when mmap()ing a
> > file.
> >
> > Practically, we seemed to be lucky that the bytes after mmap()ed
> > memory were 1) accessible and 2) somehow contained NUL bytes
> > *somewhere*.
> >
> > In a use case reported by Chris Sidi, it turned out that the mmap()ed
> > file had the precise size of a memory page, and on Windows the bytes
> > after memory-mapped pages are in general not valid.
> >
> > This patch works around that issue, giving us time to discuss the best
> > course how to fix this problem more generally.
>
> I don't know if we are in that much of a rush.
I am ;-)
> This bug has been around for many years (the thread I linked earlier is
> from 2012). Yes, it's bad and annoying, but we can probably spend a few
> days discussing the solution.
Sure we can. But I got to have a solution due to a recent switch from
storing LF to storing CR/LF in the repository (that resulted in a
noticable performance improvement): combined with -G being an integral
part of the workflow in the project that reported the issue, it is
essential that this bug gets fixed. Before I go mostly offline.
> > diff --git a/diff.c b/diff.c
> > index 534c12e..32f7f46 100644
> > --- a/diff.c
> > +++ b/diff.c
> > @@ -2826,6 +2826,15 @@ int diff_populate_filespec(struct diff_filespec *s, unsigned int flags)
> > s->data = strbuf_detach(&buf, &size);
> > s->size = size;
> > s->should_free = 1;
> > + } else {
> > + /* data must be NUL-terminated so e.g. for regexec() */
> > + char *data = xmalloc(s->size + 1);
> > + memcpy(data, s->data, s->size);
> > + data[s->size] = '\0';
> > + munmap(s->data, s->size);
> > + s->should_munmap = 0;
> > + s->data = data;
> > + s->should_free = 1;
> > }
>
> Without having done a complete audit recently, my gut and my
> recollection from previous discussions is that regexec() really is the
> culprit here for the diff code[1]. If we are going to do a workaround
> like this, I think we could limit it only to cases where know it
> matters, like --pickaxe-regex.
Sure.
We could introduce a new NEEDS_NUL flag.
It will still be quite tricky, because we have to touch a function that is
rather at the bottom of the food chain: diff_populate_filespec() is called
from fill_textconv(), which in turn is called from pickaxe_match(), and
only pickaxe_match() knows whether we want to call regexec() or not (it
depends on its regexp parameter).
Adding a flag to diff_populate_filespec() sounds really reasonable until
you see how many call sites fill_textconv() has.
See below for a better idea.
> Can it be triggered with -G?
It can, and it is, as demonstrated by the test I introduced in 1/3.
> I thought that operated on the diff content itself, which would always
> be in a heap buffer (which should be NUL terminated, but if it isn't,
> that would be a separate fix from this).
That is true.
Except when preimage or postimage does not exist. In which case we call
regexec(regexp, two->ptr, 1, ®match, 0);
or the same with one->ptr. Note the notable absence of two->size.
> [1] We do make the assumption elsewhere that git objects are
> NUL-terminated, but that is enforced by the object-reading code
> (with the exception of streamed blobs, but those are obviously dealt
> with separately anyway).
I know. I am the reason you introduced that, because I added code to
fsck.c that assumes that tag/commit messages are NUL-terminated.
So now for the better idea.
While I was researching the code for this reply, I hit upon one thing that
I never knew existed, introduced in f96e567 (grep: use REG_STARTEND for
all matching if available, 2010-05-22). Apparently, NetBSD introduced an
extension to regexec() where you can specify buffer boundaries using
REG_STARTEND. Which is pretty much what we need.
So I have this as my current proof-of-concept (which passes the test
suite, but is white-space corrupted, because I really have no time to get
non-white-space-corrupted text into this here mailer):
-- snipsnap --
diff --git a/diff.c b/diff.c
index 534c12e..2c5a360 100644
--- a/diff.c
+++ b/diff.c
@@ -951,7 +951,13 @@ static int find_word_boundaries(mmfile_t *buffer,
regex_t *word_regex,
{
if (word_regex && *begin < buffer->size) {
regmatch_t match[1];
- if (!regexec(word_regex, buffer->ptr + *begin, 1, match,
0)) {
+ int f = 0;
+#ifdef REG_STARTEND
+ match[0].rm_so = 0;
+ match[0].rm_eo = *end - *begin;
+ f = REG_STARTEND;
+#endif
+ if (!regexec(word_regex, buffer->ptr + *begin, 1, match,
f)) {
char *p = memchr(buffer->ptr + *begin +
match[0].rm_so,
'\n', match[0].rm_eo -
match[0].rm_so);
*end = p ? p - buffer->ptr : match[0].rm_eo +
*begin;
@@ -994,7 +1000,7 @@ static void diff_words_fill(struct diff_words_buffer
*buffer, mmfile_t *out,
buffer->orig[0].begin = buffer->orig[0].end = buffer->text.ptr;
buffer->orig_nr = 1;
- for (i = 0; i < buffer->text.size; i++) {
+ for (i = 0, j = buffer->text.size; i < buffer->text.size; i++) {
if (find_word_boundaries(&buffer->text, word_regex, &i,
&j))
return;
diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index 55067ca..2cd09e2 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -23,7 +23,9 @@ static void diffgrep_consume(void *priv, char *line,
unsigned long len)
{
struct diffgrep_cb *data = priv;
regmatch_t regmatch;
+#ifndef REG_STARTEND
int hold;
+#endif
if (line[0] != '+' && line[0] != '-')
return;
@@ -33,11 +35,18 @@ static void diffgrep_consume(void *priv, char *line,
unsigned long len)
* caller early.
*/
return;
+#ifdef REG_STARTEND
+ regmatch.rm_so = 0;
+ regmatch.rm_eo = len;
+ data->hit = !regexec(data->regexp, line + 1, 1,
+ ®match, REG_STARTEND);
+#else
/* Yuck -- line ought to be "const char *"! */
hold = line[len];
line[len] = '\0';
- data->hit = !regexec(data->regexp, line + 1, 1, ®match, 0);
+ data->hit = !regexec(data->regexp, line + 1, 1, ®match, f);
line[len] = hold;
+#endif
}
static int diff_grep(mmfile_t *one, mmfile_t *two,
@@ -49,10 +58,24 @@ static int diff_grep(mmfile_t *one, mmfile_t *two,
xpparam_t xpp;
xdemitconf_t xecfg;
- if (!one)
- return !regexec(regexp, two->ptr, 1, ®match, 0);
- if (!two)
- return !regexec(regexp, one->ptr, 1, ®match, 0);
+ if (!one) {
+ int flags = 0;
+#ifdef REG_STARTEND
+ regmatch.rm_so = 0;
+ regmatch.rm_eo = two->size;
+ flags = REG_STARTEND;
+#endif
+ return !regexec(regexp, two->ptr, 1, ®match, flags);
+ }
+ if (!two) {
+ int flags = 0;
+#ifdef REG_STARTEND
+ regmatch.rm_so = 0;
+ regmatch.rm_eo = one->size;
+ flags = REG_STARTEND;
+#endif
+ return !regexec(regexp, one->ptr, 1, ®match, flags);
+ }
/*
* We have both sides; need to run textual diff and see if
@@ -83,7 +106,13 @@ static unsigned int contains(mmfile_t *mf, regex_t
*regexp, kwset_t kws)
regmatch_t regmatch;
int flags = 0;
+#ifndef REG_STARTEND
assert(data[sz] == '\0');
+#else
+ regmatch.rm_so = 0;
+ regmatch.rm_eo = sz;
+ flags |= REG_STARTEND;
+#endif
while (*data && !regexec(regexp, data, 1, ®match,
flags)) {
flags |= REG_NOTBOL;
data += regmatch.rm_eo;
diff --git a/xdiff-interface.c b/xdiff-interface.c
index f34ea76..c179d43 100644
--- a/xdiff-interface.c
+++ b/xdiff-interface.c
@@ -218,7 +218,7 @@ static long ff_regexp(const char *line, long len,
struct ff_regs *regs = priv;
regmatch_t pmatch[2];
int i;
- int result = -1;
+ int result = -1, flags = 0;
/* Exclude terminating newline (and cr) from matching */
if (len > 0 && line[len-1] == '\n') {
@@ -228,11 +228,20 @@ static long ff_regexp(const char *line, long len,
len--;
}
+#ifndef REG_STARTEND
line_buffer = xstrndup(line, len); /* make NUL terminated */
+#else
+ line_buffer = (char *)line;
+ flags = REG_STARTEND;
+#endif
for (i = 0; i < regs->nr; i++) {
struct ff_reg *reg = regs->array + i;
- if (!regexec(®->re, line_buffer, 2, pmatch, 0)) {
+#ifdef REG_STARTEND
+ pmatch->rm_so = 0;
+ pmatch->rm_eo = len;
+#endif
+ if (!regexec(®->re, line_buffer, 2, pmatch, flags)) {
if (reg->negate)
goto fail;
break;
@@ -249,7 +258,9 @@ static long ff_regexp(const char *line, long len,
result--;
memcpy(buffer, line, result);
fail:
+#ifndef REG_STARTEND
free(line_buffer);
+#endif
return result;
}
^ permalink raw reply related
* Windows Git will not start external diff at all
From: Jaakko Pääkkönen @ 2016-09-06 15:57 UTC (permalink / raw)
To: git
Hi
I am using beyond compare, but it does not really matter which one
because even I create a dummy script as a external diff program, it
will not get called ever. Only internal diff is started.
Re-installing git will not remove the problem. I am using the latest git
Any hints how I can debug git difftool?
--jaakko
^ permalink raw reply
* Re: [PATCH] introduce hex2chr() for converting two hexadecimal digits to a character
From: René Scharfe @ 2016-09-06 15:48 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Git List, Junio C Hamano
In-Reply-To: <alpine.DEB.2.20.1609040947211.129229@virtualbox>
Am 04.09.2016 um 09:49 schrieb Johannes Schindelin:
> Hi René,
>
> On Sat, 3 Sep 2016, René Scharfe wrote:
>
>> Add and use a helper function that decodes the char value of two
>> hexadecimal digits. It returns a negative number on error, avoids
>> running over the end of the given string and doesn't shift negative
>> values.
>
> I like it! Maybe stress a little bit why this is a good change? Like, DRY
> up code, makes the code safer (bt avoiding shifting negative values)?
>
>> 6 files changed, 21 insertions(+), 78 deletions(-)
>
> Very, very nice!
That's the main reason: Consistency. It's intended to be a safe, easy
to use and reasonably fast replacement for those other (lengthy)
variations.
René
^ permalink raw reply
* Re: [PATCH] compat: move strdup(3) replacement to its own file
From: René Scharfe @ 2016-09-06 15:40 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Git List, Junio C Hamano
In-Reply-To: <alpine.DEB.2.20.1609040941210.129229@virtualbox>
Am 04.09.2016 um 09:46 schrieb Johannes Schindelin:
> Hi René,
>
> I imagine you Cc:ed me because the nedmalloc stuff came in via the Windows
> port, contributed by Marius (who is no longer active on the Git project
> because it works well enough for him)?
Kind of; it's also a follow-up to the recent discussion you started
about compiler warnings in that code.
> On Sat, 3 Sep 2016, René Scharfe wrote:
>
>> Move our implementation of strdup(3) out of compat/nedmalloc/ and allow
>> it to be used independently from USE_NED_ALLOCATOR. This reduces the
>> difference of our copy of nedmalloc from the original, making it easier
>> to update, and allows for easier testing and reusing of our version of
>> strdup().
>
> I would like to suggest an additional paragraph to explain why we do not
> need to #include "git-compat-util.h" in nedmalloc from now on:
>
> Please note that nedmalloc never actually uses strdup() itself,
> therefore we need not enforce gitstrdup() usage in nedmalloc.c.
Well, OK. I think the missing point is that the original nedmalloc
doesn't come with strdup() and doesn't need it. Only _users_ of
nedmalloc need it. Marius added it in nedmalloc.c, but strdup.c is a
better place for it.
René
^ permalink raw reply
* Re: [PATCH v2 02/38] rename_ref_available(): add docstring
From: Jakub Narębski @ 2016-09-06 14:25 UTC (permalink / raw)
To: Michael Haggerty, Junio C Hamano
Cc: David Turner, Ramsay Jones, Eric Sunshine, Jeff King,
Nguyễn Thái Ngọc Duy, git, David Turner
In-Reply-To: <f89d56ef0841a3cc35689436c0b9473c6773d22d.1473003902.git.mhagger@alum.mit.edu>
W dniu 04.09.2016 o 18:08, Michael Haggerty pisze:
> +/*
> + * Check whether an attempt to rename old_refname to new_refname would
> + * cause a D/F conflict with any existing reference (other than
> + * possibly old_refname). If there would be a conflict, emit an error
> + * message and return false; otherwise, return true.
> + *
> + * Note that this function is not safe against all races with other
> + * processes (though rename_ref() catches some races that might get by
> + * this check).
> + */
> +int rename_ref_available(const char *old_refname, const char *new_refname);
Just a sidenote: does Git have a naming convention for query functions
returning a boolean, for example using is_* as a prefix? That is, shouldn't
it be
int is_rename_ref_available(const char *old_refname, const char *new_refname);
I'm sorry if this is too nitpicky
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH 0/3] Fix a segfault caused by regexec() being called on mmap()ed data
From: Johannes Schindelin @ 2016-09-06 14:13 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano
In-Reply-To: <20160906065821.bxwitnk72rpn3xhl@sigill.intra.peff.net>
Hi Peff,
On Tue, 6 Sep 2016, Jeff King wrote:
> On Mon, Sep 05, 2016 at 05:44:57PM +0200, Johannes Schindelin wrote:
>
> > The problem with that: mmap()ed memory is *not* NUL-terminated, yet
> > the pickaxe code calls regexec() on it just the same.
> >
> > This problem has been reported by my colleague Chris Sidi.
> >
> > Please note that this patch series is a hot fix I applied to Git for
> > Windows (the bug does not trigger a segmentation fault for me on
> > Linux, strangely enough, but it is really a problem on Windows).
>
> This has come up before, and I think somebody mentioned that on Linux,
> you are OK unless the buffer ends right at a page boundary (i.e., the
> buffer size is a multiple of the page size). I don't know if that's true
> or not.
In my tests on Linux, even when the buffer ended right at the page
boundary, the memory after that was still legal to access, and typically
had a NUL *somewhere*.
That's happenstance, of course, and could very well result in false
positives (however unlikely that is).
> > So at least I have a workaround in place. Ideally, though, we would
> > NUL-terminate the buffers only when needed, or somehow call regexec() on
> > ptr/size parameters instead of passing a supposedly NUL-terminated
> > string to it?
>
> There's some discussion in:
>
> http://public-inbox.org/git/20121030121747.GA4231@sigill.intra.peff.net/#r
>
> and the thread below it. The quickest way to fix regexec() would be to
> have everybody use the built-in GNU regex in compat/. People seemed
> somewhat positive on that direction, but we never followed up.
I had a brief look, and it is not pretty. You would have to pay me good
money to dive in and try to implement a regexecn() based on what we have
in compat/regex/. And then people would still complain, I guess, for not
using the native regex support, where available.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH 0/3] Fix a segfault caused by regexec() being called on mmap()ed data
From: Johannes Schindelin @ 2016-09-06 14:06 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <20160906071255.ggsoj2lh2f3kubhj@sigill.intra.peff.net>
Hi Peff & Junio,
On Tue, 6 Sep 2016, Jeff King wrote:
> On Mon, Sep 05, 2016 at 12:10:11PM -0700, Junio C Hamano wrote:
>
> > * We could use <ptr,len> variant of regexp engine as you proposed,
> > which I think is a preferrable solution. Do people know of a
> > widely accepted implementation that we can throw into compat/ as
> > fallback that is compatible with GPLv2?
>
> Maybe the one already in compat/regex? ;P
Indeed. That happens to be the implementation used by Git for Windows,
anyway.
> I think re_search() the correct replacement function but it's been a
> while since I've looked into it.
The segfault I investigated happened in a call to strlen(). I see many
calls to strlen() in compat/regex/... The one that triggers the segfault
is in regexec(), compat/regex/regexec.c:241.
As to re_search(): I have not been able to reason about its callees in a
reasonable amount of time. I agree that they *should* not run over the
buffer, but I cannot easily verify it.
The bigger problem is that re_search() is defined in the __USE_GNU section
of regex.h, and I do not think it is appropriate to universally #define
said constant before #include'ing regex.h. So it would appear that major
surgery would be required if we wanted to use regular expressions on
strings that are not NUL-terminated.
So I agree that a better idea may be to simply ensure NUL-terminated
buffers when we require them, although that still might be tricky. More on
that in a reply to your comment to that end.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCHv4] diff.c: emit moved lines with a different color
From: Jakub Narębski @ 2016-09-06 14:05 UTC (permalink / raw)
To: Stefan Beller, git; +Cc: Junio C Hamano, Jacob Keller, Stefan Beller
In-Reply-To: <20160906070151.15163-1-stefanbeller@gmail.com>
W dniu 06.09.2016 o 09:01, Stefan Beller pisze:
> ---
>
> * moved new data structures into struct diff_options
> * color.moved=bool as well as --[no-]color-moved to {dis,en}able the new feature
> * color.diff.movedfrom and color.diff.movedto to control the colors
> * added a test
[...]
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 0bcb679..5daf77a 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -974,14 +974,22 @@ This does not affect linkgit:git-format-patch[1] or the
> 'git-diff-{asterisk}' plumbing commands. Can be overridden on the
> command line with the `--color[=<when>]` option.
>
> +color.moved::
> + A boolean value, whether a diff should color moved lines
> + differently. The moved lines are searched for in the diff only.
> + Duplicated lines from somewhere in the project that are not
> + part of the diff are not colored as moved.
> + Defaults to true.
[...]
> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
> index 705a873..13b6a2a 100644
> --- a/Documentation/diff-options.txt
> +++ b/Documentation/diff-options.txt
> @@ -234,6 +234,13 @@ ifdef::git-diff[]
> endif::git-diff[]
> It is the same as `--color=never`.
>
> +--[no-]color-moved::
> + Show moved blocks in a different color.
> +ifdef::git-diff[]
> + It can be changed by the `diff.ui` and `color.diff`
> + configuration settings.
> +endif::git-diff[]
If not for `color.moved`, I would have thought that instead of adding
new command line option `--color-moved` (and the fact that it is on
by default), we could simply reuse duplication of code movement
detection as a signal of stronger detection, namely "-M -M" (and also
"-C -C" to handle copy detection) that git-blame uses...
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCHv4] diff.c: emit moved lines with a different color
From: Ramsay Jones @ 2016-09-06 13:37 UTC (permalink / raw)
To: Stefan Beller, git; +Cc: jnareb, gitster, jacob.keller, Stefan Beller
In-Reply-To: <20160906070151.15163-1-stefanbeller@gmail.com>
On 06/09/16 08:01, Stefan Beller wrote:
[snip]
> This patch was motivated by e.g. reviewing 3b0c4200 ("apply: move
> libified code from builtin/apply.c to apply.{c,h}", 2016-08-08)
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>
> * moved new data structures into struct diff_options
> * color.moved=bool as well as --[no-]color-moved to {dis,en}able the new feature
> * color.diff.movedfrom and color.diff.movedto to control the colors
> * added a test
>
> Documentation/config.txt | 12 +-
> Documentation/diff-options.txt | 7 ++
> contrib/completion/git-completion.bash | 2 +
> diff.c | 211 +++++++++++++++++++++++++++++++--
> diff.h | 16 ++-
> t/t4015-diff-whitespace.sh | 44 +++++++
> 6 files changed, 282 insertions(+), 10 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 0bcb679..5daf77a 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -974,14 +974,22 @@ This does not affect linkgit:git-format-patch[1] or the
> 'git-diff-{asterisk}' plumbing commands. Can be overridden on the
> command line with the `--color[=<when>]` option.
>
> +color.moved::
> + A boolean value, whether a diff should color moved lines
> + differently. The moved lines are searched for in the diff only.
> + Duplicated lines from somewhere in the project that are not
> + part of the diff are not colored as moved.
> + Defaults to true.
> +
> color.diff.<slot>::
> Use customized color for diff colorization. `<slot>` specifies
> which part of the patch to use the specified color, and is one
> of `context` (context text - `plain` is a historical synonym),
> `meta` (metainformation), `frag`
> (hunk header), 'func' (function in hunk header), `old` (removed lines),
> - `new` (added lines), `commit` (commit headers), or `whitespace`
> - (highlighting whitespace errors).
> + `new` (added lines), `commit` (commit headers), `whitespace`
> + (highlighting whitespace errors), `movedfrom` (removed lines that
> + reappear), `movedto` (added lines that were removed elsewhere).
>
> color.decorate.<slot>::
> Use customized color for 'git log --decorate' output. `<slot>` is one
> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
> index 705a873..13b6a2a 100644
> --- a/Documentation/diff-options.txt
> +++ b/Documentation/diff-options.txt
> @@ -234,6 +234,13 @@ ifdef::git-diff[]
> endif::git-diff[]
> It is the same as `--color=never`.
>
> +--[no-]color-moved::
> + Show moved blocks in a different color.
> +ifdef::git-diff[]
> + It can be changed by the `diff.ui` and `color.diff`
> + configuration settings.
> +endif::git-diff[]
> +
> --word-diff[=<mode>]::
> Show a word diff, using the <mode> to delimit changed words.
> By default, words are delimited by whitespace; see
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 9c8f738..9827c2e 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -2115,6 +2115,8 @@ _git_config ()
> color.diff.old
> color.diff.plain
> color.diff.whitespace
> + color.diff.movedfrom
> + color.diff.movedto
> color.grep
> color.grep.context
> color.grep.filename
> diff --git a/diff.c b/diff.c
> index 534c12e..47685f3 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -18,6 +18,7 @@
> #include "ll-merge.h"
> #include "string-list.h"
> #include "argv-array.h"
> +#include "git-compat-util.h"
>
> #ifdef NO_FAST_WORKING_DIRECTORY
> #define FAST_WORKING_DIRECTORY 0
> @@ -30,6 +31,7 @@ static int diff_compaction_heuristic; /* experimental */
> static int diff_rename_limit_default = 400;
> static int diff_suppress_blank_empty;
> static int diff_use_color_default = -1;
> +static int diff_color_moved_default = -1;
> static int diff_context_default = 3;
> static const char *diff_word_regex_cfg;
> static const char *external_diff_cmd_cfg;
> @@ -52,6 +54,8 @@ static char diff_colors[][COLOR_MAXLEN] = {
> GIT_COLOR_YELLOW, /* COMMIT */
> GIT_COLOR_BG_RED, /* WHITESPACE */
> GIT_COLOR_NORMAL, /* FUNCINFO */
> + GIT_COLOR_BLUE, /* MOVED FROM */
> + GIT_COLOR_MAGENTA, /* MOVED TO */
> };
>
> static int parse_diff_color_slot(const char *var)
> @@ -72,6 +76,10 @@ static int parse_diff_color_slot(const char *var)
> return DIFF_WHITESPACE;
> if (!strcasecmp(var, "func"))
> return DIFF_FUNCINFO;
> + if (!strcasecmp(var, "movedfrom"))
> + return DIFF_FILE_MOVED_FROM;
> + if (!strcasecmp(var, "movedto"))
> + return DIFF_FILE_MOVED_TO;
> return -1;
> }
>
> @@ -180,6 +188,10 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)
> diff_use_color_default = git_config_colorbool(var, value);
> return 0;
> }
> + if (!strcmp(var, "color.moved")) {
> + diff_color_moved_default = git_config_bool(var, value);
> + return 0;
> + }
> if (!strcmp(var, "diff.context")) {
> diff_context_default = git_config_int(var, value);
> if (diff_context_default < 0)
> @@ -287,6 +299,26 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)
> return git_default_config(var, value, cb);
> }
>
> +static int diff_line_moved_entry_cmp(const struct diff_line_moved_entry *a,
> + const struct diff_line_moved_entry *b,
> + const void *unused)
> +{
> + return strcmp(a->line, b->line) &&
> + a->hash_prev_line == b->hash_prev_line;
I doubt it would make much difference, but my knee-jerk reaction to
this was to suggest swapping the order of the expression, thus:
return a->hash_prev_line == b->hash_prev_line &&
strcmp(a->line, b->line);
... but perhaps it doesn't read quite so well, and probably wouldn't affect
performance much (except in strange edge cases), so it may not be worth it.
ATB,
Ramsay Jones
^ permalink raw reply
* Re: [PATCH 0/3] Fix a segfault caused by regexec() being called on mmap()ed data
From: Johannes Schindelin @ 2016-09-06 13:21 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqwpiqp3ho.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Mon, 5 Sep 2016, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > This patch series addresses a problem where `git diff` is called using
> > `-G` or `-S --pickaxe-regex` on new-born files that are configured
> > without user diff drivers, and that hence get mmap()ed into memory.
>
> Good spotting. This has been with us almost forever; I do not think
> the original pickaxe had it, but I am sure it is broken after the
> "--pickaxe-regex" enhancement.
Agreed, regexec() is the call where it segfaults.
> I am somehow surprised that this is a problem on Windows, though.
> Wouldn't we be at least running CRLF conversions, and causing diff
> or grep machinery to work on a NUL-terminated buffer?
It is true that the CR/LF conversion hides this problem. In fact, in the
case reported to me, it turned out that the segfault happened only
recently, when the repository was switched from LF line endings to CR/LF
line endings.
That switch is unfortunately required: it saves *tons* of time because the
regular CR/LF conversion just takes too much time. It was worse before the
repository defined the .gitattributes: the auto-detection contributed to
the time spent by Git.
So yes: the CR/LF conversion hid the bug, but no, we cannot re-introduce
the CR/LF conversion into said repository.
> The convesion code would have to look at mmap'ed memory but I do not
> think it assumes NUL-termination. Perhaps people on Windows do not
> usually use straight-through and that is why this was discovered after
> many years, or something? In any case, that is a digression.
Indeed, it is.
> > Windows (the bug does not trigger a segmentation fault for me on
> > Linux, strangely enough, but it is really a problem on Windows).
>
> I think it is an issue on all platforms that lets us use mmap().
> When the size of a file is multiple of pagesize, the byte past the
> end of the file can very well fall on an unmapped address.
Correct.
> > So at least I have a workaround in place. Ideally, though, we would
> > NUL-terminate the buffers only when needed, or somehow call regexec()
> > on ptr/size parameters instead of passing a supposedly NUL-terminated
> > string to it?
>
> I see two reasonable approaches.
>
> * We may want to revisit the performance numbers to see if using
> mmap() to read from the working tree files still buys us much.
> If not, we should stop borrowing from the working tree using
> mmap(); instead just slurp in and NUL-terminate it.
I would like to warn against putting too much stock into such a test,
unless it is performed on Linux, MacOSX, Windows and various BSDs. That
would make it hard, of course, to come up with a definitive result, but we
simply should not make the mistake of over-optimizing for one platform.
We used to, of course, and look how much performance it costs e.g. on
Windows.
> * We could use <ptr,len> variant of regexp engine as you proposed,
> which I think is a preferrable solution. Do people know of a
> widely accepted implementation that we can throw into compat/ as
> fallback that is compatible with GPLv2?
As suggested by Peff, there is a compat/regex/, and I will spout my
thoughts in a reply to his mail.
Ciao,
Dscho
^ permalink raw reply
* [PATCH] gpg-interface: reflect stderr to stderr
From: Michael J Gruber @ 2016-09-06 13:13 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Junio C Hamano
In-Reply-To: <ced7502d-0095-bd90-19e3-c14d0e4d4f07@drmicha.warpmail.net>
efee955 ("gpg-interface: check gpg signature creation status",
2016-06-17) used stderr to capture gpg's status output, which is the
only reliable way for status checks. As a side effect, stderr was not
shown to the user any more.
In case of a gpg error, reflect the whole captured buffer to stderr.
Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
A full blown approach would use --status-fd=4 or such rather than hijacking stderr.
This would require an extension of pipe_command() etc. to handle yet another fd.
gpg-interface.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/gpg-interface.c b/gpg-interface.c
index 8672eda..cf35bca 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -173,9 +173,12 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig
sigchain_pop(SIGPIPE);
ret |= !strstr(gpg_status.buf, "\n[GNUPG:] SIG_CREATED ");
- strbuf_release(&gpg_status);
- if (ret)
+ if (ret) {
+ fputs(gpg_status.buf, stderr);
+ strbuf_release(&gpg_status);
return error(_("gpg failed to sign the data"));
+ }
+ strbuf_release(&gpg_status);
/* Strip CR from the line endings, in case we are on Windows. */
for (i = j = bottom; i < signature->len; i++)
--
2.10.0.rc2.333.g8ef2d05
^ permalink raw reply related
* Re: [WIP PATCH v2] diff.c: emit moved lines with a different color
From: Junio C Hamano @ 2016-09-06 12:44 UTC (permalink / raw)
To: Stefan Beller
Cc: Jacob Keller, Stefan Beller, Git mailing list,
Jakub Narębski
In-Reply-To: <CAGZ79kYPtvJ7iF8G5ihUCa5NwvDFa_Rzh+N7ar9QA2x74p2Z9g@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> This new coloring is linear to the size of the patch, i.e. O(number of
> added/removed lines) in memory and for computational efforts I'd
> think it is O(n log n) as inserting into the hashmap is an amortized
> log n.
In addition to that O(n log n) overhead for book-keeping, you are
doing at least twice the amount of work compared to the original, if
you are still running the same xdiff twice to implement the two pass
approach. That is why I thought this "twice as slow, at least"
needs to be off by default at least in the beginning.
Also there is the additional memory pressure coming from the fact
that the first pass will need to keep all the removed and added
lines in-core for all filepairs. If you keep the entire diff output
in-core from the first pass, I do not think it would be that much
more memory overhead compared to what you are already doing, so the
cost of running the same xdiff twice is relatively easy to reduce, I
would imagine? Instead of running the second xdi_diff_outf(), you
can drive your callback function out of what has been queued in the
first pass yourself. But that is the next step "optimization" once
we got the basics right.
By the way, not running xdiff twice would also remove another worry
I have about correctness, in that the approach depends on xdiff
machinery to produce byte-for-byte identical result given the same
pair of input. The output may currently be reproducible, but that
is an unnecessary and an expensive thing to rely on.
You may be able to save tons of memory if you do not store the line
contents duplicated. The first pass callback can tell the line
numbers in preimage and postimage [*1*], so your record for a
removed line could be a pair <struct diff_filespec *, long lineno>
with whatever hash value you need to throw it into the hash bucket.
I know we use a hash function and a line comparison function that
are aware of -b/-w comparison in xdiff machinery, but I didn't see
you use them in your hashtable. Can you match moved lines when
operating under various whitespace-change-ignoring modes?
Thanks.
[Footnote]
*1* You can learn all sort of things from emit_callback structure;
if you need to pass more data from the caller of xdi_diff_outf()
to the callback, you can even add new fields to it.
^ permalink raw reply
* Re: [PATCH] Unbreak interactive GPG prompt upon signing
From: Michael J Gruber @ 2016-09-06 12:32 UTC (permalink / raw)
To: Johannes Schindelin, git; +Cc: Junio C Hamano
In-Reply-To: <0b8196564ac9f1db7c59b42d7e0973ba1399ec52.1473148900.git.johannes.schindelin@gmx.de>
Johannes Schindelin venit, vidit, dixit 06.09.2016 10:01:
> With the recent update in efee955 (gpg-interface: check gpg signature
> creation status, 2016-06-17), we ask GPG to send all status updates to
> stderr, and then catch the stderr in an strbuf.
>
> But GPG might fail, and send error messages to stderr. And we simply
> do not show them to the user.
>
> Even worse: this swallows any interactive prompt for a passphrase. And
> detaches stderr from the tty so that the passphrase cannot be read.
>
> So while the first problem could be fixed (by printing the captured
> stderr upon error), the second problem cannot be easily fixed, and
> presents a major regression.
My Git has that commit and does ask me for the passphrase on the tty.
Also, I do get error messages:
git tag -u pebcak -s testt -m m
error: gpg failed to sign the data
error: unable to sign the tag
which we could (maybe should) amend by gpg's stderr.
> So let's just revert commit efee9553a4f97b2ecd8f49be19606dd4cf7d9c28.
That "just" reintroduces the problem that the orignal patch solves.
The passphrase/tty issue must be Windows specific - or the non-issue
Linux-specific, if you prefer.
> This fixes https://github.com/git-for-windows/git/issues/871
>
> Cc: Michael J Gruber <git@drmicha.warpmail.net>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> Published-As: https://github.com/dscho/git/releases/tag/fix-gpg-v1
> Fetch-It-Via: git fetch https://github.com/dscho/git fix-gpg-v1
>
> gpg-interface.c | 8 ++------
> t/t7004-tag.sh | 9 +--------
> 2 files changed, 3 insertions(+), 14 deletions(-)
>
> diff --git a/gpg-interface.c b/gpg-interface.c
> index 8672eda..3f3a3f7 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -153,11 +153,9 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig
> struct child_process gpg = CHILD_PROCESS_INIT;
> int ret;
> size_t i, j, bottom;
> - struct strbuf gpg_status = STRBUF_INIT;
>
> argv_array_pushl(&gpg.args,
> gpg_program,
> - "--status-fd=2",
> "-bsau", signing_key,
> NULL);
>
> @@ -169,12 +167,10 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig
> */
> sigchain_push(SIGPIPE, SIG_IGN);
> ret = pipe_command(&gpg, buffer->buf, buffer->len,
> - signature, 1024, &gpg_status, 0);
> + signature, 1024, NULL, 0);
> sigchain_pop(SIGPIPE);
>
> - ret |= !strstr(gpg_status.buf, "\n[GNUPG:] SIG_CREATED ");
> - strbuf_release(&gpg_status);
> - if (ret)
> + if (ret || signature->len == bottom)
> return error(_("gpg failed to sign the data"));
>
> /* Strip CR from the line endings, in case we are on Windows. */
> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
> index 8b0f71a..f9b7d79 100755
> --- a/t/t7004-tag.sh
> +++ b/t/t7004-tag.sh
> @@ -1202,17 +1202,10 @@ test_expect_success GPG,RFC1991 \
> # try to sign with bad user.signingkey
> git config user.signingkey BobTheMouse
> test_expect_success GPG \
> - 'git tag -s fails if gpg is misconfigured (bad key)' \
> + 'git tag -s fails if gpg is misconfigured' \
> 'test_must_fail git tag -s -m tail tag-gpg-failure'
> git config --unset user.signingkey
>
> -# try to produce invalid signature
> -test_expect_success GPG \
> - 'git tag -s fails if gpg is misconfigured (bad signature format)' \
> - 'test_config gpg.program echo &&
> - test_must_fail git tag -s -m tail tag-gpg-failure'
> -
> -
> # try to verify without gpg:
>
> rm -rf gpghome
>
^ permalink raw reply
* Re: Context Menu is missing
From: Johannes Schindelin @ 2016-09-06 11:59 UTC (permalink / raw)
To: Idan Shimoni; +Cc: stefan.naewe, git
In-Reply-To: <CAFfNYUmH4-Em++kJzdd-V8sDU-OrWp5Qy8kqMtfD7UmaQABg5Q@mail.gmail.com>
Hi Idan,
On Tue, 6 Sep 2016, Idan Shimoni wrote:
> ???
Please understand that you continue to annoy Stefan, who is only trying to
help you, by adding your replies *above* the quoted email. This is called
top-posting and considered very, very rude on this mailing list.
Also, your replies are very succinct, to the point of being incomplete. So
Stefan required a couple of back-and-forths until he finally found out
what your problem is. You could have volunteered more information (as you
did in the issue https://github.com/git-for-windows/git/issues/875 that
you opened in the correct place: you mentioned the Windows version as well
as the Git version, although the answer to which options were chosen
("All") was most likely incorrect, because you have to decide e.g. whether
you want CR/LF line endings, LF line endings, or the same line endings as
are stored in the Git repositories, you cannot choose all of them at the
same time).
Unfortunately, the information given in that ticket is still too succinct:
it does not mention from which version you upgraded. You force us, once
again, to guess. My guess is: Git for Windows 1.9.5.
So now to your real question, which is: why is Git Cheetah no longer
bundled with Git for Windows 2.x?
The answer to that is easy: it was too cumbersome to maintain, there have
been almost no contributions, and most users chose to opt for TortoiseGit
or GitExternals (both of which are much, much larger than Git Cheetah, but
at the same time they happen to be maintained).
So there you go.
Next time you report a bug, please spend a little more time on the bug
report. It would certainly be appreciated by the people you ask to help
you.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH v1 2/2] read-cache: make sure file handles are not inherited by child processes
From: Johannes Schindelin @ 2016-09-06 11:41 UTC (permalink / raw)
To: Lars Schneider; +Cc: git, gitster, tboegi, e
In-Reply-To: <20160905211111.72956-3-larsxschneider@gmail.com>
Hi Lars,
On Mon, 5 Sep 2016, larsxschneider@gmail.com wrote:
> [... commit message ...]
Makes sense.
> diff --git a/read-cache.c b/read-cache.c
> index 491e52d..02f74d3 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -156,7 +156,7 @@ void fill_stat_cache_info(struct cache_entry *ce, struct stat *st)
> static int ce_compare_data(const struct cache_entry *ce, struct stat *st)
> {
> int match = -1;
> - int fd = open(ce->name, O_RDONLY);
> + int fd = open(ce->name, O_RDONLY | O_CLOEXEC);
Eric's comment on 1/2 applies here, too, of course: should this cause any
problems on non-Windows platforms, we always have that FD_CLOEXEC thing
that we could probably use to fix it.
But let's cross that bridge when (or better: if) we get there.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH v1 1/2] sha1_file: open window into packfiles with CLOEXEC
From: Johannes Schindelin @ 2016-09-06 11:38 UTC (permalink / raw)
To: Eric Wong; +Cc: larsxschneider, git, gitster, tboegi
In-Reply-To: <20160905222715.GA30031@starla>
Hi Eric & Lars,
On Mon, 5 Sep 2016, Eric Wong wrote:
> larsxschneider@gmail.com wrote:
> > All processes that the Git main process spawns inherit the open file
> > descriptors of the main process. These leaked file descriptors can
> > cause problems.
>
>
> > -int git_open_noatime(const char *name)
> > +int git_open_noatime_cloexec(const char *name)
> > {
> > - static int sha1_file_open_flag = O_NOATIME;
> > + static int sha1_file_open_flag = O_NOATIME | O_CLOEXEC;
> >
> > for (;;) {
> > int fd;
>
> If there's real problems being caused by lack of cloexec
> today, I think the F_SETFD fallback I proposed in
> https://public-inbox.org/git/20160818173555.GA29253@starla/
> will be necessary.
Yes, it is good to have that patch available to go if we need it. I do not
think that we will need it, though, as the biggest problems that are
solved through the CLOEXEC flag are ones caused on Windows, when files
cannot be deleted or renamed because there are still (uselessly) open
handles referencing them.
> I question the need for the "_cloexec" suffixing in the
> function name since the old function is going away entirely.
Me, too. While it is correct, it makes things harder to read, so it may
even cause more harm than it does good.
> I prefer all FD-creating functions set cloexec by default
> for FD > 2 to avoid inadvertantly leaking FDs. So we
> ought to use pipe2, accept4, socket(..., SOCK_CLOEXEC), etc...
> and fallback to the racy+slower F_SETFD when not available.
In the original Pull Request where the change was contributed to Git for
Windows, this was tested (actually, the code did not see whether fd > 2,
but simply assumed that all newly opened file descriptors would be > 2
anyway), and it failed:
https://github.com/git-for-windows/git/pull/755#issuecomment-220247972
So it appears that we would have to exclude at least the code path to `git
upload-pack` from that magic.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH v1 1/2] sha1_file: open window into packfiles with CLOEXEC
From: Jakub Narębski @ 2016-09-06 9:36 UTC (permalink / raw)
To: Eric Wong, larsxschneider; +Cc: git, gitster, tboegi, Johannes.Schindelin
In-Reply-To: <20160905222715.GA30031@starla>
W dniu 06.09.2016 o 00:27, Eric Wong pisze:
> larsxschneider@gmail.com wrote:
>> -int git_open_noatime(const char *name)
>> +int git_open_noatime_cloexec(const char *name)
[...]
>
> I question the need for the "_cloexec" suffixing in the
> function name since the old function is going away entirely.
On the other hand the new name is descriptive...
>
> I prefer all FD-creating functions set cloexec by default
> for FD > 2 to avoid inadvertantly leaking FDs. So we
> ought to use pipe2, accept4, socket(..., SOCK_CLOEXEC), etc...
> and fallback to the racy+slower F_SETFD when not available.
>
>
> Fwiw, Perl has been setting cloexec on FDs above $^F
> (2, $SYSTEM_FD_MAX) for decades, and Ruby started
> doing it a few years ago, too.
^ permalink raw reply
* Re: Context Menu is missing
From: Idan Shimoni @ 2016-09-06 8:23 UTC (permalink / raw)
To: stefan.naewe; +Cc: git
In-Reply-To: <50fc49a6-68a2-040b-beb1-feb162c458d2@atlas-elektronik.com>
???
On Tue, Sep 6, 2016 at 11:20 AM, <stefan.naewe@atlas-elektronik.com> wrote:
> (and s/he did it again....hitting "reply all" seems really complicated)
>
> Am 06.09.2016 um 10:07 schrieb Idan Shimoni:
>> it is part of git cheetah plugin not tortoise.
>> But it was part of the git installer for windows, I did not installed
>> anything else before.
>>
>> On Tue, Sep 6, 2016 at 10:54 AM, <stefan.naewe@atlas-elektronik.com> wrote:
>>> (Please, please, please, use "reply all" in your mail reader i.e. make sure you don't
>>> remove 'git@vger.kernel.org' from the "To:" or "CC:" field. Thank you!)
>>>
>>> Am 06.09.2016 um 09:47 schrieb Idan Shimoni:
>>>> On Tue, Sep 6, 2016 at 10:33 AM, <stefan.naewe@atlas-elektronik.com> wrote:
>>>>> (Please don't top post and do "reply all")
>>>>>
>>>> I tried but you are receiving only plain text emails.
>>>> anyway...
>>>
>>> ??? ECANNOTUNDERSTAND
>>>
>>> Read about top-posting here: https://en.wikipedia.org/wiki/Posting_style#Top-posting
>>>
>>>> I reinstalled windows on my computer and then installed Git version
>>>> 2.9.3 for windows.
>>>> And the context menu were missing.
>>>
>>> In the explorer, I guess ?
>>>
>>>> I am talking about the one that you had:
>>>> - Git History
>>>> - Git Branch
>>>> - branch_1
>>>> - branch_2
>>>> ....
>>>
>>> Git for windows *doesn't* install that.
>>>
>>>> Git GUI and Git Bash are still there...
>>>
>>> Git for windows *does* install that.
>>>
>>> Maybe you had TortoiseGit installed before (just a wild guess, though)
>>>
>>> Stefan
>>> --
>>> ----------------------------------------------------------------
>>> /dev/random says: Gambling: The sure way of getting nothing for something.
>>> python -c "print '73746566616e2e6e616577654061746c61732d656c656b74726f6e696b2e636f6d'.decode('hex')"
>>> GPG Key fingerprint = 2DF5 E01B 09C3 7501 BCA9 9666 829B 49C5 9221 27AF
>>
>>
>>
>
>
> --
> ----------------------------------------------------------------
> /dev/random says: Give instruction to a wise man and he will be yet wiser.
> python -c "print '73746566616e2e6e616577654061746c61732d656c656b74726f6e696b2e636f6d'.decode('hex')"
> GPG Key fingerprint = 2DF5 E01B 09C3 7501 BCA9 9666 829B 49C5 9221 27AF
--
-Best
Idan
^ permalink raw reply
* Re: Context Menu is missing
From: stefan.naewe @ 2016-09-06 8:20 UTC (permalink / raw)
To: idan, git
In-Reply-To: <CAFfNYUkUpaCo8uYv=fofPRyCgg7db4M2fh7BJRc4WCHZdbxvsw@mail.gmail.com>
(and s/he did it again....hitting "reply all" seems really complicated)
Am 06.09.2016 um 10:07 schrieb Idan Shimoni:
> it is part of git cheetah plugin not tortoise.
> But it was part of the git installer for windows, I did not installed
> anything else before.
>
> On Tue, Sep 6, 2016 at 10:54 AM, <stefan.naewe@atlas-elektronik.com> wrote:
>> (Please, please, please, use "reply all" in your mail reader i.e. make sure you don't
>> remove 'git@vger.kernel.org' from the "To:" or "CC:" field. Thank you!)
>>
>> Am 06.09.2016 um 09:47 schrieb Idan Shimoni:
>>> On Tue, Sep 6, 2016 at 10:33 AM, <stefan.naewe@atlas-elektronik.com> wrote:
>>>> (Please don't top post and do "reply all")
>>>>
>>> I tried but you are receiving only plain text emails.
>>> anyway...
>>
>> ??? ECANNOTUNDERSTAND
>>
>> Read about top-posting here: https://en.wikipedia.org/wiki/Posting_style#Top-posting
>>
>>> I reinstalled windows on my computer and then installed Git version
>>> 2.9.3 for windows.
>>> And the context menu were missing.
>>
>> In the explorer, I guess ?
>>
>>> I am talking about the one that you had:
>>> - Git History
>>> - Git Branch
>>> - branch_1
>>> - branch_2
>>> ....
>>
>> Git for windows *doesn't* install that.
>>
>>> Git GUI and Git Bash are still there...
>>
>> Git for windows *does* install that.
>>
>> Maybe you had TortoiseGit installed before (just a wild guess, though)
>>
>> Stefan
>> --
>> ----------------------------------------------------------------
>> /dev/random says: Gambling: The sure way of getting nothing for something.
>> python -c "print '73746566616e2e6e616577654061746c61732d656c656b74726f6e696b2e636f6d'.decode('hex')"
>> GPG Key fingerprint = 2DF5 E01B 09C3 7501 BCA9 9666 829B 49C5 9221 27AF
>
>
>
--
----------------------------------------------------------------
/dev/random says: Give instruction to a wise man and he will be yet wiser.
python -c "print '73746566616e2e6e616577654061746c61732d656c656b74726f6e696b2e636f6d'.decode('hex')"
GPG Key fingerprint = 2DF5 E01B 09C3 7501 BCA9 9666 829B 49C5 9221 27AF
^ permalink raw reply
* [PATCH] Unbreak interactive GPG prompt upon signing
From: Johannes Schindelin @ 2016-09-06 8:01 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Michael J Gruber
With the recent update in efee955 (gpg-interface: check gpg signature
creation status, 2016-06-17), we ask GPG to send all status updates to
stderr, and then catch the stderr in an strbuf.
But GPG might fail, and send error messages to stderr. And we simply
do not show them to the user.
Even worse: this swallows any interactive prompt for a passphrase. And
detaches stderr from the tty so that the passphrase cannot be read.
So while the first problem could be fixed (by printing the captured
stderr upon error), the second problem cannot be easily fixed, and
presents a major regression.
So let's just revert commit efee9553a4f97b2ecd8f49be19606dd4cf7d9c28.
This fixes https://github.com/git-for-windows/git/issues/871
Cc: Michael J Gruber <git@drmicha.warpmail.net>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Published-As: https://github.com/dscho/git/releases/tag/fix-gpg-v1
Fetch-It-Via: git fetch https://github.com/dscho/git fix-gpg-v1
gpg-interface.c | 8 ++------
t/t7004-tag.sh | 9 +--------
2 files changed, 3 insertions(+), 14 deletions(-)
diff --git a/gpg-interface.c b/gpg-interface.c
index 8672eda..3f3a3f7 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -153,11 +153,9 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig
struct child_process gpg = CHILD_PROCESS_INIT;
int ret;
size_t i, j, bottom;
- struct strbuf gpg_status = STRBUF_INIT;
argv_array_pushl(&gpg.args,
gpg_program,
- "--status-fd=2",
"-bsau", signing_key,
NULL);
@@ -169,12 +167,10 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig
*/
sigchain_push(SIGPIPE, SIG_IGN);
ret = pipe_command(&gpg, buffer->buf, buffer->len,
- signature, 1024, &gpg_status, 0);
+ signature, 1024, NULL, 0);
sigchain_pop(SIGPIPE);
- ret |= !strstr(gpg_status.buf, "\n[GNUPG:] SIG_CREATED ");
- strbuf_release(&gpg_status);
- if (ret)
+ if (ret || signature->len == bottom)
return error(_("gpg failed to sign the data"));
/* Strip CR from the line endings, in case we are on Windows. */
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 8b0f71a..f9b7d79 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1202,17 +1202,10 @@ test_expect_success GPG,RFC1991 \
# try to sign with bad user.signingkey
git config user.signingkey BobTheMouse
test_expect_success GPG \
- 'git tag -s fails if gpg is misconfigured (bad key)' \
+ 'git tag -s fails if gpg is misconfigured' \
'test_must_fail git tag -s -m tail tag-gpg-failure'
git config --unset user.signingkey
-# try to produce invalid signature
-test_expect_success GPG \
- 'git tag -s fails if gpg is misconfigured (bad signature format)' \
- 'test_config gpg.program echo &&
- test_must_fail git tag -s -m tail tag-gpg-failure'
-
-
# try to verify without gpg:
rm -rf gpghome
--
2.10.0.windows.1.6.gc4f481a
base-commit: 6ebdac1bab966b720d776aa43ca188fe378b1f4b
^ 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