[PATCHv4] diff.c: emit moved lines with a different color
When we color the diff, we'll mark moved lines with a different color.
This is achieved by doing a two passes over the diff. The first pass
will inspect each line of the diff and store the removed lines and the
added lines in its own hash map.
The second pass will check for each added line if that is found in the
set of removed lines. If so it will color the added line differently as
with the new `moved-new` color mode. For each removed line we check the
set of added lines and if found emit that with the new color `moved-old`.
When detecting the moved lines, we cannot just rely on a line being equal,
but we need to take the context into account to detect when the moves were
reordered as we do not want to color moved but per-mutated lines.
To do that we use the hash of the preceding line.
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
---
* 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[=]` 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.::
Use customized color for diff colorization. `` 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.::
Use customized color for 'git log --decorate' output. `` 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[=]::
Show a word diff, using the 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_CO
Re: [PATCH 2/3] diff_populate_filespec: NUL-terminate buffers
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. 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.
> 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.
Can it be triggered with -G? 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).
-Peff
[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).
Re: [PATCH 3/3] diff_grep: add assertions verifying that the buffers are NUL-terminated
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? 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). -Peff
Context Menu is missing
Hi, The last install removed the old good context menu I used to work with. Is that on purpose or is it a bug? is there any way to get it back? -- -Best Idan
Re: [PATCH 0/3] Fix a segfault caused by regexec() being called on mmap()ed data
On Mon, Sep 05, 2016 at 12:10:11PM -0700, Junio C Hamano wrote: > * We could use 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 I think re_search() the correct replacement function but it's been a while since I've looked into it. -Peff
Re: 2.10.0: multiple versionsort.prereleasesuffix buggy?
On 06.09.2016 04:07, SZEDER Gábor wrote: [versionsort] prereleasesuffix = beta prereleasesuffix = -beta prereleasesuffix = RC prereleasesuffix = -RC Best, Gábor Yes, yes you are the best. Workaround works, tyvm. I was heading in that direction, too, but never thought to remove leading dash on the alternates - instead I tried "-b", "-R" and similar just to see what happens.
Re: [PATCH] t9903: fix broken && chain
On Mon, Sep 05, 2016 at 09:00:47PM +0200, Johannes Sixt wrote: > We might wonder why our && chain check does not catch this case: > The && chain check uses a strange exit code with the expectation that > the second or later part of a broken && chain would not exit with this > particular code. > > This expectation does not work in this case because __git_ps1, being > the first command in the second part of the broken && chain, records > the current exit code, does its work, and finally returns to the caller > with the recorded exit code. This fools our && chain check. Wow. Good find. Patch itself is obviously correct. -Peff
Re: Context Menu is missing
Am 06.09.2016 um 09:12 schrieb Idan Shimoni:
> Hi,
>
> The last install removed the old good context menu I used to work with.
>
> Is that on purpose or is it a bug? is there any way to get it back?
>
You're working on a Cray system using git 1.3.2, right ?
SCNR
--
/dev/random says: I'd explain it to you, but your brain would explode.
python -c "print
'73746566616e2e6e616577654061746c61732d656c656b74726f6e696b2e636f6d'.decode('hex')"
GPG Key fingerprint = 2DF5 E01B 09C3 7501 BCA9 9666 829B 49C5 9221 27AF
Re: [PATCH] t6026-merge-attr: wait for process to release trash directory
Hi Hannes, On Mon, 5 Sep 2016, Johannes Sixt wrote: > The process spawned in the hook uses the test's trash directory as CWD. > As long as it is alive, the directory cannot be removed on Windows. > Although the test succeeds, the 'test_done' that follows produces an > error message and leaves the trash directory around. Insert a delay to > give the hook time to go away. Maybe we should write a pid file in the sleep command instead, and kill it in the end? Something like this, maybe? -- snipsnap -- diff --git a/t/t6026-merge-attr.sh b/t/t6026-merge-attr.sh index dd8f88d..2e2beb5 100755 --- a/t/t6026-merge-attr.sh +++ b/t/t6026-merge-attr.sh @@ -184,8 +184,10 @@ test_expect_success 'up-to-date merge without common ancestor' ' test_expect_success 'custom merge does not lock index' ' git reset --hard anchor && write_script sleep-one-second.sh <<-\EOF && - sleep 1 & + sleep 10 & + echo $! >sleep.pid EOF + test_when_finished "kill -9 \$(cat sleep.pid)" && test_write_lines >.gitattributes \ "* merge=ours" "text merge=sleep-one-second" &&
Re: [PATCH] t6026-merge-attr: wait for process to release trash directory
On Mon, Sep 05, 2016 at 09:03:48PM +0200, Johannes Sixt wrote: > The process spawned in the hook uses the test's trash directory as CWD. > As long as it is alive, the directory cannot be removed on Windows. > Although the test succeeds, the 'test_done' that follows produces an > error message and leaves the trash directory around. Insert a delay to > give the hook time to go away. Ugh. I'd love it if we could avoid inserting a sleep, which wastes time in the optimistic case and is insufficient in the pessimistic one. The fact that the hook is already using sleep is even nastier, as it that's a potential race on a loaded system. Can we do some signaling with fifos to tell the hook when it is safe to exit? Then we would just need to `wait` for its parent process. -Peff
Re: Context Menu is missing
(Please don't top post and do "reply all")
Am 06.09.2016 um 09:28 schrieb Idan Shimoni:
>
> On Tue, Sep 6, 2016 at 10:23 AM, wrote:
>> Am 06.09.2016 um 09:12 schrieb Idan Shimoni:
>>> Hi,
>>>
>>> The last install removed the old good context menu I used to work with.
>>>
>>> Is that on purpose or is it a bug? is there any way to get it back?
>>>
>>
>> You're working on a Cray system using git 1.3.2, right ?
> No,
> Windows 7 64Bit
> Version: 2.9.3.windows.2
Really? That's good to know.
Would it be OK to ask you to tell us what exactly you did and which
context menu is gone, or shall I guess again?
--
/dev/random says: A few cans short of a six pack, Six short.
python -c "print
'73746566616e2e6e616577654061746c61732d656c656b74726f6e696b2e636f6d'.decode('hex')"
GPG Key fingerprint = 2DF5 E01B 09C3 7501 BCA9 9666 829B 49C5 9221 27AF
Re: Context Menu is missing
(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, 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
[PATCH] Unbreak interactive GPG prompt upon signing
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
Signed-off-by: Johannes Schindelin
---
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
Re: Context Menu is missing
(and s/he did it againhitting "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, 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, 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
Re: Context Menu is missing
???
On Tue, Sep 6, 2016 at 11:20 AM, wrote:
> (and s/he did it againhitting "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, 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,
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
re
Did you get my message?
Re: [PATCH v1 1/2] sha1_file: open window into packfiles with CLOEXEC
W dniu 06.09.2016 o 00:27, Eric Wong pisze: > larsxschnei...@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.
Re: [PATCH v1 1/2] sha1_file: open window into packfiles with CLOEXEC
Hi Eric & Lars,
On Mon, 5 Sep 2016, Eric Wong wrote:
> larsxschnei...@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
Re: [PATCH v1 2/2] read-cache: make sure file handles are not inherited by child processes
Hi Lars,
On Mon, 5 Sep 2016, larsxschnei...@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
Re: Context Menu is missing
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
Re: [PATCH] Unbreak interactive GPG prompt upon signing
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
> Signed-off-by: Johannes Schindelin
> ---
> 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
>
Re: [WIP PATCH v2] diff.c: emit moved lines with a different color
Stefan Beller 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 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.
[PATCH] gpg-interface: reflect stderr to stderr
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
---
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
Re: [PATCH 0/3] Fix a segfault caused by regexec() being called on mmap()ed data
Hi Junio, On Mon, 5 Sep 2016, Junio C Hamano wrote: > Johannes Schindelin 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 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
Re: [PATCHv4] diff.c: emit moved lines with a different color
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
> ---
>
> * 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[=]` 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.::
> Use customized color for diff colorization. `` 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.::
> Use customized color for 'git log --decorate' output. `` 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[=]::
> Show a word diff, using the 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
Re: [PATCHv4] diff.c: emit moved lines with a different color
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[=]` 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
Re: [PATCH 0/3] Fix a segfault caused by regexec() being called on mmap()ed data
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 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
Re: [PATCH 0/3] Fix a segfault caused by regexec() being called on mmap()ed data
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.ga4...@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
Re: [PATCH v2 02/38] rename_ref_available(): add docstring
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
h
Sent from my iPhone
Re: [PATCH] compat: move strdup(3) replacement to its own file
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é
Re: [PATCH] introduce hex2chr() for converting two hexadecimal digits to a character
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é
Windows Git will not start external diff at all
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
Re: [PATCH 2/3] diff_populate_filespec: NUL-terminate buffers
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_STARTEN
[PATCH v2 1/6] git-gui: consistently use the same word for "remote" in Japanese
Signed-off-by: Satoshi Yasushima --- 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
Re: [PATCH 3/3] diff_grep: add assertions verifying that the buffers are NUL-terminated
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
[PATCH v2 4/6] git-gui: Add Japanese language code
Signed-off-by: Satoshi Yasushima --- 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: しらいし ななこ \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
[PATCH v2 6/6] git-gui: Update Japanese information
Signed-off-by: Satoshi Yasushima --- po/ja.po | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/po/ja.po b/po/ja.po index deaf8e3..208651c 100644 --- a/po/ja.po +++ b/po/ja.po @@ -1,15 +1,17 @@ # Translation of git-gui to Japanese # Copyright (C) 2007 Shawn Pearce # This file is distributed under the same license as the git-gui package. +# # しらいし ななこ , 2007. +# Satoshi Yasushima , 2016. # msgid "" msgstr "" "Project-Id-Version: git-gui\n" "Report-Msgid-Bugs-To: \n" "POT-Creation-Date: 2016-05-27 17:52+0900\n" -"PO-Revision-Date: 2010-02-02 19:03+0900\n" -"Last-Translator: しらいし ななこ \n" +"PO-Revision-Date: 2016-06-22 12:50+0900\n" +"Last-Translator: Satoshi Yasushima \n" "Language-Team: Japanese\n" "Language: ja\n" "MIME-Version: 1.0\n" -- 2.8.2.windows.1
[PATCH v2 2/6] git-gui: consistently use the same word for "blame" in Japanese
Signed-off-by: Satoshi Yasushima --- 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
[PATCH v2 5/6] git-gui: Update Japanese translation
Signed-off-by: Satoshi Yasushima --- 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 からの事後復旧も困難となるためです。" +"
Re: Your email
Hi Idan, please only write public mails when discussing Open Source. On Tue, 6 Sep 2016, Idan Shimoni wrote: > "This" top-posting is auto generated when hitting on the magic button > called "Reply", and it is the generic way that the industry works. How > do you expect me to know that this is considered "very very rude on this > emailing list" if it is not mention in any place on your page. The top-posting is actually not auto-generated at all. It is your decision to write on top of the quoted text, or not. > When I sent my email in the first time I did not sent it to Stefan, I > found this email (git@vger.kernel.org) at one of the pages as a > "support" for Git for Windows. You probably also saw the note that this is an Open Source project, where that support is handled by volunteers. You did not pay anything, after all. > When I sent my email I did not think that the support is also for > different operating systems, as I found it on a "Git for Windows" > page, and as for the version from which I upgraded or from what I > upgraded to it is not really matter as you now understand now because > Git for Windows removed the Git-Cheetah. > > On the other end in my terms and my colleagues here, Stefan last email > and yours are considered very very rude, if you WANT to help people > then help them, do not attack them as you did. You know, replies like these I could do without. I do try to help. I even explain to you why your replies are met with such hostility, and how you could avoid that in the future. And what do I get in return? > And lastly as for the ticket at > https://github.com/git-for-windows/git/issues/875 I was not sure if it > is the repository for this, as later on I understood that it is > related to a different product. > > So, there you go. > > I am very very very sorry for disturbing and annoying both of you and > will not address you or your colleagues in the future any more. > > > * Sure not use any of your products as well That is your choice, of course. You will have to live with the choices Stefan and I make on our end, too. Ciao, Johannes
Re: [PATCH] gpg-interface: reflect stderr to stderr
Hi Michael,
On Tue, 6 Sep 2016, Michael J Gruber wrote:
> 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
> ---
> 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.
As I indicated in my patch, this is not enough on Windows. In fact, my
first version of a patch tried to do exactly what you presented here, and
all it did was make the error message a bit more verbose:
-- snip --
error: gpg failed to sign the data:
[GNUPG:] USERID_HINT Johannes Schindelin
[GNUPG:] NEED_PASSPHRASE 1 0
gpg: cannot open tty `no tty': No such file or directory
error: unable to sign the tag
-- snap --
This is not a fix for the issue reported on the Git for Windows, but only
half a fix.
Ciao,
Dscho
Re: Windows Git will not start external diff at all
Hi Jaakko, On Tue, 6 Sep 2016, Jaakko Pääkkönen wrote: > 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. Any chance you can come up with an MCVE [*1*] using a dummy script? > > Re-installing git will not remove the problem. I am using the latest git > > Any hints how I can debug git difftool? Sure. First hint: set GIT_TRACE=1. Next hint: difftool is a shell script, so you can modify it, say, by inserting a `set -x` which will trace the commands that are called. Ciao, Johannes Footnote *1*: http://stackoverflow.com/help/mcve
Re: [PATCH] gpg-interface: reflect stderr to stderr
Hi Michael, On Tue, 6 Sep 2016, Johannes Schindelin wrote: > On Tue, 6 Sep 2016, Michael J Gruber wrote: > > > 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. For the record: I do not know whether that would work, either. So unless we are fairly certain that it *would* work, I'd rather not spend the time. Your original issue seemed to be that the gpg command could succeed, but still no signature be seen. There *must* be a way to test whether the called program added a signature, simply by testing whether *any* characters were written. And if characters were written that were not actually a GPG signature, maybe the enterprisey user who configured the gpg command to be her magic script actually meant something else than a GPG signature to be added? Ciao, Dscho
Re: [PATCH] gpg-interface: reflect stderr to stderr
Hi Michael,
okay, final mail on this issue today:
On Tue, 6 Sep 2016, Johannes Schindelin wrote:
> Your original issue seemed to be that the gpg command could succeed, but
> still no signature be seen. There *must* be a way to test whether the
> called program added a signature, simply by testing whether *any*
> characters were written.
>
> And if characters were written that were not actually a GPG signature,
> maybe the enterprisey user who configured the gpg command to be her magic
> script actually meant something else than a GPG signature to be added?
I actually just saw that this is *precisely* what the code does already:
if (ret || signature->len == bottom)
return error(_("gpg failed to sign the data"));
Why is this not good enough?
Ciao,
Dscho
Re: [PATCH] Unbreak interactive GPG prompt upon signing
Hi Michael, On Tue, 6 Sep 2016, Michael J Gruber wrote: > 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 That is not GPG's error message. It just leaves users puzzled, is what it does. > which we could (maybe should) amend by gpg's stderr. Right. But then we still do not solve the problem. The problem being that some platforms cannot use getpass(prompt): it simply does not exist. On Windows, we do not even have a /dev/tty (technically, GPG, being an MSYS2 program, knows about /dev/tty, but we spawn it from a non-MSYS2 program, so there is a disconnect). > > So let's just revert commit efee9553a4f97b2ecd8f49be19606dd4cf7d9c28. > > That "just" reintroduces the problem that the orignal patch solves. Right. Which is: when some user misconfigures gpg, causing Git to run something different that simply succeeds, there is no signature. This is a minor issue, as it requires a user to configure gpg, and do a bad job at it. Not being able to input the passphrase on Windows is a major issue, as the user has done nothing wrong. > The passphrase/tty issue must be Windows specific - or the non-issue > Linux-specific, if you prefer. Sure. Let's talk about semantics. Oh wait, maybe we should work on resolving the issue instead. > > This fixes https://github.com/git-for-windows/git/issues/871 To reiterate: this is the problem I need to see solved. Ciao, Dscho
Re: [WIP PATCH v2] diff.c: emit moved lines with a different color
On Tue, Sep 6, 2016 at 5:44 AM, Junio C Hamano wrote: > 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. As we use different parameters to the xdiff machinery (e.g. context = 1 line) the output is not byte-for-byte identical. > The output may currently be reproducible, but that > is an unnecessary and an expensive thing to rely on. My original design was to not store the lines in the hashmap but only pointers to them, such that the additional memory pressure was assumed less than storing the whole output of the xdiff machinery. That point is moot though in the current implementation, so it would be better indeed if we run the xdiff machinery once and store all its output and then operate on that, even from a memory perspective. > > 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 > with whatever hash value you need to throw it into the hash bucket. Yeah I guess I'll go that way in the next patch then. > > 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? Not yet. Thanks, Stefan > > 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. >
Re: [PATCHv4] diff.c: emit moved lines with a different color
On Tue, Sep 6, 2016 at 7:05 AM, Jakub Narębski wrote:
> 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[=]` 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...
Can you please elaborate on how you'd use that as a user?
The -M and -C options only operate on the file level, e.g.
these options are very good at things introduced via:
git mv A B
$EDIT B # only a little.
So these options make no sense when operating only on one
file or on many files that stay the same and only change very little.
The goal of my patch here is to improve cases like 11979b98
(2005-11-18, http.c: reorder to avoid compilation failure.)
In that case we just move code around, not necessarily across file
boundaries.
So that seems orthogonal to the -M/-C option as it operates on another
level. (file vs line)
In another email you asked whether this new approach works in the
word-by-word diff, which it unfortunately doesn't yet, but I would think
that it is the same problem (line vs word granularity)
So what I am asking here is, how would you imagine a better user interface
for what I am trying to do, or do you think I should adapt my goal?
Thanks,
Stefan
>
> --
> Jakub Narębski
>
How to simulate a real checkout to test a new smudge filter?
I am looking for a way to force smudge filter to run by simulating a real life checkout. Let's say I just created a new branch and did not modify any files but want to test my new smudge filter. According to some answers such as https://stackoverflow.com/questions/22909620/git-smudge-clean-filter-between-branches and https://stackoverflow.com/questions/21652242/git-re-checkout-files-after-creating-smudge-filter it should be possible by running: git checkout HEAD -- but in doesn't work with git 2.9.0. Method suggested in accepted answer here https://stackoverflow.com/questions/17223527/how-do-i-force-git-to-checkout-the-master-branch-and-remove-carriage-returns-aft works but I don't like because it seems fragile. Is there a safe way to do what I want to do in Git still today? --
Re: [PATCHv4] diff.c: emit moved lines with a different color
W dniu 06.09.2016 o 19:03, Stefan Beller pisze: > On Tue, Sep 6, 2016 at 7:05 AM, Jakub Narębski wrote: >> 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... > > Can you please elaborate on how you'd use that as a user? > > The -M and -C options only operate on the file level, e.g. > these options are very good at things introduced via: > > git mv A B > $EDIT B # only a little. > > So these options make no sense when operating only on one > file or on many files that stay the same and only change very little. > > The goal of my patch here is to improve cases like 11979b98 > (2005-11-18, http.c: reorder to avoid compilation failure.) > > In that case we just move code around, not necessarily across file > boundaries. > > So that seems orthogonal to the -M/-C option as it operates on another > level. (file vs line) The idea for an alternative way of turning on color marking of moved lines was to follow an example of "git blame", where _doubling_ of a command means more extensive move / copy detection (accompanied by new values for `diff.renames`). >From git-blame(1) manpage: -C|| In addition to -M, detect lines moved or copied from other files that were modified in the same commit. [...]. When this option is given twice, the command additionally looks for copies from other files in the commit that creates the file. When this option is given three times, the command additionally looks for copies from other files in any commit. Color marking of moved lines may be considered enhancing of exiting whole-file movement and whole-file copy detection. But it is not a good UI if the feature is to be turned on by default. Your proposal of adding `--color-moved` and `color.moved` is better. > In another email you asked whether this new approach works in the > word-by-word diff, which it unfortunately doesn't yet, but I would think > that it is the same problem (line vs word granularity). I don't know how it is done internally, but I think word diff is done by using words (as defined by `diff..wordRegex`) in place of lines... Best, -- Jakub Narębski
Re: [PATCH] xdiff: remove unneeded declarations
On Fri, Sep 2, 2016 at 8:16 PM, Stefan Beller wrote:
> Signed-off-by: Stefan Beller
> ---
> xdiff/xemit.c | 9 -
> 1 file changed, 9 deletions(-)
Despite the moved coloring patch moving into a different direction, I
think this is still
an improvement to the code.
Thanks,
Stefan
>
> diff --git a/xdiff/xemit.c b/xdiff/xemit.c
> index 49aa16f..b52b4b9 100644
> --- a/xdiff/xemit.c
> +++ b/xdiff/xemit.c
> @@ -22,15 +22,6 @@
>
> #include "xinclude.h"
>
> -
> -
> -
> -static long xdl_get_rec(xdfile_t *xdf, long ri, char const **rec);
> -static int xdl_emit_record(xdfile_t *xdf, long ri, char const *pre,
> xdemitcb_t *ecb);
> -
> -
> -
> -
> static long xdl_get_rec(xdfile_t *xdf, long ri, char const **rec) {
>
> *rec = xdf->recs[ri]->ptr;
> --
> 2.10.0.rc2.22.g25cb54d.dirty
>
Git Miniconference at Plumbers
Folks, I have recently been enlisted by folks at the Linux Foundation to help run a Miniconference on Git at the Plumbers Conference [*] this fall. We currently have both Junio Hamano and Josh Triplett signed up to do talks. Hopefully, though not confirmed yet, Junio will give us a brief "State of the Git Union" to set the stage for a few more presentations and some discussion about the Future of Git. Josh has volunteered to talk about a patch series manager he's been developing. Rumor also suggests that the Man In The Bowtie might make an appearance as well. With luck, Mr. Bottomley might offer a speaking role as well! We'll see. I would like to solicit one or two more solid talks from the community, either the Linux Kernel community or the Git community proper. If you are so interested, please send me some mail. Thanks, jdl [*] -- http://www.linuxplumbersconf.org/2016/
Bug? ran into a "fatal" using interactive rebase
Hi, today I accidentally triggered a "fatal" using interactive rebase. If you edit the instruction sheet after 'rebase -i' and add an unknown command, Git stops because it doesn't know the command. That's fine, however, now we are in a state where 'git status' fails with interactive rebase in progress; onto 311f279 fatal: Could not open file .git/rebase-merge/done for reading: No such file or directory After finishing the interactive rebase, things are fixed. Looks like a special case that isn't handled well, yet. My Git version is the current 'next' branch. Ralf
[PATCH] rebase -i: improve advice on bad instruction lines
If we found bad instruction lines in the instruction sheet of interactive rebase, we give the user advice on how to fix it. However, we don't tell the user what to do afterwards. Give the user advice to run 'git rebase --continue' after the fix. Signed-off-by: Ralf Thielow --- git-rebase--interactive.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh index b1ba21c..029594e 100644 --- a/git-rebase--interactive.sh +++ b/git-rebase--interactive.sh @@ -1041,7 +1041,7 @@ The possible behaviours are: ignore, warn, error.")" # placed before the commit of the next action checkout_onto - warn "$(gettext "You can fix this with 'git rebase --edit-todo'.")" + warn "$(gettext "You can fix this with 'git rebase --edit-todo' and then run 'git rebase --continue'.")" die "$(gettext "Or you can abort the rebase with 'git rebase --abort'.")" fi } -- 2.10.0.304.gf2ff484
Re: [PATCH v14 30/41] Move libified code from builtin/apply.c to apply.{c,h}
On Sun, Sep 4, 2016 at 1:18 PM, Christian Couder
wrote:
> As most of the apply code in builtin/apply.c has been libified by a number of
> previous commits, it can now be moved to apply.{c,h}, so that more code can
> use it.
>
> Helped-by: Nguyễn Thái Ngọc Duy
> Helped-by: Ramsay Jones
> Signed-off-by: Christian Couder
> ---
> apply.c | 4731 ++
> apply.h | 19 +
> builtin/apply.c | 4733
> +--
> 3 files changed, 4751 insertions(+), 4732 deletions(-)
So I wanted to review this patch, so I wrote a patch to review this patch. ;)
https://public-inbox.org/git/82367750-adea-6dee-198a-e39ac7a84...@gmail.com/T/#t
> + if (!state->apply_in_reverse &&
> + state->ws_error_action != nowarn_ws_error)
> + check_whitespace(state, line, len,
> patch->ws_rule);
> + added++;
> + newlines--;
> + trailing = 0;
> + break;
> +
> + /*
> +* We allow "\ No newline at end of file". Depending
> + * on locale settings when the patch was produced we
> + * don't know what this line looks like. The only
> + * thing we do know is that it begins with "\ ".
The previous three lines are white space broken AFAICT. The seem to be
white space broken in the original location as well, so no need for a reroll
just for this. But in case you do a reroll, you may want to fix these
on the fly?
> +
> +int apply_option_parse_exclude(const struct option *opt,
> + const char *arg, int unset)
> +
> +int apply_option_parse_include(const struct option *opt,
> + const char *arg, int unset)
> +int apply_option_parse_p(const struct option *opt,
> +const char *arg,
> +int unset)
These three functions seem slightly different, not just moved.
Oh you removed the static key word!
Apart from the one minor nit and the removal of the static keyword,
the rest is just moved code.
Thanks,
Stefan
Re: [PATCH 0/3] Fix a segfault caused by regexec() being called on mmap()ed data
On Tue, Sep 06, 2016 at 04:06:32PM +0200, Johannes Schindelin wrote:
> > 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.
Yes, that is the important one, I think. The others are for patterns,
error msgs, etc. Of course strlen() is not the only function that cares
about NUL delimiters (and there might even be a "while (*p)" somewhere
in the code).
I always assumed the _point_ of re_search taking a ptr/len pair was
exactly to handle this case. The documentation[1] says:
`string` is the string you want to match; it can contain newline and
null characters. `size` is the length of that string.
Which seems pretty definitive to me (that's for re_match(), but
re_search() is defined in the docs in terms of re_match()).
[1] http://www.delorie.com/gnu/docs/regex/regex_47.html
> 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.
Between the documentation above, and the fact that your new test passes
when we switch to it (see below), I feel pretty good about 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.
We can contain this to the existing compat/regexec/regexec.c, and just
provide a wrapper that is similar to regexec but takes a ptr/len pair.
Like:
diff --git a/compat/regex/regex.h b/compat/regex/regex.h
index 61c9683..b2dd0b7 100644
--- a/compat/regex/regex.h
+++ b/compat/regex/regex.h
@@ -569,6 +569,11 @@ extern int regexec (const regex_t *__restrict __preg,
regmatch_t __pmatch[__restrict_arr],
int __eflags);
+extern int regexecn (const regex_t *__restrict __preg,
+const char *__restrict __cstring, size_t __length,
+size_t __nmatch, regmatch_t __pmatch[__restrict_arr],
+int __eflags);
+
extern size_t regerror (int __errcode, const regex_t *__restrict __preg,
char *__restrict __errbuf, size_t __errbuf_size);
diff --git a/compat/regex/regexec.c b/compat/regex/regexec.c
index eb5e1d4..8afe26b 100644
--- a/compat/regex/regexec.c
+++ b/compat/regex/regexec.c
@@ -217,15 +217,16 @@ static reg_errcode_t extend_buffers (re_match_context_t
*mctx)
We return 0 if we find a match and REG_NOMATCH if not. */
int
-regexec (
+regexecn (
const regex_t *__restrict preg,
const char *__restrict string,
+ size_t length,
size_t nmatch,
regmatch_t pmatch[],
int eflags)
{
reg_errcode_t err;
- int start, length;
+ int start;
if (eflags & ~(REG_NOTBOL | REG_NOTEOL | REG_STARTEND))
return REG_BADPAT;
@@ -238,7 +239,7 @@ regexec (
else
{
start = 0;
- length = strlen (string);
+ /* length already passed in */
}
__libc_lock_lock (dfa->lock);
@@ -252,6 +253,17 @@ regexec (
return err != REG_NOERROR;
}
+int
+regexec (
+ const regex_t *__restrict preg,
+ const char *__restrict string,
+ size_t nmatch,
+ regmatch_t pmatch[],
+ int eflags)
+{
+ return regexecn(preg, string, strlen(string), nmatch, pmatch, eflags);
+}
+
#ifdef _LIBC
# include
versioned_symbol (libc, __regexec, regexec, GLIBC_2_3_4);
diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index 55067ca..fdd08dd 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -50,9 +50,9 @@ static int diff_grep(mmfile_t *one, mmfile_t *two,
xdemitconf_t xecfg;
if (!one)
- return !regexec(regexp, two->ptr, 1, ®match, 0);
+ return !regexecn(regexp, two->ptr, two->size, 1, ®match, 0);
if (!two)
- return !regexec(regexp, one->ptr, 1, ®match, 0);
+ return !regexecn(regexp, one->ptr, one->size, 1, ®match, 0);
/*
* We have both sides; need to run textual diff and see if
Re: If a branch moves a submodule, "merge --ff[-only]" succeeds while "merge --no-ff" fails with conflicts
Is there any additional information I could provide that would be helpful? Dakota On Fri, Sep 2, 2016 at 3:22 PM, Dakota Hawkins wrote: > Below is a simple reproduction of the issue. > > The _real_ problem is that this is how our pull request merges work, > they're not allowed to do fast-forward merges. To work around this we > are having to split this up into two pull requests/merges: One that > copies the submodules to the new location and includes any fixes > required to support the move, and a second that removes the old > locations. > > ## Setup steps > git clone > https://github.com/dakotahawkins/submodule-move-merge-bug-main-repo.git > cd submodule-move-merge-bug-main-repo > ## How it was initially constructed > # git submodule add ../submodule-move-merge-bug-submodule-repo.git > ./submodule-location-1 > # git commit -m "Added submodule in its initial location" > # git push > # git checkout -b move-submodule > # git mv ./submodule-location-1 ./submodule-location-2 > # git commit -m "Moved submodule" > # git push --set-upstream origin move-submodule > git branch move-submodule origin/move-submodule > > ## Test fast-forward merge, this will work > git checkout -b merge-ff-test master # warning: unable to rmdir > submodule-location-2: Directory not empty > rm -rf ./submodule-location-2 > git merge --ff-only move-submodule > > ## Test no-fast-forward merge, this will fail with conflicts: > git checkout -b merge-no-ff-test master > git merge --no-ff move-submodule > # Auto-merging submodule-location-2 > # Adding as submodule-location-2~move-submodule instead > # Automatic merge failed; fix conflicts and then commit the result. > git status > # On branch merge-no-ff-test > # You have unmerged paths. > # (fix conflicts and run "git commit") > # (use "git merge --abort" to abort the merge) > # > # Changes to be committed: > # > # modified: .gitmodules > # deleted:submodule-location-1 > # > # Unmerged paths: > # (use "git add ..." to mark resolution) > # > # added by us: submodule-location-2 > # > # fatal: Not a git repository: 'submodule-location-1/.git' > # Submodule changes to be committed: > # > # * submodule-location-1 07fec24...000:
Re: [PATCH 2/3] diff_populate_filespec: NUL-terminate buffers
On Tue, Sep 06, 2016 at 06:02:59PM +0200, Johannes Schindelin wrote:
> 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.
I was thinking of something quite gross, like a global "switch to using
slower-but-safer NUL termination" flag (but I agree with Junio's point
elsewhere that we do not even know if it is "slower").
> > 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.
Thanks, I forgot about that case.
> > [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.
Sort of. I think it has been part of the design since e871b64
(unpack_sha1_file: zero-pad the unpacked object., 2005-05-25), though I
do recall that we missed a code path that did its allocation differently
(in index-pack, IIRC).
Anyway, that is neither here nor there for the diff code, which as you
noticed may operate on things besides git objects.
> 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.
Yes, and compat/regex support this, too. My question is whether it is
portable. I see:
> 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)) {
What happens to those poor souls on systems without REG_STARTEND? Do
they get to keep segfaulting?
I think the solution is to push them into setting NO_REGEX. So looking
at this versus a "regexecn", it seems:
- this lets people keep using their native regexec if it supports
STARTEND
- this is a bit more clunky to use at the callsites (though we could
_create_ a portable regexecn wrapper that uses this technique on top
of the native regex library)
But I much prefer this approach to copying the data just to add a NUL.
-Peff
Re: [PATCH 1/3] Demonstrate a problem: our pickaxe code assumes NUL-terminated buffers
On Mon, Sep 05, 2016 at 05:45:02PM +0200, Johannes Schindelin wrote: > Typically, on Linux the test passes. On Windows, it fails virtually > every time due to an access violation (that's a segmentation fault for > you Unix-y people out there). And Windows would be correct: the > regexec() call wants to operate on a regular, NUL-terminated string, > there is no NUL in the mmap()ed memory range, and it is undefined > whether the next byte is even legal to access. > > When run with --valgrind it demonstrates quite clearly the breakage, of > course. > > So we simply mark it with `test_expect_success` for now. I'd prefer if this were marked as expect_failure. It fails reliably for me on Linux, even without --valgrind. But even if that were not so, there is no reason to hurt bisectability of somebody running with "--valgrind" (not when it costs so little to mark it correctly). -Peff
Re: [PATCH] rebase -i: improve advice on bad instruction lines
2016-09-06 20:08 GMT+02:00 Ralf Thielow : > - warn "$(gettext "You can fix this with 'git rebase > --edit-todo'.")" > + warn "$(gettext "You can fix this with 'git rebase > --edit-todo' and then run 'git rebase --continue'.")" > die "$(gettext "Or you can abort the rebase with 'git rebase > --abort'.")" Please don't apply as is. There are some test failures due to the text change. I'll send an updated version.
Re: [PATCH v14 00/41] libify apply and use lib in am, part 2
On Sun, Sep 4, 2016 at 1:17 PM, Christian Couder wrote: > Goal > > > This is a patch series about libifying `git apply` functionality, and > using this libified functionality in `git am`, so that no 'git apply' > process is spawn anymore. This makes `git am` significantly faster, so > `git rebase`, when it uses the am backend, is also significantly > faster. > I reviewed this v14 and all patches look good to me. Thanks, Stefan
[PATCH v2] rebase -i: improve advice on bad instruction lines
If we found bad instruction lines in the instruction sheet of interactive rebase, we give the user advice on how to fix it. However, we don't tell the user what to do afterwards. Give the user advice to run 'git rebase --continue' after the fix. Signed-off-by: Ralf Thielow --- Changes in v2: - adjust tests git-rebase--interactive.sh| 2 +- t/t3404-rebase-interactive.sh | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh index b1ba21c..029594e 100644 --- a/git-rebase--interactive.sh +++ b/git-rebase--interactive.sh @@ -1041,7 +1041,7 @@ The possible behaviours are: ignore, warn, error.")" # placed before the commit of the next action checkout_onto - warn "$(gettext "You can fix this with 'git rebase --edit-todo'.")" + warn "$(gettext "You can fix this with 'git rebase --edit-todo' and then run 'git rebase --continue'.")" die "$(gettext "Or you can abort the rebase with 'git rebase --abort'.")" fi } diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index 597e94e..e38e296 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -1195,7 +1195,7 @@ To avoid this message, use "drop" to explicitly remove a commit. Use 'git config rebase.missingCommitsCheck' to change the level of warnings. The possible behaviours are: ignore, warn, error. -You can fix this with 'git rebase --edit-todo'. +You can fix this with 'git rebase --edit-todo' and then run 'git rebase --continue'. Or you can abort the rebase with 'git rebase --abort'. EOF @@ -1219,7 +1219,7 @@ cat >expect
Re: If a branch moves a submodule, "merge --ff[-only]" succeeds while "merge --no-ff" fails with conflicts
On Fri, Sep 2, 2016 at 12:22 PM, Dakota Hawkins wrote: > Below is a simple reproduction of the issue. > > The _real_ problem is that this is how our pull request merges work, So your workflow is the problem or is the actual bug just exposed in your workflow? > they're not allowed to do fast-forward merges. To work around this we > are having to split this up into two pull requests/merges: One that > copies the submodules to the new location and includes any fixes > required to support the move, and a second that removes the old > locations. > > ## Setup steps > git clone > https://github.com/dakotahawkins/submodule-move-merge-bug-main-repo.git > cd submodule-move-merge-bug-main-repo > ## How it was initially constructed > # git submodule add ../submodule-move-merge-bug-submodule-repo.git > ./submodule-location-1 > # git commit -m "Added submodule in its initial location" > # git push > # git checkout -b move-submodule > # git mv ./submodule-location-1 ./submodule-location-2 > # git commit -m "Moved submodule" > # git push --set-upstream origin move-submodule > git branch move-submodule origin/move-submodule > > ## Test fast-forward merge, this will work > git checkout -b merge-ff-test master # warning: unable to rmdir > submodule-location-2: Directory not empty > rm -rf ./submodule-location-2 > git merge --ff-only move-submodule > And no reset/rm in between, i.e. we still have submodule-location-2 from merge-ff-test still around? > ## Test no-fast-forward merge, this will fail with conflicts: > git checkout -b merge-no-ff-test master > git merge --no-ff move-submodule > # Auto-merging submodule-location-2 > # Adding as submodule-location-2~move-submodule instead > # Automatic merge failed; fix conflicts and then commit the result. > git status > # On branch merge-no-ff-test > # You have unmerged paths. > # (fix conflicts and run "git commit") > # (use "git merge --abort" to abort the merge) > # > # Changes to be committed: > # > # modified: .gitmodules > # deleted:submodule-location-1 > # > # Unmerged paths: > # (use "git add ..." to mark resolution) > # > # added by us: submodule-location-2 > # > # fatal: Not a git repository: 'submodule-location-1/.git' > # Submodule changes to be committed: > # > # * submodule-location-1 07fec24...000:
Re: Fixup of a fixup not working right
From: "Johannes Schindelin"
Hi Philip,
On Sun, 4 Sep 2016, Philip Oakley wrote:
From: "Johannes Schindelin"
> The point is that fixup! messages are really special, and are always
> intended to be squashed into the referenced commit *before* the latter
> hits `master`.
I think it's here that we have the hidden use case. I agree that all
fixups
should be squashed before they hit the blessed golden repository.
I suspect that some use cases have intermediate repositories that
contain a 'master' branch (it's just a name ;-) that isn't blessed and
golden, e.g. at the team review repo level. In such cases it is possible
for a fixup! to be passed up as part of the review, though it's not the
current norm/expectation.
In such a case (which can totally arise when criss-crossing Pull Requests
on GitHub, for example, where a Pull Request's purpose may be to fix up
commits in another Pull Request before the latter is merged), the most
appropriate course of action is... to not reorder the fixup!s prematurely.
We just need to be careful about that plural just there.
If it is multiple fixup!s for the same commit, then I believe they should be
grouped together at the same point as the first fixup! commit (in their
original order).
If they are for different commits, then they should stay in their place in
the commit series (for their first occurrence, then rule 1 applies)
> In short, I am opposed to this change.
It's not like G4W doesn't need fixup!s on the side branches e.g. 5eaffe9
("fixup! Handle new t1501 test case properly with MinGW", 2016-07-12)
I note that you don't have two fixup!s for that commit
Yeah, well, Git for Windows' `master` branch is special, in that it is
constantly rebased (as "merging rebases", to keep fast-forwardability). I
would not necessarily use Git for Windows as a role model in this respect.
I don't see GfW as 'special', rather as being a representative of a broader
realpolitik where some of the rugged individualism of open source is
moderated in some way or another.
Ciao,
Dscho
--
Philip
[PATCH] gitweb: use highlight's shebang detection
The highlight binary can detect language by shebang when we can't tell
the syntax type by the name of the file. To use highlight's shebang
detection, add highlight to the pipeline whenever highlight is enabled.
Document the shebang detection and add a test which exercises it in
t/t9500-gitweb-standalone-no-errors.sh.
Signed-off-by: Ian Kelling
---
Notes:
I wondered if adding highlight to the pipeline would make viewing a blob
with no highlighting take longer but it did not on my computer. I found
no noticeable impact on small files and strangely, on a 159k file, it
took 7% less time averaged over several requests.
Documentation/gitweb.conf.txt | 21 ++---
gitweb/gitweb.perl | 10 +-
t/t9500-gitweb-standalone-no-errors.sh | 18 +-
3 files changed, 32 insertions(+), 17 deletions(-)
diff --git a/Documentation/gitweb.conf.txt b/Documentation/gitweb.conf.txt
index a79e350..e632089 100644
--- a/Documentation/gitweb.conf.txt
+++ b/Documentation/gitweb.conf.txt
@@ -246,13 +246,20 @@ $highlight_bin::
Note that 'highlight' feature must be set for gitweb to actually
use syntax highlighting.
+
-*NOTE*: if you want to add support for new file type (supported by
-"highlight" but not used by gitweb), you need to modify `%highlight_ext`
-or `%highlight_basename`, depending on whether you detect type of file
-based on extension (for example "sh") or on its basename (for example
-"Makefile"). The keys of these hashes are extension and basename,
-respectively, and value for given key is name of syntax to be passed via
-`--syntax ` to highlighter.
+*NOTE*: for a file to be highlighted, its syntax type must be detected
+and that syntax must be supported by "highlight". The default syntax
+detection is minimal, and there are many supported syntax types with no
+detection by default. There are three options for adding syntax
+detection. The first and second priority are `%highlight_basename` and
+`%highlight_ext`, which detect based on basename (the full filename, for
+example "Makefile") and extension (for example "sh"). The keys of these
+hashes are the basename and extension, respectively, and the value for a
+given key is the name of the syntax to be passed via `--syntax `
+to "highlight". The last priority is the "highlight" configuration of
+`Shebang` regular expressions to detect the language based on the first
+line in the file, (for example, matching the line "#!/bin/bash"). See
+the highlight documentation and the default config at
+/etc/highlight/filetypes.conf for more details.
+
For example if repositories you are hosting use "phtml" extension for
PHP files, and you want to have correct syntax-highlighting for those
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 33d701d..a672181 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3931,15 +3931,16 @@ sub guess_file_syntax {
# or return original FD if no highlighting
sub run_highlighter {
my ($fd, $highlight, $syntax) = @_;
- return $fd unless ($highlight && defined $syntax);
+ return $fd unless ($highlight);
close $fd;
+ my $syntax_arg = (defined $syntax) ? "--syntax $syntax" : "--force";
open $fd, quote_command(git_cmd(), "cat-file", "blob", $hash)." | ".
quote_command($^X, '-CO', '-MEncode=decode,FB_DEFAULT',
'-pse',
'$_ = decode($fe, $_, FB_DEFAULT) if !utf8::decode($_);',
'--', "-fe=$fallback_encoding")." | ".
quote_command($highlight_bin).
- " --replace-tabs=8 --fragment --syntax $syntax |"
+ " --replace-tabs=8 --fragment $syntax_arg |"
or die_error(500, "Couldn't open file or run syntax
highlighter");
return $fd;
}
@@ -7063,8 +7064,7 @@ sub git_blob {
my $highlight = gitweb_check_feature('highlight');
my $syntax = guess_file_syntax($highlight, $mimetype, $file_name);
- $fd = run_highlighter($fd, $highlight, $syntax)
- if $syntax;
+ $fd = run_highlighter($fd, $highlight, $syntax);
git_header_html(undef, $expires);
my $formats_nav = '';
@@ -7117,7 +7117,7 @@ sub git_blob {
$line = untabify($line);
printf qq!%4i %s\n!,
$nr, esc_attr(href(-replay => 1)), $nr, $nr,
- $syntax ? sanitize($line) : esc_html($line,
-nbsp=>1);
+ $highlight ? sanitize($line) : esc_html($line,
-nbsp=>1);
}
}
close $fd
diff --git a/t/t9500-gitweb-standalone-no-errors.sh
b/t/t9500-gitweb-standalone-no-errors.sh
index e94b2f1..9e5fcfe 100755
--- a/t/t9500-gitweb-standalone-no-errors.sh
+++ b/t/t9500-gitweb-standalone-no-errors.sh
@@ -702,12 +702,20 @@ test_expect_success HIGHLIGHT \
gitweb_run "p=.git;a=blob;f=file"'
test_expect_success
Re: 2.10.0: multiple versionsort.prereleasesuffix buggy?
Hi,
Quoting Jeff King :
On Tue, Sep 06, 2016 at 03:07:59AM +0200, SZEDER Gábor wrote:
So that seems wrong. Even weirder, if I set _only_ "-beta", I get:
$ git tag -l --sort=version:refname | grep -v ^2.6.0
2.6.0-beta-2
2.6.0-beta-3
2.6.0-beta-4
2.6.0
2.6.0-RC1
2.6.0-RC2
2.6.0-beta-1
Umm...what? beta-1 is sorted away from its companions? That's weird.
I wondered if the presence of "-" after the suffix ("beta-1" rather than
"beta1") would matter. It looks like that shouldn't matter, though; it's
purely doing a prefix match on "do these names differ at a prerelease
suffix".
But something certainly seems wrong.
Some of the weirdness is caused by the '-' at the _beginning_ of the
suffixes, because versioncmp() gets confused by suffixes starting with
the same character(s).
Oh, right, that makes sense. So it's effectively not finding _any_
suffix between X-RC1 and X-beta-1, because we only start looking after
"X-", and none of them match.
I am still confused why "2.6.0-beta-1" doesn't get sorted with its
peers. I'd guess that the comparison function doesn't actually provide a
strict ordering, so the results depend on the actual sort algorithm, and
which pairs it ends up comparing.
Turns out that this weirdness is caused by that leading '-' in the suffix,
too.
Here is a manageably small recipe to reproduce:
$ git -c versionsort.prereleasesuffix=-beta tag -l
--sort=version:refname v2.1.0* v2.1.{1,2}
v2.1.0-beta-2
v2.1.0-beta-3
v2.1.0
v2.1.0-RC1
v2.1.0-RC2
v2.1.0-beta-1
v2.1.1
v2.1.2
Tracing which pairs of tagnames are compared, I found that somewhere along
the line "v2.1.0-beta-1" happens to be compared to "v2.1.0-RC2", and the
issue described in my previous email strikes again: the '-' is part of the
common part of the two tagnames, swap_prereleases() gets only "beta-1" and
"RC2", thus it can't match the configured "-beta" suffix, and since the
byte value of 'b' is higher than that of 'R', "-beta-1" is sorted after
"-RC2". OTOH, "v2.1.0-beta-2" and "v2.1.0-beta-3" are only compared to
each other or to final release tags, but never to any "-RCx" tags, hence
they are sorted properly.
Once I finish teaching versioncmp() and swap_prereleases() to cope with
leading characters of a prereleaseSuffix being part of the common part of
two tagnames, this out-of-order "beta-1" issue will be gone as well.
Best,
Gábor
Re: How to simulate a real checkout to test a new smudge filter?
On 06.09.16 19:47, john smith wrote: > I am looking for a way to force smudge filter to run by simulating a > real life checkout. Let's say I just created a new branch and did not > modify any files but want to test my new smudge filter. According to > some answers such as > https://stackoverflow.com/questions/22909620/git-smudge-clean-filter-between-branches > and > https://stackoverflow.com/questions/21652242/git-re-checkout-files-after-creating-smudge-filter > it should be possible by running: > > git checkout HEAD -- > > but in doesn't work with git 2.9.0. Method suggested in accepted > answer here > https://stackoverflow.com/questions/17223527/how-do-i-force-git-to-checkout-the-master-branch-and-remove-carriage-returns-aft > works but I don't like because it seems fragile. Is there a safe way > to do what I want to do in Git still today? > It depends what you mean with "safe way". git checkout, git checkout -f or other combinations will only overwrite/rewrite the files in the working tree, if, and only if, git comes to the conclusion that "git add" will do something, like replace a blob for a file in the index. (And by running "rm .git/index git will evaluate the "clean" filters, and the CRLF->LF conversion). If you want to test a smudge filter, simply remove the file: mv file /tmp && git checkout file
Re: If a branch moves a submodule, "merge --ff[-only]" succeeds while "merge --no-ff" fails with conflicts
On Tue, Sep 6, 2016 at 3:00 PM, Stefan Beller wrote: > On Fri, Sep 2, 2016 at 12:22 PM, Dakota Hawkins > wrote: >> Below is a simple reproduction of the issue. >> >> The _real_ problem is that this is how our pull request merges work, > > So your workflow is the problem or is the actual bug just exposed in > your workflow? I believe the workflow just exposes the problem, which is that generically for this case a fast-forward merge works without conflicts while a non-fast-forward merge fails with conflicts. Sorry if this confuses the issue, it's just how we experienced it so I wanted to add that background. >> ## Test fast-forward merge, this will work >> git checkout -b merge-ff-test master # warning: unable to rmdir >> submodule-location-2: Directory not empty >> rm -rf ./submodule-location-2 >> git merge --ff-only move-submodule >> > > And no reset/rm in between, i.e. we still have > submodule-location-2 from merge-ff-test still around? That is true in that example, but somewhat immaterial. The first merge was only to demonstrate that a fast-forward merge works without conflict. The simplest reproduction is to skip that and get straight to the failure case: ## Clone and setup branches git clone https://github.com/dakotahawkins/submodule-move-merge-bug-main-repo.git cd submodule-move-merge-bug-main-repo git branch move-submodule origin/move-submodule git checkout -b merge-no-ff-test master ## This will fail git merge --no-ff move-submodule # Auto-merging submodule-location-2 # Adding as submodule-location-2~move-submodule instead # Automatic merge failed; fix conflicts and then commit the result. Does that make things a little bit clearer? Dakota
Re: Your email
Johannes, You know what, lets end it. You are right and I am sorry. Have a great life. -- -Best Idan
Re: How to simulate a real checkout to test a new smudge filter?
On 9/6/16, Torsten Bögershausen wrote: > On 06.09.16 19:47, john smith wrote: >> I am looking for a way to force smudge filter to run by simulating a >> real life checkout. Let's say I just created a new branch and did not >> modify any files but want to test my new smudge filter. According to >> some answers such as >> https://stackoverflow.com/questions/22909620/git-smudge-clean-filter-between-branches >> and >> https://stackoverflow.com/questions/21652242/git-re-checkout-files-after-creating-smudge-filter >> it should be possible by running: >> >> git checkout HEAD -- >> >> but in doesn't work with git 2.9.0. Method suggested in accepted >> answer here >> https://stackoverflow.com/questions/17223527/how-do-i-force-git-to-checkout-the-master-branch-and-remove-carriage-returns-aft >> works but I don't like because it seems fragile. Is there a safe way >> to do what I want to do in Git still today? >> > It depends what you mean with "safe way". I want to store all my dotfiles in a single repoitory. The problem is that that some specific pieces of these files are different on different machines. I have a special .conf file that is different on every branch and contains machine-specific definitions of some variables such as EMAIL or SMTP server. In my smudge filter I call a script which parses .conf file and replace all template variable definitions saved in the given file with correct definitions. For example in my ~/.bashrc I have this on all branches: export EMAIL="@EMAIL@" and in my .conf file on `home' branch EMAIL=h...@address.com and on `work' branch: EMAIL=w...@address.com And in .gitattributes on both branches: bash/.bashrc filter=make-usable I also have single `master' branch that only contains template dotfiles and no .conf. When setting up a new machine I could just create a new branch off master branch and add a new .conf. In turn, clean filter replace all correct definitions in the given dotfiles back into template definitions. I'd prefer smudge/clean filters instead of `make' scripts etc. to convert template dotfiles into something usable and back because filters: 1. could be run automatically 2. do not modify files as shown by `git show HEAD:' and therefore no files are reported as modified by git status and also there are not conflicts when merging master into work/home branch. I have problems because with point 1 because apparently smudge filter is not run automatically every time when branch is changed if files listed in .gitattributes do not change. As the last resort I could force smudge/clean filter to run just to keep advantage specified in point 2. --
Re: [PATCH v1 2/2] read-cache: make sure file handles are not inherited by child processes
larsxschnei...@gmail.com wrote:
> 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);
>
> if (fd >= 0) {
> unsigned char sha1[20];
Also, this needs to check EINVAL when O_CLOEXEC != 0 the same
way create_tempfile currently does. Somebody could be building
with modern headers but running an old kernel that doesn't
understand O_CLOEXEC.
There should probably be a open() wrapper for handling this case
since we're now up to 3 places where open(... O_CLOEXEC) is
used.
Re: [PATCH] sequencer: support folding in rfc2822 footer
On 09/02/2016 07:23 PM, Junio C Hamano wrote: Jonathan Tan writes: Sample-field: multiple-line field body that causes a blank line below I am not sure this is unconditionally good, or may cause problems to those with workflows you did not consider when you wrote this patch. Not being too lenient here historically has been a deliberate decision to avoid misidentification of non "footers". Does Git itself produce some folded footer line? If Git itself produced such folded lines, I'd be a lot more receptive to this change, but I do not think that is the case here. I don't think Git produces any folded lines, but folded lines do appear in footers in projects that use Git. For example, some Android commits have multi-line "Test:" fields (example, [1]) and some Linux commits have multi-line "Tested-by:" fields (example, [2]). Taking the Android commit as an example, this would mean that cherrypicking that commit would create a whole new footer, and tripping up tools (for example, Gerrit, which looks for "Change-Id:" in the last paragraph). But this would not happen if "Test:" was single-line instead of multi-line - which seems inconsistent. [1] https://android.googlesource.com/platform/frameworks/base/+/4c5281862f750cbc9d7355a07ef1a5545b9b3523 [2] https://kernel.googlesource.com/pub/scm/linux/kernel/git/stable/linux-stable/+/69f92f67b68ab7028ffe15f0eea76b59f8859383 A slightly related tangent. An unconditionally good change you could make is to allow folding of in-body headers. I.e. you can have e.g. -- >8 -- Subject: [PATCH] sequencer: support in-body headers that are folded according to RFC2822 rules The first paragraph after the above long title begins here... in the body of the msssage, and I _think_ we do not fold it properly when applying such a patch. We should, as that is something that appears in format-patch output (i.e. something Git itself produces, unlike the folded "footer"). OK, I'll take a look at this.
Re: [PATCH] sequencer: support folding in rfc2822 footer
On 09/06/2016 03:08 PM, Jonathan Tan wrote: On 09/02/2016 07:23 PM, Junio C Hamano wrote: A slightly related tangent. An unconditionally good change you could make is to allow folding of in-body headers. I.e. you can have e.g. -- >8 -- Subject: [PATCH] sequencer: support in-body headers that are folded according to RFC2822 rules The first paragraph after the above long title begins here... in the body of the msssage, and I _think_ we do not fold it properly when applying such a patch. We should, as that is something that appears in format-patch output (i.e. something Git itself produces, unlike the folded "footer"). OK, I'll take a look at this. It turns out that Git seems to already do this, at least for Subject. Transcript below: $ echo one > file.txt $ git add file.txt $ git commit -m x [master (root-commit) 2389483] x 1 file changed, 1 insertion(+) create mode 100644 file.txt $ echo two > file.txt $ git commit -am 'this is a very long subject to test line wrapping this is a very long subject to test line wrapping' [master ca86792] this is a very long subject to test line wrapping this is a very long subject to test line wrapping 1 file changed, 1 insertion(+), 1 deletion(-) $ git format-patch HEAD^ 0001-this-is-a-very-long-subject-to-test-line-wrapping-th.patch $ cat 0001-this-is-a-very-long-subject-to-test-line-wrapping-th.patch Subject: [PATCH] this is a very long subject to test line wrapping this is a very long subject to test line wrapping
Re: Draft of Git Rev News edition 18
Josh Triplett wrote: > On Tue, Aug 16, 2016 at 09:27:04PM +, Eric Wong wrote: > > As for other projects, I'm not aware of anybody else using it, > > yet. I have some small projects using it, but most of those are > > one-off throwaways and I'm not comfortable promoting those along > > with public-inbox. I admit: I'm not comfortable promoting > > anything I do, really. > > Please take this as encouragement to do so. I'd love to see the > public-inbox equivalent to the main page of https://lists.debian.org/ , > as an example. (And I'd love to have public-inbox archives of Debian > mailing lists.) Just pushed out some POD (which should build to manpages), so maybe early adopters can start hosting mirrors themselves(*). https://public-inbox.org/meta/20160907004907.1479-...@80x24.org/ I hope public-inbox-overview(7) is a good starting point (along with the existing INSTALL) and there'll be more docs coming at some point... Writing documentation tends to make my attention span drift all over the place; so maybe parts don't make sense or were glossed over, but I'll be glad to help clarify anything. (Responding to emails is generally easier for me since I can answer things specifically, tough to do for generic docs) I'll try to get a tarball release out soonish, but my schedule is unpredictable. (*) None of the code has had any security audit, yet; and there's no warranty of course.
Re: [PATCH v2 02/38] rename_ref_available(): add docstring
On 09/06/2016 04:25 PM, Jakub Narębski wrote: > 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? I've never heard of an official convention like that, and don't see it documented anywhere. But there are a lot of functions (and variables) whose names start with `is_`, and it seems like a reasonable idea. > That is, shouldn't it be > > int is_rename_ref_available(const char *old_refname, const char > *new_refname); I agree, that would be a better name. But that naming change is orthogonal to this patch series, which only adds a docstring to the function. I don't think it's worth rerolling this 38-patch series to add it. So I suggest that we keep your idea in mind for the next time this code is touched (or feel free to submit a patch yourself, preferably on top of this patch series to avoid conflicts). Thanks, Michael
[PATCH v2] t6026-merge-attr: clean up background process at end of test case
The process spawned in the hook uses the test's trash directory as CWD. As long as it is alive, the directory cannot be removed on Windows. Although the test succeeds, the 'test_done' that follows produces an error message and leaves the trash directory around. Kill the process before the test case advances. Helped-by: Johannes Schindelin Signed-off-by: Johannes Sixt --- Am 06.09.2016 um 09:25 schrieb Johannes Schindelin: > Maybe we should write a pid file in the sleep command instead, and kill it > in the end? Something like this, maybe? Yes, that is much better, thank you! I did not extend the sleep time because it requires to change the file name in the same patch. I did experiment with 'while sleep 1; do :; done &' to spin indefinitely, but the loop did not terminate in time in my tests on Windows for some unknown reason (most likely because the killed process does not exit with a non-zero code -- remember, Windows is not POSIX, particularly, when it comes to signal handling). t/t6026-merge-attr.sh | 2 ++ 1 file changed, 2 insertions(+) diff --git a/t/t6026-merge-attr.sh b/t/t6026-merge-attr.sh index dd8f88d..7a6e33e 100755 --- a/t/t6026-merge-attr.sh +++ b/t/t6026-merge-attr.sh @@ -185,7 +185,9 @@ test_expect_success 'custom merge does not lock index' ' git reset --hard anchor && write_script sleep-one-second.sh <<-\EOF && sleep 1 & + echo $! >sleep.pid EOF + test_when_finished "kill \$(cat sleep.pid)" && test_write_lines >.gitattributes \ "* merge=ours" "text merge=sleep-one-second" && -- 2.10.0.85.gea34e30
Re: [PATCH] sequencer: support folding in rfc2822 footer
On Tue, Sep 06, 2016 at 04:30:24PM -0700, Jonathan Tan wrote:
> On 09/06/2016 03:08 PM, Jonathan Tan wrote:
> > On 09/02/2016 07:23 PM, Junio C Hamano wrote:
> > > A slightly related tangent. An unconditionally good change you
> > > could make is to allow folding of in-body headers. I.e. you can
> > > have e.g.
> > >
> > > -- >8 --
> > > Subject: [PATCH] sequencer: support in-body headers that are
> > > folded according to RFC2822 rules
> > >
> > > The first paragraph after the above long title begins
> > > here...
> > >
> > > in the body of the msssage, and I _think_ we do not fold it properly
> > > when applying such a patch. We should, as that is something that
> > > appears in format-patch output (i.e. something Git itself produces,
> > > unlike the folded "footer").
> >
> > OK, I'll take a look at this.
>
> It turns out that Git seems to already do this, at least for Subject.
Right, because "Subject" is actually a real RFC 2822 header in the
generated email message. Not only do we expect things like mail readers
to handle this, but we _have_ to wrap at a certain point to meet the
standard[1].
I don't think any part of Git ever shunts "Subject" to an in-body
header, though I'd guess people do it manually all the time.
> $ git format-patch HEAD^
> 0001-this-is-a-very-long-subject-to-test-line-wrapping-th.patch
> $ cat 0001-this-is-a-very-long-subject-to-test-line-wrapping-th.patch
>
> Subject: [PATCH] this is a very long subject to test line wrapping this is a
> very long subject to test line wrapping
>
So the interesting bit is what happens with:
git checkout master^
git am 0001-*
and with:
perl -lpe '
# Bump subject down to in-body header.
if (/^Subject:/) {
print "Subject: real subject";
print "";
}
' 0001-* >patch
git checkout master^
git am patch
It looks like we get the first one right, but not the second.
-Peff
[1] A careful reader may note that arbitrarily-long body lines,
including in-body headers and footers, may _also_ run afoul of
the body line-length limits. The "right" solution there is
probably quoted-printable, but it's ugly enough that I wouldn't do
so unless we see a real-world case where the line lengths are a
problem.
