[PATCH/RFC] clone: have progress report mention top level dir, not git dir

"git clone foo bar" currently reports "Cloning into
/path/to/bar/.git".  Change this message to "Cloning into bar" to more
closely match the user's expectation.

Signed-off-by: Pete Harlan <redacted>
---

This changes a progress message introduced a few weeks ago in
28ba96ab2.  Unless there's a particular reason to report the .git dir
instead of the top level dir, seeing the top level dir feels more
natural to me.

(Or printing nothing.  For a local clone, silently succeeding seems
like a reasonable default.  And for a nonlocal clone there's enough
noise that the user is comforted that something is happening.)

For a --bare clone the current message prints the top level dir
(because that is the git dir), so one could argue in favor of the
current message because it confirms for the user whether their
checkout was bare or not.  But that's only if the user is aware of how
it would appear in both cases; I doubt that the existing code intended
to make that distinction clear, and in practice I expect most users
(a) trust git to do what they asked and (b) wouldn't notice that
"Cloning into /path/to/bar" meant that it was a bare checkout.

 builtin/clone.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 0bedde4..306aacf 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c

@@ -464,7 +464,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	set_git_dir(make_absolute_path(git_dir));
 
 	if (0 <= option_verbosity)
-		printf("Cloning into %s...\n", get_git_dir());
+		printf("Cloning into %s...\n", dir);
 	init_db(option_template, INIT_DB_QUIET);
 
 	/*

-- 
1.7.1.14.gcafbfa

On Sat, May 08, 2010 at 06:23:21PM -0700, Pete Harlan wrote:

"git clone foo bar" currently reports "Cloning into
/path/to/bar/.git".  Change this message to "Cloning into bar" to more
closely match the user's expectation.

I am a little torn on this. For most users, it is just another
implementation detail that makes git's output more confusing. And it is
likely to be the very first git message seen by many people. But at the
same time, it is telling you where the repository actually is, which is
something that can help users learn about how git works.

I guess it comes down to how much detail we want to show.

For a --bare clone the current message prints the top level dir
(because that is the git dir), so one could argue in favor of the
current message because it confirms for the user whether their
checkout was bare or not.  But that's only if the user is aware of how
it would appear in both cases; I doubt that the existing code intended
to make that distinction clear, and in practice I expect most users
(a) trust git to do what they asked and (b) wouldn't notice that
"Cloning into /path/to/bar" meant that it was a bare checkout.

I do think there is some value to this distinction. But we can make it a
lot less ugly for new users with:

  $ git clone /tmp/foo
  Cloning into /tmp/foo...

  $ git clone --bare /tmp/foo
  Cloning into bare repository /tmp/foo...

or something like that.

-Peff

On 05/09/2010 04:02 AM, Jeff King wrote:

On Sat, May 08, 2010 at 06:23:21PM -0700, Pete Harlan wrote:

quoted

"git clone foo bar" currently reports "Cloning into
/path/to/bar/.git".  Change this message to "Cloning into bar" to more
closely match the user's expectation.

I am a little torn on this. For most users, it is just another
implementation detail that makes git's output more confusing. And it is
likely to be the very first git message seen by many people. But at the
same time, it is telling you where the repository actually is, which is
something that can help users learn about how git works.

I guess it comes down to how much detail we want to show.

For me it isn't only a matter of detail; I find "Cloning into
bar/.git" misleading, since bar is getting more than a .git directory.

quoted

For a --bare clone the current message prints the top level dir
(because that is the git dir), so one could argue in favor of the
current message because it confirms for the user whether their
checkout was bare or not.  But that's only if the user is aware of how
it would appear in both cases; I doubt that the existing code intended
to make that distinction clear, and in practice I expect most users
(a) trust git to do what they asked and (b) wouldn't notice that
"Cloning into /path/to/bar" meant that it was a bare checkout.

I do think there is some value to this distinction. But we can make it a
lot less ugly for new users with:

  $ git clone /tmp/foo
  Cloning into /tmp/foo...

  $ git clone --bare /tmp/foo
  Cloning into bare repository /tmp/foo...

or something like that.

Thank you for looking at this.  I agree with you, and have added a
second patch that implements that.

These two changes modify a progress message introduced a few weeks ago
in 28ba96ab2.  Unless there's a particular reason to report the .git
dir instead of the top level dir, seeing the top level dir feels more
natural to me.

For a --bare clone the current message prints the top level dir
(because that is the git dir), so one could argue in favor of the
current message because it confirms for the user whether their
checkout was bare or not.  The second patch modifies the message per
Jeff King's suggestion, to say "Cloning to bare repository bar..." to
convey that information more directly.

Pete Harlan (2):
  clone: have progress report mention top level dir, not git dir
  clone: add bare clone to the progress message

 builtin/clone.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

"git clone foo bar" currently reports "Cloning into
/path/to/bar/.git".  Change this message to "Cloning into bar" to more
closely match the user's expectation.

Signed-off-by: Pete Harlan <redacted>
---
 builtin/clone.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 0bedde4..306aacf 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c

@@ -464,7 +464,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	set_git_dir(make_absolute_path(git_dir));
 
 	if (0 <= option_verbosity)
-		printf("Cloning into %s...\n", get_git_dir());
+		printf("Cloning into %s...\n", dir);
 	init_db(option_template, INIT_DB_QUIET);
 
 	/*

-- 
1.7.1.14.gcafbfa.dirty

When cloning, we report "Cloning into foo...".  If the repository is
bare, say "Cloning into bare repository foo..." instead.

Suggested-by: Jeff King <redacted>
Signed-off-by: Pete Harlan <redacted>
---
 builtin/clone.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 306aacf..3a3625b 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c

@@ -464,7 +464,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	set_git_dir(make_absolute_path(git_dir));
 
 	if (0 <= option_verbosity)
-		printf("Cloning into %s...\n", dir);
+		printf("Cloning into %s%s...\n",
+		       option_bare ? "bare repository " : "", dir);
 	init_db(option_template, INIT_DB_QUIET);
 
 	/*

-- 
1.7.1.14.gcafbfa.dirty

On Sun, May 09, 2010 at 01:09:14PM -0700, Pete Harlan wrote:

quoted

I guess it comes down to how much detail we want to show.

For me it isn't only a matter of detail; I find "Cloning into
bar/.git" misleading, since bar is getting more than a .git directory.

Yeah, I can buy that line of reasoning. Junio's nitpick aside, I think
most users perceive the clone process as creating the whole "bar"
directory.

Thank you for looking at this.  I agree with you, and have added a
second patch that implements that.

These patches look good to me. I agree with Junio about just squashing
them.

-Peff

Jeff King venit, vidit, dixit 10.05.2010 07:47:

On Sun, May 09, 2010 at 01:09:14PM -0700, Pete Harlan wrote:

quoted

quoted

I guess it comes down to how much detail we want to show.

For me it isn't only a matter of detail; I find "Cloning into
bar/.git" misleading, since bar is getting more than a .git directory.

Yeah, I can buy that line of reasoning. Junio's nitpick aside, I think
most users perceive the clone process as creating the whole "bar"
directory.

quoted

Thank you for looking at this.  I agree with you, and have added a
second patch that implements that.

These patches look good to me. I agree with Junio about just squashing
them.

-Peff

Back from a conference, I'm being late for the party (Which way round is
better? ;) ).

But I still want to suggest not sacrificing correctness for "user's
expectations" and rather trying to do combine them. So how about saying

Cloning into $GIT_DIR...
Checking out branch $branch in $WORK_DIR...

where the latter happens for non-bare repos only, of course, and
incidently confirms the use of "-b" or of the default.

Michael

git clone reports $GIT_DIR as the destination of a clone operation,
which is correct but possibly confusing for new users cloning into
non-bare repositories.

Thus, report additionally the check out process as

checking out branch $branchname into worktree $worktree

which has the additional benefit of confirming the checked out branch
(as specified by -b, defaulting to master).

In the case of a detached head, (null) is the branch name.

Inspired-by: Pete Harlan [off-list ref]
Signed-off-by: Michael J Gruber <redacted>
---
I mean something like this. Noobs won't use --no-checkout so that a
check out message should help all possibly confused users.

 builtin/clone.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 4457922..38ca5e8 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c

@@ -629,6 +629,11 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		struct tree_desc t;
 		int fd;
 
+		if (0 <= option_verbosity)
+			printf("Checking out branch %s into worktree %s.\n",
+				skip_prefix(our_head_points_at->name, "refs/heads/"),
+				work_tree);
+
 		/* We need to be in the new work tree for the checkout */
 		setup_work_tree();
 

-- 
1.7.1.240.geeaa4d

On 05/10/2010 03:31 AM, Michael J Gruber wrote:

Jeff King venit, vidit, dixit 10.05.2010 07:47:

quoted

On Sun, May 09, 2010 at 01:09:14PM -0700, Pete Harlan wrote:

quoted

quoted

I guess it comes down to how much detail we want to show.

For me it isn't only a matter of detail; I find "Cloning into 
bar/.git" misleading, since bar is getting more than a .git 
directory.

Yeah, I can buy that line of reasoning. Junio's nitpick aside, I 
think most users perceive the clone process as creating the whole 
"bar" directory.

quoted

Thank you for looking at this.  I agree with you, and have added 
a second patch that implements that.

These patches look good to me. I agree with Junio about just 
squashing them.

-Peff

Back from a conference, I'm being late for the party (Which way
round is better? ;) ).

But I still want to suggest not sacrificing correctness for "user's 
expectations" and rather trying to do combine them. So how about 
saying

Cloning into $GIT_DIR... Checking out branch $branch in $WORK_DIR...

where the latter happens for non-bare repos only, of course, and 
incidently confirms the use of "-b" or of the default.

Michael

Thanks for looking at this.  The patch you posted reports, e.g.:

  % git clone foo bar
  Cloning into /tmp/git/bar/.git...
  done.
  Checking out branch master into worktree bar.
  %

I'd like to see "worktree" either omitted or replaced with "working
directory".  Git works on trees, but "working directory" is a term
ordinary users understand and "bar" is a directory being populated
with files so there's nothing wrong with the user thinking of it that
way.

But on a different note, I think we don't have to be so verbose.  If
the user asks for details with -v then be as chatty as we want, but
for the most part operations that succeed should do so quietly.

My original (unsent) patch was based on master from a couple of weeks
ago and was merely going to remove the db-initialization message and
replace it with nothing, so a successful local clone would look like:

  % git clone foo bar
  %

I don't think it needs to be more complicated than that.  And there's
real value in silent success: every message output has to be read by
the user because it might be an error message.

Since Junio solved the original problem in a different way (still
reporting a message but making it less scary) I made a patch to make
his message more (in my opinion) friendly, but I think output from
normal commands should be as simple as possible.

At my previous job I converted a team of ten or so people from
Subversion to Git, and virtually everyone on the team besides myself
considered Git difficult to use and not worth the trouble.  We didn't
have enough time with it (three months) so I couldn't tell if they
ever would have come around, but each little thing that a user could
perceive as complicated adds up.

So: I'm fine with your patch (with a removed "worktree" or replace it
with "working directory") if writing a thorough message is considered
desirable.  But my vote is for more simple output (as in my patches),
or better yet nothing at all unless there's a problem.  The user can
ask for progress with -v if they want it.

--Pete

Pete Harlan venit, vidit, dixit 11.05.2010 01:22:

On 05/10/2010 03:31 AM, Michael J Gruber wrote:

quoted

Jeff King venit, vidit, dixit 10.05.2010 07:47:

quoted

On Sun, May 09, 2010 at 01:09:14PM -0700, Pete Harlan wrote:

quoted

quoted

I guess it comes down to how much detail we want to show.

For me it isn't only a matter of detail; I find "Cloning into 
bar/.git" misleading, since bar is getting more than a .git 
directory.

Yeah, I can buy that line of reasoning. Junio's nitpick aside, I 
think most users perceive the clone process as creating the whole 
"bar" directory.

quoted

Thank you for looking at this.  I agree with you, and have added 
a second patch that implements that.

These patches look good to me. I agree with Junio about just 
squashing them.

-Peff

Back from a conference, I'm being late for the party (Which way
round is better? ;) ).

But I still want to suggest not sacrificing correctness for "user's 
expectations" and rather trying to do combine them. So how about 
saying

Cloning into $GIT_DIR... Checking out branch $branch in $WORK_DIR...

where the latter happens for non-bare repos only, of course, and 
incidently confirms the use of "-b" or of the default.

Michael

Thanks for looking at this.  The patch you posted reports, e.g.:

  % git clone foo bar
  Cloning into /tmp/git/bar/.git...
  done.
  Checking out branch master into worktree bar.
  %

I'd like to see "worktree" either omitted or replaced with "working
directory".  Git works on trees, but "working directory" is a term
ordinary users understand and "bar" is a directory being populated
with files so there's nothing wrong with the user thinking of it that
way.

"working tree" (short: worktree) is the Git term, core.worktree the name
of the config variable, GIT_WORK_TREE the name of the environment
variable for the directory in which a tree is checked out.

"working directory" is the name of the directory which you are in (i.e.
$(pwd)).

While, technically, the wd is the wt during the check out, using wd
would introduce users to the wrong terminology.

But on a different note, I think we don't have to be so verbose.  If
the user asks for details with -v then be as chatty as we want, but
for the most part operations that succeed should do so quietly.

In fact, we introduced that message not that long ago because until
then, only init's message would be displayed to the user ("Initializing
empty..."), which was highly confusing.

The consensus back then was, without -v nor -q, to have everyday
commands be silent on successful operation, and "infrequent" commands
report progress.

Cheers,
Michael

Michael J Gruber [off-list ref] writes:

In the case of a detached head, (null) is the branch name.

I think that depends on your particular libc implementation that helpfully
makes printf("%s", NULL) not to at least dump core.

Junio C Hamano venit, vidit, dixit 12.05.2010 03:55:

Michael J Gruber [off-list ref] writes:

quoted

In the case of a detached head, (null) is the branch name.

I think that depends on your particular libc implementation that helpfully
makes printf("%s", NULL) not to at least dump core.

Sorry, I didn't know I can't rely on that. I'll change that if going
this route (reporting check out) is desirable at all. Is it?

Michael