[PATCH] t/chainlint.sed: drop extra spaces from regex character class
This character class, like many others in this script, matches
horizontal whitespace consisting of spaces and tabs, however, a few
extra, entirely harmless, spaces somehow slipped into the expression.
Removing them is purely a cosmetic fix.
While at it, re-indent three lines with a single TAB each which were
incorrectly indented with six spaces. Also, a purely cosmetic fix.
Signed-off-by: Eric Sunshine
---
Just something I noticed while examining t/chainlint.sed after reading
Jonathan's report[1] of a false-positive.
This is atop 'es/chain-lint-in-subshell' in 'next'.
[1]:
https://public-inbox.org/git/20180730181356.ga156...@aiede.svl.corp.google.com/
t/chainlint.sed | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/t/chainlint.sed b/t/chainlint.sed
index a0de8a3882..5f0882cb38 100644
--- a/t/chainlint.sed
+++ b/t/chainlint.sed
@@ -71,9 +71,9 @@
# incomplete line -- slurp up next line
:squash
/\\$/ {
- N
- s/\\\n//
- bsquash
+ N
+ s/\\\n//
+ bsquash
}
# here-doc -- swallow it to avoid false hits within its body (but keep the
@@ -199,7 +199,7 @@ s/.*\n//
# "$(...)" -- command substitution; not closing ")"
/\$([^)][^)]*)[^)]*$/bcheckchain
# multi-line "$(...\n...)" -- command substitution; treat as nested subshell
-/\$([ ]*$/bnest
+/\$([ ]*$/bnest
# "=(...)" -- Bash array assignment; not closing ")"
/=(/bcheckchain
# closing "...) &&"
--
2.18.0.597.ga71716f1ad
Re: b on my mailing list!
dammit! those threat actors found a way and were on for quite a whileshould we do something different? These people need a life. On Mon, Jul 30, 2018 at 9:29 PM, Christine Frankel < chrisconnorsfr...@gmail.com> wrote: > >
b on my mailing list!
Re: [PATCH 1/1] Highlight keywords in remote sideband output.
On Mon, Jul 30, 2018 at 04:52:40PM +0200, Duy Nguyen wrote:
> On Mon, Jul 30, 2018 at 2:34 PM Han-Wen Nienhuys wrote:
> > + struct kwtable {
> > + const char *keyword;
> > + const char *color;
> > + } keywords[] = {
> > + {"hint", GIT_COLOR_YELLOW},
> > + {"warning", GIT_COLOR_BOLD_YELLOW},
> > + {"success", GIT_COLOR_BOLD_GREEN},
> > + {"error", GIT_COLOR_BOLD_RED},
> > + };
>
> Please let me customize these colors. "grep color.*slot
> Documentation/config.txt help.c" could give you a few examples.
Yes, please. I use a transparent terminal and I have trouble seeing the
default red in some cases due to lack of contrast, and some people with
certain forms of colorblindness may be unable to distinguish these three
colors at all.
--
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
signature.asc
Description: PGP signature
Re: [PATCH 2/2] refs: switch for_each_replace_ref back to use a ref_store
On Mon, Jul 30, 2018 at 5:19 PM Jonathan Tan wrote:
>
> > So let's go back to the clean API, just requiring a ref_store as an
> > argument.
>
> Here, you say that we want ref_store as an argument...
I do.
>
> > -int for_each_replace_ref(struct repository *r, each_ref_fn fn, void
> > *cb_data)
> > +int for_each_replace_ref(each_ref_fn fn, void *cb_data)
> > {
> > - return do_for_each_ref(get_main_ref_store(r),
> > + return do_for_each_ref(get_main_ref_store(the_repository),
> > git_replace_ref_base, fn,
> > strlen(git_replace_ref_base),
> > DO_FOR_EACH_INCLUDE_BROKEN, cb_data);
>
> ...but there is no ref_store as an argument here - instead, the
> repository argument is deleted with no replacement. I presume you meant
> to replace it with a ref_store instead? (This will also fix the issue
> that for_each_replace_ref only works on the_repository.)
Yes, I would want to pass in a ref_store and use that as the first argument
in do_for_each_ref for now.
That would reduce the API uncleanliness to have to pass the repository twice.
> Taking a step back, was there anything that prompted these patches?
I am flailing around on how to approach the ref store and the repository:
* I dislike having to pass a repository 'r' twice. (current situation after
patch 1. That patch itself is part of Stolees larger series to address
commit graphs and replace refs, so we will have that one way or another)
* So I sent out some RFC patches to have the_repository in the ref store
and pass the repo through to all the call backs to make it easy for
users inside the callback to do basic things like looking up commits.
* both Duy (on list) and Brandon (privately) expressed their dislike for
having the refs API bloated with the repository, as the repository is
not needed per se in the ref store.
* After some reflection I agreed with their concerns, which let me
to re-examine the refs API: all but a few select functions take a
ref_store as the first argument (or imply to work on the ref store
in the_repository, then neither a repo nor a ref store argument is
there)
* I want to bring back the cleanliness of the API, which is to take a
ref store when needed instead of the repository, which is rather
bloated.
> Maybe at least the 2nd one should wait until we have a situation that
> warrants it (for example, if we want to for_each_replace_ref(), but we
> only have a ref_store, not a repository).
okay, then let's drop this series for now and I'll re-examine what is
needed to have submodule handling in-core.
Thanks,
Stefan
[PATCH 8/8] diff.c: rewrite emit_line_0 more understandably
emit_line_0 has no nested conditions, but puts out all its arguments
(if set) in order. There is still a slight confusion with set
and set_sign, but let's defer that to a later patch.
'first' used be output always no matter if it was 0, but that got lost
at "diff: add an internal option to dual-color diffs of diffs",
2018-07-21), as there we broadened the meaning of 'first' to also
signal an early return.
The change in 'emit_line' makes sure that 'first' is never content, but
always under our control, a sign or special character in the beginning
of the line (or 0, in which case we ignore it).
Signed-off-by: Stefan Beller
---
diff.c | 73 +-
1 file changed, 41 insertions(+), 32 deletions(-)
diff --git a/diff.c b/diff.c
index f565a2c0c2b..2bd4d3d6839 100644
--- a/diff.c
+++ b/diff.c
@@ -580,43 +580,52 @@ static void emit_line_0(struct diff_options *o,
int first, const char *line, int len)
{
int has_trailing_newline, has_trailing_carriage_return;
- int nofirst;
int reverse = !!set && !!set_sign;
+ int needs_reset = 0;
+
FILE *file = o->file;
fputs(diff_line_prefix(o), file);
- if (len == 0) {
- has_trailing_newline = (first == '\n');
- has_trailing_carriage_return = (!has_trailing_newline &&
- (first == '\r'));
- nofirst = has_trailing_newline || has_trailing_carriage_return;
- } else {
- has_trailing_newline = (len > 0 && line[len-1] == '\n');
- if (has_trailing_newline)
- len--;
- has_trailing_carriage_return = (len > 0 && line[len-1] == '\r');
- if (has_trailing_carriage_return)
- len--;
- nofirst = 0;
- }
-
- if (len || !nofirst) {
- if (reverse && want_color(o->use_color))
- fputs(GIT_COLOR_REVERSE, file);
- if (set_sign || set)
- fputs(set_sign ? set_sign : set, file);
- if (first && !nofirst)
- fputc(first, file);
- if (len) {
- if (set_sign && set && set != set_sign)
- fputs(reset, file);
- if (set)
- fputs(set, file);
- fwrite(line, len, 1, file);
- }
- fputs(reset, file);
+ has_trailing_newline = (len > 0 && line[len-1] == '\n');
+ if (has_trailing_newline)
+ len--;
+
+ has_trailing_carriage_return = (len > 0 && line[len-1] == '\r');
+ if (has_trailing_carriage_return)
+ len--;
+
+ if (!len && !first)
+ goto end_of_line;
+
+ if (reverse && want_color(o->use_color)) {
+ fputs(GIT_COLOR_REVERSE, file);
+ needs_reset = 1;
+ }
+
+ if (set_sign || set) {
+ fputs(set_sign ? set_sign : set, file);
+ needs_reset = 1;
}
+
+ if (first)
+ fputc(first, file);
+
+ if (!len)
+ goto end_of_line;
+
+ if (set) {
+ if (set_sign && set != set_sign)
+ fputs(reset, file);
+ fputs(set, file);
+ needs_reset = 1;
+ }
+ fwrite(line, len, 1, file);
+ needs_reset |= len > 0;
+
+end_of_line:
+ if (needs_reset)
+ fputs(reset, file);
if (has_trailing_carriage_return)
fputc('\r', file);
if (has_trailing_newline)
@@ -626,7 +635,7 @@ static void emit_line_0(struct diff_options *o,
static void emit_line(struct diff_options *o, const char *set, const char
*reset,
const char *line, int len)
{
- emit_line_0(o, set, NULL, reset, line[0], line+1, len-1);
+ emit_line_0(o, set, NULL, reset, 0, line, len);
}
enum diff_symbol {
--
2.18.0.132.g195c49a2227
[PATCH 5/8] diff.c: add set_sign to emit_line_0
For now just change the signature, we'll reason about the actual
change in a follow up patch.
Pass 'set_sign' (which is output before the sign) and 'set' which
controls the color after the first character. Hence, promote any
'set's to 'set_sign' as we want to have color before the sign
for now.
Signed-off-by: Stefan Beller
---
diff.c | 21 -
1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/diff.c b/diff.c
index 8fd2171d808..a36ed92c54c 100644
--- a/diff.c
+++ b/diff.c
@@ -576,7 +576,7 @@ static void check_blank_at_eof(mmfile_t *mf1, mmfile_t *mf2,
}
static void emit_line_0(struct diff_options *o,
- const char *set, unsigned reverse, const char *reset,
+ const char *set_sign, const char *set, unsigned
reverse, const char *reset,
int first, const char *line, int len)
{
int has_trailing_newline, has_trailing_carriage_return;
@@ -606,9 +606,12 @@ static void emit_line_0(struct diff_options *o,
if (len || !nofirst) {
if (reverse && want_color(o->use_color))
fputs(GIT_COLOR_REVERSE, file);
- fputs(set, file);
+ if (set_sign && set_sign[0])
+ fputs(set_sign, file);
if (first && !nofirst)
fputc(first, file);
+ if (set)
+ fputs(set, file);
fwrite(line, len, 1, file);
fputs(reset, file);
}
@@ -621,7 +624,7 @@ static void emit_line_0(struct diff_options *o,
static void emit_line(struct diff_options *o, const char *set, const char
*reset,
const char *line, int len)
{
- emit_line_0(o, set, 0, reset, line[0], line+1, len-1);
+ emit_line_0(o, set, NULL, 0, reset, line[0], line+1, len-1);
}
enum diff_symbol {
@@ -994,17 +997,17 @@ static void emit_line_ws_markup(struct diff_options *o,
}
if (!ws && !set_sign)
- emit_line_0(o, set, 0, reset, sign, line, len);
+ emit_line_0(o, set, NULL, 0, reset, sign, line, len);
else if (!ws) {
/* Emit just the prefix, then the rest. */
- emit_line_0(o, set_sign, !!set_sign, reset, sign, "", 0);
- emit_line_0(o, set, 0, reset, 0, line, len);
+ emit_line_0(o, set_sign, NULL, !!set_sign, reset, sign, "", 0);
+ emit_line_0(o, set, NULL, 0, reset, 0, line, len);
} else if (blank_at_eof)
/* Blank line at EOF - paint '+' as well */
- emit_line_0(o, ws, 0, reset, sign, line, len);
+ emit_line_0(o, ws, NULL, 0, reset, sign, line, len);
else {
/* Emit just the prefix, then the rest. */
- emit_line_0(o, set_sign ? set_sign : set, !!set_sign, reset,
+ emit_line_0(o, set_sign ? set_sign : set, NULL, !!set_sign,
reset,
sign, "", 0);
ws_check_emit(line, len, ws_rule,
o->file, set, reset, ws);
@@ -1028,7 +1031,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);
putc('\n', o->file);
- emit_line_0(o, context, 0, reset, '\\',
+ emit_line_0(o, context, NULL, 0, reset, '\\',
nneof, strlen(nneof));
break;
case DIFF_SYMBOL_SUBMODULE_HEADER:
--
2.18.0.132.g195c49a2227
[PATCH 6/8] diff: use emit_line_0 once per line
All lines that use emit_line_0 multiple times per line, are combined
into a single call to emit_line_0, making use of the 'set' argument.
Signed-off-by: Stefan Beller
---
diff.c | 26 --
1 file changed, 12 insertions(+), 14 deletions(-)
diff --git a/diff.c b/diff.c
index a36ed92c54c..fdad7ffdd77 100644
--- a/diff.c
+++ b/diff.c
@@ -583,10 +583,7 @@ static void emit_line_0(struct diff_options *o,
int nofirst;
FILE *file = o->file;
- if (first)
- fputs(diff_line_prefix(o), file);
- else if (!len)
- return;
+ fputs(diff_line_prefix(o), file);
if (len == 0) {
has_trailing_newline = (first == '\n');
@@ -606,13 +603,17 @@ static void emit_line_0(struct diff_options *o,
if (len || !nofirst) {
if (reverse && want_color(o->use_color))
fputs(GIT_COLOR_REVERSE, file);
- if (set_sign && set_sign[0])
- fputs(set_sign, file);
+ if (set_sign || set)
+ fputs(set_sign ? set_sign : set, file);
if (first && !nofirst)
fputc(first, file);
- if (set)
- fputs(set, file);
- fwrite(line, len, 1, file);
+ if (len) {
+ if (set_sign && set && set != set_sign)
+ fputs(reset, file);
+ if (set)
+ fputs(set, file);
+ fwrite(line, len, 1, file);
+ }
fputs(reset, file);
}
if (has_trailing_carriage_return)
@@ -999,16 +1000,13 @@ static void emit_line_ws_markup(struct diff_options *o,
if (!ws && !set_sign)
emit_line_0(o, set, NULL, 0, reset, sign, line, len);
else if (!ws) {
- /* Emit just the prefix, then the rest. */
- emit_line_0(o, set_sign, NULL, !!set_sign, reset, sign, "", 0);
- emit_line_0(o, set, NULL, 0, reset, 0, line, len);
+ emit_line_0(o, set_sign, set, !!set_sign, reset, sign, line,
len);
} else if (blank_at_eof)
/* Blank line at EOF - paint '+' as well */
emit_line_0(o, ws, NULL, 0, reset, sign, line, len);
else {
/* Emit just the prefix, then the rest. */
- emit_line_0(o, set_sign ? set_sign : set, NULL, !!set_sign,
reset,
- sign, "", 0);
+ emit_line_0(o, set_sign, set, !!set_sign, reset, sign, "", 0);
ws_check_emit(line, len, ws_rule,
o->file, set, reset, ws);
}
--
2.18.0.132.g195c49a2227
[PATCH 7/8] diff.c: compute reverse locally in emit_line_0
Signed-off-by: Stefan Beller
---
diff.c | 15 ---
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/diff.c b/diff.c
index fdad7ffdd77..f565a2c0c2b 100644
--- a/diff.c
+++ b/diff.c
@@ -576,11 +576,12 @@ static void check_blank_at_eof(mmfile_t *mf1, mmfile_t
*mf2,
}
static void emit_line_0(struct diff_options *o,
- const char *set_sign, const char *set, unsigned
reverse, const char *reset,
+ const char *set_sign, const char *set, const char
*reset,
int first, const char *line, int len)
{
int has_trailing_newline, has_trailing_carriage_return;
int nofirst;
+ int reverse = !!set && !!set_sign;
FILE *file = o->file;
fputs(diff_line_prefix(o), file);
@@ -625,7 +626,7 @@ static void emit_line_0(struct diff_options *o,
static void emit_line(struct diff_options *o, const char *set, const char
*reset,
const char *line, int len)
{
- emit_line_0(o, set, NULL, 0, reset, line[0], line+1, len-1);
+ emit_line_0(o, set, NULL, reset, line[0], line+1, len-1);
}
enum diff_symbol {
@@ -998,15 +999,15 @@ static void emit_line_ws_markup(struct diff_options *o,
}
if (!ws && !set_sign)
- emit_line_0(o, set, NULL, 0, reset, sign, line, len);
+ emit_line_0(o, set, NULL, reset, sign, line, len);
else if (!ws) {
- emit_line_0(o, set_sign, set, !!set_sign, reset, sign, line,
len);
+ emit_line_0(o, set_sign, set, reset, sign, line, len);
} else if (blank_at_eof)
/* Blank line at EOF - paint '+' as well */
- emit_line_0(o, ws, NULL, 0, reset, sign, line, len);
+ emit_line_0(o, ws, NULL, reset, sign, line, len);
else {
/* Emit just the prefix, then the rest. */
- emit_line_0(o, set_sign, set, !!set_sign, reset, sign, "", 0);
+ emit_line_0(o, set_sign, set, reset, sign, "", 0);
ws_check_emit(line, len, ws_rule,
o->file, set, reset, ws);
}
@@ -1029,7 +1030,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);
putc('\n', o->file);
- emit_line_0(o, context, NULL, 0, reset, '\\',
+ emit_line_0(o, context, NULL, reset, '\\',
nneof, strlen(nneof));
break;
case DIFF_SYMBOL_SUBMODULE_HEADER:
--
2.18.0.132.g195c49a2227
[PATCH 4/8] diff.c: reorder arguments for emit_line_ws_markup
The order shall be all colors first, then the content, flags at the end.
The colors are in order.
Signed-off-by: Stefan Beller
---
diff.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/diff.c b/diff.c
index f7251c89cbb..8fd2171d808 100644
--- a/diff.c
+++ b/diff.c
@@ -980,9 +980,9 @@ 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,
- const char *set_sign, char sign,
+ const char *set_sign, const char *set,
+ const char *reset,
+ char sign, const char *line, int len,
unsigned ws_rule, int blank_at_eof)
{
const char *ws = NULL;
@@ -1066,7 +1066,7 @@ static void emit_diff_symbol_from_struct(struct
diff_options *o,
else if (c == '-')
set = diff_get_color_opt(o, DIFF_FILE_OLD);
}
- emit_line_ws_markup(o, set, reset, line, len, set_sign, ' ',
+ emit_line_ws_markup(o, set_sign, set, reset, ' ', line, len,
flags & (DIFF_SYMBOL_CONTENT_WS_MASK), 0);
break;
case DIFF_SYMBOL_PLUS:
@@ -1109,7 +1109,7 @@ static void emit_diff_symbol_from_struct(struct
diff_options *o,
set = diff_get_color_opt(o, DIFF_CONTEXT_BOLD);
flags |= WS_IGNORE_FIRST_SPACE;
}
- emit_line_ws_markup(o, set, reset, line, len, set_sign, '+',
+ emit_line_ws_markup(o, set_sign, set, reset, '+', line, len,
flags & DIFF_SYMBOL_CONTENT_WS_MASK,
flags & DIFF_SYMBOL_CONTENT_BLANK_LINE_EOF);
break;
@@ -1152,7 +1152,7 @@ static void emit_diff_symbol_from_struct(struct
diff_options *o,
else
set = diff_get_color_opt(o, DIFF_CONTEXT_DIM);
}
- emit_line_ws_markup(o, set, reset, line, len, set_sign, '-',
+ emit_line_ws_markup(o, set_sign, set, reset, '-', line, len,
flags & DIFF_SYMBOL_CONTENT_WS_MASK, 0);
break;
case DIFF_SYMBOL_WORDS_PORCELAIN:
--
2.18.0.132.g195c49a2227
[PATCH 1/8] test_decode_color: understand FAINT and ITALIC
Signed-off-by: Stefan Beller
---
t/test-lib-functions.sh | 2 ++
1 file changed, 2 insertions(+)
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 2b2181dca09..be8244c47b5 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -42,6 +42,8 @@ test_decode_color () {
function name(n) {
if (n == 0) return "RESET";
if (n == 1) return "BOLD";
+ if (n == 2) return "FAINT";
+ if (n == 3) return "ITALIC";
if (n == 7) return "REVERSE";
if (n == 30) return "BLACK";
if (n == 31) return "RED";
--
2.18.0.132.g195c49a2227
[PATCH 2/8] t3206: add color test for range-diff --dual-color
The 'expect'ed outcome has been taken by running the 'range-diff | decode'. Signed-off-by: Stefan Beller --- t/t3206-range-diff.sh | 39 +++ 1 file changed, 39 insertions(+) diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh index 2237c7f4af9..019724e61a0 100755 --- a/t/t3206-range-diff.sh +++ b/t/t3206-range-diff.sh @@ -142,4 +142,43 @@ test_expect_success 'changed message' ' test_cmp expected actual ' +test_expect_success 'dual-coloring' ' + cat >expect <<-\EOF && + 1: a4b = 1: f686024 s/5/A/ + 2: f51d370 ! 2: 4ab067d s/4/A/ + @@ -2,6 +2,8 @@ + +s/4/A/ + + +Also a silly comment here! + + +diff --git a/file b/file +--- a/file ++++ b/file + 3: 0559556 ! 3: b9cb956 s/11/B/ + @@ -10,7 +10,7 @@ + 9 + 10 +-11 + -+BB + ++B + 12 + 13 + 14 + 4: d966c5c ! 4: 8add5f1 s/12/B/ + @@ -8,7 +8,7 @@ +@@ + 9 + 10 + - BB + + B +-12 ++B + 13 + EOF + git range-diff changed...changed-message --color --dual-color >actual.raw && + test_decode_color >actual
[PATCHv2 0/8] Add color test for range-diff, simplify diff.c
addressed all of Erics feedback:
* reworded commit messages
* dropped q_to_tab and use cat instead
* use -\EOF isntead of -EOF
Thanks,
Stefan
Stefan Beller (8):
test_decode_color: understand FAINT and ITALIC
t3206: add color test for range-diff --dual-color
diff.c: simplify caller of emit_line_0
diff.c: reorder arguments for emit_line_ws_markup
diff.c: add set_sign to emit_line_0
diff: use emit_line_0 once per line
diff.c: compute reverse locally in emit_line_0
diff.c: rewrite emit_line_0 more understandably
diff.c | 94 +++--
t/t3206-range-diff.sh | 39 +
t/test-lib-functions.sh | 2 +
3 files changed, 93 insertions(+), 42 deletions(-)
./git-range-diff ws_cleanup-ontop-range-diff-2@{2.hours.ago}...HEAD
>>-cover-letter.patch
[dropped changes to range-diff itself]
13: a02ea020ae7 ! 13: 16f71b43f48 t3206: add color test for range-diff
--dual-color
@@ -2,9 +2,7 @@
t3206: add color test for range-diff --dual-color
-The 'expect'ed outcome is taken by running the 'range-diff |decode';
-it is not meant as guidance, rather as a documentation of the current
-situation.
+The 'expect'ed outcome has been taken by running the 'range-diff |
decode'.
Signed-off-by: Stefan Beller
@@ -15,8 +13,8 @@
test_cmp expected actual
'
-+test_expect_success 'simple coloring' '
-+ q_to_tab >expect <<-EOF &&
++test_expect_success 'dual-coloring' '
++ cat >expect <<-\EOF &&
+ 1: a4b = 1: f686024 s/5/A/
+ 2: f51d370 ! 2:
4ab067d s/4/A/
+ @@ -2,6 +2,8 @@
14: c8734075229 = 14: abd1ec80608 diff.c: simplify caller of emit_line_0
15: ba98acffcda = 15: bc29037f4f0 diff.c: reorder arguments for
emit_line_ws_markup
16: 5a576baeb49 ! 16: 8f6ee340f1e diff.c: add set_sign to emit_line_0
@@ -5,9 +5,10 @@
For now just change the signature, we'll reason about the actual
change in a follow up patch.
-Pass set_sign (which is output before the sign) and set that is setting
-the color after the sign. Hence, promote any 'set's to set_sign as
-we want to have color before the sign for now.
+Pass 'set_sign' (which is output before the sign) and 'set' which
+controls the color after the first character. Hence, promote any
+'set's to 'set_sign' as we want to have color before the sign
+for now.
Signed-off-by: Stefan Beller
17: 4e2d5a4c7f3 = 17: 0ab5920a9ab diff: use emit_line_0 once per line
18: 460713e1c3c = 18: 2d05ebdd280 diff.c: compute reverse locally in
emit_line_0
19: e442d722b7f ! 19: 001e6042d81 diff.c: rewrite emit_line_0 more
understandably
@@ -7,9 +7,9 @@
and set_sign, but let's defer that to a later patch.
'first' used be output always no matter if it was 0, but that got lost
-got lost at e8c285c4f9c (diff: add an internal option to dual-color
-diffs of diffs, 2018-07-21), as there we broadened the meaning of
'first'
-to also signal an early return.
+at "diff: add an internal option to dual-color diffs of diffs",
+2018-07-21), as there we broadened the meaning of 'first' to also
+signal an early return.
The change in 'emit_line' makes sure that 'first' is never content, but
always under our control, a sign or special character in the beginning
Re: [PATCH 2/2] refs: switch for_each_replace_ref back to use a ref_store
> So let's go back to the clean API, just requiring a ref_store as an
> argument.
Here, you say that we want ref_store as an argument...
> -int for_each_replace_ref(struct repository *r, each_ref_fn fn, void *cb_data)
> +int for_each_replace_ref(each_ref_fn fn, void *cb_data)
> {
> - return do_for_each_ref(get_main_ref_store(r),
> + return do_for_each_ref(get_main_ref_store(the_repository),
> git_replace_ref_base, fn,
> strlen(git_replace_ref_base),
> DO_FOR_EACH_INCLUDE_BROKEN, cb_data);
...but there is no ref_store as an argument here - instead, the
repository argument is deleted with no replacement. I presume you meant
to replace it with a ref_store instead? (This will also fix the issue
that for_each_replace_ref only works on the_repository.)
Taking a step back, was there anything that prompted these patches?
Maybe at least the 2nd one should wait until we have a situation that
warrants it (for example, if we want to for_each_replace_ref(), but we
only have a ref_store, not a repository).
Re: [PATCH] tests: make use of the test_must_be_empty function
On Fri, Jul 27, 2018 at 7:48 PM Ævar Arnfjörð Bjarmason
wrote:
>
> Change various tests that use an idiom of the form:
>
> >expect &&
> test_cmp expect actual
>
> To instead use:
>
> test_must_be_empty actual
Thanks for working on this.
> The test_must_be_empty() wrapper was introduced in ca8d148daf ("test:
> test_must_be_empty helper", 2013-06-09). Many of these tests have been
> added after that time. This was mostly found with, and manually pruned
> from:
>
> git grep '^\s+>.*expect.* &&$' t
This command doesn't output anything as it is, it should be 'git grep
-E ...'.
Furthermore, it doesn't catch the cases when the empty expected file
is written as ': > expect'. I see that you noticed and converted a
few of these, too, but running
git grep ': >.*exp.*&&' t/
turns up some more.
partnership offer,
I want us to join hands as partners because i have a deal for you.
partnership offer,
I want us to join hands as partners because i have a deal for you.
Re: [PATCH v4 11/21] range-diff: add tests
On Mon, Jul 30, 2018 at 1:18 PM Junio C Hamano wrote: > > I already pushed an update to https://github.com/gitgitgadget/git/pull/1. > > Should I take "pushed to ... GGG" to mean "do not merge what you > have to 'next' yet, as there will be an updated series (not > incremental) being prepared"? Not speaking for Johannes, but I think that *could* work. On the other hand full rerolls are more expensive to prep (at least for me, I guess GGG might change that) git range-diff gitgitgadget/pr/1...origin/js/range-diff shows there is * a different header guard: @@ -418,8 +419,8 @@ --- /dev/null +++ b/range-diff.h @@ -+#ifndef RANGE_DIFF_H -+#define RANGE_DIFF_H ++#ifndef BRANCH_DIFF_H ++#define BRANCH_DIFF_H + * another different header guard: @@ -239,8 +240,8 @@ --- /dev/null +++ b/linear-assignment.h @@ -+#ifndef LINEAR_ASSIGNMENT_H -+#define LINEAR_ASSIGNMENT_H ++#ifndef HUNGARIAN_H ++#define HUNGARIAN_H + although this one is the other way round. Did you squash in one header guard and Johannes fixed a header guard in a different file? With that said, I can send my series for more color testing and better diff.c code on top of either. (https://public-inbox.org/git/20180728030448.192177-1-sbel...@google.com/) Stefan
Re: [PATCH] refspec: allow @ on the left-hand side of refspecs
On Mon, Jul 30, 2018 at 10:50:51AM -0700, Brandon Williams wrote: > On 07/29, brian m. carlson wrote: > > The object ID parsing machinery is aware of "@" as a synonym for "HEAD" > > and this is documented accordingly in gitrevisions(7). The push > > documentation describes the source portion of a refspec as "any > > arbitrary 'SHA-1 expression'"; however, "@" is not allowed on the > > left-hand side of a refspec, since we attempt to check for it being a > > valid ref name and fail (since it is not). > > > > Teach the refspec machinery about this alias and silently substitute > > "HEAD" when we see "@". This handles the fact that HEAD is a symref and > > preserves its special behavior. We need not handle other arbitrary > > object ID expressions (such as "@^") when pushing because the revision > > machinery already handles that for us. > > So this claims that using "@^" should work despite not accounting for it > explicitly or am I misreading? Unless I'm mistaken, it looks like we > don't really support arbitrary rev syntax in refspecs since "HEAD^" > doesn't work either. Correct, it does indeed work, at least for me: genre ok % git push castro HEAD^:refs/heads/temp Total 0 (delta 0), reused 0 (delta 0) To https://git.crustytoothpaste.net/git/bmc/git.git * [new branch]HEAD^ -> temp genre ok % git push castro @^:refs/heads/temp Total 0 (delta 0), reused 0 (delta 0) To https://git.crustytoothpaste.net/git/bmc/git.git * [new branch]@^ -> temp Note that in this case, I had to specify a full ref since it didn't exist on the remote and the left side wasn't a ref name. Now it doesn't work for fetches, only pushes. Only the left side of a push refspec can be an arbitrary expression. -- brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204 signature.asc Description: PGP signature
[PATCH 1/3] t1300: document current behavior of setting options
This documents current behavior of the config machinery, when changing the value of some settings. This patch just serves to provide a baseline for the follow up that will fix some issues with the current behavior. Signed-off-by: Stefan Beller --- t/t1300-config.sh | 86 +++ 1 file changed, 86 insertions(+) diff --git a/t/t1300-config.sh b/t/t1300-config.sh index 03c223708eb..ced13012409 100755 --- a/t/t1300-config.sh +++ b/t/t1300-config.sh @@ -1218,6 +1218,92 @@ test_expect_success 'last one wins: three level vars' ' test_cmp expect actual ' +test_expect_success 'old-fashioned settings are case insensitive' ' + test_when_finished "rm -f testConfig testConfig_expect testConfig_actual" && + + cat >testConfig_actual <<-EOF && + [V.A] + r = value1 + EOF + q_to_tab >testConfig_expect <<-EOF && + [V.A] + Qr = value2 + EOF + git config -f testConfig_actual "v.a.r" value2 && + test_cmp testConfig_expect testConfig_actual && + + cat >testConfig_actual <<-EOF && + [V.A] + r = value1 + EOF + q_to_tab >testConfig_expect <<-EOF && + [V.A] + QR = value2 + EOF + git config -f testConfig_actual "V.a.R" value2 && + test_cmp testConfig_expect testConfig_actual && + + cat >testConfig_actual <<-EOF && + [V.A] + r = value1 + EOF + q_to_tab >testConfig_expect <<-EOF && + [V.A] + r = value1 + Qr = value2 + EOF + git config -f testConfig_actual "V.A.r" value2 && + test_cmp testConfig_expect testConfig_actual && + + cat >testConfig_actual <<-EOF && + [V.A] + r = value1 + EOF + q_to_tab >testConfig_expect <<-EOF && + [V.A] + r = value1 + Qr = value2 + EOF + git config -f testConfig_actual "v.A.r" value2 && + test_cmp testConfig_expect testConfig_actual +' + +test_expect_success 'setting different case sensitive subsections ' ' + test_when_finished "rm -f testConfig testConfig_expect testConfig_actual" && + + cat >testConfig_actual <<-EOF && + [V "A"] + R = v1 + [K "E"] + Y = v1 + [a "b"] + c = v1 + [d "e"] + f = v1 + EOF + q_to_tab >testConfig_expect <<-EOF && + [V "A"] + Qr = v2 + [K "E"] + Qy = v2 + [a "b"] + Qc = v2 + [d "e"] + f = v1 + Qf = v2 + EOF + # exact match + git config -f testConfig_actual a.b.c v2 && + # match section and subsection, key is cased differently. + git config -f testConfig_actual K.E.y v2 && + # section and key are matched case insensitive, but subsection needs + # to match; When writing out new values only the key is adjusted + git config -f testConfig_actual v.A.r v2 && + # subsection is not matched: + git config -f testConfig_actual d.E.f v2 && + test_cmp testConfig_expect testConfig_actual +' + for VAR in a .a a. a.0b a."b c". a."b c".0d do test_expect_success "git -c $VAR=VAL rejects invalid '$VAR'" ' -- 2.18.0.132.g195c49a2227
[PATCH 3/3] config: treat section case insensitive in store_aux_event
This is a no-op because the section names are lower-cased already in
get_base_var, this is purely for demonstration that we do not need to
care about case issues in this part of the code.
Signed-off-by: Stefan Beller
---
config.c | 16 +++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/config.c b/config.c
index de646d2c56f..c247164ad17 100644
--- a/config.c
+++ b/config.c
@@ -2355,14 +2355,28 @@ static int store_aux_event(enum config_event_t type,
store->parsed[store->parsed_nr].type = type;
if (type == CONFIG_EVENT_SECTION) {
+ char *p;
+ int slen; /* section length */
if (cf->var.len < 2 || cf->var.buf[cf->var.len - 1] != '.')
return error("invalid section name '%s'", cf->var.buf);
+ p = strchr(cf->var.buf, '.');
+ if (!p)
+ /* no subsection, so treat all as section: */
+ slen = store->baselen;
+ else
+ slen = p - cf->var.buf;
+
+ if (slen > store->baselen)
+ slen = store->baselen;
+
/* Is this the section we were looking for? */
store->is_keys_section =
store->parsed[store->parsed_nr].is_keys_section =
cf->var.len - 1 == store->baselen &&
- !strncmp(cf->var.buf, store->key, store->baselen);
+ !strncasecmp(cf->var.buf, store->key, slen) &&
+ !strncmp(cf->var.buf + slen, store->key + slen,
+store->baselen - slen);
if (store->is_keys_section) {
store->section_seen = 1;
ALLOC_GROW(store->seen, store->seen_nr + 1,
--
2.18.0.132.g195c49a2227
[PATCH 2/3] config: fix case sensitive subsection names on writing
A use reported a submodule issue regarding strange case indentation
issues, but it could be boiled down to the following test case:
$ git init test && cd test
$ git config foo."Bar".key test
$ git config foo."bar".key test
$ tail -n 3 .git/config
[foo "Bar"]
key = test
key = test
Sub sections are case sensitive and we have a test for correctly reading
them. However we do not have a test for writing out config correctly with
case sensitive subsection names, which is why this went unnoticed in
6ae996f2acf (git_config_set: make use of the config parser's event
stream, 2018-04-09)
Make the subsection case sensitive and provide a test for writing.
The test for reading is just above this test.
Reported-by: JP Sugarbroad
Signed-off-by: Stefan Beller
---
config.c | 2 +-
t/t1300-config.sh | 3 +++
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/config.c b/config.c
index 7968ef7566a..de646d2c56f 100644
--- a/config.c
+++ b/config.c
@@ -2362,7 +2362,7 @@ static int store_aux_event(enum config_event_t type,
store->is_keys_section =
store->parsed[store->parsed_nr].is_keys_section =
cf->var.len - 1 == store->baselen &&
- !strncasecmp(cf->var.buf, store->key, store->baselen);
+ !strncmp(cf->var.buf, store->key, store->baselen);
if (store->is_keys_section) {
store->section_seen = 1;
ALLOC_GROW(store->seen, store->seen_nr + 1,
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index ced13012409..e5d16f53ee1 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -1250,6 +1250,7 @@ test_expect_success 'old-fashioned settings are case
insensitive' '
q_to_tab >testConfig_expect <<-EOF &&
[V.A]
r = value1
+ [V "A"]
Qr = value2
EOF
git config -f testConfig_actual "V.A.r" value2 &&
@@ -1262,6 +1263,7 @@ test_expect_success 'old-fashioned settings are case
insensitive' '
q_to_tab >testConfig_expect <<-EOF &&
[V.A]
r = value1
+ [v "A"]
Qr = value2
EOF
git config -f testConfig_actual "v.A.r" value2 &&
@@ -1290,6 +1292,7 @@ test_expect_success 'setting different case sensitive
subsections ' '
Qc = v2
[d "e"]
f = v1
+ [d "E"]
Qf = v2
EOF
# exact match
--
2.18.0.132.g195c49a2227
[PATCH 0/3] config: fix case sensitive subsection names on writing
On Mon, Jul 30, 2018 at 5:50 AM Johannes Schindelin wrote: > 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? 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... It turns out it doesn't quite do that; The parsing code takes the old notation into account and translates any [V.A] r = ... into a lower cased "v.a." for ease of comparison. That happens in get_base_var, which would call further into get_extended_base_var if the new notation is used. The code in store_aux_event however is written without the consideration of the old code and has no way of knowing the capitalization of the section or subsection (which were forced to lowercase in the old dot notation). So either we have to do some major surgery, or the old notation gets some regression while fixing the new notation. > > 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. I wrote some patches that show more of what is happening. I suggest to drop the last patch and only take the first two, or if we decide we want to be fully correct, we'd want to discuss how to make it happen. (where to store the information that we are dealing with an old notation) Thanks, Stefan Stefan Beller (3): t1300: document current behavior of setting options config: fix case sensitive subsection names on writing config: treat section case insensitive in store_aux_event config.c | 16 - t/t1300-config.sh | 89 +++ 2 files changed, 104 insertions(+), 1 deletion(-) -- 2.18.0.132.g195c49a2227
[PATCH] transport: report refs only if transport does
Commit 989b8c4452 ("fetch-pack: put shallow info in output parameter",
2018-06-28) allows transports to report the refs that they have fetched
in a new out-parameter "fetched_refs". If they do so,
transport_fetch_refs() makes this information available to its caller.
Because transport_fetch_refs() filters the refs sent to the transport,
it cannot just report the transport's result directly, but first needs
to readd the excluded refs, pretending that they are fetched. However,
this results in a wrong result if the transport did not report the refs
that they have fetched in "fetched_refs" - the excluded refs would be
added and reported, presenting an incomplete picture to the caller.
Instead, readd the excluded refs only if the transport reported fetched
refs.
Signed-off-by: Jonathan Tan
---
Thanks for the reproduction recipe, Peff. Here's a fix. It can be
reproduced with something using a remote helper's fetch command (and not
using "connect" or "stateless-connect"), fetching at least one ref that
requires a ref update and at least one that does not (as you can see
from the included test).
---
t/t5551-http-fetch-smart.sh | 18 ++
transport.c | 32
2 files changed, 42 insertions(+), 8 deletions(-)
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 913089b144..989d034acc 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -369,6 +369,24 @@ test_expect_success 'custom http headers' '
submodule update sub
'
+test_expect_success 'using fetch command in remote-curl updates refs' '
+ SERVER="$HTTPD_DOCUMENT_ROOT_PATH/twobranch" &&
+ rm -rf "$SERVER" client &&
+
+ git init "$SERVER" &&
+ test_commit -C "$SERVER" foo &&
+ git -C "$SERVER" update-ref refs/heads/anotherbranch foo &&
+
+ git clone $HTTPD_URL/smart/twobranch client &&
+
+ test_commit -C "$SERVER" bar &&
+ git -C client -c protocol.version=0 fetch &&
+
+ git -C "$SERVER" rev-parse master >expect &&
+ git -C client rev-parse origin/master >actual &&
+ test_cmp expect actual
+'
+
test_expect_success 'GIT_REDACT_COOKIES redacts cookies' '
rm -rf clone &&
echo "Set-Cookie: Foo=1" >cookies &&
diff --git a/transport.c b/transport.c
index fdd813f684..2a2415d79c 100644
--- a/transport.c
+++ b/transport.c
@@ -1230,17 +1230,18 @@ int transport_fetch_refs(struct transport *transport,
struct ref *refs,
struct ref **heads = NULL;
struct ref *nop_head = NULL, **nop_tail = _head;
struct ref *rm;
+ struct ref *fetched_by_transport = NULL;
for (rm = refs; rm; rm = rm->next) {
nr_refs++;
if (rm->peer_ref &&
!is_null_oid(>old_oid) &&
!oidcmp(>peer_ref->old_oid, >old_oid)) {
- /*
-* These need to be reported as fetched, but we don't
-* actually need to fetch them.
-*/
if (fetched_refs) {
+ /*
+* These may need to be reported as fetched,
+* but we don't actually need to fetch them.
+*/
struct ref *nop_ref = copy_ref(rm);
*nop_tail = nop_ref;
nop_tail = _ref->next;
@@ -1264,10 +1265,25 @@ int transport_fetch_refs(struct transport *transport,
struct ref *refs,
heads[nr_heads++] = rm;
}
- rc = transport->vtable->fetch(transport, nr_heads, heads, fetched_refs);
- if (fetched_refs && nop_head) {
- *nop_tail = *fetched_refs;
- *fetched_refs = nop_head;
+ rc = transport->vtable->fetch(transport, nr_heads, heads,
+ fetched_refs ? _by_transport :
NULL);
+ if (fetched_refs) {
+ if (fetched_by_transport) {
+ /*
+* The transport reported its fetched refs. Pretend
+* that we also fetched the ones that we didn't need to
+* fetch.
+*/
+ *nop_tail = fetched_by_transport;
+ *fetched_refs = nop_head;
+ } else if (!fetched_by_transport) {
+ /*
+* The transport didn't report its fetched refs, so
+* this function will not report them either. We have
+* no use for nop_head.
+*/
+ free_refs(nop_head);
+ }
}
free(heads);
--
2.18.0.345.g5c9ce644c3-goog
あなたのApple IDはロックされています。
Re: [PATCH v4 0/4] Rerolling patch series to fix t7501
Samuel Lijin writes: > Following up on Junio's review from last time. > > Samuel Lijin (4): > t7501: add coverage for flags which imply dry runs > wt-status: rename commitable to committable > wt-status: teach wt_status_collect about merges in progress > commit: fix exit code when doing a dry run > > builtin/commit.c | 32 +++--- > ref-filter.c | 3 +- > t/t7501-commit.sh | 150 --- > wt-status.c | 258 -- > wt-status.h | 13 +-- > 5 files changed, 298 insertions(+), 158 deletions(-) It seems that t7512 & t7060 break with this topic queued.
Re: [PATCH v3 00/10] fsck: doc fixes & fetch.fsck.* implementation
't5504-fetch-receive-strict.sh', in particular the test 'push with transfer.fsckobjects' failed on me while building this branch the other day, caused by 'test_cmp' failing, because 'git push' didn't print anything to its standard input. I was unable to reproduce the failure with the usual means (running the test repeatedly for a long time while the machine is under heavy load), and given that this test has a history of raciness[1] with the same symptom, I'm not sure whether this patch series does something wrong, or perhaps the fixes[2] are incomplete. Or the Blood Moon. Sidenote: I found some of the error messages from the expectedly failing 'git fetch' and 'git push' commands misleading. Some of them output "fatal: object of unexpected type", even though the original and the corrupted objects are both of the same type (blob). 1 - 8bf4becf0c (add "ok=sigpipe" to test_must_fail and use it to fix flaky tests, 2015-11-27) 2 - five commits leading up to c4b27511ab (t5504: drop sigpipe=ok from push tests, 2016-04-19)
Re: [GSoC][PATCH v4 15/20] rebase -i: rewrite write_basic_state() in C
Hi,
Le 30/07/2018 à 20:25, SZEDER Gábor a écrit :
>> diff --git a/sequencer.c b/sequencer.c
>> index 1c035ceec7..d257903db0 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>
>> +int write_basic_state(struct replay_opts *opts, const char *head_name,
>> + const char *onto, const char *orig_head)
>> +{
>> +const char *quiet = getenv("GIT_QUIET");
>> +
>> +if (head_name)
>> +write_file(rebase_path_head_name(), "%s\n", head_name);
>> +if (onto)
>> +write_file(rebase_path_onto(), "%s\n", onto);
>> +if (orig_head)
>> +write_file(rebase_path_orig_head(), "%s\n", orig_head);
>> +
>> +if (quiet)
>> +write_file(rebase_path_quiet(), "%s\n", quiet);
>> +else
>> +write_file(rebase_path_quiet(), "");
>
> This is not a faithful conversion of the original. git-rebase.sh writes
> this 'quiet' file with:
>
> echo "$GIT_QUIET" > "$state_dir"/quiet
>
> which means that a single newline character was written even when
> $GIT_QUIET was unset/empty.
>
> I seem to recall a case in the past, when a shell-to-C conversion
> accidentally dropped a newline from a similar state-file, which then
> caused some issues later on. But I don't remember the specifics and a
> quick search didn't turn up anything relevant either...
>
I don’t think it’s a problem here, but we’re better safe than sorry.
I’ll send a fix soon.
Cheers,
Alban
Re: [PATCH] builtin.h: remove declaration of cmd_rebase__helper
Hi Ramsay,
Le 30/07/2018 à 00:44, Ramsay Jones a écrit :
>
> Commit 94d4e2fb88 ("rebase -i: move rebase--helper modes to
> rebase--interactive", 2018-07-24) removed the definition of the
> 'cmd_rebase__helper' symbol, but forgot to remove the corresponding
> declaration in the 'builtin.h' header file.
>
> Signed-off-by: Ramsay Jones
> ---
>
> Hi Alban,
>
> If you need to re-roll your 'ag/rebase-i-in-c' branch, could you
> please squash this into the relevant patch (commit 94d4e2fb88).
>
> Thanks!
>
> ATB,
> Ramsay Jones
>
> builtin.h | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/builtin.h b/builtin.h
> index aac8f5f340..6538932e99 100644
> --- a/builtin.h
> +++ b/builtin.h
> @@ -206,7 +206,6 @@ 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(int argc, const char **argv, const char *prefix);
> extern int cmd_rebase__interactive(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);
> extern int cmd_reflog(int argc, const char **argv, const char *prefix);
> extern int cmd_remote(int argc, const char **argv, const char *prefix);
>
Thank you for pointing this out! I will include it in my next reroll.
Cheers,
Alban
Re: [PATCH v4 05/21] range-diff: also show the diff between patches
On Mon, Jul 30, 2018 at 5:26 PM Thomas Gummerer wrote: > On 07/30, Johannes Schindelin wrote: > > On Sun, 29 Jul 2018, Thomas Gummerer wrote: > > > There's one more thing that I noticed here: > > > > > > git range-diff --no-patches > > > fatal: single arg format requires a symmetric range > > > > > 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? > > Good point. I can't really come up with a good option right now > either. It's not too bad, as users just typed the command, so it > should be easy enough to see from the previous line what went wrong. I think you can attain the desired behavior by making a final parse_options() call with empty 'options' list after the call to diff_setup_done(). It's pretty much a one-line fix, but can probably be done as an incremental change rather than rerolling.
Re: [PATCH 1/1] Highlight keywords in remote sideband output.
Han-Wen Nienhuys writes:
> The highlighting is done on the client-side. Supported keywords are
> "error", "warning", "hint" and "success".
>
> The colorization is controlled with the config setting "color.remote".
>
> Signed-off-by: Han-Wen Nienhuys
> ---
> sideband.c | 78 +
> t/t5409-colorize-remote-messages.sh | 34 +
> 2 files changed, 103 insertions(+), 9 deletions(-)
> create mode 100644 t/t5409-colorize-remote-messages.sh
>
> diff --git a/sideband.c b/sideband.c
> index 325bf0e97..e939cd436 100644
> --- a/sideband.c
> +++ b/sideband.c
> @@ -1,7 +1,63 @@
> #include "cache.h"
> +#include "color.h"
> +#include "config.h"
> #include "pkt-line.h"
> #include "sideband.h"
>
> +
> +/*
> + * Optionally highlight some keywords in remote output if they appear at the
> + * start of the line.
> + */
> +void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
I'll make this "static" to this file while queuing. Also hoist
"struct kwtable ... keywords[]" to an earlier place in the function
to avoid decl-after-stmt error.
sideband.c | 22 +++---
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/sideband.c b/sideband.c
index e939cd4360..74b2fcaf64 100644
--- a/sideband.c
+++ b/sideband.c
@@ -9,10 +9,20 @@
* Optionally highlight some keywords in remote output if they appear at the
* start of the line.
*/
-void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
+static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int
n)
{
static int sideband_use_color = -1;
int i;
+ struct kwtable {
+ const char *keyword;
+ const char *color;
+ } keywords[] = {
+ {"hint", GIT_COLOR_YELLOW},
+ {"warning", GIT_COLOR_BOLD_YELLOW},
+ {"success", GIT_COLOR_BOLD_GREEN},
+ {"error", GIT_COLOR_BOLD_RED},
+ };
+
if (sideband_use_color < 0) {
const char *key = "color.remote";
char *value = NULL;
@@ -25,16 +35,6 @@ void maybe_colorize_sideband(struct strbuf *dest, const char
*src, int n)
return;
}
- struct kwtable {
- const char *keyword;
- const char *color;
- } keywords[] = {
- {"hint", GIT_COLOR_YELLOW},
- {"warning", GIT_COLOR_BOLD_YELLOW},
- {"success", GIT_COLOR_BOLD_GREEN},
- {"error", GIT_COLOR_BOLD_RED},
- };
-
while (isspace(*src)) {
strbuf_addch(dest, *src);
src++;
Re: [PATCH v2 01/10] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells
On Mon, Jul 30, 2018 at 4:59 PM Jonathan Nieder wrote: > Eric Sunshine wrote: > > The subshell linter would normally fold out the here-doc content, but > > 'sed' isn't a proper programming language, so the linter can't > > recognize arbitrary here-doc tags. Instead it has hard-coded knowledge > > of the tags commonly used in the Git tests, specifically EOF, EOT, and > > INPUT_END. > > Oh, hmm. I also see some others (outside subshells, though): > > EXPECT_END > [...] Correct. The linter does fold-out top-level EOF here-docs to hedge against the body of a here-doc containing something that might look like the start of a subshell (which would activate the more strict/expensive &&-chain validation). It special-cases top-level EOF because it's such a common tag name, thus an easy way to avoid false-positives, in general. I didn't bother trying to recognize _every_ possible tag since those other here-docs don't trigger any problems. (It's heuristic-based, after all.) > I wonder if it should look for something like [A-Z][A-Z_]* to catch > all of these. I considered that, but it doesn't handle nested here-docs, which we actually have in the test suite. For instance, from t9300-fast-import: cat >input <<-INPUT_END && mark :2 data < > The linter also deals with multi-line $(...) expressions, however, it > > currently only recognizes them when the $( is on its own line. > > That's reasonable, especially if "on its own line" means "at end of > line". It does; sorry for not being clear. > What would help most is if the error message could explain what is > going on, but I understand that that can be hard to do in a sed > script. Right, unfortunately, it can't be too helpful, but, when fixing all those broken chains, I did find it useful to dump the result after 'sed' processing in order to identify what it was actually complaining about. I did so by changing the $1 in this line from test-lib.sh: error "bug in the test script: broken &&-chain or run-away HERE-DOC: $1" to print the 'sed' output instead. The linter prepends "??AMP??" and "??SEMI??" to suspect lines. It also prepends lines with ">" to indicate where it thinks a subshell ends. This information was quite helpful when figuring out what was broken in the test (or, for false positives, where a heuristic had gone wrong). I considered enabling this output by default instead of $1 but decided against it since it is only helpful for broken &&-chains in subshells, thus doesn't aid in the more common case of top-level &&-breakage, thus might be confusing. > > I could try to update the linter to not trip over this sort of input, > > however, this test code is indeed ugly and difficult to understand, > > and your rewrite[1] of it makes it far easier to grok, so I'm not sure > > the effort would be worthwhile. What do you think? > > I'd be happy to look over a change that handles more here-doc > delimiters or produces a clearer message for tests in poor style, but > I agree with you that it's not too important. I am, for a couple reasons, somewhat hesitant to tweak the heuristic. First, each tweak has the potential of causing more false-positives or (perhaps worse) false-negatives. The linter's own test-suite is supposed to protect against that, but test suite coverage is never perfect. Second, ideally, the linter should protect against new broken &&-chains from entering the codebase, so poorly coded historic tests such as these aren't necessarily good motivation for tweaking, _and_ it is (hopefully) unlikely that we would allow this sort of ugly shell code to enter the codebase going forward. (The counterargument is that this false-positive doesn't help someone coding up a new test who hasn't yet submitted the patch to the mailing list where more seasoned eyes would suggest better coding style.)
Re: [PATCH v4 05/21] range-diff: also show the diff between patches
On 07/30, Johannes Schindelin wrote:
> 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?
Good point. I can't really come up with a good option right now
either. It's not too bad, as users just typed the command, so it
should be easy enough to see from the previous line what went wrong.
One potential option may be to turn "die(_("single arg format requires
a symmetric range"));" into an 'error()', and show the usage? I think
that may be nice anyway, as "symmetric range" may not be immediately
obvious to everyone, but together with the usage it may be clearer?
> > > > > 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
On 07/30, Johannes Schindelin wrote:
> Hi Thomas,
>
> On Sun, 29 Jul 2018, Thomas Gummerer wrote:
>
> > On 07/21, Johannes Schindelin via GitGitGadget wrote:
> > >
> > > [...]
> > >
> > > +static void find_exact_matches(struct string_list *a, struct string_list
> > > *b)
> > > +{
> > > + struct hashmap map;
> > > + int i;
> > > +
> > > + hashmap_init(, (hashmap_cmp_fn)patch_util_cmp, NULL, 0);
> > > +
> > > + /* First, add the patches of a to a hash map */
> > > + for (i = 0; i < a->nr; i++) {
> > > + struct patch_util *util = a->items[i].util;
> > > +
> > > + util->i = i;
> > > + util->patch = a->items[i].string;
> > > + util->diff = util->patch + util->diff_offset;
> > > + hashmap_entry_init(util, strhash(util->diff));
> > > + hashmap_add(, util);
> > > + }
> > > +
> > > + /* Now try to find exact matches in b */
> > > + for (i = 0; i < b->nr; i++) {
> > > + struct patch_util *util = b->items[i].util, *other;
> > > +
> > > + util->i = i;
> > > + util->patch = b->items[i].string;
> > > + util->diff = util->patch + util->diff_offset;
> > > + hashmap_entry_init(util, strhash(util->diff));
> > > + other = hashmap_remove(, util, NULL);
> > > + if (other) {
> > > + if (other->matching >= 0)
> > > + BUG("already assigned!");
> > > +
> > > + other->matching = i;
> > > + util->matching = other->i;
> > > + }
> > > + }
> >
> > One possibly interesting corner case here is what happens when there
> > are two patches that have the exact same diff, for example in the
> > pathological case of commit A doing something, commit B reverting
> > commit A, and then commit C reverting commit B, so it ends up with the
> > same diff.
> >
> > Having those same commits unchanged in both ranges (e.g. if a commit
> > earlier in the range has been changed, and range B has been rebased on
> > top of that), we'd get the following mapping from range A to range B
> > for the commits in question:
> >
> > A -> C
> > B -> B
> > C -> A
> >
> > Which is not quite what I would expect as the user (even though it is
> > a valid mapping, and it probably doesn't matter too much for the end
> > result of the range diff, as nothing has changed between the commits
> > anyway). So I'm not sure it's worth fixing this, as it is a
> > pathological case, and nothing really breaks.
>
> Indeed. As far as I am concerned, this falls squarely into the "let's
> cross that bridge when, or if, we reach it" category.
Makes sense, this can definitely be addressed later.
> > > +
> > > + hashmap_free(, 0);
> > > +}
> > > +
> > > +static void diffsize_consume(void *data, char *line, unsigned long len)
> > > +{
> > > + (*(int *)data)++;
> > > +}
> > > +
> > > +static int diffsize(const char *a, const char *b)
> > > +{
> > > + xpparam_t pp = { 0 };
> > > + xdemitconf_t cfg = { 0 };
> > > + mmfile_t mf1, mf2;
> > > + int count = 0;
> > > +
> > > + mf1.ptr = (char *)a;
> > > + mf1.size = strlen(a);
> > > + mf2.ptr = (char *)b;
> > > + mf2.size = strlen(b);
> > > +
> > > + cfg.ctxlen = 3;
> > > + if (!xdi_diff_outf(, , diffsize_consume, , , ))
> > > + return count;
> > > +
> > > + error(_("failed to generate diff"));
> > > + return COST_MAX;
> > > +}
> > > +
> > > +static void get_correspondences(struct string_list *a, struct
> > > string_list *b,
> > > + int creation_factor)
> > > +{
> > > + int n = a->nr + b->nr;
> > > + int *cost, c, *a2b, *b2a;
> > > + int i, j;
> > > +
> > > + ALLOC_ARRAY(cost, st_mult(n, n));
> > > + ALLOC_ARRAY(a2b, n);
> > > + ALLOC_ARRAY(b2a, n);
> > > +
> > > + for (i = 0; i < a->nr; i++) {
> > > + struct patch_util *a_util = a->items[i].util;
> > > +
> > > + for (j = 0; j < b->nr; j++) {
> > > + struct patch_util *b_util = b->items[j].util;
> > > +
> > > + if (a_util->matching == j)
> > > + c = 0;
> > > + else if (a_util->matching < 0 && b_util->matching < 0)
> > > + c = diffsize(a_util->diff, b_util->diff);
> > > + else
> > > + c = COST_MAX;
> > > + cost[i + n * j] = c;
> > > + }
> > > +
> > > + c = a_util->matching < 0 ?
> > > + a_util->diffsize * creation_factor / 100 : COST_MAX;
> > > + for (j = b->nr; j < n; j++)
> > > + cost[i + n * j] = c;
> > > + }
> > > +
> > > + for (j = 0; j < b->nr; j++) {
> > > + struct patch_util *util = b->items[j].util;
> > > +
> > > + c = util->matching < 0 ?
> > > + util->diffsize * creation_factor / 100 : COST_MAX;
> > > + for (i = a->nr; i < n; i++)
> > > + cost[i + n * j] = c;
> > > + }
> > > +
> > > + for (i = a->nr; i < n; i++)
> > > + for (j = b->nr; j < n; j++)
> > > + cost[i + n
Re: [PATCH] mw-to-git/t9360: fix broken &&-chain
(+cc: some folks interested in git-remote-mediawiki) Hi, Eric Sunshine wrote: > Signed-off-by: Eric Sunshine > --- > Jonathan's discovery[1] of a broken &&-chain in a contrib/subtree test > prompted me to check other tests bundled in contrib/. This was the > only other problem found. Unlike the subtree &&-chain case, this > breakage is at the top-level, thus was caught by the normal > --chain-lint mechanism, not the subshell linter. > > [1]: > https://public-inbox.org/git/20180730190612.gb156...@aiede.svl.corp.google. > com/ A small detail from there would be useful to have in the commit message: namely that this was detected using --chain-lint (and what command I can run to reproduce it myself). With or without such a tweak to the commit message, Reviewed-by: Jonathan Nieder Thanks. > contrib/mw-to-git/t/t9360-mw-to-git-clone.sh | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/contrib/mw-to-git/t/t9360-mw-to-git-clone.sh > b/contrib/mw-to-git/t/t9360-mw-to-git-clone.sh > index 22f069db48..cfbfe7ddf6 100755 > --- a/contrib/mw-to-git/t/t9360-mw-to-git-clone.sh > +++ b/contrib/mw-to-git/t/t9360-mw-to-git-clone.sh > @@ -247,7 +247,7 @@ test_expect_success 'Test of resistance to modification > of category on wiki for > wiki_editpage Notconsidered "this page will not appear on local" false > && > wiki_editpage Othercategory "this page will not appear on local" false > -c=Cattwo && > wiki_editpage Tobeedited "this page have been modified" true -c=Catone > && > - wiki_delete_page Tobedeleted > + wiki_delete_page Tobedeleted && > git clone -c remote.origin.categories="Catone" \ > mediawiki::'"$WIKI_URL"' mw_dir_14 && > wiki_getallpage ref_page_14 Catone && > -- > 2.18.0.597.ga71716f1ad >
Re: [PATCH v2 0/4] Speed up unpack_trees()
On 7/29/2018 6:33 AM, Nguyễn Thái Ngọc Duy wrote: This series speeds up unpack_trees() a bit by using cache-tree. unpack-trees could bit split in three big parts - the actual tree unpacking and running n-way merging - update worktree, which could be expensive depending on how much I/O is involved - repair cache-tree This series focuses on the first part alone and could give 700% speedup (best case possible scenario, real life ones probably not that impressive). It also shows that the reparing cache-tree is kinda expensive. I have an idea of reusing cache-tree from the original index, but I'll leave that to Ben or others to try out and see if it helps at all. v2 fixes the comments from Junio, adds more performance tracing and reduces the cost of adding index entries. Nguyễn Thái Ngọc Duy (4): unpack-trees.c: add performance tracing unpack-trees: optimize walking same trees with cache-tree unpack-trees: reduce malloc in cache-tree walk unpack-trees: cheaper index update when walking by cache-tree cache-tree.c | 2 + cache.h| 1 + read-cache.c | 3 +- unpack-trees.c | 161 - unpack-trees.h | 1 + 5 files changed, 166 insertions(+), 2 deletions(-) I have a limited understanding of this code path so I'm not the best person to review this but I didn't see any issues that concerned me. I also was able to run our internal functional and performance tests in addition to the git tests and the results were positive. Ben
Re: [PATCH 1/1] Add the `p4-pre-submit` hook
Luke Diamand writes: > Possibly it should say "it takes no parameters" rather than "it takes > no parameter"; it is confusing that in English, zero takes the plural > rather than the singular. There's a PhD in linguistics there for > someone! I count three instances. Will squash in the following while queuing with your Reviewed-by. Thanks. Documentation/git-p4.txt | 2 +- Documentation/githooks.txt | 2 +- git-p4.py | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt index a7aac1b920..41780a5aa9 100644 --- a/Documentation/git-p4.txt +++ b/Documentation/git-p4.txt @@ -377,7 +377,7 @@ These options can be used to modify 'git p4 submit' behavior. Hook for submit ~~~ The `p4-pre-submit` hook is executed if it exists and is executable. -The hook takes no parameter and nothing from standard input. Exiting with +The hook takes no parameters and nothing from standard input. Exiting with non-zero status from this script prevents `git-p4 submit` from launching. One usage scenario is to run unit tests in the hook. diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt index 22fcabbe21..959044347e 100644 --- a/Documentation/githooks.txt +++ b/Documentation/githooks.txt @@ -488,7 +488,7 @@ all files and folders. p4-pre-submit ~ -This hook is invoked by `git-p4 submit`. It takes no parameter and nothing +This hook is invoked by `git-p4 submit`. It takes no parameters and nothing from standard input. Exiting with non-zero status from this script prevent `git-p4 submit` from launching. Run `git-p4 submit --help` for details. diff --git a/git-p4.py b/git-p4.py index 879abfd2b1..7fab255584 100755 --- a/git-p4.py +++ b/git-p4.py @@ -1496,7 +1496,7 @@ def __init__(self): ] self.description = """Submit changes from git to the perforce depot.\n The `p4-pre-submit` hook is executed if it exists and is executable. -The hook takes no parameter and nothing from standard input. Exiting with +The hook takes no parameters and nothing from standard input. Exiting with non-zero status from this script prevents `git-p4 submit` from launching. One usage scenario is to run unit tests in the hook."""
Re: [PATCH v2 01/10] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells
Eric Sunshine wrote: > On Mon, Jul 30, 2018 at 2:14 PM Jonathan Nieder wrote: >> ( >> chks_sub=$(cat <> $chks >> TXT >> ) && >> >> Ugly quoting, useless use of "cat", etc, aside, I don't think it's >> missing any &&. Hints? > > Yes, it's a false positive. > > The subshell linter would normally fold out the here-doc content, but > 'sed' isn't a proper programming language, so the linter can't > recognize arbitrary here-doc tags. Instead it has hard-coded knowledge > of the tags commonly used in the Git tests, specifically EOF, EOT, and > INPUT_END. Oh, hmm. I also see some others (outside subshells, though): EXPECT_END FRONTEND_END END_PART1 SETUP_END EOF2 EXPECTED END_OF_LOG INPUT_END END_EXPECT I wonder if it should look for something like [A-Z][A-Z_]* to catch all of these. > The linter also deals with multi-line $(...) expressions, however, it > currently only recognizes them when the $( is on its own line. That's reasonable, especially if "on its own line" means "at end of line". What would help most is if the error message could explain what is going on, but I understand that that can be hard to do in a sed script. [...] > I could try to update the linter to not trip over this sort of input, > however, this test code is indeed ugly and difficult to understand, > and your rewrite[1] of it makes it far easier to grok, so I'm not sure > the effort would be worthwhile. What do you think? I'd be happy to look over a change that handles more here-doc delimiters or produces a clearer message for tests in poor style, but I agree with you that it's not too important. Thanks for looking it over. Sincerely, Jonathan > [1]: > https://public-inbox.org/git/20180730190738.gd156...@aiede.svl.corp.google.com/
Re: [PATCH v2 3/4] unpack-trees: reduce malloc in cache-tree walk
On 7/29/2018 6:33 AM, Nguyễn Thái Ngọc Duy wrote:
This is a micro optimization that probably only shines on repos with
deep directory structure. Instead of allocating and freeing a new
cache_entry in every iteration, we reuse the last one and only update
the parts that are new each iteration.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
unpack-trees.c | 29 -
1 file changed, 20 insertions(+), 9 deletions(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index 39566b28fb..c33ebaf001 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -694,6 +694,8 @@ static int traverse_by_cache_tree(int pos, int nr_entries,
int nr_names,
{
struct cache_entry *src[MAX_UNPACK_TREES + 1] = { NULL, };
struct unpack_trees_options *o = info->data;
+ struct cache_entry *tree_ce = NULL;
+ int ce_len = 0;
int i, d;
if (!o->merge)
@@ -708,30 +710,39 @@ static int traverse_by_cache_tree(int pos, int
nr_entries, int nr_names,
* in the first place.
*/
for (i = 0; i < nr_entries; i++) {
- struct cache_entry *tree_ce;
- int len, rc;
+ int new_ce_len, len, rc;
src[0] = o->src_index->cache[pos + i];
len = ce_namelen(src[0]);
- tree_ce = xcalloc(1, cache_entry_size(len));
+ new_ce_len = cache_entry_size(len);
+
+ if (new_ce_len > ce_len) {
+ new_ce_len <<= 1;
+ tree_ce = xrealloc(tree_ce, new_ce_len);
+ memset(tree_ce, 0, new_ce_len);
+ ce_len = new_ce_len;
+
+ tree_ce->ce_flags = create_ce_flags(0);
+
+ for (d = 1; d <= nr_names; d++)
+ src[d] = tree_ce;
+ }
Nice optimization - especially when there are a lot of cache entries and
large trees.
tree_ce->ce_mode = src[0]->ce_mode;
- tree_ce->ce_flags = create_ce_flags(0);
tree_ce->ce_namelen = len;
oidcpy(_ce->oid, [0]->oid);
memcpy(tree_ce->name, src[0]->name, len + 1);
- for (d = 1; d <= nr_names; d++)
- src[d] = tree_ce;
-
rc = call_unpack_fn((const struct cache_entry * const *)src, o);
- free(tree_ce);
- if (rc < 0)
+ if (rc < 0) {
+ free(tree_ce);
return rc;
+ }
mark_ce_used(src[0], o);
}
+ free(tree_ce);
if (o->debug_unpack)
printf("Unpacked %d entries from %s to %s using cache-tree\n",
nr_entries,
Re: [PATCH v2 2/4] unpack-trees: optimize walking same trees with cache-tree
On 7/29/2018 6:33 AM, Nguyễn Thái Ngọc Duy wrote:
From: Duy Nguyen
In order to merge one or many trees with the index, unpack-trees code
walks multiple trees in parallel with the index and performs n-way
merge. If we find out at start of a directory that all trees are the
same (by comparing OID) and cache-tree happens to be available for
that directory as well, we could avoid walking the trees because we
already know what these trees contain: it's flattened in what's called
"the index".
The upside is of course a lot less I/O since we can potentially skip
lots of trees (think subtrees). We also save CPU because we don't have
to inflate and the apply deltas. The downside is of course more
fragile code since the logic in some functions are now duplicated
elsewhere.
"checkout -" with this patch on gcc.git:
baseline new
0.018239226 0.019365414 s: read cache .git/index
0.052541655 0.049605548 s: preload index
0.001537598 0.001571695 s: refresh index
0.168167768 0.049677212 s: unpack trees
0.002897186 0.002845256 s: update worktree after a merge
0.131661745 0.136597522 s: repair cache-tree
0.075389117 0.075422517 s: write index, changed mask = 2a
0.111702023 0.032813253 s: unpack trees
0.23245 0.22002 s: update worktree after a merge
0.111793866 0.032933140 s: diff-index
0.587933288 0.398924370 s: git command: /home/pclouds/w/git/git
This command calls unpack_trees() twice, the first time on 2way merge
and the second 1way merge. In both times, "unpack trees" time is
reduced to one third. Overall time reduction is not that impressive of
course because index operations take a big chunk. And there's that
repair cache-tree line.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
unpack-trees.c | 119 -
1 file changed, 118 insertions(+), 1 deletion(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index dc58d1f5ae..39566b28fb 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -644,6 +644,102 @@ static inline int are_same_oid(struct name_entry *name_j,
struct name_entry *nam
return name_j->oid && name_k->oid && !oidcmp(name_j->oid, name_k->oid);
}
+static int all_trees_same_as_cache_tree(int n, unsigned long dirmask,
+ struct name_entry *names,
+ struct traverse_info *info)
+{
+ struct unpack_trees_options *o = info->data;
+ int i;
+
+ if (!o->merge || dirmask != ((1 << n) - 1))
+ return 0;
+
+ for (i = 1; i < n; i++)
+ if (!are_same_oid(names, names + i))
+ return 0;
+
+ return cache_tree_matches_traversal(o->src_index->cache_tree, names,
info);
+}
+
+static int index_pos_by_traverse_info(struct name_entry *names,
+ struct traverse_info *info)
+{
+ struct unpack_trees_options *o = info->data;
+ int len = traverse_path_len(info, names);
+ char *name = xmalloc(len + 1 /* slash */ + 1 /* NUL */);
+ int pos;
+
+ make_traverse_path(name, info, names);
+ name[len++] = '/';
+ name[len] = '\0';
+ pos = index_name_pos(o->src_index, name, len);
+ if (pos >= 0)
+ BUG("This is a directory and should not exist in index");
+ pos = -pos - 1;
+ if (!starts_with(o->src_index->cache[pos]->name, name) ||
+ (pos > 0 && starts_with(o->src_index->cache[pos-1]->name, name)))
+ BUG("pos must point at the first entry in this directory");
+ free(name);
+ return pos;
+}
+
+/*
+ * Fast path if we detect that all trees are the same as cache-tree at this
+ * path. We'll walk these trees recursively using cache-tree/index instead of
+ * ODB since already know what these trees contain.
+ */
+static int traverse_by_cache_tree(int pos, int nr_entries, int nr_names,
+ struct name_entry *names,
+ struct traverse_info *info)
+{
+ struct cache_entry *src[MAX_UNPACK_TREES + 1] = { NULL, };
+ struct unpack_trees_options *o = info->data;
+ int i, d;
+
+ if (!o->merge)
+ BUG("We need cache-tree to do this optimization");
+
+ /*
+* Do what unpack_callback() and unpack_nondirectories() normally
+* do. But we walk all paths recursively in just one loop instead.
+*
+* D/F conflicts and staged entries are not a concern because
+* cache-tree would be invalidated and we would never get here
+* in the first place.
+*/
+ for (i = 0; i < nr_entries; i++) {
+ struct cache_entry *tree_ce;
+ int len, rc;
+
+ src[0] = o->src_index->cache[pos + i];
+
+ len = ce_namelen(src[0]);
+ tree_ce = xcalloc(1,
Re: [PATCH v3 00/11] rerere: handle nested conflicts
On 07/30, Junio C Hamano wrote: > Thomas Gummerer writes: > > > Thomas Gummerer (11): > > rerere: unify error messages when read_cache fails > > rerere: lowercase error messages > > rerere: wrap paths in output in sq > > rerere: mark strings for translation > > rerere: add documentation for conflict normalization > > rerere: fix crash when conflict goes unresolved > > rerere: only return whether a path has conflicts or not > > rerere: factor out handle_conflict function > > rerere: return strbuf from handle path > > rerere: teach rerere to handle nested conflicts > > rerere: recalculate conflict ID when unresolved conflict is committed > > Even though I am not certain about the last two steps, everything > before them looked trivially correct and good changes (well, the > "strbuf" one's goodness obviously depends on the goodness of the > last two, which are helped by it). > > Sorry for taking so long before getting to the series. No worries, I realize you are busy with a lot of other things. Thanks a lot for your review!
Re: [PATCH v3 07/11] rerere: only return whether a path has conflicts or not
On 07/30, Junio C Hamano wrote: > Thomas Gummerer writes: > > > We currently return the exact number of conflict hunks a certain path > > has from the 'handle_paths' function. However all of its callers only > > care whether there are conflicts or not or if there is an error. > > Return only that information, and document that only that information > > is returned. This will simplify the code in the subsequent steps. > > > > Signed-off-by: Thomas Gummerer > > --- > > rerere.c | 23 --- > > 1 file changed, 12 insertions(+), 11 deletions(-) > > I do recall writing this code without knowing if the actual number > of conflicts would be useful by callers, but it is apparent that it > wasn't. I won't mind losing that bit of info at all. Besides, we > won't risk mistaking a file with 2 billion conflicts with a file > whose conflicts cannot be parsed ;-). Hah, I would love to see someone actually achieve that ;) > The patch looks good. Thanks.
[PATCH] mw-to-git/t9360: fix broken &&-chain
Signed-off-by: Eric Sunshine --- Jonathan's discovery[1] of a broken &&-chain in a contrib/subtree test prompted me to check other tests bundled in contrib/. This was the only other problem found. Unlike the subtree &&-chain case, this breakage is at the top-level, thus was caught by the normal --chain-lint mechanism, not the subshell linter. [1]: https://public-inbox.org/git/20180730190612.gb156...@aiede.svl.corp.google. com/ contrib/mw-to-git/t/t9360-mw-to-git-clone.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contrib/mw-to-git/t/t9360-mw-to-git-clone.sh b/contrib/mw-to-git/t/t9360-mw-to-git-clone.sh index 22f069db48..cfbfe7ddf6 100755 --- a/contrib/mw-to-git/t/t9360-mw-to-git-clone.sh +++ b/contrib/mw-to-git/t/t9360-mw-to-git-clone.sh @@ -247,7 +247,7 @@ test_expect_success 'Test of resistance to modification of category on wiki for wiki_editpage Notconsidered "this page will not appear on local" false && wiki_editpage Othercategory "this page will not appear on local" false -c=Cattwo && wiki_editpage Tobeedited "this page have been modified" true -c=Catone && - wiki_delete_page Tobedeleted + wiki_delete_page Tobedeleted && git clone -c remote.origin.categories="Catone" \ mediawiki::'"$WIKI_URL"' mw_dir_14 && wiki_getallpage ref_page_14 Catone && -- 2.18.0.597.ga71716f1ad
Re: [PATCH v3 06/11] rerere: fix crash when conflict goes unresolved
On 07/30, Junio C Hamano wrote:
> Thomas Gummerer writes:
>
> > Currently when a user doesn't resolve a conflict in a file, but
> > commits the file with the conflict markers, and later the file ends up
> > in a state in which rerere can't handle it, subsequent rerere
> > operations that are interested in that path, such as 'rerere clear' or
> > 'rerere forget ' will fail, or even worse in the case of 'rerere
> > clear' segfault.
> >
> > Such states include nested conflicts, or an extra conflict marker that
> > doesn't have any match.
> >
> > This is because the first 'git rerere' when there was only one
> > conflict in the file leaves an entry in the MERGE_RR file behind. The
>
> I find this sentence, especially the "only one conflict in the file"
> part, a bit unclear. What does the sentence count as one conflict?
> One block of lines enclosed inside "<<<"...">>>" pair? The command
> behaves differently when there are two such blocks instead?
Yeah as you mentioned below, conflict marker(s) that cannot be parsed
here would make more sense. Will adjust the commit message.
> > next 'git rerere' will then pick the rerere ID for that file up, and
> > not assign a new ID as it can't successfully calculate one. It will
> > however still try to do the rerere operation, because of the existing
> > ID. As the handle_file function fails, it will remove the 'preimage'
> > for the ID in the process, while leaving the ID in the MERGE_RR file.
> >
> > Now when 'rerere clear' for example is run, it will segfault in
> > 'has_rerere_resolution', because status is NULL.
>
> I think this "status" refers to the collection->status[]. How do we
> get into that state, though?
>
> new_rerere_id() and new_rerere_id_hex() fills id->collection by
> calling find_rerere_dir(), which either finds an existing rerere_dir
> instance or manufactures one with .status==NULL. The .status[]
> array is later grown by calling fit_variant as we scan and find the
> pre/post images, but because there is no pre/post image for a file
> with unparseable conflicts, it is left NULL.
>
> So another possible fix could be to make sure that .status[] is only
> read when .status_nr says there is something worth reading. I am
> not saying that would be a better fix---I am just thinking out loud
> to make sure I understand the issue correctly.
Yeah what you are writing above matches my understanding, and that
should fix the issue as well. I haven't actually tried what you're
proposing above, but I think I find it nicer to just remove the entry
we can't do anything with anyway.
> > To fix this, remove the rerere ID from the MERGE_RR file in the case
> > when we can't handle it, and remove the corresponding variant from
> > .git/rr-cache/. Removing it unconditionally is fine here, because if
> > the user would have resolved the conflict and ran rerere, the entry
> > would no longer be in the MERGE_RR file, so we wouldn't have this
> > problem in the first place, while if the conflict was not resolved,
> > the only thing that's left in the folder is the 'preimage', which by
> > itself will be regenerated by git if necessary, so the user won't
> > loose any work.
>
> s/loose/lose/
>
> > Note that other variants that have the same conflict ID will not be
> > touched.
>
> Nice. Thanks for a fix.
>
> >
> > Signed-off-by: Thomas Gummerer
> > ---
> > rerere.c | 12 +++-
> > t/t4200-rerere.sh | 22 ++
> > 2 files changed, 29 insertions(+), 5 deletions(-)
> >
> > diff --git a/rerere.c b/rerere.c
> > index da1ab54027..895ad80c0c 100644
> > --- a/rerere.c
> > +++ b/rerere.c
> > @@ -823,10 +823,7 @@ static int do_plain_rerere(struct string_list *rr, int
> > fd)
> > struct rerere_id *id;
> > unsigned char sha1[20];
> > const char *path = conflict.items[i].string;
> > - int ret;
> > -
> > - if (string_list_has_string(rr, path))
> > - continue;
> > + int ret, has_string;
> >
> > /*
> > * Ask handle_file() to scan and assign a
> > @@ -834,7 +831,12 @@ static int do_plain_rerere(struct string_list *rr, int
> > fd)
> > * yet.
> > */
> > ret = handle_file(path, sha1, NULL);
> > - if (ret < 1)
> > + has_string = string_list_has_string(rr, path);
> > + if (ret < 0 && has_string) {
> > + remove_variant(string_list_lookup(rr, path)->util);
> > + string_list_remove(rr, path, 1);
> > + }
> > + if (ret < 1 || has_string)
> > continue;
>
> We used to say "if we know about the path we do not do anything
> here, if we do not see any conflict in the file we do nothing,
> otherwise we assign a new id"; we now say "see if we can parse
> and also see if we have conflict(s); if we know about the path and
> we cannot parse, drop it from the rr database (because otherwise the
>
Re: [PATCH v2 01/10] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells
On Mon, Jul 30, 2018 at 2:14 PM Jonathan Nieder wrote: > Eric Sunshine wrote: > > Address this shortcoming by feeding the body of the test to a > > lightweight "linter" which can peer inside subshells and identify broken > > &&-chains by pure textual inspection. > > This is causing contrib/subtree tests to fail for me: running "make -C > contrib/subtree test" produces Thanks, I forgot that some of 'contrib' had bundled tests. (In fact, I just checked the other 'contrib' tests and found that a MediaWiki test has a broken top-level &&-chain.) > The problematic test code looks like this: > > ( > chks_sub=$(cat < $chks > TXT > ) && > > Ugly quoting, useless use of "cat", etc, aside, I don't think it's > missing any &&. Hints? Yes, it's a false positive. The subshell linter would normally fold out the here-doc content, but 'sed' isn't a proper programming language, so the linter can't recognize arbitrary here-doc tags. Instead it has hard-coded knowledge of the tags commonly used in the Git tests, specifically EOF, EOT, and INPUT_END. The linter also deals with multi-line $(...) expressions, however, it currently only recognizes them when the $( is on its own line. Had this test used one of the common here-doc tags _or_ had it formatted the $(...) as described, then it wouldn't have misfired. I could try to update the linter to not trip over this sort of input, however, this test code is indeed ugly and difficult to understand, and your rewrite[1] of it makes it far easier to grok, so I'm not sure the effort would be worthwhile. What do you think? [1]: https://public-inbox.org/git/20180730190738.gd156...@aiede.svl.corp.google.com/
Re: [PATCH v3 05/11] rerere: add documentation for conflict normalization
On 07/30, Junio C Hamano wrote:
> Thomas Gummerer writes:
>
> > +Different conflict styles and branch names are normalized by stripping
> > +the labels from the conflict markers, and removing extraneous
> > +information from the `diff3` conflict style. Branches that are merged
>
> s/extraneous information/commmon ancestor version/ perhaps, to be
> fact-based without passing value judgment?
Yeah I meant "extraneous information for rerere", but common ancester
version is better.
> We drop the common ancestor version only because we cannot normalize
> from `merge` style to `diff3` style by adding one, and not because
> it is extraneous. It does help humans understand the conflict a lot
> better to have that section.
>
> > +By extension, this means that rerere should recognize that the above
> > +conflicts are the same. To do this, the labels on the conflict
> > +markers are stripped, and the diff3 output is removed. The above
>
> s/diff3 output/common ancestor version/, as "diff3 output" would
> mean the whole thing between <<< and >>> to readers.
Makes sense, will fix in the re-roll, thanks!
> > diff --git a/rerere.c b/rerere.c
> > index be98c0afcb..da1ab54027 100644
> > --- a/rerere.c
> > +++ b/rerere.c
> > @@ -394,10 +394,6 @@ static int is_cmarker(char *buf, int marker_char, int
> > marker_size)
> > * and NUL concatenated together.
> > *
> > * Return the number of conflict hunks found.
> > - *
> > - * NEEDSWORK: the logic and theory of operation behind this conflict
> > - * normalization may deserve to be documented somewhere, perhaps in
> > - * Documentation/technical/rerere.txt.
> > */
> > static int handle_path(unsigned char *sha1, struct rerere_io *io, int
> > marker_size)
> > {
>
> Thanks for finally removing this age-old NEEDSWORK comment.
Re: [PATCH v3 10/11] rerere: teach rerere to handle nested conflicts
On 07/30, Junio C Hamano wrote:
> Thomas Gummerer writes:
>
> > Currently rerere can't handle nested conflicts and will error out when
> > it encounters such conflicts. Do that by recursively calling the
> > 'handle_conflict' function to normalize the conflict.
> >
> > The conflict ID calculation here deserves some explanation:
> >
> > As we are using the same handle_conflict function, the nested conflict
> > is normalized the same way as for non-nested conflicts, which means
> > the ancestor in the diff3 case is stripped out, and the parts of the
> > conflict are ordered alphabetically.
> >
> > The conflict ID is however is only calculated in the top level
> > handle_conflict call, so it will include the markers that 'rerere'
> > adds to the output. e.g. say there's the following conflict:
> >
> > <<< HEAD
> > 1
> > ===
> > <<< HEAD
> > 3
> > ===
> > 2
> > >>> branch-2
> > >>> branch-3~
>
> Hmph, I vaguely recall that I made inner merges to use the conflict
> markers automatically lengthened (by two, if I recall correctly)
> than its immediate outer merge. Wouldn't the above look more like
>
> <<< HEAD
> 1
> ===
> < HEAD
> 3
> =
> 2
> > branch-2
> >>> branch-3~
>
> Perhaps I am not recalling it correctly.
The only way I could reproduce this is by not resolving a conflict
(just leaving the conflict markers in place, but running 'git add
conflicted'), and then merging something else, which produces another
conflict, where one of the sides was the one with conflict markers
already in the file, same as what I did in the test.
So in that case, the conflict markers of the already existing conflict
would just be treated as normal text during the merge I believe, and
thus the new conflict markers would be the same length.
The usage of git is really a bit wrong here, so I don't know if it's
actually worth helping the users at this point. But trying to
understand how rerere exactly works, I had this written up already, so
I thought I would include it in this series anyway in case it helps
somebody :)
> > it would be recorde as follows in the preimage:
> >
> > <<<
> > 1
> > ===
> > <<<
> > 2
> > ===
> > 3
> > >>>
> > >>>
> >
> > and the conflict ID would be calculated as
> >
> > sha1(1<<<
> > 2
> > ===
> > 3
> > >>>)
> >
> > Stripping out vs. leaving the conflict markers in place in the inner
> > conflict should have no practical impact, but it simplifies the
> > implementation.
> >
> > Signed-off-by: Thomas Gummerer
> > ---
> > Documentation/technical/rerere.txt | 42 ++
> > rerere.c | 10 +--
> > t/t4200-rerere.sh | 37 ++
> > 3 files changed, 87 insertions(+), 2 deletions(-)
> >
> > [..snip..]
> >
> > diff --git a/rerere.c b/rerere.c
> > index a35b88916c..f78bef80b1 100644
> > --- a/rerere.c
> > +++ b/rerere.c
> > @@ -365,12 +365,18 @@ static int handle_conflict(struct strbuf *out, struct
> > rerere_io *io,
> > RR_SIDE_1 = 0, RR_SIDE_2, RR_ORIGINAL
> > } hunk = RR_SIDE_1;
> > struct strbuf one = STRBUF_INIT, two = STRBUF_INIT;
> > - struct strbuf buf = STRBUF_INIT;
> > + struct strbuf buf = STRBUF_INIT, conflict = STRBUF_INIT;
> > int has_conflicts = -1;
> >
> > while (!io->getline(, io)) {
> > if (is_cmarker(buf.buf, '<', marker_size)) {
> > - break;
> > + if (handle_conflict(, io, marker_size, NULL) <
> > 0)
> > + break;
> > + if (hunk == RR_SIDE_1)
> > + strbuf_addbuf(, );
> > + else
> > + strbuf_addbuf(, );
>
> Hmph, do we ever see the inner conflict block while we are skipping
> and ignoring the common ancestor version, or it is impossible that
> we see '<' only while processing either our or their side?
As mentioned above, I haven't been able to reproduce creating an inner
conflict block outside of the case mentioned above, where the user
committed conflict markers, and then did another merge.
I don't think it can appear outside of that case in "normal"
operation.
> > + strbuf_release();
> > } else if (is_cmarker(buf.buf, '|', marker_size)) {
> > if (hunk != RR_SIDE_1)
> > break;
> > diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh
> > index 34f0518a5e..d63fe2b33b 100755
> > --- a/t/t4200-rerere.sh
> > +++ b/t/t4200-rerere.sh
> > @@ -602,4 +602,41 @@ test_expect_success 'rerere with unexpected conflict
> > markers does not crash' '
> > git rerere clear
> > '
> >
> > +test_expect_success 'rerere with inner conflict markers' '
> > + git reset --hard &&
> > +
> > + git
Re: [PATCH v4 11/21] range-diff: add tests
Johannes Schindelin writes: > 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. Should I take "pushed to ... GGG" to mean "do not merge what you have to 'next' yet, as there will be an updated series (not incremental) being prepared"?
Re: [PATCH v2 1/4] unpack-trees.c: add performance tracing
On 7/29/2018 6:33 AM, Nguyễn Thái Ngọc Duy wrote:
We're going to optimize unpack_trees() a bit in the following
patches. Let's add some tracing to measure how long it takes before
and after. This is the baseline ("git checkout -" on gcc.git, 80k
files on worktree)
0.018239226 s: read cache .git/index
0.052541655 s: preload index
0.001537598 s: refresh index
0.168167768 s: unpack trees
0.002897186 s: update worktree after a merge
0.131661745 s: repair cache-tree
0.075389117 s: write index, changed mask = 2a
0.111702023 s: unpack trees
0.23245 s: update worktree after a merge
0.111793866 s: diff-index
0.587933288 s: git command: /home/pclouds/w/git/git checkout -
Signed-off-by: Nguyễn Thái Ngọc Duy
I've reviewed this patch and it looks good to me. Nice to see the
additional breakdown on where time is being spent.
RE: Hash algorithm analysis
Hello all. Johannes, thanks for adding me to this discussion. So, as one of the coauthors of the SHA-1 collision detection code, I just wanted to chime in and say I'm glad to see the move to a longer hash function. Though, as a cryptographer, I have a few thoughts on the matter that I thought I would share. I think that moving to SHA256 is a fine change, and I support it. I'm not anywhere near the expert in this that Joan Daeman is. I am someone who has worked in this space more or less peripherally. However, I agree with Adam Langley that basically all of the finalists for a hash function replacement are about the same for the security needs of Git. I think that, for this community, other software engineering considerations should be more important to the selection process. I think Joan's survey of cryptanalysis papers and the numbers that he gives are interesting, and I had never seen the comparison laid out like that. So, I think that there is a good argument to be made that SHA3 has had more cryptanalysis than SHA2. Though, Joan, are the papers that you surveyed only focused on SHA2? I'm curious if you think that the design/construction of SHA2, as it can be seen as an iteration of MD5/SHA1, means that the cryptanalysis papers on those constructions can be considered to apply to SHA2? Again, I'm not an expert in this, but I do know that Marc Steven's techniques for constructing collisions also provided some small cryptanalytic improvements against the SHA2 family as well. I also think that while the paper survey is a good way to look over all of this, the more time in the position of high profile visibility that SHA2 has can give us some confidence as well. Also something worth pointing out is that the connection SHA2 has to SHA1 means that if Marc Steven's cryptanalysis of MD5/SHA-1 were ever successfully applied to SHA2, the SHA1 collision detection approach could be applied there as well, thus providing a drop in replacement in that situation. That said, we don't know that there is not a similar way of addressing issues with the SHA3/Sponge design. It's just that because we haven't seen any weaknesses of this sort in similar designs, we just don't know what a similar approach would be there yet. I don't want to put too much stock in this argument, it's just saying "Well, we already know how SHA2 is likely to break, and we've had fixes for similar things in the past." This is pragmatic but not inspiring or confidence building. So, I also want to state my biases in favor of SHA2 as an employee of Microsoft. Microsoft, being a corporation headquartered in a America, with the US Gov't as a major customer definitely prefers to defer to the US Gov't NIST standardization process. And from that perspective SHA2 or SHA3 would be good choices. I, personally, think that the NIST process is the best we have. It is relatively transparent, and NIST employs a fair number of very competent cryptographers. Also, I am encouraged by the widespread international participation that the NIST competitions and selection processes attract. As such, and reflecting this bias, in the internal discussions that Johannes alluded to, SHA2 and SHA3 were the primary suggestions. There was a slight preference for SHA2 because SHA3 is not exposed through the windows cryptographic APIs (though Git does not use those, so this is a nonissue for this discussion.) I also wanted to thank Johannes for keeping the cryptographers that he discussed this with anonymous. After all, cryptographers are known for being private. And I wanted to say that Johannes did, in fact, accurately represent our internal discussions on the matter. I also wanted to comment on the discussion of the "internal state having the same size as the output." Linus referred to this several times. This is known as narrow-pipe vs wide-pipe in the hash function design literature. Linus is correct that wide-pipe designs are more in favor currently, and IIRC, all of the serious SHA3 candidates employed this. That said, it did seem that in the discussion this was being equated with "length extension attacks." And that connection is just not accurate. Length extension attacks are primarily a motivation of the HMAC liked nested hashing design for MACs, because of a potential forgery attack. Again, this doesn't really matter because the decision has been made despite this discussion. I just wanted to set the record straight about this, as to avoid doing the right thing for the wrong reason (T.S. Elliot's "greatest treason.") One other thing that I wanted to throw out there for the future is that in the crypto community there is currently a very large push to post quantum cryptography. Whether the threat of quantum computers is real or imagined this is a hot area of research, with a NIST competition to select post quantum asymmetric cryptographic algorithms. That is not directly of
Re: [PATCH 1/2] Document git config getter return value.
Eric Sunshine writes: > On Mon, Jul 30, 2018 at 8:26 AM Han-Wen Nienhuys wrote: >> --- >> config.h | 10 -- >> 1 file changed, 8 insertions(+), 2 deletions(-) > > Missing sign-off. Besides, the patch is corrupt in that it miscounts both preimage and postimage lines and claims it applies to a 11-line block even though there are only 10 lines there.
[PATCH 2/2] refs: switch for_each_replace_ref back to use a ref_store
This effectively reverts commit 0d296c57ae (refs: allow for_each_replace_ref
to handle arbitrary repositories, 2018-04-11) and 60ce76d3581 (refs: add
repository argument to for_each_replace_ref, 2018-04-11).
The repository argument is not any special from the ref-store's point
of life. If you need a repository (for e.g. lookup_commit or friends),
you'll have to pass it through the callback cookie, whether directly or
as part of a struct tailored to your purpose.
So let's go back to the clean API, just requiring a ref_store as an
argument.
Signed-off-by: Stefan Beller
---
builtin/replace.c | 2 +-
refs.c| 4 ++--
refs.h| 2 +-
replace-object.c | 2 +-
4 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/builtin/replace.c b/builtin/replace.c
index deabda21012..52dc371eafc 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -87,7 +87,7 @@ static int list_replace_refs(const char *pattern, const char
*format)
"valid formats are 'short', 'medium' and 'long'\n",
format);
- for_each_replace_ref(the_repository, show_reference, (void *));
+ for_each_replace_ref(show_reference, (void *));
return 0;
}
diff --git a/refs.c b/refs.c
index 08fb5a99148..2d713499125 100644
--- a/refs.c
+++ b/refs.c
@@ -1441,9 +1441,9 @@ int refs_for_each_fullref_in(struct ref_store *refs,
const char *prefix,
return do_for_each_ref(refs, prefix, fn, 0, flag, cb_data);
}
-int for_each_replace_ref(struct repository *r, each_ref_fn fn, void *cb_data)
+int for_each_replace_ref(each_ref_fn fn, void *cb_data)
{
- return do_for_each_ref(get_main_ref_store(r),
+ return do_for_each_ref(get_main_ref_store(the_repository),
git_replace_ref_base, fn,
strlen(git_replace_ref_base),
DO_FOR_EACH_INCLUDE_BROKEN, cb_data);
diff --git a/refs.h b/refs.h
index cc2fb4c68c0..48d5ffd2082 100644
--- a/refs.h
+++ b/refs.h
@@ -307,7 +307,7 @@ int for_each_fullref_in(const char *prefix, each_ref_fn fn,
void *cb_data,
int for_each_tag_ref(each_ref_fn fn, void *cb_data);
int for_each_branch_ref(each_ref_fn fn, void *cb_data);
int for_each_remote_ref(each_ref_fn fn, void *cb_data);
-int for_each_replace_ref(struct repository *r, each_ref_fn fn, void *cb_data);
+int for_each_replace_ref(each_ref_fn fn, void *cb_data);
int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data);
int for_each_glob_ref_in(each_ref_fn fn, const char *pattern,
const char *prefix, void *cb_data);
diff --git a/replace-object.c b/replace-object.c
index e99fcd1ff6e..ee3374ab59b 100644
--- a/replace-object.c
+++ b/replace-object.c
@@ -41,7 +41,7 @@ static void prepare_replace_object(struct repository *r)
xmalloc(sizeof(*r->objects->replace_map));
oidmap_init(r->objects->replace_map, 0);
- for_each_replace_ref(r, register_replace_ref, r);
+ for_each_replace_ref(register_replace_ref, r);
}
/* We allow "recursive" replacement. Only within reason, though */
--
2.18.0.132.g195c49a2227
Re: [PATCH 2/8] t3206: add color test for range-diff --dual-color
On Fri, Jul 27, 2018 at 11:28 PM Eric Sunshine wrote: > > On Fri, Jul 27, 2018 at 11:05 PM Stefan Beller wrote: > > The 'expect'ed outcome is taken by running the 'range-diff |decode'; > > it is not meant as guidance, rather as a documentation of the current > > situation. > > I'm not really sure what this is trying to say. It seems _too_ brief. > > Did you want a space after the vertical bar before "decode"? I am trying to say that this patch was generated by inserting a "true && test_pause" first and then inside that paused test, I just run source /t/test-lib-functions.sh git range-diff changed...changed-message --color --dual-color \ | test_decode_color and saved that as the expected outcome, I was prepared to massage it gently by s//Q/ but that was not needed, but I forgot the q_to_tab in place. By adding the test this way, I just want to state "I observed the functionality as produced in this patch". I do not want to endorse the coloring scheme or claim it is good (it is, but I also still have nits to pick). So I tried to briefly say that the test is essentially "autogenerated" by just observing output at that point in time. > > > Signed-off-by: Stefan Beller > > --- > > diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh > > +test_expect_success 'simple coloring' ' > > + q_to_tab >expect <<-EOF && > > Why 'q_to_tab'? I don't see any "q"'s in the body. > > I also don't see any variable interpolation in the body, so maybe you > want -\EOF instead? All good suggestions! Thanks for the review, I'll incorporate them. Thanks, Stefan
[PATCH 0/2] Cleanup refs API [WAS: Re: [PATCH 2/3] refs: introduce new API, wrap old API shallowly around new API]
> > I anticipate that we need to have a lot of back pointers to the repository > > in question, hence I think we should have the repository pointer promoted > > to not just a back pointer. > > I will probably need more time to study that commit and maybe the mail > archive for the history of this series. But if I remember correctly > some of these for_each_ api is quite a pain (perhaps it's the for_each > version of reflog?) and it's probably better to redesign it (again > talking without real understanding of the problem). I stepped back a bit and reconsidered the point made above, and I do not think that the repository argument is any special. If you need a repository (for e.g. lookup_commit or friends), you'll have to pass it through the callback cookie, whether directly or as part of a struct tailored to your purpose. Instead we should strive to make the refs API smaller and cleaner, omitting the repository argument at all, and instead should be focussing on a ref_store argument instead. This series applies on master; when we decide to go this direction we can drop origin/sb/refs-in-repo. Thanks, Stefan Derrick Stolee (1): replace-objects: use arbitrary repositories Stefan Beller (1): refs: switch for_each_replace_ref back to use a ref_store builtin/replace.c | 4 +--- refs.c| 4 ++-- refs.h| 2 +- replace-object.c | 5 +++-- 4 files changed, 7 insertions(+), 8 deletions(-) -- 2.18.0.132.g195c49a2227
[PATCH 1/2] replace-objects: use arbitrary repositories
From: Derrick Stolee
This is the smallest possible change that makes prepare_replace_objects
work properly with arbitrary repositories. By supplying the repository
as the cb_data, we do not need to modify any code in the ref iterator
logic. We will likely want to do a full replacement of the ref iterator
logic to provide a repository struct as a concrete parameter.
[sb: original commit message left as-is. I disagree with it.
We want to keep the ref store API clean and focussed on struct
ref_store. There is no need to treat a repository any special
for pass-through by the callback cookie. So instead let's just
pass the repository as a cb cookie and cleanup the API in follow
up patches]
Signed-off-by: Derrick Stolee
Signed-off-by: Stefan Beller
---
replace-object.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/replace-object.c b/replace-object.c
index 801b5c16789..e99fcd1ff6e 100644
--- a/replace-object.c
+++ b/replace-object.c
@@ -14,6 +14,7 @@ static int register_replace_ref(const char *refname,
const char *slash = strrchr(refname, '/');
const char *hash = slash ? slash + 1 : refname;
struct replace_object *repl_obj = xmalloc(sizeof(*repl_obj));
+ struct repository *r = (struct repository *)cb_data;
if (get_oid_hex(hash, _obj->original.oid)) {
free(repl_obj);
@@ -25,7 +26,7 @@ static int register_replace_ref(const char *refname,
oidcpy(_obj->replacement, oid);
/* Register new object */
- if (oidmap_put(the_repository->objects->replace_map, repl_obj))
+ if (oidmap_put(r->objects->replace_map, repl_obj))
die("duplicate replace ref: %s", refname);
return 0;
@@ -40,7 +41,7 @@ static void prepare_replace_object(struct repository *r)
xmalloc(sizeof(*r->objects->replace_map));
oidmap_init(r->objects->replace_map, 0);
- for_each_replace_ref(r, register_replace_ref, NULL);
+ for_each_replace_ref(r, register_replace_ref, r);
}
/* We allow "recursive" replacement. Only within reason, though */
--
2.18.0.132.g195c49a2227
Re: [PATCH 8/8] commit-graph: close_commit_graph before shallow walk
"Derrick Stolee via GitGitGadget" writes:
> From: Derrick Stolee
>
> Make close_commit_graph() work for arbitrary repositories.
The first line (above) does not match the rest of the commit message.
> Call close_commit_graph() when about to start a rev-list walk that
> includes shallow commits. This is necessary in code paths that "fake"
> shallow commits for the sake of fetch. Specifically, test 351 in
> t5500-fetch-pack.sh runs
>
> git fetch --shallow-exclude one origin
>
> with a file-based transfer. When the "remote" has a commit-graph, we do
> not prevent the commit-graph from being loaded, but then the commits are
> intended to be dynamically transferred into shallow commits during
> get_shallow_commits_by_rev_list(). By closing the commit-graph before
> this call, we prevent this interaction.
>
> Signed-off-by: Derrick Stolee
> ---
> commit-graph.c | 8
> commit-graph.h | 1 +
> upload-pack.c | 2 ++
> 3 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/commit-graph.c b/commit-graph.c
> index 233958e10..237d4e7d1 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -256,10 +256,10 @@ static int prepare_commit_graph(struct repository *r)
> return !!r->objects->commit_graph;
> }
>
> -static void close_commit_graph(void)
> +void close_commit_graph(struct repository *r)
> {
> - free_commit_graph(the_repository->objects->commit_graph);
> - the_repository->objects->commit_graph = NULL;
> + free_commit_graph(r->objects->commit_graph);
> + r->objects->commit_graph = NULL;
> }
>
> static int bsearch_graph(struct commit_graph *g, struct object_id *oid,
> uint32_t *pos)
> @@ -871,7 +871,7 @@ void write_commit_graph(const char *obj_dir,
> write_graph_chunk_data(f, GRAPH_OID_LEN, commits.list, commits.nr);
> write_graph_chunk_large_edges(f, commits.list, commits.nr);
>
> - close_commit_graph();
> + close_commit_graph(the_repository);
> finalize_hashfile(f, NULL, CSUM_HASH_IN_STREAM | CSUM_FSYNC);
> commit_lock_file();
>
> diff --git a/commit-graph.h b/commit-graph.h
> index 76e098934..13d736cdd 100644
> --- a/commit-graph.h
> +++ b/commit-graph.h
> @@ -59,6 +59,7 @@ void write_commit_graph(const char *obj_dir,
>
> int verify_commit_graph(struct repository *r, struct commit_graph *g);
>
> +void close_commit_graph(struct repository *);
> void free_commit_graph(struct commit_graph *);
>
> #endif
> diff --git a/upload-pack.c b/upload-pack.c
> index 4ca052d0b..52ad6c8e5 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -24,6 +24,7 @@
> #include "quote.h"
> #include "upload-pack.h"
> #include "serve.h"
> +#include "commit-graph.h"
>
> /* Remember to update object flag allocation in object.h */
> #define THEY_HAVE(1u << 11)
> @@ -739,6 +740,7 @@ static void deepen_by_rev_list(int ac, const char **av,
> {
> struct commit_list *result;
>
> + close_commit_graph(the_repository);
> result = get_shallow_commits_by_rev_list(ac, av, SHALLOW, NOT_SHALLOW);
> send_shallow(result);
> free_commit_list(result);
Re: [PATCH 0/2] fix "rebase -i --root" corrupting root commit
On Mon, Jul 30, 2018 at 11:47 AM Johannes Schindelin wrote: > On Mon, 30 Jul 2018, Eric Sunshine wrote: > > 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. I don't think it was an oversight either, but it is nevertheless incorrect to use the "@" in the commit object's "author" header. As I understand it, you do "need" the "@" to distinguish a Unix epoch value assigned to GIT_AUTHOR_DATE, but the commit object format is very exacting in the datestamp format it accepts, and it does not allow "@". So, the date from GIT_AUTHOR_DATE needs to be converted to a format acceptable to the commit object, otherwise the commit is considered corrupt.
Re: [PATCH 0/2] fix "rebase -i --root" corrupting root commit
On Mon, Jul 30, 2018 at 8:14 AM Phillip Wood wrote: > On 30/07/18 10:29, Eric Sunshine wrote: > > 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 I don't see a branch with that name there. There are a couple "wip" branches, however, named wip/fix-t3403-author-script-test and wip/fix-t3404-author-script-test. I'm guessing you wanted me to look at the former. > 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. It appears that your patches are fixing issues and a test outside the issues fixed by my series (aside from the one line inserting the missing closing quote). As such, I think your patches can be built atop this series without worrying about conflicts. That would allow this commit-corruption-bug-fixing series to land without being tied to those "wip" patches which address lower-priority problems. > 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. Agreed. That seems a reasonable long-term goal but needn't hold up this series which addresses very real bugs leading to object corruption.
[PATCH 2/2] subtree test: simplify preparation of expected results
This mixture of quoting, pipes, and here-docs to produce expected results in shell variables is difficult to follow. Simplify by using simpler constructs that write output to files instead. Noticed because without this patch, t/chainlint is not able to understand the script in order to validate that its subshells use an unbroken &&-chain, causing "make -C contrib/subtree test" to fail with error: bug in the test script: broken &&-chain or run-away HERE-DOC: in t7900.21. Signed-off-by: Jonathan Nieder --- That's the end of the series. Thanks for reading. Thanks, Jonathan contrib/subtree/t/t7900-subtree.sh | 119 - 1 file changed, 30 insertions(+), 89 deletions(-) diff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh index e6a28f2c3e..57ff4b25c1 100755 --- a/contrib/subtree/t/t7900-subtree.sh +++ b/contrib/subtree/t/t7900-subtree.sh @@ -540,26 +540,10 @@ test_expect_success 'make sure exactly the right set of files ends up in the sub git fetch .. subproj-br && git merge FETCH_HEAD && - chks="sub1 -sub2 -sub3 -sub4" && - chks_sub=$(catactual && + test_cmp expect actual ) ' @@ -606,25 +590,11 @@ test_expect_success 'make sure the subproj *only* contains commits that affect t git fetch .. subproj-br && git merge FETCH_HEAD && - chks="sub1 -sub2 -sub3 -sub4" && - chks_sub=$(cat log && + sort actual && + test_cmp expect actual ) ' @@ -675,29 +645,16 @@ test_expect_success 'make sure exactly the right set of files ends up in the mai cd "$subtree_test_count" && git subtree pull --prefix="sub dir" ./"sub proj" master && - chkm="main1 -main2" && - chks="sub1 -sub2 -sub3 -sub4" && - chks_sub=$(cat chkms && + sed "s,^,sub dir/," chkms >chkms_sub && + test_write_lines sub1 sub2 sub3 sub4 >chks && + sed "s,^,sub dir/," chks >chks_sub && + + cat chkm chkms_sub chks_sub >expect && + git ls-files >actual && + test_cmp expect actual + ) ' next_test @@ -748,37 +705,21 @@ test_expect_success 'make sure each filename changed exactly once in the entire cd "$subtree_test_count" && git subtree pull --prefix="sub dir" ./"sub proj" master && - chkm="main1 -main2" && - chks="sub1 -sub2 -sub3 -sub4" && - chks_sub=$(cat chks && + test_write_lines main-sub1 main-sub2 main-sub3 main-sub4 >chkms && + sed "s,^,sub dir/," chkms >chkms_sub && # main-sub?? and /"sub dir"/main-sub?? both change, because those are the # changes that were split into their own history. And "sub dir"/sub?? never # change, since they were *only* changed in the subtree branch. - allchanges=$(git log --name-only --pretty=format:"" | sort | sed "/^$/d") && - expected=''"$(cat actual && + + cat chkms chkm chks chkms_sub >expect-unsorted && + sort expect-unsorted >expect && + test_cmp expect actual ) ' -- 2.18.0.345.g5c9ce644c3
[PATCH 1/2] subtree test: add missing && to &&-chain
Detected using t/chainlint. Signed-off-by: Jonathan Nieder --- contrib/subtree/t/t7900-subtree.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh index d05c613c97..e6a28f2c3e 100755 --- a/contrib/subtree/t/t7900-subtree.sh +++ b/contrib/subtree/t/t7900-subtree.sh @@ -708,7 +708,7 @@ test_expect_success 'make sure each filename changed exactly once in the entire test_create_commit "$subtree_test_count/sub proj" sub1 && ( cd "$subtree_test_count" && - git config log.date relative + git config log.date relative && git fetch ./"sub proj" master && git subtree add --prefix="sub dir" FETCH_HEAD ) && -- 2.18.0.345.g5c9ce644c3-goog
[PATCH 0/2] subtree: fix &&-chain and simplify tests (Re: [PATCH v2 01/10] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells)
(resetting cc list) Jonathan Nieder wrote: > This is causing contrib/subtree tests to fail for me: running "make -C > contrib/subtree test" produces [...] > error: bug in the test script: broken &&-chain or run-away HERE-DOC: [...] > Ugly quoting, useless use of "cat", etc, aside, I don't think it's > missing any &&. Hints? Turns out it was missing a && too. :) These patches are against "master". Ideally this would have come before es/chain-lint-in-subshell. Since this is contrib/, I'm okay with losing bisectability and having it come after, though. Thoughts of all kinds welcome. Jonathan Nieder (2): subtree: add missing && to &&-chain subtree: simplify preparation of expected results contrib/subtree/t/t7900-subtree.sh | 121 - 1 file changed, 31 insertions(+), 90 deletions(-)
Re: [PATCH 1/1] Add the `p4-pre-submit` hook
On 30 July 2018 at 20:07, Junio C Hamano wrote:
> Chen Bin writes:
>
>> The `p4-pre-submit` hook is executed before git-p4 submits code.
>> If the hook exits with non-zero value, submit process not start.
>>
>> Signed-off-by: Chen Bin
>> ---
>
> Luke, does this version look good to you?
>
Yes, I think so, We might find in the future that we do need an
additional hook *after* the change has been applied, but as per Chen's
comment, it sounds like that's not what is needed here; it can easily
be added in the future if necessary.
And there's no directly equivalent existing git-hook which could be
used instead, so this seems like a useful addition.
Possibly it should say "it takes no parameters" rather than "it takes
no parameter"; it is confusing that in English, zero takes the plural
rather than the singular. There's a PhD in linguistics there for
someone!
Luke
> I still think the addition to self.description is a bit too much but
> other than that the incremental since the last one looked sensible
> to my untrained eyes ;-)
>
> Thanks, both.
>
>> Documentation/git-p4.txt | 8
>> Documentation/githooks.txt | 7 +++
>> git-p4.py | 16 +++-
>> t/t9800-git-p4-basic.sh| 29 +
>> 4 files changed, 59 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
>> index f0de3b891..a7aac1b92 100644
>> --- a/Documentation/git-p4.txt
>> +++ b/Documentation/git-p4.txt
>> @@ -374,6 +374,14 @@ These options can be used to modify 'git p4 submit'
>> behavior.
>> been submitted. Implies --disable-rebase. Can also be set with
>> git-p4.disableP4Sync. Sync with origin/master still goes ahead if
>> possible.
>>
>> +Hook for submit
>> +~~~
>> +The `p4-pre-submit` hook is executed if it exists and is executable.
>> +The hook takes no parameter and nothing from standard input. Exiting with
>> +non-zero status from this script prevents `git-p4 submit` from launching.
>> +
>> +One usage scenario is to run unit tests in the hook.
>> +
>> Rebase options
>> ~~
>> These options can be used to modify 'git p4 rebase' behavior.
>> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
>> index e3c283a17..22fcabbe2 100644
>> --- a/Documentation/githooks.txt
>> +++ b/Documentation/githooks.txt
>> @@ -485,6 +485,13 @@ The exit status determines whether git will use the
>> data from the
>> hook to limit its search. On error, it will fall back to verifying
>> all files and folders.
>>
>> +p4-pre-submit
>> +~
>> +
>> +This hook is invoked by `git-p4 submit`. It takes no parameter and nothing
>> +from standard input. Exiting with non-zero status from this script prevent
>> +`git-p4 submit` from launching. Run `git-p4 submit --help` for details.
>> +
>> GIT
>> ---
>> Part of the linkgit:git[1] suite
>> diff --git a/git-p4.py b/git-p4.py
>> index b449db1cc..879abfd2b 100755
>> --- a/git-p4.py
>> +++ b/git-p4.py
>> @@ -1494,7 +1494,13 @@ def __init__(self):
>> optparse.make_option("--disable-p4sync",
>> dest="disable_p4sync", action="store_true",
>> help="Skip Perforce sync of p4/master
>> after submit or shelve"),
>> ]
>> -self.description = "Submit changes from git to the perforce depot."
>> +self.description = """Submit changes from git to the perforce
>> depot.\n
>> +The `p4-pre-submit` hook is executed if it exists and is executable.
>> +The hook takes no parameter and nothing from standard input. Exiting
>> with
>> +non-zero status from this script prevents `git-p4 submit` from
>> launching.
>> +
>> +One usage scenario is to run unit tests in the hook."""
>> +
>> self.usage += " [name of git branch to submit into perforce depot]"
>> self.origin = ""
>> self.detectRenames = False
>> @@ -2303,6 +2309,14 @@ def run(self, args):
>> sys.exit("number of commits (%d) must match number of shelved
>> changelist (%d)" %
>> (len(commits), num_shelves))
>>
>> +hooks_path = gitConfig("core.hooksPath")
>> +if len(hooks_path) <= 0:
>> +hooks_path = os.path.join(os.environ.get("GIT_DIR", ".git"),
>> "hooks")
>> +
>> +hook_file = os.path.join(hooks_path, "p4-pre-submit")
>> +if os.path.isfile(hook_file) and os.access(hook_file, os.X_OK) and
>> subprocess.call([hook_file]) != 0:
>> +sys.exit(1)
>> +
>> #
>> # Apply the commits, one at a time. On failure, ask if should
>> # continue to try the rest of the patches, or quit.
>> diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh
>> index 4849edc4e..2b7baa95d 100755
>> --- a/t/t9800-git-p4-basic.sh
>> +++ b/t/t9800-git-p4-basic.sh
>> @@ -261,6 +261,35 @@ test_expect_success 'unresolvable host in P4PORT should
>> display error' '
>> )
>> '
>>
>> +# Test
Re: [PATCH 1/2] Document git config getter return value.
On Mon, Jul 30, 2018 at 8:26 AM Han-Wen Nienhuys wrote: > --- > config.h | 10 -- > 1 file changed, 8 insertions(+), 2 deletions(-) Missing sign-off.
Re: [PATCH 2/2] sequencer: fix "rebase -i --root" corrupting author header timezone
On Mon, Jul 30, 2018 at 8:20 AM Phillip Wood wrote:
> On 30/07/18 10:29, Eric Sunshine wrote:
> > When "git rebase -i --root" creates a new root commit, it corrupts the
> > "author" header's timezone by repeating the last digit:
> > [...]
> > Signed-off-by: Eric Sunshine
> > ---
> > diff --git a/sequencer.c b/sequencer.c
> > @@ -654,6 +654,7 @@ static int write_author_script(const char *message)
> > + strbuf_addch(, '\'');
> > @@ -724,7 +725,11 @@ static const char *read_author_ident(struct strbuf
> > *buf)
> > - sq_dequote(in);
> > + if (!sq_dequote(in)) {
> > + warning(_("bad quoting on %s value in '%s'"),
> > + keys[i], rebase_path_author_script());
> > + return NULL;
>
> I think we want to handle the broken author script properly rather than
> returning NULL. If we had a single function
> int read_author_script(const char **name, const char **author, const
> char **date)
> to read the author script that tried sq_dequote() and then fell back to
> code based on read_env_script() that handled the missing "'" at the end
> and also the bad quoting of "'" if sq_dequote() failed it would make it
> easier to fix the existing bugs, rather than having to fix
> read_author_ident() and read_env_script() separately. What do you think?
That makes sense as a long-term plan, however, I'm concerned with the
immediate problem that a released version of Git can (and did, in my
case) corrupt commit objects. So, in the short term, I think it makes
sense to get this minimal fix landed, and build the more "correctly
engineered" solution on top of it, without the pressure of worrying
about corruption spreading.
Re: Is detecting endianness at compile-time unworkable?
The change was definitely made for performance. Removing the if statements, conditioned upon endianess was an approx 10% improvement, which was very important to getting this library accepted into git. Thanks, Dan On Mon, Jul 30, 2018 at 11:32 AM, Junio C Hamano wrote: > Junio C Hamano writes: > > > Ævar Arnfjörð Bjarmason writes: > > > >> And, as an aside, the reason we can't easily make it better ourselves is > >> because the build process for git.git doesn't have a facility to run > >> code to detect this type of stuff (the configure script is always > >> optional). So we can't just run this test ourselves. > > > > It won't help those who cross-compile anyway. I thought we declared > > "we make a reasonable effort to guess the target endianness from the > > system header by inspecting usual macros, but will not aim to cover > > every system on the planet---instead there is a knob to tweak it for > > those on exotic platforms" last time we discussed this? > > Well, having said all that, I do not think I personally mind if > ./configure learned to include a "compile small program and run it > to determine byte order on the build machine" as part of "we make a > reasonable effort" as long as it cleanly excludes cross building > case (and the result is made overridable just in case we misdetect > the "cross-ness" of the build). > >
Re: Is detecting endianness at compile-time unworkable?
Junio C Hamano writes: > Ævar Arnfjörð Bjarmason writes: > >> And, as an aside, the reason we can't easily make it better ourselves is >> because the build process for git.git doesn't have a facility to run >> code to detect this type of stuff (the configure script is always >> optional). So we can't just run this test ourselves. > > It won't help those who cross-compile anyway. I thought we declared > "we make a reasonable effort to guess the target endianness from the > system header by inspecting usual macros, but will not aim to cover > every system on the planet---instead there is a knob to tweak it for > those on exotic platforms" last time we discussed this? Well, having said all that, I do not think I personally mind if ./configure learned to include a "compile small program and run it to determine byte order on the build machine" as part of "we make a reasonable effort" as long as it cleanly excludes cross building case (and the result is made overridable just in case we misdetect the "cross-ness" of the build).
Re: [PATCH] doc: fix want-capability separator
On Sat, Jul 28, 2018 at 2:16 PM Masaya Suzuki wrote:
> Signed-off-by: Masaya Suzuki
The email addresses mismatch?
> Unlike ref advertisement, client capabilities and the first want are
> separated by SP, not NUL, in the implementation. Fix the documentation
> to align with the implementation.
Makes sense! Thanks for the fix!
> pack-protocol.txt is already fixed.
which has
capability-list = capability *(SP capability)
since b31222cfb7f (Update packfile transfer protocol
documentation, 2009-11-03), which is the first to mention
the capability line, so I'd claim it was always correct?
>
> ---
> Documentation/technical/http-protocol.txt | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/technical/http-protocol.txt
> b/Documentation/technical/http-protocol.txt
> index 64f49d0bb..9c5b6f0fa 100644
> --- a/Documentation/technical/http-protocol.txt
> +++ b/Documentation/technical/http-protocol.txt
> @@ -338,11 +338,11 @@ server advertises capability `allow-tip-sha1-in-want` or
>request_end
>request_end = "" / "done"
>
> - want_list = PKT-LINE(want NUL cap_list LF)
> + want_list = PKT-LINE(want SP cap_list LF)
>*(want_pkt)
>want_pkt = PKT-LINE(want LF)
>want = "want" SP id
> - cap_list = *(SP capability) SP
> + cap_list = capability *(SP capability)
>
>have_list = *PKT-LINE("have" SP id LF)
Just after these context lines we have
TODO: Document this further.
which is a good hint that the existing documentation
can benefit from patches like these.
Thanks,
Stefan
Re: [GSoC][PATCH v4 15/20] rebase -i: rewrite write_basic_state() in C
> diff --git a/sequencer.c b/sequencer.c
> index 1c035ceec7..d257903db0 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> +int write_basic_state(struct replay_opts *opts, const char *head_name,
> + const char *onto, const char *orig_head)
> +{
> + const char *quiet = getenv("GIT_QUIET");
> +
> + if (head_name)
> + write_file(rebase_path_head_name(), "%s\n", head_name);
> + if (onto)
> + write_file(rebase_path_onto(), "%s\n", onto);
> + if (orig_head)
> + write_file(rebase_path_orig_head(), "%s\n", orig_head);
> +
> + if (quiet)
> + write_file(rebase_path_quiet(), "%s\n", quiet);
> + else
> + write_file(rebase_path_quiet(), "");
This is not a faithful conversion of the original. git-rebase.sh writes
this 'quiet' file with:
echo "$GIT_QUIET" > "$state_dir"/quiet
which means that a single newline character was written even when
$GIT_QUIET was unset/empty.
I seem to recall a case in the past, when a shell-to-C conversion
accidentally dropped a newline from a similar state-file, which then
caused some issues later on. But I don't remember the specifics and a
quick search didn't turn up anything relevant either...
> +
> + if (opts->verbose)
> + write_file(rebase_path_verbose(), "");
> + if (opts->strategy)
> + write_file(rebase_path_strategy(), "%s\n", opts->strategy);
> + if (opts->xopts_nr > 0)
> + write_strategy_opts(opts);
> +
> + if (opts->allow_rerere_auto == RERERE_AUTOUPDATE)
> + write_file(rebase_path_allow_rerere_autoupdate(),
> "--rerere-autoupdate\n");
> + else if (opts->allow_rerere_auto == RERERE_NOAUTOUPDATE)
> + write_file(rebase_path_allow_rerere_autoupdate(),
> "--no-rerere-autoupdate\n");
> +
> + if (opts->gpg_sign)
> + write_file(rebase_path_gpg_sign_opt(), "-S%s\n",
> opts->gpg_sign);
> + if (opts->signoff)
> + write_file(rebase_path_signoff(), "--signoff\n");
> +
> + return 0;
> +}
> +
> static int walk_revs_populate_todo(struct todo_list *todo_list,
> struct replay_opts *opts)
> {
Re: [PATCH v2 01/10] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells
Hi, Eric Sunshine wrote: > The --chain-lint option detects broken &&-chains by forcing the test to > exit early (as the very first step) with a sentinel value. If that > sentinel is the test's overall exit code, then the &&-chain is intact; > if not, then the chain is broken. Unfortunately, this detection does not > extend to &&-chains within subshells even when the subshell itself is > properly linked into the outer &&-chain. > > Address this shortcoming by feeding the body of the test to a > lightweight "linter" which can peer inside subshells and identify broken > &&-chains by pure textual inspection. Interesting. >Although the linter does not > actually parse shell scripts, it has enough knowledge of shell syntax to > reliably deal with formatting style variations (as evolved over the > years) and to avoid being fooled by non-shell content (such as inside > here-docs and multi-line strings). This is causing contrib/subtree tests to fail for me: running "make -C contrib/subtree test" produces [...] *** t7900-subtree.sh *** ok 1 - no merge from non-existent subtree ok 2 - no pull from non-existent subtree ok 3 - add subproj as subtree into sub dir/ with --prefix ok 4 - add subproj as subtree into sub dir/ with --prefix and --message ok 5 - add subproj as subtree into sub dir/ with --prefix as -P and --message as -m ok 6 - add subproj as subtree into sub dir/ with --squash and --prefix and --message ok 7 - merge new subproj history into sub dir/ with --prefix ok 8 - merge new subproj history into sub dir/ with --prefix and --message ok 9 - merge new subproj history into sub dir/ with --squash and --prefix and --message ok 10 - merge the added subproj again, should do nothing ok 11 - merge new subproj history into subdir/ with a slash appended to the argument of --prefix ok 12 - split requires option --prefix ok 13 - split requires path given by option --prefix must exist ok 14 - split sub dir/ with --rejoin ok 15 - split sub dir/ with --rejoin from scratch ok 16 - split sub dir/ with --rejoin and --message ok 17 - split "sub dir"/ with --branch ok 18 - check hash of split ok 19 - split "sub dir"/ with --branch for an existing branch ok 20 - split "sub dir"/ with --branch for an incompatible branch error: bug in the test script: broken &&-chain or run-away HERE-DOC: subtree_test_create_repo "$subtree_test_count" && [...] ) Makefile:44: recipe for target 't7900-subtree.sh' failed The problematic test code looks like this: ( cd "$subtree_test_count/sub proj" && git fetch .. subproj-br && git merge FETCH_HEAD && chks="sub1 sub2 sub3 sub4" && chks_sub=$(cat <
Re: [PATCH v2 0/4] Speed up unpack_trees()
On 7/29/2018 6:33 AM, Nguyễn Thái Ngọc Duy wrote: This series speeds up unpack_trees() a bit by using cache-tree. unpack-trees could bit split in three big parts - the actual tree unpacking and running n-way merging - update worktree, which could be expensive depending on how much I/O is involved - repair cache-tree This series focuses on the first part alone and could give 700% speedup (best case possible scenario, real life ones probably not that impressive). It also shows that the reparing cache-tree is kinda expensive. I have an idea of reusing cache-tree from the original index, but I'll leave that to Ben or others to try out and see if it helps at all. v2 fixes the comments from Junio, adds more performance tracing and reduces the cost of adding index entries. Nguyễn Thái Ngọc Duy (4): unpack-trees.c: add performance tracing unpack-trees: optimize walking same trees with cache-tree unpack-trees: reduce malloc in cache-tree walk unpack-trees: cheaper index update when walking by cache-tree cache-tree.c | 2 + cache.h| 1 + read-cache.c | 3 +- unpack-trees.c | 161 - unpack-trees.h | 1 + 5 files changed, 166 insertions(+), 2 deletions(-) I ran "git checkout" on a large repo and averaged the results of 3 runs. This clearly demonstrates the benefit of the optimized unpack_trees() as even the final "diff-index" is essentially a 3rd call to unpack_trees(). baselinenew -- 0.535510167 0.556558733 s: read cache .git/index 0.3057373 0.3147105 s: initialize name hash 0.0184082 0.023558433 s: preload index 0.086910967 0.089085967 s: refresh index 7.889590767 2.191554433 s: unpack trees 0.120760833 0.131941267 s: update worktree after a merge 2.2583504 2.572663167 s: repair cache-tree 0.8916137 0.959495233 s: write index, changed mask = 28 3.405199233 0.2710663 s: unpack trees 0.000999667 0.0021554 s: update worktree after a merge 3.4063306 0.273318333 s: diff-index 16.9524923 9.462943133 s: git command: 'c:\git-sdk-64\usr\src\git\git.exe' checkout The first call to unpack_trees() saves 72% The 2nd and 3rd call save 92% Total time savings for the entire command was 44% In the performance game of whack-a-mole, that call to repair cache-tree is now looking quite expensive... Ben
Re: [PATCH 1/1] Add the `p4-pre-submit` hook
Chen Bin writes:
> The `p4-pre-submit` hook is executed before git-p4 submits code.
> If the hook exits with non-zero value, submit process not start.
>
> Signed-off-by: Chen Bin
> ---
Luke, does this version look good to you?
I still think the addition to self.description is a bit too much but
other than that the incremental since the last one looked sensible
to my untrained eyes ;-)
Thanks, both.
> Documentation/git-p4.txt | 8
> Documentation/githooks.txt | 7 +++
> git-p4.py | 16 +++-
> t/t9800-git-p4-basic.sh| 29 +
> 4 files changed, 59 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
> index f0de3b891..a7aac1b92 100644
> --- a/Documentation/git-p4.txt
> +++ b/Documentation/git-p4.txt
> @@ -374,6 +374,14 @@ These options can be used to modify 'git p4 submit'
> behavior.
> been submitted. Implies --disable-rebase. Can also be set with
> git-p4.disableP4Sync. Sync with origin/master still goes ahead if
> possible.
>
> +Hook for submit
> +~~~
> +The `p4-pre-submit` hook is executed if it exists and is executable.
> +The hook takes no parameter and nothing from standard input. Exiting with
> +non-zero status from this script prevents `git-p4 submit` from launching.
> +
> +One usage scenario is to run unit tests in the hook.
> +
> Rebase options
> ~~
> These options can be used to modify 'git p4 rebase' behavior.
> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
> index e3c283a17..22fcabbe2 100644
> --- a/Documentation/githooks.txt
> +++ b/Documentation/githooks.txt
> @@ -485,6 +485,13 @@ The exit status determines whether git will use the data
> from the
> hook to limit its search. On error, it will fall back to verifying
> all files and folders.
>
> +p4-pre-submit
> +~
> +
> +This hook is invoked by `git-p4 submit`. It takes no parameter and nothing
> +from standard input. Exiting with non-zero status from this script prevent
> +`git-p4 submit` from launching. Run `git-p4 submit --help` for details.
> +
> GIT
> ---
> Part of the linkgit:git[1] suite
> diff --git a/git-p4.py b/git-p4.py
> index b449db1cc..879abfd2b 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -1494,7 +1494,13 @@ def __init__(self):
> optparse.make_option("--disable-p4sync",
> dest="disable_p4sync", action="store_true",
> help="Skip Perforce sync of p4/master
> after submit or shelve"),
> ]
> -self.description = "Submit changes from git to the perforce depot."
> +self.description = """Submit changes from git to the perforce
> depot.\n
> +The `p4-pre-submit` hook is executed if it exists and is executable.
> +The hook takes no parameter and nothing from standard input. Exiting with
> +non-zero status from this script prevents `git-p4 submit` from launching.
> +
> +One usage scenario is to run unit tests in the hook."""
> +
> self.usage += " [name of git branch to submit into perforce depot]"
> self.origin = ""
> self.detectRenames = False
> @@ -2303,6 +2309,14 @@ def run(self, args):
> sys.exit("number of commits (%d) must match number of shelved
> changelist (%d)" %
> (len(commits), num_shelves))
>
> +hooks_path = gitConfig("core.hooksPath")
> +if len(hooks_path) <= 0:
> +hooks_path = os.path.join(os.environ.get("GIT_DIR", ".git"),
> "hooks")
> +
> +hook_file = os.path.join(hooks_path, "p4-pre-submit")
> +if os.path.isfile(hook_file) and os.access(hook_file, os.X_OK) and
> subprocess.call([hook_file]) != 0:
> +sys.exit(1)
> +
> #
> # Apply the commits, one at a time. On failure, ask if should
> # continue to try the rest of the patches, or quit.
> diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh
> index 4849edc4e..2b7baa95d 100755
> --- a/t/t9800-git-p4-basic.sh
> +++ b/t/t9800-git-p4-basic.sh
> @@ -261,6 +261,35 @@ test_expect_success 'unresolvable host in P4PORT should
> display error' '
> )
> '
>
> +# Test following scenarios:
> +# - Without ".git/hooks/p4-pre-submit" , submit should continue
> +# - With the hook returning 0, submit should continue
> +# - With the hook returning 1, submit should abort
> +test_expect_success 'run hook p4-pre-submit before submit' '
> + test_when_finished cleanup_git &&
> + git p4 clone --dest="$git" //depot &&
> + (
> + cd "$git" &&
> + echo "hello world" >hello.txt &&
> + git add hello.txt &&
> + git commit -m "add hello.txt" &&
> + git config git-p4.skipSubmitEdit true &&
> + git-p4 submit --dry-run >out &&
> + grep "Would apply" out &&
> + mkdir -p .git/hooks &&
> + write_script
Re: [BUG] fetching sometimes doesn't update refs
On 07/29, Jeff King wrote: > I've noticed for the past couple of weeks that some of my fetches don't > seem to actually update refs, but a follow-up fetch will. I finally > managed to catch it in the act and track it down. It bisects to your > 989b8c4452 (fetch-pack: put shallow info in output parameter, > 2018-06-27). > > A reproduction recipe is below. I can't imagine why this repo in > particular triggers it, but it was the one where I initially saw the > problem (and doing a tiny reproduction does not seem to work). I'm > guessing it has something to do with the refs, since the main change in > the offending commit is that we recompute the refmap. I've noticed this behavior sporadically as well, though I've never been able to reliably reproduce it, so thanks for creating a reproduction recipe. I suspected that it had to do with the ref-in-want series so thanks for tracking that down too. We'll take a look. > > -- >8 -- > # clone the repo as it is today > git clone https://github.com/cmcaine/tridactyl.git > cd tridactyl > > # roll back the refs so that there is something to fetch > for i in refs/heads/master refs/remotes/origin/master; do > git update-ref $i $i^ > done > > # and delete the now-unreferenced objects, pretending we are an earlier > # clone that had not yet fetched > rm -rf .git/logs > git repack -ad > > # now fetch; this will get the objects but fail to update refs > git fetch > > # and fetching again will actually update the refs > git fetch > -- 8< -- > > -Peff -- Brandon Williams
Re: [PATCH v3 09/11] rerere: return strbuf from handle path
Thomas Gummerer writes: > Currently we write the conflict to disk directly in the handle_path > function. To make it re-usable for nested conflicts, instead of > writing the conflict out directly, store it in a strbuf and let the > caller write it out. > > This does mean some slight increase in memory usage, however that > increase is limited to the size of the largest conflict we've > currently processed. We already keep one copy of the conflict in > memory, and it shouldn't be too large, so the increase in memory usage > seems acceptable. > > As a bonus this lets us get replace the rerere_io_putconflict function > with a trivial two line function. Makes sense.
Re: [PATCH v3 08/11] rerere: factor out handle_conflict function
Thomas Gummerer writes:
> Factor out the handle_conflict function, which handles a single
> conflict in a path. This is in preparation for a subsequent commit,
> where this function will be re-used. No functional changes intended.
>
> Signed-off-by: Thomas Gummerer
> ---
> rerere.c | 87 ++--
> 1 file changed, 47 insertions(+), 40 deletions(-)
Renumbering of the enum made me raise my eyebrow a bit briefly but
it is merely to keep track of the state locally and invisible from
the outside, so it is perfectly fine.
> - git_SHA_CTX ctx;
> - int has_conflicts = 0;
> enum {
> - RR_CONTEXT = 0, RR_SIDE_1, RR_SIDE_2, RR_ORIGINAL
> - } hunk = RR_CONTEXT;
> + RR_SIDE_1 = 0, RR_SIDE_2, RR_ORIGINAL
> + } hunk = RR_SIDE_1;
Re: [PATCH v3 07/11] rerere: only return whether a path has conflicts or not
Thomas Gummerer writes: > We currently return the exact number of conflict hunks a certain path > has from the 'handle_paths' function. However all of its callers only > care whether there are conflicts or not or if there is an error. > Return only that information, and document that only that information > is returned. This will simplify the code in the subsequent steps. > > Signed-off-by: Thomas Gummerer > --- > rerere.c | 23 --- > 1 file changed, 12 insertions(+), 11 deletions(-) I do recall writing this code without knowing if the actual number of conflicts would be useful by callers, but it is apparent that it wasn't. I won't mind losing that bit of info at all. Besides, we won't risk mistaking a file with 2 billion conflicts with a file whose conflicts cannot be parsed ;-). The patch looks good. Thanks.
Re: [PATCH v3 00/11] rerere: handle nested conflicts
Thomas Gummerer writes: > Thomas Gummerer (11): > rerere: unify error messages when read_cache fails > rerere: lowercase error messages > rerere: wrap paths in output in sq > rerere: mark strings for translation > rerere: add documentation for conflict normalization > rerere: fix crash when conflict goes unresolved > rerere: only return whether a path has conflicts or not > rerere: factor out handle_conflict function > rerere: return strbuf from handle path > rerere: teach rerere to handle nested conflicts > rerere: recalculate conflict ID when unresolved conflict is committed Even though I am not certain about the last two steps, everything before them looked trivially correct and good changes (well, the "strbuf" one's goodness obviously depends on the goodness of the last two, which are helped by it). Sorry for taking so long before getting to the series.
Re: [PATCH] refspec: allow @ on the left-hand side of refspecs
On 07/29, brian m. carlson wrote:
> The object ID parsing machinery is aware of "@" as a synonym for "HEAD"
> and this is documented accordingly in gitrevisions(7). The push
> documentation describes the source portion of a refspec as "any
> arbitrary 'SHA-1 expression'"; however, "@" is not allowed on the
> left-hand side of a refspec, since we attempt to check for it being a
> valid ref name and fail (since it is not).
>
> Teach the refspec machinery about this alias and silently substitute
> "HEAD" when we see "@". This handles the fact that HEAD is a symref and
> preserves its special behavior. We need not handle other arbitrary
> object ID expressions (such as "@^") when pushing because the revision
> machinery already handles that for us.
So this claims that using "@^" should work despite not accounting for it
explicitly or am I misreading? Unless I'm mistaken, it looks like we
don't really support arbitrary rev syntax in refspecs since "HEAD^"
doesn't work either.
>
> Signed-off-by: brian m. carlson
> ---
> I probably type "git push upstream HEAD" from five to thirty times a
> day, many of those where I typo "HEAD", so I decided to implement the
> shorter form. This design handles @ as HEAD in both fetch and push,
> whereas alternate solutions would not.
I'm always a fan of finding shortcuts and reducing how much I type, so
thank you :)
>
> check_refname_format explicitly rejects "@"; I tried at first to simply
> ignore that with a flag, but we end up calling that from several other
> places in the codebase and rejecting it and all of those places would
> have needed updating.
>
> I thought about putting the if/else logic in a function, but since it's
> just four lines, I decided not to. However, if people think it would be
> tidier, I can do so.
>
> Note that the test portion of the patch is best read with git diff -w;
> the current version is very noisy.
>
> refspec.c | 6 ++-
> t/t5516-fetch-push.sh | 104 +-
> 2 files changed, 58 insertions(+), 52 deletions(-)
>
> diff --git a/refspec.c b/refspec.c
> index e8010dce0c..57c2f65104 100644
> --- a/refspec.c
> +++ b/refspec.c
> @@ -62,8 +62,12 @@ static int parse_refspec(struct refspec_item *item, const
> char *refspec, int fet
> return 0;
> }
>
> + if (llen == 1 && lhs[0] == '@')
> + item->src = xstrdup("HEAD");
> + else
> + item->src = xstrndup(lhs, llen);
> +
This is probably the easiest place to put the aliasing logic so I don't
have any issue with including it here.
--
Brandon Williams
Re: [PATCH v3 05/11] rerere: add documentation for conflict normalization
Thomas Gummerer writes:
> +Different conflict styles and branch names are normalized by stripping
> +the labels from the conflict markers, and removing extraneous
> +information from the `diff3` conflict style. Branches that are merged
s/extraneous information/commmon ancestor version/ perhaps, to be
fact-based without passing value judgment?
We drop the common ancestor version only because we cannot normalize
from `merge` style to `diff3` style by adding one, and not because
it is extraneous. It does help humans understand the conflict a lot
better to have that section.
> +By extension, this means that rerere should recognize that the above
> +conflicts are the same. To do this, the labels on the conflict
> +markers are stripped, and the diff3 output is removed. The above
s/diff3 output/common ancestor version/, as "diff3 output" would
mean the whole thing between <<< and >>> to readers.
> diff --git a/rerere.c b/rerere.c
> index be98c0afcb..da1ab54027 100644
> --- a/rerere.c
> +++ b/rerere.c
> @@ -394,10 +394,6 @@ static int is_cmarker(char *buf, int marker_char, int
> marker_size)
> * and NUL concatenated together.
> *
> * Return the number of conflict hunks found.
> - *
> - * NEEDSWORK: the logic and theory of operation behind this conflict
> - * normalization may deserve to be documented somewhere, perhaps in
> - * Documentation/technical/rerere.txt.
> */
> static int handle_path(unsigned char *sha1, struct rerere_io *io, int
> marker_size)
> {
Thanks for finally removing this age-old NEEDSWORK comment.
Re: [PATCH v3 06/11] rerere: fix crash when conflict goes unresolved
Thomas Gummerer writes:
> Currently when a user doesn't resolve a conflict in a file, but
> commits the file with the conflict markers, and later the file ends up
> in a state in which rerere can't handle it, subsequent rerere
> operations that are interested in that path, such as 'rerere clear' or
> 'rerere forget ' will fail, or even worse in the case of 'rerere
> clear' segfault.
>
> Such states include nested conflicts, or an extra conflict marker that
> doesn't have any match.
>
> This is because the first 'git rerere' when there was only one
> conflict in the file leaves an entry in the MERGE_RR file behind. The
I find this sentence, especially the "only one conflict in the file"
part, a bit unclear. What does the sentence count as one conflict?
One block of lines enclosed inside "<<<"...">>>" pair? The command
behaves differently when there are two such blocks instead?
> next 'git rerere' will then pick the rerere ID for that file up, and
> not assign a new ID as it can't successfully calculate one. It will
> however still try to do the rerere operation, because of the existing
> ID. As the handle_file function fails, it will remove the 'preimage'
> for the ID in the process, while leaving the ID in the MERGE_RR file.
>
> Now when 'rerere clear' for example is run, it will segfault in
> 'has_rerere_resolution', because status is NULL.
I think this "status" refers to the collection->status[]. How do we
get into that state, though?
new_rerere_id() and new_rerere_id_hex() fills id->collection by
calling find_rerere_dir(), which either finds an existing rerere_dir
instance or manufactures one with .status==NULL. The .status[]
array is later grown by calling fit_variant as we scan and find the
pre/post images, but because there is no pre/post image for a file
with unparseable conflicts, it is left NULL.
So another possible fix could be to make sure that .status[] is only
read when .status_nr says there is something worth reading. I am
not saying that would be a better fix---I am just thinking out loud
to make sure I understand the issue correctly.
> To fix this, remove the rerere ID from the MERGE_RR file in the case
> when we can't handle it, and remove the corresponding variant from
> .git/rr-cache/. Removing it unconditionally is fine here, because if
> the user would have resolved the conflict and ran rerere, the entry
> would no longer be in the MERGE_RR file, so we wouldn't have this
> problem in the first place, while if the conflict was not resolved,
> the only thing that's left in the folder is the 'preimage', which by
> itself will be regenerated by git if necessary, so the user won't
> loose any work.
s/loose/lose/
> Note that other variants that have the same conflict ID will not be
> touched.
Nice. Thanks for a fix.
>
> Signed-off-by: Thomas Gummerer
> ---
> rerere.c | 12 +++-
> t/t4200-rerere.sh | 22 ++
> 2 files changed, 29 insertions(+), 5 deletions(-)
>
> diff --git a/rerere.c b/rerere.c
> index da1ab54027..895ad80c0c 100644
> --- a/rerere.c
> +++ b/rerere.c
> @@ -823,10 +823,7 @@ static int do_plain_rerere(struct string_list *rr, int
> fd)
> struct rerere_id *id;
> unsigned char sha1[20];
> const char *path = conflict.items[i].string;
> - int ret;
> -
> - if (string_list_has_string(rr, path))
> - continue;
> + int ret, has_string;
>
> /*
>* Ask handle_file() to scan and assign a
> @@ -834,7 +831,12 @@ static int do_plain_rerere(struct string_list *rr, int
> fd)
>* yet.
>*/
> ret = handle_file(path, sha1, NULL);
> - if (ret < 1)
> + has_string = string_list_has_string(rr, path);
> + if (ret < 0 && has_string) {
> + remove_variant(string_list_lookup(rr, path)->util);
> + string_list_remove(rr, path, 1);
> + }
> + if (ret < 1 || has_string)
> continue;
We used to say "if we know about the path we do not do anything
here, if we do not see any conflict in the file we do nothing,
otherwise we assign a new id"; we now say "see if we can parse
and also see if we have conflict(s); if we know about the path and
we cannot parse, drop it from the rr database (because otherwise the
entry will cause us trouble elsewhere later). Otherwise, if we do
not have any conflict or we already know about the path, no need to
do anything. Otherwise, i.e. a newly discovered path with conflicts
gets a new id".
Makes sense. "A known path with unparseable conflict gets dropped"
is the important change in this hunk.
> diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh
> index 8417e5a4b1..34f0518a5e 100755
> --- a/t/t4200-rerere.sh
> +++ b/t/t4200-rerere.sh
> @@ -580,4 +580,26 @@ test_expect_success 'multiple identical conflicts' '
> count_pre_post 0
Re: [PATCH v3 10/11] rerere: teach rerere to handle nested conflicts
Thomas Gummerer writes:
> Currently rerere can't handle nested conflicts and will error out when
> it encounters such conflicts. Do that by recursively calling the
> 'handle_conflict' function to normalize the conflict.
>
> The conflict ID calculation here deserves some explanation:
>
> As we are using the same handle_conflict function, the nested conflict
> is normalized the same way as for non-nested conflicts, which means
> the ancestor in the diff3 case is stripped out, and the parts of the
> conflict are ordered alphabetically.
>
> The conflict ID is however is only calculated in the top level
> handle_conflict call, so it will include the markers that 'rerere'
> adds to the output. e.g. say there's the following conflict:
>
> <<< HEAD
> 1
> ===
> <<< HEAD
> 3
> ===
> 2
> >>> branch-2
> >>> branch-3~
Hmph, I vaguely recall that I made inner merges to use the conflict
markers automatically lengthened (by two, if I recall correctly)
than its immediate outer merge. Wouldn't the above look more like
<<< HEAD
1
===
< HEAD
3
=
2
> branch-2
>>> branch-3~
Perhaps I am not recalling it correctly.
> it would be recorde as follows in the preimage:
>
> <<<
> 1
> ===
> <<<
> 2
> ===
> 3
> >>>
> >>>
>
> and the conflict ID would be calculated as
>
> sha1(1<<<
> 2
> ===
> 3
> >>>)
>
> Stripping out vs. leaving the conflict markers in place in the inner
> conflict should have no practical impact, but it simplifies the
> implementation.
>
> Signed-off-by: Thomas Gummerer
> ---
> Documentation/technical/rerere.txt | 42 ++
> rerere.c | 10 +--
> t/t4200-rerere.sh | 37 ++
> 3 files changed, 87 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/technical/rerere.txt
> b/Documentation/technical/rerere.txt
> index 4102cce7aa..60d48dc4fe 100644
> --- a/Documentation/technical/rerere.txt
> +++ b/Documentation/technical/rerere.txt
> @@ -138,3 +138,45 @@ SHA1('BC').
> If there are multiple conflicts in one file, the sha1 is calculated
> the same way with all hunks appended to each other, in the order in
> which they appear in the file, separated by a character.
> +
> +Nested conflicts
> +
> +
> +Nested conflicts are handled very similarly to "simple" conflicts.
> +Similar to simple conflicts, the conflict is first normalized by
> +stripping the labels from conflict markers, stripping the diff3
> +output, and the sorting the conflict hunks, both for the outer and the
> +inner conflict. This is done recursively, so any number of nested
> +conflicts can be handled.
> +
> +The only difference is in how the conflict ID is calculated. For the
> +inner conflict, the conflict markers themselves are not stripped out
> +before calculating the sha1.
> +
> +Say we have the following conflict for example:
> +
> +<<< HEAD
> +1
> +===
> +<<< HEAD
> +3
> +===
> +2
> +>>> branch-2
> +>>> branch-3~
> +
> +After stripping out the labels of the conflict markers, and sorting
> +the hunks, the conflict would look as follows:
> +
> +<<<
> +1
> +===
> +<<<
> +2
> +===
> +3
> +>>>
> +>>>
> +
> +and finally the conflict ID would be calculated as:
> +`sha1('1<<<\n3\n===\n2\n>>>')`
> diff --git a/rerere.c b/rerere.c
> index a35b88916c..f78bef80b1 100644
> --- a/rerere.c
> +++ b/rerere.c
> @@ -365,12 +365,18 @@ static int handle_conflict(struct strbuf *out, struct
> rerere_io *io,
> RR_SIDE_1 = 0, RR_SIDE_2, RR_ORIGINAL
> } hunk = RR_SIDE_1;
> struct strbuf one = STRBUF_INIT, two = STRBUF_INIT;
> - struct strbuf buf = STRBUF_INIT;
> + struct strbuf buf = STRBUF_INIT, conflict = STRBUF_INIT;
> int has_conflicts = -1;
>
> while (!io->getline(, io)) {
> if (is_cmarker(buf.buf, '<', marker_size)) {
> - break;
> + if (handle_conflict(, io, marker_size, NULL) <
> 0)
> + break;
> + if (hunk == RR_SIDE_1)
> + strbuf_addbuf(, );
> + else
> + strbuf_addbuf(, );
Hmph, do we ever see the inner conflict block while we are skipping
and ignoring the common ancestor version, or it is impossible that
we see '<' only while processing either our or their side?
> + strbuf_release();
> } else if (is_cmarker(buf.buf, '|', marker_size)) {
> if (hunk != RR_SIDE_1)
> break;
> diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh
> index 34f0518a5e..d63fe2b33b 100755
>
Re: [PATCH/RFC] Color merge conflicts
On Mon, Jul 30, 2018 at 9:00 AM Nguyễn Thái Ngọc Duy wrote: > > One of the things I notice when watching a normal git user face a > merge conflicts is the output is very verbose (especially when there > are multiple conflicts) and it's hard to spot the important parts to > start resolving conflicts unless you know what to look for. I usually go by git-status, but I am not the watched normal user, hopefully. Maybe we want to run git-status after a failed merge for the user, too, though? > This is the start to hopefully help that a bit. One thing I'm not sure > is what to color because that affects the config keys we use for > this. If we have to color different things, it's best to go > "color.merge." but if this is the only place to color, that slot > thing looks over-engineered. > > Perhaps another piece to color is the conflicted path? Maybe. But on > the other hand, I don't think we want a chameleon output, just enough > visual cues to go from one conflict to the next... I would think we would want to highlight the type of conflict as well, e.g. CONFLICT> \ (rename/rename): \ Rename a -> b in branch foo \ rename a ->c in bar but then again, this patch is better than not doing any colors. I wonder if we want to have certain colors associated with certain types of merge conflicts, e.g. anything involving a rename would be yellow, any conflict with deletion to be dark red, regular merge conflicts blue (and at the same time we could introduce coloring of conflict markers to also be blue) (I am just making up colors, not seriously suggesting them) I like the idea! Thanks, Stefan
Re: [PATCH] DO-NOT-MERGE: write and read commit-graph always
> I wonder though if all those changes to the testsuite shouldn't be > merged. I think Stolee doesn't want this to be merged after rereading subject and the commit message. Thanks, Stefan
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 >
[PATCH v5 3/3] builtin/rebase: support running "git rebase "
This patch gives life to the skeleton added in the previous patches:
With this change, we can perform a elementary rebase (without any
options).
It can be tested thusly by:
git -c rebase.usebuiltin=true rebase HEAD~2
The rebase backends (i.e. the shell script functions defined in
`git-rebase--`) are still at work here and the "builtin
rebase"'s purpose is simply to parse the options and set
everything up so that those rebase backends can do their work.
Note: We take an alternative approach here which is *not* to set the
environment variables via `run_command_v_opt_cd_env()` because those
variables would then be visible by processes spawned from the rebase
backends. Instead, we work hard to set them in the shell script snippet.
On Windows, some of the tests fail merely due to core.fileMode not
being heeded that's why the core.*config variables is parsed here.
The next commits will handle and support all the wonderful rebase
options.
Signed-off-by: Pratik Karki
---
builtin/rebase.c | 350 ++-
1 file changed, 349 insertions(+), 1 deletion(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index c44addb2a4..5863139204 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -9,6 +9,24 @@
#include "exec-cmd.h"
#include "argv-array.h"
#include "dir.h"
+#include "packfile.h"
+#include "refs.h"
+#include "quote.h"
+#include "config.h"
+#include "cache-tree.h"
+#include "unpack-trees.h"
+#include "lockfile.h"
+
+static GIT_PATH_FUNC(apply_dir, "rebase-apply")
+static GIT_PATH_FUNC(merge_dir, "rebase-merge")
+
+enum rebase_type {
+ REBASE_UNSPECIFIED = -1,
+ REBASE_AM,
+ REBASE_MERGE,
+ REBASE_INTERACTIVE,
+ REBASE_PRESERVE_MERGES
+};
static int use_builtin_rebase(void)
{
@@ -30,8 +48,243 @@ static int use_builtin_rebase(void)
return ret;
}
+static int apply_autostash(void)
+{
+ warning("TODO");
+ return 0;
+}
+
+struct rebase_options {
+ enum rebase_type type;
+ const char *state_dir;
+ struct commit *upstream;
+ const char *upstream_name;
+ char *head_name;
+ struct object_id orig_head;
+ struct commit *onto;
+ const char *onto_name;
+ const char *revisions;
+ const char *root;
+ const char *restrict_revision;
+};
+
+static int finish_rebase(struct rebase_options *opts)
+{
+ struct strbuf dir = STRBUF_INIT;
+ const char *argv_gc_auto[] = { "gc", "--auto", NULL };
+
+ delete_ref(NULL, "REBASE_HEAD", NULL, REF_NO_DEREF);
+ apply_autostash();
+ close_all_packs(the_repository->objects);
+ /*
+* We ignore errors in 'gc --auto', since the
+* user should see them.
+*/
+ run_command_v_opt(argv_gc_auto, RUN_GIT_CMD);
+ strbuf_addstr(, opts->state_dir);
+ remove_dir_recursively(, 0);
+ strbuf_release();
+
+ return 0;
+}
+
+static struct commit *peel_committish(const char *name)
+{
+ struct object *obj;
+ struct object_id oid;
+
+ if (get_oid(name, ))
+ return NULL;
+ obj = parse_object();
+ return (struct commit *)peel_to_type(name, 0, obj, OBJ_COMMIT);
+}
+
+static void add_var(struct strbuf *buf, const char *name, const char *value)
+{
+ strbuf_addstr(buf, name);
+ if (value) {
+ strbuf_addstr(buf, "=");
+ sq_quote_buf(buf, value);
+ }
+ strbuf_addstr(buf, "; ");
+}
+
+static int run_specific_rebase(struct rebase_options *opts)
+{
+ const char *argv[] = { NULL, NULL };
+ struct strbuf script_snippet = STRBUF_INIT;
+ int status;
+ const char *backend, *backend_func;
+
+ add_var(_snippet, "GIT_DIR", absolute_path(get_git_dir()));
+ add_var(_snippet, "state_dir", opts->state_dir);
+
+ add_var(_snippet, "upstream_name", opts->upstream_name);
+ add_var(_snippet, "upstream",
+oid_to_hex(>upstream->object.oid));
+ add_var(_snippet, "head_name", opts->head_name);
+ add_var(_snippet, "orig_head", oid_to_hex(>orig_head));
+ add_var(_snippet, "onto", oid_to_hex(>onto->object.oid));
+ add_var(_snippet, "onto_name", opts->onto_name);
+ add_var(_snippet, "revisions", opts->revisions);
+ if (opts->restrict_revision == NULL)
+ add_var(_snippet, "restrict_revision", "");
+ else
+ add_var(_snippet, "restrict_revision",
+ opts->restrict_revision);
+
+ switch (opts->type) {
+ case REBASE_AM:
+ backend = "git-rebase--am";
+ backend_func = "git_rebase__am";
+ break;
+ case REBASE_INTERACTIVE:
+ backend = "git-rebase--interactive";
+ backend_func = "git_rebase__interactive";
+ break;
+ case REBASE_MERGE:
+ backend = "git-rebase--merge";
+ backend_func = "git_rebase__merge";
+
[PATCH v5 2/3] rebase: refactor common shell functions into their own file
The functions present in `git-legacy-rebase.sh` are used by the rebase
backends as they are implemented as shell script functions in the
`git-rebase--` files.
To make the `builtin/rebase.c` work, we have to provide support via
a Unix shell script snippet that uses these functions and so, we
want to use the rebase backends *directly* from the builtin rebase
without going through `git-legacy-rebase.sh`.
This commit extracts the functions to a separate file,
`git-rebase--common`, that will be read by `git-legacy-rebase.sh` and
by the shell script snippets which will be used extensively in the
following commits.
Signed-off-by: Pratik Karki
---
Unchanged since v4.
.gitignore| 1 +
Makefile | 1 +
git-legacy-rebase.sh | 69 ++-
git-rebase--common.sh | 68 ++
4 files changed, 72 insertions(+), 67 deletions(-)
create mode 100644 git-rebase--common.sh
diff --git a/.gitignore b/.gitignore
index ec23959014..824141cba1 100644
--- a/.gitignore
+++ b/.gitignore
@@ -117,6 +117,7 @@
/git-read-tree
/git-rebase
/git-rebase--am
+/git-rebase--common
/git-rebase--helper
/git-rebase--interactive
/git-rebase--merge
diff --git a/Makefile b/Makefile
index a0c410b7d6..c59e2f64a1 100644
--- a/Makefile
+++ b/Makefile
@@ -619,6 +619,7 @@ SCRIPT_SH += git-web--browse.sh
SCRIPT_LIB += git-mergetool--lib
SCRIPT_LIB += git-parse-remote
SCRIPT_LIB += git-rebase--am
+SCRIPT_LIB += git-rebase--common
SCRIPT_LIB += git-rebase--interactive
SCRIPT_LIB += git-rebase--preserve-merges
SCRIPT_LIB += git-rebase--merge
diff --git a/git-legacy-rebase.sh b/git-legacy-rebase.sh
index 7973447645..af2cdfef03 100755
--- a/git-legacy-rebase.sh
+++ b/git-legacy-rebase.sh
@@ -57,12 +57,7 @@ cd_to_toplevel
LF='
'
ok_to_skip_pre_rebase=
-resolvemsg="
-$(gettext 'Resolve all conflicts manually, mark them as resolved with
-"git add/rm ", then run "git rebase --continue".
-You can instead skip this commit: run "git rebase --skip".
-To abort and get back to the state before "git rebase", run "git rebase
--abort".')
-"
+
squash_onto=
unset onto
unset restrict_revision
@@ -102,6 +97,7 @@ case "$(git config --bool commit.gpgsign)" in
true) gpg_sign_opt=-S ;;
*) gpg_sign_opt= ;;
esac
+. git-rebase--common
read_basic_state () {
test -f "$state_dir/head-name" &&
@@ -132,67 +128,6 @@ read_basic_state () {
}
}
-write_basic_state () {
- echo "$head_name" > "$state_dir"/head-name &&
- echo "$onto" > "$state_dir"/onto &&
- echo "$orig_head" > "$state_dir"/orig-head &&
- echo "$GIT_QUIET" > "$state_dir"/quiet &&
- test t = "$verbose" && : > "$state_dir"/verbose
- test -n "$strategy" && echo "$strategy" > "$state_dir"/strategy
- test -n "$strategy_opts" && echo "$strategy_opts" > \
- "$state_dir"/strategy_opts
- test -n "$allow_rerere_autoupdate" && echo "$allow_rerere_autoupdate" >
\
- "$state_dir"/allow_rerere_autoupdate
- test -n "$gpg_sign_opt" && echo "$gpg_sign_opt" >
"$state_dir"/gpg_sign_opt
- test -n "$signoff" && echo "$signoff" >"$state_dir"/signoff
-}
-
-output () {
- case "$verbose" in
- '')
- output=$("$@" 2>&1 )
- status=$?
- test $status != 0 && printf "%s\n" "$output"
- return $status
- ;;
- *)
- "$@"
- ;;
- esac
-}
-
-move_to_original_branch () {
- case "$head_name" in
- refs/*)
- message="rebase finished: $head_name onto $onto"
- git update-ref -m "$message" \
- $head_name $(git rev-parse HEAD) $orig_head &&
- git symbolic-ref \
- -m "rebase finished: returning to $head_name" \
- HEAD $head_name ||
- die "$(eval_gettext "Could not move back to \$head_name")"
- ;;
- esac
-}
-
-apply_autostash () {
- if test -f "$state_dir/autostash"
- then
- stash_sha1=$(cat "$state_dir/autostash")
- if git stash apply $stash_sha1 >/dev/null 2>&1
- then
- echo "$(gettext 'Applied autostash.')" >&2
- else
- git stash store -m "autostash" -q $stash_sha1 ||
- die "$(eval_gettext "Cannot store \$stash_sha1")"
- gettext 'Applying autostash resulted in conflicts.
-Your changes are safe in the stash.
-You can run "git stash pop" or "git stash drop" at any time.
-' >&2
- fi
- fi
-}
-
finish_rebase () {
rm -f "$(git rev-parse --git-path REBASE_HEAD)"
apply_autostash &&
diff --git a/git-rebase--common.sh b/git-rebase--common.sh
new file mode 100644
index 00..7e39d22871
--- /dev/null
+++ b/git-rebase--common.sh
@@ -0,0 +1,68 @@
+
+resolvemsg="
+$(gettext 'Resolve all
[PATCH v5 1/3] rebase: start implementing it as a builtin
This commit imitates the strategy that was used to convert the
difftool to a builtin. We start by renaming the shell script
`git-rebase.sh` to `git-legacy-rebase.sh` and introduce a
`builtin/rebase.c` that simply executes the shell script version,
unless the config setting `rebase.useBuiltin` is set to `true`.
The motivation behind this is to rewrite all the functionality of the
shell script version in the aforementioned `rebase.c`, one by one and
be able to conveniently test new features by configuring
`rebase.useBuiltin`.
In the original difftool conversion, if sane_execvp() that attempts to
run the legacy scripted version returned with non-negative status, the
command silently exited without doing anything with success, but
sane_execvp() should not return with non-negative status in the first
place, so we use die() to notice such an abnormal case.
We intentionally avoid reading the config directly to avoid
messing up the GIT_* environment variables when we need to fall back to
exec()ing the shell script. The test of builtin rebase can be done by
`git -c rebase.useBuiltin=true rebase ...`
Signed-off-by: Pratik Karki
---
.gitignore| 1 +
Makefile | 3 +-
builtin.h | 1 +
builtin/rebase.c | 58 +++
git-rebase.sh => git-legacy-rebase.sh | 0
git.c | 6 +++
6 files changed, 68 insertions(+), 1 deletion(-)
create mode 100644 builtin/rebase.c
rename git-rebase.sh => git-legacy-rebase.sh (100%)
diff --git a/.gitignore b/.gitignore
index 3284a1e9b1..ec23959014 100644
--- a/.gitignore
+++ b/.gitignore
@@ -78,6 +78,7 @@
/git-init-db
/git-interpret-trailers
/git-instaweb
+/git-legacy-rebase
/git-log
/git-ls-files
/git-ls-remote
diff --git a/Makefile b/Makefile
index 08e5c54549..a0c410b7d6 100644
--- a/Makefile
+++ b/Makefile
@@ -609,7 +609,7 @@ SCRIPT_SH += git-merge-one-file.sh
SCRIPT_SH += git-merge-resolve.sh
SCRIPT_SH += git-mergetool.sh
SCRIPT_SH += git-quiltimport.sh
-SCRIPT_SH += git-rebase.sh
+SCRIPT_SH += git-legacy-rebase.sh
SCRIPT_SH += git-remote-testgit.sh
SCRIPT_SH += git-request-pull.sh
SCRIPT_SH += git-stash.sh
@@ -1059,6 +1059,7 @@ BUILTIN_OBJS += builtin/prune.o
BUILTIN_OBJS += builtin/pull.o
BUILTIN_OBJS += builtin/push.o
BUILTIN_OBJS += builtin/read-tree.o
+BUILTIN_OBJS += builtin/rebase.o
BUILTIN_OBJS += builtin/rebase--helper.o
BUILTIN_OBJS += builtin/receive-pack.o
BUILTIN_OBJS += builtin/reflog.o
diff --git a/builtin.h b/builtin.h
index 0362f1ce25..44651a447f 100644
--- a/builtin.h
+++ b/builtin.h
@@ -202,6 +202,7 @@ 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_read_tree(int argc, const char **argv, const char *prefix);
+extern int cmd_rebase(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);
extern int cmd_reflog(int argc, const char **argv, const char *prefix);
diff --git a/builtin/rebase.c b/builtin/rebase.c
new file mode 100644
index 00..c44addb2a4
--- /dev/null
+++ b/builtin/rebase.c
@@ -0,0 +1,58 @@
+/*
+ * "git rebase" builtin command
+ *
+ * Copyright (c) 2018 Pratik Karki
+ */
+
+#include "builtin.h"
+#include "run-command.h"
+#include "exec-cmd.h"
+#include "argv-array.h"
+#include "dir.h"
+
+static int use_builtin_rebase(void)
+{
+ struct child_process cp = CHILD_PROCESS_INIT;
+ struct strbuf out = STRBUF_INIT;
+ int ret;
+
+ argv_array_pushl(,
+"config", "--bool", "rebase.usebuiltin", NULL);
+ cp.git_cmd = 1;
+ if (capture_command(, , 6)) {
+ strbuf_release();
+ return 0;
+ }
+
+ strbuf_trim();
+ ret = !strcmp("true", out.buf);
+ strbuf_release();
+ return ret;
+}
+
+int cmd_rebase(int argc, const char **argv, const char *prefix)
+{
+ /*
+* NEEDSWORK: Once the builtin rebase has been tested enough
+* and git-legacy-rebase.sh is retired to contrib/, this preamble
+* can be removed.
+*/
+
+ if (!use_builtin_rebase()) {
+ const char *path = mkpath("%s/git-legacy-rebase",
+ git_exec_path());
+
+ if (sane_execvp(path, (char **)argv) < 0)
+ die_errno(_("could not exec %s"), path);
+ else
+ BUG("sane_execvp() returned???");
+ }
+
+ if (argc != 2)
+ die(_("Usage: %s "), argv[0]);
+ prefix = setup_git_directory();
+ trace_repo_setup(prefix);
+ setup_work_tree();
+
+ die("TODO");
+}
diff --git a/git-rebase.sh
[GSoC] [PATCH v5 0/3] rebase: rewrite rebase in C
As a GSoC project, I have been working on the builtin rebase. The motivation behind the rewrite of rebase i.e. from shell script to C are for following reasons: 1. Writing shell scripts and getting it to production is much faster than doing the equivalent in C but lacks in performance and extra workarounds are needed for non-POSIX platforms. 2. Git for Windows is at loss as the installer size increases due to addition of extra dependencies for the shell scripts which are usually available in POSIX compliant platforms. This series of patches serves to demonstrate a minimal builtin rebase which supports running `git rebase ` and also serves to ask for reviews. Changes since v4: - Remove the `do_reset()` refactored function from sequencer. In other words `sequencer: refactor the code to detach HEAD to checkout.c` patch was dropped to introduce a new function `reset_hard()` for `rebase.c` (as suggested by Johannes). - Fix a case of leak in `rebase: start implementing it as a builtin`. (as pointed out by Andrei Rybak and Eric Sunshine). - Wrap the user visible comments in `_()` and used `BUG()` depending on the scenarios (as pointed out by Duy Nguyen). - Fix the macro `GIT_PATH_FUNC` which expands to function definition and doesn't require semicolons (as pointed out by Beat Bolli). Pratik Karki (3): rebase: start implementing it as a builtin rebase: refactor common shell functions into their own file builtin/rebase: support running "git rebase " .gitignore| 2 + Makefile | 4 +- builtin.h | 1 + builtin/rebase.c | 406 ++ git-rebase.sh => git-legacy-rebase.sh | 69 + git-rebase--common.sh | 68 + git.c | 6 + 7 files changed, 488 insertions(+), 68 deletions(-) create mode 100644 builtin/rebase.c rename git-rebase.sh => git-legacy-rebase.sh (90%) create mode 100644 git-rebase--common.sh -- 2.18.0
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_list_clear(list, 1);
> > + strbuf_release();
> > + strbuf_release();
> > + fclose(in);
> > + finish_command();
> > + return -1;
> > + }
> > + util->matching = -1;
> > + in_header = 1;
> > + continue;
> > + }
> > +
> > + if (starts_with(line.buf, "diff --git")) {
> > + in_header = 0;
> > + strbuf_addch(, '\n');
> > + if (!util->diff_offset)
> > + util->diff_offset = buf.len;
> > + strbuf_addbuf(, );
> > + } else if (in_header) {
> > + if (starts_with(line.buf, "Author: ")) {
> > + strbuf_addbuf(, );
> > +
[PATCH/RFC] Color merge conflicts
One of the things I notice when watching a normal git user face a
merge conflicts is the output is very verbose (especially when there
are multiple conflicts) and it's hard to spot the important parts to
start resolving conflicts unless you know what to look for.
This is the start to hopefully help that a bit. One thing I'm not sure
is what to color because that affects the config keys we use for
this. If we have to color different things, it's best to go
"color.merge." but if this is the only place to color, that slot
thing looks over-engineered.
Perhaps another piece to color is the conflicted path? Maybe. But on
the other hand, I don't think we want a chameleon output, just enough
visual cues to go from one conflict to the next...
Signed-off-by: Nguyễn Thái Ngọc Duy
---
merge-recursive.c | 133 +++---
2 files changed, 79 insertions(+), 55 deletions(-)
diff --git a/merge-recursive.c b/merge-recursive.c
index 113c1d6962..b800101538 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -26,6 +26,7 @@
#include "dir.h"
#include "submodule.h"
#include "revision.h"
+#include "color.h"
struct path_hashmap_entry {
struct hashmap_entry e;
@@ -286,6 +287,28 @@ static void output(struct merge_options *o, int v, const
char *fmt, ...)
flush_output(o);
}
+__attribute__((format (printf, 3, 4)))
+static void conflict_output(struct merge_options *o, int v, const char *fmt,
...)
+{
+ va_list ap;
+
+ if (!show(o, v))
+ return;
+
+ strbuf_addchars(>obuf, ' ', o->call_depth * 2);
+
+ strbuf_addf(>obuf, "%s%s%s ",
+ GIT_COLOR_RED, _("CONFLICT"), GIT_COLOR_RESET);
+
+ va_start(ap, fmt);
+ strbuf_vaddf(>obuf, fmt, ap);
+ va_end(ap);
+
+ strbuf_addch(>obuf, '\n');
+ if (!o->buffer_output)
+ flush_output(o);
+}
+
static void output_commit_title(struct merge_options *o, struct commit *commit)
{
struct merge_remote_desc *desc;
@@ -1497,27 +1520,27 @@ static int handle_change_delete(struct merge_options *o,
*/
if (!alt_path) {
if (!old_path) {
- output(o, 1, _("CONFLICT (%s/delete): %s
deleted in %s "
- "and %s in %s. Version %s of %s left in
tree."),
- change, path, delete_branch, change_past,
- change_branch, change_branch, path);
+ conflict_output(o, 1, _("(%s/delete): %s
deleted in %s "
+ "and %s in %s. Version
%s of %s left in tree."),
+ change, path, delete_branch,
change_past,
+ change_branch, change_branch,
path);
} else {
- output(o, 1, _("CONFLICT (%s/delete): %s
deleted in %s "
- "and %s to %s in %s. Version %s of %s
left in tree."),
- change, old_path, delete_branch,
change_past, path,
- change_branch, change_branch, path);
+ conflict_output(o, 1, _("(%s/delete): %s
deleted in %s "
+ "and %s to %s in %s.
Version %s of %s left in tree."),
+ change, old_path,
delete_branch, change_past, path,
+ change_branch, change_branch,
path);
}
} else {
if (!old_path) {
- output(o, 1, _("CONFLICT (%s/delete): %s
deleted in %s "
- "and %s in %s. Version %s of %s left in
tree at %s."),
- change, path, delete_branch, change_past,
- change_branch, change_branch, path,
alt_path);
+ conflict_output(o, 1, _("(%s/delete): %s
deleted in %s "
+ "and %s in %s. Version
%s of %s left in tree at %s."),
+ change, path, delete_branch,
change_past,
+ change_branch, change_branch,
path, alt_path);
} else {
- output(o, 1, _("CONFLICT (%s/delete): %s
deleted in %s "
- "and %s to %s in %s. Version %s of %s
left in tree at %s."),
- change, old_path, delete_branch,
change_past, path,
- change_branch, change_branch, path,
alt_path);
+ conflict_output(o, 1, _("(%s/delete): %s
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: Strange bug with "git log" - pdftotext?
I discovered it was an issue with the version of Git for Windows I was using. Upgraded to the latest version and it works now. -- Best regards, Jeremy Morton (Jez) On 30/07/2018 16:37, Jeff King wrote: On Mon, Jul 30, 2018 at 01:49:46PM +0100, Jeremy Morton wrote: I'm trying to search my git log history for a particular term - "unobtrusive" - so I run this command: git log -S unobtrusive --oneline When I do this, this is displayed and I'm in an interactive less terminal or something: pdftotext version 4.00 [...] That's definitely weird. My guess is that the repository has some .gitattributes set up to diff pdf files in a particular way, and you have some matching config that tries to call pdftotext. What does: git config --list | grep ^diff say? I'd expect to see an external or textconv option there running pdftotext. Another option is that your pager is somehow set up to call pdftotext, but that seems much more nonsensical to use the tool there (but try "git var GIT_PAGER" and "git config pager.log" to check). -Peff
Re: Strange bug with "git log" - pdftotext?
Am 30.07.2018 um 14:49 schrieb Jeremy Morton:
> I'm trying to search my git log history for a particular term - "unobtrusive"
> - so I run this command:
>
> git log -S unobtrusive --oneline
>
> When I do this, this is displayed and I'm in an interactive less terminal or
> something:
>
> pdftotext version 4.00
> Copyright 1996-2017 Glyph & Cog, LLC
> Usage: pdftotext [options] []
> -f : first page to convert
> -l : last page to convert
> -layout : maintain original physical layout
> -simple : simple one-column page layout
> -table : similar to -layout, but optimized for tables
> -lineprinter : use strict fixed-pitch/height layout
> -raw : keep strings in content stream order
> -fixed : assume fixed-pitch (or tabular) text
> -linespacing : fixed line spacing for LinePrinter mode
> -clip : separate clipped text
> -nodiag : discard diagonal text
> -enc : output text encoding name
> -eol : output end-of-line convention (unix, dos, or mac)
> -nopgbrk : don't insert page breaks between pages
> -bom : insert a Unicode BOM at the start of the text file
> -opw : owner password (for encrypted files)
> -upw : user password (for encrypted files)
> -q : don't print any messages or errors
> -cfg : configuration file to use in place of .xpdfrc
> -v : print copyright and version info
> :
>
> When I hit the down arrow, it scrolls down, repeating this output infinitely
> until I hit 'q'. What is going on here??
>
Wild guess: You're using "Git for Windows" on a version less than
"2.18.0.windows.1" ?
Try upgrading (at least) to that version. IIRC that's one of the issues it
fixes.
HTH
Stefan
--
/dev/random says: Never underestimate the power of human stupidity.
python -c "print
'73746566616e2e6e616577654061746c61732d656c656b74726f6e696b2e636f6d'.decode('hex')"
GPG Key fingerprint = 2DF5 E01B 09C3 7501 BCA9 9666 829B 49C5 9221 27AF
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
