{"thread":{"id":"3447","subject":"FYI: git-am allows creation of empty commits.","startedAt":"2006-02-23T15:33:50Z","lastAt":"2006-02-25T06:04:32Z","messageCount":8,"participants":["Eric W. Biederman","Junio C Hamano","Eric Wong"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"16621","messageId":"m1slqahyxt.fsf@ebiederm.dsl.xmission.com","threadId":"3447","inReplyTo":null,"subject":"FYI: git-am allows creation of empty commits.","fromName":"Eric W. Biederman","fromEmail":"ebiederm@xmission.com","sentAt":"2006-02-23T15:33:50Z","receivedAt":"2006-02-23T15:33:50Z","isPatch":false,"sender":{"key":"ebiederm@xmission.com","avatar":"https://avatars.githubusercontent.com/u/7477136?v=4"},"body":"\nCurrently I am using: git version 1.2.2.g709a.\n\nI have been rebasing and refactoring a number of patchsets using git.\n\nFrequently because of prior edits I will git a patch that does not\napply at all.  So all I get is the creation of the .dotest directory.\nThe automatic merge logic does not kick in, so I have to manually\ncall patch -p1 and resolve the differences but hand.  There everything\nis expected and fine.  Although it might have been nice to have one\nof those failed merge indicators in the index.\n\nAfter fixing it up and doing all of my edits I occasionally forget\nthe git-update-index step, before calling git-am --resolved.  This\nproceeds along it's merry way and creates an empty commit.\n\nIs this the expected correct behavior or should git-am complain about\na situation like that?\n\nEric\n"},{"id":"16653","messageId":"7v1wxtgv02.fsf@assigned-by-dhcp.cox.net","threadId":"3447","inReplyTo":"m1slqahyxt.fsf@ebiederm.dsl.xmission.com","subject":"Re: FYI: git-am allows creation of empty commits.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-02-24T05:56:29Z","receivedAt":"2006-02-24T05:56:29Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"ebiederm@xmission.com (Eric W. Biederman) writes:\n\n> After fixing it up and doing all of my edits I occasionally forget\n> the git-update-index step, before calling git-am --resolved.  This\n> proceeds along it's merry way and creates an empty commit.\n\nCertainly a safty measure is missing here.  Thanks for\nnoticing.  How about something like this?\n\n---\ndiff --git a/git-am.sh b/git-am.sh\nindex 85ecada..6730813 100755\n--- a/git-am.sh\n+++ b/git-am.sh\n@@ -300,10 +300,16 @@ do\n \t    } >\"$dotest/final-commit\"\n \t    ;;\n \t*)\n-\t\tcase \"$resolved,$interactive\" in\n-\t\ttt)\n-\t\t\t# This is used only for interactive view option.\n+\t\tcase \"$resolved\" in\n+\t\tt)\n+\t\t\t# This is used for interactive view option, but\n+\t\t\t# also we should see if the user really did\n+\t\t\t# something...\n \t\t\tgit-diff-index -p --cached HEAD >\"$dotest/patch\"\n+\t\t\ttest -s \"$dotest/patch\" || {\n+\t\t\t\techo \"You said resolved, but there is no change in the index...\"\n+\t\t\t\tstop_here $this\n+\t\t\t}\n \t\t\t;;\n \t\tesac\n \tesac\n"},{"id":"16664","messageId":"m18xs1dmp3.fsf@ebiederm.dsl.xmission.com","threadId":"3447","inReplyTo":"7v1wxtgv02.fsf@assigned-by-dhcp.cox.net","subject":"Re: FYI: git-am allows creation of empty commits.","fromName":"Eric W. Biederman","fromEmail":"ebiederm@xmission.com","sentAt":"2006-02-24T11:24:08Z","receivedAt":"2006-02-24T11:24:08Z","isPatch":false,"sender":{"key":"ebiederm@xmission.com","avatar":"https://avatars.githubusercontent.com/u/7477136?v=4"},"body":"Junio C Hamano <junkio@cox.net> writes:\n\n> ebiederm@xmission.com (Eric W. Biederman) writes:\n>\n>> After fixing it up and doing all of my edits I occasionally forget\n>> the git-update-index step, before calling git-am --resolved.  This\n>> proceeds along it's merry way and creates an empty commit.\n>\n> Certainly a safty measure is missing here.  Thanks for\n> noticing.  How about something like this?\n\nJust tested it the patch works, at least in my simple contrived\nexample :)\n\nIs this something that we always want to test for in the porcelain\nor do we want to move a check into git-commit-tree?\n\nFor getting a reasonable error message where you have the test\nseems to be the only sane place, but having the check deeper\ndown would be more likely to catch this kind of thing.\n\nEric\n"},{"id":"16667","messageId":"7vy8019d44.fsf@assigned-by-dhcp.cox.net","threadId":"3447","inReplyTo":"m18xs1dmp3.fsf@ebiederm.dsl.xmission.com","subject":"Re: FYI: git-am allows creation of empty commits.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-02-24T12:04:43Z","receivedAt":"2006-02-24T12:04:43Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"ebiederm@xmission.com (Eric W. Biederman) writes:\n\n> Is this something that we always want to test for in the porcelain\n> or do we want to move a check into git-commit-tree?\n>\n> For getting a reasonable error message where you have the test\n> seems to be the only sane place, but having the check deeper\n> down would be more likely to catch this kind of thing.\n\nI think 99.9% of the time it is a mistake if a single-parented\ncommit has the same tree as its parent commit has, so having a\ncheck in commit-tree may not be a bad idea.\n\nIf you want to do it, however, you need to be a bit careful\nabout merges.  In a multi-parented commit, it is legitimate if\nthe merge result exactly match one of the parents (in fact\n\"git-merge -s ours\" can be used to create a merge that matches\nits first parent).\n\nThe commit-tree program is one of the oldest part of the system,\nand as a mechanism, does not set nor enforce policies like this.\nIt has been more liberal than the best current practice, such as\n(1) don't do single-parent empty commit; (2) don't list the same\nparent twice.  The second one was introduced in mid-June last\nyear, which is \"long after it was invented\" in git timescale.\n\nOn the other hand, it is more anal than it could be.  It does\nnot take a tag that points to a commit to its -p parameter.\nThat's because we did not have tag objects in the beginning, and\nby the time tags were introduced, nobody ran commit-tree by hand\nanymore.\n\n-- >8 --\n[PATCH] commit-tree: disallow an empty single-parent commit.\n\nAlso allow \"git-commit-tree -p v2.6.16-rc2\", instead of having\nto say \"git-commit-tree -p v2.6.16-rc2^0\".\n\nSigned-off-by: Junio C Hamano <junkio@cox.net>\n---\ndiff --git a/commit-tree.c b/commit-tree.c\nindex 88871b0..14a7552 100644\n--- a/commit-tree.c\n+++ b/commit-tree.c\n@@ -45,12 +45,14 @@ static void check_valid(unsigned char *s\n {\n \tvoid *buf;\n \tchar type[20];\n+\tunsigned char real_sha1[20];\n \tunsigned long size;\n \n-\tbuf = read_sha1_file(sha1, type, &size);\n-\tif (!buf || strcmp(type, expect))\n+\tbuf = read_object_with_reference(sha1, expect, &size, real_sha1);\n+\tif (!buf)\n \t\tdie(\"%s is not a valid '%s' object\", sha1_to_hex(sha1), expect);\n \tfree(buf);\n+\tmemcpy(sha1, real_sha1, 20);\n }\n \n /*\n@@ -75,6 +77,19 @@ static int new_parent(int idx)\n \treturn 1;\n }\n \n+static int check_empty_commit(const unsigned char *tree_sha1,\n+\t\t\t      const unsigned char *parent_sha1)\n+{\n+\tunsigned char sha1[20];\n+\tchar refit[50];\n+\tsprintf(refit, \"%s^{tree}\", sha1_to_hex(parent_sha1));\n+\tif (get_sha1(refit, sha1))\n+\t\tdie(\"cannot determine tree in parent commit.\");\n+\tif (!memcmp(sha1, tree_sha1, 20))\n+\t\treturn error (\"empty commit?  aborting.\");\n+\treturn 0;\n+}\n+\n int main(int argc, char **argv)\n {\n \tint i;\n@@ -105,6 +120,9 @@ int main(int argc, char **argv)\n \t}\n \tif (!parents)\n \t\tfprintf(stderr, \"Committing initial tree %s\\n\", argv[1]);\n+\tif (parents == 1)\n+\t\tif (check_empty_commit(tree_sha1, parent_sha1[0]))\n+\t\t\texit(1);\n \n \tinit_buffer(&buffer, &size);\n \tadd_buffer(&buffer, &size, \"tree %s\\n\", sha1_to_hex(tree_sha1));\n"},{"id":"16668","messageId":"7vslq99cd5.fsf_-_@assigned-by-dhcp.cox.net","threadId":"3447","inReplyTo":"7vy8019d44.fsf@assigned-by-dhcp.cox.net","subject":"[PATCH] commit-tree: disallow an empty single-parent commit.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-02-24T12:20:54Z","receivedAt":"2006-02-24T12:20:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Allow \"git-commit-tree v2.6.15^{tree} -p HEAD\", instead of\nrequiring \"git-commit-tree `git rev-parse v2.6.15^{tree}` -p\nHEAD\".  The parent parameter that follows -p uses get_sha1() to\nunderstand the extended notation, and there is little reason not\nto allow it for the tree object name parameter.\n\nAlso make the check_valid() function simpler.  This function\nwhich predates sha1_object_info() so it had to do things by hand\nbut there is no reason to read the data just to discard anymore.\n\nSigned-off-by: Junio C Hamano <junkio@cox.net>\n\n---\n\n * Replacement patch.  Very lightly tested.\n\n commit-tree.c |   27 ++++++++++++++++++++-------\n 1 files changed, 20 insertions(+), 7 deletions(-)\n\n8f97300b2580b43905ce11eef21e77c5e8e809a6\ndiff --git a/commit-tree.c b/commit-tree.c\nindex 88871b0..b1d816c 100644\n--- a/commit-tree.c\n+++ b/commit-tree.c\n@@ -43,14 +43,11 @@ static void add_buffer(char **bufp, unsi\n \n static void check_valid(unsigned char *sha1, const char *expect)\n {\n-\tvoid *buf;\n \tchar type[20];\n-\tunsigned long size;\n \n-\tbuf = read_sha1_file(sha1, type, &size);\n-\tif (!buf || strcmp(type, expect))\n-\t\tdie(\"%s is not a valid '%s' object\", sha1_to_hex(sha1), expect);\n-\tfree(buf);\n+\tif (sha1_object_info(sha1, type, NULL) || strcmp(type, expect))\n+\t\tdie(\"%s is not a valid '%s' object\",\n+\t\t    sha1_to_hex(sha1), expect);\n }\n \n /*\n@@ -75,6 +72,19 @@ static int new_parent(int idx)\n \treturn 1;\n }\n \n+static int check_empty_commit(const unsigned char *tree_sha1,\n+\t\t\t      const unsigned char *parent_sha1)\n+{\n+\tunsigned char sha1[20];\n+\tchar refit[50];\n+\tsprintf(refit, \"%s^{tree}\", sha1_to_hex(parent_sha1));\n+\tif (get_sha1(refit, sha1))\n+\t\tdie(\"cannot determine tree in parent commit.\");\n+\tif (!memcmp(sha1, tree_sha1, 20))\n+\t\treturn error (\"empty commit?  aborting.\");\n+\treturn 0;\n+}\n+\n int main(int argc, char **argv)\n {\n \tint i;\n@@ -90,7 +100,7 @@ int main(int argc, char **argv)\n \n \tgit_config(git_default_config);\n \n-\tif (argc < 2 || get_sha1_hex(argv[1], tree_sha1) < 0)\n+\tif (argc < 2 || get_sha1(argv[1], tree_sha1) < 0)\n \t\tusage(commit_tree_usage);\n \n \tcheck_valid(tree_sha1, \"tree\");\n@@ -105,6 +115,9 @@ int main(int argc, char **argv)\n \t}\n \tif (!parents)\n \t\tfprintf(stderr, \"Committing initial tree %s\\n\", argv[1]);\n+\tif (parents == 1)\n+\t\tif (check_empty_commit(tree_sha1, parent_sha1[0]))\n+\t\t\texit(1);\n \n \tinit_buffer(&buffer, &size);\n \tadd_buffer(&buffer, &size, \"tree %s\\n\", sha1_to_hex(tree_sha1));\n-- \n1.2.3.g7465\n"},{"id":"16669","messageId":"m1y800dibp.fsf@ebiederm.dsl.xmission.com","threadId":"3447","inReplyTo":"7vslq99cd5.fsf_-_@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] commit-tree: disallow an empty single-parent commit.","fromName":"Eric W. Biederman","fromEmail":"ebiederm@xmission.com","sentAt":"2006-02-24T12:58:34Z","receivedAt":"2006-02-24T12:58:34Z","isPatch":true,"sender":{"key":"ebiederm@xmission.com","avatar":"https://avatars.githubusercontent.com/u/7477136?v=4"},"body":"Junio C Hamano <junkio@cox.net> writes:\n\n> Allow \"git-commit-tree v2.6.15^{tree} -p HEAD\", instead of\n> requiring \"git-commit-tree `git rev-parse v2.6.15^{tree}` -p\n> HEAD\".  The parent parameter that follows -p uses get_sha1() to\n> understand the extended notation, and there is little reason not\n> to allow it for the tree object name parameter.\n>\n> Also make the check_valid() function simpler.  This function\n> which predates sha1_object_info() so it had to do things by hand\n> but there is no reason to read the data just to discard anymore.\n>\n> Signed-off-by: Junio C Hamano <junkio@cox.net>\n\nLooks good to me. The logic at least looks complete.\n\nEric\n"},{"id":"16670","messageId":"20060224131922.GA19401@localdomain","threadId":"3447","inReplyTo":"7vy8019d44.fsf@assigned-by-dhcp.cox.net","subject":"Re: FYI: git-am allows creation of empty commits.","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2006-02-24T13:19:22Z","receivedAt":"2006-02-24T13:19:22Z","isPatch":false,"sender":{"key":"e@80x24.org","avatar":null},"body":"Junio C Hamano <junkio@cox.net> wrote:\n> ebiederm@xmission.com (Eric W. Biederman) writes:\n> \n> > Is this something that we always want to test for in the porcelain\n> > or do we want to move a check into git-commit-tree?\n> >\n> > For getting a reasonable error message where you have the test\n> > seems to be the only sane place, but having the check deeper\n> > down would be more likely to catch this kind of thing.\n> \n> I think 99.9% of the time it is a mistake if a single-parented\n> commit has the same tree as its parent commit has, so having a\n> check in commit-tree may not be a bad idea.\n\nThis would break importers, more than 0.1% I think...  Arch definitely allows\nempty commits for getting log messages in.  SVN forbids them from their POV, but\nthey also have things that we can't see when we import (properties like: mime,\nexternals, eol-style) causing us to write the same tree twice.  Not sure about\nCVS...\n\nMaybe a flag such as --force could be added.\n\n-- \nEric Wong\n"},{"id":"16726","messageId":"7v64n47z4f.fsf@assigned-by-dhcp.cox.net","threadId":"3447","inReplyTo":"20060224131922.GA19401@localdomain","subject":"Re: FYI: git-am allows creation of empty commits.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-02-25T06:04:32Z","receivedAt":"2006-02-25T06:04:32Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong <normalperson@yhbt.net> writes:\n\n>> I think 99.9% of the time it is a mistake if a single-parented\n>> commit has the same tree as its parent commit has, so having a\n>> check in commit-tree may not be a bad idea.\n>\n> This would break importers, more than 0.1% I think...  Arch\n> definitely allows empty commits for getting log messages in.\n> SVN forbids them from their POV, but they also have things\n> that we can't see when we import (properties like: mime,\n> externals, eol-style) causing us to write the same tree twice.\n> Not sure about CVS...\n>\n> Maybe a flag such as --force could be added.\n\nGood point perhaps.  Maybe an explicit --no-empty should ask for\nit, and git-am should use it.  As to git-commit I am unsure.\n\nIn the meantime I'd drop the patch.\n"}]}