Merge pull request #1556 from arrbee/diff-patch-fixes

Diff patch bug fixes

include/git2/diff.h

@@ -356,8 +356,10 @@ typedef enum {
 	GIT_DIFF_LINE_CONTEXT   = ' ',
 	GIT_DIFF_LINE_ADDITION  = '+',
 	GIT_DIFF_LINE_DELETION  = '-',
-	GIT_DIFF_LINE_ADD_EOFNL = '\n', /**< Removed line w/o LF & added one with */
-	GIT_DIFF_LINE_DEL_EOFNL = '\0', /**< LF was removed at end of file */
+
+	GIT_DIFF_LINE_CONTEXT_EOFNL = '=', /**< Both files have no LF at end */
+	GIT_DIFF_LINE_ADD_EOFNL = '>',     /**< Old has no LF at end, new does */
+	GIT_DIFF_LINE_DEL_EOFNL = '<',     /**< Old has LF at end, new does not */
 
 	/* The following values will only be sent to a `git_diff_data_cb` when
 	 * the content of a diff is being formatted (eg. through
@@ -488,6 +490,8 @@ typedef struct {
 
 /**
  * Deallocate a diff list.
+ *
+ * @param diff The previously created diff list; cannot be used after free.
  */
 GIT_EXTERN(void) git_diff_list_free(git_diff_list *diff);
 
@@ -497,12 +501,14 @@ GIT_EXTERN(void) git_diff_list_free(git_diff_list *diff);
  * This is equivalent to `git diff <old-tree> <new-tree>`
  *
  * The first tree will be used for the "old_file" side of the delta and the
- * second tree will be used for the "new_file" side of the delta.
+ * second tree will be used for the "new_file" side of the delta.  You can
+ * pass NULL to indicate an empty tree, although it is an error to pass
+ * NULL for both the `old_tree` and `new_tree`.
  *
  * @param diff Output pointer to a git_diff_list pointer to be allocated.
  * @param repo The repository containing the trees.
- * @param old_tree A git_tree object to diff from.
- * @param new_tree A git_tree object to diff to.
+ * @param old_tree A git_tree object to diff from, or NULL for empty tree.
+ * @param new_tree A git_tree object to diff to, or NULL for empty tree.
  * @param opts Structure with options to influence diff or NULL for defaults.
  */
 GIT_EXTERN(int) git_diff_tree_to_tree(
@@ -523,7 +529,7 @@ GIT_EXTERN(int) git_diff_tree_to_tree(
  *
  * @param diff Output pointer to a git_diff_list pointer to be allocated.
  * @param repo The repository containing the tree and index.
- * @param old_tree A git_tree object to diff from.
+ * @param old_tree A git_tree object to diff from, or NULL for empty tree.
  * @param index The index to diff with; repo index used if NULL.
  * @param opts Structure with options to influence diff or NULL for defaults.
  */
@@ -582,7 +588,7 @@ GIT_EXTERN(int) git_diff_index_to_workdir(
  *
  * @param diff A pointer to a git_diff_list pointer that will be allocated.
  * @param repo The repository containing the tree.
- * @param old_tree A git_tree object to diff from.
+ * @param old_tree A git_tree object to diff from, or NULL for empty tree.
  * @param opts Structure with options to influence diff or NULL for defaults.
  */
 GIT_EXTERN(int) git_diff_tree_to_workdir(
@@ -926,7 +932,14 @@ GIT_EXTERN(int) git_diff_patch_to_str(
  * to 1 and no call to the hunk_cb nor line_cb will be made (unless you pass
  * `GIT_DIFF_FORCE_TEXT` of course).
  *
- * @return 0 on success, GIT_EUSER on non-zero callback, or error code
+ * @param old_blob Blob for old side of diff, or NULL for empty blob
+ * @param new_blob Blob for new side of diff, or NULL for empty blob
+ * @param options Options for diff, or NULL for default options
+ * @param file_cb Callback for "file"; made once if there is a diff; can be NULL
+ * @param hunk_cb Callback for each hunk in diff; can be NULL
+ * @param line_cb Callback for each line in diff; can be NULL
+ * @param payload Payload passed to each callback function
+ * @return 0 on success, GIT_EUSER on non-zero callback return, or error code
  */
 GIT_EXTERN(int) git_diff_blobs(
 	const git_blob *old_blob,
@@ -949,7 +962,15 @@ GIT_EXTERN(int) git_diff_blobs(
  * entire content of the buffer added).  Passing NULL to the buffer will do
  * the reverse, with GIT_DELTA_REMOVED and blob content removed.
  *
- * @return 0 on success, GIT_EUSER on non-zero callback, or error code
+ * @param old_blob Blob for old side of diff, or NULL for empty blob
+ * @param buffer Raw data for new side of diff
+ * @param buffer_len Length of raw data for new side of diff
+ * @param options Options for diff, or NULL for default options
+ * @param file_cb Callback for "file"; made once if there is a diff; can be NULL
+ * @param hunk_cb Callback for each hunk in diff; can be NULL
+ * @param line_cb Callback for each line in diff; can be NULL
+ * @param payload Payload passed to each callback function
+ * @return 0 on success, GIT_EUSER on non-zero callback return, or error code
  */
 GIT_EXTERN(int) git_diff_blob_to_buffer(
 	const git_blob *old_blob,

src/diff.c

@@ -389,6 +389,9 @@ static int diff_list_apply_options(
 
 	/* Don't set GIT_DIFFCAPS_USE_DEV - compile time option in core git */
 
+	/* Set GIT_DIFFCAPS_TRUST_NANOSECS on a platform basis */
+	diff->diffcaps = diff->diffcaps | GIT_DIFFCAPS_TRUST_NANOSECS;
+
 	/* If not given explicit `opts`, check `diff.xyz` configs */
 	if (!opts) {
 		diff->opts.context_lines = config_int(cfg, "diff.context", 3);
@@ -529,6 +532,13 @@ cleanup:
 	return result;
 }
 
+static bool diff_time_eq(
+	const git_index_time *a, const git_index_time *b, bool use_nanos)
+{
+	return a->seconds == a->seconds &&
+		(!use_nanos || a->nanoseconds == b->nanoseconds);
+}
+
 typedef struct {
 	git_repository *repo;
 	git_iterator *old_iter;
@@ -540,11 +550,51 @@ typedef struct {
 
 #define MODE_BITS_MASK 0000777
 
+static int maybe_modified_submodule(
+	git_delta_t *status,
+	git_oid *found_oid,
+	git_diff_list *diff,
+	diff_in_progress *info)
+{
+	int error = 0;
+	git_submodule *sub;
+	unsigned int sm_status = 0;
+	const git_oid *sm_oid;
+
+	*status = GIT_DELTA_UNMODIFIED;
+
+	if (!DIFF_FLAG_IS_SET(diff, GIT_DIFF_IGNORE_SUBMODULES) &&
+		!(error = git_submodule_lookup(
+			  &sub, diff->repo, info->nitem->path)) &&
+		git_submodule_ignore(sub) != GIT_SUBMODULE_IGNORE_ALL &&
+		!(error = git_submodule_status(&sm_status, sub)))
+	{
+		/* check IS_WD_UNMODIFIED because this case is only used
+		 * when the new side of the diff is the working directory
+		 */
+		if (!GIT_SUBMODULE_STATUS_IS_WD_UNMODIFIED(sm_status))
+			*status = GIT_DELTA_MODIFIED;
+
+		/* grab OID while we are here */
+		if (git_oid_iszero(&info->nitem->oid) &&
+			(sm_oid = git_submodule_wd_id(sub)) != NULL)
+			git_oid_cpy(found_oid, sm_oid);
+	}
+
+	/* GIT_EEXISTS means a dir with .git in it was found - ignore it */
+	if (error == GIT_EEXISTS) {
+		giterr_clear();
+		error = 0;
+	}
+
+	return error;
+}
+
 static int maybe_modified(
 	git_diff_list *diff,
 	diff_in_progress *info)
 {
-	git_oid noid, *use_noid = NULL;
+	git_oid noid;
 	git_delta_t status = GIT_DELTA_MODIFIED;
 	const git_index_entry *oitem = info->oitem;
 	const git_index_entry *nitem = info->nitem;
@@ -560,6 +610,8 @@ static int maybe_modified(
 			&matched_pathspec))
 		return 0;
 
+	memset(&noid, 0, sizeof(noid));
+
 	/* on platforms with no symlinks, preserve mode of existing symlinks */
 	if (S_ISLNK(omode) && S_ISREG(nmode) && new_is_workdir &&
 		!(diff->diffcaps & GIT_DIFFCAPS_HAS_SYMLINKS))
@@ -600,55 +652,30 @@ static int maybe_modified(
 	 * circumstances that can accelerate things or need special handling
 	 */
 	else if (git_oid_iszero(&nitem->oid) && new_is_workdir) {
-		/* TODO: add check against index file st_mtime to avoid racy-git */
+		bool use_ctime = ((diff->diffcaps & GIT_DIFFCAPS_TRUST_CTIME) != 0);
+		bool use_nanos = ((diff->diffcaps & GIT_DIFFCAPS_TRUST_NANOSECS) != 0);
 
-		/* if the stat data looks exactly alike, then assume the same */
-		if (omode == nmode &&
-			oitem->file_size == nitem->file_size &&
-			(!(diff->diffcaps & GIT_DIFFCAPS_TRUST_CTIME) ||
-			 (oitem->ctime.seconds == nitem->ctime.seconds)) &&
-			oitem->mtime.seconds == nitem->mtime.seconds &&
-			(!(diff->diffcaps & GIT_DIFFCAPS_USE_DEV) ||
-			 (oitem->dev == nitem->dev)) &&
-			oitem->ino == nitem->ino &&
-			oitem->uid == nitem->uid &&
-			oitem->gid == nitem->gid)
 		status = GIT_DELTA_UNMODIFIED;
 
-		else if (S_ISGITLINK(nmode)) {
-			int err;
-			git_submodule *sub;
+		/* TODO: add check against index file st_mtime to avoid racy-git */
 
-			if (DIFF_FLAG_IS_SET(diff, GIT_DIFF_IGNORE_SUBMODULES))
-				status = GIT_DELTA_UNMODIFIED;
-			else if ((err = git_submodule_lookup(&sub, diff->repo, nitem->path)) < 0) {
-				if (err == GIT_EEXISTS)
-					status = GIT_DELTA_UNMODIFIED;
-				else
-					return err;
-			} else if (git_submodule_ignore(sub) == GIT_SUBMODULE_IGNORE_ALL)
-				status = GIT_DELTA_UNMODIFIED;
-			else {
-				unsigned int sm_status = 0;
-				if (git_submodule_status(&sm_status, sub) < 0)
+		if (S_ISGITLINK(nmode)) {
+			if (maybe_modified_submodule(&status, &noid, diff, info) < 0)
 				return -1;
+		}
 
-				/* check IS_WD_UNMODIFIED because this case is only used
-				 * when the new side of the diff is the working directory
+		/* if the stat data looks different, then mark modified - this just
+		 * means that the OID will be recalculated below to confirm change
 		 */
-				status = GIT_SUBMODULE_STATUS_IS_WD_UNMODIFIED(sm_status)
-						 ? GIT_DELTA_UNMODIFIED : GIT_DELTA_MODIFIED;
-
-				/* grab OID while we are here */
-				if (git_oid_iszero(&nitem->oid)) {
-					const git_oid *sm_oid = git_submodule_wd_id(sub);
-					if (sm_oid != NULL) {
-						git_oid_cpy(&noid, sm_oid);
-						use_noid = &noid;
-					}
-				}
-			}
-		}
+		else if (omode != nmode ||
+			oitem->file_size != nitem->file_size ||
+			!diff_time_eq(&oitem->mtime, &nitem->mtime, use_nanos) ||
+			(use_ctime &&
+			 !diff_time_eq(&oitem->ctime, &nitem->ctime, use_nanos)) ||
+			oitem->ino != nitem->ino ||
+			oitem->uid != nitem->uid ||
+			oitem->gid != nitem->gid)
+			status = GIT_DELTA_MODIFIED;
 	}
 
 	/* if mode is GITLINK and submodules are ignored, then skip */
@@ -660,11 +687,10 @@ static int maybe_modified(
 	 * haven't calculated the OID of the new item, then calculate it now
 	 */
 	if (status != GIT_DELTA_UNMODIFIED && git_oid_iszero(&nitem->oid)) {
-		if (!use_noid) {
+		if (git_oid_iszero(&noid)) {
 			if (git_diff__oid_for_file(diff->repo,
 					nitem->path, nitem->mode, nitem->file_size, &noid) < 0)
 				return -1;
-			use_noid = &noid;
 		}
 
 		/* if oid matches, then mark unmodified (except submodules, where
@@ -672,12 +698,13 @@ static int maybe_modified(
 		 * matches between the index and the workdir HEAD)
 		 */
 		if (omode == nmode && !S_ISGITLINK(omode) &&
-			git_oid_equal(&oitem->oid, use_noid))
+			git_oid_equal(&oitem->oid, &noid))
 			status = GIT_DELTA_UNMODIFIED;
 	}
 
 	return diff_delta__from_two(
-		diff, status, oitem, omode, nitem, nmode, use_noid, matched_pathspec);
+		diff, status, oitem, omode, nitem, nmode,
+		git_oid_iszero(&noid) ? NULL : &noid, matched_pathspec);
 }
 
 static bool entry_is_prefixed(

src/diff.h

@@ -26,6 +26,7 @@ enum {
 	GIT_DIFFCAPS_TRUST_MODE_BITS  = (1 << 2), /* use st_mode? */
 	GIT_DIFFCAPS_TRUST_CTIME      = (1 << 3), /* use st_ctime? */
 	GIT_DIFFCAPS_USE_DEV          = (1 << 4), /* use st_dev? */
+	GIT_DIFFCAPS_TRUST_NANOSECS   = (1 << 5), /* use stat time nanoseconds */
 };
 
 enum {

src/diff_output.c

@@ -726,7 +726,7 @@ static int diff_patch_cb(void *priv, mmbuffer_t *bufs, int len)
 		char origin =
 			(*bufs[0].ptr == '+') ? GIT_DIFF_LINE_DEL_EOFNL :
 			(*bufs[0].ptr == '-') ? GIT_DIFF_LINE_ADD_EOFNL :
-			GIT_DIFF_LINE_CONTEXT;
+			GIT_DIFF_LINE_CONTEXT_EOFNL;
 
 		if (ctxt->data_cb != NULL &&
 			ctxt->data_cb(patch->delta, &ctxt->range,
@@ -930,11 +930,13 @@ static int diff_patch_line_cb(
 
 	switch (line_origin) {
 	case GIT_DIFF_LINE_ADDITION:
+	case GIT_DIFF_LINE_DEL_EOFNL:
 		line->oldno = -1;
 		line->newno = patch->newno;
 		patch->newno += line->lines;
 		break;
 	case GIT_DIFF_LINE_DELETION:
+	case GIT_DIFF_LINE_ADD_EOFNL:
 		line->oldno = patch->oldno;
 		line->newno = -1;
 		patch->oldno += line->lines;

tests-clar/diff/diff_helpers.c

@@ -97,20 +97,15 @@ int diff_line_cb(
 	e->lines++;
 	switch (line_origin) {
 	case GIT_DIFF_LINE_CONTEXT:
+	case GIT_DIFF_LINE_CONTEXT_EOFNL: /* techically not a line */
 		e->line_ctxt++;
 		break;
 	case GIT_DIFF_LINE_ADDITION:
-		e->line_adds++;
-		break;
-	case GIT_DIFF_LINE_ADD_EOFNL:
-		/* technically not a line add, but we'll count it as such */
+	case GIT_DIFF_LINE_ADD_EOFNL: /* technically not a line add */
 		e->line_adds++;
 		break;
 	case GIT_DIFF_LINE_DELETION:
-		e->line_dels++;
-		break;
-	case GIT_DIFF_LINE_DEL_EOFNL:
-		/* technically not a line delete, but we'll count it as such */
+	case GIT_DIFF_LINE_DEL_EOFNL: /* technically not a line delete */
 		e->line_dels++;
 		break;
 	default: