diff mbox series

[v2] checkout: print something when checking out paths

Message ID 20181113182800.26984-1-pclouds@gmail.com
State New, archived
Headers show
Series [v2] checkout: print something when checking out paths | expand

Commit Message

Duy Nguyen Nov. 13, 2018, 6:28 p.m. UTC
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 unless --quiet, either

 - "HEAD is now at ..."
 - "Reset branch ..."
 - "Already on ..."
 - "Switched to and reset ..."
 - "Switched to a new branch ..."
 - "Switched to branch ..."

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. For the remaining cases of checkout we now print either

 - "Checked out ... paths out of the index"
 - "Checked out ... paths out of <abbrev hash>"

Since the purpose of printing this is to help disambiguate. Only do it
when "--" is missing.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 v2 updates the messages a bit but it does not check isatty or add
 --count-paths, for consistency reason with how messages are printed
 in the branch switching case.
 
 Consistency is not always a good reason to follow. But I haven't
 seen a strong reason to go against it.

 apply.c                  |  3 ++-
 builtin/checkout-index.c |  6 ++++--
 builtin/checkout.c       | 39 +++++++++++++++++++++++++++++++--------
 builtin/difftool.c       |  2 +-
 cache.h                  |  4 ++--
 entry.c                  | 10 ++++++----
 unpack-trees.c           |  6 +++---
 7 files changed, 49 insertions(+), 21 deletions(-)

Comments

Junio C Hamano Nov. 14, 2018, 10:12 a.m. UTC | #1
Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

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

You would have realized that this is way too noisy if you ran "make
test", which may have spewed something like this on the screen.

[19:09:19] t4120-apply-popt.sh ................................ ok     1624 ms ( 0.26 usr  0.21 sys +  5.31 cusr  3.51 csys =  9.29 CPU)
[19:09:20] t9164-git-svn-dcommit-concurrent.sh ................ skipped: Perl SVN libraries not found or unusable
[19:09:20] t1310-config-default.sh ............................ ok      177 ms ( 0.07 usr  0.01 sys +  0.89 cusr  0.66 csys =  1.63 CPU)
===(   20175;154  1297/?  155/?  6/?  3/3  2/?  4/?  4/?  3/?  5... )===Checked out 1 path out of the index
Checked out 1 path out of the index
Checked out 1 path out of the index
Checked out 1 path out of the index
Checked out 1 path out of the index
[19:09:20] t1408-packed-refs.sh ............................... ok      310 ms ( 0.06 usr  0.00 sys +  0.69 cusr  0.52 csys =  1.27 CPU)
[19:09:20] t0025-crlf-renormalize.sh .......................... ok      246 ms ( 0.03 usr  0.01 sys +  0.34 cusr  0.22 csys =  0.60 CPU)

I am very tempted to suggest to treat this as a training wheel and
enable only when checkout.showpathcount is set to true, or something
like that.
Duy Nguyen Nov. 14, 2018, 3:31 p.m. UTC | #2
On Wed, Nov 14, 2018 at 11:12 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
> > 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.
>
> You would have realized that this is way too noisy if you ran "make
> test", which may have spewed something like this on the screen.

Oh I realize it because it's part of my git build and I often use "git
co <paths>". I'm just telling (or kidding?) myself that I'm just so
used to the old behavior and may need some time to feel comfortable
with the new one.

> [19:09:19] t4120-apply-popt.sh ................................ ok     1624 ms ( 0.26 usr  0.21 sys +  5.31 cusr  3.51 csys =  9.29 CPU)
> [19:09:20] t9164-git-svn-dcommit-concurrent.sh ................ skipped: Perl SVN libraries not found or unusable
> [19:09:20] t1310-config-default.sh ............................ ok      177 ms ( 0.07 usr  0.01 sys +  0.89 cusr  0.66 csys =  1.63 CPU)
> ===(   20175;154  1297/?  155/?  6/?  3/3  2/?  4/?  4/?  3/?  5... )===Checked out 1 path out of the index
> Checked out 1 path out of the index
> Checked out 1 path out of the index
> Checked out 1 path out of the index
> Checked out 1 path out of the index
> [19:09:20] t1408-packed-refs.sh ............................... ok      310 ms ( 0.06 usr  0.00 sys +  0.69 cusr  0.52 csys =  1.27 CPU)
> [19:09:20] t0025-crlf-renormalize.sh .......................... ok      246 ms ( 0.03 usr  0.01 sys +  0.34 cusr  0.22 csys =  0.60 CPU)
>
> I am very tempted to suggest to treat this as a training wheel and
> enable only when checkout.showpathcount is set to true, or something
> like that.

Maybe we just drop it then. I'm not adding a training wheel. I'm
trying to make this complex command safer somewhat. But maybe this is
a wrong direction. I'll give the idea "switch-branch / restore-path
alternative commands" a go some time. Then the new generation can just
stick to those and old timers stay with "git checkout".
Junio C Hamano Jan. 28, 2019, 9:58 p.m. UTC | #3
Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> 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 unless --quiet, either
>
>  - "HEAD is now at ..."
>  - "Reset branch ..."
>  - "Already on ..."
>  - "Switched to and reset ..."
>  - "Switched to a new branch ..."
>  - "Switched to branch ..."
>
> 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. For the remaining cases of checkout we now print either
>
>  - "Checked out ... paths out of the index"
>  - "Checked out ... paths out of <abbrev hash>"
>
> Since the purpose of printing this is to help disambiguate. Only do it
> when "--" is missing.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  v2 updates the messages a bit but it does not check isatty or add
>  --count-paths, for consistency reason with how messages are printed
>  in the branch switching case.
>  
>  Consistency is not always a good reason to follow. But I haven't
>  seen a strong reason to go against it.

One small bug I saw since this was merged is that the message that
is given when unmerging, i.e.

	git merge other-branch   ;# conflicts
	git checkout --m <path>

is misleading.  It gives the same "checked out ... out of the index",
but it should be made a lot more distinct, perhaps "unmerged N paths".
Duy Nguyen Jan. 29, 2019, 1:26 a.m. UTC | #4
On Tue, Jan 29, 2019 at 4:58 AM Junio C Hamano <gitster@pobox.com> wrote:
> One small bug I saw since this was merged is that the message that
> is given when unmerging, i.e.
>
>         git merge other-branch   ;# conflicts
>         git checkout --m <path>
>
> is misleading.  It gives the same "checked out ... out of the index",
> but it should be made a lot more distinct, perhaps "unmerged N paths".

Right, I missed this. How about "re-created %d merge conflicts"? It's
a bit clearer, I think.
diff mbox series

Patch

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..3a0b86ec1c 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,36 @@  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) {
+		if (opts->source_tree)
+			fprintf_ln(stderr, Q_("Checked out %d path out of %s",
+					      "Checked out %d paths out of %s",
+					      nr_checkouts),
+				   nr_checkouts,
+				   find_unique_abbrev(&opts->source_tree->object.oid,
+						      DEFAULT_ABBREV));
+		else
+			fprintf_ln(stderr, Q_("Checked out %d path out of the index",
+					      "Checked out %d paths out of the index",
+					      nr_checkouts),
+				   nr_checkouts);
+	}
 
 	if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
 		die(_("unable to write new index file"));
@@ -1064,6 +1086,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);