On Mon, Nov 12, 2018 at 7:21 AM Junio C Hamano <gits...@pobox.com> wrote:
>
> Nguyễn Thái Ngọc Duy <pclo...@gmail.com> 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
