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

From: Leila <muhtasib@gmail.com>
Date: Jun 25, 2012, 19:14 UTC
Message-ID: <CAA3EhHJbKj+nbVsZtijsH+h7sFcyeBwT9K=BTeqAuMzSH0RGmg@mail.gmail.com>
In-Reply-To: <7vobo8hsee.fsf@alter.siamese.dyndns.org>

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

Show 7 quoted lines

> 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.

Show 7 quoted lines

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

Show 8 quoted lines

> 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.

Show 6 quoted lines

> 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!