git gc and worktrees

Earlier this year I had a largish merge going on in a separate worktree. With a mix of staged resolutions and unmerged paths in the index, I ran 'git gc' in the main worktree. This removed a lot of objects that were recorded in that separate worktree index. (I was able to recover them because the content was still on disk.)

Now I wanted to derive a test case that shows that breakage, but I am unable to. The objects recorded in a separate worktree index, but not committed, yet, are not pruned anymore.

Have there been any fixes in this regard recently? Or does this look like something else going on?

-- Hannes

On Tue, May 31, 2016 at 2:07 PM, Johannes Sixt <j6t@kdbg.org> wrote:

Not sure. I vaguely recall Jeff touched this pruning issue once, something about recent objects will not be pruned based on mtime. But maybe some of those objects in the index are not so young, and because I think we never check indexes in git-gc/git-prune, they have a chance to be deleted.

No I'm wrong. mark_reachable_objects() which is used by git-prune, does add objects from index and HEAD, which only covers those from _current_ worktree. This should be fixed even if it is not the root cause of your problem. I'll look into it.

On Tue, May 31, 2016 at 07:02:15PM +0700, Duy Nguyen wrote:

Right, we use the index for reachability checks (both in "prune", but also during a "repack -a", which uses the revision parser's '--indexed-objects" option). That obviously should handle per-worktree index files, but I don't know whether it currently does.

Michael (cc'd) noted to me off-list recently that there may be some special cases there regarding reflogs in other worktrees (i.e., that we don't always include them for our reachability checks). I don't know the details, though.

The "recent object" stuff you're thinking of is that we will keep a non-recent object X if it is referenced (directly or indirectly) by another object Y that _is_ recent, but not otherwise reachable. That should work independent of worktrees, because it finds the recent objects directly in the object database, and then walks their graphs (but it could indeed make writing tests a little trickier, as it may be harder to convince some objects to get pruned in the first place).

-Peff

Am 01.06.2016 um 00:14 schrieb Jeff King:

Thanks. Here's a test case to make sure that --indexed-objects looks at the indexes of separate worktrees. I'm not submitting a proper patch because I don't feel like being able to fix the problem and I'm not sure what the expected order of the listed objects is.

On 06/01/2016 12:14 AM, Jeff King wrote:

That's correct. `for_each_reflog()` does its work by walking the directory tree under `$GIT_DIR/logs`. So when run in the main repository, it never looks at the per-worktree reflogs. Therefore, objects that are only reachable from per-worktree reflogs can end up getting pruned.

This is closely tangled up with my ref-iterators patch series (which is why I noticed it), especially with

    [PATCH 13/13] for_each_reflog(): reimplement using iterators

[1].

I think reference stores are going to need two distinct types of reference iteration: one to iterate over the *logical* reference space of a single repo or worktree, and one to find all *local* references and/or reachability roots (e.g., when run within a linked repo). The current approach, where subtrees of the reference namespace are pasted ad-hoc into each other partly by manhandling the ref_cache and partly via the hack in `git_path()` is not, I think, a very good long-term design.

I'd prefer the elementary operation on a low-level reference store to be iterating over all of the references that are stored locally, and to use `merge_ref_iterator` for pasting together the parts of low-level reference stores that are needed to form a logical view of the references in a linked repo.

Michael

[1] http://thread.gmane.org/gmane.comp.version-control.git/295860/focus=295866

Michael Haggerty <mhagger@alum.mit.edu> writes:

This is hard reason about without defining *logical* and *local*.

For protecting necessary objects from pruning by anchoring them as "reachable", and for avoiding unnecessary object transfer by showing things via ".have" lines, we're better consider references and reflogs that do not appear in the namespace visible from the current worktree. I wanted to come up with an example of wanting to iterate over refs of the current worktree, but other than "what are my branches including HEAD?" I didn't think of anything interesting.

This I agree with.

Thanks.

On 06/01/2016 05:15 PM, Junio C Hamano wrote:

By "logical" I mean the references that appear to be available in the specified repository. Roughly, "what reference names can I pass to `git rev-parse`?" If you are in a linked repo, your "logical" references are most of those in the main repo plus the "HEAD" and "refs/bisect/*" and probably a couple other pseudo-refs that are stored in your worktree.

"Local" is probably a bad name. What I mean is that we have multiple physical places to store references. In particular, in the case of linked repos we have the main repo's references plus we have the worktree's local references.

Most Git commands care only about the "logical" references in the repo. But some commands (mostly having to do with reachability) need to know about all roots, and therefore have to know about the union of all of the references stored in each of the linked repositories.

Note that in the second case it is a little silly even talking about "references", because references in different linked repos might have the same names. We're really talking about reachability roots that might have a little metadata connected with them, like "this SHA-1 came from reference 'HEAD' in the 'test' worktree" or "this SHA-1 came from the reflog of 'refs/heads/master' in the main repo".

I argue that the fundamental concept in terms of the implementation should be the individual physical reference stores, and these should be compounded together to form the logical reference collections and the sets of reachability roots that are interesting at the UI level.

It doesn't show up much at the UI level, but there are some examples, like

* git log --decorate
* git for-each-ref --contains
* gitk --all

Michael

Michael Haggerty <mhagger@alum.mit.edu> writes:

That is very good in principle. How does that principle translate to the current setup (with possible enhancement with pluggable ref backends) and multiple worktrees? Let me try thinking it through aloud.

 * Without pluggable ref backend or worktrees, we start from two
   "physical reference stores"; packed-refs file lists refs that
   will be covered (overridden) by loose refs in .git/refs/.
   Symbolic refs always being in loose falls out as a natural
   consequence that packed-refs file does not record symrefs.

 * Throw in multiple worktrees to the mix.  How?  Do we consider
   selected refs/ hierarchies (like refs/bisect/*) as separate
   physical store (even though it might be backed by the files in
   the same .git/refs/ filesystem hierarchy) and represent the
   "logical" view as an overlay across the traditional two types of
   physical reference stores?  That is:

   - loose refs in .git/HEAD, .git/refs/{bisect,...} for
     per-worktree part form one physical store.  If a ref is found
     here, that is what we use as a part of the logical view.

   - loose refs in .git/refs/{branches,tags,notes,...} for common
     part form one physical store.  For a ref that is not found
     above but is found here becomes a part of the logical view.

   - packed refs in .git/packed-refs is another physical store.  For
     a ref that is not found in the above two but is found here
     becomes a part of the logical view.

Up to this point, I am all for your "separate physical stores are composited to give a logical view". I can see how multi-worktree world view fits within that framework.

 * With pluggable ref backend, we may gain yet another "physical
   reference store" possibility, e.g. one backed by lmdb.  If it
   supports symrefs, a repoitory may use lmdb backed reference store
   without the traditional two.

   But it is unclear how it would interact with the multi-worktree
   world order.

On 06/01/2016 09:39 PM, Junio C Hamano wrote:

I think I would represent the logical store of a worktree repo as follows. First, I would implement a cached_ref_store that introduces a layer of caching around another ref_store. Then

    def get_files_ref_store(dir) {
        loose = create_cached_ref_store(get_loose_ref_store(dir))
        packed = create_cached_ref_store(get_packed_ref_store(dir))
        return create_files_ref_store(loose, packed)
    }

    common_ref_store = get_files_ref_store(common_dir)

    /*
     * I think we only allow loose refs in worktrees; otherwise
     * this could be an overlay_ref_store too. Actually, we might
     * want to omit the caching here.
     */
    local_ref_store = create_cached_ref_store(
            get_loose_ref_store(git_dir))

    logical_ref_store = create_worktree_ref_store(
        local_ref_store, common_ref_store)

Where worktree_ref_store does something like

    if (is_per_worktree_ref(refname))
        lookup in local_ref_store
    else
        lookup in common_ref_store

for reading, and uses a merge_ref_iterator with a select function that does something similar for iterating.

The files_ref_store would do lookups by looking first in the loose_ref_store then in the packed_ref_store, would use an overlay_ref_iterator for iteration, and would know to do all writes in the loose_ref_store (except for deletes, which also have to go to packed_ref_store). It would have a special "pack-refs" operation, specific to files_ref_store, that shuffles references between its two backends.

Writing to a worktree_ref_store is a bit tricker, because we want to allow ref_transactions to span worktree and common refs (though we probably need to give up atomicity for any such transaction). The worktree_ref_transaction_commit() method has to split the main transaction into two sub-transactions, one for each of its component ref_stores. I planned for this when designing split_under_lock and think it is possible, though I admit I haven't implemented it yet.

One nice thing about this design is that you can skip the worktree_ref_store layer and its overhead entirely for repositories that are not linked. The decision can be made once, at instantiation time, rather than every time a reference is looked up. See the pseudocode below.

Since you could plug-and-play different ref_stores in the above scheme, I don't see any problem here.

    def get_logical_ref_store() {
        local_ref_store = get_local_ref_store(git_dir)
        if (is_linked_repo) {
            common_ref_store = get_ref_store(common_dir)
            return worktree_ref_store(local_ref_store,
                                      common_ref_store)
        } else {
            return local_ref_store;
        }
    }

get_ref_store() would read the git config to decide what the ref store to use for the specified repository, which itself might be an lmdb_ref_store or an overlay_ref_store(loose_ref_store, packed_ref_store).

Michael

Michael Haggerty <mhagger@alum.mit.edu> writes:

Sounds all sensible.  Thanks.

This series makes sure that objects referenced by all worktrees are marked reachable so that we don't accidentally delete objects that are being used. Previously per-worktree references in index, detached HEAD or per-worktree reflogs come from current worktree only, not all worktrees.

The series deals with git-prune and git-gc specifically. I left out "git rev-list". It shares the same problem because it will only consider current worktree's HEAD, index and per-worktree reflogs. The problem is I am not sure if we simply just change, say --indexed-objects, to cover all indexes, or should we only do that with "--all-worktrees --indexed-objects". I guess this is up for discussion.

Nguyễn Thái Ngọc Duy (4):
  revision.c: move read_cache() out of add_index_objects_to_pending()
  reachable.c: mark reachable objects in index from all worktrees
  reachable.c: mark reachable detached HEAD from all worktrees
  reachable.c: make reachable reflogs for all per-worktree reflogs

 reachable.c      | 47 +++++++++++++++++++++++++++++++++++++++++------
 revision.c       | 34 +++++++++++++++++++++++++++-------
 revision.h       |  7 ++++++-
 t/t5304-prune.sh | 40 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 114 insertions(+), 14 deletions(-)

A library function like this should not change global state like the_index. Granted, read_cache() will not re-read from $GIT_DIR/index and lose all in-core changes if the index is dirty (i.e. no bad side effects). But that sort of detail should not be relied on here.

Make the function take the index object from outside instead. The caller will be responsible for reading index files if necessary.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 reachable.c |  3 ++-
 revision.c  | 15 ++++++++-------
 revision.h  |  4 +++-
 3 files changed, 13 insertions(+), 9 deletions(-)

Current mark_reachable_objects() only marks objects from index from _current_ worktree as reachable instead of all worktrees. Because this function is used for pruning, there is a chance that objects referenced by other worktrees may be deleted. Fix that.

Small behavior change in "one worktree" case, the index is read again from file. In the current implementation, if the_index is already loaded, the index file will not be read from file again. This adds some more cost to this operation, hopefully insignificant because reachability test is usually very expensive already.

Reported-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 reachable.c      | 33 +++++++++++++++++++++++++++++----
 t/t5304-prune.sh |  9 +++++++++
 2 files changed, 38 insertions(+), 4 deletions(-)

On Wed, Jun 1, 2016 at 6:45 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:

Could this extra index read be avoided by taking advantage of 'struct worktree::is_current'[1] and passing the already-loaded index to add_index_objects_to_pending() if true?

Or, am I misunderstanding the issue?

[1]: http://article.gmane.org/gmane.comp.version-control.git/292194

On Thu, Jun 2, 2016 at 1:13 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:

Hah! I broke my worktree changes into many pieces and lost track of them. I thought that one was still in some unsent series. Will use it.

On Wed, 2016-06-01 at 17:45 +0700, Nguyễn Thái Ngọc Duy wrote:

Why not just struct index_state istate = {0}; ?

On Thu, Jun 2, 2016 at 1:57 AM, David Turner <dturner@twopensource.com> wrote:

My first thought was.. "hmm.. C99?" but then there are 24 of them in the code base already. Will change.

Same reason as the previous commit, this prevents potential object deletion that other worktrees need.

There is a slight change in behavior. The current implementation always adds HEAD. The new one only adds _detached_ HEAD. But that's fine because all normal refs should be already covered by for_each_ref() in mark_reachable_objects().

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 reachable.c      | 14 +++++++++++---
 t/t5304-prune.sh | 12 ++++++++++++
 2 files changed, 23 insertions(+), 3 deletions(-)

Same reason as the previous commit, this is to avoid deleting objects being referenced by per-worktree reflogs from other worktrees. "logs/HEAD" is most important. "logs/refs/bisect" should not live long enough to matter, but let's add it too for safety.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 reachable.c      |  7 ++++---
 revision.c       | 19 +++++++++++++++++++
 revision.h       |  3 +++
 t/t5304-prune.sh | 19 +++++++++++++++++++
 4 files changed, 45 insertions(+), 3 deletions(-)

On 06/01/2016 12:45 PM, Nguyễn Thái Ngọc Duy wrote:

`refs/bisect` is not a single reference. It is a namespace that contains references with names like `refs/bisect/bad` and `refs/bisect/good-66106691a1b71e445fe5e4d6b8b043dffc7dfe4c`.

Michael

On Wed, Jun 01, 2016 at 05:45:15PM +0700, Nguyễn Thái Ngọc Duy wrote:

I don't think touching reachable.c is enough. You also need to make sure that calling "git pack-objects --indexed-objects" will get them, too (otherwise they will be either ejected loose or dropped completely when --unpack-unreachable=2.weeks.ago or similar is used).

That code path uses rev-list internally. So I think you need something like "--all-worktrees --indexed-objects", and you need to pass the new option in from git-repack to pack-objects (or you need to simply make "--indexed-objects" cover all worktrees by default).

-Peff

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

I'll let this topic simmer on the list for now, instead of picking it up immediately to 'pu', as Michael in $gmane/296068 makes me wonder if we want to keep piling on the current "worktree ref iterations are bolted on" or if we want to first clean it up, whose natural fallout hopefully would eliminate the bug away.

Thanks.

(from patch 4/4 mail)

On Wed, Jun 1, 2016 at 10:51 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:

Yeah I missed that. I'm not going to write another directory walker to collect all logs/refs/bisect/*. I didn't add pending objects for refs/bisect/* of other worktrees either. At that point waiting for the new ref iterator makes more sense...

On Wed, Jun 1, 2016 at 11:06 PM, Junio C Hamano <gitster@pobox.com> wrote:

So what should be the way forward? My intention was having something that can fix the problem for now, even if a bit hacky while waiting for ref iterator to be ready, then convert to use it and clean things up, because I don't how long ref-iterator would take and losing data is serous enough that I'd like to fix it soon. If we go with "fix soon then convert to ref-iterator later", then I will drop the logs/bisect/* check, checking logs/HEAD alone should be good enough. Otherwise I'll prepare a series on top of ref-iterator.

On 06/02/2016 11:53 AM, Duy Nguyen wrote:

Fixing reachability via the index and detached HEADs feels relatively important.

Fixing refs/bisect/* feels a bit less urgent, because (1) usually bisections don't take very long, and (2) if the commits that one is bisecting are not otherwise reachable anymore, then the bisection is probably not interesting anymore. However, these are real references, so if they get broken, the worktree will be seen as corrupt and recovery is not super obvious (I guess most people would end up deleting the whole worktree).

Fixing the reflogs for HEAD and (especially) refs/bisect/* in worktrees feels even less important, because reflogs are not nearly as important as current ref values, Git is relatively tolerant of broken reflog entries, and there are easy ways to prune them if breakage occurs.

It's hard for me to predict when the ref-iterator stuff will be merged. It is a big change, but so far the feedback seems pretty good. I can tell you that pushing it and ref-stores forward is high on my priority list.

Michael

Michael Haggerty <mhagger@alum.mit.edu> writes:

I agree with the order of importance above. But "relatively" is a very good keyword. Just like bisection refs, what is in the index and the commit detached HEAD points at are expected to be tentative. As a part of still-experimental feature, I'd rather see our bandwidth spent on fixing it the right way first time, instead of piling on an unproven quick-fix as a band aid, having to rip it off and fixing it properly later.

Thanks.