{"thread":{"id":"30881","subject":"[PATCH/RFC] revision: Show friendlier message.","startedAt":"2012-06-23T19:11:21Z","lastAt":"2012-06-26T03:46:16Z","messageCount":8,"participants":["Leila Muhtasib","Junio C Hamano","Leila"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"194136","messageId":"1340478681-58476-1-git-send-email-muhtasib@gmail.com","threadId":"30881","inReplyTo":null,"subject":"[PATCH/RFC] revision: Show friendlier message.","fromName":"Leila Muhtasib","fromEmail":"muhtasib@gmail.com","sentAt":"2012-06-23T19:11:21Z","receivedAt":"2012-06-23T19:11:21Z","isPatch":true,"sender":{"key":"muhtasib@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1618875?v=4"},"body":"'git log' on a newly initialized repository\n(before any commits) shows an ugly message.\n\nSigned-off-by: Leila Muhtasib \n---\nI found this bug on the debian git bug list.\nhttp://bugs.debian.org/cgi-bin/bugreport.cgi?bug=412890\n\nSee scenario below:\n\n% mkdir test\n% cd test\n% git init\nInitialized empty Git repository in .git/\n% git log\nfatal: bad default revision 'HEAD'\n\nI've reworded the error message to something friendlier.\nI can change the message or add something like the link\nsuggests about 'Create or switch to an existent branch.\"\nBut I don't feel like that message quite captures the\nscenario of initializing a new repository. Alternatively,\nwe can print the message using printf and exit successfully\ninstead of using 'die'.\n\nThanks,\nLeila\n\n revision.c | 8 ++++++--\n 1 files changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 935e7a7..96add37 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1821,8 +1821,12 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\tunsigned char sha1[20];\n \t\tstruct object *object;\n \t\tunsigned mode;\n-\t\tif (get_sha1_with_mode(revs->def, sha1, &mode))\n-\t\t\tdie(\"bad default revision '%s'\", revs->def);\n+\t\tif (get_sha1_with_mode(revs->def, sha1, &mode)) {\n+\t\t\tif (!strcmp(argv[0], \"log\") && !strcmp(revs->def, \"HEAD\"))\n+\t\t\t\tdie(\"No commits to display.\");\n+\t\t\telse\n+\t\t\t\tdie(\"bad default revision '%s'\", revs->def);\n+\t\t}\n \t\tobject = get_reference(revs, revs->def, sha1, 0);\n \t\tadd_pending_object_with_mode(revs, object, revs->def, mode);\n \t}\n-- \n1.7.7.5 (Apple Git-26)\n"},{"id":"194176","messageId":"7vobo8hsee.fsf@alter.siamese.dyndns.org","threadId":"30881","inReplyTo":"1340478681-58476-1-git-send-email-muhtasib@gmail.com","subject":"Re: [PATCH/RFC] revision: Show friendlier message.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-06-25T05:28:25Z","receivedAt":"2012-06-25T05:28:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Leila Muhtasib writes:\n\n> % mkdir test\n> % cd test\n> % git init\n> Initialized empty Git repository in .git/\n> % git log\n> fatal: bad default revision 'HEAD'\n\nI agree that the message, while it is technically correct and does\nnot deserve to be called a bug, can be made more friendly.\n\nBut setup_revisions() is a very low level routine that is used by\nmany plumbing commands, and it is a horrible layering violation to\ntweak its behaviour based on argv[0] and also it is too inflexible\nhack as a solution. For example, don't you want to give a different\nerror message for \"git log HEAD\" with an explicit \"HEAD\" from the\ncommand line? Would you add a similar support for a command that is\nnot \"log\" by adding yet another strcmp() here?\n\nWouldn't it be a more reasonable alternative solution if you do this:\n\n 1. Check if HEAD points at a commit _before_ setting opt->def to it\n in \"git log\" (and other end-user facing programs in the \"log\"\n family, possibly in cmd_log_init_finish() if that function is\n not called by a program where the current message should not\n change), and do _NOT_ set opt->def to it;\n\n 2. Make setup_revisions() expose got_rev_arg to its callers\n (e.g. move it to struct rev_info);\n\n 3. If you did not pass HEAD in opt->def and setup_revisions() said\n it did not \"got_rev_arg\", give whatever error message that you\n think is more user friendly.\n\nThat way, if HEAD points at a commit, or if HEAD doesn't point at a\ncommit but the user gave some existing commit from the command line,\nyou wouldn't see \"bad default revision\" at all.\n\nAnd the most important part of this alternative is that the lower\nlevel machinery does not have to _care_ about the reason why the\nhigher level passed a bad HEAD to it.\n\nPersonally, I tend to think that not saying anything and reporting\nsuccess, instead of any error message, would be the right thing to\ndo if you are changing the behaviour of this case anyway.\n\nHrm?\n"},{"id":"194181","messageId":"7vzk7sgcff.fsf@alter.siamese.dyndns.org","threadId":"30881","inReplyTo":"7vobo8hsee.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH/RFC] revision: Show friendlier message.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-06-25T05:58:44Z","receivedAt":"2012-06-25T05:58:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Leila Muhtasib writes:\n>\n>> % mkdir test\n>> % cd test\n>> % git init\n>> Initialized empty Git repository in .git/\n>> % git log\n>> fatal: bad default revision 'HEAD'\n>\n> I agree that the message, while it is technically correct and does\n> not deserve to be called a bug, can be made more friendly.\n>\n> But setup_revisions() is a very low level routine that is used by\n> many plumbing commands, and it is a horrible layering violation to\n> tweak its behaviour based on argv[0] and also it is too inflexible\n> hack as a solution. For example, don't you want to give a different\n> error message for \"git log HEAD\" with an explicit \"HEAD\" from the\n> command line? Would you add a similar support for a command that is\n> not \"log\" by adding yet another strcmp() here?\n>\n> Wouldn't it be a more reasonable alternative solution if you do this:\n>\n> 1. Check if HEAD points at a commit _before_ setting opt->def to it\n> in \"git log\" (and other end-user facing programs in the \"log\"\n> family, possibly in cmd_log_init_finish() if that function is\n> not called by a program where the current message should not\n> change), and do _NOT_ set opt->def to it;\n\nThe last part of the paragraph should read:\n\n\t... and do _NOT_ set opt->def to it if HEAD does not point\n\tat a commit.\n"},{"id":"194223","messageId":"CAA3EhHJbKj+nbVsZtijsH+h7sFcyeBwT9K=BTeqAuMzSH0RGmg@mail.gmail.com","threadId":"30881","inReplyTo":"7vobo8hsee.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH/RFC] revision: Show friendlier message.","fromName":"Leila","fromEmail":"muhtasib@gmail.com","sentAt":"2012-06-25T19:14:50Z","receivedAt":"2012-06-25T19:14:50Z","isPatch":true,"sender":{"key":"muhtasib@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1618875?v=4"},"body":"On Mon, Jun 25, 2012 at 1:28 AM, Junio C Hamano wrote:\n> But setup_revisions() is a very low level routine that is used by\n> many plumbing commands, and it is a horrible layering violation to\n> tweak its behaviour based on argv[0] and also it is too inflexible\n> hack as a solution.  For example, don't you want to give a different\n> error message for \"git log HEAD\" with an explicit \"HEAD\" from the\n> command line?  Would you add a similar support for a command that is\n> not \"log\" by adding yet another strcmp() here?\n\nI noticed that it was a very low level routine used by many commands.\nInitially, I also thought it was a bit hacky to check argv[0], and yes\nit would mean that we'd need to add more strcmps (or if the message\nsufficed for all commands take out the comparison of argv[0]) in the\nfuture to handle various cases. But I figured I'd use argv[0], since\nit was avail to us in that routine. And since that routine is where we\ndisplayed the error message, I tried to keep changes local to that\narea. I was trying to change as little code as possible, because I\ndidn't want to affect the other commands. I've started implementing a\npatch with your suggestions below.\n\n\n> Wouldn't it be a more reasonable alternative solution if you do this:\n>\n>  1. Check if HEAD points at a commit _before_ setting opt->def to it\n>    in \"git log\" (and other end-user facing programs in the \"log\"\n>    family, possibly in cmd_log_init_finish() if that function is\n>    not called by a program where the current message should not\n>    change), and do _NOT_ set opt->def to it;\n\nOk. At the start of the cmd_log_init_finish, opt->def already points\nto HEAD. But I can unset it if HEAD doesn't point at a commit.\n\n>  2. Make setup_revisions() expose got_rev_arg to its callers\n>    (e.g. move it to struct rev_info);\n\nDo you mean have got_rev_args be a wrapper of argc and argv?\n\nOr is it just a mechanism to set a signal that the calling command is\n'log', so that I can do something about it without checking argv[0]?\n\n>  3. If you did not pass HEAD in opt->def and setup_revisions() said\n>    it did not \"got_rev_arg\", give whatever error message that you\n>    think is more user friendly.\n>\n\nSure, I can do this. Note: just to confirm the message/exit will still\ncome from inside of setup_revisions()?\n\n> That way, if HEAD points at a commit, or if HEAD doesn't point at a\n> commit but the user gave some existing commit from the command line,\n> you wouldn't see \"bad default revision\" at all.\n>\n> And the most important part of this alternative is that the lower\n> level machinery does not have to _care_ about the reason why the\n> higher level passed a bad HEAD to it.\n>\n\nI'll wait to comment on this until I understand what get_rev_arg is\nsupposed to do/signify, as things will be clearer then.\n\n> Personally, I tend to think that not saying anything and reporting\n> success, instead of any error message, would be the right thing to\n> do if you are changing the behaviour of this case anyway.\n>\n> Hrm?\n>\nYes, I think reporting success would be right in this case. I think\nthe message \"No commit(s) to display.\" is helpful, but I don't feel\nstrongly about this.\n\nThanks!\n"},{"id":"194225","messageId":"7vr4t3f9y6.fsf@alter.siamese.dyndns.org","threadId":"30881","inReplyTo":"CAA3EhHJbKj+nbVsZtijsH+h7sFcyeBwT9K=BTeqAuMzSH0RGmg@mail.gmail.com","subject":"Re: [PATCH/RFC] revision: Show friendlier message.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-06-25T19:49:53Z","receivedAt":"2012-06-25T19:49:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Leila writes:\n\n>> 2. Make setup_revisions() expose got_rev_arg to its callers\n>> (e.g. move it to struct rev_info);\n>\n> Do you mean have got_rev_args be a wrapper of argc and argv?\n\nNo. The setup_revisions() function knows if it saw a revision\nargument from the command line, but currently uses got_rev_args\nlocal variable, so the caller would not be able to tell. I was\nsuggesting to use \"struct rev_info *revs\" that goes in and comes out\nof the function to convey that information back to the caller.\n\nBut it turns out that it is not even needed. Read on.\n\n> Or is it just a mechanism to set a signal that the calling command is\n> 'log', so that I can do something about it without checking argv[0]?\n\nDidn't I already say not to switch on argv[0] in deeper side of the\ncallchain?\n\n>> 3. If you did not pass HEAD in opt->def and setup_revisions() said\n>> it did not \"got_rev_arg\", give whatever error message that you\n>> think is more user friendly.\n>>\n>\n> Sure, I can do this. Note: just to confirm the message/exit will still\n> come from inside of setup_revisions()?\n\nNo. I do not want any patch that butchers setup_revisions() with\nany of this kind of UI issues.\n\nSomething like this, I think, would work. After all, we already\nhave a way to expose the revs we got from the command line to the\ncaller.\n\nThe \"bad HEAD and no revs...\" part, if we choose not to even error\non this, can be removed.\n\nAlso other cmd_frotz() functions in the same file might want to use\nthe s/\"HEAD\"/default_to_head_if_exists()/ conversion.\n\n builtin/log.c | 18 +++++++++++++++++-\n 1 file changed, 17 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 4f1b42a..6ecf344 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -355,6 +355,15 @@ static int git_log_config(const char *var, const char *value, void *cb)\n \treturn git_diff_ui_config(var, value, cb);\n }\n \n+static const char *default_to_head_if_exists(void)\n+{\n+\tunsigned char sha1[20];\n+\tif (resolve_ref_unsafe(\"HEAD\", sha1, 1, NULL))\n+\t\treturn \"HEAD\";\n+\telse\n+\t\treturn NULL;\n+}\n+\n int cmd_whatchanged(int argc, const char **argv, const char *prefix)\n {\n \tstruct rev_info rev;\n@@ -553,8 +562,15 @@ int cmd_log(int argc, const char **argv, const char *prefix)\n \tinit_revisions(&rev, prefix);\n \trev.always_show_header = 1;\n \tmemset(&opt, 0, sizeof(opt));\n-\topt.def = \"HEAD\";\n+\topt.def = default_to_head_if_exists();\n \tcmd_log_init(argc, argv, prefix, &rev, &opt);\n+\n+\tif (!opt.def && !rev.cmdline.nr) {\n+\t\t/*\n+\t\t * bad HEAD and no revs on the command line\n+\t\t */\n+\t\twarning(\"Nothing to show...\");\n+\t}\n \treturn cmd_log_walk(&rev);\n }\n \n"},{"id":"194235","messageId":"CAA3EhHLy+5Vfw0T=7VEBi+2ZxjS4x2dndox+M_E06v3FtoNQXg@mail.gmail.com","threadId":"30881","inReplyTo":"7vr4t3f9y6.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH/RFC] revision: Show friendlier message.","fromName":"Leila","fromEmail":"muhtasib@gmail.com","sentAt":"2012-06-25T22:53:57Z","receivedAt":"2012-06-25T22:53:57Z","isPatch":true,"sender":{"key":"muhtasib@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1618875?v=4"},"body":"On Mon, Jun 25, 2012 at 3:49 PM, Junio C Hamano wrote:\n>>>  2. Make setup_revisions() expose got_rev_arg to its callers\n>>>    (e.g. move it to struct rev_info);\n>>\n>> Do you mean have got_rev_args be a wrapper of argc and argv?\n>\n> No.  The setup_revisions() function knows if it saw a revision\n> argument from the command line, but currently uses got_rev_args\n> local variable, so the caller would not be able to tell.  I was\n> suggesting to use \"struct rev_info *revs\" that goes in and comes out\n> of the function to convey that information back to the caller.\n\nNoted.\n\n>\n> But it turns out that it is not even needed.  Read on.\n>\n>> Or is it just a mechanism to set a signal that the calling command is\n>> 'log', so that I can do something about it without checking argv[0]?\n>\n> Didn't I already say not to switch on argv[0] in deeper side of the\n> callchain?\n\nI wasn't going to switch on argv[0], but something of the sort since I\nwas confused by what you meant by got_rev_args. But I understand now.\n\n>\n> Something like this, I think, would work.  After all, we already\n> have a way to expose the revs we got from the command line to the\n> caller.\n\nThis did work. I tried it out.\n\n>\n> The \"bad HEAD and no revs...\" part, if we choose not to even error\n> on this, can be removed.\n\nYea, I think we should return successfully, and warning() does that.\nBut if we choose to display a message, I don't think it should be a\nwarning (esp for the empty repo case). It should look like the sample\nprintf below, but the v2 of the patch I submitted doesn't include the\nmessage.\n\n+ if (!opt.def && !rev.cmdline.nr) {\n+ printf(\"No commit(s) to display.\\n\");\n+ return 0;\n+ }\n\n>\n> Also other cmd_frotz() functions in the same file might want to use\n> the s/\"HEAD\"/default_to_head_if_exists()/ conversion.\n\nOk, I've updated other functions in the same file. See new patch. I\ndidn't copy paste it into this email, because the spacing will be\nmessed up.\n\nRegarding this implementation:\n\n> +static const char *default_to_head_if_exists(void)\n> +{\n> +       unsigned char sha1[20];\n> +       if (resolve_ref_unsafe(\"HEAD\", sha1, 1, NULL))\n> +               return \"HEAD\";\n> +       else\n> +               return NULL;\n> +}\n> +\n\nI initially wrote something with this logic, do you have a preference?\n\n+static const char *default_to_head_if_exists(void)\n+{\n+ struct commit *commit = lookup_commit_reference_by_name(\"HEAD\");\n+ if(commit)\n+ return \"HEAD\";\n+ else\n+ return NULL;\n+}\n"},{"id":"194236","messageId":"7vsjdjdm7v.fsf@alter.siamese.dyndns.org","threadId":"30881","inReplyTo":"CAA3EhHLy+5Vfw0T=7VEBi+2ZxjS4x2dndox+M_E06v3FtoNQXg@mail.gmail.com","subject":"Re: [PATCH/RFC] revision: Show friendlier message.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-06-25T23:07:48Z","receivedAt":"2012-06-25T23:07:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Leila writes:\n\n>> The \"bad HEAD and no revs...\" part, if we choose not to even error\n>> on this, can be removed.\n>\n> Yea, I think we should return successfully, and warning() does that.\n> But if we choose to display a message, I don't think it should be a\n> warning (esp for the empty repo case). It should look like the sample\n> printf below, but the v2 of the patch I submitted doesn't include the\n> message.\n\nI said \"*if* we choose not to\" for a reason. It can be argued that\nit technically is a regression that \"git log\" does *not* error out\nfor an unborn history, as that is different from the way the command\nhas behaved forever.\n\n> + if (!opt.def && !rev.cmdline.nr) {\n> + printf(\"No commit(s) to display.\\n\");\n> + return 0;\n> + }\n>\n>>\n>> Also other cmd_frotz() functions in the same file might want to use\n>> the s/\"HEAD\"/default_to_head_if_exists()/ conversion.\n>\n> Ok, I've updated other functions in the same file.\n\nAgain, \"might want\" was a key phrase. I didn't look at each and\nevery one of them and thought if it made sense to change their\nbehaviour.\n\n> Regarding this implementation:\n>\n>> +static const char *default_to_head_if_exists(void)\n>> +{\n>> +       unsigned char sha1[20];\n>> +       if (resolve_ref_unsafe(\"HEAD\", sha1, 1, NULL))\n>> +               return \"HEAD\";\n>> +       else\n>> +               return NULL;\n>> +}\n>> +\n>\n> I initially wrote something with this logic, do you have a preference?\n>\n> +static const char *default_to_head_if_exists(void)\n> +{\n> + struct commit *commit = lookup_commit_reference_by_name(\"HEAD\");\n> + if(commit)\n> + return \"HEAD\";\n> + else\n> + return NULL;\n> +}\n\nThe reason why I used resolve_ref_unsafe() is because it will only\ngrab HEAD and not refs/heads/HEAD or any confusing mess, even in a\nsick repository.\n"},{"id":"194255","messageId":"CAA3EhHJfRY=UpuriqB-ARdui3BS6tCpn+Zoi_ccJ15181qGMaw@mail.gmail.com","threadId":"30881","inReplyTo":"7vsjdjdm7v.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH/RFC] revision: Show friendlier message.","fromName":"Leila","fromEmail":"muhtasib@gmail.com","sentAt":"2012-06-26T03:46:16Z","receivedAt":"2012-06-26T03:46:16Z","isPatch":true,"sender":{"key":"muhtasib@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1618875?v=4"},"body":"On Mon, Jun 25, 2012 at 7:07 PM, Junio C Hamano wrote:\n>>> The \"bad HEAD and no revs...\" part, if we choose not to even error\n>>> on this, can be removed.\n>>\n>> Yea, I think we should return successfully, and warning() does that.\n>> But if we choose to display a message, I don't think it should be a\n>> warning (esp for the empty repo case). It should look like the sample\n>> printf below, but the v2 of the patch I submitted doesn't include the\n>> message.\n>\n> I said \"*if* we choose not to\" for a reason.  It can be argued that\n> it technically is a regression that \"git log\" does *not* error out\n> for an unborn history, as that is different from the way the command\n> has behaved forever.\n>\n\nYes, this is def a concern. Ok here are my thoughts on the four options:\n1) Display error message and error. (current behavior)\nI don't agree with this, thus the patch I'm creating.\n2) Display a friendlier message and error.\nI think this is a good option, and preserving the return code will be\nless likely to break things (this idea was present in my first patch).\n3) Display success message and succeed.\nThis makes sense to me, but this would be changing the behavior drastically.\n4) Succeed silently\nI created my second patch to follow this model. I eventually chose\nthis over 3, because I figured it was more in-tune with the rest of\ngit's behavior with succeeding silently.\n\nSo how do we break the tie between #2 and #4? I think #2 is playing it\nsafer than #4, even though #4 is more ideal.\n\n> Again, \"might want\" was a key phrase.  I didn't look at each and\n> every one of them and thought if it made sense to change their\n> behaviour.\n>\n\nYes, I understood that. I believe I left one out.\n"}]}