Re: [PATCH 2/6] show: obey --textconv for blobs
Matthieu Moy venit, vidit, dixit 22.04.2013 17:54: > Jeremy Rosen writes: > >> some features detect if they are piping to a terminal... couldn't we do >> something like that ? > > That's OK for convenience features like colors or so, but that would be > really, really unexpected to have > > $ git show HEAD:file > foo > $ git show HEAD:file > tmp > $ cat tmp > bar > > IMHO. Yes, I'd either do it by default in general (my preference) or on request, but not based on tty. Another point of input: You can do git show commit and also with other object types, of course. On the other hand, there is a single rev.diffopt. Besides the nuisance of having to track whether textconv has been specified explicitely and flipping the bit in rev.diffopt per argument (or adding a parameter), which is an implementation detail, it would mean that the default for different arguments in the argument list is different, depending on type. And that is a usablility issue, I would argue: Is textconv on by default for git show? Yes and no, for some arguments yes, for others no. That's what I want to cure ;) Michael -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/6] show: obey --textconv for blobs
Jeremy Rosen writes: > some features detect if they are piping to a terminal... couldn't we do > something like that ? That's OK for convenience features like colors or so, but that would be really, really unexpected to have $ git show HEAD:file foo $ git show HEAD:file > tmp $ cat tmp bar IMHO. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/6] show: obey --textconv for blobs
> > git show --textconv HEAD~4:binary-gob | less > > > > but I doubt it is a good idea to turn it on by default this late in > > the game. > > Exactly. I certainly do not mind it as an option, and I am on the > fence > regarding it as a default (I think it might have been a sane thing to > do > from the start, but at this point the change-of-behavior makes me > hesitate). So I am perfectly willing to go either way, depending on > what > others think. > some features detect if they are piping to a terminal... couldn't we do something like that ? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/6] show: obey --textconv for blobs
On Mon, Apr 22, 2013 at 08:25:41AM -0700, Junio C Hamano wrote: > True. Applying textconv to otherwise unreadable blobs is often > useful, but I agree that it is unexpected if it is done by default, > especially given that many people have learned to do: > > git show HEAD~4:binary-gob >old-binary-gob > > to recover old version of binary contents to a temporary file when > checking the sanity of or restoring the breakage in the new one. > > It of course does _not_ forbid > > git show --textconv HEAD~4:binary-gob | less > > but I doubt it is a good idea to turn it on by default this late in > the game. Exactly. I certainly do not mind it as an option, and I am on the fence regarding it as a default (I think it might have been a sane thing to do from the start, but at this point the change-of-behavior makes me hesitate). So I am perfectly willing to go either way, depending on what others think. I'm going to be out of email contact the rest of the week, so I'll let you two talk it out. :) -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/6] show: obey --textconv for blobs
Jeff King writes: > 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. True. Applying textconv to otherwise unreadable blobs is often useful, but I agree that it is unexpected if it is done by default, especially given that many people have learned to do: git show HEAD~4:binary-gob >old-binary-gob to recover old version of binary contents to a temporary file when checking the sanity of or restoring the breakage in the new one. It of course does _not_ forbid git show --textconv HEAD~4:binary-gob | less but I doubt it is a good idea to turn it on by default this late in the game. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/6] show: obey --textconv for blobs
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/6] show: obey --textconv for blobs
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. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/6] show: obey --textconv for blobs
Jeff King venit, vidit, dixit 20.04.2013 06:06: > On Fri, Apr 19, 2013 at 06:44:45PM +0200, Michael J Gruber wrote: > >> Currently, "diff" and "cat-file" for blobs obey "--textconv" options >> (with the former defaulting to "--textconv" and the latter to >> "--no-textconv") whereas "show" does not obey this option, even though >> it takes diff options. >> >> Make "show" on blobs behave like "diff", i.e. obey "--textconv" by >> default and "--no-textconv" when given. > > 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". > I'm not convinced this is the right thing to do, though. It would break: > > git show HEAD:file.c >file.c > > Admittedly, such people should be using "checkout" or "cat-file", so I > do not mind too much breaking them if there is a good reason. But I am > not sure what that reason is. My reasoning is twofold: - consistency between "git show commit" and "git show blob" - "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). (Sorry for the repeat.) Michael -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/6] show: obey --textconv for blobs
On Fri, Apr 19, 2013 at 06:44:45PM +0200, Michael J Gruber wrote: > Currently, "diff" and "cat-file" for blobs obey "--textconv" options > (with the former defaulting to "--textconv" and the latter to > "--no-textconv") whereas "show" does not obey this option, even though > it takes diff options. > > Make "show" on blobs behave like "diff", i.e. obey "--textconv" by > default and "--no-textconv" when given. 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"? I'm not convinced this is the right thing to do, though. It would break: git show HEAD:file.c >file.c Admittedly, such people should be using "checkout" or "cat-file", so I do not mind too much breaking them if there is a good reason. But I am not sure what that reason is. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
