Re: [PATCHv2 1/5] revision.h: turn rev_info.early_output back into an unsigned int
On Mon, Jun 12, 2017 at 05:59:02PM -0400, Jeff King wrote: > The nice thing about deprecating it is that I think callers need to be > prepared to handle the case already that it does nothing. So if we just > ripped out the code and treated it as a silent noop, everything would > just work. Actually, I take that back. We might or might not have early output, but we always say "Final output". So in theory a caller would be confused if we no longer print that. (I do still suspect that there are no callers). -Peff
Re: [PATCHv2 1/5] revision.h: turn rev_info.early_output back into an unsigned int
On Mon, Jun 12, 2017 at 01:36:08PM -0700, Junio C Hamano wrote: > > I'm not sure if I was just being thick or if that point (and the fact > > that --early-output has basically been a noop since 2011!) should be > > made more explicit. > > > > Given that nobody noticed, I kind of wonder if we should consider > > ripping the feature out entirely. > > Yes, we may want to think about deprecating it, especially given > that it is not advertised anywhere. > > In any case, the patch looks correct ;-) Yeah, it's definitely orthogonal to Gábor's patches. I also wondered who might be using the feature. I assumed it was written with gitk in mind. Digging in the list archive that seems to be the case, and there was even an RFC patch for gitk to use it: http://public-inbox.org/git/18221.2285.259487.655...@cargo.ozlabs.ibm.com/ but AFAICT it was never merged. It looks like QGit had a patch around that time, too, but left the line using "--early-output" commented out until Git merged the patches. And then it never got uncommented. ;) Tig doesn't seem to use it at all. I don't know about other GUIs. I'd kind of doubt, given the obscurity of the feature (both gitk and qgit authors were involved in the discussion way back in 2007). The nice thing about deprecating it is that I think callers need to be prepared to handle the case already that it does nothing. So if we just ripped out the code and treated it as a silent noop, everything would just work. -Peff
Re: [PATCHv2 1/5] revision.h: turn rev_info.early_output back into an unsigned int
Jeff King writes: > On Fri, Jun 09, 2017 at 08:17:29PM +0200, SZEDER Gábor wrote: > >> rev_info.early_output started out as an unsigned int in cdcefbc97 (Add >> "--early-output" log flag for interactive GUI use, 2007-11-03), but >> later it was turned into a single bit in a bit field in cc243c3ce >> (show: --ignore-missing, 2011-05-18) without explanation, though its >> users still expect it to be a regular integer type. Consequently, any >> even number given via '--early-output=' effectively disabled the >> feature. >> >> Turn it back into an unsigned int, restoring its original data type. > > This confused me for a moment, as on my first read it seems like the > obvious solution is to normalize the input to a bit-field, like: > > revs->early_output = !!atoi(optarg); > > But the "users still expect" bit was a bit subtle to me, as I thought > you meant users of Git. But you mean that the feature itself is not a > boolean, but rather an integer count of how much early output to show. > > I'm not sure if I was just being thick or if that point (and the fact > that --early-output has basically been a noop since 2011!) should be > made more explicit. > > Given that nobody noticed, I kind of wonder if we should consider > ripping the feature out entirely. Yes, we may want to think about deprecating it, especially given that it is not advertised anywhere. In any case, the patch looks correct ;-)
Re: [PATCHv2 1/5] revision.h: turn rev_info.early_output back into an unsigned int
On Fri, Jun 09, 2017 at 08:17:29PM +0200, SZEDER Gábor wrote: > rev_info.early_output started out as an unsigned int in cdcefbc97 (Add > "--early-output" log flag for interactive GUI use, 2007-11-03), but > later it was turned into a single bit in a bit field in cc243c3ce > (show: --ignore-missing, 2011-05-18) without explanation, though its > users still expect it to be a regular integer type. Consequently, any > even number given via '--early-output=' effectively disabled the > feature. > > Turn it back into an unsigned int, restoring its original data type. This confused me for a moment, as on my first read it seems like the obvious solution is to normalize the input to a bit-field, like: revs->early_output = !!atoi(optarg); But the "users still expect" bit was a bit subtle to me, as I thought you meant users of Git. But you mean that the feature itself is not a boolean, but rather an integer count of how much early output to show. I'm not sure if I was just being thick or if that point (and the fact that --early-output has basically been a noop since 2011!) should be made more explicit. Given that nobody noticed, I kind of wonder if we should consider ripping the feature out entirely. -Peff
