RFC patchrevision: Show friendlier message.

'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(-)

Leila Muhtasib <muhtasib@gmail.com> writes:

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?

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

The last part of the paragraph should read:

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

On Mon, Jun 25, 2012 at 1:28 AM, Junio C Hamano <gitster@pobox.com> wrote:

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.

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.

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]?

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

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.

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!

Leila <muhtasib@gmail.com> writes:

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.

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

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(-)

On Mon, Jun 25, 2012 at 3:49 PM, Junio C Hamano <gitster@pobox.com> wrote:

Noted.

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.

This did work. I tried it out.

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;
+        }

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:

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;
+}

Leila <muhtasib@gmail.com> writes:

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.

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.

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.

On Mon, Jun 25, 2012 at 7:07 PM, Junio C Hamano <gitster@pobox.com> wrote:

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.

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