On Mon, Feb 21 2022, Johannes Schindelin wrote: [I sent out an empty reply to this earlier by mistake, sorry about that] > [...] > Which brings me to the next concern: since `merge-tree` is a low-level > tool meant to be called by programs rather than humans, we need to make > sure that those messages remain machine-parseable, even if they contain > file names. > > Concretely: while I am not currently aware of any web UI that allows to > resolve simple rename/rename conflicts, it is easily conceivable how to > implement such a thing. When that happens, we will need to be able to > teach the server-side code to discern between the cases that can be > handled in the web UI (trivial merge conflicts, trivial rename/rename > conflicts) as compared to scenarios where the conflicts are just too > complex. > > Here's an excerpt from t4301: > > -- snip -- > Auto-merging greeting > CONFLICT (content): Merge conflict in greeting > Auto-merging numbers > CONFLICT (file/directory): directory in the way of whatever from side1; moving it to whatever~side1 instead. > CONFLICT (modify/delete): whatever~side1 deleted in side2 and modified in side1. Version side1 of whatever~side1 left in tree. > -- snap -- > > This is the complete set of messages provided in the `test conflict > notices and such` test case. > > I immediately notice that every line contains at least one file name. > Looking at https://github.com/git/git/blob/v2.35.1/merge-ort.c#L1899, it > does not seem as if the file names are quoted: > > path_msg(opt, path, 1, _("Auto-merging %s"), path); > > (where `path` is used verbatim in a call to `merge_3way()` before that, > i.e. it must not have been quoted) > > I would like to register a wish to ensure that file names with special > characters (such as most notably line-feed characters) are quoted in these > messages, so that a simple server-side parser can handle messages starting > with `Auto-merging` and with `CONFLICT (content): Merge conflict in `, and > "throw the hands up in the air" if any other message prefix is seen. > > Do you think we can switch to `sq_quote_buf_pretty()` for these messages? > For the `Auto-merging` one, it would be trivial, but I fear that we will > have to work a bit on the `path_msg()` function > (https://github.com/git/git/blob/v2.35.1/merge-ort.c#L630-L649) because it > accepts a variable list of arguments without any clue whether the > arguments refer to paths or not. (And I would be loathe to switch _all_ > callers to do the quoting themselves.) > > I see 28 calls to that function, and at least a couple that pass not only > a path but also an OID (e.g. > https://github.com/git/git/blob/v2.35.1/merge-ort.c#L1611-L1613). > > We could of course be sloppy and pass even OIDs through > `sq_quote_buf_pretty()` in `path_msg()`, knowing that there won't be any > special characters in them, but it gets more complicated e.g. in > https://github.com/git/git/blob/v2.35.1/merge-ort.c#L1648-L1651, where we > pass an `strbuf` that contains a somewhat free-form commit message. > > I guess we could still pass those through `sq_quote_buf_pretty()`, even if > they are not paths, to ensure that there are no special characters in the > machine-parseable lines. > > What do you think? That sounds like a rather nasty hack, this is too, but demonstrates that we can pretty easily extract this in a machine-readable format with just a few lines now): diff --git a/merge-ort.c b/merge-ort.c index 8a5f201d190..a906881f9b3 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -633,7 +633,7 @@ static void path_msg(struct merge_options *opt, int omittable_hint, /* skippable under --remerge-diff */ const char *fmt, ...) { - va_list ap; + va_list ap, cp; struct strbuf *sb, *dest; struct strbuf tmp = STRBUF_INIT; @@ -650,7 +650,9 @@ static void path_msg(struct merge_options *opt, dest = (opt->record_conflict_msgs_as_headers ? &tmp : sb); + va_copy(cp, ap); va_start(ap, fmt); + if (opt->priv->call_depth) { strbuf_addchars(dest, ' ', 2); strbuf_addstr(dest, "From inner merge:"); @@ -659,6 +661,15 @@ static void path_msg(struct merge_options *opt, strbuf_vaddf(dest, fmt, ap); va_end(ap); + va_start(cp, fmt); + trace2_region_enter_printf("merge", "conflict/path", opt->repo, "%s", path); + trace2_region_leave("merge", "conflict/path", opt->repo); + trace2_region_enter_printf("merge", "conflict/fmt", opt->repo, "%s", fmt); + trace2_region_leave("merge", "conflict/fmt", opt->repo); + trace2_region_enter_printf_va("merge", "conflict/msg", opt->repo, fmt, cp); + trace2_region_leave("merge", "conflict/msg", opt->repo); + va_end(cp); + if (opt->record_conflict_msgs_as_headers) { int i_sb = 0, i_tmp = 0; You can run that with one of the tests added in this series to get the output as JSON, e.g.: GIT_TRACE2_EVENT=/dev/stderr GIT_TRACE2_EVENT_NESTING=10 ~/g/git/git merge-tree --write-tree --no-messages --name-only --messages side1 side2 2>&1|jq -r .| grep '"msg"' "msg": "whatever~side1" "msg": "CONFLICT (file/directory): directory in the way of %s from %s; moving it to %s instead." "msg": "CONFLICT (file/directory): directory in the way of whatever from side1; moving it to whatever~side1 instead." "msg": "whatever~side1" "msg": "CONFLICT (modify/delete): %s deleted in %s and modified in %s. Version %s of %s left in tree." "msg": "CONFLICT (modify/delete): whatever~side1 deleted in side2 and modified in side1. Version side1 of whatever~side1 left in tree." "msg": "numbers" "msg": "Auto-merging %s" "msg": "Auto-merging numbers" "msg": "greeting" "msg": "Auto-merging %s" "msg": "Auto-merging greeting" "msg": "greeting" "msg": "CONFLICT (%s): Merge conflict in %s" "msg": "CONFLICT (content): Merge conflict in greeting" A "proper" fix for this doesn't sound too hard, we'd just instrument the path_msg() function to pass along some "message category", see e.g. unpack_plumbing_errors in unpack-trees.c for one example of such a thing, or the "enum fsck_msg_id". Then we'd just allow you to emit any of the sprintf() format itself, or the expanded version, the path, or an id like "CONFLICT:file/directory" or "auto-merging" etc. I think that would be particularly useful in conjuction with the --format changes I was proposing for this (and hacked up a WIP patch for). You could just have a similar --format-messages or whatever. Then you could pick \0\0 as a delimiter for your "main" --format, and "\0" as the delimiter for your --format-messages, and thus be able to parse N-nested \0-delimited content.
