[PATCH 0/2] checkout: added two options to control progress output

[PATCH 0/2] checkout: added two options to control progress output

[Edmundo Carmona]

From: Edmundo Carmona Antoranz <eantoranz@gmail.com>

checkout will refuse to print progress information if it's not connected
to a TTY. These patches will add two options:

--progress-no-tty: enable printing progress info even if not using a TTY
--progress-lf: print progress information using LFs instead of CRs

If none of these options is used, default behavior would be used to avoid
breaking current use cases

Edmundo Carmona Antoranz (2):
  checkout: progress on non-tty. progress with lf
  checkout: adjust documentation to the two new options

 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(-)

-- 
2.6.1

Re: [PATCH 0/2] checkout: added two options to control progress output

[Jeff King]

On Sat, Oct 24, 2015 at 08:59:28AM -0600, Edmundo Carmona wrote:

> From: Edmundo Carmona Antoranz <eantoranz@gmail.com>
> 
> 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 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?

> 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

Re: [PATCH 0/2] checkout: added two options to control progress output

[Edmundo Carmona Antoranz]

On Thu, Oct 29, 2015 at 4:05 PM, Jeff King <peff@peff.net> wrote:
> On Sat, Oct 24, 2015 at 08:59:28AM -0600, Edmundo Carmona wrote:
>
>> From: Edmundo Carmona Antoranz <eantoranz@gmail.com>
>>
>> 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!

Re: [PATCH 0/2] checkout: added two options to control progress output

[Jeff King]

On Thu, Oct 29, 2015 at 06:09:06PM -0600, Edmundo Carmona Antoranz wrote:

> A comment there: I think more builtins support --progress than the ones that
> support --no-progress, right?

Hopefully they are supported equally everywhere. Anybody using parseopt
should have something like (this is from builtin/clone.c):

  OPT_BOOL(0, "progress", &option_progress,
	   N_("force progress reporting")),

That automatically gives us both --progress and --no-progress. And note
how option_progress is initialized to "-1" above that, so we know the
resulting value it stores will either be 1 (--progress),
0 (--no-progress), or -1 (no option specified).

-Peff

Re: [PATCH 0/2] checkout: added two options to control progress output

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> --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).

Yes, and as I doubted its validity, I would really have preferred to
see it done as a later step.  The "we want to say --[no-]progress"
one on the other hand looked a very sensible change, and it was sad
to see it taken hostage by that -lf thing.

Thanks.