Re: [PATCH/RFC] checkout: print something when checking out paths
Duy Nguyen writes:
>> > I see this at the same level as "Switched to branch 'foo'" which is
>> > also protected by opts->quiet. If we start hiding messages based on
>> > tty it should be done for other messages in update_refs_for_switch()
>> > too, I guess?
>
> No let's drop this for now. I promise to come back with something
> better (at least it still sounds better in my mind). If that idea does
> not work out, we can come back and see if we can improve this.
Let's leave it in 'pu'.
I do agree that this is similar to existing messages that talk about
checkout out a branch to work on etc., and I think giving feedback
when checkout paths out _is_ a good thing to do for interactive
users.
If we were to squelch such "notice" output for non-interactive use,
we should do so to the "notice" messages for checking out a branch,
as well, and also to other subcommands that report what they did
with these "notice" output. And that is a separate topic.
The primary reason why I was annoyed was because "make test" (I
think I have DEFAULT_TEST_TARGET=prove, if it matters) output was
littered with these "checked out N paths", even though I am not
asking for verbose output.
It could be that the real cause of that is perhaps because we have
too many "git checkout" that is outside test_expect_* block, in
which case we should fix these tests to perform the checkout inside
a test_expect_success block for test set-up, and my annoyance was
only shooting at the messenger.
For example, the attached patch illustrates the right problem but
addresses it in a wrong way. This checkout_files() helper does too
many things outside (and before) the test_expect_success block it
has (other helpers like commit_chk_wrnNNO share the same problem),
and making "git checkout" noisy will reveal that as a problem by
leaking the noisy output directly to the tester. But the real fix
is to enclose the set-up step inside a test_expect_success block,
which is not done by the following illustration patch, which instead
just squelches the message.
t/t0027-auto-crlf.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh
index beb5927f77..3587e454f1 100755
--- a/t/t0027-auto-crlf.sh
+++ b/t/t0027-auto-crlf.sh
@@ -293,9 +293,9 @@ checkout_files () {
do
rm crlf_false_attr__$f.txt &&
if test -z "$ceol"; then
- git checkout crlf_false_attr__$f.txt
+ git checkout -- crlf_false_attr__$f.txt
else
- git -c core.eol=$ceol checkout crlf_false_attr__$f.txt
+ git -c core.eol=$ceol checkout --
crlf_false_attr__$f.txt
fi
done
Re: [PATCH/RFC] checkout: print something when checking out paths
On Mon, Nov 19, 2018 at 2:08 PM Ævar Arnfjörð Bjarmason wrote: > > > On Mon, Nov 12 2018, Duy Nguyen wrote: > > > On Mon, Nov 12, 2018 at 7:21 AM Junio C Hamano wrote: > >> > >> Nguyễn Thái Ngọc Duy writes: > >> > >> > Since the purpose of printing this is to help disambiguate. Only do it > >> > when "--" is missing (the actual reason though is many tests check > >> > empty stderr to see that no error is raised and I'm too lazy to fix > >> > all the test cases). > >> > >> Heh, honesty is good but in this particular case the official reason > >> alone would make perfect sense, too ;-) > >> > >> As with progress output, shouldn't this automatically be turned off > >> when the standard error stream goes to non tty, as the real purpose > >> of printing is to help the user sitting in front of the terminal and > >> interacting with the command? > > > > I see this at the same level as "Switched to branch 'foo'" which is > > also protected by opts->quiet. If we start hiding messages based on > > tty it should be done for other messages in update_refs_for_switch() > > too, I guess? > > I have no real opinion either way, but whatever we can do about > "checkout" being so confusing since it does so many things is most > welcome. > > Just an alternative: Maybe we can start this out as advice() output > that's either opt-in via config (not on by default) to start with, or > have some advice_tty() that only emits it in the same circumstances we > emit the progress output? No let's drop this for now. I promise to come back with something better (at least it still sounds better in my mind). If that idea does not work out, we can come back and see if we can improve this. -- Duy
Re: [PATCH/RFC] checkout: print something when checking out paths
On Mon, Nov 12 2018, Duy Nguyen wrote: > On Mon, Nov 12, 2018 at 7:21 AM Junio C Hamano wrote: >> >> Nguyễn Thái Ngọc Duy writes: >> >> > Since the purpose of printing this is to help disambiguate. Only do it >> > when "--" is missing (the actual reason though is many tests check >> > empty stderr to see that no error is raised and I'm too lazy to fix >> > all the test cases). >> >> Heh, honesty is good but in this particular case the official reason >> alone would make perfect sense, too ;-) >> >> As with progress output, shouldn't this automatically be turned off >> when the standard error stream goes to non tty, as the real purpose >> of printing is to help the user sitting in front of the terminal and >> interacting with the command? > > I see this at the same level as "Switched to branch 'foo'" which is > also protected by opts->quiet. If we start hiding messages based on > tty it should be done for other messages in update_refs_for_switch() > too, I guess? I have no real opinion either way, but whatever we can do about "checkout" being so confusing since it does so many things is most welcome. Just an alternative: Maybe we can start this out as advice() output that's either opt-in via config (not on by default) to start with, or have some advice_tty() that only emits it in the same circumstances we emit the progress output?
Re: [PATCH/RFC] checkout: print something when checking out paths
Duy Nguyen writes: > Another thing I'm going to change (if this v1 is in the right > direction): print different messages for "git checkout -- foo" and > "git checkout foo -- bar". The first one is "%d paths have been > reverted" but the second one does different things and probably should > say "%d paths have been updated from branch foo" or something like > that. I actually think that it is a bad idea to deliberately use such misleading words like "revert", that will discourage users from weaning themselves off of SVN. "checked out N paths out of the index", "checked out N paths out of the commit X" and "checked out N paths out of branch B" would be clear enough and won't have such a problem---otherwise you are encouraging them to make a mistake echo garbage >path ... say oops ... git revert path
Re: [PATCH/RFC] checkout: print something when checking out paths
On Mon, Nov 12, 2018 at 7:21 AM Junio C Hamano wrote:
>
> Nguyễn Thái Ngọc Duy writes:
>
> > Since the purpose of printing this is to help disambiguate. Only do it
> > when "--" is missing (the actual reason though is many tests check
> > empty stderr to see that no error is raised and I'm too lazy to fix
> > all the test cases).
>
> Heh, honesty is good but in this particular case the official reason
> alone would make perfect sense, too ;-)
>
> As with progress output, shouldn't this automatically be turned off
> when the standard error stream goes to non tty, as the real purpose
> of printing is to help the user sitting in front of the terminal and
> interacting with the command?
I see this at the same level as "Switched to branch 'foo'" which is
also protected by opts->quiet. If we start hiding messages based on
tty it should be done for other messages in update_refs_for_switch()
too, I guess?
> And by this, I do not mean to say that "When -- is missing" can go
> away. I agree that "--" is a clear sign that the user knows what
> s/he is doing---to overwrite the paths in the working tree that
> match the pathspec.
My other problem with deciding to print based on the presence of "--"
is also with branch switching code: it always prints something (unless
it actually does nothing). So it's a bit strange that the
checkout_paths() behaves slightly different based on "--".
> > @@ -371,17 +374,27 @@ static int checkout_paths(const struct checkout_opts
> > *opts,
> > struct cache_entry *ce = active_cache[pos];
> > if (ce->ce_flags & CE_MATCHED) {
> > if (!ce_stage(ce)) {
> > - errs |= checkout_entry(ce, &state, NULL);
> > + errs |= checkout_entry(ce, &state,
> > +NULL, &nr_checkouts);
> > continue;
>
> As we count inside checkout_entry(), we might not actually write
> this out when we know that the working tree file is up to date
> already but we do not increment in that case either, so we keep
> track of the accurate count of "updates", not path matches (i.e. we
> are not reporting "we made sure this many paths are up to date in
> the working tree"; instead we are reporting how many paths we needed
> to overwrite in the working tree).
Yeah. I counted matched paths first, but when you do "git co .", you
get a huge (and meaningless, to me) number. Printing how many files
are updated makes more sense.
> > pos = skip_same_name(ce, pos) - 1;
> > }
> > }
> > - errs |= finish_delayed_checkout(&state);
> > + errs |= finish_delayed_checkout(&state, &nr_checkouts);
> > +
> > + if (opts->count_checkout_paths)
> > + fprintf_ln(stderr, Q_("%d path has been updated",
> > + "%d paths have been updated",
> > + nr_checkouts),
> > +nr_checkouts);
>
> This one may want to be protected behind isatty(2). Or the default
> value of count_checkout_paths could be tweakedbased on isatty(2).
Another thing I'm going to change (if this v1 is in the right
direction): print different messages for "git checkout -- foo" and
"git checkout foo -- bar". The first one is "%d paths have been
reverted" but the second one does different things and probably should
say "%d paths have been updated from branch foo" or something like
that.
--
Duy
Re: [PATCH/RFC] checkout: print something when checking out paths
Nguyễn Thái Ngọc Duy writes:
> Since the purpose of printing this is to help disambiguate. Only do it
> when "--" is missing (the actual reason though is many tests check
> empty stderr to see that no error is raised and I'm too lazy to fix
> all the test cases).
Heh, honesty is good but in this particular case the official reason
alone would make perfect sense, too ;-)
As with progress output, shouldn't this automatically be turned off
when the standard error stream goes to non tty, as the real purpose
of printing is to help the user sitting in front of the terminal and
interacting with the command?
And by this, I do not mean to say that "When -- is missing" can go
away. I agree that "--" is a clear sign that the user knows what
s/he is doing---to overwrite the paths in the working tree that
match the pathspec.
> @@ -371,17 +374,27 @@ static int checkout_paths(const struct checkout_opts
> *opts,
> struct cache_entry *ce = active_cache[pos];
> if (ce->ce_flags & CE_MATCHED) {
> if (!ce_stage(ce)) {
> - errs |= checkout_entry(ce, &state, NULL);
> + errs |= checkout_entry(ce, &state,
> +NULL, &nr_checkouts);
> continue;
As we count inside checkout_entry(), we might not actually write
this out when we know that the working tree file is up to date
already but we do not increment in that case either, so we keep
track of the accurate count of "updates", not path matches (i.e. we
are not reporting "we made sure this many paths are up to date in
the working tree"; instead we are reporting how many paths we needed
to overwrite in the working tree).
> }
> if (opts->writeout_stage)
> - errs |= checkout_stage(opts->writeout_stage,
> ce, pos, &state);
> + errs |= checkout_stage(opts->writeout_stage,
> +ce, pos,
> +&state, &nr_checkouts);
Likewike.
> else if (opts->merge)
> - errs |= checkout_merged(pos, &state);
> + errs |= checkout_merged(pos, &state,
> + &nr_checkouts);
Likewise.
> pos = skip_same_name(ce, pos) - 1;
> }
> }
> - errs |= finish_delayed_checkout(&state);
> + errs |= finish_delayed_checkout(&state, &nr_checkouts);
> +
> + if (opts->count_checkout_paths)
> + fprintf_ln(stderr, Q_("%d path has been updated",
> + "%d paths have been updated",
> + nr_checkouts),
> +nr_checkouts);
This one may want to be protected behind isatty(2). Or the default
value of count_checkout_paths could be tweakedbased on isatty(2).
> @@ -1064,6 +1077,7 @@ static int parse_branchname_arg(int argc, const char
> **argv,
> has_dash_dash = 1; /* case (3) or (1) */
> else if (dash_dash_pos >= 2)
> die(_("only one reference expected, %d given."), dash_dash_pos);
> + opts->count_checkout_paths = !opts->quiet && !has_dash_dash;
i.e. "&& isatty(2)" as well.
Of course, "--[no-]count-paths" could be added to override this.
