On Mon, Jun 25, 2012 at 1:28 AM, Junio C Hamano <gitster@xxxxxxxxx> 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! -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html