{"thread":{"id":"39580","subject":"[PATCH 2/4] bisect: replace hardcoded \"bad|good\" by variables","startedAt":"2015-06-10T07:10:37Z","lastAt":"2015-06-10T07:55:29Z","messageCount":2,"participants":["Antoine Delaite","Matthieu Moy"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"263435","messageId":"1809967391.331411.1433920237669.JavaMail.zimbra@ensimag.grenoble-inp.fr","threadId":"39580","inReplyTo":"308677275.323594.1433875347392.JavaMail.zimbra@ensimag.grenoble-inp.fr","subject":"[PATCH 2/4] bisect: replace hardcoded \"bad|good\" by variables","fromName":"Antoine Delaite","fromEmail":"antoine.delaite@ensimag.grenoble-inp.fr","sentAt":"2015-06-10T07:10:37Z","receivedAt":"2015-06-10T07:10:37Z","isPatch":true,"sender":{"key":"antoine.delaite@ensimag.grenoble-inp.fr","avatar":"https://avatars.githubusercontent.com/u/12510393?v=4"},"body":"Hi, \n\nThanks for the review, \n(sorry if you received this twice)\n\nMatthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote: \n\n>> +static const char *name_bad; \n>> +static const char *name_good; \n> \n>Same remark as PATCH 2. \n\nAfter the discussion you had with Christian I think we will \nkeep name_bad/good for now. \n\n>> - \n>> - fprintf(stderr, \"The merge base %s is bad.\\n\" \n>> - \"This means the bug has been fixed \" \n>> - \"between %s and [%s].\\n\", \n>> - bad_hex, bad_hex, good_hex); \n>> - \n>> + if (!strcmp(name_bad, \"bad\")) { \n>> + fprintf(stderr, \"The merge base %s is bad.\\n\" \n>> + \"This means the bug has been fixed \" \n>> + \"between %s and [%s].\\n\", \n>> + bad_hex, bad_hex, good_hex); \n>> + } \n> \n>You need an \"else\" here. Maybe it comes later, but as a reviewer, I want \n>to check that you did not forget it now (because I don't trust myself to \n>remember that it must be added later). \n\nShould I put an else {} with nothing in beetween? \n\n>> + name_bad = \"bad\"; \n>> + name_good = \"good\"; \n>> + } else { \n>> + strbuf_getline(&str, fp, '\\n'); \n>> + name_bad = strbuf_detach(&str, NULL); \n>> + strbuf_getline(&str, fp, '\\n'); \n>> + name_good = strbuf_detach(&str, NULL); \n>> + } \n> \n>I would have kept just \n> \n> name_bad = \"bad\"; \n> name_good = \"good\"; \n> \n>in this patch, and introduce BISECT_TERMS in a separate one. \n\nShould not I introduce BISECT_TERMS in bisect.c and git-bisect.sh \nwith the same commit? I did some rebase though and now name_bad and \nname_good appears in the first commit, and read_bisect_terms in the \nsecond. \n\n>> --- a/git-bisect.sh \n>> +++ b/git-bisect.sh \n>> @@ -77,6 +77,7 @@ bisect_start() { \n>> orig_args=$(git rev-parse --sq-quote \"$@\") \n>> bad_seen=0 \n>> eval='' \n>> + start_bad_good=0 \n>> if test \"z$(git rev-parse --is-bare-repository)\" != zfalse \n>> then \n>> mode=--no-checkout \n>> @@ -101,6 +102,9 @@ bisect_start() { \n>> die \"$(eval_gettext \"'\\$arg' does not appear to be a valid revision\")\" \n>> break \n>> } \n>> + \n>> + start_bad_good=1 \n>> + \n> \n>Why do you need this variable? It seems to me that you are hardcoding \n>once more that terms can be either \"good/bad\" or \"old/new\", which you \n>tried to eliminate from the previous round. \n\nI answered to Junio on this too, it is because our function which create \nthe bisect_terms file is not called after \n'git bisect start bad_rev good_rev'. \n\n>> + then \n>> + echo \"$NAME_BAD\" >\"$GIT_DIR/BISECT_TERMS\" && \n>> + echo \"$NAME_GOOD\" >>\"$GIT_DIR/BISECT_TERMS\" \n>> + fi && \n> \n>Why not do this unconditionnally? Whether terms are good/bad or old/new, \n>you can write them to BISECT_TERMS. \n\nBecause after a git bisect start we don't yet what terms are used. \nThis line is only for the case 'git bisect start bad_rev good_rev'. \n\n>> + fi \n>> + case \"$cmd\" in \n>> + bad|good) \n>> + if test ! -s \"$GIT_DIR/BISECT_TERMS\" \n>> + then \n>> + echo \"bad\" >\"$GIT_DIR/BISECT_TERMS\" && \n>> + echo \"good\" >>\"$GIT_DIR/BISECT_TERMS\" \n>> + fi \n>> + NAME_BAD=\"bad\" \n>> + NAME_GOOD=\"good\" ;; \n>> + esac ;; \n>> + esac \n>> +} \n>> + \n>> +bisect_voc () { \n>> + case \"$1\" in \n>> + bad) echo \"bad\" ;; \n>> + good) echo \"good\" ;; \n>> + esac \n>> +} \n> \n>It's weird to have these hardcoded \"bad\"/\"good\" when you already have \n>BISECT_TERMS in the same patch. \n\nbisect_voc is used to display what commands the user can do, thus the \nbuiltins tags. I did not find a way to not hardcode them. \n\nThe other points have been taken into account. \n"},{"id":"263447","messageId":"vpqr3pkatfy.fsf@anie.imag.fr","threadId":"39580","inReplyTo":"1809967391.331411.1433920237669.JavaMail.zimbra@ensimag.grenoble-inp.fr","subject":"Re: [PATCH 2/4] bisect: replace hardcoded \"bad|good\" by variables","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-06-10T07:55:29Z","receivedAt":"2015-06-10T07:55:29Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Antoine Delaite <antoine.delaite@ensimag.grenoble-inp.fr> writes:\n\n>>> - \n>>> - fprintf(stderr, \"The merge base %s is bad.\\n\" \n>>> - \"This means the bug has been fixed \" \n>>> - \"between %s and [%s].\\n\", \n>>> - bad_hex, bad_hex, good_hex); \n>>> - \n>>> + if (!strcmp(name_bad, \"bad\")) { \n>>> + \tfprintf(stderr, \"The merge base %s is bad.\\n\" \n>>> + \t\"This means the bug has been fixed \" \n>>> + \t\"between %s and [%s].\\n\", \n>>> + \tbad_hex, bad_hex, good_hex); \n>>> + } \n>> \n>>You need an \"else\" here. Maybe it comes later, but as a reviewer, I want \n>>to check that you did not forget it now (because I don't trust myself to \n>>remember that it must be added later). \n>\n> Should I put an else {} with nothing in beetween? \n\nWhat you want to avoid is a senario where the if branch is not taken\nsilently in the future. Two ways to avoid that:\n\nif (!strcmp(name_bad, \"bad\")) {\n\t// special case for good/bad\n} else {\n\tdie(\"BUG: terms %s/%s not managed\", name_bad, name_good);\n}\n\nThink of someone trying to debug the code later: if you encounter a\ndie(\"BUG: ...\"), gdb will immediately tell you what's going one.\nDebugging the absence of something is much more painful.\n\nAlternatively:\n\nif (!strcmp(name_bad, \"bad\")) {\n\t// special case for good/bad\n} else {\n\tfprintf(\"very generic message not saying \\\"which means that ...\\\"\");\n}\n\nIn both cases, adding a new pair of terms should look like\n\n if (!strcmp(name_bad, \"bad\")) {\n \t// special case for good/bad\n+} else if(!strcmp(name_bad, \"<new-term>\")) {\n+\t// special case for <new-term>\n } else {\n \tdie(\"BUG: terms %s/%s not managed\", name_bad, name_good);\n }\n\nI have a slight preference for the \"else\" with a generic message. It\nwill be dead code for now, but may turn useful if we ever allow\narbitrary pairs of terms.\n\n>\n>>> + name_bad = \"bad\"; \n>>> + name_good = \"good\"; \n>>> + } else { \n>>> + strbuf_getline(&str, fp, '\\n'); \n>>> + name_bad = strbuf_detach(&str, NULL); \n>>> + strbuf_getline(&str, fp, '\\n'); \n>>> + name_good = strbuf_detach(&str, NULL); \n>>> + } \n>> \n>>I would have kept just \n>> \n>> name_bad = \"bad\"; \n>> name_good = \"good\"; \n>> \n>>in this patch, and introduce BISECT_TERMS in a separate one. \n>\n> Should not I introduce BISECT_TERMS in bisect.c and git-bisect.sh \n> with the same commit?\n\nMake sure you have a bisectable history. If you use two commits, you\nshould make sure that the intermediate step is consistant. If the change\nis small enough, it's probably better to have a single commit. No strong\nopinion on that (at least not before I see the code).\n\n> I did some rebase though and now name_bad and name_good appears in the\n> first commit, and read_bisect_terms in the second.\n>\n>>> --- a/git-bisect.sh \n>>> +++ b/git-bisect.sh \n>>> @@ -77,6 +77,7 @@ bisect_start() { \n>>> orig_args=$(git rev-parse --sq-quote \"$@\") \n>>> bad_seen=0 \n>>> eval='' \n>>> + start_bad_good=0 \n>>> if test \"z$(git rev-parse --is-bare-repository)\" != zfalse \n>>> then \n>>> mode=--no-checkout \n>>> @@ -101,6 +102,9 @@ bisect_start() { \n>>> die \"$(eval_gettext \"'\\$arg' does not appear to be a valid revision\")\" \n>>> break \n>>> } \n>>> + \n>>> + start_bad_good=1 \n>>> + \n>> \n>>Why do you need this variable? It seems to me that you are hardcoding \n>>once more that terms can be either \"good/bad\" or \"old/new\", which you \n>>tried to eliminate from the previous round. \n>\n> I answered to Junio on this too, it is because our function which create \n> the bisect_terms file is not called after \n> 'git bisect start bad_rev good_rev'.\n\nThen the variable name is not good. If the variable is there to mean \"we\ndid not define the terms yet\", then bisect_terms_defined={0,1} would be\nmuch better (probably not the best possible name, though).\n\n>>> +bisect_voc () { \n>>> + case \"$1\" in \n>>> + bad) echo \"bad\" ;; \n>>> + good) echo \"good\" ;; \n>>> + esac \n>>> +} \n>> \n>>It's weird to have these hardcoded \"bad\"/\"good\" when you already have \n>>BISECT_TERMS in the same patch. \n>\n> bisect_voc is used to display what commands the user can do, thus the \n> builtins tags. I did not find a way to not hardcode them.\n\nWell, if you really want to say good/bad, then using just good/bad would\nbe simpler than $(bisect_voc good)/$(bisect_voc bad), no? bisect_voc is\njust the identitity function (or returns the empty string for input\nother than good/bad).\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"}]}