threads / rfc / 23750

RFC patchclone: have progress report mention top level dir, not git dir

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

## tl;dr

14 messages between May 9, 2010 and May 12, 2010. Diffs are folded; open one to read it.

replies: 13people: 4as markdown or json

Pete Harlan· May 9, 2010, 01:23 UTC · lore

"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 <pgit@pcharlan.com>
---

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(-)
Show changes to builtin/clone.c +1 −1
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
Jeff King· May 9, 2010, 11:02 UTC · re: Pete Harlan · lore

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

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.
Show 8 quoted lines
> 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
Pete Harlan· May 9, 2010, 20:09 UTC · re: Jeff King · lore

[PATCH v2 0/2] clone: simplify progress message

On 05/09/2010 04:02 AM, Jeff King wrote:
Show 13 quoted lines
> 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 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.

Show 19 quoted lines
>> 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(-)
Pete Harlan· May 9, 2010, 20:10 UTC · re: Pete Harlan · lore

[PATCH v2 1/2] 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 <pgit@pcharlan.com>
---
 builtin/clone.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Show changes to builtin/clone.c +1 −1
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
Pete Harlan· May 9, 2010, 20:11 UTC · re: Pete Harlan · lore

[PATCH v2 2/2] clone: add bare clone to the progress message

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

Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Pete Harlan <pgit@pcharlan.com>
---
 builtin/clone.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
Show changes to builtin/clone.c +2 −1
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
Junio C Hamano· May 9, 2010, 22:15 UTC · re: Pete Harlan · lore

Re: [PATCH v2 0/2] clone: simplify progress message

Pete Harlan <pgit@pcharlan.com> writes:
Show 17 quoted lines
> On 05/09/2010 04:02 AM, Jeff King wrote:
>> 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 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.

That is also misleading, as cloning is done into bar/.git and everything else happens locally as part of the checkout.

I didn't want to go into nitpicky details, but you asked for it ;-)
> Pete Harlan (2):
>   clone: have progress report mention top level dir, not git dir
>   clone: add bare clone to the progress message

I think the squashing these two into one patch makes quite a lot of sense. Does any of the existing test need adjustments, by the way?

Pete Harlan· May 9, 2010, 23:10 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 0/2] clone: simplify progress message

On 05/09/2010 03:15 PM, Junio C Hamano wrote:
Show 24 quoted lines
> Pete Harlan <pgit@pcharlan.com> writes:
> 
>> On 05/09/2010 04:02 AM, Jeff King wrote:
>>> 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 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.
> 
> That is also misleading, as cloning is done into bar/.git and everything
> else happens locally as part of the checkout.
> 
> I didn't want to go into nitpicky details, but you asked for it ;-)
Fair enough :)
Show 6 quoted lines
>> Pete Harlan (2):
>>   clone: have progress report mention top level dir, not git dir
>>   clone: add bare clone to the progress message
> 
> I think the squashing these two into one patch makes quite a lot of
> sense.  Does any of the existing test need adjustments, by the way?

No, the test (t5601-clone.sh) looks for "Clon", so the new message passes that just as well.

I could add a new test that ensures that "bare repository" shows up in the message when --bare is passed if you think that's worthwhile.

--Pete
Jeff King· May 10, 2010, 05:47 UTC · re: Pete Harlan · lore

Re: [PATCH v2 0/2] clone: simplify progress message

On Sun, May 09, 2010 at 01:09:14PM -0700, Pete Harlan wrote:
> > 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
Michael J Gruber· May 10, 2010, 10:31 UTC · re: Jeff King · lore

Re: [PATCH v2 0/2] clone: simplify progress message

Jeff King venit, vidit, dixit 10.05.2010 07:47:
Show 18 quoted lines
> On Sun, May 09, 2010 at 01:09:14PM -0700, Pete Harlan wrote:
> 
>>> 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

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
Michael J Gruber· May 10, 2010, 14:46 UTC · re: Michael J Gruber · lore

[PATCH] clone: report check out for non-bare clones

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 <pgit@pcharlan.com>
Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
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(-)
Show changes to builtin/clone.c +5 −0
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
Junio C Hamano· May 12, 2010, 01:55 UTC · re: Michael J Gruber · lore

Re: [PATCH] clone: report check out for non-bare clones

Michael J Gruber <git@drmicha.warpmail.net> 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.

Michael J Gruber· May 12, 2010, 07:59 UTC · re: Junio C Hamano · lore

Re: [PATCH] clone: report check out for non-bare clones

Junio C Hamano venit, vidit, dixit 12.05.2010 03:55:
Show 6 quoted lines
> Michael J Gruber <git@drmicha.warpmail.net> 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.

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
Pete Harlan· May 10, 2010, 23:22 UTC · re: Michael J Gruber · lore

Re: [PATCH v2 0/2] clone: simplify progress message

On 05/10/2010 03:31 AM, Michael J Gruber wrote:
Show 34 quoted lines
> Jeff King venit, vidit, dixit 10.05.2010 07:47:
>> On Sun, May 09, 2010 at 01:09:14PM -0700, Pete Harlan wrote:
>> 
>>>> 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
> 
> 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
Michael J Gruber· May 11, 2010, 07:27 UTC · re: Pete Harlan · lore

Re: [PATCH v2 0/2] clone: simplify progress message

Pete Harlan venit, vidit, dixit 11.05.2010 01:22:
Show 49 quoted lines
> On 05/10/2010 03:31 AM, Michael J Gruber wrote:
>> Jeff King venit, vidit, dixit 10.05.2010 07:47:
>>> On Sun, May 09, 2010 at 01:09:14PM -0700, Pete Harlan wrote:
>>>
>>>>> 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
>>
>> 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

← back to recent threads