On Thu, Oct 29, 2015 at 4:05 PM, Jeff King <peff@xxxxxxxx> wrote: > On Sat, Oct 24, 2015 at 08:59:28AM -0600, Edmundo Carmona wrote: > >> From: Edmundo Carmona Antoranz <eantoranz@xxxxxxxxx> >> >> checkout will refuse to print progress information if it's not connected >> to a TTY. These patches will add two options: > > Not just checkout, but all of git's progress code. The usual rules are: > > - if the user told us --progress, show progress > > - if the user told us --no-progress, don't show progress > > - if neither is set, guess based on isatty(2) > > So with that in mind... > >> --progress-no-tty: enable printing progress info even if not using a TTY > > This should just be "--progress", shouldn't it? It sure does! A comment there: I think more builtins support --progress than the ones that support --no-progress, right? > > It looks like checkout does not understand --progress and --no-progress. > It does have "--quiet", but elsewhere we usually use that to mean "no > progress, but also no other informational messages". We should probably > make this consistent with other commands. See how builtin/clone.c does > this, for example. Note also that clone's "quiet" option is part of > OPT__VERBOSITY(), which provides both "-q" and "-v" to turn the > verbosity level up/down. We could switch checkout to use that, too, but > I do not think it buys us anything, as we have no "-v" output for > checkout yet. > >> --progress-lf: print progress information using LFs instead of CRs > > I notice this is part of your patch 1, but it really seems orthogonal to > checkout's --progress option. It should probably be a separate patch, > and it probably needs some justification in the commit message (i.e., > explain not just _what_ you are doing in the patch, but _why_ it is > useful). > > It also seems like this has nothing to do with checkout in particular. > Should it be triggered by an environment variable or by an option to the > main git binary? E.g.: > > git --progress-lf checkout ... > > to affect the progress meter for all commands? I think it would be worth it but, given that this is a more "global" adjustment ( as in, for the whole progress and not just checkout), I think it's better I separate it from the "--progress for checkout" patch cause right now checkout is missing the --progress option that many other builtins already support.... say, for standardization's sake. > >> Edmundo Carmona Antoranz (2): >> checkout: progress on non-tty. progress with lf >> checkout: adjust documentation to the two new options > > I mentioned above that the two orthogonal features should each get their > own patches. I think you should also do the reverse with the > documentation: include it along with the implementation of the feature. > Sometimes we do documentation as a separate patch (especially if it is > large, or if the feature itself took many patches to complete). But for > a small feature, as a reviewer (and when looking back through history) I > usually find it simpler if the documentation is included in the same > commit. > >> Documentation/git-checkout.txt | 10 ++++++++++ >> builtin/checkout.c | 12 ++++++++++-- >> progress.c | 8 +++++++- >> progress.h | 1 + >> unpack-trees.c | 3 +++ >> unpack-trees.h | 2 ++ >> 6 files changed, 33 insertions(+), 3 deletions(-) > > I didn't look too carefully at the patches themselves, as they would > need to be reworked substantially to follow the suggestions above. But I > didn't notice any style or patch-formatting problems, and you seem to > generally be touching the right areas for what you want to do. > > -Peff It's ok. I knew I would have to rework them based on feedback from the list. Thank you very much! -- 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