[PATCH] format-patch: use raw format for notes

[Sam Bobroff]

If "--notes=..." is used with "git format-patch", the notes are
prefixed with the ref's local name and indented, which looks odd and
exposes the path of the ref.

Extend the test that suppresses this behaviour so that it also catches
this case, causing the notes to be included without additional
processing.

Signed-off-by: Sam Bobroff 
---

Notes (foo):
Hi,

I've noticed what appears to be a small cosmetic bug in git format-patch, as
I've described in the commit message.

I'm not sure if this patch is the right way to fix it (or perhaps it's not 
even
a bug), but it should at least help to explain what I'm talking about.

I've used "git format-patch --notes=foo" to prepare this email so that it 
is an
example of the issue :-)

Cheers,
Sam.

 log-tree.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/log-tree.c b/log-tree.c
index 410ab4f02..26bc21ad3 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -655,7 +655,8 @@ void show_log(struct rev_info *opt)
int raw;
struct strbuf notebuf = STRBUF_INIT;
 
-   raw = (opt->commit_format == CMIT_FMT_USERFORMAT);
+   raw = (opt->commit_format == CMIT_FMT_USERFORMAT) ||
+ (opt->commit_format == CMIT_FMT_EMAIL);
format_display_notes(>object.oid, ,
 get_log_output_encoding(), raw);
ctx.notes_message = notebuf.len
-- 
2.11.0

Re: [PATCH] pkt-line: re-'static'-ify buffer in packet_write_fmt_1()

[Martin Ågren]

On 28 August 2017 at 01:23, Jeff King  wrote:
> On Sun, Aug 27, 2017 at 10:04:55PM +0200, Lars Schneider wrote:
>
>> I did run all tests under valgrind using "make valgrind" and I found
>> the following files with potential issues:
>>
>> cat valgrind.out | perl -nE 'say /^==.+\((.+)\:.+\)$/' | sort | uniq -c
>> 7102
>>2 clone.c
>>   33 common-main.c
>>6 connect.c
>>   64 git.c
>>4 ls-remote.c
>>  126 run-command.c
>>   12 transport.c
>>7 worktree.c
>
> I'm not sure where valgrind.out comes from. The individual
> test-results/*.out files may have valgrind output, but I don't think
> they usually contain leak output.
>
> Doing "valgrind ./git-upload-pack . /dev/null" mentions
> leaked memory but not the locations. Adding --leak-check=full shows that
> most of it comes from format_packet().
>
> And applying Martin's patch drops the "definitely lost" category down to
> 0 bytes (there's still 550k in "still reachable", but those are in the
> "exit will free them for us" category).
>
>> No mention of "pkt-line.c". Did you run Git with valgrind on one of
>> your repositories to find it?
>
> I'm curious, too. I don't think the valgrind setup in our test suite is
> great for finding leaks right now.

Sorry for being brief. I've patched t/valgrind/valgrind.sh to say
"--leak-check=yes". Then I run "./t --valgrind", simply because
running the complete suite gives more reports than I could possibly
process.

Then I check the first few leaks, verify that they're "ok" and add
them to a suppressions-list. Lather, rinse, repeat. A couple of very
targeted and well-motivated suppressions in git.git could perhaps be
motivated, but there are many many reported leaks. My suppressions-list
is getting gross.

I started with t and t0001 because I figure, once I have those basic
suppressions in place (or leaks fixed), I can run some other more
interesting tests. Of course, the concept of "this leak is ok" is a bit
subjective. For example, we might do "return !!create_object(...);"
(function name invented on the fly), which is a leak, and unreachable.
But if this is only done once in builtin/foo.c and the object created is
small, then this could be deemed "ok", since in practice this leak will
never bring anyone over the cliff. If clean-ups in such code would not
just be code churn, then I can of course adjust my definition of "ok"
accordingly.

This is not an attempt to find and fix a huge number of leaks, it's more
to have a good reason to go through call-stacks, convince myself I know
what the code wants to do and how it does it.

Looking at only "unreachable" leaks seems like it should be an
improvement for finding the interesting cases. I'll have less time for
Git this week, but can try it out as time permits.

Thanks for your feedback, both of you.

Martin

Re: [PATCH] pkt-line: re-'static'-ify buffer in packet_write_fmt_1()

[Jeff King]

On Sun, Aug 27, 2017 at 10:04:55PM +0200, Lars Schneider wrote:

> I did run all tests under valgrind using "make valgrind" and I found
> the following files with potential issues:
> 
> cat valgrind.out | perl -nE 'say /^==.+\((.+)\:.+\)$/' | sort | uniq -c
> 7102
>2 clone.c
>   33 common-main.c
>6 connect.c
>   64 git.c
>4 ls-remote.c
>  126 run-command.c
>   12 transport.c
>7 worktree.c

I'm not sure where valgrind.out comes from. The individual
test-results/*.out files may have valgrind output, but I don't think
they usually contain leak output.

Doing "valgrind ./git-upload-pack . /dev/null" mentions
leaked memory but not the locations. Adding --leak-check=full shows that
most of it comes from format_packet().

And applying Martin's patch drops the "definitely lost" category down to
0 bytes (there's still 550k in "still reachable", but those are in the
"exit will free them for us" category).

> No mention of "pkt-line.c". Did you run Git with valgrind on one of 
> your repositories to find it?

I'm curious, too. I don't think the valgrind setup in our test suite is
great for finding leaks right now.

-Peff

Loan Offer!

[Sec Capital Loan]

Loan Offer at 3%, Feel Free to REPLY back to us for more info.

Re: [PATCH] pkt-line: re-'static'-ify buffer in packet_write_fmt_1()

[Lars Schneider]

> On 27 Aug 2017, at 21:09, Martin Ågren  wrote:
> 
> On 27 August 2017 at 20:21, Lars Schneider  wrote:
>> 
>>> On 27 Aug 2017, at 09:37, Martin Ågren  wrote:
>>> 
>>> The static-ness was silently dropped in commit 70428d1a5 ("pkt-line: add
>>> packet_write_fmt_gently()", 2016-10-16). As a result, for each call to
>>> packet_write_fmt_1, we allocate and leak a buffer.
>> 
>> Oh :-( Thanks for detecting and fixing the leak.
>> 
>> How did you actually find it? Reading the code or did you use some
>> tool?
> 
> Valgrind found it for me. Most leaks it reports are "ok" since we're
> about to exit, it just takes more or less effort to realize that...

I am sorry if I am bugging you here, but I would really like to 
understand how you found it.

I did run all tests under valgrind using "make valgrind" and I found
the following files with potential issues:

cat valgrind.out | perl -nE 'say /^==.+\((.+)\:.+\)$/' | sort | uniq -c
7102
   2 clone.c
  33 common-main.c
   6 connect.c
  64 git.c
   4 ls-remote.c
 126 run-command.c
  12 transport.c
   7 worktree.c


No mention of "pkt-line.c". Did you run Git with valgrind on one of 
your repositories to find it?

Thanks,
Lars

Re: [PATCH] pkt-line: re-'static'-ify buffer in packet_write_fmt_1()

[Jeff King]

On Sun, Aug 27, 2017 at 09:09:15PM +0200, Martin Ågren wrote:

> On 27 August 2017 at 20:21, Lars Schneider  wrote:
> >
> >> On 27 Aug 2017, at 09:37, Martin Ågren  wrote:
> >>
> >> The static-ness was silently dropped in commit 70428d1a5 ("pkt-line: add
> >> packet_write_fmt_gently()", 2016-10-16). As a result, for each call to
> >> packet_write_fmt_1, we allocate and leak a buffer.
> >
> > Oh :-( Thanks for detecting and fixing the leak.
> >
> > How did you actually find it? Reading the code or did you use some
> > tool?
> 
> Valgrind found it for me. Most leaks it reports are "ok" since we're
> about to exit, it just takes more or less effort to realize that...

This is one more thing it would be nice to have as part of our
perf-testing framework. It will run two versions of Git across a battery
of tests and report on the runtime differences for each. It would be
great if it did the same for peak heap. The tricky thing is that you
sometimes need repositories that are exaggerated in size in one
dimension to notice the differences as significant. So I don't know if
we would need new tests for this, or if existing "this repo has a lot of
refs" tests would have caught this.

Another approach, of course, is to get valgrind (or asan) to a
zero-leaks-reported steady state, and then even small leak reports would
be worth investigating. I'm not sure how easy it is to get there, and
whether it's less work to actually plug free-on-exit leaks or somehow
suppress the reports. I think most free-on-exit false positives would
show up as "leaked but still reachable", whereas leaks like this that
can grow without bound mean would not be reachable (we throw away the
pointer to the memory at the end of the function).

-Peff

Re: [PATCH] pkt-line: re-'static'-ify buffer in packet_write_fmt_1()

[Martin Ågren]

On 27 August 2017 at 20:21, Lars Schneider  wrote:
>
>> On 27 Aug 2017, at 09:37, Martin Ågren  wrote:
>>
>> The static-ness was silently dropped in commit 70428d1a5 ("pkt-line: add
>> packet_write_fmt_gently()", 2016-10-16). As a result, for each call to
>> packet_write_fmt_1, we allocate and leak a buffer.
>
> Oh :-( Thanks for detecting and fixing the leak.
>
> How did you actually find it? Reading the code or did you use some
> tool?

Valgrind found it for me. Most leaks it reports are "ok" since we're
about to exit, it just takes more or less effort to realize that...

Martin

RE: cat-file timing window on Cygwin

[Jason Pyeron]

> -Original Message-
> From: Ramsay Jones
> Sent: Sunday, August 27, 2017 11:48 AM
> To: Adam Dinwoodie
> Cc: Jeff King; git@vger.kernel.org; Johannes Schindelin
> Subject: Re: cat-file timing window on Cygwin
> 
> 
> 
> On 27/08/17 12:33, Adam Dinwoodie wrote:
> > On Sun, Aug 27, 2017 at 03:06:31AM +0100, Ramsay Jones wrote:
> >> On 26/08/17 22:11, Adam Dinwoodie wrote:
> >>> On Sat, Aug 26, 2017 at 11:53:37AM -0700, Jeff King wrote:
>  Interesting. I find it a little hard to believe there's 
> so obvious 
>  a bug as "fflush(NULL) flushes stdin", but well...that's 
> what it seems like.
> 
>  If that's truly what it is, this is the minimal 
> reproduction I came 
>  up
>  with:
> 
>  -- >8 --
>  #include 
> 
>  int main(void)
>  {
>   char buf[256];
>   while (fgets(buf, sizeof(buf), stdin)) {
>   fprintf(stdout, "got: %s", buf);
>   fflush(NULL);
>   }
>   return 0;
>  }
>  -- 8< --
> 
>  If this really is the bug, then doing something like 
> "seq 10 | ./a.out"

Tests good on latest snapshot. Fails on 

Cygwin DLL version info:
DLL version: 2.8.2
DLL epoch: 19
DLL old termios: 5
DLL malloc env: 28
Cygwin conv: 181
API major: 0
API minor: 313
Shared data: 5
DLL identifier: cygwin1
Mount registry: 3
Cygwin registry name: Cygwin
Installations name: Installations
Cygdrive default prefix:
Build date:
Shared id: cygwin1S5


>  would drop some of the input lines.
> >>>
> >>> ...yep.  It does.  Specifically, I consistently only get 
> the firsts
> >>> line:
> >>>
> >>> $ seq 10 | ./a.exe
> >>> got: 1
> >>> 
> >>> $
> >>>
> >>> If I introduce a delay between the lines of stdin (which 
> I tested by 
> >>> just typing stdin from the keyboard), it works as expected.
> >>>
> >>> Looks like this one will need to go to the Cygwin mailing 
> list; I'll 
> >>> take it there shortly.  Thank you all for your help 
> getting this far!
> >>
> >> This is apparently fixed in cygwin v2.8.3 [see commit 78ade082fe, 
> >> ('Revert "errno: Stop using _impure_ptr->_errno completely"', 
> >> 19-07-2017), commit 9cc89b0438 ("cygwin: Use errno instead of 
> >> _impure_ptr->_errno", 19-07-2017), commit a674199fc9 
> ("cygwin: Bump 
> >> DLL version to 2.8.3",
> >> 19-07-2017) and commit d2ae2f00b8 ("cygwin: add fflush fix 
> to release 
> >> notes", 19-07-2017)].
> >>
> >> I haven't had a chance to try v2.8.3 yet (it's 3am and I'm 
> about to 
> >> go get some sleep).
> > 
> > Cygwin 2.8.3 hasn't been released yet,
> 
> Heh, yes, I found that out myself this afternoon. ;-)
> 
> > but I've just tested the 
> > latest development snapshot with Jeff's simple test case, 
> and it works 
> > as expected, so I'm going to assume the Git test will start passing 
> > once that version of the Cygwin DLL is released too.
> 
> Hmm, I'm not keen on installing "snapshot"(s), so I think I 
> will wait for the release to test it. (However, as a matter 
> of interest, how would I obtain/install/test this snapshot 
> release - is it a 'low-risk' exercise?)

Using https://cygwin.com/snapshots/x86_64/cygwin-20170823.tar.xz

D:\inst\cygwin\cygwin-20170823>usr\bin\bash.exe
bash-4.4$ seq 10 | ./a.exe
got: 1
got: 2
got: 3
got: 4
got: 5
got: 6
got: 7
got: 8
got: 9
got: 10
bash-4.4$ cat cygcheck.out

Cygwin Configuration Diagnostics
Current System Time: Sun Aug 27 14:35:25 2017

Windows 10 Professional Ver 10.0 Build 14393

Cygwin DLL version info:
DLL version: 2.9.0
DLL epoch: 19
DLL old termios: 5
DLL malloc env: 28
Cygwin conv: 181
API major: 0
API minor: 317
Shared data: 5
DLL identifier: cygwin1
Mount registry: 3
Cygwin registry name: Cygwin
Installations name: Installations
Cygdrive default prefix:
Build date:
Snapshot date: 20170823-15:44:28
Shared id: cygwin1S5

bash-4.4$

v/r,

Jason Pyeron

--
-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
-   -
- Jason Pyeron  PD Inc. http://www.pdinc.us -
- Principal Consultant  10 West 24th Street #100-
- +1 (443) 269-1555 x333Baltimore, Maryland 21218   -
-   -
-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=- 

Automatically delete branches containing accepted patches?

[Lars Schneider]

Hi,

I have lots of git/git branches and once in a while some patches make it 
into git/git master. If this happens I would like to delete my branch
with the patch automatically. That's not easily possible as the hashes 
on my branches are, of course, not the same as the hashes on git/git.

How do you deal with this situation? Do you manually delete your 
branches or do you have some clever script to share?

Thanks,
Lars

Re: [PATCH] pkt-line: re-'static'-ify buffer in packet_write_fmt_1()

[Jeff King]

On Sun, Aug 27, 2017 at 11:41:52AM -0400, Jeff King wrote:

> Ouch. So this means that git since v2.11 is basically leaking every
> non-byte pack sent by upload-pack (so all of the ref advertisement and
> want/have negotiation).

Determined experimentally that the answer is: yes.

The increase in ref advertisement is roughly linear with the size of
your packed-refs, so for most people this wasn't a big deal (and even if
you have insanely large packed-refs, it's "only" 2-3x the RAM;
potentially a problem, but only if you're close to the tipping point
already).

-Peff

Re: [PATCH] pkt-line: re-'static'-ify buffer in packet_write_fmt_1()

[Lars Schneider]

> On 27 Aug 2017, at 09:37, Martin Ågren  wrote:
> 
> The static-ness was silently dropped in commit 70428d1a5 ("pkt-line: add
> packet_write_fmt_gently()", 2016-10-16). As a result, for each call to
> packet_write_fmt_1, we allocate and leak a buffer.

Oh :-( Thanks for detecting and fixing the leak.

How did you actually find it? Reading the code or did you use some
tool?


> We could keep the strbuf non-static and instead make sure we always
> release it before returning (but not before we die, so that we don't
> touch errno). That would also prepare us for threaded use. But until
> that needs to happen, let's just restore the static-ness so that we get
> back to a situation where we (eventually) do not continuosly keep
> allocating memory.
> 
> Signed-off-by: Martin Ågren 
> ---
> I waffled between "fixing the memory leak" by releasing the buffer and
> "fixing the static-ness" as in this patch. I can see arguments both ways
> and don't have any strong opinion.
> 
> pkt-line.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/pkt-line.c b/pkt-line.c
> index 7db911957..f364944b9 100644
> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -136,9 +136,10 @@ static void format_packet(struct strbuf *out, const char 
> *fmt, va_list args)
> static int packet_write_fmt_1(int fd, int gently,
> const char *fmt, va_list args)
> {
> - struct strbuf buf = STRBUF_INIT;
> + static struct strbuf buf = STRBUF_INIT;
>   ssize_t count;
> 
> + strbuf_reset();
>   format_packet(, fmt, args);
>   count = write_in_full(fd, buf.buf, buf.len);
>   if (count == buf.len)
> -- 
> 2.14.1.151.g45c1275a3.dirty

Looks good to me!

- Lars

Re: cat-file timing window on Cygwin

[Ramsay Jones]

On 27/08/17 12:33, Adam Dinwoodie wrote:
> On Sun, Aug 27, 2017 at 03:06:31AM +0100, Ramsay Jones wrote:
>> On 26/08/17 22:11, Adam Dinwoodie wrote:
>>> On Sat, Aug 26, 2017 at 11:53:37AM -0700, Jeff King wrote:
 Interesting. I find it a little hard to believe there's so obvious a bug
 as "fflush(NULL) flushes stdin", but well...that's what it seems like.

 If that's truly what it is, this is the minimal reproduction I came up
 with:

 -- >8 --
 #include 

 int main(void)
 {
char buf[256];
while (fgets(buf, sizeof(buf), stdin)) {
fprintf(stdout, "got: %s", buf);
fflush(NULL);
}
return 0;
 }
 -- 8< --

 If this really is the bug, then doing something like "seq 10 | ./a.out"
 would drop some of the input lines.
>>>
>>> ...yep.  It does.  Specifically, I consistently only get the firsts
>>> line:
>>>
>>> $ seq 10 | ./a.exe
>>> got: 1
>>> 
>>> $ 
>>>
>>> If I introduce a delay between the lines of stdin (which I tested by
>>> just typing stdin from the keyboard), it works as expected.
>>>
>>> Looks like this one will need to go to the Cygwin mailing list; I'll
>>> take it there shortly.  Thank you all for your help getting this far!
>>
>> This is apparently fixed in cygwin v2.8.3 [see commit 78ade082fe,
>> ('Revert "errno: Stop using _impure_ptr->_errno completely"', 19-07-2017),
>> commit 9cc89b0438 ("cygwin: Use errno instead of _impure_ptr->_errno", 
>> 19-07-2017), commit a674199fc9 ("cygwin: Bump DLL version to 2.8.3",
>> 19-07-2017) and commit d2ae2f00b8 ("cygwin: add fflush fix to release
>> notes", 19-07-2017)].
>>
>> I haven't had a chance to try v2.8.3 yet (it's 3am and I'm about to
>> go get some sleep).
> 
> Cygwin 2.8.3 hasn't been released yet, 

Heh, yes, I found that out myself this afternoon. ;-)

> but I've just tested the latest
> development snapshot with Jeff's simple test case, and it works as
> expected, so I'm going to assume the Git test will start passing once
> that version of the Cygwin DLL is released too.

Hmm, I'm not keen on installing "snapshot"(s), so I think I will
wait for the release to test it. (However, as a matter of interest,
how would I obtain/install/test this snapshot release - is it a
'low-risk' exercise?)

ATB,
Ramsay Jones

Re: [PATCH] pkt-line: re-'static'-ify buffer in packet_write_fmt_1()

[Jeff King]

On Sun, Aug 27, 2017 at 09:37:32AM +0200, Martin Ågren wrote:

> The static-ness was silently dropped in commit 70428d1a5 ("pkt-line: add
> packet_write_fmt_gently()", 2016-10-16). As a result, for each call to
> packet_write_fmt_1, we allocate and leak a buffer.
> 
> We could keep the strbuf non-static and instead make sure we always
> release it before returning (but not before we die, so that we don't
> touch errno). That would also prepare us for threaded use. But until
> that needs to happen, let's just restore the static-ness so that we get
> back to a situation where we (eventually) do not continuosly keep
> allocating memory.

Ouch. So this means that git since v2.11 is basically leaking every
non-byte pack sent by upload-pack (so all of the ref advertisement and
want/have negotiation).

I'm surprised nobody noticed the extra memory use, but I guess those
things aren't usually _too_ big.

> Signed-off-by: Martin Ågren 
> ---
> I waffled between "fixing the memory leak" by releasing the buffer and
> "fixing the static-ness" as in this patch. I can see arguments both ways
> and don't have any strong opinion.

I think this is a good fix for now as it takes as back to the pre-bug
state.  The only downside with the static buffer is that it's not
reentrant.  Since the function is just inherently writing out the result
and then forgetting it, in a single thread there's no opportunity for a
sub-function to try writing another packet. And I don't think we have
any code paths that write packets from multiple threads. That may be
something we do eventually, but we can deal with it then (and until
then, it's nice to avoid the malloc/free overhead).

-Peff

Re: [GSoC][PATCH v3 0/4] Incremental rewrite of git-submodules

[Prathamesh Chavan]

On Sat, Aug 26, 2017 at 12:21 AM, Junio C Hamano  wrote:
> Thanks.  I'll try to queue these before I'll go offline.
>
> Mentors may want to help the student further in adjusting the patch
> series to the more recent codebase; unfortunately the area the GSoC
> project touches is a bit fluid these days.  I resolved the conflicts
> with nd/pune-in-worktree and bw/submodule-config-cleanup topics so
> that the result would compile, but I am not sure if the resolution
> is correct (e.g. there may be a new leak I introduced while doing
> so).
>

Thanks.
I'll see the dirty merges and will resend the whole series after reviewing
the dirty merge and sending a new one with/without changes as required.

Thanks,
Prathamesh Chavan

Re: cat-file timing window on Cygwin

[Adam Dinwoodie]

On Sun, Aug 27, 2017 at 03:06:31AM +0100, Ramsay Jones wrote:
> On 26/08/17 22:11, Adam Dinwoodie wrote:
> > On Sat, Aug 26, 2017 at 11:53:37AM -0700, Jeff King wrote:
> >> Interesting. I find it a little hard to believe there's so obvious a bug
> >> as "fflush(NULL) flushes stdin", but well...that's what it seems like.
> >>
> >> If that's truly what it is, this is the minimal reproduction I came up
> >> with:
> >>
> >> -- >8 --
> >> #include 
> >>
> >> int main(void)
> >> {
> >>char buf[256];
> >>while (fgets(buf, sizeof(buf), stdin)) {
> >>fprintf(stdout, "got: %s", buf);
> >>fflush(NULL);
> >>}
> >>return 0;
> >> }
> >> -- 8< --
> >>
> >> If this really is the bug, then doing something like "seq 10 | ./a.out"
> >> would drop some of the input lines.
> > 
> > ...yep.  It does.  Specifically, I consistently only get the firsts
> > line:
> > 
> > $ seq 10 | ./a.exe
> > got: 1
> > 
> > $ 
> > 
> > If I introduce a delay between the lines of stdin (which I tested by
> > just typing stdin from the keyboard), it works as expected.
> > 
> > Looks like this one will need to go to the Cygwin mailing list; I'll
> > take it there shortly.  Thank you all for your help getting this far!
> 
> This is apparently fixed in cygwin v2.8.3 [see commit 78ade082fe,
> ('Revert "errno: Stop using _impure_ptr->_errno completely"', 19-07-2017),
> commit 9cc89b0438 ("cygwin: Use errno instead of _impure_ptr->_errno", 
> 19-07-2017), commit a674199fc9 ("cygwin: Bump DLL version to 2.8.3",
> 19-07-2017) and commit d2ae2f00b8 ("cygwin: add fflush fix to release
> notes", 19-07-2017)].
> 
> I haven't had a chance to try v2.8.3 yet (it's 3am and I'm about to
> go get some sleep).

Cygwin 2.8.3 hasn't been released yet, but I've just tested the latest
development snapshot with Jeff's simple test case, and it works as
expected, so I'm going to assume the Git test will start passing once
that version of the Cygwin DLL is released too.

[PATCH] pkt-line: re-'static'-ify buffer in packet_write_fmt_1()

[Martin Ågren]

The static-ness was silently dropped in commit 70428d1a5 ("pkt-line: add
packet_write_fmt_gently()", 2016-10-16). As a result, for each call to
packet_write_fmt_1, we allocate and leak a buffer.

We could keep the strbuf non-static and instead make sure we always
release it before returning (but not before we die, so that we don't
touch errno). That would also prepare us for threaded use. But until
that needs to happen, let's just restore the static-ness so that we get
back to a situation where we (eventually) do not continuosly keep
allocating memory.

Signed-off-by: Martin Ågren 
---
I waffled between "fixing the memory leak" by releasing the buffer and
"fixing the static-ness" as in this patch. I can see arguments both ways
and don't have any strong opinion.

 pkt-line.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/pkt-line.c b/pkt-line.c
index 7db911957..f364944b9 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -136,9 +136,10 @@ static void format_packet(struct strbuf *out, const char 
*fmt, va_list args)
 static int packet_write_fmt_1(int fd, int gently,
  const char *fmt, va_list args)
 {
-   struct strbuf buf = STRBUF_INIT;
+   static struct strbuf buf = STRBUF_INIT;
ssize_t count;
 
+   strbuf_reset();
format_packet(, fmt, args);
count = write_in_full(fd, buf.buf, buf.len);
if (count == buf.len)
-- 
2.14.1.151.g45c1275a3.dirty

What's cooking in git.git (Aug 2017, #07; Sat, 26)

[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'.  The ones marked with '.' do not appear in any of
the integration branches, but I am still holding onto them.

As I am preparing to go offline next week for about a week, a
handful of topics have been merged to 'master' and 'next' is getting
thinner.

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"]

* bc/vcs-svn-cleanup (2017-08-20) 2 commits
  (merged to 'next' on 2017-08-22 at d8494b5d5b)
 + vcs-svn: rename repo functions to "svn_repo"
 + vcs-svn: remove unused prototypes
 (this branch is used by bc/hash-algo and jn/vcs-svn-cleanup.)

 Code clean-up.


* bw/submodule-config-cleanup (2017-08-03) 17 commits
  (merged to 'next' on 2017-08-24 at e35d58c4c6)
 + submodule: remove gitmodules_config
 + unpack-trees: improve loading of .gitmodules
 + submodule-config: lazy-load a repository's .gitmodules file
 + submodule-config: move submodule-config functions to submodule-config.c
 + submodule-config: remove support for overlaying repository config
 + diff: stop allowing diff to have submodules configured in .git/config
 + submodule: remove submodule_config callback routine
 + unpack-trees: don't respect submodule.update
 + submodule: don't rely on overlayed config when setting diffopts
 + fetch: don't overlay config with submodule-config
 + submodule--helper: don't overlay config in update-clone
 + submodule--helper: don't overlay config in remote_submodule_branch
 + add, reset: ensure submodules can be added or reset
 + submodule: don't use submodule_from_name
 + t7411: check configuration parsing errors
 + Merge branch 'bc/object-id' into bw/submodule-config-cleanup
 + Merge branch 'bw/grep-recurse-submodules' into bw/submodule-config-cleanup

 Code clean-up to avoid mixing values read from the .gitmodules file
 and values read from the .git/config file.
 So, after the recent discussion around submodule..update (and
 its resolution "use submodule..active; "reset --hard" must
 ignore the .update thing"), this is now good to go, I presume?
 Please yell at me that I am clueless if that is not the case ;-)


* jc/cutoff-config (2017-08-22) 6 commits
  (merged to 'next' on 2017-08-24 at 2dcbf9ae04)
 + rerere: allow approxidate in gc.rerereResolved/gc.rerereUnresolved
 + rerere: represent time duration in timestamp_t internally
 + t4200: parameterize "rerere gc" custom expiry test
 + t4200: gather "rerere gc" together
 + t4200: make "rerere gc" test more robust
 + t4200: give us a clean slate after "rerere gc" tests

 "[gc] rerereResolved = 5.days" used to be invalid, as the variable
 is defined to take an integer counting the number of days.  It now
 is allowed.


* jk/trailers-parse (2017-08-20) 9 commits
  (merged to 'next' on 2017-08-22 at 2d4d937141)
 + doc/interpret-trailers: fix "the this" typo
  (merged to 'next' on 2017-08-19 at faa63f8196)
 + pretty: support normalization options for %(trailers)
 + t4205: refactor %(trailers) tests
 + pretty: move trailer formatting to trailer.c
 + interpret-trailers: add --parse convenience option
 + interpret-trailers: add an option to unfold values
 + interpret-trailers: add an option to show only existing trailers
 + interpret-trailers: add an option to show only the trailers
 + trailer: put process_trailers() options into a struct

 "git interpret-trailers" has been taught a "--parse" and a few
 other options to make it easier for scripts to grab existing
 trailer lines from a commit log message.
 Possible leftover bits are mentioned in
 cf. <20170820094009.z23wclpku35tx...@sigill.intra.peff.net>


* jn/vcs-svn-cleanup (2017-08-23) 4 commits
  (merged to 'next' on 2017-08-24 at c184f08f5f)
 + vcs-svn: move remaining repo_tree functions to fast_export.h
 + vcs-svn: remove repo_delete wrapper function
 + vcs-svn: remove custom mode constants
 + vcs-svn: remove more unused prototypes and declarations
 (this branch uses bc/vcs-svn-cleanup; is tangled with bc/hash-algo.)

 Code clean-up.


* js/gitweb-raw-blob-link-in-history (2017-08-22) 1 commit
  (merged to 'next' on 2017-08-23 at 16258266e2)
 + gitweb: add 'raw' blob_plain link in history overview

 "gitweb" shows a link to visit the 'raw' contents of blbos in the
 history overview page.


* jt/diff-color-move-fix (2017-08-16) 3 commits
  (merged to 'next' on 2017-08-19 at 6ae3831c8c)
 + diff: define block by number of alphanumeric chars
 + diff: respect MIN_BLOCK_LENGTH for last block
 + diff: avoid redundantly clearing a flag
 (this branch uses sb/diff-color-move.)

 A handful of bugfixes and an improvement to "diff --color-moved".


* jt/doc-pack-objects-fix (2017-08-23) 1 commit
  (merged to 'next' on 2017-08-24 at 7e4c3c0c9f)
 + Doc: clarify that pack-objects makes packs, 

Re: [PATCH 02/12] load_subtree(): remove unnecessary conditional

[Michael Haggerty]

On Sat, Aug 26, 2017 at 6:38 PM, Junio C Hamano  wrote:
> Michael Haggerty  writes:
>
>> At this point in the code, len is *always* <= 20.
>
> This is the kind of log message that makes me unconfortable, as it
> lacks "because", and the readers would need to find out themselves
> by following the same codepath the patch author already followed.
> [...]

That's a valid complaint. I've adjusted the patch series to add the
assertion and explain the reasoning better in the commit message. I've
pushed the revised series to GitHub, but I'll wait a couple of days to
see if there's more feedback before resubmitting.

Thanks,
Michael