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

Leila [off-list ref] writes:

quoted

 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.

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?

quoted

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

No.  I do not want any patch that butchers setup_revisions() with
any of this kind of UI issues.

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.

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

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

 builtin/log.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/builtin/log.c b/builtin/log.c
index 4f1b42a..6ecf344 100644
--- a/builtin/log.c
+++ b/builtin/log.c

@@ -355,6 +355,15 @@ static int git_log_config(const char *var, const char *value, void *cb)
 	return git_diff_ui_config(var, value, cb);
 }
 
+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;
+}
+
 int cmd_whatchanged(int argc, const char **argv, const char *prefix)
 {
 	struct rev_info rev;

@@ -553,8 +562,15 @@ int cmd_log(int argc, const char **argv, const char *prefix)
 	init_revisions(&rev, prefix);
 	rev.always_show_header = 1;
 	memset(&opt, 0, sizeof(opt));
-	opt.def = "HEAD";
+	opt.def = default_to_head_if_exists();
 	cmd_log_init(argc, argv, prefix, &rev, &opt);
+
+	if (!opt.def && !rev.cmdline.nr) {
+		/*
+		 * bad HEAD and no revs on the command line
+		 */
+		warning("Nothing to show...");
+	}
 	return cmd_log_walk(&rev);
 }
 

On Mon, Jun 25, 2012 at 3:49 PM, Junio C Hamano [off-list ref] wrote:

quoted

quoted

 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.

quoted

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