{"thread":{"id":"40630","subject":"[PATCH 0/2] checkout: added two options to control progress output","startedAt":"2015-10-24T14:59:28Z","lastAt":"2015-10-30T16:52:20Z","messageCount":5,"participants":["Edmundo Carmona","Jeff King","Edmundo Carmona Antoranz","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"272168","messageId":"1445698768-22614-1-git-send-email-eantoranz@gmail.com","threadId":"40630","inReplyTo":null,"subject":"[PATCH 0/2] checkout: added two options to control progress output","fromName":"Edmundo Carmona","fromEmail":"eantoranz@gmail.com","sentAt":"2015-10-24T14:59:28Z","receivedAt":"2015-10-24T14:59:28Z","isPatch":true,"sender":{"key":"eantoranz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1491018?v=4"},"body":"From: Edmundo Carmona Antoranz \n\ncheckout will refuse to print progress information if it's not connected\nto a TTY. These patches will add two options:\n\n--progress-no-tty: enable printing progress info even if not using a TTY\n--progress-lf: print progress information using LFs instead of CRs\n\nIf none of these options is used, default behavior would be used to avoid\nbreaking current use cases\n\nEdmundo Carmona Antoranz (2):\n checkout: progress on non-tty. progress with lf\n checkout: adjust documentation to the two new options\n\n Documentation/git-checkout.txt | 10 ++++++++++\n builtin/checkout.c | 12 ++++++++++--\n progress.c | 8 +++++++-\n progress.h | 1 +\n unpack-trees.c | 3 +++\n unpack-trees.h | 2 ++\n 6 files changed, 33 insertions(+), 3 deletions(-)\n\n-- \n2.6.1\n"},{"id":"272499","messageId":"20151029220519.GA466@sigill.intra.peff.net","threadId":"40630","inReplyTo":"1445698768-22614-1-git-send-email-eantoranz@gmail.com","subject":"Re: [PATCH 0/2] checkout: added two options to control progress output","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-10-29T22:05:19Z","receivedAt":"2015-10-29T22:05:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Oct 24, 2015 at 08:59:28AM -0600, Edmundo Carmona wrote:\n\n> From: Edmundo Carmona Antoranz \n> \n> checkout will refuse to print progress information if it's not connected\n> to a TTY. These patches will add two options:\n\nNot just checkout, but all of git's progress code. The usual rules are:\n\n - if the user told us --progress, show progress\n\n - if the user told us --no-progress, don't show progress\n\n - if neither is set, guess based on isatty(2)\n\nSo with that in mind...\n\n> --progress-no-tty: enable printing progress info even if not using a TTY\n\nThis should just be \"--progress\", shouldn't it?\n\nIt looks like checkout does not understand --progress and --no-progress.\nIt does have \"--quiet\", but elsewhere we usually use that to mean \"no\nprogress, but also no other informational messages\". We should probably\nmake this consistent with other commands. See how builtin/clone.c does\nthis, for example. Note also that clone's \"quiet\" option is part of\nOPT__VERBOSITY(), which provides both \"-q\" and \"-v\" to turn the\nverbosity level up/down. We could switch checkout to use that, too, but\nI do not think it buys us anything, as we have no \"-v\" output for\ncheckout yet.\n\n> --progress-lf: print progress information using LFs instead of CRs\n\nI notice this is part of your patch 1, but it really seems orthogonal to\ncheckout's --progress option. It should probably be a separate patch,\nand it probably needs some justification in the commit message (i.e.,\nexplain not just _what_ you are doing in the patch, but _why_ it is\nuseful).\n\nIt also seems like this has nothing to do with checkout in particular.\nShould it be triggered by an environment variable or by an option to the\nmain git binary? E.g.:\n\n git --progress-lf checkout ...\n\nto affect the progress meter for all commands?\n\n> Edmundo Carmona Antoranz (2):\n> checkout: progress on non-tty. progress with lf\n> checkout: adjust documentation to the two new options\n\nI mentioned above that the two orthogonal features should each get their\nown patches. I think you should also do the reverse with the\ndocumentation: include it along with the implementation of the feature.\nSometimes we do documentation as a separate patch (especially if it is\nlarge, or if the feature itself took many patches to complete). But for\na small feature, as a reviewer (and when looking back through history) I\nusually find it simpler if the documentation is included in the same\ncommit.\n\n> Documentation/git-checkout.txt | 10 ++++++++++\n> builtin/checkout.c | 12 ++++++++++--\n> progress.c | 8 +++++++-\n> progress.h | 1 +\n> unpack-trees.c | 3 +++\n> unpack-trees.h | 2 ++\n> 6 files changed, 33 insertions(+), 3 deletions(-)\n\nI didn't look too carefully at the patches themselves, as they would\nneed to be reworked substantially to follow the suggestions above. But I\ndidn't notice any style or patch-formatting problems, and you seem to\ngenerally be touching the right areas for what you want to do.\n\n-Peff\n"},{"id":"272502","messageId":"CAOc6etakOy_zZ3fH_vS5UGGqouXTd7SJFtD6UC9bZRJaYWsRQw@mail.gmail.com","threadId":"40630","inReplyTo":"20151029220519.GA466@sigill.intra.peff.net","subject":"Re: [PATCH 0/2] checkout: added two options to control progress output","fromName":"Edmundo Carmona Antoranz","fromEmail":"eantoranz@gmail.com","sentAt":"2015-10-30T00:09:06Z","receivedAt":"2015-10-30T00:09:06Z","isPatch":true,"sender":{"key":"eantoranz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1491018?v=4"},"body":"On Thu, Oct 29, 2015 at 4:05 PM, Jeff King wrote:\n> On Sat, Oct 24, 2015 at 08:59:28AM -0600, Edmundo Carmona wrote:\n>\n>> From: Edmundo Carmona Antoranz \n>>\n>> checkout will refuse to print progress information if it's not connected\n>> to a TTY. These patches will add two options:\n>\n> Not just checkout, but all of git's progress code. The usual rules are:\n>\n> - if the user told us --progress, show progress\n>\n> - if the user told us --no-progress, don't show progress\n>\n> - if neither is set, guess based on isatty(2)\n>\n> So with that in mind...\n>\n>> --progress-no-tty: enable printing progress info even if not using a TTY\n>\n> This should just be \"--progress\", shouldn't it?\n\nIt sure does!\n\nA comment there: I think more builtins support --progress than the ones that\nsupport --no-progress, right?\n\n>\n> It looks like checkout does not understand --progress and --no-progress.\n> It does have \"--quiet\", but elsewhere we usually use that to mean \"no\n> progress, but also no other informational messages\". We should probably\n> make this consistent with other commands. See how builtin/clone.c does\n> this, for example. Note also that clone's \"quiet\" option is part of\n> OPT__VERBOSITY(), which provides both \"-q\" and \"-v\" to turn the\n> verbosity level up/down. We could switch checkout to use that, too, but\n> I do not think it buys us anything, as we have no \"-v\" output for\n> checkout yet.\n>\n>> --progress-lf: print progress information using LFs instead of CRs\n>\n> I notice this is part of your patch 1, but it really seems orthogonal to\n> checkout's --progress option. It should probably be a separate patch,\n> and it probably needs some justification in the commit message (i.e.,\n> explain not just _what_ you are doing in the patch, but _why_ it is\n> useful).\n>\n> It also seems like this has nothing to do with checkout in particular.\n> Should it be triggered by an environment variable or by an option to the\n> main git binary? E.g.:\n>\n> git --progress-lf checkout ...\n>\n> to affect the progress meter for all commands?\n\nI think it would be worth it but, given that this is a more \"global\"\nadjustment (\nas in, for the whole progress and not just checkout), I think it's better I\nseparate it from the \"--progress for checkout\" patch cause right now checkout\nis missing the --progress option that many other builtins already support....\nsay, for standardization's sake.\n\n>\n>> Edmundo Carmona Antoranz (2):\n>> checkout: progress on non-tty. progress with lf\n>> checkout: adjust documentation to the two new options\n>\n> I mentioned above that the two orthogonal features should each get their\n> own patches. I think you should also do the reverse with the\n> documentation: include it along with the implementation of the feature.\n> Sometimes we do documentation as a separate patch (especially if it is\n> large, or if the feature itself took many patches to complete). But for\n> a small feature, as a reviewer (and when looking back through history) I\n> usually find it simpler if the documentation is included in the same\n> commit.\n>\n>> Documentation/git-checkout.txt | 10 ++++++++++\n>> builtin/checkout.c | 12 ++++++++++--\n>> progress.c | 8 +++++++-\n>> progress.h | 1 +\n>> unpack-trees.c | 3 +++\n>> unpack-trees.h | 2 ++\n>> 6 files changed, 33 insertions(+), 3 deletions(-)\n>\n> I didn't look too carefully at the patches themselves, as they would\n> need to be reworked substantially to follow the suggestions above. But I\n> didn't notice any style or patch-formatting problems, and you seem to\n> generally be touching the right areas for what you want to do.\n>\n> -Peff\n\nIt's ok. I knew I would have to rework them based on feedback from the list.\n\nThank you very much!\n"},{"id":"272504","messageId":"20151030001404.GB2123@sigill.intra.peff.net","threadId":"40630","inReplyTo":"CAOc6etakOy_zZ3fH_vS5UGGqouXTd7SJFtD6UC9bZRJaYWsRQw@mail.gmail.com","subject":"Re: [PATCH 0/2] checkout: added two options to control progress output","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-10-30T00:14:04Z","receivedAt":"2015-10-30T00:14:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 29, 2015 at 06:09:06PM -0600, Edmundo Carmona Antoranz wrote:\n\n> A comment there: I think more builtins support --progress than the ones that\n> support --no-progress, right?\n\nHopefully they are supported equally everywhere. Anybody using parseopt\nshould have something like (this is from builtin/clone.c):\n\n OPT_BOOL(0, \"progress\", &option_progress,\n\t N_(\"force progress reporting\")),\n\nThat automatically gives us both --progress and --no-progress. And note\nhow option_progress is initialized to \"-1\" above that, so we know the\nresulting value it stores will either be 1 (--progress),\n0 (--no-progress), or -1 (no option specified).\n\n-Peff\n"},{"id":"272532","messageId":"xmqqa8r0mh8b.fsf@gitster.mtv.corp.google.com","threadId":"40630","inReplyTo":"20151029220519.GA466@sigill.intra.peff.net","subject":"Re: [PATCH 0/2] checkout: added two options to control progress output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-30T16:52:20Z","receivedAt":"2015-10-30T16:52:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> --progress-lf: print progress information using LFs instead of CRs\n>\n> I notice this is part of your patch 1, but it really seems orthogonal to\n> checkout's --progress option. It should probably be a separate patch,\n> and it probably needs some justification in the commit message (i.e.,\n> explain not just _what_ you are doing in the patch, but _why_ it is\n> useful).\n\nYes, and as I doubted its validity, I would really have preferred to\nsee it done as a later step. The \"we want to say --[no-]progress\"\none on the other hand looked a very sensible change, and it was sad\nto see it taken hostage by that -lf thing.\n\nThanks.\n"}]}