{"thread":{"id":"22295","subject":"[PATCH] git status: do not require write permission","startedAt":"2010-01-20T00:06:17Z","lastAt":"2010-01-20T01:38:33Z","messageCount":8,"participants":["Johannes Schindelin","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"132149","messageId":"alpine.DEB.1.00.1001200105230.3164@intel-tinevez-2-302","threadId":"22295","inReplyTo":null,"subject":"[PATCH] git status: do not require write permission","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2010-01-20T00:06:17Z","receivedAt":"2010-01-20T00:06:17Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"\nToday, git status played violin on my nerves for the very last time.\nThere is no good reason, really none, for git status to require\nwrite permissions. If the index is not up-to-date, so be it, I\ncannot commit anyway.\n\nBut in most cases, the index _is_ up-to-date, and now I can tell\nmy fellow former users that their repository XYZ.git does not have any\nuncommitted changes, so can they please delete it to free up some disk\nspace, thank you very much.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin-commit.c | 12 +++++++-----\n 1 files changed, 7 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin-commit.c b/builtin-commit.c\nindex 55676fd..9eccc51 100644\n--- a/builtin-commit.c\n+++ b/builtin-commit.c\n@@ -325,11 +325,13 @@ static char *prepare_index(int argc, const char **argv, const char *prefix, int\n \t * We still need to refresh the index here.\n \t */\n \tif (!pathspec || !*pathspec) {\n-\t\tfd = hold_locked_index(&index_lock, 1);\n-\t\trefresh_cache_or_die(refresh_flags);\n-\t\tif (write_cache(fd, active_cache, active_nr) ||\n-\t\t commit_locked_index(&index_lock))\n-\t\t\tdie(\"unable to write new_index file\");\n+\t\tfd = hold_locked_index(&index_lock, 0);\n+\t\tif (fd >= 0) {\n+\t\t\trefresh_cache_or_die(refresh_flags);\n+\t\t\tif (write_cache(fd, active_cache, active_nr) ||\n+\t\t\t commit_locked_index(&index_lock))\n+\t\t\t\tdie(\"unable to write new_index file\");\n+\t\t}\n \t\tcommit_style = COMMIT_AS_IS;\n \t\treturn get_index_file();\n \t}\n-- \n1.6.4.297.gcb4cc\n"},{"id":"132150","messageId":"20100120002836.GA16824@coredump.intra.peff.net","threadId":"22295","inReplyTo":"alpine.DEB.1.00.1001200105230.3164@intel-tinevez-2-302","subject":"Re: [PATCH] git status: do not require write permission","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-01-20T00:28:36Z","receivedAt":"2010-01-20T00:28:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 20, 2010 at 01:06:17AM +0100, Johannes Schindelin wrote:\n\n> Today, git status played violin on my nerves for the very last time.\n> There is no good reason, really none, for git status to require\n> write permissions. If the index is not up-to-date, so be it, I\n> cannot commit anyway.\n\nI agree it is annoying, but this patch does not apply (and is no longer\nneeded) on master since the status-is-no-longer-commit-dry-run series\nhas been merged.\n\nI don't know if it is worth putting your fix onto maint.\n\n-Peff\n"},{"id":"132151","messageId":"alpine.DEB.1.00.1001200134380.3164@intel-tinevez-2-302","threadId":"22295","inReplyTo":"20100120002836.GA16824@coredump.intra.peff.net","subject":"Re: [PATCH] git status: do not require write permission","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2010-01-20T00:35:41Z","receivedAt":"2010-01-20T00:35:41Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 19 Jan 2010, Jeff King wrote:\n\n> On Wed, Jan 20, 2010 at 01:06:17AM +0100, Johannes Schindelin wrote:\n> \n> > Today, git status played violin on my nerves for the very last time.\n> > There is no good reason, really none, for git status to require\n> > write permissions. If the index is not up-to-date, so be it, I\n> > cannot commit anyway.\n> \n> I agree it is annoying, but this patch does not apply (and is no longer\n> needed) on master since the status-is-no-longer-commit-dry-run series\n> has been merged.\n\nI was working on top of 'next'. And only after I fetched afresh from \ngit://repo.or.cz/git.git/.\n\nSo for me and my nerves, the patch series came way too late.\n\nCiao,\nDscho\n"},{"id":"132152","messageId":"alpine.DEB.1.00.1001200138030.3164@intel-tinevez-2-302","threadId":"22295","inReplyTo":"alpine.DEB.1.00.1001200134380.3164@intel-tinevez-2-302","subject":"Re: [PATCH] git status: do not require write permission","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2010-01-20T00:38:57Z","receivedAt":"2010-01-20T00:38:57Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 20 Jan 2010, Johannes Schindelin wrote:\n\n> On Tue, 19 Jan 2010, Jeff King wrote:\n> \n> > On Wed, Jan 20, 2010 at 01:06:17AM +0100, Johannes Schindelin wrote:\n> > \n> > > Today, git status played violin on my nerves for the very last time.\n> > > There is no good reason, really none, for git status to require\n> > > write permissions. If the index is not up-to-date, so be it, I\n> > > cannot commit anyway.\n> > \n> > I agree it is annoying, but this patch does not apply (and is no longer\n> > needed) on master since the status-is-no-longer-commit-dry-run series\n> > has been merged.\n> \n> I was working on top of 'next'. And only after I fetched afresh from \n> git://repo.or.cz/git.git/.\n\nTo be precise, the commit I based mine on is \n2cd0ddca4b6f2ff24fcd7b24030c97ec20ead85c which was done on Mon Jan 18 \n18:34:16 2010 -0800 according to Git.\n\nCiao,\nDscho \"who will think 3 times before making a patch next time\"\n"},{"id":"132153","messageId":"7vhbqh7gpa.fsf@alter.siamese.dyndns.org","threadId":"22295","inReplyTo":"20100120002836.GA16824@coredump.intra.peff.net","subject":"Re: [PATCH] git status: do not require write permission","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-20T00:39:13Z","receivedAt":"2010-01-20T00:39:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Jan 20, 2010 at 01:06:17AM +0100, Johannes Schindelin wrote:\n>\n>> Today, git status played violin on my nerves for the very last time.\n>> There is no good reason, really none, for git status to require\n>> write permissions. If the index is not up-to-date, so be it, I\n>> cannot commit anyway.\n>\n> I agree it is annoying, but this patch does not apply (and is no longer\n> needed) on master since the status-is-no-longer-commit-dry-run series\n> has been merged.\n>\n> I don't know if it is worth putting your fix onto maint.\n\nI think the patch itself makes sense, even though the log message seems to\nbe with more noise and frustrationthan with useful information.\n\nAnd \"for the very last time\" is probably a good characterization, as\nstatus will no longer be \"commit --dry-run\" in the coming release ;-)\n"},{"id":"132156","messageId":"7v8wbt7far.fsf@alter.siamese.dyndns.org","threadId":"22295","inReplyTo":"7vhbqh7gpa.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] git status: do not require write permission","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-20T01:09:32Z","receivedAt":"2010-01-20T01:09:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jeff King writes:\n>\n>> On Wed, Jan 20, 2010 at 01:06:17AM +0100, Johannes Schindelin wrote:\n>>\n>>> Today, git status played violin on my nerves for the very last time.\n>>> There is no good reason, really none, for git status to require\n>>> write permissions. If the index is not up-to-date, so be it, I\n>>> cannot commit anyway.\n>>\n>> I agree it is annoying, but this patch does not apply (and is no longer\n>> needed) on master since the status-is-no-longer-commit-dry-run series\n>> has been merged.\n>>\n>> I don't know if it is worth putting your fix onto maint.\n>\n> I think the patch itself makes sense,...\n\nActually, I think it was somewhat a selfish patch and forgot that the\ncodepath is shared with a mode of operation where we should guarantee\nthat the index is updated, namely \"git commit\".\n\nI think this would be a good addition to 'maint'. I am not sure if it is\nworth forward-porting to \"commit --dry-run\", though. On one hand, it\nmight show what would happen if you ran \"commit\" better (i.e. you will be\ntold that you cannot write into it), but it is debatable if that is the\nreason why people may want to run \"commit --dry-run\".\n\n-- >8 --\nSubject: status: don't require the repository to be writable\n\nWe need to update the index before hooks run when actually making a\ncommit, but we shouldn't have to write the index when running \"status\".\nIf we can, then we have already spent cycles to refresh the index and\nit is a waste not to write it out, but it is not a disaster if we cannot\nwrite it out. The main reason the user is running \"git status\" is to get\nthe \"status\", and refreshing the index is a mere side effect that we can\ndo without.\n\nDiscovery and initial attempted fix by Dscho.\n\n---\n\n builtin-commit.c | 10 ++++++----\n 1 files changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin-commit.c b/builtin-commit.c\nindex 33aa593..83b7c35 100644\n--- a/builtin-commit.c\n+++ b/builtin-commit.c\n@@ -278,11 +278,13 @@ static char *prepare_index(int argc, const char **argv, const char *prefix, int\n \t * We still need to refresh the index here.\n \t */\n \tif (!pathspec || !*pathspec) {\n-\t\tfd = hold_locked_index(&index_lock, 1);\n+\t\tfd = hold_locked_index(&index_lock, !is_status);\n \t\trefresh_cache(refresh_flags);\n-\t\tif (write_cache(fd, active_cache, active_nr) ||\n-\t\t commit_locked_index(&index_lock))\n-\t\t\tdie(\"unable to write new_index file\");\n+\t\tif (0 <= fd) {\n+\t\t\tif (write_cache(fd, active_cache, active_nr) ||\n+\t\t\t commit_locked_index(&index_lock))\n+\t\t\t\tdie(\"unable to write new_index file\");\n+\t\t}\n \t\tcommit_style = COMMIT_AS_IS;\n \t\treturn get_index_file();\n \t}\n"},{"id":"132158","messageId":"alpine.DEB.1.00.1001200220570.4985@pacific.mpi-cbg.de","threadId":"22295","inReplyTo":"7vhbqh7gpa.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] git status: do not require write permission","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2010-01-20T01:23:44Z","receivedAt":"2010-01-20T01:23:44Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 19 Jan 2010, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > On Wed, Jan 20, 2010 at 01:06:17AM +0100, Johannes Schindelin wrote:\n> >\n> >> Today, git status played violin on my nerves for the very last time.\n> >> There is no good reason, really none, for git status to require\n> >> write permissions. If the index is not up-to-date, so be it, I\n> >> cannot commit anyway.\n> >\n> > I agree it is annoying, but this patch does not apply (and is no longer\n> > needed) on master since the status-is-no-longer-commit-dry-run series\n> > has been merged.\n> >\n> > I don't know if it is worth putting your fix onto maint.\n> \n> I think the patch itself makes sense, even though the log message seems to\n> be with more noise and frustrationthan with useful information.\n\nIf you think so.\n\n> And \"for the very last time\" is probably a good characterization, as\n> status will no longer be \"commit --dry-run\" in the coming release ;-)\n\nThis comment was because I was fully prepared to do the same as with so \nmany patches: keep them in my personal repository until time finally tells \nthat that patch should go into 'next'.\n\nAnd as you realized, I managed to forget a commit --amend, which would \nhave replaced 1 with !is_status.\n\nBTW you can tell me directly that you do not want any patches from me \nanymore, rather than dancing around the issue. I kind of figured that \nalready anyway.\n\nCiao,\nDscho\n"},{"id":"132160","messageId":"7v4omh7dye.fsf@alter.siamese.dyndns.org","threadId":"22295","inReplyTo":"alpine.DEB.1.00.1001200220570.4985@pacific.mpi-cbg.de","subject":"Re: [PATCH] git status: do not require write permission","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-20T01:38:33Z","receivedAt":"2010-01-20T01:38:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> BTW you can tell me directly that you do not want any patches from me \n> anymore, rather than dancing around the issue. I kind of figured that \n> already anyway.\n\nOh, what gave you that impression?\n\nI don't like getting patches that go only halfway, making things work\nbetter for some people but at the same time breaking things for others,\nwithout following thorough for a long time, and that is regardless of\nwhere the patches came from.\n\nThey linger on 'pu' forever and forces me to choose one of the three\noutcome:\n\n (1) discard it, even if it is promising;\n\n (2) make myself a janitor and filling the gap;\n\n (3) keep it on 'pu' even longer, while occassionally suffering merge\n conflicts with topics that touch the same textual area from more\n responsive people.\n\nI try to avoid (1) but I obviously don't want to do (2), especially when\nI know the topic originally came from a capable hand.\n\nIf anything, the more promising the patch series that the original author\ndidn't follow through, the worse it will make me feel to discard it. If\nyou feel that I get grumpy about many of your patches not followed through\nthan about other people's patches, perhaps it is a sign that I consider\nthat the aim (not necessarily the execution) of your topics are better\nthan the topics from others.\n\nSo don't take it personally.\n"}]}