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

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

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

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

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

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

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

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

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

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

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

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

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://"

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

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

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

v3 looks good to me. Thanks! Reviewed-by: Michael Haggerty Michael

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

> 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

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

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

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

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,

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

[PATCH 03/10] packed_ref_store: implement reference transactions

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 ++

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

`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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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