{"thread":{"id":"48982","subject":"[PATCH/RFC] Color merge conflicts","startedAt":"2018-07-30T16:00:02Z","lastAt":"2018-07-31T16:09:28Z","messageCount":3,"participants":["Nguyễn Thái Ngọc Duy","Stefan Beller","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"353905","messageId":"20180730155950.17329-1-pclouds@gmail.com","threadId":"48982","inReplyTo":null,"subject":"[PATCH/RFC] Color merge conflicts","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-07-30T15:59:50Z","receivedAt":"2018-07-30T16:00:02Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"One of the things I notice when watching a normal git user face a\nmerge conflicts is the output is very verbose (especially when there\nare multiple conflicts) and it's hard to spot the important parts to\nstart resolving conflicts unless you know what to look for.\n\nThis is the start to hopefully help that a bit. One thing I'm not sure\nis what to color because that affects the config keys we use for\nthis. If we have to color different things, it's best to go\n\"color.merge.<slot>\" but if this is the only place to color, that slot\nthing looks over-engineered.\n\nPerhaps another piece to color is the conflicted path? Maybe. But on\nthe other hand, I don't think we want a chameleon output, just enough\nvisual cues to go from one conflict to the next...\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n merge-recursive.c | 133 +++++++++++++++++++++++++++-------------------\n 2 files changed, 79 insertions(+), 55 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 113c1d6962..b800101538 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -26,6 +26,7 @@\n #include \"dir.h\"\n #include \"submodule.h\"\n #include \"revision.h\"\n+#include \"color.h\"\n \n struct path_hashmap_entry {\n \tstruct hashmap_entry e;\n@@ -286,6 +287,28 @@ static void output(struct merge_options *o, int v, const char *fmt, ...)\n \t\tflush_output(o);\n }\n \n+__attribute__((format (printf, 3, 4)))\n+static void conflict_output(struct merge_options *o, int v, const char *fmt, ...)\n+{\n+\tva_list ap;\n+\n+\tif (!show(o, v))\n+\t\treturn;\n+\n+\tstrbuf_addchars(&o->obuf, ' ', o->call_depth * 2);\n+\n+\tstrbuf_addf(&o->obuf, \"%s%s%s \",\n+\t\t    GIT_COLOR_RED, _(\"CONFLICT\"), GIT_COLOR_RESET);\n+\n+\tva_start(ap, fmt);\n+\tstrbuf_vaddf(&o->obuf, fmt, ap);\n+\tva_end(ap);\n+\n+\tstrbuf_addch(&o->obuf, '\\n');\n+\tif (!o->buffer_output)\n+\t\tflush_output(o);\n+}\n+\n static void output_commit_title(struct merge_options *o, struct commit *commit)\n {\n \tstruct merge_remote_desc *desc;\n@@ -1497,27 +1520,27 @@ static int handle_change_delete(struct merge_options *o,\n \t\t */\n \t\tif (!alt_path) {\n \t\t\tif (!old_path) {\n-\t\t\t\toutput(o, 1, _(\"CONFLICT (%s/delete): %s deleted in %s \"\n-\t\t\t\t       \"and %s in %s. Version %s of %s left in tree.\"),\n-\t\t\t\t       change, path, delete_branch, change_past,\n-\t\t\t\t       change_branch, change_branch, path);\n+\t\t\t\tconflict_output(o, 1, _(\"(%s/delete): %s deleted in %s \"\n+\t\t\t\t\t\t\t\"and %s in %s. Version %s of %s left in tree.\"),\n+\t\t\t\t\t\tchange, path, delete_branch, change_past,\n+\t\t\t\t\t\tchange_branch, change_branch, path);\n \t\t\t} else {\n-\t\t\t\toutput(o, 1, _(\"CONFLICT (%s/delete): %s deleted in %s \"\n-\t\t\t\t       \"and %s to %s in %s. Version %s of %s left in tree.\"),\n-\t\t\t\t       change, old_path, delete_branch, change_past, path,\n-\t\t\t\t       change_branch, change_branch, path);\n+\t\t\t\tconflict_output(o, 1, _(\"(%s/delete): %s deleted in %s \"\n+\t\t\t\t\t\t\t\"and %s to %s in %s. Version %s of %s left in tree.\"),\n+\t\t\t\t\t\tchange, old_path, delete_branch, change_past, path,\n+\t\t\t\t\t\tchange_branch, change_branch, path);\n \t\t\t}\n \t\t} else {\n \t\t\tif (!old_path) {\n-\t\t\t\toutput(o, 1, _(\"CONFLICT (%s/delete): %s deleted in %s \"\n-\t\t\t\t       \"and %s in %s. Version %s of %s left in tree at %s.\"),\n-\t\t\t\t       change, path, delete_branch, change_past,\n-\t\t\t\t       change_branch, change_branch, path, alt_path);\n+\t\t\t\tconflict_output(o, 1, _(\"(%s/delete): %s deleted in %s \"\n+\t\t\t\t\t\t\t\"and %s in %s. Version %s of %s left in tree at %s.\"),\n+\t\t\t\t\t\tchange, path, delete_branch, change_past,\n+\t\t\t\t\t\tchange_branch, change_branch, path, alt_path);\n \t\t\t} else {\n-\t\t\t\toutput(o, 1, _(\"CONFLICT (%s/delete): %s deleted in %s \"\n-\t\t\t\t       \"and %s to %s in %s. Version %s of %s left in tree at %s.\"),\n-\t\t\t\t       change, old_path, delete_branch, change_past, path,\n-\t\t\t\t       change_branch, change_branch, path, alt_path);\n+\t\t\t\tconflict_output(o, 1, _(\"(%s/delete): %s deleted in %s \"\n+\t\t\t\t\t\t\t\"and %s to %s in %s. Version %s of %s left in tree at %s.\"),\n+\t\t\t\t\t\tchange, old_path, delete_branch, change_past, path,\n+\t\t\t\t\t\tchange_branch, change_branch, path, alt_path);\n \t\t\t}\n \t\t}\n \t\t/*\n@@ -1647,12 +1670,12 @@ static int handle_rename_rename_1to2(struct merge_options *o,\n \tstruct diff_filespec *a = ci->pair1->two;\n \tstruct diff_filespec *b = ci->pair2->two;\n \n-\toutput(o, 1, _(\"CONFLICT (rename/rename): \"\n-\t       \"Rename \\\"%s\\\"->\\\"%s\\\" in branch \\\"%s\\\" \"\n-\t       \"rename \\\"%s\\\"->\\\"%s\\\" in \\\"%s\\\"%s\"),\n-\t       one->path, a->path, ci->branch1,\n-\t       one->path, b->path, ci->branch2,\n-\t       o->call_depth ? _(\" (left unresolved)\") : \"\");\n+\tconflict_output(o, 1, _(\"(rename/rename): \"\n+\t\t\t\t\"Rename \\\"%s\\\"->\\\"%s\\\" in branch \\\"%s\\\" \"\n+\t\t\t\t\"rename \\\"%s\\\"->\\\"%s\\\" in \\\"%s\\\"%s\"),\n+\t\t\tone->path, a->path, ci->branch1,\n+\t\t\tone->path, b->path, ci->branch2,\n+\t\t\to->call_depth ? _(\" (left unresolved)\") : \"\");\n \tif (o->call_depth) {\n \t\tstruct merge_file_info mfi;\n \t\tstruct diff_filespec other;\n@@ -1716,11 +1739,11 @@ static int handle_rename_rename_2to1(struct merge_options *o,\n \tstruct merge_file_info mfi_c2;\n \tint ret;\n \n-\toutput(o, 1, _(\"CONFLICT (rename/rename): \"\n-\t       \"Rename %s->%s in %s. \"\n-\t       \"Rename %s->%s in %s\"),\n-\t       a->path, c1->path, ci->branch1,\n-\t       b->path, c2->path, ci->branch2);\n+\tconflict_output(o, 1, _(\"(rename/rename): \"\n+\t\t\t\t\"Rename %s->%s in %s. \"\n+\t\t\t\t\"Rename %s->%s in %s\"),\n+\t\t\ta->path, c1->path, ci->branch1,\n+\t\t\tb->path, c2->path, ci->branch2);\n \n \tremove_file(o, 1, a->path, o->call_depth || would_lose_untracked(a->path));\n \tremove_file(o, 1, b->path, o->call_depth || would_lose_untracked(b->path));\n@@ -1973,12 +1996,12 @@ static char *handle_path_level_conflicts(struct merge_options *o,\n \t\t/* This should only happen when entry->non_unique_new_dir set */\n \t\tif (!entry->non_unique_new_dir)\n \t\t\tBUG(\"entry->non_unqiue_dir not set and !new_path\");\n-\t\toutput(o, 1, _(\"CONFLICT (directory rename split): \"\n-\t\t\t       \"Unclear where to place %s because directory \"\n-\t\t\t       \"%s was renamed to multiple other directories, \"\n-\t\t\t       \"with no destination getting a majority of the \"\n-\t\t\t       \"files.\"),\n-\t\t       path, entry->dir);\n+\t\tconflict_output(o, 1, _(\"(directory rename split): \"\n+\t\t\t\t\t\"Unclear where to place %s because directory \"\n+\t\t\t\t\t\"%s was renamed to multiple other directories, \"\n+\t\t\t\t\t\"with no destination getting a majority of the \"\n+\t\t\t\t\t\"files.\"),\n+\t\t\t\tpath, entry->dir);\n \t\tclean = 0;\n \t\treturn NULL;\n \t}\n@@ -2005,20 +2028,20 @@ static char *handle_path_level_conflicts(struct merge_options *o,\n \t\tcollision_ent->reported_already = 1;\n \t\tstrbuf_add_separated_string_list(&collision_paths, \", \",\n \t\t\t\t\t\t &collision_ent->source_files);\n-\t\toutput(o, 1, _(\"CONFLICT (implicit dir rename): Existing \"\n-\t\t\t       \"file/dir at %s in the way of implicit \"\n-\t\t\t       \"directory rename(s) putting the following \"\n-\t\t\t       \"path(s) there: %s.\"),\n-\t\t       new_path, collision_paths.buf);\n+\t\tconflict_output(o, 1, _(\"(implicit dir rename): Existing \"\n+\t\t\t\t\t\"file/dir at %s in the way of implicit \"\n+\t\t\t\t\t\"directory rename(s) putting the following \"\n+\t\t\t\t\t\"path(s) there: %s.\"),\n+\t\t\t\tnew_path, collision_paths.buf);\n \t\tclean = 0;\n \t} else if (collision_ent->source_files.nr > 1) {\n \t\tcollision_ent->reported_already = 1;\n \t\tstrbuf_add_separated_string_list(&collision_paths, \", \",\n \t\t\t\t\t\t &collision_ent->source_files);\n-\t\toutput(o, 1, _(\"CONFLICT (implicit dir rename): Cannot map \"\n-\t\t\t       \"more than one path to %s; implicit directory \"\n-\t\t\t       \"renames tried to put these paths there: %s\"),\n-\t\t       new_path, collision_paths.buf);\n+\t\tconflict_output(o, 1, _(\"(implicit dir rename): Cannot map \"\n+\t\t\t\t\t\"more than one path to %s; implicit directory \"\n+\t\t\t\t\t\"renames tried to put these paths there: %s\"),\n+\t\t\t\tnew_path, collision_paths.buf);\n \t\tclean = 0;\n \t}\n \n@@ -2107,11 +2130,11 @@ static void handle_directory_level_conflicts(struct merge_options *o,\n \t\t\t * know that head_ent->new_dir and merge_ent->new_dir\n \t\t\t * are different strings.\n \t\t\t */\n-\t\t\toutput(o, 1, _(\"CONFLICT (rename/rename): \"\n-\t\t\t\t       \"Rename directory %s->%s in %s. \"\n-\t\t\t\t       \"Rename directory %s->%s in %s\"),\n-\t\t\t       head_ent->dir, head_ent->new_dir.buf, o->branch1,\n-\t\t\t       head_ent->dir, merge_ent->new_dir.buf, o->branch2);\n+\t\t\tconflict_output(o, 1, _(\"(rename/rename): \"\n+\t\t\t\t\t\t\"Rename directory %s->%s in %s. \"\n+\t\t\t\t\t\t\"Rename directory %s->%s in %s\"),\n+\t\t\t\t\thead_ent->dir, head_ent->new_dir.buf, o->branch1,\n+\t\t\t\t\thead_ent->dir, merge_ent->new_dir.buf, o->branch2);\n \t\t\tstring_list_append(&remove_from_head,\n \t\t\t\t\t   head_ent->dir)->util = head_ent;\n \t\t\tstrbuf_release(&head_ent->new_dir);\n@@ -2758,10 +2781,10 @@ static int process_renames(struct merge_options *o,\n \t\t\t} else if (!oid_eq(&dst_other.oid, &null_oid)) {\n \t\t\t\tclean_merge = 0;\n \t\t\t\ttry_merge = 1;\n-\t\t\t\toutput(o, 1, _(\"CONFLICT (rename/add): Rename %s->%s in %s. \"\n-\t\t\t\t       \"%s added in %s\"),\n-\t\t\t\t       ren1_src, ren1_dst, branch1,\n-\t\t\t\t       ren1_dst, branch2);\n+\t\t\t\tconflict_output(o, 1, _(\"(rename/add): Rename %s->%s in %s. \"\n+\t\t\t\t\t\t\t\"%s added in %s\"),\n+\t\t\t\t\t\tren1_src, ren1_dst, branch1,\n+\t\t\t\t\t\tren1_dst, branch2);\n \t\t\t\tif (o->call_depth) {\n \t\t\t\t\tstruct merge_file_info mfi;\n \t\t\t\t\tif (merge_file_one(o, ren1_dst, &null_oid, 0,\n@@ -3079,7 +3102,7 @@ static int merge_content(struct merge_options *o,\n \tif (!mfi.clean) {\n \t\tif (S_ISGITLINK(mfi.mode))\n \t\t\treason = _(\"submodule\");\n-\t\toutput(o, 1, _(\"CONFLICT (%s): Merge conflict in %s\"),\n+\t\tconflict_output(o, 1, _(\"(%s): Merge conflict in %s\"),\n \t\t\t\treason, path);\n \t\tif (rename_conflict_info && !df_conflict_remains)\n \t\t\tif (update_stages(o, path, &one, &a, &b))\n@@ -3240,9 +3263,9 @@ static int process_entry(struct merge_options *o,\n \t\t\t       0)) {\n \t\t\tchar *new_path = unique_path(o, path, add_branch);\n \t\t\tclean_merge = 0;\n-\t\t\toutput(o, 1, _(\"CONFLICT (%s): There is a directory with name %s in %s. \"\n-\t\t\t       \"Adding %s as %s\"),\n-\t\t\t       conf, path, other_branch, path, new_path);\n+\t\t\tconflict_output(o, 1, _(\"(%s): There is a directory with name %s in %s. \"\n+\t\t\t\t\t\t\"Adding %s as %s\"),\n+\t\t\t\t\tconf, path, other_branch, path, new_path);\n \t\t\tif (update_file(o, 0, oid, mode, new_path))\n \t\t\t\tclean_merge = -1;\n \t\t\telse if (o->call_depth)\n-- \n2.18.0.656.gda699b98b3\n\n"},{"id":"353914","messageId":"CAGZ79kYnfJXRiLnx8oq9mZYrzfBa-79URn+j4QtsFu3d07Jyog@mail.gmail.com","threadId":"48982","inReplyTo":"20180730155950.17329-1-pclouds@gmail.com","subject":"Re: [PATCH/RFC] Color merge conflicts","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-07-30T17:40:20Z","receivedAt":"2018-07-30T17:40:35Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Jul 30, 2018 at 9:00 AM Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:\n>\n> One of the things I notice when watching a normal git user face a\n> merge conflicts is the output is very verbose (especially when there\n> are multiple conflicts) and it's hard to spot the important parts to\n> start resolving conflicts unless you know what to look for.\n\nI usually go by git-status, but I am not the watched normal user,\nhopefully. Maybe we want to run git-status after a failed merge\nfor the user, too, though?\n\n> This is the start to hopefully help that a bit. One thing I'm not sure\n> is what to color because that affects the config keys we use for\n> this. If we have to color different things, it's best to go\n> \"color.merge.<slot>\" but if this is the only place to color, that slot\n> thing looks over-engineered.\n>\n> Perhaps another piece to color is the conflicted path? Maybe. But on\n> the other hand, I don't think we want a chameleon output, just enough\n> visual cues to go from one conflict to the next...\n\nI would think we would want to highlight the type of conflict as well,\ne.g.\n\n  <RED> CONFLICT> <RESET> \\\n  <BOLD;RED> (rename/rename): <RESET> \\\n  Rename a -> b in branch foo \\\n  rename a ->c in bar\n\nbut then again, this patch is better than not doing any colors.\n\nI wonder if we want to have certain colors associated with certain\ntypes of merge conflicts, e.g. anything involving a rename would\nbe yellow, any conflict with deletion to be dark red, regular merge\nconflicts blue (and at the same time we could introduce coloring\nof conflict markers to also be blue)\n(I am just making up colors, not seriously suggesting them)\n\nI like the idea!\n\nThanks,\nStefan\n"},{"id":"354053","messageId":"CABPp-BEAmjx0e8AFHdPChR6LcUAyTVGHY7hAMHUiykX90kvnHg@mail.gmail.com","threadId":"48982","inReplyTo":"CAGZ79kYnfJXRiLnx8oq9mZYrzfBa-79URn+j4QtsFu3d07Jyog@mail.gmail.com","subject":"Re: [PATCH/RFC] Color merge conflicts","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-07-31T16:09:24Z","receivedAt":"2018-07-31T16:09:28Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Jul 30, 2018 at 10:40 AM, Stefan Beller <sbeller@google.com> wrote:\n> On Mon, Jul 30, 2018 at 9:00 AM Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:\n>>\n>> One of the things I notice when watching a normal git user face a\n>> merge conflicts is the output is very verbose (especially when there\n>> are multiple conflicts) and it's hard to spot the important parts to\n>> start resolving conflicts unless you know what to look for.\n>\n> I usually go by git-status, but I am not the watched normal user,\n> hopefully. Maybe we want to run git-status after a failed merge\n> for the user, too, though?\n\nI'm a little worried the git status output may be long enough that\nthey miss all the conflict messages and don't even think to scroll\nback and look at them.  Since not all conflict types are nicely\nrepresentable in git status output, that could be problematic.  (e.g.\nrenames aren't recorded in the index in any fashion, so git status\ncan't tell you a conflict was from a rename/delete or\nrename/rename(1to2), for example; it's possible that information may\nbe important in helping the user track down why the working directory\nended up the way it did to help them resolve the conflict.)\n\nIf that extra information that is currently only reported in these\nconflict messages were recorded somewhere -- either the index or the\nworking tree -- then it wouldn't be as much of a risk to hide it\nbehind git status.  In fact, it would seem safer and nicer in general\nbecause users already run the risk of losing those conflict messages.\n\n>> This is the start to hopefully help that a bit. One thing I'm not sure\n>> is what to color because that affects the config keys we use for\n>> this. If we have to color different things, it's best to go\n>> \"color.merge.<slot>\" but if this is the only place to color, that slot\n>> thing looks over-engineered.\n>>\n>> Perhaps another piece to color is the conflicted path? Maybe. But on\n>> the other hand, I don't think we want a chameleon output, just enough\n>> visual cues to go from one conflict to the next...\n>\n> I would think we would want to highlight the type of conflict as well,\n> e.g.\n>\n>   <RED> CONFLICT> <RESET> \\\n>   <BOLD;RED> (rename/rename): <RESET> \\\n>   Rename a -> b in branch foo \\\n>   rename a ->c in bar\n>\n> but then again, this patch is better than not doing any colors.\n>\n> I wonder if we want to have certain colors associated with certain\n> types of merge conflicts, e.g. anything involving a rename would\n> be yellow, any conflict with deletion to be dark red, regular merge\n> conflicts blue (and at the same time we could introduce coloring\n> of conflict markers to also be blue)\n\nProviding extra hints might be good, but we'd need to flesh out this\nidea to avoid edge and corner cases...\n\nWhat do you do with mixtures, e.g. rename/delete conflicts?  Average\nthe colors?  Colorize the \"rename\" differently than the \"delete\"?\n\nAlso, by \"regular conflicts\" I think you mean the normal content-based\nconflict markers we stick in files, rather than path-based conflicts\n(like modify/delete or rename/add).  However, something could be both\n-- e.g. for an add/add conflict we two-way merge the files so we have\nboth a path conflict (both sides added a file with that name) and a\ncontent or \"regular\" conflict (most the files were the same but they\ndiffer on these lines...).  Things could get even crazier with e.g.\nrename/add/delete or rename/rename(2to1)/delete.  So, if you want to\ncolorize regular (or content) conflicts differently than path-based\nones, what do you do when you have both types present?  Does one win?\nDo we print multiple conflict messages?  Something else?\n\n> (I am just making up colors, not seriously suggesting them)\n>\n> I like the idea!\n>\n> Thanks,\n> Stefan\n"}]}