Leila Muhtasib [off-list ref] 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?
On Mon, Jun 25, 2012 at 1:28 AM, Junio C Hamano [off-list ref] 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!