Merge pull request #1507 from arrbee/fix-look-inside-untracked-directory

Update diff handling of "untracked" directories

include/git2/diff.h

@@ -88,42 +88,61 @@ typedef enum {
 	GIT_DIFF_INCLUDE_UNTRACKED = (1 << 8),
 	/** Include unmodified files in the diff list */
 	GIT_DIFF_INCLUDE_UNMODIFIED = (1 << 9),
+
 	/** Even with GIT_DIFF_INCLUDE_UNTRACKED, an entire untracked directory
 	 *  will be marked with only a single entry in the diff list; this flag
 	 *  adds all files under the directory as UNTRACKED entries, too.
 	 */
 	GIT_DIFF_RECURSE_UNTRACKED_DIRS = (1 << 10),
+
 	/** If the pathspec is set in the diff options, this flags means to
 	 *  apply it as an exact match instead of as an fnmatch pattern.
 	 */
 	GIT_DIFF_DISABLE_PATHSPEC_MATCH = (1 << 11),
+
 	/** Use case insensitive filename comparisons */
 	GIT_DIFF_DELTAS_ARE_ICASE = (1 << 12),
+
 	/** When generating patch text, include the content of untracked files */
 	GIT_DIFF_INCLUDE_UNTRACKED_CONTENT = (1 << 13),
+
 	/** Disable updating of the `binary` flag in delta records.  This is
 	 *  useful when iterating over a diff if you don't need hunk and data
 	 *  callbacks and want to avoid having to load file completely.
 	 */
 	GIT_DIFF_SKIP_BINARY_CHECK = (1 << 14),
+
 	/** Normally, a type change between files will be converted into a
 	 *  DELETED record for the old and an ADDED record for the new; this
 	 *  options enabled the generation of TYPECHANGE delta records.
 	 */
 	GIT_DIFF_INCLUDE_TYPECHANGE = (1 << 15),
+
 	/** Even with GIT_DIFF_INCLUDE_TYPECHANGE, blob->tree changes still
 	 *  generally show as a DELETED blob.  This flag tries to correctly
 	 *  label blob->tree transitions as TYPECHANGE records with new_file's
 	 *  mode set to tree.  Note: the tree SHA will not be available.
 	 */
 	GIT_DIFF_INCLUDE_TYPECHANGE_TREES  = (1 << 16),
+
 	/** Ignore file mode changes */
 	GIT_DIFF_IGNORE_FILEMODE = (1 << 17),
+
 	/** Even with GIT_DIFF_INCLUDE_IGNORED, an entire ignored directory
 	 *  will be marked with only a single entry in the diff list; this flag
 	 *  adds all files under the directory as IGNORED entries, too.
 	 */
 	GIT_DIFF_RECURSE_IGNORED_DIRS = (1 << 18),
+
+	/** Core Git scans inside untracked directories, labeling them IGNORED
+	 *  if they are empty or only contain ignored files; a directory is
+	 *  consider UNTRACKED only if it has an actual untracked file in it.
+	 *  This scan is extra work for a case you often don't care about.  This
+	 *  flag makes libgit2 immediately label an untracked directory as
+	 *  UNTRACKED without looking insde it (which differs from core Git).
+	 *  Of course, ignore rules are still checked for the directory itself.
+	 */
+	GIT_DIFF_FAST_UNTRACKED_DIRS = (1 << 19),
 } git_diff_option_t;
 
 /**

src/diff_output.c

@@ -101,8 +101,8 @@ static bool diff_delta_is_binary_forced(
 
 	/* make sure files are conceivably mmap-able */
 	if ((git_off_t)((size_t)delta->old_file.size) != delta->old_file.size ||
-		(git_off_t)((size_t)delta->new_file.size) != delta->new_file.size)
-	{
+		(git_off_t)((size_t)delta->new_file.size) != delta->new_file.size) {
+
 		delta->old_file.flags |= GIT_DIFF_FLAG_BINARY;
 		delta->new_file.flags |= GIT_DIFF_FLAG_BINARY;
 		delta->flags |= GIT_DIFF_FLAG_BINARY;
@@ -232,8 +232,7 @@ static int get_blob_content(
 	if (git_oid_iszero(&file->oid))
 		return 0;
 
-	if (file->mode == GIT_FILEMODE_COMMIT)
-	{
+	if (file->mode == GIT_FILEMODE_COMMIT) {
 		char oidstr[GIT_OID_HEXSZ+1];
 		git_buf content = GIT_BUF_INIT;
 
@@ -299,8 +298,8 @@ static int get_workdir_sm_content(
 	char oidstr[GIT_OID_HEXSZ+1];
 
 	if ((error = git_submodule_lookup(&sm, ctxt->repo, file->path)) < 0 ||
-		(error = git_submodule_status(&sm_status, sm)) < 0)
-	{
+		(error = git_submodule_status(&sm_status, sm)) < 0) {
+
 		/* GIT_EEXISTS means a "submodule" that has not been git added */
 		if (error == GIT_EEXISTS)
 			error = 0;
@@ -312,8 +311,8 @@ static int get_workdir_sm_content(
 		const git_oid* sm_head;
 
 		if ((sm_head = git_submodule_wd_id(sm)) != NULL ||
-			(sm_head = git_submodule_head_id(sm)) != NULL)
-		{
+			(sm_head = git_submodule_head_id(sm)) != NULL) {
+
 			git_oid_cpy(&file->oid, sm_head);
 			file->flags |= GIT_DIFF_FLAG_VALID_OID;
 		}
@@ -660,8 +659,8 @@ static int diff_patch_load(
 	 */
 	if (check_if_unmodified &&
 		delta->old_file.mode == delta->new_file.mode &&
-		!git_oid__cmp(&delta->old_file.oid, &delta->new_file.oid))
-	{
+		!git_oid__cmp(&delta->old_file.oid, &delta->new_file.oid)) {
+
 		delta->status = GIT_DELTA_UNMODIFIED;
 
 		if ((ctxt->opts->flags & GIT_DIFF_INCLUDE_UNMODIFIED) == 0)
@@ -1049,6 +1048,12 @@ char git_diff_status_char(git_delta_t status)
 	return code;
 }
 
+static int callback_error(void)
+{
+	giterr_clear();
+	return GIT_EUSER;
+}
+
 static int print_compact(
 	const git_diff_delta *delta, float progress, void *data)
 {
@@ -1083,10 +1088,7 @@ static int print_compact(
 
 	if (pi->print_cb(delta, NULL, GIT_DIFF_LINE_FILE_HDR,
 			git_buf_cstr(pi->buf), git_buf_len(pi->buf), pi->payload))
-	{
-		giterr_clear();
-		return GIT_EUSER;
-	}
+		return callback_error();
 
 	return 0;
 }
@@ -1200,10 +1202,7 @@ static int print_patch_file(
 
 	if (pi->print_cb(delta, NULL, GIT_DIFF_LINE_FILE_HDR,
 			git_buf_cstr(pi->buf), git_buf_len(pi->buf), pi->payload))
-	{
-		giterr_clear();
-		return GIT_EUSER;
-	}
+		return callback_error();
 
 	if ((delta->flags & GIT_DIFF_FLAG_BINARY) == 0)
 		return 0;
@@ -1217,10 +1216,7 @@ static int print_patch_file(
 
 	if (pi->print_cb(delta, NULL, GIT_DIFF_LINE_BINARY,
 			git_buf_cstr(pi->buf), git_buf_len(pi->buf), pi->payload))
-	{
-		giterr_clear();
-		return GIT_EUSER;
-	}
+		return callback_error();
 
 	return 0;
 }
@@ -1243,10 +1239,7 @@ static int print_patch_hunk(
 
 	if (pi->print_cb(d, r, GIT_DIFF_LINE_HUNK_HDR,
 			git_buf_cstr(pi->buf), git_buf_len(pi->buf), pi->payload))
-	{
-		giterr_clear();
-		return GIT_EUSER;
-	}
+		return callback_error();
 
 	return 0;
 }
@@ -1278,10 +1271,7 @@ static int print_patch_line(
 
 	if (pi->print_cb(delta, range, line_origin,
 			git_buf_cstr(pi->buf), git_buf_len(pi->buf), pi->payload))
-	{
-		giterr_clear();
-		return GIT_EUSER;
-	}
+		return callback_error();
 
 	return 0;
 }

src/vector.c

@@ -277,13 +277,11 @@ void git_vector_swap(git_vector *a, git_vector *b)
 
 int git_vector_resize_to(git_vector *v, size_t new_length)
 {
-	if (new_length <= v->length)
-		return 0;
-
 	if (new_length > v->_alloc_size &&
 		resize_vector(v, new_length) < 0)
 		return -1;
 
+	if (new_length > v->length)
 		memset(&v->contents[v->length], 0,
 			sizeof(void *) * (new_length - v->length));
 

tests-clar/diff/diff_helpers.c

@@ -28,7 +28,15 @@ int diff_file_cb(
 {
 	diff_expects *e = payload;
 
-	GIT_UNUSED(progress);
+	if (e->debug)
+		fprintf(stderr, "%c %s (%.3f)\n",
+			git_diff_status_char(delta->status),
+			delta->old_file.path, progress);
+
+	if (e->names)
+		cl_assert_equal_s(e->names[e->files], delta->old_file.path);
+	if (e->statuses)
+		cl_assert_equal_i(e->statuses[e->files], (int)delta->status);
 
 	e->files++;
 

tests-clar/diff/diff_helpers.h

@@ -18,6 +18,13 @@ typedef struct {
 	int line_ctxt;
 	int line_adds;
 	int line_dels;
+
+	/* optional arrays of expected specific values */
+	const char **names;
+	int *statuses;
+
+	int debug;
+
 } diff_expects;
 
 typedef struct {

tests-clar/diff/workdir.c

@@ -1033,3 +1033,190 @@ void test_diff_workdir__to_tree_issue_1397(void)
 	git_diff_list_free(diff);
 	git_tree_free(a);
 }
+
+void test_diff_workdir__untracked_directory_scenarios(void)
+{
+	git_diff_options opts = GIT_DIFF_OPTIONS_INIT;
+	git_diff_list *diff = NULL;
+	diff_expects exp;
+	char *pathspec = NULL;
+	static const char *files0[] = {
+		"subdir/deleted_file",
+		"subdir/modified_file",
+		"subdir/new_file",
+		NULL
+	};
+	static const char *files1[] = {
+		"subdir/deleted_file",
+		"subdir/directory/",
+		"subdir/modified_file",
+		"subdir/new_file",
+		NULL
+	};
+	static const char *files2[] = {
+		"subdir/deleted_file",
+		"subdir/directory/more/notignored",
+		"subdir/modified_file",
+		"subdir/new_file",
+		NULL
+	};
+
+	g_repo = cl_git_sandbox_init("status");
+	cl_git_mkfile("status/.gitignore", "ignored\n");
+
+	opts.context_lines = 3;
+	opts.interhunk_lines = 1;
+	opts.flags |= GIT_DIFF_INCLUDE_IGNORED | GIT_DIFF_INCLUDE_UNTRACKED;
+	opts.pathspec.strings = &pathspec;
+	opts.pathspec.count   = 1;
+	pathspec = "subdir";
+
+	/* baseline for "subdir" pathspec */
+
+	memset(&exp, 0, sizeof(exp));
+	exp.names = files0;
+
+	cl_git_pass(git_diff_index_to_workdir(&diff, g_repo, NULL, &opts));
+
+	cl_git_pass(git_diff_foreach(diff, diff_file_cb, NULL, NULL, &exp));
+
+	cl_assert_equal_i(3, exp.files);
+	cl_assert_equal_i(0, exp.file_status[GIT_DELTA_ADDED]);
+	cl_assert_equal_i(1, exp.file_status[GIT_DELTA_DELETED]);
+	cl_assert_equal_i(1, exp.file_status[GIT_DELTA_MODIFIED]);
+	cl_assert_equal_i(0, exp.file_status[GIT_DELTA_IGNORED]);
+	cl_assert_equal_i(1, exp.file_status[GIT_DELTA_UNTRACKED]);
+
+	git_diff_list_free(diff);
+
+	/* empty directory */
+
+	cl_git_pass(p_mkdir("status/subdir/directory", 0777));
+
+	memset(&exp, 0, sizeof(exp));
+	exp.names = files1;
+
+	cl_git_pass(git_diff_index_to_workdir(&diff, g_repo, NULL, &opts));
+
+	cl_git_pass(git_diff_foreach(diff, diff_file_cb, NULL, NULL, &exp));
+
+	cl_assert_equal_i(4, exp.files);
+	cl_assert_equal_i(0, exp.file_status[GIT_DELTA_ADDED]);
+	cl_assert_equal_i(1, exp.file_status[GIT_DELTA_DELETED]);
+	cl_assert_equal_i(1, exp.file_status[GIT_DELTA_MODIFIED]);
+	cl_assert_equal_i(1, exp.file_status[GIT_DELTA_IGNORED]);
+	cl_assert_equal_i(1, exp.file_status[GIT_DELTA_UNTRACKED]);
+
+	git_diff_list_free(diff);
+
+	/* directory with only ignored files */
+
+	cl_git_pass(p_mkdir("status/subdir/directory/deeper", 0777));
+	cl_git_mkfile("status/subdir/directory/deeper/ignored", "ignore me\n");
+
+	cl_git_pass(p_mkdir("status/subdir/directory/another", 0777));
+	cl_git_mkfile("status/subdir/directory/another/ignored", "ignore me\n");
+
+	memset(&exp, 0, sizeof(exp));
+	exp.names = files1;
+
+	cl_git_pass(git_diff_index_to_workdir(&diff, g_repo, NULL, &opts));
+
+	cl_git_pass(git_diff_foreach(diff, diff_file_cb, NULL, NULL, &exp));
+
+	cl_assert_equal_i(4, exp.files);
+	cl_assert_equal_i(0, exp.file_status[GIT_DELTA_ADDED]);
+	cl_assert_equal_i(1, exp.file_status[GIT_DELTA_DELETED]);
+	cl_assert_equal_i(1, exp.file_status[GIT_DELTA_MODIFIED]);
+	cl_assert_equal_i(1, exp.file_status[GIT_DELTA_IGNORED]);
+	cl_assert_equal_i(1, exp.file_status[GIT_DELTA_UNTRACKED]);
+
+	git_diff_list_free(diff);
+
+	/* directory with ignored directory (contents irrelevant) */
+
+	cl_git_pass(p_mkdir("status/subdir/directory/more", 0777));
+	cl_git_pass(p_mkdir("status/subdir/directory/more/ignored", 0777));
+	cl_git_mkfile("status/subdir/directory/more/ignored/notignored",
+		"inside ignored dir\n");
+
+	memset(&exp, 0, sizeof(exp));
+	exp.names = files1;
+
+	cl_git_pass(git_diff_index_to_workdir(&diff, g_repo, NULL, &opts));
+
+	cl_git_pass(git_diff_foreach(diff, diff_file_cb, NULL, NULL, &exp));
+
+	cl_assert_equal_i(4, exp.files);
+	cl_assert_equal_i(0, exp.file_status[GIT_DELTA_ADDED]);
+	cl_assert_equal_i(1, exp.file_status[GIT_DELTA_DELETED]);
+	cl_assert_equal_i(1, exp.file_status[GIT_DELTA_MODIFIED]);
+	cl_assert_equal_i(1, exp.file_status[GIT_DELTA_IGNORED]);
+	cl_assert_equal_i(1, exp.file_status[GIT_DELTA_UNTRACKED]);
+
+	git_diff_list_free(diff);
+
+	/* quick version avoids directory scan */
+
+	opts.flags = opts.flags | GIT_DIFF_FAST_UNTRACKED_DIRS;
+
+	memset(&exp, 0, sizeof(exp));
+	exp.names = files1;
+
+	cl_git_pass(git_diff_index_to_workdir(&diff, g_repo, NULL, &opts));
+
+	cl_git_pass(git_diff_foreach(diff, diff_file_cb, NULL, NULL, &exp));
+
+	cl_assert_equal_i(4, exp.files);
+	cl_assert_equal_i(0, exp.file_status[GIT_DELTA_ADDED]);
+	cl_assert_equal_i(1, exp.file_status[GIT_DELTA_DELETED]);
+	cl_assert_equal_i(1, exp.file_status[GIT_DELTA_MODIFIED]);
+	cl_assert_equal_i(0, exp.file_status[GIT_DELTA_IGNORED]);
+	cl_assert_equal_i(2, exp.file_status[GIT_DELTA_UNTRACKED]);
+
+	git_diff_list_free(diff);
+
+	/* directory with nested non-ignored content */
+
+	opts.flags = opts.flags & ~GIT_DIFF_FAST_UNTRACKED_DIRS;
+
+	cl_git_mkfile("status/subdir/directory/more/notignored",
+		"not ignored deep under untracked\n");
+
+	memset(&exp, 0, sizeof(exp));
+	exp.names = files1;
+
+	cl_git_pass(git_diff_index_to_workdir(&diff, g_repo, NULL, &opts));
+
+	cl_git_pass(git_diff_foreach(diff, diff_file_cb, NULL, NULL, &exp));
+
+	cl_assert_equal_i(4, exp.files);
+	cl_assert_equal_i(0, exp.file_status[GIT_DELTA_ADDED]);
+	cl_assert_equal_i(1, exp.file_status[GIT_DELTA_DELETED]);
+	cl_assert_equal_i(1, exp.file_status[GIT_DELTA_MODIFIED]);
+	cl_assert_equal_i(0, exp.file_status[GIT_DELTA_IGNORED]);
+	cl_assert_equal_i(2, exp.file_status[GIT_DELTA_UNTRACKED]);
+
+	git_diff_list_free(diff);
+
+	/* use RECURSE_UNTRACKED_DIRS to get actual untracked files (no ignores) */
+
+	opts.flags = opts.flags & ~GIT_DIFF_INCLUDE_IGNORED;
+	opts.flags = opts.flags | GIT_DIFF_RECURSE_UNTRACKED_DIRS;
+
+	memset(&exp, 0, sizeof(exp));
+	exp.names = files2;
+
+	cl_git_pass(git_diff_index_to_workdir(&diff, g_repo, NULL, &opts));
+
+	cl_git_pass(git_diff_foreach(diff, diff_file_cb, NULL, NULL, &exp));
+
+	cl_assert_equal_i(4, exp.files);
+	cl_assert_equal_i(0, exp.file_status[GIT_DELTA_ADDED]);
+	cl_assert_equal_i(1, exp.file_status[GIT_DELTA_DELETED]);
+	cl_assert_equal_i(1, exp.file_status[GIT_DELTA_MODIFIED]);
+	cl_assert_equal_i(0, exp.file_status[GIT_DELTA_IGNORED]);
+	cl_assert_equal_i(2, exp.file_status[GIT_DELTA_UNTRACKED]);
+
+	git_diff_list_free(diff);
+}

tests-clar/status/status_helpers.c

@@ -40,7 +40,8 @@ int cb_status__single(const char *p, unsigned int s, void *payload)
 {
 	status_entry_single *data = (status_entry_single *)payload;
 
-	GIT_UNUSED(p);
+	if (data->debug)
+		fprintf(stderr, "%02d: %s (%04x)\n", data->count, p, s);
 
 	data->count++;
 	data->status = s;

tests-clar/status/status_helpers.h

@@ -24,6 +24,7 @@ extern int cb_status__count(const char *p, unsigned int s, void *payload);
 typedef struct {
 	int count;
 	unsigned int status;
+	bool debug;
 } status_entry_single;
 
 /* cb_status__single takes payload of "status_entry_single *" */

tests-clar/status/worktree.c

@@ -258,9 +258,8 @@ void test_status_worktree__ignores(void)
 
 static int cb_status__check_592(const char *p, unsigned int s, void *payload)
 {
-	GIT_UNUSED(payload);
-
-	if (s != GIT_STATUS_WT_DELETED || (payload != NULL && strcmp(p, (const char *)payload) != 0))
+	if (s != GIT_STATUS_WT_DELETED ||
+		(payload != NULL && strcmp(p, (const char *)payload) != 0))
 		return -1;
 
 	return 0;

tests-clar/submodule/status.c

@@ -383,3 +383,30 @@ void test_submodule_status__iterator(void)
 
 	cl_git_pass(git_status_foreach_ext(g_repo, &opts, confirm_submodule_status, &exp));
 }
+
+void test_submodule_status__untracked_dirs_containing_ignored_files(void)
+{
+	git_buf path = GIT_BUF_INIT;
+	unsigned int status, expected;
+	git_submodule *sm;
+
+	cl_git_pass(git_buf_joinpath(&path, git_repository_path(g_repo), "modules/sm_unchanged/info/exclude"));
+	cl_git_append2file(git_buf_cstr(&path), "\n*.ignored\n");
+
+	cl_git_pass(git_buf_joinpath(&path, git_repository_workdir(g_repo), "sm_unchanged/directory"));
+	cl_git_pass(git_futils_mkdir(git_buf_cstr(&path), NULL, 0755, 0));
+	cl_git_pass(git_buf_joinpath(&path, git_buf_cstr(&path), "i_am.ignored"));
+	cl_git_mkfile(git_buf_cstr(&path), "ignored this file, please\n");
+
+	cl_git_pass(git_submodule_lookup(&sm, g_repo, "sm_unchanged"));
+	cl_git_pass(git_submodule_status(&status, sm));
+
+	cl_assert(GIT_SUBMODULE_STATUS_IS_UNMODIFIED(status));
+
+	expected = GIT_SUBMODULE_STATUS_IN_HEAD |
+		GIT_SUBMODULE_STATUS_IN_INDEX |
+		GIT_SUBMODULE_STATUS_IN_CONFIG |
+		GIT_SUBMODULE_STATUS_IN_WD;
+
+	cl_assert(status == expected);
+}