[PATCH 2/2] remote-curl: remove spurious period
From: Johannes Schindelin
We should not interrupt. sentences in the middle.
Signed-off-by: Johannes Schindelin
---
remote-curl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/remote-curl.c b/remote-curl.c
index 99b0bedc6..fb28309e8 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -714,7 +714,7 @@ retry:
} else if (use_gzip && 1024 < rpc->len) {
/* The client backend isn't giving us compressed data so
-* we can try to deflate it ourselves, this may save on.
+* we can try to deflate it ourselves, this may save on
* the transfer time.
*/
git_zstream stream;
--
gitgitgadget
Re: [PATCH 1/1] pull --rebase=: allow single-letter abbreviations for the type
Hi Junio, On Mon, 6 Aug 2018, Junio C Hamano wrote: > "Johannes Schindelin via GitGitGadget" > writes: > > > From: Johannes Schindelin > > > > Git for Windows' original 4aa8b8c8283 (Teach 'git pull' to handle > > --rebase=interactive, 2011-10-21) had support for the very convenient > > abbreviation > > > > git pull --rebase=i > > > > which was later lost when it was ported to the builtin `git pull`, and > > it was not introduced before the patch eventually made it into Git as > > f5eb87b98dd (pull: allow interactive rebase with --rebase=interactive, > > 2016-01-13). > > > > However, it is *really* a useful short hand for the occasional rebasing > > pull on branches that do not usually want to be rebased. > > > > So let's reintroduce this convenience, at long last. > > > > Signed-off-by: Johannes Schindelin > > --- > > Makes sense, whether this patch is adding back what in the past > existed only in the Windows port but lost, and whether the lossage > was long time ago or in just a few releases go, or it is adding a > complete new feature for that matter. This looks like a good > short-hand. > > Will queue. Thanks! Dscho
Re: [PATCH 4/4] line-log: convert an assertion to a full BUG() call
Hi Eric,
On Sun, 5 Aug 2018, Eric Sunshine wrote:
> On Sat, Aug 4, 2018 at 6:18 PM Johannes Schindelin via GitGitGadget
> wrote:
> > The assertion in question really indicates a bug, when triggered, so we
> > might just as well use the sanctioned method to report it.
> >
> > Signed-off-by: Johannes Schindelin
> > ---
> > diff --git a/line-log.c b/line-log.c
> > @@ -72,7 +72,9 @@ void range_set_append(struct range_set *rs, long a, long
> > b)
> > - assert(rs->nr == 0 || rs->ranges[rs->nr-1].end <= a);
> > + if (rs->nr > 0 && rs->ranges[rs->nr-1].end > a)
> > + BUG("append %ld-%ld, after %ld-%ld?!?", a, b,
> > + rs->ranges[rs->nr-1].start, rs->ranges[rs->nr-1].end);
>
> Although this appears to be a faithful translation of the assert() to
> BUG(), as mentioned by Andrei in his review of 3/4, the existing
> assert() seems to have an off-by-1 error, which means that the "> a"
> here really ought to be ">= a".
I think Andrei's assessment is wrong. The code could not test for that
earlier, as it did allow ranges to become "abutting" in the process, by
failing to merge them. So the invariant you talked about is more of an
invariant for the initial state.
My 3/4 would make that invariant heeded throughout the process.
I am still keen on keeping the invariants straight *without* resorting to
dirty tricks like calling sort_and_merge. Calling that function would just
make it easier for bugs to hide in this code.
> Given that this file is full of assert()'s, it doesn't necessarily
> make sense to convert only this one, so perhaps the patch should be
> dropped (since I'm guessing you don't want to convert the rest of
> them).
Sure, there are 18 of them, and you're right, I lack the time to convert
them.
Ciao,
Dscho
Re: [PATCH 2/4] line-log: adjust start/end of ranges individually
Hi Eric,
On Sun, 5 Aug 2018, Eric Sunshine wrote:
> On Sat, Aug 4, 2018 at 6:18 PM Johannes Schindelin via GitGitGadget
> wrote:
> > When traversing commits and adjusting the ranges, things can get really
> > tricky. For example, when the line range of interest encloses several
> > hunks of a commit, the line range can actually shrink.
> >
> > Currently, range_set_shift_diff() does not anticipate that scenario and
> > blindly adjusts start and end by the same offset ("shift" the range).
> > [...]
> > Let's fix this by adjusting the start and the end offsets individually.
> >
> > Signed-off-by: Johannes Schindelin
> > ---
> > diff --git a/line-log.c b/line-log.c
> > @@ -438,7 +438,13 @@ static void range_set_shift_diff(struct range_set *out,
> > - (target[j].end-target[j].start);
> > j++;
> > }
> > - range_set_append(out, src[i].start+offset,
> > src[i].end+offset);
> > + start_offset = offset;
> > + while (j < diff->target.nr && src[i].end > target[j].end) {
> > + offset += (parent[j].end-parent[j].start)
> > + - (target[j].end-target[j].start);
> > + j++;
> > + }
> > + range_set_append(out, src[i].start+start_offset,
> > src[i].end+offset);
>
> I'm still trying to wrap my head around the original code, so I'm not
> even at the point of being able to say if this fix is correct. What
> happens if the "start_offset" loop consumes all of 'j' before it even
> gets to the new loop?
First of all, it is not really the `start_offset` loop, but more like the
"end_offset loop": we are still adjusting `offset`, and that is what we
use to adjust the end of the current line range, while we take pains to
keep the offset that needs to be used for the start of said line range.
> Why does the new loop use '>' whereas the existing uses '>='?
As a mathematician, I usually think of intervals as inclusive only on one
end. So I intuitively use the non-inclusive comparison on the other end.
In this instance, the first loop tries to make sure that the offset to be
used for the start offset has taken into account all of the relevant diff
hunks (meaning: the diff hunks whose target lines start before src[i],
i.e. the current line range to adjust).
The second loop, however, tries to make sure that the offset to adjust the
*end* of the current line range.
Hence what I wrote.
But now that I think about it again, I am no longer sure that the first
loop (the one I did not change) is sound, to begin with. Let's imagine a
very simple case, where we try to adjust a start line, say, 100, and the
diff has a hunk with header `@@ -90,20 +90,40 @@`. So the start line lies
somewhere around a quarter into the post-image.
The current line-log code adjusts this by a very crude method: since the
line number lies between the post-image's start, it is adjusted by adding
the length of the pre-image and subtracting the length of the post image,
i.e. 20 - 40. The result is that we pretend that it came from line number
80, which is squarely outside the pre-image range.
That method of adjustment is appropriate for lines *outside* the
post-image range, of course. If we had looked at line 140 (which is 10
lines after the post-image), then it would be totally correct to say that
it came from line 120.
But that method is pretty obviously broken for any line that lies *within*
a post-image.
So I think the entire loop is in dear need of adjustment.
The question is: how should it be adjusted? The safest way would be to
adjust start and end of the line range differently, where start is pulled
back to the pre-image's start (in the above example, 90), and end is
pushed up to the pre-image's end (in the above example, 110).
Of course, this has a slightly unintuitive consequence: imagine that some
commit made a block of, say, 20 lines a conditional one. Like,
/* Some comment */
do_something = 1;
...
print_it();
...
could have become
if (!dry_run) {
/* Some comment */
do_something = 1;
...
print_it();
...
}
If you use my proposed method to adjust the line ranges to figure out the
history of that `print_it()` line, this commit would blow up the line
range to the entire conditional block, which is probably not what you
wanted.
I don't really see a better way, though.
> Having said that, a much easier fix is to use range_set_append_unsafe()
> here, and then at the bottom of the loop, invoke
> 'sort_and_merge_range_set(out)' to restore range-set invariants and
&g
Re: [PATCH 1/4] line-log: demonstrate a bug with nearly-overlapping ranges
Hi Jonathan,
On Sat, 4 Aug 2018, Jonathan Nieder wrote:
> Johannes Schindelin wrote:
>
> > Currently, this test case throws an assertion:
> >
> > Assertion failed!
> >
> > Program: git.exe
> > File: line-log.c, Line 71
> >
> > Signed-off-by: Johannes Schindelin
> > ---
> > t/t4211-line-log.sh | 17 +
> > 1 file changed, 17 insertions(+)
>
> Thanks for finding and demonstrating it.
You're welcome.
> Can you say more about what is going on in the test case?
Certainly. I considered writing more into the commit message, but all my
attempts were really repeating what the test case does, and were therefore
poor commit message material.
Under certain circumstances, multiple ranges specified for the line-log
were adjusted incorrectly, leading to violation of assumptions as well as
to segmentation faults. This test case demonstrates this.
> Alternatively, could it be squashed in with the patch that fixes it?
There is this really awful trend on this mailing list to suggest
conflating the demonstration of a bug with the bug fix.
It is really, really important to realize how valuable it is to have the
regression test as an individual patch that can be used to verify that
there is a bug, to pinpoint where it was introduced, to test alternative
fixes, to keep records separate, and I could go on and on and on. Please
do not ignore these very good reasons, and please refrain from
recommending such conflation in the future.
> The below will be more nitpicky:
>
> [...]
> > --- a/t/t4211-line-log.sh
> > +++ b/t/t4211-line-log.sh
> > @@ -115,4 +115,21 @@ test_expect_success 'range_set_union' '
> > git log $(for x in $(test_seq 200); do echo -L $((2*x)),+1:c.c; done)
> > '
> >
> > +q_to_lf () {
> > + tr Q '\012'
> > +}
> > +
> > +test_expect_failure 'close to overlapping ranges' '
> > + test_seq 5 >a1.c &&
> > + git add a1.c &&
> > + git commit -m "5 lines" a1.c &&
>
> It would be nice to use test_tick or test_commit for a more realistic
> history (with time marching forward).
As far as this test is concerned, that realism is not necessary, and comes
at the cost of readability.
> > + sed s/3/3QaQb/ tmp &&
> > + mv tmp a1.c &&
> > + git commit -m "2 more lines" a1.c &&
>
> It's probably just me, but the bit with Q makes it hard for me to
> follow. Maybe there's a simpler way?
Maybe. I did not find it, else I would have used it.
> "sed -e '3aa' -e '3ab'" works here, but I don't know how portable it
> is.
My experience with BSD sed and the `a` command made me highly suspicious,
that's why I did not go down that route.
Besides, that `sed` invocation does not really look intuitive either.
> I'd be more tempted to do
>
> test_write_lines 1 2 3 4 5 >a1.c &&
> ...
>
> test_write_lines 1 2 3 a b 4 5 >a1.c &&
> ...
>
> test_write_lines 1 2 3 a b c 4 5 >a1.c &&
> ...
>
> which is concise and has obvious behavior.
That works for me!
Ciao,
Dscho
Re: [PATCH v2 0/2] Make git rebase work with --rebase-merges and --exec
Team,
On Mon, 6 Aug 2018, Johannes Schindelin via GitGitGadget wrote:
> It was reported via IRC that the exec lines are inserted in the wrong spots
> when using --rebase-merges.
>
> The reason is that we used a simple, incorrect implementation that happened
> to work as long as the generated todo list only contains pick, fixup and
> squash commands. Which is not the case with--rebase-merges.
>
> Fix this issue by using a correct, if longer and slightly more complex
> implementation instead.
I should have paid more attention to detail, and should have updated this
cover letter. My bad.
The last paragraph of the part quoted above should have read:
Fix this issue by using a correct implementation instead, that
even takes into account `merge` commands in the --rebase-merges
mode.
And the changes since v1:
- Replaced the "look-ahead" design by a "keep looking" one:
instead of having a nested loop that looks for the end of the
fixup/squash chain, we continue the loop, delaying the insertion
until we know where the fixup/squash chain ends, if any.
One quirk I just noticed is that the new code does not really work
correctly in all circumstances: if the todo list ends in a comment (e.g.
an empty commit being reflected by a commented-out `pick`), we still just
append the final commands to the end.
I should qualify by "correct" in this instance: the `exec` commands are
not inserted in the location that I would have liked to, but they *are*
inserted. So it is more an aesthetic thing than anything else, and it will
probably not even show up all that often in practice.
Given that v2 is easier to understand than v1, in my opinion that slightly
awkward inconsistency in insert location is okay.
Ciao,
Dscho
> Johannes Schindelin (2):
> t3430: demonstrate what -r, --autosquash & --exec should do
> rebase --exec: make it work with --rebase-merges
>
> sequencer.c | 37 +++--
> t/t3430-rebase-merges.sh | 17 +
> 2 files changed, 44 insertions(+), 10 deletions(-)
>
>
> base-commit: 1d89318c48d233d52f1db230cf622935ac3c69fa
> Published-As:
> https://github.com/gitgitgadget/git/releases/tags/pr-13%2Fdscho%2Frebase-merges-and-exec-commands-v2
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git
> pr-13/dscho/rebase-merges-and-exec-commands-v2
> Pull-Request: https://github.com/gitgitgadget/git/pull/13
>
> Range-diff vs v1:
>
> 1: 1d82eb450 = 1: 1d82eb450 t3430: demonstrate what -r, --autosquash &
> --exec should do
> 2: b29c4d979 ! 2: 7ca441a89 rebase --exec: make it work with
> --rebase-merges
> @@ -38,7 +38,7 @@
> struct strbuf *buf = _list.buf;
> size_t offset = 0, commands_len = strlen(commands);
> - int i, first;
> -+ int i, insert_final_commands;
> ++ int i, insert;
>
> if (strbuf_read_file(_list.buf, todo_file, 0) < 0)
> return error(_("could not read '%s'."), todo_file);
> @@ -52,59 +52,38 @@
> - if (item->command == TODO_PICK && !first) {
> - strbuf_insert(buf, item->offset_in_buf + offset,
> - commands, commands_len);
> -- offset += commands_len;
> + /*
> +* Insert after every pick. Here, fixup/squash chains
> +* are considered part of the pick, so we insert the commands
> *after*
> +* those chains if there are any.
> +*/
> -+ insert_final_commands = 1;
> -+ for (i = 0; i < todo_list.nr; ) {
> ++ insert = -1;
> ++ for (i = 0; i < todo_list.nr; i++) {
> + enum todo_command command = todo_list.items[i].command;
> -+ int j = 0;
> +
> -+ if (command != TODO_PICK && command != TODO_MERGE) {
> -+ i++;
> -+ continue;
> -+ }
> -+
> -+ /* skip fixup/squash chain, if any */
> -+ for (i++; i < todo_list.nr; i++, j = 0) {
> -+ command = todo_list.items[i].command;
> -+
> -+ if (is_fixup(command))
> ++ if (insert >= 0) {
> ++ /* skip fixup/squash chains */
> ++ if (command == TODO_COMMENT)
> + continue;
> -+
> -+ if (command != TODO_COMME
[PATCH v2 1/2] t3430: demonstrate what -r, --autosquash & --exec should do
From: Johannes Schindelin The --exec option's implementation is not really well-prepared for --rebase-merges. Demonstrate this. Signed-off-by: Johannes Schindelin --- t/t3430-rebase-merges.sh | 17 + 1 file changed, 17 insertions(+) diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh index 9e6229727..0bf5eaa37 100755 --- a/t/t3430-rebase-merges.sh +++ b/t/t3430-rebase-merges.sh @@ -363,4 +363,21 @@ test_expect_success 'octopus merges' ' EOF ' +test_expect_failure 'with --autosquash and --exec' ' + git checkout -b with-exec H && + echo Booh >B.t && + test_tick && + git commit --fixup B B.t && + write_script show.sh <<-\EOF && + subject="$(git show -s --format=%s HEAD)" + content="$(git diff HEAD^! | tail -n 1)" + echo "$subject: $content" + EOF + test_tick && + git rebase -ir --autosquash --exec ./show.sh A >actual && + grep "B: +Booh" actual && + grep "E: +Booh" actual && + grep "G: +G" actual +' + test_done -- gitgitgadget
[PATCH v2 2/2] rebase --exec: make it work with --rebase-merges
From: Johannes Schindelin
The idea of `--exec` is to append an `exec` call after each `pick`.
Since the introduction of fixup!/squash! commits, this idea was extended
to apply to "pick, possibly followed by a fixup/squash chain", i.e. an
exec would not be inserted between a `pick` and any of its corresponding
`fixup` or `squash` lines.
The current implementation uses a dirty trick to achieve that: it
assumes that there are only pick/fixup/squash commands, and then
*inserts* the `exec` lines before any `pick` but the first, and appends
a final one.
With the todo lists generated by `git rebase --rebase-merges`, this
simple implementation shows its problems: it produces the exact wrong
thing when there are `label`, `reset` and `merge` commands.
Let's change the implementation to do exactly what we want: look for
`pick` lines, skip any fixup/squash chains, and then insert the `exec`
line. Lather, rinse, repeat.
While at it, also add `exec` lines after `merge` commands, because they
are similar in spirit to `pick` commands: they add new commits.
Signed-off-by: Johannes Schindelin
---
sequencer.c | 37 +++--
t/t3430-rebase-merges.sh | 2 +-
2 files changed, 28 insertions(+), 11 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 31038472f..ed2e694ff 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4244,10 +4244,9 @@ int sequencer_add_exec_commands(const char *commands)
{
const char *todo_file = rebase_path_todo();
struct todo_list todo_list = TODO_LIST_INIT;
- struct todo_item *item;
struct strbuf *buf = _list.buf;
size_t offset = 0, commands_len = strlen(commands);
- int i, first;
+ int i, insert;
if (strbuf_read_file(_list.buf, todo_file, 0) < 0)
return error(_("could not read '%s'."), todo_file);
@@ -4257,19 +4256,37 @@ int sequencer_add_exec_commands(const char *commands)
return error(_("unusable todo list: '%s'"), todo_file);
}
- first = 1;
- /* insert before every pick except the first one */
- for (item = todo_list.items, i = 0; i < todo_list.nr; i++, item++) {
- if (item->command == TODO_PICK && !first) {
- strbuf_insert(buf, item->offset_in_buf + offset,
- commands, commands_len);
+ /*
+* Insert after every pick. Here, fixup/squash chains
+* are considered part of the pick, so we insert the commands *after*
+* those chains if there are any.
+*/
+ insert = -1;
+ for (i = 0; i < todo_list.nr; i++) {
+ enum todo_command command = todo_list.items[i].command;
+
+ if (insert >= 0) {
+ /* skip fixup/squash chains */
+ if (command == TODO_COMMENT)
+ continue;
+ else if (is_fixup(command)) {
+ insert = i + 1;
+ continue;
+ }
+ strbuf_insert(buf,
+ todo_list.items[insert].offset_in_buf +
+ offset, commands, commands_len);
offset += commands_len;
+ insert = -1;
}
- first = 0;
+
+ if (command == TODO_PICK || command == TODO_MERGE)
+ insert = i + 1;
}
/* append final */
- strbuf_add(buf, commands, commands_len);
+ if (insert >= 0 || !offset)
+ strbuf_add(buf, commands, commands_len);
i = write_message(buf->buf, buf->len, todo_file, 0);
todo_list_release(_list);
diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index 0bf5eaa37..90ae613e2 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -363,7 +363,7 @@ test_expect_success 'octopus merges' '
EOF
'
-test_expect_failure 'with --autosquash and --exec' '
+test_expect_success 'with --autosquash and --exec' '
git checkout -b with-exec H &&
echo Booh >B.t &&
test_tick &&
--
gitgitgadget
[PATCH v2 0/2] Make git rebase work with --rebase-merges and --exec
It was reported via IRC that the exec lines are inserted in the wrong spots
when using --rebase-merges.
The reason is that we used a simple, incorrect implementation that happened
to work as long as the generated todo list only contains pick, fixup and
squash commands. Which is not the case with--rebase-merges.
Fix this issue by using a correct, if longer and slightly more complex
implementation instead.
Johannes Schindelin (2):
t3430: demonstrate what -r, --autosquash & --exec should do
rebase --exec: make it work with --rebase-merges
sequencer.c | 37 +++--
t/t3430-rebase-merges.sh | 17 +
2 files changed, 44 insertions(+), 10 deletions(-)
base-commit: 1d89318c48d233d52f1db230cf622935ac3c69fa
Published-As:
https://github.com/gitgitgadget/git/releases/tags/pr-13%2Fdscho%2Frebase-merges-and-exec-commands-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git
pr-13/dscho/rebase-merges-and-exec-commands-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/13
Range-diff vs v1:
1: 1d82eb450 = 1: 1d82eb450 t3430: demonstrate what -r, --autosquash &
--exec should do
2: b29c4d979 ! 2: 7ca441a89 rebase --exec: make it work with --rebase-merges
@@ -38,7 +38,7 @@
struct strbuf *buf = _list.buf;
size_t offset = 0, commands_len = strlen(commands);
- int i, first;
-+ int i, insert_final_commands;
++ int i, insert;
if (strbuf_read_file(_list.buf, todo_file, 0) < 0)
return error(_("could not read '%s'."), todo_file);
@@ -52,59 +52,38 @@
- if (item->command == TODO_PICK && !first) {
- strbuf_insert(buf, item->offset_in_buf + offset,
- commands, commands_len);
-- offset += commands_len;
+ /*
+ * Insert after every pick. Here, fixup/squash chains
+ * are considered part of the pick, so we insert the commands *after*
+ * those chains if there are any.
+ */
-+ insert_final_commands = 1;
-+ for (i = 0; i < todo_list.nr; ) {
++ insert = -1;
++ for (i = 0; i < todo_list.nr; i++) {
+ enum todo_command command = todo_list.items[i].command;
-+ int j = 0;
+
-+ if (command != TODO_PICK && command != TODO_MERGE) {
-+ i++;
-+ continue;
-+ }
-+
-+ /* skip fixup/squash chain, if any */
-+ for (i++; i < todo_list.nr; i++, j = 0) {
-+ command = todo_list.items[i].command;
-+
-+ if (is_fixup(command))
++ if (insert >= 0) {
++ /* skip fixup/squash chains */
++ if (command == TODO_COMMENT)
+ continue;
-+
-+ if (command != TODO_COMMENT)
-+ break;
-+
-+ /* skip comment if followed by any fixup/squash */
-+ for (j = i + 1; j < todo_list.nr; j++)
-+ if (todo_list.items[j].command != TODO_COMMENT)
-+ break;
-+ if (j < todo_list.nr &&
-+ is_fixup(todo_list.items[j].command)) {
-+ i = j;
++ else if (is_fixup(command)) {
++ insert = i + 1;
+ continue;
+ }
-+ break;
++ strbuf_insert(buf,
++ todo_list.items[insert].offset_in_buf +
++ offset, commands, commands_len);
+ offset += commands_len;
++ insert = -1;
}
- first = 0;
+
-+ if (i >= todo_list.nr) {
-+ insert_final_commands = 1;
-+ break;
-+ }
-+
-+ strbuf_insert(buf, todo_list.items[i].offset_in_buf + offset,
-+ commands, commands_len);
-+ offset += commands_len;
-+ insert_final_commands = 0;
++ if (command == TODO_PICK || command == TODO_MERGE)
++ insert = i + 1;
}
/* append final */
- strbuf_add(buf, commands, commands_len);
-+ if (insert_final_commands)
++ if (insert >= 0 || !offset)
+ strbuf_add(buf, commands, commands_len);
i = write_message(buf->buf, buf->len, todo_file, 0);
--
gitgitgadget
Re: [PATCH 2/2] rebase --exec: make it work with --rebase-merges
Hi Junio,
On Fri, 3 Aug 2018, Junio C Hamano wrote:
> "Johannes Schindelin via GitGitGadget"
> writes:
>
> > diff --git a/sequencer.c b/sequencer.c
> > index 31038472f..dda5cdbba 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -4244,10 +4244,9 @@ int sequencer_add_exec_commands(const char *commands)
> > {
> > const char *todo_file = rebase_path_todo();
> > struct todo_list todo_list = TODO_LIST_INIT;
> > - struct todo_item *item;
> > struct strbuf *buf = _list.buf;
> > size_t offset = 0, commands_len = strlen(commands);
> > - int i, first;
> > + int i, insert_final_commands;
> >
> > if (strbuf_read_file(_list.buf, todo_file, 0) < 0)
> > return error(_("could not read '%s'."), todo_file);
> > @@ -4257,19 +4256,57 @@ int sequencer_add_exec_commands(const char
> > *commands)
> > return error(_("unusable todo list: '%s'"), todo_file);
> > }
> >
> > - first = 1;
> > - /* insert before every pick except the first one */
> > - for (item = todo_list.items, i = 0; i < todo_list.nr; i++, item++) {
> > - if (item->command == TODO_PICK && !first) {
> > - strbuf_insert(buf, item->offset_in_buf + offset,
> > - commands, commands_len);
> > - offset += commands_len;
> > + /*
> > +* Insert after every pick. Here, fixup/squash chains
> > +* are considered part of the pick, so we insert the commands *after*
> > +* those chains if there are any.
> > +*/
>
> This is a tangent, but can a merge be amended with fixup/squash? I
> am hoping that I can use this machinery to augment Meta/Reintegrate
> logic someday, and amending merges to resolve semantic conflicts
> between topiocs in flight is what needs to happen constantly.
>
> It appears the code treats TODO_PICK and TODO_MERGE the same way, so
> the answer to the question apparently is "yes", which is good.
When I rewrote `rearrange_squash()` in C, I tried to be the kind of clever
that anticipates future features and accommodates for them:
if (!item->commit || item->command == TODO_DROP) {
subjects[i] = NULL;
continue;
}
This is the snippet of code that will skip entries in the todo list from
being eligible to be fixed up. As you can see, the code is not 100%
future-proof: if somebody wants to introduce another command like `drop`
(i.e. a command that takes a commit as parameter, but does not create a
commit), the clause will have to be extended.
Technically, I could have changed the logic in `add_exec_commands()` to
use the same heuristic. But that would change the behavior, and this patch
series is about fixing a bug, not about changing behavior.
> "after every pick" needs to become "after every pick and merge", or
> if you prefer "after creating every new commit (i.e. pick and merge)".
>
> > + insert_final_commands = 1;
>
> We assume, before entering the loop, that we'd need to append
> another exec at the end.
Right. This is what the current code does, and I am not willing to change
that in this bug fix.
(This smells like the exact suggestions I was too happy in the past to
consider, causing regressions like that vexing one where the rock solid,
battle tested hideDotGit support was broken as a consequence of the code
review on this list. It is totally my fault, I should learn from such
experiences and be very wary of potentially-breaking suggestions.)
> > + for (i = 0; i < todo_list.nr; ) {
> > + enum todo_command command = todo_list.items[i].command;
> > + int j = 0;
> > +
> > + if (command != TODO_PICK && command != TODO_MERGE) {
> > + i++;
> > + continue;
>
> If we ever see a todo-list without any pick/merge, then insert_final
> is still 1 when we leave the loop and we will add one single exec at
> the end. Which may or may not make sense---I dunno, as I do not
> offhand think of a reason why the user would give us such a sequence
> in the first place, so it probably may not matter in practice.
Think of the `noop` command. It was introduced specifically to allow
rebasing patches interactively to an upstream that already applied the
local patches. In that case, an `--exec` should still run at least once,
to avoid negative surprises.
> > + }
> > +
> > + /* skip fixup/squash chain, if any */
> > + for (i++; i < todo_list.nr; i++, j = 0) {
> > +
Re: [PATCH 2/2] rebase --exec: make it work with --rebase-merges
Hi Junio,
On Fri, 3 Aug 2018, Junio C Hamano wrote:
> "Johannes Schindelin via GitGitGadget"
> writes:
>
> > + /*
> > +* Insert after every pick. Here, fixup/squash chains
> > +* are considered part of the pick, so we insert the commands *after*
> > +* those chains if there are any.
> > +*/
> > + insert_final_commands = 1;
> > + for (i = 0; i < todo_list.nr; ) {
> > + enum todo_command command = todo_list.items[i].command;
> > + int j = 0;
> > + ...
> > + /* skip fixup/squash chain, if any */
> > + for (i++; i < todo_list.nr; i++, j = 0) {
>
> Does 'j' need to be reset to 0 in each iteration? Nobody looks at
> 'j' after exiting this inner loop, and every referernce to 'j'
> inside this inner loop happens _after_ it gets assigned "i+1" at the
> beginning of "skip comment" loop.
>
> For that matter, I wonder if 'j' can be a variable local to this
> inner loop, not the outer loop that iterates over todo_list.items[].
I rewrote this code, and the `j` variable is not even there anymore.
Ciao,
Dscho
>
> > + command = todo_list.items[i].command;
> > +
> > + if (is_fixup(command))
> > + continue;
> > +
> > + if (command != TODO_COMMENT)
> > + break;
> > +
> > + /* skip comment if followed by any fixup/squash */
> > + for (j = i + 1; j < todo_list.nr; j++)
> > + if (todo_list.items[j].command != TODO_COMMENT)
> > + break;
> > + if (j < todo_list.nr &&
> > + is_fixup(todo_list.items[j].command)) {
> > + i = j;
> > + continue;
> > + }
> > + break;
> > }
>
Re: [PATCH 2/2] rebase --exec: make it work with --rebase-merges
Hi Junio, On Fri, 3 Aug 2018, Junio C Hamano wrote: > "Johannes Schindelin via GitGitGadget" > writes: > > > From: Johannes Schindelin > > > > The idea of `--exec` is to append an `exec` call after each `pick`. > > > > Since the introduction of fixup!/squash! commits, this idea was extended > > to apply to "pick, possibly followed by a fixup/squash chain", i.e. an > > exec would not be inserted between a `pick` and any of its corresponding > > `fixup` or `squash` lines. > > > > The current implementation uses a dirty trick to achieve that: it > > assumes that there are only pick/fixup/squash commands, and then > > *inserts* the `exec` lines before any `pick` but the first, and appends > > a final one. > > Ahh, it may be "dirty" but "clever" ;-) As there is no way to say > "add exec after only the third one", inserting before 'pick', > assuming the lines before that would be a "previous group" that > began with a pick and then possibly its amending operations, was > sufficient. Makes sense. Yes, it was clever. It is the kind of clever that causes regressions when new features are introduced that were not anticipated by the cleverness. Ciao, Dscho
pk/rebase-in-c, was Re: What's cooking in git.git (Aug 2018, #01; Thu, 2)
Hi Junio, On Thu, 2 Aug 2018, Junio C Hamano wrote: > * pk/rebase-in-c (2018-07-30) 3 commits > - builtin/rebase: support running "git rebase " > - rebase: refactor common shell functions into their own file > - rebase: start implementing it as a builtin > > Rewrite of the "rebase" machinery in C. > > Will merge to 'next'. Please hold. I found several bugs in the third patch, and it will need to be fixed before sending another iteration. Ciao, Dscho
[PATCH 1/4] line-log: demonstrate a bug with nearly-overlapping ranges
From: Johannes Schindelin
Currently, this test case throws an assertion:
Assertion failed!
Program: git.exe
File: line-log.c, Line 71
Signed-off-by: Johannes Schindelin
---
t/t4211-line-log.sh | 17 +
1 file changed, 17 insertions(+)
diff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh
index 436b13ad2..61ff37430 100755
--- a/t/t4211-line-log.sh
+++ b/t/t4211-line-log.sh
@@ -115,4 +115,21 @@ test_expect_success 'range_set_union' '
git log $(for x in $(test_seq 200); do echo -L $((2*x)),+1:c.c; done)
'
+q_to_lf () {
+ tr Q '\012'
+}
+
+test_expect_failure 'close to overlapping ranges' '
+ test_seq 5 >a1.c &&
+ git add a1.c &&
+ git commit -m "5 lines" a1.c &&
+ sed s/3/3QaQb/ tmp &&
+ mv tmp a1.c &&
+ git commit -m "2 more lines" a1.c &&
+ sed s/4/cQ4/ tmp &&
+ mv tmp a1.c &&
+ git commit -m "1 more line" a1.c &&
+ git --no-pager log -L 1,3:a1.c -L 5,8:a1.c
+'
+
test_done
--
gitgitgadget
[PATCH 0/4] line-log: be more careful when adjusting multiple line ranges
I am a heavy user of git log -L In fact, I use the feature where
multiple ranges can be specified extensively, via a not-exactly-trivial
shell script function that takes the currently staged changes (or if none
are staged, the current unstanged changes) and turns them into the
corresponding commit history:
staged_log () { #
diff="$(git diff --cached -U1)"
test -n "$diff" ||
diff="$(git diff -U1)"
test -n "$diff" ||
die "No changes"
eval "git log $(echo "$diff" |
sed -ne '/^--- a\//{s/^-* a\/\(.*\)/'\''\1'\''/;x}' -e \
'/^@@ -/{s/^@@ -\([^, ]*\),\([^ ]*\).*/-L
\1,+\2/;G;s/\n/:/g;p}' |
tr '\n' ' ')"
}
This is an extremely useful way to look at the history, especially when
trying to fix up a commit deep in a long branch (or a thicket of branches).
Today, however, this method failed me, by greeting me with an assertion.
When I tried to paper over that assertion by joining line ranges that became
adjacent (or overlapping), it still produced a segmentation fault when the
line-log tried to print lines past the file contents.
So I had no choice but to fix this properly.
I still wanted to keep the optimization where multiple line ranges are
joined into a single one (I am convinced that this also affects the output,
where previously multiple hunks would have been displayed, but I ran out of
time to investigate this). This is the 3rd patch. It is not purely an
optimization, as the assertion would still trigger when line ranges could be
joined.
Now, I am fairly certain that the changes are correct, but given my track
record with off-by-one bugs (and once even an off-by-two bug), I would
really appreciate some thorough review of this code, in particular the
second one that is the actual bug fix. I am specifically interested in
reviews from people who know line-log.c pretty well and can tell me whether
the src[i].end > target[j].end is correct, or whether it should actually
have been a >= (I tried to wrap my head around this, but I would feel more
comfortable if a domain expert would analyze this, whistling, and looking
Eric's way).
Cc: Eric Sunshine sunsh...@sunshineco.com [sunsh...@sunshineco.com]
Johannes Schindelin (4):
line-log: demonstrate a bug with nearly-overlapping ranges
line-log: adjust start/end of ranges individually
line-log: optimize ranges by joining them when possible
line-log: convert an assertion to a full BUG() call
line-log.c | 18 +++---
t/t4211-line-log.sh | 17 +
2 files changed, 32 insertions(+), 3 deletions(-)
base-commit: 1d89318c48d233d52f1db230cf622935ac3c69fa
Published-As:
https://github.com/gitgitgadget/git/releases/tags/pr-15%2Fdscho%2Fline-log-fix-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git
pr-15/dscho/line-log-fix-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/15
--
gitgitgadget
[PATCH 4/4] line-log: convert an assertion to a full BUG() call
From: Johannes Schindelin
The assertion in question really indicates a bug, when triggered, so we
might just as well use the sanctioned method to report it.
Signed-off-by: Johannes Schindelin
---
line-log.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/line-log.c b/line-log.c
index bc7ef69d6..0e09df9db 100644
--- a/line-log.c
+++ b/line-log.c
@@ -72,7 +72,9 @@ void range_set_append(struct range_set *rs, long a, long b)
rs->ranges[rs->nr-1].end = b;
return;
}
- assert(rs->nr == 0 || rs->ranges[rs->nr-1].end <= a);
+ if (rs->nr > 0 && rs->ranges[rs->nr-1].end > a)
+ BUG("append %ld-%ld, after %ld-%ld?!?", a, b,
+ rs->ranges[rs->nr-1].start, rs->ranges[rs->nr-1].end);
range_set_append_unsafe(rs, a, b);
}
--
gitgitgadget
[PATCH 3/4] line-log: optimize ranges by joining them when possible
From: Johannes Schindelin
Technically, it is okay to have line ranges that touch (i.e. the end of
the first range ends just before the next range begins). However, it is
inefficient, and when the user provides such touching ranges via
multiple `-L` options, we already join them.
When we traverse the history, though, we never join ranges, even they
become "touchy-feely" due to added lines (which are "removed" from
line-log's point of view because it traverses the commit history into
the past).
Let's optimize also this case.
Signed-off-by: Johannes Schindelin
---
line-log.c | 4
t/t4211-line-log.sh | 2 +-
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/line-log.c b/line-log.c
index d8d09b5ee..bc7ef69d6 100644
--- a/line-log.c
+++ b/line-log.c
@@ -68,6 +68,10 @@ void range_set_append_unsafe(struct range_set *rs, long a,
long b)
void range_set_append(struct range_set *rs, long a, long b)
{
+ if (rs->nr > 0 && rs->ranges[rs->nr-1].end + 1 == a) {
+ rs->ranges[rs->nr-1].end = b;
+ return;
+ }
assert(rs->nr == 0 || rs->ranges[rs->nr-1].end <= a);
range_set_append_unsafe(rs, a, b);
}
diff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh
index 61ff37430..ebaf5ea86 100755
--- a/t/t4211-line-log.sh
+++ b/t/t4211-line-log.sh
@@ -119,7 +119,7 @@ q_to_lf () {
tr Q '\012'
}
-test_expect_failure 'close to overlapping ranges' '
+test_expect_success 'close to overlapping ranges' '
test_seq 5 >a1.c &&
git add a1.c &&
git commit -m "5 lines" a1.c &&
--
gitgitgadget
[PATCH 2/4] line-log: adjust start/end of ranges individually
From: Johannes Schindelin
When traversing commits and adjusting the ranges, things can get really
tricky. For example, when the line range of interest encloses several
hunks of a commit, the line range can actually shrink.
Currently, range_set_shift_diff() does not anticipate that scenario and
blindly adjusts start and end by the same offset ("shift" the range).
This can lead to a couple of surprising issues, such as assertions in
range_set_append() (when the end of a given range is not adjusted
properly, it can point after the start of the next range) or even
segmentation faults (when t_end in the loop of dump_diff_hacky_one()
points outside the valid line range).
Let's fix this by adjusting the start and the end offsets individually.
Signed-off-by: Johannes Schindelin
---
line-log.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/line-log.c b/line-log.c
index 72a5fed66..d8d09b5ee 100644
--- a/line-log.c
+++ b/line-log.c
@@ -427,7 +427,7 @@ static void range_set_shift_diff(struct range_set *out,
struct diff_ranges *diff)
{
unsigned int i, j = 0;
- long offset = 0;
+ long offset = 0, start_offset;
struct range *src = rs->ranges;
struct range *target = diff->target.ranges;
struct range *parent = diff->parent.ranges;
@@ -438,7 +438,13 @@ static void range_set_shift_diff(struct range_set *out,
- (target[j].end-target[j].start);
j++;
}
- range_set_append(out, src[i].start+offset, src[i].end+offset);
+ start_offset = offset;
+ while (j < diff->target.nr && src[i].end > target[j].end) {
+ offset += (parent[j].end-parent[j].start)
+ - (target[j].end-target[j].start);
+ j++;
+ }
+ range_set_append(out, src[i].start+start_offset,
src[i].end+offset);
}
}
--
gitgitgadget
[PATCH 0/1] Support git pull --rebase=i
The patch [https://github.com/git-for-windows/git/commit/4aa8b8c82] that introduced support for pull --rebase= into the Git for Windows project still allowed the very convenient abbreviation git pull --rebase=i which was later lost when it was ported to the builtin git pull, and it was not introduced before the patch eventually made it into Git as f5eb87b98dd (pull: allow interactive rebase with --rebase=interactive, 2016-01-13). However, it is really a useful short hand for the occasional rebasing pull on branches that do not usually want to be rebased. So let's reintroduce this convenience, at long last. Johannes Schindelin (1): pull --rebase=: allow single-letter abbreviations for the type builtin/pull.c | 6 +++--- t/t5520-pull.sh | 12 2 files changed, 15 insertions(+), 3 deletions(-) base-commit: 1d89318c48d233d52f1db230cf622935ac3c69fa Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-14%2Fdscho%2Fpull-rebase-i-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-14/dscho/pull-rebase-i-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/14 -- gitgitgadget
[PATCH 1/1] pull --rebase=: allow single-letter abbreviations for the type
From: Johannes Schindelin Git for Windows' original 4aa8b8c8283 (Teach 'git pull' to handle --rebase=interactive, 2011-10-21) had support for the very convenient abbreviation git pull --rebase=i which was later lost when it was ported to the builtin `git pull`, and it was not introduced before the patch eventually made it into Git as f5eb87b98dd (pull: allow interactive rebase with --rebase=interactive, 2016-01-13). However, it is *really* a useful short hand for the occasional rebasing pull on branches that do not usually want to be rebased. So let's reintroduce this convenience, at long last. Signed-off-by: Johannes Schindelin --- builtin/pull.c | 6 +++--- t/t5520-pull.sh | 12 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/builtin/pull.c b/builtin/pull.c index 4e7893539..53bc5facf 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -48,11 +48,11 @@ static enum rebase_type parse_config_rebase(const char *key, const char *value, return REBASE_FALSE; else if (v > 0) return REBASE_TRUE; - else if (!strcmp(value, "preserve")) + else if (!strcmp(value, "preserve") || !strcmp(value, "p")) return REBASE_PRESERVE; - else if (!strcmp(value, "merges")) + else if (!strcmp(value, "merges") || !strcmp(value, "m")) return REBASE_MERGES; - else if (!strcmp(value, "interactive")) + else if (!strcmp(value, "interactive") || !strcmp(value, "i")) return REBASE_INTERACTIVE; if (fatal) diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh index 68aa5f034..5e501c8b0 100755 --- a/t/t5520-pull.sh +++ b/t/t5520-pull.sh @@ -475,10 +475,22 @@ test_expect_success 'pull.rebase=interactive' ' false EOF test_set_editor "$TRASH_DIRECTORY/fake-editor" && + test_when_finished "test_might_fail git rebase --abort" && test_must_fail git pull --rebase=interactive . copy && test "I was here" = "$(cat fake.out)" ' +test_expect_success 'pull --rebase=i' ' + write_script "$TRASH_DIRECTORY/fake-editor" <<-\EOF && + echo I was here, too >fake.out && + false + EOF + test_set_editor "$TRASH_DIRECTORY/fake-editor" && + test_when_finished "test_might_fail git rebase --abort" && + test_must_fail git pull --rebase=i . copy && + test "I was here, too" = "$(cat fake.out)" +' + test_expect_success 'pull.rebase=invalid fails' ' git reset --hard before-preserve-rebase && test_config pull.rebase invalid && -- gitgitgadget
[PATCH 0/2] Make git rebase work with --rebase-merges and --exec
It was reported via IRC that the exec lines are inserted in the wrong spots when using --rebase-merges. The reason is that we used a simple, incorrect implementation that happened to work as long as the generated todo list only contains pick, fixup and squash commands. Which is not the case with--rebase-merges. Fix this issue by using a correct, if longer and slightly more complex implementation instead. Johannes Schindelin (2): t3430: demonstrate what -r, --autosquash & --exec should do rebase --exec: make it work with --rebase-merges sequencer.c | 59 t/t3430-rebase-merges.sh | 17 2 files changed, 65 insertions(+), 11 deletions(-) base-commit: 1d89318c48d233d52f1db230cf622935ac3c69fa Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-13%2Fdscho%2Frebase-merges-and-exec-commands-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-13/dscho/rebase-merges-and-exec-commands-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/13 -- gitgitgadget
[PATCH 1/2] t3430: demonstrate what -r, --autosquash & --exec should do
From: Johannes Schindelin The --exec option's implementation is not really well-prepared for --rebase-merges. Demonstrate this. Signed-off-by: Johannes Schindelin --- t/t3430-rebase-merges.sh | 17 + 1 file changed, 17 insertions(+) diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh index 9e6229727..0bf5eaa37 100755 --- a/t/t3430-rebase-merges.sh +++ b/t/t3430-rebase-merges.sh @@ -363,4 +363,21 @@ test_expect_success 'octopus merges' ' EOF ' +test_expect_failure 'with --autosquash and --exec' ' + git checkout -b with-exec H && + echo Booh >B.t && + test_tick && + git commit --fixup B B.t && + write_script show.sh <<-\EOF && + subject="$(git show -s --format=%s HEAD)" + content="$(git diff HEAD^! | tail -n 1)" + echo "$subject: $content" + EOF + test_tick && + git rebase -ir --autosquash --exec ./show.sh A >actual && + grep "B: +Booh" actual && + grep "E: +Booh" actual && + grep "G: +G" actual +' + test_done -- gitgitgadget
[PATCH 2/2] rebase --exec: make it work with --rebase-merges
From: Johannes Schindelin
The idea of `--exec` is to append an `exec` call after each `pick`.
Since the introduction of fixup!/squash! commits, this idea was extended
to apply to "pick, possibly followed by a fixup/squash chain", i.e. an
exec would not be inserted between a `pick` and any of its corresponding
`fixup` or `squash` lines.
The current implementation uses a dirty trick to achieve that: it
assumes that there are only pick/fixup/squash commands, and then
*inserts* the `exec` lines before any `pick` but the first, and appends
a final one.
With the todo lists generated by `git rebase --rebase-merges`, this
simple implementation shows its problems: it produces the exact wrong
thing when there are `label`, `reset` and `merge` commands.
Let's change the implementation to do exactly what we want: look for
`pick` lines, skip any fixup/squash chains, and then insert the `exec`
line. Lather, rinse, repeat.
While at it, also add `exec` lines after `merge` commands, because they
are similar in spirit to `pick` commands: they add new commits.
Signed-off-by: Johannes Schindelin
---
sequencer.c | 59
t/t3430-rebase-merges.sh | 2 +-
2 files changed, 49 insertions(+), 12 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 31038472f..dda5cdbba 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4244,10 +4244,9 @@ int sequencer_add_exec_commands(const char *commands)
{
const char *todo_file = rebase_path_todo();
struct todo_list todo_list = TODO_LIST_INIT;
- struct todo_item *item;
struct strbuf *buf = _list.buf;
size_t offset = 0, commands_len = strlen(commands);
- int i, first;
+ int i, insert_final_commands;
if (strbuf_read_file(_list.buf, todo_file, 0) < 0)
return error(_("could not read '%s'."), todo_file);
@@ -4257,19 +4256,57 @@ int sequencer_add_exec_commands(const char *commands)
return error(_("unusable todo list: '%s'"), todo_file);
}
- first = 1;
- /* insert before every pick except the first one */
- for (item = todo_list.items, i = 0; i < todo_list.nr; i++, item++) {
- if (item->command == TODO_PICK && !first) {
- strbuf_insert(buf, item->offset_in_buf + offset,
- commands, commands_len);
- offset += commands_len;
+ /*
+* Insert after every pick. Here, fixup/squash chains
+* are considered part of the pick, so we insert the commands *after*
+* those chains if there are any.
+*/
+ insert_final_commands = 1;
+ for (i = 0; i < todo_list.nr; ) {
+ enum todo_command command = todo_list.items[i].command;
+ int j = 0;
+
+ if (command != TODO_PICK && command != TODO_MERGE) {
+ i++;
+ continue;
+ }
+
+ /* skip fixup/squash chain, if any */
+ for (i++; i < todo_list.nr; i++, j = 0) {
+ command = todo_list.items[i].command;
+
+ if (is_fixup(command))
+ continue;
+
+ if (command != TODO_COMMENT)
+ break;
+
+ /* skip comment if followed by any fixup/squash */
+ for (j = i + 1; j < todo_list.nr; j++)
+ if (todo_list.items[j].command != TODO_COMMENT)
+ break;
+ if (j < todo_list.nr &&
+ is_fixup(todo_list.items[j].command)) {
+ i = j;
+ continue;
+ }
+ break;
}
- first = 0;
+
+ if (i >= todo_list.nr) {
+ insert_final_commands = 1;
+ break;
+ }
+
+ strbuf_insert(buf, todo_list.items[i].offset_in_buf + offset,
+ commands, commands_len);
+ offset += commands_len;
+ insert_final_commands = 0;
}
/* append final */
- strbuf_add(buf, commands, commands_len);
+ if (insert_final_commands)
+ strbuf_add(buf, commands, commands_len);
i = write_message(buf->buf, buf->len, todo_file, 0);
todo_list_release(_list);
diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index 0bf5eaa37..90ae613e2 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -363,7 +363,7 @@ test_expect_success 'octopus merges' '
EOF
'
-test_expect_failure 'with --autosquash and --exec' '
+test_expect_success 'with --autosquash and --exec' '
git checkout -b with-exec H &&
echo Booh >B.t &&
test_tick &&
--
gitgitgadget
Re: [PATCH] negotiator/skipping: skip commits during fetch
Hi Jonathan,
On Fri, 27 Jul 2018, Johannes Schindelin wrote:
> On Thu, 26 Jul 2018, Jonathan Tan wrote:
>
> > > On Mon, 16 Jul 2018, Jonathan Tan wrote:
> > >
> > > > t/t5552-skipping-fetch-negotiator.sh | 179 +++
> > >
> > > This test seems to be failing consistently in the recent `pu` builds:
> > >
> > > https://git-for-windows.visualstudio.com/git/_build/results?buildId=14337=logs
> > >
> > > Could you have a look, please?
> >
> > Hmm...on my Linux computer, this test passes on both pu (as of the time
> > of writing) and 838143aa5c ("Merge branch 'ab/newhash-is-sha256' into
> > pu", 2018-07-25) (pu at the time of that build, according to the website
> > you linked above). If you could rerun that test with additional code,
> > could you add a "cat trace" and show me what the client sends?
>
> I can give you something even better: a playground. Just open a PR at
> https://github.com/gitgitgadget/git (all of the branches on gitster/git ar
> mirrored, including yours, I am sure, so you can target that branch
> specifically).
>
> Once you open a Pull Request, it will automatically build and run the test
> suite on Windows, macOS and Linux. You will see it in the "checks" section
> on the bottom. Example for my range-diff series:
>
> https://git-for-windows.visualstudio.com/git/_build/results?buildId=14279
>
> For a quicker turnaround, you could add a commit that forces the `all`
> target in `t/Makefile` to run only your test.
>
> > When I do that, the relevant parts are:
> >
> > packet:fetch> have 9ab46928dc282aa09f4dbf96893a252e058e7e8e
> > packet:fetch> have dc824fafb05f3229aedf1f320bbe572e35364dfe
> > packet:fetch> have caef059de69917b9119176a11b88afcef769331d
> > packet:fetch> have 41bd8dc092ee110ba80e350a346ec507ab2e42a0
> > packet:fetch> have e9a2c092a8e911567a377c881a7f6031e7f892ea
> > packet:fetch> done
> >
> > which is exactly as I (and the test) expect.
> >
> > Two possible reasons for the discrepancy that I can think of offhand are
> > that (1) my computer generates different commits from your test system,
> > and (2) the priority queue pops commits in a different order. For (1),
> > that's not possible because the SHA-1s are the same (as can be seen by
> > comparing your link and the "have" lines I quoted above), and for (2),
> > the code seems OK:
> >
> > static int compare(const void *a_, const void *b_, void *unused)
> > {
> > const struct entry *a = a_;
> > const struct entry *b = b_;
> > return compare_commits_by_commit_date(a->commit, b->commit, NULL);
> > }
> >
> > Let me know if you can observe the output of "cat trace" or if you have
> > any other ideas.
>
> Like I said, you can use those "CI" builds, I think that would be more
> effective than if you waited for me to react, I am quite overwhelmed these
> days.
Hopefully you have a chance to do so. I got the impression that it is
actually more of a flakey test than a consistent test failure:
https://git-for-windows.visualstudio.com/git/_build/results?buildId=15015=logs
Ciao,
Dscho
Re: [PATCH] color: protect against out-of-bounds array access/assignment
Hi Eric,
On Thu, 2 Aug 2018, Eric Sunshine wrote:
> want_color_fd() is designed to work only with standard input, output,
> and error file descriptors, and stores information about each descriptor
> in an array. However, it doesn't verify that the passed-in descriptor
> lives within that set, which, with a buggy caller, could lead to
> access/assignment outside the array bounds.
ACK!
Thanks,
Dscho
>
> Signed-off-by: Eric Sunshine
> ---
>
> Just something I noticed while studying this code in relation to a patch
> review.
>
> color.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/color.c b/color.c
> index b1c24c69de..b0be9ce505 100644
> --- a/color.c
> +++ b/color.c
> @@ -343,6 +343,9 @@ int want_color_fd(int fd, int var)
>
> static int want_auto[3] = { -1, -1, -1 };
>
> + if (fd < 0 || fd >= ARRAY_SIZE(want_auto))
> + BUG("file descriptor out of range: %d", fd);
> +
> if (var < 0)
> var = git_use_color_default;
>
> --
> 2.18.0.599.g4ce2a8faa4.dirty
>
>
Re: range-diff, was Re: What's cooking in git.git (Jul 2018, #03; Wed, 25)
Hi Junio, On Mon, 30 Jul 2018, Junio C Hamano wrote: > Johannes Schindelin writes: > > > FWIW I picked up your Asciidoc-underline fix, and I also fixed a typo in a > > commit message (you may want to pick that up, too, unless you want me to > > send a full new iteration, I don't care either way): > > Meaning that if you send a full new iteration it would match what we > have on 'pu' plus the one-liner below? I think we can do without > such a resend, because everybody has seen all there is to see if > that is the case. > > > -- snipsnap -- > > 11: bf0a5879361 ! 11: 0c1f5db5d01 range-diff: add tests > > @@ -3,7 +3,7 @@ > > range-diff: add tests > > > > These are essentially lifted from https://github.com/trast/tbdiff, > > with > > -light touch-ups to account for the command now being names `git > > +light touch-ups to account for the command now being named `git > > range-diff`. > > > > Apart from renaming `tbdiff` to `range-diff`, only one test case > > needed > > I'll need to remember to rebuild es/format-patch-rangediff after > amending bf0a587936 with this, but I think I should be able to push > out the result in today's round. > > If any other issue arises, I do not mind taking an update, either, > but I think at this point the topic is reaching the point of > diminishing returns and should switch to incremental. Thomas had a couple of good suggestions, still, and I am considering to try to find time to simply disable the whitespace warnings altogether in range-diff. Ciao, Dscho
Re: [PATCH v4 11/21] range-diff: add tests
Hi Eric, On Sun, 22 Jul 2018, Eric Sunshine wrote: > On Sat, Jul 21, 2018 at 6:05 PM Thomas Rast via GitGitGadget > wrote: > > These are essentially lifted from https://github.com/trast/tbdiff, with > > light touch-ups to account for the command now being names `git > > s/names/named/ Thanks. I already pushed an update to https://github.com/gitgitgadget/git/pull/1. Ciao, Dscho > > > range-diff`. > > > > Apart from renaming `tbdiff` to `range-diff`, only one test case needed > > to be adjusted: 11 - 'changed message'. > > > > The underlying reason it had to be adjusted is that diff generation is > > sometimes ambiguous. In this case, a comment line and an empty line are > > added, but it is ambiguous whether they were added after the existing > > empty line, or whether an empty line and the comment line are added > > *before* the existing empty line. And apparently xdiff picks a different > > option here than Python's difflib. > > > > Signed-off-by: Johannes Schindelin >
Re: [PATCH v4 05/21] range-diff: also show the diff between patches
Hi Thomas & Eric,
On Sun, 29 Jul 2018, Thomas Gummerer wrote:
> On 07/29, Eric Sunshine wrote:
> > On Sun, Jul 29, 2018 at 3:04 PM Thomas Gummerer
> > wrote:
> > > On 07/21, Johannes Schindelin via GitGitGadget wrote:
> > > > Just like tbdiff, we now show the diff between matching patches. This is
> > > > a "diff of two diffs", so it can be a bit daunting to read for the
> > > > beginner.
> > > > [...]
> > > > Note also: while tbdiff accepts the `--no-patches` option to suppress
> > > > these diffs between patches, we prefer the `-s` option that is
> > > > automatically supported via our use of diff_opt_parse().
> > >
> > > One slightly unfortunate thing here is that we don't show these
> > > options in 'git range-diff -h', which would be nice to have. I don't
> > > know if that's possible in git right now, if it's not easily possible,
> > > I definitely wouldn't want to delay this series for that, and we could
> > > just add it to the list of possible future enhancements that other
> > > people mentioned.
> >
> > This issue is not specific to git-range-diff; it's shared by other
> > commands which inherit diff options via diff_opt_parse(). For
> > instance, "git log -h" doesn't show diff-related options either, yet
> > it accepts them.
>
> Fair enough, that makes sense. Thanks for the pointer!
>
> There's one more thing that I noticed here:
>
> git range-diff --no-patches
> fatal: single arg format requires a symmetric range
>
> Which is a slightly confusing error message. In contrast git log does
> the following on an unrecognized argument:
>
> git log --no-patches
> fatal: unrecognized argument: --no-patches
>
> which is a little better I think. I do however also thing the "fatal:
> single arg format requires a symmetric range" is useful when someone
> genuinely tries to use the single argument version of the command. So
> I don't know what a good solution for this would be.
I immediately thought of testing for a leading `-` of the remaining
argument, but I could imagine that somebody enterprisey uses
git range-diff -- -my-first-attempt...-my-second-attempt
and I do not really want to complexify the code... Ideas?
> > > > diff --git a/range-diff.c b/range-diff.c
> > > > @@ -300,6 +325,9 @@ static void output(struct string_list *a, struct
> > > > string_list *b)
> > > > printf("%d: %s ! %d: %s\n",
> > > > b_util->matching + 1, short_oid(a_util),
> > > > j + 1, short_oid(b_util));
> > > > + if (!(diffopt->output_format &
> > > > DIFF_FORMAT_NO_OUTPUT))
> > >
> > > Looking at this line, it looks like it would be easy to support
> > > '--no-patches' as well, which may be slightly easier to understand that
> > > '-s' to someone new to the command. But again that can be added later
> > > if someone actually cares about it.
> >
> > What wasn't mentioned (but was implied) by the commit message is that
> > "-s" is short for "--no-patch", which also comes for free via
> > diff_opt_parse(). True, "--no-patch" isn't spelled exactly the same as
> > "--no-patches", but git-range-diff isn't exactly a perfect tbdiff
> > clone, so hopefully not a git problem. Moreover, "--no-patch" is
> > internally consistent within the Git builtin commands.
>
> Makes sense, thanks! "--no-patch" does make sense to me. There's
> still a lot of command line flags in git to learn for me, even after
> all this time using it ;) Might be nice to spell it out in the commit
> message for someone like me, especially as "--no-patches" is already
> mentioned. Though I guess most regulars here would know about
> "--no-patch", so maybe it's not worth it. Anyway that is definitely
> not worth another round here.
Sure, but not many users learn from reading the commit history...
:-)
Ciao,
Dscho
Re: [PATCH v4 03/21] range-diff: first rudimentary implementation
Hi Thomas,
On Sun, 29 Jul 2018, Thomas Gummerer wrote:
> On 07/21, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin
> >
> > At this stage, `git range-diff` can determine corresponding commits
> > of two related commit ranges. This makes use of the recently introduced
> > implementation of the linear assignment algorithm.
> >
> > The core of this patch is a straight port of the ideas of tbdiff, the
> > apparently dormant project at https://github.com/trast/tbdiff.
> >
> > The output does not at all match `tbdiff`'s output yet, as this patch
> > really concentrates on getting the patch matching part right.
> >
> > Note: due to differences in the diff algorithm (`tbdiff` uses the Python
> > module `difflib`, Git uses its xdiff fork), the cost matrix calculated
> > by `range-diff` is different (but very similar) to the one calculated
> > by `tbdiff`. Therefore, it is possible that they find different matching
> > commits in corner cases (e.g. when a patch was split into two patches of
> > roughly equal length).
> >
> > Signed-off-by: Johannes Schindelin
> > ---
> > Makefile | 1 +
> > builtin/range-diff.c | 43 +-
> > range-diff.c | 311 +++
> > range-diff.h | 7 +
> > 4 files changed, 361 insertions(+), 1 deletion(-)
> > create mode 100644 range-diff.c
> > create mode 100644 range-diff.h
> >
> > [...]
> >
> > diff --git a/range-diff.c b/range-diff.c
> > new file mode 100644
> > index 0..15d418afa
> > --- /dev/null
> > +++ b/range-diff.c
> > @@ -0,0 +1,311 @@
> > +#include "cache.h"
> > +#include "range-diff.h"
> > +#include "string-list.h"
> > +#include "run-command.h"
> > +#include "argv-array.h"
> > +#include "hashmap.h"
> > +#include "xdiff-interface.h"
> > +#include "linear-assignment.h"
> > +
> > +struct patch_util {
> > + /* For the search for an exact match */
> > + struct hashmap_entry e;
> > + const char *diff, *patch;
> > +
> > + int i;
> > + int diffsize;
> > + size_t diff_offset;
> > + /* the index of the matching item in the other branch, or -1 */
> > + int matching;
> > + struct object_id oid;
> > +};
> > +
> > +/*
> > + * Reads the patches into a string list, with the `util` field being
> > populated
> > + * as struct object_id (will need to be free()d).
> > + */
> > +static int read_patches(const char *range, struct string_list *list)
> > +{
> > + struct child_process cp = CHILD_PROCESS_INIT;
> > + FILE *in;
> > + struct strbuf buf = STRBUF_INIT, line = STRBUF_INIT;
> > + struct patch_util *util = NULL;
> > + int in_header = 1;
> > +
> > + argv_array_pushl(, "log", "--no-color", "-p", "--no-merges",
> > + "--reverse", "--date-order", "--decorate=no",
> > + "--no-abbrev-commit", range,
> > + NULL);
>
> Compared to tbdiff, add "--decorate=no", and "--no-abbrev-commit". I
> see we're abbreviating the commit hashes later. We don't want ref
> names here, so "--decorate=no" makes sense as well.
Indeed. Compare also to https://github.com/trast/tbdiff/pull/8
> > + cp.out = -1;
> > + cp.no_stdin = 1;
> > + cp.git_cmd = 1;
> > +
> > + if (start_command())
> > + return error_errno(_("could not start `log`"));
> > + in = fdopen(cp.out, "r");
> > + if (!in) {
> > + error_errno(_("could not read `log` output"));
> > + finish_command();
> > + return -1;
> > + }
> > +
> > + while (strbuf_getline(, in) != EOF) {
> > + const char *p;
> > +
> > + if (skip_prefix(line.buf, "commit ", )) {
> > + if (util) {
> > + string_list_append(list, buf.buf)->util = util;
> > + strbuf_reset();
> > + }
> > + util = xcalloc(sizeof(*util), 1);
> > + if (get_oid(p, >oid)) {
> > + error(_("could not parse commit '%s'"), p);
> > + free(util);
> > + string
Re: [PATCH v4 01/21] linear-assignment: a function to solve least-cost assignment problems
Hi Thomas, On Sat, 28 Jul 2018, Thomas Gummerer wrote: > On 07/21, Johannes Schindelin via GitGitGadget wrote: > > From: Johannes Schindelin > > > > The problem solved by the code introduced in this commit goes like this: > > given two sets of items, and a cost matrix which says how much it > > "costs" to assign any given item of the first set to any given item of > > the second, assign all items (except when the sets have different size) > > in the cheapest way. > > > > We use the Jonker-Volgenant algorithm to solve the assignment problem to > > answer questions such as: given two different versions of a topic branch > > (or iterations of a patch series), what is the best pairing of > > commits/patches between the different versions? > > > > Signed-off-by: Johannes Schindelin > > --- > > Makefile| 1 + > > linear-assignment.c | 201 > > linear-assignment.h | 22 + > > 3 files changed, 224 insertions(+) > > create mode 100644 linear-assignment.c > > create mode 100644 linear-assignment.h > > > > [...] > > > > diff --git a/linear-assignment.h b/linear-assignment.h > > new file mode 100644 > > index 0..fc4c502c8 > > --- /dev/null > > +++ b/linear-assignment.h > > @@ -0,0 +1,22 @@ > > +#ifndef HUNGARIAN_H > > +#define HUNGARIAN_H > > nit: maybe s/HUNGARIAN/LINEAR_ASSIGNMENT/ in the two lines above. Makes sense. Ciao, Dscho > > > + > > +/* > > + * Compute an assignment of columns -> rows (and vice versa) such that > > every > > + * column is assigned to at most one row (and vice versa) minimizing the > > + * overall cost. > > + * > > + * The parameter `cost` is the cost matrix: the cost to assign column j to > > row > > + * i is `cost[j + column_count * i]. > > + * > > + * The arrays column2row and row2column will be populated with the > > respective > > + * assignments (-1 for unassigned, which can happen only if column_count != > > + * row_count). > > + */ > > +void compute_assignment(int column_count, int row_count, int *cost, > > + int *column2row, int *row2column); > > + > > +/* The maximal cost in the cost matrix (to prevent integer overflows). */ > > +#define COST_MAX (1<<16) > > + > > +#endif > > -- > > gitgitgadget > > >
Re: [PATCH 0/2] fix "rebase -i --root" corrupting root commit
Hi Phillip, On Mon, 30 Jul 2018, Phillip Wood wrote: > On 30/07/18 10:29, Eric Sunshine wrote: > > This series fixes bugs causing corruption of the root commit when > > "rebase -i --root" is used to swap in a new root commit. In particular, > > the "author" header has trailing garbage. Some tools handle the > > corruption somewhat gracefully by showing a bogus date, but others barf > > on it (gitk, for instance). git-fsck correctly identifies the > > corruption. I discovered this after git-rebase corrupted one of my own > > projects. > > > > Unfortunately, these bugs (from js/sequencer-and-root-commits) made it > > into the v2.18.0 release. It's worrying that a released Git can be > > creating corrupt commits, but fortunately "rebase -i --root" is not > > likely used often (especially on well-established projects). > > Nevertheless, it may be 'maint' worthy and applies cleanly there. > > > > It was only after I diagnosed and fixed these bugs that I thought to > > check 'pu' and discovered that Akinori MUSHA already made a stab[1] at > > fixing one of the three bugs which this series fixes. Akinori's fix has > > the somewhat undesirable property that it adds an extra blank line to > > the end of the script, as Phillip correctly pointed out in review[2]. > > Patch 2/2 of this series has the more "correct" fix, in addition to > > fixing another bug. > > > > Moreover, patch 2/2 of this series provides a more thorough fix overall > > than Akinori, so it may make sense to replace his patch with this > > series, though perhaps keep the test his patch adds to augment the > > strict test of the "author" header added by this series. > > Johannes and I have some fixups for Akinori's patch on the branch > fix-t3403-author-script-test at https://github.com/phillipwood/git > > That branch also contains a fix for the bad quoting of names with "'" in > them. I think it would be good to somehow try and combine this series > with those patches. I would like that, too. > I'd really like to see a single function to read and another to write > the author script that is shared by 'git am' and 'git rebase -i', rather > than the two writers and three readers we have at the moment. I was > thinking of doing that in the longer term, but given the extra bug > you've found in read_author_script() maybe we should do that sooner > rather than later. You are at least the second person (after Junio) with that wish. Please note, however, that the purpose of author-script reading/writing is very different in git-am vs rebase -i, or at least it used to be: read_env_script() in sequencer.c very specifically wants to construct an env parameter for use in run_command(). Don't let me stop you from trying to consolidate the two different code paths, though. Ciao, Dscho > > > [1]: https://public-inbox.org/git/86a7qwpt9g@idaemons.org/ > > [2]: > > https://public-inbox.org/git/f5b56540-d26a-044e-5f46-1d975f889...@talktalk.net/ > > > > Eric Sunshine (2): > > sequencer: fix "rebase -i --root" corrupting author header > > sequencer: fix "rebase -i --root" corrupting author header timezone > > > > sequencer.c | 9 +++-- > > t/t3404-rebase-interactive.sh | 10 +- > > 2 files changed, 16 insertions(+), 3 deletions(-) > > > >
Re: [PATCH 0/2] fix "rebase -i --root" corrupting root commit
Hi Eric, On Mon, 30 Jul 2018, Eric Sunshine wrote: > On Mon, Jul 30, 2018 at 5:30 AM Eric Sunshine wrote: > > This series fixes bugs causing corruption of the root commit when > > "rebase -i --root" is used to swap in a new root commit. In particular, > > the "author" header has trailing garbage. Some tools handle the > > corruption somewhat gracefully by showing a bogus date, but others barf > > on it (gitk, for instance). git-fsck correctly identifies the > > corruption. I discovered this after git-rebase corrupted one of my own > > projects. > > Unfortunately, after sending this series, I see that there is > additional corruption that needs to be addressed. In particular, the > "author" header time is incorrectly prefixed with "@", so this series > is going to need a re-roll to fix that, as well. AFAIR the `@` was not an oversight, but required so that we could pass in the Unix epoch. Ciao, Dscho
Re: [PATCH 1/2] sequencer: fix "rebase -i --root" corrupting author header
Hi Eric,
On Mon, 30 Jul 2018, Eric Sunshine wrote:
> When "git rebase -i --root" creates a new root commit (say, by swapping
> in a different commit for the root), it corrupts the commit's "author"
> header with trailing garbage:
>
> author A U Thor @1112912773 -0700...@example.com
>
> This is a result of read_author_ident() neglecting to NUL-terminate the
> buffer into which it composes the "author" header.
>
> (Note that the extra "0" in the timezone is a separate bug which will be
> fixed subsequently.)
>
> Security considerations: Construction of the "author" header by
> read_author_ident() happens in-place and in parallel with parsing the
> content of "rebase-merge/author-script" which occupies the same buffer.
> This is possible because the constructed "author" header is always
> smaller than the content of "rebase-merge/author-script". Despite
> neglecting to NUL-terminate the constructed "author" header, memory is
> never accessed (either by read_author_ident() or its caller) beyond the
> allocated buffer since a NUL-terminator is present at the end of the
> loaded "rebase-merge/author-script" content, and additional NUL's are
> inserted as part of the parsing process.
>
> Signed-off-by: Eric Sunshine
ACK!
Thanks,
Dscho
> ---
> sequencer.c | 2 +-
> t/t3404-rebase-interactive.sh | 10 +-
> 2 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 16c1411054..78864d9072 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -744,7 +744,7 @@ static const char *read_author_ident(struct strbuf *buf)
> return NULL;
> }
>
> - buf->len = out - buf->buf;
> + strbuf_setlen(buf, out - buf->buf);
> return buf->buf;
> }
>
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 01616901bd..8509c89a26 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -1238,7 +1238,7 @@ rebase_setup_and_clean () {
> test_might_fail git branch -D $1 &&
> test_might_fail git rebase --abort
> " &&
> - git checkout -b $1 master
> + git checkout -b $1 ${2:-master}
> }
>
> test_expect_success 'drop' '
> @@ -1415,4 +1415,12 @@ test_expect_success 'rebase -i --gpg-sign=
> overrides commit.gpgSign' '
> test_i18ngrep "$SQ-S\"S I Gner\"$SQ" err
> '
>
> +test_expect_success 'valid author header after --root swap' '
> + rebase_setup_and_clean author-header no-conflict-branch &&
> + set_fake_editor &&
> + FAKE_LINES="2 1" git rebase -i --root &&
> + git cat-file commit HEAD^ >out &&
> + grep "^author ..* @[0-9][0-9]* [-+][0-9][0-9]*$" out
> +'
> +
> test_done
> --
> 2.18.0.597.ga71716f1ad
>
>
[PATCH v2 8/9] vscode: add a dictionary for cSpell
From: Johannes Schindelin
The quite useful cSpell extension allows VS Code to have "squiggly"
lines under spelling mistakes. By default, this would add too much
clutter, though, because so much of Git's source code uses words that
would trigger cSpell.
Let's add a few words to make the spell checking more useful by reducing
the number of false positives.
Signed-off-by: Johannes Schindelin
---
contrib/vscode/init.sh | 169 -
1 file changed, 168 insertions(+), 1 deletion(-)
diff --git a/contrib/vscode/init.sh b/contrib/vscode/init.sh
index 29f2a729d..a134cb4c5 100755
--- a/contrib/vscode/init.sh
+++ b/contrib/vscode/init.sh
@@ -32,7 +32,174 @@ cat >.vscode/settings.json.new <<\EOF ||
"files.associations": {
"*.h": "c",
"*.c": "c"
-}
+},
+"cSpell.words": [
+"DATAW",
+"DBCACHED",
+"DFCHECK",
+"DTYPE",
+"Hamano",
+"HCAST",
+"HEXSZ",
+"HKEY",
+"HKLM",
+"IFGITLINK",
+"IFINVALID",
+"ISBROKEN",
+"ISGITLINK",
+"ISSYMREF",
+"Junio",
+"LPDWORD",
+"LPPROC",
+"LPWSTR",
+"MSVCRT",
+"NOARG",
+"NOCOMPLETE",
+"NOINHERIT",
+"RENORMALIZE",
+"STARTF",
+"STARTUPINFOEXW",
+"Schindelin",
+"UCRT",
+"YESNO",
+"argcp",
+"beginthreadex",
+"committish",
+"contentp",
+"cpath",
+"cpidx",
+"ctim",
+"dequote",
+"envw",
+"ewah",
+"fdata",
+"fherr",
+"fhin",
+"fhout",
+"fragp",
+"fsmonitor",
+"hnsec",
+"idents",
+"includeif",
+"interpr",
+"iprog",
+"isexe",
+"iskeychar",
+"kompare",
+"mksnpath",
+"mktag",
+"mktree",
+"mmblob",
+"mmbuffer",
+"mmfile",
+"noenv",
+"nparents",
+"ntpath",
+"ondisk",
+"ooid",
+"oplen",
+"osdl",
+"pnew",
+"pold",
+"ppinfo",
+"pushf",
+"pushv",
+"rawsz",
+"rebasing",
+"reencode",
+"repo",
+"rerere",
+"scld",
+"sharedrepo",
+"spawnv",
+"spawnve",
+"spawnvpe",
+"strdup'ing",
+"submodule",
+"submodules",
+"topath",
+"topo",
+"tpatch",
+"unexecutable",
+"unhide",
+"unkc",
+"unkv",
+"unmark",
+"unmatch",
+"unsets",
+"unshown",
+"untracked",
+"untrackedcache",
+"unuse",
+"upos",
+"uval",
+"vreportf",
+"wargs",
+"wargv",
+"wbuffer",
+"wcmd",
+"wcsnicmp",
+"wcstoutfdup",
+"wdeltaenv",
+"wdir",
+"wenv",
+"wenvblk",
+"wenvcmp",
+"wenviron",
+"wenvpos",
+"wenvsz",
+"wfile",
+"wfilename",
+"wfopen",
+"wfreopen",
+"wfullpath",
+"which'll",
+"wlink",
+"wmain",
+"wmkdir",
+"wmktemp",
+"wnewpath",
+"wotype",
+"wpath",
+"wpathname",
+"wpgmptr",
+"wpnew",
+"wpointer",
+"wpold",
+"wpos",
+"wputenv",
+"wrmdir",
+"wship",
+"wtarget",
+"wtemplate",
+"wunlink",
+"xcalloc",
+"xgetcwd",
+"xmallocz",
+"xmemdupz",
+"xmmap",
+"xopts",
+"xrealloc",
+"xsnprintf",
+"xutftowcs",
+"xutftowcsn",
+"xwcstoutf"
+],
+"cSpell.ignoreRegExpList": [
+"\\\"(DIRC|FSMN|REUC|UNTR)\\\"",
+"u[0-9a-fA-Fx]{4}\\b",
+"\\b(filfre|frotz|xyzzy)\\b",
+"\\bCMIT_FMT_DEFAULT\\b",
+"\\bde-munge\\b",
+"\\bGET_OID_DISAMBIGUATORS\\b",
+"\\bHASH_RENORMALIZE\\b",
+"\\bTREESAMEness\\b",
+"\\bUSE_STDEV\\b",
+"\\Wchar *\\*\\W*utfs\\W",
+"cURL's",
+"nedmalloc'ed",
+"ntifs\\.h",
+],
}
EOF
die "Could not write settings.json"
--
gitgitgadget
[PATCH v2 7/9] vscode: use 8-space tabs, no trailing ws, etc for Git's source code
From: Johannes Schindelin
This adds a couple settings for the .c/.h files so that it is easier to
conform to Git's conventions while editing the source code.
Signed-off-by: Johannes Schindelin
---
contrib/vscode/init.sh | 8
1 file changed, 8 insertions(+)
diff --git a/contrib/vscode/init.sh b/contrib/vscode/init.sh
index face115e8..29f2a729d 100755
--- a/contrib/vscode/init.sh
+++ b/contrib/vscode/init.sh
@@ -21,6 +21,14 @@ cat >.vscode/settings.json.new <<\EOF ||
"editor.wordWrap": "wordWrapColumn",
"editor.wordWrapColumn": 72
},
+"[c]": {
+"editor.detectIndentation": false,
+"editor.insertSpaces": false,
+"editor.tabSize": 8,
+"editor.wordWrap": "wordWrapColumn",
+"editor.wordWrapColumn": 80,
+"files.trimTrailingWhitespace": true
+},
"files.associations": {
"*.h": "c",
"*.c": "c"
--
gitgitgadget
[PATCH v2 4/9] mingw: define WIN32 explicitly
From: Johannes Schindelin This helps VS Code's intellisense to figure out that we want to include windows.h, and that we want to define the minimum target Windows version as Windows Vista/2008R2. Signed-off-by: Johannes Schindelin --- config.mak.uname | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config.mak.uname b/config.mak.uname index 684fc5bf0..2be2f1981 100644 --- a/config.mak.uname +++ b/config.mak.uname @@ -528,7 +528,7 @@ ifneq (,$(findstring MINGW,$(uname_S))) COMPAT_OBJS += compat/mingw.o compat/winansi.o \ compat/win32/pthread.o compat/win32/syslog.o \ compat/win32/dirent.o - BASIC_CFLAGS += -DPROTECT_NTFS_DEFAULT=1 + BASIC_CFLAGS += -DWIN32 -DPROTECT_NTFS_DEFAULT=1 EXTLIBS += -lws2_32 GITLIBS += git.res PTHREAD_LIBS = -- gitgitgadget
[PATCH v2 0/9] Add support to develop Git in Visual Studio Code
[Visual Studio Code](https://code.visualstudio.com/) (nickname "VS Code") is a light-weight, cross-platform, Open Source development environment, with an increasingly powerful extension to support C/C++ development. In particular the intellisense support as well as all the other niceties developers might have come to expect from Integrated Development Environments will help accelerate development. Due to the way Git's source code and build process is structured, it can be quite challenging to use VS Code effectively for developing Git. Which is a shame, as developing with vim and the command-line causes unnecessary churn, and it is quite understandable that most Git developers simply do not want to fight with a modern development environment just to try whether they like developing Git with it. This topic branch makes it easy to get started using VS Code to develop Git. Simply run the script `./contrib/vscode/init.sh`. This will initialize the `.vscode/` directory and some files in that directory. After that, just open Git's top-level directory as "folder" in VS Code. The files have to be generated because of the curious way Git determines what flags to pass to the C compiler, in particular which constants are defined, because they change the compile flow in rather dramatic ways (determining, e.g. which SHA-1 backend to use). Changes since v1: - Clarified commit message of the first commit. Johannes Schindelin (9): contrib: add a script to initialize VS Code configuration vscode: hard-code a couple defines cache.h: extract enum declaration from inside a struct declaration mingw: define WIN32 explicitly vscode: only overwrite C/C++ settings vscode: wrap commit messages at column 72 by default vscode: use 8-space tabs, no trailing ws, etc for Git's source code vscode: add a dictionary for cSpell vscode: let cSpell work on commit messages, too .gitignore| 1 + cache.h | 24 ++- config.mak.uname | 2 +- contrib/vscode/.gitattributes | 1 + contrib/vscode/README.md | 14 ++ contrib/vscode/init.sh| 375 ++ 6 files changed, 405 insertions(+), 12 deletions(-) create mode 100644 contrib/vscode/.gitattributes create mode 100644 contrib/vscode/README.md create mode 100755 contrib/vscode/init.sh base-commit: 53f9a3e157dbbc901a02ac2c73346d375e24978c Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-2%2Fdscho%2Fvscode-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2/dscho/vscode-v2 Pull-Request: https://github.com/gitgitgadget/git/pull/2 Range-diff vs v1: 1: e2e449a00 ! 1: bbf13e40a contrib: add a script to initialize VS Code configuration @@ -4,11 +4,14 @@ VS Code is a lightweight but powerful source code editor which runs on your desktop and is available for Windows, macOS and Linux. Among other -languages, it has support for C/C++ via an extension. +languages, it has support for C/C++ via an extension, which offers to +not only build and debug the code, but also Intellisense, i.e. +code-aware completion and similar niceties. -To start developing Git with VS Code, simply run the Unix shell script -contrib/vscode/init.sh, which creates the configuration files in -.vscode/ that VS Code consumes. +This patch adds a script that helps set up the environment to work +effectively with VS Code: simply run the Unix shell script +contrib/vscode/init.sh, which creates the relevant files, and open the +top level folder of Git's source code in VS Code. Signed-off-by: Johannes Schindelin 2: 3770bd855 = 2: 6c8b5a4f2 vscode: hard-code a couple defines 3: de49c4bf2 = 3: 105b50c62 cache.h: extract enum declaration from inside a struct declaration 4: b3ce2ccf4 = 4: 4b95b1e2a mingw: define WIN32 explicitly 5: 06dcf6d97 = 5: 3dd53c04c vscode: only overwrite C/C++ settings 6: 08212c57e = 6: 384b08836 vscode: wrap commit messages at column 72 by default 7: 2e880b6f1 = 7: ba78af756 vscode: use 8-space tabs, no trailing ws, etc for Git's source code 8: ce216cf43 = 8: 358f38d3a vscode: add a dictionary for cSpell 9: 4c2aa015a = 9: b9c628b88 vscode: let cSpell work on commit messages, too -- gitgitgadget
[PATCH v2 6/9] vscode: wrap commit messages at column 72 by default
From: Johannes Schindelin
When configuring VS Code as core.editor (via `code --wait`), we really
want to adhere to the Git conventions of wrapping commit messages.
Signed-off-by: Johannes Schindelin
---
contrib/vscode/init.sh | 4
1 file changed, 4 insertions(+)
diff --git a/contrib/vscode/init.sh b/contrib/vscode/init.sh
index ba9469226..face115e8 100755
--- a/contrib/vscode/init.sh
+++ b/contrib/vscode/init.sh
@@ -17,6 +17,10 @@ cat >.vscode/settings.json.new <<\EOF ||
{
"C_Cpp.intelliSenseEngine": "Default",
"C_Cpp.intelliSenseEngineFallback": "Disabled",
+"[git-commit]": {
+"editor.wordWrap": "wordWrapColumn",
+"editor.wordWrapColumn": 72
+},
"files.associations": {
"*.h": "c",
"*.c": "c"
--
gitgitgadget
[PATCH v2 9/9] vscode: let cSpell work on commit messages, too
From: Johannes Schindelin By default, the cSpell extension ignores all files under .git/. That includes, unfortunately, COMMIT_EDITMSG, i.e. commit messages. However, spell checking is *quite* useful when writing commit messages... And since the user hardly ever opens any file inside .git (apart from commit messages, the config, and sometimes interactive rebase's todo lists), there is really not much harm in *not* ignoring .git/. The default also ignores `node_modules/`, but that does not apply to Git, so let's skip ignoring that, too. Signed-off-by: Johannes Schindelin --- contrib/vscode/init.sh | 2 ++ 1 file changed, 2 insertions(+) diff --git a/contrib/vscode/init.sh b/contrib/vscode/init.sh index a134cb4c5..27de94994 100755 --- a/contrib/vscode/init.sh +++ b/contrib/vscode/init.sh @@ -33,6 +33,8 @@ cat >.vscode/settings.json.new <<\EOF || "*.h": "c", "*.c": "c" }, +"cSpell.ignorePaths": [ +], "cSpell.words": [ "DATAW", "DBCACHED", -- gitgitgadget
[PATCH v2 1/9] contrib: add a script to initialize VS Code configuration
From: Johannes Schindelin
VS Code is a lightweight but powerful source code editor which runs on
your desktop and is available for Windows, macOS and Linux. Among other
languages, it has support for C/C++ via an extension, which offers to
not only build and debug the code, but also Intellisense, i.e.
code-aware completion and similar niceties.
This patch adds a script that helps set up the environment to work
effectively with VS Code: simply run the Unix shell script
contrib/vscode/init.sh, which creates the relevant files, and open the
top level folder of Git's source code in VS Code.
Signed-off-by: Johannes Schindelin
---
.gitignore| 1 +
contrib/vscode/.gitattributes | 1 +
contrib/vscode/README.md | 14 +++
contrib/vscode/init.sh| 165 ++
4 files changed, 181 insertions(+)
create mode 100644 contrib/vscode/.gitattributes
create mode 100644 contrib/vscode/README.md
create mode 100755 contrib/vscode/init.sh
diff --git a/.gitignore b/.gitignore
index 388cc4bee..592e8f879 100644
--- a/.gitignore
+++ b/.gitignore
@@ -206,6 +206,7 @@
/config.mak.autogen
/config.mak.append
/configure
+/.vscode/
/tags
/TAGS
/cscope*
diff --git a/contrib/vscode/.gitattributes b/contrib/vscode/.gitattributes
new file mode 100644
index 0..e89f2236e
--- /dev/null
+++ b/contrib/vscode/.gitattributes
@@ -0,0 +1 @@
+init.sh whitespace=-indent-with-non-tab
diff --git a/contrib/vscode/README.md b/contrib/vscode/README.md
new file mode 100644
index 0..8202d6203
--- /dev/null
+++ b/contrib/vscode/README.md
@@ -0,0 +1,14 @@
+Configuration for VS Code
+=
+
+[VS Code](https://code.visualstudio.com/) is a lightweight but powerful source
+code editor which runs on your desktop and is available for
+[Windows](https://code.visualstudio.com/docs/setup/windows),
+[macOS](https://code.visualstudio.com/docs/setup/mac) and
+[Linux](https://code.visualstudio.com/docs/setup/linux). Among other languages,
+it has [support for C/C++ via an
extension](https://github.com/Microsoft/vscode-cpptools).
+
+To start developing Git with VS Code, simply run the Unix shell script called
+`init.sh` in this directory, which creates the configuration files in
+`.vscode/` that VS Code consumes. `init.sh` needs access to `make` and `gcc`,
+so run the script in a Git SDK shell if you are using Windows.
diff --git a/contrib/vscode/init.sh b/contrib/vscode/init.sh
new file mode 100755
index 0..3cc93243f
--- /dev/null
+++ b/contrib/vscode/init.sh
@@ -0,0 +1,165 @@
+#!/bin/sh
+
+die () {
+ echo "$*" >&2
+ exit 1
+}
+
+cd "$(dirname "$0")"/../.. ||
+die "Could not cd to top-level directory"
+
+mkdir -p .vscode ||
+die "Could not create .vscode/"
+
+# General settings
+
+cat >.vscode/settings.json <<\EOF ||
+{
+"C_Cpp.intelliSenseEngine": "Default",
+"C_Cpp.intelliSenseEngineFallback": "Disabled",
+"files.associations": {
+"*.h": "c",
+"*.c": "c"
+}
+}
+EOF
+die "Could not write settings.json"
+
+# Infer some setup-specific locations/names
+
+GCCPATH="$(which gcc)"
+GDBPATH="$(which gdb)"
+MAKECOMMAND="make -j5 DEVELOPER=1"
+OSNAME=
+X=
+case "$(uname -s)" in
+MINGW*)
+ GCCPATH="$(cygpath -am "$GCCPATH")"
+ GDBPATH="$(cygpath -am "$GDBPATH")"
+ MAKE_BASH="$(cygpath -am /git-cmd.exe) --command=usrbinbash.exe"
+ MAKECOMMAND="$MAKE_BASH -lc \\\"$MAKECOMMAND\\\""
+ OSNAME=Win32
+ X=.exe
+ ;;
+Linux)
+ OSNAME=Linux
+ ;;
+Darwin)
+ OSNAME=macOS
+ ;;
+esac
+
+# Default build task
+
+cat >.vscode/tasks.json <https://go.microsoft.com/fwlink/?LinkId=733558
+// for the documentation about the tasks.json format
+"version": "2.0.0",
+"tasks": [
+{
+"label": "make",
+"type": "shell",
+"command": "$MAKECOMMAND",
+"group": {
+"kind": "build",
+"isDefault": true
+}
+}
+]
+}
+EOF
+die "Could not install default build task"
+
+# Debugger settings
+
+cat >.vscode/launch.json <https://go.microsoft.com/fwlink/?linkid=830387
+"version": "0.2.0",
+"configurations": [
+{
+"name": "(gdb) Launch",
+"type": "cppdbg",
+"request": "launch",
+"program": "\${workspaceFolder}/git$X",
+"args": [],
+"stopAtEn
[PATCH v2 3/9] cache.h: extract enum declaration from inside a struct declaration
From: Johannes Schindelin
While it is technically possible, it is confusing. Not only the user,
but also VS Code's intellisense.
Signed-off-by: Johannes Schindelin
---
cache.h | 24 +---
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/cache.h b/cache.h
index 89a107a7f..2380136f6 100644
--- a/cache.h
+++ b/cache.h
@@ -1425,18 +1425,20 @@ extern void *read_object_with_reference(const struct
object_id *oid,
extern struct object *peel_to_type(const char *name, int namelen,
struct object *o, enum object_type);
+enum date_mode_type {
+ DATE_NORMAL = 0,
+ DATE_RELATIVE,
+ DATE_SHORT,
+ DATE_ISO8601,
+ DATE_ISO8601_STRICT,
+ DATE_RFC2822,
+ DATE_STRFTIME,
+ DATE_RAW,
+ DATE_UNIX
+};
+
struct date_mode {
- enum date_mode_type {
- DATE_NORMAL = 0,
- DATE_RELATIVE,
- DATE_SHORT,
- DATE_ISO8601,
- DATE_ISO8601_STRICT,
- DATE_RFC2822,
- DATE_STRFTIME,
- DATE_RAW,
- DATE_UNIX
- } type;
+ enum date_mode_type type;
const char *strftime_fmt;
int local;
};
--
gitgitgadget
[PATCH v2 5/9] vscode: only overwrite C/C++ settings
From: Johannes Schindelin
The C/C++ settings are special, as they are the only generated VS Code
configurations that *will* change over the course of Git's development,
e.g. when a new constant is defined.
Therefore, let's only update the C/C++ settings, also to prevent user
modifications from being overwritten.
Ideally, we would keep user modifications in the C/C++ settings, but
that would require parsing JSON, a task for which a Unix shell script is
distinctly unsuited. So we write out .new files instead, and warn the
user if they may want to reconcile their changes.
Signed-off-by: Johannes Schindelin
---
contrib/vscode/init.sh | 23 ---
1 file changed, 20 insertions(+), 3 deletions(-)
diff --git a/contrib/vscode/init.sh b/contrib/vscode/init.sh
index 494a51ac5..ba9469226 100755
--- a/contrib/vscode/init.sh
+++ b/contrib/vscode/init.sh
@@ -13,7 +13,7 @@ die "Could not create .vscode/"
# General settings
-cat >.vscode/settings.json <<\EOF ||
+cat >.vscode/settings.json.new <<\EOF ||
{
"C_Cpp.intelliSenseEngine": "Default",
"C_Cpp.intelliSenseEngineFallback": "Disabled",
@@ -51,7 +51,7 @@ esac
# Default build task
-cat >.vscode/tasks.json <.vscode/tasks.json.new <https://go.microsoft.com/fwlink/?LinkId=733558
// for the documentation about the tasks.json format
@@ -73,7 +73,7 @@ die "Could not install default build task"
# Debugger settings
-cat >.vscode/launch.json <.vscode/launch.json.new <
[PATCH v2 2/9] vscode: hard-code a couple defines
From: Johannes Schindelin
Sadly, we do not get all of the definitions via ALL_CFLAGS. Some defines
are passed to GCC *only* when compiling specific files, such as git.o.
Let's just hard-code them into the script for the time being.
Signed-off-by: Johannes Schindelin
---
contrib/vscode/init.sh | 14 +-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/contrib/vscode/init.sh b/contrib/vscode/init.sh
index 3cc93243f..494a51ac5 100755
--- a/contrib/vscode/init.sh
+++ b/contrib/vscode/init.sh
@@ -115,7 +115,19 @@ include Makefile
vscode-init:
@mkdir -p .vscode && \
incs= && defs= && \
- for e in $(ALL_CFLAGS); do \
+ for e in $(ALL_CFLAGS) \
+ '-DGIT_EXEC_PATH="$(gitexecdir_SQ)"' \
+ '-DGIT_LOCALE_PATH="$(localedir_relative_SQ)"' \
+ '-DBINDIR="$(bindir_relative_SQ)"' \
+ '-DFALLBACK_RUNTIME_PREFIX="$(prefix_SQ)"' \
+ '-DDEFAULT_GIT_TEMPLATE_DIR="$(template_dir_SQ)"' \
+ '-DETC_GITCONFIG="$(ETC_GITCONFIG_SQ)"' \
+ '-DETC_GITATTRIBUTES="$(ETC_GITATTRIBUTES_SQ)"' \
+ '-DGIT_LOCALE_PATH="$(localedir_relative_SQ)"' \
+ '-DCURL_DISABLE_TYPECHECK', \
+ '-DGIT_HTML_PATH="$(htmldir_relative_SQ)"' \
+ '-DGIT_MAN_PATH="$(mandir_relative_SQ)"' \
+ '-DGIT_INFO_PATH="$(infodir_relative_SQ)"'; do \
case "$$e" in \
-I.) \
incs="$$(printf '% 16s"$${workspaceRoot}",\n%s' \
--
gitgitgadget
Re: [PATCH 7/9] vscode: use 8-space tabs, no trailing ws, etc for Git's source code
Hi Hannes,
On Wed, 25 Jul 2018, Johannes Sixt wrote:
> Am 23.07.2018 um 15:52 schrieb Johannes Schindelin via GitGitGadget:
> > From: Johannes Schindelin
> >
> > This adds a couple settings for the .c/.h files so that it is easier to
> > conform to Git's conventions while editing the source code.
> >
> > Signed-off-by: Johannes Schindelin
> > ---
> > contrib/vscode/init.sh | 8
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/contrib/vscode/init.sh b/contrib/vscode/init.sh
> > index face115e8..29f2a729d 100755
> > --- a/contrib/vscode/init.sh
> > +++ b/contrib/vscode/init.sh
> > @@ -21,6 +21,14 @@ cat >.vscode/settings.json.new <<\EOF ||
> > "editor.wordWrap": "wordWrapColumn",
> > "editor.wordWrapColumn": 72
> > },
> > +"[c]": {
> > +"editor.detectIndentation": false,
> > +"editor.insertSpaces": false,
> > +"editor.tabSize": 8,
> > +"editor.wordWrap": "wordWrapColumn",
> > +"editor.wordWrapColumn": 80,
> > +"files.trimTrailingWhitespace": true
> > +},
>
> I am a VS Code user, but I haven't used these settings before.
>
> With these settings, does the editor break lines while I am typing? Or does it
> just insert a visual cue that tells where I should insert a line break? If the
> former, it would basically make the editor unusable for my taste. I want to
> have total control over the code I write. The 80 column limit is just a
> recommendation, not a hard requirement.
Fear not. It is giving you a very clear visual cue, but that's all. It
does show the line wrapped, but the line number column quite clearly shows
that it did not insert a new-line.
Ciao,
Dscho
>
> > "files.associations": {
> > "*.h": "c",
> > "*.c": "c"
> >
>
> -- Hannes
>
>
Re: [PATCH 2/9] vscode: hard-code a couple defines
Hi Jonathan, On Mon, 23 Jul 2018, Jonathan Nieder wrote: > Johannes Schindelin via GitGitGadget wrote: > > > From: Johannes Schindelin > > > > Sadly, we do not get all of the definitions via ALL_CFLAGS. Some defines > > are passed to GCC *only* when compiling specific files, such as git.o. > > > > Let's just hard-code them into the script for the time being. > > Could we move these to ALL_CFLAGS? Is there any downside other than > increased rebuilds when options change? I assumed that it gives us a bit more liberty at using names for constants that might otherwise be too general. I did not even consider the rebuild time, but that's a good point. The idea to move those definitions to ALL_CFLAGS sounds good to me, yet I am loathe to introduce this into a patch series that is not about far-reaching design decisions at all, but merely about making a decent tool available to the finger tips of all Git contributors. Read: I think this would make for a pretty good micro-project, maybe in next year's GSoC (assuming that it happens and that we're accepted again). Ciao, Dscho
Re: [PATCH 1/9] contrib: add a script to initialize VS Code configuration
Hi Junio, On Mon, 23 Jul 2018, Junio C Hamano wrote: > "Johannes Schindelin via GitGitGadget" > writes: > > > diff --git a/contrib/vscode/init.sh b/contrib/vscode/init.sh > > new file mode 100755 > > index 0..3cc93243f > > --- /dev/null > > +++ b/contrib/vscode/init.sh > > @@ -0,0 +1,165 @@ > > +#!/bin/sh > > This is caued by our usual "git apply --whitespace=warn" as it > contains lines indented by spaces not tabs (perhaps the json > literals?) A `git diff --check` does not show any red flag. > Can we have contrib/vscode/.gitattributes to tweak the whitespace > breakage checking rules so that this file is excempt? I would have appreciated quite a bit more clear a message, including how I can trigger this, and what is the appropriate setting. As it is, I cannot reproduce your problem with a `git show`, so I assume that the main problem is that this commit *does* add that .gitattributes, but of course your `git apply` ran before that file existed. For your own pleasure, here is the link to the .gitattributes file in the commit that you applied yourself: https://github.com/gitster/git/commit/44559f0388a4e256bea3eb2f57b92a2b9e3d06f9#diff-046ba41c5a21694c62b8dc5e93d7f0c2R1 I assume that this is good enough for you, and your comment is moot? Ciao, Dscho
Re: [PATCH 1/9] contrib: add a script to initialize VS Code configuration
Hi Jonathan, On Mon, 23 Jul 2018, Jonathan Nieder wrote: > Johannes Schindelin via GitGitGadget wrote: > > > From: Johannes Schindelin > > > > VS Code is a lightweight but powerful source code editor which runs on > > your desktop and is available for Windows, macOS and Linux. Among other > > languages, it has support for C/C++ via an extension. > > This doesn't really seem relevant to the change. The relevant bits > are that (1) VS Code is a popular source code editor, and that (2) > it's one worth recommending to newcomers. To the contrary. This paragraph describes the motivation of the patch. It is, if you want, the most important part of the entire patch series. > > To start developing Git with VS Code, simply run the Unix shell script > > contrib/vscode/init.sh, which creates the configuration files in > > .vscode/ that VS Code consumes. > > > > Signed-off-by: Johannes Schindelin > > This doesn't tell me what the patch will actually do. True. Will add a paragraph. > VSCode is an editor. Is the idea that this will help configure the > editor to do the right thing with whitespace or something? Or does it > configure IDE features to build git? It is a start of all of the above. Ideally, I will be eventually be able to consistently use VS Code for Git development, whether I have to work on a Linux, a macOS or a Windows machine. And I am not happy until the same ease is also available to others. Hence my contribution. Ciao, Dscho
range-diff, was Re: What's cooking in git.git (Jul 2018, #03; Wed, 25)
Hi Junio,
On Fri, 27 Jul 2018, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason writes:
>
> > Count me in the "this is useful" camp, but also I did look at the latest
> > submission this time around, but had nothing to say, so I didn't say
> > anything :)
>
> Please make it a habit to do say something to show that you did
> carefully review the series especially in such a case, which helps
> to differentiate a change that is interesting to nobody, and one
> that is so obviously well done that there is nothing to add.
>
> What I have been doing from time to time, when I think there is
> nothing bad in particular to point out, is to quote a part that is
> especially tricky to get right and think it aloud to show how I
> would reason why the result of the change is correct. This type of
> review helps in two and a half ways:
>
> (1) We can see a correction to what the reviewer said, which could
> lead to further improvement ("no, the code does not quite work
> that way, so it is still buggy", or "no, the code does not work
> that way, but triggering that bug is impossible because the
> caller high above will not call us in that case", etc.),
>
> (2) The reviewer can demonstrate to others that s/he understands
> the area being touched well enough to explain how it works
> correctly, and a "looks good to me" comment from the reviewer
> on that change becomes more credible than a mere "looked good
> and I have nothing else to add".
>
> Thanks.
FWIW I picked up your Asciidoc-underline fix, and I also fixed a typo in a
commit message (you may want to pick that up, too, unless you want me to
send a full new iteration, I don't care either way):
-- snipsnap --
11: bf0a5879361 ! 11: 0c1f5db5d01 range-diff: add tests
@@ -3,7 +3,7 @@
range-diff: add tests
These are essentially lifted from https://github.com/trast/tbdiff, with
-light touch-ups to account for the command now being names `git
+light touch-ups to account for the command now being named `git
range-diff`.
Apart from renaming `tbdiff` to `range-diff`, only one test case needed
Re: [PATCH] config: fix case sensitive subsection names on writing
Hi, On Fri, 27 Jul 2018, Junio C Hamano wrote: > Stefan Beller writes: > > [...] Thanks for the patch! The only thing that was not clear to me from the patch and from the commit message was: the first part *is* case insensitive, right? How does the patch take care of that? Is it relying on `git_config_parse_key()` to do that? If so, I don't see it... > I would still hold the judgment on "all except only this one" > myself. That's a bit too early in my mind. Agreed. I seem to remember that I had a funny problem "in the reverse", where http..* is case-sensitive, but in an unexpected way: if the URL contains upper-case characters, the part of the config key needs to be downcased, otherwise the setting won't be picked up. Ciao, Dscho
Re: Hash algorithm analysis
Hi Brian, On Tue, 24 Jul 2018, brian m. carlson wrote: > On Tue, Jul 24, 2018 at 02:13:07PM -0700, Junio C Hamano wrote: > > Yup. I actually was leaning toward saying "all of them are OK in > > practice, so the person who is actually spear-heading the work gets to > > choose", but if we picked SHA-256 now, that would not be a choice that > > Brian has to later justify for choosing against everybody else's > > wishes, which makes it the best choice ;-) > > This looks like a rough consensus. As I grew really uncomfortable with having a decision that seems to be based on hunches by non-experts (we rejected the preference of the only cryptographer who weighed in, after all, precisely like we did over a decade ago), I asked whether I could loop in one of our in-house experts with this public discussion. Y'all should be quite familiar with his work: Dan Shumow. Dan, thank you for agreeing to chime in publicly. Ciao, Dscho
Re: [PATCH] sequencer.c: terminate the last line of author-script properly
Hi Junio, On Thu, 26 Jul 2018, Junio C Hamano wrote: > Johannes Schindelin writes: > > > Hi Junio, > > > > On Tue, 17 Jul 2018, Junio C Hamano wrote: > > > >> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh > >> index 2d189da2f1..b0cef509ab 100755 > >> --- a/t/t3404-rebase-interactive.sh > >> +++ b/t/t3404-rebase-interactive.sh > >> @@ -81,11 +81,13 @@ test_expect_success 'rebase -i writes out > >> .git/rebase-merge/author-script in "ed > > > > You missed a very long line here. > > > >>set_fake_editor && > >>FAKE_LINES="edit 1" git rebase -i HEAD^ && > >>test -f .git/rebase-merge/author-script && > > > > Why do we need this, if we already have an `eval` later on? > > You are commenting on a wrong version. Comment on the original. Sorry, no time. Take this review, or leave it. Ciao, Dscho
Re: [PATCH] negotiator/skipping: skip commits during fetch
Hi Jonathan,
On Thu, 26 Jul 2018, Jonathan Tan wrote:
> > On Mon, 16 Jul 2018, Jonathan Tan wrote:
> >
> > > t/t5552-skipping-fetch-negotiator.sh | 179 +++
> >
> > This test seems to be failing consistently in the recent `pu` builds:
> >
> > https://git-for-windows.visualstudio.com/git/_build/results?buildId=14337=logs
> >
> > Could you have a look, please?
>
> Hmm...on my Linux computer, this test passes on both pu (as of the time
> of writing) and 838143aa5c ("Merge branch 'ab/newhash-is-sha256' into
> pu", 2018-07-25) (pu at the time of that build, according to the website
> you linked above). If you could rerun that test with additional code,
> could you add a "cat trace" and show me what the client sends?
I can give you something even better: a playground. Just open a PR at
https://github.com/gitgitgadget/git (all of the branches on gitster/git ar
mirrored, including yours, I am sure, so you can target that branch
specifically).
Once you open a Pull Request, it will automatically build and run the test
suite on Windows, macOS and Linux. You will see it in the "checks" section
on the bottom. Example for my range-diff series:
https://git-for-windows.visualstudio.com/git/_build/results?buildId=14279
For a quicker turnaround, you could add a commit that forces the `all`
target in `t/Makefile` to run only your test.
> When I do that, the relevant parts are:
>
> packet:fetch> have 9ab46928dc282aa09f4dbf96893a252e058e7e8e
> packet:fetch> have dc824fafb05f3229aedf1f320bbe572e35364dfe
> packet:fetch> have caef059de69917b9119176a11b88afcef769331d
> packet:fetch> have 41bd8dc092ee110ba80e350a346ec507ab2e42a0
> packet:fetch> have e9a2c092a8e911567a377c881a7f6031e7f892ea
> packet:fetch> done
>
> which is exactly as I (and the test) expect.
>
> Two possible reasons for the discrepancy that I can think of offhand are
> that (1) my computer generates different commits from your test system,
> and (2) the priority queue pops commits in a different order. For (1),
> that's not possible because the SHA-1s are the same (as can be seen by
> comparing your link and the "have" lines I quoted above), and for (2),
> the code seems OK:
>
> static int compare(const void *a_, const void *b_, void *unused)
> {
> const struct entry *a = a_;
> const struct entry *b = b_;
> return compare_commits_by_commit_date(a->commit, b->commit, NULL);
> }
>
> Let me know if you can observe the output of "cat trace" or if you have
> any other ideas.
Like I said, you can use those "CI" builds, I think that would be more
effective than if you waited for me to react, I am quite overwhelmed these
days.
Ciao,
Dscho
Re: [RFC PATCH 2/5] format-patch: add --range-diff option to embed diff in cover letter
Hi Eric,
On Thu, 26 Jul 2018, Eric Sunshine wrote:
> On Thu, Jul 26, 2018 at 6:56 AM Johannes Schindelin
> wrote:
> > On Tue, 17 Jul 2018, Eric Sunshine wrote:
> > > On Tue, Jul 17, 2018 at 6:31 AM Johannes Schindelin
> > > wrote:
> > > > BTW I like to have an extra space in front of all the range-diff
> > > > lines, to make it easier to discern them from the rest.
> > >
> > > I'm not sure what you mean. Perhaps I'm misreading your comment.
> >
> > Sorry, I was really unclear.
> >
> > In the cover letters sent out by GitGitGadget (or earlier, my
> > mail-patch-series.sh command), I took pains to indent the entire
> > range-diff (or interdiff) with a single space. That is, the footer
> > "Range-diff vs v:" is not indented at all, but all subsequent lines
> > of the range-diff have a leading space.
> >
> > The original reason was to stop confusing `git apply` when sending an
> > interdiff as part of a single patch without a cover letter (in which
> > case mail-patch-series.sh inserted the interdiff below the `---`
> > marker, and the interdiff would have looked like the start of the real
> > diff otherwise).
>
> The new version[1] likewise indents the interdiff to avoid confusing
> git-am / git-apply.
>
> [1]:
> https://public-inbox.org/git/20180722095717.17912-1-sunsh...@sunshineco.com/
Great!
> > In the meantime, I got used to this indentation so much that I do not
> > want to miss it, it is a relatively easy and intuitive visual marker.
> >
> > This, however, will be harder to achieve now that you are using the
> > libified range-diff.
>
> I toyed with indenting the range-diff in both the cover letter and
> below the "---" line in a patch. With the libified range-diff, doing
> so involves modifying the range-diff implementation (rather than
> having the consumer of the range-diff manage the indentation locally),
> so it adds a bit of complexity to show_range_diff(), though perhaps
> not too much.
>
> However, I opted against it for a few reasons. First, "header" lines
> apart, all lines of the range-diff are already indented, and the
> existing indentation was sufficient (for me, at least) as a visual
> marker. Second, range-diffs tend to be _wide_, especially the header
> lines, and I was loath to make it wider by indenting more. Third, due
> to the existing indentation of the diff proper, a range-diff won't
> confuse git-am / git-apply, nor will the unindented header lines, so
> extra indentation seemed superfluous.
Totally understandable. For some reason, I never thought about that fact
(a range-diff is *not* a diff) when changing mail-patch-series.ts to use
range-diffs instead of interdiffs.
> > > > > @@ -1438,6 +1480,7 @@ int cmd_format_patch(int argc, const char
> > > > > **argv, const char *prefix)
> > > > > + const char *range_diff = NULL;
> > > >
> > > > Maybe `range_diff_opt`? It's not exactly the range diff that is
> > > > contained in this variable.
> > >
> > > I could, though I was trying to keep it shorter rather than longer.
> > > This is still the same in the re-roll, but I can rename it if you
> > > insist.
> >
> > I think it will confuse me in the future if I read `range_diff` and
> > even the data type suggests that it could hold the output of a `git
> > range-diff ` run.
> >
> > So I would like to insist.
>
> In the new version[1], this variable is named 'rdiff_prev' (the
> "previous" version against which the range-diff is to be generated).
Thank you.
> > > > > +format_patch () {
> > > > > + title=$1 &&
> > > > > + range=$2 &&
> > > > > + test_expect_success "format-patch --range-diff ($title)" '
> > > > > + git format-patch --stdout --cover-letter
> > > > > --range-diff=$range \
> > > > > + master..unmodified >actual &&
> > > > > + grep "= 1: .* s/5/A" actual &&
> > > > > + grep "= 2: .* s/4/A" actual &&
> > > > > + grep "= 3: .* s/11/B" actual &&
> > > > > + grep "= 4: .* s/12/B" actual
> > > >
> > > > I guess this might make sense if `format_patch` was not a
> > > > function, but it is specifically marked as a function... so...
> > > > shouldn't these `grep`s also be using function parameters?
> > >
>
Re: [RFC PATCH] sequencer: fix quoting in write_author_script
Hi Phillip, Junio and Akinori,
I just noticed that t3404 is broken without my patches (but with Junio's
fixup), on Windows, macOS and Linux. (See log at the end.)
On Fri, 27 Jul 2018, Phillip Wood wrote:
> On 26/07/18 13:33, Johannes Schindelin wrote:
> >
> > On Wed, 18 Jul 2018, Phillip Wood wrote:
> >
> >> Single quotes should be escaped as \' not \\'. Note that this only
> >> affects authors that contain a single quote and then only external
> >> scripts that read the author script and users whose git is upgraded from
> >> the shell version of rebase -i while rebase was stopped. This is because
> >> the parsing in read_env_script() expected the broken version and for
> >> some reason sq_dequote() called by read_author_ident() seems to handle
> >> the broken quoting correctly.
> >>
> >> Ideally write_author_script() would be rewritten to use
> >> split_ident_line() and sq_quote_buf() but this commit just fixes the
> >> immediate bug.
> >
> >> This is untested, unfortuantely I don't have really have time to write a
> >> test or
> >> follow this up at the moment, if someone else want to run with it then
> >> please
> >> do.
> >
> > I modified the test that was added by Akinori. As it was added very early,
> > and as there is still a test case *after* Akinori's that compares a
> > hard-coded SHA-1, I refrained from using `test_commit` (which would change
> > that SHA-1). See below.
>
> Thanks for adding a test, that sounds like sensible approach, however
> having thought about it I wonder if we should just be writing a plain
> text file (e.g rebase-merge/author-data) and fixing the reader to read
> that if it exists and only then fall back to reading the legacy
> rebase-merge/author-script with a fix to correctly handle the script
> written by the shell version - what do you think? The author-script
> really should be just an implementation detail. If anyone really wants
> to read it they can still do 'read -r l' and split the lines with
> ${l%%=*} and ${l#*=}
In contrast to `git am`, there *is* a use case where power users might
have come to rely on the presence of the .git/rebase-merge/author-script
file *and* its nature as a shell script snippet: we purposefully allow
scripting `rebase -i`.
So I don't think that we can declare the file and its format as
implementation detail, even if the idea is very, very tempting.
> >> diff --git a/sequencer.c b/sequencer.c
> >> index 5354d4d51e..0b78d1f100 100644
> >> --- a/sequencer.c
> >> +++ b/sequencer.c
> >> @@ -638,21 +638,21 @@ static int write_author_script(const char *message)
> >>else if (*message != '\'')
> >>strbuf_addch(, *(message++));
> >>else
> >> - strbuf_addf(, "'%c'", *(message++));
> >> + strbuf_addf(, "'\\%c'", *(message++));
> >>strbuf_addstr(, "'\nGIT_AUTHOR_EMAIL='");
> >>while (*message && *message != '\n' && *message != '\r')
> >>if (skip_prefix(message, "> ", ))
> >>break;
> >>else if (*message != '\'')
> >>strbuf_addch(, *(message++));
> >>else
> >> - strbuf_addf(, "'%c'", *(message++));
> >> + strbuf_addf(, "'\\%c'", *(message++));
> >>strbuf_addstr(, "'\nGIT_AUTHOR_DATE='@");
> >>while (*message && *message != '\n' && *message != '\r')
> >>if (*message != '\'')
> >>strbuf_addch(, *(message++));
> >>else
> >> - strbuf_addf(, "'%c'", *(message++));
> >> + strbuf_addf(, "'\\%c'", *(message++));
> >>res = write_message(buf.buf, buf.len, rebase_path_author_script(), 1);
> >
> > I resolved the merge conflict with Akinori's patch. FWIW I pushed all of
> > this, including the fixup to Junio's fixup to the
> > `fix-t3404-author-script-test` branch at https://github.com/dscho/git.
> >
> >>strbuf_release();
> >>return res;
> >> @@ -666,13 +666,21 @@ static int read_env_script(struct argv_array *env)
> >> {
> >>struct strbuf script = STRBUF_INIT;
> >>int i, count = 0;
> >> - char *p, *p2;
> >> + const char *p2;
> >> + char *p;
> >>
> >>if (strbuf_read_file(, rebase_path_author_script(), 256) <= 0
Re: [RFC PATCH 0/5] format-patch: automate cover letter range-diff
Hi Andrei, On Thu, 26 Jul 2018, Andrei Rybak wrote: > On 2018-05-30 10:03, Eric Sunshine wrote: > > Dscho recently implemented a 'tbdiff' replacement as a Git builtin named > > git-branch-diff[1] which computes differences between two versions of a > > patch series. Such a diff can be a useful aid for reviewers when > > inserted into a cover letter. However, doing so requires manual > > generation (invoking git-branch-diff) and copy/pasting the result into > > the cover letter. > > > > This series fully automates the process by adding a --range-diff option > > to git-format-patch. > > [...] > > > > > Eric Sunshine (5): > > format-patch: allow additional generated content in > > make_cover_letter() > > format-patch: add --range-diff option to embed diff in cover letter > > format-patch: extend --range-diff to accept revision range > > format-patch: teach --range-diff to respect -v/--reroll-count > > format-patch: add --creation-weight tweak for --range-diff > > > > Documentation/git-format-patch.txt | 18 + > > builtin/log.c | 119 - > > t/t7910-branch-diff.sh | 16 > > 3 files changed, 132 insertions(+), 21 deletions(-) > > Would it make sense to mention new option in the cover letter section of > Documentation/SubmittingPatches? I would be hesitant to add it there already. Let's prove first that these options are really as useful as we hope they are. It might turn out that in many cases, the range-diff is just stupidly unreadable, for example. Personally, I already miss the color coding when looking at range-diffs sent via mails. Ciao, Johannes
Re: [PATCH] negotiator/skipping: skip commits during fetch
Hi Jonathan, On Thu, 26 Jul 2018, Johannes Schindelin wrote: > On Mon, 16 Jul 2018, Jonathan Tan wrote: > > > t/t5552-skipping-fetch-negotiator.sh | 179 +++ > > This test seems to be failing consistently in the recent `pu` builds: > > https://git-for-windows.visualstudio.com/git/_build/results?buildId=14337=logs It now also causes `next` builds to fail: https://git-for-windows.visualstudio.com/git/_build/results?buildId=14345=logs Please have a look, Dscho > Could you have a look, please? > > Ciao, > Dscho > > P.S.: For your convenience, I will paste the last part of the output with > `-i -v -x` here: > > -- snipsnap -- > 2018-07-26T08:18:39.7864833Z expecting success: > 2018-07-26T08:18:39.7868553Z rm -rf server client trace && > 2018-07-26T08:18:39.7869403Z git init server && > 2018-07-26T08:18:39.7869606Z test_commit -C server to_fetch && > 2018-07-26T08:18:39.7870066Z > 2018-07-26T08:18:39.7870281Z git init client && > 2018-07-26T08:18:39.7870403Z > 2018-07-26T08:18:39.7870579Z # 2 regular commits > 2018-07-26T08:18:39.7870779Z test_tick=20 && > 2018-07-26T08:18:39.7870943Z test_commit -C client c1 && > 2018-07-26T08:18:39.7871103Z test_commit -C client c2 && > 2018-07-26T08:18:39.7871228Z > 2018-07-26T08:18:39.7871419Z # 4 old commits > 2018-07-26T08:18:39.7871575Z test_tick=10 && > 2018-07-26T08:18:39.7871734Z git -C client checkout c1 && > 2018-07-26T08:18:39.7871916Z test_commit -C client old1 && > 2018-07-26T08:18:39.7872081Z test_commit -C client old2 && > 2018-07-26T08:18:39.7872396Z test_commit -C client old3 && > 2018-07-26T08:18:39.7872598Z test_commit -C client old4 && > 2018-07-26T08:18:39.7872743Z > 2018-07-26T08:18:39.7872918Z # "c2" and "c1" are popped first, then "old4" > to "old1". "old1" would > 2018-07-26T08:18:39.7873114Z # normally be skipped, but is treated as a > commit without a parent here > 2018-07-26T08:18:39.7873329Z # and sent, because (due to clock skew) its > only parent has already been > 2018-07-26T08:18:39.7873524Z # popped off the priority queue. > 2018-07-26T08:18:39.7873700Z test_config -C client > fetch.negotiationalgorithm skipping && > 2018-07-26T08:18:39.7873908Z GIT_TRACE_PACKET="$(pwd)/trace" git -C client > fetch "$(pwd)/server" && > 2018-07-26T08:18:39.7874091Z have_sent c2 c1 old4 old2 old1 && > 2018-07-26T08:18:39.7874262Z have_not_sent old3 > 2018-07-26T08:18:39.7874383Z > 2018-07-26T08:18:39.8353323Z ++ rm -rf server client trace > 2018-07-26T08:18:40.3404166Z ++ git init server > 2018-07-26T08:18:40.3756394Z Initialized empty Git repository in > D:/a/1/s/t/trash directory.t5552-skipping-fetch-negotiator/server/.git/ > 2018-07-26T08:18:40.3769512Z ++ test_commit -C server to_fetch > 2018-07-26T08:18:40.3776271Z ++ notick= > 2018-07-26T08:18:40.3777103Z ++ signoff= > 2018-07-26T08:18:40.3777282Z ++ indir= > 2018-07-26T08:18:40.3777465Z ++ test 3 '!=' 0 > 2018-07-26T08:18:40.3777648Z ++ case "$1" in > 2018-07-26T08:18:40.3777801Z ++ indir=server > 2018-07-26T08:18:40.3777948Z ++ shift > 2018-07-26T08:18:40.3778093Z ++ shift > 2018-07-26T08:18:40.3778493Z ++ test 1 '!=' 0 > 2018-07-26T08:18:40.3778921Z ++ case "$1" in > 2018-07-26T08:18:40.3779072Z ++ break > 2018-07-26T08:18:40.3779241Z ++ indir=server/ > 2018-07-26T08:18:40.3779431Z ++ file=to_fetch.t > 2018-07-26T08:18:40.3779603Z ++ echo to_fetch > 2018-07-26T08:18:40.3779923Z ++ git -C server/ add to_fetch.t > 2018-07-26T08:18:40.4072248Z ++ test -z '' > 2018-07-26T08:18:40.4072727Z ++ test_tick > 2018-07-26T08:18:40.4072948Z ++ test -z set > 2018-07-26T08:18:40.4073113Z ++ test_tick=1112913673 > 2018-07-26T08:18:40.4073758Z ++ GIT_COMMITTER_DATE='1112913673 -0700' > 2018-07-26T08:18:40.4074001Z ++ GIT_AUTHOR_DATE='1112913673 -0700' > 2018-07-26T08:18:40.4074178Z ++ export GIT_COMMITTER_DATE GIT_AUTHOR_DATE > 2018-07-26T08:18:40.4074357Z ++ git -C server/ commit -m to_fetch > 2018-07-26T08:18:40.4485364Z [master (root-commit) ff85695] to_fetch > 2018-07-26T08:18:40.4485997Z Author: A U Thor > 2018-07-26T08:18:40.4486201Z 1 file changed, 1 insertion(+) > 2018-07-26T08:18:40.4486414Z create mode 100644 to_fetch.t > 2018-07-26T08:18:40.4499970Z ++ git -C server/ tag to_fetch > 2018-07-26T08:18:40.4809208Z ++ git init client > 2018-07-26T08:18:40.5139949Z Initialized empty Git repository in > D:/a/1/s/t/trash directory.t5552-skipping-fetch-negotiator/client/.git/ > 2018-07-26T08:18:40.5158270Z ++ test_tick=20 > 2018-07-26T08:18:40.5158
Re: [RFC PATCH 0/5] Add delta islands support
Hi Chris,
On Sun, 22 Jul 2018, Christian Couder wrote:
> This patch series is upstreaming work made by GitHub and available in:
>
> https://github.com/peff/git/commits/jk/delta-islands
>
> The patch in the above branch has been split into 5 patches with their
> own new commit message, but no other change has been made.
>
> I kept Peff as the author and took the liberty to add his
> Signed-off-by to all the patches.
>
> The patches look good to me except perhaps that "pack.islandCore" is
> not documented. Maybe something could be added in
> Documentation/technical/ too, or maybe improving commit messages could
> be enough.
>
> Anyway I wanted to send this nearly "as is" first to get early
> feedback about what should be done.
>
> This patch series is also available on GitHub in:
>
> https://github.com/chriscool/git/commits/delta-islands
It might make sense to explain *somewhere* in the cover letter what the
patches are all about, i.e. what problem they are supposed to solve. I did
not see any indication here.
Ciao,
Dscho
>
> Jeff King (5):
> packfile: make get_delta_base() non static
> Add delta-islands.{c,h}
> pack-objects: add delta-islands support
> repack: add delta-islands support
> t: add t9930-delta-islands.sh
>
> Documentation/config.txt | 8 +
> Documentation/git-pack-objects.txt | 88 ++
> Documentation/git-repack.txt | 5 +
> Makefile | 1 +
> builtin/pack-objects.c | 130 +---
> builtin/repack.c | 9 +
> delta-islands.c| 490 +
> delta-islands.h| 11 +
> pack-objects.h | 4 +
> packfile.c | 10 +-
> packfile.h | 3 +
> t/t9930-delta-islands.sh | 143 +
> 12 files changed, 856 insertions(+), 46 deletions(-)
> create mode 100644 delta-islands.c
> create mode 100644 delta-islands.h
> create mode 100755 t/t9930-delta-islands.sh
>
> --
> 2.18.0.237.gffdb1dbdaa
>
>
Re: [PATCH] sequencer.c: terminate the last line of author-script properly
Hi Phillip, On Thu, 19 Jul 2018, Phillip Wood wrote: > On 18/07/18 18:17, Junio C Hamano wrote: > > Phillip Wood writes: > > > >>> (I think we had code to do so in "git am" > >>> that was rewritten in C first). > >> > >> The code in builtin/am.c doesn't try to write valid posix shell (if > >> one assumes it is the only consumer of the author script then it > >> doesn't need to) which results in simpler code, but external scripts > >> cannot safely eval it anymore. > > > > Are you sure about that? If so we probably should see if we can fix> the > > writer, and better yet, if we can share code with the writer > > discussed here, as presumably we are fixing it in this thread. > > > > But I do not see how builtin/am.c::write_author_script() would > > produce something that would not eval correctly. sq_quote_buf() was > > introduced specifically to write correct string for shell's > > consumption. > > You're right, I'm not sure how I missed the calls to sq_quote_buf() > yesterday, sharing the am code with the sequencer would clean things up > nicely. No, actually Phillip was right. The `author-script` file written by `git-am` was always an implementation detail, and as there was no (intended) way to call shell scripts while running `git-am`, the only shell script to intentionally use `author-script` was `git-am` itself. Ever since `git-am` is a builtin, the `author-script` file format could be changed, because it is an implementation detail, no more nor less, and I think it *should* be changed, too. We're spending useless cycles on quoting and dequoting, when writing a NUL-separated list of var=value pairs would be totally sufficient to our ends. Ciao, Dscho
Re: [RFC PATCH] sequencer: fix quoting in write_author_script
Hi Phillip,
On Wed, 18 Jul 2018, Phillip Wood wrote:
> From: Phillip Wood
>
> Single quotes should be escaped as \' not \\'. Note that this only
> affects authors that contain a single quote and then only external
> scripts that read the author script and users whose git is upgraded from
> the shell version of rebase -i while rebase was stopped. This is because
> the parsing in read_env_script() expected the broken version and for
> some reason sq_dequote() called by read_author_ident() seems to handle
> the broken quoting correctly.
>
> Ideally write_author_script() would be rewritten to use
> split_ident_line() and sq_quote_buf() but this commit just fixes the
> immediate bug.
>
> Signed-off-by: Phillip Wood
> ---
Good catch.
> This is untested, unfortuantely I don't have really have time to write a test
> or
> follow this up at the moment, if someone else want to run with it then please
> do.
I modified the test that was added by Akinori. As it was added very early,
and as there is still a test case *after* Akinori's that compares a
hard-coded SHA-1, I refrained from using `test_commit` (which would change
that SHA-1). See below.
> diff --git a/sequencer.c b/sequencer.c
> index 5354d4d51e..0b78d1f100 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -638,21 +638,21 @@ static int write_author_script(const char *message)
> else if (*message != '\'')
> strbuf_addch(, *(message++));
> else
> - strbuf_addf(, "'%c'", *(message++));
> + strbuf_addf(, "'\\%c'", *(message++));
> strbuf_addstr(, "'\nGIT_AUTHOR_EMAIL='");
> while (*message && *message != '\n' && *message != '\r')
> if (skip_prefix(message, "> ", ))
> break;
> else if (*message != '\'')
> strbuf_addch(, *(message++));
> else
> - strbuf_addf(, "'%c'", *(message++));
> + strbuf_addf(, "'\\%c'", *(message++));
> strbuf_addstr(, "'\nGIT_AUTHOR_DATE='@");
> while (*message && *message != '\n' && *message != '\r')
> if (*message != '\'')
> strbuf_addch(, *(message++));
> else
> - strbuf_addf(, "'%c'", *(message++));
> + strbuf_addf(, "'\\%c'", *(message++));
> res = write_message(buf.buf, buf.len, rebase_path_author_script(), 1);
I resolved the merge conflict with Akinori's patch. FWIW I pushed all of
this, including the fixup to Junio's fixup to the
`fix-t3404-author-script-test` branch at https://github.com/dscho/git.
> strbuf_release();
> return res;
> @@ -666,13 +666,21 @@ static int read_env_script(struct argv_array *env)
> {
> struct strbuf script = STRBUF_INIT;
> int i, count = 0;
> - char *p, *p2;
> + const char *p2;
> + char *p;
>
> if (strbuf_read_file(, rebase_path_author_script(), 256) <= 0)
> return -1;
>
> for (p = script.buf; *p; p++)
> - if (skip_prefix(p, "'''", (const char **)))
> + /*
> + * write_author_script() used to escape "'" incorrectly as
> + * "'''" rather than "'\\''" so we check for the correct
> + * version the incorrect version in case git was upgraded while
> + * rebase was stopped.
> + */
> + if (skip_prefix(p, "'\\''", ) ||
> + skip_prefix(p, "'''", ))
I think in this form, it is possibly unsafe because it assumes that the
new code cannot generate output that would trigger that same code path.
Although I have to admit that I did not give this a great deal of thought.
In any case, if you have to think long and hard about some fix, it might
be better to go with something that is easier to reason about. So how
about this: we already know that the code is buggy, Akinori fixed the bug,
where the author-script missed its trailing single-quote. We can use this
as a tell-tale for *this* bug. Assuming that Junio will advance both your
and Akinori's fix in close proximity.
Again, this is pushed to the `fix-t3404-author-script-test` branch at
https://github.com/dscho/git; My fixup on top of your patch looks like
this (feel free to drop the sq_bug part and only keep the test part):
-- snipsnap --
diff --git a/sequencer.c b/sequencer.c
index 46c0b3e720f..7abe78dc78e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -573,13 +573,14 @@ static int write_author_script(const char *message)
static int read_env_script(struct argv_array *env)
{
struct strbuf script = STRBUF_INIT;
- int i, count = 0;
+ int i, count = 0, sq_bug;
const char *p2;
char *p;
if (strbuf_read_file(, rebase_path_author_script(), 256) <= 0)
return -1;
+ sq_bug = script.len && script.buf[script.len - 1] != '\'';
for (p = script.buf; *p; p++)
Re: [PATCH] sequencer.c: terminate the last line of author-script properly
Hi Junio, On Tue, 17 Jul 2018, Junio C Hamano wrote: > diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh > index 2d189da2f1..b0cef509ab 100755 > --- a/t/t3404-rebase-interactive.sh > +++ b/t/t3404-rebase-interactive.sh > @@ -81,11 +81,13 @@ test_expect_success 'rebase -i writes out > .git/rebase-merge/author-script in "ed You missed a very long line here. > set_fake_editor && > FAKE_LINES="edit 1" git rebase -i HEAD^ && > test -f .git/rebase-merge/author-script && Why do we need this, if we already have an `eval` later on? > - unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE && > - eval "$(cat .git/rebase-merge/author-script)" && > - test "$(git show --quiet --pretty=format:%an)" = "$GIT_AUTHOR_NAME" && > - test "$(git show --quiet --pretty=format:%ae)" = "$GIT_AUTHOR_EMAIL" && > - test "$(git show --quiet --date=raw --pretty=format:@%ad)" = > "$GIT_AUTHOR_DATE" > + ( > + sane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE && > + eval "$(cat .git/rebase-merge/author-script)" && Why not . .git/rebase-merge/author-script instead? Less roundabout, easier to read, I think. > + test "$(git show --quiet --pretty=format:%an)" = > "$GIT_AUTHOR_NAME" && How is this even working without `-s`? *clicketyclick* Ah, --quiet does this. Wait. `git show --quiet` is not even documented. All of those lines are too long, though. I am surprised you did not catch that. Besides, this would be more compact, less repetitive, *and* more readable as test "$(git show -s --date=raw --format=%an,%ae,@%ad)" = \ "$GIT_AUTHOR_NAME,$GIT_AUTHOR_EMAIL,$GIT_AUTHOR_DATE" t3404-rebase-interactive.sh already takes 8 minutes (last I checked, anyway) to run on a *fast* machine. There is absolutely no need to introduce even more spawning, not when it is so easily avoided. > + test "$(git show --quiet --pretty=format:%ae)" = > "$GIT_AUTHOR_EMAIL" && > + test "$(git show --quiet --date=raw --pretty=format:@%ad)" = > "$GIT_AUTHOR_DATE" It is a shame that we cannot use %at directly here. > + ) > ' > > test_expect_success 'rebase -i with the exec command' ' Note: this is not a criticism of the original patch. It is a criticism of the review which could really have been better. I also saw that the test_when_finished uses a shell construct that shell script aficionados might like, but these days it is a lot better to use `test_might_fail` instead. Let's do this, then. So here goes, the clean-up patch on top of your 843654e435e (why does it have to be so darned tedious to get from a mail to the corresponding commit in `pu`), in all its glory: -- snipsnap -- diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index b0cef509ab7..97f0b4bf881 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -75,18 +75,16 @@ test_expect_success 'rebase --keep-empty' ' test_line_count = 6 actual ' -test_expect_success 'rebase -i writes out .git/rebase-merge/author-script in "edit" that sh(1) can parse' ' - test_when_finished "git rebase --abort ||:" && +test_expect_success 'rebase -i writes correct author-script' ' + test_when_finished "test_might_fail git rebase --abort" && git checkout master && set_fake_editor && FAKE_LINES="edit 1" git rebase -i HEAD^ && - test -f .git/rebase-merge/author-script && ( sane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE && - eval "$(cat .git/rebase-merge/author-script)" && - test "$(git show --quiet --pretty=format:%an)" = "$GIT_AUTHOR_NAME" && - test "$(git show --quiet --pretty=format:%ae)" = "$GIT_AUTHOR_EMAIL" && - test "$(git show --quiet --date=raw --pretty=format:@%ad)" = "$GIT_AUTHOR_DATE" + . .git/rebase-merge/author-script && + test "$(git show -s --date=raw --format=%an,%ae,@%ad)" = \ + "$GIT_AUTHOR_NAME,$GIT_AUTHOR_EMAIL,$GIT_AUTHOR_DATE" ) '
Re: [RFC PATCH 2/5] format-patch: add --range-diff option to embed diff in cover letter
Hi Eric,
On Tue, 17 Jul 2018, Eric Sunshine wrote:
> On Tue, Jul 17, 2018 at 6:31 AM Johannes Schindelin
> wrote:
> > On Wed, 30 May 2018, Eric Sunshine wrote:
>
> > > + if (range_diff) {
> > > + struct argv_array ranges = ARGV_ARRAY_INIT;
> > > + infer_diff_ranges(, range_diff, head);
> > > + if (get_range_diff(, ))
> > > + die(_("failed to generate range-diff"));
> >
> > BTW I like to have an extra space in front of all the range-diff lines, to
> > make it easier to discern them from the rest.
>
> I'm not sure what you mean. Perhaps I'm misreading your comment.
Sorry, I was really unclear.
In the cover letters sent out by GitGitGadget (or earlier, my
mail-patch-series.sh command), I took pains to indent the entire
range-diff (or interdiff) with a single space. That is, the footer
"Range-diff vs v:" is not indented at all, but all subsequent lines of
the range-diff have a leading space.
The original reason was to stop confusing `git apply` when sending an
interdiff as part of a single patch without a cover letter (in which case
mail-patch-series.sh inserted the interdiff below the `---` marker, and
the interdiff would have looked like the start of the real diff
otherwise).
In the meantime, I got used to this indentation so much that I do not want
to miss it, it is a relatively easy and intuitive visual marker.
This, however, will be harder to achieve now that you are using the
libified range-diff.
> > > @@ -1438,6 +1480,7 @@ int cmd_format_patch(int argc, const char **argv,
> > > const char *prefix)
> > > + const char *range_diff = NULL;
> >
> > Maybe `range_diff_opt`? It's not exactly the range diff that is contained
> > in this variable.
>
> I could, though I was trying to keep it shorter rather than longer.
> This is still the same in the re-roll, but I can rename it if you
> insist.
I think it will confuse me in the future if I read `range_diff` and even
the data type suggests that it could hold the output of a `git range-diff
` run.
So I would like to insist.
> > > +format_patch () {
> > > + title=$1 &&
> > > + range=$2 &&
> > > + test_expect_success "format-patch --range-diff ($title)" '
> > > + git format-patch --stdout --cover-letter
> > > --range-diff=$range \
> > > + master..unmodified >actual &&
> > > + grep "= 1: .* s/5/A" actual &&
> > > + grep "= 2: .* s/4/A" actual &&
> > > + grep "= 3: .* s/11/B" actual &&
> > > + grep "= 4: .* s/12/B" actual
> >
> > I guess this might make sense if `format_patch` was not a function, but it
> > is specifically marked as a function... so... shouldn't these `grep`s also
> > be using function parameters?
>
> A later patch adds a second test which specifies the same ranges but
> in a different way, so the result will be the same, hence the
> hard-coded grep'ing. The function avoids repetition across the two
> tests. I suppose I could do this a bit differently, though, to avoid
> pretending it's a general-purpose function.
If you can think of a way that would make this easier to read for, say,
myself if I ever find myself debugging a regression caught by this test, I
would appreciate that.
Ciao,
Dscho
Re: [PATCH v2 0/6] git-submodule.sh: convert part of cmd_update to C
Hi Stefan, On Tue, 17 Jul 2018, Stefan Beller wrote: > > A tangent. > > > > Because this "-- " is a conventional signature separator, MUAs like > > Emacs message-mode seems to omit everything below it from the quote > > while responding, making it cumbersome to comment on the tbdiff. > > > > Something to think about if somebody is contemplating on adding more > > to format-patch's cover letter. > > +cc Eric who needs to think about this tangent, then. > https://public-inbox.org/git/20180530080325.37520-1-sunsh...@sunshineco.com/ I think this is just a natural fall-out from the users' choice of mail program. Personally, I have no difficulty commenting on anything below the `--` separator. FWIW GitGitGadget follows the example of the `base-commit` footer and places this information *above* the `--` separator. > > >> 1: d4e1ec45740 ! 1: bbc8697a8ca git-submodule.sh: align error > > >> reporting for update mode to use path > > >> @@ -6,7 +6,6 @@ > > >> on its path, so let's do that for invalid update modes, too. > > >> > > >> Signed-off-by: Stefan Beller > > >> -Signed-off-by: Junio C Hamano > > >> > > >> diff --git a/git-submodule.sh b/git-submodule.sh > > >> --- a/git-submodule.sh > > > > This is quite unfortunate. I wonder if it is easy to tell > > range-diff that certain differences in the log message are to be > > ignored so that we can show that the first patch is unchanged in a > > case like this. This series has 4 really changed ones with 2 > > otherwise unchanged ones shown all as changed, which is not too bad, > > but for a series like sb/diff-colro-move-more reroll that has 9 > > patches, out of only two have real updated patches, showing > > otherwise unchanged 7 as changed like this hunk does would make the > > cover letter useless. It is a shame that adding range-diff to the > > cover does have so much potential. > > Actually I thought it was really cool, i.e. when using your queued branch > instead of my last sent branch, I can see any edits *you* did > (including fixing up typos or applying at slightly different bases). This is probably a good indicator that the practice on insisting on signing off on every patch, rather than just the merge commit, is something to re-think. Those are real changes relative to the original commit, after all, and if they are not desired, they should not be made. > The sign offs are a bit unfortunate as they are repetitive. > I have two conflicting points of view on that: > > (A) This sign off is inherent to the workflow. So we could > change the workflow, i.e. you pull series instead of applying them. > I think this "more in git, less in email" workflow would find supporters, > such as DScho (cc'd). > > The downside is that (1) you'd have to change your > workflow, i.e. instead of applying the patches at the base you think is > best for maintenance you'd have to tell the author "please rebase to $X"; > but that also has upsides, such as "If you want to have your series integrated > please merge with $Y and $Z" (looking at the object store stuff). > > The other (2) downside is that everyone else (authors, reviewers) have > to adapt as well. For authors this might be easy to adapt (push instead > of sending email sounds like a win). For reviewers we'd need to have > an easy way to review things "stored in git" and not exposed via email, > which is not obvious how to do. > > (B) The other point of view that I can offer is that we teach range-diff > to ignore certain patterns. Maybe in combination with interpret-trailers > this can be an easy configurable thing, or even a default to ignore > all sign offs? I thought about that myself. The reason: I was surprised, a couple of times, when I realized long after the fact, that some of my patches were changed without my knowledge nor blessing before being merged into `master`. To allow me to protest in a timely manner, I wanted to teach GitGitGadget (which is the main reason I work on range-diff, as you undoubtedly suspect by now) to warn me about such instances. The range-diff patch series has simmered too long at this stage, though, and I did not try to address such a "ignore " feature *specifically* so that the range-diff command could be available sooner than later. I already missed one major version, please refrain from forcing me to miss another one. Ciao, Dscho
Re: [PATCH] negotiator/skipping: skip commits during fetch
Hi Jonathan, On Mon, 16 Jul 2018, Jonathan Tan wrote: > t/t5552-skipping-fetch-negotiator.sh | 179 +++ This test seems to be failing consistently in the recent `pu` builds: https://git-for-windows.visualstudio.com/git/_build/results?buildId=14337=logs Could you have a look, please? Ciao, Dscho P.S.: For your convenience, I will paste the last part of the output with `-i -v -x` here: -- snipsnap -- 2018-07-26T08:18:39.7864833Z expecting success: 2018-07-26T08:18:39.7868553Zrm -rf server client trace && 2018-07-26T08:18:39.7869403Zgit init server && 2018-07-26T08:18:39.7869606Ztest_commit -C server to_fetch && 2018-07-26T08:18:39.7870066Z 2018-07-26T08:18:39.7870281Zgit init client && 2018-07-26T08:18:39.7870403Z 2018-07-26T08:18:39.7870579Z# 2 regular commits 2018-07-26T08:18:39.7870779Ztest_tick=20 && 2018-07-26T08:18:39.7870943Ztest_commit -C client c1 && 2018-07-26T08:18:39.7871103Ztest_commit -C client c2 && 2018-07-26T08:18:39.7871228Z 2018-07-26T08:18:39.7871419Z# 4 old commits 2018-07-26T08:18:39.7871575Ztest_tick=10 && 2018-07-26T08:18:39.7871734Zgit -C client checkout c1 && 2018-07-26T08:18:39.7871916Ztest_commit -C client old1 && 2018-07-26T08:18:39.7872081Ztest_commit -C client old2 && 2018-07-26T08:18:39.7872396Ztest_commit -C client old3 && 2018-07-26T08:18:39.7872598Ztest_commit -C client old4 && 2018-07-26T08:18:39.7872743Z 2018-07-26T08:18:39.7872918Z# "c2" and "c1" are popped first, then "old4" to "old1". "old1" would 2018-07-26T08:18:39.7873114Z# normally be skipped, but is treated as a commit without a parent here 2018-07-26T08:18:39.7873329Z# and sent, because (due to clock skew) its only parent has already been 2018-07-26T08:18:39.7873524Z# popped off the priority queue. 2018-07-26T08:18:39.7873700Ztest_config -C client fetch.negotiationalgorithm skipping && 2018-07-26T08:18:39.7873908ZGIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch "$(pwd)/server" && 2018-07-26T08:18:39.7874091Zhave_sent c2 c1 old4 old2 old1 && 2018-07-26T08:18:39.7874262Zhave_not_sent old3 2018-07-26T08:18:39.7874383Z 2018-07-26T08:18:39.8353323Z ++ rm -rf server client trace 2018-07-26T08:18:40.3404166Z ++ git init server 2018-07-26T08:18:40.3756394Z Initialized empty Git repository in D:/a/1/s/t/trash directory.t5552-skipping-fetch-negotiator/server/.git/ 2018-07-26T08:18:40.3769512Z ++ test_commit -C server to_fetch 2018-07-26T08:18:40.3776271Z ++ notick= 2018-07-26T08:18:40.3777103Z ++ signoff= 2018-07-26T08:18:40.3777282Z ++ indir= 2018-07-26T08:18:40.3777465Z ++ test 3 '!=' 0 2018-07-26T08:18:40.3777648Z ++ case "$1" in 2018-07-26T08:18:40.3777801Z ++ indir=server 2018-07-26T08:18:40.3777948Z ++ shift 2018-07-26T08:18:40.3778093Z ++ shift 2018-07-26T08:18:40.3778493Z ++ test 1 '!=' 0 2018-07-26T08:18:40.3778921Z ++ case "$1" in 2018-07-26T08:18:40.3779072Z ++ break 2018-07-26T08:18:40.3779241Z ++ indir=server/ 2018-07-26T08:18:40.3779431Z ++ file=to_fetch.t 2018-07-26T08:18:40.3779603Z ++ echo to_fetch 2018-07-26T08:18:40.3779923Z ++ git -C server/ add to_fetch.t 2018-07-26T08:18:40.4072248Z ++ test -z '' 2018-07-26T08:18:40.4072727Z ++ test_tick 2018-07-26T08:18:40.4072948Z ++ test -z set 2018-07-26T08:18:40.4073113Z ++ test_tick=1112913673 2018-07-26T08:18:40.4073758Z ++ GIT_COMMITTER_DATE='1112913673 -0700' 2018-07-26T08:18:40.4074001Z ++ GIT_AUTHOR_DATE='1112913673 -0700' 2018-07-26T08:18:40.4074178Z ++ export GIT_COMMITTER_DATE GIT_AUTHOR_DATE 2018-07-26T08:18:40.4074357Z ++ git -C server/ commit -m to_fetch 2018-07-26T08:18:40.4485364Z [master (root-commit) ff85695] to_fetch 2018-07-26T08:18:40.4485997Z Author: A U Thor 2018-07-26T08:18:40.4486201Z 1 file changed, 1 insertion(+) 2018-07-26T08:18:40.4486414Z create mode 100644 to_fetch.t 2018-07-26T08:18:40.4499970Z ++ git -C server/ tag to_fetch 2018-07-26T08:18:40.4809208Z ++ git init client 2018-07-26T08:18:40.5139949Z Initialized empty Git repository in D:/a/1/s/t/trash directory.t5552-skipping-fetch-negotiator/client/.git/ 2018-07-26T08:18:40.5158270Z ++ test_tick=20 2018-07-26T08:18:40.5158466Z ++ test_commit -C client c1 2018-07-26T08:18:40.5159077Z ++ notick= 2018-07-26T08:18:40.5159492Z ++ signoff= 2018-07-26T08:18:40.5159697Z ++ indir= 2018-07-26T08:18:40.5159855Z ++ test 3 '!=' 0 2018-07-26T08:18:40.5160010Z ++ case "$1" in 2018-07-26T08:18:40.5160209Z ++ indir=client 2018-07-26T08:18:40.5160362Z ++ shift 2018-07-26T08:18:40.5160507Z ++ shift 2018-07-26T08:18:40.5160657Z ++ test 1 '!=' 0 2018-07-26T08:18:40.5160831Z ++ case "$1" in 2018-07-26T08:18:40.5161289Z ++ break 2018-07-26T08:18:40.5161582Z ++ indir=client/ 2018-07-26T08:18:40.5161764Z ++ file=c1.t 2018-07-26T08:18:40.5161916Z ++ echo c1 2018-07-26T08:18:40.5162231Z ++ git -C client/ add c1.t 2018-07-26T08:18:40.5456318Z ++ test -z '' 2018-07-26T08:18:40.5460548Z ++ test_tick 2018-07-26T08:18:40.5461417Z ++ test -z set
Re: Hash algorithm analysis
Hi Joan, On Sun, 22 Jul 2018, Joan Daemen wrote: > I wanted to react to some statements I read in this discussion. But > first let me introduce myself. I'm Joan Daemen and I'm working in > symmetric cryptography since 1988. Vincent Rijmen and I designed > Rijndael that was selected to become AES and Guido Bertoni, Michael > Peeters and Gilles Van Assche and I (the Keccak team, later extended > with Ronny Van Keer) designed Keccak that was selected to become SHA3. > Of course as a member of the Keccak team I'm biased in this discussion > but I'll try to keep it factual. Thank you *so* much for giving your valuable time and expertise on this subject. I really would hate for the decision to be made due to opinions of people who are overconfident in their abilities to judge cryptographic matters despite clearly being out of their league (which includes me, I want to add specifically). On a personal note: back in the day, I have been following the Keccak with a lot of interest, being intrigued by the deliberate deviation from the standard primitives, and I am pretty much giddy about the fact that I am talking to you right now. > [... interesting, and thorough background information ...] > > Anyway, these numbers support the opinion that the safety margins taken > in K12 are better understood than those in SHA-256, SHA-512 and BLAKE2. This is very, very useful information in my mind. > Adam Langley continues: > > Thus I think the question is primarily concerned with performance and > implementation availability > > > Table 2 in our ACNS paper on K12 (available at > https://eprint.iacr.org/2016/770) shows that performance of K12 is quite > competitive. Moreover, there is a lot of code available under CC0 > license in the Keccak Code Package on github > https://github.com/gvanas/KeccakCodePackage. If there is shortage of > code for some platforms in the short term, we will be happy to work on that. > > In the long term, it is likely that the relative advantage of K12 will > increase as it has more potential for hardware acceleration, e.g., by > instruction set extension. This is thanks to the fact that it does not > use addition, as opposed to so-called addition-xor-rotation (ARX) > designs such as the SHA-2 and BLAKE2 families. This is already > illustrated in our Table 2 I referred to above, in the transition from > Skylake to SkylakeX. I *really* hope that more accessible hardware acceleration for this materializes at some stage. And by "more accessible", I mean commodity hardware such as ARM or AMD/Intel processors: big hosters could relatively easily develop appropriate FPGAs (we already do this for AI, after all). > Maybe also interesting for this discussion are the two notes we (Keccak > team) wrote on our choice to not go for ARX and the one on "open source > crypto" at https://keccak.team/2017/not_arx.html and > https://keccak.team/2017/open_source_crypto.html respectively. I had read those posts when they came out, and still find them insightful. Hopefully other readers of this mailing list will spend the time to read them, too. Again, thank you so much for a well-timed dose of domain expertise in this thread. Ciao, Dscho
Re: Hash algorithm analysis
Hi Eric, On Sun, 22 Jul 2018, Eric Deplagne wrote: > On Sun, 22 Jul 2018 14:21:48 +, brian m. carlson wrote: > > On Sun, Jul 22, 2018 at 11:34:42AM +0200, Eric Deplagne wrote: > > > On Sat, 21 Jul 2018 23:59:41 +, brian m. carlson wrote: > > > > I don't know your colleagues, and they haven't commented here. One > > > > person that has commented here is Adam Langley. It is my impression > > > > (and anyone is free to correct me if I'm incorrect) that he is indeed a > > > > cryptographer. To quote him[0]: > > > > > > > > I think this group can safely assume that SHA-256, SHA-512, BLAKE2, > > > > K12, etc are all secure to the extent that I don't believe that making > > > > comparisons between them on that axis is meaningful. Thus I think the > > > > question is primarily concerned with performance and implementation > > > > availability. > > > > > > > > […] > > > > > > > > So, overall, none of these choices should obviously be excluded. The > > > > considerations at this point are not cryptographic and the tradeoff > > > > between implementation ease and performance is one that the git > > > > community would have to make. > > > > > > Am I completely out of the game, or the statement that > > > "the considerations at this point are not cryptographic" > > > is just the wrongest ? > > > > > > I mean, if that was true, would we not be sticking to SHA1 ? > > > > I snipped a portion of the context, but AGL was referring to the > > considerations involved in choosing from the proposed ones for NewHash. > > In context, he meant that the candidates for NewHash “are all secure” > > and are therefore a better choice than SHA-1. > > Maybe a little bit sensitive, but I really did read > "we don't care if it's weak or strong, that's not the matter". Thank you for your concern. I agree that we need to be careful in considering the security implications. We made that mistake before (IIRC there was a cryptographer who was essentially shouted off the list when he suggested *not* to hard-code SHA-1), and we should absolutely refrain from making that same mistake again. > > I think we can all agree that SHA-1 is weak and should be replaced. Indeed. So at this point, we already excluded pretty much all the unsafe options (although it does concern me that BLAKE2b has been weakened purposefully, I understand the reasoning, but still). Which means that by now, considering the security implications of the cipher is no longer a criterion that helps us whittle down the candidates further. So from my point of view, there are two criterions that can help us further: - Which cipher is the least likely to be broken (or just weakened by new attacks)? - As energy considerations not only ecologically inspired, but also in terms of money for elecricity: which cipher is most likely to get decent hardware support any time soon? Even if my original degree (prime number theory) is closer to cryptanalysis than pretty much all other prolific core Git contributors, I do not want you to trust *my* word on answering those questions. Therefore, I will ask my colleagues to enter the hornet's nest that is this mailing list. Ciao, Dscho
Re: [PATCH v4 00/21] Add `range-diff`, a `tbdiff` lookalike
Hi Stefan,
On Mon, 23 Jul 2018, Stefan Beller wrote:
> On Sat, Jul 21, 2018 at 3:04 PM Johannes Schindelin via GitGitGadget
> wrote:
>
> > Range-diff vs v3:
> >
> > 1: 39272eefc ! 1: f7e70689e linear-assignment: a function to solve
> > least-cost assignment problems
> > @@ -223,9 +223,7 @@
> > + BUG("negative j: %d", j);
> > + i = pred[j];
> > + column2row[j] = i;
> > -+ k = j;
> > -+ j = row2column[i];
> > -+ row2column[i] = k;
> > ++ SWAP(j, row2column[i]);
>
> The dual color option (as a default) really helps here. Thanks for that!
> Does it have to be renamed though? (It's more than two colors; originally
> it was inverting the beginning signs)
I understand (and understood) the "dual" to mean that there are two
separate coloring methods, the coloring of the inner, and the coloring of
the outer diff. (And in my mind, the dimming is not so much an "inner"
diff things as an "outer" diff thing.)
> Maybe --color=emphasize-later assuming there will be other modes for
> coloring, such as "diff-only", which would correspond with
> --no-dual-color, or "none" that will correspond would be --no-color. I
> imagine there could be more fancy things, hence I would propose a mode
> rather than a switch. (Feel free to push back if discussing naming here
> feels like bike shedding)
Your suggestion does not really feel like bike-shedding to me, here, I can
see the merit of it.
It's just that 1) overloading `--color` here would be cumbersome, as
`--color` is *already* a diff option that we actually use, and 2) I am not
all that certain that new fancy things will crop up anytime soon. It was
hard enough to think of the dimming feature, and then implementing it.
So while I think your idea has merit, I still think that we can do that
later. The --no-dual-color option can easily be deprecated in favor of,
say, --color-mode=, when (and if) new modes crop up.
> 2: 7f15b26d4ea ! 82: 88134121d2a Introduce `range-diff` to compare
> iterations of a topic branch
> [...]
> > diff --git a/Makefile b/Makefile
> > --- a/Makefile
> > +++ b/Makefile
>
> The line starting with --- is red (default removed color) and the line
> with +++ is green (default add color).
>
> Ideally these two lines and the third line above starting with "diff --git"
> would render in GIT_COLOR_BOLD ("METAINFO").
I see where you are coming from, but given how complicated it seems to me
to implement this (dual color mode gets away with working line-based for
the moment, and what you ask for would require state, and would not even
be fool-proof, as the `diff --git` line might not even be part of the
context.
Seeing how long this patch series has already simmered, I would want to
invoke that old adage "the perfect is the enemy of the good", and rather
see a version of range-diff enter Git's source code, if need be marked as
"EXPERIMENTAL" so that the maintainer can claim that it is okay to be
buggy, and then invite contributions from other sides than from me.
> > 16: dfa7b1e71 < -: - range-diff --dual-color: work around bogus
> > white-space warning
> > -: - > 16: f4252f2b2 range-diff --dual-color: fix bogus
> > white-space warning
>
> Ah; here my initial assumption of only reviewing the range-diff breaks down
> now.
> I'll dig into patch 16 separately.
Right. In this case, it is a total rewrite, anyway (and I'll have to ask
you to overlook my frustration with how complex and hard it is to work on
ws.c without breaking anything). For the sake of review, you should ignore
the old patch. Unless you find that the new version is so complex and
prone to introduce bugs (with which I would agree) that we should go back
to the original workaround, which is so easy to understand that there are
no obvious bugs lurking inside it.
> Maybe it is worth having an option to expand all "new" patches.
Sure. And I would love to have this in a separate patch series, as it is
well beyond the original purpose of this patch series to simply make
tbdiff a builtin.
I should have known better, of course, but I was really not keen on
improving range-diff *before* it enters the code base, to the point of
introducing new features that might very well introduce new regressions in
unrelated commands, too.
Essentially, I am declaring a feature-freeze on this patch series until it
enters `next`.
> (Given that the range-diff
> pr-1/dscho/branch-diff-v3...pr-1/dscho/branch-diff-v4 told me you used a
> different base, this is a hard problem, as I c
Re: No rule to make target `git-daemon'
Hi, On Fri, 20 Jul 2018, brian m. carlson wrote: > On Fri, Jul 20, 2018 at 05:51:46PM -0400, Jeffrey Walton wrote: > > > (If anyone is interested in first class Solaris support then I am > > happy to help. The patch set needed for the platform has been stable > > for the last couple of years). > > I'd certainly be interested for you to send it to the list. I'm not > sure I can provide a helpful review, since I don't run Solaris, but > having better support out of the box would definitely be nice. The cURL team has some SunOS build IIRC that was nicely integrated with their GitHub-based CI. Maybe we could have something similar? Ciao, Dscho
[PATCH 3/9] cache.h: extract enum declaration from inside a struct declaration
From: Johannes Schindelin
While it is technically possible, it is confusing. Not only the user,
but also VS Code's intellisense.
Signed-off-by: Johannes Schindelin
---
cache.h | 24 +---
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/cache.h b/cache.h
index 89a107a7f..2380136f6 100644
--- a/cache.h
+++ b/cache.h
@@ -1425,18 +1425,20 @@ extern void *read_object_with_reference(const struct
object_id *oid,
extern struct object *peel_to_type(const char *name, int namelen,
struct object *o, enum object_type);
+enum date_mode_type {
+ DATE_NORMAL = 0,
+ DATE_RELATIVE,
+ DATE_SHORT,
+ DATE_ISO8601,
+ DATE_ISO8601_STRICT,
+ DATE_RFC2822,
+ DATE_STRFTIME,
+ DATE_RAW,
+ DATE_UNIX
+};
+
struct date_mode {
- enum date_mode_type {
- DATE_NORMAL = 0,
- DATE_RELATIVE,
- DATE_SHORT,
- DATE_ISO8601,
- DATE_ISO8601_STRICT,
- DATE_RFC2822,
- DATE_STRFTIME,
- DATE_RAW,
- DATE_UNIX
- } type;
+ enum date_mode_type type;
const char *strftime_fmt;
int local;
};
--
gitgitgadget
[PATCH 7/9] vscode: use 8-space tabs, no trailing ws, etc for Git's source code
From: Johannes Schindelin
This adds a couple settings for the .c/.h files so that it is easier to
conform to Git's conventions while editing the source code.
Signed-off-by: Johannes Schindelin
---
contrib/vscode/init.sh | 8
1 file changed, 8 insertions(+)
diff --git a/contrib/vscode/init.sh b/contrib/vscode/init.sh
index face115e8..29f2a729d 100755
--- a/contrib/vscode/init.sh
+++ b/contrib/vscode/init.sh
@@ -21,6 +21,14 @@ cat >.vscode/settings.json.new <<\EOF ||
"editor.wordWrap": "wordWrapColumn",
"editor.wordWrapColumn": 72
},
+"[c]": {
+"editor.detectIndentation": false,
+"editor.insertSpaces": false,
+"editor.tabSize": 8,
+"editor.wordWrap": "wordWrapColumn",
+"editor.wordWrapColumn": 80,
+"files.trimTrailingWhitespace": true
+},
"files.associations": {
"*.h": "c",
"*.c": "c"
--
gitgitgadget
[PATCH 5/9] vscode: only overwrite C/C++ settings
From: Johannes Schindelin
The C/C++ settings are special, as they are the only generated VS Code
configurations that *will* change over the course of Git's development,
e.g. when a new constant is defined.
Therefore, let's only update the C/C++ settings, also to prevent user
modifications from being overwritten.
Ideally, we would keep user modifications in the C/C++ settings, but
that would require parsing JSON, a task for which a Unix shell script is
distinctly unsuited. So we write out .new files instead, and warn the
user if they may want to reconcile their changes.
Signed-off-by: Johannes Schindelin
---
contrib/vscode/init.sh | 23 ---
1 file changed, 20 insertions(+), 3 deletions(-)
diff --git a/contrib/vscode/init.sh b/contrib/vscode/init.sh
index 494a51ac5..ba9469226 100755
--- a/contrib/vscode/init.sh
+++ b/contrib/vscode/init.sh
@@ -13,7 +13,7 @@ die "Could not create .vscode/"
# General settings
-cat >.vscode/settings.json <<\EOF ||
+cat >.vscode/settings.json.new <<\EOF ||
{
"C_Cpp.intelliSenseEngine": "Default",
"C_Cpp.intelliSenseEngineFallback": "Disabled",
@@ -51,7 +51,7 @@ esac
# Default build task
-cat >.vscode/tasks.json <.vscode/tasks.json.new <https://go.microsoft.com/fwlink/?LinkId=733558
// for the documentation about the tasks.json format
@@ -73,7 +73,7 @@ die "Could not install default build task"
# Debugger settings
-cat >.vscode/launch.json <.vscode/launch.json.new <
[PATCH 4/9] mingw: define WIN32 explicitly
From: Johannes Schindelin This helps VS Code's intellisense to figure out that we want to include windows.h, and that we want to define the minimum target Windows version as Windows Vista/2008R2. Signed-off-by: Johannes Schindelin --- config.mak.uname | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config.mak.uname b/config.mak.uname index 684fc5bf0..2be2f1981 100644 --- a/config.mak.uname +++ b/config.mak.uname @@ -528,7 +528,7 @@ ifneq (,$(findstring MINGW,$(uname_S))) COMPAT_OBJS += compat/mingw.o compat/winansi.o \ compat/win32/pthread.o compat/win32/syslog.o \ compat/win32/dirent.o - BASIC_CFLAGS += -DPROTECT_NTFS_DEFAULT=1 + BASIC_CFLAGS += -DWIN32 -DPROTECT_NTFS_DEFAULT=1 EXTLIBS += -lws2_32 GITLIBS += git.res PTHREAD_LIBS = -- gitgitgadget
[PATCH 9/9] vscode: let cSpell work on commit messages, too
From: Johannes Schindelin By default, the cSpell extension ignores all files under .git/. That includes, unfortunately, COMMIT_EDITMSG, i.e. commit messages. However, spell checking is *quite* useful when writing commit messages... And since the user hardly ever opens any file inside .git (apart from commit messages, the config, and sometimes interactive rebase's todo lists), there is really not much harm in *not* ignoring .git/. The default also ignores `node_modules/`, but that does not apply to Git, so let's skip ignoring that, too. Signed-off-by: Johannes Schindelin --- contrib/vscode/init.sh | 2 ++ 1 file changed, 2 insertions(+) diff --git a/contrib/vscode/init.sh b/contrib/vscode/init.sh index a134cb4c5..27de94994 100755 --- a/contrib/vscode/init.sh +++ b/contrib/vscode/init.sh @@ -33,6 +33,8 @@ cat >.vscode/settings.json.new <<\EOF || "*.h": "c", "*.c": "c" }, +"cSpell.ignorePaths": [ +], "cSpell.words": [ "DATAW", "DBCACHED", -- gitgitgadget
[PATCH 8/9] vscode: add a dictionary for cSpell
From: Johannes Schindelin
The quite useful cSpell extension allows VS Code to have "squiggly"
lines under spelling mistakes. By default, this would add too much
clutter, though, because so much of Git's source code uses words that
would trigger cSpell.
Let's add a few words to make the spell checking more useful by reducing
the number of false positives.
Signed-off-by: Johannes Schindelin
---
contrib/vscode/init.sh | 169 -
1 file changed, 168 insertions(+), 1 deletion(-)
diff --git a/contrib/vscode/init.sh b/contrib/vscode/init.sh
index 29f2a729d..a134cb4c5 100755
--- a/contrib/vscode/init.sh
+++ b/contrib/vscode/init.sh
@@ -32,7 +32,174 @@ cat >.vscode/settings.json.new <<\EOF ||
"files.associations": {
"*.h": "c",
"*.c": "c"
-}
+},
+"cSpell.words": [
+"DATAW",
+"DBCACHED",
+"DFCHECK",
+"DTYPE",
+"Hamano",
+"HCAST",
+"HEXSZ",
+"HKEY",
+"HKLM",
+"IFGITLINK",
+"IFINVALID",
+"ISBROKEN",
+"ISGITLINK",
+"ISSYMREF",
+"Junio",
+"LPDWORD",
+"LPPROC",
+"LPWSTR",
+"MSVCRT",
+"NOARG",
+"NOCOMPLETE",
+"NOINHERIT",
+"RENORMALIZE",
+"STARTF",
+"STARTUPINFOEXW",
+"Schindelin",
+"UCRT",
+"YESNO",
+"argcp",
+"beginthreadex",
+"committish",
+"contentp",
+"cpath",
+"cpidx",
+"ctim",
+"dequote",
+"envw",
+"ewah",
+"fdata",
+"fherr",
+"fhin",
+"fhout",
+"fragp",
+"fsmonitor",
+"hnsec",
+"idents",
+"includeif",
+"interpr",
+"iprog",
+"isexe",
+"iskeychar",
+"kompare",
+"mksnpath",
+"mktag",
+"mktree",
+"mmblob",
+"mmbuffer",
+"mmfile",
+"noenv",
+"nparents",
+"ntpath",
+"ondisk",
+"ooid",
+"oplen",
+"osdl",
+"pnew",
+"pold",
+"ppinfo",
+"pushf",
+"pushv",
+"rawsz",
+"rebasing",
+"reencode",
+"repo",
+"rerere",
+"scld",
+"sharedrepo",
+"spawnv",
+"spawnve",
+"spawnvpe",
+"strdup'ing",
+"submodule",
+"submodules",
+"topath",
+"topo",
+"tpatch",
+"unexecutable",
+"unhide",
+"unkc",
+"unkv",
+"unmark",
+"unmatch",
+"unsets",
+"unshown",
+"untracked",
+"untrackedcache",
+"unuse",
+"upos",
+"uval",
+"vreportf",
+"wargs",
+"wargv",
+"wbuffer",
+"wcmd",
+"wcsnicmp",
+"wcstoutfdup",
+"wdeltaenv",
+"wdir",
+"wenv",
+"wenvblk",
+"wenvcmp",
+"wenviron",
+"wenvpos",
+"wenvsz",
+"wfile",
+"wfilename",
+"wfopen",
+"wfreopen",
+"wfullpath",
+"which'll",
+"wlink",
+"wmain",
+"wmkdir",
+"wmktemp",
+"wnewpath",
+"wotype",
+"wpath",
+"wpathname",
+"wpgmptr",
+"wpnew",
+"wpointer",
+"wpold",
+"wpos",
+"wputenv",
+"wrmdir",
+"wship",
+"wtarget",
+"wtemplate",
+"wunlink",
+"xcalloc",
+"xgetcwd",
+"xmallocz",
+"xmemdupz",
+"xmmap",
+"xopts",
+"xrealloc",
+"xsnprintf",
+"xutftowcs",
+"xutftowcsn",
+"xwcstoutf"
+],
+"cSpell.ignoreRegExpList": [
+"\\\"(DIRC|FSMN|REUC|UNTR)\\\"",
+"u[0-9a-fA-Fx]{4}\\b",
+"\\b(filfre|frotz|xyzzy)\\b",
+"\\bCMIT_FMT_DEFAULT\\b",
+"\\bde-munge\\b",
+"\\bGET_OID_DISAMBIGUATORS\\b",
+"\\bHASH_RENORMALIZE\\b",
+"\\bTREESAMEness\\b",
+"\\bUSE_STDEV\\b",
+"\\Wchar *\\*\\W*utfs\\W",
+"cURL's",
+"nedmalloc'ed",
+"ntifs\\.h",
+],
}
EOF
die "Could not write settings.json"
--
gitgitgadget
[PATCH 6/9] vscode: wrap commit messages at column 72 by default
From: Johannes Schindelin
When configuring VS Code as core.editor (via `code --wait`), we really
want to adhere to the Git conventions of wrapping commit messages.
Signed-off-by: Johannes Schindelin
---
contrib/vscode/init.sh | 4
1 file changed, 4 insertions(+)
diff --git a/contrib/vscode/init.sh b/contrib/vscode/init.sh
index ba9469226..face115e8 100755
--- a/contrib/vscode/init.sh
+++ b/contrib/vscode/init.sh
@@ -17,6 +17,10 @@ cat >.vscode/settings.json.new <<\EOF ||
{
"C_Cpp.intelliSenseEngine": "Default",
"C_Cpp.intelliSenseEngineFallback": "Disabled",
+"[git-commit]": {
+"editor.wordWrap": "wordWrapColumn",
+"editor.wordWrapColumn": 72
+},
"files.associations": {
"*.h": "c",
"*.c": "c"
--
gitgitgadget
[PATCH 0/9] Add support to develop Git in Visual Studio Code
[Visual Studio Code](https://code.visualstudio.com/) (nickname "VS Code") is a light-weight, cross-platform, Open Source development environment, with an increasingly powerful extension to support C/C++ development. In particular the intellisense support as well as all the other niceties developers might have come to expect from Integrated Development Environments will help accelerate development. This topic branch makes it easy to get started using VS Code to develop Git itself. To get started, run the script `./contrib/vscode/init.sh`. This will initialize the `.vscode/` directory and some files in that directory. After that, simply open Git's top-level directory as "folder" in VS Code. The files have to be generated because of the curious way Git determines what flags to pass to the C compiler, in particular which constants are defined, because they change the compile flow in rather dramatic ways (determining, e.g. which SHA-1 backend to use). Johannes Schindelin (9): contrib: add a script to initialize VS Code configuration vscode: hard-code a couple defines cache.h: extract enum declaration from inside a struct declaration mingw: define WIN32 explicitly vscode: only overwrite C/C++ settings vscode: wrap commit messages at column 72 by default vscode: use 8-space tabs, no trailing ws, etc for Git's source code vscode: add a dictionary for cSpell vscode: let cSpell work on commit messages, too .gitignore| 1 + cache.h | 24 ++- config.mak.uname | 2 +- contrib/vscode/.gitattributes | 1 + contrib/vscode/README.md | 14 ++ contrib/vscode/init.sh| 375 ++ 6 files changed, 405 insertions(+), 12 deletions(-) create mode 100644 contrib/vscode/.gitattributes create mode 100644 contrib/vscode/README.md create mode 100755 contrib/vscode/init.sh base-commit: 53f9a3e157dbbc901a02ac2c73346d375e24978c Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-2%2Fdscho%2Fvscode-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2/dscho/vscode-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/2 -- gitgitgadget
[PATCH 1/9] contrib: add a script to initialize VS Code configuration
From: Johannes Schindelin
VS Code is a lightweight but powerful source code editor which runs on
your desktop and is available for Windows, macOS and Linux. Among other
languages, it has support for C/C++ via an extension.
To start developing Git with VS Code, simply run the Unix shell script
contrib/vscode/init.sh, which creates the configuration files in
.vscode/ that VS Code consumes.
Signed-off-by: Johannes Schindelin
---
.gitignore| 1 +
contrib/vscode/.gitattributes | 1 +
contrib/vscode/README.md | 14 +++
contrib/vscode/init.sh| 165 ++
4 files changed, 181 insertions(+)
create mode 100644 contrib/vscode/.gitattributes
create mode 100644 contrib/vscode/README.md
create mode 100755 contrib/vscode/init.sh
diff --git a/.gitignore b/.gitignore
index 388cc4bee..592e8f879 100644
--- a/.gitignore
+++ b/.gitignore
@@ -206,6 +206,7 @@
/config.mak.autogen
/config.mak.append
/configure
+/.vscode/
/tags
/TAGS
/cscope*
diff --git a/contrib/vscode/.gitattributes b/contrib/vscode/.gitattributes
new file mode 100644
index 0..e89f2236e
--- /dev/null
+++ b/contrib/vscode/.gitattributes
@@ -0,0 +1 @@
+init.sh whitespace=-indent-with-non-tab
diff --git a/contrib/vscode/README.md b/contrib/vscode/README.md
new file mode 100644
index 0..8202d6203
--- /dev/null
+++ b/contrib/vscode/README.md
@@ -0,0 +1,14 @@
+Configuration for VS Code
+=
+
+[VS Code](https://code.visualstudio.com/) is a lightweight but powerful source
+code editor which runs on your desktop and is available for
+[Windows](https://code.visualstudio.com/docs/setup/windows),
+[macOS](https://code.visualstudio.com/docs/setup/mac) and
+[Linux](https://code.visualstudio.com/docs/setup/linux). Among other languages,
+it has [support for C/C++ via an
extension](https://github.com/Microsoft/vscode-cpptools).
+
+To start developing Git with VS Code, simply run the Unix shell script called
+`init.sh` in this directory, which creates the configuration files in
+`.vscode/` that VS Code consumes. `init.sh` needs access to `make` and `gcc`,
+so run the script in a Git SDK shell if you are using Windows.
diff --git a/contrib/vscode/init.sh b/contrib/vscode/init.sh
new file mode 100755
index 0..3cc93243f
--- /dev/null
+++ b/contrib/vscode/init.sh
@@ -0,0 +1,165 @@
+#!/bin/sh
+
+die () {
+ echo "$*" >&2
+ exit 1
+}
+
+cd "$(dirname "$0")"/../.. ||
+die "Could not cd to top-level directory"
+
+mkdir -p .vscode ||
+die "Could not create .vscode/"
+
+# General settings
+
+cat >.vscode/settings.json <<\EOF ||
+{
+"C_Cpp.intelliSenseEngine": "Default",
+"C_Cpp.intelliSenseEngineFallback": "Disabled",
+"files.associations": {
+"*.h": "c",
+"*.c": "c"
+}
+}
+EOF
+die "Could not write settings.json"
+
+# Infer some setup-specific locations/names
+
+GCCPATH="$(which gcc)"
+GDBPATH="$(which gdb)"
+MAKECOMMAND="make -j5 DEVELOPER=1"
+OSNAME=
+X=
+case "$(uname -s)" in
+MINGW*)
+ GCCPATH="$(cygpath -am "$GCCPATH")"
+ GDBPATH="$(cygpath -am "$GDBPATH")"
+ MAKE_BASH="$(cygpath -am /git-cmd.exe) --command=usrbinbash.exe"
+ MAKECOMMAND="$MAKE_BASH -lc \\\"$MAKECOMMAND\\\""
+ OSNAME=Win32
+ X=.exe
+ ;;
+Linux)
+ OSNAME=Linux
+ ;;
+Darwin)
+ OSNAME=macOS
+ ;;
+esac
+
+# Default build task
+
+cat >.vscode/tasks.json <https://go.microsoft.com/fwlink/?LinkId=733558
+// for the documentation about the tasks.json format
+"version": "2.0.0",
+"tasks": [
+{
+"label": "make",
+"type": "shell",
+"command": "$MAKECOMMAND",
+"group": {
+"kind": "build",
+"isDefault": true
+}
+}
+]
+}
+EOF
+die "Could not install default build task"
+
+# Debugger settings
+
+cat >.vscode/launch.json <https://go.microsoft.com/fwlink/?linkid=830387
+"version": "0.2.0",
+"configurations": [
+{
+"name": "(gdb) Launch",
+"type": "cppdbg",
+"request": "launch",
+"program": "\${workspaceFolder}/git$X",
+"args": [],
+"stopAtEntry": false,
+"cwd": "\${workspaceFolder}",
+"environment": [],
+"externalConsole": true,
+"MIMode&
[PATCH 2/9] vscode: hard-code a couple defines
From: Johannes Schindelin
Sadly, we do not get all of the definitions via ALL_CFLAGS. Some defines
are passed to GCC *only* when compiling specific files, such as git.o.
Let's just hard-code them into the script for the time being.
Signed-off-by: Johannes Schindelin
---
contrib/vscode/init.sh | 14 +-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/contrib/vscode/init.sh b/contrib/vscode/init.sh
index 3cc93243f..494a51ac5 100755
--- a/contrib/vscode/init.sh
+++ b/contrib/vscode/init.sh
@@ -115,7 +115,19 @@ include Makefile
vscode-init:
@mkdir -p .vscode && \
incs= && defs= && \
- for e in $(ALL_CFLAGS); do \
+ for e in $(ALL_CFLAGS) \
+ '-DGIT_EXEC_PATH="$(gitexecdir_SQ)"' \
+ '-DGIT_LOCALE_PATH="$(localedir_relative_SQ)"' \
+ '-DBINDIR="$(bindir_relative_SQ)"' \
+ '-DFALLBACK_RUNTIME_PREFIX="$(prefix_SQ)"' \
+ '-DDEFAULT_GIT_TEMPLATE_DIR="$(template_dir_SQ)"' \
+ '-DETC_GITCONFIG="$(ETC_GITCONFIG_SQ)"' \
+ '-DETC_GITATTRIBUTES="$(ETC_GITATTRIBUTES_SQ)"' \
+ '-DGIT_LOCALE_PATH="$(localedir_relative_SQ)"' \
+ '-DCURL_DISABLE_TYPECHECK', \
+ '-DGIT_HTML_PATH="$(htmldir_relative_SQ)"' \
+ '-DGIT_MAN_PATH="$(mandir_relative_SQ)"' \
+ '-DGIT_INFO_PATH="$(infodir_relative_SQ)"'; do \
case "$$e" in \
-I.) \
incs="$$(printf '% 16s"$${workspaceRoot}",\n%s' \
--
gitgitgadget
[PATCH 1/1] t7406: avoid failures solely due to timing issues
From: Johannes Schindelin Regression tests are automated tests which try to ensure a specific behavior. The idea is: if the test case fails, the behavior indicated in the test case's title regressed. If a regression test that fails, even occasionally, for any reason other than to indicate the particular regression(s) it tries to catch, it is less useful than when it really only fails when there is a bug in the (non-test) code that needs to be fixed. In the instance of the test case "submodule update --init --recursive from subdirectory" of the script t7406-submodule-update.sh, the exact output of a recursive clone is compared with a pre-generated one. And this is a racy test because the structure of the submodules only guarantees a *partial* order. The 'none' and the 'rebasing' submodules *can* be cloned in any order, which means that a mismatch with the hard-coded order does not necessarily indicate a bug in the tested code. See for example: https://git-for-windows.visualstudio.com/git/_build/results?buildId=14035=logs To prevent such false positives from unnecessarily costing time when investigating test failures, let's take the exact order of the lines out of the equation by sorting them before comparing them. This test script seems not to have any more test cases that try to verify any specific order in which recursive clones process the submodules, therefore this is the only test case that is changed in this manner. Signed-off-by: Johannes Schindelin --- t/t7406-submodule-update.sh | 11 ++- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh index 9e0d31700..4937ebb67 100755 --- a/t/t7406-submodule-update.sh +++ b/t/t7406-submodule-update.sh @@ -115,17 +115,17 @@ Submodule path '../super/submodule': checked out '$submodulesha1' EOF cat <expect2 +Cloning into '$pwd/recursivesuper/super/merging'... +Cloning into '$pwd/recursivesuper/super/none'... +Cloning into '$pwd/recursivesuper/super/rebasing'... +Cloning into '$pwd/recursivesuper/super/submodule'... Submodule 'merging' ($pwd/merging) registered for path '../super/merging' Submodule 'none' ($pwd/none) registered for path '../super/none' Submodule 'rebasing' ($pwd/rebasing) registered for path '../super/rebasing' Submodule 'submodule' ($pwd/submodule) registered for path '../super/submodule' -Cloning into '$pwd/recursivesuper/super/merging'... done. -Cloning into '$pwd/recursivesuper/super/none'... done. -Cloning into '$pwd/recursivesuper/super/rebasing'... done. -Cloning into '$pwd/recursivesuper/super/submodule'... done. EOF @@ -137,7 +137,8 @@ test_expect_success 'submodule update --init --recursive from subdirectory' ' git submodule update --init --recursive ../super >../../actual 2>../../actual2 ) && test_i18ncmp expect actual && - test_i18ncmp expect2 actual2 + sort actual2 >actual2.sorted && + test_i18ncmp expect2 actual2.sorted ' cat <expect2 -- gitgitgadget
[PATCH 0/1] t7406: avoid failures solely due to timing issues
This fixes a regression test that produces false positives occasionally: https://git-for-windows.visualstudio.com/git/_build/results?buildId=14035=logs Johannes Schindelin (1): t7406: avoid failures solely due to timing issues t/t7406-submodule-update.sh | 11 ++- 1 file changed, 6 insertions(+), 5 deletions(-) base-commit: b7bd9486b055c3f967a870311e704e3bb0654e4f Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-12%2Fdscho%2Fmore-robust-t7406-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-12/dscho/more-robust-t7406-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/12 -- gitgitgadget
Re: Hash algorithm analysis
Hi Brian, On Fri, 20 Jul 2018, brian m. carlson wrote: > On Mon, Jun 11, 2018 at 12:29:42PM -0700, Jonathan Nieder wrote: > > My understanding of the discussion so far: > > > > Keccak team encourages us[1] to consider a variant like K12 instead of > > SHA3. > > > > AGL explains[2] that the algorithms considered all seem like > > reasonable choices and we should decide using factors like > > implementation ease and performance. > > > > If we choose a Keccak-based function, AGL also[3] encourages using a > > variant like K12 instead of SHA3. > > > > Dscho strongly prefers[4] SHA-256, because of > > - wide implementation availability, including in future hardware > > - has been widely analyzed > > - is fast > > > > Yves Orton and Linus Torvalds prefer[5] SHA3 over SHA2 because of how > > it is constructed. > > I know this discussion has sort of petered out, but I'd like to see if > we can revive it. I'm writing index v3 and having a decision would help > me write tests for it. > > To summarize the discussion that's been had in addition to the above, > Ævar has also stated a preference for SHA-256 and I would prefer BLAKE2b > over SHA-256 over SHA3-256, although any of them would be fine. > > Are there other contributors who have a strong opinion? Are there > things I can do to help us coalesce around an option? Do you really want to value contributors' opinion more than cryptographers'? I mean, that's exactly what got us into this hard-coded SHA-1 mess in the first place. And to set the record straight: I do not have a strong preference of the hash algorithm. But cryprographers I have the incredible luck to have access to, by virtue of being a colleague, did mention their preference. I see no good reason to just blow their advice into the wind. Ciao, Dscho
Re: [PATCH v3 09/20] range-diff: adjust the output of the commit pairs
Hi Stefan, On Fri, 20 Jul 2018, Stefan Beller wrote: > > 1. To roll again. > > > > A player who rolls two sixes can reroll the dice for an additional > > turn. > > This is where I had my AHA moment! > (Consider my software development process as chaotic as a dice roll > So rerolling is really just rolling the dice again to "get my patch > accepted" ;-) Wouldn't that be nice? But you only get to reroll if you had two sixes. Tough luck for you, Stefan. > > 2. (programming) To convert (an unrolled instruction sequence) back into > >a loop. quotations ▼ > > We do not have unrolled loops? When resending patch series? *rolls eyes* > This was good back in the day where the cost of each instruction weighted > heavy on the CPU, such that the JMPs that are needed (and the loop > variable check that might have had a bad branch prediction) for the loop were > slowing down the execution. > > Nowadays (when I was studying 5 years ago) the branch prediction and > individual instruction execution are really good, but the bottleneck > that I measured (when I had a lot of time at my disposal and attending a > class/project on micro architectures), was the CPU instruction cache > size, i.e. loop unrolling made the code *slower* than keeping tight > loops loaded in memory. > https://stackoverflow.com/questions/24196076/is-gcc-loop-unrolling-flag-really-effective > > > Noun > > > > reroll (plural rerolls) > > > > (dice games) A situation in the rules of certain dice games where a > > player is given the option to reroll an undesirable roll of the dice. > > > > > > You will notice how this does not list *any* hint at referring to > > something that Junio calls "reroll". > > We have undesirable patches that were 'rolled' onto the mailing list, > so they have to be rerolled? > > > Footnote *1*: https://en.wiktionary.org/wiki/commit#Noun does not even > > bother to acknowledge our use of referring to a snapshot of a source code > > base as a "commit". > > When Git was a content addressable file system, a commit was precisely > "a database transaction, [...] making it a permanent change." > > Side note: > I was just giving a talk to my colleagues about diff aglorithms > (and eventually describing a bug in the histogram diff algorithm) > and we got really riled up with "Longest Common Subsequence", > as the mathematical definition is different than what the code > or I (after studying the code) had in mind. > > Naming things is hard, and sometimes the collective wisdom got > it wrong, but changing it would be very costly in the short/medium > term. My point is not that naming is hard. But picking names that are *different* from what is established nomenclature is... unwise. In this case, it makes an already unnecessarily awkward code contribution process even more unnecessarily uninviting. > Another note about "rolling things": At $DAYJOB I review changes > that are committed to the another revision control system w.r.t. its > compliance of open source licenses (hence I am exposed to a lot > of different projects), and some of those changes are titled > "Roll up to version $X" which I found strange, but knew > what was meant. To "roll up" is, as far as this non-native speaker can tell, an established way to express this action. In short: nothing you wrote can adequately defend why the Git project chooses to confuse new contributors seemingly on purpose. Ciao, Dscho
[PATCH v4 21/21] range-diff: use dim/bold cues to improve dual color mode
From: Johannes Schindelin It *is* a confusing thing to look at a diff of diffs. All too easy is it to mix up whether the -/+ markers refer to the "inner" or the "outer" diff, i.e. whether a `+` indicates that a line was added by either the old or the new diff (or both), or whether the new diff does something different than the old diff. To make things easier to process for normal developers, we introduced the dual color mode which colors the lines according to the commit diff, i.e. lines that are added by a commit (whether old, new, or both) are colored in green. In non-dual color mode, the lines would be colored according to the outer diff: if the old commit added a line, it would be colored red (because that line addition is only present in the first commit range that was specified on the command-line, i.e. the "old" commit, but not in the second commit range, i.e. the "new" commit). However, this dual color mode is still not making things clear enough, as we are looking at two levels of diffs, and we still only pick a color according to *one* of them (the outer diff marker is colored differently, of course, but in particular with deep indentation, it is easy to lose track of that outer diff marker's background color). Therefore, let's add another dimension to the mix. Still use green/red/normal according to the commit diffs, but now also dim the lines that were only in the old commit, and use bold face for the lines that are only in the new commit. That way, it is much easier not to lose track of, say, when we are looking at a line that was added in the previous iteration of a patch series but the new iteration adds a slightly different version: the obsolete change will be dimmed, the current version of the patch will be bold. At least this developer has a much easier time reading the range-diffs that way. Signed-off-by: Johannes Schindelin --- Documentation/config.txt | 6 -- Documentation/git-range-diff.txt | 17 + color.h | 6 ++ diff.c | 28 ++-- diff.h | 8 +++- 5 files changed, 52 insertions(+), 13 deletions(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index a32172a43..6dbfc9a09 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -1159,8 +1159,10 @@ color.diff.:: (highlighting whitespace errors), `oldMoved` (deleted lines), `newMoved` (added lines), `oldMovedDimmed`, `oldMovedAlternative`, `oldMovedAlternativeDimmed`, `newMovedDimmed`, `newMovedAlternative` - and `newMovedAlternativeDimmed` (See the '' - setting of '--color-moved' in linkgit:git-diff[1] for details). + `newMovedAlternativeDimmed` (See the '' + setting of '--color-moved' in linkgit:git-diff[1] for details), + `contextDimmed`, `oldDimmed`, `newDimmed`, `contextBold`, + `oldBold`, and `newBold` (see linkgit:git-range-diff[1] for details). color.decorate.:: Use customized color for 'git log --decorate' output. `` is one diff --git a/Documentation/git-range-diff.txt b/Documentation/git-range-diff.txt index e3c0be559..0027f35a2 100644 --- a/Documentation/git-range-diff.txt +++ b/Documentation/git-range-diff.txt @@ -35,10 +35,19 @@ OPTIONS When the commit diffs differ, `git range-diff` recreates the original diffs' coloring, and adds outer -/+ diff markers with the *background* being red/green to make it easier to see e.g. - when there was a change in what exact lines were added. This is - known to `range-diff` as "dual coloring". Use `--no-dual-color` - to revert to color all lines according to the outer diff markers - (and completely ignore the inner diff when it comes to color). + when there was a change in what exact lines were added. ++ +Additionally, the commit diff lines that are only present in the first commit +range are shown "dimmed" (this can be overridden using the `color.diff.` +config setting where `` is one of `contextDimmed`, `oldDimmed` and +`newDimmed`), and the commit diff lines that are only present in the second +commit range are shown in bold (which can be overridden using the config +settings `color.diff.` with `` being one of `contextBold`, +`oldBold` or `newBold`). ++ +This is known to `range-diff` as "dual coloring". Use `--no-dual-color` +to revert to color all lines according to the outer diff markers +(and completely ignore the inner diff when it comes to color). --creation-factor=:: Set the creation/deletion cost fudge factor to ``. diff --git a/color.h b/color.h index 33e786342..98894d6a1 100644 --- a/color.h +++ b/color.h @@ -36,6 +36,12 @@ struct strbuf; #define GIT_COLOR_BOLD_BLUE"\033[1;34m" #define GIT_COLOR_BOLD_MAGENTA "\033[1;35m" #define GIT_COLOR_BOLD_CYAN"\033[
[PATCH v4 20/21] range-diff: make --dual-color the default mode
From: Johannes Schindelin
After using this command extensively for the last two months, this
developer came to the conclusion that even if the dual color mode still
leaves a lot of room for confusion about what was actually changed, the
non-dual color mode is substantially worse in that regard.
Therefore, we really want to make the dual color mode the default.
Signed-off-by: Johannes Schindelin
---
Documentation/git-range-diff.txt | 32 +++---
builtin/range-diff.c | 10
contrib/completion/git-completion.bash | 2 +-
3 files changed, 25 insertions(+), 19 deletions(-)
diff --git a/Documentation/git-range-diff.txt b/Documentation/git-range-diff.txt
index f1a6737f8..e3c0be559 100644
--- a/Documentation/git-range-diff.txt
+++ b/Documentation/git-range-diff.txt
@@ -9,7 +9,7 @@ SYNOPSIS
[verse]
'git range-diff' [--color=[]] [--no-color] []
- [--dual-color] [--creation-factor=]
+ [--no-dual-color] [--creation-factor=]
( | ... |)
DESCRIPTION
@@ -31,11 +31,14 @@ all of their ancestors have been shown.
OPTIONS
---
---dual-color::
- When the commit diffs differ, recreate the original diffs'
- coloring, and add outer -/+ diff markers with the *background*
- being red/green to make it easier to see e.g. when there was a
- change in what exact lines were added.
+--no-dual-color::
+ When the commit diffs differ, `git range-diff` recreates the
+ original diffs' coloring, and adds outer -/+ diff markers with
+ the *background* being red/green to make it easier to see e.g.
+ when there was a change in what exact lines were added. This is
+ known to `range-diff` as "dual coloring". Use `--no-dual-color`
+ to revert to color all lines according to the outer diff markers
+ (and completely ignore the inner diff when it comes to color).
--creation-factor=::
Set the creation/deletion cost fudge factor to ``.
@@ -118,15 +121,16 @@ line (with a perfect match) is yellow like the commit
header of `git
show`'s output, and the third line colors the old commit red, the new
one green and the rest like `git show`'s commit header.
-The color-coded diff is actually a bit hard to read, though, as it
-colors the entire lines red or green. The line that added "What is
-unexpected" in the old commit, for example, is completely red, even if
-the intent of the old commit was to add something.
+A naive color-coded diff of diffs is actually a bit hard to read,
+though, as it colors the entire lines red or green. The line that added
+"What is unexpected" in the old commit, for example, is completely red,
+even if the intent of the old commit was to add something.
-To help with that, use the `--dual-color` mode. In this mode, the diff
-of diffs will retain the original diff colors, and prefix the lines with
--/+ markers that have their *background* red or green, to make it more
-obvious that they describe how the diff itself changed.
+To help with that, `range` uses the `--dual-color` mode by default. In
+this mode, the diff of diffs will retain the original diff colors, and
+prefix the lines with -/+ markers that have their *background* red or
+green, to make it more obvious that they describe how the diff itself
+changed.
Algorithm
diff --git a/builtin/range-diff.c b/builtin/range-diff.c
index c25d88317..77ac3bff7 100644
--- a/builtin/range-diff.c
+++ b/builtin/range-diff.c
@@ -20,11 +20,11 @@ int cmd_range_diff(int argc, const char **argv, const char
*prefix)
{
int creation_factor = 60;
struct diff_options diffopt = { NULL };
- int dual_color = 0;
+ int simple_color = -1;
struct option options[] = {
OPT_INTEGER(0, "creation-factor", _factor,
N_("Percentage by which creation is weighted")),
- OPT_BOOL(0, "dual-color", _color,
+ OPT_BOOL(0, "no-dual-color", _color,
N_("color both diff and diff-between-diffs")),
OPT_END()
};
@@ -55,8 +55,10 @@ int cmd_range_diff(int argc, const char **argv, const char
*prefix)
argc = j;
diff_setup_done();
- if (dual_color) {
- diffopt.use_color = 1;
+ if (simple_color < 1) {
+ if (!simple_color)
+ /* force color when --dual-color was used */
+ diffopt.use_color = 1;
diffopt.flags.dual_color_diffed_diffs = 1;
}
diff --git a/contrib/completion/git-completion.bash
b/contrib/completion/git-completion.bash
index 402490673..e35fc28fc 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1981,7 +1981,7 @@ _git_range_diff ()
case "$cur" in
--*)
__gitcomp "
[PATCH v4 06/21] range-diff: right-trim commit messages
From: Johannes Schindelin
When comparing commit messages, we need to keep in mind that they are
indented by four spaces. That is, empty lines are no longer empty, but
have "trailing whitespace". When displaying them in color, that results
in those nagging red lines.
Let's just right-trim the lines in the commit message, it's not like
trailing white-space in the commit messages are important enough to care
about in `git range-diff`.
Signed-off-by: Johannes Schindelin
---
range-diff.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/range-diff.c b/range-diff.c
index 71883a4b7..1ecee2c09 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -85,6 +85,7 @@ static int read_patches(const char *range, struct string_list
*list)
strbuf_addbuf(, );
strbuf_addstr(, "\n\n");
} else if (starts_with(line.buf, "")) {
+ strbuf_rtrim();
strbuf_addbuf(, );
strbuf_addch(, '\n');
}
--
gitgitgadget
[PATCH v4 17/21] range-diff: populate the man page
From: Johannes Schindelin
The bulk of this patch consists of a heavily butchered version of
tbdiff's README written by Thomas Rast and Thomas Gummerer, lifted from
https://github.com/trast/tbdiff.
Signed-off-by: Johannes Schindelin
---
Documentation/git-range-diff.txt | 229 +++
1 file changed, 229 insertions(+)
diff --git a/Documentation/git-range-diff.txt b/Documentation/git-range-diff.txt
index de0ca5df4..f1a6737f8 100644
--- a/Documentation/git-range-diff.txt
+++ b/Documentation/git-range-diff.txt
@@ -5,6 +5,235 @@ NAME
git-range-diff - Compare two commit ranges (e.g. two versions of a branch)
+SYNOPSIS
+
+[verse]
+'git range-diff' [--color=[]] [--no-color] []
+ [--dual-color] [--creation-factor=]
+ ( | ... |)
+
+DESCRIPTION
+---
+
+This command shows the differences between two versions of a patch
+series, or more generally, two commit ranges (ignoring merge commits).
+
+To that end, it first finds pairs of commits from both commit ranges
+that correspond with each other. Two commits are said to correspond when
+the diff between their patches (i.e. the author information, the commit
+message and the commit diff) is reasonably small compared to the
+patches' size. See ``Algorithm`` below for details.
+
+Finally, the list of matching commits is shown in the order of the
+second commit range, with unmatched commits being inserted just after
+all of their ancestors have been shown.
+
+
+OPTIONS
+---
+--dual-color::
+ When the commit diffs differ, recreate the original diffs'
+ coloring, and add outer -/+ diff markers with the *background*
+ being red/green to make it easier to see e.g. when there was a
+ change in what exact lines were added.
+
+--creation-factor=::
+ Set the creation/deletion cost fudge factor to ``.
+ Defaults to 60. Try a larger value if `git range-diff` erroneously
+ considers a large change a total rewrite (deletion of one commit
+ and addition of another), and a smaller one in the reverse case.
+ See the ``Algorithm`` section below for an explanation why this is
+ needed.
+
+ ::
+ Compare the commits specified by the two ranges, where
+ `` is considered an older version of ``.
+
+...::
+ Equivalent to passing `..` and `..`.
+
+ ::
+ Equivalent to passing `..` and `..`.
+ Note that `` does not need to be the exact branch point
+ of the branches. Example: after rebasing a branch `my-topic`,
+ `git range-diff my-topic@{u} my-topic@{1} my-topic` would
+ show the differences introduced by the rebase.
+
+`git range-diff` also accepts the regular diff options (see
+linkgit:git-diff[1]), most notably the `--color=[]` and
+`--no-color` options. These options are used when generating the "diff
+between patches", i.e. to compare the author, commit message and diff of
+corresponding old/new commits. There is currently no means to tweak the
+diff options passed to `git log` when generating those patches.
+
+
+CONFIGURATION
+-
+This command uses the `diff.color.*` and `pager.range-diff` settings
+(the latter is on by default).
+See linkgit:git-config[1].
+
+
+EXAMPLES
+
+
+When a rebase required merge conflicts to be resolved, compare the changes
+introduced by the rebase directly afterwards using:
+
+
+$ git range-diff @{u} @{1} @
+
+
+
+A typical output of `git range-diff` would look like this:
+
+
+-: --- > 1: 0ddba11 Prepare for the inevitable!
+1: c0debee = 2: cab005e Add a helpful message at the start
+2: f00dbal ! 3: decafe1 Describe a bug
+@@ -1,3 +1,3 @@
+ Author: A U Thor
+
+-TODO: Describe a bug
++Describe a bug
+@@ -324,5 +324,6
+ This is expected.
+
+-+What is unexpected is that it will also crash.
+++Unexpectedly, it also crashes. This is a bug, and the jury is
+++still out there how to fix it best. See ticket #314 for details.
+
+ Contact
+3: bedead < -: --- TO-UNDO
+
+
+In this example, there are 3 old and 3 new commits, where the developer
+removed the 3rd, added a new one before the first two, and modified the
+commit message of the 2nd commit as well its diff.
+
+When the output goes to a terminal, it is color-coded by default, just
+like regular `git diff`'s output. In addition, the first line (adding a
+commit) is green, the last line (deleting a commit) is red, the second
+line (with a perfect match) is yellow like the commit header of `git
+show`'s output, and the third line colors the old commit red, the new
+one green and the rest like `git show`'s commit header.
+
+The color-coded diff is actually a bit hard to read, though, as it
+colors the entire lines red or green. The line that added "What is
+unexpected" in the old commit, for example, is completely red, even if
+the intent of the old commit was to add something.
+
+To help
[PATCH v4 07/21] range-diff: indent the diffs just like tbdiff
From: Johannes Schindelin
The main information in the `range-diff` view comes from the list of
matching and non-matching commits, the diffs are additional information.
Indenting them helps with the reading flow.
Signed-off-by: Johannes Schindelin
---
builtin/range-diff.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/builtin/range-diff.c b/builtin/range-diff.c
index 093202117..96e8bf841 100644
--- a/builtin/range-diff.c
+++ b/builtin/range-diff.c
@@ -11,6 +11,11 @@ N_("git range-diff [] "),
NULL
};
+static struct strbuf *output_prefix_cb(struct diff_options *opt, void *data)
+{
+ return data;
+}
+
int cmd_range_diff(int argc, const char **argv, const char *prefix)
{
int creation_factor = 60;
@@ -21,12 +26,16 @@ int cmd_range_diff(int argc, const char **argv, const char
*prefix)
OPT_END()
};
int i, j, res = 0;
+ struct strbuf four_spaces = STRBUF_INIT;
struct strbuf range1 = STRBUF_INIT, range2 = STRBUF_INIT;
git_config(git_diff_ui_config, NULL);
diff_setup();
diffopt.output_format = DIFF_FORMAT_PATCH;
+ diffopt.output_prefix = output_prefix_cb;
+ strbuf_addstr(_spaces, "");
+ diffopt.output_prefix_data = _spaces;
argc = parse_options(argc, argv, NULL, options,
builtin_range_diff_usage, PARSE_OPT_KEEP_UNKNOWN);
@@ -80,6 +89,7 @@ int cmd_range_diff(int argc, const char **argv, const char
*prefix)
strbuf_release();
strbuf_release();
+ strbuf_release(_spaces);
return res;
}
--
gitgitgadget
[PATCH v4 05/21] range-diff: also show the diff between patches
From: Johannes Schindelin
Just like tbdiff, we now show the diff between matching patches. This is
a "diff of two diffs", so it can be a bit daunting to read for the
beginner.
An alternative would be to display an interdiff, i.e. the hypothetical
diff which is the result of first reverting the old diff and then
applying the new diff.
Especially when rebasing often, an interdiff is often not feasible,
though: if the old diff cannot be applied in reverse (due to a moving
upstream), an interdiff can simply not be inferred.
This commit brings `range-diff` closer to feature parity with regard
to tbdiff.
To make `git range-diff` respect e.g. color.diff.* settings, we have
to adjust git_branch_config() accordingly.
Note: while we now parse diff options such as --color, the effect is not
yet the same as in tbdiff, where also the commit pairs would be colored.
This is left for a later commit.
Note also: while tbdiff accepts the `--no-patches` option to suppress
these diffs between patches, we prefer the `-s` option that is
automatically supported via our use of diff_opt_parse().
Signed-off-by: Johannes Schindelin
---
builtin/range-diff.c | 25 ++---
range-diff.c | 34 +++---
range-diff.h | 4 +++-
3 files changed, 56 insertions(+), 7 deletions(-)
diff --git a/builtin/range-diff.c b/builtin/range-diff.c
index 3881da246..093202117 100644
--- a/builtin/range-diff.c
+++ b/builtin/range-diff.c
@@ -2,6 +2,7 @@
#include "builtin.h"
#include "parse-options.h"
#include "range-diff.h"
+#include "config.h"
static const char * const builtin_range_diff_usage[] = {
N_("git range-diff [] .. .."),
@@ -13,16 +14,33 @@ NULL
int cmd_range_diff(int argc, const char **argv, const char *prefix)
{
int creation_factor = 60;
+ struct diff_options diffopt = { NULL };
struct option options[] = {
OPT_INTEGER(0, "creation-factor", _factor,
N_("Percentage by which creation is weighted")),
OPT_END()
};
- int res = 0;
+ int i, j, res = 0;
struct strbuf range1 = STRBUF_INIT, range2 = STRBUF_INIT;
+ git_config(git_diff_ui_config, NULL);
+
+ diff_setup();
+ diffopt.output_format = DIFF_FORMAT_PATCH;
+
argc = parse_options(argc, argv, NULL, options,
-builtin_range_diff_usage, 0);
+builtin_range_diff_usage, PARSE_OPT_KEEP_UNKNOWN);
+
+ for (i = j = 0; i < argc; ) {
+ int c = diff_opt_parse(, argv + i, argc - i, prefix);
+
+ if (!c)
+ argv[j++] = argv[i++];
+ else
+ i += c;
+ }
+ argc = j;
+ diff_setup_done();
if (argc == 2) {
if (!strstr(argv[0], ".."))
@@ -57,7 +75,8 @@ int cmd_range_diff(int argc, const char **argv, const char
*prefix)
usage_with_options(builtin_range_diff_usage, options);
}
- res = show_range_diff(range1.buf, range2.buf, creation_factor);
+ res = show_range_diff(range1.buf, range2.buf, creation_factor,
+ );
strbuf_release();
strbuf_release();
diff --git a/range-diff.c b/range-diff.c
index 2d94200d3..71883a4b7 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -6,6 +6,7 @@
#include "hashmap.h"
#include "xdiff-interface.h"
#include "linear-assignment.h"
+#include "diffcore.h"
struct patch_util {
/* For the search for an exact match */
@@ -258,7 +259,31 @@ static const char *short_oid(struct patch_util *util)
return find_unique_abbrev(>oid, DEFAULT_ABBREV);
}
-static void output(struct string_list *a, struct string_list *b)
+static struct diff_filespec *get_filespec(const char *name, const char *p)
+{
+ struct diff_filespec *spec = alloc_filespec(name);
+
+ fill_filespec(spec, _oid, 0, 0644);
+ spec->data = (char *)p;
+ spec->size = strlen(p);
+ spec->should_munmap = 0;
+ spec->is_stdin = 1;
+
+ return spec;
+}
+
+static void patch_diff(const char *a, const char *b,
+ struct diff_options *diffopt)
+{
+ diff_queue(_queued_diff,
+ get_filespec("a", a), get_filespec("b", b));
+
+ diffcore_std(diffopt);
+ diff_flush(diffopt);
+}
+
+static void output(struct string_list *a, struct string_list *b,
+ struct diff_options *diffopt)
{
int i = 0, j = 0;
@@ -300,6 +325,9 @@ static void output(struct string_list *a, struct
string_list *b)
printf("%d: %s ! %d: %s\n",
b_util->matching + 1, short_oid(a_util),
j
[PATCH v4 19/21] range-diff: left-pad patch numbers
From: Johannes Schindelin
As pointed out by Elijah Newren, tbdiff has this neat little alignment
trick where it outputs the commit pairs with patch numbers that are
padded to the maximal patch number's width:
1: cafedead = 1: acefade first patch
[...]
314: beefeada < 314: facecab up to PI!
Let's do the same in range-diff, too.
Signed-off-by: Johannes Schindelin
---
range-diff.c | 16 +---
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/range-diff.c b/range-diff.c
index ab1e71e10..347b4a79f 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -259,6 +259,7 @@ static void get_correspondences(struct string_list *a,
struct string_list *b,
}
static void output_pair_header(struct diff_options *diffopt,
+ int patch_no_width,
struct strbuf *buf,
struct strbuf *dashes,
struct patch_util *a_util,
@@ -295,9 +296,9 @@ static void output_pair_header(struct diff_options *diffopt,
strbuf_reset(buf);
strbuf_addstr(buf, status == '!' ? color_old : color);
if (!a_util)
- strbuf_addf(buf, "-: %s ", dashes->buf);
+ strbuf_addf(buf, "%*s: %s ", patch_no_width, "-", dashes->buf);
else
- strbuf_addf(buf, "%d: %s ", a_util->i + 1,
+ strbuf_addf(buf, "%*d: %s ", patch_no_width, a_util->i + 1,
find_unique_abbrev(_util->oid, DEFAULT_ABBREV));
if (status == '!')
@@ -307,9 +308,9 @@ static void output_pair_header(struct diff_options *diffopt,
strbuf_addf(buf, "%s%s", color_reset, color_new);
if (!b_util)
- strbuf_addf(buf, " -: %s", dashes->buf);
+ strbuf_addf(buf, " %*s: %s", patch_no_width, "-", dashes->buf);
else
- strbuf_addf(buf, " %d: %s", b_util->i + 1,
+ strbuf_addf(buf, " %*d: %s", patch_no_width, b_util->i + 1,
find_unique_abbrev(_util->oid, DEFAULT_ABBREV));
commit = lookup_commit_reference(oid);
@@ -362,6 +363,7 @@ static void output(struct string_list *a, struct
string_list *b,
struct diff_options *diffopt)
{
struct strbuf buf = STRBUF_INIT, dashes = STRBUF_INIT;
+ int patch_no_width = decimal_width(1 + (a->nr > b->nr ? a->nr : b->nr));
int i = 0, j = 0;
/*
@@ -383,7 +385,7 @@ static void output(struct string_list *a, struct
string_list *b,
/* Show unmatched LHS commit whose predecessors were shown. */
if (i < a->nr && a_util->matching < 0) {
- output_pair_header(diffopt,
+ output_pair_header(diffopt, patch_no_width,
, , a_util, NULL);
i++;
continue;
@@ -391,7 +393,7 @@ static void output(struct string_list *a, struct
string_list *b,
/* Show unmatched RHS commits. */
while (j < b->nr && b_util->matching < 0) {
- output_pair_header(diffopt,
+ output_pair_header(diffopt, patch_no_width,
, , NULL, b_util);
b_util = ++j < b->nr ? b->items[j].util : NULL;
}
@@ -399,7 +401,7 @@ static void output(struct string_list *a, struct
string_list *b,
/* Show matching LHS/RHS pair. */
if (j < b->nr) {
a_util = a->items[b_util->matching].util;
- output_pair_header(diffopt,
+ output_pair_header(diffopt, patch_no_width,
, , a_util, b_util);
if (!(diffopt->output_format & DIFF_FORMAT_NO_OUTPUT))
patch_diff(a->items[b_util->matching].string,
--
gitgitgadget
[PATCH v4 13/21] color: add the meta color GIT_COLOR_REVERSE
From: Johannes Schindelin This "color" simply reverts background and foreground. It will be used in the upcoming "dual color" mode of `git range-diff`, where we will reverse colors for the -/+ markers and the fragment headers of the "outer" diff. Signed-off-by: Johannes Schindelin --- color.h | 1 + 1 file changed, 1 insertion(+) diff --git a/color.h b/color.h index 5b744e1bc..33e786342 100644 --- a/color.h +++ b/color.h @@ -44,6 +44,7 @@ struct strbuf; #define GIT_COLOR_BG_CYAN "\033[46m" #define GIT_COLOR_FAINT"\033[2m" #define GIT_COLOR_FAINT_ITALIC "\033[2;3m" +#define GIT_COLOR_REVERSE "\033[7m" /* A special value meaning "no color selected" */ #define GIT_COLOR_NIL "NIL" -- gitgitgadget
[PATCH v4 16/21] range-diff --dual-color: fix bogus white-space warning
From: Johannes Schindelin
When displaying a diff of diffs, it is possible that there is an outer
`+` before a context line. That happens when the context changed between
old and new commit. When that context line starts with a tab (after the
space that marks it as context line), our diff machinery spits out a
white-space error (space before tab), but in this case, that is
incorrect.
Fix this by adding a specific whitespace flag that simply ignores the
first space in the output.
Of course, this flag is *really* specific to the "diff of diffs" use
case. The original idea was to simply skip the space from the output,
but that workaround was rejected by the Git maintainer as causing
headaches.
Note: as the original code did not leave any space in the bit mask
before the WSEH_* bits, the diff of this commit looks unnecessarily
involved: the diff is dominated by making room for one more bit to be
used by the whitespace rules.
Signed-off-by: Johannes Schindelin
---
cache.h | 3 ++-
diff.c | 15 ---
diff.h | 6 +++---
ws.c| 11 ++-
4 files changed, 23 insertions(+), 12 deletions(-)
diff --git a/cache.h b/cache.h
index 8b447652a..8abfbeb73 100644
--- a/cache.h
+++ b/cache.h
@@ -1681,11 +1681,12 @@ void shift_tree_by(const struct object_id *, const
struct object_id *, struct ob
#define WS_CR_AT_EOL 01000
#define WS_BLANK_AT_EOF02000
#define WS_TAB_IN_INDENT 04000
+#define WS_IGNORE_FIRST_SPACE 01
#define WS_TRAILING_SPACE (WS_BLANK_AT_EOL|WS_BLANK_AT_EOF)
#define WS_DEFAULT_RULE (WS_TRAILING_SPACE|WS_SPACE_BEFORE_TAB|8)
#define WS_TAB_WIDTH_MASK077
/* All WS_* -- when extended, adapt diff.c emit_symbol */
-#define WS_RULE_MASK 0
+#define WS_RULE_MASK 01
extern unsigned whitespace_rule_cfg;
extern unsigned whitespace_rule(const char *);
extern unsigned parse_whitespace_rule(const char *);
diff --git a/diff.c b/diff.c
index e163bc8a3..03ed235c7 100644
--- a/diff.c
+++ b/diff.c
@@ -650,14 +650,14 @@ enum diff_symbol {
};
/*
* Flags for content lines:
- * 0..12 are whitespace rules
- * 13-15 are WSEH_NEW | WSEH_OLD | WSEH_CONTEXT
- * 16 is marking if the line is blank at EOF
+ * 0..14 are whitespace rules
+ * 14-16 are WSEH_NEW | WSEH_OLD | WSEH_CONTEXT
+ * 17 is marking if the line is blank at EOF
*/
-#define DIFF_SYMBOL_CONTENT_BLANK_LINE_EOF (1<<16)
-#define DIFF_SYMBOL_MOVED_LINE (1<<17)
-#define DIFF_SYMBOL_MOVED_LINE_ALT (1<<18)
-#define DIFF_SYMBOL_MOVED_LINE_UNINTERESTING (1<<19)
+#define DIFF_SYMBOL_CONTENT_BLANK_LINE_EOF (1<<17)
+#define DIFF_SYMBOL_MOVED_LINE (1<<18)
+#define DIFF_SYMBOL_MOVED_LINE_ALT (1<<19)
+#define DIFF_SYMBOL_MOVED_LINE_UNINTERESTING (1<<20)
#define DIFF_SYMBOL_CONTENT_WS_MASK (WSEH_NEW | WSEH_OLD | WSEH_CONTEXT |
WS_RULE_MASK)
/*
@@ -1094,6 +1094,7 @@ static void emit_diff_symbol_from_struct(struct
diff_options *o,
set = diff_get_color_opt(o, DIFF_FRAGINFO);
else if (c != '+')
set = diff_get_color_opt(o, DIFF_CONTEXT);
+ flags |= WS_IGNORE_FIRST_SPACE;
}
emit_line_ws_markup(o, set, reset, line, len, set_sign, '+',
flags & DIFF_SYMBOL_CONTENT_WS_MASK,
diff --git a/diff.h b/diff.h
index 79beb6eea..892416a14 100644
--- a/diff.h
+++ b/diff.h
@@ -160,9 +160,9 @@ struct diff_options {
int abbrev;
int ita_invisible_in_index;
/* white-space error highlighting */
-#define WSEH_NEW (1<<12)
-#define WSEH_CONTEXT (1<<13)
-#define WSEH_OLD (1<<14)
+#define WSEH_NEW (1<<13)
+#define WSEH_CONTEXT (1<<14)
+#define WSEH_OLD (1<<15)
unsigned ws_error_highlight;
const char *prefix;
int prefix_length;
diff --git a/ws.c b/ws.c
index a07caedd5..e02365a6a 100644
--- a/ws.c
+++ b/ws.c
@@ -20,6 +20,7 @@ static struct whitespace_rule {
{ "blank-at-eol", WS_BLANK_AT_EOL, 0 },
{ "blank-at-eof", WS_BLANK_AT_EOF, 0 },
{ "tab-in-indent", WS_TAB_IN_INDENT, 0, 1 },
+ { "ignore-first-space", WS_IGNORE_FIRST_SPACE, 0, 1 },
};
unsigned parse_whitespace_rule(const char *string)
@@ -177,8 +178,16 @@ static unsigned ws_check_emit_1(const char *line, int len,
unsigned ws_rule,
if (trailing_whitespace == -1)
trailing_whitespace = len;
+ if ((ws_rule & WS_IGNORE_FIRST_SPACE) && len && line[0] == ' ') {
+ if (stream)
+ fwrite(line, 1, 1, stream);
+ written++;
+ if (!trailing_whitespace)
+ trailing_whitespace++;
+ }
+
/* Check indentation */
- for (i = 0; i < trailing_whitespace; i++) {
+
[PATCH v4 03/21] range-diff: first rudimentary implementation
From: Johannes Schindelin
At this stage, `git range-diff` can determine corresponding commits
of two related commit ranges. This makes use of the recently introduced
implementation of the linear assignment algorithm.
The core of this patch is a straight port of the ideas of tbdiff, the
apparently dormant project at https://github.com/trast/tbdiff.
The output does not at all match `tbdiff`'s output yet, as this patch
really concentrates on getting the patch matching part right.
Note: due to differences in the diff algorithm (`tbdiff` uses the Python
module `difflib`, Git uses its xdiff fork), the cost matrix calculated
by `range-diff` is different (but very similar) to the one calculated
by `tbdiff`. Therefore, it is possible that they find different matching
commits in corner cases (e.g. when a patch was split into two patches of
roughly equal length).
Signed-off-by: Johannes Schindelin
---
Makefile | 1 +
builtin/range-diff.c | 43 +-
range-diff.c | 311 +++
range-diff.h | 7 +
4 files changed, 361 insertions(+), 1 deletion(-)
create mode 100644 range-diff.c
create mode 100644 range-diff.h
diff --git a/Makefile b/Makefile
index 45c9dea1b..41b93689a 100644
--- a/Makefile
+++ b/Makefile
@@ -921,6 +921,7 @@ LIB_OBJS += progress.o
LIB_OBJS += prompt.o
LIB_OBJS += protocol.o
LIB_OBJS += quote.o
+LIB_OBJS += range-diff.o
LIB_OBJS += reachable.o
LIB_OBJS += read-cache.o
LIB_OBJS += reflog-walk.o
diff --git a/builtin/range-diff.c b/builtin/range-diff.c
index 36788ea4f..3881da246 100644
--- a/builtin/range-diff.c
+++ b/builtin/range-diff.c
@@ -1,6 +1,7 @@
#include "cache.h"
#include "builtin.h"
#include "parse-options.h"
+#include "range-diff.h"
static const char * const builtin_range_diff_usage[] = {
N_("git range-diff [] .. .."),
@@ -17,9 +18,49 @@ int cmd_range_diff(int argc, const char **argv, const char
*prefix)
N_("Percentage by which creation is weighted")),
OPT_END()
};
+ int res = 0;
+ struct strbuf range1 = STRBUF_INIT, range2 = STRBUF_INIT;
argc = parse_options(argc, argv, NULL, options,
builtin_range_diff_usage, 0);
- return 0;
+ if (argc == 2) {
+ if (!strstr(argv[0], ".."))
+ die(_("no .. in range: '%s'"), argv[0]);
+ strbuf_addstr(, argv[0]);
+
+ if (!strstr(argv[1], ".."))
+ die(_("no .. in range: '%s'"), argv[1]);
+ strbuf_addstr(, argv[1]);
+ } else if (argc == 3) {
+ strbuf_addf(, "%s..%s", argv[0], argv[1]);
+ strbuf_addf(, "%s..%s", argv[0], argv[2]);
+ } else if (argc == 1) {
+ const char *b = strstr(argv[0], "..."), *a = argv[0];
+ int a_len;
+
+ if (!b)
+ die(_("single arg format requires a symmetric range"));
+
+ a_len = (int)(b - a);
+ if (!a_len) {
+ a = "HEAD";
+ a_len = strlen(a);
+ }
+ b += 3;
+ if (!*b)
+ b = "HEAD";
+ strbuf_addf(, "%s..%.*s", b, a_len, a);
+ strbuf_addf(, "%.*s..%s", a_len, a, b);
+ } else {
+ error(_("need two commit ranges"));
+ usage_with_options(builtin_range_diff_usage, options);
+ }
+
+ res = show_range_diff(range1.buf, range2.buf, creation_factor);
+
+ strbuf_release();
+ strbuf_release();
+
+ return res;
}
diff --git a/range-diff.c b/range-diff.c
new file mode 100644
index 0..15d418afa
--- /dev/null
+++ b/range-diff.c
@@ -0,0 +1,311 @@
+#include "cache.h"
+#include "range-diff.h"
+#include "string-list.h"
+#include "run-command.h"
+#include "argv-array.h"
+#include "hashmap.h"
+#include "xdiff-interface.h"
+#include "linear-assignment.h"
+
+struct patch_util {
+ /* For the search for an exact match */
+ struct hashmap_entry e;
+ const char *diff, *patch;
+
+ int i;
+ int diffsize;
+ size_t diff_offset;
+ /* the index of the matching item in the other branch, or -1 */
+ int matching;
+ struct object_id oid;
+};
+
+/*
+ * Reads the patches into a string list, with the `util` field being populated
+ * as struct object_id (will need to be free()d).
+ */
+static int read_patches(const char *range, struct string_list *list)
+{
+ struct child_process cp = CHILD_PROCESS_INIT;
+ FILE *in;
+ struct strbuf buf = STRBUF_INIT, line = STRBUF_INIT;
+ struct patch_util *util = NU
[PATCH v4 10/21] range-diff: do not show "function names" in hunk headers
From: Johannes Schindelin
We are comparing complete, formatted commit messages with patches. There
are no function names here, so stop looking for them.
Signed-off-by: Johannes Schindelin
---
range-diff.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/range-diff.c b/range-diff.c
index 8329f52e7..3fc3a4018 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -9,6 +9,7 @@
#include "diffcore.h"
#include "commit.h"
#include "pretty.h"
+#include "userdiff.h"
struct patch_util {
/* For the search for an exact match */
@@ -307,6 +308,10 @@ static void output_pair_header(struct strbuf *buf,
fwrite(buf->buf, buf->len, 1, stdout);
}
+static struct userdiff_driver no_func_name = {
+ .funcname = { "$^", 0 }
+};
+
static struct diff_filespec *get_filespec(const char *name, const char *p)
{
struct diff_filespec *spec = alloc_filespec(name);
@@ -316,6 +321,7 @@ static struct diff_filespec *get_filespec(const char *name,
const char *p)
spec->size = strlen(p);
spec->should_munmap = 0;
spec->is_stdin = 1;
+ spec->driver = _func_name;
return spec;
}
--
gitgitgadget
[PATCH v4 09/21] range-diff: adjust the output of the commit pairs
From: Johannes Schindelin
This change brings `git range-diff` yet another step closer to
feature parity with tbdiff: it now shows the oneline, too, and indicates
with `=` when the commits have identical diffs.
Signed-off-by: Johannes Schindelin
---
range-diff.c | 64
1 file changed, 55 insertions(+), 9 deletions(-)
diff --git a/range-diff.c b/range-diff.c
index 1ecee2c09..8329f52e7 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -7,6 +7,8 @@
#include "xdiff-interface.h"
#include "linear-assignment.h"
#include "diffcore.h"
+#include "commit.h"
+#include "pretty.h"
struct patch_util {
/* For the search for an exact match */
@@ -255,9 +257,54 @@ static void get_correspondences(struct string_list *a,
struct string_list *b,
free(b2a);
}
-static const char *short_oid(struct patch_util *util)
+static void output_pair_header(struct strbuf *buf,
+ struct strbuf *dashes,
+ struct patch_util *a_util,
+ struct patch_util *b_util)
{
- return find_unique_abbrev(>oid, DEFAULT_ABBREV);
+ struct object_id *oid = a_util ? _util->oid : _util->oid;
+ struct commit *commit;
+
+ if (!dashes->len)
+ strbuf_addchars(dashes, '-',
+ strlen(find_unique_abbrev(oid,
+ DEFAULT_ABBREV)));
+
+ strbuf_reset(buf);
+ if (!a_util)
+ strbuf_addf(buf, "-: %s ", dashes->buf);
+ else
+ strbuf_addf(buf, "%d: %s ", a_util->i + 1,
+ find_unique_abbrev(_util->oid, DEFAULT_ABBREV));
+
+ if (!a_util)
+ strbuf_addch(buf, '>');
+ else if (!b_util)
+ strbuf_addch(buf, '<');
+ else if (strcmp(a_util->patch, b_util->patch))
+ strbuf_addch(buf, '!');
+ else
+ strbuf_addch(buf, '=');
+
+ if (!b_util)
+ strbuf_addf(buf, " -: %s", dashes->buf);
+ else
+ strbuf_addf(buf, " %d: %s", b_util->i + 1,
+ find_unique_abbrev(_util->oid, DEFAULT_ABBREV));
+
+ commit = lookup_commit_reference(oid);
+ if (commit) {
+ const char *commit_buffer = get_commit_buffer(commit, NULL);
+ const char *subject;
+
+ find_commit_subject(commit_buffer, );
+ strbuf_addch(buf, ' ');
+ format_subject(buf, subject, " ");
+ unuse_commit_buffer(commit, commit_buffer);
+ }
+ strbuf_addch(buf, '\n');
+
+ fwrite(buf->buf, buf->len, 1, stdout);
}
static struct diff_filespec *get_filespec(const char *name, const char *p)
@@ -286,6 +333,7 @@ static void patch_diff(const char *a, const char *b,
static void output(struct string_list *a, struct string_list *b,
struct diff_options *diffopt)
{
+ struct strbuf buf = STRBUF_INIT, dashes = STRBUF_INIT;
int i = 0, j = 0;
/*
@@ -307,25 +355,21 @@ static void output(struct string_list *a, struct
string_list *b,
/* Show unmatched LHS commit whose predecessors were shown. */
if (i < a->nr && a_util->matching < 0) {
- printf("%d: %s < -: \n",
- i + 1, short_oid(a_util));
+ output_pair_header(, , a_util, NULL);
i++;
continue;
}
/* Show unmatched RHS commits. */
while (j < b->nr && b_util->matching < 0) {
- printf("-: > %d: %s\n",
- j + 1, short_oid(b_util));
+ output_pair_header(, , NULL, b_util);
b_util = ++j < b->nr ? b->items[j].util : NULL;
}
/* Show matching LHS/RHS pair. */
if (j < b->nr) {
a_util = a->items[b_util->matching].util;
- printf("%d: %s ! %d: %s\n",
- b_util->matching + 1, short_oid(a_util),
- j + 1, short_oid(b_util));
+ output_pair_header(, , a_util, b_util);
if (!(diffopt->output_format & DIFF_FORMAT_NO_OUTPUT))
patch_diff(a->items[b_util->matching].string,
b->items[j].string, diffopt);
@@ -333,6 +377,8 @@ static void output(struct string_list *a, struct
string_list *b,
j++;
}
}
+ strbuf_release();
+ strbuf_release();
}
int show_range_diff(const char *range1, const char *range2,
--
gitgitgadget
[PATCH v4 14/21] diff: add an internal option to dual-color diffs of diffs
From: Johannes Schindelin
When diffing diffs, it can be quite daunting to figure out what the heck
is going on, as there are nested +/- signs.
Let's make this easier by adding a flag in diff_options that allows
color-coding the outer diff sign with inverted colors, so that the
preimage and postimage is colored like the diff it is.
Of course, this really only makes sense when the preimage and postimage
*are* diffs. So let's not expose this flag via a command-line option for
now.
This is a feature that was invented by git-tbdiff, and it will be used
by `git range-diff` in the next commit, by offering it via a new option:
`--dual-color`.
Signed-off-by: Johannes Schindelin
---
diff.c | 83 +++---
diff.h | 1 +
2 files changed, 69 insertions(+), 15 deletions(-)
diff --git a/diff.c b/diff.c
index a94a8214f..e163bc8a3 100644
--- a/diff.c
+++ b/diff.c
@@ -563,14 +563,18 @@ static void check_blank_at_eof(mmfile_t *mf1, mmfile_t
*mf2,
ecbdata->blank_at_eof_in_postimage = (at - l2) + 1;
}
-static void emit_line_0(struct diff_options *o, const char *set, const char
*reset,
+static void emit_line_0(struct diff_options *o,
+ const char *set, unsigned reverse, const char *reset,
int first, const char *line, int len)
{
int has_trailing_newline, has_trailing_carriage_return;
int nofirst;
FILE *file = o->file;
- fputs(diff_line_prefix(o), file);
+ if (first)
+ fputs(diff_line_prefix(o), file);
+ else if (!len)
+ return;
if (len == 0) {
has_trailing_newline = (first == '\n');
@@ -588,8 +592,10 @@ static void emit_line_0(struct diff_options *o, const char
*set, const char *res
}
if (len || !nofirst) {
+ if (reverse && want_color(o->use_color))
+ fputs(GIT_COLOR_REVERSE, file);
fputs(set, file);
- if (!nofirst)
+ if (first && !nofirst)
fputc(first, file);
fwrite(line, len, 1, file);
fputs(reset, file);
@@ -603,7 +609,7 @@ static void emit_line_0(struct diff_options *o, const char
*set, const char *res
static void emit_line(struct diff_options *o, const char *set, const char
*reset,
const char *line, int len)
{
- emit_line_0(o, set, reset, line[0], line+1, len-1);
+ emit_line_0(o, set, 0, reset, line[0], line+1, len-1);
}
enum diff_symbol {
@@ -963,7 +969,8 @@ static void dim_moved_lines(struct diff_options *o)
static void emit_line_ws_markup(struct diff_options *o,
const char *set, const char *reset,
- const char *line, int len, char sign,
+ const char *line, int len,
+ const char *set_sign, char sign,
unsigned ws_rule, int blank_at_eof)
{
const char *ws = NULL;
@@ -974,14 +981,20 @@ static void emit_line_ws_markup(struct diff_options *o,
ws = NULL;
}
- if (!ws)
- emit_line_0(o, set, reset, sign, line, len);
- else if (blank_at_eof)
+ if (!ws && !set_sign)
+ emit_line_0(o, set, 0, reset, sign, line, len);
+ else if (!ws) {
+ /* Emit just the prefix, then the rest. */
+ emit_line_0(o, set_sign ? set_sign : set, !!set_sign, reset,
+ sign, "", 0);
+ emit_line_0(o, set, 0, reset, 0, line, len);
+ } else if (blank_at_eof)
/* Blank line at EOF - paint '+' as well */
- emit_line_0(o, ws, reset, sign, line, len);
+ emit_line_0(o, ws, 0, reset, sign, line, len);
else {
/* Emit just the prefix, then the rest. */
- emit_line_0(o, set, reset, sign, "", 0);
+ emit_line_0(o, set_sign ? set_sign : set, !!set_sign, reset,
+ sign, "", 0);
ws_check_emit(line, len, ws_rule,
o->file, set, reset, ws);
}
@@ -991,7 +1004,7 @@ static void emit_diff_symbol_from_struct(struct
diff_options *o,
struct emitted_diff_symbol *eds)
{
static const char *nneof = " No newline at end of file\n";
- const char *context, *reset, *set, *meta, *fraginfo;
+ const char *context, *reset, *set, *set_sign, *meta, *fraginfo;
struct strbuf sb = STRBUF_INIT;
enum diff_symbol s = eds->s;
@@ -1004,7 +1017,7 @@ static void emit_diff_symbol_from_struct(struct
diff_options *o,
context = diff_get_color_opt(o, DIFF_CONTEXT);
reset = diff_get_color_opt(o, DIFF_RESET);
[PATCH v4 08/21] range-diff: suppress the diff headers
From: Johannes Schindelin
When showing the diff between corresponding patches of the two branch
versions, we have to make up a fake filename to run the diff machinery.
That filename does not carry any meaningful information, hence tbdiff
suppresses it. So we should, too.
Signed-off-by: Johannes Schindelin
---
builtin/range-diff.c | 1 +
diff.c | 5 -
diff.h | 1 +
3 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/builtin/range-diff.c b/builtin/range-diff.c
index 96e8bf841..10065315d 100644
--- a/builtin/range-diff.c
+++ b/builtin/range-diff.c
@@ -33,6 +33,7 @@ int cmd_range_diff(int argc, const char **argv, const char
*prefix)
diff_setup();
diffopt.output_format = DIFF_FORMAT_PATCH;
+ diffopt.flags.suppress_diff_headers = 1;
diffopt.output_prefix = output_prefix_cb;
strbuf_addstr(_spaces, "");
diffopt.output_prefix_data = _spaces;
diff --git a/diff.c b/diff.c
index dc53a19ba..a94a8214f 100644
--- a/diff.c
+++ b/diff.c
@@ -3190,13 +3190,16 @@ static void builtin_diff(const char *name_a,
memset(, 0, sizeof(xpp));
memset(, 0, sizeof(xecfg));
memset(, 0, sizeof(ecbdata));
+ if (o->flags.suppress_diff_headers)
+ lbl[0] = NULL;
ecbdata.label_path = lbl;
ecbdata.color_diff = want_color(o->use_color);
ecbdata.ws_rule = whitespace_rule(name_b);
if (ecbdata.ws_rule & WS_BLANK_AT_EOF)
check_blank_at_eof(, , );
ecbdata.opt = o;
- ecbdata.header = header.len ? : NULL;
+ if (header.len && !o->flags.suppress_diff_headers)
+ ecbdata.header =
xpp.flags = o->xdl_opts;
xpp.anchors = o->anchors;
xpp.anchors_nr = o->anchors_nr;
diff --git a/diff.h b/diff.h
index dedac472c..928f48995 100644
--- a/diff.h
+++ b/diff.h
@@ -94,6 +94,7 @@ struct diff_flags {
unsigned funccontext:1;
unsigned default_follow_renames:1;
unsigned stat_with_summary:1;
+ unsigned suppress_diff_headers:1;
};
static inline void diff_flags_or(struct diff_flags *a,
--
gitgitgadget
[PATCH v4 12/21] range-diff: use color for the commit pairs
From: Johannes Schindelin
Arguably the most important part of `git range-diff`'s output is the
list of commits in the two branches, together with their relationships.
For that reason, tbdiff introduced color-coding that is pretty
intuitive, especially for unchanged patches (all dim yellow, like the
first line in `git show`'s output) vs modified patches (old commit is
red, new commit is green). Let's imitate that color scheme.
Signed-off-by: Johannes Schindelin
---
range-diff.c | 51 ++-
1 file changed, 38 insertions(+), 13 deletions(-)
diff --git a/range-diff.c b/range-diff.c
index 3fc3a4018..ab1e71e10 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -258,34 +258,53 @@ static void get_correspondences(struct string_list *a,
struct string_list *b,
free(b2a);
}
-static void output_pair_header(struct strbuf *buf,
+static void output_pair_header(struct diff_options *diffopt,
+ struct strbuf *buf,
struct strbuf *dashes,
struct patch_util *a_util,
struct patch_util *b_util)
{
struct object_id *oid = a_util ? _util->oid : _util->oid;
struct commit *commit;
+ char status;
+ const char *color_reset = diff_get_color_opt(diffopt, DIFF_RESET);
+ const char *color_old = diff_get_color_opt(diffopt, DIFF_FILE_OLD);
+ const char *color_new = diff_get_color_opt(diffopt, DIFF_FILE_NEW);
+ const char *color_commit = diff_get_color_opt(diffopt, DIFF_COMMIT);
+ const char *color;
if (!dashes->len)
strbuf_addchars(dashes, '-',
strlen(find_unique_abbrev(oid,
DEFAULT_ABBREV)));
+ if (!b_util) {
+ color = color_old;
+ status = '<';
+ } else if (!a_util) {
+ color = color_new;
+ status = '>';
+ } else if (strcmp(a_util->patch, b_util->patch)) {
+ color = color_commit;
+ status = '!';
+ } else {
+ color = color_commit;
+ status = '=';
+ }
+
strbuf_reset(buf);
+ strbuf_addstr(buf, status == '!' ? color_old : color);
if (!a_util)
strbuf_addf(buf, "-: %s ", dashes->buf);
else
strbuf_addf(buf, "%d: %s ", a_util->i + 1,
find_unique_abbrev(_util->oid, DEFAULT_ABBREV));
- if (!a_util)
- strbuf_addch(buf, '>');
- else if (!b_util)
- strbuf_addch(buf, '<');
- else if (strcmp(a_util->patch, b_util->patch))
- strbuf_addch(buf, '!');
- else
- strbuf_addch(buf, '=');
+ if (status == '!')
+ strbuf_addf(buf, "%s%s", color_reset, color);
+ strbuf_addch(buf, status);
+ if (status == '!')
+ strbuf_addf(buf, "%s%s", color_reset, color_new);
if (!b_util)
strbuf_addf(buf, " -: %s", dashes->buf);
@@ -298,12 +317,15 @@ static void output_pair_header(struct strbuf *buf,
const char *commit_buffer = get_commit_buffer(commit, NULL);
const char *subject;
+ if (status == '!')
+ strbuf_addf(buf, "%s%s", color_reset, color);
+
find_commit_subject(commit_buffer, );
strbuf_addch(buf, ' ');
format_subject(buf, subject, " ");
unuse_commit_buffer(commit, commit_buffer);
}
- strbuf_addch(buf, '\n');
+ strbuf_addf(buf, "%s\n", color_reset);
fwrite(buf->buf, buf->len, 1, stdout);
}
@@ -361,21 +383,24 @@ static void output(struct string_list *a, struct
string_list *b,
/* Show unmatched LHS commit whose predecessors were shown. */
if (i < a->nr && a_util->matching < 0) {
- output_pair_header(, , a_util, NULL);
+ output_pair_header(diffopt,
+ , , a_util, NULL);
i++;
continue;
}
/* Show unmatched RHS commits. */
while (j < b->nr && b_util->matching < 0) {
- output_pair_header(, , NULL, b_util);
+ output_pair_header(diffopt,
+ , , NULL, b_util);
b_util = ++j < b->nr ? b->items[j].util : NULL;
}
/* Show matching LHS/RHS pair. */
if (j < b->nr) {
a_util = a->items[b_util->m
[PATCH v4 01/21] linear-assignment: a function to solve least-cost assignment problems
From: Johannes Schindelin
The problem solved by the code introduced in this commit goes like this:
given two sets of items, and a cost matrix which says how much it
"costs" to assign any given item of the first set to any given item of
the second, assign all items (except when the sets have different size)
in the cheapest way.
We use the Jonker-Volgenant algorithm to solve the assignment problem to
answer questions such as: given two different versions of a topic branch
(or iterations of a patch series), what is the best pairing of
commits/patches between the different versions?
Signed-off-by: Johannes Schindelin
---
Makefile| 1 +
linear-assignment.c | 201
linear-assignment.h | 22 +
3 files changed, 224 insertions(+)
create mode 100644 linear-assignment.c
create mode 100644 linear-assignment.h
diff --git a/Makefile b/Makefile
index 08e5c5454..56326ab2b 100644
--- a/Makefile
+++ b/Makefile
@@ -868,6 +868,7 @@ LIB_OBJS += gpg-interface.o
LIB_OBJS += graph.o
LIB_OBJS += grep.o
LIB_OBJS += hashmap.o
+LIB_OBJS += linear-assignment.o
LIB_OBJS += help.o
LIB_OBJS += hex.o
LIB_OBJS += ident.o
diff --git a/linear-assignment.c b/linear-assignment.c
new file mode 100644
index 0..9b3e56e28
--- /dev/null
+++ b/linear-assignment.c
@@ -0,0 +1,201 @@
+/*
+ * Based on: Jonker, R., & Volgenant, A. (1987). A shortest augmenting path
+ * algorithm for dense and sparse linear assignment problems. Computing,
+ * 38(4), 325-340.
+ */
+#include "cache.h"
+#include "linear-assignment.h"
+
+#define COST(column, row) cost[(column) + column_count * (row)]
+
+/*
+ * The parameter `cost` is the cost matrix: the cost to assign column j to row
+ * i is `cost[j + column_count * i].
+ */
+void compute_assignment(int column_count, int row_count, int *cost,
+ int *column2row, int *row2column)
+{
+ int *v, *d;
+ int *free_row, free_count = 0, saved_free_count, *pred, *col;
+ int i, j, phase;
+
+ memset(column2row, -1, sizeof(int) * column_count);
+ memset(row2column, -1, sizeof(int) * row_count);
+ ALLOC_ARRAY(v, column_count);
+
+ /* column reduction */
+ for (j = column_count - 1; j >= 0; j--) {
+ int i1 = 0;
+
+ for (i = 1; i < row_count; i++)
+ if (COST(j, i1) > COST(j, i))
+ i1 = i;
+ v[j] = COST(j, i1);
+ if (row2column[i1] == -1) {
+ /* row i1 unassigned */
+ row2column[i1] = j;
+ column2row[j] = i1;
+ } else {
+ if (row2column[i1] >= 0)
+ row2column[i1] = -2 - row2column[i1];
+ column2row[j] = -1;
+ }
+ }
+
+ /* reduction transfer */
+ ALLOC_ARRAY(free_row, row_count);
+ for (i = 0; i < row_count; i++) {
+ int j1 = row2column[i];
+ if (j1 == -1)
+ free_row[free_count++] = i;
+ else if (j1 < -1)
+ row2column[i] = -2 - j1;
+ else {
+ int min = COST(!j1, i) - v[!j1];
+ for (j = 1; j < column_count; j++)
+ if (j != j1 && min > COST(j, i) - v[j])
+ min = COST(j, i) - v[j];
+ v[j1] -= min;
+ }
+ }
+
+ if (free_count ==
+ (column_count < row_count ? row_count - column_count : 0)) {
+ free(v);
+ free(free_row);
+ return;
+ }
+
+ /* augmenting row reduction */
+ for (phase = 0; phase < 2; phase++) {
+ int k = 0;
+
+ saved_free_count = free_count;
+ free_count = 0;
+ while (k < saved_free_count) {
+ int u1, u2;
+ int j1 = 0, j2, i0;
+
+ i = free_row[k++];
+ u1 = COST(j1, i) - v[j1];
+ j2 = -1;
+ u2 = INT_MAX;
+ for (j = 1; j < column_count; j++) {
+ int c = COST(j, i) - v[j];
+ if (u2 > c) {
+ if (u1 < c) {
+ u2 = c;
+ j2 = j;
+ } else {
+ u2 = u1;
+ u1 = c;
+ j2 = j1;
+ j1 = j;
+ }
+ }
+
[PATCH v4 02/21] Introduce `range-diff` to compare iterations of a topic branch
From: Johannes Schindelin
This command does not do a whole lot so far, apart from showing a usage
that is oddly similar to that of `git tbdiff`. And for a good reason:
the next commits will turn `range-branch` into a full-blown replacement
for `tbdiff`.
At this point, we ignore tbdiff's color options, as they will all be
implemented later using diff_options.
Since f318d739159 (generate-cmds.sh: export all commands to
command-list.h, 2018-05-10), every new command *requires* a man page to
build right away, so let's also add a blank man page, too.
Signed-off-by: Johannes Schindelin
---
.gitignore | 1 +
Documentation/git-range-diff.txt | 10 ++
Makefile | 1 +
builtin.h| 1 +
builtin/range-diff.c | 25 +
command-list.txt | 1 +
git.c| 1 +
7 files changed, 40 insertions(+)
create mode 100644 Documentation/git-range-diff.txt
create mode 100644 builtin/range-diff.c
diff --git a/.gitignore b/.gitignore
index 3284a1e9b..cc0ad74b4 100644
--- a/.gitignore
+++ b/.gitignore
@@ -113,6 +113,7 @@
/git-pull
/git-push
/git-quiltimport
+/git-range-diff
/git-read-tree
/git-rebase
/git-rebase--am
diff --git a/Documentation/git-range-diff.txt b/Documentation/git-range-diff.txt
new file mode 100644
index 0..de0ca5df4
--- /dev/null
+++ b/Documentation/git-range-diff.txt
@@ -0,0 +1,10 @@
+git-range-diff(1)
+==
+
+NAME
+
+git-range-diff - Compare two commit ranges (e.g. two versions of a branch)
+
+GIT
+---
+Part of the linkgit:git[1] suite
diff --git a/Makefile b/Makefile
index 56326ab2b..45c9dea1b 100644
--- a/Makefile
+++ b/Makefile
@@ -1059,6 +1059,7 @@ BUILTIN_OBJS += builtin/prune-packed.o
BUILTIN_OBJS += builtin/prune.o
BUILTIN_OBJS += builtin/pull.o
BUILTIN_OBJS += builtin/push.o
+BUILTIN_OBJS += builtin/range-diff.o
BUILTIN_OBJS += builtin/read-tree.o
BUILTIN_OBJS += builtin/rebase--helper.o
BUILTIN_OBJS += builtin/receive-pack.o
diff --git a/builtin.h b/builtin.h
index 0362f1ce2..99206df4b 100644
--- a/builtin.h
+++ b/builtin.h
@@ -201,6 +201,7 @@ extern int cmd_prune(int argc, const char **argv, const
char *prefix);
extern int cmd_prune_packed(int argc, const char **argv, const char *prefix);
extern int cmd_pull(int argc, const char **argv, const char *prefix);
extern int cmd_push(int argc, const char **argv, const char *prefix);
+extern int cmd_range_diff(int argc, const char **argv, const char *prefix);
extern int cmd_read_tree(int argc, const char **argv, const char *prefix);
extern int cmd_rebase__helper(int argc, const char **argv, const char *prefix);
extern int cmd_receive_pack(int argc, const char **argv, const char *prefix);
diff --git a/builtin/range-diff.c b/builtin/range-diff.c
new file mode 100644
index 0..36788ea4f
--- /dev/null
+++ b/builtin/range-diff.c
@@ -0,0 +1,25 @@
+#include "cache.h"
+#include "builtin.h"
+#include "parse-options.h"
+
+static const char * const builtin_range_diff_usage[] = {
+N_("git range-diff [] .. .."),
+N_("git range-diff [] ..."),
+N_("git range-diff [] "),
+NULL
+};
+
+int cmd_range_diff(int argc, const char **argv, const char *prefix)
+{
+ int creation_factor = 60;
+ struct option options[] = {
+ OPT_INTEGER(0, "creation-factor", _factor,
+ N_("Percentage by which creation is weighted")),
+ OPT_END()
+ };
+
+ argc = parse_options(argc, argv, NULL, options,
+builtin_range_diff_usage, 0);
+
+ return 0;
+}
diff --git a/command-list.txt b/command-list.txt
index e1c26c1bb..a9dda3b8a 100644
--- a/command-list.txt
+++ b/command-list.txt
@@ -139,6 +139,7 @@ git-prune-packedplumbingmanipulators
git-pullmainporcelain remote
git-pushmainporcelain remote
git-quiltimport foreignscminterface
+git-range-diff mainporcelain
git-read-tree plumbingmanipulators
git-rebase mainporcelain history
git-receive-packsynchelpers
diff --git a/git.c b/git.c
index 3fded7451..6901cf328 100644
--- a/git.c
+++ b/git.c
@@ -517,6 +517,7 @@ static struct cmd_struct commands[] = {
{ "prune-packed", cmd_prune_packed, RUN_SETUP },
{ "pull", cmd_pull, RUN_SETUP | NEED_WORK_TREE },
{ "push", cmd_push, RUN_SETUP },
+ { "range-diff", cmd_range_diff, RUN_SETUP | USE_PAGER },
{ "read-tree", cmd_read_tree, RUN_SETUP | SUPPORT_SUPER_PREFIX},
{ "rebase--helper", cmd_rebase__helper, RUN_SETUP | NEED_WORK_TREE },
{ "receive-pack", cmd_receive_pack },
--
gitgitgadget
[PATCH v4 00/21] Add `range-diff`, a `tbdiff` lookalike
y new". Changes since v2: - Right-aligned the patch numbers in the commit pairs. - Used ALLOC_ARRAY() in hungarian.c instead of xmalloc(sizeof()*size). - Changed compute_assignment()s return type from int to void, as it always succeeds. - Changed the Hungarian Algorithm to use an integer cost matrix. - Changed the --creation-weight option to --creation-factor where is an integer. - Retitled 1/19 and 2/19 to better conform with the current conventions, as pointed out (and suggested) by Junio. - Shut up Coverity, and at the same time avoided passing the unnecessary `i` and `j` parameters to output_pair_header(). - Removed support for the `--no-patches` option: we inherit diff_options' support for `-s` already (and much more). - Removed the ugly `_INV` enum values, and introduced a beautiful GIT_COLOR_REVERSE instead. This way, whatever the user configured as color.diff.new (or .old) will be used in reverse in the dual color mode. - Instead of overriding the fragment header color, the dual color mode will now reverse the "outer" fragment headers, too. - Turned the stand-alone branch-diff command into the `--diff` option of `git branch`. Adjusted pretty much *all* commit messages to account for this. This change should no longer be visible: see below. - Pretty much re-wrote the completion, to support the new --diff mode of git-branch. See below: it was reverted for range-diff. - Renamed t7910 to t3206, to be closer to the git-branch tests. - Ensured that git_diff_ui_config() gets called, and therefore color.diff.* respected. - Avoided leaking `four_spaces`. - Fixed a declaration in a for (;;) statement (which Junio had as a fixup! that I almost missed). - Renamed `branch --diff`, which had been renamed from `branch-diff` (which was picked to avoid re-using `tbdiff`) to `range-diff`. - Renamed `hungarian.c` and its header to `linear-assignment.c` - Made `--dual-color` the default, and changed it to still auto-detect whether color should be used rather than forcing it Johannes Schindelin (20): linear-assignment: a function to solve least-cost assignment problems Introduce `range-diff` to compare iterations of a topic branch range-diff: first rudimentary implementation range-diff: improve the order of the shown commits range-diff: also show the diff between patches range-diff: right-trim commit messages range-diff: indent the diffs just like tbdiff range-diff: suppress the diff headers range-diff: adjust the output of the commit pairs range-diff: do not show "function names" in hunk headers range-diff: use color for the commit pairs color: add the meta color GIT_COLOR_REVERSE diff: add an internal option to dual-color diffs of diffs range-diff: offer to dual-color the diffs range-diff --dual-color: fix bogus white-space warning range-diff: populate the man page completion: support `git range-diff` range-diff: left-pad patch numbers range-diff: make --dual-color the default mode range-diff: use dim/bold cues to improve dual color mode Thomas Rast (1): range-diff: add tests .gitignore | 1 + Documentation/config.txt | 6 +- Documentation/git-range-diff.txt | 252 +++ Makefile | 3 + builtin.h | 1 + builtin/range-diff.c | 106 + cache.h| 3 +- color.h| 7 + command-list.txt | 1 + contrib/completion/git-completion.bash | 14 + diff.c | 119 - diff.h | 16 +- git.c | 1 + linear-assignment.c| 201 linear-assignment.h| 22 + range-diff.c | 440 ++ range-diff.h | 9 + t/.gitattributes | 1 + t/t3206-range-diff.sh | 145 ++ t/t3206/history.export | 604 + ws.c | 11 +- 21 files changed, 1932 insertions(+), 31 deletions(-) create mode 100644 Documentation/git-range-diff.txt create mode 100644 builtin/range-diff.c create mode 100644 linear-assignment.c create mode 100644 linear-assignment.h create mode 100644 range-diff.c create mode 100644 range-diff.h create mode 100755 t/t3206-range-diff.sh create mode 100644 t/t3206/history.export base-commit: b7bd9486b055c3f967a870311e704e3bb0654e4f Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-1%2Fdscho%2Fbranch-diff-v4 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1/dscho/branch-diff-v4 Pull-Request: https://github.com/gitgitgadget/git/pull/1 Range-diff vs v3: 1: 39272eefc ! 1: f7e70689e linear-assignment: a function to solve least-cost assignment problems @@
[PATCH v4 18/21] completion: support `git range-diff`
From: Johannes Schindelin
Tab completion of `git range-diff` is very convenient, especially
given that the revision arguments to specify the commit ranges to
compare are typically more complex than, say, what is normally passed
to `git log`.
Signed-off-by: Johannes Schindelin
---
contrib/completion/git-completion.bash | 14 ++
1 file changed, 14 insertions(+)
diff --git a/contrib/completion/git-completion.bash
b/contrib/completion/git-completion.bash
index 94c95516e..402490673 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1976,6 +1976,20 @@ _git_push ()
__git_complete_remote_or_refspec
}
+_git_range_diff ()
+{
+ case "$cur" in
+ --*)
+ __gitcomp "
+ --creation-factor= --dual-color
+ $__git_diff_common_options
+ "
+ return
+ ;;
+ esac
+ __git_complete_revlist
+}
+
_git_rebase ()
{
__git_find_repo_path
--
gitgitgadget
