[PATCH v2 09/20] refs: split `ref_cache` code into separate files

2017-03-31 Thread Michael Haggerty
, adds declarations, and changes the visibility of some functions, but doesn't change any code. The modules are still too tightly coupled, but the situation will be improved in subsequent commits. Signed-off-by: Michael Haggerty --- Makefile | 1 + refs/files-backend.c

[PATCH v2 19/20] files_pack_refs(): use reference iteration

2017-03-31 Thread Michael Haggerty
Use reference iteration rather than do_for_each_entry_in_dir() in the definition of files_pack_refs(). Signed-off-by: Michael Haggerty --- refs/files-backend.c | 143 +-- 1 file changed, 60 insertions(+), 83 deletions(-) diff --git a/refs/files

[PATCH v2 12/20] ref-cache: use a callback function to fill the cache

2017-03-31 Thread Michael Haggerty
supply a pointer to function `read_loose_refs` (renamed to `loose_fill_ref_dir`) when creating the ref cache for its loose refs. This means that we can generify the type of the back-pointer in `struct ref_cache` from `files_ref_store` to `ref_store`. Signed-off-by: Michael Haggerty --- refs/files

[PATCH v2 08/20] ref-cache: rename `remove_entry()` to `remove_entry_from_dir()`

2017-03-31 Thread Michael Haggerty
This function's visibility is about to be increased, so give it a more distinctive name. Signed-off-by: Michael Haggerty --- refs/files-backend.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index 6768c8c86b..b4c11

[PATCH v2 11/20] refs: record the ref_store in ref_cache, not ref_dir

2017-03-31 Thread Michael Haggerty
the size of every `ref_dir` instance. Signed-off-by: Michael Haggerty --- refs/files-backend.c | 6 +++--- refs/ref-cache.c | 12 +++- refs/ref-cache.h | 9 ++--- 3 files changed, 16 insertions(+), 11 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c

[PATCH v2 16/20] get_loose_ref_cache(): new function

2017-03-31 Thread Michael Haggerty
Extract a new function, `get_loose_ref_cache()`, from get_loose_ref_dir(). The function returns the `ref_cache` for the loose refs of a `files_ref_store`. Signed-off-by: Michael Haggerty --- refs/files-backend.c | 9 +++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/refs

[PATCH v2 06/20] ref-cache: rename `add_ref()` to `add_ref_entry()`

2017-03-31 Thread Michael Haggerty
This function's visibility is about to be increased, so give it a more distinctive name. Signed-off-by: Michael Haggerty --- refs/files-backend.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index cad56efb04..4d579

[PATCH v2 10/20] ref-cache: introduce a new type, ref_cache

2017-03-31 Thread Michael Haggerty
For now, it just wraps a `ref_entry *` that points at the root of the tree. Soon it will hold more information. Add two new functions, `create_ref_cache()` and `free_ref_cache()`. Make `free_ref_entry()` private. Change files-backend to use this type to hold its caches. Signed-off-by: Michael

[PATCH v2 14/20] do_for_each_entry_in_dir(): eliminate `offset` argument

2017-03-31 Thread Michael Haggerty
It was never used. Signed-off-by: Michael Haggerty --- refs/files-backend.c | 4 ++-- refs/ref-cache.c | 6 +++--- refs/ref-cache.h | 11 +-- 3 files changed, 10 insertions(+), 11 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index 7b5f5c1240

[PATCH v2 20/20] do_for_each_entry_in_dir(): delete function

2017-03-31 Thread Michael Haggerty
Its only remaining caller was itself. Signed-off-by: Michael Haggerty --- refs/ref-cache.c | 21 - refs/ref-cache.h | 11 --- 2 files changed, 32 deletions(-) diff --git a/refs/ref-cache.c b/refs/ref-cache.c index b3a30350d7..6059362f1d 100644 --- a/refs/ref-cache.c

[PATCH v2 13/20] refs: handle "refs/bisect/" in `loose_fill_ref_dir()`

2017-03-31 Thread Michael Haggerty
That "refs/bisect/" has to be handled specially when filling the ref_cache for loose references is a peculiarity of the files backend, and the ref-cache code shouldn't need to know about it. So move this code to the callback function, `loose_fill_ref_dir()`. Signed-off-by: M

[PATCH v2 18/20] commit_packed_refs(): use reference iteration

2017-03-31 Thread Michael Haggerty
ode or the new. Signed-off-by: Michael Haggerty --- refs/files-backend.c | 38 +- 1 file changed, 17 insertions(+), 21 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index 736a6c9ff7..0ea42826c8 100644 --- a/refs/files-backend.c +++ b/re

[PATCH v2 17/20] cache_ref_iterator_begin(): make function smarter

2017-03-31 Thread Michael Haggerty
ator_begin()` to be made more ignorant of the internals of `ref_cache`, and `find_containing_dir()` and `prime_ref_dir()` to be made private. Signed-off-by: Michael Haggerty --- refs/files-backend.c | 44 +--- refs/ref-cache.

Re: [PATCH v7 19/28] files-backend: replace submodule_allowed check in files_downcast()

2017-03-31 Thread Michael Haggerty
t; refs = (struct files_ref_store *)ref_store; > > - if (!submodule_allowed) > - files_assert_main_repository(refs, caller); > + if ((refs->store_flags & required_flags) != required_flags) > + die("BUG: unallowed operation (%s), requires %x, has %x\n", > + caller, required_flags, refs->store_flags); Same comment about "unallowed". Maybe BUG: operation %s requires abilities 0x%x but only have 0x%x > > return refs; > } > [...] Michael

Re: [PATCH v7 22/28] refs: new transaction related ref-store api

2017-03-31 Thread Michael Haggerty
tely differ) rather than a concrete problem. I haven't yet had any difficulties working with your interface. But I wanted to mention my observation anyway. As far as I'm concerned, you don't need to change it. > [...] Michael [*] The name could obviously be improved, but you get the idea.

Re: [PATCH v7 23/28] files-backend: avoid ref api targetting main ref store

2017-03-31 Thread Michael Haggerty
nd testing is required before that can > happen. Well, except peel_ref(). I'm pretty sure that function is safe. > [...] Michael

Re: [PATCH v7 00/28] Remove submodule from files-backend.c

2017-03-31 Thread Michael Haggerty
fully, and send a couple of minor comments. But with or without changes, it looks good to me and the whole series is Reviewed-by: Michael Haggerty Thanks! Michael

Re: [PATCH v2 00/20] Separate `ref_cache` into a separate module

2017-03-31 Thread Michael Haggerty
On 03/31/2017 06:01 PM, Junio C Hamano wrote: > Michael Haggerty writes: > >> This version literally only contains a few commit message changes and >> one minor comment changes relative to v1. The code is identical. I >> wasn't sure whether it is even worth sending t

Re: [PATCH v6 4/5] dir_iterator: refactor state machine model

2017-04-01 Thread Michael Haggerty
ver letter. That makes it easier for casual readers to fetch the code and/or look at it online, and also makes it 100% clear what branch point I've chosen. This is by no means obligatory but I find it helpful. There doesn't seem to be much point reviewing broken code, so I'll wait for your feedback and/or fix. Michael [1] https://github.com/mhagger/git-test

Re: [PATCH v7 4/5] dir_iterator: refactor state machine model

2017-04-02 Thread Michael Haggerty
-p dir2/a/b/c/ && >dir2/a/b/c/d ' Note that at least one of the test directories has to be renamed so that they don't conflict with each other. There is no need for the individual tests to delete their test directories; the test harness will take care of that. But if you *did* need to clean up after a test, you should do it like this: mkdir foo && test_when_finished "rm -rf foo" && ...tests involving foo... The advantage of `test_when_finished` is that it ensures that the cleanup code is run even if some part of the test fails. > cat >expect-pre-order-output <<-\EOF && > [d] (a) ./dir/a > [d] (a/b) ./dir/a/b > @@ -41,14 +57,53 @@ cat >expect-pre-order-output <<-\EOF && > [f] (a/b/c/d) ./dir/a/b/c/d > EOF > > -test_expect_success 'dir-iterator should list files in the correct order' ' > +test_expect_success 'dir-iterator should list files properly on pre-order > mode' ' > mkdir -p dir/a/b/c/ && > >dir/a/b/c/d && > > - test-dir-iterator ./dir >actual-pre-order-output && > + test-dir-iterator ./dir 1 >actual-pre-order-output && > rm -rf dir && > > test_cmp expect-pre-order-output actual-pre-order-output > ' > > +cat >expect-post-order-output <<-\EOF && > +[f] (a/b/c/d) ./dir/a/b/c/d > +[d] (a/b/c) ./dir/a/b/c > +[d] (a/b) ./dir/a/b > +[d] (a) ./dir/a > +EOF > + > +test_expect_success 'dir-iterator should list files properly on post-order > mode' ' > + mkdir -p dir/a/b/c/ && > + >dir/a/b/c/d && > + > + test-dir-iterator ./dir 2 >actual-post-order-output && > + rm -rf dir && > + > + test_cmp expect-post-order-output actual-post-order-output > +' > + > +cat >expect-pre-order-post-order-root-dir-output <<-\EOF && > +[d] (.) ./dir > +[d] (a) ./dir/a > +[d] (a/b) ./dir/a/b > +[d] (a/b/c) ./dir/a/b/c > +[f] (a/b/c/d) ./dir/a/b/c/d > +[d] (a/b/c) ./dir/a/b/c > +[d] (a/b) ./dir/a/b > +[d] (a) ./dir/a > +[d] (.) ./dir > +EOF > + > +test_expect_success 'dir-iterator should list files properly on pre-order + > post-order + root-dir mode' ' > + mkdir -p dir/a/b/c/ && > + >dir/a/b/c/d && > + > + test-dir-iterator ./dir 7 >actual-pre-order-post-order-root-dir-output > && > + rm -rf dir && > + > + test_cmp expect-pre-order-post-order-root-dir-output > actual-pre-order-post-order-root-dir-output > +' > + > test_done Michael

Re: [PATCH v7 5/5] remove_subtree(): reimplement using iterators

2017-04-02 Thread Michael Haggerty
;, path->buf); > } > > static int create_file(const char *path, unsigned int mode) > @@ -312,7 +298,7 @@ int checkout_entry(struct cache_entry *ce, > return 0; > if (!state->force) > return error("%s is a directory", path.buf); > - remove_subtree(&path); > + remove_subtree(path.buf); > } else if (unlink(path.buf)) > return error_errno("unable to unlink old '%s'", > path.buf); > } else if (state->not_new) > That's a gratifying decrease in code size :-) Michael

Re: [PATCH v7 4/5] dir_iterator: refactor state machine model

2017-04-03 Thread Michael Haggerty
Signed-off-by: Michael Haggerty --- Daniel, > Although it does make tests harder to understand, if we were to > specify how to iterate with human-readable flags we'd add the getopt() > + flag configuration overhead to this helper program to be able to > handle all cases prope

Re: [PATCH v2 03/20] refs_ref_iterator_begin(): new function

2017-04-15 Thread Michael Haggerty
On 04/07/2017 12:57 PM, Duy Nguyen wrote: > On Fri, Mar 31, 2017 at 9:11 PM, Michael Haggerty > wrote: >> Extract a new function from `do_for_each_ref()`. It will be useful >> elsewhere. >> >> Signed-off-by: Michael Haggerty >> --- >> refs.c

Re: [PATCH v2 04/20] refs_verify_refname_available(): implement once for all backends

2017-04-15 Thread Michael Haggerty
On 04/07/2017 01:20 PM, Duy Nguyen wrote: > On Fri, Mar 31, 2017 at 9:11 PM, Michael Haggerty > wrote: >> It turns out that we can now implement >> `refs_verify_refname_available()` based on the other virtual >> functions, so there is no need for it to be defined at the b

Re: [PATCH v2 10/20] ref-cache: introduce a new type, ref_cache

2017-04-15 Thread Michael Haggerty
On 04/07/2017 01:32 PM, Duy Nguyen wrote: > On Fri, Mar 31, 2017 at 9:11 PM, Michael Haggerty > wrote: >> +void free_ref_cache(struct ref_cache *cache) >> +{ >> + free_ref_entry(cache->root); >> + free(cache); >> +} > > free(NULL) is no-o

Re: [PATCH v2 11/20] refs: record the ref_store in ref_cache, not ref_dir

2017-04-15 Thread Michael Haggerty
On 04/07/2017 01:38 PM, Duy Nguyen wrote: > On Fri, Mar 31, 2017 at 9:11 PM, Michael Haggerty > wrote: >> Instead of keeping a pointer to the ref_store in every ref_dir entry, >> store it once in `struct ref_cache`, and change `struct ref_dir` to >> include a pointer to i

Re: [PATCH v2 19/20] files_pack_refs(): use reference iteration

2017-04-15 Thread Michael Haggerty
On 04/07/2017 01:51 PM, Duy Nguyen wrote: > On Fri, Mar 31, 2017 at 9:11 PM, Michael Haggerty > wrote: >> Use reference iteration rather than do_for_each_entry_in_dir() in the >> definition of files_pack_refs(). > > A "why" is missing here. My guess is readabil

Re: [PATCH v2 00/20] Separate `ref_cache` into a separate module

2017-04-15 Thread Michael Haggerty
On 04/07/2017 01:53 PM, Duy Nguyen wrote: > On Wed, Apr 5, 2017 at 9:03 PM, Duy Nguyen wrote: >> On Sat, Apr 1, 2017 at 12:16 PM, Michael Haggerty >> wrote: >>> Duy, have you looked over my patch series? Since you've been working in >>> the area, your feedb

[PATCH v3 00/20] Separate `ref_cache` into a separate module

2017-04-15 Thread Michael Haggerty
-ref-cache". These patches depend on Duy's nd/files-backend-git-dir branch. [1] http://public-inbox.org/git/cover.1490966385.git.mhag...@alum.mit.edu/ [2] http://public-inbox.org/git/cover.1490026594.git.mhag...@alum.mit.edu/ [3] https://github.com/mhagger/git Michael Haggerty (20): get_r

[PATCH v3 01/20] get_ref_dir(): don't call read_loose_refs() for "refs/bisect"

2017-04-15 Thread Michael Haggerty
lazy mechanism, and this time the read was done correctly. This code has been broken since it was first introduced. Signed-off-by: Michael Haggerty --- refs/files-backend.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index 4d705b4037..2a0

[PATCH v3 19/20] files_pack_refs(): use reference iteration

2017-04-15 Thread Michael Haggerty
`pack_refs_cb_data` to preserve intermediate state. This removes the last callers of `entry_resolves_to_object()` and `get_loose_ref_dir()`, so delete those functions. Signed-off-by: Michael Haggerty --- refs/files-backend.c | 143 +-- 1 file changed, 60

[PATCH v3 13/20] refs: handle "refs/bisect/" in `loose_fill_ref_dir()`

2017-04-15 Thread Michael Haggerty
That "refs/bisect/" has to be handled specially when filling the ref_cache for loose references is a peculiarity of the files backend, and the ref-cache code shouldn't need to know about it. So move this code to the callback function, `loose_fill_ref_dir()`. Signed-off-by: M

[PATCH v3 11/20] refs: record the ref_store in ref_cache, not ref_dir

2017-04-15 Thread Michael Haggerty
`. So change `create_dir_entry()` to take a `ref_cache` parameter, and change its callers to pass the correct `ref_cache` depending on the purpose of the new `dir_entry`. Signed-off-by: Michael Haggerty --- refs/files-backend.c | 6 +++--- refs/ref-cache.c | 12 +++- refs/ref-cache.h

[PATCH v3 10/20] ref-cache: introduce a new type, ref_cache

2017-04-15 Thread Michael Haggerty
For now, it just wraps a `ref_entry *` that points at the root of the tree. Soon it will hold more information. Add two new functions, `create_ref_cache()` and `free_ref_cache()`. Make `free_ref_entry()` private. Change files-backend to use this type to hold its caches. Signed-off-by: Michael

[PATCH v3 18/20] commit_packed_refs(): use reference iteration

2017-04-15 Thread Michael Haggerty
ode or the new. Signed-off-by: Michael Haggerty --- refs/files-backend.c | 38 +- 1 file changed, 17 insertions(+), 21 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index 8c360869c1..1419512d51 100644 --- a/refs/files-backend.c +++ b/re

[PATCH v3 08/20] ref-cache: rename `remove_entry()` to `remove_entry_from_dir()`

2017-04-15 Thread Michael Haggerty
This function's visibility is about to be increased, so give it a more distinctive name. Signed-off-by: Michael Haggerty --- refs/files-backend.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index 6e08bc798c..f980a

[PATCH v3 15/20] get_loose_ref_dir(): function renamed from get_loose_refs()

2017-04-15 Thread Michael Haggerty
The new name is more analogous to `get_packed_ref_dir()`. Signed-off-by: Michael Haggerty --- refs/files-backend.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index c0550ad9d6..3beab0b752 100644 --- a/refs/files-backend.c

[PATCH v3 20/20] do_for_each_entry_in_dir(): delete function

2017-04-15 Thread Michael Haggerty
Its only remaining caller was itself. Signed-off-by: Michael Haggerty --- refs/ref-cache.c | 21 - refs/ref-cache.h | 11 --- 2 files changed, 32 deletions(-) diff --git a/refs/ref-cache.c b/refs/ref-cache.c index b3a30350d7..6059362f1d 100644 --- a/refs/ref-cache.c

[PATCH v3 06/20] ref-cache: rename `add_ref()` to `add_ref_entry()`

2017-04-15 Thread Michael Haggerty
This function's visibility is about to be increased, so give it a more distinctive name. Signed-off-by: Michael Haggerty --- refs/files-backend.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index cf1c18cffb..05029

[PATCH v3 05/20] refs_verify_refname_available(): use function in more places

2017-04-15 Thread Michael Haggerty
(and will be regained later). These were the last callers of `verify_refname_available_dir()`, so also delete that (very complicated) function. Signed-off-by: Michael Haggerty --- refs/files-backend.c | 171 --- 1 file changed, 11 insertions(+)

[PATCH v3 14/20] do_for_each_entry_in_dir(): eliminate `offset` argument

2017-04-15 Thread Michael Haggerty
It was never used. Signed-off-by: Michael Haggerty --- refs/files-backend.c | 4 ++-- refs/ref-cache.c | 6 +++--- refs/ref-cache.h | 11 +-- 3 files changed, 10 insertions(+), 11 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index 079ba941ef

[PATCH v3 12/20] ref-cache: use a callback function to fill the cache

2017-04-15 Thread Michael Haggerty
supply a pointer to function `read_loose_refs` (renamed to `loose_fill_ref_dir`) when creating the ref cache for its loose refs. This means that we can generify the type of the back-pointer in `struct ref_cache` from `files_ref_store` to `ref_store`. Signed-off-by: Michael Haggerty --- refs/files

[PATCH v3 16/20] get_loose_ref_cache(): new function

2017-04-15 Thread Michael Haggerty
Extract a new function, `get_loose_ref_cache()`, from get_loose_ref_dir(). The function returns the `ref_cache` for the loose refs of a `files_ref_store`. Signed-off-by: Michael Haggerty --- refs/files-backend.c | 9 +++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/refs

[PATCH v3 02/20] refs_read_raw_ref(): new function

2017-04-15 Thread Michael Haggerty
Extract a new function from `refs_resolve_ref_unsafe()`. It will be useful elsewhere. Signed-off-by: Michael Haggerty --- refs.c | 11 +-- refs/refs-internal.h | 4 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/refs.c b/refs.c index bad05ba861

[PATCH v3 09/20] refs: split `ref_cache` code into separate files

2017-04-15 Thread Michael Haggerty
, adds declarations, and changes the visibility of some functions, but doesn't change any code. The modules are still too tightly coupled, but the situation will be improved in subsequent commits. Signed-off-by: Michael Haggerty --- Makefile | 1 + refs/files-backend.c

[PATCH v3 07/20] ref-cache: rename `find_ref()` to `find_ref_entry()`

2017-04-15 Thread Michael Haggerty
This function's visibility is about to be increased, so give it a more distinctive name. Signed-off-by: Michael Haggerty --- refs/files-backend.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index 05029d43b8..6e08b

[PATCH v3 17/20] cache_ref_iterator_begin(): make function smarter

2017-04-15 Thread Michael Haggerty
ator_begin()` to be made more ignorant of the internals of `ref_cache`, and `find_containing_dir()` and `prime_ref_dir()` to be made private. Signed-off-by: Michael Haggerty --- refs/files-backend.c | 44 +--- refs/ref-cache.

[PATCH v3 04/20] refs_verify_refname_available(): implement once for all backends

2017-04-15 Thread Michael Haggerty
It turns out that we can now implement `refs_verify_refname_available()` based on the other virtual functions, so there is no need for it to be defined at the backend level. Instead, define it once in `refs.c` and remove the `files_backend` definition. Signed-off-by: Michael Haggerty --- refs.c

[PATCH v3 03/20] refs_ref_iterator_begin(): new function

2017-04-15 Thread Michael Haggerty
Extract a new function from `do_for_each_ref()`. It will be useful elsewhere. Signed-off-by: Michael Haggerty --- refs.c | 15 +-- refs/refs-internal.h | 11 +++ 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/refs.c b/refs.c index aa461156c4

Re: [PATCH v8 0/5] [GSoC] remove_subtree(): reimplement using iterators

2017-04-16 Thread Michael Haggerty
-21504-1-git-send-email-bnm...@gmail.com/T/#t > v7: > https://public-inbox.org/git/1491163388-41255-1-git-send-email-bnm...@gmail.com/T/#t > > Travis CI build: https://travis-ci.org/theiostream/git/jobs/21982 > > In this version, I applied pretty much all suggestions Michael an

Re: [PATCH v8 4/5] dir_iterator: refactor state machine model

2017-04-16 Thread Michael Haggerty
RDER_TRAVERSAL; > + else if (!strcmp(*myargv, "--post-order")) > + flag |= DIR_ITERATOR_POST_ORDER_TRAVERSAL; > + else if (!strcmp(*myargv, "--list-root-dir")) > + flag |= DIR_ITERATOR_LIST_ROOT_DIR; > + else if (!strcmp(*myargv, "--")) { > + myargc--; > + myargv++; > + break; > + } else > +die("Unrecognized option: %s", *myargv); The indentation above is wrong. > + } > > - strbuf_add(&path, argv[1], strlen(argv[1])); > + if (myargc != 1) > + die("expected exactly one non-option argument"); > + strbuf_addstr(&path, *myargv); > > - diter = dir_iterator_begin(path.buf); > + diter = dir_iterator_begin(path.buf, flag); > > while (dir_iterator_advance(diter) == ITER_OK) { > if (S_ISDIR(diter->st.st_mode)) > [...] Michael

Re: [PATCH] files_for_each_reflog_ent_reverse(): close stream and free strbuf on error

2017-04-17 Thread Michael Haggerty
log for %s: > %s", > - cnt, refname, strerror(errno)); > + if (nread != 1) { > + ret = error("cannot read %d bytes from reflog for %s: > %s", > + cnt, refname, strerror(errno)); > + break; > + } > pos -= cnt; > > scanp = endp = buf + cnt; > Reviewed-by: Michael Haggerty Michael

Re: [PATCH v10 4/5] dir_iterator: rewrite state machine model

2017-04-20 Thread Michael Haggerty
fs/files-backend.c to pass > the flags parameter introduced, as well as handle the case in which it > fails to open the directory. > > Improve t/t0065-dir-iterator.sh and t/helper/test-dir-iterator.c to > test "post-order" and "iterate-over-root" modes. > &

Re: [PATCH] refs.h: rename submodule arguments to submodule_path

2017-04-20 Thread Michael Haggerty
On 04/21/2017 03:12 AM, Junio C Hamano wrote: > Stefan Beller writes: > >> + Junio > > Just like Michael, I do not have strong enough opinion for or > against this patch to comment on it. > > I do agree with you that it would be a good longer-term direction to >

Re: [PATCH] refs.h: rename submodule arguments to submodule_path

2017-04-20 Thread Michael Haggerty
On 04/21/2017 08:32 AM, Michael Haggerty wrote: > [...] > I've CCed Duy because I don't know whether he has more plans regarding > submodule references [...] get rid of the > `for_each_ref_submodule()` family of functions entirely. > > So perhaps the code that this p

Re: [PATCH v4 3/5] refs: introduce get_worktree_ref_store()

2017-04-21 Thread Michael Haggerty
nstant appears another place, too. It might make sense to define a constant `REF_STORE_ALL_CAPABILITIES` in `refs-internal.h` alongside the individual bit values. If you prefer not to, please at least declare this variable `const` to spare the reader the trouble of looking to see whether it is modified before it is used. Otherwise, looks fine to me. > [...] Michael

Re: [PATCH v4 0/5] Kill manual ref parsing code in worktree.c

2017-04-21 Thread Michael Haggerty
;> I mentioned about moving these hashmap back to submodule.c and >> worktree.c where it can map more than just ref stores (in worktree >> case, we can cache 'struct worktree', for example). But I'm not doing >> it now to keep things small. >> >> The com

Re: [PATCH v3 04/12] refs.c: refactor get_submodule_ref_store(), share common free block

2017-04-21 Thread Michael Haggerty
dule); > - if (ret) { > - strbuf_release(&submodule_sb); > - return NULL; > - } > + if (ret) > + goto done; After this change, the temporary variable `ret` could be eliminated. > [...] Michael

Re: [PATCH v3 05/12] refs: move submodule slash stripping code to get_submodule_ref_store

2017-04-21 Thread Michael Haggerty
hat's what the code did before this patch, but it seems to me more like an accident of the old design rather than something worth supporting. In other words, if a caller would really pass us such a string, it seems like we could declare the caller buggy, no? > [...] Otherwise, looks good and makes a lot of sense. Michael

Re: [PATCH v3 08/12] refs: remove dead for_each_*_submodule()

2017-04-21 Thread Michael Haggerty
On 04/19/2017 01:01 PM, Nguyễn Thái Ngọc Duy wrote: > These are used in revision.c. After the last patch they are replaced > with the refs_ version. Delete them (except for_each_remote_ref_submodule > which is still used by submodule.c) ❤❤ I love the way this is going. Michael

Re: [PATCH v3 09/12] revision.c: --all adds HEAD from all worktrees

2017-04-21 Thread Michael Haggerty
ktrees (even if that's not possible now, maybe they will have done it with some future version of git or with another tool). If the problem is only that this version of git is too stupid to handle pruning safely in that situation, it would be more appropriate to use something more like if (!refs->single_worktree) die("error: git is currently unable to handle submodules that use linked worktrees"); > refs = get_submodule_ref_store(submodule); > } else > refs = get_main_ref_store(); > [...] Michael

Re: [PATCH v3 06/12] refs: add refs_head_ref()

2017-04-21 Thread Michael Haggerty
*cb_data); > int refs_for_each_ref(struct ref_store *refs, > each_ref_fn fn, void *cb_data); > int refs_for_each_ref_in(struct ref_store *refs, const char *prefix, > I'm seeing segfaults in t3600 after this patch, apparently because `refs==NULL` gets passed from `head_ref_submodule()` to `refs_head_ref()`. Michael

Re: [PATCH v3 10/12] files-backend: make reflog iterator go through per-worktree reflog

2017-04-22 Thread Michael Haggerty
", (*diter)->path.buf); continue; } iter->base.oid = &iter->oid; iter->base.flags = flags; return ITER_OK; } if (ref_iterator_abort(ref_iterator) == ITER_ERROR) return ITER_ERROR; return ok; } > if (ref_iterator_abort(ref_iterator) == ITER_ERROR) > ok = ITER_ERROR; > return ok; > [...] Michael

Re: [PATCH v3 00/12] Fix git-gc losing objects in multi worktree

2017-04-22 Thread Michael Haggerty
leaner distinction between "logical" and "physical" ref_stores. But given the current state of the code, your implementation is reasonable. Michael

Re: [PATCH 38/53] refs: convert struct ref_update to use struct object_id

2017-04-26 Thread Michael Haggerty
On 04/23/2017 11:34 PM, brian m. carlson wrote: > Convert struct ref_array_item to use struct object_id by changing the > definition and applying the following semantic patch, plus the standard > object_id transforms: > [...] This commit LGTM. Michael

Re: [PATCH 39/53] refs/files-backend: convert many internals to struct object_id

2017-04-26 Thread Michael Haggerty
strbuf will be NUL-terminated and that parse_oid_hex will fail on > truncated input to avoid the need to check for an explicit length. > > This is a requirement to convert parse_object later on. > [...] This patch also looks fine to me. Michael

Re: [PATCHv2 0/3] Make diff plumbing commands respect the indentHeuristic.

2017-04-29 Thread Michael Haggerty
>> diff: have the diff-* builtins configure diff before initializing >> revisions >> >> Stefan Beller (1): >> diff: enable indent heuristic by default > > Thanks, these look fine to me. I'd like to get an ACK from Michael, in > case he had some

Re: Git 2.13.0 segfaults on Solaris SPARC due to DC_SHA1=YesPlease being on by default

2017-05-15 Thread Michael Kebe
Hi Marc, works like a charm! Michael 2017-05-15 16:33 GMT+02:00 Marc Stevens : > Hi Michael, > > > > See the latest commit to the SHA1DC repo: > > https://github.com/cr-marcstevens/sha1collisiondetection/commit/33a694a9ee1b79c24be45f9eab5ac0e1aeeaf271 > > Just appl

Performance regression in `git branch` due to ref-filter usage

2017-05-17 Thread Michael Haggerty
ether other commands have similar regressions. Michael [1] One wonders why the file has to be read more than once, but that's a different story and probably harder to fix.

[PATCH 00/23] Prepare to separate out a packed_ref_store

2017-05-17 Thread Michael Haggerty
d like to see a preview of the rest of the changes (which works but is not yet polished), checkout the `mmap-packed-refs` branch from the same place. Michael [1] http://public-inbox.org/git/cover.1490026594.git.mhag...@alum.mit.edu/ http://public-inbox.org/git/cover.1490966385.git.mhag...@a

[PATCH 01/23] t3600: clean up permissions test properly

2017-05-17 Thread Michael Haggerty
`test_must_fail` block so that it can't be skipped. Signed-off-by: Michael Haggerty --- t/t3600-rm.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh index 5f9913ba33..4a35c378c8 100755 --- a/t/t3600-rm.sh +++ b/t/t3600-rm.sh @@ -98,8

[PATCH 04/23] prefix_ref_iterator: don't trim too much

2017-05-17 Thread Michael Haggerty
7;s be cautious here. Skip over any references whose names are not longer than `trim`. Signed-off-by: Michael Haggerty --- refs/iterator.c | 14 +- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/refs/iterator.c b/refs/iterator.c index bce1f192f7..f33d1b3a39 100644 --- a/

[PATCH 03/23] ref_iterator_begin_fn(): fix docstring

2017-05-17 Thread Michael Haggerty
, `cache_ref_iterator_begin()` (from which the files reference iterator gets its values) automatically wraps its output using `prefix_ref_iterator_begin()` when necessary, so it has the stricter behavior. Signed-off-by: Michael Haggerty --- refs/refs-internal.h | 4 ++-- 1 file changed, 2 insertions

[PATCH 02/23] refs.h: clarify docstring for the ref_transaction_update()-related fns

2017-05-17 Thread Michael Haggerty
In particular, make it clear that they make copies of the sha1 arguments. Signed-off-by: Michael Haggerty --- refs.h | 13 + 1 file changed, 13 insertions(+) diff --git a/refs.h b/refs.h index d18ef47128..a7d7b1afdf 100644 --- a/refs.h +++ b/refs.h @@ -427,6 +427,19 @@ struct

[PATCH 07/23] ref_store: take `logmsg` parameter when deleting references

2017-05-17 Thread Michael Haggerty
Just because the files backend can't retain reflogs for deleted references is no reason that they shouldn't be supported by the virtual method interface. Let's add them now before the interface becomes truly polymorphic and increases the work. Signed-off-by: Michael Haggerty ---

[PATCH 16/23] should_pack_ref(): new function, extracted from `files_pack_refs()`

2017-05-17 Thread Michael Haggerty
Extract a function for deciding whether a reference should be packed. It is a self-contained bit of logic, so splitting it out improves readability. Signed-off-by: Michael Haggerty --- refs/files-backend.c | 42 -- 1 file changed, 28 insertions(+), 14

[PATCH 09/23] files-backend: move `lock` member to `files_ref_store`

2017-05-17 Thread Michael Haggerty
Move the `lock` member from `packed_ref_cache` to `files_ref_store`, since at most one cache can have a locked "packed-refs" file associated with it. Rename it to `packlock` to make its purpose clearer in its new home. More changes are coming here shortly. Signed-off-by: Michae

[PATCH 12/23] ref_transaction_commit(): break into multiple functions

2017-05-17 Thread Michael Haggerty
sub-transactions. Only if all of the "prepare" steps succeed would we "finish" each of them. Signed-off-by: Michael Haggerty --- refs.c | 34 ++--- refs.h | 37 ++- refs/files-backend.c | 71 +

[PATCH 05/23] refs_ref_iterator_begin(): don't check prefixes redundantly

2017-05-17 Thread Michael Haggerty
The backend already takes care of the prefix. By passing the prefix again to `prefix_ref_iterator`, we were forcing that iterator to do redundant prefix comparisons. So set it to the empty string. Signed-off-by: Michael Haggerty --- refs.c | 8 +++- 1 file changed, 7 insertions(+), 1

[PATCH 17/23] get_packed_ref_cache(): assume "packed-refs" won't change while locked

2017-05-17 Thread Michael Haggerty
If we've got the "packed-refs" file locked, then it can't change; there's no need to keep calling `stat_validity_check()` on it. Signed-off-by: Michael Haggerty --- refs/files-backend.c | 16 +++- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git

[PATCH 10/23] files_ref_store: put the packed files lock directly in this struct

2017-05-17 Thread Michael Haggerty
p track of whether it is locked. This keeps related data together and makes the main reference store less of a special case. Signed-off-by: Michael Haggerty --- refs/files-backend.c | 29 +++-- 1 file changed, 11 insertions(+), 18 deletions(-) diff --git a/refs/files-backen

[PATCH 19/23] read_packed_refs(): report unexpected fopen() failures

2017-05-17 Thread Michael Haggerty
re such problems. So report any failures that are not due to ENOENT. Signed-off-by: Michael Haggerty --- refs/files-backend.c | 15 +-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index 6a037e1d61..eb74d1119a 100644 --- a/

[PATCH 18/23] read_packed_refs(): do more of the work of reading packed refs

2017-05-17 Thread Michael Haggerty
Teach `read_packed_refs()` to also * Allocate and initialize the new `packed_ref_cache` * Open and close the `packed-refs` file * Update the `validity` field of the new object This decreases the coupling between `packed_refs_cache` and `files_ref_store` by a little bit. Signed-off-by: Michael

[PATCH 06/23] refs: use `size_t` indexes when iterating over ref transaction updates

2017-05-17 Thread Michael Haggerty
Signed-off-by: Michael Haggerty --- refs.c | 2 +- refs/files-backend.c | 6 -- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/refs.c b/refs.c index f4a485cd8a..ea8233c67d 100644 --- a/refs.c +++ b/refs.c @@ -848,7 +848,7 @@ struct ref_transaction

[PATCH 23/23] cache_ref_iterator_begin(): avoid priming unneeded directories

2017-05-17 Thread Michael Haggerty
e latter yet, because it might be useful for another patch series that I'm working on. Signed-off-by: Michael Haggerty --- refs/ref-cache.c | 93 ++-- 1 file changed, 83 insertions(+), 10 deletions(-) diff --git a/refs/ref-cache.c b/refs/ref

[PATCH 14/23] ref_update_reject_duplicates(): use `size_t` rather than `int`

2017-05-17 Thread Michael Haggerty
Signed-off-by: Michael Haggerty --- refs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/refs.c b/refs.c index 43d65bc9c6..ffc9bd0be5 100644 --- a/refs.c +++ b/refs.c @@ -1692,7 +1692,7 @@ int create_symref(const char *ref_target, const char *refs_heads_master, int

[PATCH 21/23] create_ref_entry(): remove `check_name` option

2017-05-17 Thread Michael Haggerty
Only one caller was using it, so move the check to that caller. Signed-off-by: Michael Haggerty --- refs/files-backend.c | 12 refs/ref-cache.c | 6 +- refs/ref-cache.h | 3 +-- 3 files changed, 10 insertions(+), 11 deletions(-) diff --git a/refs/files-backend.c b

[PATCH 08/23] lockfile: add a new method, is_lock_file_locked()

2017-05-17 Thread Michael Haggerty
It will soon prove useful. Signed-off-by: Michael Haggerty --- lockfile.h | 8 1 file changed, 8 insertions(+) diff --git a/lockfile.h b/lockfile.h index 7b715f9e77..572064939c 100644 --- a/lockfile.h +++ b/lockfile.h @@ -175,6 +175,14 @@ static inline int hold_lock_file_for_update

[PATCH 15/23] ref_update_reject_duplicates(): add a sanity check

2017-05-17 Thread Michael Haggerty
It's pretty cheap to make sure that the caller didn't pass us an unsorted list by accident, so do so. Signed-off-by: Michael Haggerty --- refs.c | 10 -- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/refs.c b/refs.c index ffc9bd0be5..68a0872562 100644 --- a/re

[PATCH 11/23] files_transaction_cleanup(): new helper function

2017-05-17 Thread Michael Haggerty
Extract function from `files_transaction_commit()`. It will soon have another caller. Signed-off-by: Michael Haggerty --- refs/files-backend.c | 33 - 1 file changed, 24 insertions(+), 9 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index

[PATCH 20/23] refs_ref_iterator_begin(): handle `GIT_REF_PARANOIA`

2017-05-17 Thread Michael Haggerty
Instead of handling `GIT_REF_PARANOIA` in `files_ref_iterator_begin()`, handle it in `refs_ref_iterator_begin()`, where it will cover all reference stores. Signed-off-by: Michael Haggerty --- refs.c | 5 + refs/files-backend.c | 11 --- 2 files changed, 9 insertions

[PATCH 22/23] ref-filter: limit traversal to prefix

2017-05-17 Thread Michael Haggerty
t probably doesn't come up that often, and (b) it is more awkward to deal with multiple patterns (e.g., the patterns might not be disjoint). So, since this is just an optimization, punt on the case of multiple patterns. Signed-off-by: Jeff King Signed-off-by: Michael Haggerty --- re

[PATCH 13/23] ref_update_reject_duplicates(): expose function to whole refs module

2017-05-17 Thread Michael Haggerty
It will soon have some other users. Signed-off-by: Michael Haggerty --- refs.c | 17 + refs/files-backend.c | 17 - refs/refs-internal.h | 8 3 files changed, 25 insertions(+), 17 deletions(-) diff --git a/refs.c b/refs.c index 689362db1e

Re: [PATCH v3 10/12] files-backend: make reflog iterator go through per-worktree reflog

2017-05-17 Thread Michael Haggerty
Hi, I put off reviewing this patch, thinking that it would appear in a re-roll, then never came back to it. :-( On 04/23/2017 06:44 AM, Duy Nguyen wrote: > On Sat, Apr 22, 2017 at 10:05:02AM +0200, Michael Haggerty wrote: >> I find this implementation confusing: >>

Re: [PATCH 01/23] t3600: clean up permissions test properly

2017-05-17 Thread Michael Haggerty
On 05/17/2017 02:42 PM, Jeff King wrote: > On Wed, May 17, 2017 at 02:05:24PM +0200, Michael Haggerty wrote: > >> The test of failing `git rm -f` removes the write permissions on the >> test directory, but fails to restore them if the test fails. This >> means that the

Re: [PATCH 04/23] prefix_ref_iterator: don't trim too much

2017-05-17 Thread Michael Haggerty
On 05/17/2017 02:55 PM, Jeff King wrote: > On Wed, May 17, 2017 at 02:05:27PM +0200, Michael Haggerty wrote: > >> diff --git a/refs/iterator.c b/refs/iterator.c >> index bce1f192f7..f33d1b3a39 100644 >> --- a/refs/iterator.c >> +++ b/refs/iterator.c >

Re: [PATCH 05/23] refs_ref_iterator_begin(): don't check prefixes redundantly

2017-05-17 Thread Michael Haggerty
On 05/17/2017 02:59 PM, Jeff King wrote: > On Wed, May 17, 2017 at 02:05:28PM +0200, Michael Haggerty wrote: > >> The backend already takes care of the prefix. By passing the prefix >> again to `prefix_ref_iterator`, we were forcing that iterator to do >> redundant prefi

Re: [PATCH 07/23] ref_store: take `logmsg` parameter when deleting references

2017-05-17 Thread Michael Haggerty
On 05/17/2017 03:12 PM, Jeff King wrote: > On Wed, May 17, 2017 at 02:05:30PM +0200, Michael Haggerty wrote: > >> Just because the files backend can't retain reflogs for deleted >> references is no reason that they shouldn't be supported by the >> virtual me

Re: [PATCH 10/23] files_ref_store: put the packed files lock directly in this struct

2017-05-17 Thread Michael Haggerty
On 05/17/2017 03:17 PM, Jeff King wrote: > On Wed, May 17, 2017 at 02:05:33PM +0200, Michael Haggerty wrote: > >> Instead of using a global `lock_file` instance for the main >> "packed-refs" file and using a pointer in `files_ref_store` to keep >> track

Re: [PATCH 19/23] read_packed_refs(): report unexpected fopen() failures

2017-05-17 Thread Michael Haggerty
On 05/17/2017 03:28 PM, Jeff King wrote: > On Wed, May 17, 2017 at 02:05:42PM +0200, Michael Haggerty wrote: > >> The old code ignored any errors encountered when trying to fopen the >> "packed-refs" file, treating all such failures as if the file didn't >&g

<    1   2   3   4   5   6   7   8   9   10   >