FYI: git-am allows creation of empty commits.

FYI: git-am allows creation of empty commits.

[Eric W. Biederman]

Currently I am using: git version 1.2.2.g709a.

I have been rebasing and refactoring a number of patchsets using git.

Frequently because of prior edits I will git a patch that does not
apply at all.  So all I get is the creation of the .dotest directory.
The automatic merge logic does not kick in, so I have to manually
call patch -p1 and resolve the differences but hand.  There everything
is expected and fine.  Although it might have been nice to have one
of those failed merge indicators in the index.

After fixing it up and doing all of my edits I occasionally forget
the git-update-index step, before calling git-am --resolved.  This
proceeds along it's merry way and creates an empty commit.

Is this the expected correct behavior or should git-am complain about
a situation like that?

Eric

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

[Junio C Hamano]

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

> After fixing it up and doing all of my edits I occasionally forget
> the git-update-index step, before calling git-am --resolved.  This
> proceeds along it's merry way and creates an empty commit.

Certainly a safty measure is missing here.  Thanks for
noticing.  How about something like this?

---
diff --git a/git-am.sh b/git-am.sh
index 85ecada..6730813 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -300,10 +300,16 @@ do
 	    } >"$dotest/final-commit"
 	    ;;
 	*)
-		case "$resolved,$interactive" in
-		tt)
-			# This is used only for interactive view option.
+		case "$resolved" in
+		t)
+			# This is used for interactive view option, but
+			# also we should see if the user really did
+			# something...
 			git-diff-index -p --cached HEAD >"$dotest/patch"
+			test -s "$dotest/patch" || {
+				echo "You said resolved, but there is no change in the index..."
+				stop_here $this
+			}
 			;;
 		esac
 	esac

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

[Eric W. Biederman]

Junio C Hamano <junkio@cox.net> writes:

> ebiederm@xmission.com (Eric W. Biederman) writes:
>
>> After fixing it up and doing all of my edits I occasionally forget
>> the git-update-index step, before calling git-am --resolved.  This
>> proceeds along it's merry way and creates an empty commit.
>
> Certainly a safty measure is missing here.  Thanks for
> noticing.  How about something like this?

Just tested it the patch works, at least in my simple contrived
example :)

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.

Eric

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

[Junio C Hamano]

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 <junkio@cox.net>
---
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));

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

[Junio C Hamano]

Allow "git-commit-tree v2.6.15^{tree} -p HEAD", instead of
requiring "git-commit-tree `git rev-parse v2.6.15^{tree}` -p
HEAD".  The parent parameter that follows -p uses get_sha1() to
understand the extended notation, and there is little reason not
to allow it for the tree object name parameter.

Also make the check_valid() function simpler.  This function
which predates sha1_object_info() so it had to do things by hand
but there is no reason to read the data just to discard anymore.

Signed-off-by: Junio C Hamano <junkio@cox.net>

---

 * Replacement patch.  Very lightly tested.

 commit-tree.c |   27 ++++++++++++++++++++-------
 1 files changed, 20 insertions(+), 7 deletions(-)

8f97300b2580b43905ce11eef21e77c5e8e809a6
diff --git a/commit-tree.c b/commit-tree.c
index 88871b0..b1d816c 100644
--- a/commit-tree.c
+++ b/commit-tree.c
@@ -43,14 +43,11 @@ static void add_buffer(char **bufp, unsi
 
 static void check_valid(unsigned char *sha1, const char *expect)
 {
-	void *buf;
 	char type[20];
-	unsigned long size;
 
-	buf = read_sha1_file(sha1, type, &size);
-	if (!buf || strcmp(type, expect))
-		die("%s is not a valid '%s' object", sha1_to_hex(sha1), expect);
-	free(buf);
+	if (sha1_object_info(sha1, type, NULL) || strcmp(type, expect))
+		die("%s is not a valid '%s' object",
+		    sha1_to_hex(sha1), expect);
 }
 
 /*
@@ -75,6 +72,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;
@@ -90,7 +100,7 @@ int main(int argc, char **argv)
 
 	git_config(git_default_config);
 
-	if (argc < 2 || get_sha1_hex(argv[1], tree_sha1) < 0)
+	if (argc < 2 || get_sha1(argv[1], tree_sha1) < 0)
 		usage(commit_tree_usage);
 
 	check_valid(tree_sha1, "tree");
@@ -105,6 +115,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));
-- 
1.2.3.g7465

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

[Eric W. Biederman]

Junio C Hamano <junkio@cox.net> writes:

> Allow "git-commit-tree v2.6.15^{tree} -p HEAD", instead of
> requiring "git-commit-tree `git rev-parse v2.6.15^{tree}` -p
> HEAD".  The parent parameter that follows -p uses get_sha1() to
> understand the extended notation, and there is little reason not
> to allow it for the tree object name parameter.
>
> Also make the check_valid() function simpler.  This function
> which predates sha1_object_info() so it had to do things by hand
> but there is no reason to read the data just to discard anymore.
>
> Signed-off-by: Junio C Hamano <junkio@cox.net>

Looks good to me. The logic at least looks complete.

Eric

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

[Eric Wong]

Junio C Hamano <junkio@cox.net> wrote:
> 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.

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

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

[Junio C Hamano]

Eric Wong <normalperson@yhbt.net> writes:

>> 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.