{"thread":{"id":"43278","subject":"Re: [PATCH] Make git-commit cleverer - have it figure out whether it needs -a automatically","startedAt":"2006-12-01T11:06:42Z","lastAt":"2006-12-03T09:57:02Z","messageCount":7,"participants":["Andy Parkins","Carl Worth","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"298455","messageId":"200612011106.42272.andyparkins@gmail.com","threadId":"43278","inReplyTo":null,"subject":"[PATCH] Make git-commit cleverer - have it figure out whether it needs -a automatically","fromName":"Andy Parkins","fromEmail":"andyparkins@gmail.com","sentAt":"2006-12-01T11:06:42Z","receivedAt":"2006-12-01T11:06:42Z","isPatch":true,"sender":{"key":"andyparkins@gmail.com","avatar":null},"body":"Raimund Bauer offered this suggestion (paraphrased):\n\n\"Maybe we could do git-commit -a _only_ if the index matches HEAD, and\notherwise keep current behavior? So people who don't care about the\nindex won't get tripped up, and when you do have a dirty index, you get\ntold about it?\"\n\nJohannes Schindelin pointed out that this isn't the right thing to do for\nan --amend, so that is checked for. Additionally, it's probably not the\nright thing to do if any files are specified with \"--only\" or\n\"--include\", so they turn this behaviour off as well.\n\nNguyen Thai Ngoc Duy asked that git-commit let you know it's done this\nby adding an extra comment to the commit message.\n\nSigned-off-by: Andy Parkins \n---\nThis time we also inhibit if \"all\" mode is already set; if the user\nhas specified \"-a\" we don't need to tell them that we've switched \"-a\"\nmode on.\n\n\n git-commit.sh | 10 ++++++++++\n 1 files changed, 10 insertions(+), 0 deletions(-)\n\ndiff --git a/git-commit.sh b/git-commit.sh\nindex 81c3a0c..4552727 100755\n--- a/git-commit.sh\n+++ b/git-commit.sh\n@@ -265,6 +265,16 @@ $1\"\n done\n case \"$edit_flag\" in t) no_edit= ;; esac\n \n+# Clever commit - if this commit would do nothing, then make it an \"all\"\n+# commit\n+if [ -z \"$(git-diff-index --cached --name-only HEAD)\" \\\n+\t-a -z \"$amend\" -a -z \"$only\" -a -z \"$also\" -a -z \"$all\" ]; then\n+\techo \"# There was nothing to commit but changes were detected in the\" > $GIT_DIR/SQUASH_MSG\n+\techo \"# working tree. 'git commit -a' mode activated.\" >> $GIT_DIR/SQUASH_MSG\n+\techo \"#\" >> $GIT_DIR/SQUASH_MSG\n+\tall=t\n+fi\n+\n ################################################################\n # Sanity check options\n \n-- \n1.4.4.1.g3ece-dirty\n"},{"id":"296662","messageId":"7vd573amuy.fsf@assigned-by-dhcp.cox.net","threadId":"43278","inReplyTo":"200612011106.42272.andyparkins@gmail.com","subject":"Re: [PATCH] Make git-commit cleverer - have it figure out whether it needs -a automatically","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-12-01T11:15:17Z","receivedAt":"2006-12-01T11:15:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andy Parkins writes:\n\n> Raimund Bauer offered this suggestion (paraphrased):\n>\n> \"Maybe we could do git-commit -a _only_ if the index matches HEAD, and\n> otherwise keep current behavior? So people who don't care about the\n> index won't get tripped up, and when you do have a dirty index, you get\n> told about it?\"\n>\n> Johannes Schindelin pointed out that this isn't the right thing to do for\n> an --amend, so that is checked for. Additionally, it's probably not the\n> right thing to do if any files are specified with \"--only\" or\n> \"--include\", so they turn this behaviour off as well.\n>\n> Nguyen Thai Ngoc Duy asked that git-commit let you know it's done this\n> by adding an extra comment to the commit message.\n\nI think another exception should be needed. If the index does\nnot match the working tree, it should not default to \"-a\".\n\nOtherwise,\n\nI want to fix another thing in pickaxe.\n\n\t$ edit builtin-blame.c\n\nMy wife calls me. Away from desk for 20 minutes. Later I come\nback.\n\n\t$ git update-index builtin-pickaxe.c\n\nI am so used to that name and did not realize that typo, and I\nwas not paying too much attention. My wife calls me again.\nAway from desk and back in 20 minutes.\n\n\t$ git commit -m 'git-blame: Another fix.'\n\nOops.\n\nSo, please turn this \"cleverness\" off when the index does not\nmatch the working tree.\n\n\n"},{"id":"296790","messageId":"7v7ixbam2u.fsf@assigned-by-dhcp.cox.net","threadId":"43278","inReplyTo":"7vd573amuy.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Make git-commit cleverer - have it figure out whether it needs -a automatically","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-12-01T11:32:09Z","receivedAt":"2006-12-01T11:32:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I think another exception should be needed. If the index does\n> not match the working tree, it should not default to \"-a\".\n>\n> Otherwise,\n\nI think there needs a bit of explanation and additional step\nthat happened here. This by the way is not a made-up example.\nEverything, including the 20-minute away, were what happened\nwhen I did the latest blame fix you saw a few days ago.\n\n * I am still futzing with blame from time to time, and have\n this change almost permanently in my working tree.\n\n\t$ cat P.diff\n diff --git a/builtin-blame.c b/builtin-blame.c\n index dc3ffea..46ce45c 100644\n --- a/builtin-blame.c\n +++ b/builtin-blame.c\n @@ -38,7 +38,7 @@ static int max_digits;\n static int max_score_digits;\n\n #ifndef DEBUG\n -#define DEBUG 0\n +#define DEBUG 1\n #endif\n\n * I also have the GIT-VERSION-GEN change in my working tree.\n\n> I want to fix another thing in pickaxe.\n>\n> \t$ edit builtin-blame.c\n>\n\n * Of course I did tests here.\n\n> My wife calls me. Away from desk for 20 minutes. Later I come\n> back.\n\n * And then reverted the DEBUG back to 0 in preparation for\n \"checking into the index\"\n\n\t$ edit builtin-blame.c\n\n> \t$ git update-index builtin-pickaxe.c\n\n * And then I reverted it back for later futing.\n\n \t$ git apply P.diff ;# that is a permanent-temporary file.\n\n>\n> I am so used to that name and did not realize that typo, and I\n> was not paying too much attention. My wife calls me again.\n> Away from desk and back in 20 minutes.\n>\n> \t$ git commit -m 'git-blame: Another fix.'\n>\n> Oops.\n\n * Oops here is not just that builtin-blame.c would have been\n committed; I'd almost never do \"commit -a\" in this repository,\n because it would take that \"DEBUG 1\" change _and_\n GIT-VERSION-GEN change into the commit.\n\n> So, please turn this \"cleverness\" off when the index does not\n> match the working tree.\n"},{"id":"295643","messageId":"200612011233.21210.andyparkins@gmail.com","threadId":"43278","inReplyTo":"7vd573amuy.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Make git-commit cleverer - have it figure out whether it needs -a automatically","fromName":"Andy Parkins","fromEmail":"andyparkins@gmail.com","sentAt":"2006-12-01T12:33:20Z","receivedAt":"2006-12-01T12:33:20Z","isPatch":true,"sender":{"key":"andyparkins@gmail.com","avatar":null},"body":"On Friday 2006 December 01 11:15, Junio C Hamano wrote:\n\n> I think another exception should be needed. If the index does\n> not match the working tree, it should not default to \"-a\".\n\nNo problem: just don't apply the patch :-) What you've asked for leaves it as \na no-op.\n\nThis patch activates \"-a\" when the index equals HEAD. i.e. git-commit would \ndo nothing in this situation. If it is disabled when the index doesn't match \nthe working tree, then we're back to \"do nothing\". i.e. HEAD==index==working \ntree.\n\n> So, please turn this \"cleverness\" off when the index does not\n> match the working tree.\n\nHow does that help you? You've updated the index manually, so the \nautomatic \"-a\" is already disabled. Without this patch you would still have \ncommitted the wrong thing.\n\n $ edit builtin-blame.c\n $ git update-index builtin-pickaxe.c\n $ git commit\n\nWhat is that you would like to have happened at this point?\n\n\nAndy\n-- \nDr Andy Parkins, M Eng (hons), MIEE\n"},{"id":"297090","messageId":"7vbqmmzplm.fsf@assigned-by-dhcp.cox.net","threadId":"43278","inReplyTo":"200612011106.42272.andyparkins@gmail.com","subject":"Re: [PATCH] Make git-commit cleverer - have it figure out whether it needs -a automatically","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-12-02T08:09:09Z","receivedAt":"2006-12-02T08:09:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andy Parkins writes:\n\n> Raimund Bauer offered this suggestion (paraphrased):\n>\n> \"Maybe we could do git-commit -a _only_ if the index matches HEAD, and\n> otherwise keep current behavior? So people who don't care about the\n> index won't get tripped up, and when you do have a dirty index, you get\n> told about it?\"\n>\n> Johannes Schindelin pointed out that this isn't the right thing to do for\n> an --amend, so that is checked for. Additionally, it's probably not the\n> right thing to do if any files are specified with \"--only\" or\n> \"--include\", so they turn this behaviour off as well.\n>\n> Nguyen Thai Ngoc Duy asked that git-commit let you know it's done this\n> by adding an extra comment to the commit message.\n>\n> Signed-off-by: Andy Parkins \n\nAside from the \"working tree not matching index\" safety valve I\nasked for, there is a more important case that \"when the index\nmatches HEAD\" is not a safe enough check for this 'cleverness'.\n\nIt's related to this code in git-commit:\n\n if [ \"$?\" != \"0\" -a ! -f \"$GIT_DIR/MERGE_HEAD\" -a -z \"$amend\" ]\n then\n rm -f \"$GIT_DIR/COMMIT_EDITMSG\" \"$GIT_DIR/SQUASH_MSG\"\n run_status\n exit 1\n fi\n\nThis allows you to make a new commit with an identical tree as\nthe current HEAD, when making a merge.\n\nWe originally did not allow a commit that has identical tree\nwith its first parent. It was pointed out as a bug in the real\nworld setting, where two bugs are independently fixed by two\npeople, textually and also logically differently, and the merge\nresults in a conflict. In such a case, after examining the text\n(git diff --cc) and motivation (git log --merge) of the changes\nthat conflicted from both ends, if you decide that you prefer\nyour own fix over the fix made by the other party, your edit of\nthe conflicted file would end up to be identical to what you had\noriginally in the HEAD.\n\nIn such a situation, it makes sense to record the fact that the\ntwo development paths came to the same conclusion as a merge.\nIt would also simplify later merges from the other side, and the\nmerges the other side attempts by pulling from you.\n\nSo after you fix the conflicts, you would mark the conflicted\nfile with Nico's shiny new \"git add\". The index _still_ matches\nHEAD. The 'cleverer' \"git commit\" would not notice this\nsituation and commits unrelated changes that the user does not\nhave any intention to commit in the working tree by mistake.\n\nSo that is _another_ exception you must handle.\n\nBut I think the problem with this 'cleverer' commit runs\ndeeper.\n\nNotice that you needed to say \"The main idea is this cleverness,\nbut foo pointed out this special case, and bar pointed out\nanother, and we fixed all of these known ones and now this is\ngood, let's apply it.\" in your proposed commit log message? You\nshould smell fishiness in that kind of reasoning.\n\nIt's not that you cannot be sure that these suggestions covered\nall cases; you should not have to cover everything under the sun\nin your initial revision. New exceptions can certainly be added\nincrementally to address breakages. However, having to have\nexception from the beginning is a sign that the general rule you\nstarted from is a broken rule. To realize that yourself, Think\nabout how you would describe the behaviour to a new user.\n\nThe currently proposed rewording of \"git add\" and \"git commit\"\nmanual pages would define \"git commit\" to behave in two primary\nmodes:\n\n 1. With paths, \"git commit \" means \"forget for a moment\n the changes I staged to be committed, and make a commit that\n includes only these paths (i.e. the new commit and the\n current HEAD are different at exactly these paths and\n nowhere else, and the new commit has contents from the\n working tree for these paths)\".\n\n 2. Without paths, \"git commit\" means \"make a commit out of\n everything I have told you to commit (aka 'staged') so far\".\n The primary ways to tell git to stage contents are \"git\n add/rm/mv\". But as a short-hand, you can say \"git commit\n -a\" to ask the command to place all the changes in the\n working tree in the changeset to be committed before making\n the new commit.\n\nHow would your version be described?\n\n 1. (ditto)\n\n 2. (ditto). However, there are number of exceptions you should\n be aware of:\n\n 2-a. The above general rule only applies if you have already\n told git to stage something for the next commit.\n\n 2-b. However, even if you had told git to stage new contents,\n if the new contents happen to be the same as HEAD, then\n the command grabs every change in the working tree and\n makes a commit out of it.\n\n 2-c. Contrary to the above exception, even if you staged the\n same contents as in HEAD, if you are in the middle of the\n merge, it does not include the working tree changes in the\n new commit. This is because....\n\n 2-d. Another exception is that if you are amending an existing\n commit, ... This is because....\n\nWith careful reading, the users may understand it after they\nread the above description, but would that understanding really\nbe committed to memory? Are you confident that you will\nremember your own reasoning for the exceptions and be prepared\nto defend this change (and perhaps follow-up additions to the\nlist of exceptions), if 6 months down the road when an old timer\nfrom the kernel circle says \"I updated to recent git, and its\n'commit' behaves funnily in the way I did not expect. Why does\nit do that\"?\n\nI really think the users would be much better off with\nconsistent behaviour that is easy and simple to describe than a\ncomplex magic that does the right thing 99.9% of the time,\nbecause you either understand the complex magic or constantly in\nfear of the tool that can work against you 0.01% of the time.\n"},{"id":"294551","messageId":"87slfy3a37.wl%cworth@cworth.org","threadId":"43278","inReplyTo":"7vbqmmzplm.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Make git-commit cleverer - have it figure out whether it needs -a automatically","fromName":"Carl Worth","fromEmail":"cworth@cworth.org","sentAt":"2006-12-02T09:45:16Z","receivedAt":"2006-12-02T09:45:16Z","isPatch":true,"sender":{"key":"cworth@cworth.org","avatar":"https://gravatar.com/avatar/3746dc28cde609bdbd7f939058356e7e2bbd16d21e32274df0725eb3d998bc5b?d=mp&s=160"},"body":"On Sat, 02 Dec 2006 00:09:09 -0800, Junio C Hamano wrote:\n> But I think the problem with this 'cleverer' commit runs\n> deeper.\n\nI agree. Being too clever is a problem.\n\nIt's very helpful to estimate usability and learnability by the length\nof prose needed to describe a command.\n\n> 1. With paths, \"git commit \" means \"forget for a moment\n> the changes I staged to be committed, and make a commit that\n> includes only these paths (i.e. the new commit and the\n> current HEAD are different at exactly these paths and\n> nowhere else, and the new commit has contents from the\n> working tree for these paths)\".\n>\n> 2. Without paths, \"git commit\" means \"make a commit out of\n> everything I have told you to commit (aka 'staged') so far\".\n> The primary ways to tell git to stage contents are \"git\n> add/rm/mv\". But as a short-hand, you can say \"git commit\n> -a\" to ask the command to place all the changes in the\n> working tree in the changeset to be committed before making\n> the new commit.\n\nJunio, thanks so much for these descriptions. They help ground the\ndiscussion quite nicely, (and will also contribute to improved\ndocumentation).\n\nHere's pseudo-code for the above descriptions:\n\n\tif (command-line has paths) {\n\t\tignore staging area, commit named files\n\telse {\n\t\tif (commit -a)\n\t\t\tupdate all files into staging area\n\t\tcommit staging area\n\t}\n\nOne problem I see in that is that the primary distinction is made\nbased on what appears on the command-line, rather than what job the\nuser is trying to perform. Also, \"commit -a\" is define in terms of the\nstaging area, even though the staging area is basically irrelevant to\nthis operation, (just as it is in the case of a commit with paths).\n\nSo I would re-factor that in a way that focuses on what the user is\ntrying to do:\n\n\tif (! doing a staged commit) {\n\t\tif (file list is empty)\n\t\t\tfile list = all tracked files\n\t\tcommit file list\n\t} else {\n\t\tcommit staging area\n\t}\n\nThis brings the description of \"commit -a\" and \"commit files...\"\ntogether, (which I think are conceptually more related than \"commit\n-a\" is to a commit of the staging area, (and yes, this ignores the\nhistory of the implementation). What we're talking about is how to\ndocument what the user wants to do, not how the implementation does\nit.\n\nNotice also that \"staging area\" never appears in the description of\nthe else clause, (which is good since the conceptual use of these\ncommands does not involve staging).\n\nSo translating my re-factored version back into prose, we might get:\n\n commit \n commit -a\n\n\tCommit the working-tree contents of the named , (or all\n\ttracked paths for -a). Files which no longer exist in the\n\tworking tree will be removed. New files to be tracked must be\n\tadded with \"git add\".\n\n commit\n\n\tCommit the content that exists in the staging area. The\n\tstaging area initially consists of the contents of the most\n\trecent commit, but can be modified with the \"git add\",\n\t\"git rm\", and \"git mv\".\n\nSo that's shorter. I think it's also more clear and focused on what\nthe user wants to do without being any less accurate.\n\nIt doesn't make it obvious that \"commit -a\" is the most common form\nand what users should look at first. So what I'd like to see is the\nsemantic changes that would allow us to document this as:\n\n commit\n commit \n\n\tCommit the working-tree contents of all tracked paths, (or\n\tjust the specific paths listed). Files which no longer exist\n\tin the working tree will be removed. New files to be tracked\n\tmust be added with \"git add\".\n\n commit --staged\n\n\tCommit the content that exists in the staging area. The\n\tstaging area initially consists of the contents of the most\n\trecent commit, but updated content from the working tree can\n\tbe placed into it with \"git stage \".\n\n-Carl\n"},{"id":"294206","messageId":"200612030957.03592.andyparkins@gmail.com","threadId":"43278","inReplyTo":"7vbqmmzplm.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Make git-commit cleverer - have it figure out whether it needs -a automatically","fromName":"Andy Parkins","fromEmail":"andyparkins@gmail.com","sentAt":"2006-12-03T09:57:02Z","receivedAt":"2006-12-03T09:57:02Z","isPatch":true,"sender":{"key":"andyparkins@gmail.com","avatar":null},"body":"On Saturday 2006, December 02 08:09, Junio C Hamano wrote:\n\n> Aside from the \"working tree not matching index\" safety valve I\n> asked for, there is a more important case that \"when the index\n> matches HEAD\" is not a safe enough check for this 'cleverness'.\n\nAs I pointed out, that safety valve made the whole thing a no-op.\n\n> So that is _another_ exception you must handle.\n>\n> But I think the problem with this 'cleverer' commit runs\n> deeper.\n>\n> Notice that you needed to say \"The main idea is this cleverness,\n> but foo pointed out this special case, and bar pointed out\n> another, and we fixed all of these known ones and now this is\n> good, let's apply it.\" in your proposed commit log message? You\n> should smell fishiness in that kind of reasoning.\n\nI don't believe so. In deep parts of a program cleverness is always a bad \nidea. It just obfuscates functionality. However, this \"cleverness\" is about \nmaking things easier for a human. Humans have a notoriously illogical set of \nrequirements for doing the right thing. As an example I'd hold up git's \nclever date specification code; a computer would be perfectly happy to accept \nepoch time, but instead humans like to be able to say \"three weeks ago this \nThursday at five to four\".\n\nThis patch is merely to make git-commit do something sensible when it would \nnormally do nothing. I was sure there would be more exceptions to when it \nshould activate; as git-commit is already a mess of \"useful\" extra switches. \nTo blame this patch for the fact that git-commit does a lot of things in \nslightly different ways hardly seems fair.\n\nAs usual though: I don't mind, I'm not some huge proponent of this \nfunctionality - /I/ get along fine with git-commit\n\n> I really think the users would be much better off with\n> consistent behaviour that is easy and simple to describe than a\n> complex magic that does the right thing 99.9% of the time,\n> because you either understand the complex magic or constantly in\n> fear of the tool that can work against you 0.01% of the time.\n\nI suspect that this patch is the least of git-commit's problems in that \ndepartment. \"Easy to describe\" is certainly not the case already, otherwise \nnone of this discussion (or patch) would ever have started.\n\n\nAndy\n-- \nDr Andrew Parkins, M Eng (Hons), AMIEE\n"}]}