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