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

From: Leila <muhtasib@gmail.com>
Date: Jun 25, 2012, 22:53 UTC
Message-ID: <CAA3EhHLy+5Vfw0T=7VEBi+2ZxjS4x2dndox+M_E06v3FtoNQXg@mail.gmail.com>
In-Reply-To: <7vr4t3f9y6.fsf@alter.siamese.dyndns.org>

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

Show 10 quoted lines

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

Noted.

Show 8 quoted lines

>
> But it turns out that it is not even needed.  Read on.
>
>> 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]?
>
> Didn't I already say not to switch on argv[0] in deeper side of the
> callchain?

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.

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

This did work. I tried it out.

>
> The "bad HEAD and no revs..." part, if we choose not to even error
> on this, can be removed.

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

>
> Also other cmd_frotz() functions in the same file might want to use
> the s/"HEAD"/default_to_head_if_exists()/ conversion.

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:

Show 9 quoted lines

> +static const char *default_to_head_if_exists(void)
> +{
> +       unsigned char sha1[20];
> +       if (resolve_ref_unsafe("HEAD", sha1, 1, NULL))
> +               return "HEAD";
> +       else
> +               return NULL;
> +}
> +

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