[PATCH 10/39] sha1_file: add repository argument to link_alt_odb_entries

2017-08-29 Thread Jonathan Nieder
From: Stefan Beller Add a repository argument to allow the link_alt_odb_entries 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. As with the previous commits,

[PATCH 09/39] sha1_file: add repository argument to read_info_alternates

2017-08-29 Thread Jonathan Nieder
From: Stefan Beller Add a repository argument to allow the read_info_alternates caller to be more specific about which repository to read from. This is a small mechanical change; it doesn't change the implementation to handle repositories other than the_repository yet. As with the previous commi

[PATCH 08/39] sha1_file: add repository argument to link_alt_odb_entry

2017-08-29 Thread Jonathan Nieder
From: Stefan Beller Add a repository argument to allow the link_alt_odb_entry 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. As with the previous commit, us

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

2017-08-29 Thread Jonathan Nieder
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 y

[PATCH 06/39] pack: move approximate object count to object store

2017-08-29 Thread Jonathan Nieder
The approximate_object_count() function maintains a rough count of objects in a repository to estimate how long object name abbreviates should be. Object names are scoped to a repository and the appropriate length may differ by repository, so the object count should not be global. Signed-off-by:

[PATCH 04/39] object-store: move packed_git and packed_git_mru to object store

2017-08-29 Thread Jonathan Nieder
In a process with multiple repositories open, packfile accessors should be associated to a single repository and not shared globally. Move packed_git and packed_git_mru into the_repository and adjust callers to reflect this. Patch generated by 1. Moving the struct packed_git declaration to objec

[PATCH 05/39] pack: move prepare_packed_git_run_once to object store

2017-08-29 Thread Jonathan Nieder
Each repository's object store can be initialized independently, so they must not share a run_once variable. Signed-off-by: Stefan Beller Signed-off-by: Jonathan Nieder --- object-store.h | 8 +++- packfile.c | 7 +++ 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/o

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

2017-08-29 Thread Michael Haggerty
On 08/30/2017 06:55 AM, Jeff King wrote: > [...] > Something like this, which AFAICT is about as safe as the existing code > in its list manipulation. It retains the "active" flag as an additional > check which I think isn't strictly necessary, but potentially catches > some logic errors. > > The

[PATCH 03/39] object-store: move alt_odb_list and alt_odb_tail to object store

2017-08-29 Thread Jonathan Nieder
From: Stefan Beller In a process with multiple repositories open, alternates should be associated to a single repository and not shared globally. Move alt_odb_list and alt_odb_tail into the_repository and adjust callers to reflect this. No functional change intended. Signed-off-by: Stefan Belle

[PATCH 02/39] repository: introduce object store field

2017-08-29 Thread Jonathan Nieder
From: Stefan Beller The object store field will contain any objects needed for access to objects in a given repository. This patch introduces the object store but for now it is empty. C99 forbids empty structs, but common C compilers cope well with them and this struct will gain members very so

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

2017-08-29 Thread Jonathan Nieder
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 = &packed_git_mru_storage; Callers never assign to the packed_git_mru pointer, though, so we can simplify by eliminating i

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

2017-08-29 Thread Jonathan Nieder
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 sub

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

2017-08-29 Thread Jeff King
On Wed, Aug 30, 2017 at 12:55:55AM -0400, Jeff King wrote: > > I feel like right now we meet (1) and not (2). But I think if we keep to > > that lower bar of (1), it might not be that bad. We're assuming now that > > there's no race on the tempfile->active flag, for instance. We could > > probably

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

2017-08-29 Thread Jeff King
On Tue, Aug 29, 2017 at 05:48:09PM -0400, Jeff King wrote: > On Tue, Aug 29, 2017 at 09:22:08PM +0200, Martin Ågren wrote: > > > One take-away for me from this thread is that memory-leak-plugging seems > > to be appreciated, i.e., it's not just an academic problem. I think I'll > > look into it s

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

2017-08-29 Thread Jeff King
On Wed, Aug 30, 2017 at 12:31:47AM -0400, Jeff King wrote: > > It was surprisingly hard trying to get that code to do the right thing, > > non-racily, in every error path. Since `remove_tempfiles()` can be > > called any time (even from a signal handler), the linked list of > > `tempfile` objects

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

2017-08-29 Thread Michael Haggerty
On Wed, Aug 30, 2017 at 6:31 AM, Jeff King wrote: > On Wed, Aug 30, 2017 at 05:25:18AM +0200, Michael Haggerty wrote: >> It was surprisingly hard trying to get that code to do the right thing, >> non-racily, in every error path. Since `remove_tempfiles()` can be >> called any time (even from a sig

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

2017-08-29 Thread Jeff King
On Wed, Aug 30, 2017 at 05:25:18AM +0200, Michael Haggerty wrote: > >>> In the long run we may want to drop the "tempfiles must remain forever" > >>> rule. This is certainly not the first time it has caused confusion or > >>> leaks. And I don't think it's a fundamental issue, just the way the code

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

2017-08-29 Thread Michael Haggerty
On 08/29/2017 09:12 PM, Jeff King wrote: > On Tue, Aug 29, 2017 at 12:09:28PM -0700, Brandon Williams wrote: > >>> -- >8 -- >>> Subject: [PATCH] config: use a static lock_file struct >>> >>> When modifying git config, we xcalloc() a struct lock_file >>> but never free it. This is necessary because

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

2017-08-29 Thread Jeff King
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://" prot

Re: [PATCH 04/10] packed_delete_refs(): implement method

2017-08-29 Thread Michael Haggerty
On 08/29/2017 08:07 PM, Brandon Williams wrote: > On 08/29, Michael Haggerty wrote: >> [...] >> diff --git a/refs/packed-backend.c b/refs/packed-backend.c >> index d19b3bfba5..83a088118f 100644 >> --- a/refs/packed-backend.c >> +++ b/refs/packed-backend.c >> @@ -1082,7 +1082,50 @@ static int packed

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

2017-08-29 Thread Michael Haggerty
v3 looks good to me. Thanks! Reviewed-by: Michael Haggerty Michael

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

2017-08-29 Thread Jeff King
On Tue, Aug 29, 2017 at 09:22:08PM +0200, Martin Ågren wrote: > One take-away for me from this thread is that memory-leak-plugging seems > to be appreciated, i.e., it's not just an academic problem. I think I'll > look into it some more, i.e., slowly pursue option 2. :-) That might help > give som

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

2017-08-29 Thread Brandon Williams
On 08/29, Jeff Hostetler wrote: > > > On 8/25/2017 1:35 PM, Jonathan Nieder wrote: > >Hi, > > > >Jeff King wrote: > >>On Thu, Aug 24, 2017 at 03:53:21PM -0700, Brandon Williams wrote: > > > >>>Another version of Git's wire protocol is a topic that has been discussed > >>>and > >>>attempted by ma

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

2017-08-29 Thread Jeff Hostetler
On 8/25/2017 1:35 PM, Jonathan Nieder wrote: Hi, Jeff King wrote: On Thu, Aug 24, 2017 at 03:53:21PM -0700, Brandon Williams wrote: Another version of Git's wire protocol is a topic that has been discussed and attempted by many in the community over the years. The biggest challenge, as fa

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

2017-08-29 Thread Martin Ågren
On 29 August 2017 at 20:53, Jeff King wrote: > On Tue, Aug 29, 2017 at 06:51:52PM +0100, Lars Schneider wrote: >> What if we run a few selected tests with valgrind and count all files that >> valgrind mentions (a single leak has multiple file mentions because of >> the stack trace and other leak i

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

2017-08-29 Thread Jeff King
On Tue, Aug 29, 2017 at 12:09:28PM -0700, Brandon Williams wrote: > > -- >8 -- > > Subject: [PATCH] config: use a static lock_file struct > > > > When modifying git config, we xcalloc() a struct lock_file > > but never free it. This is necessary because the tempfile > > code (upon which the locki

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

2017-08-29 Thread Brandon Williams
On 08/29, Brandon Williams wrote: > On 08/29, Jeff King wrote: > > On Tue, Aug 29, 2017 at 02:53:41PM -0400, Jeff King wrote: > > > > > It looks like the config code has a minor-ish leak. Patch to follow. > > > > Here it is. > > > > -- >8 -- > > Subject: [PATCH] config: use a static lock_file st

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

2017-08-29 Thread Brandon Williams
On 08/29, Jeff King wrote: > On Tue, Aug 29, 2017 at 02:53:41PM -0400, Jeff King wrote: > > > It looks like the config code has a minor-ish leak. Patch to follow. > > Here it is. > > -- >8 -- > Subject: [PATCH] config: use a static lock_file struct > > When modifying git config, we xcalloc() a

[PATCH] config: use a static lock_file struct

2017-08-29 Thread Jeff King
On Tue, Aug 29, 2017 at 02:53:41PM -0400, Jeff King wrote: > It looks like the config code has a minor-ish leak. Patch to follow. Here it is. -- >8 -- Subject: [PATCH] config: use a static lock_file struct When modifying git config, we xcalloc() a struct lock_file but never free it. This is nec

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

2017-08-29 Thread Jeff King
On Tue, Aug 29, 2017 at 06:51:52PM +0100, Lars Schneider wrote: > I set $TOOL_OPTIONS in valgrind.sh: to > '--leak-check=full --errors-for-leak-kinds=definite' > > ... but I also had to adjust t/test-lib-functions.sh:test_create_repo > as I ran into the error "cannot run git init -- have you bui

Re: [PATCH 00/10] Implement transactions for the packed ref store

2017-08-29 Thread Brandon Williams
On 08/29, Michael Haggerty wrote: > This should be the second-to-last patch series in the quest for > mmapped packed-refs. > > Before this patch series, there is still more coupling than necessary > between `files_ref_store` and `packed_ref_store`: > > * `files_ref_store` adds packed references (

Re: [PATCH 09/10] packed-backend: rip out some now-unused code

2017-08-29 Thread Brandon Williams
On 08/29, Michael Haggerty wrote: > Now the outside world interacts with the packed ref store only via the > generic refs API plus a few lock-related functions. This allows us to > delete some functions that are no longer used, thereby completing the > encapsulation of the packed ref store. Love s

Re: [PATCH 04/10] packed_delete_refs(): implement method

2017-08-29 Thread Brandon Williams
On 08/29, Michael Haggerty wrote: > Implement `packed_delete_refs()` using a reference transaction. This > means that `files_delete_refs()` can use `refs_delete_refs()` instead > of `repack_without_refs()` to delete any packed references, decreasing > the coupling between the classes. > > Signed-o

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

2017-08-29 Thread Lars Schneider
> On 28 Aug 2017, at 06:11, Martin Ågren wrote: > > On 28 August 2017 at 01:23, Jeff King wrote: >> On Sun, Aug 27, 2017 at 10:04:55PM +0200, Lars Schneider wrote: >> >>> I did run all tests under valgrind using "make valgrind" and I found >>> the following files with potential issues: >>> >>

[PATCH v3 3/3] refs/files-backend: correct return value in lock_ref_for_update

2017-08-29 Thread Martin Ågren
In one code path we return a literal -1 and not a symbolic constant. The value -1 would be interpreted as TRANSACTION_NAME_CONFLICT, which is wrong. Use TRANSACTION_GENERIC_ERROR instead (that is the only other return value we have to choose from). Noticed-by: Michael Haggerty Signed-off-by: Mart

[PATCH v3 2/3] refs/files-backend: fix memory leak in lock_ref_for_update

2017-08-29 Thread Martin Ågren
After the previous patch, none of the functions we call hold on to `referent.buf`, so we can safely release the string buffer before returning. Signed-off-by: Martin Ågren --- v3: identical to v2 refs/files-backend.c | 31 --- 1 file changed, 20 insertions(+), 11 del

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

2017-08-29 Thread Martin Ågren
split_symref_update() receives a string-pointer `referent` and adds it to the list of `affected_refnames`. The list simply holds on to the pointers it is given, it does not copy the strings and it does not ever free them. The `referent` string in split_symref_update() belongs to a string buffer in

Re: [PATCH] diff-highlight: Add clean target to Makefile

2017-08-29 Thread Jeff King
On Tue, Aug 29, 2017 at 12:23:11PM +0100, Daniel Watkins wrote: > Now that `make` produces a file, we should have a clean target to remove > it. > [...] > +clean: > + $(RM) diff-highlight > + Makes sense. Thanks! -Peff

Re: [PATCH 2/3] merge-recursive: remove return value from get_files_dirs

2017-08-29 Thread Jeff King
On Tue, Aug 29, 2017 at 03:58:00PM +, Kevin Willford wrote: > > > The return value of the get_files_dirs call is never being used. > > > Looking at the history of the file and it was originally only > > > being used for debug output statements. Also when > > > read_tree_recursive return value

RE: [PATCH 2/3] merge-recursive: remove return value from get_files_dirs

2017-08-29 Thread Kevin Willford
> > On Mon, Aug 28, 2017 at 02:28:28PM -0600, Kevin Willford wrote: > > > The return value of the get_files_dirs call is never being used. > > Looking at the history of the file and it was originally only > > being used for debug output statements. Also when > > read_tree_recursive return value

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

2017-08-29 Thread Christian Couder
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. >> + >> +Regist

Re: git describe and "the smallest number of commits possible"

2017-08-29 Thread Michael J Gruber
Stefan Beller venit, vidit, dixit 28.08.2017 20:24: > On Sat, Aug 26, 2017 at 7:47 AM, Kévin Le Gouguec > wrote: >> Hi, >> >> I've asked this question on the git-users Google Groups list[1], and >> while the answers there were interesting, I still cannot figure >> whether my problem comes from an

[PATCH] diff-highlight: Add clean target to Makefile

2017-08-29 Thread Daniel Watkins
Now that `make` produces a file, we should have a clean target to remove it. Signed-off-by: Daniel Watkins --- contrib/diff-highlight/Makefile | 3 +++ 1 file changed, 3 insertions(+) diff --git a/contrib/diff-highlight/Makefile b/contrib/diff-highlight/Makefile index fbf5c5824..f2be7cc92 10064

Re: [PATCH v2 2/2] refs/files-backend: fix memory leak in lock_ref_for_update

2017-08-29 Thread Martin Ågren
On 29 August 2017 at 10:39, Michael Haggerty wrote: > On 08/28/2017 10:32 PM, Martin Ågren wrote: >> After the previous patch, none of the functions we call hold on to >> `referent.buf`, so we can safely release the string buffer before >> returning. > > This patch looks good to me, but I did noti

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

2017-08-29 Thread Christian Couder
On Fri, Aug 25, 2017 at 11:23 PM, Jonathan Tan wrote: > On Fri, 25 Aug 2017 08:14:08 +0200 > Christian Couder wrote: > >> As Git is used by more and more by people having different needs, I >> think it is not realistic to expect that we can optimize its object >> storage for all these different n

Re: [PATCH 3/3] merge-recursive: change current file dir string_lists to hashmap

2017-08-29 Thread Jeff King
On Mon, Aug 28, 2017 at 02:28:29PM -0600, Kevin Willford wrote: > The code was using two string_lists, one for the directories and > one for the files. The code never checks the lists independently > so we should be able to only use one list. The string_list also > is a O(log n) for lookup and i

Re: [PATCH v2 2/2] refs/files-backend: fix memory leak in lock_ref_for_update

2017-08-29 Thread Michael Haggerty
On 08/28/2017 10:32 PM, Martin Ågren wrote: > After the previous patch, none of the functions we call hold on to > `referent.buf`, so we can safely release the string buffer before > returning. This patch looks good to me, but I did notice a pre-existing problem in the area... > --- > refs/files

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

2017-08-29 Thread Michael Haggerty
On 08/28/2017 10:32 PM, Martin Ågren wrote: > split_symref_update() receives a string-pointer `referent` and adds it > to the list of `affected_refnames`. The list simply holds on to the > pointers it is given, it does not copy the strings and it never frees > them. The `referent` string in split_s

[PATCH 03/10] packed_ref_store: implement reference transactions

2017-08-29 Thread Michael Haggerty
Implement the methods needed to support reference transactions for the packed-refs backend. The new methods are not yet used. Signed-off-by: Michael Haggerty --- refs/packed-backend.c | 313 +- refs/packed-backend.h | 9 ++ 2 files changed, 319 i

[PATCH 02/10] struct ref_transaction: add a place for backends to store data

2017-08-29 Thread Michael Haggerty
`packed_ref_store` is going to want to store some transaction-wide data, so make a place for it. Signed-off-by: Michael Haggerty --- refs/refs-internal.h | 1 + 1 file changed, 1 insertion(+) diff --git a/refs/refs-internal.h b/refs/refs-internal.h index b02dc5a7e3..d7d344de73 100644 --- a/refs

[PATCH 10/10] files_transaction_finish(): delete reflogs before references

2017-08-29 Thread Michael Haggerty
If the deletion steps unexpectedly fail, it is less bad to leave a reference without its reflog than it is to leave a reflog without its reference, since the latter is an invalid repository state. Signed-off-by: Michael Haggerty --- refs/files-backend.c | 35 +--

[PATCH 09/10] packed-backend: rip out some now-unused code

2017-08-29 Thread Michael Haggerty
Now the outside world interacts with the packed ref store only via the generic refs API plus a few lock-related functions. This allows us to delete some functions that are no longer used, thereby completing the encapsulation of the packed ref store. Signed-off-by: Michael Haggerty --- refs/packe

[PATCH 08/10] files_ref_store: use a transaction to update packed refs

2017-08-29 Thread Michael Haggerty
When processing a `files_ref_store` transaction, it is sometimes necessary to delete some references from the "packed-refs" file. Do that using a reference transaction conducted against the `packed_ref_store`. This change further decouples `files_ref_store` from `packed_ref_store`. It also fixes m

[PATCH 01/10] packed-backend: don't adjust the reference count on lock/unlock

2017-08-29 Thread Michael Haggerty
The old code incremented the packed ref cache reference count when acquiring the packed-refs lock, and decremented the count when releasing the lock. This is unnecessary because a locked packed-refs file cannot be changed, so there is no reason for the cache to become stale. Moreover, the extra re

[PATCH 04/10] packed_delete_refs(): implement method

2017-08-29 Thread Michael Haggerty
Implement `packed_delete_refs()` using a reference transaction. This means that `files_delete_refs()` can use `refs_delete_refs()` instead of `repack_without_refs()` to delete any packed references, decreasing the coupling between the classes. Signed-off-by: Michael Haggerty --- refs/files-backe

[PATCH 06/10] files_initial_transaction_commit(): use a transaction for packed refs

2017-08-29 Thread Michael Haggerty
Used a `packed_ref_store` transaction in the implementation of `files_initial_transaction_commit()` rather than using internal features of the packed ref store. This further decouples `files_ref_store` from `packed_ref_store`. Signed-off-by: Michael Haggerty --- refs/files-backend.c | 29 +++

[PATCH 05/10] files_pack_refs(): use a reference transaction to write packed refs

2017-08-29 Thread Michael Haggerty
Now that the packed reference store supports transactions, we can use a transaction to write the packed versions of references that we want to pack. This decreases the coupling between `files_ref_store` and `packed_ref_store`. Signed-off-by: Michael Haggerty --- refs/files-backend.c | 24 +++

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

2017-08-29 Thread Michael Haggerty
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 refe

[PATCH 00/10] Implement transactions for the packed ref store

2017-08-29 Thread Michael Haggerty
This should be the second-to-last patch series in the quest for mmapped packed-refs. Before this patch series, there is still more coupling than necessary between `files_ref_store` and `packed_ref_store`: * `files_ref_store` adds packed references (e.g., during `git clone` and `git pack-refs`)

Re: [PATCH 2/3] merge-recursive: remove return value from get_files_dirs

2017-08-29 Thread Jeff King
On Mon, Aug 28, 2017 at 06:45:29PM -0400, Ben Peart wrote: > > Since the debug output has been removed and the caller isn't > > checking the return value there is no reason to keep calulating > > and returning a value. > > > > Did a quick search and you're right, nothing is using the return valu

Re: [PATCH 2/3] merge-recursive: remove return value from get_files_dirs

2017-08-29 Thread Jeff King
On Mon, Aug 28, 2017 at 02:28:28PM -0600, Kevin Willford wrote: > The return value of the get_files_dirs call is never being used. > Looking at the history of the file and it was originally only > being used for debug output statements. Also when > read_tree_recursive return value is non zero it

Re: [PATCH 1/3] merge-recursive: fix memory leak

2017-08-29 Thread Jeff King
On Mon, Aug 28, 2017 at 02:28:27PM -0600, Kevin Willford wrote: > In merge_trees if process_renames or process_entry returns less > than zero, the method will just return and not free re_merge, > re_head, or entries. > > This change cleans up the allocated variables before returning > to the call