{"thread":{"id":"16632","subject":"[PATCH 2/3] diff: allow turning on textconv explicitly for plumbing","startedAt":"2008-12-08T02:57:01Z","lastAt":"2008-12-09T05:48:24Z","messageCount":8,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"97325","messageId":"20081208025700.GB22072@coredump.intra.peff.net","threadId":"16632","inReplyTo":null,"subject":"[PATCH 2/3] diff: allow turning on textconv explicitly for plumbing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-12-08T02:57:01Z","receivedAt":"2008-12-08T02:57:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Some history viewers use the diff plumbing to generate diffs\nrather than going through the \"git diff\" porcelain.\nCurrently, there is no way for them to specify that they\nwould like to see the text-converted version of the diff.\n\nThis patch adds a \"--textconv\" option to allow such a\nplumbing user to allow text conversion.  The user can then\ntell the viewer whether or not they would like text\nconversion enabled.\n\nWhile it may be tempting add a configuration option rather\nthan requiring each plumbing user to be configured to pass\n--textconv, that is somewhat dangerous. Text-converted diffs\ngenerally cannot be applied directly, so each plumbing user\nshould \"opt in\" to generating such a diff, either by\nexplicit request of the user or by confirming that their\noutput will not be fed to patch.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nMy ultimate goal is to see these diffs in gitk, which is implemented in\n3/3. As a bonus side effect, the --no-textconv option can be used with\n\"git diff\" or \"git log\" if you really don't want to see them there\n(e.g., because you are abusing \"git log\" to produce a binary patch you\nmean to apply).\n\nI know this is not strictly a bugfix and we are in -rc, but:\n\n  1. It is an enhancement to a previously unreleased feature, and\n     shouldn't affect anything outside of that.\n\n  2. It affects the scripting interface to textconv, so I would like to\n     get it in before textconv is ever released so that it is always the\n     \"right way\" to turn text conversion off or on.\n\n diff.c |    4 ++++\n diff.c |    4 ++++\n 1 files changed, 4 insertions(+), 0 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex f644947..e21af3b 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2477,6 +2477,10 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)\n \t\tDIFF_OPT_SET(options, ALLOW_EXTERNAL);\n \telse if (!strcmp(arg, \"--no-ext-diff\"))\n \t\tDIFF_OPT_CLR(options, ALLOW_EXTERNAL);\n+\telse if (!strcmp(arg, \"--textconv\"))\n+\t\tDIFF_OPT_SET(options, ALLOW_TEXTCONV);\n+\telse if (!strcmp(arg, \"--no-textconv\"))\n+\t\tDIFF_OPT_CLR(options, ALLOW_TEXTCONV);\n \telse if (!strcmp(arg, \"--ignore-submodules\"))\n \t\tDIFF_OPT_SET(options, IGNORE_SUBMODULES);\n \n-- \n1.6.1.rc2.285.gc1cf2\n"},{"id":"97329","messageId":"7vfxkz9v8f.fsf@gitster.siamese.dyndns.org","threadId":"16632","inReplyTo":"20081208025700.GB22072@coredump.intra.peff.net","subject":"Re: [PATCH 2/3] diff: allow turning on textconv explicitly for plumbing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-08T03:55:12Z","receivedAt":"2008-12-08T03:55:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I know this is not strictly a bugfix and we are in -rc, but:\n>\n>   1. It is an enhancement to a previously unreleased feature, and\n>      shouldn't affect anything outside of that.\n>\n>   2. It affects the scripting interface to textconv, so I would like to\n>      get it in before textconv is ever released so that it is always the\n>      \"right way\" to turn text conversion off or on.\n\nI'd agree with #1, especially if you said \"doesn't\" instead of\n\"shouldn't\".\n\nBut I am not 100% sure if the scripting part is \"the right way\".\n\nIf a script wants to take whatever Porcelain users are happy as the\n\"presentation for human consumption\" and pass that through as its own\noutput to the end user, maybe it is better off reading from Porcelain,\ninstead of reading from the plumbing (the latter of which requires making\nthe plumbing output less reliable)?\n\nWhen we later enhance textconv output from the \"diff\" Porcelain to benefit\ninteractive users, it will automatically help the script that passes\nthrough the \"diff\" output to the end users.\n\nYou can certainly argue that this \"textconv\" feature that is grafted from\nPorcelain into plumbing is a special case in that its output is subject to\nchange any time to help human consumption and we never strive for its\nstability as we do for other features in the plumbing to support machine\nreadability by scripts.  You can propagate the later enhancement of\ntextconv diff output we'd make for Porcelain to the scripted users that\nreads from the plumbing that way.\n\nBut then wouldn't it be the same for these scripts that do value the\n\"presentation meant for human consumption\" over \"machine readability\" to\nread from Porcelain?  That would not have to blur the distinction between\nthe Porcelain and plumbing like the approach you are suggesting here.\n"},{"id":"97331","messageId":"7vbpvn9um4.fsf@gitster.siamese.dyndns.org","threadId":"16632","inReplyTo":"7vfxkz9v8f.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/3] diff: allow turning on textconv explicitly for plumbing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-08T04:08:35Z","receivedAt":"2008-12-08T04:08:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Jeff King <peff@peff.net> writes:\n>\n>> I know this is not strictly a bugfix and we are in -rc, but:\n>>\n>>   1. It is an enhancement to a previously unreleased feature, and\n>>      shouldn't affect anything outside of that.\n>>\n>>   2. It affects the scripting interface to textconv, so I would like to\n>>      get it in before textconv is ever released so that it is always the\n>>      \"right way\" to turn text conversion off or on.\n>\n> I'd agree with #1, especially if you said \"doesn't\" instead of\n> \"shouldn't\".\n>\n> But I am not 100% sure if the scripting part is \"the right way\".\n\nBut I'll apply them anyway.\n"},{"id":"97332","messageId":"20081208045924.GA22780@coredump.intra.peff.net","threadId":"16632","inReplyTo":"7vfxkz9v8f.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/3] diff: allow turning on textconv explicitly for plumbing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-12-08T04:59:24Z","receivedAt":"2008-12-08T04:59:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Dec 07, 2008 at 07:55:12PM -0800, Junio C Hamano wrote:\n\n> But I am not 100% sure if the scripting part is \"the right way\".\n> \n> If a script wants to take whatever Porcelain users are happy as the\n> \"presentation for human consumption\" and pass that through as its own\n> output to the end user, maybe it is better off reading from Porcelain,\n> instead of reading from the plumbing (the latter of which requires making\n> the plumbing output less reliable)?\n\nBut I don't think having gitk call the porcelain makes sense, either.\nThat would enable, for example, external diff drivers which might\nactually spawn an entirely new window. And I don't think most people\nwould want that from gitk.\n\nNow, obviously it could call \"git diff --no-ext-diff\" to opt out of\nthat. But I think it is backwards to call porcelain but opt out of\nfeatures that you don't want. It makes more sense to start with the bare\nminimum (i.e., plumbing) and opt in to features that you are OK with.\n\nAnd yes, you increase the risk that a script which blindly passes\noptions to plumbing may now be asked to generate a textconv'd diff which\nwill be useless to the user. But:\n\n  1. that is already a risk with --ext-diff\n\n  2. it is actually a _feature_. It is asking the user to say \"is this\n     a sane option to pass for my situation or not\" instead of forcing\n     the script to make that decision.\n\n> When we later enhance textconv output from the \"diff\" Porcelain to\n> benefit interactive users, it will automatically help the script that\n> passes through the \"diff\" output to the end users.\n\nI'm not sure what enhancement you mean. But it is possible that gitk\nwould not _want_ such an enhancement, and it would then have to opt out\nof it.\n\n> You can certainly argue that this \"textconv\" feature that is grafted from\n> Porcelain into plumbing is a special case in that its output is subject to\n> change any time to help human consumption and we never strive for its\n> stability as we do for other features in the plumbing to support machine\n> readability by scripts.  You can propagate the later enhancement of\n> textconv diff output we'd make for Porcelain to the scripted users that\n> reads from the plumbing that way.\n\nRight. _If_ it's a change that won't upset any plumbing consumers. If it\nis, then it needs to be a separate option so that the plumbing consumers\nwho don't mind the change can start using it.\n\n> But then wouldn't it be the same for these scripts that do value the\n> \"presentation meant for human consumption\" over \"machine readability\" to\n> read from Porcelain?  That would not have to blur the distinction between\n> the Porcelain and plumbing like the approach you are suggesting here.\n\nI don't agree that this is blurring the distinction. I don't see\ntextconv diffs as a \"porcelain feature\" that has been grafted on to\nplumbing. I see it as a core diff feature which can be turned off or on.\nPorcelain has it on by default, and plumbing has it off by default. But\nyou can ask either to change their default settings[1].\n\n-Peff\n\n[1]: You can argue that the current implementation is buggy in this\nsense, though, since some porcelains will accept \"--textconv\" but not\nchange their behavior. I think blame, for example, will have that\nproblem, because it accepts diff options but does not necessarily use\nthem to create diffs. But I think that just means we have a bug and\neventually should handle the case of \"git blame --textconv\" correctly.\n"},{"id":"97335","messageId":"7vskoz88g8.fsf@gitster.siamese.dyndns.org","threadId":"16632","inReplyTo":"20081208045924.GA22780@coredump.intra.peff.net","subject":"Re: [PATCH 2/3] diff: allow turning on textconv explicitly for plumbing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-08T06:52:39Z","receivedAt":"2008-12-08T06:52:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> You can certainly argue that this \"textconv\" feature that is grafted from\n>> Porcelain into plumbing is a special case in that its output is subject to\n>> change any time to help human consumption and we never strive for its\n>> stability as we do for other features in the plumbing to support machine\n>> readability by scripts.  You can propagate the later enhancement of\n>> textconv diff output we'd make for Porcelain to the scripted users that\n>> reads from the plumbing that way.\n>\n> Right. _If_ it's a change that won't upset any plumbing consumers. If it\n> is, then it needs to be a separate option so that the plumbing consumers\n> who don't mind the change can start using it.\n\nIn the above argument, you are assuming that you can have a complete\nenumeration of \"plumbing consumers\", so that we can tell what kind of\nchanges to textconv output do affect them in a negative way, and what kind\nof changes are safe.\n\nYes, you can enumerate in-tree consumers, but that misses the whole point\nof having \"plumbing\", which can be used by any scripts out of tree and\nthey can rely on stable output from the plumbing.  By giving --textconv to\nthem to use with the plumbing, you are effecively casting textconv output\nin stone.  By definition we will never know what kind of changes to the\noutput from the textconv filter out of tree consumers would mind.\n\nCurrently \"diff\" machinery uses the output of textconv filter as-is\nwithout any further embellishment, but I think it is too early to tell if\nthat is really what we would want, or we may need some minimum\npostprocessing on what we receive from the textconv filter before feeding\nthat to the textual diff machinery, because the feature has not been used\nin the field at all yet.\n\nIn any case, I've applied the series for an entirely different reason.\nThe patch is the most natural way to allow users of Porcelain to disable\ntextconv with the --no-textconv option, just like --no-ext-diff can be\nused to disable the external diff.\n\nWe _might_ want to add another patch to make the plumbing layer ignore (or\nerror out) --textconv option given from the command line before 1.6.1\nships, and after we gain confidence with the stability of the feature,\nrevert that patch to allow use of --textconv freely from the plumbing.\n\nIt always is easier to introduce a new feature in a more limited form and\nthen later loosen it, than adding it in an unrestricted form and having\nlater to limit it for whatever reason.\n"},{"id":"97336","messageId":"20081208071449.GA24020@coredump.intra.peff.net","threadId":"16632","inReplyTo":"7vskoz88g8.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/3] diff: allow turning on textconv explicitly for plumbing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-12-08T07:14:49Z","receivedAt":"2008-12-08T07:14:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Dec 07, 2008 at 10:52:39PM -0800, Junio C Hamano wrote:\n\n> > Right. _If_ it's a change that won't upset any plumbing consumers. If it\n> > is, then it needs to be a separate option so that the plumbing consumers\n> > who don't mind the change can start using it.\n> \n> In the above argument, you are assuming that you can have a complete\n> enumeration of \"plumbing consumers\", so that we can tell what kind of\n> changes to textconv output do affect them in a negative way, and what kind\n> of changes are safe.\n\nNo, I'm assuming we have an idea of what is a \"reasonable\" change to the\nfeature that won't break consumers, and what is a change that will\nbreak them. And it's the same judgement we have to make all the time\nwhen thinking about how changes will impact the scriptable interface.\n\nAnd furthermore, I'm proposing to take the safest path, which is to\nsay that the scriptable interface _doesn't_ change unless each consumer\ndecides to ask it to do so. That is, the output of \"git diff-tree\" is\nunchanged unless the caller explicitly allows textconv. So now we have\ngiven a meaning to \"--textconv\". If you want a _new_ textconv variant\n(let's say you want it to output the textconv patch with some \"this\nisn't a real patch\" munging, and also include the binary patch -- which\nwould make it both human readable and applicable), then I think it is\nsafe to say that is an incompatible change. And it gets called\n--textconv-with-patch or whatever, and porcelain can move to it by\ndefault. But the plumbing consumers still get the same thing via\n--textconv, and they can opt into the new format if that is useful to\nthem.\n\nSo uptake of the new feature is slower (you have to wait for the\nmaintainer of the script to support it) but you never break\ncompatibility.\n\n> Yes, you can enumerate in-tree consumers, but that misses the whole point\n> of having \"plumbing\", which can be used by any scripts out of tree and\n> they can rely on stable output from the plumbing.  By giving --textconv to\n> them to use with the plumbing, you are effecively casting textconv output\n> in stone.  By definition we will never know what kind of changes to the\n> output from the textconv filter out of tree consumers would mind.\n\nRight. But we _have_ to cast it in stone if we want to make it available\nto them at all. So I am proposing to give what exists now a name, and\nfurther enhancements will have to have a different name.\n\nIf you want to argue that it is too early to cast in stone, since we\nwill have to carry around this implementation and the name \"--textconv\"\nforever, then I can accept that. But it is a bit annoying, since I want\nto use it in gitk _now_. :)\n\nBut I do run 'next' usually, so maybe it is worth pushing it off until\nthe next release cycle.\n\n> In any case, I've applied the series for an entirely different reason.\n> The patch is the most natural way to allow users of Porcelain to disable\n> textconv with the --no-textconv option, just like --no-ext-diff can be\n> used to disable the external diff.\n\nYes, I agree that is worthwhile and should have been there since the\nbeginning (and I think should definitely ship in 1.6.1).\n\n> We _might_ want to add another patch to make the plumbing layer ignore (or\n> error out) --textconv option given from the command line before 1.6.1\n> ships, and after we gain confidence with the stability of the feature,\n> revert that patch to allow use of --textconv freely from the plumbing.\n\nIf you want to do that, the simplest thing is to simply take the\n\"--no-textconv\" part of 2/3 and drop the rest. There is not really any\npoint in supporting --textconv just for porcelain, which already has it\non by default.\n\n-Peff\n"},{"id":"97339","messageId":"7v63lv842a.fsf@gitster.siamese.dyndns.org","threadId":"16632","inReplyTo":"7vskoz88g8.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/3] diff: allow turning on textconv explicitly for plumbing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-08T08:27:25Z","receivedAt":"2008-12-08T08:27:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> In any case, I've applied the series for an entirely different reason.\n> The patch is the most natural way to allow users of Porcelain to disable\n> textconv with the --no-textconv option, just like --no-ext-diff can be\n> used to disable the external diff.\n\nI imagine that web developers may want to use a textconv filter that\nreplaces \">\" with \"\\n>\" on the HTML files their designer-colleagues throw\nin the source tree to make \"git log -p\" of the whole project easier to\nfollow.  When the developers would want to suggest improvements to what\ntheir designer-colleagues did, however, by running \"git diff --stat -p\" in\ntheir dirty work tree to produce a patch (like I just did just now,\nvisible from the mnemonic prefixes below), they would want to disable\ntextconv temporarily to get an appliable patch with --no-textconv option.\n\nOnce we have --no-textconv, somebody would inevitably ask about its\npositive counterpart, --textconv option.  Even though it might not make\nsense from patch applicability viewpoint, the option would allow the end\nuser to explicitly ask for \"git format-patch --textconv\" and get a patch\nthat can only be reviewed by humans but cannot be applied.  Hence the\nattached update to the draft release notes to version 1.6.1 [Update #1].\n\nYou raised an intriguing possibility to use textconv in blame.  It would\nalso be useful if we allowed \"git show --textconv $blob\" to pass the blob\nvia textconv filter and any other transformation controlled by the\nattributes mechanism..  When \"git show\" sees the above command line, it\nonly knows the blob object name and not the path, so we may need to allow\na new option to tell the command to pretend as if the content came from a\npath, perhaps with a syntax like:\n\n\t$ git show --attribute-path=a/b/c $blob\n\t$ git show --attribute-path=a/b/c --textconv $blob\n\nNote that I envision that the above two commands would produce different\nresults.  The former would behave as if $blob were recorded at path a/b/c\nand what would be checked out (i.e. usual crlf, ident, and smudge that are\nin effect for the path are applied) to the standard output.  The latter\nwould apply the textconv filter that would apply to what is recorded at\nthe given path and show that.\n\n[Update #1]\n\n Documentation/RelNotes-1.6.1.txt |    3 ++-\n 1 files changed, 2 insertions(+), 1 deletions(-)\n\ndiff --git c/Documentation/RelNotes-1.6.1.txt w/Documentation/RelNotes-1.6.1.txt\nindex 0405309..09bbcad 100644\n--- c/Documentation/RelNotes-1.6.1.txt\n+++ w/Documentation/RelNotes-1.6.1.txt\n@@ -133,7 +133,8 @@ on.\n   contents can be munged into human readable form and the difference\n   between the results of the conversion can be viewed (obviously this\n   cannot produce a patch that can be applied, so this is disabled in\n-  format-patch among other things).\n+  format-patch and plumbing, but if you really wanted to, you can enable\n+  it by giving them --textconv command line option explicitly).\n \n * \"--cached\" option to \"git diff has an easier to remember synonym \"--staged\",\n   to ask \"what is the difference between the given commit and the\n"},{"id":"97399","messageId":"20081209054824.GA2972@coredump.intra.peff.net","threadId":"16632","inReplyTo":"7v63lv842a.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/3] diff: allow turning on textconv explicitly for plumbing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-12-09T05:48:24Z","receivedAt":"2008-12-09T05:48:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 08, 2008 at 12:27:25AM -0800, Junio C Hamano wrote:\n\n> I imagine that web developers may want to use a textconv filter that\n> replaces \">\" with \"\\n>\" on the HTML files their designer-colleagues throw\n> in the source tree to make \"git log -p\" of the whole project easier to\n\nHrm, didn't you argue against textconv'ing non-binary contents earlier\nin the cycle? At any rate, I agree that is a reasonable use. Maybe a\nbetter name would have been \"normalize\" or \"canonicalize\".\n\n> follow.  When the developers would want to suggest improvements to what\n> their designer-colleagues did, however, by running \"git diff --stat -p\" in\n> their dirty work tree to produce a patch (like I just did just now,\n> visible from the mnemonic prefixes below), they would want to disable\n> textconv temporarily to get an appliable patch with --no-textconv option.\n\nYep, exactly. I use \"git diff >patch\" all the time instead of\nformat-patch.\n\n> You raised an intriguing possibility to use textconv in blame.  It would\n> also be useful if we allowed \"git show --textconv $blob\" to pass the blob\n> via textconv filter and any other transformation controlled by the\n> attributes mechanism..  When \"git show\" sees the above command line, it\n> only knows the blob object name and not the path, so we may need to allow\n> a new option to tell the command to pretend as if the content came from a\n> path, perhaps with a syntax like:\n> \n> \t$ git show --attribute-path=a/b/c $blob\n> \t$ git show --attribute-path=a/b/c --textconv $blob\n\nI think that makes sense. If you are going to implement a \"use this as\nthe attribute path\" feature, though, you might also want just \"use these\nattributes\" which I can imagine being simpler in some cases. Like:\n\n  $ git show --attribute diff=whatever $blob\n\nBut on the subject of \"other places to see textconv\", another one I\nconsidered was git log -S. I haven't looked to see if it even checks\nbinary files at all, but certainly if I was searching for some text\ncontent I would want to find it in the text version (and depending on\nthe file format, there may be other binary cruft preventing you from\nfinding it in the binary version at all).\n\nThe series I posted was to show the text in gitk diffs. However, it does\nnothing for finding text in gitk's \"find commits introducing this\nstring\", which I assume just uses git's pickaxe.\n\nUnfortunately, it would probably be painfully slow. But still better\nthan nothing. One enhancement for textconv that I am considering is to\nname the tempfiles based on the blob sha1. Then a smart helper could\ncache expensive conversions if it wanted (and dumb helpers would\nobviously just ignore the filename).\n\nSide note: For one use case, storing word processor documents, this\napproach seems sensible. But for my jpg example, it almost seems that\nI could use git more efficiently by having two files: one with the exif\ntext, and one with the image content. And then I could \"build\" the\nresulting tagged jpgs by combining the two, but operations on the text\nwould be very efficient. The downside, of course, being that the \"build\"\nstep is annoying, and consumes a ton of disk space. But it just occurred\nto me as an alternate way to think about the use case.\n\n> @@ -133,7 +133,8 @@ on.\n>    contents can be munged into human readable form and the difference\n>    between the results of the conversion can be viewed (obviously this\n>    cannot produce a patch that can be applied, so this is disabled in\n> -  format-patch among other things).\n> +  format-patch and plumbing, but if you really wanted to, you can enable\n> +  it by giving them --textconv command line option explicitly).\n\nI wonder if we want to word this even more strongly: --textconv is there\nand you can play with it if you want, but we are not promising that it\nis part of the stable API yet.\n\n-Peff\n"}]}