* [PATCH] Unbreak interactive GPG prompt upon signing
From: Johannes Schindelin @ 2016-09-06 8:01 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Michael J Gruber
With the recent update in efee955 (gpg-interface: check gpg signature
creation status, 2016-06-17), we ask GPG to send all status updates to
stderr, and then catch the stderr in an strbuf.
But GPG might fail, and send error messages to stderr. And we simply
do not show them to the user.
Even worse: this swallows any interactive prompt for a passphrase. And
detaches stderr from the tty so that the passphrase cannot be read.
So while the first problem could be fixed (by printing the captured
stderr upon error), the second problem cannot be easily fixed, and
presents a major regression.
So let's just revert commit efee9553a4f97b2ecd8f49be19606dd4cf7d9c28.
This fixes https://github.com/git-for-windows/git/issues/871
Cc: Michael J Gruber <git@drmicha.warpmail.net>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Published-As: https://github.com/dscho/git/releases/tag/fix-gpg-v1
Fetch-It-Via: git fetch https://github.com/dscho/git fix-gpg-v1
gpg-interface.c | 8 ++------
t/t7004-tag.sh | 9 +--------
2 files changed, 3 insertions(+), 14 deletions(-)
diff --git a/gpg-interface.c b/gpg-interface.c
index 8672eda..3f3a3f7 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -153,11 +153,9 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig
struct child_process gpg = CHILD_PROCESS_INIT;
int ret;
size_t i, j, bottom;
- struct strbuf gpg_status = STRBUF_INIT;
argv_array_pushl(&gpg.args,
gpg_program,
- "--status-fd=2",
"-bsau", signing_key,
NULL);
@@ -169,12 +167,10 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig
*/
sigchain_push(SIGPIPE, SIG_IGN);
ret = pipe_command(&gpg, buffer->buf, buffer->len,
- signature, 1024, &gpg_status, 0);
+ signature, 1024, NULL, 0);
sigchain_pop(SIGPIPE);
- ret |= !strstr(gpg_status.buf, "\n[GNUPG:] SIG_CREATED ");
- strbuf_release(&gpg_status);
- if (ret)
+ if (ret || signature->len == bottom)
return error(_("gpg failed to sign the data"));
/* Strip CR from the line endings, in case we are on Windows. */
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 8b0f71a..f9b7d79 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1202,17 +1202,10 @@ test_expect_success GPG,RFC1991 \
# try to sign with bad user.signingkey
git config user.signingkey BobTheMouse
test_expect_success GPG \
- 'git tag -s fails if gpg is misconfigured (bad key)' \
+ 'git tag -s fails if gpg is misconfigured' \
'test_must_fail git tag -s -m tail tag-gpg-failure'
git config --unset user.signingkey
-# try to produce invalid signature
-test_expect_success GPG \
- 'git tag -s fails if gpg is misconfigured (bad signature format)' \
- 'test_config gpg.program echo &&
- test_must_fail git tag -s -m tail tag-gpg-failure'
-
-
# try to verify without gpg:
rm -rf gpghome
--
2.10.0.windows.1.6.gc4f481a
base-commit: 6ebdac1bab966b720d776aa43ca188fe378b1f4b
^ permalink raw reply related
* Re: Context Menu is missing
From: stefan.naewe @ 2016-09-06 7:54 UTC (permalink / raw)
To: idan; +Cc: git
In-Reply-To: <CAFfNYUmBaydosF8vYwfj1+px3uUfiH4F=jgdCYBdzEw3-5FUGQ@mail.gmail.com>
(Please, please, please, use "reply all" in your mail reader i.e. make sure you don't
remove 'git@vger.kernel.org' from the "To:" or "CC:" field. Thank you!)
Am 06.09.2016 um 09:47 schrieb Idan Shimoni:
> On Tue, Sep 6, 2016 at 10:33 AM, <stefan.naewe@atlas-elektronik.com> wrote:
>> (Please don't top post and do "reply all")
>>
> I tried but you are receiving only plain text emails.
> anyway...
??? ECANNOTUNDERSTAND
Read about top-posting here: https://en.wikipedia.org/wiki/Posting_style#Top-posting
> I reinstalled windows on my computer and then installed Git version
> 2.9.3 for windows.
> And the context menu were missing.
In the explorer, I guess ?
> I am talking about the one that you had:
> - Git History
> - Git Branch
> - branch_1
> - branch_2
> ....
Git for windows *doesn't* install that.
> Git GUI and Git Bash are still there...
Git for windows *does* install that.
Maybe you had TortoiseGit installed before (just a wild guess, though)
Stefan
--
----------------------------------------------------------------
/dev/random says: Gambling: The sure way of getting nothing for something.
python -c "print '73746566616e2e6e616577654061746c61732d656c656b74726f6e696b2e636f6d'.decode('hex')"
GPG Key fingerprint = 2DF5 E01B 09C3 7501 BCA9 9666 829B 49C5 9221 27AF
^ permalink raw reply
* Re: Context Menu is missing
From: stefan.naewe @ 2016-09-06 7:33 UTC (permalink / raw)
To: idan; +Cc: git
In-Reply-To: <CAFfNYUk4QWii7AyG7u2B8G-JGT1Dn0OTopM=FTgaA7DBjYPhfA@mail.gmail.com>
(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, <stefan.naewe@atlas-elektronik.com> 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
^ permalink raw reply
* Re: [PATCH] t6026-merge-attr: wait for process to release trash directory
From: Jeff King @ 2016-09-06 7:27 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Johannes Schindelin, Git Mailing List
In-Reply-To: <3fd38b71-26bd-bdb3-fe5c-e7038abf51e0@kdbg.org>
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
^ permalink raw reply
* Re: [PATCH] t6026-merge-attr: wait for process to release trash directory
From: Johannes Schindelin @ 2016-09-06 7:25 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Git Mailing List
In-Reply-To: <3fd38b71-26bd-bdb3-fe5c-e7038abf51e0@kdbg.org>
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" &&
^ permalink raw reply related
* Re: Context Menu is missing
From: stefan.naewe @ 2016-09-06 7:23 UTC (permalink / raw)
To: idan, git
In-Reply-To: <CAFfNYUmMdamkW=Ca-iRDJ35ucr=S8A_qLTmjoD5j1M_ohdCgSg@mail.gmail.com>
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
^ permalink raw reply
* Re: [PATCH] t9903: fix broken && chain
From: Jeff King @ 2016-09-06 7:15 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Git Mailing List
In-Reply-To: <61de943c-df93-58e9-ca48-c469a71a1d43@kdbg.org>
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
^ permalink raw reply
* Re: [PATCH 0/3] Fix a segfault caused by regexec() being called on mmap()ed data
From: Jeff King @ 2016-09-06 7:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, git
In-Reply-To: <xmqqwpiqp3ho.fsf@gitster.mtv.corp.google.com>
On Mon, Sep 05, 2016 at 12:10:11PM -0700, Junio C Hamano wrote:
> * We could use <ptr,len> variant of regexp engine as you proposed,
> which I think is a preferrable solution. Do people know of a
> widely accepted implementation that we can throw into compat/ as
> fallback that is compatible with GPLv2?
Maybe the one already in compat/regex? ;P
I think re_search() the correct replacement function but it's been a
while since I've looked into it.
-Peff
^ permalink raw reply
* Re: 2.10.0: multiple versionsort.prereleasesuffix buggy?
From: Leho Kraav (Conversion Ready) @ 2016-09-06 7:12 UTC (permalink / raw)
To: SZEDER Gábor, peff; +Cc: git
In-Reply-To: <20160906010759.14883-1-szeder@ira.uka.de>
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.
^ permalink raw reply
* Context Menu is missing
From: Idan Shimoni @ 2016-09-06 7:12 UTC (permalink / raw)
To: git
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
^ permalink raw reply
* Re: [PATCH 3/3] diff_grep: add assertions verifying that the buffers are NUL-terminated
From: Jeff King @ 2016-09-06 7:08 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano
In-Reply-To: <a264f55769cd6cfedb842710791d71b11d830091.1473090278.git.johannes.schindelin@gmx.de>
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
^ permalink raw reply
* Re: [PATCH 2/3] diff_populate_filespec: NUL-terminate buffers
From: Jeff King @ 2016-09-06 7:06 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano
In-Reply-To: <f899957fa71537aa2686f17ce18aaf16f2fea2ac.1473090278.git.johannes.schindelin@gmx.de>
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).
^ permalink raw reply
* [PATCHv4] diff.c: emit moved lines with a different color
From: Stefan Beller @ 2016-09-06 7:01 UTC (permalink / raw)
To: git; +Cc: jnareb, gitster, jacob.keller, Stefan Beller, Stefan Beller
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 <sbeller@google.com>
---
* moved new data structures into struct diff_options
* color.moved=bool as well as --[no-]color-moved to {dis,en}able the new feature
* color.diff.movedfrom and color.diff.movedto to control the colors
* added a test
Documentation/config.txt | 12 +-
Documentation/diff-options.txt | 7 ++
contrib/completion/git-completion.bash | 2 +
diff.c | 211 +++++++++++++++++++++++++++++++--
diff.h | 16 ++-
t/t4015-diff-whitespace.sh | 44 +++++++
6 files changed, 282 insertions(+), 10 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 0bcb679..5daf77a 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -974,14 +974,22 @@ This does not affect linkgit:git-format-patch[1] or the
'git-diff-{asterisk}' plumbing commands. Can be overridden on the
command line with the `--color[=<when>]` option.
+color.moved::
+ A boolean value, whether a diff should color moved lines
+ differently. The moved lines are searched for in the diff only.
+ Duplicated lines from somewhere in the project that are not
+ part of the diff are not colored as moved.
+ Defaults to true.
+
color.diff.<slot>::
Use customized color for diff colorization. `<slot>` specifies
which part of the patch to use the specified color, and is one
of `context` (context text - `plain` is a historical synonym),
`meta` (metainformation), `frag`
(hunk header), 'func' (function in hunk header), `old` (removed lines),
- `new` (added lines), `commit` (commit headers), or `whitespace`
- (highlighting whitespace errors).
+ `new` (added lines), `commit` (commit headers), `whitespace`
+ (highlighting whitespace errors), `movedfrom` (removed lines that
+ reappear), `movedto` (added lines that were removed elsewhere).
color.decorate.<slot>::
Use customized color for 'git log --decorate' output. `<slot>` is one
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 705a873..13b6a2a 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -234,6 +234,13 @@ ifdef::git-diff[]
endif::git-diff[]
It is the same as `--color=never`.
+--[no-]color-moved::
+ Show moved blocks in a different color.
+ifdef::git-diff[]
+ It can be changed by the `diff.ui` and `color.diff`
+ configuration settings.
+endif::git-diff[]
+
--word-diff[=<mode>]::
Show a word diff, using the <mode> to delimit changed words.
By default, words are delimited by whitespace; see
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 9c8f738..9827c2e 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2115,6 +2115,8 @@ _git_config ()
color.diff.old
color.diff.plain
color.diff.whitespace
+ color.diff.movedfrom
+ color.diff.movedto
color.grep
color.grep.context
color.grep.filename
diff --git a/diff.c b/diff.c
index 534c12e..47685f3 100644
--- a/diff.c
+++ b/diff.c
@@ -18,6 +18,7 @@
#include "ll-merge.h"
#include "string-list.h"
#include "argv-array.h"
+#include "git-compat-util.h"
#ifdef NO_FAST_WORKING_DIRECTORY
#define FAST_WORKING_DIRECTORY 0
@@ -30,6 +31,7 @@ static int diff_compaction_heuristic; /* experimental */
static int diff_rename_limit_default = 400;
static int diff_suppress_blank_empty;
static int diff_use_color_default = -1;
+static int diff_color_moved_default = -1;
static int diff_context_default = 3;
static const char *diff_word_regex_cfg;
static const char *external_diff_cmd_cfg;
@@ -52,6 +54,8 @@ static char diff_colors[][COLOR_MAXLEN] = {
GIT_COLOR_YELLOW, /* COMMIT */
GIT_COLOR_BG_RED, /* WHITESPACE */
GIT_COLOR_NORMAL, /* FUNCINFO */
+ GIT_COLOR_BLUE, /* MOVED FROM */
+ GIT_COLOR_MAGENTA, /* MOVED TO */
};
static int parse_diff_color_slot(const char *var)
@@ -72,6 +76,10 @@ static int parse_diff_color_slot(const char *var)
return DIFF_WHITESPACE;
if (!strcasecmp(var, "func"))
return DIFF_FUNCINFO;
+ if (!strcasecmp(var, "movedfrom"))
+ return DIFF_FILE_MOVED_FROM;
+ if (!strcasecmp(var, "movedto"))
+ return DIFF_FILE_MOVED_TO;
return -1;
}
@@ -180,6 +188,10 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)
diff_use_color_default = git_config_colorbool(var, value);
return 0;
}
+ if (!strcmp(var, "color.moved")) {
+ diff_color_moved_default = git_config_bool(var, value);
+ return 0;
+ }
if (!strcmp(var, "diff.context")) {
diff_context_default = git_config_int(var, value);
if (diff_context_default < 0)
@@ -287,6 +299,26 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)
return git_default_config(var, value, cb);
}
+static int diff_line_moved_entry_cmp(const struct diff_line_moved_entry *a,
+ const struct diff_line_moved_entry *b,
+ const void *unused)
+{
+ return strcmp(a->line, b->line) &&
+ a->hash_prev_line == b->hash_prev_line;
+}
+
+static struct diff_line_moved_entry *
+prepare_diff_line_moved_entry(const char *line,
+ unsigned long len,
+ int hash_prev_line)
+{
+ struct diff_line_moved_entry *ret = xmalloc(sizeof(*ret));
+ ret->ent.hash = memhash(line, len) ^ hash_prev_line;
+ ret->line = xmemdupz(line, len);
+ ret->hash_prev_line = hash_prev_line;
+ return ret;
+}
+
static char *quote_two(const char *one, const char *two)
{
int need_one = quote_c_style(one, NULL, NULL, 1);
@@ -537,16 +569,42 @@ static void emit_add_line(const char *reset,
struct emit_callback *ecbdata,
const char *line, int len)
{
+ enum color_diff color = DIFF_FILE_NEW;
+ unsigned ws_error_highlight = WSEH_NEW;
+
+ if (ecbdata->opt->color_moved) {
+ struct diff_options *o = ecbdata->opt;
+ int hash = memhash(line, len);
+ struct diff_line_moved_entry *keydata =
+ prepare_diff_line_moved_entry(line, len,
+ o->hash_prev_added);
+ if (hashmap_get(o->moved_del, keydata, keydata))
+ color = DIFF_FILE_MOVED_TO;
+ o->hash_prev_added = hash;
+ }
emit_line_checked(reset, ecbdata, line, len,
- DIFF_FILE_NEW, WSEH_NEW, '+');
+ color, ws_error_highlight, '+');
}
static void emit_del_line(const char *reset,
struct emit_callback *ecbdata,
const char *line, int len)
{
+ enum color_diff color = DIFF_FILE_OLD;
+ unsigned ws_error_highlight = WSEH_OLD;
+
+ if (ecbdata->opt->color_moved) {
+ struct diff_options *o = ecbdata->opt;
+ int hash = memhash(line, len);
+ struct diff_line_moved_entry *keydata =
+ prepare_diff_line_moved_entry(line, len,
+ o->hash_prev_removed);
+ if (hashmap_get(ecbdata->opt->moved_add, keydata, keydata))
+ color = DIFF_FILE_MOVED_FROM;
+ o->hash_prev_removed = hash;
+ }
emit_line_checked(reset, ecbdata, line, len,
- DIFF_FILE_OLD, WSEH_OLD, '-');
+ color, ws_error_highlight, '-');
}
static void emit_context_line(const char *reset,
@@ -555,6 +613,11 @@ static void emit_context_line(const char *reset,
{
emit_line_checked(reset, ecbdata, line, len,
DIFF_CONTEXT, WSEH_CONTEXT, ' ');
+ if (ecbdata->opt->color_moved) {
+ int h = memhash(line, len);
+ ecbdata->opt->hash_prev_removed = h;
+ ecbdata->opt->hash_prev_added = h;
+ }
}
static void emit_hunk_header(struct emit_callback *ecbdata,
@@ -1323,6 +1386,47 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
}
}
+static void fn_prepare_consume(void *priv, char *line, unsigned long len)
+{
+ struct emit_callback *ecbdata = priv;
+ struct diff_options *o = ecbdata->opt;
+ struct diff_line_moved_entry *d;
+ int hash = memhash(line + 1, len - 1);
+
+ switch (line[0]) {
+ case ' ':
+ hashmap_add(o->moved_del,
+ prepare_diff_line_moved_entry(line + 1, len - 1, 0));
+ hashmap_add(o->moved_add,
+ prepare_diff_line_moved_entry(line + 1, len - 1, 0));
+ o->hash_prev_added = hash;
+ o->hash_prev_removed = hash;
+ break;
+ case '+':
+ if (o->hash_prev_added) {
+ d = prepare_diff_line_moved_entry(line + 1, len - 1,
+ o->hash_prev_added);
+ hashmap_add(o->moved_add, d);
+ }
+ o->hash_prev_added = hash;
+ o->hash_prev_removed = 0;
+ break;
+ case '-':
+ if (o->hash_prev_removed) {
+ d = prepare_diff_line_moved_entry(line + 1, len - 1,
+ o->hash_prev_removed);
+ hashmap_add(o->moved_del, d);
+ }
+ o->hash_prev_added = 0;
+ o->hash_prev_removed = hash;
+ break;
+ default:
+ o->hash_prev_added = 0;
+ o->hash_prev_removed = 0;
+ break;
+ }
+}
+
static char *pprint_rename(const char *a, const char *b)
{
const char *old = a;
@@ -2279,6 +2383,57 @@ struct userdiff_driver *get_textconv(struct diff_filespec *one)
return userdiff_get_textconv(one->driver);
}
+static void prepare_moved_lines(struct diff_filepair *p, struct diff_options *o)
+{
+ mmfile_t mf1, mf2;
+ xpparam_t xpp;
+ xdemitconf_t xecfg;
+ struct emit_callback ecbdata;
+ struct diff_filespec *one = p->one;
+ struct diff_filespec *two = p->two;
+ struct userdiff_driver *textconv_one = NULL;
+ struct userdiff_driver *textconv_two = NULL;
+
+ if (S_ISGITLINK(one->mode) ||
+ S_ISGITLINK(two->mode))
+ return;
+
+ if (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) {
+ textconv_one = get_textconv(one);
+ textconv_two = get_textconv(two);
+ }
+
+ if (!DIFF_OPT_TST(o, TEXT) &&
+ ( (!textconv_one && diff_filespec_is_binary(one)) ||
+ (!textconv_two && diff_filespec_is_binary(two)) ))
+ return;
+
+ mf1.size = fill_textconv(textconv_one, one, &mf1.ptr);
+ mf2.size = fill_textconv(textconv_two, two, &mf2.ptr);
+
+ memset(&xpp, 0, sizeof(xpp));
+ memset(&xecfg, 0, sizeof(xecfg));
+ memset(&ecbdata, 0, sizeof(ecbdata));
+ ecbdata.opt = o;
+
+ xpp.flags = o->xdl_opts;
+ xecfg.ctxlen = 1;
+
+ if (o->word_diff)
+ init_diff_words_data(&ecbdata, o, one, two);
+ if (xdi_diff_outf(&mf1, &mf2, fn_prepare_consume, &ecbdata,
+ &xpp, &xecfg))
+ die("unable to generate generate moved color output for %s",
+ one->path);
+ if (o->word_diff)
+ free_diff_words_data(&ecbdata);
+ if (textconv_one)
+ free(mf1.ptr);
+ if (textconv_two)
+ free(mf2.ptr);
+ xdiff_clear_find_func(&xecfg);
+}
+
static void builtin_diff(const char *name_a,
const char *name_b,
struct diff_filespec *one,
@@ -3291,6 +3446,7 @@ void diff_setup(struct diff_options *options)
options->change = diff_change;
options->add_remove = diff_addremove;
options->use_color = diff_use_color_default;
+ options->color_moved = diff_color_moved_default;
options->detect_rename = diff_detect_rename_default;
options->xdl_opts |= diff_algorithm;
if (diff_compaction_heuristic)
@@ -3413,6 +3569,9 @@ void diff_setup_done(struct diff_options *options)
if (DIFF_OPT_TST(options, FOLLOW_RENAMES) && options->pathspec.nr != 1)
die(_("--follow requires exactly one pathspec"));
+
+ if (!options->use_color || external_diff())
+ options->color_moved = 0;
}
static int opt_arg(const char *arg, int arg_short, const char *arg_long, int *val)
@@ -3863,6 +4022,10 @@ int diff_opt_parse(struct diff_options *options,
}
else if (!strcmp(arg, "--no-color"))
options->use_color = 0;
+ else if (!strcmp(arg, "--color-moved"))
+ options->color_moved = 1;
+ else if (!strcmp(arg, "--no-color-moved"))
+ options->color_moved = 0;
else if (!strcmp(arg, "--color-words")) {
options->use_color = 1;
options->word_diff = DIFF_WORDS_COLOR;
@@ -4622,6 +4785,43 @@ void diff_warn_rename_limit(const char *varname, int needed, int degraded_cc)
warning(rename_limit_advice, varname, needed);
}
+static void diff_flush_format_patch(struct diff_options *o)
+{
+ int i;
+ struct diff_queue_struct *q = &diff_queued_diff;
+ if (o->color_moved) {
+ o->moved_add = xmalloc(sizeof(*o->moved_add));
+ o->moved_del = xmalloc(sizeof(*o->moved_del));
+ hashmap_init(o->moved_add, (hashmap_cmp_fn)diff_line_moved_entry_cmp, 0);
+ hashmap_init(o->moved_del, (hashmap_cmp_fn)diff_line_moved_entry_cmp, 0);
+
+ for (i = 0; i < q->nr; i++) {
+ struct diff_filepair *p = q->queue[i];
+ if (check_pair_status(p) && !diff_unmodified_pair(p))
+ prepare_moved_lines(p, o);
+ }
+ }
+
+ for (i = 0; i < q->nr; i++) {
+ struct diff_filepair *p = q->queue[i];
+ if (check_pair_status(p))
+ diff_flush_patch(p, o);
+ }
+
+ if (o->color_moved) {
+ struct hashmap_iter iter;
+ struct diff_line_moved_entry *e;
+ hashmap_iter_init(o->moved_add, &iter);
+ while ((e = hashmap_iter_next(&iter)))
+ free(e->line);
+ hashmap_iter_init(o->moved_del, &iter);
+ while ((e = hashmap_iter_next(&iter)))
+ free(e->line);
+ hashmap_free(o->moved_add, 1);
+ hashmap_free(o->moved_del, 1);
+ }
+}
+
void diff_flush(struct diff_options *options)
{
struct diff_queue_struct *q = &diff_queued_diff;
@@ -4696,6 +4896,7 @@ void diff_flush(struct diff_options *options)
if (!options->file)
die_errno("Could not open /dev/null");
options->close_file = 1;
+ options->color_moved = 0;
for (i = 0; i < q->nr; i++) {
struct diff_filepair *p = q->queue[i];
if (check_pair_status(p))
@@ -4716,11 +4917,7 @@ void diff_flush(struct diff_options *options)
}
}
- for (i = 0; i < q->nr; i++) {
- struct diff_filepair *p = q->queue[i];
- if (check_pair_status(p))
- diff_flush_patch(p, options);
- }
+ diff_flush_format_patch(options);
}
if (output_format & DIFF_FORMAT_CALLBACK)
diff --git a/diff.h b/diff.h
index 7883729..236baa8 100644
--- a/diff.h
+++ b/diff.h
@@ -110,6 +110,12 @@ enum diff_words_type {
DIFF_WORDS_COLOR
};
+struct diff_line_moved_entry {
+ struct hashmap_entry ent;
+ char *line;
+ int hash_prev_line;
+};
+
struct diff_options {
const char *orderfile;
const char *pickaxe;
@@ -178,6 +184,12 @@ struct diff_options {
void *output_prefix_data;
int diff_path_counter;
+
+ int color_moved;
+ struct hashmap *moved_add;
+ struct hashmap *moved_del;
+ int hash_prev_added;
+ int hash_prev_removed;
};
enum color_diff {
@@ -189,7 +201,9 @@ enum color_diff {
DIFF_FILE_NEW = 5,
DIFF_COMMIT = 6,
DIFF_WHITESPACE = 7,
- DIFF_FUNCINFO = 8
+ DIFF_FUNCINFO = 8,
+ DIFF_FILE_MOVED_TO = 9,
+ DIFF_FILE_MOVED_FROM = 10
};
const char *diff_get_color(int diff_use_color, enum color_diff ix);
#define diff_get_color_opt(o, ix) \
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 2434157..9fdc5bd 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -934,4 +934,48 @@ test_expect_success 'the same with --ws-error-highlight' '
test_cmp expected current
'
+test_expect_success 'detect moved code' '
+ git reset --hard &&
+ cat >test.c <<-\EOF &&
+
+ #include<stdio.h>
+ main()
+ {
+ printf("Hello World");
+ }
+ EOF
+ git add test.c &&
+ git commit -m "add main function" &&
+ git mv test.c main.c &&
+ git diff HEAD --no-renames | test_decode_color >actual &&
+ cat >expected <<-\EOF &&
+ <BOLD>diff --git a/main.c b/main.c<RESET>
+ <BOLD>new file mode 100644<RESET>
+ <BOLD>index 0000000..f068530<RESET>
+ <BOLD>--- /dev/null<RESET>
+ <BOLD>+++ b/main.c<RESET>
+ <CYAN>@@ -0,0 +1,6 @@<RESET>
+ <GREEN>+<RESET>
+ <BLUE>+<RESET><BLUE>#include<stdio.h><RESET>
+ <BLUE>+<RESET><BLUE>main()<RESET>
+ <BLUE>+<RESET><BLUE>{<RESET>
+ <BLUE>+<RESET><BLUE>printf("Hello World");<RESET>
+ <BLUE>+<RESET><BLUE>}<RESET>
+ <BOLD>diff --git a/test.c b/test.c<RESET>
+ <BOLD>deleted file mode 100644<RESET>
+ <BOLD>index f068530..0000000<RESET>
+ <BOLD>--- a/test.c<RESET>
+ <BOLD>+++ /dev/null<RESET>
+ <CYAN>@@ -1,6 +0,0 @@<RESET>
+ <RED>-<RESET>
+ <MAGENTA>-#include<stdio.h><RESET>
+ <MAGENTA>-main()<RESET>
+ <MAGENTA>-{<RESET>
+ <MAGENTA>-printf("Hello World");<RESET>
+ <MAGENTA>-}<RESET>
+ EOF
+
+ test_cmp expected actual
+'
+
test_done
--
2.10.0.1.g233b7f3.dirty
^ permalink raw reply related
* Re: [PATCHv3] diff.c: emit moved lines with a different color
From: Jacob Keller @ 2016-09-06 6:58 UTC (permalink / raw)
To: Stefan Beller
Cc: Stefan Beller, Git mailing list, Jakub Narębski,
Junio C Hamano
In-Reply-To: <CAGZ79ka7VV6Pqw-rcVFv0xU4J4D9dHWPg-oocPPWJdY5JVhT+A@mail.gmail.com>
On Mon, Sep 5, 2016 at 7:20 PM, Stefan Beller <sbeller@google.com> wrote:
>> If I understand, this is to ensure that we don't keep re-hashing each
>> line right?
>
> No, this is to ensure we have the context sensitivity of one prior line.
>
> In the collection phase we look at each line of the patch and make a hash of it.
> Then we store the hash temporarily (think of a state machine that goes line by
> line and always keeps the hash of the last line)
>
> What we store in the hashmaps is the hash(current line) ^
> hash(previous applicable line).
> With previous applicable line I mean any line starting with " " or "+"
> when the current
> line starts with "+" and " " or "-" when the current line starts with "-".
>
> When going through the second pass and actually emitting colored lines
> we only find matches in the hash map if the current line AND the previous line
> match as we lookup by hash code, i.e. if we have a moved line, but the
> previous line
> changed we do not find it in the hashmap and we don't color it, such
> that the reviewer
> can spot a permutation.
>
> So in the second phase we also need to have access to previous line, so maybe
> we could also go with just taking the line with us instead of 2 hash codes.
> But that implementation detail seems like a trade off to me, where I'd lean
> on keeping the hashes around as lines may be very long in bad cases, whereas
> the hashcode is short and it is a cheap hash.
>
> (I am referring to http://i.imgur.com/MnaSZ1D.png where in the malicious
> case all lines were moved to there as well, but permutated)
Good that clears up a lot of my questions. It seems like a reasonable
algorithm, and I like the example of per-muted lines.
I think if we can find some real-world data on how much slower this
is, we can find out what kind of configuration we want to give it? It
seems like something incredibly helpful to me, but having a way out is
nice incase it ends up being too costly. I suspect that will only be
the case for a large patch.
Thanks,
Jake
^ permalink raw reply
* Re: [PATCH 0/3] Fix a segfault caused by regexec() being called on mmap()ed data
From: Jeff King @ 2016-09-06 6:58 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano
In-Reply-To: <cover.1473090278.git.johannes.schindelin@gmx.de>
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.
> So at least I have a workaround in place. Ideally, though, we would
> NUL-terminate the buffers only when needed, or somehow call regexec() on
> ptr/size parameters instead of passing a supposedly NUL-terminated
> string to it?
There's some discussion in:
http://public-inbox.org/git/20121030121747.GA4231@sigill.intra.peff.net/#r
and the thread below it. The quickest way to fix regexec() would be to
have everybody use the built-in GNU regex in compat/. People seemed
somewhat positive on that direction, but we never followed up.
-Peff
^ permalink raw reply
* Re: 2.10.0: multiple versionsort.prereleasesuffix buggy?
From: Jeff King @ 2016-09-06 4:07 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: git, leho
In-Reply-To: <20160906010759.14883-1-szeder@ira.uka.de>
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.
-Peff
^ permalink raw reply
* Re: [PATCHv3] diff.c: emit moved lines with a different color
From: Stefan Beller @ 2016-09-06 2:20 UTC (permalink / raw)
To: Jacob Keller
Cc: Stefan Beller, Git mailing list, Jakub Narębski,
Junio C Hamano
In-Reply-To: <CA+P7+xr=Lp36wDw2BRuhQ9Genv4q6iu9beGQe5hzOn5PJYDPfw@mail.gmail.com>
On Mon, Sep 5, 2016 at 6:17 PM, Jacob Keller <jacob.keller@gmail.com> wrote:
> On Sun, Sep 4, 2016 at 4:42 PM, Stefan Beller <stefanbeller@gmail.com> wrote:
>> When we color the diff, we'll mark moved lines with a different color.
>>
>
> Excellent idea. This is a very neat way to show extra information
> without cluttering the diff output.
>
>> 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`.
>>
>
> Makes sense.
>
>> 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.
>
> Also makes sense.
>
>>
>> This patch was motivated by e.g. reviewing 3b0c4200 ("apply: move
>> libified code from builtin/apply.c to apply.{c,h}", 2016-08-08)
>>
>
> Yes, this would be quite helpful.
>
>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> index 0bcb679..f4f51c2 100644
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>> @@ -980,8 +980,9 @@ color.diff.<slot>::
>> 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), `moved-old` (removed lines that
>> + reappear), `moved-new` (added lines that were removed elsewhere).
>>
>
> I liked Junio's "Moved from" and "moved to" but I think moved old and
> moved new are ok as well.
as we do not want to see dashes ('moved-old'), I think I'l go with
"movedfrom" and "movedto".
>
>> @@ -287,6 +304,25 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)
>> return git_default_config(var, value, cb);
>> }
>>
>> +static int moved_entry_cmp(const struct moved_entry *a,
>> + const struct moved_entry *b,
>> + const void *unused)
>> +{
>> + return strcmp(a->line, b->line) &&
>> + a->hash_prev_line == b->hash_prev_line;
>
> So we're only comparing them if they match and have a matching
> previous line? That seems pretty reasonable to reduce the cost of
> computing exact copied sequences.
>
>> + if (ecbdata->opt->color_moved) {
>> + int h = memhash(line, len);
>> + hash_prev_removed = h;
>> + hash_prev_added = h;
>> + }
>> }
>
> If I understand, this is to ensure that we don't keep re-hashing each
> line right?
No, this is to ensure we have the context sensitivity of one prior line.
In the collection phase we look at each line of the patch and make a hash of it.
Then we store the hash temporarily (think of a state machine that goes line by
line and always keeps the hash of the last line)
What we store in the hashmaps is the hash(current line) ^
hash(previous applicable line).
With previous applicable line I mean any line starting with " " or "+"
when the current
line starts with "+" and " " or "-" when the current line starts with "-".
When going through the second pass and actually emitting colored lines
we only find matches in the hash map if the current line AND the previous line
match as we lookup by hash code, i.e. if we have a moved line, but the
previous line
changed we do not find it in the hashmap and we don't color it, such
that the reviewer
can spot a permutation.
So in the second phase we also need to have access to previous line, so maybe
we could also go with just taking the line with us instead of 2 hash codes.
But that implementation detail seems like a trade off to me, where I'd lean
on keeping the hashes around as lines may be very long in bad cases, whereas
the hashcode is short and it is a cheap hash.
(I am referring to http://i.imgur.com/MnaSZ1D.png where in the malicious
case all lines were moved to there as well, but permutated)
^ permalink raw reply
* Re: [WIP PATCH v2] diff.c: emit moved lines with a different color
From: Stefan Beller @ 2016-09-06 2:09 UTC (permalink / raw)
To: Jacob Keller
Cc: Junio C Hamano, Stefan Beller, Git mailing list,
Jakub Narębski
In-Reply-To: <CA+P7+xqx4PHHoKT_PK5s5eOtZMG9QJTSHED_=+BHZ=U330AQtw@mail.gmail.com>
On Mon, Sep 5, 2016 at 6:09 PM, Jacob Keller <jacob.keller@gmail.com> wrote:
> On Mon, Sep 5, 2016 at 11:57 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Stefan Beller <stefanbeller@gmail.com> writes:
>>
>>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>>> index 0bcb679..f4f51c2 100644
>>> --- a/Documentation/config.txt
>>> +++ b/Documentation/config.txt
>>> @@ -980,8 +980,9 @@ color.diff.<slot>::
>>> 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), `moved-old` (removed lines that
>>> + reappear), `moved-new` (added lines that were removed elsewhere).
>>
>> Could we have a config to disable this rather costly new feature,
>> too?
>
> That seems entirely reasonable, though we *do* have a configuration
> for disabling color altogether.. is there any numbers on how much more
> this costs to compute?
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.
>
>>> +static struct hashmap *duplicates_added;
>>> +static struct hashmap *duplicates_removed;
>>> +static int hash_previous_line_added;
>>> +static int hash_previous_line_removed;
>>
>> I think these should be added as new fields to diff_options
>> structure.
>
> Agreed, those seem like good choices for diff_options.
yup.
^ permalink raw reply
* Re: [WIP PATCH v2] diff.c: emit moved lines with a different color
From: Stefan Beller @ 2016-09-06 2:07 UTC (permalink / raw)
To: Junio C Hamano
Cc: Stefan Beller, git@vger.kernel.org, Jakub Narębski,
Jacob Keller
In-Reply-To: <xmqq1t0yqinr.fsf@gitster.mtv.corp.google.com>
On Mon, Sep 5, 2016 at 11:57 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> + `new` (added lines), `commit` (commit headers), `whitespace`
>> + (highlighting whitespace errors), `moved-old` (removed lines that
>> + reappear), `moved-new` (added lines that were removed elsewhere).
>
> Could we have a config to disable this rather costly new feature,
> too?
And by config option you mean both a command line parameter
`--color=yes-but-no-move-detection` as well as a diff.color config option?
As it is currently `--color=<when>` with when={always, never,auto},
I don't think we want to add it as another parameter there, so maybe
--color-type=<style> with style={minimal, full} whereas the minimal/full
describes the amount of work needed. Though I think that is bad as there
might be other orthogonal features to this.
So maybe just a `--[no-]color-moved` ?
As a config option we'd go with color.moved=<bool> for now?
I imagine we may want to refine the moved detection algorithm in the
future, e.g. moved just in the patch, or moved from elsewhere in the
repo or whether the moved detection takes permutations into account
etc, so actually we'd want to have color.moved={none, this-patch} for
now. The command line parameter --color-moved=<style> would be the
same.
>
> Also the first and the third level configuration names (the <slot>
> is at the third level) used by the core-git do not use dashed-words
> format. Please adhere to the current convention.
will do.
>
>> diff --git a/diff.c b/diff.c
>> index 534c12e..d37cb4f 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
>> @@ -42,6 +43,11 @@ static int diff_dirstat_permille_default = 30;
>> static struct diff_options default_diff_options;
>> static long diff_algorithm;
>>
>> +static struct hashmap *duplicates_added;
>> +static struct hashmap *duplicates_removed;
>> +static int hash_previous_line_added;
>> +static int hash_previous_line_removed;
>
> I think these should be added as new fields to diff_options
> structure.
Makes sense.
^ permalink raw reply
* Re: [PATCHv3] diff.c: emit moved lines with a different color
From: Jacob Keller @ 2016-09-06 1:17 UTC (permalink / raw)
To: Stefan Beller
Cc: Git mailing list, Jakub Narębski, Junio C Hamano,
Stefan Beller
In-Reply-To: <20160904234237.10548-1-stefanbeller@gmail.com>
On Sun, Sep 4, 2016 at 4:42 PM, Stefan Beller <stefanbeller@gmail.com> wrote:
> When we color the diff, we'll mark moved lines with a different color.
>
Excellent idea. This is a very neat way to show extra information
without cluttering the diff output.
> 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`.
>
Makes sense.
> 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.
Also makes sense.
>
> This patch was motivated by e.g. reviewing 3b0c4200 ("apply: move
> libified code from builtin/apply.c to apply.{c,h}", 2016-08-08)
>
Yes, this would be quite helpful.
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 0bcb679..f4f51c2 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -980,8 +980,9 @@ color.diff.<slot>::
> 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), `moved-old` (removed lines that
> + reappear), `moved-new` (added lines that were removed elsewhere).
>
I liked Junio's "Moved from" and "moved to" but I think moved old and
moved new are ok as well.
> @@ -287,6 +304,25 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)
> return git_default_config(var, value, cb);
> }
>
> +static int moved_entry_cmp(const struct moved_entry *a,
> + const struct moved_entry *b,
> + const void *unused)
> +{
> + return strcmp(a->line, b->line) &&
> + a->hash_prev_line == b->hash_prev_line;
So we're only comparing them if they match and have a matching
previous line? That seems pretty reasonable to reduce the cost of
computing exact copied sequences.
> + if (ecbdata->opt->color_moved) {
> + int h = memhash(line, len);
> + hash_prev_removed = h;
> + hash_prev_added = h;
> + }
> }
If I understand, this is to ensure that we don't keep re-hashing each
line right?
Thanks,
Jake
^ permalink raw reply
* Re: [WIP PATCH v2] diff.c: emit moved lines with a different color
From: Jacob Keller @ 2016-09-06 1:09 UTC (permalink / raw)
To: Junio C Hamano
Cc: Stefan Beller, Git mailing list, Jakub Narębski,
Stefan Beller
In-Reply-To: <xmqq1t0yqinr.fsf@gitster.mtv.corp.google.com>
On Mon, Sep 5, 2016 at 11:57 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <stefanbeller@gmail.com> writes:
>
>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> index 0bcb679..f4f51c2 100644
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>> @@ -980,8 +980,9 @@ color.diff.<slot>::
>> 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), `moved-old` (removed lines that
>> + reappear), `moved-new` (added lines that were removed elsewhere).
>
> Could we have a config to disable this rather costly new feature,
> too?
That seems entirely reasonable, though we *do* have a configuration
for disabling color altogether.. is there any numbers on how much more
this costs to compute?
>> +static struct hashmap *duplicates_added;
>> +static struct hashmap *duplicates_removed;
>> +static int hash_previous_line_added;
>> +static int hash_previous_line_removed;
>
> I think these should be added as new fields to diff_options
> structure.
Agreed, those seem like good choices for diff_options.
Thanks,
Jake
^ permalink raw reply
* Re: 2.10.0: multiple versionsort.prereleasesuffix buggy?
From: SZEDER Gábor @ 2016-09-06 1:07 UTC (permalink / raw)
To: peff; +Cc: git, leho, SZEDER Gábor
In-Reply-To: <20160905232145.fr46rgfb66eeymwp@sigill.intra.peff.net>
> On Tue, Sep 06, 2016 at 01:42:28AM +0300, Leho Kraav (Conversion Ready) wrote:
>
> > Here's the testing tree https://github.com/woothemes/woocommerce
> >
> > .git/config has:
> >
> > [versionsort]
> >
> >
> > prereleasesuffix = -beta
> > prereleasesuffix = -RC
> >
> > $ git tag -l --sort=version:refname
> > [...]
> > 2.6.0-RC1
> > 2.6.0-RC2
> > 2.6.0-beta-1
> > 2.6.0-beta-2
> > 2.6.0-beta-3
> > 2.6.0-beta-4
>
> 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).
versioncmp() consumes two tagnames up to the first different character
and then calls swap_prereleases() to try to match prerelease suffixes
starting at those characters. This works fine when comparing a
release with a prerelease, e.g. "2.6.0" and "2.6.0-RC1", because
swap_prereleases() gets "" and "-RC1" and the latter does match one of
the configured suffixes. However, when comparing two prereleases,
e.g. "2.6.0-beta1" and "2.6.0-RC1", then the '-' is consumed from both
tagnames because the first differing characters are 'b' and 'R', thus
swap_prereleases() gets "beta1" and "RC1", which, of course, don't
match any of the configured suffixes without the leading '-'.
It's way past my bedtime, so for the time being I can only come up
with a hacky configuration workaround that seems to deliver the
expected results:
[versionsort]
prereleasesuffix = beta
prereleasesuffix = -beta
prereleasesuffix = RC
prereleasesuffix = -RC
Best,
Gábor
^ permalink raw reply
* Re: [PATCH] stash: allow ref of a stash by index
From: Jeff King @ 2016-09-05 23:52 UTC (permalink / raw)
To: Øystein Walle
Cc: Aaron M Watson, Git, Jon Seymour, David Caldwell,
Ævar Arnfjörð Bjarmason, David Aguilar,
Alex Henrie
In-Reply-To: <CAFaJEqu-JUcwLjrQBk_huSa3DZfCf8O4eAZ=UgcXHzN=CLgtpw@mail.gmail.com>
On Mon, Sep 05, 2016 at 11:46:34PM +0200, Øystein Walle wrote:
> The bash-specific code is a no-go, so here's a way to do it in a way
> that I think is in line with Git's code style for shell scripts. I took
> the liberty of removing the '|| exit 1' since the rev is verified later
> on anyway, as can be seen in the last piece of context. That way the
> argument munging can be done at a later stage where we don't have to
> loop over multiple ones. The first rev-parse's purpose is just to apply
> --sq.
I wondered how that would impact the error message when there is no such
stash. It looks like rev-parse will still return the bogus name on
stdout, so we do not run afoul of the "No stash found" code path. We do
get:
$ git.compile stash show foobar
foobar is not a valid reference
instead of:
$ git stash show foobar
fatal: ambiguous argument 'foobar': unknown revision or path not in
the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'
But I do not see that as a big downside (I might even call it an
improvement).
-Peff
^ permalink raw reply
* Re: 2.10.0: multiple versionsort.prereleasesuffix buggy?
From: Jeff King @ 2016-09-05 23:21 UTC (permalink / raw)
To: Leho Kraav (Conversion Ready); +Cc: git
In-Reply-To: <beba7c55-3f18-66a2-acd9-77f2a5cc55a7@conversionready.com>
On Tue, Sep 06, 2016 at 01:42:28AM +0300, Leho Kraav (Conversion Ready) wrote:
> Here's the testing tree https://github.com/woothemes/woocommerce
>
> .git/config has:
>
> [versionsort]
>
>
> prereleasesuffix = -beta
> prereleasesuffix = -RC
>
> $ git tag -l --sort=version:refname
> [...]
> 2.6.0-RC1
> 2.6.0-RC2
> 2.6.0-beta-1
> 2.6.0-beta-2
> 2.6.0-beta-3
> 2.6.0-beta-4
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.
-Peff
^ permalink raw reply
* 2.10.0: multiple versionsort.prereleasesuffix buggy?
From: Leho Kraav (Conversion Ready) @ 2016-09-05 22:42 UTC (permalink / raw)
To: git
Hi all
Here's the testing tree https://github.com/woothemes/woocommerce
.git/config has:
[versionsort]
prereleasesuffix = -beta
prereleasesuffix = -RC
$ git tag -l --sort=version:refname
...
2.5.0-RC1
2.5.0-RC2
2.5.0-RC3
2.5.0-beta-1
2.5.0-beta-2
2.5.0-beta-3
2.5.0
2.5.1
2.5.2
2.5.3
2.5.4
2.5.5
2.6.0-RC1
2.6.0-RC2
2.6.0-beta-1
2.6.0-beta-2
2.6.0-beta-3
2.6.0-beta-4
2.6.0
2.6.1
2.6.2
2.6.3
2.6.4
Per documentation, I'm supposed to see something like
...
2.5.0-beta-1
2.5.0-beta-2
2.5.0-beta-3
2.5.0-RC1
2.5.0-RC2
2.5.0-RC3
2.5.0
...
No matter what I do in `.git/config`, RC goes up front. What's going on?
(Yes, this project's tag capitalization is messed up.)
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox