[PATCH/RFC] revision: Show friendlier message.

[PATCH/RFC] revision: Show friendlier message.

[Leila Muhtasib]

'git log' on a newly initialized repository
(before any commits) shows an ugly message.

Signed-off-by: Leila Muhtasib <muhtasib@gmail.com>
---
I found this bug on the debian git bug list.
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=412890

See scenario below:

% mkdir test
% cd test
% git init
Initialized empty Git repository in .git/
% git log
fatal: bad default revision 'HEAD'

I've reworded the error message to something friendlier.
I can change the message or add something like the link
suggests about 'Create or switch to an existent branch."
But I don't feel like that message quite captures the
scenario of initializing a new repository. Alternatively,
we can print the message using printf and exit successfully
instead of using 'die'.

Thanks,
Leila

 revision.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/revision.c b/revision.c
index 935e7a7..96add37 100644
--- a/revision.c
+++ b/revision.c
@@ -1821,8 +1821,12 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 		unsigned char sha1[20];
 		struct object *object;
 		unsigned mode;
-		if (get_sha1_with_mode(revs->def, sha1, &mode))
-			die("bad default revision '%s'", revs->def);
+		if (get_sha1_with_mode(revs->def, sha1, &mode)) {
+			if (!strcmp(argv[0], "log") && !strcmp(revs->def, "HEAD"))
+				die("No commits to display.");
+			else
+				die("bad default revision '%s'", revs->def);
+		}
 		object = get_reference(revs, revs->def, sha1, 0);
 		add_pending_object_with_mode(revs, object, revs->def, mode);
 	}
-- 
1.7.7.5 (Apple Git-26)

Re: [PATCH/RFC] revision: Show friendlier message.

[Junio C Hamano]

Leila Muhtasib <muhtasib@gmail.com> writes:

> % mkdir test
> % cd test
> % git init
> Initialized empty Git repository in .git/
> % git log
> fatal: bad default revision 'HEAD'

I agree that the message, while it is technically correct and does
not deserve to be called a bug, can be made more friendly.

But setup_revisions() is a very low level routine that is used by
many plumbing commands, and it is a horrible layering violation to
tweak its behaviour based on argv[0] and also it is too inflexible
hack as a solution.  For example, don't you want to give a different
error message for "git log HEAD" with an explicit "HEAD" from the
command line?  Would you add a similar support for a command that is
not "log" by adding yet another strcmp() here?

Wouldn't it be a more reasonable alternative solution if you do this:

 1. Check if HEAD points at a commit _before_ setting opt->def to it
    in "git log" (and other end-user facing programs in the "log"
    family, possibly in cmd_log_init_finish() if that function is
    not called by a program where the current message should not
    change), and do _NOT_ set opt->def to it;

 2. Make setup_revisions() expose got_rev_arg to its callers
    (e.g. move it to struct rev_info);

 3. If you did not pass HEAD in opt->def and setup_revisions() said
    it did not "got_rev_arg", give whatever error message that you
    think is more user friendly.

That way, if HEAD points at a commit, or if HEAD doesn't point at a
commit but the user gave some existing commit from the command line,
you wouldn't see "bad default revision" at all.

And the most important part of this alternative is that the lower
level machinery does not have to _care_ about the reason why the
higher level passed a bad HEAD to it.

Personally, I tend to think that not saying anything and reporting
success, instead of any error message, would be the right thing to
do if you are changing the behaviour of this case anyway.

Hrm?

Re: [PATCH/RFC] revision: Show friendlier message.

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Leila Muhtasib <muhtasib@gmail.com> writes:
>
>> % mkdir test
>> % cd test
>> % git init
>> Initialized empty Git repository in .git/
>> % git log
>> fatal: bad default revision 'HEAD'
>
> I agree that the message, while it is technically correct and does
> not deserve to be called a bug, can be made more friendly.
>
> But setup_revisions() is a very low level routine that is used by
> many plumbing commands, and it is a horrible layering violation to
> tweak its behaviour based on argv[0] and also it is too inflexible
> hack as a solution.  For example, don't you want to give a different
> error message for "git log HEAD" with an explicit "HEAD" from the
> command line?  Would you add a similar support for a command that is
> not "log" by adding yet another strcmp() here?
>
> Wouldn't it be a more reasonable alternative solution if you do this:
>
>  1. Check if HEAD points at a commit _before_ setting opt->def to it
>     in "git log" (and other end-user facing programs in the "log"
>     family, possibly in cmd_log_init_finish() if that function is
>     not called by a program where the current message should not
>     change), and do _NOT_ set opt->def to it;

The last part of the paragraph should read:

	... and do _NOT_ set opt->def to it if HEAD does not point
	at a commit.

Re: [PATCH/RFC] revision: Show friendlier message.

[Leila]

On Mon, Jun 25, 2012 at 1:28 AM, Junio C Hamano <gitster@pobox.com> wrote:
> But setup_revisions() is a very low level routine that is used by
> many plumbing commands, and it is a horrible layering violation to
> tweak its behaviour based on argv[0] and also it is too inflexible
> hack as a solution.  For example, don't you want to give a different
> error message for "git log HEAD" with an explicit "HEAD" from the
> command line?  Would you add a similar support for a command that is
> not "log" by adding yet another strcmp() here?

I noticed that it was a very low level routine used by many commands.
Initially, I also thought it was a bit hacky to check argv[0], and yes
it would mean that we'd need to add more strcmps (or if the message
sufficed for all commands take out the comparison of argv[0]) in the
future to handle various cases. But I figured I'd use argv[0], since
it was avail to us in that routine. And since that routine is where we
displayed the error message, I tried to keep changes local to that
area. I was trying to change as little code as possible, because I
didn't want to affect the other commands. I've started implementing a
patch with your suggestions below.


> Wouldn't it be a more reasonable alternative solution if you do this:
>
>  1. Check if HEAD points at a commit _before_ setting opt->def to it
>    in "git log" (and other end-user facing programs in the "log"
>    family, possibly in cmd_log_init_finish() if that function is
>    not called by a program where the current message should not
>    change), and do _NOT_ set opt->def to it;

Ok. At the start of the cmd_log_init_finish, opt->def already points
to HEAD. But I can unset it if HEAD doesn't point at a commit.

>  2. Make setup_revisions() expose got_rev_arg to its callers
>    (e.g. move it to struct rev_info);

Do you mean have got_rev_args be a wrapper of argc and argv?

Or is it just a mechanism to set a signal that the calling command is
'log', so that I can do something about it without checking argv[0]?

>  3. If you did not pass HEAD in opt->def and setup_revisions() said
>    it did not "got_rev_arg", give whatever error message that you
>    think is more user friendly.
>

Sure, I can do this. Note: just to confirm the message/exit will still
come from inside of setup_revisions()?

> That way, if HEAD points at a commit, or if HEAD doesn't point at a
> commit but the user gave some existing commit from the command line,
> you wouldn't see "bad default revision" at all.
>
> And the most important part of this alternative is that the lower
> level machinery does not have to _care_ about the reason why the
> higher level passed a bad HEAD to it.
>

I'll wait to comment on this until I understand what get_rev_arg is
supposed to do/signify, as things will be clearer then.

> Personally, I tend to think that not saying anything and reporting
> success, instead of any error message, would be the right thing to
> do if you are changing the behaviour of this case anyway.
>
> Hrm?
>
Yes, I think reporting success would be right in this case. I think
the message "No commit(s) to display." is helpful, but I don't feel
strongly about this.

Thanks!

Re: [PATCH/RFC] revision: Show friendlier message.

[Junio C Hamano]

Leila <muhtasib@gmail.com> writes:

>>  2. Make setup_revisions() expose got_rev_arg to its callers
>>    (e.g. move it to struct rev_info);
>
> Do you mean have got_rev_args be a wrapper of argc and argv?

No.  The setup_revisions() function knows if it saw a revision
argument from the command line, but currently uses got_rev_args
local variable, so the caller would not be able to tell.  I was
suggesting to use "struct rev_info *revs" that goes in and comes out
of the function to convey that information back to the caller.

But it turns out that it is not even needed.  Read on.

> Or is it just a mechanism to set a signal that the calling command is
> 'log', so that I can do something about it without checking argv[0]?

Didn't I already say not to switch on argv[0] in deeper side of the
callchain?

>>  3. If you did not pass HEAD in opt->def and setup_revisions() said
>>    it did not "got_rev_arg", give whatever error message that you
>>    think is more user friendly.
>>
>
> Sure, I can do this. Note: just to confirm the message/exit will still
> come from inside of setup_revisions()?

No.  I do not want any patch that butchers setup_revisions() with
any of this kind of UI issues.

Something like this, I think, would work.  After all, we already
have a way to expose the revs we got from the command line to the
caller.

The "bad HEAD and no revs..." part, if we choose not to even error
on this, can be removed.

Also other cmd_frotz() functions in the same file might want to use
the s/"HEAD"/default_to_head_if_exists()/ conversion.

 builtin/log.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/builtin/log.c b/builtin/log.c
index 4f1b42a..6ecf344 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -355,6 +355,15 @@ static int git_log_config(const char *var, const char *value, void *cb)
 	return git_diff_ui_config(var, value, cb);
 }
 
+static const char *default_to_head_if_exists(void)
+{
+	unsigned char sha1[20];
+	if (resolve_ref_unsafe("HEAD", sha1, 1, NULL))
+		return "HEAD";
+	else
+		return NULL;
+}
+
 int cmd_whatchanged(int argc, const char **argv, const char *prefix)
 {
 	struct rev_info rev;
@@ -553,8 +562,15 @@ int cmd_log(int argc, const char **argv, const char *prefix)
 	init_revisions(&rev, prefix);
 	rev.always_show_header = 1;
 	memset(&opt, 0, sizeof(opt));
-	opt.def = "HEAD";
+	opt.def = default_to_head_if_exists();
 	cmd_log_init(argc, argv, prefix, &rev, &opt);
+
+	if (!opt.def && !rev.cmdline.nr) {
+		/*
+		 * bad HEAD and no revs on the command line
+		 */
+		warning("Nothing to show...");
+	}
 	return cmd_log_walk(&rev);
 }
 

Re: [PATCH/RFC] revision: Show friendlier message.

[Leila]

On Mon, Jun 25, 2012 at 3:49 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>>  2. Make setup_revisions() expose got_rev_arg to its callers
>>>    (e.g. move it to struct rev_info);
>>
>> Do you mean have got_rev_args be a wrapper of argc and argv?
>
> No.  The setup_revisions() function knows if it saw a revision
> argument from the command line, but currently uses got_rev_args
> local variable, so the caller would not be able to tell.  I was
> suggesting to use "struct rev_info *revs" that goes in and comes out
> of the function to convey that information back to the caller.

Noted.

>
> But it turns out that it is not even needed.  Read on.
>
>> Or is it just a mechanism to set a signal that the calling command is
>> 'log', so that I can do something about it without checking argv[0]?
>
> Didn't I already say not to switch on argv[0] in deeper side of the
> callchain?

I wasn't going to switch on argv[0], but something of the sort since I
was confused by what you meant by got_rev_args. But I understand now.

>
> Something like this, I think, would work.  After all, we already
> have a way to expose the revs we got from the command line to the
> caller.

This did work. I tried it out.

>
> The "bad HEAD and no revs..." part, if we choose not to even error
> on this, can be removed.

Yea, I think we should return successfully, and warning() does that.
But if we choose to display a message, I don't think it should be a
warning (esp for the empty repo case). It should look like the sample
printf below, but the v2 of the patch I submitted doesn't include the
message.

+ if (!opt.def && !rev.cmdline.nr) {
+          printf("No commit(s) to display.\n");
+          return 0;
+        }

>
> Also other cmd_frotz() functions in the same file might want to use
> the s/"HEAD"/default_to_head_if_exists()/ conversion.

Ok, I've updated other functions in the same file. See new patch. I
didn't copy paste it into this email, because the spacing will be
messed up.

Regarding this implementation:

> +static const char *default_to_head_if_exists(void)
> +{
> +       unsigned char sha1[20];
> +       if (resolve_ref_unsafe("HEAD", sha1, 1, NULL))
> +               return "HEAD";
> +       else
> +               return NULL;
> +}
> +

I initially wrote something with this logic, do you have a preference?

+static const char *default_to_head_if_exists(void)
+{
+       struct commit *commit = lookup_commit_reference_by_name("HEAD");
+       if(commit)
+               return "HEAD";
+       else
+               return NULL;
+}

Re: [PATCH/RFC] revision: Show friendlier message.

[Junio C Hamano]

Leila <muhtasib@gmail.com> writes:

>> The "bad HEAD and no revs..." part, if we choose not to even error
>> on this, can be removed.
>
> Yea, I think we should return successfully, and warning() does that.
> But if we choose to display a message, I don't think it should be a
> warning (esp for the empty repo case). It should look like the sample
> printf below, but the v2 of the patch I submitted doesn't include the
> message.

I said "*if* we choose not to" for a reason.  It can be argued that
it technically is a regression that "git log" does *not* error out
for an unborn history, as that is different from the way the command
has behaved forever.

> + if (!opt.def && !rev.cmdline.nr) {
> +          printf("No commit(s) to display.\n");
> +          return 0;
> +        }
>
>>
>> Also other cmd_frotz() functions in the same file might want to use
>> the s/"HEAD"/default_to_head_if_exists()/ conversion.
>
> Ok, I've updated other functions in the same file.

Again, "might want" was a key phrase.  I didn't look at each and
every one of them and thought if it made sense to change their
behaviour.

> Regarding this implementation:
>
>> +static const char *default_to_head_if_exists(void)
>> +{
>> +       unsigned char sha1[20];
>> +       if (resolve_ref_unsafe("HEAD", sha1, 1, NULL))
>> +               return "HEAD";
>> +       else
>> +               return NULL;
>> +}
>> +
>
> I initially wrote something with this logic, do you have a preference?
>
> +static const char *default_to_head_if_exists(void)
> +{
> +       struct commit *commit = lookup_commit_reference_by_name("HEAD");
> +       if(commit)
> +               return "HEAD";
> +       else
> +               return NULL;
> +}

The reason why I used resolve_ref_unsafe() is because it will only
grab HEAD and not refs/heads/HEAD or any confusing mess, even in a
sick repository.

Re: [PATCH/RFC] revision: Show friendlier message.

[Leila]

On Mon, Jun 25, 2012 at 7:07 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> The "bad HEAD and no revs..." part, if we choose not to even error
>>> on this, can be removed.
>>
>> Yea, I think we should return successfully, and warning() does that.
>> But if we choose to display a message, I don't think it should be a
>> warning (esp for the empty repo case). It should look like the sample
>> printf below, but the v2 of the patch I submitted doesn't include the
>> message.
>
> I said "*if* we choose not to" for a reason.  It can be argued that
> it technically is a regression that "git log" does *not* error out
> for an unborn history, as that is different from the way the command
> has behaved forever.
>

Yes, this is def a concern. Ok here are my thoughts on the four options:
1) Display error message and error. (current behavior)
I don't agree with this, thus the patch I'm creating.
2) Display a friendlier message and error.
I think this is a good option, and preserving the return code will be
less likely to break things (this idea was present in my first patch).
3) Display success message and succeed.
This makes sense to me, but this would be changing the behavior drastically.
4) Succeed silently
I created my second patch to follow this model. I eventually chose
this over 3, because I figured it was more in-tune with the rest of
git's behavior with succeeding silently.

So how do we break the tie between #2 and #4? I think #2 is playing it
safer than #4, even though #4 is more ideal.

> Again, "might want" was a key phrase.  I didn't look at each and
> every one of them and thought if it made sense to change their
> behaviour.
>

Yes, I understood that. I believe I left one out.