More filtering tests including order

This adds more tests of filters, including the ident filter when
mixed with custom filters.  I was able to combine with the reverse
filter and demonstrate that the order of filter application with
the default priority constants matches the order of core Git.

Also, this fixes two issues in the ident filter: preventing ident
expansion on binary files and avoiding a NULL dereference when
dollar sign characters are found without Id.

include/git2/sys/filter.h

@@ -29,10 +29,24 @@ GIT_EXTERN(git_filter *) git_filter_lookup(const char *name);
 #define GIT_FILTER_CRLF  "crlf"
 #define GIT_FILTER_IDENT "ident"
 
+/**
+ * This is priority that the internal CRLF filter will be registered with
+ */
 #define GIT_FILTER_CRLF_PRIORITY 0
+
+/**
+ * This is priority that the internal ident filter will be registered with
+ */
 #define GIT_FILTER_IDENT_PRIORITY 100
 
 /**
+ * This is priority to use with a custom filter to imitate a core Git
+ * filter driver, so that it will be run last on checkout and first on
+ * checkin.  You do not have to use this, but it helps compatibility.
+ */
+#define GIT_FILTER_DRIVER_PRIORITY 200
+
+/**
  * Create a new empty filter list
  *
  * Normally you won't use this because `git_filter_list_load` will create

src/ident.c

@@ -8,6 +8,7 @@
 #include "git2/sys/filter.h"
 #include "filter.h"
 #include "buffer.h"
+#include "buf_text.h"
 
 static int ident_find_id(
 	const char **id_start, const char **id_end, const char *start, size_t len)
@@ -24,7 +25,7 @@ static int ident_find_id(
 		len = remaining - 1;
 	}
 
-	if (len < 3)
+	if (!found || len < 3)
 		return GIT_ENOTFOUND;
 	*id_start = found;
 
@@ -99,6 +100,10 @@ static int ident_apply(
 {
 	GIT_UNUSED(self); GIT_UNUSED(payload);
 
+	/* Don't filter binary files */
+	if (git_buf_text_is_binary(from))
+		return GIT_ENOTFOUND;
+
 	if (git_filter_source_mode(src) == GIT_FILTER_SMUDGE)
 		return ident_insert_id(to, from, src);
 	else

tests-clar/checkout/crlf.c

@@ -159,21 +159,30 @@ void test_checkout_crlf__with_ident(void)
 
 	cl_git_mkfile("crlf/lf.ident", ALL_LF_TEXT_RAW "\n$Id: initial content$\n");
 	cl_git_mkfile("crlf/crlf.ident", ALL_CRLF_TEXT_RAW "\r\n$Id$\r\n\r\n");
+	cl_git_mkfile("crlf/more1.identlf", "$Id$\n" MORE_LF_TEXT_RAW);
+	cl_git_mkfile("crlf/more2.identcrlf", "\r\n$Id: $\r\n" MORE_CRLF_TEXT_RAW);
 
 	cl_git_pass(git_repository_index(&index, g_repo));
 	cl_git_pass(git_index_add_bypath(index, "lf.ident"));
 	cl_git_pass(git_index_add_bypath(index, "crlf.ident"));
+	cl_git_pass(git_index_add_bypath(index, "more1.identlf"));
+	cl_git_pass(git_index_add_bypath(index, "more2.identcrlf"));
 	cl_repo_commit_from_index(NULL, g_repo, NULL, 0, "Some ident files\n");
 
 	git_checkout_head(g_repo, &opts);
 
-	/* check that blob has $Id$ */
+	/* check that blobs have $Id$ */
 
 	cl_git_pass(git_blob_lookup(&blob, g_repo,
 		& git_index_get_bypath(index, "lf.ident", 0)->oid));
 	cl_assert_equal_s(
 		ALL_LF_TEXT_RAW "\n$Id$\n", git_blob_rawcontent(blob));
+	git_blob_free(blob);
 
+	cl_git_pass(git_blob_lookup(&blob, g_repo,
+		& git_index_get_bypath(index, "more2.identcrlf", 0)->oid));
+	cl_assert_equal_s(
+		"\n$Id$\n" MORE_CRLF_TEXT_AS_LF, git_blob_rawcontent(blob));
 	git_blob_free(blob);
 
 	/* check that filesystem is initially untouched - matching core Git */
@@ -184,20 +193,39 @@ void test_checkout_crlf__with_ident(void)
 	/* check that forced checkout rewrites correctly */
 
 	p_unlink("crlf/lf.ident");
-	p_unlink("crlf/crlflf.ident");
+	p_unlink("crlf/crlf.ident");
+	p_unlink("crlf/more1.identlf");
+	p_unlink("crlf/more2.identcrlf");
 
 	git_checkout_head(g_repo, &opts);
 
-	if (GIT_EOL_NATIVE == GIT_EOL_LF)
+	if (GIT_EOL_NATIVE == GIT_EOL_LF) {
 		cl_assert_equal_file(
 			ALL_LF_TEXT_RAW
 			"\n$Id: fcf6d4d9c212dc66563b1171b1cd99953c756467$\n",
 			0, "crlf/lf.ident");
-	else
+		cl_assert_equal_file(
+			ALL_CRLF_TEXT_AS_LF
+			"\n$Id: f2c66ad9b2b5a734d9bf00d5000cc10a62b8a857$\n\n",
+			0, "crlf/crlf.ident");
+	} else {
 		cl_assert_equal_file(
 			ALL_LF_TEXT_AS_CRLF
 			"\r\n$Id: fcf6d4d9c212dc66563b1171b1cd99953c756467$\r\n",
 			0, "crlf/lf.ident");
+		cl_assert_equal_file(
+			ALL_CRLF_TEXT_RAW
+			"\r\n$Id: f2c66ad9b2b5a734d9bf00d5000cc10a62b8a857$\r\n\r\n",
+			0, "crlf/crlf.ident");
+	}
+
+	cl_assert_equal_file(
+		"$Id: f7830382dac1f1583422be5530fdfbd26289431b$\n"
+		MORE_LF_TEXT_AS_LF, 0, "crlf/more1.identlf");
+
+	cl_assert_equal_file(
+		"\r\n$Id: 74677a68413012ce8d7e7cfc3f12603df3a3eac4$\r\n"
+		MORE_CRLF_TEXT_AS_CRLF, 0, "crlf/more2.identcrlf");
 
 	git_index_free(index);
 }

tests-clar/filter/custom.c

@@ -6,8 +6,9 @@
 #include "git2/sys/filter.h"
 #include "git2/sys/repository.h"
 
-#define BITFLIP_FILTER_PRIORITY 20
-#define REVERSE_FILTER_PRIORITY 25
+/* picked these to be >= GIT_FILTER_DRIVER_PRIORITY */
+#define BITFLIP_FILTER_PRIORITY 200
+#define REVERSE_FILTER_PRIORITY 250
 
 #define VERY_SECURE_ENCRYPTION(b) ((b) ^ 0xff)
 
@@ -255,3 +256,62 @@ void test_filter_custom__can_register_a_custom_filter_in_the_repository(void)
 	cl_assert_equal_sz(0, git_filter_list_length(fl));
 	git_filter_list_free(fl);
 }
+
+void test_filter_custom__order_dependency(void)
+{
+	git_index *index;
+	git_blob *blob;
+	git_buf buf = { 0 };
+
+	/* so if ident and reverse are used together, an interesting thing
+	 * happens - a reversed "$Id$" string is no longer going to trigger
+	 * ident correctly.  When checking out, the filters should be applied
+	 * in order CLRF, then ident, then reverse, so ident expansion should
+	 * work correctly.  On check in, the content should be reversed, then
+	 * ident, then CRLF filtered.  Let's make sure that works...
+	 */
+
+	cl_git_mkfile(
+		"empty_standard_repo/.gitattributes",
+		"hero.*.rev-ident text ident reverse eol=lf\n");
+
+	cl_git_mkfile(
+		"empty_standard_repo/hero.1.rev-ident",
+		"This is a test\n$Id$\nHave fun!\n");
+
+	cl_git_mkfile(
+		"empty_standard_repo/hero.2.rev-ident",
+		"Another test\n$dI$\nCrazy!\n");
+
+	cl_git_pass(git_repository_index(&index, g_repo));
+	cl_git_pass(git_index_add_bypath(index, "hero.1.rev-ident"));
+	cl_git_pass(git_index_add_bypath(index, "hero.2.rev-ident"));
+	cl_repo_commit_from_index(NULL, g_repo, NULL, 0, "Filter chains\n");
+	git_index_free(index);
+
+	cl_git_pass(git_blob_lookup(&blob, g_repo,
+		& git_index_get_bypath(index, "hero.1.rev-ident", 0)->oid));
+	cl_assert_equal_s(
+		"\n!nuf evaH\n$dI$\ntset a si sihT", git_blob_rawcontent(blob));
+	cl_git_pass(git_blob_filtered_content(&buf, blob, "hero.1.rev-ident", 0));
+	/* no expansion because id was reversed at checkin and now at ident
+	 * time, reverse is not applied yet */
+	cl_assert_equal_s(
+		"This is a test\n$Id$\nHave fun!\n", buf.ptr);
+	git_blob_free(blob);
+
+	cl_git_pass(git_blob_lookup(&blob, g_repo,
+		& git_index_get_bypath(index, "hero.2.rev-ident", 0)->oid));
+	cl_assert_equal_s(
+		"\n!yzarC\n$Id$\ntset rehtonA", git_blob_rawcontent(blob));
+	cl_git_pass(git_blob_filtered_content(&buf, blob, "hero.2.rev-ident", 0));
+	/* expansion because reverse was applied at checkin and at ident time,
+	 * reverse is not applied yet */
+	cl_assert_equal_s(
+		"Another test\n$59001fe193103b1016b27027c0c827d036fd0ac8 :dI$\nCrazy!\n", buf.ptr);
+	cl_assert_equal_i(0, git_oid_strcmp(
+		git_blob_id(blob), "8ca0df630d728c0c72072b6101b301391ef10095"));
+	git_blob_free(blob);
+
+	git_buf_free(&buf);
+}