{"thread":{"id":"8540","subject":"[Qgit RFC] commit --amend","startedAt":"2007-06-10T15:08:39Z","lastAt":"2007-07-08T13:49:27Z","messageCount":17,"participants":["Jan Hudec","Marco Costalba","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"44617","messageId":"20070610150839.GG4084@efreet.light.src","threadId":"8540","inReplyTo":null,"subject":"[Qgit RFC] commit --amend","fromName":"Jan Hudec","fromEmail":"bulb@ucw.cz","sentAt":"2007-06-10T15:08:39Z","receivedAt":"2007-06-10T15:08:39Z","isPatch":false,"sender":{"key":"bulb@ucw.cz","avatar":null},"body":"Hello,\n\nI am thinking about adding commit --amend support to Qgit.\n\nHowever I see three ways to do it, so I'd like to ask whether anybody knows\nsome reason why any of them is better or worse, which I don't:\n\n - Add a button for \"Amend\". This would fit with what is there currently,\n because with stgit it has buttons \"New patch\" and \"Add to top\" (refresh),\n which correspond exactly to core git's \"commit\" and \"commit --amend\"\n respectively.\n\n Disadvantage (for current approach too) is, that when amending (resp.\n refreshing) the previous commit message is not there to be edited.\n\n - Add a radio button the way git-gui does. Switching the button to \"amend\"\n would load the previous commit message.\n\n This would be more invasive change, but it would allow editing the commit\n message when amending/refreshing. I fear it will be more user-error-prone\n too, though.\n\n - Add a separate action to the menu. This action would take over the refresh\n (Add to top) operation when using stgit.\n \n I believe this has lower risk of user errors than the previous option. It\n also has the advantage, that I don't have to touch the disabling logic for\n the commit action. Amending last commit is always possible, even if there\n are no changes, because you might want to edit the message (eg. if you\n forget to sign-off or forget to mention some change or something).\n\nI'll try doing the first option now, unless somebody persuades me that it's\na nonsense.\n\n-- \n\t\t\t\t\t\t Jan 'Bulb' Hudec \n"},{"id":"44651","messageId":"e5bfff550706101510x6d685944ja70c9d9dbb3668f6@mail.gmail.com","threadId":"8540","inReplyTo":"20070610150839.GG4084@efreet.light.src","subject":"Re: [Qgit RFC] commit --amend","fromName":"Marco Costalba","fromEmail":"mcostalba@gmail.com","sentAt":"2007-06-10T22:10:20Z","receivedAt":"2007-06-10T22:10:20Z","isPatch":false,"sender":{"key":"mcostalba@gmail.com","avatar":null},"body":"On 6/10/07, Jan Hudec wrote:\n> Hello,\n>\n> I am thinking about adding commit --amend support to Qgit.\n>\n\nGood!\n\n> However I see three ways to do it, so I'd like to ask whether anybody knows\n> some reason why any of them is better or worse, which I don't:\n>\n> - Add a button for \"Amend\". This would fit with what is there currently,\n> because with stgit it has buttons \"New patch\" and \"Add to top\" (refresh),\n> which correspond exactly to core git's \"commit\" and \"commit --amend\"\n> respectively.\n>\n\n>From 'Edit->commit' menu it is possible to open a dialog to commit\nmodified files in working dir.\n\nCurrently, at the bottom of the dialog there are 4 buttons:\n\n- Settings (to open settings dialog on 'commit options' tab)\n\n- Cancel (to cancel ;-) )\n\n- Sync (to update the index but without writing a new tree)\n\n- Commit (to commit a new revision)\n\nIn case the repository is under a StGIT stack the buttons are slightly\ndifferent, in particular:\n\nSync --> becames 'Add to top' (to fold and refresh changes)\n\nCommit --> becames 'New patch' (to create a new patch with selected\nmodified files)\n\n\nIf I have understood correctly you plan to substitute 'Sync' button\nwith 'Amend'. Is it correct?\n\n> Disadvantage (for current approach too) is, that when amending (resp.\n> refreshing) the previous commit message is not there to be edited.\n>\n\nYes but see below.\n\n> - Add a radio button the way git-gui does. Switching the button to \"amend\"\n> would load the previous commit message.\n>\n> This would be more invasive change, but it would allow editing the commit\n> message when amending/refreshing. I fear it will be more user-error-prone\n> too, though.\n>\n\nI agree.\n\n> - Add a separate action to the menu. This action would take over the refresh\n> (Add to top) operation when using stgit.\n>\n> I believe this has lower risk of user errors than the previous option. It\n> also has the advantage, that I don't have to touch the disabling logic for\n> the commit action. Amending last commit is always possible, even if there\n> are no changes, because you might want to edit the message (eg. if you\n> forget to sign-off or forget to mention some change or something).\n>\n\nYes. But amending is an option of commit (also in git) so probably the\namend action will fire the commit dialog anyway and we are back to\nprevious situation. The only advantage is that we can load the message\nof the tip revision as default instead of git-status output as the\ncurrent.\n\n\n> I'll try doing the first option now, unless somebody persuades me that it's\n> a nonsense.\n>\n\nI think it's the best: 'Sync' button is very seldom used (I think I've\nnever used it but for testing that it works) and updating the index is\nsomething very plumbing anyway.\n\nWhat we could add is another button 'Load prev msg' to do what it\nsays, so we would end up with 5 buttons:\n\n- Settings /Cancel/ Load prev msg / Amend /Commit\n\nI don't see a reason to set 'Load prev msg' as a check button, you may\nwant to reload the prev msg as many times as you need, (re)clicking\neverytime on the button, also for a normal commit where as example you\nwant to keep the header or part of the subject of the previous\nrevision.\n\nPlease let me know if and where you find something obscure/messy with\nthe code, I will be happy to help you.\n\n\nMarco\n"},{"id":"44689","messageId":"20070611044258.GJ4084@efreet.light.src","threadId":"8540","inReplyTo":"e5bfff550706101510x6d685944ja70c9d9dbb3668f6@mail.gmail.com","subject":"Re: [Qgit RFC] commit --amend","fromName":"Jan Hudec","fromEmail":"bulb@ucw.cz","sentAt":"2007-06-11T04:42:58Z","receivedAt":"2007-06-11T04:42:58Z","isPatch":false,"sender":{"key":"bulb@ucw.cz","avatar":null},"body":"On Mon, Jun 11, 2007 at 00:10:20 +0200, Marco Costalba wrote:\n> On 6/10/07, Jan Hudec wrote:\n> >Hello,\n> >\n> >I am thinking about adding commit --amend support to Qgit.\n> >\n> \n> Good!\n> \n> > - Add a separate action to the menu. This action would take over the \n> > refresh\n> > (Add to top) operation when using stgit.\n> >\n> > I believe this has lower risk of user errors than the previous option. \n> > It\n> > also has the advantage, that I don't have to touch the disabling logic \n> > for\n> > the commit action. Amending last commit is always possible, even if \n> > there\n> > are no changes, because you might want to edit the message (eg. if you\n> > forget to sign-off or forget to mention some change or something).\n> >\n> \n> Yes. But amending is an option of commit (also in git) so probably the\n> amend action will fire the commit dialog anyway and we are back to\n> previous situation. The only advantage is that we can load the message\n> of the tip revision as default instead of git-status output as the\n> current.\n> \n> \n> >I'll try doing the first option now, unless somebody persuades me that it's\n> >a nonsense.\n> >\n> \n> I think it's the best: 'Sync' button is very seldom used (I think I've\n> never used it but for testing that it works) and updating the index is\n> something very plumbing anyway.\n> \n> What we could add is another button 'Load prev msg' to do what it\n> says, so we would end up with 5 buttons:\n> \n> - Settings /Cancel/ Load prev msg / Amend /Commit\n> \n> I don't see a reason to set 'Load prev msg' as a check button, you may\n> want to reload the prev msg as many times as you need, (re)clicking\n> everytime on the button, also for a normal commit where as example you\n> want to keep the header or part of the subject of the previous\n> revision.\n\nI think it would be somewhat complicated unfortunately :-(. The loading of\nprevious message should be really tied to whether you are about to\namend/refresh or to commit/add new. In this respect this would actually match\nthe command-line interface, because you say whether you want to amend\n*before* you edit the message.\n\nSo I am now inclined more to the separate action and trying to go that way.\nBut the difference between the variants would not be that big.\n\n> Please let me know if and where you find something obscure/messy with\n> the code, I will be happy to help you.\n\nI think I mostly understood it now. Thank you.\n\n-- \n\t\t\t\t\t\t Jan 'Bulb' Hudec \n"},{"id":"44691","messageId":"e5bfff550706102224k5fd32cf9x5e449f9bed708585@mail.gmail.com","threadId":"8540","inReplyTo":"20070611044258.GJ4084@efreet.light.src","subject":"Re: [Qgit RFC] commit --amend","fromName":"Marco Costalba","fromEmail":"mcostalba@gmail.com","sentAt":"2007-06-11T05:24:00Z","receivedAt":"2007-06-11T05:24:00Z","isPatch":false,"sender":{"key":"mcostalba@gmail.com","avatar":null},"body":"On 6/11/07, Jan Hudec wrote:\n>\n> So I am now inclined more to the separate action and trying to go that way.\n> But the difference between the variants would not be that big.\n>\n\nOk for me. Please, feel free to implement what's the best for you and\nI will apply the patch.\n\nMarco\n"},{"id":"44692","messageId":"e5bfff550706102245p27aea579w65ee96161630a624@mail.gmail.com","threadId":"8540","inReplyTo":"20070611044258.GJ4084@efreet.light.src","subject":"Re: [Qgit RFC] commit --amend","fromName":"Marco Costalba","fromEmail":"mcostalba@gmail.com","sentAt":"2007-06-11T05:45:51Z","receivedAt":"2007-06-11T05:45:51Z","isPatch":false,"sender":{"key":"mcostalba@gmail.com","avatar":null},"body":"On 6/11/07, Jan Hudec wrote:\n>\n> I think I mostly understood it now. Thank you.\n>\n\nAnyhow I think this could be useful to you:\n\n\t/*\n\t getAllRefSha() returns the list of sha of a given\n\t type, where type is a mask of Git::RefType flags\n\t see src/git.h.\n\t In this case we want the sha of the current branch\n\t*/\n\tQStringList revs = getAllRefSha(CUR_BRANCH);\n\t\n\tif (!revs.isEmpty()) {\n\n\t\t// all the sha info is stored in this QGit::Rev\n\t\t// class defined in src/common.h\n\t\tconst Rev* r = revLookup(revs.first());\n\n\t\t// short log (title) is\n\t\tr->shortLog();\n\n\t\t// message body is\n\t\tr->longLog();\n\n\t\t// etc....\n\t}\n"},{"id":"46162","messageId":"20070701122625.GC26243@efreet.light.src","threadId":"8540","inReplyTo":"e5bfff550706102245p27aea579w65ee96161630a624@mail.gmail.com","subject":"Re: [Qgit RFC] commit --amend","fromName":"Jan Hudec","fromEmail":"bulb@ucw.cz","sentAt":"2007-07-01T12:26:25Z","receivedAt":"2007-07-01T12:26:25Z","isPatch":false,"sender":{"key":"bulb@ucw.cz","avatar":null},"body":"On Mon, Jun 11, 2007 at 07:45:51 +0200, Marco Costalba wrote:\n> On 6/11/07, Jan Hudec wrote:\n> >\n> >I think I mostly understood it now. Thank you.\n> >\n> \n> Anyhow I think this could be useful to you:\n> \n> \t/*\n> \t getAllRefSha() returns the list of sha of a given\n> \t type, where type is a mask of Git::RefType flags\n> \t see src/git.h.\n> \t In this case we want the sha of the current branch\n> \t*/\n> \tQStringList revs = getAllRefSha(CUR_BRANCH);\n> \t\n> \tif (!revs.isEmpty()) {\n> \n> \t\t// all the sha info is stored in this QGit::Rev\n> \t\t// class defined in src/common.h\n> \t\tconst Rev* r = revLookup(revs.first());\n> \n> \t\t// short log (title) is\n> \t\tr->shortLog();\n> \n> \t\t// message body is\n> \t\tr->longLog();\n> \n> \t\t// etc....\n> \t}\n\nThanks.\n\nI got stuck at git-commit --amend -F not working (because it's explicitely\nforbidden), so I have to reimplement all of the commit stuff with plumbing\ncommands. I am in the middle of rewriting it currently. Unfortunately the\nplumbing is a little too low-level.\n\nHowever, I am currently not sure how to handle errors. If the current commit\nfails, it will show a message box with it's output, but I can't see where it\nis generated. It seems it's somewhere inside MyProcess, so I don't have to do\nanything special though, right?\n\n-- \n\t\t\t\t\t\t Jan 'Bulb' Hudec \n"},{"id":"46178","messageId":"e5bfff550707010909p4eba184ekff2025fb158a4aee@mail.gmail.com","threadId":"8540","inReplyTo":"20070701122625.GC26243@efreet.light.src","subject":"Re: [Qgit RFC] commit --amend","fromName":"Marco Costalba","fromEmail":"mcostalba@gmail.com","sentAt":"2007-07-01T16:09:37Z","receivedAt":"2007-07-01T16:09:37Z","isPatch":false,"sender":{"key":"mcostalba@gmail.com","avatar":null},"body":"On 7/1/07, Jan Hudec wrote:\n> On Mon, Jun 11, 2007 at 07:45:51 +0200, Marco Costalba wrote:\n> > On 6/11/07, Jan Hudec wrote:\n> > >\n>\n> However, I am currently not sure how to handle errors. If the current commit\n> fails, it will show a message box with it's output, but I can't see where it\n> is generated. It seems it's somewhere inside MyProcess, so I don't have to do\n> anything special though, right?\n>\n\nYou have 2 ways to start a git command:\n\nQGit::run() and QGit::runAsync()\n\nThe first starts the command and wait for completion, a bool is\nreturned to indicate success as example\n\nif (!run(\"git read-tree --reset HEAD\"))\n\treturn false;\n\nThe second one is use to start a command and return immediately, so\nit's used for long commands that should be non-blocking, as example:\n\nif (!runAsync(\"git diff-index -r -m --patch-with-stat HEAD\"))\n return false;\n\nIn the latter case success it means the command has been _started_ successfully.\n\n\nA bool is the only flag returned, because error detect is done at\nlower level, in MyProcess::sendErrorMsg() in file myprocess.cpp that\nhandles the low level of running an external process and is called by\nrun().\n\nIn case of an error MyProcess::sendErrorMsg() is called to inform the\nGUI (a popup dialog box) about the error and the stderr output\nreceived.\n\nWhat is an error ? :-)\n\nIt's not so trivial due to different OS and git commands behaviours\nregarding stderr and exiting codes, check the comment at the beginning\nof MyProcess::on_finished() in myprocess.cpp to see *how* qgit detects\nan error has occurred and informs upstream.\n\n\nSo the bottom line is: no, you don't have to do anything special. The\nreturned 'false' value from run() call is for your use only, if\nneeded, you don't have to propagate upstream to let user be informed.\n\nHope this helps.\n\nMarco\n\n\nP.S: Why 'git-commit --amend -F' it's explicitely forbidden?\n"},{"id":"46262","messageId":"20070702180309.GA4400@efreet.light.src","threadId":"8540","inReplyTo":"e5bfff550707010909p4eba184ekff2025fb158a4aee@mail.gmail.com","subject":"Re: [Qgit RFC] commit --amend","fromName":"Jan Hudec","fromEmail":"bulb@ucw.cz","sentAt":"2007-07-02T18:03:09Z","receivedAt":"2007-07-02T18:03:09Z","isPatch":false,"sender":{"key":"bulb@ucw.cz","avatar":null},"body":"On Sun, Jul 01, 2007 at 18:09:37 +0200, Marco Costalba wrote:\n> [...]\n> QGit::run() and QGit::runAsync()\n> [...]\n>\n> A bool is the only flag returned, because error detect is done at\n> lower level, in MyProcess::sendErrorMsg() in file myprocess.cpp that\n> handles the low level of running an external process and is called by\n> run().\n>\n> In case of an error MyProcess::sendErrorMsg() is called to inform the\n> GUI (a popup dialog box) about the error and the stderr output\n> received.\n\nThanks. That's the bit I didn't see in MyProcess. Ok, so I'll simply return\nfrom the function when some of the run()s return false.\n\n> [...]\n> Hope this helps.\n\nThanks.\n\n> P.S: Why 'git-commit --amend -F' it's explicitely forbidden?\n\nThe logic for preparing the commit message seems to be incompatible in\ngit-commit.sh for some reason and whoever wrote it fobode those flags\ntogether instead of making it compatible. In any case, the error is reported\nin git-commit.sh:291-296:\n\ncase \"$log_given\" in\ntt*)\n\tdie \"Only one of -c/-C/-F/--amend can be used.\" ;;\n*tm*|*mt*)\n\tdie \"Option -m cannot be combined with -c/-C/-F/--amend.\" ;;\nesac\n\nAnd in any case the git policy is, that frontends should not be using\nporcelain commands. What I would really like to see is something between\ngit-commit and git-commit-tree, that would:\n - run pre-commit hook\n - write-tree\n - commit-tree\n - update-ref\n - rerere\n - run post-commit hook\nBut it does not exist and git-gui obviously does that manually, so I did it\nmanually as well. Now I have to test it.\n\nThe other thing that didn't work (or at least from reading the code it seems\nit couldn't) is commiting merges, because when commiting merge, filename\narguments are forbidden on git-commit.\n\nCommiting merges will require some special handling, because you have to at\nleast commit all files that were merged. And you can't re-create pristine\nindex with read-tree the way normal commit does it either.\n\nUnfortunately I don't fully understand -- and remember the right commands for\nworking with it -- the multi-stage index.\n\n-- \n\t\t\t\t\t\t Jan 'Bulb' Hudec \n"},{"id":"46438","messageId":"7vy7hwlpo4.fsf@assigned-by-dhcp.cox.net","threadId":"8540","inReplyTo":"20070702180309.GA4400@efreet.light.src","subject":"Re: [Qgit RFC] commit --amend","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-07-04T05:10:35Z","receivedAt":"2007-07-04T05:10:35Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jan Hudec writes:\n\n>> P.S: Why 'git-commit --amend -F' it's explicitely forbidden?\n\nThe reasoning goes like this (here, I am not particularly trying\nto justify it, but am merely explaining the original reasoning\nand intended use case as a historical background):\n\n To amend means to remove the tip commit, replace with a new\n one, possibly but not necessarily with a different tree from\n the removed one.\n\n Since you are \"amending\", the spirit of the commit you are\n going to create, in order to replace the old one, ought to\n be the same as the one being replaced.\n\n - You may be only adding a change that you forgot to add\n before making the previous commit (in which case your tree\n is slightly different, but what you are going to say in\n the commit log message is exactly the same), or\n\n - you may found a typo in the commit log message and trying\n to fix it (in which case your tree is identical but the\n commit log message would be slightly different).\n\n In either case, the resulting commit log message would be\n very similar to the existing one, so the tool helps you by\n letting re-use and re-edit the commit log message instead of\n forcing you to re-type it.\n\nThere is no room for -F, -c, nor -m to make sense for these use\ncases, and giving them to \"commit --amend\" is most likely a user\nerror, and diagnozed as such, because \"commit --amend\" is an\nend-user level Porcelain program.\n\nIf you are popping one commit and replacing with a totally\n**unrelated** commit, that is not what --amend is about. What\nyou are doing is \"reset --soft HEAD^\" followed by \"add \"\nfollowed by \"commit\".\n\nAt the mechanical level, you could argue that --amend is doing\nthe same thing. After all, that reset/add/commit sequence is\nexactly what is done by --amend internally.\n\nBut if a Porcelain like StGIT or Qgit would want to do that kind\nof operation for different use case than \"amending\", it can and\nshould use plumbing commands, just like the implementation of\n\"commit --amend\" does, with different constraints and error\nchecks.\n"},{"id":"46474","messageId":"e5bfff550707040544l6272bdeao3a891c1793d29eae@mail.gmail.com","threadId":"8540","inReplyTo":"7vy7hwlpo4.fsf@assigned-by-dhcp.cox.net","subject":"Re: [Qgit RFC] commit --amend","fromName":"Marco Costalba","fromEmail":"mcostalba@gmail.com","sentAt":"2007-07-04T12:44:16Z","receivedAt":"2007-07-04T12:44:16Z","isPatch":false,"sender":{"key":"mcostalba@gmail.com","avatar":null},"body":"On 7/4/07, Junio C Hamano wrote:\n> Jan Hudec writes:\n>\n> >> P.S: Why 'git-commit --amend -F' it's explicitely forbidden?\n>\n> The reasoning goes like this (here, I am not particularly trying\n> to justify it, but am merely explaining the original reasoning\n> and intended use case as a historical background):\n>\n\nThanks for your explanation, of course ;-) I think you will agree that...\n\n>\n> There is no room for -F, -c, nor -m to make sense for these use\n> cases, and giving them to \"commit --amend\" is most likely a user\n> error, and diagnozed as such, because \"commit --amend\" is an\n> end-user level Porcelain program.\n>\n\nWhy an 'end-user' should _erroneusly_ write '-F' option to 'git commit\n--amend' ?\n\nAn error IMHO could occur if user *forgets* to write something, not if\nhe intentionally writes a very specific '-F' option, in that case I\nwould say user knows what it writes.\n\nAnd speaking about errors one can always write 'git reset HEAD^' with\nworst results.\n\nProbably you agree it's very _artificial_ try to guess what is in the\nhead of the user and what is not, especially if this guess is made by\na tool.\n\nSo I would dare to say this could be a good occasion to remove that\nillusory and obscure check.\n\n> But if a Porcelain like StGIT or Qgit would want to do that kind\n> of operation for different use case than \"amending\", it can and\n> should use plumbing commands, just like the implementation of\n> \"commit --amend\" does, with different constraints and error\n> checks.\n>\n\nI always prefer qgit to use the highest level commands as possible because:\n\n1- Less error prone\n2- Easier to implement\n3- More robust to API change\n4- Less easy to break by changes in git.\n\n\nHaving said that, from '-F' option documentation:\n\n-F ::\n\tTake the commit message from the given file. Use '-' to\n\tread the message from the standard input.\n\n\nJan, what about to use '-' and feed message from stdin?\n\nIndeed the full signature of run() is:\n\nbool Git::run(SCRef runCmd, QString* runOutput, QObject* receiver, SCRef buf)\n\nWhere the last parameter 'buf' it's a string that, if not empty, is\npassed to the launched program stdin.\n\nI don't know if it is already too late, but I would suggest to stick\nto git-commit if possible, I see only downsides in not doing so. But\nof course who writes the code decides.\n\nMarco\n"},{"id":"46503","messageId":"20070704182806.GA3268@efreet.light.src","threadId":"8540","inReplyTo":"e5bfff550707040544l6272bdeao3a891c1793d29eae@mail.gmail.com","subject":"Re: [Qgit RFC] commit --amend","fromName":"Jan Hudec","fromEmail":"bulb@ucw.cz","sentAt":"2007-07-04T18:28:06Z","receivedAt":"2007-07-04T18:28:06Z","isPatch":false,"sender":{"key":"bulb@ucw.cz","avatar":null},"body":"On Wed, Jul 04, 2007 at 14:44:16 +0200, Marco Costalba wrote:\n> On 7/4/07, Junio C Hamano wrote:\n>> But if a Porcelain like StGIT or Qgit would want to do that kind\n>> of operation for different use case than \"amending\", it can and\n>> should use plumbing commands, just like the implementation of\n>> \"commit --amend\" does, with different constraints and error\n>> checks.\n\nI would prefer if there was something between git-commit-tree and git-commit.\nThere are several steps one has to do for commit, that are the same for most\nways of commit:\n - call pre-commit hook (unless --no-verify)\n - write-tree\n - commit-tree\n - update-ref\n - mv next-index index\n - call post-commit hook (unless --no-verify or unconditionally?)\n\nWould factoring out such script from the end of git-commit.sh be accepted?\n\nOr would it be possible to get just that from git-commit with right options?\nBasically I need to replicate the logic with \n\nI would suggest a name git-commit-index. It would take the index to commit,\nindex to move in after commit, head to update, list of parents and commit\nmessage on standard input (as commit-tree does).\n\nThe other thing is, that of course qgit (or any other frontend) can't start\nusing it until it's accepted and released with git. So I'll first try to get\nit working in qgit and than think about making it a separate plumbing command\nin git.\n\n> I always prefer qgit to use the highest level commands as possible because:\n>\n> 1- Less error prone\n> 2- Easier to implement\n\nDefinitely.\n\n> 3- More robust to API change\n> 4- Less easy to break by changes in git.\n\nActually, no. The porcelains are more likely to change than the plumbing.\n\n> Having said that, from '-F' option documentation:\n>\n> -F ::\n> \tTake the commit message from the given file. Use '-' to\n> \tread the message from the standard input.\n>\n> Jan, what about to use '-' and feed message from stdin?\n\nI actually am, because I am rewriting it to use plumbing, which means\ngit-write-tree and git-commit-tree directly. And git-commit-tree always reads\ncommit message from stdin.\n\n> Indeed the full signature of run() is:\n>\n> bool Git::run(SCRef runCmd, QString* runOutput, QObject* receiver, SCRef \n> buf)\n>\n> Where the last parameter 'buf' it's a string that, if not empty, is\n> passed to the launched program stdin.\n\n... except if I read the code correctly, it will create a temporary file\nanyway. The comment in QGit::startProcess says it is because of windows, but\nthere is nothing to disable it in Unix, so to me it seems temporary file is\nused anyway.\n\n> I don't know if it is already too late, but I would suggest to stick\n> to git-commit if possible, I see only downsides in not doing so. But\n> of course who writes the code decides.\n\nThe old code needs rewriting in any case, because if I read it correctly, it\nwill not commit merges either. Yes, you rarely do it, because git\nautomatically commits non-conflicting merges, but amending such commits is\nmuch more common.\n\nI would personally most like qgit to do all playing with index on it's own\nand than call a single command to commit the index. But commit can't really\ndo that, because I can't give commit the parent list and I don't like the\nidea of reset --soft HEAD^ + reinstate whatever MERGE_HEAD there needs to be\n(to not clutter reflog and also to leave the tree is as sensible state as\npossible if something goes wrong).\n\n-- \n\t\t\t\t\t\t Jan 'Bulb' Hudec \n"},{"id":"46507","messageId":"7vir90gd67.fsf@assigned-by-dhcp.cox.net","threadId":"8540","inReplyTo":"20070704182806.GA3268@efreet.light.src","subject":"Re: [Qgit RFC] commit --amend","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-07-04T19:51:44Z","receivedAt":"2007-07-04T19:51:44Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jan Hudec writes:\n\n> On Wed, Jul 04, 2007 at 14:44:16 +0200, Marco Costalba wrote:\n>> On 7/4/07, Junio C Hamano wrote:\n>>> But if a Porcelain like StGIT or Qgit would want to do that kind\n>>> of operation for different use case than \"amending\", it can and\n>>> should use plumbing commands, just like the implementation of\n>>> \"commit --amend\" does, with different constraints and error\n>>> checks.\n>\n> I would prefer if there was something between git-commit-tree and git-commit.\n> There are several steps one has to do for commit, that are the same for most\n> ways of commit:\n> - call pre-commit hook (unless --no-verify)\n> - write-tree\n> - commit-tree\n> - update-ref\n> - mv next-index index\n> - call post-commit hook (unless --no-verify or unconditionally?)\n>\n> Would factoring out such script from the end of git-commit.sh be accepted?\n>\n> Or would it be possible to get just that from git-commit with right options?\n> Basically I need to replicate the logic with \n>\n> I would suggest a name git-commit-index. It would take the index to commit,\n> index to move in after commit, head to update, list of parents and commit\n> message on standard input (as commit-tree does).\n\nJudging from the existing users (am, commit, filter-branch, and\nmerge), I would say \"write-tree then commit-tree then\nupdate-ref\" sequence could be made to a new built-in plumbing if\nyou really wanted to. \"mv next-index index\" and calling of\nhooks definitely do _not_ belong there, though, since the hook\nthat needs to be called are different, and the caller of such a\nplumbing command is very well equipped to do them by themselves\nanyway.\n"},{"id":"46562","messageId":"20070705185426.GB3991@efreet.light.src","threadId":"8540","inReplyTo":"e5bfff550706102245p27aea579w65ee96161630a624@mail.gmail.com","subject":"Re: [Qgit RFC] commit --amend","fromName":"Jan Hudec","fromEmail":"bulb@ucw.cz","sentAt":"2007-07-05T18:54:26Z","receivedAt":"2007-07-05T18:54:26Z","isPatch":false,"sender":{"key":"bulb@ucw.cz","avatar":null},"body":"Hello,\n\nOn Mon, Jun 11, 2007 at 07:45:51 +0200, Marco Costalba wrote:\n> On 6/11/07, Jan Hudec wrote:\n>> I think I mostly understood it now. Thank you.\n>\n> Anyhow I think this could be useful to you:\n>\n> \t/*\n> \t getAllRefSha() returns the list of sha of a given\n> \t type, where type is a mask of Git::RefType flags\n> \t see src/git.h.\n> \t In this case we want the sha of the current branch\n> \t*/\n> \tQStringList revs = getAllRefSha(CUR_BRANCH);\n> \t\n> \tif (!revs.isEmpty()) {\n>\n> \t\t// all the sha info is stored in this QGit::Rev\n> \t\t// class defined in src/common.h\n> \t\tconst Rev* r = revLookup(revs.first());\n>\n> \t\t// short log (title) is\n> \t\tr->shortLog();\n>\n> \t\t// message body is\n> \t\tr->longLog();\n>\n> \t\t// etc....\n> \t}\n\nI thought I should be using something more explicit. Like getRefSha(\"HEAD\",\nANY_REF, false) -- only to find that it wouldn't work. That means that this\ncode would not work either. Why? Well, because HEAD does not have to be\na symbolic ref. If you check out anything else than branch (which you can),\nHEAD will be set to the SHA1 directly and if you commit in such state (which\nyou also can), the HEAD will be different from anything in refs/.\n\nTherefore I'll either have to always ask git via run(\"git-rev-parse HEAD\",\nhead), add HEAD into the map, or store HEAD somewhere in the Git object.\nWhich do you think makes most sense?\n\n(Note: Yes, I noted that getRefSha(\"HEAD\", ANY_REF, true) should work, but of\ncourse that is the run(\"git-rev-parse HEAD\") case.\n\n-- \n\t\t\t\t\t\t Jan 'Bulb' Hudec \n"},{"id":"46600","messageId":"e5bfff550707060054qdbe7e50ic217b8ed6927408e@mail.gmail.com","threadId":"8540","inReplyTo":"20070704182806.GA3268@efreet.light.src","subject":"Re: [Qgit RFC] commit --amend","fromName":"Marco Costalba","fromEmail":"mcostalba@gmail.com","sentAt":"2007-07-06T07:54:54Z","receivedAt":"2007-07-06T07:54:54Z","isPatch":false,"sender":{"key":"mcostalba@gmail.com","avatar":null},"body":"On 7/4/07, Jan Hudec wrote:\n>\n> > 3- More robust to API change\n> > 4- Less easy to break by changes in git.\n>\n> Actually, no. The porcelains are more likely to change than the plumbing.\n>\n\nWell, changing internal API does not break compatibility, _modifying_\nuser commands behaviour yes and you need to release a new version for\nthis.\n\nIf for \"change\" you mean adding stuff then, yes, I agree with you, but\nadding stuff is not a problem.\n\n> > Having said that, from '-F' option documentation:\n> >\n> > -F ::\n> > Take the commit message from the given file. Use '-' to\n> > read the message from the standard input.\n> >\n> > Jan, what about to use '-' and feed message from stdin?\n>\n> I actually am, because I am rewriting it to use plumbing, which means\n> git-write-tree and git-commit-tree directly. And git-commit-tree always reads\n> commit message from stdin.\n>\n> > Indeed the full signature of run() is:\n> >\n> > bool Git::run(SCRef runCmd, QString* runOutput, QObject* receiver, SCRef\n> > buf)\n> >\n> > Where the last parameter 'buf' it's a string that, if not empty, is\n> > passed to the launched program stdin.\n>\n> ... except if I read the code correctly, it will create a temporary file\n> anyway. The comment in QGit::startProcess says it is because of windows, but\n> there is nothing to disable it in Unix, so to me it seems temporary file is\n> used anyway.\n>\n\nYes you are right, but the file is redirected to process stdin by the call\n\nproc->setStandardInputFile()\n\njust below the comment you reported. I can assure you it works because\nto read file names it is used \"git diff-tree -r -C --stdin\" without\nproblems.\n\n\nSorry for mt late reply, but I'm abroad this week and can access the\nemail only seldom.\n\n\nMarco\n"},{"id":"46602","messageId":"e5bfff550707060112p29b9565bw9ccba6601745b850@mail.gmail.com","threadId":"8540","inReplyTo":"20070705185426.GB3991@efreet.light.src","subject":"Re: [Qgit RFC] commit --amend","fromName":"Marco Costalba","fromEmail":"mcostalba@gmail.com","sentAt":"2007-07-06T08:12:50Z","receivedAt":"2007-07-06T08:12:50Z","isPatch":false,"sender":{"key":"mcostalba@gmail.com","avatar":null},"body":"On 7/5/07, Jan Hudec wrote:\n\n>\n> Therefore I'll either have to always ask git via run(\"git-rev-parse HEAD\",\n> head), add HEAD into the map, or store HEAD somewhere in the Git object.\n> Which do you think makes most sense?\n>\n\nAsking git when you need it and keep HEAD value only for the minimum\ntime required to execute the commit command.\n\n- HEAD is very 'volatile'\n\n- commit is _not_ performance critical.\n\n- commit, being a write operation, is instead bugs/misbehaviour\ncritical (a big point to use an high level \"git-commit\" BTW)\n\n- asking git with getRefSha(\"HEAD\", ANY_REF, true) is very quick and\nin any case much quicker then the whole commit dance.\n\n- someone can always change the repo behind you, qgit is not the only\ninterface to git ;-) does exist also the command line.\n\n\nMarco\n"},{"id":"46754","messageId":"20070708133825.GE3991@efreet.light.src","threadId":"8540","inReplyTo":"e5bfff550707060112p29b9565bw9ccba6601745b850@mail.gmail.com","subject":"Re: [Qgit RFC] commit --amend","fromName":"Jan Hudec","fromEmail":"bulb@ucw.cz","sentAt":"2007-07-08T13:38:26Z","receivedAt":"2007-07-08T13:38:26Z","isPatch":false,"sender":{"key":"bulb@ucw.cz","avatar":null},"body":"On Fri, Jul 06, 2007 at 10:12:50 +0200, Marco Costalba wrote:\n> On 7/5/07, Jan Hudec wrote:\n>> Therefore I'll either have to always ask git via run(\"git-rev-parse HEAD\",\n>> head), add HEAD into the map, or store HEAD somewhere in the Git object.\n>> Which do you think makes most sense?\n>\n> Asking git when you need it and keep HEAD value only for the minimum\n> time required to execute the commit command.\n>\n> - HEAD is very 'volatile'\n>\n> - commit is _not_ performance critical.\n>\n> - commit, being a write operation, is instead bugs/misbehaviour\n> critical (a big point to use an high level \"git-commit\" BTW)\n>\n> - asking git with getRefSha(\"HEAD\", ANY_REF, true) is very quick and\n> in any case much quicker then the whole commit dance.\n\nYes. It is also much faster to write in code, but...\n\n> - someone can always change the repo behind you, qgit is not the only\n> interface to git ;-) does exist also the command line.\n\nAnd the commit in qgit should better fail loudly if that happens, because the\nlist of files or something else might no longer make sense.\n\nThere is actually just one thing I need the head for -- passing it as 3rd\nargument to git-update-ref. That should be done purely as safety measure --\nif the value does not match, the command will fail. And for that safety\nmeasure to be useful, I need value of the head at the time user openend the\ncommit dialog, NOT the time user clicked on commit button.\n\n-- \n\t\t\t\t\t\t Jan 'Bulb' Hudec \n"},{"id":"46755","messageId":"e5bfff550707080649r493616cw4bbf3396e059698d@mail.gmail.com","threadId":"8540","inReplyTo":"20070708133825.GE3991@efreet.light.src","subject":"Re: [Qgit RFC] commit --amend","fromName":"Marco Costalba","fromEmail":"mcostalba@gmail.com","sentAt":"2007-07-08T13:49:27Z","receivedAt":"2007-07-08T13:49:27Z","isPatch":false,"sender":{"key":"mcostalba@gmail.com","avatar":null},"body":"On 7/8/07, Jan Hudec wrote:\n> On Fri, Jul 06, 2007 at 10:12:50 +0200, Marco Costalba wrote:\n> > On 7/5/07, Jan Hudec wrote:\n> >> Therefore I'll either have to always ask git via run(\"git-rev-parse HEAD\",\n> >> head), add HEAD into the map, or store HEAD somewhere in the Git object.\n> >> Which do you think makes most sense?\n> >\n> > Asking git when you need it and keep HEAD value only for the minimum\n> > time required to execute the commit command.\n> >\n> > - HEAD is very 'volatile'\n> >\n> > - commit is _not_ performance critical.\n> >\n> > - commit, being a write operation, is instead bugs/misbehaviour\n> > critical (a big point to use an high level \"git-commit\" BTW)\n> >\n> > - asking git with getRefSha(\"HEAD\", ANY_REF, true) is very quick and\n> > in any case much quicker then the whole commit dance.\n>\n> Yes. It is also much faster to write in code, but...\n>\n> > - someone can always change the repo behind you, qgit is not the only\n> > interface to git ;-) does exist also the command line.\n>\n> And the commit in qgit should better fail loudly if that happens, because the\n> list of files or something else might no longer make sense.\n>\n> There is actually just one thing I need the head for -- passing it as 3rd\n> argument to git-update-ref. That should be done purely as safety measure --\n> if the value does not match, the command will fail. And for that safety\n> measure to be useful, I need value of the head at the time user openend the\n> commit dialog, NOT the time user clicked on commit button.\n>\n\nIt it has \"commit dialog\" life span I would suggets to save in a\n\"commit dialog\" object member data.\n\nMarco\n"}]}