Support git attrs from index (and bare repo) #664

Merged
merged 4 commits into from May 5, 2012

Projects

None yet

3 participants

Member
arrbee commented May 3, 2012

Depending on the operation, we need to consider gitattributes in both the work dir and the index. This adds a parameter to all of the gitattributes related functions that allows user control of attribute reading behavior (i.e. prefer workdir, prefer index, only use index).

This fix also covers allowing us to check attributes (and hence do diff and status) on bare repositories.

This was a somewhat larger change that I hoped because it had to change the cache key used for gitattributes files.

I took the opportunity to update the lacking docs in the attr.h header.

@arrbee arrbee Support reading attributes from index
Depending on the operation, we need to consider gitattributes
in both the work dir and the index.  This adds a parameter to
all of the gitattributes related functions that allows user
control of attribute reading behavior (i.e. prefer workdir,
prefer index, only use index).

This fix also covers allowing us to check attributes (and
hence do diff and status) on bare repositories.

This was a somewhat larger change that I hoped because it had
to change the cache key used for gitattributes files.
f917481
@vmg vmg commented on the diff May 4, 2012
include/git2/attr.h
const char **value);
/**
- * Lookup list of attributes for path, populating array of strings
+ * Look up a list of git attributes for path.
+ *
+ * Use this if you have a known list of attributes that you want to
+ * look up in a single call. This is somewhat more efficient than
+ * calling `git_attr_get()` multiple times.
+ *
+ * For example, you might write:
+ *
+ * const char *attrs[] = { "crlf", "diff", "foo" };
+ * const char **values[3];
vmg
vmg May 4, 2012 Owner

This doesn't match the signature. One pointer too many?

Owner
vmg commented May 4, 2012

I'm liking this a lot. Certainly not a major feature, so I'll try to fit this into the release.

@nulltoken nulltoken commented on the diff May 4, 2012
src/attr.c
attr_walk_up_info *info = (attr_walk_up_info *)ref;
- return push_attrs(info->repo, info->files, path->ptr, GIT_ATTR_FILE);
+ git_attr_file_source src[2];
+
+ n_src = git_attr_cache__decide_sources(
+ info->flags, info->workdir != NULL, info->index != NULL, src);
nulltoken
nulltoken May 4, 2012 Member

info->index != NULL

I'm not sure about this. The result may vary depending on when this is being evaluated. From what I remember:

  • git_repository_open() doesn't deal with the index at all
  • git_repository_index__weakptr() will delegate to git_index_open() which will either load the index from disk (if it exists) or build a new in-memory one.

So, invoking this after the opening of a repo will always set has_index to false. And invoking this after a call to git_repository_index() will always end up with has_indexset to true.

On a personal side, I don't feel very at ease with the automatic generation of the index upon a call to git_repository_index(). How about:

  • expose a get_repository_hasindex() accessor returning true if an index is currently loaded or if an index file exists in the gitdir
  • add a bool create_if_absent parameter to git_index_open(), and preven git_repository_index() from triggering the dynamic creation of a new index.

/cc @carlosmn

nulltoken
nulltoken May 10, 2012 Member

@arrbee Would you like me to draft something along these lines? ^^

arrbee
arrbee May 10, 2012 Member

@nulltoken I'm on the fence. I don't want to kick this off as a large project, but I agree that auto-creating the index for the repository is undesirable in this case. Let me read over the relevant code again...

arrbee
arrbee May 10, 2012 Member

@nulltoken So, I'd like to have a little more discussion about this, maybe bringing in @tanoku too. You are right, the way this is written today, the test is not useful. However, I think because of the way the parent function (collect_attr_files) is written, the behavior is at least predictable - i.e. the index will never be NULL - it will always be allocated.

The question I'm left with is whether it is worth it to add the complexity to change this. Given a newly created in-memory index, then the code which looks for .gitattributes files will never find them in the index, so the behavior will still be correct (i.e. the index versions of attributes / ignores will not be used).

I could eliminate the info->index != NULL test, since right now it always returns true, or I can leave it in, so that at a future point we can rewrite collect_attr_files to not allocated the in-memory index if there is no on-disk available. I'm not feeling like it is urgent, although maybe I should add a comment.

What do you think? @tanoku Do you have an opinion?

Member

Should fix #640

Member
arrbee commented May 4, 2012

Okay, that last commit is a step in the right direction. It fixes the valgrind warnings I'm seeing on my machine. I still have a couple more things to look into though, including a unit test failure and the comments that @nulltoken makes above.

arrbee added some commits May 4, 2012
@arrbee arrbee Fixing issue with test data 3ec1fa5
@arrbee arrbee Fix valgrind issues
There are three changes here:
- correctly propogate error code from failed object lookups
- make zlib inflate use our allocators
- add OID to notfound error in ODB lookups
282283a
@vmg vmg merged commit 4ef14af into libgit2:development-merge May 5, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment