Re: [PATCH 1/6] repack: do not accidentally pack kept objects by default

On Tue, Jun 10, 2014 at 04:08:37PM -0400, Jeff King wrote: > Note the update to t7700. It failed to turn on bitmaps, > meaning we were actually confirming the wrong behavior! After writing this, I was thinking about the test, and why we didn't notice this regression. True, the test added to t7700

Project Investment?

Haven obtained your email address from an Internet Directory. I'm happy to introduce my self to you. My name is Mr. Paul Cardle, I am soliciting your sincere assistance over my Investment Proposal which I planned to invest with your 100% assistance. I am seeking for your co-operation in buildi

Re: [PATCH v2 00/11] Zsh prompt tests

On Tue, Jun 10, 2014 at 04:28:46PM -0400, Richard Hansen wrote: > On 2014-06-10 16:06, Torsten Bögershausen wrote: > > On 2014-06-04 23.01, Richard Hansen wrote: > > [] > > I haven't digged too deep, but this is what I get on pu: > > > > ./t9904-zsh-prompt.sh > > ./lib-zsh.sh:emulate:42: too many

Re: [PATCH v14 31/40] refs.c: make prune_ref use a transaction to delete the ref

Ronnie Sahlberg wrote: > Signed-off-by: Ronnie Sahlberg > --- > refs.c | 28 +--- > refs.h | 11 ++- > 2 files changed, 31 insertions(+), 8 deletions(-) Reviewed-by: Jonathan Nieder -- To unsubscribe from this list: send the line "unsubscribe git" in the body of

Re: [PATCH v1] config: Add hashtable for config parsing & retrival

One other minor point... On Mon, Jun 9, 2014 at 8:49 AM, Tanay Abhra wrote: > Subject: config: Add hashtable for config parsing & retrival s/retrival/retrieval/ > Add a hash table to cache all key-value pairs read from config files > (repo specific .git/config, user wide ~/.gitconfig and the gl

Re: [PATCH v14 26/40] walker.c: use ref transaction for ref updates

Ronnie Sahlberg wrote: > Signed-off-by: Ronnie Sahlberg > --- > walker.c | 59 +++ > 1 file changed, 35 insertions(+), 24 deletions(-) Reviewed-by: Jonathan Nieder -- To unsubscribe from this list: send the line "unsubscribe git" in the b

Re: [PATCH v14 25/40] fast-import.c: use a ref transaction when dumping tags

Ronnie Sahlberg wrote: > Signed-off-by: Ronnie Sahlberg > --- > fast-import.c | 29 +++-- > 1 file changed, 23 insertions(+), 6 deletions(-) Reviewed-by: Jonathan Nieder -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord

Re: [PATCH v14 24/40] receive-pack.c: use a reference transaction for updating the refs

Ronnie Sahlberg wrote: > --- a/builtin/receive-pack.c > +++ b/builtin/receive-pack.c > @@ -46,6 +46,7 @@ static void *head_name_to_free; > static int sent_capabilities; > static int shallow_update; > static const char *alt_shallow_file; > +static struct string_list error_strings = STRING_LIST_I

Re: [PATCH v14 16/40] refs.c: add transaction.status and track OPEN/CLOSED/ERROR

Ronnie Sahlberg wrote: > Signed-off-by: Ronnie Sahlberg > --- > refs.c | 40 +++- > 1 file changed, 39 insertions(+), 1 deletion(-) Reviewed-by: Jonathan Nieder This makes the API pretty strict, which should make it easier to experiment later if operations

Re: [PATCH v14 08/40] refs.c: add an err argument to delete_ref_loose

Ronnie Sahlberg wrote: > --- a/refs.c > +++ b/refs.c > @@ -2510,16 +2510,38 @@ static int repack_without_ref(const char *refname) [...] > +static int unlink_or_err(const char *file, struct strbuf *err) > +{ > + if (err) > + return add_err_if_unremovable("unlink", file, err, > +

Re: [PATCH] Fix git-p4 submit in non --prepare-p4-only mode

frrr...@gmail.com wrote on Tue, 10 Jun 2014 13:14 +0100: > b4073bb387ef303c9ac3c044f46d6a8ae6e190f0 broke git p4 submit, here > is a proper fix, including proper handling for windows end of lines. I guess we don't have test coverage for these cases? Is this something that should get put into a ma

[PATCH v15 04/48] refs.c: allow passing NULL to ref_transaction_free

Allow ref_transaction_free(NULL) as a no-op. This makes ref_transaction_free easier to use and more similar to plain 'free'. In particular, it lets us rollback unconditionally as part of cleanup code after setting 'transaction = NULL' if a transaction has been committed or rolled back already. Re

[PATCH v15 09/48] refs.c: make sure log_ref_setup returns a meaningful errno

Making errno when returning from log_ref_setup() meaningful, Signed-off-by: Ronnie Sahlberg --- refs.c | 29 - refs.h | 4 +++- 2 files changed, 23 insertions(+), 10 deletions(-) diff --git a/refs.c b/refs.c index b4b05bd..6f85bd8 100644 --- a/refs.c +++ b/refs.c @@

[PATCH v15 05/48] refs.c: add a strbuf argument to ref_transaction_commit for error logging

Add a strbuf argument to _commit so that we can pass an error string back to the caller. So that we can do error logging from the caller instead of from _commit. Longer term plan is to first convert all callers to use onerr==QUIET_ON_ERR and craft any log messages from the callers themselves and f

[PATCH v15 00/48] Use ref transactions

Final version. This patch series can also be found at https://github.com/rsahlberg/git/tree/ref-transactions This patch series is based on next and expands on the transaction API. It converts all ref updates, inside refs.c as well as external, to use the transaction API for updates. This makes mo

[PATCH v15 07/48] lockfile.c: make lock_file return a meaningful errno on failurei

Making errno when returning from lock_file() meaningful, which should fix * an existing almost-bug in lock_ref_sha1_basic where it assumes errno==ENOENT is meaningful and could waste some work on retries * an existing bug in repack_without_refs where it prints strerror(errno) and picks ad

[PATCH v15 03/48] refs.c: constify the sha arguments for ref_transaction_create|delete|update

ref_transaction_create|delete|update has no need to modify the sha1 arguments passed to it so it should use const unsigned char* instead of unsigned char*. Some functions, such as fast_forward_to(), already have its old/new sha1 arguments as consts. This function will at some point need to use ref

[PATCH v15 06/48] lockfile.c: add a new public function unable_to_lock_message

Introducing a new unable_to_lock_message helper, which has nicer semantics than unable_to_lock_error and cleans up lockfile.c a little. Signed-off-by: Ronnie Sahlberg --- cache.h| 2 ++ lockfile.c | 22 -- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/

[PATCH v15 13/48] refs.c: make resolve_ref_unsafe set errno to something meaningful on error

Making errno when returning from resolve_ref_unsafe() meaningful, which should fix * a bug in lock_ref_sha1_basic, where it assumes EISDIR means it failed due to a directory being in the way Signed-off-by: Ronnie Sahlberg --- cache.h | 2 +- refs.c | 19 +++ 2 files change

[PATCH v15 32/48] receive-pack.c: use a reference transaction for updating the refs

Wrap all the ref updates inside a transaction. Signed-off-by: Ronnie Sahlberg --- builtin/receive-pack.c | 31 ++- 1 file changed, 22 insertions(+), 9 deletions(-) diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index c323081..13f4a63 100644 --- a/builti

[PATCH v15 02/48] refs.c: ref_transaction_commit should not free the transaction

Reviewed-by: Jonathan Nieder Signed-off-by: Ronnie Sahlberg --- builtin/update-ref.c | 1 + refs.c | 1 - refs.h | 5 ++--- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/builtin/update-ref.c b/builtin/update-ref.c index 405267f..1fd7a89 100644 --- a/b

[PATCH v15 08/48] refs.c: add an err argument to repack_without_refs

Update repack_without_refs to take an err argument and update it if there is a failure. Pass the err variable from ref_transaction_commit to this function so that callers can print a meaningful error message if _commit fails due to this function. Signed-off-by: Ronnie Sahlberg --- refs.c | 19 ++

[PATCH v15 10/48] refs.c: verify_lock should set errno to something meaningful

Making errno when returning from verify_lock() meaningful, which should almost but not completely fix * a bug in "git fetch"'s s_update_ref, which trusts the result of an errno == ENOTDIR check to detect D/F conflicts ENOTDIR makes sense as a sign that a file was in the way of a directory we

[PATCH v15 29/48] fast-import.c: change update_branch to use ref transactions

Change update_branch() to use ref transactions for updates. Reviewed-by: Jonathan Nieder Signed-off-by: Ronnie Sahlberg --- fast-import.c | 25 +++-- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/fast-import.c b/fast-import.c index 6707a66..4a7b196 100644 -

[PATCH v15 15/48] refs.c: make ref_update_reject_duplicates take a strbuf argument for errors

Make ref_update_reject_duplicates return any error that occurs through a new strbuf argument. This means that when a transaction commit fails in this function we will now be able to pass a helpful error message back to the caller. Reviewed-by: Jonathan Nieder Signed-off-by: Ronnie Sahlberg ---

[PATCH v15 11/48] refs.c: make remove_empty_directories alwasy set errno to something sane

Making errno when returning from remove_empty_directories() more obviously meaningful, which should provide some peace of mind for people auditing lock_ref_sha1_basic. Signed-off-by: Ronnie Sahlberg --- refs.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/refs.c b/refs.

[PATCH v15 42/48] refs.c: pass NULL as *flags to read_ref_full

We call read_ref_full with a pointer to flags from rename_ref but since we never actually use the returned flags we can just pass NULL here instead. Reviewed-by: Jonathan Nieder Signed-off-by: Ronnie Sahlberg --- refs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/refs.c

[PATCH v15 17/48] refs.c: make update_ref_write update a strbuf on failure

Change update_ref_write to also update an error strbuf on failure. This makes the error available to ref_transaction_commit callers if the transaction failed due to update_ref_sha1/write_ref_sha1 failures. Reviewed-by: Jonathan Nieder Signed-off-by: Ronnie Sahlberg --- refs.c | 9 ++--- 1 f

[PATCH v15 40/48] refs.c: make delete_ref use a transaction

Change delete_ref to use a ref transaction for the deletion. At the same time since we no longer have any callers of repack_without_ref we can now delete this function. Change delete_ref to return 0 on success and 1 on failure instead of the previous 0 on success either 1 or -1 on failure. Review

[PATCH v15 37/48] refs.c: remove the update_ref_write function

Since we only call update_ref_write from a single place and we only call it with onerr==QUIET_ON_ERR we can just as well get rid of it and just call write_ref_sha1 directly. This changes the return status for _commit from 1 to -1 on failures when writing to the ref. Eventually we will want _commit

[PATCH v15 46/48] refs.c: propagate any errno==ENOTDIR from _commit back to the callers

In _commit, ENOTDIR can happen in the call to lock_ref_sha1_basic, either when we lstat the new refname and it returns ENOTDIR or if the name checking function reports that the same type of conflict happened. In both cases it means that we can not create the new ref due to a name conflict. For the

[PATCH v15 35/48] refs.c: make lock_ref_sha1 static

No external callers reference lock_ref_sha1 any more so lets declare it static. Reviewed-by: Jonathan Nieder Signed-off-by: Ronnie Sahlberg --- refs.c | 2 +- refs.h | 6 -- 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/refs.c b/refs.c index b9a6adc..ace6f87 100644 --- a/ref

[PATCH v15 16/48] refs.c: add an err argument to delete_ref_loose

Add an err argument to delete_loose_ref so that we can pass a descriptive error string back to the caller. Pass the err argument from transaction commit to this function so that transaction users will have a nice error string if the transaction failed due to delete_loose_ref. Add a new function un

[PATCH v15 33/48] fast-import.c: use a ref transaction when dumping tags

Signed-off-by: Ronnie Sahlberg --- fast-import.c | 29 +++-- 1 file changed, 23 insertions(+), 6 deletions(-) diff --git a/fast-import.c b/fast-import.c index 4a7b196..587ef4a 100644 --- a/fast-import.c +++ b/fast-import.c @@ -1735,15 +1735,32 @@ static void dump_tags(voi

[PATCH v15 47/48] fetch.c: change s_update_ref to use a ref transaction

Change s_update_ref to use a ref transaction for the ref update. Signed-off-by: Ronnie Sahlberg --- builtin/fetch.c | 33 +++-- 1 file changed, 23 insertions(+), 10 deletions(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index faa1233..52f1ebc 100644 --- a/builti

[PATCH v15 48/48] refs.c: make write_ref_sha1 static

No external users call write_ref_sha1 any more so lets declare it static. Signed-off-by: Ronnie Sahlberg --- refs.c | 10 -- refs.h | 3 --- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/refs.c b/refs.c index 4cee9e70..5617552 100644 --- a/refs.c +++ b/refs.c @@ -2639,6

[PATCH v15 38/48] refs.c: remove lock_ref_sha1

lock_ref_sha1 was only called from one place in refc.c and only provided a check that the refname was sane before adding back the initial "refs/" part of the ref path name, the initial "refs/" that this caller had already stripped off before calling lock_ref_sha1. Reviewed-by: Jonathan Nieder Sig

[PATCH v15 43/48] refs.c: move the check for valid refname to lock_ref_sha1_basic

Move the check for check_refname_format from lock_any_ref_for_update to lock_ref_sha1_basic. At some later stage we will get rid of lock_any_ref_for_update completely. If lock_ref_sha1_basic fails the check_refname_format test, set errno to EINVAL before returning NULL. This to guarantee that we w

[PATCH v15 41/48] refs.c: pass the ref log message to _create/delete/update instead of _commit

Change the reference transactions so that we pass the reflog message through to the create/delete/update function instead of the commit message. This allows for individual messages for each change in a multi ref transaction. Reviewed-by: Jonathan Nieder Signed-off-by: Ronnie Sahlberg --- branch

[PATCH v15 45/48] refs.c: pass a skip list to name_conflict_fn

Allow passing a list of refs to skip checking to name_conflict_fn. There are some conditions where we want to allow a temporary conflict and skip checking those refs. For example if we have a transaction that 1, guarantees that m is a packed refs and there is no loose ref for m 2, the transaction w

[PATCH v15 34/48] walker.c: use ref transaction for ref updates

Switch to using ref transactions in walker_fetch(). As part of the refactoring to use ref transactions we also fix a potential memory leak where in the original code if write_ref_sha1() would fail we would end up returning from the function without free()ing the msg string. Note that this function

[PATCH v15 44/48] refs.c: call lock_ref_sha1_basic directly from commit

Skip using the lock_any_ref_for_update wrapper and call lock_ref_sha1_basic directly from the commit function. Reviewed-by: Jonathan Nieder Signed-off-by: Ronnie Sahlberg --- refs.c | 12 ++-- 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/refs.c b/refs.c index 5d35e2e..f

[PATCH v15 39/48] refs.c: make prune_ref use a transaction to delete the ref

Change prune_ref to delete the ref using a ref transaction. To do this we also need to add a new flag REF_ISPRUNING that will tell the transaction that we do not want to delete this ref from the packed refs. This flag is private to refs.c and not exposed to external callers. Signed-off-by: Ronnie

[PATCH v15 31/48] refs.c: change update_ref to use a transaction

Change the update_ref helper function to use a ref transaction internally. Reviewed-by: Jonathan Nieder Signed-off-by: Ronnie Sahlberg --- refs.c | 25 + 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/refs.c b/refs.c index b47abdd..b9a6adc 100644 --- a/re

[PATCH v15 26/48] replace.c: use the ref transaction functions for updates

Update replace.c to use ref transactions for updates. Reviewed-by: Jonathan Nieder Signed-off-by: Ronnie Sahlberg --- builtin/replace.c | 15 +-- 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/builtin/replace.c b/builtin/replace.c index 4b3705d..cf92e5d 100644 --- a/b

[PATCH v15 18/48] update-ref: use err argument to get error from ref_transaction_commit

Call ref_transaction_commit with QUIET_ON_ERR and use the strbuf that is returned to print a log message if/after the transaction fails. Reviewed-by: Jonathan Nieder Signed-off-by: Ronnie Sahlberg --- builtin/update-ref.c | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) diff --

[PATCH v15 23/48] refs.c: make ref_transaction_begin take an err argument

Add an err argument to _begin so that on non-fatal failures in future ref backends we can report a nice error back to the caller. While _begin can currently never fail for other reasons than OOM, in which case we die() anyway, we may add other types of backends in the future. For example, a hypothe

[PATCH v15 24/48] refs.c: add transaction.status and track OPEN/CLOSED/ERROR

Track the status of a transaction in a new status field. Check the field for sanity, i.e. that status must be OPEN when _commit/_create/_delete or _update is called or else die(BUG:...) Signed-off-by: Ronnie Sahlberg --- refs.c | 40 +++- 1 file changed, 39 in

[PATCH v15 20/48] refs.c: change ref_transaction_update() to do error checking and return status

Update ref_transaction_update() do some basic error checking and return non-zero on error. Update all callers to check ref_transaction_update() for error. There are currently no conditions in _update that will return error but there will be in the future. Add an err argument that will be updated on

[PATCH v15 19/48] refs.c: remove the onerr argument to ref_transaction_commit

Since all callers now use QUIET_ON_ERR we no longer need to provide an onerr argument any more. Remove the onerr argument from the ref_transaction_commit signature. Reviewed-by: Jonathan Nieder Signed-off-by: Ronnie Sahlberg --- builtin/update-ref.c | 3 +-- refs.c | 22 +++--

[PATCH v15 22/48] refs.c: update ref_transaction_delete to check for error and return status

Change ref_transaction_delete() to do basic error checking and return non-zero of error. Update all callers to check the return for ref_transaction_delete(). There are currently no conditions in _delete that will return error but there will be in the future. Add an err argument that will be updated

[PATCH v15 21/48] refs.c: change ref_transaction_create to do error checking and return status

Do basic error checking in ref_transaction_create() and make it return non-zero on error. Update all callers to check the result of ref_transaction_create(). There are currently no conditions in _create that will return error but there will be in the future. Add an err argument that will be updated

[PATCH v15 25/48] tag.c: use ref transactions when doing updates

Change tag.c to use ref transactions for all ref updates. Reviewed-by: Jonathan Nieder Signed-off-by: Ronnie Sahlberg --- builtin/tag.c | 15 +-- 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/builtin/tag.c b/builtin/tag.c index c6e8a71..c9bfc9a 100644 --- a/builtin/t

[PATCH v15 36/48] refs.c: remove the update_ref_lock function

Since we now only call update_ref_lock with onerr==QUIET_ON_ERR we no longer need this function and can replace it with just calling lock_any_ref_for_update directly. Reviewed-by: Jonathan Nieder Signed-off-by: Ronnie Sahlberg --- refs.c | 30 ++ 1 file changed, 6 in

[PATCH v15 28/48] sequencer.c: use ref transactions for all ref updates

Change to use ref transactions for all updates to refs. Reviewed-by: Jonathan Nieder Signed-off-by: Ronnie Sahlberg --- sequencer.c | 24 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/sequencer.c b/sequencer.c index 0a80c58..fd8acaf 100644 --- a/sequenc

[PATCH v15 30/48] branch.c: use ref transaction for all ref updates

Change create_branch to use a ref transaction when creating the new branch. This also fixes a race condition in the old code where two concurrent create_branch could race since the lock_any_ref_for_update/write_ref_sha1 did not protect against the ref already existing. I.e. one thread could end up

[PATCH v15 14/48] refs.c: log_ref_write should try to return meaningful errno

Making errno from write_ref_sha1() meaningful, which should fix * a bug in "git checkout -b" where it prints strerror(errno)  despite errno possibly being zero or clobbered * a bug in "git fetch"'s s_update_ref, which trusts the result of an  errno == ENOTDIR check to detect D/F conflicts Sign

[PATCH v15 27/48] commit.c: use ref transactions for updates

Change commit.c to use ref transactions for all ref updates. Make sure we pass a NULL pointer to ref_transaction_update if have_old is false. Reviewed-by: Jonathan Nieder Signed-off-by: Ronnie Sahlberg --- builtin/commit.c | 24 +++- 1 file changed, 11 insertions(+), 13 dele

[PATCH v15 12/48] refs.c: commit_packed_refs to return a meaningful errno on failure

Making errno when returning from commit_packed_refs() meaningful, which should fix * a bug in "git clone" where it prints strerror(errno) based on errno, despite errno possibly being zero and potentially having been clobbered by that point * the same kind of bug in "git pack-refs" and pre

[PATCH v15 01/48] refs.c: remove ref_transaction_rollback

We do not yet need both a rollback and a free function for transactions. Remove ref_transaction_rollback and use ref_transaction_free instead. At a later stage we may reintroduce a rollback function if we want to start adding reusable transactions and similar. Reviewed-by: Jonathan Nieder Signed

What's cooking in git.git (Jun 2014, #03; Tue, 10)

Here are the topics that have been cooking. Commits prefixed with '-' are only in 'pu' (proposed updates) while commits prefixed with '+' are in 'next'. The tip of 'next' has been rewound. I originally wanted to eject many younger topics and slim the branch down, but decided against it. You can

Re: [PATCH 10/15] imap-send.c: rearrange xcalloc arguments

Brian, On Tue, May 27, 2014 at 12:33:51AM +0900, Brian Gesiak wrote: > xcalloc takes two arguments: the number of elements and their size. > imap_open_store passes the arguments in reverse order, passing the > size of an imap_store*, followed by the number to allocate. > Rearrgange them so they ar

Re: [PATCH 05/15] sequencer: use logmsg_reencode in get_message

On Tue, Jun 10, 2014 at 02:40:30PM -0700, Junio C Hamano wrote: > Jeff King writes: > > > Note that we may be fixing a bug here. The existing code > > does: > > > > if (same_encoding(to, from)) > > reencode_string(buf, to, from); > > > > That probably should have been "!same_encoding". >

Re: [PATCH 03/15] do not create "struct commit" with xcalloc

On Tue, Jun 10, 2014 at 02:35:48PM -0700, Junio C Hamano wrote: > Jeff King writes: > > > In both blame and merge-recursive, we sometimes create a > > "fake" commit struct for convenience (e.g., to represent the > > HEAD state as if we would commit it). By allocating > > ourselves rather than us

Re: [PATCH 15/20] t/t5000-tar-tree.sh: avoid "test -a/-o "

Am 10.06.2014 22:08, schrieb David Aguilar: [Resent using René's correct email address this time, sorry for the noise] On Fri, Jun 06, 2014 at 07:55:58AM -0700, Elia Pinto wrote: The construct is error-prone; "test" being built-in in most modern shells, the reason to avoid "test && test " spaw

Re: [PATCH v2 0/17] store length of commit->buffer

On Tue, Jun 10, 2014 at 05:35:09PM -0400, Jeff King wrote: > Here's a re-roll of the commit-slab series. It fixes the issues pointed > out by Eric and Christian (thanks, both). Side note: I marked this as v2, but forgot to do so in each individual patch (I write my cover letters first, and then i

Re: [PATCH v14 06/40] refs.c: add an err argument to repack_without_refs

Thanks. On Tue, Jun 10, 2014 at 1:10 PM, Jonathan Nieder wrote: > Ronnie Sahlberg wrote: > >> Update repack_without_refs to take an err argument and update it if there >> is a failure. Pass the err variable from ref_transaction_commit to this >> function so that callers can print a meaningful err

[PATCH 17/17] commit: record buffer length in cache

Most callsites which use the commit buffer try to use the cached version attached to the commit, rather than re-reading from disk. Unfortunately, that interface provides only a pointer to the NUL-terminated buffer, with no indication of the original length. For the most part, this doesn't matter.

[PATCH 15/17] commit-slab: provide a static initializer

Callers currently must use init_foo_slab() at runtime before accessing a slab. For global slabs, it's much nicer if we can initialize them in BSS, so that each user does not have to add code to check-and-initialize. Signed-off-by: Jeff King --- There was no comment on this one in v1. I'd be curio

[PATCH 16/17] commit: convert commit->buffer to a slab

This will make it easier to manage the buffer cache independently of the "struct commit" objects. It also shrinks "struct commit" by one pointer, which may be helpful. Unfortunately it does not reduce the max memory size of something like "rev-list", because rev-list uses get_cached_commit_buffer(

[PATCH 13/17] convert logmsg_reencode to get_commit_buffer

Like the callsites in the previous commit, logmsg_reencode already falls back to read_sha1_file when necessary. However, I split its conversion out into its own commit because it's a bit more complex. We return either: 1. The original commit->buffer 2. A newly allocated buffer from read_sha1

[PATCH 14/17] use get_commit_buffer everywhere

Each of these sites assumes that commit->buffer is valid. Since they would segfault if this was not the case, they are likely to be correct in practice. However, we can future-proof them by using get_commit_buffer. And as a side effect, we abstract away the final bare uses of commit->buffer. Sign

[PATCH 12/17] use get_commit_buffer to avoid duplicate code

For both of these sites, we already do the "fallback to read_sha1_file" trick. But we can shorten the code by just using get_commit_buffer. Note that the error cases are slightly different when read_sha1_file fails. get_commit_buffer will die() if the object cannot be loaded, or is a non-commit.

[PATCH 10/17] provide helpers to access the commit buffer

Many sites look at commit->buffer to get more detailed information than what is in the parsed commit struct. However, we sometimes drop commit->buffer to save memory, in which case the caller would need to read the object afresh. Some callers do this (leading to duplicated code), and others do not

[PATCH 08/17] provide a helper to free commit buffer

This converts two lines into one at each caller. But more importantly, it abstracts the concept of freeing the buffer, which will make it easier to change later. Note that we also need to provide a "detach" mechanism for the weird case in fsck which passes the buffer back to be freed. Signed-off-

[PATCH 09/17] provide a helper to set the commit buffer

Right now this is just a one-liner, but abstracting it will make it easier to change later. Signed-off-by: Jeff King --- builtin/blame.c | 2 +- commit.c| 7 ++- commit.h| 6 ++ object.c| 2 +- 4 files changed, 14 insertions(+), 3 deletions(-) diff --git a/builti

Re: [PATCH 05/15] sequencer: use logmsg_reencode in get_message

Jeff King writes: > Note that we may be fixing a bug here. The existing code > does: > > if (same_encoding(to, from)) > reencode_string(buf, to, from); > > That probably should have been "!same_encoding". > > Signed-off-by: Jeff King > --- > I didn't actually test for the bug, so it's

[PATCH 11/17] use get_cached_commit_buffer where appropriate

Some call sites check commit->buffer to see whether we have a cached buffer, and if so, do some work with it. In the long run we may want to switch these code paths to make their decision on a different boolean flag (because checking the cache may get a little more expensive in the future). But for

[PATCH 06/17] logmsg_reencode: return const buffer

The return value from logmsg_reencode may be either a newly allocated buffer or a pointer to the existing commit->buffer. We would not want the caller to accidentally free() or modify the latter, so let's mark it as const. We can cast away the constness in logmsg_free, but only once we have determ

[PATCH 07/17] sequencer: use logmsg_reencode in get_message

This simplifies the code, as logmsg_reencode handles the reencoding for us in a single call. It also means we learn logmsg_reencode's trick of pulling the buffer from disk when commit->buffer is NULL (we currently just silently return!). It is doubtful this matters in practice, though, as sequencer

[PATCH 05/17] do not create "struct commit" with xcalloc

In both blame and merge-recursive, we sometimes create a "fake" commit struct for convenience (e.g., to represent the HEAD state as if we would commit it). By allocating ourselves rather than using alloc_commit_node, we do not properly set the "index" field of the commit. This can produce subtle bu

[PATCH 04/17] commit: push commit_index update into alloc_commit_node

Whenever we create a commit object via lookup_commit, we give it a unique index to be used with the commit-slab API. The theory is that any "struct commit" we create would follow this code path, so any such struct would get an index. However, callers could use alloc_commit_node() directly (and get

[PATCH 03/17] alloc: include any-object allocations in alloc_report

When 2c1cbec (Use proper object allocators for unknown object nodes too, 2007-04-16), added a special "any_object" allocator, it never taught alloc_report to report on it. To do so we need to add an extra type argument to the REPORT macro, as that commit did for DEFINE_ALLOCATOR. Signed-off-by: Je

[PATCH 02/17] replace dangerous uses of strbuf_attach

It is not a good idea to strbuf_attach an arbitrary pointer just because a function you are calling wants a strbuf. Attaching implies a transfer of memory ownership; if anyone were to modify or release the resulting strbuf, we would free() the pointer, leading to possible problems: 1. Other user

[PATCH 01/17] commit_tree: take a pointer/len pair rather than a const strbuf

While strbufs are pretty common throughout our code, it is more flexible for functions to take a pointer/len pair than a strbuf. It's easy to turn a strbuf into such a pair (by dereferencing its members), but less easy to go the other way (you can strbuf_attach, but that has implications about memo

Re: [PATCH 03/15] do not create "struct commit" with xcalloc

Jeff King writes: > In both blame and merge-recursive, we sometimes create a > "fake" commit struct for convenience (e.g., to represent the > HEAD state as if we would commit it). By allocating > ourselves rather than using alloc_commit_node, we do not > properly set the "index" field of the comm

[PATCH v2 0/17] store length of commit->buffer

Here's a re-roll of the commit-slab series. It fixes the issues pointed out by Eric and Christian (thanks, both). It adds two new patches at the beginning to clean up the dangerous strbufs that we discussed elsewhere. And as a result, silences the warning from the old 12/15. I even compiled with a

Re: [PATCH 02/15] commit: push commit_index update into alloc_commit_node

Jeff King writes: > This will make the alloc_report output a little uglier (it will say > raw_commit), but I don't think anyone cares. And I wanted to make sure > there wasn't an easy way to accidentally call the wrong alloc function > that doesn't handle the index. Thanks; I like this change.

Re: [PATCH 15/15] commit: record buffer length in cache

From: Jeff King Subject: Re: [PATCH 15/15] commit: record buffer length in cache Date: Tue, 10 Jun 2014 16:33:49 -0400 > On Tue, Jun 10, 2014 at 01:27:13AM -0400, Jeff King wrote: > >> > I find the above strange. I would have done something like: >> > >> > - set_commit_buffer(commit, strbuf_de

Re: [PATCH 4/6] pack-objects: stop respecting pack.writebitmaps

On Tue, Jun 10, 2014 at 02:07:37PM -0700, Junio C Hamano wrote: > > Another option is to track it to graduate to master during the next > > cycle. I.e., decide that the possible regression isn't a big deal. > > My gut feeling is that the last one is sufficient. These low level > subcommands that

Re: [PATCH 12/15] use get_commit_buffer everywhere

On Tue, Jun 10, 2014 at 09:06:35AM -0700, Junio C Hamano wrote: > > So any call to strbuf_detach on the result would be disastrous. > > You are right. Where did this original crap come from X-<... I do not know if that face means you actually looked at the history, but in case you did not... I

Re: [PATCH v1] config: Add hashtable for config parsing & retrival

On Tue, Jun 10, 2014 at 8:35 AM, Tanay Abhra wrote: > Thanks for the review, Eric. I have replied to your comments below, > I will try to reply early and more promptly now. Thanks for responding. More below. > On 06/10/2014 04:27 AM, Eric Sunshine wrote: >>> --- >>> diff --git a/config.c b/confi

Re: [PATCH 4/6] pack-objects: stop respecting pack.writebitmaps

Jeff King writes: > I'm not sure what we want to do with this. It _is_ a possible regression > as explained above, but I really do find it improbable that anyone will > care. Even at GitHub, where we use a custom script instead of running > `git gc`, we hook into the repack code, and not directly

Re: [PATCH 10/15] use get_commit_buffer to avoid duplicate code

On Tue, Jun 10, 2014 at 04:01:29AM -0400, Eric Sunshine wrote: > > For record_author_date, the new behavior is probably better; > > we notify the user of the error instead of silently ignoring > > it. And because it's used only for sorting by author-date, > > somebody examining a corrupt can fallb

Re: [PATCH] git-submodule.sh: avoid "test -a/-o "

On Tue, Jun 10, 2014 at 12:43 PM, Elia Pinto wrote: > The construct is error-prone; "test" being built-in in most modern > shells, the reason to avoid "test && test " spawning > one extra process by using a single "test -a " no > longer exists. > > Signed-off-by: Elia Pinto > --- > This is the

Re: [PATCH 15/15] commit: record buffer length in cache

On Tue, Jun 10, 2014 at 01:27:13AM -0400, Jeff King wrote: > > I find the above strange. I would have done something like: > > > > - set_commit_buffer(commit, strbuf_detach(&msg, NULL)); > > + size_t size; > > + char *buf = strbuf_detach(&msg, &size); > > + set_commit_buffer(commit, buf,

Re: Disk waste with packs and .keep files

On Tue, Jun 10, 2014 at 02:53:21PM -0400, Jeff King wrote: > This patch is the minimal fix to restore the desired > behavior for the default state. However, the real fix will > be more involved. This patch actually breaks t7700, but because the test is wrong. Double yikes. All fixed in my series,

Re: [PATCH v2 00/11] Zsh prompt tests

On 2014-06-10 16:06, Torsten Bögershausen wrote: > On 2014-06-04 23.01, Richard Hansen wrote: > [] > I haven't digged too deep, but this is what I get on pu: > > ./t9904-zsh-prompt.sh > ./lib-zsh.sh:emulate:42: too many arguments > ./lib-zsh.sh:emulate:52: too many arguments > /lib-prompt-tests.s

[PATCH 6/6] repack: introduce repack.writeBitmaps config option

We currently have pack.writeBitmaps, which originally operated at the pack-objects level. This should really have been a repack.* option from day one. Let's give it the more sensible name, but keep the old version as a deprecated synonym. Signed-off-by: Jeff King --- We can still do this if we do

[PATCH 5/6] repack: simplify handling of --write-bitmap-index

We previously needed to pass --no-write-bitmap-index explicitly to pack-objects to override its reading of pack.writebitmaps from the config. Now that it no longer does so, we can assume that bitmaps are off by default, and only turn them on when necessary. This also lets us avoid a confusing tri-s

  1   2   >