| Message ID | 20181110133525.17538-1-pclouds@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PATCH/RFC] checkout: print something when checking out paths | expand |
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? 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.
On Mon, Nov 12, 2018 at 7:21 AM Junio C Hamano <gitster@pobox.com> wrote: > > 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 "--". > > @@ -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 Nguyen <pclouds@gmail.com> 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
On Mon, Nov 12 2018, Duy Nguyen wrote: > On Mon, Nov 12, 2018 at 7:21 AM Junio C Hamano <gitster@pobox.com> wrote: >> >> 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? 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?
On Mon, Nov 19, 2018 at 2:08 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > > > On Mon, Nov 12 2018, Duy Nguyen wrote: > > > On Mon, Nov 12, 2018 at 7:21 AM Junio C Hamano <gitster@pobox.com> wrote: > >> > >> 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? > > 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 Nguyen <pclouds@gmail.com> 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
diff --git a/apply.c b/apply.c index 073d5f0451..5876b02197 100644 --- a/apply.c +++ b/apply.c @@ -3352,7 +3352,8 @@ static int checkout_target(struct index_state *istate, costate.refresh_cache = 1; costate.istate = istate; - if (checkout_entry(ce, &costate, NULL) || lstat(ce->name, st)) + if (checkout_entry(ce, &costate, NULL, NULL) || + lstat(ce->name, st)) return error(_("cannot checkout %s"), ce->name); return 0; } diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c index 88b86c8d9f..bada491f58 100644 --- a/builtin/checkout-index.c +++ b/builtin/checkout-index.c @@ -67,7 +67,8 @@ static int checkout_file(const char *name, const char *prefix) continue; did_checkout = 1; if (checkout_entry(ce, &state, - to_tempfile ? topath[ce_stage(ce)] : NULL) < 0) + to_tempfile ? topath[ce_stage(ce)] : NULL, + NULL) < 0) errs++; } @@ -111,7 +112,8 @@ static void checkout_all(const char *prefix, int prefix_length) write_tempfile_record(last_ce->name, prefix); } if (checkout_entry(ce, &state, - to_tempfile ? topath[ce_stage(ce)] : NULL) < 0) + to_tempfile ? topath[ce_stage(ce)] : NULL, + NULL) < 0) errs++; last_ce = ce; } diff --git a/builtin/checkout.c b/builtin/checkout.c index acdafc6e4c..13ed041dc1 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -44,6 +44,7 @@ struct checkout_opts { int ignore_skipworktree; int ignore_other_worktrees; int show_progress; + int count_checkout_paths; /* * If new checkout options are added, skip_merge_working_tree * should be updated accordingly. @@ -165,12 +166,13 @@ static int check_stages(unsigned stages, const struct cache_entry *ce, int pos) } static int checkout_stage(int stage, const struct cache_entry *ce, int pos, - const struct checkout *state) + const struct checkout *state, int *nr_checkouts) { while (pos < active_nr && !strcmp(active_cache[pos]->name, ce->name)) { if (ce_stage(active_cache[pos]) == stage) - return checkout_entry(active_cache[pos], state, NULL); + return checkout_entry(active_cache[pos], state, + NULL, nr_checkouts); pos++; } if (stage == 2) @@ -179,7 +181,7 @@ static int checkout_stage(int stage, const struct cache_entry *ce, int pos, return error(_("path '%s' does not have their version"), ce->name); } -static int checkout_merged(int pos, const struct checkout *state) +static int checkout_merged(int pos, const struct checkout *state, int *nr_checkouts) { struct cache_entry *ce = active_cache[pos]; const char *path = ce->name; @@ -242,7 +244,7 @@ static int checkout_merged(int pos, const struct checkout *state) ce = make_transient_cache_entry(mode, &oid, path, 2); if (!ce) die(_("make_cache_entry failed for path '%s'"), path); - status = checkout_entry(ce, state, NULL); + status = checkout_entry(ce, state, NULL, nr_checkouts); discard_cache_entry(ce); return status; } @@ -257,6 +259,7 @@ static int checkout_paths(const struct checkout_opts *opts, struct commit *head; int errs = 0; struct lock_file lock_file = LOCK_INIT; + int nr_checkouts = 0; if (opts->track != BRANCH_TRACK_UNSPECIFIED) die(_("'%s' cannot be used with updating paths"), "--track"); @@ -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; } if (opts->writeout_stage) - errs |= checkout_stage(opts->writeout_stage, ce, pos, &state); + errs |= checkout_stage(opts->writeout_stage, + ce, pos, + &state, &nr_checkouts); else if (opts->merge) - errs |= checkout_merged(pos, &state); + errs |= checkout_merged(pos, &state, + &nr_checkouts); 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); if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK)) die(_("unable to write new index file")); @@ -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; if (!strcmp(arg, "-")) arg = "@{-1}"; diff --git a/builtin/difftool.c b/builtin/difftool.c index 544b0e8639..71318c26e1 100644 --- a/builtin/difftool.c +++ b/builtin/difftool.c @@ -323,7 +323,7 @@ static int checkout_path(unsigned mode, struct object_id *oid, int ret; ce = make_transient_cache_entry(mode, oid, path, 0); - ret = checkout_entry(ce, state, NULL); + ret = checkout_entry(ce, state, NULL, NULL); discard_cache_entry(ce); return ret; diff --git a/cache.h b/cache.h index 8b1ee42ae9..52fb6ba148 100644 --- a/cache.h +++ b/cache.h @@ -1540,9 +1540,9 @@ struct checkout { #define CHECKOUT_INIT { NULL, "" } #define TEMPORARY_FILENAME_LENGTH 25 -extern int checkout_entry(struct cache_entry *ce, const struct checkout *state, char *topath); +extern int checkout_entry(struct cache_entry *ce, const struct checkout *state, char *topath, int *nr_checkouts); extern void enable_delayed_checkout(struct checkout *state); -extern int finish_delayed_checkout(struct checkout *state); +extern int finish_delayed_checkout(struct checkout *state, int *nr_checkouts); struct cache_def { struct strbuf path; diff --git a/entry.c b/entry.c index 5d136c5d55..5f213c30fe 100644 --- a/entry.c +++ b/entry.c @@ -161,7 +161,7 @@ static int remove_available_paths(struct string_list_item *item, void *cb_data) return !available; } -int finish_delayed_checkout(struct checkout *state) +int finish_delayed_checkout(struct checkout *state, int *nr_checkouts) { int errs = 0; unsigned delayed_object_count; @@ -226,7 +226,7 @@ int finish_delayed_checkout(struct checkout *state) ce = index_file_exists(state->istate, path->string, strlen(path->string), 0); if (ce) { - errs |= checkout_entry(ce, state, NULL); + errs |= checkout_entry(ce, state, NULL, nr_checkouts); filtered_bytes += ce->ce_stat_data.sd_size; display_throughput(progress, filtered_bytes); } else @@ -435,8 +435,8 @@ static void mark_colliding_entries(const struct checkout *state, * its name is returned in topath[], which must be able to hold at * least TEMPORARY_FILENAME_LENGTH bytes long. */ -int checkout_entry(struct cache_entry *ce, - const struct checkout *state, char *topath) +int checkout_entry(struct cache_entry *ce, const struct checkout *state, + char *topath, int *nr_checkouts) { static struct strbuf path = STRBUF_INIT; struct stat st; @@ -506,5 +506,7 @@ int checkout_entry(struct cache_entry *ce, return 0; create_directories(path.buf, path.len, state); + if (nr_checkouts) + (*nr_checkouts)++; return write_entry(ce, path.buf, state, 0); } diff --git a/unpack-trees.c b/unpack-trees.c index 7570df481b..17f1e601da 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -294,7 +294,7 @@ static void load_gitmodules_file(struct index_state *index, repo_read_gitmodules(the_repository); } else if (state && (ce->ce_flags & CE_UPDATE)) { submodule_free(the_repository); - checkout_entry(ce, state, NULL); + checkout_entry(ce, state, NULL, NULL); repo_read_gitmodules(the_repository); } } @@ -450,12 +450,12 @@ static int check_updates(struct unpack_trees_options *o) display_progress(progress, ++cnt); ce->ce_flags &= ~CE_UPDATE; if (o->update && !o->dry_run) { - errs |= checkout_entry(ce, &state, NULL); + errs |= checkout_entry(ce, &state, NULL, NULL); } } } stop_progress(&progress); - errs |= finish_delayed_checkout(&state); + errs |= finish_delayed_checkout(&state, NULL); if (o->update) git_attr_set_direction(GIT_ATTR_CHECKIN);
One of the problems with "git checkout" is that it does so many different things and could confuse people specially when we fail to handle ambiguation correctly. One way to help with that is tell the user what sort of operation is actually carried out. When switching branches, we always print something [1]. Checking out paths however is silent. Print something so that if we got the user intention wrong, they won't waste too much time to find that out. 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). [1] Knowing the number of paths updated could also be useful even in normal case. Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- This is related to another patch in https://public-inbox.org/git/20181110120707.25846-1-pclouds@gmail.com/T/#u While writing that patch I thought printing something would also help. But if we get ambiguation right (in that particular case) then we don't actually need this. But it still seems a good idea... apply.c | 3 ++- builtin/checkout-index.c | 6 ++++-- builtin/checkout.c | 30 ++++++++++++++++++++++-------- builtin/difftool.c | 2 +- cache.h | 4 ++-- entry.c | 10 ++++++---- unpack-trees.c | 6 +++--- 7 files changed, 40 insertions(+), 21 deletions(-)