{"thread":{"id":"24375","subject":"[PATCH 1/2] revert: refactor code that prints success or failure message","startedAt":"2010-07-12T11:54:53Z","lastAt":"2010-07-15T09:26:57Z","messageCount":7,"participants":["Christian Couder","Jonathan Nieder","Ramkumar Ramachandra"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"145368","messageId":"20100712115455.12251.53947.chriscool@tuxfamily.org","threadId":"24375","inReplyTo":null,"subject":"[PATCH 1/2] revert: refactor code that prints success or failure message","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2010-07-12T11:54:53Z","receivedAt":"2010-07-12T11:54:53Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"This patch refactors the code that prints a message like\n\"Automatic cherry-pick failed. <help message>\". This code was\nduplicated in both do_recursive_merge() and do_pick_commit().\n\nTo do that, now do_recursive_merge() returns an int to signal\nsuccess or failure. And in case of failure we just return 1\nfrom do_pick_commit() instead of doing \"exit(1)\" from either\ndo_recursive_merge() or do_pick_commit().\n\nIn case of successful merge, do_recursive_merge() used to print\nsomething like \"Finished one cherry-pick.\" but nothing was\nprinted in case a special strategy like \"resolve\" was used.\nNow in this case a message like \"Finished one cherry-pick with\nstrategy resolve.\" is printed.\n\nSo the command behavior should be more consistent wether we are\nusing a special strategy or not.\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n\tI also wonder why messages like \"Finished one cherry-pick.\"\n\tare printed on stderr instead of stdout.\n\tMaybe this could be changed in this patch or in\n\tanother one.\n\n\tAnd while at making a backward incompatible change, maybe\n\twe could change the message to something like:\n\n\t\"Finished cherry-pick <sha1>\"\n\n builtin/revert.c                    |   51 ++++++++++++++++++++---------------\n t/t3508-cherry-pick-many-commits.sh |   26 +++++++++++++++++-\n 2 files changed, 54 insertions(+), 23 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 8b9d829..b082bb4 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -311,10 +311,9 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from)\n \treturn write_ref_sha1(ref_lock, to, \"cherry-pick\");\n }\n \n-static void do_recursive_merge(struct commit *base, struct commit *next,\n-\t\t\t       const char *base_label, const char *next_label,\n-\t\t\t       unsigned char *head, struct strbuf *msgbuf,\n-\t\t\t       char *defmsg)\n+static int do_recursive_merge(struct commit *base, struct commit *next,\n+\t\t\t      const char *base_label, const char *next_label,\n+\t\t\t      unsigned char *head, struct strbuf *msgbuf)\n {\n \tstruct merge_options o;\n \tstruct tree *result, *next_tree, *base_tree, *head_tree;\n@@ -357,14 +356,9 @@ static void do_recursive_merge(struct commit *base, struct commit *next,\n \t\t\t\t\ti++;\n \t\t\t}\n \t\t}\n-\t\twrite_message(msgbuf, defmsg);\n-\t\tfprintf(stderr, \"Automatic %s failed.%s\\n\",\n-\t\t\tme, help_msg());\n-\t\trerere(allow_rerere_auto);\n-\t\texit(1);\n \t}\n-\twrite_message(msgbuf, defmsg);\n-\tfprintf(stderr, \"Finished one %s.\\n\", me);\n+\n+\treturn !clean;\n }\n \n static int do_pick_commit(void)\n@@ -375,6 +369,8 @@ static int do_pick_commit(void)\n \tstruct commit_message msg = { NULL, NULL, NULL, NULL, NULL };\n \tchar *defmsg = NULL;\n \tstruct strbuf msgbuf = STRBUF_INIT;\n+\tstruct strbuf mebuf = STRBUF_INIT;\n+\tint res;\n \n \tif (no_commit) {\n \t\t/*\n@@ -470,30 +466,41 @@ static int do_pick_commit(void)\n \t\t}\n \t}\n \n-\tif (!strategy || !strcmp(strategy, \"recursive\") || action == REVERT)\n-\t\tdo_recursive_merge(base, next, base_label, next_label,\n-\t\t\t\t   head, &msgbuf, defmsg);\n-\telse {\n-\t\tint res;\n+\tstrbuf_addstr(&mebuf, me);\n+\n+\tif (!strategy || !strcmp(strategy, \"recursive\") || action == REVERT) {\n+\t\tres = do_recursive_merge(base, next, base_label, next_label,\n+\t\t\t\t\t head, &msgbuf);\n+\t\twrite_message(&msgbuf, defmsg);\n+\t} else {\n \t\tstruct commit_list *common = NULL;\n \t\tstruct commit_list *remotes = NULL;\n+\n+\t\tstrbuf_addf(&mebuf, \" with strategy %s\", strategy);\n \t\twrite_message(&msgbuf, defmsg);\n+\n \t\tcommit_list_insert(base, &common);\n \t\tcommit_list_insert(next, &remotes);\n \t\tres = try_merge_command(strategy, common,\n \t\t\t\t\tsha1_to_hex(head), remotes);\n \t\tfree_commit_list(common);\n \t\tfree_commit_list(remotes);\n-\t\tif (res) {\n-\t\t\tfprintf(stderr, \"Automatic %s with strategy %s failed.%s\\n\",\n-\t\t\t\tme, strategy, help_msg());\n-\t\t\trerere(allow_rerere_auto);\n-\t\t\texit(1);\n-\t\t}\n \t}\n \n+\tif (res) {\n+\t\tfprintf(stderr, \"Automatic %s failed.%s\\n\",\n+\t\t\tmebuf.buf, help_msg());\n+\t\trerere(allow_rerere_auto);\n+\t} else {\n+\t\tfprintf(stderr, \"Finished one %s.\\n\", mebuf.buf);\n+\t}\n+\n+\tstrbuf_release(&mebuf);\n \tfree_message(&msg);\n \n+\tif (res)\n+\t\treturn 1;\n+\n \t/*\n \t *\n \t * If we are cherry-pick, and if the merge did not result in\ndiff --git a/t/t3508-cherry-pick-many-commits.sh b/t/t3508-cherry-pick-many-commits.sh\nindex f90ed3d..6555f92 100755\n--- a/t/t3508-cherry-pick-many-commits.sh\n+++ b/t/t3508-cherry-pick-many-commits.sh\n@@ -23,12 +23,36 @@ test_expect_success setup '\n '\n \n test_expect_success 'cherry-pick first..fourth works' '\n+\tcat <<-EOF > expected &&\n+\tFinished one cherry-pick.\n+\tFinished one cherry-pick.\n+\tFinished one cherry-pick.\n+\tEOF\n+\n+\tgit checkout -f master &&\n+\tgit reset --hard first &&\n+\ttest_tick &&\n+\tgit cherry-pick first..fourth 2>actual &&\n+\tgit diff --quiet other &&\n+\tgit diff --quiet HEAD other &&\n+\ttest_cmp expected actual &&\n+\ttest \"$(git rev-parse --verify HEAD)\" != \"$(git rev-parse --verify fourth)\"\n+'\n+\n+test_expect_success 'cherry-pick --strategy resolve first..fourth works' '\n+\tcat <<-EOF > expected &&\n+\tFinished one cherry-pick with strategy resolve.\n+\tFinished one cherry-pick with strategy resolve.\n+\tFinished one cherry-pick with strategy resolve.\n+\tEOF\n+\n \tgit checkout -f master &&\n \tgit reset --hard first &&\n \ttest_tick &&\n-\tgit cherry-pick first..fourth &&\n+\tgit cherry-pick --strategy resolve first..fourth 2>actual &&\n \tgit diff --quiet other &&\n \tgit diff --quiet HEAD other &&\n+\ttest_cmp expected actual &&\n \ttest \"$(git rev-parse --verify HEAD)\" != \"$(git rev-parse --verify fourth)\"\n '\n \n-- \n1.7.2.rc1.213.gf3fb81\n"},{"id":"145374","messageId":"20100712163043.GA1531@burratino","threadId":"24375","inReplyTo":"20100712115455.12251.53947.chriscool@tuxfamily.org","subject":"Re: [PATCH 1/2] revert: refactor code that prints success or failure message","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-07-12T16:30:43Z","receivedAt":"2010-07-12T16:30:43Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nChristian Couder wrote:\n\n> This patch refactors the code that prints a message like\n> \"Automatic cherry-pick failed. <help message>\".\n\nFrom the point of view of refactoring, it does not seem\nso promising: the patch adds more code than it removes.\n\n $ git diff --numstat HEAD^\n 29\t22\tbuiltin/revert.c\n 25\t 1\tt/t3508-cherry-pick-many-commits.sh\n\nSo I am hoping there is some other reason.\n\n> To do that, now do_recursive_merge() returns an int to signal\n> success or failure. And in case of failure we just return 1\n> from do_pick_commit() instead of doing \"exit(1)\" from either\n> do_recursive_merge() or do_pick_commit().\n\nThis part sounds like libification...\n\n> In case of successful merge, do_recursive_merge() used to print\n> something like \"Finished one cherry-pick.\" but nothing was\n> printed in case a special strategy like \"resolve\" was used.\n> Now in this case a message like \"Finished one cherry-pick with\n> strategy resolve.\" is printed.\n>\n> So the command behavior should be more consistent wether we are\n> using a special strategy or not.\n\nThis part sounds like improving output in the cherry-pick --strategy\nsuccess case.\n\n> \tI also wonder why messages like \"Finished one cherry-pick.\"\n> \tare printed on stderr instead of stdout.\n\nProgress reports tend to go to stderr, since they are not what the\nuser wanted to learn from the command but side-effects.  In other\nwords, I think the general principle is that\n\n $ git <foo> 2>/dev/null\n\nshould output whatever a traditional Unix command would (usually\nnothing).\n\n> \tAnd while at making a backward incompatible change, maybe\n> \twe could change the message to something like:\n> \n> \t\"Finished cherry-pick <sha1>\"\n\nDoes <sha1> represent the old commit or the new one?\n\nI can’t come up with a reason to worry about backward compatibility in\nthis case.  Messages to stderr from porcelain are not guaranteed to be\nstable.\n\n> +++ b/builtin/revert.c\n> @@ -357,14 +356,9 @@ static void do_recursive_merge(struct commit *base, struct commit *next,\n>  \t\t\t\t\ti++;\n>  \t\t\t}\n>  \t\t}\n> -\t\twrite_message(msgbuf, defmsg);\n> -\t\tfprintf(stderr, \"Automatic %s failed.%s\\n\",\n> -\t\t\tme, help_msg());\n> -\t\trerere(allow_rerere_auto);\n> -\t\texit(1);\n>  \t}\n> -\twrite_message(msgbuf, defmsg);\n> -\tfprintf(stderr, \"Finished one %s.\\n\", me);\n> +\n> +\treturn !clean;\n>  }\n\nThe return value is 1 if dirty, 0 if clean.  In any other\nsituation (e.g., the index locking or the merge machinery breaks)\nit will still die, so this is not about libification.  Is it\nis for unification with the try_merge_command case?\n\n> @@ -470,30 +466,41 @@ static int do_pick_commit(void)\n[...]\n> +\tif (res) {\n> +\t\tfprintf(stderr, \"Automatic %s failed.%s\\n\",\n> +\t\t\tmebuf.buf, help_msg());\n> +\t\trerere(allow_rerere_auto);\n> +\t} else {\n> +\t\tfprintf(stderr, \"Finished one %s.\\n\", mebuf.buf);\n> +\t}\n> +\n> +\tstrbuf_release(&mebuf);\n\nYep, looks like it is.\n\n[...]\n> --- a/t/t3508-cherry-pick-many-commits.sh\n> +++ b/t/t3508-cherry-pick-many-commits.sh\n> @@ -23,12 +23,36 @@ test_expect_success setup '\n\nNew tests for the success output.\n\n>  '\n>  \n>  test_expect_success 'cherry-pick first..fourth works' '\n> +\tcat <<-EOF > expected &&\n> +\tFinished one cherry-pick.\n> +\tFinished one cherry-pick.\n> +\tFinished one cherry-pick.\n> +\tEOF\n> +\n> +\tgit checkout -f master &&\n> +\tgit reset --hard first &&\n> +\ttest_tick &&\n> +\tgit cherry-pick first..fourth 2>actual &&\n> +\tgit diff --quiet other &&\n> +\tgit diff --quiet HEAD other &&\n> +\ttest_cmp expected actual &&\n\nThis breaks test runs with sh -x or GIT_TRACE=1 (I am not sure\nwhether we support them, but I think it is worth avoiding\nproblems where possible).  Maybe:\n\n\t...\n\tgrep \"Finished one cherry-pick\\.\" actual >filtered &&\n\tn=$(wc -l <filtered) &&\n\t... &&\n\ttest $n -eq 3\n\nSummary: I was misled by the commit message.  Maybe something\nlike\n\n\tSubject: cherry-pick --strategy: report success\n\n\t\"git cherry-pick foo\" has always reported success\n\twith \"Finished one cherry-pick\" and cherry-pick\n\t--strategy is starting to feel left out.  Move the\n\tcode to write that message from do_recursive_merge\n\tto do_cherry_pick so other strategies can share it.\n\nwould have set me straight.\n"},{"id":"145386","messageId":"20100712182733.GD17630@debian","threadId":"24375","inReplyTo":"20100712115455.12251.53947.chriscool@tuxfamily.org","subject":"Re: [PATCH 1/2] revert: refactor code that prints success or failure message","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2010-07-12T18:27:33Z","receivedAt":"2010-07-12T18:27:33Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Christian,\n\nChristian Couder writes:\n> This patch refactors the code that prints a message like\n> \"Automatic cherry-pick failed. <help message>\". This code was\n[...]\n\nThis is good stuff. Can we expect the git-rebase--interactive.sh to be\nrefactored to use the \"cherry pick a range\" feature in future?\n\n-- Ram\n"},{"id":"145462","messageId":"201007132302.59324.chriscool@tuxfamily.org","threadId":"24375","inReplyTo":"20100712163043.GA1531@burratino","subject":"Re: [PATCH 1/2] revert: refactor code that prints success or failure message","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2010-07-13T21:02:58Z","receivedAt":"2010-07-13T21:02:58Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Monday 12 July 2010 18:30:43 Jonathan Nieder wrote:\n> Hi,\n> \n> Christian Couder wrote:\n> > This patch refactors the code that prints a message like\n> > \"Automatic cherry-pick failed. <help message>\".\n> \n> From the point of view of refactoring, it does not seem\n> so promising: the patch adds more code than it removes.\n> \n>  $ git diff --numstat HEAD^\n>  29\t22\tbuiltin/revert.c\n>  25\t 1\tt/t3508-cherry-pick-many-commits.sh\n> \n> So I am hoping there is some other reason.\n\nIf it makes the code easier to maintain it's still a refactoring even if it \nadds some lines, but anyway as I say below I will improve the commit message.\n \n> > To do that, now do_recursive_merge() returns an int to signal\n> > success or failure. And in case of failure we just return 1\n> > from do_pick_commit() instead of doing \"exit(1)\" from either\n> > do_recursive_merge() or do_pick_commit().\n> \n> This part sounds like libification...\n\nYes, but it's just because it's needed for the refactoring.\n\n> > In case of successful merge, do_recursive_merge() used to print\n> > something like \"Finished one cherry-pick.\" but nothing was\n> > printed in case a special strategy like \"resolve\" was used.\n> > Now in this case a message like \"Finished one cherry-pick with\n> > strategy resolve.\" is printed.\n> > \n> > So the command behavior should be more consistent wether we are\n> > using a special strategy or not.\n> \n> This part sounds like improving output in the cherry-pick --strategy\n> success case.\n\nYes.\n\n> > \tI also wonder why messages like \"Finished one cherry-pick.\"\n> > \tare printed on stderr instead of stdout.\n> \n> Progress reports tend to go to stderr, since they are not what the\n> user wanted to learn from the command but side-effects.  In other\n> words, I think the general principle is that\n> \n>  $ git <foo> 2>/dev/null\n> \n> should output whatever a traditional Unix command would (usually\n> nothing).\n\nOk.\n\n> > \tAnd while at making a backward incompatible change, maybe\n> > \twe could change the message to something like:\n> > \t\n> > \t\"Finished cherry-pick <sha1>\"\n> \n> Does <sha1> represent the old commit or the new one?\n\nThe old one.\n\n> I can’t come up with a reason to worry about backward compatibility in\n> this case.  Messages to stderr from porcelain are not guaranteed to be\n> stable.\n\nOk, so I think I will propose a patch to do that.\n\n> > +++ b/builtin/revert.c\n> > @@ -357,14 +356,9 @@ static void do_recursive_merge(struct commit *base,\n> > struct commit *next,\n> > \n> >  \t\t\t\t\ti++;\n> >  \t\t\t\n> >  \t\t\t}\n> >  \t\t\n> >  \t\t}\n> > \n> > -\t\twrite_message(msgbuf, defmsg);\n> > -\t\tfprintf(stderr, \"Automatic %s failed.%s\\n\",\n> > -\t\t\tme, help_msg());\n> > -\t\trerere(allow_rerere_auto);\n> > -\t\texit(1);\n> > \n> >  \t}\n> > \n> > -\twrite_message(msgbuf, defmsg);\n> > -\tfprintf(stderr, \"Finished one %s.\\n\", me);\n> > +\n> > +\treturn !clean;\n> > \n> >  }\n> \n> The return value is 1 if dirty, 0 if clean.  In any other\n> situation (e.g., the index locking or the merge machinery breaks)\n> it will still die, so this is not about libification.  Is it\n> is for unification with the try_merge_command case?\n\nYes.\n\n> > @@ -470,30 +466,41 @@ static int do_pick_commit(void)\n> \n> [...]\n> \n> > +\tif (res) {\n> > +\t\tfprintf(stderr, \"Automatic %s failed.%s\\n\",\n> > +\t\t\tmebuf.buf, help_msg());\n> > +\t\trerere(allow_rerere_auto);\n> > +\t} else {\n> > +\t\tfprintf(stderr, \"Finished one %s.\\n\", mebuf.buf);\n> > +\t}\n> > +\n> > +\tstrbuf_release(&mebuf);\n> \n> Yep, looks like it is.\n\nRight.\n\n> [...]\n> \n> > --- a/t/t3508-cherry-pick-many-commits.sh\n> > +++ b/t/t3508-cherry-pick-many-commits.sh\n> > @@ -23,12 +23,36 @@ test_expect_success setup '\n> \n> New tests for the success output.\n> \n> >  '\n> >  \n> >  test_expect_success 'cherry-pick first..fourth works' '\n> > \n> > +\tcat <<-EOF > expected &&\n> > +\tFinished one cherry-pick.\n> > +\tFinished one cherry-pick.\n> > +\tFinished one cherry-pick.\n> > +\tEOF\n> > +\n> > +\tgit checkout -f master &&\n> > +\tgit reset --hard first &&\n> > +\ttest_tick &&\n> > +\tgit cherry-pick first..fourth 2>actual &&\n> > +\tgit diff --quiet other &&\n> > +\tgit diff --quiet HEAD other &&\n> > +\ttest_cmp expected actual &&\n> \n> This breaks test runs with sh -x or GIT_TRACE=1 (I am not sure\n> whether we support them, but I think it is worth avoiding\n> problems where possible).  Maybe:\n\nI don't know about sh -x but there is this code in test-lib.sh to warn about \nGIT_TRACE use:\n\ncase $(echo $GIT_TRACE |tr \"[A-Z]\" \"[a-z]\") in\n\t1|2|true)\n\t\techo \"* warning: Some tests will not work if GIT_TRACE\" \\\n\t\t\t\"is set as to trace on STDERR ! *\"\n\t\techo \"* warning: Please set GIT_TRACE to something\" \\\n\t\t\t\"other than 1, 2 or true ! *\"\n\t\t;;\nesac\n\n> \t...\n> \tgrep \"Finished one cherry-pick\\.\" actual >filtered &&\n> \tn=$(wc -l <filtered) &&\n> \t... &&\n> \ttest $n -eq 3\n> \n> Summary: I was misled by the commit message.  Maybe something\n> like\n> \n> \tSubject: cherry-pick --strategy: report success\n> \n> \t\"git cherry-pick foo\" has always reported success\n> \twith \"Finished one cherry-pick\" and cherry-pick\n> \t--strategy is starting to feel left out.  Move the\n> \tcode to write that message from do_recursive_merge\n> \tto do_cherry_pick so other strategies can share it.\n> \n> would have set me straight.\n\nOk, I will improve it.\n\nThanks,\nChristian.\n"},{"id":"145464","messageId":"201007132316.48645.chriscool@tuxfamily.org","threadId":"24375","inReplyTo":"20100712182733.GD17630@debian","subject":"Re: [PATCH 1/2] revert: refactor code that prints success or failure message","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2010-07-13T21:16:48Z","receivedAt":"2010-07-13T21:16:48Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi Ram,\n\nOn Monday 12 July 2010 20:27:33 Ramkumar Ramachandra wrote:\n> Hi Christian,\n> \n> Christian Couder writes:\n> > This patch refactors the code that prints a message like\n> > \"Automatic cherry-pick failed. <help message>\". This code was\n> \n> [...]\n> \n> This is good stuff. Can we expect the git-rebase--interactive.sh to be\n> refactored to use the \"cherry pick a range\" feature in future?\n\nI am trying to improve cherry-pick step by step so that hopefully in the end \nwe have some sequencer like functionalities. So yes, I hope that it will be \npossible to refactor git-rebase--interactive.sh and perhaps other such shell \nscripts.\n\nThanks,\nChristian.\n"},{"id":"145479","messageId":"20100713221620.GA4763@burratino","threadId":"24375","inReplyTo":"201007132302.59324.chriscool@tuxfamily.org","subject":"Re: [PATCH 1/2] revert: refactor code that prints success or failure message","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-07-13T22:16:20Z","receivedAt":"2010-07-13T22:16:20Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Christian Couder wrote:\n\n> I don't know about sh -x but there is this code in test-lib.sh to warn about \n> GIT_TRACE use:\n> \n> case $(echo $GIT_TRACE |tr \"[A-Z]\" \"[a-z]\") in\n> \t1|2|true)\n> \t\techo \"* warning: Some tests will not work if GIT_TRACE\" \\\n> \t\t\t\"is set as to trace on STDERR ! *\"\n> \t\techo \"* warning: Please set GIT_TRACE to something\" \\\n> \t\t\t\"other than 1, 2 or true ! *\"\n> \t\t;;\n> esac\n\nOh!  I just remembered\n\n  http://thread.gmane.org/gmane.comp.version-control.git/146767\n\nI guess the answer is we half-support it.  Maybe a test_cmp_err()\nhelper to strip out xtrace[1] and GIT_TRACE[2] output is needed.\n\n>> Summary: I was misled by the commit message.\n[...]\n> Ok, I will improve it.\n\nThanks, and sorry for the unfocused review.\n\nJonathan\n\n[1] Sadly, it is not obvious to me that the format of output produced\nby set -x is portable.  Any sufficiently unixy system (e.g., any\nsystem supporting the Posix User Portability Utilities option) will\nuse $PS4 which defaults to ‘+ ’ at the beginning of trace lines, but\neven for Posix that is only optional.\n\n[2] All trace output starts with ‘trace: ’, with one exception.\n\ndiff --git a/exec_cmd.c b/exec_cmd.c\nindex bf22570..75d2930 100644\n--- a/exec_cmd.c\n+++ b/exec_cmd.c\n@@ -28,7 +28,7 @@ const char *system_path(const char *path)\n \t    !(prefix = strip_path_suffix(argv0_path, BINDIR)) &&\n \t    !(prefix = strip_path_suffix(argv0_path, \"git\"))) {\n \t\tprefix = PREFIX;\n-\t\ttrace_printf(\"RUNTIME_PREFIX requested, \"\n+\t\ttrace_printf(\"trace: RUNTIME_PREFIX requested, \"\n \t\t\t\t\"but prefix computation failed.  \"\n \t\t\t\t\"Using static fallback '%s'.\\n\", prefix);\n \t}\n-- \n"},{"id":"145609","messageId":"201007151126.57840.chriscool@tuxfamily.org","threadId":"24375","inReplyTo":"20100713221620.GA4763@burratino","subject":"Re: [PATCH 1/2] revert: refactor code that prints success or failure message","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2010-07-15T09:26:57Z","receivedAt":"2010-07-15T09:26:57Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi,\n\nOn Wednesday 14 July 2010 00:16:20 Jonathan Nieder wrote:\n> Christian Couder wrote:\n> > I don't know about sh -x but there is this code in test-lib.sh to warn\n> > about GIT_TRACE use:\n> > \n> > case $(echo $GIT_TRACE |tr \"[A-Z]\" \"[a-z]\") in\n> > \n> > \t1|2|true)\n> > \t\n> > \t\techo \"* warning: Some tests will not work if GIT_TRACE\" \\\n> > \t\t\n> > \t\t\t\"is set as to trace on STDERR ! *\"\n> > \t\t\n> > \t\techo \"* warning: Please set GIT_TRACE to something\" \\\n> > \t\t\n> > \t\t\t\"other than 1, 2 or true ! *\"\n> > \t\t\n> > \t\t;;\n> > \n> > esac\n> \n> Oh!  I just remembered\n> \n>   http://thread.gmane.org/gmane.comp.version-control.git/146767\n> \n> I guess the answer is we half-support it.  Maybe a test_cmp_err()\n> helper to strip out xtrace[1] and GIT_TRACE[2] output is needed.\n\nI don't understand the result of the above thread but when I try \"bash -x \nt3508-cherry-pick-many-commits.sh\" it works on my machine.\nSo I didn't change the tests.\n\n> >> Summary: I was misled by the commit message.\n> \n> [...]\n> \n> > Ok, I will improve it.\n> \n> Thanks, and sorry for the unfocused review.\n\nNo problem it's still better than no review :-)\n\nThanks,\nChristian.\n"}]}