Re: diff-highlight highlight words?

[Jeff King]

[+cc git@vger, since this may be of interest to others]

On Tue, Nov 11, 2014 at 02:40:59PM -0800, Scott Baker wrote:

> I'd like to recreate the github style diffs on the command line. It
> appears that your diff-highlight is very close. The current version only
> allows you to "invert the colors" which isn't ideal.

Yes, I never built any configurability into the script. However, you can
tweak the definitions at the top to get different effects.
Traditionally, ANSI colors on the terminal only came in two flavors:
"normal" and "bright" (which is attached to the "bold" attribute").
Instead of reversing video, you can switch on brightness like this:

diff --git a/contrib/diff-highlight/diff-highlight 
b/contrib/diff-highlight/diff-highlight
index c4404d4..c99de99 100755
--- a/contrib/diff-highlight/diff-highlight
+++ b/contrib/diff-highlight/diff-highlight
@@ -5,8 +5,8 @@ use strict;
 
 # Highlight by reversing foreground and background. You could do
 # other things like bold or underline if you prefer.
-my $HIGHLIGHT   = "\x1b[7m";
-my $UNHIGHLIGHT = "\x1b[27m";
+my $HIGHLIGHT   = "\x1b[1m";
+my $UNHIGHLIGHT = "\x1b[22m";
 my $COLOR = qr/\x1b\[[0-9;]*m/;
 my $BORING = qr/$COLOR|\s/;
 

However on most modern terminals the color difference between bright and
normal is very subtle, and this doesn't look good.

XTerm (and other modern terminals) has 256-color support, so you could
do better there (assuming your terminal supports it). The current code
builds on the existing colors produced by git (because the operations
are only "invert colors" and "uninvert colors"). Doing anything fancier
requires handling add/del differently.  That patch might look something
like this (which uses dark red/green for most of the line, and a much
brighter variant for the highlighted text):

diff --git a/contrib/diff-highlight/diff-highlight 
b/contrib/diff-highlight/diff-highlight
index c4404d4..4e08f3c 100755
--- a/contrib/diff-highlight/diff-highlight
+++ b/contrib/diff-highlight/diff-highlight
@@ -5,11 +5,16 @@ use strict;
 
 # Highlight by reversing foreground and background. You could do
 # other things like bold or underline if you prefer.
-my $HIGHLIGHT   = "\x1b[7m";
-my $UNHIGHLIGHT = "\x1b[27m";
 my $COLOR = qr/\x1b\[[0-9;]*m/;
 my $BORING = qr/$COLOR|\s/;
 
+# Elements:
+# 0 - highlighted text
+# 1 - unhighlighted text
+# 2 - reset to normal
+my @ADD_HIGHLIGHT = ("\x1b[38;2;100;255;100m", "\x1b[38;2;0;255;0m", 
"\x1b[30m");
+my @DEL_HIGHLIGHT = ("\x1b[38;2;255;100;100m", "\x1b[38;2;255;0;0m", 
"\x1b[30m");
+
 my @removed;
 my @added;
 my $in_hunk;
@@ -128,8 +133,8 @@ sub highlight_pair {
}
 
if (is_pair_interesting(\@a, $pa, $sa, \@b, $pb, $sb)) {
-   return highlight_line(\@a, $pa, $sa),
-  highlight_line(\@b, $pb, $sb);
+   return highlight_line(\@a, $pa, $sa, @DEL_HIGHLIGHT),
+  highlight_line(\@b, $pb, $sb, @ADD_HIGHLIGHT);
}
else {
return join('', @a),
@@ -144,15 +149,18 @@ sub split_line {
 }
 
 sub highlight_line {
-   my ($line, $prefix, $suffix) = @_;
+   my ($line, $prefix, $suffix, $highlight, $unhighlight, $reset) = @_;
 
-   return join('',
+   my $r = join('',
+   $unhighlight,
@{$line}[0..($prefix-1)],
-   $HIGHLIGHT,
+   $highlight,
@{$line}[$prefix..$suffix],
-   $UNHIGHLIGHT,
-   @{$line}[($suffix+1)..$#$line]
+   $unhighlight,
+   @{$line}[($suffix+1)..$#$line],
);
+   $r =~ s/\n$/$reset$&/;
+   return $r;
 }
 
 # Pairs are interesting to highlight only if we are going to end up


The result does not look terrible to me, though I think I find the
reverse-video more obvious when scanning the diff. To look more like
GitHub's view, you could instead set the background by doing this on
top:

diff --git a/contrib/diff-highlight/diff-highlight 
b/contrib/diff-highlight/diff-highlight
index 4e08f3c..6f98db4 100755
--- a/contrib/diff-highlight/diff-highlight
+++ b/contrib/diff-highlight/diff-highlight
@@ -12,8 +12,8 @@ my $BORING = qr/$COLOR|\s/;
 # 0 - highlighted text
 # 1 - unhighlighted text
 # 2 - reset to normal
-my @ADD_HIGHLIGHT = ("\x1b[38;2;100;255;100m", "\x1b[38;2;0;255;0m", 
"\x1b[30m");
-my @DEL_HIGHLIGHT = ("\x1b[38;2;255;100;100m", "\x1b[38;2;255;0;0m", 
"\x1b[30m");
+my @ADD_HIGHLIGHT = ("\x1b[30;48;2;100;255;100m", "\x1b[30;48;2;0;255;0m", 
"\x1b[0m");
+my @DEL_HIGHLIGHT = ("\x1b[30;48;2;255;100;100m", "\x1b[30;48;2;255;0;0m", 
"\x1b[0m");
 
 my @removed;
 my @added;
@@ -151,14 +151,18 @@ sub split_line {
 sub highlight_line {
my ($line, $prefix, $suffix, $highlight, $unhighlight, $reset) = @_;
 
+   # strip out existing colors from git, which will clash
+   # both due to contrast and because of random ANSI resets
+   # inside the content
+   my $p = join('', @{$line}[0..($prefix-1)]);
+   my $t = join(''

Kedves: Webmail Előfizető

[webmail administrator®2014]

-- 
A postaláda korlátozza, kérjük, kattintson ide prekrocila

 http://updattw2qq.jigsy.com

Ellen?rizze a elektronikus levél köszönet

Rendszergazda E-mail System.
Köszönjük az együttm?ködést!
Levél a Web Team @ 2014

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv5 9/9] t3301: Modernize

[Eric Sunshine]

On Tue, Nov 11, 2014 at 7:40 PM, Johan Herland  wrote:
> Make this test script appear somewhat less old-fashioned:
> Signed-off-by: Johan Herland 
> ---
> diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
> index 416ed9e..861c159 100755
> --- a/t/t3301-notes.sh
> +++ b/t/t3301-notes.sh
> @@ -50,206 +44,186 @@ test_expect_success 'handle empty notes gracefully' '
>  '
>
>  test_expect_success 'create notes' '
> -   git config core.notesRef refs/notes/commits &&
> MSG=b4 git notes add &&
> -   test ! -f .git/NOTES_EDITMSG &&
> -   test 1 = $(git ls-tree refs/notes/commits | wc -l) &&
> -   test b4 = $(git notes show) &&
> +   test_path_is_missing .git/NOTES_EDITMSG &&
> +   git ls-tree -r refs/notes/commits >actual &&
> +   test_line_count = 1 actual

Broken &&-chain. This problem is repeated each place use invoke
test_line_count().

> +   test "b4" = "$(git notes show)" &&
> git show HEAD^ &&
> test_must_fail git notes show HEAD^
>  '
>
>  test_expect_success 'show notes entry with %N' '
> -   for l in A b4 B
> -   do
> -   echo "$l"
> -   done >expect &&
> -   git show -s --format='A%n%NB' >output &&
> -   test_cmp expect output
> +   test_write_lines A b4 B >expect &&
> +   git show -s --format="A%n%NB" >actual &&
> +   test_cmp expect actual
>  '
>
> -cat >expect < -d423f8c refs/notes/commits@{0}: notes: Notes added by 'git notes add'
> -EOF
> -
>  test_expect_success 'create reflog entry' '
> -   git reflog show refs/notes/commits >output &&
> -   test_cmp expect output
> +   cat <<-EOF >expect &&
> +   a1d8fa6 refs/notes/commits@{0}: notes: Notes added by 
> '"'"'git notes add'"'"'
> +   EOF
> +   git reflog show refs/notes/commits >actual &&
> +   test_cmp expect actual
>  '
>
>  test_expect_success 'edit existing notes' '
> MSG=b3 git notes edit &&
> -   test ! -f .git/NOTES_EDITMSG &&
> -   test 1 = $(git ls-tree refs/notes/commits | wc -l) &&
> -   test b3 = $(git notes show) &&
> +   test_path_is_missing .git/NOTES_EDITMSG &&
> +   git ls-tree -r refs/notes/commits >actual &&
> +   test_line_count = 1 actual
> +   test "b3" = "$(git notes show)" &&
> git show HEAD^ &&
> test_must_fail git notes show HEAD^
>  '
>
>  test_expect_success 'cannot "git notes add -m" where notes already exists' '
> test_must_fail git notes add -m "b2" &&
> -   test ! -f .git/NOTES_EDITMSG &&
> -   test 1 = $(git ls-tree refs/notes/commits | wc -l) &&
> -   test b3 = $(git notes show) &&
> +   test_path_is_missing .git/NOTES_EDITMSG &&
> +   git ls-tree -r refs/notes/commits >actual &&
> +   test_line_count = 1 actual
> +   test "b3" = "$(git notes show)" &&
> git show HEAD^ &&
> test_must_fail git notes show HEAD^
>  '
>
>  test_expect_success 'can overwrite existing note with "git notes add -f -m"' 
> '
> git notes add -f -m "b1" &&
> -   test ! -f .git/NOTES_EDITMSG &&
> -   test 1 = $(git ls-tree refs/notes/commits | wc -l) &&
> -   test b1 = $(git notes show) &&
> +   test_path_is_missing .git/NOTES_EDITMSG &&
> +   git ls-tree -r refs/notes/commits >actual &&
> +   test_line_count = 1 actual
> +   test "b1" = "$(git notes show)" &&
> git show HEAD^ &&
> test_must_fail git notes show HEAD^
>  '
>
>  test_expect_success 'add w/no options on existing note morphs into edit' '
> MSG=b2 git notes add &&
> -   test ! -f .git/NOTES_EDITMSG &&
> -   test 1 = $(git ls-tree refs/notes/commits | wc -l) &&
> -   test b2 = $(git notes show) &&
> +   test_path_is_missing .git/NOTES_EDITMSG &&
> +   git ls-tree -r refs/notes/commits >actual &&
> +   test_line_count = 1 actual
> +   test "b2" = "$(git notes show)" &&
> git show HEAD^ &&
> test_must_fail git notes show HEAD^
>  '
>
>  test_expect_success 'can overwrite existing note with "git notes add -f"' '
> MSG=b1 git notes add -f &&
> -   test ! -f .git/NOTES_EDITMSG &&
> -   test 1 = $(git ls-tree refs/notes/commits | wc -l) &&
> -   test b1 = $(git notes show) &&
> +   test_path_is_missing .git/NOTES_EDITMSG &&
> +   git ls-tree -r refs/notes/commits >actual &&
> +   test_line_count = 1 actual
> +   test "b1" = "$(git notes show)" &&
> git show HEAD^ &&
> test_must_fail git notes show HEAD^
>  '
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv5 9/9] t3301: Modernize

[Johan Herland]

Make this test script appear somewhat less old-fashioned:
 - Use test helper functions:
- write_script
- test_commit
- test_write_lines
- test_line_count
- test_config
- test_unconfig
- test_path_is_missing
 - Remove whitespace between redirection operators and their targets.
 - Move preparation of "expect" files into tests.
 - Rename "output" files to "actual".
 - More consistent quoting, especially around commands that might
   expand to nothing.
 - More visibility of important whitespace with ${indent}.
 - Combine pairs of tests that unnecessarily split setup and verification.

Improved-by: Eric Sunshine 
Improved-by: Junio C Hamano 
Improved-by: Michael Blume 
Improved-by: Jeff King 
Signed-off-by: Johan Herland 
---
 t/t3301-notes.sh | 1300 +-
 1 file changed, 601 insertions(+), 699 deletions(-)

diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
index 416ed9e..861c159 100755
--- a/t/t3301-notes.sh
+++ b/t/t3301-notes.sh
@@ -7,28 +7,22 @@ test_description='Test commit notes'
 
 . ./test-lib.sh
 
-cat > fake_editor.sh << \EOF
-#!/bin/sh
-echo "$MSG" > "$1"
-echo "$MSG" >& 2
+write_script fake_editor <<\EOF
+echo "$MSG" >"$1"
+echo "$MSG" >&2
 EOF
-chmod a+x fake_editor.sh
-GIT_EDITOR=./fake_editor.sh
+GIT_EDITOR=./fake_editor
 export GIT_EDITOR
 
+indent=""
+
 test_expect_success 'cannot annotate non-existing HEAD' '
test_must_fail env MSG=3 git notes add
 '
 
-test_expect_success setup '
-   : > a1 &&
-   git add a1 &&
-   test_tick &&
-   git commit -m 1st &&
-   : > a2 &&
-   git add a2 &&
-   test_tick &&
-   git commit -m 2nd
+test_expect_success 'setup' '
+   test_commit 1st &&
+   test_commit 2nd
 '
 
 test_expect_success 'need valid notes ref' '
@@ -50,206 +44,186 @@ test_expect_success 'handle empty notes gracefully' '
 '
 
 test_expect_success 'show non-existent notes entry with %N' '
-   for l in A B
-   do
-   echo "$l"
-   done >expect &&
-   git show -s --format='A%n%NB' >output &&
-   test_cmp expect output
+   test_write_lines A B >expect &&
+   git show -s --format="A%n%NB" >actual &&
+   test_cmp expect actual
 '
 
 test_expect_success 'create notes' '
-   git config core.notesRef refs/notes/commits &&
MSG=b4 git notes add &&
-   test ! -f .git/NOTES_EDITMSG &&
-   test 1 = $(git ls-tree refs/notes/commits | wc -l) &&
-   test b4 = $(git notes show) &&
+   test_path_is_missing .git/NOTES_EDITMSG &&
+   git ls-tree -r refs/notes/commits >actual &&
+   test_line_count = 1 actual
+   test "b4" = "$(git notes show)" &&
git show HEAD^ &&
test_must_fail git notes show HEAD^
 '
 
 test_expect_success 'show notes entry with %N' '
-   for l in A b4 B
-   do
-   echo "$l"
-   done >expect &&
-   git show -s --format='A%n%NB' >output &&
-   test_cmp expect output
+   test_write_lines A b4 B >expect &&
+   git show -s --format="A%n%NB" >actual &&
+   test_cmp expect actual
 '
 
-cat >expect 
expect &&
+   a1d8fa6 refs/notes/commits@{0}: notes: Notes added by '"'"'git 
notes add'"'"'
+   EOF
+   git reflog show refs/notes/commits >actual &&
+   test_cmp expect actual
 '
 
 test_expect_success 'edit existing notes' '
MSG=b3 git notes edit &&
-   test ! -f .git/NOTES_EDITMSG &&
-   test 1 = $(git ls-tree refs/notes/commits | wc -l) &&
-   test b3 = $(git notes show) &&
+   test_path_is_missing .git/NOTES_EDITMSG &&
+   git ls-tree -r refs/notes/commits >actual &&
+   test_line_count = 1 actual
+   test "b3" = "$(git notes show)" &&
git show HEAD^ &&
test_must_fail git notes show HEAD^
 '
 
 test_expect_success 'cannot "git notes add -m" where notes already exists' '
test_must_fail git notes add -m "b2" &&
-   test ! -f .git/NOTES_EDITMSG &&
-   test 1 = $(git ls-tree refs/notes/commits | wc -l) &&
-   test b3 = $(git notes show) &&
+   test_path_is_missing .git/NOTES_EDITMSG &&
+   git ls-tree -r refs/notes/commits >actual &&
+   test_line_count = 1 actual
+   test "b3" = "$(git notes show)" &&
git show HEAD^ &&
test_must_fail git notes show HEAD^
 '
 
 test_expect_success 'can overwrite existing note with "git notes add -f -m"' '
git notes add -f -m "b1" &&
-   test ! -f .git/NOTES_EDITMSG &&
-   test 1 = $(git ls-tree refs/notes/commits | wc -l) &&
-   test b1 = $(git notes show) &&
+   test_path_is_missing .git/NOTES_EDITMSG &&
+   git ls-tree -r refs/notes/commits >actual &&
+   test_line_count = 1 actual
+   test "b1" = "$(git notes show)" &&
git show HEAD^ &&
test_must_fail git notes show HEAD^
 '
 
 test_expect_success 'add w/no options on existing note morphs into edit' '
MSG=b2

[PATCHv5 2/9] t3301: Verify that 'git notes' removes empty notes by default

[Johan Herland]

Add test cases documenting the current behavior when trying to
add/append/edit empty notes. This is in preparation for adding
--allow-empty; to allow empty notes to be stored.

Improved-by: Eric Sunshine 
Improved-by: Junio C Hamano 
Signed-off-by: Johan Herland 
---
 t/t3301-notes.sh | 27 +++
 1 file changed, 27 insertions(+)

diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
index cfd67ff..f74b3fa 100755
--- a/t/t3301-notes.sh
+++ b/t/t3301-notes.sh
@@ -1239,4 +1239,31 @@ test_expect_success 'git notes get-ref (--ref)' '
test "$(GIT_NOTES_REF=refs/notes/bar git notes --ref=baz get-ref)" = 
"refs/notes/baz"
 '
 
+test_expect_success 'setup testing of empty notes' '
+   test_unconfig core.notesRef &&
+   test_commit 16th &&
+   empty_blob=$(git hash-object -w /dev/null)
+'
+
+while read cmd
+do
+   test_expect_success "'git notes $cmd' removes empty note" "
+   test_might_fail git notes remove HEAD &&
+   MSG= git notes $cmd &&
+   test_must_fail git notes list HEAD
+   "
+done <<\EOF
+add
+add -F /dev/null
+add -m ""
+add -c "$empty_blob"
+add -C "$empty_blob"
+append
+append -F /dev/null
+append -m ""
+append -c "$empty_blob"
+append -C "$empty_blob"
+edit
+EOF
+
 test_done
-- 
2.0.0.rc4.501.gdaf83ca

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv5 7/9] builtin/notes: Add --allow-empty, to allow storing empty notes

[Johan Herland]

Although the "git notes" man page advertises that we support binary-safe
notes addition (using the -C option), we currently do not support adding
the empty note (i.e. using the empty blob to annotate an object). Instead,
an empty note is always treated as an intent to remove the note
altogether.

Introduce the --allow-empty option to the add/append/edit subcommands,
to explicitly allow an empty note to be stored into the notes tree.

Also update the documentation, and add test cases for the new option.

Reported-by: James H. Fisher 
Improved-by: Kyle J. McKay 
Improved-by: Junio C Hamano 
Signed-off-by: Johan Herland 
---
 Documentation/git-notes.txt | 12 
 builtin/notes.c | 17 +++--
 t/t3301-notes.sh| 10 +-
 3 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
index 310f0a5..851518d 100644
--- a/Documentation/git-notes.txt
+++ b/Documentation/git-notes.txt
@@ -9,10 +9,10 @@ SYNOPSIS
 
 [verse]
 'git notes' [list []]
-'git notes' add [-f] [-F  | -m  | (-c | -C) ] []
+'git notes' add [-f] [--allow-empty] [-F  | -m  | (-c | -C) 
] []
 'git notes' copy [-f] ( --stdin |   )
-'git notes' append [-F  | -m  | (-c | -C) ] []
-'git notes' edit []
+'git notes' append [--allow-empty] [-F  | -m  | (-c | -C) ] 
[]
+'git notes' edit [--allow-empty] []
 'git notes' show []
 'git notes' merge [-v | -q] [-s  ] 
 'git notes' merge --commit [-v | -q]
@@ -155,6 +155,10 @@ OPTIONS
Like '-C', but with '-c' the editor is invoked, so that
the user can further edit the note message.
 
+--allow-empty::
+   Allow an empty note object to be stored. The default behavior is
+   to automatically remove empty notes.
+
 --ref ::
Manipulate the notes tree in .  This overrides
'GIT_NOTES_REF' and the "core.notesRef" configuration.  The ref
@@ -287,7 +291,7 @@ arbitrary files using 'git hash-object':
 
 $ cc *.c
 $ blob=$(git hash-object -w a.out)
-$ git notes --ref=built add -C "$blob" HEAD
+$ git notes --ref=built add --allow-empty -C "$blob" HEAD
 
 
 (You cannot simply use `git notes --ref=built add -F a.out HEAD`
diff --git a/builtin/notes.c b/builtin/notes.c
index ed32d63..a9f37d0 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -22,10 +22,10 @@
 
 static const char * const git_notes_usage[] = {
N_("git notes [--ref ] [list []]"),
-   N_("git notes [--ref ] add [-f] [-m  | -F  | (-c 
| -C) ] []"),
+   N_("git notes [--ref ] add [-f] [--allow-empty] [-m  | 
-F  | (-c | -C) ] []"),
N_("git notes [--ref ] copy [-f]  "),
-   N_("git notes [--ref ] append [-m  | -F  | (-c | 
-C) ] []"),
-   N_("git notes [--ref ] edit []"),
+   N_("git notes [--ref ] append [--allow-empty] [-m  | -F 
 | (-c | -C) ] []"),
+   N_("git notes [--ref ] edit [--allow-empty] []"),
N_("git notes [--ref ] show []"),
N_("git notes [--ref ] merge [-v | -q] [-s  ] 
"),
N_("git notes merge --commit [-v | -q]"),
@@ -381,7 +381,7 @@ static int append_edit(int argc, const char **argv, const 
char *prefix);
 
 static int add(int argc, const char **argv, const char *prefix)
 {
-   int force = 0;
+   int force = 0, allow_empty = 0;
const char *object_ref;
struct notes_tree *t;
unsigned char object[20], new_note[20];
@@ -400,6 +400,8 @@ static int add(int argc, const char **argv, const char 
*prefix)
{ OPTION_CALLBACK, 'C', "reuse-message", &d, N_("object"),
N_("reuse specified note object"), PARSE_OPT_NONEG,
parse_reuse_arg},
+   OPT_BOOL(0, "allow-empty", &allow_empty,
+   N_("allow storing empty note")),
OPT__FORCE(&force, N_("replace existing notes")),
OPT_END()
};
@@ -445,7 +447,7 @@ static int add(int argc, const char **argv, const char 
*prefix)
}
 
prepare_note_data(object, &d, note);
-   if (d.buf.len) {
+   if (d.buf.len || allow_empty) {
write_note_data(&d, new_note);
if (add_note(t, object, new_note, combine_notes_overwrite))
die("BUG: combine_notes_overwrite failed");
@@ -540,6 +542,7 @@ out:
 
 static int append_edit(int argc, const char **argv, const char *prefix)
 {
+   int allow_empty = 0;
const char *object_ref;
struct notes_tree *t;
unsigned char object[20], new_note[20];
@@ -560,6 +563,8 @@ static int append_edit(int argc, const char **argv, const 
char *prefix)
{ OPTION_CALLBACK, 'C', "reuse-message", &d, N_("object"),
N_("reuse specified note object"), PARSE_OPT_NONEG,
parse_reuse_arg},
+   OPT_BOOL(0, "allow-empty", &allow_empty,
+   N_("allow storing empty note")),
OPT_END()
};
int edit = !strcm

[PATCHv5 1/9] builtin/notes: Fix premature failure when trying to add the empty blob

[Johan Herland]

This fixes a small buglet when trying to explicitly add the empty blob
as a note object using the -c or -C option to git notes add/append.
Instead of failing with a nonsensical error message indicating that the
empty blob does not exist, we should rather behave as if an empty notes
message was given (e.g. using -m "" or -F /dev/null).

The next patch contains a test that verifies the fixed behavior.

Found-by: Eric Sunshine 
Signed-off-by: Johan Herland 
---
 builtin/notes.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index 68b6cd8..9ee6816 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -266,7 +266,7 @@ static int parse_reuse_arg(const struct option *opt, const 
char *arg, int unset)
 
if (get_sha1(arg, object))
die(_("Failed to resolve '%s' as a valid ref."), arg);
-   if (!(buf = read_sha1_file(object, &type, &len)) || !len) {
+   if (!(buf = read_sha1_file(object, &type, &len))) {
free(buf);
die(_("Failed to read object '%s'."), arg);
}
-- 
2.0.0.rc4.501.gdaf83ca

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv5 8/9] notes: Empty notes should be shown by 'git log'

[Johan Herland]

If the user has gone through the trouble of explicitly adding an empty
note, then "git log" should not silently skip it (as if it didn't exist).

Signed-off-by: Johan Herland 
---
 notes.c  |  3 +--
 t/t3301-notes.sh | 12 
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/notes.c b/notes.c
index 5fe691d..62bc6e1 100644
--- a/notes.c
+++ b/notes.c
@@ -1218,8 +1218,7 @@ static void format_note(struct notes_tree *t, const 
unsigned char *object_sha1,
if (!sha1)
return;
 
-   if (!(msg = read_sha1_file(sha1, &type, &msglen)) || !msglen ||
-   type != OBJ_BLOB) {
+   if (!(msg = read_sha1_file(sha1, &type, &msglen)) || type != OBJ_BLOB) {
free(msg);
return;
}
diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
index 70ec5c3..416ed9e 100755
--- a/t/t3301-notes.sh
+++ b/t/t3301-notes.sh
@@ -1274,4 +1274,16 @@ append -C "$empty_blob"
 edit
 EOF
 
+test_expect_success 'empty notes are displayed by git log' '
+   test_commit 17th &&
+   git log -1 >expect &&
+   cat >>expect <<\EOF &&
+
+Notes:
+EOF
+   git notes add -C "$empty_blob" --allow-empty &&
+   git log -1 >actual &&
+   test_cmp expect actual
+'
+
 test_done
-- 
2.0.0.rc4.501.gdaf83ca

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv5 5/9] builtin/notes: Simplify early exit code in add()

[Johan Herland]

Remove the need for 'retval' and the unnecessary goto. Also reorganize
to only call free_note_data() is actually needed.

Improved-by: Junio C Hamano 
Signed-off-by: Johan Herland 
---
 builtin/notes.c | 35 +--
 1 file changed, 17 insertions(+), 18 deletions(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index 1017472..acdedbd 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -399,7 +399,7 @@ static int append_edit(int argc, const char **argv, const 
char *prefix);
 
 static int add(int argc, const char **argv, const char *prefix)
 {
-   int retval = 0, force = 0;
+   int force = 0;
const char *object_ref;
struct notes_tree *t;
unsigned char object[20], new_note[20];
@@ -441,23 +441,23 @@ static int add(int argc, const char **argv, const char 
*prefix)
 
if (note) {
if (!force) {
-   if (!d.given) {
-   /*
-* Redirect to "edit" subcommand.
-*
-* We only end up here if none of -m/-F/-c/-C
-* or -f are given. The original args are
-* therefore still in argv[0-1].
-*/
-   argv[0] = "edit";
+   free_notes(t);
+   if (d.given) {
free_note_data(&d);
-   free_notes(t);
-   return append_edit(argc, argv, prefix);
+   return error(_("Cannot add notes. "
+   "Found existing notes for object %s. "
+   "Use '-f' to overwrite existing notes"),
+   sha1_to_hex(object));
}
-   retval = error(_("Cannot add notes. Found existing 
notes "
-  "for object %s. Use '-f' to overwrite "
-  "existing notes"), sha1_to_hex(object));
-   goto out;
+   /*
+* Redirect to "edit" subcommand.
+*
+* We only end up here if none of -m/-F/-c/-C or -f are
+* given. The original args are therefore still in
+* argv[0-1].
+*/
+   argv[0] = "edit";
+   return append_edit(argc, argv, prefix);
}
fprintf(stderr, _("Overwriting existing notes for object %s\n"),
sha1_to_hex(object));
@@ -474,9 +474,8 @@ static int add(int argc, const char **argv, const char 
*prefix)
snprintf(logmsg, sizeof(logmsg), "Notes %s by 'git notes %s'",
 is_null_sha1(new_note) ? "removed" : "added", "add");
commit_notes(t, logmsg);
-out:
free_notes(t);
-   return retval;
+   return 0;
 }
 
 static int copy(int argc, const char **argv, const char *prefix)
-- 
2.0.0.rc4.501.gdaf83ca

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv5 6/9] builtin/notes: Split create_note() to clarify add vs. remove logic

[Johan Herland]

create_note() has a non-trivial interface, and comprises three loosely
related parts:

 1. launching the editor with the note contents, if needed
 2. appending to an existing note, if append_only was given
 3. adding or removing the resulting note, based on whether it's non-empty

Split it along those lines to make the logic clearer: The first part
goes into a new function - prepare_note_data(), with a simpler interface.
The second part is moved into append_edit(), which is the only user of
this code. Finally, the add vs. remove decision is moved into the callers
(add() and append_edit()), keeping the logic for writing the actual note
object in a separate function: write_note_data().

Suggested-by: Junio C Hamano 
Signed-off-by: Johan Herland 
---
 builtin/notes.c | 103 +---
 1 file changed, 54 insertions(+), 49 deletions(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index acdedbd..ed32d63 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -159,9 +159,8 @@ static void write_commented_object(int fd, const unsigned 
char *object)
sha1_to_hex(object));
 }
 
-static void create_note(const unsigned char *object, struct note_data *d,
-   int append_only, const unsigned char *prev,
-   unsigned char *result)
+static void prepare_note_data(const unsigned char *object, struct note_data *d,
+   const unsigned char *old_note)
 {
if (d->use_editor || !d->given) {
int fd;
@@ -175,8 +174,8 @@ static void create_note(const unsigned char *object, struct 
note_data *d,
 
if (d->given)
write_or_die(fd, d->buf.buf, d->buf.len);
-   else if (prev && !append_only)
-   copy_obj_to_fd(fd, prev);
+   else if (old_note)
+   copy_obj_to_fd(fd, old_note);
 
strbuf_addch(&buf, '\n');
strbuf_add_commented_lines(&buf, note_template, 
strlen(note_template));
@@ -194,33 +193,16 @@ static void create_note(const unsigned char *object, 
struct note_data *d,
}
stripspace(&d->buf, 1);
}
+}
 
-   if (prev && append_only) {
-   /* Append buf to previous note contents */
-   unsigned long size;
-   enum object_type type;
-   char *prev_buf = read_sha1_file(prev, &type, &size);
-
-   strbuf_grow(&d->buf, size + 1);
-   if (d->buf.len && prev_buf && size)
-   strbuf_insert(&d->buf, 0, "\n", 1);
-   if (prev_buf && size)
-   strbuf_insert(&d->buf, 0, prev_buf, size);
-   free(prev_buf);
-   }
-
-   if (!d->buf.len) {
-   fprintf(stderr, _("Removing note for object %s\n"),
-   sha1_to_hex(object));
-   hashclr(result);
-   } else {
-   if (write_sha1_file(d->buf.buf, d->buf.len, blob_type, result)) 
{
-   error(_("unable to write note object"));
-   if (d->edit_path)
-   error(_("The note contents have been left in 
%s"),
- d->edit_path);
-   exit(128);
-   }
+static void write_note_data(struct note_data *d, unsigned char *sha1)
+{
+   if (write_sha1_file(d->buf.buf, d->buf.len, blob_type, sha1)) {
+   error(_("unable to write note object"));
+   if (d->edit_path)
+   error(_("The note contents have been left in %s"),
+   d->edit_path);
+   exit(128);
}
 }
 
@@ -403,7 +385,6 @@ static int add(int argc, const char **argv, const char 
*prefix)
const char *object_ref;
struct notes_tree *t;
unsigned char object[20], new_note[20];
-   char logmsg[100];
const unsigned char *note;
struct note_data d = { 0, 0, NULL, STRBUF_INIT };
struct option options[] = {
@@ -463,17 +444,20 @@ static int add(int argc, const char **argv, const char 
*prefix)
sha1_to_hex(object));
}
 
-   create_note(object, &d, 0, note, new_note);
-   free_note_data(&d);
-
-   if (is_null_sha1(new_note))
+   prepare_note_data(object, &d, note);
+   if (d.buf.len) {
+   write_note_data(&d, new_note);
+   if (add_note(t, object, new_note, combine_notes_overwrite))
+   die("BUG: combine_notes_overwrite failed");
+   commit_notes(t, "Notes added by 'git notes add'");
+   } else {
+   fprintf(stderr, _("Removing note for object %s\n"),
+   sha1_to_hex(object));
remove_note(t, object);
-   else if (add_note(t, object, new_note, combine_notes_overwrite))
-   die("BUG: combine_notes_overwrite failed");
+

[PATCHv5 4/9] builtin/notes: Refactor note file path into struct note_data

[Johan Herland]

Move the 'path' variable from create_note() and into the
note_data struct. Unify cleanup of note_data objects with
a free_note_data() function.

This might not make too much sense on its own, but it makes the
future refactoring of create_note() considerably cleaner.

Signed-off-by: Johan Herland 
---
 builtin/notes.c | 38 +-
 1 file changed, 21 insertions(+), 17 deletions(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index 3cf22cb..1017472 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -95,9 +95,19 @@ static const char note_template[] =
 struct note_data {
int given;
int use_editor;
+   char *edit_path;
struct strbuf buf;
 };
 
+static void free_note_data(struct note_data *d)
+{
+   if (d->edit_path) {
+   unlink_or_warn(d->edit_path);
+   free(d->edit_path);
+   }
+   strbuf_release(&d->buf);
+}
+
 static int list_each_note(const unsigned char *object_sha1,
const unsigned char *note_sha1, char *note_path,
void *cb_data)
@@ -153,17 +163,15 @@ static void create_note(const unsigned char *object, 
struct note_data *d,
int append_only, const unsigned char *prev,
unsigned char *result)
 {
-   char *path = NULL;
-
if (d->use_editor || !d->given) {
int fd;
struct strbuf buf = STRBUF_INIT;
 
/* write the template message before editing: */
-   path = git_pathdup("NOTES_EDITMSG");
-   fd = open(path, O_CREAT | O_TRUNC | O_WRONLY, 0600);
+   d->edit_path = git_pathdup("NOTES_EDITMSG");
+   fd = open(d->edit_path, O_CREAT | O_TRUNC | O_WRONLY, 0600);
if (fd < 0)
-   die_errno(_("could not create file '%s'"), path);
+   die_errno(_("could not create file '%s'"), 
d->edit_path);
 
if (d->given)
write_or_die(fd, d->buf.buf, d->buf.len);
@@ -181,7 +189,7 @@ static void create_note(const unsigned char *object, struct 
note_data *d,
strbuf_release(&buf);
strbuf_reset(&d->buf);
 
-   if (launch_editor(path, &d->buf, NULL)) {
+   if (launch_editor(d->edit_path, &d->buf, NULL)) {
die(_("Please supply the note contents using either -m 
or -F option"));
}
stripspace(&d->buf, 1);
@@ -208,17 +216,12 @@ static void create_note(const unsigned char *object, 
struct note_data *d,
} else {
if (write_sha1_file(d->buf.buf, d->buf.len, blob_type, result)) 
{
error(_("unable to write note object"));
-   if (path)
+   if (d->edit_path)
error(_("The note contents have been left in 
%s"),
- path);
+ d->edit_path);
exit(128);
}
}
-
-   if (path) {
-   unlink_or_warn(path);
-   free(path);
-   }
 }
 
 static int parse_msg_arg(const struct option *opt, const char *arg, int unset)
@@ -402,7 +405,7 @@ static int add(int argc, const char **argv, const char 
*prefix)
unsigned char object[20], new_note[20];
char logmsg[100];
const unsigned char *note;
-   struct note_data d = { 0, 0, STRBUF_INIT };
+   struct note_data d = { 0, 0, NULL, STRBUF_INIT };
struct option options[] = {
{ OPTION_CALLBACK, 'm', "message", &d, N_("message"),
N_("note contents as a string"), PARSE_OPT_NONEG,
@@ -447,6 +450,7 @@ static int add(int argc, const char **argv, const char 
*prefix)
 * therefore still in argv[0-1].
 */
argv[0] = "edit";
+   free_note_data(&d);
free_notes(t);
return append_edit(argc, argv, prefix);
}
@@ -460,6 +464,7 @@ static int add(int argc, const char **argv, const char 
*prefix)
}
 
create_note(object, &d, 0, note, new_note);
+   free_note_data(&d);
 
if (is_null_sha1(new_note))
remove_note(t, object);
@@ -471,7 +476,6 @@ static int add(int argc, const char **argv, const char 
*prefix)
commit_notes(t, logmsg);
 out:
free_notes(t);
-   strbuf_release(&d.buf);
return retval;
 }
 
@@ -559,7 +563,7 @@ static int append_edit(int argc, const char **argv, const 
char *prefix)
const unsigned char *note;
char logmsg[100];
const char * const *usage;
-   struct note_data d = { 0, 0, STRBUF_INIT };
+   struct note_data d = { 0, 0, NULL, STRBUF_INIT };
struct option options[] = {
{ 

[PATCHv5 3/9] builtin/notes: Improve naming

[Johan Herland]

In preparation for some needed refactoring, rename struct msg_arg to
struct note_data, and rename its instances from "msg" to "d" (also
removing some unnecessary parentheses). The 'msg_arg' name was
inherited from tag.c, but is not really a good name for the contents
of a note.

Also rename write_note_data() to copy_obj_to_fd(), which more aptly
describes what it actually does: Copying the contents of a git object
(given by its SHA1) into a given file descriptor.

Signed-off-by: Johan Herland 
---
 builtin/notes.c | 109 
 1 file changed, 54 insertions(+), 55 deletions(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index 9ee6816..3cf22cb 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -92,7 +92,7 @@ static const char * const git_notes_get_ref_usage[] = {
 static const char note_template[] =
"\nWrite/edit the notes for the following object:\n";
 
-struct msg_arg {
+struct note_data {
int given;
int use_editor;
struct strbuf buf;
@@ -106,7 +106,7 @@ static int list_each_note(const unsigned char *object_sha1,
return 0;
 }
 
-static void write_note_data(int fd, const unsigned char *sha1)
+static void copy_obj_to_fd(int fd, const unsigned char *sha1)
 {
unsigned long size;
enum object_type type;
@@ -149,13 +149,13 @@ static void write_commented_object(int fd, const unsigned 
char *object)
sha1_to_hex(object));
 }
 
-static void create_note(const unsigned char *object, struct msg_arg *msg,
+static void create_note(const unsigned char *object, struct note_data *d,
int append_only, const unsigned char *prev,
unsigned char *result)
 {
char *path = NULL;
 
-   if (msg->use_editor || !msg->given) {
+   if (d->use_editor || !d->given) {
int fd;
struct strbuf buf = STRBUF_INIT;
 
@@ -165,10 +165,10 @@ static void create_note(const unsigned char *object, 
struct msg_arg *msg,
if (fd < 0)
die_errno(_("could not create file '%s'"), path);
 
-   if (msg->given)
-   write_or_die(fd, msg->buf.buf, msg->buf.len);
+   if (d->given)
+   write_or_die(fd, d->buf.buf, d->buf.len);
else if (prev && !append_only)
-   write_note_data(fd, prev);
+   copy_obj_to_fd(fd, prev);
 
strbuf_addch(&buf, '\n');
strbuf_add_commented_lines(&buf, note_template, 
strlen(note_template));
@@ -179,13 +179,12 @@ static void create_note(const unsigned char *object, 
struct msg_arg *msg,
 
close(fd);
strbuf_release(&buf);
-   strbuf_reset(&(msg->buf));
+   strbuf_reset(&d->buf);
 
-   if (launch_editor(path, &(msg->buf), NULL)) {
-   die(_("Please supply the note contents using either -m" 
\
-   " or -F option"));
+   if (launch_editor(path, &d->buf, NULL)) {
+   die(_("Please supply the note contents using either -m 
or -F option"));
}
-   stripspace(&(msg->buf), 1);
+   stripspace(&d->buf, 1);
}
 
if (prev && append_only) {
@@ -194,20 +193,20 @@ static void create_note(const unsigned char *object, 
struct msg_arg *msg,
enum object_type type;
char *prev_buf = read_sha1_file(prev, &type, &size);
 
-   strbuf_grow(&(msg->buf), size + 1);
-   if (msg->buf.len && prev_buf && size)
-   strbuf_insert(&(msg->buf), 0, "\n", 1);
+   strbuf_grow(&d->buf, size + 1);
+   if (d->buf.len && prev_buf && size)
+   strbuf_insert(&d->buf, 0, "\n", 1);
if (prev_buf && size)
-   strbuf_insert(&(msg->buf), 0, prev_buf, size);
+   strbuf_insert(&d->buf, 0, prev_buf, size);
free(prev_buf);
}
 
-   if (!msg->buf.len) {
+   if (!d->buf.len) {
fprintf(stderr, _("Removing note for object %s\n"),
sha1_to_hex(object));
hashclr(result);
} else {
-   if (write_sha1_file(msg->buf.buf, msg->buf.len, blob_type, 
result)) {
+   if (write_sha1_file(d->buf.buf, d->buf.len, blob_type, result)) 
{
error(_("unable to write note object"));
if (path)
error(_("The note contents have been left in 
%s"),
@@ -224,45 +223,45 @@ static void create_note(const unsigned char *object, 
struct msg_arg *msg,
 
 static int parse_msg_arg(const struct option *opt, const char *arg, int unset)
 {
-   struct msg_arg *msg = opt->value;
+   struct note_data *d = opt->value;
 
-   strbuf_grow(&(msg->buf), strlen(arg)

[PATCHv5 0/9] Handling empty notes

[Johan Herland]

Changes v4 -> v5:

 - Patch #5: Reorder code to ease readability and fix issue with
   checking d.given after free_note_data(&d), noticed by Junio.

 - Patch #9: Fix typo in commit message, noticed by Junio.

 - Patch #9: Rename "output" files to "actual", suggested by Eric.

 - Patch #9: Use test_line_count instead of wc -l, which behaves
   differently on Macs. noticed by Michael, diagnosed by Eric, and
   fix suggested by Peff.

 - Patch #9: Use -r option to ls-tree when counting entries in notes,
   tree as this also works for trees with fanout level > 0. Noticed
   by Peff.


Have fun! :)

...Johan


Johan Herland (9):
  builtin/notes: Fix premature failure when trying to add the empty blob
  t3301: Verify that 'git notes' removes empty notes by default
  builtin/notes: Improve naming
  builtin/notes: Refactor note file path into struct note_data
  builtin/notes: Simplify early exit code in add()
  builtin/notes: Split create_note() to clarify add vs. remove logic
  builtin/notes: Add --allow-empty, to allow storing empty notes
  notes: Empty notes should be shown by 'git log'
  t3301: Modernize

 Documentation/git-notes.txt |   12 +-
 builtin/notes.c |  258 +
 notes.c |3 +-
 t/t3301-notes.sh| 1341 +--
 4 files changed, 789 insertions(+), 825 deletions(-)

-- 
2.0.0.rc4.501.gdaf83ca

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: mac test failure -- test 3301

[Johan Herland]

On Tue, Nov 11, 2014 at 3:41 AM, Jeff King  wrote:
> On Tue, Nov 11, 2014 at 02:40:19AM +0100, Johan Herland wrote:
>> > This and all other failures are due to the output of 'wc -l', which on
>> > Mac is "{whitespace}1" rather than just "1" as it is on other
>> > platforms. fbe4f748 added quotes around the $(... | wc -l) invocation
>> > which caused the whitespace to be retained. A minimum fix would be to
>> > drop the quotes surrounding $().
>>
>> Ah, thanks!
>>
>> I thought that quoting command output was a good idea in general. Am I
>> wrong, or is this just one exception to an otherwise good guideline?
>
> It usually is a good idea to prevent whitespace splitting by the shell
> (and especially with things that might unexpectedly be empty, in which
> case they end up as "no argument" and not an empty one). So yeah, this
> is an exception.
>
>> Anyway, here is the minimal diff to remove quoting from the $(... | wc
>> -l) commands (probably whitespace damaged by GMail - sorry). Junio:
>> feel free to squash this in, or ask for a re-roll:
>
> I think we have test_line_count exactly because of this unportability
> with wc output.
>
> You'd want:
>
>   git ls-tree refs/notes/commits >actual &&
>   test_line_count = 1 actual
>
> or similar.

Thanks. Will use this in the re-roll.

> By the way, the point of that "ls-tree" appears to be to check the
> number of total notes stored. Since notes are stored in a hashed
> structure, should it be "ls-tree -r"? Otherwise, we see only the leading
> directories; two notes whose sha1s start with the same two characters
> would be considered a single entry.

Yes and no... The notes' nested fanout structure is developed
dynamically as the number of notes increase. As long as the number of
notes stay low, the notes tree remain at fanout level 0 (i.e. no
nesting), so ls-tree happens to work here. Still ls-tree -r is the
correct for any size notes tree. Will fix to use ls-tree -r.


...Johan

-- 
Johan Herland, 
www.herland.net
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

What's cooking in git.git (Nov 2014, #02; Tue, 11)

[Junio C Hamano]

Here are the topics that have been cooking.  Commits prefixed with
'-' are only in 'pu' (proposed updates) while commits prefixed with
'+' are in 'next'.

You can find the changes described here in the integration branches
of the repositories listed at

http://git-blame.blogspot.com/p/git-public-repositories.html

--
[Graduated to "master"]

* jk/fetch-reflog-df-conflict (2014-11-04) 2 commits
  (merged to 'next' on 2014-11-05 at b0476c9)
 + ignore stale directories when checking reflog existence
 + fetch: load all default config at startup

 Corner-case bugfixes for "git fetch" around reflog handling.


* jk/cache-tree-protect-from-broken-libgit2 (2014-10-30) 1 commit
  (merged to 'next' on 2014-11-03 at 6ebd88d)
 + cache-tree: avoid infinite loop on zero-entry tree

 Corner-case bugfixes when using a corrupt index file.


* js/diff-highlight-avoid-sigpipe (2014-11-04) 1 commit
  (merged to 'next' on 2014-11-05 at b0fadd3)
 + diff-highlight: exit when a pipe is broken


* nd/gitignore-trailing-whitespace (2014-11-04) 1 commit
  (merged to 'next' on 2014-11-05 at e7f43d6)
 + gitignore.txt: fix spelling of "backslash"


* rs/use-child-process-init-more (2014-10-30) 4 commits
  (merged to 'next' on 2014-11-03 at a82d885)
 + bundle: split out ref writing from bundle_create
 + bundle: split out a helper function to compute and write prerequisites
 + bundle: split out a helper function to create pack data
 + use child_process_init() to initialize struct child_process variables


* tm/line-log-first-parent (2014-11-04) 1 commit
  (merged to 'next' on 2014-11-05 at 8a6f650)
 + line-log: fix crash when --first-parent is used

 "git log --first-parent -L..." used to crash.

--
[New Topics]

* jk/fetch-reflog-df-conflict (2014-11-10) 1 commit
  (merged to 'next' on 2014-11-11 at 88c1491)
 + t1410: fix breakage on case-insensitive filesystems

 Will merge to 'master'.


* br/imap-send-verbosity (2014-11-05) 1 commit
 - imap-send: use parse options API to determine verbosity
 (this branch is used by br/imap-send-via-libcurl.)

 Will merge to 'next' and cook throughout the remainder of the cycle.


* br/imap-send-via-libcurl (2014-11-10) 1 commit
 - git-imap-send: use libcurl for implementation
 (this branch uses br/imap-send-verbosity.)

 Will merge to 'next' and cook throughout the remainder of the cycle.


* jc/doc-commit-only (2014-11-07) 1 commit
 - Documentation/git-commit: clarify that --only/--include records the working 
tree contents


* cc/interpret-trailers (2014-11-10) 2 commits
 - trailer: display a trailer without its trailing newline
 - trailer: ignore comment lines inside the trailers
 (this branch is used by cc/interpret-trailers-more.)


* cc/interpret-trailers-more (2014-11-10) 4 commits
 - trailer: add test with an old style conflict block
 - trailer: reuse ignore_non_trailer() to ignore conflict lines
 - commit: make ignore_non_trailer() non static
 - Merge branch 'jc/conflict-hint' into cc/interpret-trailers-more
 (this branch uses cc/interpret-trailers and jc/conflict-hint.)


* js/push-to-update (2014-11-10) 2 commits
 - receive-pack: add two more options for receive.denyCurrentBranch
 - finish_command(): clean stale environment pointer


* rs/env-array-in-child-process (2014-11-10) 1 commit
 - use args member of struct child_process


* tq/git-ssh-command (2014-11-10) 1 commit
 - git_connect: set ssh shell command in GIT_SSH_COMMAND

--
[Stalled]

* je/quiltimport-no-fuzz (2014-10-21) 2 commits
 - git-quiltimport: flip the default not to allow fuzz
 - git-quiltimport.sh: allow declining fuzz with --exact option

 "quiltimport" drove "git apply" always with -C1 option to reduce
 context of the patch in order to give more chance to somewhat stale
 patches to apply.  Add an "--exact" option to disable, and also
 "-C$n" option to customize this behaviour.  The top patch
 optionally flips the default to "--exact".

 Waiting for an Ack.


* jc/push-cert-hmac-optim (2014-09-25) 2 commits
 - receive-pack: truncate hmac early and convert only necessary bytes
 - sha1_to_hex: split out "hex-format n bytes" helper and use it

 This is "we could do this if we wanted to", not "we measured and it
 improves performance critical codepath".

 Will perhaps drop.


* nd/multiple-work-trees (2014-09-27) 32 commits
 . t2025: add a test to make sure grafts is working from a linked checkout
 . checkout: don't require a work tree when checking out into a new one
 . git_path(): keep "info/sparse-checkout" per work-tree
 . count-objects: report unused files in $GIT_DIR/worktrees/...
 . gc: support prune --worktrees
 . gc: factor out gc.pruneexpire parsing code
 . gc: style change -- no SP before closing parenthesis
 . checkout: clean up half-prepared directories in --to mode
 . checkout: reject if the branch is already checked out elsewhere
 . prune: strategies for linked ch

Re: What is the default refspec for fetch?

[Junio C Hamano]

Christian Halstrick  writes:

> ok, thanks. Then I'll teach JGit to fetch at least "HEAD" if nothing
> is configured and nothing explicitly specified as refspec.

Sounds like a sensible thing to do to match what JGit does to what
we did from the time immemorial ;-)

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: What is the default refspec for fetch?

[Christian Halstrick]

ok, thanks. Then I'll teach JGit to fetch at least "HEAD" if nothing
is configured and nothing explicitly specified as refspec.
Ciao
  Chris


On Sun, Nov 9, 2014 at 6:23 PM, Junio C Hamano  wrote:
> Jakub Narębski  writes:
>
>> W dniu 2014-11-08 11:52, Jeff King pisze:
>>> On Fri, Nov 07, 2014 at 04:31:08PM +0100, Christian Halstrick wrote:
>>>
 In a repo where no remote..fetch config parameter is set what
 should a "git fetch" do? My experiments let me think it's
 "HEAD:FETCH_HEAD". Right?
>>>
>>> Basically, yes. We always write FETCH_HEAD, regardless of the refspec.
>>> We choose "HEAD" if no other refspec was provided. So it is really more
>>> like
>>>
>>>git fetch $remote HEAD
>>>
>>> This is what makes one-off bare-url pulls work, like:
>>>
>>>git pull git://...
>>>
>>> It runs fetch under the hood, which writes into FETCH_HEAD, and then we
>>> merge that.
>>
>> Actually FETCH_HEAD consists of multiple lines, one per ref...
>> but only top ref is merged.
>
> Incorrect in that "only top" is misinformation, and irrelevant in
> the context of discussing the "what is fetched in a one-off fetch".
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] gittutorial.txt: remove reference to ancient Git version

[Junio C Hamano]

Thomas Ackermann  writes:

> Signed-off-by: Thomas Ackermann 
> ---
>  Documentation/gittutorial.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/gittutorial.txt b/Documentation/gittutorial.txt
> index af9f709..710e636 100644
> --- a/Documentation/gittutorial.txt
> +++ b/Documentation/gittutorial.txt
> @@ -3,7 +3,7 @@ gittutorial(7)
>  
>  NAME
>  
> -gittutorial - A tutorial introduction to Git (for version 1.5.1 or newer)
> +gittutorial - A tutorial introduction to Git
>  
>  SYNOPSIS
>  

Yeah, sounds like going in the right direction.

Perhaps we should start doing a serious re-reading of this document
and update the contents to suggest better ways that were invented
to solve common problems since this document was written.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] doc: add some crossrefs between manual pages

[Junio C Hamano]

Max Horn  writes:

> I did this because I was browsing the remote helper docs online quite a bit,
> and was wishing for some more direct links between the pages. While I can
> manyally edit the URL, it seems logical to offer these links directly.

> diff --git a/Documentation/git-fast-export.txt 
> b/Documentation/git-fast-export.txt
> ...
> +linkgit:git-fast-import[1]

> diff --git a/Documentation/git-fast-import.txt 
> b/Documentation/git-fast-import.txt
> ...
> +linkgit:git-fast-export[1]

Makes sense to have these pair refer to each other.

> diff --git a/Documentation/git-remote-ext.txt 
> b/Documentation/git-remote-ext.txt
> ...
> +linkgit:gitremote-helpers[1]

> diff --git a/Documentation/git-remote-fd.txt b/Documentation/git-remote-fd.txt
> +linkgit:gitremote-helpers[1]

Likewise.  git-remote-* are instances of gitremote-helpers.

> diff --git a/Documentation/gitremote-helpers.txt 
> b/Documentation/gitremote-helpers.txt
> index 64f7ad2..8edf72c 100644
> --- a/Documentation/gitremote-helpers.txt
> +++ b/Documentation/gitremote-helpers.txt
> @@ -452,8 +452,14 @@ SEE ALSO
>  
>  linkgit:git-remote[1]
>  
> +linkgit:git-remote-ext[1]
> +
> +linkgit:git-remote-fd[1]
> +
>  linkgit:git-remote-testgit[1]

Makes sense.

> +linkgit:git-fast-import[1]

This looks somewhat out of place; fast-import is not the only or
even the primary way to do a remote-helper, is it?

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] doc: add some crossrefs between manual pages

[Max Horn]

In particular, git-fast-import and -export link to each
other, and gitremote-helpers links to existing remote
helpers, and vice versa. Also link to fast-import from the
remote helper spec, as this is relevant for remote helpers
using the fast-import format.

Signed-off-by: Max Horn 
---
I did this because I was browsing the remote helper docs online quite a bit,
and was wishing for some more direct links between the pages. While I can
manyally edit the URL, it seems logical to offer these links directly.

 Documentation/git-fast-export.txt   | 4 
 Documentation/git-fast-import.txt   | 4 
 Documentation/git-remote-ext.txt| 5 -
 Documentation/git-remote-fd.txt | 4 
 Documentation/gitremote-helpers.txt | 6 ++
 5 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-fast-export.txt 
b/Documentation/git-fast-export.txt
index 221506b..769689d 100644
--- a/Documentation/git-fast-export.txt
+++ b/Documentation/git-fast-export.txt
@@ -148,6 +148,10 @@ Since 'git fast-import' cannot tag trees, you will not be
 able to export the linux.git repository completely, as it contains
 a tag referencing a tree instead of a commit.
 
+SEE ALSO
+
+linkgit:git-fast-import[1]
+
 GIT
 ---
 Part of the linkgit:git[1] suite
diff --git a/Documentation/git-fast-import.txt 
b/Documentation/git-fast-import.txt
index 377eeaa..f71fb01 100644
--- a/Documentation/git-fast-import.txt
+++ b/Documentation/git-fast-import.txt
@@ -1441,6 +1441,10 @@ operator can use this facility to peek at the objects 
and refs from an
 import in progress, at the cost of some added running time and worse
 compression.
 
+SEE ALSO
+
+linkgit:git-fast-export[1]
+
 GIT
 ---
 Part of the linkgit:git[1] suite
diff --git a/Documentation/git-remote-ext.txt b/Documentation/git-remote-ext.txt
index cd0bb77..74817b0 100644
--- a/Documentation/git-remote-ext.txt
+++ b/Documentation/git-remote-ext.txt
@@ -72,7 +72,6 @@ GIT_EXT_SERVICE_NOPREFIX::
Set to long name (upload-pack, etc...) of service helper needs
to invoke.
 
-
 EXAMPLES:
 -
 This remote helper is transparently used by Git when
@@ -116,6 +115,10 @@ begins with `ext::`.  Examples:
determined by the helper using environment variables (see
above).
 
+SEE ALSO
+
+linkgit:gitremote-helpers[1]
+
 GIT
 ---
 Part of the linkgit:git[1] suite
diff --git a/Documentation/git-remote-fd.txt b/Documentation/git-remote-fd.txt
index bcd3766..e700baf 100644
--- a/Documentation/git-remote-fd.txt
+++ b/Documentation/git-remote-fd.txt
@@ -50,6 +50,10 @@ EXAMPLES
 `git push fd::7,8/bar master`::
Same as above.
 
+SEE ALSO
+
+linkgit:gitremote-helpers[1]
+
 GIT
 ---
 Part of the linkgit:git[1] suite
diff --git a/Documentation/gitremote-helpers.txt 
b/Documentation/gitremote-helpers.txt
index 64f7ad2..8edf72c 100644
--- a/Documentation/gitremote-helpers.txt
+++ b/Documentation/gitremote-helpers.txt
@@ -452,8 +452,14 @@ SEE ALSO
 
 linkgit:git-remote[1]
 
+linkgit:git-remote-ext[1]
+
+linkgit:git-remote-fd[1]
+
 linkgit:git-remote-testgit[1]
 
+linkgit:git-fast-import[1]
+
 GIT
 ---
 Part of the linkgit:git[1] suite
-- 
2.1.3.dirty

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] gittutorial.txt: remove reference to ancient Git version

[Thomas Ackermann]

Signed-off-by: Thomas Ackermann 
---
 Documentation/gittutorial.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/gittutorial.txt b/Documentation/gittutorial.txt
index af9f709..710e636 100644
--- a/Documentation/gittutorial.txt
+++ b/Documentation/gittutorial.txt
@@ -3,7 +3,7 @@ gittutorial(7)
 
 NAME
 
-gittutorial - A tutorial introduction to Git (for version 1.5.1 or newer)
+gittutorial - A tutorial introduction to Git
 
 SYNOPSIS
 
-- 
1.9.4.msysgit.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Opinions] Integrated tickets

[Junio C Hamano]

Junio C Hamano  writes:

> Either way, I do not see how such an arrangement is the most
> convenient way to organize the tickets and ask questions such as
> "what are the known, untriaged, or unresolved issues in v1.8.5?",
> "what are the issues that didn't exist in v1.7.0 but appear in
> v1.8.5?", "what are the outstanding issues around refs handling that
> are the highest priority?", etc.  With your arrangement of data, any
> of the common questions I think of asking would require a linear
> scan of a commit range, followed by an enumeration and parsing of
> all the notes attached to the commits to answer.
>
> So I would have to say that your expectation makes even less sense
> than annotating an exact buggy commit with a note saying what is
> broken by it.

Not that annotating the commit as "this commit has this bug" makes
much sense, though, of course ;-)  But at least it would let us
answer "Does this commit introduce a bug?" question, and if the
annotated information also records "... and that other commit is a
fix that can be cherry-picked (or merged)", that would be even
better.  That would allow us, when merging down the commit thusly
annotated, to stop and consider either not merging (because it is
known to introduce a bug) or merging with fixes also merged (because
the solution is already known and recorded).
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Opinions] Integrated tickets

[Holger Hellmuth]

Am 11.11.2014 um 18:17 schrieb Junio C Hamano:

Holger Hellmuth  writes:


Am 06.11.2014 um 19:45 schrieb Junio C Hamano:

This is a tangent, but I personally do not think "ticket" meshes
very well with "commit".  If you already know which commit was
problematic, why are you annotating it with a ticket before
reverting it first?


I would expect a ticket to be annotating the commit or version tag
where the bug was found, which usually isn't the commit where the bug
was introduced.


[...]


Either way, I do not see how such an arrangement is the most
convenient way to organize the tickets and ask questions such as
"what are the known, untriaged, or unresolved issues in v1.8.5?",
"what are the issues that didn't exist in v1.7.0 but appear in
v1.8.5?", "what are the outstanding issues around refs handling that
are the highest priority?", etc.  With your arrangement of data, any
of the common questions I think of asking would require a linear
scan of a commit range, followed by an enumeration and parsing of
all the notes attached to the commits to answer.

So I would have to say that your expectation makes even less sense
than annotating an exact buggy commit with a note saying what is
broken by it.


Not less sense, because with tickets attached to the exact buggy commit 
one would have the same problems answering the questions above. I don't 
dispute that tickets and commits don't mesh, it was the reason that you 
gave the first time that didn't sound right. Sorry if I have wasted your 
time, but looking at it from the management side removed any lingering 
doubts for me that there might be a benefit to an integration, even if 
some sort of indexing or database was used.


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [GIT PULL] l10n updates for 2.2.0 round 1

[Junio C Hamano]

Jiang Xin  writes:

> Please pull the following git l10n updates.
>
> The following changes since commit 4ace7ff4557350b7e0b57d024a2ea311b332e01d:
>
>   Git 2.2.0-rc0 (2014-10-31 11:57:23 -0700)
>
> are available in the git repository at:
>
>   git://github.com/git-l10n/git-po master
>
> for you to fetch changes up to 6c31a5e94af1036bb29da8a75f1e735e662aee92:
>
>   l10n: Updated Bulgarian translation of git (2296t,0f,0u) (2014-11-02
> 19:11:08 +0200)

Thanks, all.

>
> 
> Alexander Shopov (1):
>   l10n: Updated Bulgarian translation of git (2296t,0f,0u)
>
> Jean-Noel Avila (1):
>   l10n: fr.po (2296t) update for version 2.2.0
>
> Jiang Xin (4):
>   l10n: git.pot: v2.2.0 round 1 (62 new, 23 removed)
>   Merge branch 'master' of git://github.com/nafmo/git-l10n-sv
>   Merge branch 'fr_2.2.0' of git://github.com/jnavila/git
>   l10n: zh_CN: translations for git v2.2.0-rc0
>
> Peter Krefting (1):
>   l10n: sv.po: Update Swedish translation (2296t0f0u)
>
> Trần Ngọc Quân (1):
>   l10n: vi.po: Update new message strings
>
>  po/bg.po| 3211 +++--
>  po/fr.po| 3199 +++--
>  po/git.pot  | 3109 +--
>  po/sv.po| 3187 ++--
>  po/vi.po| 3306 
> ---
>  po/zh_CN.po | 3197 +++--
>  6 files changed, 10327 insertions(+), 8882 deletions(-)
>
> --
> Jiang Xin
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] run-command: use void to declare that functions take no parameters

[Junio C Hamano]

Jeff King  writes:

> I had always just assumed that -Wstrict-prototypes was part of -Wall,
> but it is not (nor even part of -Wextra!). Maybe it is time to add it to
> your integration-build flags. :)

Yup, I had -Wold-style-definition but not -Wstrict-prototypes in the
mix.  Thanks for spotting.

>
> Looks like we also need this on top of hv/submodule-config (still in pu,
> so squashing is probably best):
>
> diff --git a/submodule-config.h b/submodule-config.h
> index 58afc83..9061e4e 100644
> --- a/submodule-config.h
> +++ b/submodule-config.h
> @@ -24,6 +24,6 @@ const struct submodule *submodule_from_name(const unsigned 
> char *commit_sha1,
>   const char *name);
>  const struct submodule *submodule_from_path(const unsigned char *commit_sha1,
>   const char *path);
> -void submodule_free();
> +void submodule_free(void);
>  
>  #endif /* SUBMODULE_CONFIG_H */
>
> -Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Opinions] Integrated tickets

[Junio C Hamano]

Holger Hellmuth  writes:

> Am 06.11.2014 um 19:45 schrieb Junio C Hamano:
>> This is a tangent, but I personally do not think "ticket" meshes
>> very well with "commit".  If you already know which commit was
>> problematic, why are you annotating it with a ticket before
>> reverting it first?
>
> I would expect a ticket to be annotating the commit or version tag
> where the bug was found, which usually isn't the commit where the bug
> was introduced.

You could arrange your "tickets" in such a way, but in general, the
way you organize your data should match how the data is expected to
commonly be accessed.

If somebody finds a bug when the version he happened to be using was
v1.8.5-9-g144d846, do you mean to attach that ticket to that exact
commit?  Or do you use v1.8.5^0 (i.e. the closest tagged version)
after making sure that it is not a commit between these two that
introduced it as a new bug?

Either way, I do not see how such an arrangement is the most
convenient way to organize the tickets and ask questions such as
"what are the known, untriaged, or unresolved issues in v1.8.5?",
"what are the issues that didn't exist in v1.7.0 but appear in
v1.8.5?", "what are the outstanding issues around refs handling that
are the highest priority?", etc.  With your arrangement of data, any
of the common questions I think of asking would require a linear
scan of a commit range, followed by an enumeration and parsing of
all the notes attached to the commits to answer.

So I would have to say that your expectation makes even less sense
than annotating an exact buggy commit with a note saying what is
broken by it.











--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: mac test failure -- test 3301

[Junio C Hamano]

Johan Herland  writes:

> Ah, thanks!
>
> I thought that quoting command output was a good idea in general. Am I
> wrong, or is this just one exception to an otherwise good guideline?

It is not a good practice to blindly follow any guideline ;-).

When you anticipate that different platforms throw you unnecessary
spaces around a single token you expect to see, not quoting is
obviously the right thing.  If you are unsure if the comamand gives
you one or multiple tokens, you obviously risk the output getting
split by not quoting.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] Clean stale environment pointer in finish_command()

[Junio C Hamano]

Jeff King  writes:

> I don't think this is "unfortunately"; freeing the memory was the entire
> purpose in adding env_array. If you want to easily reuse the same
> environment in multiple commands, it is still perfectly fine to use
> "env" directly, like:
>
>   struct argv_array env = ARGV_ARRAY_INIT;
>   struct child_process one = CHILD_PROCESS_INIT;
>   struct child_process two = CHILD_PROCESS_INIT;
>
>   ... setup env with argv_array_push ...
>
>   one.argv = foo;
>   one.env = env.argv;
>   run_command(&one);
>
>   two.argv = bar;
>   two.env = env.argv;
>   run_command(&two);
>
>   argv_array_clear(&env);
>
> You do not get the benefit of the auto-cleanup (you have to call
> argv_array_clear yourself), but that is less bad than repeating the
> setup of "env" twice.

Yeah, the above looks like the best option, better than using a
single child_process and having to re-initialize fds and envs.

Thanks for helping.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv4 5/9] builtin/notes: Simplify early exit code in add()

[Junio C Hamano]

Johan Herland  writes:

> On Mon, Nov 10, 2014 at 9:36 PM, Junio C Hamano  wrote:
>> Johan Herland  writes:
>>
>>> Signed-off-by: Johan Herland 
>>> ---
>>>  builtin/notes.c | 12 +---
>>>  1 file changed, 5 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/builtin/notes.c b/builtin/notes.c
>>> index 1017472..f1480cf 100644
>>> --- a/builtin/notes.c
>>> +++ b/builtin/notes.c
>>> @@ -399,7 +399,7 @@ static int append_edit(int argc, const char **argv, 
>>> const char *prefix);
>>>
>>>  static int add(int argc, const char **argv, const char *prefix)
>>>  {
>>> - int retval = 0, force = 0;
>>> + int force = 0;
>>>   const char *object_ref;
>>>   struct notes_tree *t;
>>>   unsigned char object[20], new_note[20];
>>> @@ -441,6 +441,8 @@ static int add(int argc, const char **argv, const char 
>>> *prefix)
>>>
>>>   if (note) {
>>>   if (!force) {
>>> + free_note_data(&d);
>>> + free_notes(t);
>>>   if (!d.given) {
>>
>> It looks a bit strange to refer to d.given after calling a function
>> that sounds as if it is meant to clear what is recorded in and to
>> invalidate &d; yes, I can read the implementation to see that
>> d.given keeps its stale value, but that is something other people
>> may want to "clean up" later and this reference to d.given will get
>> in the way when that happens.
>
> Yes, that was obviously an oversight on my part.
>
>> At this point of the code, it makes sense to free t in preparation
>> to either switching to append codepath or erroring out, but does &d
>> have anything in it already to necessitate its freeing?
>
> Actually, no, although verifying that required double-checking that
> each of the -m/-F/-c/-C parsers which store data into &d, do in fact
> set d.given.
>
> I suggest to either move the free_note_data(&d) call below the "if
> (!d.given)" block, or reorganize into this (which at the moment reads
> better to me):
>
> if (note) {
> if (!force) {
> free_notes(t);
> if (d.given) {
> free_note_data(&d);
> return error(_("Cannot add notes. "
> "Found existing notes for object %s. "
> "Use '-f' to overwrite existing notes"),
> sha1_to_hex(object));
> }
> /*
>  * Redirect to "edit" subcommand.
>  *
>  * We only end up here if none of -m/-F/-c/-C or -f are
>  * given. The original args are therefore still in
>  * argv[0-1].
>  */
> argv[0] = "edit";
> return append_edit(argc, argv, prefix);
> }
> fprintf(stderr, _("Overwriting existing notes for object %s\n"),
> sha1_to_hex(object));
> }
>
> This keeps the two free_* calls close together, only separated by if
> (d.given), which nicely indicates whether we need to call
> free_notes_data(&d) at all.
>
> If that looks good to you, do you want a re-roll, or is it easier to
> fix up yourself?

I strongly prefer reroll for anything bigger than single-line tweaks
to avoid mistakes.

Thans.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 02/15] refs.c: return error instead of dying when locking fails during transaction

[Ronnie Sahlberg]

On Tue, Nov 11, 2014 at 2:34 AM, Jeff King  wrote:
> On Tue, Oct 21, 2014 at 01:36:47PM -0700, Ronnie Sahlberg wrote:
>
>> commit e193c10fc4f9274d1e751cfcdcc4507818e8d498 upstream.
>>
>> Change lock_ref_sha1_basic to return an error instead of dying when
>> we fail to lock a file during a transaction.
>> This function is only called from transaction_commit() and it knows how
>> to handle these failures.
>> [...]
>> - else
>> - unable_to_lock_die(ref_file, errno);
>> + else {
>> + struct strbuf err = STRBUF_INIT;
>> + unable_to_lock_message(ref_file, errno, &err);
>> + error("%s", err.buf);
>> + strbuf_reset(&err);
>> + goto error_return;
>> + }
>
> I coincidentally just wrote almost the identical patch, because this
> isn't just a cleanup; it fixes a real bug. During pack_refs, we call
> prune_ref to lock and delete the loose ref. If the lock fails, that's
> OK; that just means somebody else is updating it at the same time, and
> we can skip our pruning step. But due to the unable_to_lock_die call
> here in lock_ref_sha1_basic, the pack-refs process may die prematurely.
>
> I wonder if it is worth pulling this one out from the rest of the
> series, as it has value (and can be applied) on its own. I did some
> digging on the history of this, too. Here's the rationale I wrote:
>
>
> lock_ref_sha1_basic: do not die on locking errors
>
> lock_ref_sha1_basic is inconsistent about when it calls
> die() and when it returns NULL to signal an error. This is
> annoying to any callers that want to recover from a locking
> error.
>
> This seems to be mostly historical accident. It was added in
> 4bd18c4 (Improve abstraction of ref lock/write.,
> 2006-05-17), which returned an error in all cases except
> calling safe_create_leading_directories, in which case it
> died.  Later, 40aaae8 (Better error message when we are
> unable to lock the index file, 2006-08-12) asked
> hold_lock_file_for_update to die for us, leaving the
> resolve_ref code-path the only one which returned NULL.
>
> We tried to correct that in 5cc3cef (lock_ref_sha1(): do not
> sometimes error() and sometimes die()., 2006-09-30),
> by converting all of the die() calls into returns. But we
> missed the "die" flag passed to the lock code, leaving us
> inconsistent. This state persisted until e5c223e
> (lock_ref_sha1_basic(): if locking fails with ENOENT, retry,
> 2014-01-18). Because of its retry scheme, it does not ask
> the lock code to die, but instead manually dies with
> unable_to_lock_die().
>
> We can make this consistent with the other return paths by
> converting this to use unable_to_lock_message(), and
> returning NULL. This is safe to do because all callers
> already needed to check the return value of the function,
> since it could fail (and return NULL) for other reasons.
>
> I also have some other cleanups to lock_ref_sha1_basic's error handling.
> I'd be happy to take over this patch and send it along with those
> cleanups as a separate series.


 Sounds Good To Me.


Thanks,
Ronnie Sahlberg
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Opinions] Integrated tickets

[Holger Hellmuth]

Am 06.11.2014 um 19:45 schrieb Junio C Hamano:

This is a tangent, but I personally do not think "ticket" meshes
very well with "commit".  If you already know which commit was
problematic, why are you annotating it with a ticket before
reverting it first?


I would expect a ticket to be annotating the commit or version tag where 
the bug was found, which usually isn't the commit where the bug was 
introduced.




--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC] On watchman support

[Duy Nguyen]

I've come to the last piece to speed up "git status", watchman
support. And I realized it's not as good as I thought.

Watchman could be used for two things: to avoid refreshing the index,
and to avoid searching for ignored files. The first one can be done
(with the patch below as demonstration). And it should keep refresh
cost to near zero in the best case, the cost is proportional to the
number of modified files.

For avoiding searching for ignored files. My intention was to build on
top of untracked cache. If watchman can tell me what files are added
or deleted since last observed time, then I can invalidate just
directories that contain them, or even better, calculate ignore status
for those files only.

This is important because in reality compilers and editors tend to
update files by creating a new version then rename them, updating
directory mtime and invalidating untracked cache as a consequence. As
you edit more files (or your rebuild touches more dirs), untracked
cache performance drops (until the next "git status"). The numbers I
posted so far are the best case.

The problem with watchman is it cannot tell me "new" files since the
last observed time (let's say 'T'). If a file exists at 'T', gets
deleted then recreated, then watchman tells me it's a new file. I want
to separate those from ones that do not exist before 'T'.

David's watchman approach does not have this problem because he keeps
track of all entries under $GIT_WORK_TREE and knows which files are
truely new. But I don't really want to keep the whole file list around,
especially when watchman already manages the same list.

So we got a few options:

1) Convince watchman devs to add something to make it work

2) Fork watchman

3) Make another daemon to keep file list around, or put it in a shared
   memory.

4) Move David's watchman series forward (and maybe make use of shared
   mem for fs_cache).

5) Go with something similar to the patch below and accept untracked
   cache performance degrades from time to time

6) ??

I'm working on 1). 2) is just bad taste, listed for completeness
only. If we go with 3) and watchman starts to support Windows (seems
to be in their plan), we'll need to rework some how. And I really
don't like 3)

If 1-3 does not work out, we're left without 4) and 5). We could
support both, but proobably not worth the code complexity and should
just go with one.

And if we go with 4) we should probably think of dropping untracked
cache if watchman will support Windows in the end. 4) also has another
advantage over untracked cache, that it could speed up listing ignored
files as well as untracked files.

Comments?

-- 8< --
diff --git a/Makefile b/Makefile
index fa58a53..a2be728 100644
--- a/Makefile
+++ b/Makefile
@@ -406,6 +406,7 @@ TCLTK_PATH = wish
 XGETTEXT = xgettext
 MSGFMT = msgfmt
 PTHREAD_LIBS = -lpthread
+WATCHMAN_LIBS = -lwatchman
 PTHREAD_CFLAGS =
 GCOV = gcov
 
@@ -1453,6 +1454,13 @@ else
LIB_OBJS += thread-utils.o
 endif
 
+ifdef USE_WATCHMAN
+   LIB_H += watchman-support.h
+   LIB_OBJS += watchman-support.o
+   EXTLIBS += $(WATCHMAN_LIBS)
+   BASIC_CFLAGS += -DUSE_WATCHMAN
+endif
+
 ifdef HAVE_PATHS_H
BASIC_CFLAGS += -DHAVE_PATHS_H
 endif
@@ -,6 +2230,7 @@ GIT-BUILD-OPTIONS: FORCE
@echo NO_PERL=\''$(subst ','\'',$(subst ','\'',$(NO_PERL)))'\' >>$@
@echo NO_PYTHON=\''$(subst ','\'',$(subst ','\'',$(NO_PYTHON)))'\' >>$@
@echo NO_UNIX_SOCKETS=\''$(subst ','\'',$(subst 
','\'',$(NO_UNIX_SOCKETS)))'\' >>$@
+   @echo USE_WATCHMAN=\''$(subst ','\'',$(subst 
','\'',$(USE_WATCHMAN)))'\' >>$@
 ifdef TEST_OUTPUT_DIRECTORY
@echo TEST_OUTPUT_DIRECTORY=\''$(subst ','\'',$(subst 
','\'',$(TEST_OUTPUT_DIRECTORY)))'\' >>$@
 endif
diff --git a/builtin/update-index.c b/builtin/update-index.c
index f23ec83..1da2b15 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -882,6 +882,7 @@ int cmd_update_index(int argc, const char **argv, const 
char *prefix)
 {
int newfd, entries, has_errors = 0, line_termination = '\n';
int untracked_cache = -1;
+   int use_watchman = -1;
int read_from_stdin = 0;
int prefix_length = prefix ? strlen(prefix) : 0;
int preferred_index_format = 0;
@@ -977,6 +978,8 @@ int cmd_update_index(int argc, const char **argv, const 
char *prefix)
N_("enable/disable untracked cache")),
OPT_SET_INT(0, "force-untracked-cache", &untracked_cache,
N_("enable untracked cache without testing the 
filesystem"), 2),
+   OPT_BOOL(0, "watchman", &use_watchman,
+   N_("use or not use watchman to reduce refresh cost")),
OPT_END()
};
 
@@ -1102,6 +1105,14 @@ int cmd_update_index(int argc, const char **argv, const 
char *prefix)
the_index.cache_changed |= UNTRACKED_CHANGED;
}
 
+   if (use_watchman > 0) {
+   the_index.last_update  

Re: [PATCH 02/15] refs.c: return error instead of dying when locking fails during transaction

[Jeff King]

On Tue, Oct 21, 2014 at 01:36:47PM -0700, Ronnie Sahlberg wrote:

> commit e193c10fc4f9274d1e751cfcdcc4507818e8d498 upstream.
> 
> Change lock_ref_sha1_basic to return an error instead of dying when
> we fail to lock a file during a transaction.
> This function is only called from transaction_commit() and it knows how
> to handle these failures.
> [...]
> - else
> - unable_to_lock_die(ref_file, errno);
> + else {
> + struct strbuf err = STRBUF_INIT;
> + unable_to_lock_message(ref_file, errno, &err);
> + error("%s", err.buf);
> + strbuf_reset(&err);
> + goto error_return;
> + }

I coincidentally just wrote almost the identical patch, because this
isn't just a cleanup; it fixes a real bug. During pack_refs, we call
prune_ref to lock and delete the loose ref. If the lock fails, that's
OK; that just means somebody else is updating it at the same time, and
we can skip our pruning step. But due to the unable_to_lock_die call
here in lock_ref_sha1_basic, the pack-refs process may die prematurely.

I wonder if it is worth pulling this one out from the rest of the
series, as it has value (and can be applied) on its own. I did some
digging on the history of this, too. Here's the rationale I wrote:


lock_ref_sha1_basic: do not die on locking errors

lock_ref_sha1_basic is inconsistent about when it calls
die() and when it returns NULL to signal an error. This is
annoying to any callers that want to recover from a locking
error.

This seems to be mostly historical accident. It was added in
4bd18c4 (Improve abstraction of ref lock/write.,
2006-05-17), which returned an error in all cases except
calling safe_create_leading_directories, in which case it
died.  Later, 40aaae8 (Better error message when we are
unable to lock the index file, 2006-08-12) asked
hold_lock_file_for_update to die for us, leaving the
resolve_ref code-path the only one which returned NULL.

We tried to correct that in 5cc3cef (lock_ref_sha1(): do not
sometimes error() and sometimes die()., 2006-09-30),
by converting all of the die() calls into returns. But we
missed the "die" flag passed to the lock code, leaving us
inconsistent. This state persisted until e5c223e
(lock_ref_sha1_basic(): if locking fails with ENOENT, retry,
2014-01-18). Because of its retry scheme, it does not ask
the lock code to die, but instead manually dies with
unable_to_lock_die().

We can make this consistent with the other return paths by
converting this to use unable_to_lock_message(), and
returning NULL. This is safe to do because all callers
already needed to check the return value of the function,
since it could fail (and return NULL) for other reasons.

I also have some other cleanups to lock_ref_sha1_basic's error handling.
I'd be happy to take over this patch and send it along with those
cleanups as a separate series.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html