Jeff King venit, vidit, dixit 21.04.2013 05:37: > On Sat, Apr 20, 2013 at 03:38:53PM +0200, Michael J Gruber wrote: > >>> Wait, this does the opposite of the last patch. If we do want to do >>> this, shouldn't the last one have been an "expect_failure"? >> >> The last patch just documents the status quo, which is not a bug per se. >> Therefore, no failure, but change in the definition of "success". > > IMHO, the series is easier to review if you it does not go back and > forth. If you have one patch that says "X is the right behavior", and > then another patch that flips it to say "Y is the right behavior", the > reviewer would read each in sequence and want to be convinced by your > arguments for X and Y. But you probably cannot make a good argument for > X if you are trying to end up at Y. :) > > So I'd much rather see the test introduced with the desired end > behavior, marked as expect_failure, and the commit message contain an > argument about why Y is a good thing (and squashing the tests in with > the actual fix is often even better, because the fix itself would want > to contain the same argument). > > Just my two cents as a reviewer. > >> My reasoning is twofold: >> >> - consistency between "git show commit" and "git show blob" > > I'm not sure I agree with this line of reasoning. "git show commit" is > showing a diff, not the file contents; textconv has always been about > munging the contents to produce a textual diff. It may be reasonable to > extend its definition to "this is the preferred human view of this > content, and that happens to be what you would want to produce a diff". > But I do not think it is necessarily inconsistent not to apply it for > the blob case. > >> - "git show" is a user facing command, and as such should produce output >> consumable by humans; whereas "git cat-file" is plumbing and should >> produce raw data unless told otherwise (-p, --textconv). > > That holds if the textconv is the only (or best) human-readable version > of the file. And maybe that is reasonable. But is it possible that > somebody uses "textconv" to produce a better diff of some already > human-readable format? For example, imagine I define a textconv for XML > files that normalizes the formatting to make diffs less noisy. When I am > not looking at a diff, what is the best human-readable version? The > original, or the normalized one? I'm not sure. > > Note that I'm somewhat playing devil's advocate here. For the cases > where I have used textconv in the real world, I think I would probably > prefer seeing the converted contents, and I am happy to call it user > error if I use "git show HEAD:foo.jpg >bar.jpg" accidentally. But I also > want to make sure we are not regressing somebody else unnecessarily. Yes, the thing is that textconv helps diff by converting content (to text) before the (textual) diff. So it's somehow a double-faced beast. It's clearly activated by a "diff" attribute; so that would be a strong argument against my patch, at least against defaulting to --textconv for blobs. OTOH, textconv does have this aspect of converting text to a form digestable by humans (pre-diff, granted), which is the argument for defaulting to --textconv in porcellain. We could use a separate attribute "show" in addition to "diff", but I don't think it's worth going there, unless there is a strong use case for "diff-specific textconv" which one would not want to apply when showing just the content. Michael -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html