{"thread":{"id":"31024","subject":"[PATCH] status: default in-progress color to header color","startedAt":"2012-07-14T12:28:29Z","lastAt":"2012-07-16T11:39:54Z","messageCount":5,"participants":["Jeff King","Junio C Hamano","Matthieu Moy"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"195044","messageId":"20120714122828.GA6259@sigill.intra.peff.net","threadId":"31024","inReplyTo":null,"subject":"[PATCH] status: default in-progress color to header color","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-07-14T12:28:29Z","receivedAt":"2012-07-14T12:28:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The \"status\" command recently learned to describe the\nin-progress operation in its long output format (e.g.,\nrebasing, am, etc). The color for this message defaults to\n\"normal\" (i.e., no color). However, if the user has set\ntheir default header color to something besides normal, then\nthe message sticks out. A much saner default is to pick the\nuser's header color, making this message match the rest of\nthe header.\n\nWe already do the same trick with the \"onbranch\" message.\nRather than expand the current conditional to list all types\nof messages which will default back to the header color, we\ncan just default all \"nil\" colors to do so, simplifying the\ncode. That encompasses all of the current nil color slots,\nand will likely cover any future ones that will be added.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis goes on top of lk/more-helpful-status-hints.\n\nMy intent was that this would also let \"color.status.inprogress\"\noverride it, in case a user really wanted a green message or something.\nHowever, I notice that the original series did not add such a config\noption, so this color cannot be configured at all. Furthermore, I think\nthe color formatting for this message is somewhat buggy.  Even if we set\nit to green, you would end up with (imagine our header color is blue):\n\n  <blue># On branch master<reset>\n  <green># You are in the middle of a rebase.<reset>\n\nwhen what you would probably want is:\n\n  <blue># On branch master<reset>\n  <blue># <reset><green>You are in the middle of a rebase.<reset>\n\nI.e., the \"#\" bit should always be in the header color, and only the\nmessage text should change colors. This is how the \"onbranch\" message is\nhandled.\n\nBut given that this is not even configurable in the current code, I\nreally wonder if it needs to have its own color at all. Do people really\nwant to set the color of this message separately? Maybe we should just\nuse WT_STATUS_HEADER instead.\n\n wt-status.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/wt-status.c b/wt-status.c\nindex c749267..2f724b4 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -24,7 +24,7 @@ static char default_wt_status_colors[][COLOR_MAXLEN] = {\n \tGIT_COLOR_GREEN,  /* WT_STATUS_LOCAL_BRANCH */\n \tGIT_COLOR_RED,    /* WT_STATUS_REMOTE_BRANCH */\n \tGIT_COLOR_NIL,    /* WT_STATUS_ONBRANCH */\n-\tGIT_COLOR_NORMAL, /* WT_STATUS_IN_PROGRESS */\n+\tGIT_COLOR_NIL,    /* WT_STATUS_IN_PROGRESS */\n };\n \n static const char *color(int slot, struct wt_status *s)\n@@ -32,7 +32,7 @@ static const char *color(int slot, struct wt_status *s)\n \tconst char *c = \"\";\n \tif (want_color(s->use_color))\n \t\tc = s->color_palette[slot];\n-\tif (slot == WT_STATUS_ONBRANCH && color_is_nil(c))\n+\tif (color_is_nil(c))\n \t\tc = s->color_palette[WT_STATUS_HEADER];\n \treturn c;\n }\n-- \n1.7.10.5.40.gbbc17de\n"},{"id":"195045","messageId":"20120714123638.GA10431@sigill.intra.peff.net","threadId":"31024","inReplyTo":"20120714122828.GA6259@sigill.intra.peff.net","subject":"Re: [PATCH] status: default in-progress color to header color","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-07-14T12:36:38Z","receivedAt":"2012-07-14T12:36:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jul 14, 2012 at 08:28:28AM -0400, Jeff King wrote:\n\n> My intent was that this would also let \"color.status.inprogress\"\n> override it, in case a user really wanted a green message or something.\n> However, I notice that the original series did not add such a config\n> option, so this color cannot be configured at all. Furthermore, I think\n> the color formatting for this message is somewhat buggy.  Even if we set\n> it to green, you would end up with (imagine our header color is blue):\n> \n>   <blue># On branch master<reset>\n>   <green># You are in the middle of a rebase.<reset>\n> \n> when what you would probably want is:\n> \n>   <blue># On branch master<reset>\n>   <blue># <reset><green>You are in the middle of a rebase.<reset>\n> \n> I.e., the \"#\" bit should always be in the header color, and only the\n> message text should change colors. This is how the \"onbranch\" message is\n> handled.\n> \n> But given that this is not even configurable in the current code, I\n> really wonder if it needs to have its own color at all. Do people really\n> want to set the color of this message separately? Maybe we should just\n> use WT_STATUS_HEADER instead.\n\nAnd that patch would look like this (directly on top of\nlk/more-helpful-status-hints):\n\n-- >8 --\nSubject: [PATCH] status: color in-progress message like other header messages\n\nThe \"status\" command recently learned to describe the\nin-progress operation in its long output format (e.g.,\nrebasing, am, etc). This message gets its own slot in the\ncolor table, even though it is not configurable. As a\nresult, if the user has set color.status.header to a\nnon-default value, this message will not match (and cannot\nbe made to match, as there is no config option).\n\nIt is probably more sane to just color it like the rest of\nthe text (i.e., just use color.status.header). This would\nnot allow users to customize the color of this message\nindependently, but it is not likely that anyone will want to\n(and they cannot with the current code, anyway).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n wt-status.c | 3 +--\n wt-status.h | 1 -\n 2 files changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/wt-status.c b/wt-status.c\nindex c749267..c110cbc 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -24,7 +24,6 @@ static char default_wt_status_colors[][COLOR_MAXLEN] = {\n \tGIT_COLOR_GREEN,  /* WT_STATUS_LOCAL_BRANCH */\n \tGIT_COLOR_RED,    /* WT_STATUS_REMOTE_BRANCH */\n \tGIT_COLOR_NIL,    /* WT_STATUS_ONBRANCH */\n-\tGIT_COLOR_NORMAL, /* WT_STATUS_IN_PROGRESS */\n };\n \n static const char *color(int slot, struct wt_status *s)\n@@ -931,7 +930,7 @@ static void show_bisect_in_progress(struct wt_status *s,\n \n static void wt_status_print_state(struct wt_status *s)\n {\n-\tconst char *state_color = color(WT_STATUS_IN_PROGRESS, s);\n+\tconst char *state_color = color(WT_STATUS_HEADER, s);\n \tstruct wt_status_state state;\n \tstruct stat st;\n \ndiff --git a/wt-status.h b/wt-status.h\nindex c1066a0..f8fc58c 100644\n--- a/wt-status.h\n+++ b/wt-status.h\n@@ -15,7 +15,6 @@ enum color_wt_status {\n \tWT_STATUS_LOCAL_BRANCH,\n \tWT_STATUS_REMOTE_BRANCH,\n \tWT_STATUS_ONBRANCH,\n-\tWT_STATUS_IN_PROGRESS,\n \tWT_STATUS_MAXSLOT\n };\n \n-- \n1.7.10.5.40.gbbc17de\n"},{"id":"195057","messageId":"7vvchq5c3c.fsf@alter.siamese.dyndns.org","threadId":"31024","inReplyTo":"20120714122828.GA6259@sigill.intra.peff.net","subject":"Re: [PATCH] status: default in-progress color to header color","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-14T20:20:23Z","receivedAt":"2012-07-14T20:20:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> But given that this is not even configurable in the current code, I\n> really wonder if it needs to have its own color at all. Do people really\n> want to set the color of this message separately? Maybe we should just\n> use WT_STATUS_HEADER instead.\n\nI would prefer that myself, but let's see if Matthieu's folks have\nopinions on this or something you and I overlooked.\n\nThanks.\n"},{"id":"195101","messageId":"vpq8vek10xa.fsf@bauges.imag.fr","threadId":"31024","inReplyTo":"20120714123638.GA10431@sigill.intra.peff.net","subject":"Re: [PATCH] status: default in-progress color to header color","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2012-07-16T09:59:45Z","receivedAt":"2012-07-16T09:59:45Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Jeff King <peff@peff.net> writes:\n\n> Subject: [PATCH] status: color in-progress message like other header messages\n\nMy feeling is that these \"in progress\" messages would deserve to be more\nvisible than the usual headers (like \"Not currently on any branch.\",\nwhich is both legit and likely to be a user-error). For example, the\nother day, the patch told me I was bisecting. I thought it was a bug in\nthe patch, but it was indeed a user-error of mine, I did not end this\nbisect session properly around a month ago ;-).\n\nThis would argue in favor of having a different, configurable color.\n\nBut as the code currently does not allow user-configuration anyway, it's\nprobably best to make the code clean and uniform. If someone wants to\nadd customizability afterwards, it won't be that hard (it's probably a\ngood idea to have people leave with these messages for a while before\ndeciding what color it should be).\n\nThis second patch looks better to me, but no strong opinion here.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"195103","messageId":"20120716113954.GB4962@sigill.intra.peff.net","threadId":"31024","inReplyTo":"vpq8vek10xa.fsf@bauges.imag.fr","subject":"Re: [PATCH] status: default in-progress color to header color","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-07-16T11:39:54Z","receivedAt":"2012-07-16T11:39:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 16, 2012 at 11:59:45AM +0200, Matthieu Moy wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > Subject: [PATCH] status: color in-progress message like other header messages\n> \n> My feeling is that these \"in progress\" messages would deserve to be more\n> visible than the usual headers (like \"Not currently on any branch.\",\n> which is both legit and likely to be a user-error). For example, the\n> other day, the patch told me I was bisecting. I thought it was a bug in\n> the patch, but it was indeed a user-error of mine, I did not end this\n> bisect session properly around a month ago ;-).\n> \n> This would argue in favor of having a different, configurable color.\n\nSure, I have no problem with that reasoning. Only the half-done\nimplementation. :)\n\n> But as the code currently does not allow user-configuration anyway, it's\n> probably best to make the code clean and uniform. If someone wants to\n> add customizability afterwards, it won't be that hard (it's probably a\n> good idea to have people leave with these messages for a while before\n> deciding what color it should be).\n> \n> This second patch looks better to me, but no strong opinion here.\n\nLet's go with the second patch for now, then. It fixes the immediate\nproblem, and it does not make it much harder for somebody to do a\nseparate configurable color on top if they want to do so (now or later).\nThey would have to re-add the slot, but that is the least of the effort\ninvolved; handling the partial-line coloring and the default-to-header\nas ONBRANCH does is the more interesting bit, and the current code does\nnot do that at all.\n\nHere's a repost with a slightly modified commit message (on top of\nlk/more-helpful-status-hints, as before).\n\n-- >8 --\nSubject: status: color in-progress message like other header messages\n\nThe \"status\" command recently learned to describe the\nin-progress operation in its long output format (e.g.,\nrebasing, am, etc). This message gets its own slot in the\ncolor table, even though it is not configurable. As a\nresult, if the user has set color.status.header to a\nnon-default value, this message will not match (and cannot\nbe made to match, as there is no config option).\n\nIt is probably more sane to just color it like the rest of\nthe text (i.e., just use color.status.header). This would\nnot allow users to customize the color of this message\nindependently, but they cannot do that with the current code\nanyway, and if somebody wants to build customizable\ncolorization later, this patch does not make it much harder\nto do so.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n wt-status.c | 3 +--\n wt-status.h | 1 -\n 2 files changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/wt-status.c b/wt-status.c\nindex c749267..c110cbc 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -24,7 +24,6 @@ static char default_wt_status_colors[][COLOR_MAXLEN] = {\n \tGIT_COLOR_GREEN,  /* WT_STATUS_LOCAL_BRANCH */\n \tGIT_COLOR_RED,    /* WT_STATUS_REMOTE_BRANCH */\n \tGIT_COLOR_NIL,    /* WT_STATUS_ONBRANCH */\n-\tGIT_COLOR_NORMAL, /* WT_STATUS_IN_PROGRESS */\n };\n \n static const char *color(int slot, struct wt_status *s)\n@@ -931,7 +930,7 @@ static void show_bisect_in_progress(struct wt_status *s,\n \n static void wt_status_print_state(struct wt_status *s)\n {\n-\tconst char *state_color = color(WT_STATUS_IN_PROGRESS, s);\n+\tconst char *state_color = color(WT_STATUS_HEADER, s);\n \tstruct wt_status_state state;\n \tstruct stat st;\n \ndiff --git a/wt-status.h b/wt-status.h\nindex c1066a0..f8fc58c 100644\n--- a/wt-status.h\n+++ b/wt-status.h\n@@ -15,7 +15,6 @@ enum color_wt_status {\n \tWT_STATUS_LOCAL_BRANCH,\n \tWT_STATUS_REMOTE_BRANCH,\n \tWT_STATUS_ONBRANCH,\n-\tWT_STATUS_IN_PROGRESS,\n \tWT_STATUS_MAXSLOT\n };\n \n-- \n1.7.10.5.40.gbbc17de\n"}]}