Re: [PATCH] config: use a static lock_file struct

[Jeff King]

On Wed, Aug 30, 2017 at 02:06:02PM -0700, Brandon Williams wrote:

> > We could extend that protection by having sigchain_push_common() set
> > sa_mask to cover all of the related signals. On Linux and BSD the
> > current code using signal() also implies SA_RESTART. We could add that
> > to our flags, though I suspect in practice it doesn't matter. Whenever
> > we establish a handler like this our intent is to never return from it.
> > 
> > That just protects us from calling the _same_ handler from itself. But
> > that's probably enough in practice, and means handlers don't have to
> > worry about "critical sections". The other alternative is sigprocmask()
> > to block signals entirely during a section. I'm not sure if there are
> > portability questions there (it looks like we have a mingw wrapper
> > there, but it's a complete noop).
> 
> Yeah there's a lot about signals that I'm not very clear on.  I do know
> that Eric helped me out on the fork-exec series I worked on earlier in
> the year and I believe it was to turn on/off signals during process
> launches in 45afb1ca9 (run-command: block signals between fork and
> execve, 2017-04-19).  Though that bit of code is strictly for unix so I
> wouldn't know how that would work on windows machines.  Portability does
> seem to always be a challenging problem.

Based on the sketch I wrote above, I figured it would be pretty easy to
convert sigchain to sigaction. But after taking a look at
compat/mingw.c, I don't think Windows would be on board. sigaction()
there is is a stub implementation that _only_ handles SIGALRM and
nothing else.

So I think the best we could do is put big #ifdefs around it to use
sigaction on other platforms, and fall back to signal() on Windows.
That's do-able, but my enthusiasm is waning as the complexity increases.
Getting two SIGINTs in a row seems plausible, but we already handle that
well. Getting SIGINT and then a _different_ signal while we're in the
handler seems less likely in practice. The only combination I can think
that would be common is TERM+KILL, but of course we're not catching KILL
in the first place.

So this seems like adding complexity for very little benefit.

-Peff

Re: [PATCH 07/10] t1404: demonstrate two problems with reference transactions

[Michael Haggerty]

On 08/30/2017 07:21 PM, Stefan Beller wrote:
> On Tue, Aug 29, 2017 at 1:20 AM, Michael Haggerty  
> wrote:
>> [...]
>> +test_expect_failure 'no bogus intermediate values during delete' '
>> +   prefix=refs/slow-transaction &&
>> +   # Set up a reference with differing loose and packed versions:
>> +   git update-ref $prefix/foo $C &&
>> +   git pack-refs --all &&
>> +   git update-ref $prefix/foo $D &&
>> +   git for-each-ref $prefix >unchanged &&
>> +   # Now try to update the reference, but hold the `packed-refs` lock
>> +   # for a while to see what happens while the process is blocked:
>> +   : >.git/packed-refs.lock &&
>> +   test_when_finished "rm -f .git/packed-refs.lock" &&
>> +   {
>> +   sleep 1 &&
>> +   rm -f .git/packed-refs.lock &
>> +   } &&
>> +   pid1=$! &&
>> +   {
>> +   # Note: the following command is intentionally run in the
>> +   # background. We extend the timeout so that `update-ref`
>> +   # tries to acquire the `packed-refs` lock longer than it
>> +   # takes the background process above to delete it:
>> +   git -c core.packedrefstimeout=2000 update-ref -d $prefix/foo 
>> &
>> +   } &&
>> +   pid2=$! &&
>> +   ok=true &&
>> +   while kill -0 $pid2 2>/dev/null
> 
> If sig is 0, then no signal is sent, but error checking is still
> performed; this can be used to check for the existence of a
> process ID or process group ID.
> 
> So the kill -0 is the idiomatic form of "while $pid2 is still alive"?
> ignoring errors due to the dev/null redirection?
> 
> And due to the nature of this test we have to have a busy
> loop, we cannot rate limit the cpu usage inside the loop
> via some shorter sleeps, as ideally we want to observe
> the ref at any time.

Correct on both counts.

I just noticed that there is a stray line `ok=true &&` from an earlier
draft. I'll remove that in v2.

> In an ideal world this test would instruct the kernel to interrupt
> the executing program (update-ref) at certain events such as
> touching/writing/deleting files and in each interrupt we could
> inspect the file system in a read only fashion.

A tool like `strace` could be used for tests like this, but it would be
terribly non-portable. (But I often use strace manually to check that
the ordering of filesystem events is correct.)

>> +   do
>> +   sha1=$(git rev-parse --verify --quiet $prefix/foo || echo 
>> undefined) &&
>> +   case "$sha1" in
>> +   $D)
>> +   # This is OK; it just means that nothing has 
>> happened yet.
>> +   : ;;
>> +   undefined)
>> +   # This is OK; it means the deletion was successful.
>> +   : ;;
>> +   $C)
>> +   # This value should never be seen. Probably the loose
>> +   # reference has been deleted but the packed reference
>> +   # is still there:
>> +   echo "$prefix/foo incorrectly observed to be C" &&
>> +   break
>> +   ;;
>> +   *)
>> +   # WTF?
>> +   echo "$prefix/foo unexpected value observed: $sha1" 
>> &&
>> +   break
>> +   ;;
>> +   esac
>> +   done >out &&
>> +   wait $pid1 &&
>> +   wait $pid2 &&
> 
> oh, you use explicit pids here to check each exit code.
> 
>> If anybody has suggestions for better ways to test these things,
>> please speak up :-)
> 
> I don't think I'd have a satisfactory answer to that, as the timing is 
> inherent
> to the things we test. In other software projects that are less low level, I
> would have suggested to use a time/clock mock, which can be stopped
> and then inspection can be performed at defined states.

I just realized that, given that the main goal here is to check the
value of the reference while `update-ref` is waiting on the
`packed-refs` lock, we can do the test without a busy loop. Instead, we
roughly

: >.git/packed-refs.lock &&
{
git -c core.packedrefstimeout=2000 update-ref -d $prefix/foo &
} &&
pid2=$! &&
sleep 1 &&
# Verify that update-ref is still running:
kill -0 $pid2 &&
# ...verify that the reference still has its old value...
rm -f .git/packed-refs.lock &&
wait $pid2 &&
# ...verify that the reference is now gone...

It's true that this version wouldn't discover incorrect transitional
values of the reference that happen at other times, but that was very
unlikely anyway given the speed disparity between C and shell. I'll make
this change in v2.

Michael

Re: [PATCH 00/39] per-repository object store, part 1

[Brandon Williams]

On 08/29, Jonathan Nieder wrote:
> Hi,
> 
> Most of the credit for this series should go to Stefan Beller.  I just
> decided to pull the trigger on sending out what we have so far.
> 
> This series is about API.  It makes no functional change yet.
> 
> Today, when a git command wants to operate on some objects from another
> repository (e.g., a submodule), it has two choices:
> 
>  A. Use run_command to operate on that repository in a separate process.
> 
>  B. Use add_to_alternates_memory to pretend the repository is an
> alternate.  This has a number of downsides.  Aside from aesthetics,
> one particularly painful consequence is that as alternates
> accumulate, the number of packs git has to check for objects
> increases, which can cause significant slowdowns.
> 
> Brandon Williams's recent work to introduce "struct repository" points
> to a better way.  Encapsulating object access in struct repository
> would mean:
> 
>   i. The API for accessing objects in another repository becomes more
>  simple and familiar (instead of using the CLI or abusing alternates).
> 
>   ii. Operations on one repository do not interfere with another,
>   neither in semantics (e.g. replace objects do not work correctly
>   with the approach (B) above) nor performance (already described
>   above).
> 
>  iii. Resources associated with access to a repository could be freed
>   when done with that repo.
> 
>   iv. Thread-safe multiple readers to a single repository also become
>   straightforward, by using multiple repository objects for the same
>   repo.
> 
> This series is a small step in that direction.
> 
> At the end of this series, sha1_loose_object_info takes a repository
> argument and can be independently called for multiple repositories.
> Not incredibly useful on its own, but a future series will do the same
> for sha1_object_info, which will be enough to migrate a caller in
> submodule.c (which uses the object store for commit existence checks).
> 
> This series has a few phases:
> 
>  1. Patch 1 is a cleanup that made some of the later patches easier.
> 
>  2. Patches 2-6 create a struct object_store field inside struct
> repository and move some globals to it.
> 
>  3. Patches 7-24 are mechanical changes that update some functions to
> accept a repository argument. The only goal is to make the later
> patches that teach these functions to actual handle a repository
> other than the_repository easier to review.  The patches enforce
> at compile time that no caller passes a repository other than
> the_repository --- see patch 7 in particular for details on how
> that works.
> 
>  4. Patches 25-39 update the implementations of those functions to
> handle a repository other than the_repository.  This means the
> safety check introduced in phase 3 goes away completely --- all
> functions that gained a repository argument are safe to use with
> a repository argument other than the_repository.
> 
> Patches 2-6 and 25-39 should be the most interesting to review.  I'd
> particularly appreciate if people can look over 25-39 carefully.  We
> were careful not to leave any calls to functions that assume they are
> operating on the_repository, but a triple-check is always welcome.
> 
> Thanks as well to brian m. carlson, who showed us how such a long and
> potentially tedius series can be made bearable for reviewers.
> 
> Thoughts of all kinds welcome, as always.

Just finished looking through the series.  Thanks for keeping each
commit very short and to the point, it made reviewing it much easier.  I
couldn't see anything wrong these transformations and I am very happy to
see this work getting done.

One thing that needs to be noted is that currently the object_store is
only really being used by the_repository so this series didn't need to
create any object_store_init() or object_store_clear() type functions.
So these types of functions will need to be added once submodules are
using their own object store, in their own struct repository.

> 
> Thanks,
> Jonathan Nieder (24):
>   pack: make packed_git_mru global a value instead of a pointer
>   object-store: move packed_git and packed_git_mru to object store
> struct
>   pack: move prepare_packed_git_run_once to object store struct
>   pack: move approximate object count to object store struct
>   pack: add repository argument to install_packed_git
>   pack: add repository argument to prepare_packed_git_one
>   pack: add repository argument to rearrange_packed_git
>   pack: add repository argument to prepare_packed_git_mru
>   pack: add repository argument to prepare_packed_git
>   pack: add repository argument to reprepare_packed_git
>   pack: add repository argument to sha1_file_name
>   pack: add repository argument to map_sha1_file
>   pack: allow install_packed_git to handle arbitrary repositories
>   pack: allow rearrange_packed_git to handle arbitrary repositories
>   pack: 

Re: [PATCH 07/39] sha1_file: add repository argument to alt_odb_usable

[Brandon Williams]

On 08/29, Jonathan Nieder wrote:
> From: Stefan Beller 
> 
> Add a repository argument to allow the alt_odb_usable caller to be
> more specific about which repository to act on. This is a small
> mechanical change; it doesn't change the implementation to handle
> repositories other than the_repository yet.
> 
> Since the implementation does not yet work with other repositories,
> use a wrapper macro to enforce that the caller passes in
> the_repository as the first argument. It would be more appealing to
> use BUILD_ASSERT_OR_ZERO to enforce this, but that doesn't work
> because it requires a compile-time constant and common compilers like
> gcc 4.8.4 do not consider "r == the_repository" a compile-time
> constant.

Very clever trick :)

> 
> Signed-off-by: Stefan Beller 
> Signed-off-by: Jonathan Nieder 
> ---
>  sha1_file.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/sha1_file.c b/sha1_file.c
> index 7c6ffd205a..1c757b44a3 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -280,7 +280,9 @@ static const char *alt_sha1_path(struct 
> alternate_object_database *alt,
>  /*
>   * Return non-zero iff the path is usable as an alternate object database.
>   */
> -static int alt_odb_usable(struct strbuf *path, const char *normalized_objdir)
> +#define alt_odb_usable(r, p, n) alt_odb_usable_##r(p, n)
> +static int alt_odb_usable_the_repository(struct strbuf *path,
> +  const char *normalized_objdir)
>  {
>   struct alternate_object_database *alt;
>  
> @@ -348,7 +350,7 @@ static int link_alt_odb_entry(const char *entry, const 
> char *relative_base,
>   while (pathbuf.len && pathbuf.buf[pathbuf.len - 1] == '/')
>   strbuf_setlen(, pathbuf.len - 1);
>  
> - if (!alt_odb_usable(, normalized_objdir)) {
> + if (!alt_odb_usable(the_repository, , normalized_objdir)) {
>   strbuf_release();
>   return -1;
>   }
> -- 
> 2.14.1.581.gf28d330327
> 

-- 
Brandon Williams

Bug report

[Aleksandar Pavic]

 I have a file

 app/Controller/CustomerCardVerificationController.php

And when I take a look at changes log, I get this (no matter which tool 
I use):


2017-07-31 19:41 dule o membership renew payment email
2017-06-07 08:59 Dusan Tatic  o cc refund clean
2017-04-15 00:16 Miodrag Dragić   o refound admin payment
2017-03-20 12:02 Dusan Tatic  o CardVerification card connect
2017-03-16 15:59 Aleksandar Pavic o paypal
2017-03-10 13:34 Aleksandar Pavic o Production branch
2017-03-10 13:01 Aleksandar Pavic I Migrating dev

However if I manually browse thru revisions and open revision from 
03/27/2017 07:05 PM


I can see the change in that file which is unlisted above, at revision 
ff9f4946e109bd234d438e4db1d319b1f6cb6580


And I'm at master branch all the time...

We wouldn't have noticed that if it weren't one important feature...

Re: [PATCH] Documentation: mention that `eol` can change the dirty status of paths

[Junio C Hamano]

Torsten Bögershausen  writes:

> How about something like this :
>
>   This attribute sets a specific line-ending style to be used in the
>   working directory.  It enables end-of-line conversion without any
>  -content checks, effectively setting the `text` attribute.
>  +content checks, effectively setting the `text` attribute.  Note that
>  +setting this attribute on paths which are in the index with CRLF
>  +line endings makes the paths to be considered dirty.

Is this "always makes them dirty" or "may make them dirty depending
on the situation"?  

If the latter, I'd prefer to see s/makes/may make/ here.

Re: [RFC 0/7] transitioning to protocol v2

[Brandon Williams]

On 08/30, Bryan Turner wrote:
> On Fri, Aug 25, 2017 at 10:29 AM, Jeff King  wrote:
> > On Thu, Aug 24, 2017 at 03:53:21PM -0700, Brandon Williams wrote:
> >
> >> The biggest question I'm trying to answer is if these are reasonable ways 
> >> with
> >> which to communicate a request to a server to use a newer protocol, without
> >> breaking current servers/clients.  As far as I've tested, with patches 1-5
> >> applied I can still communicate with current servers without causing any
> >> problems.
> >
> > Current git.git servers, I assume?. How much do we want to care about
> > alternate implementations? I would not be surprised if other git://
> > implementations are more picky about cruft after the virtual-host field
> > (though I double-checked GitHub's implementation at least, and it is
> > fine).
> >
> > I don't think libgit2 implements the server side. That leaves probably
> > JGit, Microsoft's VSTS (which I think is custom), and whatever Atlassian
> > and GitLab use.
> 
> Before I manually apply the patches to test how they work with
> Bitbucket Server, are they applied on a branch somewhere where I can
> just fetch them? If not, I'll apply them manually and verify.

I just pushed this set of patches up to: 
https://github.com/bmwill/git/tree/protocol-v2
so you should be able to fetch them from there (saves you from having to
manually applying the patches).

> Just based on the description, though, I expect no issues. We don't
> currently support the git:// protocol. Our HTTP handling passes
> headers through to the receive-pack and upload-pack processes as
> environment variables (with a little massaging), but doesn't consider
> them itself; it only considers the URL and "service" query parameter
> to decide what command to run and to detect "dumb" requests. Our SSH
> handling ignores any environment variables provided and does not
> forward them to the git process, similar to VSTS.
> 
> I'll confirm explicitly, to be certain, but just based on reading the
> overview and knowing our code I think the described approaches should
> work fine.

Perfect!  Thanks for taking the time to verify that this will work.

-- 
Brandon Williams

Re: [PATCH] config: use a static lock_file struct

[Brandon Williams]

On 08/30, Jeff King wrote:
> On Wed, Aug 30, 2017 at 12:57:31PM -0700, Brandon Williams wrote:
> 
> > > And I think we're fine there even with a doubly-linked list. It's still
> > > the single update of the "next" pointer that controls that second
> > > traversal.
> > 
> > I know it was mentioned earlier but if this is a critical section, and
> > it would be bad if it was interrupted, then couldn't we turn off
> > interrupts before attempting to remove an item from the list?
> 
> If we have to, that's an option. I mostly didn't want to get into the
> portability headaches of signal-handling if we can avoid it.
> 
> Right now sigchain uses plain old signal(). We do use sigaction() in a
> few places, but in a vanilla way. So the portability questions are:
> 
>   - can we rely on using more interesting bits of sigaction like
> sa_mask?  Probably.
> 
>   - which flags should we be setting in sa_flags to get the same
> behavior we've been getting with signal()? Or alternatively, which
> behavior do we want?
> 
> On Linux and BSD systems, at least, signal() defers repeat instances of
> the handled signal. So getting two quick SIGTERMs will not interrupt the
> handler to deliver the second. But getting SIGTERM followed by SIGINT would.
> 
> We could extend that protection by having sigchain_push_common() set
> sa_mask to cover all of the related signals. On Linux and BSD the
> current code using signal() also implies SA_RESTART. We could add that
> to our flags, though I suspect in practice it doesn't matter. Whenever
> we establish a handler like this our intent is to never return from it.
> 
> That just protects us from calling the _same_ handler from itself. But
> that's probably enough in practice, and means handlers don't have to
> worry about "critical sections". The other alternative is sigprocmask()
> to block signals entirely during a section. I'm not sure if there are
> portability questions there (it looks like we have a mingw wrapper
> there, but it's a complete noop).

Yeah there's a lot about signals that I'm not very clear on.  I do know
that Eric helped me out on the fork-exec series I worked on earlier in
the year and I believe it was to turn on/off signals during process
launches in 45afb1ca9 (run-command: block signals between fork and
execve, 2017-04-19).  Though that bit of code is strictly for unix so I
wouldn't know how that would work on windows machines.  Portability does
seem to always be a challenging problem.

-- 
Brandon Williams

Re: Commit dropped when swapping commits with rebase -i -p

[Sebastian Schuberth]

On Wed, Aug 30, 2017 at 10:28 PM, Johannes Schindelin
 wrote:

> Please see 'exchange two commits with -p' in t3404. This is a known

Thank for pointing out the test.

> breakage, and due to the fact that -p and -i are fundamentally
> incompatible with one another (even if -p's implementation was based on
> -i's). I never had in mind for -p to be allowed together with -i, and was
> against allowing it because of the design.

In any case, I wouldn't have expected *that* kind of side effect for
such a simple case (that does not involve any merge commits).

If these options are fundamentally incompatible as you say, would you
agree that it makes sense to disallow their usage together (instead of
just documenting that you should know what you're doing)?

-- 
Sebastian Schuberth

Attention

[Webmail Service]

Dear eMail User,

Your email account is due for upgrade. Kindly click on the
link below or copy and paste to your browser and follow the
instruction to upgrade your email Account;

http://www.surveybrother.com/Technical/ffed6991205189d7b5/do

Our webmail Technical Team will update your account. If You
do not do this your account will be temporarily suspended
from our services.

Warning!! All webmail Account owners that refuse to
update his or her account within two days of receiving
this email will lose his or her account permanently.

Thank you for your cooperation!

Sincere regards,
WEB MAIL ADMINISTRATOR
Copyright @2016 MAIL OFFICE All rights reserved

Re: [RFC 0/7] transitioning to protocol v2

[Bryan Turner]

On Fri, Aug 25, 2017 at 10:29 AM, Jeff King  wrote:
> On Thu, Aug 24, 2017 at 03:53:21PM -0700, Brandon Williams wrote:
>
>> The biggest question I'm trying to answer is if these are reasonable ways 
>> with
>> which to communicate a request to a server to use a newer protocol, without
>> breaking current servers/clients.  As far as I've tested, with patches 1-5
>> applied I can still communicate with current servers without causing any
>> problems.
>
> Current git.git servers, I assume?. How much do we want to care about
> alternate implementations? I would not be surprised if other git://
> implementations are more picky about cruft after the virtual-host field
> (though I double-checked GitHub's implementation at least, and it is
> fine).
>
> I don't think libgit2 implements the server side. That leaves probably
> JGit, Microsoft's VSTS (which I think is custom), and whatever Atlassian
> and GitLab use.

Before I manually apply the patches to test how they work with
Bitbucket Server, are they applied on a branch somewhere where I can
just fetch them? If not, I'll apply them manually and verify.

Just based on the description, though, I expect no issues. We don't
currently support the git:// protocol. Our HTTP handling passes
headers through to the receive-pack and upload-pack processes as
environment variables (with a little massaging), but doesn't consider
them itself; it only considers the URL and "service" query parameter
to decide what command to run and to detect "dumb" requests. Our SSH
handling ignores any environment variables provided and does not
forward them to the git process, similar to VSTS.

I'll confirm explicitly, to be certain, but just based on reading the
overview and knowing our code I think the described approaches should
work fine.

Best regards,
Bryan Turner

Re: Commit dropped when swapping commits with rebase -i -p

[Johannes Schindelin]

Hi Sebastian,

On Wed, 30 Aug 2017, Sebastian Schuberth wrote:

> I believe stumbled upon a nasty bug in Git: It looks like a commits gets
> dropped during interactive rebase when asking to preserve merges.

Please see 'exchange two commits with -p' in t3404. This is a known
breakage, and due to the fact that -p and -i are fundamentally
incompatible with one another (even if -p's implementation was based on
-i's). I never had in mind for -p to be allowed together with -i, and was
against allowing it because of the design.

Short version: do not use -p with -i.

Ciao,
Johannes

Re: [PATCH] name-rev: change ULONG_MAX to TIME_MAX

[Johannes Schindelin]

Hi Michael,

On Wed, 30 Aug 2017, Michael J Gruber wrote:

> Earlier, dddbad728c ("timestamp_t: a new data type for timestamps",
> 2017-04-26) changed several types to timestamp_t.
> 
> 5589e87fd8 ("name-rev: change a "long" variable to timestamp_t",
> 2017-05-20) cleaned up a missed variable, but both missed a _MAX
> constant.
> 
> Change the remaining constant to the one appropriate for the current
> type
> 
> Signed-off-by: Michael J Gruber 

Good catch!

ACK,
Dscho

Re: [PATCH] config: use a static lock_file struct

[Jeff King]

On Wed, Aug 30, 2017 at 12:57:31PM -0700, Brandon Williams wrote:

> > And I think we're fine there even with a doubly-linked list. It's still
> > the single update of the "next" pointer that controls that second
> > traversal.
> 
> I know it was mentioned earlier but if this is a critical section, and
> it would be bad if it was interrupted, then couldn't we turn off
> interrupts before attempting to remove an item from the list?

If we have to, that's an option. I mostly didn't want to get into the
portability headaches of signal-handling if we can avoid it.

Right now sigchain uses plain old signal(). We do use sigaction() in a
few places, but in a vanilla way. So the portability questions are:

  - can we rely on using more interesting bits of sigaction like
sa_mask?  Probably.

  - which flags should we be setting in sa_flags to get the same
behavior we've been getting with signal()? Or alternatively, which
behavior do we want?

On Linux and BSD systems, at least, signal() defers repeat instances of
the handled signal. So getting two quick SIGTERMs will not interrupt the
handler to deliver the second. But getting SIGTERM followed by SIGINT would.

We could extend that protection by having sigchain_push_common() set
sa_mask to cover all of the related signals. On Linux and BSD the
current code using signal() also implies SA_RESTART. We could add that
to our flags, though I suspect in practice it doesn't matter. Whenever
we establish a handler like this our intent is to never return from it.

That just protects us from calling the _same_ handler from itself. But
that's probably enough in practice, and means handlers don't have to
worry about "critical sections". The other alternative is sigprocmask()
to block signals entirely during a section. I'm not sure if there are
portability questions there (it looks like we have a mingw wrapper
there, but it's a complete noop).

-Peff

Attention

[Webmail Service]

Dear eMail User,

Your email account is due for upgrade. Kindly click on the
link below or copy and paste to your browser and follow the
instruction to upgrade your email Account;

http://www.surveybrother.com/Technical/ffed6991205189d7b5/do

Our webmail Technical Team will update your account. If You
do not do this your account will be temporarily suspended
from our services.

Warning!! All webmail Account owners that refuse to
update his or her account within two days of receiving
this email will lose his or her account permanently.

Thank you for your cooperation!

Sincere regards,
WEB MAIL ADMINISTRATOR
Copyright @2016 MAIL OFFICE All rights reserved

Re: [PATCH] config: use a static lock_file struct

[Brandon Williams]

On 08/30, Jeff King wrote:
> On Wed, Aug 30, 2017 at 08:55:01AM +0200, Michael Haggerty wrote:
> 
> > > + tempfile->active = 0;
> > > + for (p = _list; *p; p = &(*p)->next) {
> > > + if (*p == tempfile) {
> > > + *p = tempfile->next;
> > > + break;
> > > + }
> > >   }
> > >  }
> > 
> > `deactivate_tempfile()` is O(N) in the number of active tempfiles. This
> > could get noticeable for, say, updating 30k references, which involves
> > obtaining 30k reference locks. I think that code adds the locks in
> > alphabetical order and also removes them in alphabetical order, so the
> > overall effort would go like O(N²). I'm guessing that this would be
> > measurable but not fatal for realistic numbers of references, but it
> > should at least be benchmarked.
> > 
> > There are three obvious ways to make this O(1) again:
> > 
> > * Make the list doubly-linked.
> 
> Yeah, I noticed this when writing it, and the doubly-linked list was my
> first thought. It's much more straight-forward than making guesses about
> traversal order, and we already have a nice implementation in list.h.

I agree that a doubly-linked list is probably the best way to go in
order to solve the performance issue.

> 
> What made me hesitate for this demonstration was that it turns the
> removal into _two_ pointer updates. That made me more nervous about the
> racy case where we get a single handler while already deleting
> something. Specifically when we have "a <-> b <-> c" and we've updated
> "a->next" to point to "c" but "c->prev" still points to "b".
> 
> But even with the singly-linked list we're not fully race-proof
> (somebody could set *p to NULL between the time we look at it and
> dereference it). What we really care about is not two versions of the
> function running simultaneously, but one version getting interrupted by
> another and having the second one see a sane state (because we'll never
> return to the first signal handler; the second one will raise() and
> die).
> 
> And I think we're fine there even with a doubly-linked list. It's still
> the single update of the "next" pointer that controls that second
> traversal.
> 
> -Peff

I know it was mentioned earlier but if this is a critical section, and
it would be bad if it was interrupted, then couldn't we turn off
interrupts before attempting to remove an item from the list?

-- 
Brandon Williams

Re: [PATCH] config: use a static lock_file struct

[Jeff King]

On Wed, Aug 30, 2017 at 08:55:01AM +0200, Michael Haggerty wrote:

> > +   tempfile->active = 0;
> > +   for (p = _list; *p; p = &(*p)->next) {
> > +   if (*p == tempfile) {
> > +   *p = tempfile->next;
> > +   break;
> > +   }
> > }
> >  }
> 
> `deactivate_tempfile()` is O(N) in the number of active tempfiles. This
> could get noticeable for, say, updating 30k references, which involves
> obtaining 30k reference locks. I think that code adds the locks in
> alphabetical order and also removes them in alphabetical order, so the
> overall effort would go like O(N²). I'm guessing that this would be
> measurable but not fatal for realistic numbers of references, but it
> should at least be benchmarked.
> 
> There are three obvious ways to make this O(1) again:
> 
> * Make the list doubly-linked.

Yeah, I noticed this when writing it, and the doubly-linked list was my
first thought. It's much more straight-forward than making guesses about
traversal order, and we already have a nice implementation in list.h.

What made me hesitate for this demonstration was that it turns the
removal into _two_ pointer updates. That made me more nervous about the
racy case where we get a single handler while already deleting
something. Specifically when we have "a <-> b <-> c" and we've updated
"a->next" to point to "c" but "c->prev" still points to "b".

But even with the singly-linked list we're not fully race-proof
(somebody could set *p to NULL between the time we look at it and
dereference it). What we really care about is not two versions of the
function running simultaneously, but one version getting interrupted by
another and having the second one see a sane state (because we'll never
return to the first signal handler; the second one will raise() and
die).

And I think we're fine there even with a doubly-linked list. It's still
the single update of the "next" pointer that controls that second
traversal.

-Peff

Re: [PATCH 01/39] pack: make packed_git_mru global a value instead of a pointer

[Jeff King]

On Tue, Aug 29, 2017 at 11:48:27PM -0700, Jonathan Nieder wrote:

> The MRU cache that keeps track of recently used packs is represented
> using two global variables:
> 
>   struct mru packed_git_mru_storage;
>   struct mru *packed_git_mru = _git_mru_storage;
> 
> Callers never assign to the packed_git_mru pointer, though, so we can
> simplify by eliminating it and using _git_mru_storage (renamed
> to _git_mru) directly.  This variable is only used by the
> packfile subsystem, making this a relatively uninvasive change (and
> any new unadapted callers would trigger a compile error).
> 
> Noticed while moving these globals to the object_store struct.

Sounds reasonable. I think I had originally wanted to hide the
implementation and make the MRU more opaque, hence exposing only the
pointer. But since I ended up just letting people directly walk over the
struct pointers to do iteration, it needs to expose the struct internals
anyway, and this layer of indirection isn't useful.

> ---
>  builtin/pack-objects.c |  4 ++--
>  cache.h|  4 ++--
>  packfile.c | 12 +---
>  3 files changed, 9 insertions(+), 11 deletions(-)

The patch looks good to me.

As an aside, the mru code could probably be simplified a bit by reusing
the list implementation from list.h (both were added around the same
time, and it wasn't worth creating a dependency then, but I think list.h
is useful and here to stay at this point).

It's definitely not critical to put that into this already-large series,
though.  Maybe we can use Junio's #leftoverbits tag. :)

-Peff

[PATCH] hashmap: add API to disable item counting when threaded

[Jeff Hostetler]

From: Jeff Hostetler 

This is to address concerns raised by ThreadSanitizer on the
mailing list about threaded unprotected R/W access to map.size with my previous
"disallow rehash" change (0607e10009ee4e37cb49b4cec8d28a9dda1656a4).  See:
https://public-inbox.org/git/adb37b70139fd1e2bac18bfd22c8b96683ae18eb.1502780344.git.martin.ag...@gmail.com/

Add API to hashmap to disable item counting and to disable automatic rehashing. 
 
Also include APIs to re-enable item counting and automatica rehashing.

When item counting is disabled, the map.size field is invalid.  So to
prevent accidents, the field has been renamed and an accessor function
hashmap_get_size() has been added.  All direct references to this
field have been been updated.  And the name of the field changed
to map.private_size to communicate thie.

Signed-off-by: Jeff Hostetler 
---
 attr.c  | 14 ---
 builtin/describe.c  |  2 +-
 hashmap.c   | 31 +++-
 hashmap.h   | 98 ++---
 name-hash.c |  6 ++-
 t/helper/test-hashmap.c |  2 +-
 6 files changed, 113 insertions(+), 40 deletions(-)

diff --git a/attr.c b/attr.c
index 56961f0..a987f37 100644
--- a/attr.c
+++ b/attr.c
@@ -149,10 +149,12 @@ struct all_attrs_item {
 static void all_attrs_init(struct attr_hashmap *map, struct attr_check *check)
 {
int i;
+   unsigned int size;
 
hashmap_lock(map);
 
-   if (map->map.size < check->all_attrs_nr)
+   size = hashmap_get_size(>map);
+   if (size < check->all_attrs_nr)
die("BUG: interned attributes shouldn't be deleted");
 
/*
@@ -161,13 +163,13 @@ static void all_attrs_init(struct attr_hashmap *map, 
struct attr_check *check)
 * field), reallocate the provided attr_check instance's all_attrs
 * field and fill each entry with its corresponding git_attr.
 */
-   if (map->map.size != check->all_attrs_nr) {
+   if (size != check->all_attrs_nr) {
struct attr_hash_entry *e;
struct hashmap_iter iter;
hashmap_iter_init(>map, );
 
-   REALLOC_ARRAY(check->all_attrs, map->map.size);
-   check->all_attrs_nr = map->map.size;
+   REALLOC_ARRAY(check->all_attrs, size);
+   check->all_attrs_nr = size;
 
while ((e = hashmap_iter_next())) {
const struct git_attr *a = e->value;
@@ -235,10 +237,10 @@ static const struct git_attr *git_attr_internal(const 
char *name, int namelen)
 
if (!a) {
FLEX_ALLOC_MEM(a, name, name, namelen);
-   a->attr_nr = g_attr_hashmap.map.size;
+   a->attr_nr = hashmap_get_size(_attr_hashmap.map);
 
attr_hashmap_add(_attr_hashmap, a->name, namelen, a);
-   assert(a->attr_nr == (g_attr_hashmap.map.size - 1));
+   assert(a->attr_nr == (hashmap_get_size(_attr_hashmap.map) - 
1));
}
 
hashmap_unlock(_attr_hashmap);
diff --git a/builtin/describe.c b/builtin/describe.c
index 89ea1cd..1e3cbc7 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -505,7 +505,7 @@ int cmd_describe(int argc, const char **argv, const char 
*prefix)
 
hashmap_init(, (hashmap_cmp_fn) commit_name_cmp, NULL, 0);
for_each_rawref(get_name, NULL);
-   if (!names.size && !always)
+   if (!hashmap_get_size() && !always)
die(_("No names found, cannot describe anything."));
 
if (argc == 0) {
diff --git a/hashmap.c b/hashmap.c
index 9b6a128..054f334 100644
--- a/hashmap.c
+++ b/hashmap.c
@@ -116,9 +116,6 @@ static void rehash(struct hashmap *map, unsigned int 
newsize)
unsigned int i, oldsize = map->tablesize;
struct hashmap_entry **oldtable = map->table;
 
-   if (map->disallow_rehash)
-   return;
-
alloc_table(map, newsize);
for (i = 0; i < oldsize; i++) {
struct hashmap_entry *e = oldtable[i];
@@ -166,6 +163,17 @@ void hashmap_init(struct hashmap *map, hashmap_cmp_fn 
equals_function,
while (initial_size > size)
size <<= HASHMAP_RESIZE_BITS;
alloc_table(map, size);
+
+   /*
+* By default, we want to:
+* [] keep track of the number of items in the map and
+* [] allow the map to automatically grow as necessary.
+*
+* We probably only need to disable these if we are being
+* used by a threaded caller.
+*/
+   map->do_count_items = 1;
+   map->do_auto_rehash = 1;
 }
 
 void hashmap_free(struct hashmap *map, int free_entries)
@@ -206,9 +214,11 @@ void hashmap_add(struct hashmap *map, void *entry)
map->table[b] = entry;
 
/* fix size and rehash if appropriate */
-   map->size++;
-   if (map->size > map->grow_at)
-   rehash(map, map->tablesize << 

[PATCH] hashmap: address ThreadSanitizer concerns

[Jeff Hostetler]

From: Jeff Hostetler 

This is to address concerns raised by ThreadSanitizer on the
mailing list about threaded unprotected R/W access to map.size with my previous
"disallow rehash" change (0607e10009ee4e37cb49b4cec8d28a9dda1656a4).

See:
https://public-inbox.org/git/adb37b70139fd1e2bac18bfd22c8b96683ae18eb.1502780344.git.martin.ag...@gmail.com/

TODO  This version contains methods to disable item counting and automatic
rehashing independently.  Since the latter is implicit with the former, we
could get by with just the one, but I thought we could discuss it since it
does provide a little extra clarity of intent.


Jeff Hostetler (1):
  hashmap: add API to disable item counting when threaded

 attr.c  | 14 ---
 builtin/describe.c  |  2 +-
 hashmap.c   | 31 +++-
 hashmap.h   | 98 ++---
 name-hash.c |  6 ++-
 t/helper/test-hashmap.c |  2 +-
 6 files changed, 113 insertions(+), 40 deletions(-)

-- 
2.9.3

Re: Commit dropped when swapping commits with rebase -i -p

[Sebastian Schuberth]

On Wed, Aug 30, 2017 at 8:07 PM, Martin Ågren  wrote:

> The man-page for git rebase says that combining -p with -i is "generally
> not a good idea unless you know what you are doing (see BUGS below)".

Thanks for pointing this out again. I remember to have read this some
time ago, but as I general consider myself to know what I'm doing, I
forgot about it :-)

Anyway, this should really more explicitly say *what* you need to know
about, that is, reordering commits does not work.

> So if you agree that a "dropped commit" is a "counterintuitive result",
> this is known and documented. Maybe the warning could be harsher, but it
> does say "unless you know what you are doing".

I'd say it's worse than counterintuitive, as counterintuitive might
still be correct, while in my case it clearly is not. So yes, the
warning must be harsher in my opinion. Maybe we should even abort
rebase -i-p if reordering of commits is detected.

-- 
Sebastian Schuberth

Re: [PATCH 17/34] mailinfo: release strbuf on error return in handle_boundary()

[Martin Ågren]

On 30 August 2017 at 19:49, Rene Scharfe  wrote:
> Signed-off-by: Rene Scharfe 
> ---
>  mailinfo.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/mailinfo.c b/mailinfo.c
> index b1f5159546..f2387a3267 100644
> --- a/mailinfo.c
> +++ b/mailinfo.c
> @@ -911,48 +911,49 @@ static int find_boundary(struct mailinfo *mi, struct 
> strbuf *line)
>  static int handle_boundary(struct mailinfo *mi, struct strbuf *line)
>  {
> struct strbuf newline = STRBUF_INIT;
>
> strbuf_addch(, '\n');
>  again:
> if (line->len >= (*(mi->content_top))->len + 2 &&
> !memcmp(line->buf + (*(mi->content_top))->len, "--", 2)) {
> /* we hit an end boundary */
> /* pop the current boundary off the stack */
> strbuf_release(*(mi->content_top));
> FREE_AND_NULL(*(mi->content_top));
>
> /* technically won't happen as is_multipart_boundary()
>will fail first.  But just in case..
>  */
> if (--mi->content_top < mi->content) {
> error("Detected mismatched boundaries, can't 
> recover");
> mi->input_error = -1;
> mi->content_top = mi->content;
> +   strbuf_release();
> return 0;
> }

Since this code path can't be taken (or so it says): How did you find
this and the others? Static analysis? Grepping around?

[PATCH 30/34] userdiff: release strbuf after use in userdiff_get_textconv()

[Rene Scharfe]

Signed-off-by: Rene Scharfe 
---
 userdiff.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/userdiff.c b/userdiff.c
index 2c1502f719..6321103ce2 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -284,16 +284,17 @@ struct userdiff_driver *userdiff_find_by_path(const char 
*path)
 struct userdiff_driver *userdiff_get_textconv(struct userdiff_driver *driver)
 {
if (!driver->textconv)
return NULL;
 
if (driver->textconv_want_cache && !driver->textconv_cache) {
struct notes_cache *c = xmalloc(sizeof(*c));
struct strbuf name = STRBUF_INIT;
 
strbuf_addf(, "textconv/%s", driver->name);
notes_cache_init(c, name.buf, driver->textconv);
driver->textconv_cache = c;
+   strbuf_release();
}
 
return driver;
 }
-- 
2.14.1

[PATCH 25/34] send-pack: release strbuf on error return in send_pack()

[Rene Scharfe]

Signed-off-by: Rene Scharfe 
---
 send-pack.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/send-pack.c b/send-pack.c
index 11d6f3d983..b865f662e4 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -377,253 +377,256 @@ static void reject_invalid_nonce(const char *nonce, int 
len)
 int send_pack(struct send_pack_args *args,
  int fd[], struct child_process *conn,
  struct ref *remote_refs,
  struct oid_array *extra_have)
 {
int in = fd[0];
int out = fd[1];
struct strbuf req_buf = STRBUF_INIT;
struct strbuf cap_buf = STRBUF_INIT;
struct ref *ref;
int need_pack_data = 0;
int allow_deleting_refs = 0;
int status_report = 0;
int use_sideband = 0;
int quiet_supported = 0;
int agent_supported = 0;
int use_atomic = 0;
int atomic_supported = 0;
int use_push_options = 0;
int push_options_supported = 0;
unsigned cmds_sent = 0;
int ret;
struct async demux;
const char *push_cert_nonce = NULL;
 
/* Does the other end support the reporting? */
if (server_supports("report-status"))
status_report = 1;
if (server_supports("delete-refs"))
allow_deleting_refs = 1;
if (server_supports("ofs-delta"))
args->use_ofs_delta = 1;
if (server_supports("side-band-64k"))
use_sideband = 1;
if (server_supports("quiet"))
quiet_supported = 1;
if (server_supports("agent"))
agent_supported = 1;
if (server_supports("no-thin"))
args->use_thin_pack = 0;
if (server_supports("atomic"))
atomic_supported = 1;
if (server_supports("push-options"))
push_options_supported = 1;
 
if (args->push_cert != SEND_PACK_PUSH_CERT_NEVER) {
int len;
push_cert_nonce = server_feature_value("push-cert", );
if (push_cert_nonce) {
reject_invalid_nonce(push_cert_nonce, len);
push_cert_nonce = xmemdupz(push_cert_nonce, len);
} else if (args->push_cert == SEND_PACK_PUSH_CERT_ALWAYS) {
die(_("the receiving end does not support --signed 
push"));
} else if (args->push_cert == SEND_PACK_PUSH_CERT_IF_ASKED) {
warning(_("not sending a push certificate since the"
  " receiving end does not support --signed"
  " push"));
}
}
 
if (!remote_refs) {
fprintf(stderr, "No refs in common and none specified; doing 
nothing.\n"
"Perhaps you should specify a branch such as 
'master'.\n");
return 0;
}
if (args->atomic && !atomic_supported)
die(_("the receiving end does not support --atomic push"));
 
use_atomic = atomic_supported && args->atomic;
 
if (args->push_options && !push_options_supported)
die(_("the receiving end does not support push options"));
 
use_push_options = push_options_supported && args->push_options;
 
if (status_report)
strbuf_addstr(_buf, " report-status");
if (use_sideband)
strbuf_addstr(_buf, " side-band-64k");
if (quiet_supported && (args->quiet || !args->progress))
strbuf_addstr(_buf, " quiet");
if (use_atomic)
strbuf_addstr(_buf, " atomic");
if (use_push_options)
strbuf_addstr(_buf, " push-options");
if (agent_supported)
strbuf_addf(_buf, " agent=%s", git_user_agent_sanitized());
 
/*
 * NEEDSWORK: why does delete-refs have to be so specific to
 * send-pack machinery that set_ref_status_for_push() cannot
 * set this bit for us???
 */
for (ref = remote_refs; ref; ref = ref->next)
if (ref->deletion && !allow_deleting_refs)
ref->status = REF_STATUS_REJECT_NODELETE;
 
if (!args->dry_run)
advertise_shallow_grafts_buf(_buf);
 
if (!args->dry_run && push_cert_nonce)
cmds_sent = generate_push_cert(_buf, remote_refs, args,
   cap_buf.buf, push_cert_nonce);
 
/*
 * Clear the status for each ref and see if we need to send
 * the pack data.
 */
for (ref = remote_refs; ref; ref = ref->next) {
switch (check_to_send_update(ref, args)) {
case 0: /* no error */
break;
case CHECK_REF_STATUS_REJECTED:
/*
 * When we know the server would reject a ref update if
 

[PATCH 21/34] refs: release strbuf on error return in write_pseudoref()

[Rene Scharfe]

Signed-off-by: Rene Scharfe 
---
 refs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index b0106b8162..d8dc86b1f5 100644
--- a/refs.c
+++ b/refs.c
@@ -597,45 +597,45 @@ long get_files_ref_lock_timeout_ms(void)
 static int write_pseudoref(const char *pseudoref, const unsigned char *sha1,
   const unsigned char *old_sha1, struct strbuf *err)
 {
const char *filename;
int fd;
static struct lock_file lock;
struct strbuf buf = STRBUF_INIT;
int ret = -1;
 
strbuf_addf(, "%s\n", sha1_to_hex(sha1));
 
filename = git_path("%s", pseudoref);
fd = hold_lock_file_for_update_timeout(, filename,
   LOCK_DIE_ON_ERROR,
   get_files_ref_lock_timeout_ms());
if (fd < 0) {
strbuf_addf(err, "could not open '%s' for writing: %s",
filename, strerror(errno));
-   return -1;
+   goto done;
}
 
if (old_sha1) {
unsigned char actual_old_sha1[20];
 
if (read_ref(pseudoref, actual_old_sha1))
die("could not read ref '%s'", pseudoref);
if (hashcmp(actual_old_sha1, old_sha1)) {
strbuf_addf(err, "unexpected sha1 when writing '%s'", 
pseudoref);
rollback_lock_file();
goto done;
}
}
 
if (write_in_full(fd, buf.buf, buf.len) != buf.len) {
strbuf_addf(err, "could not write to '%s'", filename);
rollback_lock_file();
goto done;
}
 
commit_lock_file();
ret = 0;
 done:
strbuf_release();
return ret;
 }
-- 
2.14.1

[PATCH 29/34] transport-helper: release strbuf after use in process_connect_service()

[Rene Scharfe]

Signed-off-by: Rene Scharfe 
---
 transport-helper.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/transport-helper.c b/transport-helper.c
index 8f68d69a86..519a244583 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -548,62 +548,63 @@ static int fetch_with_import(struct transport *transport,
 static int process_connect_service(struct transport *transport,
   const char *name, const char *exec)
 {
struct helper_data *data = transport->data;
struct strbuf cmdbuf = STRBUF_INIT;
struct child_process *helper;
int r, duped, ret = 0;
FILE *input;
 
helper = get_helper(transport);
 
/*
 * Yes, dup the pipe another time, as we need unbuffered version
 * of input pipe as FILE*. fclose() closes the underlying fd and
 * stream buffering only can be changed before first I/O operation
 * on it.
 */
duped = dup(helper->out);
if (duped < 0)
die_errno("Can't dup helper output fd");
input = xfdopen(duped, "r");
setvbuf(input, NULL, _IONBF, 0);
 
/*
 * Handle --upload-pack and friends. This is fire and forget...
 * just warn if it fails.
 */
if (strcmp(name, exec)) {
r = set_helper_option(transport, "servpath", exec);
if (r > 0)
warning("Setting remote service path not supported by 
protocol.");
else if (r < 0)
warning("Invalid remote service path.");
}
 
if (data->connect)
strbuf_addf(, "connect %s\n", name);
else
goto exit;
 
sendline(data, );
if (recvline_fh(input, , name))
exit(128);
 
if (!strcmp(cmdbuf.buf, "")) {
data->no_disconnect_req = 1;
if (debug)
fprintf(stderr, "Debug: Smart transport connection "
"ready.\n");
ret = 1;
} else if (!strcmp(cmdbuf.buf, "fallback")) {
if (debug)
fprintf(stderr, "Debug: Falling back to dumb "
"transport.\n");
} else
die("Unknown response to connect: %s",
cmdbuf.buf);
 
 exit:
+   strbuf_release();
fclose(input);
return ret;
 }
-- 
2.14.1

[PATCH 33/34] wt-status: release strbuf after use in read_rebase_todolist()

[Rene Scharfe]

Signed-off-by: Rene Scharfe 
---
 wt-status.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/wt-status.c b/wt-status.c
index 77c27c5113..cafafb5ecd 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1175,24 +1175,25 @@ static void abbrev_sha1_in_line(struct strbuf *line)
 static int read_rebase_todolist(const char *fname, struct string_list *lines)
 {
struct strbuf line = STRBUF_INIT;
FILE *f = fopen(git_path("%s", fname), "r");
 
if (!f) {
if (errno == ENOENT)
return -1;
die_errno("Could not open file %s for reading",
  git_path("%s", fname));
}
while (!strbuf_getline_lf(, f)) {
if (line.len && line.buf[0] == comment_line_char)
continue;
strbuf_trim();
if (!line.len)
continue;
abbrev_sha1_in_line();
string_list_append(lines, line.buf);
}
fclose(f);
+   strbuf_release();
return 0;
 }
 
-- 
2.14.1

[PATCH 31/34] utf8: release strbuf on error return in strbuf_utf8_replace()

[Rene Scharfe]

Signed-off-by: Rene Scharfe 
---
 utf8.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/utf8.c b/utf8.c
index 0c8e011a58..47a42047c8 100644
--- a/utf8.c
+++ b/utf8.c
@@ -354,49 +354,50 @@ void strbuf_add_wrapped_bytes(struct strbuf *buf, const 
char *data, int len,
 void strbuf_utf8_replace(struct strbuf *sb_src, int pos, int width,
 const char *subst)
 {
struct strbuf sb_dst = STRBUF_INIT;
char *src = sb_src->buf;
char *end = src + sb_src->len;
char *dst;
int w = 0, subst_len = 0;
 
if (subst)
subst_len = strlen(subst);
strbuf_grow(_dst, sb_src->len + subst_len);
dst = sb_dst.buf;
 
while (src < end) {
char *old;
size_t n;
 
while ((n = display_mode_esc_sequence_len(src))) {
memcpy(dst, src, n);
src += n;
dst += n;
}
 
if (src >= end)
break;
 
old = src;
n = utf8_width((const char**), NULL);
if (!src)   /* broken utf-8, do nothing */
-   return;
+   goto out;
if (n && w >= pos && w < pos + width) {
if (subst) {
memcpy(dst, subst, subst_len);
dst += subst_len;
subst = NULL;
}
w += n;
continue;
}
memcpy(dst, old, src - old);
dst += src - old;
w += n;
}
strbuf_setlen(_dst, dst - sb_dst.buf);
strbuf_swap(sb_src, _dst);
+out:
strbuf_release(_dst);
 }
 
-- 
2.14.1

[PATCH 32/34] vcs-svn: release strbuf after use in end_revision()

[Rene Scharfe]

Signed-off-by: Rene Scharfe 
---
 vcs-svn/svndump.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c
index ec6b350611..08d136b8cc 100644
--- a/vcs-svn/svndump.c
+++ b/vcs-svn/svndump.c
@@ -311,13 +311,14 @@ static void begin_revision(const char *remote_ref)
 static void end_revision(const char *note_ref)
 {
struct strbuf mark = STRBUF_INIT;
if (rev_ctx.revision) {
fast_export_end_commit(rev_ctx.revision);
fast_export_begin_note(rev_ctx.revision, "remote-svn",
"Note created by remote-svn.", 
rev_ctx.timestamp, note_ref);
strbuf_addf(, ":%"PRIu32, rev_ctx.revision);
fast_export_note(mark.buf, "inline");
fast_export_buf_to_data(_ctx.note);
+   strbuf_release();
}
 }
 
-- 
2.14.1

[PATCH 34/34] wt-status: release strbuf after use in wt_longstatus_print_tracking()

[Rene Scharfe]

If format_tracking_info() returns 0 only if it didn't touch its strbuf
parameter, so it's OK to exit early in that case.  Clean up sb in the
other case.

Signed-off-by: Rene Scharfe 
---
 wt-status.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/wt-status.c b/wt-status.c
index cafafb5ecd..ac972acbab 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -998,34 +998,35 @@ static void wt_longstatus_print_verbose(struct wt_status 
*s)
 static void wt_longstatus_print_tracking(struct wt_status *s)
 {
struct strbuf sb = STRBUF_INIT;
const char *cp, *ep, *branch_name;
struct branch *branch;
char comment_line_string[3];
int i;
 
assert(s->branch && !s->is_initial);
if (!skip_prefix(s->branch, "refs/heads/", _name))
return;
branch = branch_get(branch_name);
if (!format_tracking_info(branch, ))
return;
 
i = 0;
if (s->display_comment_prefix) {
comment_line_string[i++] = comment_line_char;
comment_line_string[i++] = ' ';
}
comment_line_string[i] = '\0';
 
for (cp = sb.buf; (ep = strchr(cp, '\n')) != NULL; cp = ep + 1)
color_fprintf_ln(s->fp, color(WT_STATUS_HEADER, s),
 "%s%.*s", comment_line_string,
 (int)(ep - cp), cp);
if (s->display_comment_prefix)
color_fprintf_ln(s->fp, color(WT_STATUS_HEADER, s), "%c",
 comment_line_char);
else
fputs("\n", s->fp);
+   strbuf_release();
 }
 
 static int has_unmerged(struct wt_status *s)
-- 
2.14.1

[PATCH 28/34] sequencer: release strbuf after use in save_head()

[Rene Scharfe]

Signed-off-by: Rene Scharfe 
---
 sequencer.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index fcceabb80f..60636ce54b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1563,23 +1563,26 @@ static int create_seq_dir(void)
 static int save_head(const char *head)
 {
static struct lock_file head_lock;
struct strbuf buf = STRBUF_INIT;
int fd;
+   ssize_t written;
 
fd = hold_lock_file_for_update(_lock, git_path_head_file(), 0);
if (fd < 0) {
rollback_lock_file(_lock);
return error_errno(_("could not lock HEAD"));
}
strbuf_addf(, "%s\n", head);
-   if (write_in_full(fd, buf.buf, buf.len) < 0) {
+   written = write_in_full(fd, buf.buf, buf.len);
+   strbuf_release();
+   if (written < 0) {
rollback_lock_file(_lock);
return error_errno(_("could not write to '%s'"),
   git_path_head_file());
}
if (commit_lock_file(_lock) < 0) {
rollback_lock_file(_lock);
return error(_("failed to finalize '%s'."), 
git_path_head_file());
}
return 0;
 }
-- 
2.14.1

Re: Commit dropped when swapping commits with rebase -i -p

[Martin Ågren]

On 30 August 2017 at 12:11, Sebastian Schuberth  wrote:
> Hi,
>
> I believe stumbled upon a nasty bug in Git: It looks like a commits gets 
> dropped during interactive rebase when asking to preserve merges. Steps:
>
> $ git clone -b git-bug --single-branch 
> https://github.com/heremaps/scancode-toolkit.git
> $ git rebase -i -p HEAD~2
> # In the editor, swap the order of the two (non-merge) commits.
> $ git diff origin/git-bug
>
> The last command will show a non-empty diff which looks as if the "Do not 
> shadow the os package with a variable name" commit has been dropped, and 
> indeed "git log" confirms this. The commit should not get dropped, and it 
> does not if just using "git rebase -i -p HEAD~2" (without "-p").
>
> I'm observing this with Git 2.14.1 on Linux.

The man-page for git rebase says that combining -p with -i is "generally
not a good idea unless you know what you are doing (see BUGS below)".

Under BUGS, it says

"The todo list presented by --preserve-merges --interactive does not
represent the topology of the revision graph. Editing commits and
rewording their commit messages should work fine, but attempts to
reorder commits tend to produce counterintuitive results."

So if you agree that a "dropped commit" is a "counterintuitive result",
this is known and documented. Maybe the warning could be harsher, but it
does say "unless you know what you are doing".

Martin

[PATCH 08/34] connect: release strbuf on error return in git_connect()

[Rene Scharfe]

Reduce the scope of the variable cmd and release it before returning
early.

Signed-off-by: Rene Scharfe 
---
 connect.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/connect.c b/connect.c
index 49b28b83be..df56c0cbff 100644
--- a/connect.c
+++ b/connect.c
@@ -775,146 +775,148 @@ static void handle_ssh_variant(const char *ssh_command, 
int is_cmdline,
 struct child_process *git_connect(int fd[2], const char *url,
  const char *prog, int flags)
 {
char *hostandport, *path;
struct child_process *conn = _fork;
enum protocol protocol;
-   struct strbuf cmd = STRBUF_INIT;
 
/* Without this we cannot rely on waitpid() to tell
 * what happened to our children.
 */
signal(SIGCHLD, SIG_DFL);
 
protocol = parse_connect_url(url, , );
if ((flags & CONNECT_DIAG_URL) && (protocol != PROTO_SSH)) {
printf("Diag: url=%s\n", url ? url : "NULL");
printf("Diag: protocol=%s\n", prot_name(protocol));
printf("Diag: hostandport=%s\n", hostandport ? hostandport : 
"NULL");
printf("Diag: path=%s\n", path ? path : "NULL");
conn = NULL;
} else if (protocol == PROTO_GIT) {
/*
 * Set up virtual host information based on where we will
 * connect, unless the user has overridden us in
 * the environment.
 */
char *target_host = getenv("GIT_OVERRIDE_VIRTUAL_HOST");
if (target_host)
target_host = xstrdup(target_host);
else
target_host = xstrdup(hostandport);
 
transport_check_allowed("git");
 
/* These underlying connection commands die() if they
 * cannot connect.
 */
if (git_use_proxy(hostandport))
conn = git_proxy_connect(fd, hostandport);
else
git_tcp_connect(fd, hostandport, flags);
/*
 * Separate original protocol components prog and path
 * from extended host header with a NUL byte.
 *
 * Note: Do not add any other headers here!  Doing so
 * will cause older git-daemon servers to crash.
 */
packet_write_fmt(fd[1],
 "%s %s%chost=%s%c",
 prog, path, 0,
 target_host, 0);
free(target_host);
} else {
+   struct strbuf cmd = STRBUF_INIT;
+
conn = xmalloc(sizeof(*conn));
child_process_init(conn);
 
if (looks_like_command_line_option(path))
die("strange pathname '%s' blocked", path);
 
strbuf_addstr(, prog);
strbuf_addch(, ' ');
sq_quote_buf(, path);
 
/* remove repo-local variables from the environment */
conn->env = local_repo_env;
conn->use_shell = 1;
conn->in = conn->out = -1;
if (protocol == PROTO_SSH) {
const char *ssh;
int needs_batch = 0;
int port_option = 'p';
char *ssh_host = hostandport;
const char *port = NULL;
transport_check_allowed("ssh");
get_host_and_port(_host, );
 
if (!port)
port = get_port(ssh_host);
 
if (flags & CONNECT_DIAG_URL) {
printf("Diag: url=%s\n", url ? url : "NULL");
printf("Diag: protocol=%s\n", 
prot_name(protocol));
printf("Diag: userandhost=%s\n", ssh_host ? 
ssh_host : "NULL");
printf("Diag: port=%s\n", port ? port : "NONE");
printf("Diag: path=%s\n", path ? path : "NULL");
 
free(hostandport);
free(path);
free(conn);
+   strbuf_release();
return NULL;
}
 
if (looks_like_command_line_option(ssh_host))
die("strange hostname '%s' blocked", ssh_host);
 
ssh = get_ssh_command();
if (ssh)
handle_ssh_variant(ssh, 1, _option,
   _batch);
else {
/*
 * GIT_SSH is the no-shell version 

Re: [PATCH v3 1/3] refs/files-backend: add longer-scoped copy of string to list

[Martin Ågren]

On 30 August 2017 at 04:52, Michael Haggerty  wrote:
> v3 looks good to me. Thanks!
>
> Reviewed-by: Michael Haggerty 

Thank _you_ for very helpful feedback on the earlier versions.

Martin

[PATCH 27/34] shortlog: release strbuf after use in insert_one_record()

[Rene Scharfe]

Signed-off-by: Rene Scharfe 
---
 builtin/shortlog.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index 43c4799ea9..48af16c681 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -50,66 +50,67 @@ static int compare_by_list(const void *a1, const void *a2)
 static void insert_one_record(struct shortlog *log,
  const char *author,
  const char *oneline)
 {
struct string_list_item *item;
const char *mailbuf, *namebuf;
size_t namelen, maillen;
struct strbuf namemailbuf = STRBUF_INIT;
struct ident_split ident;
 
if (split_ident_line(, author, strlen(author)))
return;
 
namebuf = ident.name_begin;
mailbuf = ident.mail_begin;
namelen = ident.name_end - ident.name_begin;
maillen = ident.mail_end - ident.mail_begin;
 
map_user(>mailmap, , , , );
strbuf_add(, namebuf, namelen);
 
if (log->email)
strbuf_addf(, " <%.*s>", (int)maillen, mailbuf);
 
item = string_list_insert(>list, namemailbuf.buf);
+   strbuf_release();
 
if (log->summary)
item->util = (void *)(UTIL_TO_INT(item) + 1);
else {
const char *dot3 = log->common_repo_prefix;
char *buffer, *p;
struct strbuf subject = STRBUF_INIT;
const char *eol;
 
/* Skip any leading whitespace, including any blank lines. */
while (*oneline && isspace(*oneline))
oneline++;
eol = strchr(oneline, '\n');
if (!eol)
eol = oneline + strlen(oneline);
if (starts_with(oneline, "[PATCH")) {
char *eob = strchr(oneline, ']');
if (eob && (!eol || eob < eol))
oneline = eob + 1;
}
while (*oneline && isspace(*oneline) && *oneline != '\n')
oneline++;
format_subject(, oneline, " ");
buffer = strbuf_detach(, NULL);
 
if (dot3) {
int dot3len = strlen(dot3);
if (dot3len > 5) {
while ((p = strstr(buffer, dot3)) != NULL) {
int taillen = strlen(p) - dot3len;
memcpy(p, "/.../", 5);
memmove(p + 5, p + dot3len, taillen + 
1);
}
}
}
 
if (item->util == NULL)
item->util = xcalloc(1, sizeof(struct string_list));
string_list_append(item->util, buffer);
}
 }
-- 
2.14.1

[PATCH 26/34] sha1_file: release strbuf on error return in index_path()

[Rene Scharfe]

strbuf_readlink() already frees the buffer for us on error.  Clean up
if write_sha1_file() fails as well instead of returning early.

Signed-off-by: Rene Scharfe 
---
 sha1_file.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index f56bb5cae7..7d9c9aed2f 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1819,33 +1819,33 @@ int index_fd(struct object_id *oid, int fd, struct stat 
*st,
 int index_path(struct object_id *oid, const char *path, struct stat *st, 
unsigned flags)
 {
int fd;
struct strbuf sb = STRBUF_INIT;
+   int rc = 0;
 
switch (st->st_mode & S_IFMT) {
case S_IFREG:
fd = open(path, O_RDONLY);
if (fd < 0)
return error_errno("open(\"%s\")", path);
if (index_fd(oid, fd, st, OBJ_BLOB, path, flags) < 0)
return error("%s: failed to insert into database",
 path);
break;
case S_IFLNK:
if (strbuf_readlink(, path, st->st_size))
return error_errno("readlink(\"%s\")", path);
if (!(flags & HASH_WRITE_OBJECT))
hash_sha1_file(sb.buf, sb.len, blob_type, oid->hash);
else if (write_sha1_file(sb.buf, sb.len, blob_type, oid->hash))
-   return error("%s: failed to insert into database",
-path);
+   rc = error("%s: failed to insert into database", path);
strbuf_release();
break;
case S_IFDIR:
return resolve_gitlink_ref(path, "HEAD", oid->hash);
default:
return error("%s: unsupported file type", path);
}
-   return 0;
+   return rc;
 }
 
 int read_pack_header(int fd, struct pack_header *header)
-- 
2.14.1

[PATCH 24/34] remote: release strbuf after use in set_url()

[Rene Scharfe]

Signed-off-by: Rene Scharfe 
---
 builtin/remote.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index 0a56d7da66..33ba739332 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -1509,87 +1509,87 @@ static int get_url(int argc, const char **argv)
 static int set_url(int argc, const char **argv)
 {
int i, push_mode = 0, add_mode = 0, delete_mode = 0;
int matches = 0, negative_matches = 0;
const char *remotename = NULL;
const char *newurl = NULL;
const char *oldurl = NULL;
struct remote *remote;
regex_t old_regex;
const char **urlset;
int urlset_nr;
struct strbuf name_buf = STRBUF_INIT;
struct option options[] = {
OPT_BOOL('\0', "push", _mode,
 N_("manipulate push URLs")),
OPT_BOOL('\0', "add", _mode,
 N_("add URL")),
OPT_BOOL('\0', "delete", _mode,
N_("delete URLs")),
OPT_END()
};
argc = parse_options(argc, argv, NULL, options, 
builtin_remote_seturl_usage,
 PARSE_OPT_KEEP_ARGV0);
 
if (add_mode && delete_mode)
die(_("--add --delete doesn't make sense"));
 
if (argc < 3 || argc > 4 || ((add_mode || delete_mode) && argc != 3))
usage_with_options(builtin_remote_seturl_usage, options);
 
remotename = argv[1];
newurl = argv[2];
if (argc > 3)
oldurl = argv[3];
 
if (delete_mode)
oldurl = newurl;
 
remote = remote_get(remotename);
if (!remote_is_configured(remote, 1))
die(_("No such remote '%s'"), remotename);
 
if (push_mode) {
strbuf_addf(_buf, "remote.%s.pushurl", remotename);
urlset = remote->pushurl;
urlset_nr = remote->pushurl_nr;
} else {
strbuf_addf(_buf, "remote.%s.url", remotename);
urlset = remote->url;
urlset_nr = remote->url_nr;
}
 
/* Special cases that add new entry. */
if ((!oldurl && !delete_mode) || add_mode) {
if (add_mode)
git_config_set_multivar(name_buf.buf, newurl,
   "^$", 0);
else
git_config_set(name_buf.buf, newurl);
-   strbuf_release(_buf);
-
-   return 0;
+   goto out;
}
 
/* Old URL specified. Demand that one matches. */
if (regcomp(_regex, oldurl, REG_EXTENDED))
die(_("Invalid old URL pattern: %s"), oldurl);
 
for (i = 0; i < urlset_nr; i++)
if (!regexec(_regex, urlset[i], 0, NULL, 0))
matches++;
else
negative_matches++;
if (!delete_mode && !matches)
die(_("No such URL found: %s"), oldurl);
if (delete_mode && !negative_matches && !push_mode)
die(_("Will not delete all non-push URLs"));
 
regfree(_regex);
 
if (!delete_mode)
git_config_set_multivar(name_buf.buf, newurl, oldurl, 0);
else
git_config_set_multivar(name_buf.buf, NULL, oldurl, 1);
+out:
+   strbuf_release(_buf);
return 0;
 }
 
-- 
2.14.1

[PATCH 23/34] remote: release strbuf after use in migrate_file()

[Rene Scharfe]

Signed-off-by: Rene Scharfe 
---
 builtin/remote.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/remote.c b/builtin/remote.c
index d0bf999abf..0a56d7da66 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -579,23 +579,24 @@ static int read_remote_branches(const char *refname,
 static int migrate_file(struct remote *remote)
 {
struct strbuf buf = STRBUF_INIT;
int i;
 
strbuf_addf(, "remote.%s.url", remote->name);
for (i = 0; i < remote->url_nr; i++)
git_config_set_multivar(buf.buf, remote->url[i], "^$", 0);
strbuf_reset();
strbuf_addf(, "remote.%s.push", remote->name);
for (i = 0; i < remote->push_refspec_nr; i++)
git_config_set_multivar(buf.buf, remote->push_refspec[i], "^$", 
0);
strbuf_reset();
strbuf_addf(, "remote.%s.fetch", remote->name);
for (i = 0; i < remote->fetch_refspec_nr; i++)
git_config_set_multivar(buf.buf, remote->fetch_refspec[i], 
"^$", 0);
if (remote->origin == REMOTE_REMOTES)
unlink_or_warn(git_path("remotes/%s", remote->name));
else if (remote->origin == REMOTE_BRANCHES)
unlink_or_warn(git_path("branches/%s", remote->name));
+   strbuf_release();
 
return 0;
 }
-- 
2.14.1

[PATCH 25/34] send-pack: release strbuf on error return in send_pack()

[Rene Scharfe]

Signed-off-by: Rene Scharfe 
---
 send-pack.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/send-pack.c b/send-pack.c
index 11d6f3d983..b865f662e4 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -377,253 +377,256 @@ static void reject_invalid_nonce(const char *nonce, int 
len)
 int send_pack(struct send_pack_args *args,
  int fd[], struct child_process *conn,
  struct ref *remote_refs,
  struct oid_array *extra_have)
 {
int in = fd[0];
int out = fd[1];
struct strbuf req_buf = STRBUF_INIT;
struct strbuf cap_buf = STRBUF_INIT;
struct ref *ref;
int need_pack_data = 0;
int allow_deleting_refs = 0;
int status_report = 0;
int use_sideband = 0;
int quiet_supported = 0;
int agent_supported = 0;
int use_atomic = 0;
int atomic_supported = 0;
int use_push_options = 0;
int push_options_supported = 0;
unsigned cmds_sent = 0;
int ret;
struct async demux;
const char *push_cert_nonce = NULL;
 
/* Does the other end support the reporting? */
if (server_supports("report-status"))
status_report = 1;
if (server_supports("delete-refs"))
allow_deleting_refs = 1;
if (server_supports("ofs-delta"))
args->use_ofs_delta = 1;
if (server_supports("side-band-64k"))
use_sideband = 1;
if (server_supports("quiet"))
quiet_supported = 1;
if (server_supports("agent"))
agent_supported = 1;
if (server_supports("no-thin"))
args->use_thin_pack = 0;
if (server_supports("atomic"))
atomic_supported = 1;
if (server_supports("push-options"))
push_options_supported = 1;
 
if (args->push_cert != SEND_PACK_PUSH_CERT_NEVER) {
int len;
push_cert_nonce = server_feature_value("push-cert", );
if (push_cert_nonce) {
reject_invalid_nonce(push_cert_nonce, len);
push_cert_nonce = xmemdupz(push_cert_nonce, len);
} else if (args->push_cert == SEND_PACK_PUSH_CERT_ALWAYS) {
die(_("the receiving end does not support --signed 
push"));
} else if (args->push_cert == SEND_PACK_PUSH_CERT_IF_ASKED) {
warning(_("not sending a push certificate since the"
  " receiving end does not support --signed"
  " push"));
}
}
 
if (!remote_refs) {
fprintf(stderr, "No refs in common and none specified; doing 
nothing.\n"
"Perhaps you should specify a branch such as 
'master'.\n");
return 0;
}
if (args->atomic && !atomic_supported)
die(_("the receiving end does not support --atomic push"));
 
use_atomic = atomic_supported && args->atomic;
 
if (args->push_options && !push_options_supported)
die(_("the receiving end does not support push options"));
 
use_push_options = push_options_supported && args->push_options;
 
if (status_report)
strbuf_addstr(_buf, " report-status");
if (use_sideband)
strbuf_addstr(_buf, " side-band-64k");
if (quiet_supported && (args->quiet || !args->progress))
strbuf_addstr(_buf, " quiet");
if (use_atomic)
strbuf_addstr(_buf, " atomic");
if (use_push_options)
strbuf_addstr(_buf, " push-options");
if (agent_supported)
strbuf_addf(_buf, " agent=%s", git_user_agent_sanitized());
 
/*
 * NEEDSWORK: why does delete-refs have to be so specific to
 * send-pack machinery that set_ref_status_for_push() cannot
 * set this bit for us???
 */
for (ref = remote_refs; ref; ref = ref->next)
if (ref->deletion && !allow_deleting_refs)
ref->status = REF_STATUS_REJECT_NODELETE;
 
if (!args->dry_run)
advertise_shallow_grafts_buf(_buf);
 
if (!args->dry_run && push_cert_nonce)
cmds_sent = generate_push_cert(_buf, remote_refs, args,
   cap_buf.buf, push_cert_nonce);
 
/*
 * Clear the status for each ref and see if we need to send
 * the pack data.
 */
for (ref = remote_refs; ref; ref = ref->next) {
switch (check_to_send_update(ref, args)) {
case 0: /* no error */
break;
case CHECK_REF_STATUS_REJECTED:
/*
 * When we know the server would reject a ref update if
 

[PATCH 22/34] remote: release strbuf after use in read_remote_branches()

[Rene Scharfe]

Signed-off-by: Rene Scharfe 
---
 builtin/remote.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/remote.c b/builtin/remote.c
index a995ea86c1..d0bf999abf 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -554,23 +554,24 @@ struct rename_info {
 static int read_remote_branches(const char *refname,
const struct object_id *oid, int flags, void *cb_data)
 {
struct rename_info *rename = cb_data;
struct strbuf buf = STRBUF_INIT;
struct string_list_item *item;
int flag;
struct object_id orig_oid;
const char *symref;
 
strbuf_addf(, "refs/remotes/%s/", rename->old);
if (starts_with(refname, buf.buf)) {
item = string_list_append(rename->remote_branches, 
xstrdup(refname));
symref = resolve_ref_unsafe(refname, RESOLVE_REF_READING,
orig_oid.hash, );
if (flag & REF_ISSYMREF)
item->util = xstrdup(symref);
else
item->util = NULL;
}
+   strbuf_release();
 
return 0;
 }
-- 
2.14.1

[PATCH 08/34] connect: release strbuf on error return in git_connect()

[Rene Scharfe]

Reduce the scope of the variable cmd and release it before returning
early.

Signed-off-by: Rene Scharfe 
---
 connect.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/connect.c b/connect.c
index 49b28b83be..df56c0cbff 100644
--- a/connect.c
+++ b/connect.c
@@ -775,146 +775,148 @@ static void handle_ssh_variant(const char *ssh_command, 
int is_cmdline,
 struct child_process *git_connect(int fd[2], const char *url,
  const char *prog, int flags)
 {
char *hostandport, *path;
struct child_process *conn = _fork;
enum protocol protocol;
-   struct strbuf cmd = STRBUF_INIT;
 
/* Without this we cannot rely on waitpid() to tell
 * what happened to our children.
 */
signal(SIGCHLD, SIG_DFL);
 
protocol = parse_connect_url(url, , );
if ((flags & CONNECT_DIAG_URL) && (protocol != PROTO_SSH)) {
printf("Diag: url=%s\n", url ? url : "NULL");
printf("Diag: protocol=%s\n", prot_name(protocol));
printf("Diag: hostandport=%s\n", hostandport ? hostandport : 
"NULL");
printf("Diag: path=%s\n", path ? path : "NULL");
conn = NULL;
} else if (protocol == PROTO_GIT) {
/*
 * Set up virtual host information based on where we will
 * connect, unless the user has overridden us in
 * the environment.
 */
char *target_host = getenv("GIT_OVERRIDE_VIRTUAL_HOST");
if (target_host)
target_host = xstrdup(target_host);
else
target_host = xstrdup(hostandport);
 
transport_check_allowed("git");
 
/* These underlying connection commands die() if they
 * cannot connect.
 */
if (git_use_proxy(hostandport))
conn = git_proxy_connect(fd, hostandport);
else
git_tcp_connect(fd, hostandport, flags);
/*
 * Separate original protocol components prog and path
 * from extended host header with a NUL byte.
 *
 * Note: Do not add any other headers here!  Doing so
 * will cause older git-daemon servers to crash.
 */
packet_write_fmt(fd[1],
 "%s %s%chost=%s%c",
 prog, path, 0,
 target_host, 0);
free(target_host);
} else {
+   struct strbuf cmd = STRBUF_INIT;
+
conn = xmalloc(sizeof(*conn));
child_process_init(conn);
 
if (looks_like_command_line_option(path))
die("strange pathname '%s' blocked", path);
 
strbuf_addstr(, prog);
strbuf_addch(, ' ');
sq_quote_buf(, path);
 
/* remove repo-local variables from the environment */
conn->env = local_repo_env;
conn->use_shell = 1;
conn->in = conn->out = -1;
if (protocol == PROTO_SSH) {
const char *ssh;
int needs_batch = 0;
int port_option = 'p';
char *ssh_host = hostandport;
const char *port = NULL;
transport_check_allowed("ssh");
get_host_and_port(_host, );
 
if (!port)
port = get_port(ssh_host);
 
if (flags & CONNECT_DIAG_URL) {
printf("Diag: url=%s\n", url ? url : "NULL");
printf("Diag: protocol=%s\n", 
prot_name(protocol));
printf("Diag: userandhost=%s\n", ssh_host ? 
ssh_host : "NULL");
printf("Diag: port=%s\n", port ? port : "NONE");
printf("Diag: path=%s\n", path ? path : "NULL");
 
free(hostandport);
free(path);
free(conn);
+   strbuf_release();
return NULL;
}
 
if (looks_like_command_line_option(ssh_host))
die("strange hostname '%s' blocked", ssh_host);
 
ssh = get_ssh_command();
if (ssh)
handle_ssh_variant(ssh, 1, _option,
   _batch);
else {
/*
 * GIT_SSH is the no-shell version 

[PATCH 21/34] refs: release strbuf on error return in write_pseudoref()

[Rene Scharfe]

Signed-off-by: Rene Scharfe 
---
 refs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index b0106b8162..d8dc86b1f5 100644
--- a/refs.c
+++ b/refs.c
@@ -597,45 +597,45 @@ long get_files_ref_lock_timeout_ms(void)
 static int write_pseudoref(const char *pseudoref, const unsigned char *sha1,
   const unsigned char *old_sha1, struct strbuf *err)
 {
const char *filename;
int fd;
static struct lock_file lock;
struct strbuf buf = STRBUF_INIT;
int ret = -1;
 
strbuf_addf(, "%s\n", sha1_to_hex(sha1));
 
filename = git_path("%s", pseudoref);
fd = hold_lock_file_for_update_timeout(, filename,
   LOCK_DIE_ON_ERROR,
   get_files_ref_lock_timeout_ms());
if (fd < 0) {
strbuf_addf(err, "could not open '%s' for writing: %s",
filename, strerror(errno));
-   return -1;
+   goto done;
}
 
if (old_sha1) {
unsigned char actual_old_sha1[20];
 
if (read_ref(pseudoref, actual_old_sha1))
die("could not read ref '%s'", pseudoref);
if (hashcmp(actual_old_sha1, old_sha1)) {
strbuf_addf(err, "unexpected sha1 when writing '%s'", 
pseudoref);
rollback_lock_file();
goto done;
}
}
 
if (write_in_full(fd, buf.buf, buf.len) != buf.len) {
strbuf_addf(err, "could not write to '%s'", filename);
rollback_lock_file();
goto done;
}
 
commit_lock_file();
ret = 0;
 done:
strbuf_release();
return ret;
 }
-- 
2.14.1

[PATCH 08/34] connect: release strbuf on error return in git_connect()

[Rene Scharfe]

Reduce the scope of the variable cmd and release it before returning
early.

Signed-off-by: Rene Scharfe 
---
 connect.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/connect.c b/connect.c
index 49b28b83be..df56c0cbff 100644
--- a/connect.c
+++ b/connect.c
@@ -775,146 +775,148 @@ static void handle_ssh_variant(const char *ssh_command, 
int is_cmdline,
 struct child_process *git_connect(int fd[2], const char *url,
  const char *prog, int flags)
 {
char *hostandport, *path;
struct child_process *conn = _fork;
enum protocol protocol;
-   struct strbuf cmd = STRBUF_INIT;
 
/* Without this we cannot rely on waitpid() to tell
 * what happened to our children.
 */
signal(SIGCHLD, SIG_DFL);
 
protocol = parse_connect_url(url, , );
if ((flags & CONNECT_DIAG_URL) && (protocol != PROTO_SSH)) {
printf("Diag: url=%s\n", url ? url : "NULL");
printf("Diag: protocol=%s\n", prot_name(protocol));
printf("Diag: hostandport=%s\n", hostandport ? hostandport : 
"NULL");
printf("Diag: path=%s\n", path ? path : "NULL");
conn = NULL;
} else if (protocol == PROTO_GIT) {
/*
 * Set up virtual host information based on where we will
 * connect, unless the user has overridden us in
 * the environment.
 */
char *target_host = getenv("GIT_OVERRIDE_VIRTUAL_HOST");
if (target_host)
target_host = xstrdup(target_host);
else
target_host = xstrdup(hostandport);
 
transport_check_allowed("git");
 
/* These underlying connection commands die() if they
 * cannot connect.
 */
if (git_use_proxy(hostandport))
conn = git_proxy_connect(fd, hostandport);
else
git_tcp_connect(fd, hostandport, flags);
/*
 * Separate original protocol components prog and path
 * from extended host header with a NUL byte.
 *
 * Note: Do not add any other headers here!  Doing so
 * will cause older git-daemon servers to crash.
 */
packet_write_fmt(fd[1],
 "%s %s%chost=%s%c",
 prog, path, 0,
 target_host, 0);
free(target_host);
} else {
+   struct strbuf cmd = STRBUF_INIT;
+
conn = xmalloc(sizeof(*conn));
child_process_init(conn);
 
if (looks_like_command_line_option(path))
die("strange pathname '%s' blocked", path);
 
strbuf_addstr(, prog);
strbuf_addch(, ' ');
sq_quote_buf(, path);
 
/* remove repo-local variables from the environment */
conn->env = local_repo_env;
conn->use_shell = 1;
conn->in = conn->out = -1;
if (protocol == PROTO_SSH) {
const char *ssh;
int needs_batch = 0;
int port_option = 'p';
char *ssh_host = hostandport;
const char *port = NULL;
transport_check_allowed("ssh");
get_host_and_port(_host, );
 
if (!port)
port = get_port(ssh_host);
 
if (flags & CONNECT_DIAG_URL) {
printf("Diag: url=%s\n", url ? url : "NULL");
printf("Diag: protocol=%s\n", 
prot_name(protocol));
printf("Diag: userandhost=%s\n", ssh_host ? 
ssh_host : "NULL");
printf("Diag: port=%s\n", port ? port : "NONE");
printf("Diag: path=%s\n", path ? path : "NULL");
 
free(hostandport);
free(path);
free(conn);
+   strbuf_release();
return NULL;
}
 
if (looks_like_command_line_option(ssh_host))
die("strange hostname '%s' blocked", ssh_host);
 
ssh = get_ssh_command();
if (ssh)
handle_ssh_variant(ssh, 1, _option,
   _batch);
else {
/*
 * GIT_SSH is the no-shell version 

[PATCH 21/34] refs: release strbuf on error return in write_pseudoref()

[Rene Scharfe]

Signed-off-by: Rene Scharfe 
---
 refs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index b0106b8162..d8dc86b1f5 100644
--- a/refs.c
+++ b/refs.c
@@ -597,45 +597,45 @@ long get_files_ref_lock_timeout_ms(void)
 static int write_pseudoref(const char *pseudoref, const unsigned char *sha1,
   const unsigned char *old_sha1, struct strbuf *err)
 {
const char *filename;
int fd;
static struct lock_file lock;
struct strbuf buf = STRBUF_INIT;
int ret = -1;
 
strbuf_addf(, "%s\n", sha1_to_hex(sha1));
 
filename = git_path("%s", pseudoref);
fd = hold_lock_file_for_update_timeout(, filename,
   LOCK_DIE_ON_ERROR,
   get_files_ref_lock_timeout_ms());
if (fd < 0) {
strbuf_addf(err, "could not open '%s' for writing: %s",
filename, strerror(errno));
-   return -1;
+   goto done;
}
 
if (old_sha1) {
unsigned char actual_old_sha1[20];
 
if (read_ref(pseudoref, actual_old_sha1))
die("could not read ref '%s'", pseudoref);
if (hashcmp(actual_old_sha1, old_sha1)) {
strbuf_addf(err, "unexpected sha1 when writing '%s'", 
pseudoref);
rollback_lock_file();
goto done;
}
}
 
if (write_in_full(fd, buf.buf, buf.len) != buf.len) {
strbuf_addf(err, "could not write to '%s'", filename);
rollback_lock_file();
goto done;
}
 
commit_lock_file();
ret = 0;
 done:
strbuf_release();
return ret;
 }
-- 
2.14.1

[PATCH 11/34] diff: release strbuf after use in show_rename_copy()

[Rene Scharfe]

Signed-off-by: Rene Scharfe 
---
 diff.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/diff.c b/diff.c
index 4148ba6980..33c65f492d 100644
--- a/diff.c
+++ b/diff.c
@@ -5281,14 +5281,15 @@ static void show_mode_change(struct diff_options *opt, 
struct diff_filepair *p,
 static void show_rename_copy(struct diff_options *opt, const char *renamecopy,
struct diff_filepair *p)
 {
struct strbuf sb = STRBUF_INIT;
char *names = pprint_rename(p->one->path, p->two->path);
strbuf_addf(, " %s %s (%d%%)\n",
renamecopy, names, similarity_index(p));
free(names);
emit_diff_symbol(opt, DIFF_SYMBOL_SUMMARY,
 sb.buf, sb.len, 0);
show_mode_change(opt, p, 0);
+   strbuf_release();
 }
 
 static void diff_summary(struct diff_options *opt, struct diff_filepair *p)
-- 
2.14.1

[PATCH 04/34] check-ref-format: release strbuf after use in check_ref_format_branch()

[Rene Scharfe]

Signed-off-by: Rene Scharfe 
---
 builtin/check-ref-format.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c
index eac499450f..6c40ff110b 100644
--- a/builtin/check-ref-format.c
+++ b/builtin/check-ref-format.c
@@ -39,12 +39,13 @@ static char *collapse_slashes(const char *refname)
 static int check_ref_format_branch(const char *arg)
 {
struct strbuf sb = STRBUF_INIT;
int nongit;
 
setup_git_directory_gently();
if (strbuf_check_branch_ref(, arg))
die("'%s' is not a valid branch name", arg);
printf("%s\n", sb.buf + 11);
+   strbuf_release();
return 0;
 }
 
-- 
2.14.1

[PATCH 09/34] convert: release strbuf on error return in filter_buffer_or_fd()

[Rene Scharfe]

Signed-off-by: Rene Scharfe 
---
 convert.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/convert.c b/convert.c
index c5f0b21037..4a0ed8d3cb 100644
--- a/convert.c
+++ b/convert.c
@@ -393,63 +393,65 @@ struct filter_params {
 static int filter_buffer_or_fd(int in, int out, void *data)
 {
/*
 * Spawn cmd and feed the buffer contents through its stdin.
 */
struct child_process child_process = CHILD_PROCESS_INIT;
struct filter_params *params = (struct filter_params *)data;
int write_err, status;
const char *argv[] = { NULL, NULL };
 
/* apply % substitution to cmd */
struct strbuf cmd = STRBUF_INIT;
struct strbuf path = STRBUF_INIT;
struct strbuf_expand_dict_entry dict[] = {
{ "f", NULL, },
{ NULL, NULL, },
};
 
/* quote the path to preserve spaces, etc. */
sq_quote_buf(, params->path);
dict[0].value = path.buf;
 
/* expand all %f with the quoted path */
strbuf_expand(, params->cmd, strbuf_expand_dict_cb, );
strbuf_release();
 
argv[0] = cmd.buf;
 
child_process.argv = argv;
child_process.use_shell = 1;
child_process.in = -1;
child_process.out = out;
 
-   if (start_command(_process))
+   if (start_command(_process)) {
+   strbuf_release();
return error("cannot fork to run external filter '%s'", 
params->cmd);
+   }
 
sigchain_push(SIGPIPE, SIG_IGN);
 
if (params->src) {
write_err = (write_in_full(child_process.in,
   params->src, params->size) < 0);
if (errno == EPIPE)
write_err = 0;
} else {
write_err = copy_fd(params->fd, child_process.in);
if (write_err == COPY_WRITE_ERROR && errno == EPIPE)
write_err = 0;
}
 
if (close(child_process.in))
write_err = 1;
if (write_err)
error("cannot feed the input to external filter '%s'", 
params->cmd);
 
sigchain_pop(SIGPIPE);
 
status = finish_command(_process);
if (status)
error("external filter '%s' failed %d", params->cmd, status);
 
strbuf_release();
return (write_err || status);
 }
-- 
2.14.1

[PATCH 12/34] diff: release strbuf after use in show_stats()

[Rene Scharfe]

Signed-off-by: Rene Scharfe 
---
 diff.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/diff.c b/diff.c
index 33c65f492d..64cdcf2331 100644
--- a/diff.c
+++ b/diff.c
@@ -2334,255 +2334,256 @@ void print_stat_summary(FILE *fp, int files,
 static void show_stats(struct diffstat_t *data, struct diff_options *options)
 {
int i, len, add, del, adds = 0, dels = 0;
uintmax_t max_change = 0, max_len = 0;
int total_files = data->nr, count;
int width, name_width, graph_width, number_width = 0, bin_width = 0;
const char *reset, *add_c, *del_c;
int extra_shown = 0;
const char *line_prefix = diff_line_prefix(options);
struct strbuf out = STRBUF_INIT;
 
if (data->nr == 0)
return;
 
count = options->stat_count ? options->stat_count : data->nr;
 
reset = diff_get_color_opt(options, DIFF_RESET);
add_c = diff_get_color_opt(options, DIFF_FILE_NEW);
del_c = diff_get_color_opt(options, DIFF_FILE_OLD);
 
/*
 * Find the longest filename and max number of changes
 */
for (i = 0; (i < count) && (i < data->nr); i++) {
struct diffstat_file *file = data->files[i];
uintmax_t change = file->added + file->deleted;
 
if (!file->is_interesting && (change == 0)) {
count++; /* not shown == room for one more */
continue;
}
fill_print_name(file);
len = strlen(file->print_name);
if (max_len < len)
max_len = len;
 
if (file->is_unmerged) {
/* "Unmerged" is 8 characters */
bin_width = bin_width < 8 ? 8 : bin_width;
continue;
}
if (file->is_binary) {
/* "Bin XXX -> YYY bytes" */
int w = 14 + decimal_width(file->added)
+ decimal_width(file->deleted);
bin_width = bin_width < w ? w : bin_width;
/* Display change counts aligned with "Bin" */
number_width = 3;
continue;
}
 
if (max_change < change)
max_change = change;
}
count = i; /* where we can stop scanning in data->files[] */
 
/*
 * We have width = stat_width or term_columns() columns total.
 * We want a maximum of min(max_len, stat_name_width) for the name part.
 * We want a maximum of min(max_change, stat_graph_width) for the +- 
part.
 * We also need 1 for " " and 4 + decimal_width(max_change)
 * for " |  " and one the empty column at the end, altogether
 * 6 + decimal_width(max_change).
 *
 * If there's not enough space, we will use the smaller of
 * stat_name_width (if set) and 5/8*width for the filename,
 * and the rest for constant elements + graph part, but no more
 * than stat_graph_width for the graph part.
 * (5/8 gives 50 for filename and 30 for the constant parts + graph
 * for the standard terminal size).
 *
 * In other words: stat_width limits the maximum width, and
 * stat_name_width fixes the maximum width of the filename,
 * and is also used to divide available columns if there
 * aren't enough.
 *
 * Binary files are displayed with "Bin XXX -> YYY bytes"
 * instead of the change count and graph. This part is treated
 * similarly to the graph part, except that it is not
 * "scaled". If total width is too small to accommodate the
 * guaranteed minimum width of the filename part and the
 * separators and this message, this message will "overflow"
 * making the line longer than the maximum width.
 */
 
if (options->stat_width == -1)
width = term_columns() - strlen(line_prefix);
else
width = options->stat_width ? options->stat_width : 80;
number_width = decimal_width(max_change) > number_width ?
decimal_width(max_change) : number_width;
 
if (options->stat_graph_width == -1)
options->stat_graph_width = diff_stat_graph_width;
 
/*
 * Guarantee 3/8*16==6 for the graph part
 * and 5/8*16==10 for the filename part
 */
if (width < 16 + 6 + number_width)
width = 16 + 6 + number_width;
 
/*
 * First assign sizes that are wanted, ignoring available width.
 * strlen("Bin XXX -> YYY bytes") == bin_width, and the part
 * starting from "XXX" should fit in graph_width.
 */
graph_width = max_change + 4 > bin_width ? max_change : bin_width - 4;
if 

[PATCH 03/34] am: release strbuf after use in safe_to_abort()

[Rene Scharfe]

Signed-off-by: Rene Scharfe 
---
 builtin/am.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/am.c b/builtin/am.c
index 3d38b3fe9f..d7513f5375 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2095,29 +2095,30 @@ static void am_skip(struct am_state *state)
 static int safe_to_abort(const struct am_state *state)
 {
struct strbuf sb = STRBUF_INIT;
struct object_id abort_safety, head;
 
if (file_exists(am_path(state, "dirtyindex")))
return 0;
 
if (read_state_file(, state, "abort-safety", 1) > 0) {
if (get_oid_hex(sb.buf, _safety))
die(_("could not parse %s"), am_path(state, 
"abort-safety"));
} else
oidclr(_safety);
+   strbuf_release();
 
if (get_oid("HEAD", ))
oidclr();
 
if (!oidcmp(, _safety))
return 1;
 
warning(_("You seem to have moved HEAD since the last 'am' failure.\n"
"Not rewinding to ORIG_HEAD"));
 
return 0;
 }
 
 /**
  * Aborts the current am session if it is safe to do so.
  */
-- 
2.14.1

[PATCH 02/34] am: release strbuf on error return in hg_patch_to_mail()

[Rene Scharfe]

Signed-off-by: Rene Scharfe 
---
 builtin/am.c | 29 +++--
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 3c50b03faa..3d38b3fe9f 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -881,75 +881,84 @@ static int split_mail_stgit_series(struct am_state 
*state, const char **paths,
 static int hg_patch_to_mail(FILE *out, FILE *in, int keep_cr)
 {
struct strbuf sb = STRBUF_INIT;
+   int rc = 0;
 
while (!strbuf_getline_lf(, in)) {
const char *str;
 
if (skip_prefix(sb.buf, "# User ", ))
fprintf(out, "From: %s\n", str);
else if (skip_prefix(sb.buf, "# Date ", )) {
timestamp_t timestamp;
long tz, tz2;
char *end;
 
errno = 0;
timestamp = parse_timestamp(str, , 10);
-   if (errno)
-   return error(_("invalid timestamp"));
+   if (errno) {
+   rc = error(_("invalid timestamp"));
+   goto exit;
+   }
 
-   if (!skip_prefix(end, " ", ))
-   return error(_("invalid Date line"));
+   if (!skip_prefix(end, " ", )) {
+   rc = error(_("invalid Date line"));
+   goto exit;
+   }
 
errno = 0;
tz = strtol(str, , 10);
-   if (errno)
-   return error(_("invalid timezone offset"));
+   if (errno) {
+   rc = error(_("invalid timezone offset"));
+   goto exit;
+   }
 
-   if (*end)
-   return error(_("invalid Date line"));
+   if (*end) {
+   rc = error(_("invalid Date line"));
+   goto exit;
+   }
 
/*
 * mercurial's timezone is in seconds west of UTC,
 * however git's timezone is in hours + minutes east of
 * UTC. Convert it.
 */
tz2 = labs(tz) / 3600 * 100 + labs(tz) % 3600 / 60;
if (tz > 0)
tz2 = -tz2;
 
fprintf(out, "Date: %s\n", show_date(timestamp, tz2, 
DATE_MODE(RFC2822)));
} else if (starts_with(sb.buf, "# ")) {
continue;
} else {
fprintf(out, "\n%s\n", sb.buf);
break;
}
}
 
strbuf_reset();
while (strbuf_fread(, 8192, in) > 0) {
fwrite(sb.buf, 1, sb.len, out);
strbuf_reset();
}
-
+exit:
strbuf_release();
-   return 0;
+   return rc;
 }
 
 /**
  * Splits a list of files/directories into individual email patches. Each path
  * in `paths` must be a file/directory that is formatted according to
  * `patch_format`.
  *
  * Once split out, the individual email patches will be stored in the state
  * directory, with each patch's filename being its index, padded to state->prec
  * digits.
  *
  * state->cur will be set to the index of the first mail, and state->last will
  * be set to the index of the last mail.
  *
  * Set keep_cr to 0 to convert all lines ending with \r\n to end with \n, 1
  * to disable this behavior, -1 to use the default configured setting.
  *
  * Returns 0 on success, -1 on failure.
  */
-- 
2.14.1

[PATCH 20/34] notes: release strbuf after use in notes_copy_from_stdin()

[Rene Scharfe]

Signed-off-by: Rene Scharfe 
---
 builtin/notes.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/notes.c b/builtin/notes.c
index 4303848e04..8e54f2d146 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -278,56 +278,57 @@ static int parse_reedit_arg(const struct option *opt, 
const char *arg, int unset
 static int notes_copy_from_stdin(int force, const char *rewrite_cmd)
 {
struct strbuf buf = STRBUF_INIT;
struct notes_rewrite_cfg *c = NULL;
struct notes_tree *t = NULL;
int ret = 0;
const char *msg = "Notes added by 'git notes copy'";
 
if (rewrite_cmd) {
c = init_copy_notes_for_rewrite(rewrite_cmd);
if (!c)
return 0;
} else {
init_notes(NULL, NULL, NULL, NOTES_INIT_WRITABLE);
t = _notes_tree;
}
 
while (strbuf_getline_lf(, stdin) != EOF) {
struct object_id from_obj, to_obj;
struct strbuf **split;
int err;
 
split = strbuf_split(, ' ');
if (!split[0] || !split[1])
die(_("malformed input line: '%s'."), buf.buf);
strbuf_rtrim(split[0]);
strbuf_rtrim(split[1]);
if (get_oid(split[0]->buf, _obj))
die(_("failed to resolve '%s' as a valid ref."), 
split[0]->buf);
if (get_oid(split[1]->buf, _obj))
die(_("failed to resolve '%s' as a valid ref."), 
split[1]->buf);
 
if (rewrite_cmd)
err = copy_note_for_rewrite(c, _obj, _obj);
else
err = copy_note(t, _obj, _obj, force,
combine_notes_overwrite);
 
if (err) {
error(_("failed to copy notes from '%s' to '%s'"),
  split[0]->buf, split[1]->buf);
ret = 1;
}
 
strbuf_list_free(split);
}
 
if (!rewrite_cmd) {
commit_notes(t, msg);
free_notes(t);
} else {
finish_copy_notes_for_rewrite(c, msg);
}
+   strbuf_release();
return ret;
 }
 
-- 
2.14.1

[PATCH 12/34] diff: release strbuf after use in show_stats()

[Rene Scharfe]

Signed-off-by: Rene Scharfe 
---
 diff.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/diff.c b/diff.c
index 33c65f492d..64cdcf2331 100644
--- a/diff.c
+++ b/diff.c
@@ -2334,255 +2334,256 @@ void print_stat_summary(FILE *fp, int files,
 static void show_stats(struct diffstat_t *data, struct diff_options *options)
 {
int i, len, add, del, adds = 0, dels = 0;
uintmax_t max_change = 0, max_len = 0;
int total_files = data->nr, count;
int width, name_width, graph_width, number_width = 0, bin_width = 0;
const char *reset, *add_c, *del_c;
int extra_shown = 0;
const char *line_prefix = diff_line_prefix(options);
struct strbuf out = STRBUF_INIT;
 
if (data->nr == 0)
return;
 
count = options->stat_count ? options->stat_count : data->nr;
 
reset = diff_get_color_opt(options, DIFF_RESET);
add_c = diff_get_color_opt(options, DIFF_FILE_NEW);
del_c = diff_get_color_opt(options, DIFF_FILE_OLD);
 
/*
 * Find the longest filename and max number of changes
 */
for (i = 0; (i < count) && (i < data->nr); i++) {
struct diffstat_file *file = data->files[i];
uintmax_t change = file->added + file->deleted;
 
if (!file->is_interesting && (change == 0)) {
count++; /* not shown == room for one more */
continue;
}
fill_print_name(file);
len = strlen(file->print_name);
if (max_len < len)
max_len = len;
 
if (file->is_unmerged) {
/* "Unmerged" is 8 characters */
bin_width = bin_width < 8 ? 8 : bin_width;
continue;
}
if (file->is_binary) {
/* "Bin XXX -> YYY bytes" */
int w = 14 + decimal_width(file->added)
+ decimal_width(file->deleted);
bin_width = bin_width < w ? w : bin_width;
/* Display change counts aligned with "Bin" */
number_width = 3;
continue;
}
 
if (max_change < change)
max_change = change;
}
count = i; /* where we can stop scanning in data->files[] */
 
/*
 * We have width = stat_width or term_columns() columns total.
 * We want a maximum of min(max_len, stat_name_width) for the name part.
 * We want a maximum of min(max_change, stat_graph_width) for the +- 
part.
 * We also need 1 for " " and 4 + decimal_width(max_change)
 * for " |  " and one the empty column at the end, altogether
 * 6 + decimal_width(max_change).
 *
 * If there's not enough space, we will use the smaller of
 * stat_name_width (if set) and 5/8*width for the filename,
 * and the rest for constant elements + graph part, but no more
 * than stat_graph_width for the graph part.
 * (5/8 gives 50 for filename and 30 for the constant parts + graph
 * for the standard terminal size).
 *
 * In other words: stat_width limits the maximum width, and
 * stat_name_width fixes the maximum width of the filename,
 * and is also used to divide available columns if there
 * aren't enough.
 *
 * Binary files are displayed with "Bin XXX -> YYY bytes"
 * instead of the change count and graph. This part is treated
 * similarly to the graph part, except that it is not
 * "scaled". If total width is too small to accommodate the
 * guaranteed minimum width of the filename part and the
 * separators and this message, this message will "overflow"
 * making the line longer than the maximum width.
 */
 
if (options->stat_width == -1)
width = term_columns() - strlen(line_prefix);
else
width = options->stat_width ? options->stat_width : 80;
number_width = decimal_width(max_change) > number_width ?
decimal_width(max_change) : number_width;
 
if (options->stat_graph_width == -1)
options->stat_graph_width = diff_stat_graph_width;
 
/*
 * Guarantee 3/8*16==6 for the graph part
 * and 5/8*16==10 for the filename part
 */
if (width < 16 + 6 + number_width)
width = 16 + 6 + number_width;
 
/*
 * First assign sizes that are wanted, ignoring available width.
 * strlen("Bin XXX -> YYY bytes") == bin_width, and the part
 * starting from "XXX" should fit in graph_width.
 */
graph_width = max_change + 4 > bin_width ? max_change : bin_width - 4;
if 

[PATCH 04/34] check-ref-format: release strbuf after use in check_ref_format_branch()

[Rene Scharfe]

Signed-off-by: Rene Scharfe 
---
 builtin/check-ref-format.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c
index eac499450f..6c40ff110b 100644
--- a/builtin/check-ref-format.c
+++ b/builtin/check-ref-format.c
@@ -39,12 +39,13 @@ static char *collapse_slashes(const char *refname)
 static int check_ref_format_branch(const char *arg)
 {
struct strbuf sb = STRBUF_INIT;
int nongit;
 
setup_git_directory_gently();
if (strbuf_check_branch_ref(, arg))
die("'%s' is not a valid branch name", arg);
printf("%s\n", sb.buf + 11);
+   strbuf_release();
return 0;
 }
 
-- 
2.14.1

[PATCH 03/34] am: release strbuf after use in safe_to_abort()

[Rene Scharfe]

Signed-off-by: Rene Scharfe 
---
 builtin/am.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/am.c b/builtin/am.c
index 3d38b3fe9f..d7513f5375 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2095,29 +2095,30 @@ static void am_skip(struct am_state *state)
 static int safe_to_abort(const struct am_state *state)
 {
struct strbuf sb = STRBUF_INIT;
struct object_id abort_safety, head;
 
if (file_exists(am_path(state, "dirtyindex")))
return 0;
 
if (read_state_file(, state, "abort-safety", 1) > 0) {
if (get_oid_hex(sb.buf, _safety))
die(_("could not parse %s"), am_path(state, 
"abort-safety"));
} else
oidclr(_safety);
+   strbuf_release();
 
if (get_oid("HEAD", ))
oidclr();
 
if (!oidcmp(, _safety))
return 1;
 
warning(_("You seem to have moved HEAD since the last 'am' failure.\n"
"Not rewinding to ORIG_HEAD"));
 
return 0;
 }
 
 /**
  * Aborts the current am session if it is safe to do so.
  */
-- 
2.14.1

[PATCH 11/34] diff: release strbuf after use in show_rename_copy()

[Rene Scharfe]

Signed-off-by: Rene Scharfe 
---
 diff.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/diff.c b/diff.c
index 4148ba6980..33c65f492d 100644
--- a/diff.c
+++ b/diff.c
@@ -5281,14 +5281,15 @@ static void show_mode_change(struct diff_options *opt, 
struct diff_filepair *p,
 static void show_rename_copy(struct diff_options *opt, const char *renamecopy,
struct diff_filepair *p)
 {
struct strbuf sb = STRBUF_INIT;
char *names = pprint_rename(p->one->path, p->two->path);
strbuf_addf(, " %s %s (%d%%)\n",
renamecopy, names, similarity_index(p));
free(names);
emit_diff_symbol(opt, DIFF_SYMBOL_SUMMARY,
 sb.buf, sb.len, 0);
show_mode_change(opt, p, 0);
+   strbuf_release();
 }
 
 static void diff_summary(struct diff_options *opt, struct diff_filepair *p)
-- 
2.14.1

[PATCH 08/34] connect: release strbuf on error return in git_connect()

[Rene Scharfe]

Reduce the scope of the variable cmd and release it before returning
early.

Signed-off-by: Rene Scharfe 
---
 connect.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/connect.c b/connect.c
index 49b28b83be..df56c0cbff 100644
--- a/connect.c
+++ b/connect.c
@@ -775,146 +775,148 @@ static void handle_ssh_variant(const char *ssh_command, 
int is_cmdline,
 struct child_process *git_connect(int fd[2], const char *url,
  const char *prog, int flags)
 {
char *hostandport, *path;
struct child_process *conn = _fork;
enum protocol protocol;
-   struct strbuf cmd = STRBUF_INIT;
 
/* Without this we cannot rely on waitpid() to tell
 * what happened to our children.
 */
signal(SIGCHLD, SIG_DFL);
 
protocol = parse_connect_url(url, , );
if ((flags & CONNECT_DIAG_URL) && (protocol != PROTO_SSH)) {
printf("Diag: url=%s\n", url ? url : "NULL");
printf("Diag: protocol=%s\n", prot_name(protocol));
printf("Diag: hostandport=%s\n", hostandport ? hostandport : 
"NULL");
printf("Diag: path=%s\n", path ? path : "NULL");
conn = NULL;
} else if (protocol == PROTO_GIT) {
/*
 * Set up virtual host information based on where we will
 * connect, unless the user has overridden us in
 * the environment.
 */
char *target_host = getenv("GIT_OVERRIDE_VIRTUAL_HOST");
if (target_host)
target_host = xstrdup(target_host);
else
target_host = xstrdup(hostandport);
 
transport_check_allowed("git");
 
/* These underlying connection commands die() if they
 * cannot connect.
 */
if (git_use_proxy(hostandport))
conn = git_proxy_connect(fd, hostandport);
else
git_tcp_connect(fd, hostandport, flags);
/*
 * Separate original protocol components prog and path
 * from extended host header with a NUL byte.
 *
 * Note: Do not add any other headers here!  Doing so
 * will cause older git-daemon servers to crash.
 */
packet_write_fmt(fd[1],
 "%s %s%chost=%s%c",
 prog, path, 0,
 target_host, 0);
free(target_host);
} else {
+   struct strbuf cmd = STRBUF_INIT;
+
conn = xmalloc(sizeof(*conn));
child_process_init(conn);
 
if (looks_like_command_line_option(path))
die("strange pathname '%s' blocked", path);
 
strbuf_addstr(, prog);
strbuf_addch(, ' ');
sq_quote_buf(, path);
 
/* remove repo-local variables from the environment */
conn->env = local_repo_env;
conn->use_shell = 1;
conn->in = conn->out = -1;
if (protocol == PROTO_SSH) {
const char *ssh;
int needs_batch = 0;
int port_option = 'p';
char *ssh_host = hostandport;
const char *port = NULL;
transport_check_allowed("ssh");
get_host_and_port(_host, );
 
if (!port)
port = get_port(ssh_host);
 
if (flags & CONNECT_DIAG_URL) {
printf("Diag: url=%s\n", url ? url : "NULL");
printf("Diag: protocol=%s\n", 
prot_name(protocol));
printf("Diag: userandhost=%s\n", ssh_host ? 
ssh_host : "NULL");
printf("Diag: port=%s\n", port ? port : "NONE");
printf("Diag: path=%s\n", path ? path : "NULL");
 
free(hostandport);
free(path);
free(conn);
+   strbuf_release();
return NULL;
}
 
if (looks_like_command_line_option(ssh_host))
die("strange hostname '%s' blocked", ssh_host);
 
ssh = get_ssh_command();
if (ssh)
handle_ssh_variant(ssh, 1, _option,
   _batch);
else {
/*
 * GIT_SSH is the no-shell version 

[PATCH 13/34] help: release strbuf on error return in exec_man_konqueror()

[Rene Scharfe]

Signed-off-by: Rene Scharfe 
---
 builtin/help.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/help.c b/builtin/help.c
index 334a8494ab..991a8bb16c 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -137,21 +137,22 @@ static void exec_woman_emacs(const char *path, const char 
*page)
 static void exec_man_konqueror(const char *path, const char *page)
 {
const char *display = getenv("DISPLAY");
if (display && *display) {
struct strbuf man_page = STRBUF_INIT;
const char *filename = "kfmclient";
 
/* It's simpler to launch konqueror using kfmclient. */
if (path) {
size_t len;
if (strip_suffix(path, "/konqueror", ))
path = xstrfmt("%.*s/kfmclient", (int)len, 
path);
filename = basename((char *)path);
} else
path = "kfmclient";
strbuf_addf(_page, "man:%s(1)", page);
execlp(path, filename, "newTab", man_page.buf, (char *)NULL);
warning_errno(_("failed to exec '%s'"), path);
+   strbuf_release(_page);
}
 }
 
-- 
2.14.1

[PATCH 09/34] convert: release strbuf on error return in filter_buffer_or_fd()

[Rene Scharfe]

Signed-off-by: Rene Scharfe 
---
 convert.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/convert.c b/convert.c
index c5f0b21037..4a0ed8d3cb 100644
--- a/convert.c
+++ b/convert.c
@@ -393,63 +393,65 @@ struct filter_params {
 static int filter_buffer_or_fd(int in, int out, void *data)
 {
/*
 * Spawn cmd and feed the buffer contents through its stdin.
 */
struct child_process child_process = CHILD_PROCESS_INIT;
struct filter_params *params = (struct filter_params *)data;
int write_err, status;
const char *argv[] = { NULL, NULL };
 
/* apply % substitution to cmd */
struct strbuf cmd = STRBUF_INIT;
struct strbuf path = STRBUF_INIT;
struct strbuf_expand_dict_entry dict[] = {
{ "f", NULL, },
{ NULL, NULL, },
};
 
/* quote the path to preserve spaces, etc. */
sq_quote_buf(, params->path);
dict[0].value = path.buf;
 
/* expand all %f with the quoted path */
strbuf_expand(, params->cmd, strbuf_expand_dict_cb, );
strbuf_release();
 
argv[0] = cmd.buf;
 
child_process.argv = argv;
child_process.use_shell = 1;
child_process.in = -1;
child_process.out = out;
 
-   if (start_command(_process))
+   if (start_command(_process)) {
+   strbuf_release();
return error("cannot fork to run external filter '%s'", 
params->cmd);
+   }
 
sigchain_push(SIGPIPE, SIG_IGN);
 
if (params->src) {
write_err = (write_in_full(child_process.in,
   params->src, params->size) < 0);
if (errno == EPIPE)
write_err = 0;
} else {
write_err = copy_fd(params->fd, child_process.in);
if (write_err == COPY_WRITE_ERROR && errno == EPIPE)
write_err = 0;
}
 
if (close(child_process.in))
write_err = 1;
if (write_err)
error("cannot feed the input to external filter '%s'", 
params->cmd);
 
sigchain_pop(SIGPIPE);
 
status = finish_command(_process);
if (status)
error("external filter '%s' failed %d", params->cmd, status);
 
strbuf_release();
return (write_err || status);
 }
-- 
2.14.1

[PATCH 10/34] diff: release strbuf after use in diff_summary()

[Rene Scharfe]

Signed-off-by: Rene Scharfe 
---
 diff.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/diff.c b/diff.c
index 3d3e553a98..4148ba6980 100644
--- a/diff.c
+++ b/diff.c
@@ -5294,28 +5294,29 @@ static void show_rename_copy(struct diff_options *opt, 
const char *renamecopy,
 static void diff_summary(struct diff_options *opt, struct diff_filepair *p)
 {
switch(p->status) {
case DIFF_STATUS_DELETED:
show_file_mode_name(opt, "delete", p->one);
break;
case DIFF_STATUS_ADDED:
show_file_mode_name(opt, "create", p->two);
break;
case DIFF_STATUS_COPIED:
show_rename_copy(opt, "copy", p);
break;
case DIFF_STATUS_RENAMED:
show_rename_copy(opt, "rename", p);
break;
default:
if (p->score) {
struct strbuf sb = STRBUF_INIT;
strbuf_addstr(, " rewrite ");
quote_c_style(p->two->path, , NULL, 0);
strbuf_addf(, " (%d%%)\n", similarity_index(p));
emit_diff_symbol(opt, DIFF_SYMBOL_SUMMARY,
 sb.buf, sb.len, 0);
+   strbuf_release();
}
show_mode_change(opt, p, !p->score);
break;
}
 }
-- 
2.14.1

[PATCH 16/34] mailinfo: release strbuf after use in handle_from()

[Rene Scharfe]

Clean up at the end and jump there instead of returning early.

Signed-off-by: Rene Scharfe 
---
 mailinfo.c | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/mailinfo.c b/mailinfo.c
index bd574cb752..b1f5159546 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -138,66 +138,65 @@ static void unquote_quoted_pair(struct strbuf *line)
 static void handle_from(struct mailinfo *mi, const struct strbuf *from)
 {
char *at;
size_t el;
struct strbuf f;
 
strbuf_init(, from->len);
strbuf_addbuf(, from);
 
unquote_quoted_pair();
 
at = strchr(f.buf, '@');
if (!at) {
parse_bogus_from(mi, from);
-   return;
+   goto out;
}
 
/*
 * If we already have one email, don't take any confusing lines
 */
-   if (mi->email.len && strchr(at + 1, '@')) {
-   strbuf_release();
-   return;
-   }
+   if (mi->email.len && strchr(at + 1, '@'))
+   goto out;
 
/* Pick up the string around '@', possibly delimited with <>
 * pair; that is the email part.
 */
while (at > f.buf) {
char c = at[-1];
if (isspace(c))
break;
if (c == '<') {
at[-1] = ' ';
break;
}
at--;
}
el = strcspn(at, " \n\t\r\v\f>");
strbuf_reset(>email);
strbuf_add(>email, at, el);
strbuf_remove(, at - f.buf, el + (at[el] ? 1 : 0));
 
/* The remainder is name.  It could be
 *
 * - "John Doe "   (a), or
 * - "john.doe@xz (John Doe)"   (b), or
 * - "John (zzz) Doe  (Comment)"   (c)
 *
 * but we have removed the email part, so
 *
 * - remove extra spaces which could stay after email (case 'c'), and
 * - trim from both ends, possibly removing the () pair at the end
 *   (cases 'a' and 'b').
 */
cleanup_space();
strbuf_trim();
if (f.buf[0] == '(' && f.len && f.buf[f.len - 1] == ')') {
strbuf_remove(, 0, 1);
strbuf_setlen(, f.len - 1);
}
 
get_sane_name(>name, , >email);
+out:
strbuf_release();
 }
 
-- 
2.14.1

[PATCH 18/34] merge: release strbuf after use in save_state()

[Rene Scharfe]

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

diff --git a/builtin/merge.c b/builtin/merge.c
index 7df3fe3927..4f8418246b 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -250,27 +250,31 @@ static void drop_save(void)
 static int save_state(struct object_id *stash)
 {
int len;
struct child_process cp = CHILD_PROCESS_INIT;
struct strbuf buffer = STRBUF_INIT;
const char *argv[] = {"stash", "create", NULL};
+   int rc = -1;
 
cp.argv = argv;
cp.out = -1;
cp.git_cmd = 1;
 
if (start_command())
die(_("could not run stash."));
len = strbuf_read(, cp.out, 1024);
close(cp.out);
 
if (finish_command() || len < 0)
die(_("stash failed"));
else if (!len)  /* no changes */
-   return -1;
+   goto out;
strbuf_setlen(, buffer.len-1);
if (get_oid(buffer.buf, stash))
die(_("not a valid object: %s"), buffer.buf);
-   return 0;
+   rc = 0;
+out:
+   strbuf_release();
+   return rc;
 }
 
 static void read_empty(unsigned const char *sha1, int verbose)
-- 
2.14.1

[PATCH 17/34] mailinfo: release strbuf on error return in handle_boundary()

[Rene Scharfe]

Signed-off-by: Rene Scharfe 
---
 mailinfo.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/mailinfo.c b/mailinfo.c
index b1f5159546..f2387a3267 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -911,48 +911,49 @@ static int find_boundary(struct mailinfo *mi, struct 
strbuf *line)
 static int handle_boundary(struct mailinfo *mi, struct strbuf *line)
 {
struct strbuf newline = STRBUF_INIT;
 
strbuf_addch(, '\n');
 again:
if (line->len >= (*(mi->content_top))->len + 2 &&
!memcmp(line->buf + (*(mi->content_top))->len, "--", 2)) {
/* we hit an end boundary */
/* pop the current boundary off the stack */
strbuf_release(*(mi->content_top));
FREE_AND_NULL(*(mi->content_top));
 
/* technically won't happen as is_multipart_boundary()
   will fail first.  But just in case..
 */
if (--mi->content_top < mi->content) {
error("Detected mismatched boundaries, can't recover");
mi->input_error = -1;
mi->content_top = mi->content;
+   strbuf_release();
return 0;
}
handle_filter(mi, );
strbuf_release();
if (mi->input_error)
return 0;
 
/* skip to the next boundary */
if (!find_boundary(mi, line))
return 0;
goto again;
}
 
/* set some defaults */
mi->transfer_encoding = TE_DONTCARE;
strbuf_reset(>charset);
 
/* slurp in this section's info */
while (read_one_header_line(line, mi->input))
check_header(mi, line, mi->p_hdr_data, 0);
 
strbuf_release();
/* replenish line */
if (strbuf_getline_lf(line, mi->input))
return 0;
strbuf_addch(line, '\n');
return 1;
 }
-- 
2.14.1

[PATCH 19/34] merge: release strbuf after use in write_merge_heads()

[Rene Scharfe]

Signed-off-by: Rene Scharfe 
---
 builtin/merge.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/merge.c b/builtin/merge.c
index 4f8418246b..7bc3fe4b6d 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -920,24 +920,25 @@ static int setup_with_upstream(const char ***argv)
 static void write_merge_heads(struct commit_list *remoteheads)
 {
struct commit_list *j;
struct strbuf buf = STRBUF_INIT;
 
for (j = remoteheads; j; j = j->next) {
struct object_id *oid;
struct commit *c = j->item;
if (c->util && merge_remote_util(c)->obj) {
oid = _remote_util(c)->obj->oid;
} else {
oid = >object.oid;
}
strbuf_addf(, "%s\n", oid_to_hex(oid));
}
write_file_buf(git_path_merge_head(), buf.buf, buf.len);
 
strbuf_reset();
if (fast_forward == FF_NO)
strbuf_addstr(, "no-ff");
write_file_buf(git_path_merge_mode(), buf.buf, buf.len);
+   strbuf_release();
 }
 
 static void write_merge_state(struct commit_list *remoteheads)
-- 
2.14.1

[PATCH 07/34] commit: release strbuf on error return in commit_tree_extended()

[Rene Scharfe]

Signed-off-by: Rene Scharfe 
---
 commit.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/commit.c b/commit.c
index 8b28415939..51f969fcbc 100644
--- a/commit.c
+++ b/commit.c
@@ -1514,60 +1514,63 @@ N_("Warning: commit message did not conform to UTF-8.\n"
 int commit_tree_extended(const char *msg, size_t msg_len,
 const unsigned char *tree,
 struct commit_list *parents, unsigned char *ret,
 const char *author, const char *sign_commit,
 struct commit_extra_header *extra)
 {
int result;
int encoding_is_utf8;
struct strbuf buffer;
 
assert_sha1_type(tree, OBJ_TREE);
 
if (memchr(msg, '\0', msg_len))
return error("a NUL byte in commit log message not allowed.");
 
/* Not having i18n.commitencoding is the same as having utf-8 */
encoding_is_utf8 = is_encoding_utf8(git_commit_encoding);
 
strbuf_init(, 8192); /* should avoid reallocs for the headers */
strbuf_addf(, "tree %s\n", sha1_to_hex(tree));
 
/*
 * NOTE! This ordering means that the same exact tree merged with a
 * different order of parents will be a _different_ changeset even
 * if everything else stays the same.
 */
while (parents) {
struct commit *parent = pop_commit();
strbuf_addf(, "parent %s\n",
oid_to_hex(>object.oid));
}
 
/* Person/date information */
if (!author)
author = git_author_info(IDENT_STRICT);
strbuf_addf(, "author %s\n", author);
strbuf_addf(, "committer %s\n", 
git_committer_info(IDENT_STRICT));
if (!encoding_is_utf8)
strbuf_addf(, "encoding %s\n", git_commit_encoding);
 
while (extra) {
add_extra_header(, extra);
extra = extra->next;
}
strbuf_addch(, '\n');
 
/* And add the comment */
strbuf_add(, msg, msg_len);
 
/* And check the encoding */
if (encoding_is_utf8 && !verify_utf8())
fprintf(stderr, _(commit_utf8_warn));
 
-   if (sign_commit && do_sign_commit(, sign_commit))
-   return -1;
+   if (sign_commit && do_sign_commit(, sign_commit)) {
+   result = -1;
+   goto out;
+   }
 
result = write_sha1_file(buffer.buf, buffer.len, commit_type, ret);
+out:
strbuf_release();
return result;
 }
-- 
2.14.1

[PATCH 15/34] help: release strbuf on error return in exec_woman_emacs()

[Rene Scharfe]

Signed-off-by: Rene Scharfe 
---
 builtin/help.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/help.c b/builtin/help.c
index 12fb48933e..b3f60a8f30 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -123,14 +123,15 @@ static int check_emacsclient_version(void)
 static void exec_woman_emacs(const char *path, const char *page)
 {
if (!check_emacsclient_version()) {
/* This works only with emacsclient version >= 22. */
struct strbuf man_page = STRBUF_INIT;
 
if (!path)
path = "emacsclient";
strbuf_addf(_page, "(woman \"%s\")", page);
execlp(path, "emacsclient", "-e", man_page.buf, (char *)NULL);
warning_errno(_("failed to exec '%s'"), path);
+   strbuf_release(_page);
}
 }
 
-- 
2.14.1

[PATCH 14/34] help: release strbuf on error return in exec_man_man()

[Rene Scharfe]

Signed-off-by: Rene Scharfe 
---
 builtin/help.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/help.c b/builtin/help.c
index 991a8bb16c..12fb48933e 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -167,9 +167,10 @@ static void exec_man_man(const char *path, const char 
*page)
 static void exec_man_cmd(const char *cmd, const char *page)
 {
struct strbuf shell_cmd = STRBUF_INIT;
strbuf_addf(_cmd, "%s %s", cmd, page);
execl(SHELL_PATH, SHELL_PATH, "-c", shell_cmd.buf, (char *)NULL);
warning(_("failed to exec '%s'"), cmd);
+   strbuf_release(_cmd);
 }
 
 static void add_man_viewer(const char *name)
-- 
2.14.1

[PATCH 00/34] plug strbuf memory leaks

[Rene Scharfe]

Release allocated strbufs in functions that are at least potentionally
library-like; cmd_*() functions are out of scope because the process
ends with them and the OS cleans up for us anyway.  The patches are
split by function and were generated with --function-context for easier
reviewing.

  am: release strbufs after use in detect_patch_format()
  am: release strbuf on error return in hg_patch_to_mail()
  am: release strbuf after use in safe_to_abort()
  check-ref-format: release strbuf after use in check_ref_format_branch()
  clean: release strbuf after use in remove_dirs()
  clone: release strbuf after use in remove_junk()
  commit: release strbuf on error return in commit_tree_extended()
  connect: release strbuf on error return in git_connect()
  convert: release strbuf on error return in filter_buffer_or_fd()
  diff: release strbuf after use in diff_summary()
  diff: release strbuf after use in show_rename_copy()
  diff: release strbuf after use in show_stats()
  help: release strbuf on error return in exec_man_konqueror()
  help: release strbuf on error return in exec_man_man()
  help: release strbuf on error return in exec_woman_emacs()
  mailinfo: release strbuf after use in handle_from()
  mailinfo: release strbuf on error return in handle_boundary()
  merge: release strbuf after use in save_state()
  merge: release strbuf after use in write_merge_heads()
  notes: release strbuf after use in notes_copy_from_stdin()
  refs: release strbuf on error return in write_pseudoref()
  remote: release strbuf after use in read_remote_branches()
  remote: release strbuf after use in migrate_file()
  remote: release strbuf after use in set_url()
  send-pack: release strbuf on error return in send_pack()
  sha1_file: release strbuf on error return in index_path()
  shortlog: release strbuf after use in insert_one_record()
  sequencer: release strbuf after use in save_head()
  transport-helper: release strbuf after use in process_connect_service()
  userdiff: release strbuf after use in userdiff_get_textconv()
  utf8: release strbuf on error return in strbuf_utf8_replace()
  vcs-svn: release strbuf after use in end_revision()
  wt-status: release strbuf after use in read_rebase_todolist()
  wt-status: release strbuf after use in wt_longstatus_print_tracking()

 builtin/am.c   | 34 ++
 builtin/check-ref-format.c |  1 +
 builtin/clean.c|  7 +--
 builtin/clone.c|  2 +-
 builtin/help.c |  3 +++
 builtin/merge.c|  9 +++--
 builtin/notes.c|  1 +
 builtin/remote.c   |  8 +---
 builtin/shortlog.c |  1 +
 commit.c   |  7 +--
 connect.c  |  4 +++-
 convert.c  |  4 +++-
 diff.c |  3 +++
 mailinfo.c | 10 +-
 refs.c |  2 +-
 send-pack.c|  5 -
 sequencer.c|  5 -
 sha1_file.c|  6 +++---
 transport-helper.c |  1 +
 userdiff.c |  1 +
 utf8.c |  3 ++-
 vcs-svn/svndump.c  |  1 +
 wt-status.c|  2 ++
 23 files changed, 84 insertions(+), 36 deletions(-)

-- 
2.14.1

[PATCH 06/34] clone: release strbuf after use in remove_junk()

[Rene Scharfe]

Signed-off-by: Rene Scharfe 
---
 builtin/clone.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 8d11b570a1..dbddd98f80 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -487,28 +487,28 @@ N_("Clone succeeded, but checkout failed.\n"
 static void remove_junk(void)
 {
struct strbuf sb = STRBUF_INIT;
 
switch (junk_mode) {
case JUNK_LEAVE_REPO:
warning("%s", _(junk_leave_repo_msg));
/* fall-through */
case JUNK_LEAVE_ALL:
return;
default:
/* proceed to removal */
break;
}
 
if (junk_git_dir) {
strbuf_addstr(, junk_git_dir);
remove_dir_recursively(, 0);
strbuf_reset();
}
if (junk_work_tree) {
strbuf_addstr(, junk_work_tree);
remove_dir_recursively(, 0);
-   strbuf_reset();
}
+   strbuf_release();
 }
 
 static void remove_junk_on_signal(int signo)
-- 
2.14.1

[PATCH 05/34] clean: release strbuf after use in remove_dirs()

[Rene Scharfe]

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

diff --git a/builtin/clean.c b/builtin/clean.c
index 21a7a32994..733b6d3745 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -151,104 +151,107 @@ static int exclude_cb(const struct option *opt, const 
char *arg, int unset)
 static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
int dry_run, int quiet, int *dir_gone)
 {
DIR *dir;
struct strbuf quoted = STRBUF_INIT;
struct dirent *e;
int res = 0, ret = 0, gone = 1, original_len = path->len, len;
struct string_list dels = STRING_LIST_INIT_DUP;
 
*dir_gone = 1;
 
if ((force_flag & REMOVE_DIR_KEEP_NESTED_GIT) && 
is_nonbare_repository_dir(path)) {
if (!quiet) {
quote_path_relative(path->buf, prefix, );
printf(dry_run ?  _(msg_would_skip_git_dir) : 
_(msg_skip_git_dir),
quoted.buf);
}
 
*dir_gone = 0;
-   return 0;
+   goto out;
}
 
dir = opendir(path->buf);
if (!dir) {
/* an empty dir could be removed even if it is unreadble */
res = dry_run ? 0 : rmdir(path->buf);
if (res) {
int saved_errno = errno;
quote_path_relative(path->buf, prefix, );
errno = saved_errno;
warning_errno(_(msg_warn_remove_failed), quoted.buf);
*dir_gone = 0;
}
-   return res;
+   ret = res;
+   goto out;
}
 
strbuf_complete(path, '/');
 
len = path->len;
while ((e = readdir(dir)) != NULL) {
struct stat st;
if (is_dot_or_dotdot(e->d_name))
continue;
 
strbuf_setlen(path, len);
strbuf_addstr(path, e->d_name);
if (lstat(path->buf, ))
; /* fall thru */
else if (S_ISDIR(st.st_mode)) {
if (remove_dirs(path, prefix, force_flag, dry_run, 
quiet, ))
ret = 1;
if (gone) {
quote_path_relative(path->buf, prefix, );
string_list_append(, quoted.buf);
} else
*dir_gone = 0;
continue;
} else {
res = dry_run ? 0 : unlink(path->buf);
if (!res) {
quote_path_relative(path->buf, prefix, );
string_list_append(, quoted.buf);
} else {
int saved_errno = errno;
quote_path_relative(path->buf, prefix, );
errno = saved_errno;
warning_errno(_(msg_warn_remove_failed), 
quoted.buf);
*dir_gone = 0;
ret = 1;
}
continue;
}
 
/* path too long, stat fails, or non-directory still exists */
*dir_gone = 0;
ret = 1;
break;
}
closedir(dir);
 
strbuf_setlen(path, original_len);
 
if (*dir_gone) {
res = dry_run ? 0 : rmdir(path->buf);
if (!res)
*dir_gone = 1;
else {
int saved_errno = errno;
quote_path_relative(path->buf, prefix, );
errno = saved_errno;
warning_errno(_(msg_warn_remove_failed), quoted.buf);
*dir_gone = 0;
ret = 1;
}
}
 
if (!*dir_gone && !quiet) {
int i;
for (i = 0; i < dels.nr; i++)
printf(dry_run ?  _(msg_would_remove) : _(msg_remove), 
dels.items[i].string);
}
+out:
+   strbuf_release();
string_list_clear(, 0);
return ret;
 }
-- 
2.14.1

[PATCH 02/34] am: release strbuf on error return in hg_patch_to_mail()

[Rene Scharfe]

Signed-off-by: Rene Scharfe 
---
 builtin/am.c | 29 +++--
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 3c50b03faa..3d38b3fe9f 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -881,75 +881,84 @@ static int split_mail_stgit_series(struct am_state 
*state, const char **paths,
 static int hg_patch_to_mail(FILE *out, FILE *in, int keep_cr)
 {
struct strbuf sb = STRBUF_INIT;
+   int rc = 0;
 
while (!strbuf_getline_lf(, in)) {
const char *str;
 
if (skip_prefix(sb.buf, "# User ", ))
fprintf(out, "From: %s\n", str);
else if (skip_prefix(sb.buf, "# Date ", )) {
timestamp_t timestamp;
long tz, tz2;
char *end;
 
errno = 0;
timestamp = parse_timestamp(str, , 10);
-   if (errno)
-   return error(_("invalid timestamp"));
+   if (errno) {
+   rc = error(_("invalid timestamp"));
+   goto exit;
+   }
 
-   if (!skip_prefix(end, " ", ))
-   return error(_("invalid Date line"));
+   if (!skip_prefix(end, " ", )) {
+   rc = error(_("invalid Date line"));
+   goto exit;
+   }
 
errno = 0;
tz = strtol(str, , 10);
-   if (errno)
-   return error(_("invalid timezone offset"));
+   if (errno) {
+   rc = error(_("invalid timezone offset"));
+   goto exit;
+   }
 
-   if (*end)
-   return error(_("invalid Date line"));
+   if (*end) {
+   rc = error(_("invalid Date line"));
+   goto exit;
+   }
 
/*
 * mercurial's timezone is in seconds west of UTC,
 * however git's timezone is in hours + minutes east of
 * UTC. Convert it.
 */
tz2 = labs(tz) / 3600 * 100 + labs(tz) % 3600 / 60;
if (tz > 0)
tz2 = -tz2;
 
fprintf(out, "Date: %s\n", show_date(timestamp, tz2, 
DATE_MODE(RFC2822)));
} else if (starts_with(sb.buf, "# ")) {
continue;
} else {
fprintf(out, "\n%s\n", sb.buf);
break;
}
}
 
strbuf_reset();
while (strbuf_fread(, 8192, in) > 0) {
fwrite(sb.buf, 1, sb.len, out);
strbuf_reset();
}
-
+exit:
strbuf_release();
-   return 0;
+   return rc;
 }
 
 /**
  * Splits a list of files/directories into individual email patches. Each path
  * in `paths` must be a file/directory that is formatted according to
  * `patch_format`.
  *
  * Once split out, the individual email patches will be stored in the state
  * directory, with each patch's filename being its index, padded to state->prec
  * digits.
  *
  * state->cur will be set to the index of the first mail, and state->last will
  * be set to the index of the last mail.
  *
  * Set keep_cr to 0 to convert all lines ending with \r\n to end with \n, 1
  * to disable this behavior, -1 to use the default configured setting.
  *
  * Returns 0 on success, -1 on failure.
  */
-- 
2.14.1

[PATCH 01/34] am: release strbufs after use in detect_patch_format()

[Rene Scharfe]

Don't reset the strbufs l2 and l3 before use as if they were static, but
release them at the end instead.

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

diff --git a/builtin/am.c b/builtin/am.c
index c369dd1dce..3c50b03faa 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -633,73 +633,73 @@ static int is_mail(FILE *fp)
 static int detect_patch_format(const char **paths)
 {
enum patch_format ret = PATCH_FORMAT_UNKNOWN;
struct strbuf l1 = STRBUF_INIT;
struct strbuf l2 = STRBUF_INIT;
struct strbuf l3 = STRBUF_INIT;
FILE *fp;
 
/*
 * We default to mbox format if input is from stdin and for directories
 */
if (!*paths || !strcmp(*paths, "-") || is_directory(*paths))
return PATCH_FORMAT_MBOX;
 
/*
 * Otherwise, check the first few lines of the first patch, starting
 * from the first non-blank line, to try to detect its format.
 */
 
fp = xfopen(*paths, "r");
 
while (!strbuf_getline(, fp)) {
if (l1.len)
break;
}
 
if (starts_with(l1.buf, "From ") || starts_with(l1.buf, "From: ")) {
ret = PATCH_FORMAT_MBOX;
goto done;
}
 
if (starts_with(l1.buf, "# This series applies on GIT commit")) {
ret = PATCH_FORMAT_STGIT_SERIES;
goto done;
}
 
if (!strcmp(l1.buf, "# HG changeset patch")) {
ret = PATCH_FORMAT_HG;
goto done;
}
 
-   strbuf_reset();
strbuf_getline(, fp);
-   strbuf_reset();
strbuf_getline(, fp);
 
/*
 * If the second line is empty and the third is a From, Author or Date
 * entry, this is likely an StGit patch.
 */
if (l1.len && !l2.len &&
(starts_with(l3.buf, "From:") ||
 starts_with(l3.buf, "Author:") ||
 starts_with(l3.buf, "Date:"))) {
ret = PATCH_FORMAT_STGIT;
goto done;
}
 
if (l1.len && is_mail(fp)) {
ret = PATCH_FORMAT_MBOX;
goto done;
}
 
 done:
fclose(fp);
strbuf_release();
+   strbuf_release();
+   strbuf_release();
return ret;
 }
 
 /**
  * Splits out individual email patches from `paths`, where each path is either
  * a mbox file or a Maildir. Returns 0 on success, -1 on failure.
  */
-- 
2.14.1

Re: [PATCH 07/10] t1404: demonstrate two problems with reference transactions

[Stefan Beller]

On Tue, Aug 29, 2017 at 1:20 AM, Michael Haggerty  wrote:
> Currently, a loose reference is deleted even before locking the
> `packed-refs` file, let alone deleting any packed version of the
> reference. This leads to two problems, demonstrated by two new tests:
>
> * While a reference is being deleted, other processes might see the
>   old, packed value of the reference for a moment before the packed
>   version is deleted. Normally this would be hard to observe, but we
>   can prolong the window by locking the `packed-refs` file externally
>   before running `update-ref`, then unlocking it before `update-ref`'s
>   attempt to acquire the lock times out.
>
> * If the `packed-refs` file is locked so long that `update-ref` fails
>   to lock it, then the reference can be left permanently in the
>   incorrect state described in the previous point.
>
> In a moment, both problems will be fixed.
>
> Signed-off-by: Michael Haggerty 
> ---
> The first of the new tests is rather involved; it uses two background
> processes plus a foreground process that polls the value of a
> reference. But despite sensitivity to timing, I think it should be
> robust even in this broken state. Once the functionality being tested
> is fixed, this test should never produce false positives, though
> really bad timing (e.g., if it takes more than a second for
> `update-ref` to get going) could still lead to false negatives.
>
> Each of the new tests takes about a second to run because they
> simulate lock contention.
>
> If anybody has suggestions for better ways to test these things,
> please speak up :-)
>
>  t/t1404-update-ref-errors.sh | 71 
> 
>  1 file changed, 71 insertions(+)
>
> diff --git a/t/t1404-update-ref-errors.sh b/t/t1404-update-ref-errors.sh
> index c34ece48f5..752f83c377 100755
> --- a/t/t1404-update-ref-errors.sh
> +++ b/t/t1404-update-ref-errors.sh
> @@ -404,4 +404,75 @@ test_expect_success 'broken reference blocks indirect 
> create' '
> test_cmp expected output.err
>  '
>
> +test_expect_failure 'no bogus intermediate values during delete' '
> +   prefix=refs/slow-transaction &&
> +   # Set up a reference with differing loose and packed versions:
> +   git update-ref $prefix/foo $C &&
> +   git pack-refs --all &&
> +   git update-ref $prefix/foo $D &&
> +   git for-each-ref $prefix >unchanged &&
> +   # Now try to update the reference, but hold the `packed-refs` lock
> +   # for a while to see what happens while the process is blocked:
> +   : >.git/packed-refs.lock &&
> +   test_when_finished "rm -f .git/packed-refs.lock" &&
> +   {
> +   sleep 1 &&
> +   rm -f .git/packed-refs.lock &
> +   } &&
> +   pid1=$! &&
> +   {
> +   # Note: the following command is intentionally run in the
> +   # background. We extend the timeout so that `update-ref`
> +   # tries to acquire the `packed-refs` lock longer than it
> +   # takes the background process above to delete it:
> +   git -c core.packedrefstimeout=2000 update-ref -d $prefix/foo &
> +   } &&
> +   pid2=$! &&
> +   ok=true &&
> +   while kill -0 $pid2 2>/dev/null

If sig is 0, then no signal is sent, but error checking is still
performed; this can be used to check for the existence of a
process ID or process group ID.

So the kill -0 is the idiomatic form of "while $pid2 is still alive"?
ignoring errors due to the dev/null redirection?

And due to the nature of this test we have to have a busy
loop, we cannot rate limit the cpu usage inside the loop
via some shorter sleeps, as ideally we want to observe
the ref at any time.

In an ideal world this test would instruct the kernel to interrupt
the executing program (update-ref) at certain events such as
touching/writing/deleting files and in each interrupt we could
inspect the file system in a read only fashion.


> +   do
> +   sha1=$(git rev-parse --verify --quiet $prefix/foo || echo 
> undefined) &&
> +   case "$sha1" in
> +   $D)
> +   # This is OK; it just means that nothing has happened 
> yet.
> +   : ;;
> +   undefined)
> +   # This is OK; it means the deletion was successful.
> +   : ;;
> +   $C)
> +   # This value should never be seen. Probably the loose
> +   # reference has been deleted but the packed reference
> +   # is still there:
> +   echo "$prefix/foo incorrectly observed to be C" &&
> +   break
> +   ;;
> +   *)
> +   # WTF?
> +   echo "$prefix/foo unexpected value observed: $sha1" &&
> +   

Re: [RFC 0/7] transitioning to protocol v2

[Brandon Williams]

On 08/30, Jeff Hostetler wrote:
> 
> 
> On 8/29/2017 11:06 PM, Jeff King wrote:
> >On Tue, Aug 29, 2017 at 04:08:25PM -0400, Jeff Hostetler wrote:
> >
> >>I just wanted to jump in here and say I've done some initial
> >>testing of this against VSTS and so far it seems fine.  And yes,
> >>we have a custom git server.
> >
> >Great, thank you for checking.
> >
> >>VSTS doesn't support the "git://" protocol, so the double-null trick
> >>isn't an issue for us.  But "https://; worked just fine.  I'm still
> >>asking around internally whether we support passing SSH environment
> >>variables.
> >
> >The key thing for ssh is not whether you support passing environment
> >variables. It's whether you quietly ignore unknown variables rather than
> >cutting off the connection.
> >
> >To support the v2 protocol you'd need to pass the new variables, but
> >you'd also need to modify your server to actually do something useful
> >with them anyway. At this point we're mostly concerned with whether we
> >can safely pass the variables to current implementations unconditionally
> >and get a reasonable outcome.
> 
> Right.  I just spoke with our server folks and, currently, our SSH
> support quietly eats ALL variables.   So we're safe :-)
> 
> I'm starting a conversation with them to pass them thru so we can
> be ready for this.  (Assuming we choose to go this way.)

Perfect! Thanks again.

-- 
Brandon Williams

Re: Produce contents of index with existing directory as cache

[David Lloyd]

Ah, does it not?  Perhaps add a "git clean -d -f" as well.

On Wed, Aug 30, 2017 at 9:18 AM, Florian Weimer  wrote:
> On 08/30/2017 04:15 PM, David Lloyd wrote:
>> git reset --hard ?
>
> That doesn't remove unstaged files.
>
> Florian



-- 
- DML

Re: Produce contents of index with existing directory as cache

[Florian Weimer]

On 08/30/2017 04:15 PM, David Lloyd wrote:
> git reset --hard ?

That doesn't remove unstaged files.

Florian

Re: Produce contents of index with existing directory as cache

[David Lloyd]

git reset --hard ?

On Wed, Aug 30, 2017 at 9:11 AM, Florian Weimer  wrote:
> Is there a variant of “git checkout-index” which will produce the
> existing index contents, like “git checkout-index” would do with an
> empty directory, but can reuse an existing directory tree, to avoid
> writing all files from scratch?
>
> I'm writing some analysis scripts which need to change a checked-out
> working tree.  Right now I'm throwing away the tree after making the
> changes and create the next tree (which is expected to be quite similar
> in contents) from scratch, starting with an empty directory.  This is
> quite slow.
>
> Thanks,
> Florian



-- 
- DML

Re: [PATCH v5 35/40] Add Documentation/technical/external-odb.txt

[Christian Couder]

On Wed, Aug 30, 2017 at 2:50 PM, Ben Peart  wrote:
>
>
> On 8/29/2017 11:43 AM, Christian Couder wrote:
>>
>> On Mon, Aug 28, 2017 at 8:59 PM, Ben Peart  wrote:
>>>
>>>
>>> On 8/3/2017 5:19 AM, Christian Couder wrote:


 +Helpers
 +===
 +
 +ODB helpers are commands that have to be registered using either the
 +"odb..subprocessCommand" or the "odb..scriptCommand"
 +config variables.
 +
 +Registering such a command tells Git that an external odb called
 + exists and that the registered command should be used to
 +communicate with it.
>>>
>>>
>>> What order are the odb handlers called? Are they called before or after
>>> the
>>> regular object store code for loose, pack and alternates?  Is the order
>>> configurable?
>>
>>
>> For get_*_object instructions the regular code is called before the odb
>> helpers.
>> (So the odb helper code is at the end of stat_sha1_file() and of
>> open_sha1_file() in sha1_file.c.)
>>
>> For put_*_object instructions the regular code is called after the odb
>> helpers.
>> (So the odb helper code is at the beginning of write_sha1_file() in
>> sha1_file.c.)
>>
>> And no this order is not configurable, but of course it could be made
>> configurable.
>>
 + - 'get_direct '
 +
 +This instruction is similar as the other 'get_*' instructions except
 +that no object should be sent from the helper to Git. Instead the
 +helper should directly write the requested object into a loose object
 +file in the ".git/objects" directory.
 +
 +After the helper has sent the "status=success" packet and the
 +following flush packet in process mode, or after it has exited in the
 +script mode, Git should lookup again for a loose object file with the
 +requested sha1.
>>>
>>>
>>> When will git call get_direct vs one of the other get_* functions?
>>
>>
>> It is called just before exiting when git cannot find an object.
>> It is not exactly at the same place as other get_* instructions as I
>> tried to reuse your code and as it looks like it makes it easier to
>> retry the regular code after the odb helper code.
>>
>>> Could the
>>> functionality of enabling a helper to populate objects into the regular
>>> object store be provided by having a ODB helper that returned the object
>>> data as requested by get_git_obj or get_raw_obj but also stored it in the
>>> regular object store as a loose object (or pack file) for future calls?
>>
>>
>> I am not sure I understand what you mean.
>> If a helper returns the object data as requested by get_git_obj or
>> get_raw_obj, then currently Git will itself store the object locally
>> in its regular object store, so it is redundant for the helper to also
>> store or try to store the object in the regular object store.
>>
>
> Doesn't this mean that objects will "leak out" into the regular object store
> as they are used?  For example, at checkout, all objects in the requested
> commit would be retrieved from the various object stores and if they came
> from a "large blob" ODB handler, they would get retrieved from the ODB
> handler and then written to the regular object store (presumably as a loose
> object).  From then on, the object would be retrieved from the regular
> object store.
>
> This would seem to defeat the goal of enabling specialized object handlers
> to handle large or other "unusual" objects that git normally doesn't deal
> well with.

Yeah, I agree that storing the objects in the regular object store
should not be done in all the cases.
There should be a way to control that.

Produce contents of index with existing directory as cache

[Florian Weimer]

Is there a variant of “git checkout-index” which will produce the
existing index contents, like “git checkout-index” would do with an
empty directory, but can reuse an existing directory tree, to avoid
writing all files from scratch?

I'm writing some analysis scripts which need to change a checked-out
working tree.  Right now I'm throwing away the tree after making the
changes and create the next tree (which is expected to be quite similar
in contents) from scratch, starting with an empty directory.  This is
quite slow.

Thanks,
Florian

[PATCH v2] Documentation: mention that `eol` can change the dirty status of paths

[Ben Boeckel]

When setting the `eol` attribute, paths can change their dirty status
without any change in the working directory. This can cause confusion
and should at least be mentioned with a remedy.

Reviewed-by: Torsten Bögershausen 
Signed-off-by: Ben Boeckel 
---
 Documentation/gitattributes.txt | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index c4f2be2..b940d37 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -151,7 +151,10 @@ unspecified.
 
 This attribute sets a specific line-ending style to be used in the
 working directory.  It enables end-of-line conversion without any
-content checks, effectively setting the `text` attribute.
+content checks, effectively setting the `text` attribute.  Note that
+setting this attribute on paths which are in the index with CRLF line
+endings makes the paths to be considered dirty.  Adding the path to the
+index again will normalize the line endings in the index.
 
 Set to string value "crlf"::
 
-- 
2.9.5

Re: [PATCH] Documentation: mention that `eol` can change the dirty status of paths

[Ben Boeckel]

On Thu, Aug 24, 2017 at 07:50:54 +0200, Torsten Bögershausen wrote:
>   This attribute sets a specific line-ending style to be used in the
>   working directory.  It enables end-of-line conversion without any
>  -content checks, effectively setting the `text` attribute.
>  +content checks, effectively setting the `text` attribute.  Note that
>  +setting this attribute on paths which are in the index with CRLF
>  +line endings makes the paths to be considered dirty.
>  +Adding the path to the index again will normalize the line
>  +endings in the index.

Yes, that sounds better. Will resubmit the patch.

--Ben

Re: [RFC 0/7] transitioning to protocol v2

[Jeff Hostetler]

On 8/29/2017 11:06 PM, Jeff King wrote:

On Tue, Aug 29, 2017 at 04:08:25PM -0400, Jeff Hostetler wrote:


I just wanted to jump in here and say I've done some initial
testing of this against VSTS and so far it seems fine.  And yes,
we have a custom git server.


Great, thank you for checking.


VSTS doesn't support the "git://" protocol, so the double-null trick
isn't an issue for us.  But "https://; worked just fine.  I'm still
asking around internally whether we support passing SSH environment
variables.


The key thing for ssh is not whether you support passing environment
variables. It's whether you quietly ignore unknown variables rather than
cutting off the connection.

>

To support the v2 protocol you'd need to pass the new variables, but
you'd also need to modify your server to actually do something useful
with them anyway. At this point we're mostly concerned with whether we
can safely pass the variables to current implementations unconditionally
and get a reasonable outcome.


Right.  I just spoke with our server folks and, currently, our SSH
support quietly eats ALL variables.   So we're safe :-)

I'm starting a conversation with them to pass them thru so we can
be ready for this.  (Assuming we choose to go this way.)

Thanks,
Jeff

Re: [RFC PATCH 0/7] Implement ref namespaces as a ref storage backend

[Richard Maw]

On Thu, Aug 24, 2017 at 06:17:07PM +0200, Michael Haggerty wrote:
> On Sun, Aug 13, 2017 at 9:36 PM, Richard Maw  wrote:
> > [...]
> > Fortunately the pluggable ref backends work provided an easier starting 
> > point.
> 
> :-) I'm glad my years-long obsession is finally yielding fruit.
> 
> First a general comment about the approach...
> 
> I've always thought that a workable "Git with namespaces" would
> probably look more like git worktrees:
> 
> * One main repository holding all of the objects and all of the
> non-pseudo references.
> 
> * One lightweight directory per namespace-view, holding the
> "core.namespace" config and the pseudorefs. HEAD should probably be
> stored in the main repository (?).

I had pondered something like this as a later extension,
since it's an iterable way of finding out which namespaces exist in a repository
and allows you to explicitly say which namespaces are meant to exist
since looking for all (refs/namespaces/[^/]+/).*HEAD
will also return HEADs that were created by requesting a symbolic ref creation.

Given how many of my problems have been because
namespaced head isn't located at ${GITDIR}/HEAD
I'm not sure how this helps.

> Both the main repository and the namespace-view directories would probably be
> bare, though perhaps somebody can think of an application for allowing
> non-bare repositories.

The only application I've come up with so far is:

1.  Your git server stores configuration in git repositories (gitano does).
2.  The configuration is stored in a git namespace (gitano wants to do this).
3.  Instead of making a clone of the namespace, making changes and pushing that
it would be nicer to create a workspace to make the changes in instead.

It's not a particularly strong application,
since normally you'd be administering by doing a clone remotely,
and even if that doesn't work you can do a clone locally and push that.

Similarly you should be able to create a workspace
and check out the namespace's head yourself.

> Even though this scheme implies the need for extra directories, I
> think that it would make it easier to fix a lot of your problems:
> 
> * Each namespace-view could define its own namespace,
> quasi-permanently. You wouldn't have to pass it via the environment.
> (You might even want to *forbid* changing the namespace via the
> environment or command line!) So fetches and pushes from one namespace
> to another would work correctly.
> 
> * There would be one place for each namespace-view's pseudorefs. You
> wouldn't have to squeeze them into a single reference tree.

I'm not sure that un-namespaced pseudorefs are a problem.
At least for the git server use-case you wouldn't typically have them.
It would be nice for it to just work of course.

> * The main repository would know all of the references of all of the
> namespace views, so maintenance tools like `git gc` would work without
> changes.

The same is/will be true for worktrees from walking the gitdirs
stored in the commondir.

> * The namespace-view directories wouldn't be mistakable for full
> repositories, so tools like `git gc` would refuse to run in them. I
> think this would also make it a little bit harder for reference values
> to "leak" from one namespace-view to another.
> 
> * Remote access could use different paths for different
> namespace-views. The externally-visible path need not, of course, be
> the same as the path of the namespace-view's directory.

This talk of worktrees got me thinking about an alternative implementation
where instead of mangling refs to add and remove prefixes
a separate git directory is maintained, like worktrees,
and the namespace handling in the refs backend would be
to create a files (or reftable) backend for the namespace's git directory.

Having a separate namespave-view would be like having a bare worktree,
which would presumably be just the gitfile.

This would require extra tooling to create namespaces
since you can't just create a sub-namespace by copying some refs
and adding a symbolic ref for HEAD.

However since you can't (or couldn't last time I tried to find a way to)
push a symbolic ref, so the git server needs extra tooling anyway.
You would lose the ability to trivially see sub-namespaces by fetching refs,
but the only use I can think of for that is mirroring for backup.

> By the way, there are certainly still places in the code that don't go
> through the refs API (e.g., the one that Junio found). That's because
> the refs API has still not been used for anything very interesting, so
> the bugs haven't been flushed out. I see you've found some more.
> That's because you're doing something interesting :-)

> > [...]
> > Bugs
> > 
> >
> > Most boil down to how special refs like HEAD are handled.
> >
> > 1.  Logged messages display the namespaced path,
> > which a human may deal with but confuses the test suite.
> 
> I think it's clear that the logged messages should reflect the shorter
> reference 

Re: [PATCH v5 35/40] Add Documentation/technical/external-odb.txt

[Ben Peart]

On 8/29/2017 11:43 AM, Christian Couder wrote:

On Mon, Aug 28, 2017 at 8:59 PM, Ben Peart  wrote:


On 8/3/2017 5:19 AM, Christian Couder wrote:


+Helpers
+===
+
+ODB helpers are commands that have to be registered using either the
+"odb..subprocessCommand" or the "odb..scriptCommand"
+config variables.
+
+Registering such a command tells Git that an external odb called
+ exists and that the registered command should be used to
+communicate with it.


What order are the odb handlers called? Are they called before or after the
regular object store code for loose, pack and alternates?  Is the order
configurable?


For get_*_object instructions the regular code is called before the odb helpers.
(So the odb helper code is at the end of stat_sha1_file() and of
open_sha1_file() in sha1_file.c.)

For put_*_object instructions the regular code is called after the odb helpers.
(So the odb helper code is at the beginning of write_sha1_file() in
sha1_file.c.)

And no this order is not configurable, but of course it could be made
configurable.


+ - 'get_direct '
+
+This instruction is similar as the other 'get_*' instructions except
+that no object should be sent from the helper to Git. Instead the
+helper should directly write the requested object into a loose object
+file in the ".git/objects" directory.
+
+After the helper has sent the "status=success" packet and the
+following flush packet in process mode, or after it has exited in the
+script mode, Git should lookup again for a loose object file with the
+requested sha1.


When will git call get_direct vs one of the other get_* functions?


It is called just before exiting when git cannot find an object.
It is not exactly at the same place as other get_* instructions as I
tried to reuse your code and as it looks like it makes it easier to
retry the regular code after the odb helper code.


Could the
functionality of enabling a helper to populate objects into the regular
object store be provided by having a ODB helper that returned the object
data as requested by get_git_obj or get_raw_obj but also stored it in the
regular object store as a loose object (or pack file) for future calls?


I am not sure I understand what you mean.
If a helper returns the object data as requested by get_git_obj or
get_raw_obj, then currently Git will itself store the object locally
in its regular object store, so it is redundant for the helper to also
store or try to store the object in the regular object store.



Doesn't this mean that objects will "leak out" into the regular object 
store as they are used?  For example, at checkout, all objects in the 
requested commit would be retrieved from the various object stores and 
if they came from a "large blob" ODB handler, they would get retrieved 
from the ODB handler and then written to the regular object store 
(presumably as a loose object).  From then on, the object would be 
retrieved from the regular object store.


This would seem to defeat the goal of enabling specialized object 
handlers to handle large or other "unusual" objects that git normally 
doesn't deal well with.

Re: [RFC 5/7] http: send Git-Protocol-Version header

[Kevin Daudt]

On Thu, Aug 24, 2017 at 03:53:26PM -0700, Brandon Williams wrote:
> Tell a serve that protocol v2 can be used by sending an http header
> indicating this.

s/serve/server/

Commit dropped when swapping commits with rebase -i -p

[Sebastian Schuberth]

Hi,

I believe stumbled upon a nasty bug in Git: It looks like a commits gets 
dropped during interactive rebase when asking to preserve merges. Steps:

$ git clone -b git-bug --single-branch 
https://github.com/heremaps/scancode-toolkit.git
$ git rebase -i -p HEAD~2
# In the editor, swap the order of the two (non-merge) commits.
$ git diff origin/git-bug

The last command will show a non-empty diff which looks as if the "Do not 
shadow the os package with a variable name" commit has been dropped, and indeed 
"git log" confirms this. The commit should not get dropped, and it does not if 
just using "git rebase -i -p HEAD~2" (without "-p").

I'm observing this with Git 2.14.1 on Linux.

Regards,
Sebastian

[PATCH] name-rev: change ULONG_MAX to TIME_MAX

[Michael J Gruber]

Earlier, dddbad728c ("timestamp_t: a new data type for timestamps",
2017-04-26) changed several types to timestamp_t.

5589e87fd8 ("name-rev: change a "long" variable to timestamp_t",
2017-05-20) cleaned up a missed variable, but both missed a _MAX
constant.

Change the remaining constant to the one appropriate for the current
type

Signed-off-by: Michael J Gruber 
---
 builtin/name-rev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index c41ea7c2a6..598da6c8bc 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -253,7 +253,7 @@ static int name_ref(const char *path, const struct 
object_id *oid, int flags, vo
struct commit *commit = (struct commit *)o;
int from_tag = starts_with(path, "refs/tags/");
 
-   if (taggerdate == ULONG_MAX)
+   if (taggerdate == TIME_MAX)
taggerdate = ((struct commit *)o)->date;
path = name_ref_abbrev(path, can_abbreviate_output);
name_rev(commit, xstrdup(path), taggerdate, 0, 0,
-- 
2.14.1.603.gf58147c36e

[PATCH 39/39] pack: allow sha1_loose_object_info to handle arbitrary repositories

[Jonathan Nieder]

Signed-off-by: Stefan Beller 
Signed-off-by: Jonathan Nieder 
---
That's the end of the series.  Thanks for reading.

 sha1_file.c | 11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index dc3c0b07ea..0f302a8c48 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1096,10 +1096,9 @@ int parse_sha1_header(const char *hdr, unsigned long 
*sizep)
return parse_sha1_header_extended(hdr, , 0);
 }
 
-#define sha1_loose_object_info(r, s, o, f) sha1_loose_object_info_##r(s, o, f)
-static int sha1_loose_object_info_the_repository(const unsigned char *sha1,
-struct object_info *oi,
-int flags)
+static int sha1_loose_object_info(struct repository *r,
+ const unsigned char *sha1,
+ struct object_info *oi, int flags)
 {
int status = 0;
unsigned long mapsize;
@@ -1123,14 +1122,14 @@ static int sha1_loose_object_info_the_repository(const 
unsigned char *sha1,
if (!oi->typep && !oi->typename && !oi->sizep && !oi->contentp) {
const char *path;
struct stat st;
-   if (stat_sha1_file(the_repository, sha1, , ) < 0)
+   if (stat_sha1_file(r, sha1, , ) < 0)
return -1;
if (oi->disk_sizep)
*oi->disk_sizep = st.st_size;
return 0;
}
 
-   map = map_sha1_file(the_repository, sha1, );
+   map = map_sha1_file(r, sha1, );
if (!map)
return -1;
 
-- 
2.14.1.581.gf28d330327

[PATCH 37/39] pack: allow map_sha1_file_1 to handle arbitrary repositories

[Jonathan Nieder]

Signed-off-by: Stefan Beller 
Signed-off-by: Jonathan Nieder 
---
 sha1_file.c | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 930705f59b..7fc5ebf2af 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -876,10 +876,8 @@ static int open_sha1_file(struct repository *r,
  * Map the loose object at "path" if it is not NULL, or the path found by
  * searching for a loose object named "sha1".
  */
-#define map_sha1_file_1(r, p, s, si) map_sha1_file_1_##r(p, s, si)
-static void *map_sha1_file_1_the_repository(const char *path,
-   const unsigned char *sha1,
-   unsigned long *size)
+static void *map_sha1_file_1(struct repository *r, const char *path,
+const unsigned char *sha1, unsigned long *size)
 {
void *map;
int fd;
@@ -887,7 +885,7 @@ static void *map_sha1_file_1_the_repository(const char 
*path,
if (path)
fd = git_open(path);
else
-   fd = open_sha1_file(the_repository, sha1, );
+   fd = open_sha1_file(r, sha1, );
map = NULL;
if (fd >= 0) {
struct stat st;
-- 
2.14.1.581.gf28d330327

[PATCH 38/39] pack: allow map_sha1_file to handle arbitrary repositories

[Jonathan Nieder]

Signed-off-by: Stefan Beller 
Signed-off-by: Jonathan Nieder 
---
 object-store.h | 3 +--
 sha1_file.c| 5 +++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/object-store.h b/object-store.h
index fe2187fd65..57b8d89738 100644
--- a/object-store.h
+++ b/object-store.h
@@ -61,8 +61,7 @@ struct packed_git {
  * is overwritten each time the function is called.
  */
 extern const char *sha1_file_name(struct repository *r, const unsigned char 
*sha1);
-#define map_sha1_file(r, s, sz) map_sha1_file_##r(s, sz)
-extern void *map_sha1_file_the_repository(const unsigned char *sha1, unsigned 
long *size);
+extern void *map_sha1_file(struct repository *r, const unsigned char *sha1, 
unsigned long *size);
 
 extern void prepare_alt_odb(struct repository *r);
 
diff --git a/sha1_file.c b/sha1_file.c
index 7fc5ebf2af..dc3c0b07ea 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -904,9 +904,10 @@ static void *map_sha1_file_1(struct repository *r, const 
char *path,
return map;
 }
 
-void *map_sha1_file_the_repository(const unsigned char *sha1, unsigned long 
*size)
+void *map_sha1_file(struct repository *r,
+   const unsigned char *sha1, unsigned long *size)
 {
-   return map_sha1_file_1(the_repository, NULL, sha1, size);
+   return map_sha1_file_1(r, NULL, sha1, size);
 }
 
 static int unpack_sha1_short_header(git_zstream *stream,
-- 
2.14.1.581.gf28d330327

[PATCH 36/39] pack: allow open_sha1_file to handle arbitrary repositories

[Jonathan Nieder]

Signed-off-by: Stefan Beller 
Signed-off-by: Jonathan Nieder 
---
 sha1_file.c | 11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 282ed7bd1b..930705f59b 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -846,22 +846,21 @@ static int stat_sha1_file(struct repository *r, const 
unsigned char *sha1,
  * Like stat_sha1_file(), but actually open the object and return the
  * descriptor. See the caveats on the "path" parameter above.
  */
-#define open_sha1_file(r, s, p) open_sha1_file_##r(s, p)
-static int open_sha1_file_the_repository(const unsigned char *sha1,
-const char **path)
+static int open_sha1_file(struct repository *r,
+ const unsigned char *sha1, const char **path)
 {
int fd;
struct alternate_object_database *alt;
int most_interesting_errno;
 
-   *path = sha1_file_name(the_repository, sha1);
+   *path = sha1_file_name(r, sha1);
fd = git_open(*path);
if (fd >= 0)
return fd;
most_interesting_errno = errno;
 
-   prepare_alt_odb(the_repository);
-   for (alt = the_repository->objects.alt_odb_list; alt; alt = alt->next) {
+   prepare_alt_odb(r);
+   for (alt = r->objects.alt_odb_list; alt; alt = alt->next) {
*path = alt_sha1_path(alt, sha1);
fd = git_open(*path);
if (fd >= 0)
-- 
2.14.1.581.gf28d330327

[PATCH 35/39] pack: allow stat_sha1_file to handle arbitrary repositories

[Jonathan Nieder]

Signed-off-by: Stefan Beller 
Signed-off-by: Jonathan Nieder 
---
 sha1_file.c | 11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index ac57eea0f2..282ed7bd1b 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -822,19 +822,18 @@ int git_open_cloexec(const char *name, int flags)
  * Note that it may point to static storage and is only valid until another
  * call to sha1_file_name(), etc.
  */
-#define stat_sha1_file(r, s, st, p) stat_sha1_file_##r(s, st, p)
-static int stat_sha1_file_the_repository(const unsigned char *sha1,
-struct stat *st, const char **path)
+static int stat_sha1_file(struct repository *r, const unsigned char *sha1,
+ struct stat *st, const char **path)
 {
struct alternate_object_database *alt;
 
-   *path = sha1_file_name(the_repository, sha1);
+   *path = sha1_file_name(r, sha1);
if (!lstat(*path, st))
return 0;
 
-   prepare_alt_odb(the_repository);
+   prepare_alt_odb(r);
errno = ENOENT;
-   for (alt = the_repository->objects.alt_odb_list; alt; alt = alt->next) {
+   for (alt = r->objects.alt_odb_list; alt; alt = alt->next) {
*path = alt_sha1_path(alt, sha1);
if (!lstat(*path, st))
return 0;
-- 
2.14.1.581.gf28d330327

[PATCH 34/39] pack: allow sha1_file_name to handle arbitrary repositories

[Jonathan Nieder]

Signed-off-by: Stefan Beller 
Signed-off-by: Jonathan Nieder 
---
 object-store.h | 3 +--
 sha1_file.c| 4 ++--
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/object-store.h b/object-store.h
index 518da80367..fe2187fd65 100644
--- a/object-store.h
+++ b/object-store.h
@@ -60,8 +60,7 @@ struct packed_git {
  * The return value is a pointer to a statically allocated buffer that
  * is overwritten each time the function is called.
  */
-#define sha1_file_name(r, s) sha1_file_name_##r(s)
-extern const char *sha1_file_name_the_repository(const unsigned char *sha1);
+extern const char *sha1_file_name(struct repository *r, const unsigned char 
*sha1);
 #define map_sha1_file(r, s, sz) map_sha1_file_##r(s, sz)
 extern void *map_sha1_file_the_repository(const unsigned char *sha1, unsigned 
long *size);
 
diff --git a/sha1_file.c b/sha1_file.c
index 81bc1ab309..ac57eea0f2 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -254,12 +254,12 @@ static void fill_sha1_path(struct strbuf *buf, const 
unsigned char *sha1)
}
 }
 
-const char *sha1_file_name_the_repository(const unsigned char *sha1)
+const char *sha1_file_name(struct repository *r, const unsigned char *sha1)
 {
static struct strbuf buf = STRBUF_INIT;
 
strbuf_reset();
-   strbuf_addf(, "%s/", get_object_directory());
+   strbuf_addf(, "%s/", r->objectdir);
 
fill_sha1_path(, sha1);
return buf.buf;
-- 
2.14.1.581.gf28d330327

[PATCH 32/39] pack: allow prepare_packed_git to handle arbitrary repositories

[Jonathan Nieder]

Signed-off-by: Stefan Beller 
Signed-off-by: Jonathan Nieder 
---
 packfile.c | 18 +-
 packfile.h |  3 +--
 2 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/packfile.c b/packfile.c
index 4652be0b6e..23a835f7fb 100644
--- a/packfile.c
+++ b/packfile.c
@@ -865,19 +865,19 @@ static void prepare_packed_git_mru(struct repository *r)
mru_append(>objects.packed_git_mru, p);
 }
 
-void prepare_packed_git_the_repository(void)
+void prepare_packed_git(struct repository *r)
 {
struct alternate_object_database *alt;
 
-   if (the_repository->objects.packed_git_initialized)
+   if (r->objects.packed_git_initialized)
return;
-   prepare_packed_git_one(the_repository, get_object_directory(), 1);
-   prepare_alt_odb(the_repository);
-   for (alt = the_repository->objects.alt_odb_list; alt; alt = alt->next)
-   prepare_packed_git_one(the_repository, alt->path, 0);
-   rearrange_packed_git(the_repository);
-   prepare_packed_git_mru(the_repository);
-   the_repository->objects.packed_git_initialized = 1;
+   prepare_packed_git_one(r, r->objectdir, 1);
+   prepare_alt_odb(r);
+   for (alt = r->objects.alt_odb_list; alt; alt = alt->next)
+   prepare_packed_git_one(r, alt->path, 0);
+   rearrange_packed_git(r);
+   prepare_packed_git_mru(r);
+   r->objects.packed_git_initialized = 1;
 }
 
 void reprepare_packed_git_the_repository(void)
diff --git a/packfile.h b/packfile.h
index ba6f08be99..75be3cb877 100644
--- a/packfile.h
+++ b/packfile.h
@@ -32,8 +32,7 @@ extern struct packed_git *parse_pack_index(unsigned char 
*sha1, const char *idx_
 #define PACKDIR_FILE_GARBAGE 4
 extern void (*report_garbage)(unsigned seen_bits, const char *path);
 
-#define prepare_packed_git(r) prepare_packed_git_##r()
-extern void prepare_packed_git_the_repository(void);
+extern void prepare_packed_git(struct repository *r);
 #define reprepare_packed_git(r) reprepare_packed_git_##r()
 extern void reprepare_packed_git_the_repository(void);
 extern void install_packed_git(struct repository *r, struct packed_git *pack);
-- 
2.14.1.581.gf28d330327

[PATCH 33/39] pack: allow reprepare_packed_git to handle arbitrary repositories

[Jonathan Nieder]

Signed-off-by: Stefan Beller 
Signed-off-by: Jonathan Nieder 
---
 packfile.c | 8 
 packfile.h | 3 +--
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/packfile.c b/packfile.c
index 23a835f7fb..67274d01fd 100644
--- a/packfile.c
+++ b/packfile.c
@@ -880,11 +880,11 @@ void prepare_packed_git(struct repository *r)
r->objects.packed_git_initialized = 1;
 }
 
-void reprepare_packed_git_the_repository(void)
+void reprepare_packed_git(struct repository *r)
 {
-   the_repository->objects.approximate_object_count_valid = 0;
-   the_repository->objects.packed_git_initialized = 0;
-   prepare_packed_git(the_repository);
+   r->objects.approximate_object_count_valid = 0;
+   r->objects.packed_git_initialized = 0;
+   prepare_packed_git(r);
 }
 
 unsigned long unpack_object_header_buffer(const unsigned char *buf,
diff --git a/packfile.h b/packfile.h
index 75be3cb877..b3138816e7 100644
--- a/packfile.h
+++ b/packfile.h
@@ -33,8 +33,7 @@ extern struct packed_git *parse_pack_index(unsigned char 
*sha1, const char *idx_
 extern void (*report_garbage)(unsigned seen_bits, const char *path);
 
 extern void prepare_packed_git(struct repository *r);
-#define reprepare_packed_git(r) reprepare_packed_git_##r()
-extern void reprepare_packed_git_the_repository(void);
+extern void reprepare_packed_git(struct repository *r);
 extern void install_packed_git(struct repository *r, struct packed_git *pack);
 
 /*
-- 
2.14.1.581.gf28d330327

[PATCH 30/39] pack: allow prepare_packed_git_mru to handle arbitrary repositories

[Jonathan Nieder]

Signed-off-by: Stefan Beller 
Signed-off-by: Jonathan Nieder 
---
 packfile.c | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/packfile.c b/packfile.c
index 977e714d9a..9bb93ce256 100644
--- a/packfile.c
+++ b/packfile.c
@@ -857,14 +857,13 @@ static void rearrange_packed_git(struct repository *r)
set_next_packed_git, sort_pack);
 }
 
-#define prepare_packed_git_mru(r) prepare_packed_git_mru_##r()
-static void prepare_packed_git_mru_the_repository(void)
+static void prepare_packed_git_mru(struct repository *r)
 {
struct packed_git *p;
 
-   mru_clear(_repository->objects.packed_git_mru);
-   for (p = the_repository->objects.packed_git; p; p = p->next)
-   mru_append(_repository->objects.packed_git_mru, p);
+   mru_clear(>objects.packed_git_mru);
+   for (p = r->objects.packed_git; p; p = p->next)
+   mru_append(>objects.packed_git_mru, p);
 }
 
 void prepare_packed_git_the_repository(void)
-- 
2.14.1.581.gf28d330327

[PATCH 31/39] pack: allow prepare_packed_git_one to handle arbitrary repositories

[Jonathan Nieder]

Signed-off-by: Stefan Beller 
Signed-off-by: Jonathan Nieder 
---
 packfile.c | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/packfile.c b/packfile.c
index 9bb93ce256..4652be0b6e 100644
--- a/packfile.c
+++ b/packfile.c
@@ -719,8 +719,8 @@ static void report_pack_garbage(struct string_list *list)
report_helper(list, seen_bits, first, list->nr);
 }
 
-#define prepare_packed_git_one(r, o, l) prepare_packed_git_one_##r(o, l)
-static void prepare_packed_git_one_the_repository(char *objdir, int local)
+static void prepare_packed_git_one(struct repository *r,
+  char *objdir, int local)
 {
struct strbuf path = STRBUF_INIT;
size_t dirnamelen;
@@ -753,8 +753,7 @@ static void prepare_packed_git_one_the_repository(char 
*objdir, int local)
base_len = path.len;
if (strip_suffix_mem(path.buf, _len, ".idx")) {
/* Don't reopen a pack we already have. */
-   for (p = the_repository->objects.packed_git; p;
-p = p->next) {
+   for (p = r->objects.packed_git; p; p = p->next) {
size_t len;
if (strip_suffix(p->pack_name, ".pack", ) &&
len == base_len &&
@@ -767,7 +766,7 @@ static void prepare_packed_git_one_the_repository(char 
*objdir, int local)
 * corresponding .pack file that we can map.
 */
(p = add_packed_git(path.buf, path.len, local)) != 
NULL)
-   install_packed_git(the_repository, p);
+   install_packed_git(r, p);
}
 
if (!report_garbage)
-- 
2.14.1.581.gf28d330327

[PATCH 28/39] pack: allow install_packed_git to handle arbitrary repositories

[Jonathan Nieder]

Signed-off-by: Stefan Beller 
Signed-off-by: Jonathan Nieder 
---
 packfile.c | 6 +++---
 packfile.h | 3 +--
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/packfile.c b/packfile.c
index 86c3964018..51af035965 100644
--- a/packfile.c
+++ b/packfile.c
@@ -664,13 +664,13 @@ struct packed_git *add_packed_git(const char *path, 
size_t path_len, int local)
return p;
 }
 
-void install_packed_git_the_repository(struct packed_git *pack)
+void install_packed_git(struct repository *r, struct packed_git *pack)
 {
if (pack->pack_fd != -1)
pack_open_fds++;
 
-   pack->next = the_repository->objects.packed_git;
-   the_repository->objects.packed_git = pack;
+   pack->next = r->objects.packed_git;
+   r->objects.packed_git = pack;
 }
 
 void (*report_garbage)(unsigned seen_bits, const char *path);
diff --git a/packfile.h b/packfile.h
index 74f293c439..ba6f08be99 100644
--- a/packfile.h
+++ b/packfile.h
@@ -36,8 +36,7 @@ extern void (*report_garbage)(unsigned seen_bits, const char 
*path);
 extern void prepare_packed_git_the_repository(void);
 #define reprepare_packed_git(r) reprepare_packed_git_##r()
 extern void reprepare_packed_git_the_repository(void);
-#define install_packed_git(r, p) install_packed_git_##r(p)
-extern void install_packed_git_the_repository(struct packed_git *pack);
+extern void install_packed_git(struct repository *r, struct packed_git *pack);
 
 /*
  * Give a rough count of objects in the repository. This sacrifices accuracy
-- 
2.14.1.581.gf28d330327

[PATCH 29/39] pack: allow rearrange_packed_git to handle arbitrary repositories

[Jonathan Nieder]

Signed-off-by: Stefan Beller 
Signed-off-by: Jonathan Nieder 
---
 packfile.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/packfile.c b/packfile.c
index 51af035965..977e714d9a 100644
--- a/packfile.c
+++ b/packfile.c
@@ -850,11 +850,10 @@ static int sort_pack(const void *a_, const void *b_)
return -1;
 }
 
-#define rearrange_packed_git(r) rearrange_packed_git_##r()
-static void rearrange_packed_git_the_repository(void)
+static void rearrange_packed_git(struct repository *r)
 {
-   the_repository->objects.packed_git = llist_mergesort(
-   the_repository->objects.packed_git, get_next_packed_git,
+   r->objects.packed_git = llist_mergesort(
+   r->objects.packed_git, get_next_packed_git,
set_next_packed_git, sort_pack);
 }
 
-- 
2.14.1.581.gf28d330327

[PATCH 27/39] object-store: allow foreach_alt_odb to handle arbitrary repositories

[Jonathan Nieder]

From: Stefan Beller 

Signed-off-by: Stefan Beller 
Signed-off-by: Jonathan Nieder 
---
 object-store.h |  3 +--
 sha1_file.c| 14 +++---
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/object-store.h b/object-store.h
index fcf36c7370..518da80367 100644
--- a/object-store.h
+++ b/object-store.h
@@ -68,7 +68,6 @@ extern void *map_sha1_file_the_repository(const unsigned char 
*sha1, unsigned lo
 extern void prepare_alt_odb(struct repository *r);
 
 typedef int alt_odb_fn(struct alternate_object_database *, void *);
-#define foreach_alt_odb(r, fn, cb) foreach_alt_odb_##r(fn, cb)
-extern int foreach_alt_odb_the_repository(alt_odb_fn, void*);
+extern int foreach_alt_odb(struct repository *r, alt_odb_fn, void*);
 
 #endif /* OBJECT_STORE_H */
diff --git a/sha1_file.c b/sha1_file.c
index 452c8f0bbd..81bc1ab309 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -604,18 +604,18 @@ char *compute_alternate_path(const char *path, struct 
strbuf *err)
return ref_git;
 }
 
-int foreach_alt_odb_the_repository(alt_odb_fn fn, void *cb)
+int foreach_alt_odb(struct repository *r, alt_odb_fn fn, void *cb)
 {
struct alternate_object_database *ent;
-   int r = 0;
+   int ret = 0;
 
-   prepare_alt_odb(the_repository);
-   for (ent = the_repository->objects.alt_odb_list; ent; ent = ent->next) {
-   r = fn(ent, cb);
-   if (r)
+   prepare_alt_odb(r);
+   for (ent = r->objects.alt_odb_list; ent; ent = ent->next) {
+   ret = fn(ent, cb);
+   if (ret)
break;
}
-   return r;
+   return ret;
 }
 
 void prepare_alt_odb(struct repository *r)
-- 
2.14.1.581.gf28d330327

[PATCH 26/39] object-store: allow prepare_alt_odb to handle arbitrary repositories

[Jonathan Nieder]

From: Stefan Beller 

Signed-off-by: Jonathan Nieder 
Signed-off-by: Stefan Beller 
---
 cache.h|  4 
 object-store.h |  3 +--
 sha1_file.c| 65 ++
 3 files changed, 39 insertions(+), 33 deletions(-)

diff --git a/cache.h b/cache.h
index 454cfb50c4..d204048d3d 100644
--- a/cache.h
+++ b/cache.h
@@ -1512,6 +1512,10 @@ struct alternate_object_database {
char loose_objects_subdir_seen[256];
struct oid_array loose_objects_cache;
 
+   /*
+* Path to the alternate object database, relative to the
+* current working directory.
+*/
char path[FLEX_ARRAY];
 };
 extern char *compute_alternate_path(const char *path, struct strbuf *err);
diff --git a/object-store.h b/object-store.h
index 92fbf4a9ca..fcf36c7370 100644
--- a/object-store.h
+++ b/object-store.h
@@ -65,8 +65,7 @@ extern const char *sha1_file_name_the_repository(const 
unsigned char *sha1);
 #define map_sha1_file(r, s, sz) map_sha1_file_##r(s, sz)
 extern void *map_sha1_file_the_repository(const unsigned char *sha1, unsigned 
long *size);
 
-#define prepare_alt_odb(r) prepare_alt_odb_##r()
-extern void prepare_alt_odb_the_repository(void);
+extern void prepare_alt_odb(struct repository *r);
 
 typedef int alt_odb_fn(struct alternate_object_database *, void *);
 #define foreach_alt_odb(r, fn, cb) foreach_alt_odb_##r(fn, cb)
diff --git a/sha1_file.c b/sha1_file.c
index b854cad970..452c8f0bbd 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -326,11 +326,10 @@ static int alt_odb_usable(struct repository *r, struct 
strbuf *path,
  * SHA1, an extra slash for the first level indirection, and the
  * terminating NUL.
  */
-#define read_info_alternates(r, rb, d) read_info_alternates_##r(rb, d)
-static void read_info_alternates_the_repository(const char *relative_base,
-   int depth);
-#define link_alt_odb_entry(r, e, rb, d, n) link_alt_odb_entry_##r(e, rb, d, n)
-static int link_alt_odb_entry_the_repository(const char *entry,
+static void read_info_alternates(struct repository *r,
+const char *relative_base,
+int depth);
+static int link_alt_odb_entry(struct repository *r, const char *entry,
const char *relative_base, int depth, const char *normalized_objdir)
 {
struct alternate_object_database *ent;
@@ -356,7 +355,7 @@ static int link_alt_odb_entry_the_repository(const char 
*entry,
while (pathbuf.len && pathbuf.buf[pathbuf.len - 1] == '/')
strbuf_setlen(, pathbuf.len - 1);
 
-   if (!alt_odb_usable(the_repository, , normalized_objdir)) {
+   if (!alt_odb_usable(r, , normalized_objdir)) {
strbuf_release();
return -1;
}
@@ -364,12 +363,12 @@ static int link_alt_odb_entry_the_repository(const char 
*entry,
ent = alloc_alt_odb(pathbuf.buf);
 
/* add the alternate entry */
-   *the_repository->objects.alt_odb_tail = ent;
-   the_repository->objects.alt_odb_tail = &(ent->next);
+   *r->objects.alt_odb_tail = ent;
+   r->objects.alt_odb_tail = &(ent->next);
ent->next = NULL;
 
/* recursively add alternates */
-   read_info_alternates(the_repository, pathbuf.buf, depth + 1);
+   read_info_alternates(r, pathbuf.buf, depth + 1);
 
strbuf_release();
return 0;
@@ -404,12 +403,8 @@ static const char *parse_alt_odb_entry(const char *string,
return end;
 }
 
-#define link_alt_odb_entries(r, a, l, s, rb, d) \
-   link_alt_odb_entries_##r(a, l, s, rb, d)
-static void link_alt_odb_entries_the_repository(const char *alt, int len,
-   int sep,
-   const char *relative_base,
-   int depth)
+static void link_alt_odb_entries(struct repository *r, const char *alt, int 
len,
+int sep, const char *relative_base, int depth)
 {
struct strbuf objdirbuf = STRBUF_INIT;
struct strbuf entry = STRBUF_INIT;
@@ -420,7 +415,7 @@ static void link_alt_odb_entries_the_repository(const char 
*alt, int len,
return;
}
 
-   strbuf_add_absolute_path(, get_object_directory());
+   strbuf_add_absolute_path(, r->objectdir);
if (strbuf_normalize_path() < 0)
die("unable to normalize object directory: %s",
objdirbuf.buf);
@@ -429,15 +424,16 @@ static void link_alt_odb_entries_the_repository(const 
char *alt, int len,
alt = parse_alt_odb_entry(alt, sep, );
if (!entry.len)
continue;
-   link_alt_odb_entry(the_repository, entry.buf,
+   link_alt_odb_entry(r, entry.buf,
   relative_base, depth,