Re: [PATCH/RFC] checkout: print something when checking out paths

From: Duy Nguyen <pclouds@gmail.com>
Date: Nov 12, 2018, 16:27 UTC
Message-ID: <CACsJy8BGgf0J=iKNc3qmz_rTMNdaPmR_1v+9i3nhGKcuOH4AFA@mail.gmail.com>
In-Reply-To: <xmqq8t1y3jex.fsf@gitster-ct.c.googlers.com>

On Mon, Nov 12, 2018 at 7:21 AM Junio C Hamano <gitster@pobox.com> wrote:

Show 15 quoted lines

>
> Nguyễn Thái Ngọc Duy  <pclouds@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 "--".

Show 16 quoted lines

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

Show 14 quoted lines

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