{"thread":{"id":"36382","subject":"On \"interpret-trailers\" standalone tool","startedAt":"2014-04-09T19:57:26Z","lastAt":"2014-04-16T18:54:44Z","messageCount":7,"participants":["Junio C Hamano","Christian Couder"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"238602","messageId":"xmqq61mi1djt.fsf@gitster.dls.corp.google.com","threadId":"36382","inReplyTo":null,"subject":"On \"interpret-trailers\" standalone tool","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-09T19:57:26Z","receivedAt":"2014-04-09T19:57:26Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"So far I've mostly been ignoring how the command line would look\nlike, because the intermediate goal to my mind was to have it as a\nhook that are added by people better versed with Git than an average\nend-user, and if the command line interface had to change then they\nare capable of updating it, so it is more acceptable than the usual\nend-user tools to break compatibility between an early prototype and\nlater versions, and because the final goal would be to libify the\ninternal logic and integrate it into places we would invoke hooks,\nmaking the standalone command irrelevant.\n\nHowever, I started to care ;-)  For example, wouldn't it be nice if\nyou can do\n\n    $ git format-patch -5 --cover-letter -o +my-series/ my-topic\n    $ git interpret-trailers \"some args\" ./+my-series/0*.patch\n\nto fix-up the \"trailers\" portion of the proposed log message in the\nformatted patches?  There may be other possible uses that having a\nstandalone tool would be helpful, even after we removed the need for\nsuch a tool from commit, rebase, etc. by integrating the internal\nlogic to the implementation of these commands.\n\nHowever, I am wondering if the current \"everything on the command\nline is instruction to the command\" is too limiting to allow the use\nof the tool both as a filter and as a tool that can work on one or\nmore files named on the command line.  If we start from there, the\nonly way to later add \"these arguments are names of the files to be\noperated on\" is to add \"--file <file1> --file <file2>...\" options,\nwhich feels quite backwards as a UNIX tool.\n\nIt would be easier to explain and understand if the command line\noption set is modeled after things like \"cat\" or \"sed\", where\nnon-option arguments are filenames, instructions are given in the\nform of \"--option <arg>\" (e.g. \"-e 's/foo/bar/'\" given to sed), and\nhaving no non-option arguments on the command line signals that the\ntool is working as a filter.\n"},{"id":"238778","messageId":"20140412.213003.73509455950485398.chriscool@tuxfamily.org","threadId":"36382","inReplyTo":"xmqq61mi1djt.fsf@gitster.dls.corp.google.com","subject":"Re: On \"interpret-trailers\" standalone tool","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-04-12T19:30:03Z","receivedAt":"2014-04-12T19:30:03Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Junio C Hamano <gitster@pobox.com>\n>\n> So far I've mostly been ignoring how the command line would look\n> like,\n\nI don't really feel this way ;-)\n\n> because the intermediate goal to my mind was to have it as a\n> hook that are added by people better versed with Git than an average\n> end-user, and if the command line interface had to change then they\n> are capable of updating it, so it is more acceptable than the usual\n> end-user tools to break compatibility between an early prototype and\n> later versions, and because the final goal would be to libify the\n> internal logic and integrate it into places we would invoke hooks,\n> making the standalone command irrelevant.\n> \n> However, I started to care ;-)  For example, wouldn't it be nice if\n> you can do\n> \n>     $ git format-patch -5 --cover-letter -o +my-series/ my-topic\n>     $ git interpret-trailers \"some args\" ./+my-series/0*.patch\n> \n> to fix-up the \"trailers\" portion of the proposed log message in the\n> formatted patches?  There may be other possible uses that having a\n> standalone tool would be helpful, even after we removed the need for\n> such a tool from commit, rebase, etc. by integrating the internal\n> logic to the implementation of these commands.\n> \n> However, I am wondering if the current \"everything on the command\n> line is instruction to the command\" is too limiting to allow the use\n> of the tool both as a filter and as a tool that can work on one or\n> more files named on the command line.  If we start from there, the\n> only way to later add \"these arguments are names of the files to be\n> operated on\" is to add \"--file <file1> --file <file2>...\" options,\n> which feels quite backwards as a UNIX tool.\n\nYeah, except that we could add for example a '-o' option that would\ntake a directory as argument and that would mean that the command\nshould operate on all the files in this directory. It would be like\nthe -o option of the format-patch command.\n\n> It would be easier to explain and understand if the command line\n> option set is modeled after things like \"cat\" or \"sed\", where\n> non-option arguments are filenames, instructions are given in the\n> form of \"--option <arg>\" (e.g. \"-e 's/foo/bar/'\" given to sed), and\n> having no non-option arguments on the command line signals that the\n> tool is working as a filter.\n\nYeah, that's an interesting idea. I am not against making yet another\nnumber of changes to \"git interpret-trailers\" to make something like\nthe above possible. But I think there are a few things that should be\ndiscussed first.\n\nFirst, if you think that the command might often be used along with\nformat-patch, then it might be interesting for the user to have the\ncommand behave like format-patch instead of like cat or sed. This\nmeans that we could add the -o option I suggest above. And we don't\nneed to do it now. We could add this option later instead of having to\nmake the command work on many files now.\n\nSecond, if the command should accept a patch as input instead of just\na commit message, or both, this means that the command should have a\nway to tell if it is passed a patch, and then locate the commit\nmessage part in the patch. This means yet other changes to the\ncommand. Maybe these changes could be made later, in another series,\nor when the need arises to use it on full patches.\n\nThird, if trailers arguments are passed to the command using an option\nlike \"-z token=value\" or \"-z token:value\", it would be nice to the\nuser for consistency if the same option could be used when passing the\nsame arguments to \"git commit\" and perhaps other commands like \"git\nrebase\", \"git cherry-pick\" and so on. This means that we now have to\nchoose carefully the name of this option. Perhaps we can just give it\na long name now like --trailer and care later about a short name, but\nI think it would not be very nice to the user to only have a long name\nfor this option as it will very often be used.\n\nFourth, some users might want the command to be passed some files as\ninput, but they might not want the command to modify these input\nfiles. They might prefer the command to write its ouput into another\nset of output files. Maybe a syntax like cat or sed is not very well\nsuited for this kind of use, while having a -o option for the output\ndirectory and a -i option for the input directory (if different from\nthe output dir) would be nicer.\n\nThanks,\nChristian.\n"},{"id":"238870","messageId":"xmqqy4z7lhc4.fsf@gitster.dls.corp.google.com","threadId":"36382","inReplyTo":"20140412.213003.73509455950485398.chriscool@tuxfamily.org","subject":"Re: On \"interpret-trailers\" standalone tool","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-14T21:41:15Z","receivedAt":"2014-04-14T21:41:15Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <chriscool@tuxfamily.org> writes:\n\n>> However, I am wondering if the current \"everything on the command\n>> line is instruction to the command\" is too limiting to allow the use\n>> of the tool both as a filter and as a tool that can work on one or\n>> more files named on the command line.  If we start from there, the\n>> only way to later add \"these arguments are names of the files to be\n>> operated on\" is to add \"--file <file1> --file <file2>...\" options,\n>> which feels quite backwards as a UNIX tool.\n>\n> Yeah, except that we could add for example a '-o' option that would\n> take a directory as argument and that would mean that the command\n> should operate on all the files in this directory. It would be like\n> the -o option of the format-patch command.\n\nFor output for which users do not know offhand what files are to be\nproduced, giving a single directory with -o makes tons of sense, but\nfor input, naming each individual file (and with help with shell\nglobs *) is a lot more natural UNIX tool way, I would think.  \"Take\neverything from this directory\" cannot be substitute for that, even\nthough the reverse (i.e. by naming the input files with \"dir/*\") is\ntrue.  It is not a viable replacement.\n\n> First, if you think that the command might often be used along with\n> format-patch,\n\n... I am not singling out format-patch output.  Any text file/stream\nthat has the commit log message may benefit from the \"trailers\" filter,\nand format-patch output is merely one very obvious example.  As to\nthe detection of the end of commit log message, the current \"EOF is\nwhere the log message ends (but we would remote trailing blank line)\"\ncan easily be updated to \"EOF or the first three-dash line\".\n\n> Third, if trailers arguments are passed to the command using an\n> option like \"-z token=value\" or \"-z token:value\", it would be nice\n> to the user for consistency if the same option could be used when\n> passing the same arguments to \"git commit\" and perhaps other\n> commands like \"git rebase\", \"git cherry-pick\" and so on. This\n> means that we now have to choose carefully the name of this\n> option. Perhaps we can just give it a long name now like --trailer\n> and care later about a short name,...\n\nAbsolutely.  That is a very sensible way to go.\n\n> Fourth, some users might want the command to be passed some files as\n> input, but they might not want the command to modify these input\n> files. They might prefer the command to write its ouput into another\n> set of output files. Maybe a syntax like cat or sed is not very well\n> suited for this kind of use, while having a -o option for the output\n> directory and a -i option for the input directory (if different from\n> the output dir) would be nicer.\n\nSure.  I would expect we would require something like Perl's '-i'\n(in-place rewrite) option for this sequence to really work:\n\n\tgit format-patch -o there -5\n\tgit that-command --options -i there/*\n\nand without, I would expect the output to come to its standard\noutput.\n\nThanks.\n"},{"id":"238932","messageId":"CAP8UFD0w9xUwuk9UZ_mu=qvjtYN1r5skidweruM8t1KbfQpQyA@mail.gmail.com","threadId":"36382","inReplyTo":"xmqqy4z7lhc4.fsf@gitster.dls.corp.google.com","subject":"Re: On \"interpret-trailers\" standalone tool","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2014-04-16T12:27:03Z","receivedAt":"2014-04-16T12:27:03Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, Apr 14, 2014 at 11:41 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Christian Couder <chriscool@tuxfamily.org> writes:\n>\n>> Yeah, except that we could add for example a '-o' option that would\n>> take a directory as argument and that would mean that the command\n>> should operate on all the files in this directory. It would be like\n>> the -o option of the format-patch command.\n>\n> For output for which users do not know offhand what files are to be\n> produced, giving a single directory with -o makes tons of sense, but\n> for input, naming each individual file (and with help with shell\n> globs *) is a lot more natural UNIX tool way, I would think.\n\nYeah, but the \"git interpret-trailers\" command is a special, because,\nif it takes files as arguments, then it is logical that its output\nwould be also files and not stdout. (See also at the end of this\nmessage.)\n\n> \"Take\n> everything from this directory\" cannot be substitute for that, even\n> though the reverse (i.e. by naming the input files with \"dir/*\") is\n> true.  It is not a viable replacement.\n>\n>> First, if you think that the command might often be used along with\n>> format-patch,\n>\n> ... I am not singling out format-patch output.  Any text file/stream\n> that has the commit log message may benefit from the \"trailers\" filter,\n> and format-patch output is merely one very obvious example.  As to\n> the detection of the end of commit log message, the current \"EOF is\n> where the log message ends (but we would remote trailing blank line)\"\n> can easily be updated to \"EOF or the first three-dash line\".\n\nOk, I think that it's an interesting feature anyway, so I can add it\nnow instead of later.\n\n>> Third, if trailers arguments are passed to the command using an\n>> option like \"-z token=value\" or \"-z token:value\", it would be nice\n>> to the user for consistency if the same option could be used when\n>> passing the same arguments to \"git commit\" and perhaps other\n>> commands like \"git rebase\", \"git cherry-pick\" and so on. This\n>> means that we now have to choose carefully the name of this\n>> option. Perhaps we can just give it a long name now like --trailer\n>> and care later about a short name,...\n>\n> Absolutely.  That is a very sensible way to go.\n\nOk, I will use \"--trailer\" then. As I said in my previous message,\nthis unfortunately means that the command will not be very user\nfriendly until we integrate it with other commands like \"git commit\"\nand find a short option name that hopefully work for all the commands.\n\n>> Fourth, some users might want the command to be passed some files as\n>> input, but they might not want the command to modify these input\n>> files. They might prefer the command to write its ouput into another\n>> set of output files. Maybe a syntax like cat or sed is not very well\n>> suited for this kind of use, while having a -o option for the output\n>> directory and a -i option for the input directory (if different from\n>> the output dir) would be nicer.\n>\n> Sure.  I would expect we would require something like Perl's '-i'\n> (in-place rewrite) option for this sequence to really work:\n>\n>         git format-patch -o there -5\n>         git that-command --options -i there/*\n>\n> and without, I would expect the output to come to its standard\n> output.\n\nIf the input comes from stdin, then I agree that the command should be\na filter, so its output should be on stdout. But if the input comes\nfrom files given as arguments, then I would say that the command\nshould behave as an editor and by default it should edit its input\nfile inplace. Its input and output files should be different only if\nit is given one or more special option,\n\nOtherwise the example you gave:\n\n    $ git format-patch -5 --cover-letter -o +my-series/ my-topic\n    $ git interpret-trailers \"some args\" ./+my-series/0*.patch\n\nwould result in having on stdout all the patches edited by \"git\ninterpret-trailers\".\nHow would people could then easily send these edited patches?\n\nThanks,\nChristian.\n"},{"id":"238967","messageId":"xmqq38hdi2zr.fsf@gitster.dls.corp.google.com","threadId":"36382","inReplyTo":"CAP8UFD0w9xUwuk9UZ_mu=qvjtYN1r5skidweruM8t1KbfQpQyA@mail.gmail.com","subject":"Re: On \"interpret-trailers\" standalone tool","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-16T17:43:52Z","receivedAt":"2014-04-16T17:43:52Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <christian.couder@gmail.com> writes:\n\n> If the input comes from stdin, then I agree that the command should be\n> a filter, so its output should be on stdout. But if the input comes\n> from files given as arguments, then I would say that the command\n> should behave as an editor and by default it should edit its input\n> file inplace.\n\nI do not see where that \"should\" comes from.  I am looking at this\nmore from the angle of obtaining a useful building block, while you\nseem to be thinking of this as a specialized tool for a narrow set\nof specifkc tasks.\n\nThinking of examples in \"sort\" and \"sed\", I would say \"read multiple\nfiles, futz with the contents and spit out a single output stream\"\nis the way people expect a command to operate without being told to\noperate in some other way.  Overwriting existing files should never\nbe the default for safety---otherwise you would require people who\nwant safety to do something like:\n\n    cp realfile tmp\n    futz tmp\n    verify tmp\n    mv tmp realfile\n\nOn the other hand, a usual \"sort/sed/cat\"-like command, even without\nthe \"in-place edit\" option, can be used as in-place with\n\n    mv realfile tmp\n    futz tmp >realfile\n\neasily, and is more flexible as a building block.  Of course, that\ndoes not rule out an option to work in-place (e.g. in a similar way\nto \"sort -o file file\", or \"perl -i -e 'y/a-z/A-Z/' frotz nitfol\").\n\n> Its input and output files should be different only if\n> it is given one or more special option,\n>\n> Otherwise the example you gave:\n>\n>     $ git format-patch -5 --cover-letter -o +my-series/ my-topic\n>     $ git interpret-trailers \"some args\" ./+my-series/0*.patch\n>\n> would result in having on stdout all the patches edited by \"git\n> interpret-trailers\".\n\nDidn't I mention that I do not mind \"-i\" already if in-place edit is\ndesired?  Add \"-i\" to the command line arguments among \"some args\",\nand your complaints will disappear, no?\n"},{"id":"238971","messageId":"xmqqlhv5glst.fsf@gitster.dls.corp.google.com","threadId":"36382","inReplyTo":"xmqq38hdi2zr.fsf@gitster.dls.corp.google.com","subject":"Re: On \"interpret-trailers\" standalone tool","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-16T18:40:34Z","receivedAt":"2014-04-16T18:40:34Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> ...  I am looking at this\n> more from the angle of obtaining a useful building block, while you\n> seem to be thinking of this as a specialized tool for a narrow set\n> of specifkc tasks.\n\nBy the way, I am speaking with a bitter experience of designing the\noriginal \"format-patch\" command line parameters, thinking that \"This\nis a specialized tool to let me send what Linus hasn't picked up\nyet, so the command should take where Linus is and where I am\".\n\nNot using the A..B syntax turned out to be a poor choice but that\nrealization came long after the ship sailed---back then, A..B syntax\nwas relatively new and it was unclear that it would become the\nuniversal syntax we use throughout the system to denote a range of\ncommits in the DAG.\n\nThe design mistake for starting at a specialized tool for a narrow\nset of specific tasks took considerable effort to retrofit the\ngeneral syntax while not breaking the traditional usage.  I am being\ncautious here because I do not see us making the same mistake.\n"},{"id":"238972","messageId":"xmqqeh0xgl57.fsf@gitster.dls.corp.google.com","threadId":"36382","inReplyTo":"xmqqlhv5glst.fsf@gitster.dls.corp.google.com","subject":"Re: On \"interpret-trailers\" standalone tool","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-16T18:54:44Z","receivedAt":"2014-04-16T18:54:44Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> ...  I am being\n> cautious here because I do not see us making the same mistake.\n\ns/do not/& want to/\n\nSorry for the noise.\n"}]}