Re: FYI: git-am allows creation of empty commits.

ebiederm@xmission.com (Eric W. Biederman) writes:

Is this something that we always want to test for in the porcelain
or do we want to move a check into git-commit-tree?

For getting a reasonable error message where you have the test
seems to be the only sane place, but having the check deeper
down would be more likely to catch this kind of thing.

I think 99.9% of the time it is a mistake if a single-parented
commit has the same tree as its parent commit has, so having a
check in commit-tree may not be a bad idea.

If you want to do it, however, you need to be a bit careful
about merges.  In a multi-parented commit, it is legitimate if
the merge result exactly match one of the parents (in fact
"git-merge -s ours" can be used to create a merge that matches
its first parent).

The commit-tree program is one of the oldest part of the system,
and as a mechanism, does not set nor enforce policies like this.
It has been more liberal than the best current practice, such as
(1) don't do single-parent empty commit; (2) don't list the same
parent twice.  The second one was introduced in mid-June last
year, which is "long after it was invented" in git timescale.

On the other hand, it is more anal than it could be.  It does
not take a tag that points to a commit to its -p parameter.
That's because we did not have tag objects in the beginning, and
by the time tags were introduced, nobody ran commit-tree by hand
anymore.

-- >8 --
[PATCH] commit-tree: disallow an empty single-parent commit.

Also allow "git-commit-tree -p v2.6.16-rc2", instead of having
to say "git-commit-tree -p v2.6.16-rc2^0".

Signed-off-by: Junio C Hamano <redacted>
---

diff --git a/commit-tree.c b/commit-tree.c
index 88871b0..14a7552 100644
--- a/commit-tree.c
+++ b/commit-tree.c

@@ -45,12 +45,14 @@ static void check_valid(unsigned char *s
 {
 	void *buf;
 	char type[20];
+	unsigned char real_sha1[20];
 	unsigned long size;
 
-	buf = read_sha1_file(sha1, type, &size);
-	if (!buf || strcmp(type, expect))
+	buf = read_object_with_reference(sha1, expect, &size, real_sha1);
+	if (!buf)
 		die("%s is not a valid '%s' object", sha1_to_hex(sha1), expect);
 	free(buf);
+	memcpy(sha1, real_sha1, 20);
 }
 
 /*

@@ -75,6 +77,19 @@ static int new_parent(int idx)
 	return 1;
 }
 
+static int check_empty_commit(const unsigned char *tree_sha1,
+			      const unsigned char *parent_sha1)
+{
+	unsigned char sha1[20];
+	char refit[50];
+	sprintf(refit, "%s^{tree}", sha1_to_hex(parent_sha1));
+	if (get_sha1(refit, sha1))
+		die("cannot determine tree in parent commit.");
+	if (!memcmp(sha1, tree_sha1, 20))
+		return error ("empty commit?  aborting.");
+	return 0;
+}
+
 int main(int argc, char **argv)
 {
 	int i;

@@ -105,6 +120,9 @@ int main(int argc, char **argv)
 	}
 	if (!parents)
 		fprintf(stderr, "Committing initial tree %s\n", argv[1]);
+	if (parents == 1)
+		if (check_empty_commit(tree_sha1, parent_sha1[0]))
+			exit(1);
 
 	init_buffer(&buffer, &size);
 	add_buffer(&buffer, &size, "tree %s\n", sha1_to_hex(tree_sha1));

Junio C Hamano [off-list ref] wrote:

ebiederm@xmission.com (Eric W. Biederman) writes:

quoted

Is this something that we always want to test for in the porcelain
or do we want to move a check into git-commit-tree?

For getting a reasonable error message where you have the test
seems to be the only sane place, but having the check deeper
down would be more likely to catch this kind of thing.

I think 99.9% of the time it is a mistake if a single-parented
commit has the same tree as its parent commit has, so having a
check in commit-tree may not be a bad idea.

This would break importers, more than 0.1% I think...  Arch definitely allows
empty commits for getting log messages in.  SVN forbids them from their POV, but
they also have things that we can't see when we import (properties like: mime,
externals, eol-style) causing us to write the same tree twice.  Not sure about
CVS...

Maybe a flag such as --force could be added.

-- 
Eric Wong

Eric Wong [off-list ref] writes:

quoted

I think 99.9% of the time it is a mistake if a single-parented
commit has the same tree as its parent commit has, so having a
check in commit-tree may not be a bad idea.

This would break importers, more than 0.1% I think...  Arch
definitely allows empty commits for getting log messages in.
SVN forbids them from their POV, but they also have things
that we can't see when we import (properties like: mime,
externals, eol-style) causing us to write the same tree twice.
Not sure about CVS...

Maybe a flag such as --force could be added.

Good point perhaps.  Maybe an explicit --no-empty should ask for
it, and git-am should use it.  As to git-commit I am unsure.

In the meantime I'd drop the patch.