Re: [WIP-PATCH 1/2] send-email: create email parser subroutine

[Eric Wong]

Matthieu Moy  wrote:
> Samuel GROOT  writes:
> 
> > Parsing and processing in send-email is done in the same loop.
> >
> > To make the code more maintainable, we create two subroutines:
> > - `parse_email` to separate header and body
> > - `parse_header` to retrieve data from header
> 
> These routines are not specific to git send-email, nor to Git.
> 
> Does it make sense to use an external library, like
> http://search.cpan.org/~rjbs/Email-Simple-2.210/lib/Email/Simple.pm ,
> either by depending on it, or by copying it in Git's source tree ?

That might be overkill and increase installation/maintenance
burden.  Bundling it would probably be problematic to distros,
too.

> If not, I think it would be better to introduce an email parsing library
> in a dedicated Perl module in perl/ in our source tree, to keep
> git-send-email.perl more focused on the "send-email" logic.

Sounds good, Git.pm already has parse_mailboxes

> > +sub parse_email {
> > +   my @header = ();
> > +   my @body = ();
> > +   my $fh = shift;
> > +
> > +   # First unfold multiline header fields
> > +   while (<$fh>) {
> > +   last if /^\s*$/;
> > +   if (/^\s+\S/ and @header) {
> > +   chomp($header[$#header]);
> > +   s/^\s+/ /;
> > +   $header[$#header] .= $_;
> > +   } else {
> > +   push(@header, $_);
> > +   }
> > +   }
> > +
> > +   # Now unfold the message body
> 
> Why "unfold"? Don't you mean "split message body into a list of lines"?
> 
> > +   while (<$fh>) {
> > +   push @body, $_;
> > +   }

I'd rather avoid the loops entirely and do this:

local $/ = "\n"; # in case caller clobbers $/
@body = (<$fh>);

> > +   return (@header, @body);
> > +}


> > +   if (defined $input_format && $input_format eq 'mbox') {
> > +   if (/^Subject:\s+(.*)$/i) {
> > +   $subject = $1;
> > +   } elsif (/^From:\s+(.*)$/i) {
> > +   $from = $1;
> 
> Not sure we need thes if/elsif/ for generic headers. Email::Simple's API
> seems much simpler and general: $email->header("From");

Right.  Reading this, it would've been easier to parse headers into a
hash (normalized keys to lowercase) up front inside parse_email.
--
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

error: Can't cherry-pick into empty head

[Fabrizio Cucci]

Hello everyone,

I'm trying to understand why I'm getting the error as per subject.

The scenario is the following: I'm on the master branch (which
contains several commits) and I would like to create a new empty
branch (let's call it new-orphan) and cherry-pick only the commits
related to a specific folder (let's call it my-folder) from the master
branch.

So, I tried the following command sequence:

  master $ git checkout --orphan new-orphan
  new-orphan $ git rm --cached -r .
  new-orphan $ git clean -df

After confirming that I'm in a clean state (with "$ git status") I tried:

  new-orphan $ git rev-list --reverse master -- my-folder/ | git
cherry-pick --stdin

as suggested https://git-scm.com/docs/git-cherry-pick, but what I get
is "error: Can't cherry-pick into empty head".

What I don't really understand is:

1) if I cherry-pick a single commit instead of multiple commits,
everything works fine:

  new-orphan $ git cherry-pick 

2) if I commit something before trying the above command, everything works fine:

  new-orphan $ touch README
  new-orphan $ git add README
  new-orphan $ git commit -m "added README"
  new-orphan $ git rev-list --reverse master -- my-folder/ | git
cherry-pick --stdin

Can someone please help me understand this?

Thanks,
Fabrizio
--
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] patch-id: use starts_with() and skip_prefix()

[René Scharfe]

Get rid of magic numbers and avoid running over the end of a NUL
terminated string by using starts_with() and skip_prefix() instead
of memcmp().

Signed-off-by: Rene Scharfe 
---
 builtin/patch-id.c | 23 ++-
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/builtin/patch-id.c b/builtin/patch-id.c
index 366ce5a..a84d000 100644
--- a/builtin/patch-id.c
+++ b/builtin/patch-id.c
@@ -81,16 +81,13 @@ static int get_one_patchid(struct object_id *next_oid, 
struct object_id *result,
 
while (strbuf_getwholeline(line_buf, stdin, '\n') != EOF) {
char *line = line_buf->buf;
-   char *p = line;
+   const char *p = line;
int len;
 
-   if (!memcmp(line, "diff-tree ", 10))
-   p += 10;
-   else if (!memcmp(line, "commit ", 7))
-   p += 7;
-   else if (!memcmp(line, "From ", 5))
-   p += 5;
-   else if (!memcmp(line, "\\ ", 2) && 12 < strlen(line))
+   if (!skip_prefix(line, "diff-tree ", ) &&
+   !skip_prefix(line, "commit ", ) &&
+   !skip_prefix(line, "From ", ) &&
+   starts_with(line, "\\ ") && 12 < strlen(line))
continue;
 
if (!get_oid_hex(p, next_oid)) {
@@ -99,14 +96,14 @@ static int get_one_patchid(struct object_id *next_oid, 
struct object_id *result,
}
 
/* Ignore commit comments */
-   if (!patchlen && memcmp(line, "diff ", 5))
+   if (!patchlen && !starts_with(line, "diff "))
continue;
 
/* Parsing diff header?  */
if (before == -1) {
-   if (!memcmp(line, "index ", 6))
+   if (starts_with(line, "index "))
continue;
-   else if (!memcmp(line, "--- ", 4))
+   else if (starts_with(line, "--- "))
before = after = 1;
else if (!isalpha(line[0]))
break;
@@ -114,14 +111,14 @@ static int get_one_patchid(struct object_id *next_oid, 
struct object_id *result,
 
/* Looking for a valid hunk header?  */
if (before == 0 && after == 0) {
-   if (!memcmp(line, "@@ -", 4)) {
+   if (starts_with(line, "@@ -")) {
/* Parse next hunk, but ignore line numbers.  */
scan_hunk_header(line, , );
continue;
}
 
/* Split at the end of the patch.  */
-   if (memcmp(line, "diff ", 5))
+   if (!starts_with(line, "diff "))
break;
 
/* Else we're parsing another header.  */
-- 
2.8.3

--
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] apply: remove unused parameters from name_terminate()

[René Scharfe]

Signed-off-by: Rene Scharfe 
---
 builtin/apply.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 8e4da2e..c770d7d 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -442,7 +442,7 @@ static int is_dev_null(const char *str)
 #define TERM_SPACE 1
 #define TERM_TAB   2
 
-static int name_terminate(const char *name, int namelen, int c, int terminate)
+static int name_terminate(int c, int terminate)
 {
if (c == ' ' && !(terminate & TERM_SPACE))
return 0;
@@ -671,7 +671,7 @@ static char *find_name_common(const char *line, const char 
*def,
if (!end && isspace(c)) {
if (c == '\n')
break;
-   if (name_terminate(start, line-start, c, terminate))
+   if (name_terminate(c, terminate))
break;
}
line++;
-- 
2.8.3

--
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: [WIP-PATCH 1/2] send-email: create email parser subroutine

[Matthieu Moy]

Samuel GROOT  writes:

> Parsing and processing in send-email is done in the same loop.
>
> To make the code more maintainable, we create two subroutines:
> - `parse_email` to separate header and body
> - `parse_header` to retrieve data from header

These routines are not specific to git send-email, nor to Git.

Does it make sense to use an external library, like
http://search.cpan.org/~rjbs/Email-Simple-2.210/lib/Email/Simple.pm ,
either by depending on it, or by copying it in Git's source tree ?

If not, I think it would be better to introduce an email parsing library
in a dedicated Perl module in perl/ in our source tree, to keep
git-send-email.perl more focused on the "send-email" logic.

> +sub parse_email {
> + my @header = ();
> + my @body = ();
> + my $fh = shift;
> +
> + # First unfold multiline header fields
> + while (<$fh>) {
> + last if /^\s*$/;
> + if (/^\s+\S/ and @header) {
> + chomp($header[$#header]);
> + s/^\s+/ /;
> + $header[$#header] .= $_;
> + } else {
> + push(@header, $_);
> + }
> + }
> +
> + # Now unfold the message body

Why "unfold"? Don't you mean "split message body into a list of lines"?

> + while (<$fh>) {
> + push @body, $_;
> + }
> +
> + return (@header, @body);
> +}

Please document your functions. See e.g. perl/Git.pm for an example of
what perldoc allows you to do.

This also lacks tests. One advantage of having a clean API is that it
also makes it simpler to do unit-testing. Grep "Test::More" in t/ to see
some existing unit-tests in Perl.

> + foreach(@_) {

Style: space before (.

> + if (defined $input_format && $input_format eq 'mbox') {
> + if (/^Subject:\s+(.*)$/i) {
> + $subject = $1;
> + } elsif (/^From:\s+(.*)$/i) {
> + $from = $1;

Not sure we need thes if/elsif/ for generic headers. Email::Simple's API
seems much simpler and general: $email->header("From");

> + foreach my $addr (parse_address_line($1)) {
> + push @to, $addr;
> + }

3 lines for an array concatenation in a high-level language. It looks
like 2 more than needed ;-).

> + }
> +
> + } else {

Useless blank line.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
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 v2 8/8] grep: -W: don't extend context to trailing empty lines

[René Scharfe]

Empty lines between functions are shown by grep -W, as it considers them
to be part of the function preceding them.  They are not interesting in
most languages.  The previous patches stopped showing them for diff -W.

Stop showing empty lines trailing a function with grep -W.  Grep scans
the lines of a buffer from top to bottom and prints matching lines
immediately.  Thus we need to peek ahead in order to determine if an
empty line is part of a function body and worth showing or not.

Remember how far ahead we peeked in order to avoid having to do so
repeatedly when handling multiple consecutive empty lines.

Signed-off-by: Rene Scharfe 
---
 grep.c  | 28 ++--
 t/t7810-grep.sh |  2 +-
 2 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/grep.c b/grep.c
index ec6f7ff..1e15b62 100644
--- a/grep.c
+++ b/grep.c
@@ -1396,9 +1396,17 @@ static int fill_textconv_grep(struct userdiff_driver 
*driver,
return 0;
 }
 
+static int is_empty_line(const char *bol, const char *eol)
+{
+   while (bol < eol && isspace(*bol))
+   bol++;
+   return bol == eol;
+}
+
 static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int 
collect_hits)
 {
char *bol;
+   char *peek_bol = NULL;
unsigned long left;
unsigned lno = 1;
unsigned last_hit = 0;
@@ -1543,8 +1551,24 @@ static int grep_source_1(struct grep_opt *opt, struct 
grep_source *gs, int colle
show_function = 1;
goto next_line;
}
-   if (show_function && match_funcname(opt, gs, bol, eol))
-   show_function = 0;
+   if (show_function && (!peek_bol || peek_bol < bol)) {
+   unsigned long peek_left = left;
+   char *peek_eol = eol;
+
+   /*
+* Trailing empty lines are not interesting.
+* Peek past them to see if they belong to the
+* body of the current function.
+*/
+   peek_bol = bol;
+   while (is_empty_line(peek_bol, peek_eol)) {
+   peek_bol = peek_eol + 1;
+   peek_eol = end_of_line(peek_bol, _left);
+   }
+
+   if (match_funcname(opt, gs, peek_bol, peek_eol))
+   show_function = 0;
+   }
if (show_function ||
(last_hit && lno <= last_hit + opt->post_context)) {
/* If the last hit is within the post context,
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 26912dc..960425a 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -748,7 +748,7 @@ hello.c-#include 
 hello.c:#include 
 EOF
 
-test_expect_failure 'grep -W shows no trailing empty lines' '
+test_expect_success 'grep -W shows no trailing empty lines' '
git grep -W stdio >actual &&
test_cmp expected actual
 '
-- 
2.8.3

--
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 v2 7/8] t7810: add test for grep -W and trailing empty context lines

[René Scharfe]

Add a test demonstrating that git grep -W prints empty lines following
the function context we're actually interested in.  The modified test
file makes it necessary to adjust three unrelated test cases.

Signed-off-by: Rene Scharfe 
---
 t/t7810-grep.sh | 19 ---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 1e72971..26912dc 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -9,7 +9,9 @@ test_description='git grep various.
 . ./test-lib.sh
 
 cat >hello.c <
 #include 
+
 int main(int argc, const char **argv)
 {
printf("Hello world.\n");
@@ -715,6 +717,7 @@ test_expect_success 'grep -p' '
 
 cat >expected <
+hello.c-
 hello.c=int main(int argc, const char **argv)
 hello.c-{
 hello.c-   printf("Hello world.\n");
@@ -741,6 +744,16 @@ test_expect_success 'grep -W' '
 '
 
 cat >expected <
+hello.c:#include 
+EOF
+
+test_expect_failure 'grep -W shows no trailing empty lines' '
+   git grep -W stdio >actual &&
+   test_cmp expected actual
+'
+
+cat >expected char **argv)
-6: /* char ?? */
+4:int main(int argc, const char **argv)
+8: /* char ?? */
 
 hello_world
 3:Hello_world
@@ -1340,7 +1353,7 @@ test_expect_success 'grep --color -e A --and --not -e B 
with context' '
 '
 
 cat >expected <
+hello.c-
 hello.c=int main(int argc, const char **argv)
 hello.c-{
 hello.c:   printf("Hello world.\n");
-- 
2.8.3

--
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 v2 6/8] xdiff: don't trim common tail with -W

[René Scharfe]

The function trim_common_tail() exits early if context lines are
requested.  If -U0 and -W are specified together then it can still trim
context lines that might belong to a changed function.  As a result
that function is shown incompletely.

Fix that by calling trim_common_tail() only if no function context or
fixed context is requested.  The parameter ctx is no longer needed now;
remove it.

While at it fix an outdated comment as well.

Signed-off-by: Rene Scharfe 
---
 t/t4051-diff-function-context.sh |  2 +-
 xdiff-interface.c| 10 --
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/t/t4051-diff-function-context.sh b/t/t4051-diff-function-context.sh
index 9fe590f..88a3308 100755
--- a/t/t4051-diff-function-context.sh
+++ b/t/t4051-diff-function-context.sh
@@ -145,7 +145,7 @@ test_expect_success ' context includes begin' '
grep "^ .*Begin of first part" long_common_tail.diff
 '
 
-test_expect_failure ' context includes end' '
+test_expect_success ' context includes end' '
grep "^ .*End of second part" long_common_tail.diff
 '
 
diff --git a/xdiff-interface.c b/xdiff-interface.c
index 54236f2..f34ea76 100644
--- a/xdiff-interface.c
+++ b/xdiff-interface.c
@@ -100,9 +100,9 @@ static int xdiff_outf(void *priv_, mmbuffer_t *mb, int nbuf)
 
 /*
  * Trim down common substring at the end of the buffers,
- * but leave at least ctx lines at the end.
+ * but end on a complete line.
  */
-static void trim_common_tail(mmfile_t *a, mmfile_t *b, long ctx)
+static void trim_common_tail(mmfile_t *a, mmfile_t *b)
 {
const int blk = 1024;
long trimmed = 0, recovered = 0;
@@ -110,9 +110,6 @@ static void trim_common_tail(mmfile_t *a, mmfile_t *b, long 
ctx)
char *bp = b->ptr + b->size;
long smaller = (a->size < b->size) ? a->size : b->size;
 
-   if (ctx)
-   return;
-
while (blk + trimmed <= smaller && !memcmp(ap - blk, bp - blk, blk)) {
trimmed += blk;
ap -= blk;
@@ -134,7 +131,8 @@ int xdi_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const 
*xpp, xdemitconf_t co
if (mf1->size > MAX_XDIFF_SIZE || mf2->size > MAX_XDIFF_SIZE)
return -1;
 
-   trim_common_tail(, , xecfg->ctxlen);
+   if (!xecfg->ctxlen && !(xecfg->flags & XDL_EMIT_FUNCCONTEXT))
+   trim_common_tail(, );
 
return xdl_diff(, , xpp, xecfg, xecb);
 }
-- 
2.8.3

--
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: [WIP-PATCH 0/2] send-email: refactor the email parser loop

[Matthieu Moy]

Eric Wong  writes:

> Samuel GROOT  wrote:
>
>>(mbox) Adding cc: A  from line 'Cc: 
>> A, One'
>>(mbox) Adding cc: One  from line 'Cc: 
>> A, One'
>> 
>> Though `git send-email` now outputs something like:
>> 
>>(mbox) Adding cc: A  from line 'Cc: 
>> A'
>>(mbox) Adding cc: One  from line 'Cc: 
>> One'
> I actually like neither, and would prefer something shorter:
>
> (mbox) Adding cc: A  from Cc: header
> (mbox) Adding cc: One  from Cc: header
> (mbox) Adding cc: SoB  from Signed-off-by: trailer
>
> That way, there's no redundant addresses in each line and less
> likely to wrap.

I agree with this. Actually, I'd even say that the current output of
"git send-email" looks sloppy, and internal refactoring may be a good
opportunity to get it cleaner.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
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 v2 5/8] xdiff: -W: don't include common trailing empty lines in context

[René Scharfe]

Empty lines between functions are shown by diff -W, as it considers them
to be part of the function preceding them.  They are not interesting in
most languages.  The previous patch stopped showing them in the special
case of a function added at the end of a file.

Stop extending context to those empty lines by skipping back over them
from the start of the next function.

Signed-off-by: Rene Scharfe 
---
 t/t4051-diff-function-context.sh | 4 ++--
 xdiff/xemit.c| 2 ++
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/t/t4051-diff-function-context.sh b/t/t4051-diff-function-context.sh
index b191c655..9fe590f 100755
--- a/t/t4051-diff-function-context.sh
+++ b/t/t4051-diff-function-context.sh
@@ -85,7 +85,7 @@ test_expect_success ' context does not include preceding 
empty lines' '
test "$(first_context_line i1 + xche->chg1,
 xe->xdf1.nrec);
+   while (fe1 > 0 && is_empty_rec(>xdf1, fe1 - 1))
+   fe1--;
if (fe1 < 0)
fe1 = xe->xdf1.nrec;
if (fe1 > e1) {
-- 
2.8.3

--
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 v2 4/8] xdiff: ignore empty lines before added functions with -W

[René Scharfe]

If a new function and a preceding empty line is appended, diff -W shows
the previous function in full in order to provide context for that empty
line.  In most languages empty lines between sections are not
interesting in and off themselves and showing a whole extra function for
them is not what we want.

Skip empty lines when checking of the appended chunk starts with a
function line, thereby avoiding to extend the context just for them.

Helped-by: Ramsay Jones 
Signed-off-by: Rene Scharfe 
---
 t/t4051-diff-function-context.sh |  2 +-
 xdiff/xemit.c| 22 --
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/t/t4051-diff-function-context.sh b/t/t4051-diff-function-context.sh
index d78461d..b191c655 100755
--- a/t/t4051-diff-function-context.sh
+++ b/t/t4051-diff-function-context.sh
@@ -117,7 +117,7 @@ test_expect_success ' context includes end' '
grep "^[+].*End of first part" appended.diff
 '
 
-test_expect_failure ' context does not include other functions' '
+test_expect_success ' context does not include other functions' '
test $(grep -c "^[ +-].*Begin" appended.diff) -le 1
 '
 
diff --git a/xdiff/xemit.c b/xdiff/xemit.c
index 969100d..29cec12 100644
--- a/xdiff/xemit.c
+++ b/xdiff/xemit.c
@@ -155,6 +155,18 @@ static long get_func_line(xdfenv_t *xe, xdemitconf_t const 
*xecfg,
return -1;
 }
 
+static int is_empty_rec(xdfile_t *xdf, long ri)
+{
+   const char *rec;
+   long len = xdl_get_rec(xdf, ri, );
+
+   while (len > 0 && XDL_ISSPACE(*rec)) {
+   rec++;
+   len--;
+   }
+   return !len;
+}
+
 int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,
  xdemitconf_t const *xecfg) {
long s1, s2, e1, e2, lctx;
@@ -176,12 +188,18 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, 
xdemitcb_t *ecb,
/* Appended chunk? */
if (i1 >= xe->xdf1.nrec) {
char dummy[1];
+   long i2 = xch->i2;
 
/*
 * We don't need additional context if
-* a whole function was added.
+* a whole function was added, possibly
+* starting with empty lines.
 */
-   if (match_func_rec(>xdf2, xecfg, xch->i2,
+   while (i2 < xe->xdf2.nrec &&
+  is_empty_rec(>xdf2, i2))
+   i2++;
+   if (i2 < xe->xdf2.nrec &&
+   match_func_rec(>xdf2, xecfg, i2,
   dummy, sizeof(dummy)) >= 0)
goto post_context_calculation;
 
-- 
2.8.3

--
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: [RFC-PATCH v2 2/2] send-email: quote-email quotes the message body

[Matthieu Moy]

Tom Russello  writes:

> Currently, `send-email` without `--compose` implies `--annotate`.

I don't get it. Did you mean s/without/with/? Even if so, this is not
exactly true: "git send-email --compose -1" will open the editor only
for the cover-letter, while adding --annotate will also open it for the
patch.

> Keeping that behavior when using `--quote-email` populates the patch file with
> the quoted message body, and the patch is saved no matter what. If the user
> closes his editor and then exits `send-email`, changes will be saved.
>
> Should we keep the current behavior for the user, keeping the changes 
> (including
> the quoted message body) in the patch, or should we discard them?

(Note: we discussed this off-list already, but I'll try to summarize my
thoughts here)

I don't have strong opinion on this, but I think there's a difference
between launching the editor directly on the input patch files
(resulting in _user_'s edit being done directly on them) and having the
script modify it in-place (resulting in automatic changes done directly
on the user's files).

I usually use "git send-email" directly without using "git
format-patch", so I'm not the best juge. But I can imagine a flow like

1) run "git send-email *.patch"

2) start editting

3) notice there's something wrong, give up for now (answer 'q' when git
   send-email prompts for confirmation, or kill it via Control-C in a
   terminal)

4) run "git send-email *.patch" again

5) be happy that changes done at 2) are still there.

With --quote-email, it's different. The scenario above would result in

5') WTF, why is the email quoted twice?

Unfortunately, I don't really have a solution for this. My first thought
was that we should copy the files to a temporary location before
starting the editor (that what I'm used to when using "git send-email"
without "git format-patch"), but that would prevent 5) above.

> @@ -109,7 +109,10 @@ is not set, this will be prompted for.
>  --quote-email=::
>   Reply to the given email and automatically populate the "To:", "Cc:" and
>   "In-Reply-To:" fields. If `--compose` is set, this will also fill the
> - subject field with "Re: ['s subject]".
> + subject field with "Re: ['s subject]" and quote the message 
> body
> + of .

I'd add "in the introductory message".

> + while (<$fh>) {
> + # Only for files containing crlf line endings
> + s/\r//g;

The comment doesn't really say what it does.

What about "turn crlf line endings into lf-only"?

>  } elsif ($annotate) {
> - do_edit(@files);
> + if ($quote_email) {
> + my $quote_email_filename = ($repo ?
> + tempfile(".gitsendemail.msg.XX",
> + DIR => $repo->repo_path()) :
> + tempfile(".gitsendemail.msg.XX",
> + DIR => "."))[1];
> +
> + do_insert_quoted_message($quote_email_filename, $files[0]);
> +
> + my $tmp = $files[0];
> + $files[0] = $quote_email_filename;
> +
> + do_edit(@files);
> +
> + # Erase the original patch
> + move($quote_email_filename, $tmp);
> + $files[0] = $tmp;

When writing comment, always try to ask the question "why?" more than
"what?". This part is possibly controversial, think about a contributor
finding this piece of code later without having followed the current
conversation. He'd probably expect an explanation about why you need a
temp file here and not elsewhere.

> + open my $c, "<", $original_file
> + or die "Failed to open $original_file : " . $!;
> +
> + open my $c2, ">", $tmp_file
> + or die "Failed to open $tmp_file : " . $!;

No space before :.

> --- a/t/t9001-send-email.sh
> +++ b/t/t9001-send-email.sh
> @@ -1916,6 +1916,12 @@ test_expect_success $PREREQ 'Fields with --quote-email 
> are correct' '
>   echo "$cc_adr" | grep c...@example.com
>  '
>  
> +test_expect_success $PREREQ 'correct quoted message with --quote-email' '
> + grep "On Sat, 12 Jun 2010 15:53:58 +0200, aut...@example.com wrote:" 
> msgtxt1 &&
> + grep "> Have you seen my previous email?" msgtxt1 &&
> + grep ">> Previous content" msgtxt1
> +'

When the spec says "if --compose ... then ...", "after the triple-dash",
and "in the first patch", one would expect at least one test with
--compose and one without, something to check that the insertion was
done below the triple-dash, and one test with two patches, checking that
the second patch is not altered by --quote-email.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
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 v2 3/8] xdiff: handle appended chunks better with -W

[René Scharfe]

If lines are added at the end of a file, diff -W shows the whole file.
That's because get_func_line() only considers the pre-image and gives up
if it sees a record index beyond its end.

Consider the post-image as well to see if the added lines already make
up a full function.  If it doesn't then search for the previous function
line by starting from the bottom of the pre-image, thereby avoiding to
confuse get_func_line().

Reuse the existing label called "again", as it's exactly where we need
to jump to when we're done handling the pre-context, but rename it to
"post_context_calculation" in order to document its new purpose better.

Reported-by: Junio C Hamano 
Initial-patch-by: Junio C Hamano 
Signed-off-by: Rene Scharfe 
---
 t/t4051-diff-function-context.sh |  2 +-
 xdiff/xemit.c| 27 ---
 2 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/t/t4051-diff-function-context.sh b/t/t4051-diff-function-context.sh
index a3adba4..d78461d 100755
--- a/t/t4051-diff-function-context.sh
+++ b/t/t4051-diff-function-context.sh
@@ -131,7 +131,7 @@ test_expect_success ' context includes end' '
grep "^[+].*End of second part" extended.diff
 '
 
-test_expect_failure ' context does not include other functions' '
+test_expect_success ' context does not include other functions' '
test $(grep -c "^[ +-].*Begin" extended.diff) -le 2
 '
 
diff --git a/xdiff/xemit.c b/xdiff/xemit.c
index 0c87637..969100d 100644
--- a/xdiff/xemit.c
+++ b/xdiff/xemit.c
@@ -171,7 +171,28 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, 
xdemitcb_t *ecb,
s2 = XDL_MAX(xch->i2 - xecfg->ctxlen, 0);
 
if (xecfg->flags & XDL_EMIT_FUNCCONTEXT) {
-   long fs1 = get_func_line(xe, xecfg, NULL, xch->i1, -1);
+   long fs1, i1 = xch->i1;
+
+   /* Appended chunk? */
+   if (i1 >= xe->xdf1.nrec) {
+   char dummy[1];
+
+   /*
+* We don't need additional context if
+* a whole function was added.
+*/
+   if (match_func_rec(>xdf2, xecfg, xch->i2,
+  dummy, sizeof(dummy)) >= 0)
+   goto post_context_calculation;
+
+   /*
+* Otherwise get more context from the
+* pre-image.
+*/
+   i1 = xe->xdf1.nrec - 1;
+   }
+
+   fs1 = get_func_line(xe, xecfg, NULL, i1, -1);
if (fs1 < 0)
fs1 = 0;
if (fs1 < s1) {
@@ -180,7 +201,7 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, 
xdemitcb_t *ecb,
}
}
 
- again:
+ post_context_calculation:
lctx = xecfg->ctxlen;
lctx = XDL_MIN(lctx, xe->xdf1.nrec - (xche->i1 + xche->chg1));
lctx = XDL_MIN(lctx, xe->xdf2.nrec - (xche->i2 + xche->chg2));
@@ -209,7 +230,7 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, 
xdemitcb_t *ecb,
if (l <= e1 ||
get_func_line(xe, xecfg, NULL, l, e1) < 0) {
xche = xche->next;
-   goto again;
+   goto post_context_calculation;
}
}
}
-- 
2.8.3

--
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 v2 2/8] xdiff: factor out match_func_rec()

[René Scharfe]

Add match_func_rec(), a helper that wraps accessing a record and calling
the appropriate function for checking if it contains a function line.

Signed-off-by: Rene Scharfe 
---
 xdiff/xemit.c | 15 +++
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/xdiff/xemit.c b/xdiff/xemit.c
index 993724b..0c87637 100644
--- a/xdiff/xemit.c
+++ b/xdiff/xemit.c
@@ -120,6 +120,16 @@ static long def_ff(const char *rec, long len, char *buf, 
long sz, void *priv)
return -1;
 }
 
+static long match_func_rec(xdfile_t *xdf, xdemitconf_t const *xecfg, long ri,
+  char *buf, long sz)
+{
+   const char *rec;
+   long len = xdl_get_rec(xdf, ri, );
+   if (!xecfg->find_func)
+   return def_ff(rec, len, buf, sz, xecfg->find_func_priv);
+   return xecfg->find_func(rec, len, buf, sz, xecfg->find_func_priv);
+}
+
 struct func_line {
long len;
char buf[80];
@@ -128,7 +138,6 @@ struct func_line {
 static long get_func_line(xdfenv_t *xe, xdemitconf_t const *xecfg,
  struct func_line *func_line, long start, long limit)
 {
-   find_func_t ff = xecfg->find_func ? xecfg->find_func : def_ff;
long l, size, step = (start > limit) ? -1 : 1;
char *buf, dummy[1];
 
@@ -136,9 +145,7 @@ static long get_func_line(xdfenv_t *xe, xdemitconf_t const 
*xecfg,
size = func_line ? sizeof(func_line->buf) : sizeof(dummy);
 
for (l = start; l != limit && 0 <= l && l < xe->xdf1.nrec; l += step) {
-   const char *rec;
-   long reclen = xdl_get_rec(>xdf1, l, );
-   long len = ff(rec, reclen, buf, size, xecfg->find_func_priv);
+   long len = match_func_rec(>xdf1, xecfg, l, buf, size);
if (len >= 0) {
if (func_line)
func_line->len = len;
-- 
2.8.3

--
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 v2 1/8] t4051: rewrite, add more tests

[René Scharfe]

Remove the tests that checked diff -W output against a fixed expected
result and replace them with more focused checks of desired properties
of the created diffs.  That way we get more detailed and meaningful
diagnostics.

Store test file contents in files in a subdirectory in order to avoid
cluttering the test script with them.

Use tagged commits to store the changes to test diff -W against instead
of using changes to the worktree.  Use the worktree instead to try and
apply the generated patch in order to validate it.

Document unwanted features: trailing empty lines, too much context for
appended functions, insufficient context at the end with -U0.

Signed-off-by: Rene Scharfe 
---
 t/t4051-diff-function-context.sh | 216 +--
 t/t4051/appended1.c  |  15 +++
 t/t4051/appended2.c  |  35 +++
 t/t4051/dummy.c  |   7 ++
 t/t4051/hello.c  |  21 
 t/t4051/includes.c   |  20 
 6 files changed, 240 insertions(+), 74 deletions(-)
 create mode 100644 t/t4051/appended1.c
 create mode 100644 t/t4051/appended2.c
 create mode 100644 t/t4051/dummy.c
 create mode 100644 t/t4051/hello.c
 create mode 100644 t/t4051/includes.c

diff --git a/t/t4051-diff-function-context.sh b/t/t4051-diff-function-context.sh
index 001d678..a3adba4 100755
--- a/t/t4051-diff-function-context.sh
+++ b/t/t4051-diff-function-context.sh
@@ -3,90 +3,158 @@
 test_description='diff function context'
 
 . ./test-lib.sh
-. "$TEST_DIRECTORY"/diff-lib.sh
 
+dir="$TEST_DIRECTORY/t4051"
 
-cat <<\EOF >hello.c
-#include 
-
-static int a(void)
-{
-   /*
-* Dummy.
-*/
+commit_and_tag () {
+   message=$1 &&
+   shift &&
+   git add $@ &&
+   test_tick &&
+   git commit -m $message &&
+   git tag $message
 }
 
-static int hello_world(void)
-{
-   /* Classic. */
-   printf("Hello world.\n");
-
-   /* Success! */
-   return 0;
+first_context_line () {
+   awk '
+   found {print; exit}
+   /^@@/ {found = 1}
+   '
 }
-static int b(void)
-{
-   /*
-* Dummy, too.
-*/
+
+last_context_line () {
+   sed -n '$ p'
 }
 
-int main(int argc, char **argv)
-{
-   a();
-   b();
-   return hello_world();
+check_diff () {
+   name=$1
+   desc=$2
+   options="-W $3"
+
+   test_expect_success "$desc" '
+   git diff $options "$name^" "$name" >"$name.diff"
+   '
+
+   test_expect_success ' diff applies' '
+   test_when_finished "git reset --hard" &&
+   git checkout --detach "$name^" &&
+   git apply "$name.diff" &&
+   git diff --exit-code "$name"
+   '
 }
-EOF
 
 test_expect_success 'setup' '
-   git add hello.c &&
-   test_tick &&
-   git commit -m initial &&
-
-   grep -v Classic hello.c.new &&
-   mv hello.c.new hello.c
-'
-
-cat <<\EOF >expected
-diff --git a/hello.c b/hello.c
 a/hello.c
-+++ b/hello.c
-@@ -10,8 +10,7 @@ static int a(void)
- static int hello_world(void)
- {
--  /* Classic. */
-   printf("Hello world.\n");
- 
-   /* Success! */
-   return 0;
- }
-EOF
-
-test_expect_success 'diff -U0 -W' '
-   git diff -U0 -W >actual &&
-   compare_diff_patch actual expected
-'
-
-cat <<\EOF >expected
-diff --git a/hello.c b/hello.c
 a/hello.c
-+++ b/hello.c
-@@ -9,9 +9,8 @@ static int a(void)
- 
- static int hello_world(void)
- {
--  /* Classic. */
-   printf("Hello world.\n");
- 
-   /* Success! */
-   return 0;
- }
-EOF
-
-test_expect_success 'diff -W' '
-   git diff -W >actual &&
-   compare_diff_patch actual expected
+   cat "$dir/includes.c" "$dir/dummy.c" "$dir/dummy.c" "$dir/hello.c" \
+   "$dir/dummy.c" "$dir/dummy.c" >file.c &&
+   commit_and_tag initial file.c &&
+
+   grep -v "delete me from hello" file.c.new &&
+   mv file.c.new file.c &&
+   commit_and_tag changed_hello file.c &&
+
+   grep -v "delete me from includes" file.c.new &&
+   mv file.c.new file.c &&
+   commit_and_tag changed_includes file.c &&
+
+   cat "$dir/appended1.c" >>file.c &&
+   commit_and_tag appended file.c &&
+
+   cat "$dir/appended2.c" >>file.c &&
+   commit_and_tag extended file.c &&
+
+   grep -v "Begin of second part" file.c.new &&
+   mv file.c.new file.c &&
+   commit_and_tag long_common_tail file.c
+'
+
+check_diff changed_hello 'changed function'
+
+test_expect_success ' context includes begin' '
+   grep "^ .*Begin of hello" changed_hello.diff
+'
+
+test_expect_success ' context includes end' '
+   grep "^ .*End of hello" changed_hello.diff
+'
+
+test_expect_success ' context does not include other functions' '
+   test $(grep -c "^[ +-].*Begin" changed_hello.diff) -le 1
+'
+
+test_expect_success ' context does not include preceding empty lines' '
+   test "$(first_context_line 

Re: [Bug?] log -p -W showing the whole file for a patch that adds to the end?

[René Scharfe]

Am 21.05.2016 um 20:42 schrieb René Scharfe:
> Am 11.05.2016 um 00:51 schrieb Junio C Hamano:
>> The helper function get_func_line() however gets confused when a
>> hunk adds a new function at the very end, and returns -1 to signal
>> that it did not find a suitable "function header line", i.e. the
>> beginning of previous function.  The caller then takes this signal
>> and shows from the very beginning of the file.  We end up showing
>> the entire file, starting from the very beginning to the end of the
>> newly added lines.
> 
> In this case we need to look at the added lines in the post-image to
> see if the original context suffices.  We currently only look at the
> pre-image.  And if we have to extend the context simply search from the
> bottom of the pre-image.  That's what the second patch does; the first
> one is just a small preparation.
> 
> The last three patches introduce special handling of empty lines.
> Before them the code for -W only distinguished between function lines
> and non-function lines.  Not allowing empty lines between functions to
> extend the context with -W is most useful in the case of appended full
> functions -- otherwise we'd get the preceding function shown as well as
> it "belongs" to the empty line separating them.
> 
> And if we do that then it's easy and more consistent to stop showing
> empty lines trailing functions with -W (unless they're already included
> in the one requested with -u/-U, of course).  Doing the same for grep
> then is only fair.
> 
> Considering empty lines as uninteresting collides with languages like
> Whitespace.  I'm not sure -W was useful for it to begin with, though.
> Any other possible downsides?
> 
> NB: Comments are still not handled specially.  That means they are
> considered to be part of the preceding function.  Fixing that is not in
> scope for this series.  (And I'm not sure it would be worth the hassle.)
> 
>diff: factor out match_func_rec()
>diff: handle appended chunks better with -W
>diff: ignore empty lines before added functions with -W
>diff: don't include common trailing empty lines with -W
>grep: don't extend context to trailing empty lines with -W
> 
>   grep.c| 28 --
>   xdiff/xemit.c | 63 
> ---
>   2 files changed, 82 insertions(+), 9 deletions(-)

And here's the second round, this time with tests, a fix for a
signedness issue found by Ramsay, a small cleanup of the new function
is_empty_rec() and a new fix for the case of using git diff -W -U0.
An interdiff for the files touched by the original series is at the
bottom.


  t4051: rewrite, add more tests
  xdiff: factor out match_func_rec()
  xdiff: handle appended chunks better with -W
  xdiff: ignore empty lines before added functions with -W
  xdiff: -W: don't include common trailing empty lines in context
  xdiff: don't trim common tail with -W
  t7810: add test for grep -W and trailing empty context lines
  grep: -W: don't extend context to trailing empty lines

 grep.c   |  28 -
 t/t4051-diff-function-context.sh | 216 +--
 t/t4051/appended1.c  |  15 +++
 t/t4051/appended2.c  |  35 +++
 t/t4051/dummy.c  |   7 ++
 t/t4051/hello.c  |  21 
 t/t4051/includes.c   |  20 
 t/t7810-grep.sh  |  19 +++-
 xdiff-interface.c|  10 +-
 xdiff/xemit.c|  62 +--
 10 files changed, 341 insertions(+), 92 deletions(-)
 create mode 100644 t/t4051/appended1.c
 create mode 100644 t/t4051/appended2.c
 create mode 100644 t/t4051/dummy.c
 create mode 100644 t/t4051/hello.c
 create mode 100644 t/t4051/includes.c


diff --git a/xdiff/xemit.c b/xdiff/xemit.c
index d0c0738..bfa53d3 100644
--- a/xdiff/xemit.c
+++ b/xdiff/xemit.c
@@ -155,12 +155,12 @@ static long get_func_line(xdfenv_t *xe, xdemitconf_t 
const *xecfg,
return -1;
 }
 
-static int is_empty_rec(xdfile_t *xdf, xdemitconf_t const *xecfg, long ri)
+static int is_empty_rec(xdfile_t *xdf, long ri)
 {
const char *rec;
long len = xdl_get_rec(xdf, ri, );
 
-   while (len > 0 && isspace(*rec)) {
+   while (len > 0 && XDL_ISSPACE(*rec)) {
rec++;
len--;
}
@@ -196,7 +196,7 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, 
xdemitcb_t *ecb,
 * starting with empty lines.
 */
while (i2 < xe->xdf2.nrec &&
-  is_empty_rec(>xdf2, xecfg, i2))
+  is_empty_rec(>xdf2, i2))
i2++;
if (i2 < xe->xdf2.nrec &&
match_func_rec(>xdf2, xecfg, i2,
@@ -231,8 +231,7 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t 

Re: [RFC-PATCH v2 1/2] send-email: quote-email populates the fields

[Matthieu Moy]

Tom Russello  writes:

> --- a/Documentation/git-send-email.txt
> +++ b/Documentation/git-send-email.txt
> @@ -106,6 +106,11 @@ illustration below where `[PATCH v2 0/3]` is in reply to 
> `[PATCH 0/2]`:
>  Only necessary if --compose is also set.  If --compose
>  is not set, this will be prompted for.
>  
> +--quote-email=::
> + Reply to the given email and automatically populate the "To:", "Cc:" and
> + "In-Reply-To:" fields.

I think this is a bit too technical for a user documentation. To: and
Cc: is OK, but people need not know about "In-Reply-To:" to understand
this. See what the doc of --in-reply-to says. If you want to be
technical, you'd need to mention the References: field too.

Talking about Reference: field, something your patch could do is to add
all references in  to the references of the new email (see
what a mailer is doing when replying). This way, the recipient can still
get threading if the last message being replied-to is missing.

> +"Re: ['s subject]".

Perhaps `Re: ...` instead of double-quotes.

> +if ($quote_email) {
> + my $error = validate_patch($quote_email);
> + $error and die "fatal: $quote_email: $error\nwarning: no patches were 
> sent\n";

I know it's done this way elsewhere, but I don't like this "$error and
die", I'd rather see a proper if here.

> + if (defined $input_format && $input_format eq 'mbox') {

To me, the input format refers to patch files, not the .

I'm not sure anyone still use the "lots of email" format, and you are
not testing it. So, this is claiming that we have a feature without
being sure we have it nor that anyone's ever going to use it.

I'd just drop this "if" and the "else" branch, and just assume the email
file is a normal email file.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
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] blame.c: don't drop origin blobs as eagerly

[David Kastrup]

Johannes Schindelin  writes:

> Hi David,
>
> On Sat, 28 May 2016, David Kastrup wrote:
>
>> > The short version of your answer is that you will leave this patch in
>> > its current form and address none of my concerns because you moved on,
>> > correct? If so, that's totally okay, it just needs to be spelled out.
>> 
>> Yes, that's it.  You'll notice that the code change itself is both
>> minuscule as well purely functional, so it contains nothing
>> copyrightable.
>
> That is unfortunately an interpretation of the law that would need to
> come from a professional lawyer.

A professional lawyer would laugh at "Signed-off-by:" being of more
legal significance than a written record of intent but of course you
know that.  This is mere bluster.

> As it is, the patch was clearly authored by you, and anybody else who
> would claim authorship would most likely be breaking the law.

The _diff_ is not "clearly authored" by me but just the simplest
expression of the intent.  The commit message is clearly authored by me
but is not acceptable anyway.  Whoever gets to write an acceptable
commit message is up for all copyrightable credit in my book.  Feel free
to keep the authorship if you really want to, but when replacing the
commit message it is not a particularly accurate attribution.

> So I won't touch it.

Signed-off-by: David Kastrup 

You don't get more than that for other patches either and it's a few
bytes compared to a mail conversation.  Here is a PGP signature on top.

As I said: I am not going to put more work into it anyway and if it's an
occasion for theatralics, it has at least accomplished something.

-- 
David Kastrup


signature.asc
Description: PGP signature

ssh key

[matveevma]

Hi,

Can i add SSH id_rsa.pub to GIT by shell terminal?

Thank you!

Mikhail.
--
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

IT- Desk Password Update

[hcorn0614]

Dear user

Due to the congestion in all users accounts, Would be shutting down all unused 
accounts. click here to 
activate Your account Now.

Sincerely,
IT- Desk Service Desk


--
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] blame.c: don't drop origin blobs as eagerly

[Johannes Schindelin]

Hi David,

On Sat, 28 May 2016, David Kastrup wrote:

> > The short version of your answer is that you will leave this patch in
> > its current form and address none of my concerns because you moved on,
> > correct? If so, that's totally okay, it just needs to be spelled out.
> 
> Yes, that's it.  You'll notice that the code change itself is both
> minuscule as well purely functional, so it contains nothing
> copyrightable.

That is unfortunately an interpretation of the law that would need to come
from a professional lawyer. As it is, the patch was clearly authored by
you, and anybody else who would claim authorship would most likely be
breaking the law.

So I won't touch it.

Sorry,
Johannes
--
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: RFC: new git-splice subcommand for non-interactive branch splicing

[Adam Spiers]

On Sat, May 28, 2016 at 09:06:59AM +0200, Johannes Schindelin wrote:
> Hi Adam,
> 
> please reply-to-all on this list.

Sorry, I forgot that was the policy here.  Every list and individual
has different preferences on whether to Cc: on list mail, so I find it
almost impossible to keep track of who prefers what :-/

> On Fri, 27 May 2016, Adam Spiers wrote:
> > My feeling is that rebase -i provides something tremendously
> > important, which the vast majority of users use on a regular basis,
> > but that git is currently missing a convenient way to
> > *non-interactively* perform the same magic which rebase -i
> > facilitates.
> 
> Would it not make sense to enhance `rebase -i`, then?

You mean enhance it to support non-interactive usage?  That wouldn't
make much sense to me, given that -i is short for --interactive.  Even
if we added a new non-interactive rebase mode which let you edit the
commits prior to rebasing them, I can't imagine how it would need to
be any different to how non-interactive rebase -i currently works,
i.e. setting GIT_SEQUENCE_EDITOR to a non-interactive command which
modifies the rebase todo file passed via $1.

Or maybe you are suggesting to enhance it to perform operations on the
rebase todo list, such as removing a commit from the todo list, or
moving a commit to a different position?  But that sounds like scope
creep to me; IMHO it would be cleaner for rebase -i to remain an
unopinionated platform for history editing, rather than to make
assumptions about common history editing workflows.  I think those
assumptions belong in higher-level porcelain tools.

Or if you have some other enhancement in mind, please share details!

> I, for one, plan to port my Git garden shears (at least partially) once I
> managed to get my rebase--helper work in. The shears script is kind of a
> "--preseve-merges done right":
> 
> https://github.com/git-for-windows/build-extra/blob/master/shears.sh
> 
> It allows you to (re-)create a branch structure like this:
> 
> bud
> pick cafebabe XYZ
> pick 01234567 Another patch
> mark the-first-branch
> 
> bud
> pick 98765432 This patch was unrelated
> mark the-second-branch
> 
> bud
> merge the-first-branch
> merge the-second-branch
> 
> Of course, this is interactive.

Interesting approach; thanks for sharing.  At a first glance, this
does sound similar to what topgit and gitwork are trying to achieve.
I don't entirely understand it yet, however; it's hard to without
knowing more about the structure of Git for Windows' integration
branch and seeing a concrete example and/or comprehensive
documentation.  For example, it's not clear to me how
generate_script() works, or how it lets you modify just one of the
topic "sub-branches" and then automatically update all other
sub-branches which depend on it?

> But quite frankly, you want to be able to
> perform quite complicated stuff

Why do you say that?  Splice and transplant operations are
conceptually very straightforward, and not even particularly hard to
implement.  git-splice only took me a day or so, and I expect
git-transplant to be quicker.

The only complicated thing I want to implement is git-explode, but if
I have splice and transplant "primitives" available, then it should be
quite easy to implement git-explode as a series of splice/transplant
operations.

> and I think the command-line offers only an inadequate interface for
> this.

Please can you give an example where it would be inadequate?

> > I suspect the most popular use-case in the short term would be the
> > infamous "oops, I only just noticed that I put that commit on the
> > wrong branch, and now there's already a whole bunch of other commits
> > on top of it".
> 
> I have two workflows for that. The simpler one:
> 
> git checkout other-branch
> git commit
> git checkout @{-1}
>
> after those calls.
> 
> The more complicated one comes in handy when a complete rebuild takes a
> long time, and branch switching would trigger a rebuild:
> 
> # Here, I stash what I *want* on the other branch
> git stash -p
> git worktree add throwaway other-branch
> cd throwaway
> git stash apply
> git commit

Neither of these workflows work with the scenario I described.  In my
scenario, the commit is already buried beneath other commits.

> I did use the approach you proposed a couple of times: just commit in the
> middle, and sort things out later. The problem: I frequently forgot, and
> if I did not, reordering the commits resulted in stupid and avoidable
> merge conflicts.

I think you are misunderstanding me - I am not proposing this
workflow; in fact that would be stupid because it's already in common
usage.  But I'm not even advocating it.  I'm saying:

  - I do it regularly by accident (since when I am in the hacking
zone, I am usually focused on the code, not on branch maintenance).

  - When I eventually realise I've done it, I need to go back afterwards
and 

[PATCH v2] Require 0 context lines in git-blame algorithm

[David Kastrup]

Previously, the core part of git blame -M required 1 context line.
There is no rationale to be found in the code (one guess would be that
the old blame algorithm was unable to deal with immediately adjacent
regions), and it causes artifacts like discussed in the thread
.


sheds some more light on the history of the previous choice.

Signed-off-by: David Kastrup 
---
 builtin/blame.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 21f42b0..a3f6874 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -134,7 +134,7 @@ struct progress_info {
int blamed_lines;
 };
 
-static int diff_hunks(mmfile_t *file_a, mmfile_t *file_b, long ctxlen,
+static int diff_hunks(mmfile_t *file_a, mmfile_t *file_b,
  xdl_emit_hunk_consume_func_t hunk_func, void *cb_data)
 {
xpparam_t xpp = {0};
@@ -142,7 +142,6 @@ static int diff_hunks(mmfile_t *file_a, mmfile_t *file_b, 
long ctxlen,
xdemitcb_t ecb = {NULL};
 
xpp.flags = xdl_opts;
-   xecfg.ctxlen = ctxlen;
xecfg.hunk_func = hunk_func;
ecb.priv = cb_data;
return xdi_diff(file_a, file_b, , , );
@@ -980,7 +979,7 @@ static void pass_blame_to_parent(struct scoreboard *sb,
fill_origin_blob(>revs->diffopt, target, _o);
num_get_patch++;
 
-   if (diff_hunks(_p, _o, 0, blame_chunk_cb, ))
+   if (diff_hunks(_p, _o, blame_chunk_cb, ))
die("unable to generate diff (%s -> %s)",
oid_to_hex(>commit->object.oid),
oid_to_hex(>commit->object.oid));
@@ -1129,7 +1128,7 @@ static void find_copy_in_blob(struct scoreboard *sb,
 * file_p partially may match that image.
 */
memset(split, 0, sizeof(struct blame_entry [3]));
-   if (diff_hunks(file_p, _o, 1, handle_split_cb, ))
+   if (diff_hunks(file_p, _o, handle_split_cb, ))
die("unable to generate diff (%s)",
oid_to_hex(>commit->object.oid));
/* remainder, if any, all match the preimage */
-- 
2.7.4

--
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 v8 0/9] connect: various cleanups

[Mike Hommey]

On Sat, May 28, 2016 at 10:17:19AM +0200, Torsten Bögershausen wrote:
> On 28.05.16 07:33, Mike Hommey wrote:
> > On Sat, May 28, 2016 at 07:02:01AM +0200, Torsten Bögershausen wrote:
> >> On 27.05.16 23:59, Mike Hommey wrote:
> >>> On Fri, May 27, 2016 at 04:24:20PM +0200, Torsten Bögershausen wrote:
>  On 27.05.16 04:27, Mike Hommey wrote:
> > Changes from v7:
> > - Fixed comments.
> >
> > Mike Hommey (9):
> >> All in all, a great improvement.
> >> 2 things left.
> >> - The comment about [] is now stale, isn't it ?
> > No, it's still valid at this point, that's what the 2nd argument to
> > host_end (0) does.
> >
> > Mike
> 
> The protocol (specific) code doesn't do this anymore, 
> because that is now common to all protocols.
> 
> 
>/*
> * Don't do destructive transforms now, the
> * '[]' unwrapping is done in get_host_and_port()
> */
> 

I'm not following what you're saying. The code explicitly calls host_end
so that it doesn't remove the square brackets from the string it's
passed. That's what the comment says, and that's what happens.

Mike
--
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] blame.c: don't drop origin blobs as eagerly

[David Kastrup]

Johannes Schindelin  writes:

> On Fri, 27 May 2016, David Kastrup wrote:
>
>> Johannes Schindelin  writes:
>> 
>> > On Fri, 27 May 2016, David Kastrup wrote:
>> >
>> >> pressure particularly when the history contains lots of merges from
>> >> long-diverged branches.  In practice, this optimization appears to
>> >> behave quite benignly,
>> >
>> > Why not just stop here?
>> 
>> Because there is a caveat.
>> 
>> > I say that because...
>> >
>> >> and a viable strategy for limiting the total amount of cached blobs in a
>> >> useful manner seems rather hard to implement.
>> >
>> > ... this sounds awfully handwaving.
>> 
>> Because it is.
>
> Hrm. Are you really happy with this, on public record?

Shrug.  git-blame has limits for its memory usage which it generally
heeds, throwing away potentially useful data to do so.  git-blame -C has
a store of data retained outside of those limits because nobody bothered
reining them in, and this patch similarly retains data outside of those
limits, though strictly limited to the pending priority queue size which
controls the descent in reverse commit time order.

In actual measurements on actual heavy-duty repositories I could not
discern a difference in memory usage after the patch.  The difference is
likely only noticeable when you have lots of old long branches which is
not a typical development model.

The current data structures don't make harder constraints on memory
pressure feasible, and enforcing such constraints would incur a lot of
swapping (not by the operating system which does it more efficiently
particularly since it does not need to decompress every time, but by the
application constantly rereading and recompressing data).

git-blame -C cares jack-shit about that (and it's used by git-gui's
second pass if I remember correctly) and nobody raised concerns about
its memory usage ever.

I've taken out a lot of hand-waving of the "eh, good enough" kind from
git-blame.  If one wanted to have the memory limits enforced more
stringently than they currently are, one would need a whole new layer of
FIFO and accounting.  This is not done in the current code base for
existing functionality.

And nobody bothered implementing it in decades or complaining about it
in spite of it affecting git-blame -C (and consequently git-gui blame).

Yes, this patch is another hand waving beside an already waving crowd.
I am not going to lie about it but I am also not going to invest the
time to fix yet another part of git-blame that has in decades not shown
to be a problem anybody considered worth reporting let alone fixing.

The limits for git-blame are set quite conservative: on actual developer
systems, exceeding them by a factor of 10 will not exhaust the available
memory.  And I could not even measure a difference in memory usage in a
small sample set of large examples.

That's good enough for me (and better than the shape most of git-blame
was in before I started on it and also after I finished), but it's still
hand-waving.  And it's not like it doesn't have a lot of company.

>> > Since we already have reference counting, it sounds fishy to claim
>> > that simply piggybacking a global counter on top of it would be
>> > "rather hard".
>> 
>> You'll see that the patch is from 2014.
>
> No I do not. There was no indication of that.

I thought that git-send-email/git-am retained most patch metadata as
opposed to git-patch, but you are entirely correct that the author date
is nowhere to be found.  Sorry for the presumption.

>> When I actively worked on it, I found no convincing/feasible way to
>> enforce a reasonable hard limit.  I am not picking up work on this
>> again but am merely flushing my queue so that the patch going to
>> waste is not on my conscience.
>
> Hmm. Speaking from my point of view as a maintainer, this raises a
> yellow alert. Sure, I do not maintain git.git, but if I were, I would
> want my confidence in the quality of this patch, and in the patch
> author being around when things go south with it, strengthened. This
> paragraph achieves the opposite.

It's completely reasonable to apply the same standards here as with an
author having terminal cancer.  I am not going to be around when things
go South with git-blame but I should be very much surprised if this
patch were significantly involved.

>> > Besides, -C is *supposed* to look harder.
>> 
>> We are not talking about "looking harder" but "taking more memory
>> than the set limit".
>
> I meant to say: *of course* it uses more memory, it has a much harder
> job.

Still a non-sequitur.  If you apply memory limits harder, it will still
achieve the same job but finish later due to (decompressing) swapping.

>> > Also: is there an easy way to reproduce your claims of better I/O
>> > characteristics? Something like a command-line, ideally with a file
>> > in git.git's own history, that demonstrates the I/O before and
>> > after the patch, would be an 

Re: [PATCH v2 00/22] i18n and test updates

[Vasco Almeida]

Às 17:11 de 27-05-2016, Junio C Hamano escreveu:
> Vasco Almeida  writes:
> 
>> Marks several messages for translation and updates tests to pass under
>> GETTEXT_POISON. Some tests were updated to fit previous i18n marks, others
>> were updated to fit marks made by these patches. Patches that only touch
>> test file refer to marks done in commits previous to these ones.
> 
> Whew, this series is quite a lot of work.
> 
Do you mean review work?
Submitting patches is still a new thing for me.
I don't know how to organize or split well the patch series, if they're
too long, in order to make other's work easier and the patches
themselves more appealing.

I have got other patches that I've made on top of these ones, but
don't know whether I should 1) rebase them on top of master, if they
apply cleanly, and send them in a new patch series, or just 2) send them
together in the next re-roll. New patches are also about i18n.

I've tried scavenging mailing list and documentation (not too hard, I
confess) for a explicit hint on this and eventually other best/desired
practices but found nothing so far, beside the content of
Documentation/SubmittingPatches of course.
If somebody could point me to something like that, that would be great.

I also naturally expect that, if I'm doing something wrong or lesser,
someone will bring it to my attention so I can correct.

(The first time I've sent patches here, I then sent 2 more patches alone
in their own series [1], but Junio Hamano put them together in one
va/i18n-misc-updates branch, reasonable decision since that and the
previous series were all i18n patches, hence all related. That made me
suspect that option 2) is better.
After that, I've sent a re-roll of first series, and Junio Hamano
thought for a moment that I had dropped the 2 patches from the second
one [2]. So it seems that, at least in this case, option 1) can
confuse people.)

[1] http://thread.gmane.org/gmane.comp.version-control.git/291386
[2]
http://thread.gmane.org/gmane.comp.version-control.git/291860/focus=291914


--
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 v8 0/9] connect: various cleanups

[Torsten Bögershausen]

On 28.05.16 07:33, Mike Hommey wrote:
> On Sat, May 28, 2016 at 07:02:01AM +0200, Torsten Bögershausen wrote:
>> On 27.05.16 23:59, Mike Hommey wrote:
>>> On Fri, May 27, 2016 at 04:24:20PM +0200, Torsten Bögershausen wrote:
 On 27.05.16 04:27, Mike Hommey wrote:
> Changes from v7:
> - Fixed comments.
>
> Mike Hommey (9):
>> All in all, a great improvement.
>> 2 things left.
>> - The comment about [] is now stale, isn't it ?
> No, it's still valid at this point, that's what the 2nd argument to
> host_end (0) does.
>
> Mike

The protocol (specific) code doesn't do this anymore, 
because that is now common to all protocols.


   /*
* Don't do destructive transforms now, the
* '[]' unwrapping is done in get_host_and_port()
*/

--
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: RFC: new git-splice subcommand for non-interactive branch splicing

[Johannes Schindelin]

Hi Adam,

please reply-to-all on this list.

On Fri, 27 May 2016, Adam Spiers wrote:

> My feeling is that rebase -i provides something tremendously
> important, which the vast majority of users use on a regular basis,
> but that git is currently missing a convenient way to
> *non-interactively* perform the same magic which rebase -i
> facilitates.

Would it not make sense to enhance `rebase -i`, then?

I, for one, plan to port my Git garden shears (at least partially) once I
managed to get my rebase--helper work in. The shears script is kind of a
"--preseve-merges done right":

https://github.com/git-for-windows/build-extra/blob/master/shears.sh

It allows you to (re-)create a branch structure like this:

bud
pick cafebabe XYZ
pick 01234567 Another patch
mark the-first-branch

bud
pick 98765432 This patch was unrelated
mark the-second-branch

bud
merge the-first-branch
merge the-second-branch

Of course, this is interactive. But quite frankly, you want to be able to
perform quite complicated stuff, and I think the command-line offers only
an inadequate interface for this.

> I suspect the most popular use-case in the short term would be the
> infamous "oops, I only just noticed that I put that commit on the
> wrong branch, and now there's already a whole bunch of other commits
> on top of it".

I have two workflows for that. The simpler one:

git checkout other-branch
git commit
git checkout @{-1}

Sometimes I need to call `git stash -p` before, and `git stash apply`
after those calls.

The more complicated one comes in handy when a complete rebuild takes a
long time, and branch switching would trigger a rebuild:

# Here, I stash what I *want* on the other branch
git stash -p
git worktree add throwaway other-branch
cd throwaway
git stash apply
git commit

I did use the approach you proposed a couple of times: just commit in the
middle, and sort things out later. The problem: I frequently forgot, and
if I did not, reordering the commits resulted in stupid and avoidable
merge conflicts.

> > > In the longer term however, I'd like to write two more subcommands:
> > > 
> > >   - git-transplant(1) which wraps around git-splice(1) and enables
> > > easy non-interactive transplanting of a range of commits from
> > > one branch to another.  This should be pretty straightforward
> > > to implement.
> > 
> > This is just cherry-pick with a range...
> 
> No it's not:
> 
>   - git-transplant would be able to splice commits from one branch
> *into* (i.e. inside, *not* onto) another branch.

Okay, but in case of merge conflicts, you still have to switch to the
other branch, right?

>   - git-transplant would also take care of removing the commits from
> the source branch, but not before they were safely inside the
> destination branch.

That assumes a workflow where you develop on one big messy branch and
later sort it out into the appropriate, separate branches, right? I admit
that I used to do that, too, but ever since worktrees arrived, I do not do
that anymore: it resulted in too much clean-up work. Better to put the
commits into the correct branch right away. Of course, that is just *my*
preference.

>   - git-transplant would orchestrate the whole workflow with a single
> command, complete with --abort and --continue.

cherry-pick also sports --abort and --continue.

> > >   - git-explode(1) which wraps around git-transplant(1) and
> > > git-deps(1), and automatically breaks a linear sequence of commits
> > > into multiple smaller sequences, forming a commit graph where
> > > ancestry mirrors commit dependency, as mentioned above.  I expect
> > > this to be more difficult, and would probably write it in Python.
> > 
> > You mean something like Darcs on top of Git. Essentially, you want to end
> > up with an octopus merge of branches whose commits would conflict if
> > exchanged.
> 
> Something like that, yes, but it's not as simple as a single octopus
> merge.  It would support arbitrarily deep DAGs of topic branches.

Yes, of course. Because

A - B - C - D

might need to resolve into

A - M1 - C - M3
  X/
B - M2 - D

> > I implemented the logic for this in a shell script somewhere, so it is not
> > *all* that hard (Python not required). But I ended up never quite using it
> > because it turns out that in practice, the commit "dependency" (as defined
> > by the commit diffs) does not really reflect the true dependency.
> >
> > For example,
> 
> [snipped examples]
> 
> Sure - I already covered this concern in footnote [0] of my previous
> mail; maybe you missed that?

I think it deserves more prominent a place than a footnote.

> > So I think that this is a nice exercise, but in practice it will
> > require a human to determine which commits really depend on each
> > other.
> 
> Of course - this is 

Re: Small rerere in rebase regression

[Johannes Schindelin]

Hi Hannes,

On Fri, 27 May 2016, Johannes Sixt wrote:

> Subject: [PATCH] rebase -i: remove an unnecessary 'rerere' invocation
> 
> Interactive rebase uses 'git cherry-pick' and 'git merge' to replay
> commits. Both invoke the 'rerere' machinery when they fail due to merge
> conflicts. Note that all code paths with these two commands also invoke
> the shell function die_with_patch when the commands fail.
> 
> Since commit 629716d2 ("rerere: do use multiple variants") the second
> operation of the rerere machinery can be observed by a duplicated
> message "Recorded preimage for 'file'". This second operation records
> the same preimage as the first one and, hence, only wastes cycles.
> Remove the 'git rerere' invocation from die_with_patch.
> 
> Shell function die_with_patch can be called after the failure of
> "git commit", too, which also calls into the rerere machinery, but it
> does so only after a successful commit to record the resolution.
> Therefore, it is wrong to call 'git rerere' from die_with_patch after
> "git commit" fails.
> 
> Signed-off-by: Johannes Sixt 

ACK

Ciao,
Dscho
--
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] blame.c: don't drop origin blobs as eagerly

[Johannes Schindelin]

Hi David,

On Fri, 27 May 2016, David Kastrup wrote:

> Johannes Schindelin  writes:
> 
> > On Fri, 27 May 2016, David Kastrup wrote:
> >
> >> pressure particularly when the history contains lots of merges from
> >> long-diverged branches.  In practice, this optimization appears to
> >> behave quite benignly,
> >
> > Why not just stop here?
> 
> Because there is a caveat.
> 
> > I say that because...
> >
> >> and a viable strategy for limiting the total amount of cached blobs in a
> >> useful manner seems rather hard to implement.
> >
> > ... this sounds awfully handwaving.
> 
> Because it is.

Hrm. Are you really happy with this, on public record?

> > Since we already have reference counting, it sounds fishy to claim
> > that simply piggybacking a global counter on top of it would be
> > "rather hard".
> 
> You'll see that the patch is from 2014.

No I do not. There was no indication of that.

> When I actively worked on it, I found no convincing/feasible way to
> enforce a reasonable hard limit.  I am not picking up work on this again
> but am merely flushing my queue so that the patch going to waste is not
> on my conscience.

Hmm. Speaking from my point of view as a maintainer, this raises a yellow
alert. Sure, I do not maintain git.git, but if I were, I would want my
confidence in the quality of this patch, and in the patch author being
around when things go south with it, strengthened. This paragraph achieves
the opposite.

> >> In addition, calling git-blame with -C leads to similar memory retention
> >> patterns.
> >
> > This is a red herring. Just delete it. I, for one, being a heavy user of
> > `git blame`, could count the number of times I used blame's -C option
> > without any remaining hands. Zero times.
> >
> > Besides, -C is *supposed* to look harder.
> 
> We are not talking about "looking harder" but "taking more memory than
> the set limit".

I meant to say: *of course* it uses more memory, it has a much harder job.

> > Also: is there an easy way to reproduce your claims of better I/O
> > characteristics? Something like a command-line, ideally with a file in
> > git.git's own history, that demonstrates the I/O before and after the
> > patch, would be an excellent addition to the commit message.
> 
> I've used it on the wortliste repository and system time goes down from
> about 70 seconds to 50 seconds (this is a flash drive).  User time from
> about 4:20 to 4:00.  It is a rather degenerate repository (predominantly
> small changes in one humongous text file) so savings for more typical
> cases might end up less than that.  But then it is degenerate
> repositories that are most costly to blame.

Well, obviously I did not mean to measure time, but I/O. Something like an
strace call that pipes into some grep that in turn pipes into wc.

> > Further: I would have at least expected some rudimentary discussion
> > why this patch -- which seems to at least partially contradict 7c3c796
> > (blame: drop blob data after passing blame to the parent, 2007-12-11)
> > -- is not regressing on the intent of said commit.
> 
> It is regressing on the intent of said commit by _retaining_ blob data
> that it is _sure_ to look at again because of already having this data
> asked for again in the priority queue.  That's the point.  It only drops
> the blob data for which it has no request queued yet.  But there is no
> handle on when the request is going to pop up until it actually leaves
> the priority queue: the priority queue is a heap IIRC and thus only
> provides partial orderings.

Again, this lowers my confidence in the patch. Mind you, the patch could
be totally sound! Yet its commit message, and unfortunately even more so
the discussion we are having right now, offer no adequate reason to assume
that. If you, as the patch author, state that you are not sure what this
thing does exactly, we must conservatively assume that it contains flaws.

> >> diff --git a/builtin/blame.c b/builtin/blame.c
> >> index 21f42b0..2596fbc 100644
> >> --- a/builtin/blame.c
> >> +++ b/builtin/blame.c
> >> @@ -1556,7 +1556,8 @@ finish:
> >>}
> >>for (i = 0; i < num_sg; i++) {
> >>if (sg_origin[i]) {
> >> -  drop_origin_blob(sg_origin[i]);
> >> +  if (!sg_origin[i]->suspects)
> >> +  drop_origin_blob(sg_origin[i]);
> >>origin_decref(sg_origin[i]);
> >>}
> >
> > It would be good to mention in the commit message that this patch does not
> > change anything for blobs with only one remaining reference (the current
> > one) because origin_decref() would do the same job as drop_origin_blob
> > when decrementing the reference counter to 0.
> 
> A sizable number of references but not blobs are retained and needed for
> producing the information in the final printed output (at least when
> using the default sequential output instead of --incremental or
> --porcelaine or similar).

Sorry, please