[Bug] data loss with cyclic alternates

Hi,

git seems to have issues with alternates when cycles are present (repo A has B/objects as alternates, B has A/objects as alternates). In such cases, gc and repack might delete objects that are present in only one of the alternates, leading to data loss.

I understand that this is no big use case, and requires manual editing of objects/info/alternates. But for the sake of preventing unneccesary data loss, and since I found no warning regarding alternate cycles in the documentation, it might make sense to handle such cases properly (maybe it's a simple "after finding all alternates directories, remove duplicates"?).

Here is a small test case that adds the object storage of a repository as alternate to itsself. After git repack -adl, all objects are deleted.

--- rm -rf test_repo && mkdir test_repo && cd test_repo && git init && touch a && git add a && git commit -m "c1" && git repack -adl && echo $PWD/.git/objects > .git/objects/info/alternates && echo ">> re-packing..." && git repack -adl && echo ">> git fsck..." && git fsck ---

Output:

---
Initialized empty Git repository in /somewhere/test_repo/.git/
[master (root-commit) ab9e123] c1
  1 file changed, 0 insertions(+), 0 deletions(-)
  create mode 100644 a
Counting objects: 3, done.
Writing objects: 100% (3/3), done.
Total 3 (delta 0), reused 0 (delta 0)
 >> re-packing...
Nothing new to pack.
error: refs/heads/master does not point to a valid object!
 >> git fsck...
Checking object directories: 100% (256/256), done.
Checking object directories: 100% (256/256), done.
error: HEAD: invalid sha1 pointer 1494ec24356cbbbd66e19f22cef762dd83de7f54
error: refs/heads/master does not point to a valid object!
notice: No default references
missing blob e69de29bb2d1d6434b8b29ae775ad8c2e48c5391
---

Thanks
- Eph

Ephrim Khong <dr.khong@gmail.com> writes:

Yeah, don't do that. A thinks "eh, the other guy must have it" and B thinks the same. In general, do not prune or gc a repository other repositories borrow from, even if there is no cycle, because the borrowee does not know anythning about objects that it itself no longer needs but are still needed by its borrowers.

On Fri, 2014-07-11 at 09:01 -0700, Junio C Hamano wrote:

Doesn't gc get run automatically at some points? Is the automatic gc run setup to avoid this problem?

Thanks, Jake

On Fri, Jul 11, 2014 at 06:01:46PM +0000, Keller, Jacob E wrote:

No, the automatic gc doesn't avoid this. It can't in the general case, as the parent repository does not know how many or which children are pointed to it as an alternate. And the borrowing repository does not even need to have write permission to the parent, so it cannot write a backpointer.

If people are using alternates, they should probably turn off gc.auto in the borrowee (it doesn't seem unreasonable to me to do so automatically via "clone -s" in cases where we can write to the alternates repo, and to issue a warning otherwise).

-Peff

Yes, if this is a known problem and we can avoid it by disabling garbage collection, that makes sense to me to do so..

Thanks, Jake

Am 11.07.14 18:01, schrieb Junio C Hamano:

It seems that there is a safeguard against this in sha1_file.c, link_alt_odb_entry(), that doesn't work as intended:

	if (!strcmp(ent->base, objdir)) {
		free(ent);
		return -1;
	}

However, printf-debugging tells me that ent->base is absolute and objdir is relative (".git/objects") at this point, so the strings are different even though the files are the same.

I never submitted a patch to git. Do you think someone can fix this hickup, otherwise I'll give it a shot next week.

When adding alternate object directories, we try not to add the directory of the current repository to avoid cycles. Unfortunately, that test was broken, since it compared an absolute with a relative path.

Signed-off-by: Ephrim Khong <dr.khong@gmail.com>
---
My first patch, so be harsh.  I'm not sure about the filename of the 
test, the behavior is tested with repack, but it affects gc and others 
as well.

  sha1_file.c                        |  2 +-
  t/t7702-repack-cyclic-alternate.sh | 24 ++++++++++++++++++++++++
  2 files changed, 25 insertions(+), 1 deletion(-)
  create mode 100755 t/t7702-repack-cyclic-alternate.sh

On Mon, Jul 14, 2014 at 4:02 PM, Ephrim Khong <dr.khong@gmail.com> wrote:

Not blaming anyone. I'm simply interested in code archeology. The first introduction of this strcmp is from 1494e03 (sha1_file.c: make sure packs in an alternate odb is named properly. - 2005-12-04). The intention is good as described in the commit message. But I think it's broken even back then because "objdir" (or get_object_directory()) will always be relative (unless someone sets it explicitly) and ent->base back then is already absolute (but not normalized).

I think you want to normalize objdir in addition to making it absolute, in case someone sets GIT_OBJECT_DIR to foo///bar. ent->base is normalized already. Oh and maybe use strcmp_icase to be nice on case-insensitive filesystems..

Kinda nit picking, but perhaps this path absolute-ization/normalization should be done by the caller link_alt_odb_entries so you don't have to redo it for every entry in alternates file. I know there's a loop of memcmp() right above this that may be more expensive than this micro-optimization when we have lots of alternate entries.

When adding alternate object directories, we try not to add the directory of the current repository to avoid cycles. Unfortunately, that test was broken, since it compared an absolute with a relative path.

Signed-off-by: Ephrim Khong <dr.khong@gmail.com>
---
As proposed by Duy, v2 of the patch normalizes the object directory,
uses strcmp_icase for case-insensitive file systems, and moves the
path normalization to link_alt_odb_entries to save some cycles.

Additionally, the test is now more compact by using the already prepared git repository instead of a new one, and tests if the path normalization for both objectdir and alternate works.

 sha1_file.c                        | 13 +++++++++----
 t/t7702-repack-cyclic-alternate.sh | 24 ++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 4 deletions(-)
 create mode 100755 t/t7702-repack-cyclic-alternate.sh

Am 14.07.2014 14:53, schrieb Ephrim Khong:

We need a Windows-style path in the file, double-quotes to protect special characters, and a small style fix. Therefore:

	echo "$(pwd)"/.git/objects/../objects >.git/objects/info/alternates

When adding alternate object directories, we try not to add the directory of the current repository to avoid cycles. Unfortunately, that test was broken, since it compared an absolute with a relative path.

Signed-off-by: Ephrim Khong <dr.khong@gmail.com>
---
Since v2: Added Johannes' comments.

 sha1_file.c                        | 13 +++++++++----
 t/t7702-repack-cyclic-alternate.sh | 24 ++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 4 deletions(-)
 create mode 100755 t/t7702-repack-cyclic-alternate.sh

Ephrim Khong <dr.khong@gmail.com> writes:

Not a problem with your patch, but we should rethink the name of this function when the code base is more quiet. It always makes me wonder if it is something similar to strcasecmp(), but in fact it is not. It is meant to be used *only* for pathnames; pathname_cmp() or something that has "path" in its name would be appropriate, but it is wrong to call it "str"-anything.

This is somewhat a strange usage of a strbuf.

 - it relies on that normalize_path_copy() only shrinks, never
   lengthens, which is not too bad;

 - if the operation ever shrinks, objdirbuf.len becomes
   meaningless.  The allocated length is objdirbuf.alloc, length
   of the string is strlen(objdirbuf.buf).

 - abspath.c::absolute_path() is still restricted to PATH_MAX, so
   you are not gaining much by using strbuf here.

But at least this patch is not making things any worse, so....

Do we really have to waste a new test file only for this test?

Don't we have any test that already uses alternate that these two new test pieces can be added to?

    $ git grep info/alternates t/

seems to show a few existing ones, including 1450 (fsck) and 7700 (repack) that look very relevant (I didn't check what the tests in them are about, though).

On 15.07.2014 21:26, Junio C Hamano wrote:

There might be a more elegant way, but I tried to mimic the local coding style and simply copied how the alternate paths were normalized. Let me know if you want this re-rolled without strbuf, otherwise I'll leave it as-is.

Absolutely not. I'll add it to 7700, which seems appropriate.

On Tue, Jul 15, 2014 at 7:29 AM, Ephrim Khong <dr.khong@gmail.com> wrote:

Since the existence of 'a' is significant here, not its timestamp, it would be clearer to create the file with:

    >a &&

Ephrim Khong <dr.khong@gmail.com> writes:

Do you need this artificially strange environment settings for the problem to manifest itself, or is it sufficient to have a non-canonical pathname in the info/alternates file?

Exporting an environment early in the test and having later tests in the file depend on it makes it harder to debug when things go wrong, than leaving an info/alternates file in the repository, primarily because the latter can be inspected more easily in the trash directory after "t7702-*.sh -i" dies, hence the above question.

Don't use 'touch' if you are not interested in timestams.  Write this as

	>a &&

because what you care about here in this test is that an empty file 'a' exists, so that you can run "git add" on it.

On 15.07.2014 21:48, Junio C Hamano wrote:

The test should check the normalization of both the paths in info/alternates and of GIT_OBJECT_DIRECTORY, so I'd prefer to leave it in. It is not necessary to reproduce the original problem, though.

That sounds reasonable. The variable is not required at the initialization anyway, so I'll set it right before the actual test and wrap it into a subshell to make debugging easier.