git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Leila Muhtasib <muhtasib@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH/RFC] revision: Show friendlier message.
Date: Sun, 24 Jun 2012 22:28:25 -0700	[thread overview]
Message-ID: <7vobo8hsee.fsf@alter.siamese.dyndns.org> (raw)
In-Reply-To: <1340478681-58476-1-git-send-email-muhtasib@gmail.com> (Leila Muhtasib's message of "Sat, 23 Jun 2012 15:11:21 -0400")

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?

  reply	other threads:[~2012-06-25  5:28 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-06-23 19:11 [PATCH/RFC] revision: Show friendlier message Leila Muhtasib
2012-06-25  5:28 ` Junio C Hamano [this message]
2012-06-25  5:58   ` Junio C Hamano
2012-06-25 19:14   ` Leila
2012-06-25 19:49     ` Junio C Hamano
2012-06-25 22:53       ` Leila
2012-06-25 23:07         ` Junio C Hamano
2012-06-26  3:46           ` Leila

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=7vobo8hsee.fsf@alter.siamese.dyndns.org \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=muhtasib@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).