On Mon, Jun 25, 2012 at 3:49 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>>> 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.
>
> 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:
> +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;
+}
--
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